diff --git a/package-lock.json b/package-lock.json index 9559db78f..c51449d0d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17,7 +17,7 @@ ], "dependencies": { "@testing-library/dom": "^10.4.1", - "ink": "^6.2.3", + "ink": "^7.0.3", "simple-git": "^3.28.0" }, "bin": { @@ -91,9 +91,9 @@ } }, "node_modules/@alcalzone/ansi-tokenize": { - "version": "0.2.5", - "resolved": "https://registry.npmjs.org/@alcalzone/ansi-tokenize/-/ansi-tokenize-0.2.5.tgz", - "integrity": "sha512-3NX/MpTdroi0aKz134A6RC2Gb2iXVECN4QaAXnvCIxxIm3C3AVB1mkUe8NaaiyvOpDfsrqWhYtj+Q6a62RrTsw==", + "version": "0.3.0", + "resolved": "https://registry.npmjs.org/@alcalzone/ansi-tokenize/-/ansi-tokenize-0.3.0.tgz", + "integrity": "sha512-p+CMKJ93HFmLkjXKlXiVGlMQEuRb6H0MokBSwUsX+S6BRX8eV5naFZpQJFfJHjRZY0Hmnqy1/r6UWl3x+19zYA==", "license": "MIT", "dependencies": { "ansi-styles": "^6.2.1", @@ -6277,6 +6277,7 @@ "version": "5.2.0", "resolved": "https://registry.npmjs.org/cli-truncate/-/cli-truncate-5.2.0.tgz", "integrity": "sha512-xRwvIOMGrfOAnM1JYtqQImuaNtDEv9v6oIYAs4LIHwTiKee8uwvIi363igssOC0O5U04i4AlENs79LQLu9tEMw==", + "dev": true, "license": "MIT", "dependencies": { "slice-ansi": "^8.0.0", @@ -6293,6 +6294,7 @@ "version": "6.2.3", "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-6.2.3.tgz", "integrity": "sha512-4Dj6M28JB+oAH8kFkTLUo+a2jwOFkuqb3yucU0CANcRRUbxS0cP0nZYCGjcc3BNXwRIsUVmDGgzawme7zvJHvg==", + "dev": true, "license": "MIT", "engines": { "node": ">=12" @@ -6305,6 +6307,7 @@ "version": "5.1.0", "resolved": "https://registry.npmjs.org/is-fullwidth-code-point/-/is-fullwidth-code-point-5.1.0.tgz", "integrity": "sha512-5XHYaSyiqADb4RnZ1Bdad6cPp8Toise4TzEjcOYDHZkTCbKgiUl7WTUCpNWHuxmDt91wnsZBc9xinNzopv3JMQ==", + "dev": true, "license": "MIT", "dependencies": { "get-east-asian-width": "^1.3.1" @@ -6320,6 +6323,7 @@ "version": "8.0.0", "resolved": "https://registry.npmjs.org/slice-ansi/-/slice-ansi-8.0.0.tgz", "integrity": "sha512-stxByr12oeeOyY2BlviTNQlYV5xOj47GirPr4yA1hE9JCtxfQN0+tVbkxwCtYDQWhEKWFHsEK48ORg5jrouCAg==", + "dev": true, "license": "MIT", "dependencies": { "ansi-styles": "^6.2.3", @@ -6336,6 +6340,7 @@ "version": "8.2.1", "resolved": "https://registry.npmjs.org/string-width/-/string-width-8.2.1.tgz", "integrity": "sha512-IIaP0g3iy9Cyy18w3M9YcaDudujEAVHKt3a3QJg1+sr/oX96TbaGUubG0hJyCjCBThFH+tFpcIyoUHUn1ogaLA==", + "dev": true, "license": "MIT", "dependencies": { "get-east-asian-width": "^1.5.0", @@ -9820,43 +9825,43 @@ } }, "node_modules/ink": { - "version": "6.8.0", - "resolved": "https://registry.npmjs.org/ink/-/ink-6.8.0.tgz", - "integrity": "sha512-sbl1RdLOgkO9isK42WCZlJCFN9hb++sX9dsklOvfd1YQ3bQ2AiFu12Q6tFlr0HvEUvzraJntQCCpfEoUe9DSzA==", + "version": "7.0.3", + "resolved": "https://registry.npmjs.org/ink/-/ink-7.0.3.tgz", + "integrity": "sha512-5kxHkIj9+RuqCU3zyvP4qvYWNOSHP2TW/SHayHGHOmk87KwfVcZwvJGemi9ch+ci2gXUqerK/Eh2DGEDt5q45g==", "license": "MIT", "dependencies": { - "@alcalzone/ansi-tokenize": "^0.2.4", + "@alcalzone/ansi-tokenize": "^0.3.0", "ansi-escapes": "^7.3.0", - "ansi-styles": "^6.2.1", + "ansi-styles": "^6.2.3", "auto-bind": "^5.0.1", - "chalk": "^5.6.0", - "cli-boxes": "^3.0.0", + "chalk": "^5.6.2", + "cli-boxes": "^4.0.1", "cli-cursor": "^4.0.0", - "cli-truncate": "^5.1.1", + "cli-truncate": "^6.0.0", "code-excerpt": "^4.0.0", - "es-toolkit": "^1.39.10", + "es-toolkit": "^1.45.1", "indent-string": "^5.0.0", "is-in-ci": "^2.0.0", "patch-console": "^2.0.0", "react-reconciler": "^0.33.0", "scheduler": "^0.27.0", "signal-exit": "^3.0.7", - "slice-ansi": "^8.0.0", + "slice-ansi": "^9.0.0", "stack-utils": "^2.0.6", - "string-width": "^8.1.1", + "string-width": "^8.2.0", "terminal-size": "^4.0.1", - "type-fest": "^5.4.1", + "type-fest": "^5.5.0", "widest-line": "^6.0.0", - "wrap-ansi": "^9.0.0", - "ws": "^8.18.0", + "wrap-ansi": "^10.0.0", + "ws": "^8.20.0", "yoga-layout": "~3.2.1" }, "engines": { - "node": ">=20" + "node": ">=22" }, "peerDependencies": { - "@types/react": ">=19.0.0", - "react": ">=19.0.0", + "@types/react": ">=19.2.0", + "react": ">=19.2.0", "react-devtools-core": ">=6.1.2" }, "peerDependenciesMeta": { @@ -10034,6 +10039,34 @@ "url": "https://github.com/chalk/chalk?sponsor=1" } }, + "node_modules/ink/node_modules/cli-boxes": { + "version": "4.0.1", + "resolved": "https://registry.npmjs.org/cli-boxes/-/cli-boxes-4.0.1.tgz", + "integrity": "sha512-5IOn+jcCEHEraYolBPs/sT4BxYCe2nHg374OPiItB1O96KZFseS2gthU4twyYzeDcFew4DaUM/xwc5BQf08JJw==", + "license": "MIT", + "engines": { + "node": ">=18.20 <19 || >=20.10" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, + "node_modules/ink/node_modules/cli-truncate": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/cli-truncate/-/cli-truncate-6.0.0.tgz", + "integrity": "sha512-3+YKIUFsohD9MIoOFPFBldjAlnfCmCDcqe6aYGFqlDTRKg80p4wg35L+j83QQ63iOlKRccEkbn8IuM++HsgEjA==", + "license": "MIT", + "dependencies": { + "slice-ansi": "^9.0.0", + "string-width": "^8.2.0" + }, + "engines": { + "node": ">=22" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/ink/node_modules/is-fullwidth-code-point": { "version": "5.1.0", "resolved": "https://registry.npmjs.org/is-fullwidth-code-point/-/is-fullwidth-code-point-5.1.0.tgz", @@ -10071,16 +10104,16 @@ "license": "ISC" }, "node_modules/ink/node_modules/slice-ansi": { - "version": "8.0.0", - "resolved": "https://registry.npmjs.org/slice-ansi/-/slice-ansi-8.0.0.tgz", - "integrity": "sha512-stxByr12oeeOyY2BlviTNQlYV5xOj47GirPr4yA1hE9JCtxfQN0+tVbkxwCtYDQWhEKWFHsEK48ORg5jrouCAg==", + "version": "9.0.0", + "resolved": "https://registry.npmjs.org/slice-ansi/-/slice-ansi-9.0.0.tgz", + "integrity": "sha512-SO/3iYL5S3W57LLEniscOGPZgOqZUPCx6d3dB+52B80yJ0XstzsC/eV8gnA4tM3MHDrKz+OCFSLNjswdSC+/bA==", "license": "MIT", "dependencies": { "ansi-styles": "^6.2.3", "is-fullwidth-code-point": "^5.1.0" }, "engines": { - "node": ">=20" + "node": ">=22" }, "funding": { "url": "https://github.com/chalk/slice-ansi?sponsor=1" @@ -10132,6 +10165,23 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/ink/node_modules/wrap-ansi": { + "version": "10.0.0", + "resolved": "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-10.0.0.tgz", + "integrity": "sha512-SGcvg80f0wUy2/fXES19feHMz8E0JoXv2uNgHOu4Dgi2OrCy1lqwFYEJz1BLbDI0exjPMe/ZdzZ/YpGECBG/aQ==", + "license": "MIT", + "dependencies": { + "ansi-styles": "^6.2.3", + "string-width": "^8.2.0", + "strip-ansi": "^7.1.2" + }, + "engines": { + "node": ">=20" + }, + "funding": { + "url": "https://github.com/chalk/wrap-ansi?sponsor=1" + } + }, "node_modules/ink/node_modules/ws": { "version": "8.20.0", "resolved": "https://registry.npmjs.org/ws/-/ws-8.20.0.tgz", @@ -13054,7 +13104,6 @@ "os": [ "darwin" ], - "peer": true, "engines": { "node": "^8.16.0 || ^10.6.0 || >=11.0.0" } @@ -17231,7 +17280,7 @@ "fzf": "^0.5.2", "glob": "^10.5.0", "highlight.js": "^11.11.1", - "ink": "^6.2.3", + "ink": "^7.0.3", "ink-gradient": "^3.0.0", "ink-link": "^4.1.0", "ink-spinner": "^5.0.0", @@ -17239,7 +17288,7 @@ "open": "^10.1.2", "p-limit": "^7.3.0", "prompts": "^2.4.2", - "react": "^19.1.0", + "react": "^19.2.4", "read-package-up": "^11.0.0", "shell-quote": "^1.8.3", "simple-git": "^3.28.0", diff --git a/package.json b/package.json index 7dcba6936..6880a519c 100644 --- a/package.json +++ b/package.json @@ -79,7 +79,11 @@ "wrap-ansi": "7.0.0" }, "baseline-browser-mapping": "^2.9.19", - "normalize-package-data": "^7.0.1" + "normalize-package-data": "^7.0.1", + "react": "^19.2.4", + "react-dom": "^19.2.4", + "@types/react": "^19.2.0", + "@types/react-dom": "^19.2.0" }, "bin": { "qwen": "dist/cli.js" @@ -130,7 +134,7 @@ }, "dependencies": { "@testing-library/dom": "^10.4.1", - "ink": "^6.2.3", + "ink": "^7.0.3", "simple-git": "^3.28.0" }, "optionalDependencies": { diff --git a/packages/cli/package.json b/packages/cli/package.json index 962cf9834..5c710597a 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -60,7 +60,7 @@ "fzf": "^0.5.2", "glob": "^10.5.0", "highlight.js": "^11.11.1", - "ink": "^6.2.3", + "ink": "^7.0.3", "ink-gradient": "^3.0.0", "ink-link": "^4.1.0", "ink-spinner": "^5.0.0", @@ -68,7 +68,7 @@ "open": "^10.1.2", "p-limit": "^7.3.0", "prompts": "^2.4.2", - "react": "^19.1.0", + "react": "^19.2.4", "read-package-up": "^11.0.0", "shell-quote": "^1.8.3", "simple-git": "^3.28.0", diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index e818d03ed..ca2c44979 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -2895,6 +2895,188 @@ describe('AppContainer State Management', () => { expect(mockCloseModelDialog).toHaveBeenCalled(); }); }); + + // Coverage for the AppContainer onModelChange wiring. The Static header + // (key = `${historyRemountKey}-${currentModel}`) and MainContent's + // progressive-replay reset (keyed on historyRemountKey) both depend on + // these two state updates landing in the same commit on a real model + // change — see the comment in AppContainer.tsx around the + // config.onModelChange subscription and PR #4119 review discussion. + describe('Model change refreshStatic wiring', () => { + function captureModelChangeListener(config: Config) { + // Track every subscribe/unsubscribe pair. The CLI test harness + // tears down ink's renderer after the initial render flush, which + // runs the effect's cleanup synchronously — but the captured + // callback closure is still callable (and AppContainer's setState + // still updates state because React's update queue is independent + // of the listener registration). We therefore fire on the LAST + // captured callback, regardless of whether ink considers the + // effect mounted, and assert on the number of subscribe/cleanup + // calls separately for unsubscribe coverage. + const subs: Array<{ + cb: (model: string) => void; + active: boolean; + }> = []; + const fakeOnModelChange = vi.fn((cb: (model: string) => void) => { + const entry = { cb, active: true }; + subs.push(entry); + return () => { + entry.active = false; + }; + }); + ( + config as unknown as { onModelChange: typeof fakeOnModelChange } + ).onModelChange = fakeOnModelChange; + return { + spy: fakeOnModelChange, + notify: (model: string) => { + if (subs.length === 0) { + throw new Error('AppContainer never subscribed to onModelChange'); + } + // Always fire on the most-recent captured callback. + subs[subs.length - 1].cb(model); + }, + subscribeCount: () => subs.length, + activeCount: () => subs.filter((s) => s.active).length, + }; + } + + // Effects run after the synchronous render returns. Flushing two + // microtasks lines up the same pattern used by other async tests in + // this file (search "Let the userMessages-fetching effect resolve"). + const flushEffects = async () => { + await Promise.resolve(); + await Promise.resolve(); + }; + + it('fires refreshStatic in the same handler that updates currentModel', async () => { + // Wenshao's PR #4119 [Critical]: if refreshStatic (which bumps + // historyRemountKey) and setCurrentModel were split into two + // separate effects, the first commit would show the new + // currentModel against the OLD historyRemountKey — MainContent's + // would + // remount BEFORE the progressive-replay reset, dumping the full + // history in one frame. + // + // The fix moves refreshStatic into the event handler itself so + // both side effects (clearTerminal + setHistoryRemountKey via + // refreshStatic, plus setCurrentModel) run inside the same + // synchronous JS task — React 18+ batches all setState calls in + // an event-handler-style task into one commit. We verify this + // synchronously by inspecting mockStdout.write the moment the + // listener returns: clearTerminal must already be written, proving + // refreshStatic runs in-handler rather than queued for a later + // useEffect tick. (We cannot observe the post-commit React state + // through capturedUIState here because ink-testing-library tears + // down the renderer once render() returns, so setState calls + // queued from the listener never produce a follow-up commit. The + // synchronous side-effect ordering is the part that matters for + // the bug wenshao flagged.) + vi.spyOn(mockConfig, 'getModel').mockReturnValue('model-a'); + const trigger = captureModelChangeListener(mockConfig); + + render( + , + ); + await flushEffects(); + mockStdout.write.mockClear(); + + // Synchronous notification → refreshStatic must run BEFORE the + // notify() call returns (i.e., before any React batch tick). + trigger.notify('model-b'); + + expect(mockStdout.write).toHaveBeenCalledWith(ansiEscapes.clearTerminal); + }); + + it('skips refreshStatic when the notified model matches the current one', async () => { + vi.spyOn(mockConfig, 'getModel').mockReturnValue('model-a'); + const trigger = captureModelChangeListener(mockConfig); + + render( + , + ); + await flushEffects(); + + const baselineRemountKey = capturedUIState.historyRemountKey; + mockStdout.write.mockClear(); + + trigger.notify('model-a'); + await flushEffects(); + + expect(mockStdout.write).not.toHaveBeenCalledWith( + ansiEscapes.clearTerminal, + ); + expect(capturedUIState.historyRemountKey).toBe(baselineRemountKey); + expect(capturedUIState.currentModel).toBe('model-a'); + }); + + it('fires refreshStatic only once per real model change (StrictMode-safe)', async () => { + // StrictMode double-invokes state updater functions in dev. The + // refreshStatic side-effect therefore must NOT live inside a + // setState updater — it lives in the event handler, with a ref + // guard to de-dupe redundant notifications. We simulate the + // StrictMode-style re-fire by calling the listener twice with the + // same value (e.g. if a deduplicator upstream missed it). + vi.spyOn(mockConfig, 'getModel').mockReturnValue('model-a'); + const trigger = captureModelChangeListener(mockConfig); + + render( + , + ); + await flushEffects(); + mockStdout.write.mockClear(); + + trigger.notify('model-b'); + trigger.notify('model-b'); + await flushEffects(); + + const clearWrites = mockStdout.write.mock.calls.filter( + ([arg]) => arg === ansiEscapes.clearTerminal, + ); + expect(clearWrites).toHaveLength(1); + }); + + it('returns an unsubscribe function that AppContainer wires up', async () => { + // AppContainer's effect returns the unsubscribe so React can call it + // on unmount or when deps change. We verify both halves of the + // subscribe/cleanup contract were exercised — every subscribe must + // have paired with a cleanup invocation by the time the renderer + // tears down. + vi.spyOn(mockConfig, 'getModel').mockReturnValue('model-a'); + const trigger = captureModelChangeListener(mockConfig); + + const { unmount } = render( + , + ); + await flushEffects(); + expect(trigger.subscribeCount()).toBeGreaterThanOrEqual(1); + + unmount(); + await flushEffects(); + + expect(trigger.activeCount()).toBe(0); + }); + }); }); describe('dedupeNewestFirst', () => { diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index e43d8eaa5..3f81cac35 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -738,17 +738,36 @@ export const AppContainer = (props: AppContainerProps) => { // Keep the static header in sync with model changes without polling. // Ink's output is append-only, so model changes must explicitly // clear and remount the static region to redraw the banner at the top. + // + // Two requirements pull in opposite directions: + // (a) refreshStatic() must NOT be called from inside a setState updater, + // because React.StrictMode double-invokes updaters in dev and we'd + // fire two clearTerminal writes per model swap. + // (b) setHistoryRemountKey (inside refreshStatic) and setCurrentModel + // MUST land in the SAME commit. MainContent's key is + // `${historyRemountKey}-${currentModel}` and its render-phase + // progressive-replay reset (lastRemountKey !== historyRemountKey) + // only fires when historyRemountKey changes. If currentModel + // changes first in its own render, Static remounts with the OLD + // remount key and the unreset (full-length) replayCount — i.e. + // a full-history Static render that bypasses progressive replay + // (the issue #3899 freeze regression). See PR #4119 review. + // + // Fix: side-effect lives in the event handler (NOT the updater); a ref + // guard de-dupes same-model notifications. React batches the + // setHistoryRemountKey (via refreshStatic) and setCurrentModel calls in + // this event handler into a single commit, so the render-phase reset + // and the Static remount happen together — no full-history flash. + const lastNotifiedModelRef = useRef(currentModel); useEffect(() => { const unsubscribe = config.onModelChange((model) => { - setCurrentModel((prev) => { - if (prev === model) { - return prev; - } - refreshStatic(); - return model; - }); + if (lastNotifiedModelRef.current === model) { + return; + } + lastNotifiedModelRef.current = model; + refreshStatic(); + setCurrentModel(model); }); - return unsubscribe; }, [config, refreshStatic]); diff --git a/packages/cli/src/ui/components/MainContent.test.tsx b/packages/cli/src/ui/components/MainContent.test.tsx index cfe1acd11..8e4495e07 100644 --- a/packages/cli/src/ui/components/MainContent.test.tsx +++ b/packages/cli/src/ui/components/MainContent.test.tsx @@ -456,4 +456,75 @@ describe('', () => { expect(staticItemsSpy.mock.calls.at(-1)?.[0]).toHaveLength(53); }); + + it('does NOT reset progressive replay when only currentModel changes (PR #4119 regression guard)', async () => { + // Wenshao's review on PR #4119: if AppContainer splits the model-change + // wiring into two separate effects (setCurrentModel first, refreshStatic + // -> historyRemountKey bump second), there is a render where currentModel + // is new but historyRemountKey is still the old value. 's key is + // `${historyRemountKey}-${currentModel}`, so the key changes (Ink remounts + // Static), but the render-phase reset (lastRemountKey !== historyRemountKey) + // does NOT fire — so the new is mounted with the full pre-catch-up + // replayCount, and Ink does the synchronous full-history layout the PR is + // meant to avoid. + // + // This test reproduces only the dangerous half of that interleaving: + // currentModel flips while historyRemountKey is held constant. Under the + // correct (single-batch) AppContainer wiring this combination never + // appears in practice, but the test pins the MainContent invariant — + // currentModel alone must not trigger progressive-replay reset, which + // makes any future "two-effect" regression visible here as a freeze. + staticItemsSpy.mockClear(); + const history = Array.from({ length: 200 }, (_, i) => ({ + type: 'user' as const, + id: i, + text: `msg ${i}`, + })); + const TOTAL = 203; + + const { rerender } = renderMainContent( + createUIState({ + history, + historyRemountKey: 1, + currentModel: 'model-a', + }), + ); + + // Drive the chunked replay to completion (replayCount === TOTAL). + for (let i = 0; i < 50; i++) { + const len = staticItemsSpy.mock.calls.at(-1)?.[0].length ?? 0; + if (len === TOTAL) break; + await new Promise((resolve) => setImmediate(resolve)); + } + expect(staticItemsSpy.mock.calls.at(-1)?.[0]).toHaveLength(TOTAL); + + // Re-render with a NEW currentModel but the SAME historyRemountKey. + // 's key will change (Ink remounts), but replayCount must stay + // at TOTAL — i.e. progressive replay must NOT re-trigger. Any future + // refactor that re-introduces a one-render gap between setCurrentModel + // and the historyRemountKey bump will trip this assertion the moment + // someone correctly drives the reset off the model dimension instead. + rerender( + + + + + + + + + + + , + ); + + // No reset means the LAST staticItemsSpy call still received TOTAL. + expect(staticItemsSpy.mock.calls.at(-1)?.[0]).toHaveLength(TOTAL); + }); });