fix(tui): skip cross-group tool merge in <Static> mode to eliminate screen flash#4795
Conversation
|
Hey @zzhenyao — noting this is still a draft, so sharing gate findings early for discussion. Template mostly solid ✓ — "Tested on" OS table and "Risk & Scope" section from the template are missing; easy to fill in before marking ready. On direction: This solves a real, multi-reported pain point — compact mode screen flash has been filed at least three times (#4794, #4561, #3979). Clearly within scope. No direct Claude Code CHANGELOG hit for "compact mode flicker" specifically, but compaction-related rendering fixes are common territory. The fix addresses the root cause cleanly: Static can't handle key-count changes, so don't give it key-count changes. On approach: The core fix (skip merge in Static mode + remove the dead Have you considered deferring Flagging this before diving into code review. The fundamental approach is sound — just want to see if we can ship the fix without the future flag. 🎯 中文说明嘿 @zzhenyao — 注意到这还是个 draft，先分享 Stage 1 的发现供讨论。 模板基本完整 ✓ — 缺少 模板 中的 "Tested on" 操作系统表格和 "Risk & Scope" 部分，标记 ready 前补上即可。 方向： 解决了一个真实且多次报告的问题 — 紧凑模式闪屏至少在 #4794、#4561、#3979 中被提过。完全在项目范围内。Claude Code 的 CHANGELOG 没有直接关于"紧凑模式闪烁"的条目，但压缩相关的渲染修复是常见领域。修复方式直击根因：Static 无法处理 key 数量变化，那就别给它 key 数量变化。 方案： 核心修复（Static 模式下跳过合并 + 移除失效的 refreshStatic useEffect）正确且聚焦。但 有没有考虑把 在深入代码审查前先提出这个。根本方案是对的 — 只是想看看能不能不挂未来标记就发布修复。🎯 — Qwen Code · qwen3.7-max |
Good point on simplicity. The reason I kept
|
|
@wenshao Hi, would you have time to take a look at this PR? Reproduction steps for the flash: From the
You will see the screen clear and re-render every time a batch of tool calls finishes. The flash gets worse with longer history because refreshStatic clears and remounts the entire The root cause is that |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: N. Taylor Mullen <ntaylormullen@google.com>
wenshao
left a comment
There was a problem hiding this comment.
Two additional observations that could not be anchored to diff lines:
Stale comments referencing removed refreshStatic heuristic. The comment at MainContent.tsx:172-173 ("Summaries for absorbed call IDs are dropped during merge so refreshStatic fires") and two comments in mergeCompactToolGroups.ts:177,201 still reference the refreshStatic detection mechanism that this PR removes. These should be updated to reflect the new approach (skip merge entirely rather than detect-and-refresh).
No tests for new behavior. All test changes are mechanical (compactInline: false in existing providers). No test exercises compactInline: true or useTerminalBuffer: false with compactMode: true to verify that mergeCompactToolGroups is correctly skipped. Since compactInline is described as "for future use," this may be intentional, but consider adding at least one regression test for the !useTerminalBuffer path — that is the core bug fix.
— qwen3.7-plus via Qwen Code /review
…ression test - Remove references to deleted refreshStatic heuristic in MainContent.tsx and mergeCompactToolGroups.ts, reflecting the new skip-merge approach - Add regression test for compactMode + !useTerminalBuffer path to ensure consecutive tool_groups are not merged in Static mode
compactInline is read via useState with no dynamic setter, so changes require a CLI restart to take effect.
- Set compactInline requiresRestart to true (no dynamic setter exists) - Split combined skip-merge condition into two distinct branches: !useTerminalBuffer (bug fix for Static append-only flash) and compactInline (user preference for per-group display)
- Set compactInline requiresRestart to true (no dynamic setter exists) - Split combined skip-merge condition into two distinct branches: !useTerminalBuffer (bug fix for Static append-only flash) and compactInline (user preference for per-group display) - Gate absorbedCallIds on useTerminalBuffer && !compactInline so that standalone summary labels render in Static mode where <Static> cannot repaint committed items
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] Stale comment in mergeCompactToolGroups.test.ts:397 references the removed refreshStatic heuristic: "MainContent's refreshStatic heuristic would never fire". The useEffect was removed in this PR. This line is outside the PR diff so it could not be anchored inline. Suggested update: "the summary is consumed by the compact header and dropped from merged output to avoid double-displaying the label."
Other Suggestion-level findings (consolidate 3 early-return guards, compactInline silent no-op without compactMode, compactInline/isHiddenInCompact test gaps) are already covered by existing open inline comments from prior rounds and are not repeated here.
— qwen3.7-plus via Qwen Code /review
Local Verification ReportBranch: TypeScript Compilation (
|
| Package | PR Branch | main | Status |
|---|---|---|---|
packages/core |
0 errors | 0 errors | ✅ Clean |
packages/cli |
106 errors | 106 errors | ✅ No new errors (identical) |
Unit Tests (vitest)
| Test File | PR Branch | main | Status |
|---|---|---|---|
MainContent.test.tsx |
12 passed | 11 passed | ✅ All pass (+1 new test) |
ToolGroupMessage.test.tsx |
32 passed | — | ✅ All pass |
ToolMessage.test.tsx |
35 passed | — | ✅ All pass |
SettingsDialog.test.tsx |
53 passed | — | ✅ All pass |
New test verifies that two consecutive tool_group items are NOT merged in <Static> mode (compactMode=true, useTerminalBuffer=false), confirming 5 items (3 prefix + 2 raw) instead of the pre-fix 4 (3 prefix + 1 merged).
Code Review
Problem: Ink's <Static> component is append-only — once items are committed to the terminal buffer, they can't be updated. When compact mode merged consecutive tool groups (reducing item count), the old code called refreshStatic() / clearTerminal to force a full re-render, causing visible screen flash.
Fix — two-pronged approach:
-
Skip cross-group merge in
<Static>mode (MainContent.tsx):- When
!useTerminalBuffer(Static path),mergeCompactToolGroups()is bypassed entirely — raw history items are passed through unchanged - When
compactInlineis enabled, same bypass applies - The
refreshStatic()/clearTerminalheuristic (useEffectblock) is removed entirely — no longer needed since merges don't happen in Static mode
- When
-
Hide absorbed items at render level (
HistoryItemDisplay.tsx):gemini_thought,gemini_thought_content, and absorbedtool_use_summaryitems returnnullin compact mode- This replaces the previous approach of filtering them at the merge level
-
New
compactInlinesetting (settingsSchema.ts,CompactModeContext.tsx,AppContainer.tsx):ui.compactInline: boolean(defaultfalse) — compact display within each group without cross-group merging- Exposed in settings dialog and VS Code schema
Summary line accounting in absorbedCallIds:
- When no cross-group merge will happen (
!useTerminalBuffer || compactInline), returnsEMPTY_ABSORBED_CALL_IDSso standalone● <label>lines can render — correct because<Static>can't repaint committed items
Verdict
✅ Ready to merge — No regressions. All 132 affected tests pass across 4 test files. TSC error count identical to main. Clean fix that eliminates the screen flash root cause rather than working around it.
|
@wenshao Thanks for the review and approval! |
|
Thanks for the review! @wenshao R2 comments addressed:
|
LaZzyMan
left a comment
There was a problem hiding this comment.
Re-review of the current diff (verified line-by-line + against CI)
The structural fix is sound: <Static> is append-only, so skipping the item-count-changing merge is the right fix rather than the previous detect-and-refreshStatic heuristic — and dropping that effect also removes its streaming re-render churn.
Prior [Critical] findings are all resolved in the current diff (flagging this so the stale CHANGES_REQUESTED doesn't cause confusion):
status: 'completed'→ nowToolCallStatus.Success(TS error gone, CI green).- Double-display of the summary label →
getCompactLabelis now guarded by!useTerminalBuffer || compactInline, symmetric with theabsorbedCallIdsguard. - Dropped async label in default Static compact mode →
absorbedCallIdsreturns empty in Static, so the standalone● <label>line renders as a clean append. requiresRestart→true; stalerefreshStaticcomments updated.
The Ctrl+O repaint path is independent of the removed effect (it goes through compactToggleHasVisualEffect → refreshStatic in AppContainer), so toggling compact mode in Static still repaints correctly. CI is green on macOS / Ubuntu / Windows.
The one thing I'd still push on: compactInline scope + test gap
compactInline only changes behavior in VP mode. In mergedHistory, if (!useTerminalBuffer) return raw short-circuits first, so on the default Static path compactInline is a silent no-op — it only matters when useTerminalBuffer=true. Two consequences:
- Zero test coverage for the path it actually affects. Consider a
{ compactMode: true, compactInline: true, useTerminalBuffer: true }test asserting two consecutivetool_groups are not merged (the mirror of the new Static-mode test). - The setting description says "Requires compactMode to be enabled" but not that it also requires
useTerminalBuffer; a user enabling it in the default config sees no change.
Since it's a "for future use" flag, splitting it into a follow-up PR — keeping this one focused on the flash fix — is also a fine option.
Minor (non-blocking)
- The PR description says the
isHiddenInCompactearly-return "runs before theuseMemo"; in the code it runs afteruseMemo(escapeAnsiCtrlCodes)(correctly — moving it earlier would break the Rules of Hooks). So the memo still runs for hidden items; worth fixing the wording. - The
tool_use_summaryJSX guard(!compactMode || !summaryAbsorbed)is now always-true once the early-return handles thecompactMode && summaryAbsorbedcase — harmless, can be simplified. - Release-notes note: default (Static) compact mode now loses cross-group merging; the summary label appears as a standalone
● <label>append line instead of in the merged header. Intentional, but a visible change for default-config users.
Verdict
Approve with minor suggestions — the core fix is correct and verified; the only substantive remaining item is the compactInline scope / test gap above.
LaZzyMan
left a comment
There was a problem hiding this comment.
Approving — core flash fix is correct and verified (all prior Criticals resolved, CI green on all three platforms). The remaining items from my re-review (compactInline test gap / VP-only scope, description wording, redundant guard) are non-blocking nits; feel free to fold the compactInline test into this PR or a follow-up.
|
@zzhenyao One more thing — taking a look at your contribution history, You have contributed several high-quality issues and pull requests to the qwencode project. Really appreciate the open-source work! If you'd like, drop a shipping address at pomelo.lcw@alibaba-inc.com and we'd love to send you some Qwen swag as a small thank-you. |
|
Thanks @LaZzyMan for the review and approve! 🙏 |
| if (item.type !== 'tool_group' || item.tools.length === 0) | ||
| return undefined; | ||
| // When merge is skipped (Static mode or compactInline), tool_groups | ||
| // render via the full ToolGroupMessage path which ignores compactLabel. |
There was a problem hiding this comment.
[Suggestion] Misleading comment: states "tool_groups render via the full ToolGroupMessage path which ignores compactLabel," but CompactToolGroupDisplay does use compactLabel (line 166-167: compactLabel ? renderSummaryHeader(...) : renderDefaultHeader(...)). The label doesn't appear because getCompactLabel returns undefined here, not because the rendering path ignores the prop. A future maintainer reading this comment might conclude compactLabel is unused and pass a value without realizing it would cause double-display.
| // render via the full ToolGroupMessage path which ignores compactLabel. | |
| // When merge is skipped (Static mode or compactInline), return undefined | |
| // so CompactToolGroupDisplay falls back to its default "Tool × N" header. | |
| // The standalone `● <label>` line in HistoryItemDisplay is the label's | |
| // only path to the screen; providing compactLabel here would cause | |
| // double-display. | |
| if (!uiState.useTerminalBuffer || compactInline) return undefined; |
— qwen3.7-max via Qwen Code /review
| // 3 prefix items (header / debug / notifications) + 2 raw history items | ||
| // The 2 tool_groups should NOT be merged into 1. | ||
| expect(staticItemsSpy.mock.calls.at(-1)?.[0]).toHaveLength(5); | ||
| // Verify both tool_group ids are present via historyItemDisplayPropsSpy. |
There was a problem hiding this comment.
[Suggestion] This test (and the preceding one at line 585) asserts item count and IDs via historyItemDisplayPropsSpy, but never asserts the compactLabel prop value. The new getCompactLabel early-return at MainContent.tsx:273 (if (!uiState.useTerminalBuffer || compactInline) return undefined) is therefore untested. If a future change accidentally makes getCompactLabel return a value in Static mode, the label would double-display — but these tests would still pass.
Consider adding:
// Verify compactLabel is undefined for all rendered tool_groups in Static mode.
const compactLabels = historyItemDisplayPropsSpy.mock.calls
.map((call) => call[0].compactLabel)
.filter((label) => label !== undefined);
expect(compactLabels).toHaveLength(0);— qwen3.7-max via Qwen Code /review
|
Thanks for the R3 review @wenshao! |
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
What this PR does
Fixes the full-screen flash that occurs in compact mode every time a batch of tool calls completes:
<Static>mode: WhenuseTerminalBufferis false,mergedHistoryreturns the raw history array unchanged, so<Static>never sees a key count change. The merge still runs in VP mode where items can be updated in place.refreshStaticdetection useEffect: With no merge happening, the length-mismatch detector that calledrefreshStatic()is dead code. Removing it also eliminates unnecessary re-renders during streaming.compactInlinesetting: A new boolean (defaultfalse) for future use — when enabled, skips cross-group merging even in VP mode. Read from settings at startup, passed throughCompactModeContext. Not toggled byCtrl+O.HistoryItemDisplayalso gets a small cleanup: theisHiddenInCompactearly-return now runs before theuseMemoforitemForDisplay, avoiding unnecessary work for items that won't render.Fixes: #4794 #4561 #3979
Why it's needed
In compact mode,
mergeCompactToolGroupsmerges consecutive tool groups into fewer items. This changes the history array length. Ink's<Static>component is append-only — when the number of keys changes, it can't do its normal incremental update.A
useEffectinMainContent.tsxdetected the length mismatch and calledrefreshStatic(), which writesclearTerminalto stdout and remounts the entire<Static>tree. This didn't prevent the flash.Reviewer Test Plan
Evidence (Before & After)
Before:
After:
How to Verify
Build and run
Trigger sequential tool calls in compact mode
Ctrl+ODo not think. First run ls packages/cli/src/ui/components/ to see what files are there, then read the first 3 lines of the first file, and finally use wc -l to count the total lines of that file.Regression
中文
此 PR 做了什么
修复紧凑模式下每批工具调用完成时的全屏闪烁：
<Static>模式下跳过数据层合并：当useTerminalBuffer为 false 时，mergedHistory直接返回原始 history 数组，<Static>不会感知到 key 数量变化。VP 模式下合并仍然生效（因为 VP 可以原地更新 item）。refreshStatic检测 useEffect：既然不再合并，检测数组长度不匹配并调用refreshStatic()的代码就是死代码。移除后也减少了流式输出期间的不必要重渲染。compactInline设置：新的布尔值（默认false），为后续使用预留——启用后即使在 VP 模式下也跳过跨组合并。从 settings 读取，通过CompactModeContext传递，不受Ctrl+O切换。HistoryItemDisplay也有一个小清理：isHiddenInCompact的提前返回现在移到了itemForDisplay的useMemo之前，避免对不会渲染的 item 做不必要的计算。为什么需要
紧凑模式下，
mergeCompactToolGroups将连续的工具组合并为更少的 item。这改变了 history 数组的长度。Ink 的<Static>组件是 append-only 的——当 key 数量变化时，无法做正常的增量更新。MainContent.tsx中的useEffect检测到长度不匹配后调用refreshStatic()，该函数向 stdout 写入clearTerminal并重新挂载整个<Static>树。Reviewer 测试计划
证据（修改前 vs 修改后）
修改前：
修改后：
如何验证
构建并运行
在紧凑模式下触发串行工具调用
Ctrl+O不要思考，先运行 ls packages/cli/src/ui/components/ 看有哪些文件，然后读取其中第一个文件的前3行，最后用 wc -l 统计该文件总行数回归测试