Repository navigation
perf(cli): code-split lowlight to cut startup V8 parse cost #4070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
c201ba2
d581da0
872d971
fd9da1a
3a97c45
d5acc21
ffd7f8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,9 +74,12 @@ const external = [ | |
|
|
||
| esbuild | ||
| .build({ | ||
| entryPoints: ['packages/cli/index.ts'], | ||
| entryPoints: { cli: 'packages/cli/index.ts' }, | ||
| bundle: true, | ||
| outfile: 'dist/cli.js', | ||
| outdir: 'dist', | ||
| entryNames: '[name]', | ||
| chunkNames: 'chunks/[name]-[hash]', | ||
| splitting: true, | ||
| platform: 'node', | ||
| format: 'esm', | ||
| target: 'node22', | ||
|
|
@@ -103,6 +106,11 @@ esbuild | |
| '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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] — DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a contributor-facing warning to the 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 |
||
| // vendored libraries that emit their own `var __dirname` locals don't | ||
| // collide with our injected bindings when code-splitting is enabled. | ||
| __dirname: '__qwen_dirname', | ||
| __filename: '__qwen_filename', | ||
| }, | ||
| loader: { '.node': 'file' }, | ||
| plugins: [wasmBinaryPlugin, wasmLoader({ mode: 'embedded' })], | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -61,6 +61,7 @@ import { | |||||||||||||||||
| ToolNames, | ||||||||||||||||||
| } from '@qwen-code/qwen-code-core'; | ||||||||||||||||||
| import { buildResumedHistoryItems } from './utils/resumeHistoryUtils.js'; | ||||||||||||||||||
| import { loadLowlight } from './utils/lowlightLoader.js'; | ||||||||||||||||||
| import { | ||||||||||||||||||
| getStickyTodos, | ||||||||||||||||||
| getStickyTodoMaxVisibleItems, | ||||||||||||||||||
|
|
@@ -406,6 +407,31 @@ export const AppContainer = (props: AppContainerProps) => { | |||||||||||||||||
| const lastTitleRef = useRef<string | null>(null); | ||||||||||||||||||
| const staticExtraHeight = 3; | ||||||||||||||||||
|
|
||||||||||||||||||
| // Prefetch the lowlight chunk on mount so the dynamic import is already | ||||||||||||||||||
| // in flight before the first code block needs colorizing. Without this | ||||||||||||||||||
| // kick-off, 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`. Firing the load | ||||||||||||||||||
| // from mount keeps the startup parse-cost win (V8 still parses off the | ||||||||||||||||||
| // critical path) while restoring the "first paint sees a loaded | ||||||||||||||||||
| // instance" guarantee. Errors are silently swallowed; CodeColorizer | ||||||||||||||||||
| // already falls back to plain text on miss. | ||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||
| void loadLowlight().catch((err) => { | ||||||||||||||||||
| // The loader latches its failure permanently (see `lowlightFailed` in | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Comment references
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||
| // `lowlightLoader.ts`), so this catch only fires once per session. 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. | ||||||||||||||||||
| debugLogger.warn( | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||
| `Failed to load lowlight chunk; code blocks will render as plain text: ${err instanceof Error ? err.message : String(err)}`, | ||||||||||||||||||
| ); | ||||||||||||||||||
| }); | ||||||||||||||||||
| }, []); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Initialize config (runs once on mount) | ||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||
| (async () => { | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |||||||||||||||||
|
|
||||||||||||||||||
| import React from 'react'; | ||||||||||||||||||
| import { Text, Box } from 'ink'; | ||||||||||||||||||
| import { common, createLowlight } from 'lowlight'; | ||||||||||||||||||
| import type { | ||||||||||||||||||
| Root, | ||||||||||||||||||
| Element, | ||||||||||||||||||
|
|
@@ -22,11 +21,20 @@ import { | |||||||||||||||||
| } from '../components/shared/MaxSizedBox.js'; | ||||||||||||||||||
| import type { LoadedSettings } from '../../config/settings.js'; | ||||||||||||||||||
| import { createDebugLogger } from '@qwen-code/qwen-code-core'; | ||||||||||||||||||
| import { getLowlightInstance, loadLowlight } from './lowlightLoader.js'; | ||||||||||||||||||
|
|
||||||||||||||||||
| // Configure theming and parsing utilities. | ||||||||||||||||||
| const lowlight = createLowlight(common); | ||||||||||||||||||
| const debugLogger = createDebugLogger('CODE_COLORIZER'); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Lowlight is heavy (~1.5 MB bundled, ~36–60 ms V8 parse). It's loaded lazily | ||||||||||||||||||
| // from `./lowlightLoader.js` via dynamic import so it lives in a separate | ||||||||||||||||||
| // esbuild chunk that's only parsed once a code block actually needs | ||||||||||||||||||
| // highlighting. To avoid leaving code blocks committed to ink's append-only | ||||||||||||||||||
| // <Static> region as plain text for the rest of the session, AppContainer | ||||||||||||||||||
| // fires `loadLowlight()` from a mount effect — in steady state the import | ||||||||||||||||||
| // is already resolved by the time any colorize call lands. The fallback | ||||||||||||||||||
| // below still handles the brief window before resolution and any | ||||||||||||||||||
| // permanent-failure path (latched inside lowlightLoader). | ||||||||||||||||||
|
|
||||||||||||||||||
| function renderHastNode( | ||||||||||||||||||
| node: Root | Element | HastText | RootContent, | ||||||||||||||||||
| theme: Theme, | ||||||||||||||||||
|
|
@@ -97,11 +105,21 @@ function highlightAndRenderLine( | |||||||||||||||||
| language: string | null, | ||||||||||||||||||
| theme: Theme, | ||||||||||||||||||
| ): React.ReactNode { | ||||||||||||||||||
| // 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(); | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] On failure,
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 3a97c45 with the exact pattern you suggested: extracted Side benefit: |
||||||||||||||||||
| if (!ll) { | ||||||||||||||||||
| void loadLowlight().catch((err) => { | ||||||||||||||||||
| debugLogger.error('[CodeColorizer] failed to load lowlight:', err); | ||||||||||||||||||
| }); | ||||||||||||||||||
| return line; | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 So a code block that lands in The simplest fix is to fire — claude-opus-4-7[1m] via Qwen Code /qreview
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch on the Also updated the |
||||||||||||||||||
| } | ||||||||||||||||||
| try { | ||||||||||||||||||
| const getHighlightedLine = () => | ||||||||||||||||||
| !language || !lowlight.registered(language) | ||||||||||||||||||
| ? lowlight.highlightAuto(line) | ||||||||||||||||||
| : lowlight.highlight(language, line); | ||||||||||||||||||
| !language || !ll.registered(language) | ||||||||||||||||||
| ? ll.highlightAuto(line) | ||||||||||||||||||
| : ll.highlight(language, line); | ||||||||||||||||||
|
|
||||||||||||||||||
| const renderedNode = renderHastNode(getHighlightedLine(), theme, undefined); | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,72 @@ | ||||||||||||||||||
| /** | ||||||||||||||||||
| * @license | ||||||||||||||||||
| * Copyright 2025 Google LLC | ||||||||||||||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||
| */ | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Standalone loader for the lowlight syntax-highlight engine. | ||||||||||||||||||
| * | ||||||||||||||||||
| * Kept in its own module — with zero imports beyond `lowlight` itself — so | ||||||||||||||||||
| * that priming the cache from `test-setup.ts` does not transitively pull | ||||||||||||||||||
| * `themeManager`, settings, or `@qwen-code/qwen-code-core` into every test | ||||||||||||||||||
| * file's module graph. That cascade was observed to alter theme/config test | ||||||||||||||||||
| * outcomes (e.g. theme-manager auto-detection and QWEN_HOME env tests). | ||||||||||||||||||
| */ | ||||||||||||||||||
|
|
||||||||||||||||||
| import type { Root } from 'hast'; | ||||||||||||||||||
|
|
||||||||||||||||||
| export type Lowlight = { | ||||||||||||||||||
| registered(language: string): boolean; | ||||||||||||||||||
| highlight(language: string, value: string): Root; | ||||||||||||||||||
| highlightAuto(value: string): Root; | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| let lowlightInstance: Lowlight | null = null; | ||||||||||||||||||
| let lowlightLoad: Promise<Lowlight> | null = null; | ||||||||||||||||||
| // Latches once the dynamic import has failed permanently. Without this, | ||||||||||||||||||
| // every React render of a code block would re-call `loadLowlight()` and | ||||||||||||||||||
| // re-attempt `import('lowlight')` — wasting CPU and spamming debug logs on | ||||||||||||||||||
| // every keystroke if the chunk file is permanently missing (corrupted | ||||||||||||||||||
| // install). A single permanent latch is acceptable because the colorizer | ||||||||||||||||||
| // already falls back to plain text on miss; recovery requires a fresh | ||||||||||||||||||
| // process anyway. | ||||||||||||||||||
| let lowlightFailed = false; | ||||||||||||||||||
| let lowlightError: Error | null = null; | ||||||||||||||||||
|
|
||||||||||||||||||
| export function getLowlightInstance(): Lowlight | null { | ||||||||||||||||||
| return lowlightInstance; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Kicks off (or returns the in-flight) load of the lowlight chunk. Exported | ||||||||||||||||||
| * for two callers: | ||||||||||||||||||
| * 1. `CodeColorizer.tsx` — fires the load on first colorize call so the | ||||||||||||||||||
| * next React commit picks up the highlighted output. | ||||||||||||||||||
| * 2. `test-setup.ts` — awaits this once to keep snapshot tests | ||||||||||||||||||
| * deterministic without dragging more modules into the test graph. | ||||||||||||||||||
| * | ||||||||||||||||||
| * Once an import attempt fails the failure is latched (see `lowlightFailed`) | ||||||||||||||||||
| * and subsequent calls return the same rejection without retrying. | ||||||||||||||||||
| */ | ||||||||||||||||||
| export function loadLowlight(): Promise<Lowlight> { | ||||||||||||||||||
| if (lowlightInstance) return Promise.resolve(lowlightInstance); | ||||||||||||||||||
| if (lowlightFailed) { | ||||||||||||||||||
| return Promise.reject( | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The — DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call — replaced the permanent latch with a 30-second cooldown in 3a97c45. Mechanics: Tradeoff vs options (a)/(b)/(c) from the review:
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. |
||||||||||||||||||
| lowlightError ?? new Error('lowlight import previously failed'), | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
| if (lowlightLoad) return lowlightLoad; | ||||||||||||||||||
| lowlightLoad = import('lowlight') | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||||||
| .then((mod) => { | ||||||||||||||||||
| lowlightInstance = mod.createLowlight(mod.common) as Lowlight; | ||||||||||||||||||
| return lowlightInstance; | ||||||||||||||||||
| }) | ||||||||||||||||||
| .catch((err) => { | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Consider adding a permanent-failure sentinel so once
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 872d971. Added a The |
||||||||||||||||||
| lowlightFailed = true; | ||||||||||||||||||
| lowlightError = err instanceof Error ? err : new Error(String(err)); | ||||||||||||||||||
| lowlightLoad = null; | ||||||||||||||||||
| throw err; | ||||||||||||||||||
| }); | ||||||||||||||||||
| return lowlightLoad; | ||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2025 Qwen team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import * as path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| /** | ||
| * Resolves the on-disk directory a module should treat as a sibling of the | ||
| * bundled `cli.js` entry, given the caller's `import.meta.url`. | ||
| * | ||
| * Why this exists: `esbuild.config.js` ships with `splitting: true` and | ||
| * `chunkNames: 'chunks/[name]-[hash]'`, so modules that are hoisted into a | ||
| * shared chunk live at `dist/chunks/<chunk>.js`. Any code that derives a path | ||
| * from `import.meta.url` and joins a sibling asset (e.g. `bundled/`, | ||
| * `vendor/`, `locales/`, `examples/`) would otherwise land in | ||
| * `dist/chunks/<asset>` and miss the actual `dist/<asset>` location. | ||
| * | ||
| * The fix is intentionally narrow: only strip the trailing path segment when | ||
| * its basename is exactly `chunks`. In source / transpiled / non-split | ||
| * builds the trailing segment is the source directory's own name, never | ||
| * `chunks`, so this is a no-op there. | ||
| * | ||
| * Centralising the check keeps the coupling to `esbuild.config.js`'s | ||
| * `chunkNames` setting in one place — if that ever changes, only this helper | ||
| * needs to be updated (rather than each call site). | ||
| * | ||
| * @param importMetaUrl Pass `import.meta.url` from the caller. It must be | ||
| * evaluated at the caller's chunk so the resolution matches that chunk's | ||
| * on-disk location; centralising the `fileURLToPath`/`dirname` work here | ||
| * does not change that. | ||
| * @returns The directory that should be used as the anchor for sibling | ||
| * asset lookups (`path.join(result, 'bundled')`, etc.). | ||
| */ | ||
| export function resolveBundleDir(importMetaUrl: string): string { | ||
| const moduleDir = path.dirname(fileURLToPath(importMetaUrl)); | ||
| return path.basename(moduleDir) === 'chunks' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] — DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 3a97c45: extracted I considered importing the TS constant directly into If you'd still prefer a runtime check (e.g. a build-time assertion that the two strings agree), happy to add a small |
||
| ? path.dirname(moduleDir) | ||
| : moduleDir; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,8 +5,8 @@ | |||||||
| */ | ||||||||
|
|
||||||||
| import path from 'node:path'; | ||||||||
| import { fileURLToPath } from 'node:url'; | ||||||||
| import { execFile } from 'node:child_process'; | ||||||||
| import { resolveBundleDir } from './bundlePaths.js'; | ||||||||
| import { fileExists } from './fileUtils.js'; | ||||||||
| import { execCommand, isCommandAvailable } from './shell-utils.js'; | ||||||||
| import { createDebugLogger } from './debugLogger.js'; | ||||||||
|
|
@@ -57,9 +57,11 @@ function wslTimeout(): number { | |||||||
| : RIPGREP_RUN_TIMEOUT_MS; | ||||||||
| } | ||||||||
|
|
||||||||
| // 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 | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] The local
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed broken — and the bug is reachable in dev mode (
Vitest hides the bug because Vite's transform synthesises a string Fix in 3a97c45: restored the local |
||||||||
| // lookups (here: the vendored ripgrep binary copied to `dist/vendor/`). See | ||||||||
| // `resolveBundleDir` for the rationale behind stripping a trailing `chunks/` | ||||||||
| // segment when this module is hoisted into a shared esbuild chunk. | ||||||||
| const __dirname = resolveBundleDir(import.meta.url); | ||||||||
|
|
||||||||
| type Platform = 'darwin' | 'linux' | 'win32'; | ||||||||
| type Architecture = 'x64' | 'arm64'; | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,9 +36,15 @@ const TARGETS = new Map([ | |||||||||||
| ['win-x64', { outputExtension: 'zip', nodeExecutable: ['node.exe'] }], | ||||||||||||
| ]); | ||||||||||||
|
|
||||||||||||
| const DIST_REQUIRED_PATHS = ['cli.js', 'vendor', 'bundled/qc-helper/docs']; | ||||||||||||
| const DIST_REQUIRED_PATHS = [ | ||||||||||||
| 'cli.js', | ||||||||||||
| 'chunks', | ||||||||||||
| 'vendor', | ||||||||||||
| 'bundled/qc-helper/docs', | ||||||||||||
| ]; | ||||||||||||
| const DIST_ALLOWED_ENTRIES = new Set([ | ||||||||||||
| 'cli.js', | ||||||||||||
| 'chunks', | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
— claude-opus-4-7[1m] via Qwen Code /qreview
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d581da0. Verified on a fresh |
||||||||||||
| 'vendor', | ||||||||||||
| 'bundled', | ||||||||||||
| 'package.json', | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical] Enabling
splitting: truesilently breaks three internal modules that derive runtime paths fromimport.meta.url. Each one assumed it would be co-located withcli.jsindist/; once esbuild hoists them intodist/chunks/chunk-*.js,path.dirname(fileURLToPath(import.meta.url))resolves todist/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.urlresolves to the real source file and the heuristics work. Suggested fixes (any one is enough):packages/cli/index.tsso esbuild keeps them incli.js). Smallest change, preserves the lowlight win.cli.js(whoseimport.meta.urlisdist/cli.js) and use that as the anchor in each module instead ofimport.meta.url. Most robust...until the expected sibling (bundled/,vendor/ripgrep/,locales/) is found.At minimum, please add a smoke test that runs the built
dist/cli.jsand 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified the bug empirically against the bundle this PR produces and fixed in 8d1da04d. All three modules now strip a trailing
chunkssegment whenpath.basename(moduleDir) === 'chunks', so the sibling-asset lookups resolve underdist/rather thandist/chunks/. In source / transpiled modes the basename is neverchunks, so the fallback is a no-op.Sanity check after rebuild:
Also added
chunkstoDIST_REQUIRED_PATHSincreate-standalone-package.jsso 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.