[chat] Fix global preload error from the remend import - #23263
Conversation
`remend` was imported through a variable specifier plus `@vite-ignore` /
`webpackIgnore` hints, which no bundler can statically analyze. Browser
bundles were left with a bare `import('remend')` that can never resolve
without an import map, so the richer repair was dead code in every bundled
app, and under Vite 8 (rolldown) the unresolvable import is wrapped in
`__vitePreload` — dispatching a global `vite:preloadError` on every
assistant reply.
Inlining the specifier alone is not enough: `remend` is ESM-only, so the
CJS build downlevels to `require('remend')` and webpack hard-fails with
`"." is not exported under the conditions [...require...]`. The specifier
has to differ per output format.
Use a `#remend` subpath import that resolves to the real package under the
`import` condition and to a local stub under `require`. The loader becomes
a plain, statically analyzable `import('#remend')`, so bundlers resolve and
code-split `remend` like any other dependency, while the CJS output never
names it and degrades to `fallbackRepair` as before.
There was a problem hiding this comment.
Pull request overview
This PR fixes @mui/x-chat’s lazy remend import so bundlers can statically resolve it, preventing unresolved import('remend') leftovers in browser bundles (and avoiding Vite 8 vite:preloadError on each assistant render) by switching to a #remend package subpath import that maps differently for ESM vs CJS.
Changes:
- Replace the unanalyzable variable-based dynamic import with a static
import('#remend')instreamingMarkdownRepair.ts. - Add a
#remendentry topackages/x-chat/package.json#importsto resolve toremendfor ESM and a local stub for other conditions. - Add regression tests and a
remendUnavailablestub module to validate the specifier and per-format contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/x-chat/src/internals/streamingMarkdownRepair.ts | Switches lazy loader to import('#remend') so bundlers can resolve and bundle remend. |
| packages/x-chat/src/internals/streamingMarkdownRepair.test.ts | Adds regression coverage for specifier analyzability and package.json import mapping (note: contains a path bug in packageRoot). |
| packages/x-chat/src/internals/remendUnavailable.ts | Adds CJS/“non-import condition” stub target used by #remend to force graceful fallback behavior. |
| packages/x-chat/package.json | Adds #remend subpath import mapping to support ESM real dependency and CJS stub resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { describe, expect, it, vi, beforeEach } from 'vitest'; | ||
| import { fallbackRepair, loadRemend, resetRemendCache } from './streamingMarkdownRepair'; | ||
|
|
||
| const packageRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); |
There was a problem hiding this comment.
Thanks for looking, but I don't think this one holds — it's one .. level off.
The test file is at packages/x-chat/src/internals/streamingMarkdownRepair.test.ts, so path.dirname(fileURLToPath(import.meta.url)) is packages/x-chat/src/internals, not packages/x-chat/src. Resolving '../..' from there walks up twice — internals → src → x-chat — landing on the package root:
packageRoot = /…/packages/x-chat
package.json exists: true
src/internals/streamingMarkdownRepair.ts exists: true
The reads resolve to packages/x-chat/package.json and packages/x-chat/src/internals/streamingMarkdownRepair.ts, both of which exist. It's also confirmed empirically: these tests pass locally and CI's test_unit job is green — a src/src/... path would have failed the suite outright.
Note the code has since moved to src/tests/packagingGuard/remendSpecifier.test.ts (to keep Node-only imports out of the browser project), where the same resolution is '../../..' for the extra directory level.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de36528a4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import fs from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; |
There was a problem hiding this comment.
The x-chat browser configuration includes this test file, so pnpm test:browser --project "x-chat" --run will load these top-level node:fs, node:path, and node:url imports in Chromium. Vite externalizes Node built-ins for browser compatibility, and the top-level path.resolve(...) or later fs.readFileSync(...) will fail the suite before these assertions can run. Move the filesystem-based regression checks into a separately excluded Node-only test file, as is already done for docsCorrectnessGuard, while leaving the runtime tests browser-compatible.
AGENTS.md reference: AGENTS.md:L40-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this was the cause of the test_browser / test_browser_react_18 failures, and you're right that the file never even got to the assertions: it failed to collect in Chromium (streamingMarkdownRepair.test.ts (0 test)), while the other 284 browser tests passed. I'd only run test:unit locally, which is why it slipped through.
Fixed in e12a218 exactly as suggested, following the docsCorrectnessGuard precedent:
- moved the two filesystem-based checks to
src/tests/packagingGuard/remendSpecifier.test.ts - added
**/packagingGuard/**to the exclude list invitest.config.browser.mts - left the runtime tests in
streamingMarkdownRepair.test.ts
Keeping the runtime tests browser-compatible turned out to be worth more than just unblocking CI — they now assert in Chromium that #remend resolves and that remend actually loads, which is precisely the behaviour #23160 was about. That file went from 0 collected tests to 11.
Verified: browser 27 files / 295 tests passing, unit 940 passing / 33 skipped. I also re-ran the mutation check (restoring the old variable specifier) to confirm the guard still fails from its new location.
There was a problem hiding this comment.
Correction to my reply above: I attributed the test_browser failures to this issue, and that was wrong. I inferred it from timing instead of reading the CI log. Pulling the log showed the original run had failed identically — all 27 x-chat suites, same setupVitest error — both before and after this fix. Your finding was a real defect (the file did fail to collect in Chromium locally) but it was not what CI was red on.
The actual cause was a side effect of the fix working. Now that import('#remend') genuinely resolves, the browser suite loads remend for real, and Vite's scanner can't see it up front behind a lazy subpath import:
[vite] (client) dependency optimized: remend
[vite] (client) optimized dependencies changed. reloading
[vitest] Vite unexpectedly reloaded a test.
The mid-run re-optimization reloads the page and drops every in-flight suite in the project. It passed locally only because my optimizer cache was warm — I reproduced it by clearing node_modules/.vite. Fixed in 862a26f by adding remend to optimizeDeps.include, as that warning recommends.
test_browser now gets through the whole x-chat project; the only remaining failure is an unrelated flaky DataGridPro data-source test (expected 11 to equal 12, retried 4x).
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
The two filesystem-based regression checks imported `node:fs`, `node:path` and `node:url` at the top level of a file the browser project collects, so `test:browser` failed to import the module before any assertion could run. Move them to `src/tests/packagingGuard/remendSpecifier.test.ts` and exclude that directory from the browser config, mirroring `docsCorrectnessGuard`. The runtime tests stay in `streamingMarkdownRepair.test.ts`, where they are worth more: they now also assert in Chromium that `#remend` resolves.
Making the specifier statically analyzable also made it genuinely loadable,
so the browser suite now imports `remend` for real. Vite's dependency
scanner cannot see it up front — it sits behind a lazy `import('#remend')` —
so it was discovered mid-run, and the re-optimization reloaded the page and
dropped every in-flight suite in the project:
[vite] dependency optimized: remend
[vite] optimized dependencies changed. reloading
[vitest] Vite unexpectedly reloaded a test.
Add it to `optimizeDeps.include`, as that warning recommends. Reproduced and
verified against a cold optimizer cache, which is why it only showed on CI.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mui#23160 was invisible to every in-process test: `loadRemend` resolved `remend` fine under Node and the Vitest dev server, while the bundled output shipped to users carried a bare `import('remend')` that could never resolve in a browser. The upgrade was dead code everywhere it mattered and nothing failed. Bundle the loader with Vite the way a consumer would, then run the bundle and assert the markdown comes back repaired (`a **bold` -> `a **bold**`), plus that the dynamic import points at an emitted chunk rather than a bare specifier. Restoring the original variable specifier fails the structural assertion here and the Chromium run of `streamingMarkdownRepair.test.ts` at runtime.
Leaving React and `@mui/*` external meant the emitted module still had to
resolve bare specifiers from a temp directory at import time, which worked
locally but not under CI's install layout:
Cannot find package '@mui/x-chat-headless' imported from
node_modules/.tmp-esm-bundle-test/bundle.mjs
Bundle everything instead — the hook and React tree-shake away, so the build
gets faster rather than slower — and assert the output has no bare specifiers
left, so the same fragility cannot come back unnoticed. The generated entry
moves out of the output directory so reading the build back never sees it.
The self-containment check scanned the bundle with a regex, which is unsound: the bundle inlines third-party source, and a plain string literal in there reads exactly like an import specifier. React 18 ships one — its element validator builds a warning out of `... from " + getComponentNameFromType(...)` — so `test_unit_react_18` reported it as a leftover external. Use Rollup's own parsed records instead. They state the fact directly: the entry chunk's `dynamicImports` is `['dist-<hash>.mjs']` with the fix and `[]` with the original variable specifier, and no chunk imports anything that was not emitted. Verified against both React 19 and a React 18 bundle.
Closes #23160
The bug
remendwas imported through a variable specifier plus@vite-ignore/webpackIgnorehints:No bundler can statically analyze that, so browser bundles were left with a bare
import('remend')that can never resolve without an import map. Two consequences, both reported in the issue:remendupgrade was dead code in every bundled app —.catch(() => fallbackRepair)always ran, regardless of bundler.__vitePreload, which ends inbaseModule().catch(handlePreloadError)and dispatches a globalvite:preloadError. Apps following Vite's documented reload recipe reload on every assistant reply.I reproduced both by building
@mui/x-chatand consuming it from a real Vite 8 production build. The output bundle containedc = "remend"; __vitePreload(() => import(c), [])with noremendcode bundled, and the page logged:Why not just inline the specifier
The variable was guarding something real.
remendis ESM-only — itsexportsmap declares norequirecondition — so a plainimport('remend')downlevels torequire('remend')in the CJS build, and webpack then hard-fails:I confirmed this by trying it. The specifier genuinely has to differ per output format; the previous code just picked an escape hatch that broke the browser instead.
The fix
A
#remendsubpath import inpackages/x-chat/package.json:code-infraalready rewrites#-prefixed imports that point at source files into built paths withimport/require/typesconditions (createPackageImports), producing:So the loader becomes a plain, statically analyzable
import('#remend'). ESM resolves the real package; CJS resolves a stub whose non-function export trips the existingtypeof remend !== 'function'branch and degrades tofallbackRepair, exactly as before.Verification
Since this is a packaging bug, unit tests alone can't confirm it. I built the package and exercised all four consumption paths:
preloadError,remendnot bundledremendcode-split (11.8 kB) and workingtarget: nodeover CJS outputremendremendfallbackRepairfallbackRepair, no uncaught rejectionThe repro page now shows
REPAIR: remend/OUTPUT: "a **bold**".Tests
Four cases added to
streamingMarkdownRepair.test.ts:remendend-to-endfallbackRepairpackage.jsonper-format contractI mutation-tested these by restoring the original buggy specifier — the guard goes red, so it catches the actual regression rather than just passing.
pnpm test:unit --project "x-chat*"→ 940 passed / 33 skipped. Typecheck and lint clean.Note for reviewers
remendhas never actually run in a bundled browser app until now, so this also changes rendering output for partial inline markdown —a **boldmid-stream now completes toa **bold**instead of rendering raw. That is the intended behaviour of the feature, but it is a real visible change, not just a silenced event.Follow-up tracked separately:
remendbeing ESM-only means CJS/SSR consumers still getfallbackRepair. Vendoring it (or upstreaming arequirecondition) would remove the#remendbranch entirely.Changelog
Fixed the
remendmarkdown-repair import so it resolves in bundled applications. It previously used a specifier no bundler could analyze, which left the repair inactive in every browser bundle and dispatched a globalvite:preloadErroron each assistant reply under Vite 8.