fix(cli): default to virtualized terminal history#5738
Conversation
|
E2E / TUI verification report for this PR:
Additional local checks: Tested locally on macOS. Windows/Linux terminal combinations were not manually tested. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (windows-latest, Node 22.x).
CI note: the failing Windows job is in packages/core's gitWorktreeService.symlinks.integ.test.ts (git init -q -b main), outside this PR's changed files.
— GPT-5 Codex via Qwen Code /review
Verification Report — default to virtualized terminal historyBuilt and exercised the real CLI from the PR head, with a focus on the one thing a default-flip can silently get wrong: leaving a runtime gate behind. Result: correct, complete, and well-tested. The only open item is a product/UX judgement call (alt‑screen now on by default), not a defect. Environment: PR head Scope checkThe PR is a focused 10‑file, single‑commit change that flips 1. Gate‑completeness audit ✅ (avoids the schema‑only trap)A schema This PR does not fall into that trap. There are exactly two runtime reads of
2. Real tmux E2E (A/B) ✅ — the headlineTwo server‑side probes were used because they independently exercise the two different gates:
The third row is a dist‑level swap ( 3. UX consequence reviewers should weigh
|
| Gate | 控制 | 状态 |
|---|---|---|
gemini.tsx:367(useVP) |
render(..., { alternateScreen: useVP }) |
?? false → ?? true ✅ |
AppContainer.tsx:957(useTerminalBuffer) |
VP 渲染路径 + refreshStatic;经 uiState.useTerminalBuffer 传给 MainContent/ScrollableList |
?? false → ?? true ✅ |
MainContent.tsx 通过 uiState 继承该值(没有独立读取)。settingsSchema.ts 和 vscode‑ide 的 JSON 默认值也一并翻转,所以设置对话框与运行时保持一致。serve/routes/workspace-settings.ts 那处只是一个 TUI‑only 分类集合(default 字段自动从 schema 派生)。ACP / serve / 非交互路径都不读这个设置 —— 改动仅限 TUI。由于用的是 ??,未设置的 key 会正确落到新默认值,与 merge 是否 seed 无关。
2. 真实 tmux E2E(A/B)✅ —— 核心证据
用了两个 server 端探针,因为它们分别独立地命中两个不同的 gate:#{alternate_on} ← gemini.tsx,#{mouse_any_flag} ← AppContainer → ScrollableList。同一个构建,两个隔离的 $HOME,两个 arm 都干净地到达聊天输入框(无 auth 对话框)。
| 构建 / 配置 | #{alternate_on} |
#{mouse_any_flag} |
含义 |
|---|---|---|---|
| PR + 设置未设(新默认) | 1 | 1 | 默认开启 VP |
PR + ui.useTerminalBuffer: false(opt‑out) |
0 | 0 | 旧路径保留 |
| base(在 dist 里还原 PR 改动)+ 未设 | 0 | 0 | 证明这 2 行改动是唯一原因 |
第三行是 dist 级替换(把两行编译产物 ?? true → ?? false,$HOME 同样未设),从而把 PR 改动隔离成唯一变量。两个 gate 都验证通过,逃生开关也验证通过。
3. Reviewer 需要权衡的 UX 影响 ⚠️ (非 bug)
新默认会进入 alternate screen。已具体演示:在启动前打印的 host marker,在 qwen 运行期间被隐藏(alternate_on=1),/quit 之后被还原,且 qwen 界面消失:
运行期间: alternate_on=1 | qwen banner 可见 | host marker 被隐藏
/quit 之后: alternate_on=0 | qwen banner 消失 | host marker 还原
也就是说退出时整个 qwen 会话会从宿主终端 scrollback 中抹掉(标准的 vim/less 式 alt‑screen 行为)。这是为消除闪屏而做的取舍,文档里的 opt‑out(ui.useTerminalBuffer: false)有效 —— 但对那些退出后还要在终端往回滚、或依赖原生 click‑drag 选择文本的用户来说是个实质变化。建议产品侧明确签字确认。
4. 单元测试 ✅
- PR 改动的 3 个文件:147 / 147 通过(与 PR 描述一致)。
- 对 6 个受 default 翻转影响的测试套件做反向审计扫描(
MainContent、App、DefaultAppLayout、useResizeSettleRepaint、terminalRedrawOptimizer、InputPrompt):221 / 221 通过。PR 正确地在共享的AppContainerfixture 里钉死useTerminalBuffer: false,使依赖旧路径的测试(如 Terminal resize during streaming leaves fragmented content at wrong widths in scrollback #4891 的 resize‑clearTerminal 契约)继续有效。未发现连带破坏。
5. CI 状态
Test (ubuntu) 和 Test (macos) 通过。Test (windows) 失败,但失败项是 gitWorktreeService.symlinks.integ.test.ts(15 中 1 个)外加 exit code 127(shell command‑not‑found)—— 这是 Windows symlink/工具链问题,而该文件本 PR 完全没动(merge‑base 与 PR head 逐字节相同)。不是本 PR 引入的回归。
结论
从正确性角度,本 PR 可以合并:gate 审计干净,两个 runtime fallback 与显示默认值一致翻转,Linux/macOS 测试全绿,真实终端 A/B 同时验证了新默认与 opt‑out。剩下的只是"是否默认进入 alternate screen"的产品决定(见 §3)。Windows CI 的红是既有/无关问题。
Verified by building the PR head and running it under tmux on Linux/Node 22. Probes: #{alternate_on} (gemini.tsx gate) and #{mouse_any_flag} (AppContainer→ScrollableList gate), plus a dist‑level base A/B isolating the PR delta.
|
Follow-up on the remaining UX point: Defaulting The compatibility path is preserved: users who prefer native host scrollback can set I do not plan further code changes for this unless maintainers prefer a more conservative rollout strategy than default-on. CI note: the current Windows red is in |
hi, thank you for the PR. But I think it's not time to change the default setting of
If you glad with this, you can help to test and welcome pr with this. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Suggestion] docs/design/virtual-viewport/README.md:326 — The paragraph after the PR-sequence table still reads "V.3 (integration tests) is the remaining critical-path item before flipping the default." However, V.3 remains "pending" in the table while this PR flips the default. Future readers will conclude the default was flipped prematurely or that V.3 was silently dropped. Suggest updating to reflect the actual decision, e.g.: "V.3 (integration tests) remains desirable for long-session regression coverage but is no longer a gating prerequisite for the default flip."
— qwen3.7-max via Qwen Code /review
| // re-reading `mergedHistory` / `allVirtualItems` on whatever state | ||
| // change triggered refreshStatic (Ctrl+O, model change, etc.). | ||
| const useTerminalBuffer = settings.merged.ui?.useTerminalBuffer ?? false; | ||
| const useTerminalBuffer = settings.merged.ui?.useTerminalBuffer ?? true; |
There was a problem hiding this comment.
[Suggestion] Accessibility regression for screen-reader users. Flipping the default to true silently routes screen-reader users into the virtualized-viewport path, which drops the append-only <Static> rendering that screen-reader mode is built around.
Chain: with ui.useTerminalBuffer unset, this is now true → uiState.useTerminalBuffer is true → MainContent takes the if (useVirtualScroll) branch (MainContent.tsx:596) and renders <ScrollableList> with no <Static> (the legacy <Static> branch is at :626). Screen-reader users reach this same MainContent — App.tsx routes them to ScreenReaderAppLayout, which itself renders <MainContent />. Ink's screen-reader render path emits committed history to the terminal only via staticOutput (a <Static> node); with no <Static>, history is no longer presented append-only. Separately, gemini.tsx:367 now passes alternateScreen: true for screen-reader + TTY (Ink's resolveAlternateScreenOption doesn't exclude screen-reader mode), entering the alternate screen that assistive tech / native scrollback rely on.
Before this PR (default false) screen-reader users got the <Static> path. Suggest gating the default on screen-reader mode here (and mirror it at gemini.tsx:367 for alternateScreen):
| const useTerminalBuffer = settings.merged.ui?.useTerminalBuffer ?? true; | |
| const useTerminalBuffer = | |
| (settings.merged.ui?.useTerminalBuffer ?? true) && !config.getScreenReader(); |
No test currently covers the screen-reader + unset-useTerminalBuffer combination.
— claude-opus-4-8[1m] via Qwen Code /qreview
There was a problem hiding this comment.
Fixed in 90ea976.
Screen-reader mode now stays out of the VP and alternate-screen path in both runtime gates: AppContainer keeps uiState.useTerminalBuffer false and startInteractiveUI passes alternateScreen false when config.getScreenReader() is true. Added regression coverage for screen reader mode with unset useTerminalBuffer in AppContainer and gemini.
ZevGit
force-pushed
the
fix/cli-scroll-flicker
branch
from
ee5d0ea to
90ea976
Compare
|
Updated PR head to 90ea976 after #5751 landed. Handled review items:
Verification:
Local build note:
|
Build failure root cause — it's a
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] packages/cli/index.ts (not in diff) — The uncaughtException handler calls process.exit(1) without runExitCleanup(), so instance.unmount() (which writes exitAlternateScreen) never runs. Pre-existing bug, but now that VP is the default and all users enter alt-screen, any crash leaves the terminal stuck in alt-screen with no visible prompt (user must run reset or close the tab).
Fix: add a process.on('exit') handler that unconditionally writes \x1b[?1049l as a safety net, or call runExitCleanup() in the uncaughtException handler.
CI note: 3 Test jobs failing (ubuntu, macos, windows) — pre-existing, not caused by this PR.
— qwen3.7-max via Qwen Code /review
|
|
||
| const useVP = settings.merged.ui?.useTerminalBuffer ?? false; | ||
| const useVP = | ||
| (settings.merged.ui?.useTerminalBuffer ?? true) && |
There was a problem hiding this comment.
[Suggestion] TOCTOU between one-shot alternateScreen and per-render useTerminalBuffer: useVP is computed once here and passed to ink's render(), locking the terminal into alt-screen for the entire session. But AppContainer.tsx:958 recomputes useTerminalBuffer on every render from live settings.merged. If a user toggles useTerminalBuffer off mid-session (the schema declares requiresRestart: false), the React tree switches to the legacy <Static> path while ink stays in alt-screen — history is silently written to the alt-buffer and lost on exit.
Pre-existing, but the default flip makes this the common path. Consider either locking useTerminalBuffer at startup (matching alternateScreen's lifetime), or setting requiresRestart: true so the settings dialog prompts a restart.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 8f5eb98.
The VP decision is now startup-scoped in AppContainer, matching Ink alternateScreen lifetime. ui.useTerminalBuffer is also marked requiresRestart, so SettingsDialog no longer presents this as a live-safe toggle.
| // change triggered refreshStatic (Ctrl+O, model change, etc.). | ||
| const useTerminalBuffer = settings.merged.ui?.useTerminalBuffer ?? false; | ||
| const useTerminalBuffer = | ||
| (settings.merged.ui?.useTerminalBuffer ?? true) && |
There was a problem hiding this comment.
[Suggestion] The VP-mode guard (settings.merged.ui?.useTerminalBuffer ?? true) && !config.getScreenReader() is duplicated verbatim in gemini.tsx:368. These two sites control different but tightly coupled decisions (ink's alternateScreen vs React rendering path) and must always agree.
Consider extracting a shared helper:
// e.g., packages/cli/src/ui/utils/terminalBuffer.ts
export function shouldUseVirtualViewport(
settings: LoadedSettings,
config: Config,
): boolean {
return (settings.merged.ui?.useTerminalBuffer ?? true) && !config.getScreenReader();
}— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 8f5eb98.
Both runtime gates now use a shared shouldUseVirtualViewport helper, so the alternate-screen decision and React VP path stay coupled.
| expect(options).toMatchObject({ alternateScreen: false }); | ||
| }); | ||
|
|
||
| it('should not use alternate screen in screen reader mode when VP mode is unset', async () => { |
There was a problem hiding this comment.
[Suggestion] Test gap: the screen-reader guard is tested when useTerminalBuffer is unset (defaults to true), but not when it's explicitly true. Add a test with useTerminalBuffer: true + getScreenReader(): true to cover the true && !true branch:
it('should not use alternate screen in screen reader mode even when VP mode is explicitly enabled', async () => {
// useTerminalBuffer: true, getScreenReader: () => true
// expect alternateScreen: false
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Covered in 8f5eb98.
Added the explicit useTerminalBuffer: true + screen-reader case for startInteractiveUI, asserting alternateScreen remains false.
ZevGit
force-pushed
the
fix/cli-scroll-flicker
branch
from
90ea976 to
8f5eb98
Compare
|
Updated PR head to 8f5eb98 after reviewing the latest comments and CI failure. What changed:
Verification:
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No review findings. LGTM! ✅
All prior suggestions (duplicated VP formula, missing screen-reader test coverage) have been addressed — shared shouldUseVirtualViewport() helper, session-locked VP mode via useState, alternate-screen exit handler, and requiresRestart: true are clean improvements.
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
Turns virtualized history on by default for interactive CLI sessions. New users and existing users without an explicit setting now get the in-app scrollable history viewport, while users who prefer host terminal scrollback can still opt out by setting
ui.useTerminalBuffertofalse.The settings schema, generated IDE schema, tests, and user/design docs are updated so the runtime behavior and documented behavior match.
Why it's needed
The legacy terminal scrollback rendering path can visibly flash or make the host terminal scrollbar jump during long sessions, refreshes, resize settle repaint, compact-mode toggles, and similar TUI updates. The virtualized history path already avoids the physical terminal clear/replay behavior that causes this class of flicker, but it was still opt-in, so most users continued to hit the old path unless they discovered the setting.
Defaulting to the virtualized path makes the stable behavior the common path, reduces repeated flicker reports, and keeps a small compatibility escape hatch for terminals or workflows that still prefer native scrollback.
Reviewer Test Plan
How to verify
Start the interactive CLI with no
ui.useTerminalBuffersetting and confirm it opens in the virtualized history flow. Scrolling history should use Shift+Up/Down, PgUp/PgDn, Ctrl+Home/End, or the mouse wheel instead of native terminal scrollback.Start the interactive CLI with
ui.useTerminalBuffer: falseand confirm the legacy host scrollback path is still available.Run the focused unit tests and type/build checks listed below.
Evidence (Before & After)
Before: with no explicit setting, the CLI defaulted to the legacy terminal scrollback path, so refreshes could clear/replay terminal history and produce visible scrollbar jumps in long sessions.
After: a local PTY smoke test launched the built CLI twice. With the setting unset, the captured ANSI stream entered the alternate screen and reached the input prompt. With
ui.useTerminalBuffer: false, the captured ANSI stream did not enter the alternate screen and still reached the input prompt.Verification commands run locally:
Tested on
Environment (optional)
Local source checkout, built CLI, sandbox disabled for the PTY smoke. The smoke did not send a model request; it only verified startup rendering and the terminal mode selected by default versus explicit opt-out.
Risk & Scope
ui.useTerminalBuffertofalseto keep the legacy host scrollback behavior.Linked Issues
N/A
中文说明
What this PR does
这个 PR 将交互式 CLI 的虚拟化历史视图默认开启。新用户和没有显式配置该选项的现有用户会默认使用应用内可滚动历史视口；如果用户更偏好宿主 terminal scrollback，仍然可以通过设置
ui.useTerminalBuffer为false选择旧路径。配置 schema、生成的 IDE schema、测试以及用户/设计文档都同步更新，保证运行时行为和文档描述一致。
Why it's needed
旧的 terminal scrollback 渲染路径在长会话、刷新、resize settle repaint、compact mode 切换等 TUI 更新场景下，可能出现明显闪屏或宿主 terminal 滚动条跳动。虚拟化历史路径已经避免了导致这类 flicker 的物理清屏和历史重放行为，但之前仍是 opt-in，大多数用户如果没有发现这个设置，依然会走旧路径。
把虚拟化路径设为默认，可以让更稳定的行为成为常规路径，减少重复的闪屏反馈，同时保留一个很小的兼容逃生口，给仍然偏好原生 scrollback 的终端或工作流使用。
Reviewer Test Plan
How to verify
在没有设置
ui.useTerminalBuffer的情况下启动交互式 CLI，确认它进入虚拟化历史流程。历史滚动应使用 Shift+Up/Down、PgUp/PgDn、Ctrl+Home/End 或鼠标滚轮，而不是依赖原生 terminal scrollback。在
ui.useTerminalBuffer: false的情况下启动交互式 CLI，确认旧的宿主 scrollback 路径仍然可用。运行上面列出的聚焦单测、typecheck 和 build 检查。
Evidence (Before & After)
Before：没有显式配置时，CLI 默认走旧的 terminal scrollback 路径，因此刷新时可能清屏并重放 terminal 历史，在长会话中表现为可见滚动条跳动。
After：本地 PTY smoke test 启动了两次构建后的 CLI。配置未设置时，捕获到的 ANSI 流进入了 alternate screen，并正常到达输入提示。显式设置
ui.useTerminalBuffer: false时，捕获到的 ANSI 流没有进入 alternate screen，也正常到达输入提示。本地执行的验证命令：
Tested on
Environment (optional)
本地源码 checkout，使用构建后的 CLI，PTY smoke 中关闭 sandbox。这个 smoke 没有发送模型请求，只验证启动渲染以及默认配置和显式 opt-out 分别选择的 terminal mode。
Risk & Scope
ui.useTerminalBuffer为false可以保留旧的宿主 scrollback 行为。Linked Issues
N/A