fix(ui): reduce UI flicker — throttle + startTransition + batch STREAM_TEXT + debounce refresh + resize settle - #5396
Conversation
|
@qwen-code /triage |
|
Re-triage (July 7) — third pass. The PR was force-pushed on July 2 (expanding to 5 fixes / 7 issues) and had a merge conflict resolved today by Template: still incomplete — no Reviewer Test Plan (no "How to verify", no Before/After), no Risk & Scope, no Tested on table. This was flagged in the first triage on June 19 and remains unaddressed after three rounds. Not blocking on its own, but a flicker fix spanning 7 issues and 5 independent changes really needs a repro path for reviewers. Problem: all 7 referenced issues (#4561, #3838, #2928, #6137, #3979, #5971, #1184) are real, open issues with user reports. Observed bugs ✓. Direction: UI flicker is squarely in the terminal-UX space. Aligned ✅. Size: 43 production lines in core ( Approach: same concerns carried forward from the July 2 review, unchanged because the code is unchanged:
Splitting into per-issue PRs would let the good parts (C1, B1, D3) merge independently of the blocker (D2). Moving on to code review. 🔍 中文说明重新审查（7 月 7 日） — 第三轮。PR 于 7 月 2 日 force-push（扩展到 5 项修复 / 7 个 issue），今天由 模板：仍不完整 — 没有 Reviewer Test Plan（无"如何验证"、无 Before/After）、没有 Risk & Scope、没有 Tested on 表格。这在 6 月 19 日首轮审查时已指出，三轮过后仍未修改。单独看不阻塞，但一个覆盖 7 个 issue、5 项独立修复的闪烁修复确实需要给审查者一条复现路径。 问题：7 个引用的 issue 都是真实的、有用户报告的开放 issue。已观测的 bug ✓。 方向：UI 闪烁属于终端 UX 范畴。方向对齐 ✅。 规模：核心 43 行生产代码（ 方案：与 7 月 2 日审查相同的问题，因代码未变而保持不变：
拆成按 issue 分的多个 PR，能让好的部分（C1、B1、D3）独立合并，不被阻塞项（D2）拖住。进入代码审查 🔍 — Qwen Code · qwen3.7-max |
🔍 Maintainer verification — real local + tmux testingBuilt the PR head ( A2 —
|
| Check | Result |
|---|---|
| Build | ✅ exit 0 |
B1 useGeminiStream tests (100ms) |
✅ 124 / 124 |
C1 existing STREAM_TEXT test |
✅ pass |
| C1 batching (added test) | ✅ 3 parts → 1 event; mutation → 3 events |
| C1 ordering change | [A,T,B] → [T,AB] (mutation-confirmed) |
| D loop-detection mirror | ❌ test =4 vs source =6; passes silently |
| A2 vs #4561 | ❌ #4561 is the CLI; A2 patches web-shell |
| tmux CLI Ctrl+O compact | ✅ toggles (the #4561 surface, untouched); 0 stderr |
Verdict
Not ready to merge as-is. Recommend: (1) revert the loopDetectionService.test.ts edit (D); (2) relocate A2 to the CLI compact path (AppContainer.tsx) or drop the #4561 claim — the web-shell change can't fix a CLI issue; (3) add C1's batching test and decide whether the thought-first reorder is acceptable; (4) reframe B1 as a general throttle tweak rather than a #3838 fix. Splitting into per-issue PRs would make each independently verifiable.
🇨🇳 中文版（点击展开）
🔍 维护者验证 —— 本地真实 + tmux 测试
在隔离 worktree（Node v22.22.2）构建 PR head（c11cb049），通读了全部 5 个文件。结论：这个捆绑 PR 把一个尚可接受的改动（C1，但没带测试）和两个修错地方的 issue 修复、以及一处错误且无关的测试改动混在一起。建议合并前修改 —— 最好拆成按 issue 分的多个 PR。
A2 —— startTransition compact toggle 修在了错误的位置，解决不了 #4561
- qwencode在紧缩模式下，ctrl+O开启后，执行任务会频繁闪屏。 #4561 明确是 CLI（Ink 终端）在 Windows 上的问题："window的qwen-code，使用 ctrl+o 打开紧缩模式，让他执行任务，就可以看到频繁刷新闪屏"（labels
scope/windows、scope/rendering）。 - CLI 的 compact toggle 在
packages/cli/src/ui/AppContainer.tsx:3137-3139——Command.TOGGLE_COMPACT_MODE（key.ctrl && key.name === 'o'）→ 直接调用setCompactMode(newValue)，没有startTransition。 - 而 A2 是在
packages/web-shell/client/App.tsx（浏览器应用，不是 CLI）里给setCompactMode套了startTransition。CLI 那条路径根本没动。 - 真实 CLI（tmux）： Ctrl+O 真的会切换 compact 模式 —— 它把 thought 块收起，这正是 qwencode在紧缩模式下，ctrl+O开启后，执行任务会频繁闪屏。 #4561 的面向，而本 PR 在这里什么都没改：
帧 A（普通）： 帧 B（按 Ctrl+O 后 —— compact）：
Thought for 0s Thought for 0s (ctrl+o to expand)
Simple mental math. ✦ 17 × 23 = … = 391
✦ 17 × 23 = … = 391
→ A2 无法修复 #4561（改错了 package）。（另外 startTransition 是 React-DOM 的并发特性；Ink 终端渲染器并不会以相同方式对待它。）
D —— loopDetectionService.test.ts 是错误且无关的改动，请回退
- 该测试镜像了一个源码常量，注释写着 "Mirrored from loopDetectionService.ts … failures point to the constant that changed."。PR 把镜像改成
GLOBAL_DUPLICATE_THRESHOLD 6 → 4，但源码没改 ——loopDetectionService.ts:50仍是6。镜像现在谎报了源码值，彻底破坏了镜像的意义。 - 它还把一个循环边界
TOOL_CALL_LOOP_THRESHOLD - 1改成GLOBAL_DUPLICATE_THRESHOLD - 1，而这个测试名为should not detect a loop for fewer than TOOL_CALL_LOOP_THRESHOLD identical calls—— 用了不相关的常量（GLOBAL_DUPLICATE 管的是重复内容检测 L671；工具调用循环用的是 TOOL_CALL_LOOP_THRESHOLD L298）。 - 它仍然静默通过（3 < 5），所以 CI 不会报警。它和 flicker 毫无关系。请回退这两处。
C1 —— 按 chunk 批量 STREAM_TEXT：方向对（#2928），但没带测试且改变了顺序
- 已验证确实在批量：一个 chunk 里 3 个 stream part → FIXED 合成 1 个事件（
abc），pre-fix 是 3 个（变异测试确认）。减少 sub-agent 的 UI 更新 → 合理地有助于 [Bug] 模型调用并行子Agent时, TUI出现频繁闪烁 #2928（并行子 Agent 闪烁，这是本 PR 唯一对得上的 issue）。 - batching 没有任何测试。 唯一的
STREAM_TEXT测试只喂 1 个 thought + 1 个 stream part，根本不触发 batching。我加了一个临时测试来证明这个行为；PR 应当自带一个。 - 行为变化： batching 会先发 thought、并合并非相邻的 stream part。一个 chunk
[A(text), T(thought), B(text)]会变成[T(thought), AB(text)]（变异测试确认 pre-fix 顺序是[A, T, B]）—— 原有的交错顺序没保留。累积的最终文本（roundText）不受影响，但流式显示顺序变了。值得做个明确决定 + 补测试。
B1 —— throttle 60 → 100ms：一致，但只是频率启发式，并非 #3838 那个循环的修复
- 常量与它的 7 处测试更新一致（124/124 通过）。
- 终端界面无限滚动/刷新循环 #3838 是一个无限重渲染循环（"滚动条无限拉长…内容像被反复渲染"，Windows）。throttle 只会减慢更新节奏，并不能停止一个重渲染反馈循环 —— 治标不治本。这里无法复现（Windows 专属；闪烁是
capture-pane无法测量的帧间属性）。
证据
| 检查项 | 结果 |
|---|---|
| 构建 | ✅ exit 0 |
B1 useGeminiStream 测试（100ms） |
✅ 124 / 124 |
C1 既有 STREAM_TEXT 测试 |
✅ 通过 |
| C1 batching（我加的测试） | ✅ 3 part → 1 事件；变异 → 3 事件 |
| C1 顺序变化 | [A,T,B] → [T,AB]（变异确认） |
| D loop-detection 镜像 | ❌ 测试 =4 vs 源码 =6；静默通过 |
| A2 vs #4561 | ❌ #4561 是 CLI；A2 改的是 web-shell |
| tmux CLI Ctrl+O compact | ✅ 可切换（#4561 面向，未被触碰）；0 stderr |
结论
当前状态不宜合并。建议：（1）回退 loopDetectionService.test.ts（D）；（2）把 A2 移到 CLI 的 compact 路径（AppContainer.tsx），或撤掉 #4561 的关联 —— web-shell 的改动修不了 CLI 的问题；（3）为 C1 的 batching 补测试，并就"thought 先发"的重排是否可接受做出决定；（4）把 B1 重新定位为一个通用的 throttle 微调，而非 #3838 的修复。拆成按 issue 分的 PR 能让每项独立可验证。
Method: isolated worktree build of c11cb049 · per-file review with issue cross-check (#4561/#3838/#2928) · mutation on agent-core.ts (C1 batches 3→1, reorders [A,T,B]→[T,AB]) · loop-detection mirror mismatch (test 4 vs source 6, silent pass) · 124/124 useGeminiStream + agent STREAM_TEXT tests · real-CLI tmux Ctrl+O compact-mode demo (the #4561 surface, untouched by the PR). No Windows/flicker repro available here.
Code Review (July 7, third pass)The PR diff is unchanged since the July 2 force-push (only a merge-conflict import dedup was added today). My independent code read confirms every concern the maintainer raised — and CI got worse. D2 — debounce refreshStatic: breaks CI (still 4 failures, confirmed)The PR wraps
D2 defers Two additional issues in the same hunk: no unmount cleanup for B1 — throttle 60→100ms: 4 NEW test failures (CI now 8 failed, up from 4)The PR updated 7 timer advancements in the throttle tests but missed 4 more in the Cancellation block:
These tests advance timers by 60ms expecting the throttle to fire, but the throttle is now 100ms so nothing happens. CI went from 4 failed → 8 failed ( C1 — batch STREAM_TEXT: logic sound, test still vacuousThe batching logic in The reorder concern also stands: A2 — startTransition compact toggle: wrong surface (web-shell ≠ CLI)The PR modifies B1/D3 — heuristic knobs, not root-cause fixesB1 (throttle 60→100ms) slows a feedback loop but doesn't stop #3838's re-render cycle. D3 (resize settle 200→400ms) doubles post-resize repaint latency with no test pinning 400 as the correct value (all assertions use the symbol TestingCI run The maintainer's July 2 verification ( CI evidence中文说明代码审查（7 月 7 日，第三轮）PR diff 自 7 月 2 日 force-push 以来无变化（今天仅添加了合并冲突 import 去重）。独立代码阅读确认了维护者提出的所有问题 — 且 CI 变得更糟。 D2 — debounce refreshStatic：仍然破坏 CI（4 个失败，已确认）PR 将 B1 — throttle 60→100ms：4 个新测试失败（CI 从 4 个增加到 8 个）PR 更新了 7 处计时器但遗漏了 Cancellation 块中的 4 处： C1 — batch STREAM_TEXT：逻辑正确，测试空过批处理逻辑正确。但新加的测试创建裸 A2 — startTransition compact toggle：改错了地方PR 修改的是 测试CI 运行 — Qwen Code · qwen3.7-max |
Verdict (July 7, third pass)Not mergeable. CI is red with 8 deterministic failures — 4 more than the last review — all caused by this PR's own changes (D2 and B1). The code hasn't changed since the maintainer's July 2 verification, and every concern from that review stands. The good parts are real: C1's batching logic is sound, B1's throttle bump and D3's settle increase are low-risk heuristics. But the PR bundles 5 independent fixes into one, and the two problematic changes (D2, A2) block the rest. What needs to happen before this can merge:
The author asked the maintainers to fix the flicker themselves (July 2 comment), which is fair — these are real bugs with real user impact. But a PR that adds 4 new test failures on top of 4 existing ones, and targets the wrong surface for one of its claims, isn't the path forward. @aspnmy — the fastest path to getting parts of this merged: split into per-issue PRs, fix D2's synchronous contract, and update the missed timer values. C1 + B1 + D3 could land quickly on their own. 中文说明结论（7 月 7 日，第三轮）不可合并。CI 红色，8 个确定性失败 — 比上次审查多 4 个 — 全部由本 PR 自己的改动引起（D2 和 B1）。代码自 7 月 2 日维护者验证以来未变，该审查中的每个问题都依然存在。 好的部分是真实的：C1 的批处理逻辑合理，B1 的 throttle 调整和 D3 的 settle 增加是低风险的启发式调整。但 PR 将 5 项独立修复捆绑在一起，两个有问题的改动（D2、A2）阻塞了其余部分。 合并前需要做的事：
作者在 7 月 2 日评论中请维护者自己修闪烁问题，这很合理 — 这些是真实的 bug，有真实的用户影响。但一个在原有 4 个测试失败基础上又新增 4 个失败的 PR，且其中一个修复改错了地方，不是正确的路径。 @aspnmy — 最快的合并路径：拆成按 issue 分的 PR，修复 D2 的同步契约，更新遗漏的计时器值。C1 + B1 + D3 可以很快独立合并。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
The three UI flicker fixes look great — clean, minimal, and well-targeted. However, the loopDetectionService.test.ts change is unrelated to this PR and breaks 2 tests (test-local threshold lowered to 4 while production stays at 6). Please revert that file and this should be good to merge. See review comments above for details. 🙏
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Requesting changes: the PR currently fails CI. packages/core/src/services/loopDetectionService.test.ts has 2 failing tests (confirmed locally: 2 failed | 58 passed; the Test (macos-latest, Node 22.x) check is red) — the test mirror was lowered to a threshold the production code doesn't use. The UI-flicker changes themselves (throttle, STREAM_TEXT batching, compact startTransition) are reasonable; see inline notes.
— claude-opus-4-8 via Claude Code /qreview
| // self-describing and failures point to the constant that changed. | ||
| const FILE_READ_WINDOW = 15; | ||
| const GLOBAL_DUPLICATE_THRESHOLD = 6; | ||
| const GLOBAL_DUPLICATE_THRESHOLD = 4; |
There was a problem hiding this comment.
[Critical] This file's changes break CI — confirmed by running the suite: loopDetectionService.test.ts → 2 failed | 58 passed, and the PR's Test (macos-latest, Node 22.x) check is already red. The failures are should fire when same (tool, args) appears threshold times non-consecutively (line 1199) and should fire for consecutive identical calls via both detectors (line 1238): both expect true but get false, because the production threshold is unchanged (loopDetectionService.ts:50 is still GLOBAL_DUPLICATE_THRESHOLD = 6 on this branch and on main) while this local mirror was lowered to 4 — so the tests now drive only 4 duplicates and the detector correctly doesn't fire until 6.
This loop-detection edit is also unrelated to the UI-flicker objective, so the clean fix is to drop all loopDetectionService changes from this PR (revert the whole file). One caveat: reverting only this constant back to 6 is not sufficient — the other edit at line 86 (TOOL_CALL_LOOP_THRESHOLD - 1 → GLOBAL_DUPLICATE_THRESHOLD - 1) must be reverted too. With the mirror at 6 but line 86 left changed, that test fails as well: it then drives 5 consecutive identical calls and trips the consecutive detector at TOOL_CALL_LOOP_THRESHOLD = 5 (its title and sibling test both correctly use TOOL_CALL_LOOP_THRESHOLD). If you do intend to lower the threshold, please split it into a separate PR that also updates the production constant.
— claude-opus-4-8 via Claude Code /qreview
| const previous = compactModeRef.current; | ||
| const next = !compactModeRef.current; | ||
| setCompactMode(next); | ||
| startTransition(() => setCompactMode(next)); |
There was a problem hiding this comment.
[Suggestion] Wrapping this toggle in startTransition can leave the compact toggle stuck under load. handleToggleCompact is a stable useCallback (deps [reportError, setWorkspaceSetting, t] — compactMode is intentionally excluded) that reads the current value from compactModeRef, which is only refreshed on a committed render (line 1453). Previously setCompactMode(next) was an urgent update, so the ref was current by the next keypress. As a transition (low-priority lane) the compactMode commit can be deferred/starved under exactly the streaming load this PR targets; if a second toggle (Ctrl+O auto-repeat, or a quick double-press) arrives before it commits, compactModeRef.current is still stale, so next = !current recomputes the same value — the UI fails to alternate and setWorkspaceSetting even persists the wrong value.
If the transition is meant to defer the compact re-layout, read the value without the ref — e.g. add compactMode to the callback deps (and drop compactModeRef) so previous/next are always fresh. Otherwise a plain setCompactMode(next) (a single boolean flip is cheap) restores the ref-freshness this stable callback relies on.
— claude-opus-4-8 via Claude Code /qreview
fork版本用4 的原因是 阿里源Api 重试次数是4，这里只是和api接口一致，避免产生不必要报错，造成未知问题，纯粹我自己的强迫症习惯，可以回退 |
| // self-describing and failures point to the constant that changed. | ||
| const FILE_READ_WINDOW = 15; | ||
| const GLOBAL_DUPLICATE_THRESHOLD = 6; | ||
| const GLOBAL_DUPLICATE_THRESHOLD = 4; |
There was a problem hiding this comment.
[Critical] This breaks CI — Test is failing on macOS/ubuntu/windows. GLOBAL_DUPLICATE_THRESHOLD here is a mirror of the source constant (loopDetectionService.ts:50 = 6), kept in sync per this file's own comment. This PR changes the mirror to 4 without changing the source, so the two Global Tool Call Duplicate Detection tests that loop THRESHOLD times and assert a loop fires now only reach 4 (real threshold is 6) → AssertionError: expected false to be true. The companion edit at line 86 (TOOL_CALL_LOOP_THRESHOLD - 1 → GLOBAL_DUPLICATE_THRESHOLD - 1) also mis-uses the constant inside a test literally named “fewer than TOOL_CALL_LOOP_THRESHOLD”, weakening that boundary check. None of this is related to the UI-flicker fix — please revert the whole loopDetectionService.test.ts change (restore the mirror to 6 and the original TOOL_CALL_LOOP_THRESHOLD loop bound at line 86).
| const GLOBAL_DUPLICATE_THRESHOLD = 4; | |
| const GLOBAL_DUPLICATE_THRESHOLD = 6; |
— claude-opus-4-8 via Claude Code /qreview
| chunkStreamText += txt; | ||
| } | ||
| } | ||
| if (chunkThoughtText) |
There was a problem hiding this comment.
[Suggestion] The per-chunk batching emits all thought text before all regular text, which reorders vs the original per-part order. For standard reasoning_content providers (always thought-then-text) this is a no-op — but for tagged-thinking models (<think> tags, e.g. the MiniMax provider with taggedThinkingTags: true), TaggedThinkingParser yields interleaved parts like [text, thought, text] within a single chunk ("answer <think>reasoning</think> more answer"). STREAM_TEXT's only live consumer is the web-shell sub-agent view (SubAgentTracker → ACP agent_thought_chunk/agent_message_chunk → appendSubContent, concatenated in arrival order), so reasoning text would render ahead of answer text the model emitted before it, and answer fragments straddling </think>…<think> get merged across the reasoning block. (The CLI is unaffected — it renders sub-agents from committed ROUND_TEXT, not STREAM_TEXT.)
Suggest coalescing only adjacent same-kind runs instead of bucketing by type — flush the running buffer whenever isThought flips — which keeps the flicker win (one emit per contiguous run) without reordering across a type boundary, and still emits exactly thought-then-text for the common path. Please also add a test feeding one chunk with [{text:'A'},{text:'B',thought:true},{text:'C'}] and asserting stream order is preserved — the existing STREAM_TEXT test only feeds thought-first input, so it can't catch this.
— claude-opus-4-8 via Claude Code /qreview
| const previous = compactModeRef.current; | ||
| const next = !compactModeRef.current; | ||
| setCompactMode(next); | ||
| startTransition(() => setCompactMode(next)); |
There was a problem hiding this comment.
[Suggestion] handleToggleCompact derives next from compactModeRef.current, but wrapping setCompactMode(next) in startTransition makes the resulting commit (and the render-time compactModeRef.current = compactMode resync) non-urgent and interruptible. The toggle is a raw keydown handler (Ctrl+O), so holding the key fires this many times/sec via OS key-repeat — and under the heavy streaming this PR targets, transitions are exactly when commits get deferred. Two invocations landing before the transition commits both read the same stale ref, compute the same next, so the toggle stops alternating and issues duplicate setWorkspaceSetting writes (and the .catch rollback can clobber a newer value with a stale previous).
Suggest a functional updater so it never depends on the deferred ref:
startTransition(() => setCompactMode((prev) => !prev));(compute/persist next inside the updater, or sync compactModeRef.current synchronously before scheduling).
— claude-opus-4-8 via Claude Code /qreview
| it(`should not detect a loop for fewer than TOOL_CALL_LOOP_THRESHOLD identical calls`, () => { | ||
| const event = createToolCallRequestEvent('testTool', { param: 'value' }); | ||
| for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD - 1; i++) { | ||
| for (let i = 0; i < GLOBAL_DUPLICATE_THRESHOLD - 1; i++) { |
There was a problem hiding this comment.
[Suggestion] The test name says "should not detect a loop for fewer than TOOL_CALL_LOOP_THRESHOLD identical calls" but the loop body was changed from TOOL_CALL_LOOP_THRESHOLD - 1 to GLOBAL_DUPLICATE_THRESHOLD - 1. This weakens the boundary the test claims to validate — it now iterates only 3 times (4−1) instead of 4 (5−1), which is below both thresholds but no longer tests the consecutive-call boundary described in the test name.
Either revert the loop bound back to TOOL_CALL_LOOP_THRESHOLD - 1 (this test is about the consecutive-call threshold), or update the test name to reference GLOBAL_DUPLICATE_THRESHOLD.
— qwen3.7-max via Qwen Code /review
|
Thanks for the PR! Heads-up on the red CI — the failure is a unit test, not a build/compile error. What's failingThe three
Root causeThis PR lowers the mirrored constant Both tests feed exactly A few signs this edit was unintended:
(For reference, on Fix — pick one
中文版感谢提交 PR！提醒一下红掉的 CI——失败的是单元测试，不是编译/构建错误。 失败内容三个
根因本 PR 在测试文件里把那个镜像常量 这两个用例都会喂恰好 几个说明这是误改的旁证：
（作为参考：在 修复方式——二选一
|
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #5396ConflictFile:
Root cause
Both sides touched adjacent lines of the same constant block, producing a text conflict. ResolutionKept both changes: const GLOBAL_DUPLICATE_THRESHOLD = 4;
const SHELL_COMMAND_STAGNATION_THRESHOLD = 8;
Commit
No other files were modified. |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
The throttle interval change (60→100 ms) and the startTransition wrap in App.tsx look reasonable for reducing flicker. The agent-core.ts batching change (emit once per chunk rather than per part) is a sensible reduction in re-render frequency.
The existing inline threads from @wenshao cover the key issues I identified (loop-detection test threshold mismatch, useEffect dependency array with initialInputChunks, and the batching ordering concern), so I have no new findings to add.
Note: Test (ubuntu-latest, Node 22.x) is currently failing on this PR. Please check CI before merge.
Automated review — findings reflect static analysis of the diff only.
Generated by Claude Code
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Found 2 issues: a test-production mismatch in loopDetectionService.test.ts where the local mirror constant GLOBAL_DUPLICATE_THRESHOLD is reduced to 4 while the production value stays at 6, and a related semantic error where the TOOL_CALL_LOOP_THRESHOLD-governed loop boundary is incorrectly replaced with the now-mismatched GLOBAL_DUPLICATE_THRESHOLD constant.
Generated by Claude Code
| // self-describing and failures point to the constant that changed. | ||
| const FILE_READ_WINDOW = 15; | ||
| const GLOBAL_DUPLICATE_THRESHOLD = 6; | ||
| const GLOBAL_DUPLICATE_THRESHOLD = 4; |
There was a problem hiding this comment.
Issue: The PR lowers the local test mirror constant GLOBAL_DUPLICATE_THRESHOLD from 6 to 4, but the production constant in loopDetectionService.ts remains 6. The comment above this constant explicitly states it is "mirrored from loopDetectionService.ts" — the divergence means the test now validates against a threshold that does not exist in production code, so the global-duplicate detection tests (lines ~1507–1596) will pass even when the production threshold of 6 is not being exercised correctly.
Suggested fix: If the intent was to lower the production threshold, update GLOBAL_DUPLICATE_THRESHOLD in loopDetectionService.ts to 4 as well. If the intent was only to adjust tests, the constant must not be changed independently — it must always mirror the production value.
Generated by Claude Code
| it(`should not detect a loop for fewer than TOOL_CALL_LOOP_THRESHOLD identical calls`, () => { | ||
| const event = createToolCallRequestEvent('testTool', { param: 'value' }); | ||
| for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD - 1; i++) { | ||
| for (let i = 0; i < GLOBAL_DUPLICATE_THRESHOLD - 1; i++) { |
There was a problem hiding this comment.
Issue: The PR replaces TOOL_CALL_LOOP_THRESHOLD - 1 with GLOBAL_DUPLICATE_THRESHOLD - 1 in the loop body of the test named "should not detect a loop for fewer than TOOL_CALL_LOOP_THRESHOLD identical calls". These are different constants governing different detection mechanisms: TOOL_CALL_LOOP_THRESHOLD (= 5) guards consecutive identical calls; GLOBAL_DUPLICATE_THRESHOLD (= 6 in production, now 4 in the test) guards non-consecutive repeated calls across a turn. After this change the test only exercises 3 iterations (4 − 1) instead of 4 (5 − 1), so it no longer correctly tests the consecutive-call boundary. The test description still references TOOL_CALL_LOOP_THRESHOLD, making the substitution a clear semantic error.
Suggested fix: Revert this line to use TOOL_CALL_LOOP_THRESHOLD - 1 so it correctly tests the threshold it describes:
for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD - 1; i++) {Generated by Claude Code
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
No blocking issues found in this automated review pass.\n\n_— claude-sonnet-4-6 via Qwen Code /review_
Generated by Claude Code
|
UI闪烁问题已经很烦了 麻烦官方自己修一下 |
目前的版本上 UI闪烁问题是否有解决？我英文看不了 麻烦中文回复 |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Clean, focused flicker-reduction PR. The throttle increase, batched STREAM_TEXT emits, and startTransition wrapping are all correct approaches to reducing UI repaints. The agent-core batching preserves the thought/non-thought separation while cutting event volume per chunk.
— qwen3-coder via Qwen Code /review
2e5d322 to
127211f
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
…sition compact toggle + batch STREAM_TEXT (QwenLM#4561, QwenLM#3838, QwenLM#2928)
🔍 Maintainer verification — real local build + tmux (PR head
|
| Check | Result |
|---|---|
| Build + bundle | ✅ exit 0; D2/C1/B1 all shipped in dist/chunks/* |
CI Test (ubuntu) |
❌ Tests 4 failed — AppContainer.test.tsx |
| D2 local repro | ❌ 4 failed / 91 passed, 5× deterministic — matches CI |
| D2 A/B | ✅ revert D2 → 95/95 pass; restore → 4 fail (load-bearing) |
| D2 vs #4119 | ❌ reverts the synchronous in-handler guarantee (single-frame history dump risk) |
| D2 timer cleanup | setTimeout |
| C1 new test | ❌ vacuous — passes with batching reverted (mutation-confirmed) |
| C1 reorder | [A,T,B] → [T,AB] (final text unchanged) |
B1 useGeminiStream |
✅ 152/152 (value=100) |
| D3 resize | ✅ 6/6, but no test pins 400 (symbol-relative) |
| A2 vs #4561 | ❌ web-shell ≠ CLI surface |
| Real binary (tmux) | ✅ boots v0.19.5, streams thought+answer via C1 path |
(Note: a real-TTY run surfaces 7 extra Terminal Title failures, but the D2-reverted A/B shows those 7 fail too → pre-existing TTY flakiness, not this PR. Only the 4 refreshStatic failures are D2's.)
Verdict
Not ready to merge — CI is red because of D2. Suggested path:
- D2: don't blanket-defer. Keep
refreshStaticsynchronous on the model-change and cancel paths (or exempt them from debouncing) so chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed) #4119's single-commit guarantee holds; add unmount cleanup; update the 4 tests only after the synchronous contract is preserved; fix the "microtask" wording. - C1: make the test drive
runInAgentFramesand assert the emit count drops; decide whether the thought-first reorder is acceptable and pin it. - B1/D3: fine as heuristics — frame them as such (not root-cause fixes) and drop D3's dependence on an unmeasured constant, or add a test that pins intent.
- A2: relocate to the CLI compact path or drop the qwencode在紧缩模式下，ctrl+O开启后，执行任务会频繁闪屏。 #4561 claim.
Splitting into per-issue PRs would make each independently verifiable and unblock the good parts (C1) from the blocker (D2).
🇨🇳 中文版（点击展开）
🔍 维护者验证 —— 本地真实构建 + tmux（PR head cdc9399）
在隔离 worktree（Node v22.22.2）针对当前 head 重新验证：npm ci → npm run build && npm run bundle（exit 0），确认全部 5 项改动都进了真实 bundle，然后逐项测试 + 变异 A/B + 真实二进制 tmux 运行。
先肯定改进 —— 这个"clean version"修掉了上一轮的两点：无关的 loopDetectionService.test.ts 改动已删除 ✅；C1 现在带测试了 ✅。新增了两项改动（D2、D3）。
结论：当前状态不可合并 —— CI 是红的，而且原因正是本 PR 自己的 D2 改动。 D2 把 refreshStatic 改成异步，破坏了 4 个既有测试，同时回退了 PR #4119 [Critical] 特意引入的同步批处理保证（该保证用于防止切换模型时的闪烁）。C1 新加的测试是空过的（把 batching 全部还原后测试照样通过）。逐项证据如下。
❌ D2 —— debounce refreshStatic —— 弄红了 CI（4 个测试）并回退了 #4119 的修复
本 PR 的 CI 失败是真实、由代码引起的，不是基础设施抖动。失败步骤（Run tests and generate reports）报 Tests 4 failed | 10666 passed，4 个全在 AppContainer.test.tsx。我用真实 vitest（alias 到 src）本地复现，确定性 5 次，与 CI 完全一致（见上方第一张图）。
根因。 D2 把 refreshStatic 的函数体包进了 setTimeout(() => { …clearTerminal…; remountStaticHistory() }, 0)，于是 clearTerminal 写入变成异步，那 4 个断言它同步发生的测试就看到 Number of calls: 0。干净 A/B —— 只还原 D2 那一处 hunk，95 个全绿；恢复后正好挂 4 个（见上方 A/B 图）。
这不是"改改测试"就行。 生产代码的 handler 本身（AppContainer.tsx:1002-1008）就是把 refreshStatic(); setCurrentModel(model); 放在一起调用，紧挨其上的注释（为 PR #4119 而写）说明了为什么必须同步：
(b)
setHistoryRemountKey（在 refreshStatic 内）和setCurrentModel必须落在同一个 commit。……如果 currentModel 先在自己那次 render 里变，Static 会用旧的 remount key remount……一次绕过渐进重放的整段历史 Static 渲染（即 issue #3899 卡死回归）。
D2 的 setTimeout(0) 把 setHistoryRemountKey 推迟到后续 macrotask，于是它不再和同步的 setCurrentModel 批处理在一起 —— 正是注释禁止的那个拆分。切换模型时 <Static> 会先用旧 key remount 一次（整段历史闪现 / #3899 卡死），定时器触发后再 remount 一次。于是一个"降低闪烁"的 PR 重新引入了 #4119 修好的那个卡死/闪烁。 同样的推迟也伤到取消路径（测试 #2）：抹掉刚取消 prompt 的 clearTerminal 被推迟，可能闪现两次（transcript + 输入框）—— 与初衷相反。
同一处 hunk 还有两个问题：
- 没有 unmount 清理。 新的
refreshStaticRef定时器在卸载时从不清除（没有useEffectcleanup）。挂起的定时器会在组件拆卸后触发 → 退出时多写一次stdout.write(clearTerminal)+ 卸载后setState。 - 术语。 正文称之为 "microtask debounce"，但
setTimeout(…, 0)是 macrotask（在 microtask/promise 之后才跑），这也是为什么 StrictMode 那个测试的两次await Promise.resolve()观测不到它。
让 D2 与 #4119 共存，光加个 throttle 不够：切换模型（和取消）路径必须让
refreshStatic保持同步 / 在 handler 内。对每次刷新一刀切地setTimeout(0)满足不了这个要求。
⚠️ C1 —— 按 chunk 批量 STREAM_TEXT —— 逻辑没问题，但新测试空过
批处理本身没问题也进了 bundle（chunkThoughtText/chunkStreamText 已在 bundle 中核实；真实二进制流式输出了批量后的 thought+answer，见上方第三张图）。它合理地有助于 #2928。
但新加的测试根本没调用 AgentCore —— 它只是手动往一个裸 AgentEventEmitter emit 两个事件，然后断言它们按序到达。变异证明： 把 agent-core.ts 里的整段 batching 全部还原，"STREAM_TEXT batching (#2928)" 测试照样通过。它测的是 EventEmitter，不是批处理循环。真正的测试应当驱动 runInAgentFrames、喂一个多 part 的 chunk、断言 emit 次数下降（N→≤2）。
另外与上一轮一样：批处理会重排一个 chunk —— [A(text), T(thought), B(text)] 变成 [T, AB]（thought 被提前、非相邻 text 被合并）。累积的最终文本不受影响，但流式顺序变了；值得做个明确决定 + 补测试。
✅/➖ B1 & D3 —— 一致，但属启发式且部分未测
- B1（throttle
60→100ms）：内部一致 ——useGeminiStream.test.tsx152/152，=100已进 bundle。但它是频率旋钮，不是 终端界面无限滚动/刷新循环 #3838 重渲染循环的根因修复（throttle 只放慢循环，停不住反馈循环）。 - D3（resize settle
200→400ms）：useResizeSettleRepaint.test.ts6/6，但每个断言用的都是符号RESIZE_REPAINT_SETTLE_MS，所以没有测试钉住新值 —— 400 vs 200 是个未经测量的判断，还把 resize 后的重绘延迟翻倍。风险低，但没有证据表明 400 优于 200。
❌ A2 —— startTransition compact toggle —— 对 #4561 仍是改错了地方
与上一轮一致：A2 在 packages/web-shell/client/App.tsx（浏览器应用）。而 #4561 是 CLI（Ink）在 Windows 的 Ctrl+O 紧缩模式。CLI 的 toggle（AppContainer.tsx TOGGLE_COMPACT_MODE → 直接 setCompactMode）根本没动。A2（浏览器）和 C1（子 Agent 流式）都没碰 CLI 的 Ctrl+O 路径，所以 #4561 的关联仍无支撑。
真实二进制确认可启动并跑通一轮（Ctrl+O 确实会走 D2 改过的 refreshStatic —— remount 时 Tips 行会重新随机，见上方第三张图）。
证据
| 检查项 | 结果 |
|---|---|
| 构建 + bundle | ✅ exit 0；D2/C1/B1 均已进 dist/chunks/* |
CI Test (ubuntu) |
❌ Tests 4 failed —— AppContainer.test.tsx |
| D2 本地复现 | ❌ 4 failed / 91 passed，5 次确定性 —— 与 CI 一致 |
| D2 A/B | ✅ 还原 D2 → 95/95 通过；恢复 → 挂 4（承重） |
| D2 vs #4119 | ❌ 回退了同步 in-handler 保证（单帧历史 dump 风险） |
| D2 定时器清理 | setTimeout 无 unmount 清理 |
| C1 新测试 | ❌ 空过 —— 还原 batching 后照样通过（变异确认） |
| C1 重排 | [A,T,B] → [T,AB]（最终文本不变） |
B1 useGeminiStream |
✅ 152/152（值=100） |
| D3 resize | ✅ 6/6，但没测试钉住 400（相对符号） |
| A2 vs #4561 | ❌ web-shell ≠ CLI 面向 |
| 真实二进制（tmux） | ✅ 启动 v0.19.5，经 C1 路径流式 thought+answer |
（说明：真实 TTY 下会多出 7 个 Terminal Title 失败，但 D2 还原后的 A/B 显示这 7 个照样失败 → 是既有的 TTY flakiness，与本 PR 无关。 只有那 4 个 refreshStatic 失败才是 D2 造成的。）
结论
不宜合并 —— CI 红是因为 D2。 建议路径：
- D2： 别一刀切地推迟。切换模型和取消路径上让
refreshStatic保持同步（或把它们排除在 debounce 之外），以守住 chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed) #4119 的单次 commit 保证；加 unmount 清理；只有在同步契约保住之后再去更新那 4 个测试；修掉 "microtask" 措辞。 - C1： 让测试真正驱动
runInAgentFrames并断言 emit 次数下降；就"thought 先发"的重排做出决定并钉测试。 - B1/D3： 作为启发式没问题 —— 就按启发式来表述（别当根因修复），并去掉 D3 对未经测量常量的依赖，或补一个钉住意图的测试。
- A2： 移到 CLI 的 compact 路径，或撤掉 qwencode在紧缩模式下，ctrl+O开启后，执行任务会频繁闪屏。 #4561 的关联。
拆成按 issue 分的多个 PR，能让每项独立可验证，也能让好的部分（C1）不被 blocker（D2）拖住。
Method: isolated worktree build of cdc9399 (npm ci + build + bundle, exit 0) · real vitest aliased to src · D2 A/B (revert hunk → 95/95; restore → 4 fail, reproduced 5×, matches CI's 4) · the #4119 regression is argued from the production comment at AppContainer.tsx:985-999 (same-commit requirement + named #3899 freeze), i.e. from the documented mechanism, not a visual flicker repro · C1 mutation (revert batching → new test still green = vacuous) · 152/152 useGeminiStream · real dist/cli.js v0.19.5 in tmux against a fake OpenAI endpoint (thought+answer via C1 path, Ctrl+O static remount) · TTY-flake A/B isolates 7 pre-existing Terminal Title failures from the 4 D2 failures. Screenshots are rendered from the real terminal captures.
While a markdown table streams, the frontier line is often a half-typed row like `| a | b` with no closing `|` yet. Because TABLE_ROW_RE requires both a leading and trailing pipe, that partial line does not match, so the parser closed the table and rendered the partial as a plain text line below it — then, once the closing `|` arrived, flipped it into the table. This per-token flip changed the frame height and re-ran column autosizing on every keystroke, jittering the live table. Hold the partial row back instead: when pending, if the final line is an unterminated table row and at least one complete row already exists, skip it so `inTable` stays set and the end-of-content handler keeps rendering the accumulated rows as a live table. The row appears the moment it terminates. The `tableRows.length > 0` guard keeps the header + separator from blanking out while the very first row is still being typed. Note: this smooths the table content itself; it does not change the streaming repaint frequency, so the fixed bottom controls still repaint on each tick (that is the domain of the flicker-reduction work, e.g. QwenLM#5396). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e, hold back partial table rows (QwenLM#6340) * fix(cli): drop redundant "generating more" cue from the live preview In non-VP mode the live markdown preview is clipped to a rendered-height budget so the frame never overflows the viewport and triggers ink's scroll-to-top full redraw. It used to append a "... generating more ..." cue (and code/math/mermaid blocks appended their own) to signal that the clipped tail was still coming. Since QwenLM#6170 landed the incremental scrollback commit, that tail is streamed into <Static> in real time — clipped content is "still streaming" and reappears within a commit cycle, not "delayed output". The cue is therefore redundant noise that flickers in step with the commit cycle, so remove all four occurrences (outer preview clip, code block, mermaid block, math block). The row each cue used to occupy is reclaimed for content, so the total rendered height is unchanged: the code/math/mermaid RESERVED_LINES drop by one and the outer slice trigger switches from the (now-inlined) `clipped` flag to `keptLines < allLines.length`. The TableRenderer "… more rows streaming …" clamp is intentionally kept — an in-progress oversized table is not yet in scrollback, so that cue still carries information. Also gitignore the nested `.qwen/computer-use/` marker so the auto-generated artifact stops showing up as untracked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): hold back the unterminated table row while streaming While a markdown table streams, the frontier line is often a half-typed row like `| a | b` with no closing `|` yet. Because TABLE_ROW_RE requires both a leading and trailing pipe, that partial line does not match, so the parser closed the table and rendered the partial as a plain text line below it — then, once the closing `|` arrived, flipped it into the table. This per-token flip changed the frame height and re-ran column autosizing on every keystroke, jittering the live table. Hold the partial row back instead: when pending, if the final line is an unterminated table row and at least one complete row already exists, skip it so `inTable` stays set and the end-of-content handler keeps rendering the accumulated rows as a live table. The row appears the moment it terminates. The `tableRows.length > 0` guard keeps the header + separator from blanking out while the very first row is still being typed. Note: this smooths the table content itself; it does not change the streaming repaint frequency, so the fixed bottom controls still repaint on each tick (that is the domain of the flicker-reduction work, e.g. QwenLM#5396). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(cli): fix two stale "generating more" cue references in comments Review follow-up: two comments still referenced the removed outer cue. - TABLE_PENDING_RESERVED_ROWS: reword "marginY 2 + the outer cue" to "marginY 2 + one row of wrapped-cell safety headroom". The reserve stays at 3 on purpose — tables under-estimate their rendered height the most (wrapped cells), so they keep one more backstop row than the other blocks; lowering it would shrink that safety margin. - pending-rendered-height PendingSliceResult.keptLines JSDoc: drop the "plus a 'more' cue" phrasing — the caller now renders nothing rather than an oversized row. Comment-only; no behaviour change. 155 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): hold back the partial first table row too; de-dup reserve constant Review follow-up on the streaming table hold-back. - The `tableRows.length > 0` guard skipped the hold-back for the FIRST data row: a partial first row fell through to the table-closing branch, which also requires a row, so the header + separator were dropped and the partial rendered as a stray text line — the same per-token flip the change is meant to remove, just for the first row. Relax the guard to `tableHeaders.length > 0` so an unterminated first row/separator is held back too; the table is simply not drawn until its first row terminates, then pops in complete and grows one row at a time. Comment corrected to describe the actual behaviour. - Add a test for that edge case (partial first row held back, table appears once the row terminates). - De-duplicate the magic `3`: the slice-side `tableClampRows` estimate now references `TABLE_PENDING_RESERVED_ROWS` (moved to the top-of-file constants) instead of a literal, so the estimate and RenderTable's render-side `maxHeight` cap can never diverge. 157 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
@qwen-code /triage |
|
@qwen-code /resolve |
…ventEmitter import, drop duplicate ToolNames Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #5396ConflictFile: What happenedThe PR branch added two imports at the end of the import block: import { AgentEventEmitter, AgentEventType } from './agent-events.js';
import { ToolNames } from '../../tools/tool-names.js';Meanwhile, Resolution
ResultSingle clean import for |
|
|
||
| await act(async () => { | ||
| vi.advanceTimersByTime(60); | ||
| vi.advanceTimersByTime(100); |
There was a problem hiding this comment.
[Critical] Two vi.advanceTimersByTime(60) calls were missed when STREAM_UPDATE_THROTTLE_MS was bumped from 60 to 100. While 7 occurrences in this file were correctly updated, the calls at line 4446 and line 4513 (inside the streamContent helper) still advance timers by only 60ms — not enough to flush the 100ms throttle. Both tests fail: line 4446 produces expected 0 to be greater than 0 (pending items empty), and line 4513 produces expected 1 to be 25. This is likely the cause of the failing CI Test check.
Also, the comment at line 7308 still says STREAM_UPDATE_THROTTLE_MS (60ms) — stale after the constant change.
— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('AgentCore STREAM_TEXT batching (#2928)', () => { |
There was a problem hiding this comment.
[Suggestion] This test creates an AgentEventEmitter directly and calls emit() twice, verifying that the emitter dispatches events to listeners. However, the actual batching code lives in agent-core.ts:880-907 — the chunkThoughtText/chunkStreamText accumulation loop inside AgentCore's stream processing. This test never instantiates AgentCore or runs that loop, so it provides false confidence about the batching behavior.
A proper test would feed a multi-part chunk through the actual streaming code path (or extract the batch-and-emit logic into a testable function) and assert that fewer STREAM_TEXT events are emitted than there are parts, with correctly concatenated text.
— qwen3.7-max via Qwen Code /review
| remountStaticHistory(); | ||
| refreshStaticRef.current = setTimeout(() => { | ||
| refreshStaticRef.current = null; | ||
| if (!useTerminalBuffer) { |
There was a problem hiding this comment.
[Suggestion] The new ref-based debounce (clearTimeout + setTimeout(0)) introduces a cancellation contract: calling refreshStatic N times in rapid succession should execute remountStaticHistory() exactly once. No existing test covers this coalescing behavior. The closest test (fires refreshStatic only once per real model change) tests a different dedup mechanism (lastNotifiedModelRef), not the setTimeout cancellation.
Consider adding a test with vi.useFakeTimers(): call refreshStatic() twice without advancing timers, then vi.advanceTimersByTime(0), and assert remountStaticHistory ran exactly once.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
|
@qwen-code /resolve |
…ce throttle with transcript-open guard
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #5396ConflictFile: HEAD (PR branch
|
| remountStaticHistory(); | ||
| refreshStaticRef.current = setTimeout(() => { | ||
| refreshStaticRef.current = null; | ||
| if (!useTerminalBuffer) { |
There was a problem hiding this comment.
[Critical] setTimeout(0) defers the entire refreshStatic body (including setHistoryRemountKey inside remountStaticHistory()) to a macrotask. The onModelChange handler (line ~1183) calls refreshStatic() then setCurrentModel(model) synchronously — the comment block at lines 1158-1175 explicitly warns these MUST land in the same React commit or the #3899 freeze regression reoccurs.
This affects 3 call sites: onModelChange, handleCancelAndRewind (line ~2528), and conversation-rewind (line ~3211). All expect synchronous ordering with refreshStatic.
| if (!useTerminalBuffer) { | |
| if (!useTerminalBuffer) { | |
| stdout.write(ansiEscapes.clearTerminal); | |
| } | |
| remountStaticHistory(); |
Keep refreshStatic synchronous for direct callers. If debounce is needed for resize/Ctrl+O paths, extract it into a separate refreshStaticDebounced wrapper used only there.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| describe('AgentCore STREAM_TEXT batching (#2928)', () => { | ||
| it('batches thought and response text separately per chunk', () => { |
There was a problem hiding this comment.
[Critical] This test creates an AgentEventEmitter directly and calls emit() twice — bypassing the actual batching logic in agent-core.ts:899-937 (the chunkThoughtText/chunkStreamText accumulation loop). The emitter is a passthrough; this test would pass identically with the old per-part code.
A proper test should feed a chunk with multiple text parts through AgentCore and assert that thought and non-thought text are coalesced into exactly 2 events with concatenated text.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
agent-core.ts:919 |
Batching emits all thought text before all response text, destroying any interleaving within a chunk. Low probability with current Gemini API but a latent correctness issue. | Document the assumption or detect thought/non-thought transitions and flush at each boundary. |
App.tsx:2305 |
startTransition defers setCompactMode, so compactModeRef.current may be stale when a rapid second toggle fires or when the .catch rollback runs. |
Use compactModeRef.current in the catch instead of captured previous, or guard against subsequent toggles. |
AppContainer.tsx:1083 |
refreshStaticRef timer is never cleared on component unmount. Inconsistent with other timer-ref patterns in the file (e.g., transcript-close effect at line ~1148). |
Add useEffect(() => () => { if (refreshStaticRef.current) clearTimeout(refreshStaticRef.current); }, []); |
AppContainer.tsx |
No test verifies that calling refreshStatic() N times in rapid succession coalesces into one execution — the primary debounce behavior. |
Add a test: call refreshStatic() 3×, advance timers, assert only one clearTerminal + remountStaticHistory. |
useGeminiStream.test.tsx:4446,4513 |
5 tests fail because streamContent helper uses advanceTimersByTime(60) which never fires the 100ms throttle timer. |
Change to advanceTimersByTime(100) at both lines. |
| PR metadata | closingIssuesReferences is empty — none of the 7 claimed issues (#4561, #3838, #2928, #6137, #3979, #5971, #1184) will auto-close. Fix A2 targets web-shell, not CLI (#4561). #6137 root cause is missing synchronized output, not timing. |
Add Closes #NNNN keywords for genuinely fixed issues. Reassess #4561 and #6137 attribution. |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] CI still red — 9 test failures confirmed by running the suite:
AppContainer.test.tsx: 4 failures —setTimeout(0)defersrefreshStaticpast synchronous assertions (mockStdout.writenever called). Tests: "refreshStatic clears the terminal before remounting history", "auto-restores prompt on cancel", "fires refreshStatic in the same handler that updates currentModel", "fires refreshStatic only once per real model change".useGeminiStream.test.tsx: 5 failures in the Cancellation group — twoadvanceTimersByTime(60)calls remain at lines 4489 and 4556 (thestreamContentshared helper). Fixing line 4556 alone resolves 4 of the 5 failures.
All other code-level concerns have been covered in prior review rounds.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] refreshStatic is now deferred via setTimeout(0), but AppContainer.test.tsx has 3-4 tests that synchronously assert mockStdout.write after calling refreshStatic() — these will fail because the callback hasn't fired yet. Affected tests include "refreshStatic clears the terminal before remounting history", "fires refreshStatic in the same handler that updates currentModel", and the StrictMode dedup test. These tests either need fake timers with vi.advanceTimersByTime(0) after calling refreshStatic(), or the production code needs a synchronous bypass for callers that depend on same-commit batching.
— qwen3.7-max via Qwen Code /review
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #5396 fix(ui): reduce UI flicker — throttle + startTransition + batch STREAM_TEXT + debounce refresh + resize settle
Type: Bug Fix
Change size: +79/-23 across 7 files (8 commits)
HEAD: 7a09466
Findings Summary
- Critical/Major: 0 new (all previously raised by wenshao/ci-bot)
- Minor: 0 unique
- Nit: 2 unique items
Key Observations
This is a well-structured, focused flicker-fix PR with five independent, targeted changes. Each addresses a specific flicker scenario with an appropriate mechanism: throttle tuning (B1), React concurrent features (A2), event batching (C1), macrotask coalescing (D2), and settle-time tuning (D3). The changes are low-risk individually and the PR correctly maps each fix to its covered issues.
The core agent-core.ts batching change is correct: accumulating thought/response text per chunk and emitting at most 2 events (vs N per-part) reduces UI re-renders without losing information or changing total content. The AppContainer.tsx debounce correctly coalesces synchronous cascading refreshes via setTimeout(0).
The PR has active CHANGES_REQUESTED from wenshao and ci-bot. Key outstanding items: the setTimeout(0) same-commit invariant concern, the test coverage gap for the actual batching path, and the startTransition/compactModeRef staleness interaction.
Unique Findings
1. [Nit] PR body is outdated
Body claims "6 files changed, 51 insertions(+), 23 deletions(-)" and "zero conflict", but actual stats are 7 files, +79/-23, and mergeable: false. Consider updating the description to reflect the current state.
2. [Nit] "microtask debounce" terminology
The PR body describes D2 as "trailing-edge microtask debounce" but setTimeout(..., 0) schedules a macrotask, not a microtask. The behavioral difference matters: microtasks run before the next render (Promise.then), macrotasks run after. The current implementation (macrotask) is actually correct for the goal (coalesce within a React commit cycle), but the terminology is misleading.
Cross-Validation
| Finding | Other Reviewer | My Assessment |
|---|---|---|
setTimeout(0) breaks same-commit batching invariant |
ci-bot (Critical) | Confirmed — onModelChange calls refreshStatic() then setModelName() synchronously; deferring the former to a macrotask means the remount happens AFTER the model name state commits, potentially causing a flash of stale content |
startTransition + compactModeRef staleness |
wenshao (Suggestion) | Confirmed — rapid toggles before transition commits will derive next from stale ref; acceptable trade-off for preventing UI freeze on Windows but worth documenting |
| Test doesn't exercise actual batching path | ci-bot (Critical) | Confirmed — agent-core.test.ts creates an emitter and calls emit() directly, bypassing the chunkThoughtText/chunkStreamText accumulation in agent-core.ts:899-937 |
| Per-chunk batching reorders thought before response | wenshao (Suggestion) | Confirmed — original per-part emission preserved interleaving; new code emits ALL thought then ALL response. Benign for standard providers (thought always precedes response) but technically a semantic change |
refreshStaticRef no unmount cleanup |
ci-bot (Suggestion) | Confirmed — no useEffect cleanup; benign since AppContainer is root and rarely unmounts |
Missed advanceTimersByTime(60) calls |
ci-bot (Critical) | Not independently verified at current HEAD — the diff shows 7 correct updates; if additional 60ms calls exist for unrelated timers they may be intentionally unchanged |
| Unique-1: PR body outdated | — | Nit |
| Unique-2: microtask vs macrotask terminology | — | Nit |
Additional Audit Coverage
Areas I independently checked beyond existing findings:
- Event ordering correctness: Verified that
chunkThoughtTextis emitted beforechunkStreamText, preserving the dominant-case ordering (thought precedes response in standard reasoning models). - Debounce cancellation semantics: Verified
clearTimeout+setTimeoutpattern correctly implements trailing-edge coalescing for synchronous call bursts. useCallbackdependency array:refreshStaticdeps[useTerminalBuffer, remountStaticHistory, stdout]correctly capture all closed-over values.- Rollback path in handleToggleCompact: The
.catchhandler callssetCompactMode(previous)directly (not in startTransition), correctly making the rollback urgent. - Throttle change impact: 60→100ms reduces streaming update frequency from ~16.7fps to 10fps — acceptable trade-off for flicker reduction, imperceptible for text streaming.
- Resize settle 200→400ms: Combined with the existing debounce mechanism (clearTimeout + re-set on each resize event), the 400ms only applies AFTER the last resize event, so continuous resizing doesn't accumulate delay.
Final Verdict
Needs Changes — deferring to wenshao's and ci-bot's active CHANGES_REQUESTED. My independent review found no Critical/Major issues; the five changes are individually sound and correctly targeted. The blocking items are the previously-raised concerns about the setTimeout(0) same-commit invariant and the test coverage gap for the batching path.
This review was generated by QoderWork AI
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Critical] packages/cli/src/ui/hooks/useGeminiStream.test.tsx:4489,4556 — Two vi.advanceTimersByTime(60) calls were missed when STREAM_UPDATE_THROTTLE_MS was bumped from 60 to 100. The streamContent helper at line 4556 and the direct call at line 4489 still advance timers by only 60ms, insufficient to flush the 100ms throttle. Confirmed by CI: 5 test failures in useGeminiStream.test.tsx.
— qwen3.7-max via Qwen Code /review
| refreshStaticRef.current = setTimeout(() => { | ||
| refreshStaticRef.current = null; | ||
| if (!useTerminalBuffer) { | ||
| stdout.write(ansiEscapes.clearTerminal); | ||
| } | ||
| remountStaticHistory(); | ||
| }, 0); |
There was a problem hiding this comment.
[Critical] setTimeout(0) defers remountStaticHistory() (which calls setHistoryRemountKey) to a macrotask, breaking the same-commit batching invariant documented at lines 1155–1175 of this file. The onModelChange handler (line 1183) calls refreshStatic() then setCurrentModel(model) synchronously — previously both state updates landed in the same React commit. Now setCurrentModel commits first with the OLD historyRemountKey, producing a full-history Static render that bypasses progressive replay — the exact #3899 freeze regression that PR #4119 fixed.
Additionally, refreshStaticRef has no useEffect cleanup on unmount — a pending timer can fire setHistoryRemountKey on an unmounted component.
— Failure scenario: user changes model (Ctrl+M or /model) → refreshStatic() schedules macrotask → setCurrentModel commits immediately → <Static> renders with new model but old remount key → full-history flash / freeze.
| refreshStaticRef.current = setTimeout(() => { | |
| refreshStaticRef.current = null; | |
| if (!useTerminalBuffer) { | |
| stdout.write(ansiEscapes.clearTerminal); | |
| } | |
| remountStaticHistory(); | |
| }, 0); | |
| if (!useTerminalBuffer) { | |
| stdout.write(ansiEscapes.clearTerminal); | |
| } | |
| remountStaticHistory(); |
Keep refreshStatic synchronous. If debounce is needed for the resize path, extract a separate refreshStaticDebounced wrapper used only by those callers.
— qwen3.7-max via Qwen Code /review
| it('batches thought and response text separately per chunk', () => { | ||
| const emitter = new AgentEventEmitter(); | ||
| const events: Array<{ text: string; thought: boolean }> = []; | ||
| emitter.on(AgentEventType.STREAM_TEXT, (e) => { | ||
| events.push({ text: e.text, thought: e.thought ?? false }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] This test creates a bare AgentEventEmitter and calls emit() twice directly — it tests the event emitter's passthrough behavior, not the batching logic added in agent-core.ts:902–936. The production batching (accumulating chunkThoughtText/chunkStreamText across multiple parts in a chunk, emitting once per type) is never exercised.
— Concrete cost: if a future refactor breaks the batching (e.g. reverts to per-part emission, or concatenates thought + response text into one emit), this test still passes. It provides zero regression protection for the change it claims to verify.
| it('batches thought and response text separately per chunk', () => { | |
| const emitter = new AgentEventEmitter(); | |
| const events: Array<{ text: string; thought: boolean }> = []; | |
| emitter.on(AgentEventType.STREAM_TEXT, (e) => { | |
| events.push({ text: e.text, thought: e.thought ?? false }); | |
| }); | |
| describe('AgentCore STREAM_TEXT batching (#2928)', () => { | |
| it('coalesces multiple same-type text parts into one emit per chunk', () => { | |
| // Drive AgentCore's streaming loop with a mock ContentGenerator | |
| // that returns a chunk with 2 thought parts + 1 response part, | |
| // then assert exactly 2 STREAM_TEXT events (1 thought, 1 response) | |
| // with concatenated text. | |
| }); | |
| }); |
— qwen3.7-max via Qwen Code /review
|
@qwen-code /resolve |
…g, adopt upstream removal of resize-settle repaint Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution summary (PR #5396 ← main)Root cause
Semantic (same logic modified on both sides)
const refreshStatic = useCallback(() => {
if (refreshStaticRef.current) clearTimeout(refreshStaticRef.current);
refreshStaticRef.current = setTimeout(() => {
refreshStaticRef.current = null;
if (!useTerminalBuffer) stdout.write(ansiEscapes.clearTerminal);
remountStaticHistory();
}, 0);
}, [useTerminalBuffer, remountStaticHistory, stdout]);
Load-bearing
Could not verifyNo build/typecheck/tests run. The batching reorders within a chunk (all thought text emits before all response text; upstream interleaved in part order) — intentional per the PR, but 中文说明冲突根源：main 删除了 transcript（备用屏）功能，将 解决（语义合并）： 关键约束：批量事件必须携带 |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] D2 — refreshStatic now wraps BOTH stdout.write(clearTerminal) and remountStaticHistory() (which calls setHistoryRemountKey) in setTimeout(0) (AppContainer.tsx:1311). The config.onModelChange handler (AppContainer.tsx:~1349) still calls refreshStatic(); setCurrentModel(model); synchronously, but the invariant documented directly below it (AppContainer.tsx:1321-1342, added for PR #4119) requires setHistoryRemountKey and setCurrentModel to land in the SAME React commit — otherwise <Static key={${historyRemountKey}-${currentModel}}> remounts with the OLD remount key and bypasses progressive replay, i.e. the issue #3899 full-history freeze regression this code was written to prevent. With setTimeout(0), setCurrentModel commits first and setHistoryRemountKey lands a macrotask late. Confirmed at this head by test execution: npx vitest run src/ui/AppContainer.test.tsx -t "Model change refreshStatic wiring" → 2 failures (fires refreshStatic in the same handler that updates currentModel, fires refreshStatic only once per real model change (StrictMode-safe)). This is already tracked in the open inline comment at AppContainer.tsx:1311 and STILL STANDS at the current head. Fix: keep refreshStatic synchronous on the model-change path (or move setCurrentModel inside the deferred callback) so the two state updates share a commit.
— qwen3.8-max-preview via Qwen Code /review
| ToolNames.NOTEBOOK_EDIT, | ||
| ]); | ||
| const STREAM_UPDATE_THROTTLE_MS = 60; | ||
| const STREAM_UPDATE_THROTTLE_MS = 100; |
There was a problem hiding this comment.
[Critical] Bumping STREAM_UPDATE_THROTTLE_MS 60 → 100 requires updating every vi.advanceTimersByTime(60) in useGeminiStream.test.tsx, but three were missed — lines 6343, 6435, and 6502. Those sites feed tests that flush the buffered stream only via the throttle timer (the stream is held open, so no Finished event triggers an early flush); advancing 60 ms no longer reaches the 100 ms deadline, so flushBufferedStreamEvents() never fires. — Failure scenario: npx vitest run src/ui/hooks/useGeminiStream.test.tsx reports 8 failed | 175 passed (e.g. expected 0 to be greater than 0, expected 1 to be 25). This breakage was reintroduced by the latest merge of main — the prior head had these passing. Fix: change the three remaining vi.advanceTimersByTime(60) calls (6343, 6435, 6502) to (100), or reference STREAM_UPDATE_THROTTLE_MS directly so future bumps don't require hunting through the test file.
— qwen3.8-max-preview via Qwen Code /review
8 participants
What
Five targeted changes to eliminate UI flicker across all reported scenarios:
B1 — throttle 60→100ms
useGeminiStream.ts:STREAM_UPDATE_THROTTLE_MS60 → 100useGeminiStream.test.tsx: 7 test timers updated accordinglyA2 — compactMode toggle via startTransition
App.tsx: wrapsetCompactModeinstartTransitionto avoid blocking the UI thread on WindowsC1 — batch STREAM_TEXT per chunk
agent-core.ts: accumulate text parts in a chunk, emit once per chunk instead of per-partD2 — debounce refreshStatic
AppContainer.tsx: trailing-edge microtask debounce so rapid cascading refreshes (model change + settings + compact toggle) coalesce into one clearTerminal + remountD3 — increase resize settle time
useResizeSettleRepaint.ts:RESIZE_REPAINT_SETTLE_MS200 → 400msCovered Issues
Files