perf(cli): code-split lowlight to cut startup V8 parse cost - #4070
Conversation
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
17d00eb to
8f3813e
Compare
b0b9496 to
6115095
Compare
Move the syntax-highlight engine out of the synchronously-parsed cli.js
entry into a separately-emitted chunk and load it via dynamic import on
the first code-block render. Until the chunk arrives, code blocks render
as plain text; the next React commit of the surrounding subtree picks up
the highlighted version, so users never see incorrect highlighting –
just an imperceptibly later transition for the very first code block.
Mechanics:
- esbuild config: switch entry to outdir + splitting:true so that
`await import('lowlight')` produces an actual on-disk chunk that's
only parsed by V8 when first needed.
- esbuild-shims: rename injected __dirname/__filename to qwen-prefixed
symbols + use `define` to redirect free references. Previous inject
collided with vendored libraries (yargs) that ship their own
`var __dirname` ESM-compat polyfill once splitting flattens chunks.
- prepare-package: include the new chunks/ directory in the published
package's files list.
- CodeColorizer: keep the public colorize{Code,Line} signatures and HAST
rendering identical; on first call when the chunk hasn't loaded it
returns the plain line and fires the dynamic import via a tiny
standalone loader module.
- lowlightLoader (new): isolates the lazy-load surface to a module with
zero transitive imports (no themeManager, settings, or core). This
lets test-setup prime the cache without dragging the whole UI module
graph into every test file, which was observed to perturb theme and
settings test outcomes when CodeColorizer was imported directly.
- test-setup: await loadLowlight() once via the standalone loader so
synchronous snapshot tests see the highlighted output deterministically.
Measurements (real $HOME, n=15 interleaved A/B vs main HEAD, macOS):
| Metric | Before (mean±sd ms) | After (mean±sd ms) | Δ | t | p |
| ------------------ | ------------------- | ------------------ | -------- | ------ | -------- |
| firstByte (wall) | 1633.5 ± 88.7 | 1475.8 ± 73.3 | -157.7 | 5.31 | 1.33e-5 |
| idle (wall) | 2048.7 ± 93.6 | 1902.3 ± 80.2 | -146.3 | 4.60 | 8.71e-5 |
| cli.js size | 25 MB | 6.9 MB | -18.1 MB | — | — |
Both metrics clear the +50ms-or-10% Welch's t-test bar by an order of
magnitude. cli.js drops 72%; total payload (cli.js + chunks/) is
similar but only cli.js is parsed at module-eval time, which is the
phase that dominates the user-visible startup gap.
How to validate:
npm run bundle
ls dist/ # cli.js + chunks/lowlight-*.js
node dist/cli.js -y # interactive UI still renders
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
6115095 to
c201ba2
Compare
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
LGTM! The core code-splitting approach is sound and well-measured. A few robustness suggestions (non-blocking):
chunks/validation:prepare-package.js,create-standalone-package.js, andvscode-ide-companion/scripts/copy-bundled-cli.jsshould verifydist/chunks/exists before packaging — a partial build could ship without the split chunk.- No tests for
lowlightLoader.ts: the new loader module has zero direct test coverage; the plain-text fallback rendering path is never exercised becausetest-setup.tsprimes the cache eagerly. Consider adding unit tests for the load/success/failure state machine. - GitHub Release artifact:
release.ymluploads baredist/cli.jswithoutchunks/, so the chunk-basedimport('lowlight')will fail for that channel.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| lowlightInstance = mod.createLowlight(mod.common) as Lowlight; | ||
| return lowlightInstance; | ||
| }) | ||
| .catch((err) => { |
There was a problem hiding this comment.
[Suggestion] The .catch handler resets lowlightLoad = null on import failure, which means every subsequent React render will retry import('lowlight') unboundedly. If the chunk file is permanently missing (corrupted install), this loops forever — wasting CPU and spamming debug logs on every keystroke.
Consider adding a permanent-failure sentinel so once import('lowlight') fails, subsequent calls short-circuit immediately instead of retrying:
| .catch((err) => { | |
| .catch((err) => { | |
| lowlightLoad = null; | |
| lowlightFailed = true; | |
| throw err; | |
| }); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 872d971. Added a lowlightFailed sentinel that latches on the first import failure, so subsequent loadLowlight() calls short-circuit to a rejected promise without re-attempting import('lowlight'). The colorizer already falls back to plain text on miss; recovery requires a fresh process anyway.
The lowlightLoad = null reset on the failure path is intentionally kept — it pairs with the Promise.reject(lowlightError) short-circuit so the latched error replays without holding the unresolved promise.
| // module (no transitive imports of themeManager / settings / core) so this | ||
| // prime does not perturb any other test's module graph. | ||
| import { loadLowlight } from './src/ui/utils/lowlightLoader.js'; | ||
| await loadLowlight(); |
There was a problem hiding this comment.
[Suggestion] The top-level await loadLowlight() has no error handling. If lowlight is corrupted or the import fails (e.g. transient CI issue), vitest crashes during initialization — killing all CLI tests, including those unrelated to syntax highlighting.
Wrap in try/catch to degrade gracefully:
| await loadLowlight(); | |
| import { loadLowlight } from './src/ui/utils/lowlightLoader.js'; | |
| try { | |
| await loadLowlight(); | |
| } catch (err) { | |
| console.warn( | |
| '[test-setup] Failed to prime lowlight cache, ' + | |
| 'snapshot tests may show plain-text fallback:', | |
| String(err), | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 872d971. The top-level await loadLowlight() is now wrapped in try/catch — a transient import failure logs a warning and falls through, so vitest no longer crashes during init and unrelated tests still run. Snapshot tests that hit a code block will render the plain-text fallback.
| outdir: 'dist', | ||
| entryNames: '[name]', | ||
| chunkNames: 'chunks/[name]-[hash]', | ||
| splitting: true, |
There was a problem hiding this comment.
[Critical] Enabling splitting: true silently breaks three internal modules that derive runtime paths from import.meta.url. Each one assumed it would be co-located with cli.js in dist/; once esbuild hoists them into dist/chunks/chunk-*.js, path.dirname(fileURLToPath(import.meta.url)) resolves to dist/chunks/, but the assets they look up still live one level above. Verified empirically against the bundle this PR produces:
packages/core/src/skills/skill-manager.ts:88-92—bundledSkillsDirends up asdist/chunks/bundled. The actual skills directory isdist/bundled/{batch,loop,qc-helper,review}(created bycopy_bundle_assets.js). TheexistsSyncguard atskill-manager.ts:896swallows the miss into adebugLogger.warn, so all four bundled skills (/review,/qc-helper,/batch,/loop) silently disappear from any package built from this branch — no startup error, no user-visible signal.packages/core/src/utils/ripgrepUtils.ts:111-122—getBuiltinRipgrep()computesdist/chunks/vendor/ripgrep/<arch>-<plat>/rg, which doesn't exist. The bundled binary atdist/vendor/ripgrep/...becomes unreachable.resolveRipgrepthen falls back to systemrgif installed, or returnsnullon minimal hosts (CI runners, fresh containers) — makinggrepfall back to a slow internal scanner on the very environments the vendored binary was shipped for.packages/cli/src/i18n/index.ts:45-48— same pattern;getBuiltinLocalesDir()returnsdist/chunks/locales. User-visible behavior survives becausetryImportBundledTranslations(i18n/index.ts:114) loads via static glob import, but the loose-on-disk override path is now dead code.
Existing unit tests don't catch any of this — they all run in source mode where import.meta.url resolves to the real source file and the heuristics work. Suggested fixes (any one is enough):
- Mark the three modules as part of the entry chunk (e.g. eagerly import them from
packages/cli/index.tsso esbuild keeps them incli.js). Smallest change, preserves the lowlight win. - Pass the package root in from
cli.js(whoseimport.meta.urlisdist/cli.js) and use that as the anchor in each module instead ofimport.meta.url. Most robust. - Add a runtime fallback in each function that walks
..until the expected sibling (bundled/,vendor/ripgrep/,locales/) is found.
At minimum, please add a smoke test that runs the built dist/cli.js and asserts each of the three is reachable, otherwise the next code-splitting change will hit the same trap.
— claude-opus-4-7[1m] via Qwen Code /qreview
There was a problem hiding this comment.
Verified the bug empirically against the bundle this PR produces and fixed in 8d1da04d. All three modules now strip a trailing chunks segment when path.basename(moduleDir) === 'chunks', so the sibling-asset lookups resolve under dist/ rather than dist/chunks/. In source / transpiled modes the basename is never chunks, so the fallback is a no-op.
Sanity check after rebuild:
bundledSkillsDir resolves to: dist/bundled
exists: true
subdirs: [ 'batch', 'loop', 'qc-helper', 'review' ]
Also added chunks to DIST_REQUIRED_PATHS in create-standalone-package.js so a regressed bundle without chunks fails the packager check (separate from the [Suggestion] thread on line 42).
Re: smoke test for the built dist/cli.js — agree this is the right long-term gate. Not adding it in this round to keep the diff focused on the three concrete misses; will file a follow-up so the next code-splitting change has CI cover.
| void loadLowlight().catch((err) => { | ||
| debugLogger.error('[CodeColorizer] failed to load lowlight:', err); | ||
| }); | ||
| return line; |
There was a problem hiding this comment.
[Suggestion] The header comment promises that the highlighted version arrives "once React next re-renders the surrounding subtree (typically on the next user keystroke or message)." That holds for items still in the pending area, but not for items committed to ink's <Static> — packages/cli/src/ui/components/MainContent.tsx:205-211 explicitly notes Static is append-only: once an item is rendered to the terminal buffer it cannot be replaced, except via refreshStatic() (only triggered by theme switch / compact-mode merge / Ctrl+O — never on lowlight load).
So a code block that lands in <Static> before the dynamic import resolves stays plain text for the rest of the session. In typical streaming use the import wins easily, but it bites for: short --prompt -p runs that finalize quickly, Ctrl+C-cancelled first turns, and --resumed sessions whose first-paint history slice rides through the progressive Static replay path. The fact that test-setup.ts has to prime the loader synchronously to keep snapshots stable is itself evidence this race is reachable in production.
The simplest fix is to fire loadLowlight() from app boot (a non-awaited dispatch from AppContainer's mount effect), so the import is already in flight long before the first colorize call. The startup-cost win is preserved — V8 still parses the chunk off the critical path — but the "first paint sees a loaded instance" guarantee is restored. A heavier alternative is to broadcast a load-completion signal via context so memo'd subtrees can opt into re-render, but that doesn't help anything already absorbed into Static.
— claude-opus-4-7[1m] via Qwen Code /qreview
There was a problem hiding this comment.
Good catch on the <Static> race. Fixed in 872d971 by firing loadLowlight() from an AppContainer mount effect, exactly as suggested — the dynamic import is already in flight long before any colorize call lands, so the steady-state "first paint sees a loaded instance" guarantee is restored while the startup parse-cost win is preserved (V8 still parses off the critical path).
Also updated the CodeColorizer.tsx header comment to point at the AppContainer prefetch instead of claiming the next re-render fixes it.
| const DIST_REQUIRED_PATHS = ['cli.js', 'vendor', 'bundled/qc-helper/docs']; | ||
| const DIST_ALLOWED_ENTRIES = new Set([ | ||
| 'cli.js', | ||
| 'chunks', |
There was a problem hiding this comment.
[Suggestion] chunks is in DIST_ALLOWED_ENTRIES but missing from DIST_REQUIRED_PATHS on line 39. With splitting: true enabled, the chunks/ directory is functionally required — without it the bundled cli.js will throw ERR_MODULE_NOT_FOUND for every static import "./chunks/..." reference at startup, plus ERR_MODULE_NOT_FOUND mid-session for any lazy await import(). As the standalone packager stands today, a regressed bundle that produces only cli.js (e.g. someone reverts splitting: true only in esbuild.config.js, or a partial build) will be packaged into a broken archive without any error from this script — the failure first surfaces when an end user runs qwen from the unpacked tarball.
| 'chunks', | |
| const DIST_REQUIRED_PATHS = ['cli.js', 'chunks', 'vendor', 'bundled/qc-helper/docs']; | |
| const DIST_ALLOWED_ENTRIES = new Set([ | |
| 'cli.js', | |
| 'chunks', |
— claude-opus-4-7[1m] via Qwen Code /qreview
There was a problem hiding this comment.
Fixed in d581da0. chunks is now in DIST_REQUIRED_PATHS alongside cli.js, vendor, and bundled/qc-helper/docs, so a regressed bundle without the chunks directory fails the standalone packager check up front instead of producing a broken tarball.
Verified on a fresh npm run bundle — dist/chunks/ is populated (lowlight-FYAAUU5J.js, chunk-CYG2IFDL.js, chunk-UWSICIN3.js, et al.) and the packager check passes against it.
| export const __filename = fileURLToPath(import.meta.url); | ||
| export const __dirname = dirname(__filename); | ||
| export const __qwen_filename = fileURLToPath(import.meta.url); | ||
| export const __qwen_dirname = dirname(__qwen_filename); |
There was a problem hiding this comment.
[Suggestion] The header comment explains why the rename was needed (vendor var __dirname collisions), but doesn't warn future contributors about a real semantic change for first-party code: __qwen_dirname is computed once from this shim's import.meta.url, which now lives in dist/chunks/chunk-A4BMJM77.js. So any free __dirname/__filename reference written in our own source code will resolve to dist/chunks/, not the directory of the source file or even dist/. This is the same root cause as the skill-manager / ripgrepUtils / i18n bugs flagged on esbuild.config.js:82.
Existing first-party files survive only because they happen to declare a local shadow (const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename);) which esbuild's define correctly leaves alone. The next contributor who writes path.join(__dirname, '../docs') directly will silently get dist/chunks/../docs at runtime — works in npm run dev (where ts-node provides a real __dirname), fails or points to the wrong place in production.
Please add a contributor-facing note here (and adjacent to the define block in esbuild.config.js) along the lines of:
| export const __qwen_dirname = dirname(__qwen_filename); | |
| export const require = _require; | |
| // IMPORTANT: __qwen_filename / __qwen_dirname always resolve to this shim's | |
| // chunk file — i.e. dist/chunks/ in a built bundle, NOT the directory of any | |
| // source file that uses bare __dirname / __filename. esbuild's `define` | |
| // rewrites all free references in source code to these symbols, so to get a | |
| // per-file path you MUST declare a local shadow at the top of your module: | |
| // const __filename = fileURLToPath(import.meta.url); | |
| // const __dirname = path.dirname(__filename); | |
| export const __qwen_filename = fileURLToPath(import.meta.url); | |
| export const __qwen_dirname = dirname(__qwen_filename); |
— claude-opus-4-7[1m] via Qwen Code /qreview
There was a problem hiding this comment.
Fixed in 872d971. Added a contributor-facing block comment in esbuild-shims.js next to the __qwen_filename / __qwen_dirname exports, explaining that:
- These symbols always resolve to the shim's chunk file (
dist/chunks/), not the directory of the source file using them. - To get a per-file path you must declare a local shadow with
fileURLToPath(import.meta.url)at the top of the module. - Even with a local shadow, under code-splitting the path can still point to
dist/chunks/, so sibling-asset lookups (vendor/,bundled/,locales/) must strip a trailingchunkssegment — seeskill-manager.ts/ripgrepUtils.ts/i18n/index.tsfor the pattern.
Cross-referenced from the define block isn't added — kept the note in one place at the symbol definition site so it's the first thing a contributor reading either symbol's resolution sees.
With `splitting: true`, esbuild hoists modules with shared dependencies into `dist/chunks/`. Three modules derived runtime paths from `import.meta.url` assuming they were co-located with `cli.js`; once hoisted, `path.dirname(fileURLToPath(import.meta.url))` resolved to `dist/chunks/` and sibling-asset lookups silently missed: - `skill-manager.ts`: bundledSkillsDir → `dist/chunks/bundled` (actual `dist/bundled/`). The `existsSync` guard swallowed the miss, dropping all four bundled skills (`/review`, `/qc-helper`, `/batch`, `/loop`) with no user-visible signal. - `ripgrepUtils.ts`: `getBuiltinRipgrep()` → `dist/chunks/vendor/...`. Falls back to system rg if installed, otherwise null on minimal hosts — degrading grep to the slow internal scanner. - `i18n/index.ts`: `getBuiltinLocalesDir()` → `dist/chunks/locales`. User-visible behavior survives via the static glob import in `tryImportBundledTranslations`, but the loose-on-disk override path is dead. Each module now strips a trailing `chunks` segment when present, so the lookup resolves under `dist/`. In source / transpiled modes the basename is never `chunks`, so the fallback is a no-op. Also: - Add `chunks` to `DIST_REQUIRED_PATHS` in `create-standalone-package.js` so a regressed bundle that produces only `cli.js` fails the pre-packaging check instead of shipping a broken archive. - Expand `esbuild-shims.js` header so future contributors understand that `__qwen_filename` / `__qwen_dirname` always resolve to the shim's chunk file (dist/chunks/) and that sibling-asset lookups must strip the `chunks` segment. Reported by claude-opus-4-7 via Qwen Code /qreview on #4070. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Three follow-ups to the lowlight code-split:
- AppContainer fires `loadLowlight()` from a mount effect so the dynamic
import is already in flight before any code block needs colorizing.
Without this, code blocks committed to ink's append-only `<Static>`
region before the import resolves stay plain text for the rest of
the session — Static can only be re-rendered via `refreshStatic`,
which is not wired to lowlight load completion. Common reachable
paths: short `--prompt -p` runs that finalize quickly, Ctrl+C-
cancelled first turns, and the first-paint history replay on
`--resume`. The startup parse-cost win is preserved (V8 still
parses off the critical path).
- `lowlightLoader.ts` latches the first import failure so subsequent
calls short-circuit to a rejected promise instead of re-attempting
`import('lowlight')` on every keystroke. The colorizer already falls
back to plain text on miss; recovery requires a fresh process anyway.
- `test-setup.ts` wraps the top-level `await loadLowlight()` in
try/catch. A transient import failure no longer crashes the entire
vitest run — tests that hit a code block render the plain-text
fallback and surface a warning.
- `CodeColorizer.tsx` header comment updated to point at the
AppContainer prefetch instead of claiming first-paint always sees
a loaded instance.
Reported by DeepSeek/deepseek-v4-pro and claude-opus-4-7 via Qwen Code
/review and /qreview on #4070.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] packages/cli/src/commands/extensions/new.ts:22 — EXAMPLES_PATH = join(__dirname, 'examples') lacks the chunks/ path-stripping logic present in i18n/index.ts, skill-manager.ts, and ripgrepUtils.ts. Currently safe (module stays in cli.js), but will break silently if esbuild ever hoists it to a shared chunk — apply the same path.basename(moduleDir) === 'chunks' check.
[Suggestion] packages/cli/src/ui/utils/lowlightLoader.ts (new file, 72 lines) has no dedicated tests. The module's four state paths — loaded fast-path, failed latch, in-flight dedup, first-call — are only exercised implicitly. Consider adding lowlightLoader.test.ts.
| // instance" guarantee. Errors are silently swallowed; CodeColorizer | ||
| // already falls back to plain text on miss. | ||
| useEffect(() => { | ||
| void loadLowlight().catch(() => {}); |
There was a problem hiding this comment.
[Suggestion] Silent error swallowing — void loadLowlight().catch(() => {}) gives zero user-visible feedback if the lowlight chunk fails to load (corrupted install, missing chunk). Combined with CodeColorizer.tsx logging only to the debug channel (QWEN_DEBUG_LOG_FILE), users see plain-text code blocks with no clue what went wrong.
Consider logging a one-time warning on first failure so the degraded state is visible:
void loadLowlight().catch((err) => {
// Log once — lowlightLoader already latches failures permanently
console.warn('[Qwen] Syntax highlighting unavailable:', String(err));
});— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Done — switched to debugLogger.warn from this file's existing logger:
useEffect(() => {
void loadLowlight().catch((err) => {
debugLogger.warn(
`Failed to load lowlight chunk; code blocks will render as plain text: ${err instanceof Error ? err.message : String(err)}`,
);
});
}, []);Deliberately routed to debugLogger rather than console.warn so we don't paint a warning onto the user's TTY while ink is mounted — the loader already latches failures permanently (so the catch fires at most once per session), and CodeColorizer still falls back to plain text, so the user-visible degradation is the only signal that ever reaches them on stdout. The debug-log breadcrumb is for the support path (QWEN_DEBUG_LOG_FILE=...). Commit fd9da1a.
| // segment so the lookup resolves under `dist/`. In source / transpiled | ||
| // modes the basename is never `chunks`, so this is a no-op. | ||
| let moduleDir = path.dirname(__filename); | ||
| if (path.basename(moduleDir) === 'chunks') { |
There was a problem hiding this comment.
[Suggestion] The path.basename(moduleDir) === 'chunks' check is hardcoded identically in 3 files (i18n/index.ts, skill-manager.ts:96, ripgrepUtils.ts:69), implicitly coupling them to esbuild.config.js chunkNames: 'chunks/[name]-[hash]'. If chunkNames changes, all 3 locations must be updated in sync — missing one causes silent path resolution failure in the built bundle only.
Consider extracting a shared utility (e.g., resolveBundleDir(import.meta.url) in packages/core) to keep the coupling in one place.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Done — extracted resolveBundleDir(import.meta.url) to packages/core/src/utils/bundlePaths.ts and re-exported from @qwen-code/qwen-code-core. All 4 sites (i18n/index.ts, skill-manager.ts, ripgrepUtils.ts, plus commands/extensions/new.ts from the other suggestion in this review) now route through the helper, so the coupling to esbuild.config.js's chunkNames: 'chunks/[name]-[hash]' lives in one place. Verified after npm run bundle that import.meta.url is still evaluated at each caller's chunk (so the chunks-strip continues to fire only when the caller actually lives under dist/chunks/). Existing tests pass (154 in core, 191 in cli). Commit fd9da1a.
…s/new Centralises the `chunks/` strip pattern that three sites (`i18n/index.ts`, `skills/skill-manager.ts`, `utils/ripgrepUtils.ts`) each duplicated after the round-3 fix in d581da0. The implicit coupling to `esbuild.config.js`'s `chunkNames: 'chunks/[name]-[hash]'` now lives in a single helper (`packages/core/src/utils/bundlePaths.ts`), so a future rename only needs updating in one place. Also applies the same anchor to `commands/extensions/new.ts:EXAMPLES_PATH`. That module is currently bundled into `cli.js` (so the strip is a no-op today), but `qwen extensions new --help` always reads the examples directory in its yargs `builder` — confirmed against the built bundle that the lookup hits `dist/examples/` (sibling of `cli.js`). Using the helper future-proofs against esbuild later hoisting the module into a shared chunk, where the bare `__dirname`/`import.meta.url` lookup would silently break the command for every end user. While here, surface lowlight-load failures from `AppContainer`'s prefetch effect to the debug channel (`debugLogger.warn`) instead of swallowing them silently. The loader already latches failures permanently, so this fires at most once per session; `CodeColorizer` continues to fall back to plain text on miss, so user-visible behaviour is unchanged. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Re #4070 (review) — addressed both review-body suggestions in commit fd9da1a: 1. 2. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings not mappable to specific diff lines:
[Suggestion] Missing startup check: prepare-package.js validates cli.js, vendor/, and bundled/ exist at startup but does not verify chunks/. If splitting: true is disabled, packaging succeeds but runtime fails on import('lowlight').
[Suggestion] Silent failure: AppContainer.tsx logs lowlight load failure only to debugLogger.warn (file-based). Users see plain text with zero indication highlighting is broken. Consider adding a one-time stderr warning.
[Suggestion] Missing test coverage: lowlightLoader.ts (failure latch, request dedup) and bundlePaths.ts (chunks-strip branch) have no unit tests. CodeColorizer.tsx null-instance fallback path is never tested because test-setup.ts primes lowlight before tests.
| // Get the directory of the current module | ||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
| // Resolved at module load to the directory that should anchor sibling-asset |
There was a problem hiding this comment.
[Critical] The local const __filename = fileURLToPath(import.meta.url) was removed here, but getBuiltinRipgrep() at line 114 still references bare __filename (__filename.includes(path.join('src', 'utils')) and __filename.endsWith('.ts')). In dev mode (tsx), __filename is not a global in ESM — this throws ReferenceError. In bundled mode, esbuild's define rewrites it to __qwen_filename (the shim chunk path), making inSrcUtils always false and levelsUp always 0 — accidentally correct but fragile.
| // Resolved at module load to the directory that should anchor sibling-asset | |
| const __filename = fileURLToPath(import.meta.url); | |
| const __dirname = resolveBundleDir(import.meta.url); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Confirmed broken — and the bug is reachable in dev mode (
@qwen-code/qwen-code@0.15.10 dev
node scripts/dev.js → tsx), not just bundled output. Repro:
$ npx tsx -e "import {getBuiltinRipgrep} from './packages/core/src/utils/ripgrepUtils.ts'; getBuiltinRipgrep()"
ReferenceError: __filename is not defined
Vitest hides the bug because Vite's transform synthesises a string __filename for the loaded module; pure node ESM (tsx, the dev runner) does not. In the production bundle esbuild's define rewrites __filename to __qwen_filename (the shim chunk path), which makes inSrcUtils accidentally false and levelsUp accidentally 0 — wenshao's read is exactly right.
Fix in 3a97c45: restored the local const __filename = fileURLToPath(import.meta.url); shadow alongside resolveBundleDir(import.meta.url). Centralising into resolveBundleDir alone can't replace the per-file shadow because the heuristic needs both the per-file path and the chunk-stripped dirname. Added a comment block explaining why both bindings stick around.
| // Trigger the lazy load on first use; until it resolves, fall back to a | ||
| // plain-text rendering of the line. The next React render of the | ||
| // surrounding subtree will pick up the highlighted version. | ||
| const ll = getLowlightInstance(); |
There was a problem hiding this comment.
[Suggestion] On failure, highlightAndRenderLine calls void loadLowlight().catch(...) on EVERY code line of EVERY code block. After lowlightFailed latches, each call returns Promise.reject() immediately — the .catch() handler fires for every line, creating hundreds/thousands of duplicate debug log entries. The load trigger and error logging should happen once per code block, not per line.
| const ll = getLowlightInstance(); | |
| // In colorizeCode, before lines.map(): | |
| const ll = getLowlightInstance(); | |
| if (!ll) { | |
| void loadLowlight().catch((err) => { | |
| debugLogger.error('[CodeColorizer] failed to load lowlight:', err); | |
| }); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 3a97c45 with the exact pattern you suggested: extracted ensureLowlightLoading() and hoisted the call into colorizeCode (once per block) instead of highlightAndRenderLine (once per line). highlightAndRenderLine now takes the resolved Lowlight | null as a parameter, so it's pure lookup with no side effect on the per-line path.
Side benefit: colorizeLine (single-line code, used by inline backticks) goes through the same ensureLowlightLoading() helper, so its trigger path also gets the cooldown short-circuit from #3234507014 — duplicate logs there were less severe but also gone.
| */ | ||
| export function resolveBundleDir(importMetaUrl: string): string { | ||
| const moduleDir = path.dirname(fileURLToPath(importMetaUrl)); | ||
| return path.basename(moduleDir) === 'chunks' |
There was a problem hiding this comment.
[Suggestion] resolveBundleDir hardcodes path.basename(moduleDir) === 'chunks', coupling it to esbuild.config.js's chunkNames setting. If chunkNames changes to e.g. 'shared/[name]-[hash]', the check silently fails and all 5 call-site asset lookups (vendor/, bundled/, locales/, examples/) resolve to the wrong directory. No compile-time or runtime check guards this coupling. Consider exporting a shared CHUNK_DIR constant.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Done in 3a97c45: extracted BUNDLE_CHUNK_DIR = 'chunks' as a named export from packages/core/src/utils/bundlePaths.ts and updated esbuild.config.js to interpolate the same name into chunkNames (\${BUNDLE_CHUNK_DIR}/[name]-[hash]``).
I considered importing the TS constant directly into esbuild.config.js but that would force esbuild.config.js to run through tsx (currently plain node esbuild.config.js via npm run bundle), which is a bigger change than the coupling warrants. Instead both sides now declare BUNDLE_CHUNK_DIR = 'chunks' with explicit cross-reference comments — renaming on one side without the other stands out at review time, and the shared name makes the linkage greppable.
If you'd still prefer a runtime check (e.g. a build-time assertion that the two strings agree), happy to add a small scripts/check-bundle-config.js invoked from npm run bundle before esbuild runs — let me know.
| 'process.env.CLI_VERSION': JSON.stringify(pkg.version), | ||
| // Make global available for compatibility | ||
| global: 'globalThis', | ||
| // Redirect free __dirname/__filename references to the shim so that |
There was a problem hiding this comment.
[Suggestion] define: { __dirname: '__qwen_dirname', __filename: '__qwen_filename' } silently rewrites ALL bare __dirname/__filename references in bundled source files to the shim chunk path — not the source file's directory. Any new code adding path.join(__dirname, 'some-asset') gets a wrong path at runtime with no warning. Consider adding an ESLint rule banning bare __dirname/__filename in packages/*/src/.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Added a contributor-facing warning to the define block in 3a97c45 — it spells out that bare __dirname / __filename resolves to the shim chunk path (dist/chunks/), shows the fileURLToPath(import.meta.url) shadow pattern, and points future contributors at resolveBundleDir(import.meta.url) for sibling-asset lookups in modules that may end up hoisted into a shared chunk.
Left the ESLint rule out for now — adding a custom rule (no plugin handles this exact pattern, since the rule has to be "bare reference not shadowed by a local same-name binding") felt heavier than the bug count justifies. If we hit this trap a third time I'd happily land a small custom rule under packages/cli/eslint-plugin-internal/. The new comment also calls out the canonical safe pattern so reviewers have a concrete reference when this comes up.
| export function loadLowlight(): Promise<Lowlight> { | ||
| if (lowlightInstance) return Promise.resolve(lowlightInstance); | ||
| if (lowlightFailed) { | ||
| return Promise.reject( |
There was a problem hiding this comment.
[Suggestion] The lowlightFailed latch is permanent — once set, syntax highlighting is disabled for the entire process lifetime with no recovery path. Transient errors (EMFILE, disk I/O hiccup, antivirus file lock) cause irreversible degradation. Consider adding a cooldown-based retry (e.g., reset after 30 seconds) to recover from transient failures.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Good call — replaced the permanent latch with a 30-second cooldown in 3a97c45.
Mechanics: loadLowlight() records lowlightLastFailureAt = Date.now() on rejection and returns the cached rejection synchronously for any call within LOWLIGHT_RETRY_COOLDOWN_MS (30s). After the cooldown the next call retries the dynamic import. Exposed isLowlightCoolingDown() so the render-hot caller (CodeColorizer.ensureLowlightLoading) can skip both the call and the duplicate failure log.
Tradeoff vs options (a)/(b)/(c) from the review:
- (a) cooldown, picked: recovers from transient errors (EMFILE, antivirus locks, slow disk after wake-from-sleep). For a permanently-broken install the retry fires at most once every 30s — negligible vs the per-render hot loop the cooldown is designed to prevent. The user gets working highlights back within 30s if the underlying issue clears.
- (b) permanent latch + debug log, the previous behaviour: simplest, but leaves syntax highlighting dead for the whole session on any one-shot error. Combined with the per-line trigger in CodeColorizer (now also fixed) this was the worst-case path.
- (c) explicit reset: needs a UI trigger and a way to surface "highlighting is off, press X to retry" — more product surface than the failure mode warrants today.
If we ever wire up the "degraded-state stderr warning" suggested in your top-level review, that's a natural place to surface a one-shot retry too.
…ht loader
Round-4 review (wenshao 2026-05-13 13:12) flagged five issues in the
recent code-split work. This commit addresses all of them.
CRITICAL — `packages/core/src/utils/ripgrepUtils.ts`: the round-3
`resolveBundleDir` refactor removed the local `__filename` declaration
but `getBuiltinRipgrep` still references bare `__filename` to decide
how many `..` segments to walk. In `npm run dev` (tsx, ESM) `__filename`
is undefined so the function throws `ReferenceError`. In the bundle
esbuild's `define` rewrites it to `__qwen_filename` (the shim chunk
path), which is the wrong string but happens to short-circuit to
`levelsUp = 0` — accidentally correct only because the chunk-path
string never contains `path.join('src', 'utils')`. Reproduced via tsx:
`__filename is not defined`; fixed by re-introducing the explicit
local shadow plus a comment explaining why centralising both helpers
into `resolveBundleDir` cannot replace the per-file shadow.
`packages/cli/src/ui/utils/lowlightLoader.ts`: the previous permanent
`lowlightFailed` latch left syntax highlighting dead for the entire
process lifetime on transient errors (EMFILE, antivirus locks,
slow-disk-after-wake). Replaced with a 30-second cooldown — within the
window subsequent calls return the cached rejection synchronously
(keeps the per-render short-circuit that protects against
permanently-broken installs); after the cooldown the next call retries
the dynamic import. Exposes `isLowlightCoolingDown()` so render-hot
callers can also skip duplicate failure logging.
`packages/cli/src/ui/utils/CodeColorizer.tsx`: hoisted
`loadLowlight()` + log out of the per-line render loop into a single
`ensureLowlightLoading()` call at the top of `colorizeCode`. In the
failure case this collapses hundreds of duplicate debug entries (one
per line) to one per block. The instance is now passed down to
`highlightAndRenderLine` as a parameter.
`packages/core/src/utils/bundlePaths.ts` + `esbuild.config.js`:
exposed `BUNDLE_CHUNK_DIR = 'chunks'` as a named constant and updated
`esbuild.config.js` to interpolate the same name into `chunkNames`
(plus an explicit "MUST stay in sync" comment). Renaming on one side
without the other now stands out at review time. Also expanded the
`define` comment with a contributor-facing warning describing exactly
why bare `__dirname` / `__filename` in source files becomes the shim
chunk path, and pointing future contributors at the
`fileURLToPath(import.meta.url)` shadow pattern (and
`resolveBundleDir` for sibling-asset lookups).
Verified:
- typecheck (all 4 workspaces): clean
- packages/core tests: 7747 passing (no regressions)
- packages/cli tests: only the pre-existing `useAtCompletion.test.ts`
filesystem-order failures remain (confirmed against `git stash`)
- `npm run bundle` succeeds; `node dist/cli.js --version` returns
`0.15.10`; `node dist/cli.js --help` renders normally
- `npx tsx <call getBuiltinRipgrep>` now returns the vendored path
instead of throwing `ReferenceError`
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] Missing test coverage for new modules (4-6):
packages/cli/src/ui/utils/lowlightLoader.ts— new 99-line module with critical loading logic (in-flight dedup, cooldown, retry) has no test file.packages/core/src/utils/bundlePaths.ts— new 60-line module withresolveBundleDir()used by 4 callers has no test file.CodeColorizer.tsxplain-text fallback —ensureLowlightLoading()returningnulltriggers untested plain-text code path.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| * | ||
| * Hardcoded here to match `esbuild.config.js`'s | ||
| * `chunkNames: 'chunks/[name]-[hash]'` setting. Exported so the linkage is | ||
| * visible at the build-config side too — `esbuild.config.js` imports this |
There was a problem hiding this comment.
[Suggestion] Comment claims esbuild.config.js imports this constant but esbuild.config.js actually defines its own independent copy at line 81 (const BUNDLE_CHUNK_DIR = 'chunks'). The two are manually synced — renaming here does NOT update esbuild.config.js. A future maintainer trusting this comment could break resolveBundleDir for all 4 callers.
| * visible at the build-config side too — `esbuild.config.js` imports this | |
| * Hardcoded here to match `esbuild.config.js`'s | |
| * `chunkNames: 'chunks/[name]-[hash]'` setting. The two files each | |
| * define their own copy (esbuild.config.js cannot import TS source), | |
| * so renaming here requires renaming there too. The comment in | |
| * esbuild.config.js cross-references this file as authoritative. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in d5acc21 — updated the comment to accurately describe the dual-definition arrangement. esbuild.config.js runs before any TS compile step so it can't import this module directly; documenting that constraint avoids the rename-trap the original wording implied.
| // already falls back to plain text on miss. | ||
| useEffect(() => { | ||
| void loadLowlight().catch((err) => { | ||
| // The loader latches its failure permanently (see `lowlightFailed` in |
There was a problem hiding this comment.
[Suggestion] Comment references lowlightFailed which no longer exists in lowlightLoader.ts. The current implementation uses a 30s cooldown with lowlightLastFailureAt / lowlightError — the loader does NOT latch failure permanently. A maintainer debugging highlight failures may search for the wrong variable and misunderstand the recovery behavior.
| // The loader latches its failure permanently (see `lowlightFailed` in | |
| // The loader caches rejection with a cooldown (see | |
| // `LOWLIGHT_RETRY_COOLDOWN_MS` / `lowlightLastFailureAt` in | |
| // `lowlightLoader.ts`). This useEffect runs once on mount, so this | |
| // catch fires at most once per session regardless. Log to the debug | |
| // channel so a degraded syntax-highlight state (corrupted install, | |
| // missing chunk) leaves a breadcrumb without spamming the user's | |
| // TTY — `CodeColorizer` already falls back to plain text. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ); | ||
| } | ||
| if (lowlightLoad) return lowlightLoad; | ||
| lowlightLoad = import('lowlight') |
There was a problem hiding this comment.
[Suggestion] mod.createLowlight(mod.common) as Lowlight has no runtime API shape validation. If lowlight changes its API (e.g., method renaming), the as cast silently coerces the mismatched object. The resulting TypeError in highlightAndRenderLine's try/catch is swallowed, causing all code blocks to silently render plain text with zero logs — no debug warning, no cooldown trigger.
| lowlightLoad = import('lowlight') | |
| const instance = mod.createLowlight(mod.common); | |
| if (typeof instance?.registered !== 'function' || | |
| typeof instance?.highlight !== 'function' || | |
| typeof instance?.highlightAuto !== 'function') { | |
| throw new Error('lowlight instance does not match expected API'); | |
| } | |
| lowlightInstance = instance as Lowlight; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in d5acc21 — added the three-method shape check before the cast. A mismatched object now throws a descriptive Error which routes through the existing .catch (sets lowlightLastFailureAt/lowlightError) so the cooldown latch engages and the degraded state surfaces in the debug channel exactly once. Extra cost is one typeof triple per session (load runs once).
… tests - lowlightLoader: validate runtime shape of createLowlight() before the `as Lowlight` cast so an upstream API rename routes through the cooldown latch instead of silently degrading every code block to plain text. - bundlePaths: correct doc comment — esbuild.config.js maintains its own `BUNDLE_CHUNK_DIR` constant rather than importing this one (it runs before any TS compile step). - AppContainer: update prefetch-failure comment to reference the cooldown symbols (`LOWLIGHT_RETRY_COOLDOWN_MS` / `lowlightLastFailureAt`) that replaced the removed `lowlightFailed` latch. - New unit tests covering the lowlightLoader state machine (success, in-flight dedup, shape mismatch, cooldown skip, post-cooldown retry) and `resolveBundleDir`'s strip-only-on-exact-match contract.
|
Re: test-coverage suggestion on review #4287700966 — added in d5acc21:
The |
…aths CI failure on Windows: the new `resolveBundleDir` tests built expected values with `path.join(path.sep, ...)` (e.g. `\tmp\dist`), but `pathToFileURL` resolves drive-less paths against the current drive on Windows. The URL -> `fileURLToPath` round-trip returned `D:\tmp\dist`, while the expectation stayed `\tmp\dist`, tripping all three new assertions. Switched both the URL source and the expected value to a single `path.resolve(path.sep, ...)` anchor per test so both sides absorb whatever the platform considers absolute. POSIX behaviour is unchanged (`/tmp/dist` -> `/tmp/dist`).
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Non-blocking suggestions not mapped to specific diff lines:
-
scripts/prepare-package.js— Missingchunks/startup validation. The file checkscli.js,vendor/,bundled/exist at startup but notchunks/. Ifsplitting: trueis enabled but the chunks directory is missing, the npm package publishes without chunk files, causing silent runtime degradation (all code blocks fall back to plain text with no error surfaced to the user). -
packages/cli/src/utils/sandbox.ts— Not migrated toresolveBundleDir. Four other asset-anchor sites were updated butsandbox.tsstill uses barenew URL('sandbox-macos-${profile}.sb', import.meta.url). If esbuild hoists this module intodist/chunks/, the path resolves incorrectly, breaking the macOS Seatbelt sandbox. -
Test coverage gap (pattern): Several key paths in the new async loading behavior lack test coverage —
loadLowlight()already-loaded fast path (lowlightLoader.ts:78),ensureLowlightLoadingfast-return incolorizeCodecontext (CodeColorizer.tsx:120), thecatch (_error)fallback inhighlightAndRenderLine(CodeColorizer.tsx:146), and the AppContainer mount effect. ThelowlightLoader.test.tscovers the loader module in isolation well, but the integration wiring throughCodeColorizeris not tested.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // channel so a degraded syntax-highlight state (corrupted install, | ||
| // missing chunk) leaves a breadcrumb without spamming the user's | ||
| // TTY — `CodeColorizer` already falls back to plain text. | ||
| debugLogger.warn( |
There was a problem hiding this comment.
[Suggestion] debugLogger.warn() here may be a no-op at mount time if Config.initialize() hasn't completed yet — the debug logger silently drops messages when no active session exists (session is set during Config construction). If the lowlight chunk fails to load on mount, the error breadcrumb could be lost entirely.
| debugLogger.warn( | |
| // Fallback to stderr in case debugLogger session isn't active yet | |
| const msg = `Failed to load lowlight chunk; code blocks will render as plain text: ${err instanceof Error ? err.message : String(err)}`; | |
| debugLogger.warn(msg); | |
| console.error(`[lowlight] ${msg}`); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| void loadLowlight().catch((err) => { | ||
| debugLogger.error('[CodeColorizer] failed to load lowlight:', err); | ||
| }); | ||
| } |
There was a problem hiding this comment.
[Suggestion] ensureLowlightLoading() returns null silently during the 30s cooldown window — no log is emitted. If the AppContainer mount effect triggered the first import failure (starting the cooldown), subsequent colorizeCode calls skip the load with no CODE_COLORIZER log entry, only the APP_CONTAINER warning. This makes cross-component debugging harder.
| } | |
| if (!isLowlightCoolingDown()) { | |
| void loadLowlight().catch((err) => { | |
| debugLogger.error('[CodeColorizer] failed to load lowlight:', err); | |
| }); | |
| } else { | |
| debugLogger.debug('[CodeColorizer] lowlight load in cooldown, skipping retry'); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
2 participants
Summary
Splits
lowlight(the syntax-highlighter, ~1.5 MB bundled, ~36–60 ms V8 parse cost) out of the synchronously-evaluatedcli.jsentry into a separately-emitted esbuild chunk that's only loaded when the first code block needs highlighting. Public API ofCodeColorizeris unchanged: callers still get back React/Ink nodes, but the very first render before the chunk arrives returns plain text, and the next React commit of the surrounding subtree picks up the highlighted version, so users never see incorrect highlighting — just an imperceptibly later upgrade for the very first code block in a session.cli.jsshrinks from 25 MB → 6.9 MB (–72 %). Total payload (cli.js+ newchunks/) is similar, but onlycli.jsis parsed at module-eval time — and that's the phase that dominates the perceived 1–3 s qwen-code cold-start gap.Real-world measurements (vs
mainHEAD, not stacked)n=20 interleaved A/B, randomized order each iteration, real
$HOME, macOS,qwen -yvianode-pty. Baseline ismainat76d8c0ce8.cli.jsfirstByte= wall time from spawn to first byte at the PTY;idle= wall time until stdout has been quiet for ≥ 1 s (proxy for "fully painted, ready"). Both metrics clear the ≥ 10 % or ≥ 50 ms Welch's t-test bar by an order of magnitude.What changed (5 files)
esbuild.config.js— switch entry tooutdir+splitting: truesoawait import('lowlight')becomes an on-disk chunk. Adddefinerewrites for__dirname/__filenameto qwen-prefixed symbols (see below).scripts/esbuild-shims.js— rename the injected__dirname/__filenameexports. The previous shim collided with vendored libraries (e.g. yargs) that ship their ownvar __dirnameESM-compat polyfill once splitting flattens chunks into the entry; rewriting free references in our source code keeps vendor-declared locals untouched.scripts/prepare-package.js— include the newchunks/directory in the publishedfileslist.packages/cli/src/ui/utils/CodeColorizer.tsx— keep the publiccolorize{Code,Line}signatures and HAST rendering identical. First call when the chunk hasn't arrived returns the plainlinestring and fires the dynamic import. Every subsequent React render of the surrounding subtree (which happens on each keystroke / message update) re-invokescolorize*and picks up the loaded instance — no manualsetStateplumbing needed.packages/cli/test-setup.ts—await loadLowlight()once in the global vitest setup so snapshot tests, which calllastFrame()synchronously, see the deterministic highlighted output.How to validate
Test plan
cli.jsshrinks ≥ 60 %dist/chunks/lowlight-*.jsis emittedMarkdownDisplay.test.tsxsnapshot suite passes (101 tests)--promptheadless mode renders highlighted code in conversation historyRollback
Single
git revert 8f3813e28undoes the entire change. No cross-PR dependencies; this PR is independent of #3994 (PR-A) and stacks directly onmain.Follow-ups (not in this PR)
Same mechanism extends naturally to:
tree-sitter-wasms(1.4 MB),react-devtools-core(564 KB, prod-only),grammy(487 KB, telegram channel),openai(317 KB, OpenAI auth only). Each should land as its own measured PR.🤖 Generated with Qwen Code