Skip to content

chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed) #4119

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.

Sign up for GitHub

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

Merged
chiga0 merged 4 commits into from
May 15, 2026
Merged

chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed) #4119

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5bb49d9
chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix la…
qinqi-ali May 13, 2026
b25831b
fix(deps,cli): add @types/react overrides + move refreshStatic out of…
qinqi-ali May 14, 2026
e631f48
fix(cli): collapse model-change effect back into one batched handler
qinqi-ali May 14, 2026
1266adc
Merge remote-tracking branch 'origin/main' into chore/re-upgrade-ink-…
qinqi-ali May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 77 additions & 28 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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": {
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,15 @@
"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",
"lowlight": "^3.3.0",
"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",
Expand Down
24 changes: 15 additions & 9 deletions packages/cli/src/ui/AppContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,19 +591,25 @@ export const AppContainer = (props: AppContainerProps) => {
// Keep the static header in sync with model changes without polling.
// Ink's <Static> output is append-only, so model changes must explicitly
// clear and remount the static region to redraw the banner at the top.
//
// refreshStatic() is fired from an effect (not inside the setState updater)
// so React.StrictMode's double-invoke of state updaters doesn't translate
// into two clearTerminal writes per model change. Side-effects belong in
// effects; the updater stays pure.
useEffect(() => {

@wenshao wenshao May 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The entire model-change-to-refreshStatic path has no test coverage. AppContainer.test.tsx exercises refreshStatic via manually exposed capturedUIActions.refreshStatic(), which bypasses the new two-effect architecture entirely.

Missing coverage:

  • Model change triggers refreshStatic (via clearTerminal + historyRemountKey bump)
  • Same-model change correctly skips refreshStatic
  • StrictMode double-invoke does not cause double clearTerminal
  • Unsubscribe runs on unmount, preventing stale listener calls

Consider adding tests that spy on the config.onModelChange callback and verify the end-to-end path from model change notification to terminal output.

— DeepSeek/deepseek-v4-pro via Qwen Code /review

@chiga0 chiga0 May 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in e631f48 by adding four focused tests in AppContainer.test.tsx under the new describe('Model change refreshStatic wiring', ...) block, plus one in MainContent.test.tsx:

AppContainer.test.tsx — the harness captures config.onModelChange listeners via a direct method override (captureModelChangeListener), letting each test fire the listener and observe the synchronous side effect:

  1. "fires refreshStatic in the same handler that updates currentModel" — covers your first bullet. Asserts mockStdout.write was called with ansiEscapes.clearTerminal SYNCHRONOUSLY (no flush/await) after trigger.notify('model-b'). Under the broken two-effect refactor this fails: the clearTerminal write is queued behind a React commit + useEffect tick. Under the corrected code it passes because refreshStatic() runs in-handler.
  2. "skips refreshStatic when the notified model matches the current one" — covers your second bullet (same-model no-op). The ref guard short-circuits the listener; no clearTerminal is written and historyRemountKey is untouched.
  3. "fires refreshStatic only once per real model change (StrictMode-safe)" — covers your third bullet. Fires the listener twice with the same target model; asserts exactly ONE clearTerminal write. This is the StrictMode-double-invoke property that motivated b25831b in the first place; the new code preserves it by virtue of the side effect living in the event handler rather than a setState updater, and the ref guard de-duping the second call.
  4. "returns an unsubscribe function that AppContainer wires up" — covers your fourth bullet. Asserts the subscribe count is ≥1 after render and the active subscription count drops to 0 after unmount().

MainContent.test.tsx — added "does NOT reset progressive replay when only currentModel changes (PR #4119 regression guard)" which directly pins the MainContent invariant: when currentModel changes but historyRemountKey is held constant, replayCount must NOT reset. This is the post-condition that the broken two-effect AppContainer code accidentally violated. Any future "split into two effects" refactor will trip this assertion the moment someone correctly drives the reset off the model dimension instead.

The new AppContainer tests are intentionally end-to-end on the config.onModelChange notification path (your original ask) rather than going through capturedUIActions.refreshStatic() — so we're now exercising the same code path a real model swap (e.g. VLM auto-switch, fallback, /model command) would hit.

Verified: 82/82 tests green in AppContainer.test.tsx + MainContent.test.tsx.

const unsubscribe = config.onModelChange((model) => {
setCurrentModel((prev) => {
if (prev === model) {
return prev;
}
refreshStatic();
return model;
});
setCurrentModel(model);
});

return unsubscribe;
}, [config, refreshStatic]);
}, [config]);

const prevCurrentModelRef = useRef(currentModel);
useEffect(() => {
if (prevCurrentModelRef.current !== currentModel) {

@wenshao wenshao May 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Splitting the model-change handler into two effects causes progressive replay to be bypassed during model switches.

Root cause: setCurrentModel(model) fires first (new effect 1), triggering a render with the new currentModel but the old historyRemountKey. Since the <Static> key at MainContent.tsx:356 is `${historyRemountKey}-${currentModel}`, the key changes immediately, forcing Ink to remount <Static>. However, MainContent's progressive replay reset (lastRemountKey !== historyRemountKey, line 315) only watches historyRemountKey — which hasn't changed yet. So replayCount stays at full mergedHistory.length, and <Static> renders ALL history items in one shot, bypassing the chunked replay introduced for issue #3899. The second effect (line 608) bumps historyRemountKey too late — after the freeze-causing render already happened.

Impact: On sessions with >100 history items, every model switch triggers a full unframed <Static> render, causing a noticeable UI freeze. This is precisely the regression progressive replay was designed to prevent.

Suggested change
if (prevCurrentModelRef.current !== currentModel) {
// Bump historyRemountKey synchronously with the model change so that
// progressive replay's lastRemountKey check resets in the same render
// batch — avoiding a full-history Static render before the clear.
useEffect(() => {
const unsubscribe = config.onModelChange((model) => {
setHistoryRemountKey((prev) => prev + 1);
setCurrentModel(model);
});
return unsubscribe;
}, [config]);
// The separate model-change-detection effect (prevCurrentModelRef +
// refreshStatic) can then be removed entirely.

— DeepSeek/deepseek-v4-pro via Qwen Code /review

@chiga0 chiga0 May 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: correct, real bug. Verified by tracing the four-step interleaving you described against the actual code:

  • AppContainer.tsx:600-603 (old): setCurrentModel(model) commits → React renders with new currentModel, old historyRemountKey.
  • MainContent.tsx:356: <Static key={${historyRemountKey}-${uiState.currentModel}}> — the key changes (currentModel did), Ink remounts Static immediately.
  • MainContent.tsx:314-318: render-phase reset if (lastRemountKey !== historyRemountKey) — false (historyRemountKey hadn't changed yet), so replayCount stays at the prior catch-up value (e.g. full mergedHistory.length after the previous Static catch-up).
  • The remounted Static synchronously lays out the full history. The second effect's refreshStatic() arrives one render too late.

So splitting the handler into two effects DID reintroduce the #3899 freeze on every model switch. My refactor was strictly worse: the StrictMode double-invoke concern was real but smaller than this regression.

Fix in e631f48: kept the side-effect-out-of-updater principle but moved both refreshStatic() and setCurrentModel() back into the same event-handler body, with a useRef guard for same-model notifications:

const lastNotifiedModelRef = useRef(currentModel);
useEffect(() => {
  const unsubscribe = config.onModelChange((model) => {
    if (lastNotifiedModelRef.current === model) return;
    lastNotifiedModelRef.current = model;
    refreshStatic();
    setCurrentModel(model);
  });
  return unsubscribe;
}, [config, refreshStatic]);

This is the "third approach":

  • Side effect lives in the event handler (not in a setState updater) → StrictMode's double-invoke of state updater functions doesn't multiply the clearTerminal write.
  • Both setHistoryRemountKey (via refreshStatic) and setCurrentModel are called in the same synchronous JS task → React 18+ batches them into one commit → MainContent sees both the new currentModel AND the new historyRemountKey on the first render, the render-phase reset fires, replayCount drops to the first chunk, and Static remounts with chunked replay intact.
  • Ref guard short-circuits redundant same-model fires (e.g. if upstream deduping misses).

Your suggestion of setHistoryRemountKey + setCurrentModel in the handler is structurally equivalent for the batching invariant; I additionally kept refreshStatic() so the clearTerminal write (needed because Ink's <Static> is append-only and the banner needs to redraw at the top) stays alongside the remount-key bump. Same single commit either way.

Regression-pinned by:

  • AppContainer.test.tsx "fires refreshStatic in the same handler that updates currentModel" — mockStdout.write.mockClear(); trigger.notify('model-b'); expect(...clearTerminal) synchronously. Under the broken two-effect code this would fail because clearTerminal is queued behind a React commit + useEffect tick.
  • MainContent.test.tsx "does NOT reset progressive replay when only currentModel changes (PR chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed) #4119 regression guard)" — directly pins the MainContent invariant the two-effect refactor accidentally relied on.

Verified: npx vitest run AppContainer.test.tsx MainContent.test.tsx → 82/82 green. Typecheck clean.

Thanks for the careful read — this is a much better resting state than my previous refactor.

prevCurrentModelRef.current = currentModel;
refreshStatic();
}
}, [currentModel, refreshStatic]);

const {
isThemeDialogOpen,
Expand Down
Loading