Skip to content

fix(cli): allow clicking to expand/collapse thought while streaming - #8443

Draft
chiga0 wants to merge 14 commits into
QwenLM:mainfrom
chiga0:fix/thinking-pending-expand-click
Draft

fix(cli): allow clicking to expand/collapse thought while streaming#8443
chiga0 wants to merge 14 commits into
QwenLM:mainfrom
chiga0:fix/thinking-pending-expand-click

Conversation

@chiga0

@chiga0 chiga0 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Previously, the thought (thinking) block could only be expanded or collapsed by mouse click after the model finished thinking. While the response was still streaming (pending state), the click handler was disabled and the expand/collapse hint was hidden. This PR removes that restriction so that users can click to expand a streaming thought to watch the reasoning live, and collapse it again — the same toggle behavior that was already available via the keyboard shortcut.

The collapsed-state hint line is now also shown while pending (with the animated pending icon and elapsed duration), and the collapse hint is shown while pending and expanded. The "click or <key> to expand" wording only includes "click" when the terminal is in virtual-viewport mode where clicking is actually supported.

Why it's needed

When a model is reasoning for a long time, users often want to peek at the thinking content as it streams rather than waiting for completion. The keyboard shortcut already worked during streaming, but mouse users had no way to expand until the thought was committed. Making the block clickable while pending gives a consistent, live-watchable experience.

Reviewer Test Plan

How to verify

  1. Start a query that produces a long thinking block (any prompt works).
  2. While the "Thinking…" indicator is streaming, click on it in the terminal.
  3. Expected: the thought expands and the reasoning streams live. The hint changes to "collapse". Click again to collapse.
  4. After the thought completes, click behavior is unchanged (still toggles).

Automated coverage: unit tests assert that the click handler stays active while pending, that a full press+release toggles a pending thought, that the expand hint is shown while pending (with/without the "click" wording depending on virtual-viewport mode), and that the collapse hint appears while pending and expanded.

Test command: cd packages/cli && npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/messages/ConversationMessages.test.tsx — 57 tests pass.

Evidence (Before & After)

Before: while streaming, the thought showed "Thinking…" with no expand hint and ignored mouse clicks.
After: while streaming, the thought shows "Thinking…(Ns) (click or A to expand)" and responds to clicks.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

npm run dev on macOS; unit tests via vitest.

Risk & Scope

  • Main risk or tradeoff: the click handler is now active during streaming; if a user is selecting text by dragging over the thought line, the existing drag-suppression logic still prevents accidental toggles.
  • Not validated / out of scope: behavior on Windows/Linux terminals not locally tested; no changes to keyboard shortcut path.
  • Breaking changes / migration notes: none.

Linked Issues

N/A

中文说明

这个 PR 做了什么

此前,思考(thinking)块只有在模型思考完成后才能通过鼠标点击展开/收起。在响应仍在流式输出(pending 状态)时,点击处理被禁用,展开/收起提示也被隐藏。本 PR 移除了该限制,用户可以在思考流式输出过程中点击展开以实时观看推理内容,也可以再次点击收起——与键盘快捷键已有的切换行为一致。

收起状态的提示行现在在 pending 时也会显示(带动画 pending 图标和已用时长),pending 且展开时会显示收起提示。"click or <key> to expand" 的措辞仅在终端处于支持点击的虚拟视口模式时才包含 "click"。

为什么需要

当模型长时间推理时,用户往往希望在流式输出过程中就查看思考内容,而不是等待完成。键盘快捷键在流式输出期间本就能用,但鼠标用户在思考提交前无法展开。让 pending 状态的块可点击,提供了一致的、可实时观看的体验。

审阅者测试计划

如何验证

  1. 发起一个会产生较长思考块的查询(任意提示词均可)。
  2. 在 "Thinking…" 指示器流式输出期间,在终端中点击它。
  3. 预期:思考展开并实时流式显示推理内容,提示变为 "collapse"。再次点击可收起。
  4. 思考完成后,点击行为不变(仍可切换)。

自动化覆盖:单元测试断言了点击处理在 pending 时保持激活、完整的按下+释放可切换 pending 思考、展开提示在 pending 时显示(根据虚拟视口模式决定是否含 "click" 措辞)、以及 pending 且展开时显示收起提示。

测试命令:cd packages/cli && npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/messages/ConversationMessages.test.tsx —— 57 个测试通过。

证据(前后对比)

之前:流式输出期间,思考块显示 "Thinking…",无展开提示且忽略鼠标点击。
之后:流式输出期间,思考块显示 "Thinking…(Ns) (click or A to expand)" 并响应点击。

测试环境

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

macOS 上 npm run dev;单元测试用 vitest。

风险与范围

  • 主要风险或权衡:点击处理现在在流式输出期间激活;若用户在思考行上拖拽选择文本,已有的拖拽抑制逻辑仍会防止误触发切换。
  • 未验证 / 超出范围:未在 Windows/Linux 终端本地测试;键盘快捷键路径无改动。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

@chiga0

chiga0 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on 73533ec and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— 73533ec 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Aug 3, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed, not theoretical. The base code corroborates it directly: ClickableThinkMessage sets isActive = !isPending (the click subscription is torn down while streaming), and ThinkMessage's collapsed-hint branch is gated on !isPending && !expanded — so a pending thought shows no expand hint and ignores the mouse, while the Ctrl+O / Alt+T keyboard toggle already expands pending thoughts. A real, trivially reproducible parity gap. No linked issue, but the before/after description plus the code state is sufficient evidence.

Direction: aligned — a TUI usability fix for the thinking block, squarely in the CLI's core experience. Claude Code's CHANGELOG has no exact match for click-to-watch-streaming-thinking, but that area is actively maintained there (thinking status-row rendering, elapsed-time updates), so the direction is consistent.

Size: no core paths touched — everything is in packages/cli/src/ui/**. ~22 production lines (12 added / 10 removed) vs ~83 test lines. Well under every threshold; no maintainer awareness needed.

Approach: the scope feels right. The change is the minimum possible — it removes the two isPending gates instead of adding new machinery, reuses the existing press/release handler (with its drag-suppression), hint strings, and pending icon, and adds focused tests including one asserting the VP gate stays intact (bypassVpGate remains false). No unrelated edits in the diff.

Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的问题,不是理论性的。基础代码直接印证:ClickableThinkMessage 设置 isActive = !isPending(流式输出期间点击订阅被释放),且 ThinkMessage 的收起提示分支以 !isPending && !expanded 为前提——因此 pending 思考不显示展开提示且忽略鼠标,而 Ctrl+O / Alt+T 键盘切换本就可以展开 pending 思考。这是一个真实、极易复现的交互不对等问题。虽然没有关联 issue,但前后行为描述加上代码现状已足够作为证据。

方向:对齐——思考块的 TUI 易用性修复,属于 CLI 核心体验。Claude Code 的 CHANGELOG 没有与"点击实时观看流式思考"完全对应的条目,但该领域在那里维护活跃(思考状态行渲染、耗时更新),方向一致。

规模:未触及核心路径——全部在 packages/cli/src/ui/**。约 22 行生产代码(增 12 / 删 10),约 83 行测试。远低于所有阈值,无需维护者关注。

方案:范围合理。改动已是最小——移除两处 isPending 门控而非新增机制,复用既有的按下/释放处理器(含拖拽抑制)、提示文案和 pending 图标,并补充了针对性测试(包括断言 VP 门控保持不变,即 bypassVpGate 仍为 false)。diff 中没有无关改动。

风险:无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at 73533ecfcdc90143bbb6277c8e99831abbc558fb · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Code review

I reconstructed the minimal fix from the problem statement before reading the diff: the behavior is exactly two isPending gates — the mouse subscription (isActive = !isPending in ClickableThinkMessage) and the collapsed-hint branch (!isPending && !expanded in ThinkMessage) — so lifting those two gates is the whole job. That is precisely what this PR does; I didn't find a simpler path it missed.

What I checked along the way:

  • No change to terminal-mode gating. useMouseEvents now receives isActive: true, but the internal gates are untouched: the VP gate (no bypassVpGate — the new test pins it to false), the ui.mouseTracking setting, raw mode, and the TTY check. Outside VP mode the subscription still never activates, so no wheel-hijacking regression on the non-VP main screen.
  • Drag suppression carries over unchanged. The handler is isPending-agnostic — press and release must land on the same cell, and any move clears the press. The new pending-click test and the existing drag test exercise the same code path; a dedicated pending-drag test is absent, but there is no pending-specific branch to cover. Not a blocker.
  • Visual continuity. The pending collapsed line reuses exactly what the pending expanded view already rendered — Thinking…(Ns) with THINKING_ICON_PENDING — and the "click" wording appears only when clickable (VP mode + mouse tracking). The collapse hint now shows while pending-and-expanded, matching the new affordance.
  • No other consumers. ThinkMessage is only rendered through ClickableThinkMessage (grep-verified); surfaces that force expansion (e.g. SessionPreview) take the expanded branch and are unaffected.
  • The renamed test (should advertise expansion while pending) flips its old assertion — that flip is the intended behavior change itself, not coverage being watered down.

No critical findings, no convention violations.

CI evidence (PR's own checks on the reviewed commit, fetched via API)

The main ubuntu unit suite is still running (it takes ~30 minutes — not polling). The macOS / Windows / integration checks are skipped by design: ci.yml runs them only in the merge queue (merge_group event), so a skipped report under their exact names is the expected PR state, not a failure. Everything else that ran is green; no failing job to excerpt.

Final CI results for 73533ec (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
route ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle the one claim static review can't: @qwen-code /tmux — whether a real terminal click expands a streaming thought end-to-end (live SGR mouse events through a re-rendering Ink tree) is not observable from the diff, and the unit tests mock useMouseEvents and position measurement. The author has write access, so a maintainer can trigger it directly.

Not verified: live click behaviour in a real terminal (unattended CI run — no local tmux pass here); Windows/Linux terminals (author tested on macOS only — the author's claim, not independently re-run).

中文说明

代码审查:先根据问题描述独立推导最小修复——该行为恰好由两处 isPending 门控造成(ClickableThinkMessageisActive = !isPendingThinkMessage!isPending && !expanded),移除这两处门控即达成目标。PR 的实现与此完全一致,没有遗漏更简单的路径。

核对过的点:终端模式门控未变(VP 门控、ui.mouseTracking 设置、raw 模式、TTY 检查均保持原样,新测试还断言 bypassVpGate 保持 false,非 VP 主屏不会出现滚轮劫持回归);拖拽抑制逻辑原样沿用——按下与释放必须落在同一格、任何移动都会取消按压,新的 pending 点击测试与既有拖拽测试走的是同一条无 pending 分支的代码路径,缺一个 pending 拖拽专项测试但不构成阻塞;pending 收起行复用了展开视图本就在渲染的 Thinking…(Ns)THINKING_ICON_PENDING 图标,"click" 措辞仅在 clickable(VP 模式 + 鼠标追踪)时出现,pending 且展开时显示收起提示与新的可点击能力一致;ThinkMessage 只经由 ClickableThinkMessage 渲染(已 grep 验证),强制展开的界面走展开分支不受影响;被改名的测试(should advertise expansion while pending)翻转了旧断言——该翻转就是本次有意的行为变更本身,不是削弱覆盖。

无阻塞性问题,无规范违规。

CI 证据(通过 API 获取的被审提交上的 PR 自身检查):ubuntu 主测试套件仍在运行(约 30 分钟,不轮询等待);macOS / Windows / 集成测试按 ci.yml 设计仅在合并队列(merge_group 事件)运行,PR 阶段以其确切名称报告"跳过"属预期状态而非失败;其余已完成的检查全部通过,没有失败任务可摘录。上方表格区域可被 finalize 工作流在 CI 结束后原地更新。

沙箱验证可补齐静态审查看不到的唯一论断:@qwen-code /tmux——真实终端点击能否端到端展开流式思考(重渲染中的 Ink 树接收实时 SGR 鼠标事件)无法从 diff 看出,且单元测试 mock 了 useMouseEvents 与位置测量。作者有写权限,维护者可直接触发。

未验证:真实终端中的实际点击行为(本次为 CI 无人值守运行,不做本地 tmux 测试);Windows/Linux 终端(作者仅在 macOS 上测试——这是作者的自述,不是独立复测结果)。

Qwen Code · qwen3.8-max

Reviewed at 73533ecfcdc90143bbb6277c8e99831abbc558fb · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal, well-tested parity fix; the only reservation is that the live click path is exercised through mocked unit tests, so end-to-end behaviour rests on the /tmux lane named above.

Honest read: this is the kind of PR that's easy to approve for the right reasons. The problem is real and verified in the base code (two isPending gates), the fix is the minimum possible — remove them and reuse everything else — the diff carries nothing unrelated, and the tests pin not just the new behavior but also the unchanged VP gate. My independent proposal matched the implementation essentially line for line, which is rare and a good sign. What keeps this at 4 rather than 5: the unit tests mock the mouse plumbing, so a real-terminal click through a live, re-rendering tree is exactly what the suite cannot prove, and the author has tested on macOS only. Neither point blocks merge — the named sandboxed lane covers both if a maintainer wants belt-and-suspenders.

CI is still running (the ubuntu unit suite), so approval is deferred until CI lands green on 73533ecfcdc90143bbb6277c8e99831abbc558fb.

中文说明

置信度:4/5 —— 干净、最小化、测试充分的交互对等修复;唯一保留是真实点击路径由 mock 的单元测试覆盖,端到端行为依赖上文指出的 /tmux 通道。

直接说:这个 PR 值得以正确的理由被批准。问题真实存在且已在基础代码中核实(两处 isPending 门控);修复是最小可能——移除门控、其余全部复用;diff 无无关改动;测试不仅钉住新行为,还钉住了未变的 VP 门控。我独立推导的方案与实现基本逐行一致,这很少见,也是好迹象。给 4 分而不是 5 分的原因:单元测试 mock 了鼠标管道,真实终端中穿透实时重渲染树的点击恰恰是测试套件无法证明的,且作者只在 macOS 上测试过。两点都不阻塞合并——如果维护者想加双保险,指定好的沙箱通道可以覆盖这两点。

CI 仍在运行(ubuntu 单元测试套件),因此批准推迟到 CI 在被审提交上变绿之后。

Qwen Code · qwen3.8-max

Reviewed at 73533ecfcdc90143bbb6277c8e99831abbc558fb · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

LGTM, looks ready to ship — CI landed green after the review. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Test Plan (not a blocker): 57 tests pass — this review observed 17023 passed.

中文说明

Test Plan(非阻断):57 tests pass — this review observed 17023 passed

— qwen3.8-max via Qwen Code /review (v0.21.4)

Comment on lines +175 to +177
// Active while pending too: clicking a streaming thought expands it so the
// reasoning can be watched live (and collapsed again).
{ isActive: true },

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] Enabling the click handler while pending makes toggleThought(thoughtGroupHeadId) reachable for streaming thoughts, but MainContent renders every pending item as item={{ ...item, id: 0 }} without a thoughtHeadId (VP path MainContent.tsx:382, Static pending path :544), so thoughtGroupHeadId = thoughtHeadId ?? item.id resolves to the shared sentinel 0, and expandedThoughtHeadIds is session-lifetime state that is never cleared or migrated.

Failure scenario: the user clicks to expand a streaming thought → toggleThought(0). When the thought commits (commitPendingThought re-adds it under a real baseTimestamp + counter id, never 0), expandedHeadIds.has(realHeadId) is false → the thought the user expanded snaps collapsed the moment streaming finishes, defeating this PR's stated purpose. The stuck 0 also flips the initial state of every subsequent pending thought in the session. A third manifestation: reasoning exceeding STREAM_PENDING_ITEM_MAX_CHARS (16 384) commits its head chunk under a real id while the tail stays pending keyed off 0 — expanding the committed head leaves the live streaming tail invisible. The new tests render HistoryItemDisplay directly with thoughtItem.id = 1, bypassing the id: 0 remap, so they pass while production loses the state (probe-verified through the real MainContent render path, with a flip-check against the base).

Suggested fix: thread the thought-group head id through the pending render path (pass thoughtHeadId for pending items in MainContent) and migrate the pending expansion entry to the committed head id when the thought commits. A fix that only clears key 0 at commit still leaves split long thoughts broken.

中文说明

[Critical] 在 pending 时启用点击处理后,流式思考块会触发 toggleThought(thoughtGroupHeadId),但 MainContent 把每个 pending 项渲染为 item={{ ...item, id: 0 }} 且不传 thoughtHeadId(VP 路径 MainContent.tsx:382 与 Static pending 路径 :544 均如此),因此 thoughtGroupHeadId = thoughtHeadId ?? item.id 解析为共享哨兵值 0,而 expandedThoughtHeadIds 是会话级状态,从不清理或迁移。

失败场景:用户点击展开流式思考 → toggleThought(0)。思考提交时(commitPendingThought 以真实的 baseTimestamp + counter id 重新添加,永不为 0),expandedHeadIds.has(realHeadId) 为 false → 用户展开的思考块在流式结束的瞬间自动折叠,与本 PR 的目标直接冲突。残留的 0 还会翻转本会话中后续每个 pending 思考块的初始状态。第三种表现:推理文本超过 STREAM_PENDING_ITEM_MAX_CHARS(16 384)时,头部块以真实 id 提交,而尾部仍以 0 为键处于 pending——展开已提交的头部后,正在流式输出的尾部却不可见。新增测试直接以 thoughtItem.id = 1 渲染 HistoryItemDisplay,绕过了 id: 0 重映射,因此测试通过而生产环境丢失状态(已通过真实 MainContent 渲染路径的探针验证,并对 base 做了翻转对照)。

建议修复:将思考组的 head id 贯通到 pending 渲染路径(在 MainContent 中为 pending 项传入 thoughtHeadId),并在思考提交时把 pending 展开记录迁移到提交后的 head id——仅在提交时清理 0 键仍会导致被切分的长思考块失步。

— qwen3.8-max via Qwen Code /review (v0.21.4)

Comment on lines +348 to +350
const collapseHint = ` ${t('({{keyHint}} to collapse)', {
keyHint: toggleKeyHint,
})}`;

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 collapse hint now shown while a pending thought is expanded advertises ctrl+o, but ctrl+o toggles the app-wide full-detail switch (setThoughtExpanded in AppContainer), not the per-item expandedThoughtHeadIds set — it cannot collapse a thought that was expanded by clicking.

Failure scenario: the user clicks a streaming thought to expand it; the header reads "∵ Thinking… (ctrl+o to collapse)". Pressing ctrl+o force-expands every thought and tool group in the session (the opposite of the advertised effect); pressing it again returns to per-item state, which still contains the clicked key — so the block remains expanded. Only a second mouse click collapses it. This mismatch pre-exists for committed click-expanded thoughts, but this diff newly extends the hint to pending thoughts, where clicking is the primary expansion path.

Suggested fix: make the hint accurate for the click-expanded state — e.g. mention clicking again to collapse when clickable.

中文说明

[Suggestion] pending 思考块展开时显示的收起提示写着 ctrl+o,但 ctrl+o 切换的是应用级的 full-detail 开关(AppContainer 中的 setThoughtExpanded),而不是逐项的 expandedThoughtHeadIds 集合——它无法收起通过点击展开的思考块。

失败场景:用户点击流式思考块将其展开,头部显示 "∵ Thinking… (ctrl+o to collapse)"。按下 ctrl+o 会强制展开会话中的所有思考块和工具组(与提示效果相反);再按一次回到逐项状态,但其中仍包含被点击的键——该块依然保持展开。只有再次鼠标点击才能收起。这一错配在已提交的、通过点击展开的思考块上本就存在,但本 diff 首次将该提示扩展到 pending 思考块,而点击正是其主要展开途径。

建议修复:让提示在点击展开的状态下保持准确——例如在 clickable 时提示再次点击可收起。

— qwen3.8-max via Qwen Code /review (v0.21.4)

Comment on lines +331 to +333
const label = isPending
? `${t('Thinking')}…${durationSuffix}`
: (completedLabel ?? t('Thinking'));

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 collapsed branch added by this diff duplicates the expanded branch's pending-label arm — `${t('Thinking')}…${durationSuffix}` appears at both ~line 332 and ~line 346 — and the pending-icon ternary {isPending ? THINKING_ICON_PENDING : THINKING_ICON} also appears twice (~line 339 and ~line 355). Before this PR a pending thought always fell through to the expanded branch, so the pending header was computed in exactly one place; now both branches are live during streaming.

Concrete cost: any change to the streaming header (wording, duration suffix, animated icon/spinner) must be made in two verbatim copies ~15 lines apart; missing one makes the collapsed and expanded states visibly disagree on every toggle during streaming.

Suggested fix: hoist the shared pieces above the if (!expanded) early return — e.g. const pendingLabel = ${t('Thinking')}…${durationSuffix}; and const headerIcon = isPending ? THINKING_ICON_PENDING : THINKING_ICON; — and use them in both branches (the committed fallbacks differ deliberately and should stay separate).

中文说明

[Suggestion] 本 diff 新增的 collapsed 分支复制了 expanded 分支的 pending 标签臂——`${t('Thinking')}…${durationSuffix}` 同时出现在约第 332 行与约第 346 行——pending 图标三元表达式 {isPending ? THINKING_ICON_PENDING : THINKING_ICON} 也出现两次(约第 339 行与约第 355 行)。在本 PR 之前,pending 思考块总是落入 expanded 分支,pending 头部只在一处计算;现在两个分支在流式输出期间都会被走到。

具体代价:对流式头部的任何改动(措辞、时长后缀、动画图标/spinner)都必须在相隔约 15 行的两份逐字拷贝中同步修改,漏改其一会导致收起与展开状态在用户流式切换时肉眼可见地不一致。

建议修复:把共享部分提升到 if (!expanded) 提前返回之前——例如 const pendingLabel = ${t('Thinking')}…${durationSuffix};const headerIcon = isPending ? THINKING_ICON_PENDING : THINKING_ICON;——并在两个分支中复用(committed 的兜底文案有意不同,应保持分开)。

— qwen3.8-max via Qwen Code /review (v0.21.4)

Comment on lines +175 to +177
// Active while pending too: clicking a streaming thought expands it so the
// reasoning can be watched live (and collapsed again).
{ isActive: true },

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] onToggle calls toggleThought(thoughtGroupHeadId) unconditionally, with no full-detail guard: while fullDetail is active (resolvedThoughtExpanded = fullDetail || (...)), a click on a pending thought is a visual no-op that silently records a per-item toggle.

Failure scenario: with Ctrl+O full-detail on, the user clicks a streaming thought's header to collapse just that stream — nothing visibly changes, but the sentinel id is added to expandedThoughtHeadIds. When the user later exits full-detail via Ctrl+O (which the on-screen collapse hint advertises), everything else collapses but this thought stays expanded — the opposite of the two collapse attempts. The entry persists for the session. Pre-PR this was unreachable while pending (isActive = !isPending plus a detached ref); this diff newly enables it (the committed-thought variant predates the PR and is out of scope).

Suggested fix (probe-verified to flip the behavior): onToggle={fullDetail ? () => {} : () => toggleThought(thoughtGroupHeadId)}

中文说明

[Suggestion] onToggle 无条件调用 toggleThought(thoughtGroupHeadId),没有 full-detail 守卫:在 fullDetail 激活时(resolvedThoughtExpanded = fullDetail || (...)),点击 pending 思考块在界面上毫无变化,却会悄悄记录一次逐项切换。

失败场景:在 Ctrl+O full-detail 开启时,用户点击某个流式思考块的头部想只收起它——界面没有任何变化,但哨兵 id 已被加入 expandedThoughtHeadIds。当用户稍后再次按 Ctrl+O 退出 full-detail(屏幕上的收起提示正是这样引导的)时,其他内容都收起了,这个思考块却保持展开——与用户两次收起操作的意图相反。该记录在整个会话期间保留。PR 之前 pending 时无法触发(isActive = !isPending 且 ref 未挂载),本 diff 使其首次可达(committed 思考块的同类问题在本 PR 之前就存在,不在本次范围内)。

建议修复(已经探针验证可翻转该行为):onToggle={fullDetail ? () => {} : () => toggleThought(thoughtGroupHeadId)}

— qwen3.8-max via Qwen Code /review (v0.21.4)

@qwen-code-dev-bot

qwen-code-dev-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

⚠️ AutoFix round 13 ended without publishing a reportview run.

中文说明

⚠️ AutoFix 第 13 轮结束但未发布报告 —— 查看运行

In mapToDisplay's non-'error' branch every TrackedToolCall variant carries a required tool and invocation; the 'error' variant that lacks them is already excluded by the discriminant above. Assert non-null so tsc narrows cleanly. Pre-existing error that blocked npm run typecheck.
Address review feedback on PR QwenLM#8443:
- Key a pending thought's click expansion off a dedicated sentinel and migrate
  it to the real committed head id when the thought commits, so a thought
  expanded while streaming stays expanded instead of snapping collapsed. A
  split (long) thought's pending tail keys off the already-committed head id.
- Collapse hint now advertises clicking when the thought is clickable, since
  ctrl+o is the app-wide full-detail switch and cannot collapse a
  click-expanded thought.
- Hoist the shared pending header label/icon so the collapsed and expanded
  branches no longer duplicate them.
- Ignore clicks while ctrl+o fullDetail is active so they don't silently
  record a per-item toggle.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下:

All four review findings are addressed and resolved in the code. No conflicts (--conflict false), no base merge needed.

Findings

R1-1 — Critical: pending-thought expansion keyed off the shared 0 sentinel (rc:3703420764)

Confirmed the reported defect by tracing the real render path: MainContent rendered every pending item as item={{ ...item, id: 0 }} with no thoughtHeadId, so a clicked streaming thought toggled the shared key 0 in the session-lifetime expandedThoughtHeadIds set. On commit the thought re-enters history under a real baseTimestamp + counter id, so the expanded state was lost (the thought snapped collapsed the moment streaming finished) and the stuck 0 flipped the initial state of every later pending thought; a split long thought desynced its committed head from its pending tail.

Fix:

  • Added PENDING_THOUGHT_HEAD_ID (a negative sentinel that can never collide with a real committed head id) in ThoughtExpandedContext.
  • MainContent now threads a thoughtHeadId for pending thought items in both the virtual-viewport and Static pending paths: a pending gemini_thought head keys off the sentinel; a pending gemini_thought_content tail (long-reasoning split) keys off the most recently committed head id so it expands/collapses with its head.
  • useGeminiStream now settles the provisional key when the thought commits: commitPendingThought (unsplit head) and the split loop (head chunk) migrate the sentinel entry to the real committed head id; a fresh thought starting also clears any stale leftover key. AppContainer provides settlePendingThoughtExpansion which moves the sentinel to the committed id (or drops it when passed null).

This keeps a thought expanded by clicking while streaming expanded after it commits, fixes the 0 cross-contamination, and keeps split long thoughts in sync.

R1-2 — Suggestion: collapse hint advertised ctrl+o, which cannot collapse a click-expanded thought (rc:3703420770)

Confirmed: ctrl+o is the app-wide full-detail switch, not a per-item collapse. When the thought is clickable, the expanded header now advertises clicking to collapse ((click or ctrl+o to collapse)); non-clickable keeps the keyboard-only hint.

R1-3 — Suggestion: collapsed branch duplicated the expanded branch's pending-label/icon arms (rc:3703420773)

Hoisted the shared headerIcon and pendingLabel above the if (!expanded) early return and reused them in both branches. The committed fallbacks remain separate as intended.

R1-4 — Suggestion: onToggle recorded a per-item toggle during fullDetail (rc:3703420777)

Confirmed: with Ctrl+O full-detail on, a click is visually a no-op but silently recorded a toggle that would flip once full-detail turned off. The toggle is now ignored while fullDetail is active.

Additional change (not a review finding)

useReactToolScheduler.ts (committed separately): a pre-existing strict-null error in mapToDisplay's non-error branch blocked npm run typecheck. Every non-error TrackedToolCall variant has a required tool/invocation (the error variant that lacks them is excluded by the discriminant above), so the branch now asserts non-null. Verified pre-existing by reproducing on the clean tree before my changes.

Verification

  • npm run typecheck — passed (all packages; the pre-existing useReactToolScheduler strict-null error was fixed to unblock it)
  • npm run build — passed
  • npm run lint — passed
  • vitest src/ui/components/MainContent.test.tsx — 23 passed (4 new keying tests through the real render path)
  • vitest src/ui/components/HistoryItemDisplay.test.tsx — 38 passed (1 new fullDetail-guard test)
  • vitest src/ui/components/messages/ConversationMessages.test.tsx — 21 passed (1 new clickable-collapse-hint test)
  • vitest src/ui/hooks/useGeminiStream.test.tsx — 183 passed
  • vitest src/ui/hooks/useReactToolScheduler.test.tsx — 2 passed

Commits (on fix/thinking-pending-expand-click):

  • cb344ee69 fix(cli): resolve strict-null errors in tool scheduler display
  • b56dfa0a7 fix(cli): keep clicked thoughts expanded after streaming completes
中文说明

四条评审意见均已处理并在代码中解决。无冲突(--conflict false),无需合并 base。

评审意见

R1-1 — Critical:pending 思考块的展开状态被记录在共享哨兵 0 上(rc:3703420764)

通过真实渲染路径确认了该缺陷:MainContent 把每个 pending 项渲染为 item={{ ...item, id: 0 }} 且不传 thoughtHeadId,因此点击流式思考块会切换会话级 expandedThoughtHeadIds 集合中的共享键 0。思考块提交时以真实的 baseTimestamp + counter id 重新进入历史,展开状态随之丢失(流式结束的瞬间思考块自动折叠),残留的 0 还会翻转后续每个 pending 思考块的初始状态;被切分的长思考块其已提交头部与 pending 尾部也会失步。

修复:

  • ThoughtExpandedContext 中新增 PENDING_THOUGHT_HEAD_ID(负值哨兵,永不会与真实提交 head id 冲突)。
  • MainContent 现在在虚拟视口(VP)与 Static 两条 pending 渲染路径中为 pending 思考项贯通 thoughtHeadId:pending 的 gemini_thought 头部以哨兵为键;pending 的 gemini_thought_content 尾部(长推理被切分)以最近一次已提交的 head id 为键,从而与其头部同步展开/收起。
  • useGeminiStream 在思考块提交时结算该临时键:commitPendingThought(未切分的头部)与切分循环(头部块)把哨兵记录迁移到真实提交的 head id;新思考块开始时还会清理任何残留的旧键。AppContainer 提供 settlePendingThoughtExpansion,把哨兵迁移到已提交 id(传 null 时仅清除)。

这样在流式期间通过点击展开的思考块,在提交后仍保持展开,修复了 0 的交叉污染,并使被切分的长思考块保持同步。

R1-2 — Suggestion:收起提示写着 ctrl+o,但 ctrl+o 无法收起通过点击展开的思考块(rc:3703420770)

已确认:ctrl+o 是应用级的 full-detail 开关,并非逐项收起。当思考块可点击时,展开状态的头部现在提示通过点击收起((click or ctrl+o to collapse));不可点击时保持仅键盘提示。

R1-3 — Suggestion:collapsed 分支复制了 expanded 分支的 pending 标签/图标臂(rc:3703420773)

把共享的 headerIconpendingLabel 提升到 if (!expanded) 提前返回之前,并在两个分支中复用。committed 的兜底文案按原意保持分开。

R1-4 — Suggestion:onTogglefullDetail 期间仍记录逐项切换(rc:3703420777)

已确认:在 Ctrl+O full-detail 开启时,点击在界面上毫无变化,却会悄悄记录一次切换,待 full-detail 关闭时翻转状态。现在 fullDetail 激活时会忽略该切换。

附加改动(非评审意见)

useReactToolScheduler.ts(单独提交):mapToDisplay 的非 error 分支存在一个预先存在的 strict-null 错误,阻断了 npm run typecheck。所有非 errorTrackedToolCall 变体都有必需的 tool/invocation(缺少它们的 error 变体已被上方的判别条件排除),因此该分支现在断言非空。已通过在我改动前的干净工作树上复现,确认其为预先存在的问题。

验证

  • npm run typecheck — 通过(所有包;修复了预先存在的 useReactToolScheduler strict-null 错误以解除阻断)
  • npm run build — 通过
  • npm run lint — 通过
  • vitest src/ui/components/MainContent.test.tsx — 23 通过(新增 4 个走真实渲染路径的键控测试)
  • vitest src/ui/components/HistoryItemDisplay.test.tsx — 38 通过(新增 1 个 fullDetail 守卫测试)
  • vitest src/ui/components/messages/ConversationMessages.test.tsx — 21 通过(新增 1 个可点击收起提示测试)
  • vitest src/ui/hooks/useGeminiStream.test.tsx — 183 通过
  • vitest src/ui/hooks/useReactToolScheduler.test.tsx — 2 通过

提交(位于 fix/thinking-pending-expand-click):

  • cb344ee69 fix(cli): resolve strict-null errors in tool scheduler display
  • b56dfa0a7 fix(cli): keep clicked thoughts expanded after streaming completes

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

— qwen3.8-max via Qwen Code /review (v0.21.3)

Comment thread packages/cli/src/ui/AppContainer.tsx Outdated
Comment on lines +794 to +796
const settlePendingThoughtExpansion = useCallback(
(committedHeadId: number | null) => {
setExpandedThoughtHeadIds((prev) => {

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 pending→committed expansion migration — the mechanism that keeps a thought expanded after the user clicked it open during streaming — has zero test coverage: no test references this reducer or its three call sites in useGeminiStream.ts (settle(null) when a fresh thought starts, the split-commit, and commitPendingThought). The new MainContent tests pin only the render-side keying before commit, and the HistoryItemDisplay tests mock toggle — nothing crosses the pending→commit boundary.

Failure scenario: deleting next.add(committedHeadId) in this reducer, or the settlePendingThoughtExpansion?.(committedId) call in commitPendingThought, ships green — a thought the user clicked open during streaming silently snaps collapsed the instant it commits (the exact regression the round-1 Critical described), and nothing in the suite fails.

Suggested fix: add a useGeminiStream-level test passing a spy settlePendingThoughtExpansion — assert it is called with the committed head id on thought commit, with the split head id on a long-thought split, and with null when a fresh thought begins — and/or a unit test of this reducer (seed the set with PENDING_THOUGHT_HEAD_ID: settle(42) migrates to 42, settle(null) drops, settle without the sentinel no-ops).

中文说明

[Suggestion] pending→committed 展开迁移机制——让用户在流式期间点击展开的思考块在提交后保持展开——完全没有测试覆盖:没有任何测试引用此 reducer 或其在 useGeminiStream.ts 中的三个调用点(新思考开始时的 settle(null)、split 提交、commitPendingThought)。新增的 MainContent 测试只钉住提交前的渲染侧键控,HistoryItemDisplay 测试 mock 了 toggle——没有任何测试跨越 pending→commit 边界。

失败场景:删除此 reducer 中的 next.add(committedHeadId),或删除 commitPendingThought 中的 settlePendingThoughtExpansion?.(committedId) 调用,整个套件仍会全绿——用户在流式期间点击展开的思考块会在提交瞬间悄悄折叠(正是第一轮 Critical 描述的回归),没有任何测试能捕获。

建议修复:添加一个 useGeminiStream 层测试,传入 spy settlePendingThoughtExpansion——断言思考提交时以 committed head id 被调用、长思考 split 时以 split head id 被调用、新思考开始时以 null 被调用——和/或为此 reducer 补充单元测试(set 中预置 PENDING_THOUGHT_HEAD_IDsettle(42) 迁移到 42、settle(null) 丢弃、无哨兵时 no-op)。

— qwen3.8-max via Qwen Code /review (v0.21.4)

Comment on lines +348 to +350
const collapseHint = clickable
? ` ${t('(click or {{keyHint}} to collapse)', { keyHint: toggleKeyHint })}`
: ` ${t('({{keyHint}} to collapse)', { keyHint: toggleKeyHint })}`;

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] While fullDetail (Ctrl+O) is active, this hint advertises "click … to collapse", but the fullDetail click guard added in this same PR (HistoryItemDisplay.tsx: if (!fullDetail) toggleThought(...)) silently swallows clicks — an emergent contradiction between this round's two fixes (the click wording here and the fullDetail guard there). clickable is VP-gated only and fullDetail-unaware. Before this PR the collapse hint never mentioned clicking, so the dead affordance was not advertised.

Failure scenario: VP mode + mouse tracking on; the user presses Ctrl+O → every thought renders expanded showing "(click or ctrl+o to collapse)"; clicking a thought header does nothing (the guard swallows it), repeatedly, while the hint keeps promising the click works. Only Ctrl+O — which collapses everything — functions.

Suggested fix: thread fullDetail into ClickableThinkMessage and pass clickable={clickable && !fullDetail} to ThinkMessage (or otherwise suppress the click wording while fullDetail is on), so fullDetail mode shows the keyboard-only collapse hint.

中文说明

[Suggestion]fullDetail(Ctrl+O)激活时,此提示宣传 "click … to collapse",但本 PR 同一轮新增的 fullDetail 点击守卫(HistoryItemDisplay.tsxif (!fullDetail) toggleThought(...))会静默吞掉点击——这是本轮两个修复(此处的 click 措辞与那里的 fullDetail 守卫)之间的涌现矛盾。clickable 只受 VP 门控,不感知 fullDetail。本 PR 之前收起提示从不提及点击,因此这一死操作此前并未被宣传。

失败场景:VP 模式 + 鼠标追踪开启;用户按 Ctrl+O → 每个思考块都展开并显示 "(click or ctrl+o to collapse)";点击思考块头部毫无反应(守卫吞掉点击),反复点击依旧无效,而提示一直承诺点击可用。唯一有效的是 Ctrl+O——它会把所有内容全部收起。

建议修复:将 fullDetail 透传进 ClickableThinkMessage,向 ThinkMessageclickable={clickable && !fullDetail}(或以其他方式在 fullDetail 开启时抑制 click 措辞),使 fullDetail 模式下只显示键盘收起提示。

— qwen3.8-max via Qwen Code /review (v0.21.4)

Comment on lines +365 to +367
displayName = trackedCall.tool!.displayName;
description = trackedCall.invocation!.getDescription();
renderOutputAsMarkdown = trackedCall.tool!.isOutputMarkdown;

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] These three non-null assertions (from the autofix commit cb344ee) are unnecessary at this commit — verified empirically: with the ! removed, npx tsc --noEmit in packages/cli still passes, because the status === 'error' discriminant already excludes ErroredToolCall, the only union member with optional tool / missing invocation. They are also unrelated to this PR's thought-expansion feature.

Failure scenario: if a future core change makes tool/invocation optional on a non-error ToolCall variant, these assertions keep the compiler silent and mapToDisplay throws a TypeError on every tool-call render, with no compile-time warning.

Suggested change
displayName = trackedCall.tool!.displayName;
description = trackedCall.invocation!.getDescription();
renderOutputAsMarkdown = trackedCall.tool!.isOutputMarkdown;
displayName = trackedCall.tool.displayName;
description = trackedCall.invocation.getDescription();
renderOutputAsMarkdown = trackedCall.tool.isOutputMarkdown;
中文说明

[Suggestion] 这三个非空断言(来自 autofix 提交 cb344ee)在此提交下并不必要——已经实验验证:移除 ! 后,packages/cli 中的 npx tsc --noEmit 依然通过,因为 status === 'error' 判别式已排除 ErroredToolCall(唯一 tool 可选、无 invocation 的联合成员)。它们也与本 PR 的思考块展开功能无关。

失败场景:若未来 core 改动使某个非 error 状态的 ToolCall 变体的 tool/invocation 变为可选,这些断言会让编译器保持沉默,mapToDisplay 将在每次工具调用渲染时抛出 TypeError,且没有任何编译期警告。

— qwen3.8-max via Qwen Code /review (v0.21.4)

- Pin the pending->committed expansion settle calls with useGeminiStream
  tests: null on a fresh thought start, the committed head id on commit,
  the split head id on oversized-thought splits, and never for
  gemini_thought_content tails.
- Suppress the click wording in the collapse hint while fullDetail
  (Ctrl+O) is active, since the fullDetail guard swallows clicks.
- Drop the unnecessary non-null assertions in mapToDisplay; the
  status === 'error' discriminant already narrows them away, restoring
  the file to its pre-PR state.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下:

Autofix review round — PR #8443

All three inline suggestions from the automated reviewer (round 2) were addressed in one commit: 9a13ddb1d.

Feedback points and dispositions

  1. rc:3705476751 — [Suggestion] Zero test coverage for the pending→committed expansion migrationAddressed. Added a Pending thought expansion settlement describe block to useGeminiStream.test.tsx with a spy passed through renderTestHook as the new settlePendingThoughtExpansion parameter. The three tests pin all three settle call sites: null when a fresh thought starts, the committed head id when the thought commits (matched against the id addItem returned for the gemini_thought item), and the split head id when an oversized thought splits — additionally asserting that gemini_thought_content tails never settle a second entry. To prove the tests pin the regression the finding described, each settle call was temporarily deleted (mutation check): removing the commit-site call failed the commit test, and removing the split-site call failed the split test. The mutations were reverted; the committed source is unchanged. The AppContainer-side reducer remains covered indirectly through these call-site contracts (the finding asked for the hook-level test and/or a reducer unit test).
  2. rc:3705476759 — [Suggestion] "click … to collapse" hint advertised while the fullDetail guard swallows clicksAddressed. fullDetail is now threaded into ClickableThinkMessage, and clickable passed to ThinkMessage is VP && mouseTracking && !fullDetail, so fullDetail mode shows the keyboard-only collapse hint. The mouse handler itself is unchanged (the existing onToggle guard keeps ignoring clicks under fullDetail). Added a regression test asserting the collapse hint contains the key hint but not the word "click" when fullDetail is active in VP mode.
  3. rc:3705476767 — [Suggestion] Three unnecessary non-null assertions in mapToDisplayAddressed. Removed the ! assertions; npm run typecheck passes without them, confirming the reviewer's empirical claim that the status === 'error' discriminant already excludes ErroredToolCall (the only union member with optional tool / missing invocation). With the assertions gone, useReactToolScheduler.ts is byte-identical to the branch base, so the unrelated hunk from commit cb344ee69 drops out of this PR entirely.

Conflict notes

--conflict false — no merge performed. Informational: origin/main has advanced past the branch base and #8180 touches the same mapToDisplay region (it widens the error condition with explicit tool === undefined || invocation === undefined checks and adds executionStatus). A future merge will conflict there; the correct resolution keeps main's widened condition and drops the assertions, which main's condition also justifies. No action taken this round per the no-unnecessary-merge rule.

Checks

  • No failed checks and no still-red checks were reported for this round.

Verification

Commands actually run and their results:

  • npm run build — passed
  • npm run typecheck — passed (also validates R2-3: passes with the non-null assertions removed)
  • npm run lint — passed
  • npx prettier --check on the four changed files — passed
  • cd packages/cli && npx vitest run src/ui/hooks/useGeminiStream.test.tsx src/ui/hooks/useReactToolScheduler.test.tsx — 188 passed (includes the 3 new settlement tests)
  • cd packages/cli && npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/messages/ConversationMessages.test.tsx src/ui/components/MainContent.test.tsx — 83 passed (includes the new fullDetail hint test)
  • Mutation checks (temporary, reverted before commit): deleting the settle call in commitPendingThought failed "settles the committed head id…" (expected […, 1001], called only once); deleting the settle call in the split path failed "settles the split head id…" — both tests pin their settle call
  • Integration tests after npm run bundle — not run: the touched behavior (TUI click-to-expand and hint wording) is exercised by the unit tests above, not only through the bundled CLI or the integration harness
中文说明

Autofix 审查轮次 — PR #8443

自动审查者(第二轮)提出的三条行内建议已全部在一个提交中处理:9a13ddb1d

反馈点及处理结果

  1. rc:3705476751 — [Suggestion] pending→committed 展开迁移机制零测试覆盖已处理。useGeminiStream.test.tsx 中新增 Pending thought expansion settlement describe 块,通过 renderTestHook 新增的 settlePendingThoughtExpansion 参数传入 spy。三个测试钉住全部三个 settle 调用点:新思考开始时传 null;思考提交时传 committed head id(与 addItemgemini_thought 条目返回的 id 比对);超长思考 split 时传 split head id——并额外断言 gemini_thought_content 尾部永远不会 settle 第二个条目。为证明测试确实能捕获该 finding 描述的回归,对每个 settle 调用做了临时删除(变异检查):删除提交点调用会使提交测试失败,删除 split 点调用会使 split 测试失败。变异均已还原;提交的源码未受影响。AppContainer 侧的 reducer 通过这些调用点契约间接覆盖(该 finding 要求的是 hook 层测试和/或 reducer 单元测试)。
  2. rc:3705476759 — [Suggestion] fullDetail 守卫吞掉点击时仍宣传 "click … to collapse"已处理。 fullDetail 现在透传进 ClickableThinkMessage,传给 ThinkMessageclickable 变为 VP && mouseTracking && !fullDetail,因此 fullDetail 模式下只显示键盘收起提示。鼠标处理器本身不变(现有 onToggle 守卫继续在 fullDetail 下忽略点击)。新增回归测试,断言 VP 模式下 fullDetail 激活时收起提示包含按键提示但不含 "click" 字样。
  3. rc:3705476767 — [Suggestion] mapToDisplay 中三个不必要的非空断言已处理。 移除 ! 断言后 npm run typecheck 通过,证实了审查者的实验结论:status === 'error' 判别式已排除 ErroredToolCall(唯一 tool 可选、无 invocation 的联合成员)。断言移除后,useReactToolScheduler.ts 与分支基点逐字节一致,提交 cb344ee69 引入的无关改动整体退出本 PR。

冲突说明

--conflict false — 未执行合并。提示信息:origin/main 已越过分支基点前进,#8180 触及同一 mapToDisplay 区域(将 error 条件放宽为显式的 tool === undefined || invocation === undefined 检查并新增 executionStatus)。未来合并时该处会冲突;正确的解法是保留 main 的放宽条件并去掉断言——main 的条件同样能证明断言多余。依据"不做不必要合并"规则,本轮未采取行动。

检查项

  • 本轮没有失败检查,也没有持续标红的检查。

验证(Verification)

实际执行的命令及结果:

  • npm run build — 通过
  • npm run typecheck — 通过(同时验证了 R2-3:移除非空断言后依然通过)
  • npm run lint — 通过
  • 对四个改动文件执行 npx prettier --check — 通过
  • cd packages/cli && npx vitest run src/ui/hooks/useGeminiStream.test.tsx src/ui/hooks/useReactToolScheduler.test.tsx — 188 通过(含 3 个新的 settlement 测试)
  • cd packages/cli && npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/messages/ConversationMessages.test.tsx src/ui/components/MainContent.test.tsx — 83 通过(含新的 fullDetail 提示测试)
  • 变异检查(临时修改、提交前已还原):删除 commitPendingThought 中的 settle 调用后 "settles the committed head id…" 失败(期望 […, 1001],实际只被调用一次);删除 split 路径的 settle 调用后 "settles the split head id…" 失败——两个测试均钉住了各自的 settle 调用
  • npm run bundle 后的集成测试 — 未执行:本次触及的行为(TUI 点击展开与提示措辞)由上述单元测试覆盖,并非只能通过打包后的 CLI 或集成测试框架验证

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

— qwen3.8-max via Qwen Code /review (v0.21.3)

Comment on lines +344 to +345
// turned back off. Ignore it.
if (!fullDetail) toggleThought(thoughtGroupHeadId);

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] R3-1: The thoughtHeadId consumption seam (thoughtGroupHeadId = thoughtHeadId ?? item.id, ~line 263) is exercised by no test — no test passes a thoughtHeadId prop to HistoryItemDisplay (its test file never sets it; MainContent.test.tsx mocks the component and asserts only on delivered props), so the prop-present branch never runs. — Failure scenario: verified by live mutation: replacing this consumption site with const thoughtGroupHeadId = item.id; leaves all 86 relevant tests passing, yet breaks the feature end-to-end in production — pending items render with item.id = 0, so a click records the toggle under 0 instead of the sentinel -1; settlePendingThoughtExpansion then finds no sentinel to migrate at commit, and the thought the user expanded while streaming collapses when it commits; split thought groups also un-link (tails no longer follow the head's expansion).

Suggested fix — add tests that exercise the seam:

// HistoryItemDisplay.test.tsx — render a pending thought with the sentinel
<HistoryItemDisplay
  item={thoughtItem}
  terminalWidth={100}
  isPending={true}
  thoughtHeadId={PENDING_THOUGHT_HEAD_ID}
/>
// assert a complete press+release click calls toggle with
// PENDING_THOUGHT_HEAD_ID (not item.id), and that a committed
// gemini_thought_content with thoughtHeadId={42} resolves
// expanded from expandedHeadIds.has(42)
中文说明

thoughtHeadId 的消费接缝(thoughtGroupHeadId = thoughtHeadId ?? item.id,约第 263 行)没有任何测试覆盖——没有测试向 HistoryItemDisplay 传入 thoughtHeadId prop(其测试文件从未设置该 prop;MainContent.test.tsx 直接 mock 了该组件,只断言下发的 props),因此“prop 存在”的分支从未被执行。 — 失败场景(已通过活体突变验证):把该消费点替换为 const thoughtGroupHeadId = item.id;,全部 86 个相关测试仍然通过,但生产环境中功能端到端失效——pending 项以 item.id = 0 渲染,点击会把切换记录在 0 下而非哨兵值 -1;提交时 settlePendingThoughtExpansion 找不到可迁移的哨兵键,用户在流式期间展开的思考块会在提交瞬间折叠;被切分的思考组也会失联(尾部不再跟随头部展开)。

建议修复——新增覆盖该接缝的测试:以 thoughtHeadId={PENDING_THOUGHT_HEAD_ID} 渲染 pending 思考块,断言完整的按下+释放点击后 togglePENDING_THOUGHT_HEAD_ID(而非 item.id)被调用;再断言已提交的 gemini_thought_contentthoughtHeadId={42})的展开状态取自 expandedHeadIds.has(42)

— qwen3.8-max via Qwen Code /review (v0.21.3)

Comment on lines +413 to +416
thoughtHeadId={pendingThoughtHeadId(
item.type,
lastCommittedThoughtHeadIdRef.current,
)}

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] R3-2: Split-tail keying (pending gemini_thought_content → committed head id) is only tested through the Static render path; this VP path — the only mode where clicking works — has no tail test (its only sentinel test passes a gemini_thought head, which ignores the second argument of pendingThoughtHeadId). — Failure scenario: a regression that breaks this site's second argument — the ref read dropped during a refactor of renderVirtualItem, or the lastCommittedThoughtHeadIdRef.current = … assignment line deleted — survives every existing test. User-visible result in VP mode: while streaming an oversized split thought, the pending tail stops resolving against the head's expansion key, so clicking the head expands only the head chunk and the streaming tail stays collapsed — a fragmented live view, in the only mode where clicking exists.

Suggested fix — mirror the existing Static-path test through the VP path:

// MainContent.test.tsx — same scenario, VP mode
renderMainContent(
  createUIState({
    useTerminalBuffer: true,
    history: [{ id: 42, type: 'gemini_thought', text: 'head chunk' }],
    pendingHistoryItems: [
      { type: 'gemini_thought_content', text: 'streaming tail…' },
    ],
  }),
);
// assert the pending tail's thoughtHeadId is 42
中文说明

切分尾部的键控(pending gemini_thought_content → 已提交的 head id)只在 Static 渲染路径下有测试;VP 路径——唯一支持点击的模式——没有尾部测试(该路径唯一的哨兵测试传入的是 gemini_thought 头部,不会触及 pendingThoughtHeadId 的第二个参数)。 — 失败场景:任何破坏该调用点第二个参数的回归——重构 renderVirtualItem 时删掉 ref 读取,或删除 lastCommittedThoughtHeadIdRef.current = … 赋值行——都能在现有全部测试下存活。VP 模式下的用户可见后果:流式输出超长被切分的思考时,pending 尾部不再解析到头部的展开键,点击头部只会展开头部块,正在流式输出的尾部保持折叠——在唯一支持点击的模式下出现割裂的实时视图。

建议修复——把现有的 Static 路径测试镜像到 VP 路径:以 useTerminalBuffer: true 渲染“已提交 head(id 42)+ pending gemini_thought_content 尾部”的场景,断言尾部的 thoughtHeadId 为 42。

— qwen3.8-max via Qwen Code /review (v0.21.3)

Comment on lines +579 to +582
thoughtHeadId={pendingThoughtHeadId(
item.type,
lastCommittedThoughtHeadId,
)}

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] R3-4: No test pins fullDetail propagation to pending items on either render path (this Static site and the VP twin at ~line 413). This diff makes pending thoughts clickable and adds the if (!fullDetail) toggleThought(…) guard in HistoryItemDisplay, whose effectiveness depends on MainContent forwarding fullDetail here. Probe-verified: deleting fullDetail={fullDetail} from both pending sites survives the whole existing suite (all 23 MainContent + 39 HistoryItemDisplay tests pass), while a test asserting the pending spy call received fullDetail: true fails on both paths under the mutant and passes on this PR. — Failure scenario: if a future refactor drops fullDetail={fullDetail} from one/both pending sites, every test still passes, and in production during Ctrl+O full-detail mode a streaming thought advertises “click … to expand/collapse” and records a per-head toggle on click; when fullDetail turns off, that thought's expansion silently flips — the deferred-flip regression the added guard's comment explicitly exists to prevent.

Suggested fix:

// MainContent.test.tsx — pending item under fullDetail
renderMainContent(
  createUIState({
    pendingHistoryItems: [{ type: 'gemini_thought', text: 'reasoning…' }],
  }),
  { thoughtExpandedProviderValue: { allExpanded: true, expandedHeadIds: new Set(), toggle } },
);
// assert the pending HistoryItemDisplay spy call received fullDetail: true
// (mirror the existing committed tool_group assertion, VP and Static paths)
中文说明

没有任何测试固定“fullDetail 向 pending 项的传播”(此 Static 调用点与约第 413 行的 VP 对应点均无)。本 PR 让 pending 思考块可点击,并在 HistoryItemDisplay 中新增了 if (!fullDetail) toggleThought(…) 守卫,而该守卫是否生效取决于 MainContent 在此处转发 fullDetail。探针验证:从两个 pending 调用点删除 fullDetail={fullDetail} 后,现有全部测试(MainContent 23 个 + HistoryItemDisplay 39 个)依然通过;而断言 pending spy 收到 fullDetail: true 的测试在该突变下两条路径均失败,在本 PR 代码下均通过。 — 失败场景:未来重构若从一个或两个 pending 调用点删除 fullDetail={fullDetail},所有测试仍然绿灯,而生产环境中 Ctrl+O 全详情模式下,流式思考块会显示“click … to expand/collapse”并在点击时记录按头切换;fullDetail 关闭后,该思考块的展开状态被静默翻转——正是新增守卫注释明确要防止的“延迟翻转”回归。

建议修复:在 ThoughtExpandedProviderallExpanded: true)下渲染 pending gemini_thought,断言 HistoryItemDisplay 的 spy 调用收到 fullDetail: true(镜像现有已提交 tool_group 的断言,覆盖 VP 与 Static 两条路径)。

— qwen3.8-max via Qwen Code /review (v0.21.3)

Comment on lines +337 to +339
const lastCommittedThoughtHeadId = useMemo(() => {
for (let i = visibleHistory.length - 1; i >= 0; i--) {
const item = visibleHistory[i];

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] R3-5: This backward scan's “most recent committed thought head, not the first” property is pinned by no test — every existing keying test uses a history with exactly one committed thought (id 42). Probe-verified: a first-match mutant (iterating forward) survives the whole suite, while a two-head probe fails under it with expected 41 to be 42 on both render paths and passes on this PR. — Failure scenario: turn 1 commits a thought head A; in turn 2 a long thought splits (>16 384 chars), head B commits, and under the mutant the pending gemini_thought_content tail gets thoughtHeadId = A. Clicking collapsed head B in VP mode expands B but the actively-streaming tail stays hidden (the exact collapse regression this PR exists to fix), and conversely a previously click-expanded thought A makes turn 2's tail appear expanded under a collapsed head B.

Suggested fix:

// MainContent.test.tsx — two committed heads + pending tail
renderMainContent(
  createUIState({
    history: [
      { id: 41, type: 'gemini_thought', text: 'first head' },
      { id: 42, type: 'gemini_thought', text: 'second head' },
    ],
    pendingHistoryItems: [
      { type: 'gemini_thought_content', text: 'streaming tail…' },
    ],
  }),
);
// assert the pending tail's thoughtHeadId is 42, not 41
// (ideally on both the Static and VP paths)
中文说明

该反向扫描“取最近一个已提交的思考头部(而非第一个)”的性质没有任何测试固定——现有键控测试使用的历史都只有一个已提交思考头(id 42)。探针验证:首匹配突变(改为正向遍历)能在整个测试套件下存活;而双头部探针在该突变下以 expected 41 to be 42 在两条渲染路径均失败,在本 PR 代码下通过。 — 失败场景:第 1 轮提交思考头 A;第 2 轮长思考被切分(>16 384 字符),头部 B 提交,突变下 pending gemini_thought_content 尾部拿到 thoughtHeadId = A。在 VP 模式点击折叠的头部 B 时,B 展开但正在流式输出的尾部仍被隐藏(正是本 PR 要修复的折叠回归);反过来,此前被点击展开的思考 A 会让第 2 轮的尾部在折叠的头部 B 下显示为展开。

建议修复:新增“两个已提交头部 + pending 尾部”的键控测试,断言尾部的 thoughtHeadId 是 42 而非 41(最好同时覆盖 Static 与 VP 路径)。

— qwen3.8-max via Qwen Code /review (v0.21.3)

Comment on lines +126 to +127
if (type === 'gemini_thought') return PENDING_THOUGHT_HEAD_ID;
if (type === 'gemini_thought_content') return lastCommittedThoughtHeadId;

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] R3-7: The pending-head → sentinel keying is only pinned against an empty committed history: both head-sentinel tests (Static ~line 1075, VP ~line 1125 in MainContent.test.tsx) use the default history: [], and no test renders a pending gemini_thought head when committed thought heads already exist. Probe-verified: the mutant if (type === 'gemini_thought') return lastCommittedThoughtHeadId ?? PENDING_THOUGHT_HEAD_ID; leaves all 23 existing tests green, while a non-empty-history probe fails under it with expected 42 to be -1 on both paths and passes on this PR. — Failure scenario: from the second thought of any session onward (the common case), under the mutant clicking a streaming thought's head calls toggleThought(lastCommittedHeadId) — expanding/collapsing the previous thought group instead of recording the pending sentinel; settlePendingThoughtExpansion at commit then finds no PENDING_THOUGHT_HEAD_ID entry to migrate, so the thought the user clicked open while streaming collapses at commit — silently reintroducing the exact regression this PR fixes.

Suggested fix:

// MainContent.test.tsx — pending head against non-empty history
renderMainContent(
  createUIState({
    history: [{ id: 42, type: 'gemini_thought', text: 'earlier head' }],
    pendingHistoryItems: [{ type: 'gemini_thought', text: 'reasoning…' }],
  }),
);
// assert the pending head's thoughtHeadId is PENDING_THOUGHT_HEAD_ID, not 42
// (on both the Static and VP paths)
中文说明

pending 头部 → 哨兵键控只在“已提交历史为空”时被固定:两个头部哨兵测试(MainContent.test.tsx Static 约第 1075 行、VP 约第 1125 行)都使用默认的 history: [],没有任何测试在已存在已提交思考头时渲染 pending gemini_thought 头部。探针验证:突变 if (type === 'gemini_thought') return lastCommittedThoughtHeadId ?? PENDING_THOUGHT_HEAD_ID; 下现有 23 个测试全部绿灯,而非空历史探针在该突变下以 expected 42 to be -1 在两条路径均失败,在本 PR 代码下通过。 — 失败场景:从会话的第二个思考块开始(常见情形),突变下点击流式思考头部会调用 toggleThought(lastCommittedHeadId)——展开/折叠的是上一个思考组,而不是记录 pending 哨兵;提交时 settlePendingThoughtExpansion 找不到 PENDING_THOUGHT_HEAD_ID 条目可迁移,用户在流式期间点击展开的思考块会在提交时折叠——静默地重新引入本 PR 要修复的回归。

建议修复:扩展键控测试,以 history: [{ id: 42, type: 'gemini_thought', … }]pendingHistoryItems: [{ type: 'gemini_thought', … }] 渲染,断言 pending 头部的 thoughtHeadIdPENDING_THOUGHT_HEAD_ID(而非 42),覆盖 Static 与 VP 两条路径。

— qwen3.8-max via Qwen Code /review (v0.21.3)

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 3/100 轮)。改动内容与我反驳保留之处如下:

Round-3 review feedback — addressed

All five findings were automated-reviewer [Suggestion] items, each mutation-verified by the reviewer (a named mutant survives the existing suite but breaks the feature in production). All five are test-only additions pinning seams this PR introduces; no production code changed. Every finding was implemented — nothing declined, deferred, or escalated.

Finding Disposition Change
R3-1 — the thoughtHeadId consumption seam in HistoryItemDisplay (thoughtHeadId ?? item.id) had zero coverage Implemented Two tests: a pending thought rendered with thoughtHeadId={PENDING_THOUGHT_HEAD_ID} records its click under the sentinel (not item.id); a committed gemini_thought_content with thoughtHeadId={42} resolves expansion from expandedHeadIds.has(42), not its own id
R3-2 — split-tail keying (pending gemini_thought_content → committed head id) only tested on the Static path, not the VP path where clicking exists Implemented Two tests: the Static tail-keying test mirrored through the VP path, plus a re-render test where the head commits mid-stream (first render pending-only, second render committed head 42 + pending tail) to pin the lastCommittedThoughtHeadIdRef.current refresh — the single-render variant cannot kill the dropped-assignment mutant the finding names
R3-4 — no test pins fullDetail propagation to pending items on either render path Implemented Two tests (Static + VP) asserting the pending HistoryItemDisplay spy call receives fullDetail: true under ThoughtExpandedProvider with allExpanded: true, mirroring the existing committed tool_group assertion
R3-5 — the backward scan's "most recent committed thought head, not the first" property unpinned (all keying tests used a single committed head) Implemented Two tests (Static + VP) with committed heads 41 and 42 plus a pending tail, asserting the tail's thoughtHeadId is 42
R3-7 — pending-head → sentinel keying only pinned against an empty committed history Implemented Two tests (Static + VP) rendering a pending gemini_thought head against history: [{ id: 42, type: 'gemini_thought' }], asserting the head keeps PENDING_THOUGHT_HEAD_ID instead of 42

Supporting tweak: the renderThoughtWithToggle / renderWithThoughtExpanded test helpers each gained one optional parameter (thoughtHeadId / UIState overrides) to carry the new scenarios without duplicating render boilerplate.

Mutation evidence (temporary source mutations, reverted after each run):

  • Dropping the thoughtHeadId ?? item.id seam → both new HistoryItemDisplay tests fail.
  • pendingThoughtHeadId returning lastCommittedThoughtHeadId ?? PENDING_THOUGHT_HEAD_ID for heads → both new non-empty-history sentinel tests fail.
  • Forward first-match scan instead of backward scan → both two-head tests fail.
  • Dropping fullDetail={fullDetail} from the pending render sites → both new fullDetail-pending tests fail.
  • Dropping the lastCommittedThoughtHeadIdRef.current = … assignment → the mid-stream re-render test fails.

No conflict resolution was needed (--conflict false).

Verification

  • npm run build — passed (exit 0, re-run after final edit)
  • npm run typecheck — passed (exit 0, re-run after final edit)
  • npm run lint — passed (exit 0, re-run after final edit)
  • npx prettier --check on both touched test files — passed
  • npx vitest run src/ui/components/MainContent.test.tsx src/ui/components/HistoryItemDisplay.test.tsx (packages/cli, touched) — 72 passed (31 MainContent incl. 8 new, 41 HistoryItemDisplay incl. 2 new)
  • Mutation spot-checks against all five named seams — every mutant killed by the new tests, source restored clean after each run (verified with git diff)
中文说明

第 3 轮审查反馈 — 已处理

五个发现全部来自自动审查器的 [Suggestion] 条目,且每条都由审查器做了突变验证(指定的突变体能在现有测试套件下存活,但会在生产中破坏功能)。五条均为纯测试补充,用于固定本 PR 引入的接缝;生产代码未改动。所有发现均已实现——没有拒绝、推迟或升级项。

发现 处理 变更
R3-1 — HistoryItemDisplaythoughtHeadId 的消费接缝(thoughtHeadId ?? item.id)零覆盖 已实现 两个测试:以 thoughtHeadId={PENDING_THOUGHT_HEAD_ID} 渲染的 pending 思考块,点击记录在哨兵键下(而非 item.id);带 thoughtHeadId={42} 的已提交 gemini_thought_content 的展开状态取自 expandedHeadIds.has(42),而非自身 id
R3-2 — 切分尾部键控(pending gemini_thought_content → 已提交 head id)只在 Static 路径有测试,唯一支持点击的 VP 路径没有 已实现 两个测试:把 Static 尾部键控测试镜像到 VP 路径;另加一个重渲染测试——头部在流式中途提交(首次渲染仅 pending,第二次渲染已提交 head 42 + pending 尾部),固定 lastCommittedThoughtHeadIdRef.current 的刷新——单次渲染版本无法杀死该发现点名的"删除赋值行"突变体
R3-4 — 没有任何测试固定 fullDetail 向 pending 项的传播(两条渲染路径均无) 已实现 两个测试(Static + VP),在 allExpanded: trueThoughtExpandedProvider 下断言 pending HistoryItemDisplay 的 spy 调用收到 fullDetail: true,镜像现有已提交 tool_group 的断言
R3-5 — 反向扫描"取最近一个已提交思考头部(而非第一个)"的性质未被固定(现有键控测试都只用一个已提交头部) 已实现 两个测试(Static + VP),已提交头部 41 与 42 加 pending 尾部,断言尾部的 thoughtHeadId 为 42
R3-7 — pending 头部 → 哨兵键控只在已提交历史为空时被固定 已实现 两个测试(Static + VP),在 history: [{ id: 42, type: 'gemini_thought' }] 下渲染 pending gemini_thought 头部,断言头部保持 PENDING_THOUGHT_HEAD_ID 而非 42

配套微调:renderThoughtWithToggle / renderWithThoughtExpanded 两个测试助手各增加一个可选参数(thoughtHeadId / UIState 覆盖项),以承载新场景而不复制渲染样板代码。

突变证据(临时源码突变,每次运行后还原):

  • 删除 thoughtHeadId ?? item.id 接缝 → 两个新 HistoryItemDisplay 测试失败。
  • pendingThoughtHeadId 对头部返回 lastCommittedThoughtHeadId ?? PENDING_THOUGHT_HEAD_ID → 两个新的非空历史哨兵测试失败。
  • 正向首匹配扫描替代反向扫描 → 两个双头部测试失败。
  • 从 pending 渲染点删除 fullDetail={fullDetail} → 两个新的 fullDetail-pending 测试失败。
  • 删除 lastCommittedThoughtHeadIdRef.current = … 赋值行 → 流式中途重渲染测试失败。

无需冲突处理(--conflict false)。

验证

  • npm run build — 通过(exit 0,最终修改后重跑)
  • npm run typecheck — 通过(exit 0,最终修改后重跑)
  • npm run lint — 通过(exit 0,最终修改后重跑)
  • 对两个改动的测试文件执行 npx prettier --check — 通过
  • npx vitest run src/ui/components/MainContent.test.tsx src/ui/components/HistoryItemDisplay.test.tsx(packages/cli,改动包)— 72 通过(MainContent 31 个含新增 8 个,HistoryItemDisplay 41 个含新增 2 个)
  • 对全部五个指定接缝做突变抽查 — 每个突变体均被新测试杀死,每次运行后源码已还原干净(用 git diff 验证)

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +1762 to +1764
if (pendingThoughtType === 'gemini_thought') {
settlePendingThoughtExpansion?.(committedSplitId);
}

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] R4-3: The migration rule "when a gemini_thought head commits, settle the pending expansion onto its committed id" is pasted into two callbacks in this same PR — here in the handleThoughtEvent split loop (guarded on pendingThoughtType === 'gemini_thought'), and again in commitPendingThought (~lines 1798-1800, guarded on item.type === 'gemini_thought'). The "heads settle, tails never settle" invariant — the seam the three new settlement tests pin — now lives in two places expressed in two terms.

Failure scenario: a future commit path or rule change (e.g. settling on drop) updated in one callback but not the other silently loses the provisional expansion entry — a thought the user clicked open while streaming collapses the moment it commits, with no error pointing at the missed call site.

Suggested fix — one hook-level wrapper used at both call sites (buildThoughtItem(pendingThoughtType, beforeText).type === pendingThoughtType, so the split site is covered by the same item.type check):

const addItemAndSettleThoughtHead = useCallback(
  (item: HistoryItemWithoutId, ts: number) => {
    const id = addItem(item, ts);
    if (item.type === 'gemini_thought') settlePendingThoughtExpansion?.(id);
    return id;
  },
  [addItem, settlePendingThoughtExpansion],
);
中文说明

[Suggestion] R4-3:迁移规则"当 gemini_thought 头部提交时,把 pending 展开记录落到其提交后的 id 上"在本 PR 中被粘贴进了两个回调——此处 handleThoughtEvent 的拆分循环(以 pendingThoughtType === 'gemini_thought' 为条件),以及 commitPendingThought(约 1798-1800 行,以 item.type === 'gemini_thought' 为条件)。"头部 settle、尾部永不 settle"这一不变量——三个新 settlement 测试所钉住的接缝——现在以两种表述存在于两处。

失败场景:未来新增提交路径或修改规则(例如在丢弃时 settle)时,若只更新其中一个回调,会静默丢失临时展开记录——用户在流式输出期间点击展开的思考块会在提交瞬间折叠,且没有任何错误指向被遗漏的调用点。

建议修复——在两个调用点共用一个 hook 级包装函数(buildThoughtItem(pendingThoughtType, beforeText).type === pendingThoughtType,因此拆分点同样被 item.type 检查覆盖):

const addItemAndSettleThoughtHead = useCallback(
  (item: HistoryItemWithoutId, ts: number) => {
    const id = addItem(item, ts);
    if (item.type === 'gemini_thought') settlePendingThoughtExpansion?.(id);
    return id;
  },
  [addItem, settlePendingThoughtExpansion],
);

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +54 to +56
it('hints click-to-collapse when expanded and clickable (VP mode)', () => {
const withoutClick = render(
<ThinkMessage {...defaultProps} isPending={true} expanded={true} />,

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] R4-4: The new collapseHint = clickable ? '(click or …)' : '(…)' expression (ConversationMessages.tsx:348) now governs both pending AND committed expanded thoughts, but the clickable variant is only tested with isPending={true} — here and in the pending case below. The committed+expanded+clickable case is rendered nowhere (both to collapse assertions in HistoryItemDisplay.test.tsx have clickable=false). The mutant const collapseHint = clickable && isPending ? … : … passes every test in this diff, and the old code had exactly an !isPending && restriction in this spot, so a regression reintroducing it is plausible and currently untestable.

Failure scenario: a regression gating the click wording on isPending (as the pre-PR code did) silently drops the "click" wording from the collapse hint of a click-expanded committed thought in VP mode, and no test fails.

Suggested fix — add a sibling case for the committed branch:

it('hints click-to-collapse when committed, expanded and clickable', () => {
  const withClick = render(
    <ThinkMessage
      {...defaultProps}
      isPending={false}
      expanded={true}
      clickable={true}
    />,
  ).lastFrame();
  expect(withClick).toContain('click');
  expect(withClick).toContain('to collapse');
});
中文说明

[Suggestion] R4-4:新的 collapseHint = clickable ? '(click or …)' : '(…)' 表达式(ConversationMessages.tsx:348)现在同时约束 pending 与已提交(committed)的展开思考块,但 clickable 变体只在 isPending={true} 下被测试——即此处与下方的 pending 用例。committed+expanded+clickable 的组合在任何测试中都未被渲染(HistoryItemDisplay.test.tsx 中两处 to collapse 断言的 clickable=false)。变异体 const collapseHint = clickable && isPending ? … : … 能通过本 diff 的全部测试,而旧代码此处恰好就有 !isPending && 限制,因此重新引入该限制的回归是可能发生的,且目前无法被测试捕获。

失败场景:若回归把 click 措辞重新限定在 isPending 上(与 PR 前的代码一致),VP 模式下已提交且通过点击展开的思考块,其折叠提示会悄悄丢掉 "click" 措辞,而没有任何测试失败。

建议修复——为 committed 分支补充一个平行用例(见上方代码块)。

— qwen3.8-max via Qwen Code /review (v0.21.5)

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 4/100 轮)。改动内容与我反驳保留之处如下:

Round 4 review addressed (PR #8443)

Commit: c8441833cfix(cli): address round-4 review on thought expansion (#8443)

Feedback points and dispositions

  1. rc:3710315899 — R4-3 [Suggestion]: settlement rule duplicated in two callbacks — Resolved. Verified against the code: the rule "when a gemini_thought head commits, settle the pending expansion onto its committed id" was expressed twice in useGeminiStream.ts — in the handleThoughtEvent split loop (guarded on pendingThoughtType === 'gemini_thought') and in commitPendingThought (guarded on item.type === 'gemini_thought'). Fix: introduced one hook-level wrapper addItemAndSettleThoughtHead and used it at both call sites. Because buildThoughtItem(t, text).type === t, the wrapper's item.type === 'gemini_thought' check covers the split site exactly as before, so the "heads settle, tails never settle" invariant now lives in one place. Callback dependency arrays were updated accordingly: commitPendingThought no longer touches addItem / settlePendingThoughtExpansion directly, while handleThoughtEvent keeps its direct settlePendingThoughtExpansion dependency because it still settles null when a fresh thought starts. The three pre-existing settlement tests (settle null on fresh thought, settle committed head id on commit, settle split head id on oversized split and never for content tails) pass unchanged, confirming identical behavior.

  2. rc:3710315904 — R4-4 [Suggestion]: committed + expanded + clickable collapse hint untested — Resolved. Verified: the clickable variant of collapseHint in ConversationMessages.tsx was only rendered by tests with isPending={true}, so the mutant gating the click wording on isPending (which the pre-PR code had) would pass every test in the diff. Fix: added a sibling case hints click-to-collapse when committed, expanded and clickable (VP mode) in ConversationMessages.test.tsx, rendering committed + expanded both without and with clickable and asserting the keyboard-only vs click wording, mirroring the existing pending-side test.

  3. Review-level note (not a finding): the automated reviewer reported that the build-and-test "Integration Tests (CLI, No Sandbox)" suite was skipped in CI and did not run locally, so it was outside its review scope. No action item — the behavior changed this round (hook-level settlement and the Ink collapse hint) is exercised directly by the unit tests below, not only through the bundled CLI or integration harness, so no integration run was required this round.

Conflict

None (--conflict false); no merge performed.

Verification

  • cd packages/cli && npx vitest run src/ui/hooks/useGeminiStream.test.tsx src/ui/components/messages/ConversationMessages.test.tsx — passed: 2 files, 208/208 tests (includes the 3 settlement tests pinning the new wrapper and the new committed click-to-collapse test)
  • npm run typecheck — passed (exit 0)
  • npm run lint — passed (exit 0)
  • npm run build — passed (exit 0)
  • Integration tests after npm run bundle — not run: touched behavior is covered by the unit tests above and is not only exercised through the bundled CLI or integration harness
  • npm run generate:settings-schema — not needed: no settings source changed
中文说明

已处理第 4 轮审查(PR #8443

提交:c8441833cfix(cli): address round-4 review on thought expansion (#8443)

反馈点及处理结果

  1. rc:3710315899 — R4-3 [建议]:settlement 规则在两个回调中重复 — 已解决。 已对照代码核实:"gemini_thought 头部提交时,把 pending 展开记录落到其提交后的 id 上"这一规则在 useGeminiStream.ts 中出现了两次——handleThoughtEvent 的拆分循环(以 pendingThoughtType === 'gemini_thought' 为条件)和 commitPendingThought(以 item.type === 'gemini_thought' 为条件)。修复:引入一个 hook 级包装函数 addItemAndSettleThoughtHead,并在两个调用点共用。由于 buildThoughtItem(t, text).type === t,包装函数中 item.type === 'gemini_thought' 的检查与拆分点原有的判断完全等价,因此"头部 settle、尾部永不 settle"这一不变量现在只存在于一处。两个回调的依赖数组已同步更新:commitPendingThought 不再直接使用 addItem / settlePendingThoughtExpansionhandleThoughtEvent 保留对 settlePendingThoughtExpansion 的直接依赖,因为它在新思考开始时仍需 settle null。三个既有的 settlement 测试(新思考时 settle null、提交时 settle 已提交头部 id、超长拆分时 settle 拆分头部 id 且内容尾部永不 settle)在不改动的情况下全部通过,确认行为完全一致。

  2. rc:3710315904 — R4-4 [建议]:committed + expanded + clickable 的折叠提示缺少测试 — 已解决。 已核实:ConversationMessages.tsxcollapseHint 的 clickable 变体只在 isPending={true} 下被测试渲染,因此把 click 措辞限定在 isPending 上的变异体(PR 前的代码正是如此)能通过本 diff 的全部测试。修复:在 ConversationMessages.test.tsx 中补充平行用例 hints click-to-collapse when committed, expanded and clickable (VP mode),分别在无 clickable 与有 clickable 下渲染 committed + expanded,并断言纯键盘提示与 click 措辞,与既有的 pending 侧测试保持对称。

  3. 审查级说明(非发现项): 自动审查器报告 build-and-test 的 "Integration Tests (CLI, No Sandbox)" 套件在 CI 中被跳过、本地也未运行,因此不在其审查范围内。无行动项——本轮改动的行为(hook 级 settlement 与 Ink 折叠提示)由下述单元测试直接覆盖,并非只能通过打包后的 CLI 或集成测试框架验证,因此本轮无需运行集成测试。

冲突

无(--conflict false);未执行任何合并。

验证

  • cd packages/cli && npx vitest run src/ui/hooks/useGeminiStream.test.tsx src/ui/components/messages/ConversationMessages.test.tsx — 通过:2 个文件,208/208 个测试(包含钉住新包装函数的 3 个 settlement 测试,以及新增的 committed click-to-collapse 测试)
  • npm run typecheck — 通过(退出码 0)
  • npm run lint — 通过(退出码 0)
  • npm run build — 通过(退出码 0)
  • npm run bundle 后的集成测试 — 未运行:改动的行为已由上述单元测试覆盖,并非只能通过打包后的 CLI 或集成测试框架验证
  • npm run generate:settings-schema — 无需执行:未改动任何 settings 源文件

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run.

中文说明

🔀 已更新 base:红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 57 tests pass — this review observed 17127 passed.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan(非阻断):57 tests pass — this review observed 17127 passed

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +181 to +182
// Active while pending too: clicking a streaming thought expands it so the
// reasoning can be watched live (and collapsed again).

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] R5-1: The fullDetail click-suppression rule is encoded twice — in clickable (mouseTrackingEnabled && !fullDetail, ~line 135) and again in the parent onToggle guard (if (!fullDetail) toggleThought(...), ~line 345) — because useMouseEvents here is given a hardcoded { isActive: true }. useMouseEvents already applies the VP / mouse-tracking / TTY gates independently (enabled = isActive && isRawModeSupported && vpGateOpen && mouseTrackingEnabled && Boolean(stdout.isTTY)), so { isActive: clickable } plus an unguarded onToggle is behaviorally identical with one !fullDetail check instead of two. (Distinct from the round-1 thread at line 183, which demanded the then-missing guard; this is about the shape of the resulting double encoding.) — Failure scenario: duplication with drift risk: a future change to the click-swallow policy must update both copies (~200 lines apart, two components); updating only clickable ships a hint advertising clicks that are silently ignored, and updating only the guard advertises nothing while clicks work. The new test 'keeps the click handler active while the thought is pending' pins isActive === true without a VP provider, locking the current encoding. Suggested fix: pass { isActive: clickable } here and keep exactly one of the two !fullDetail checks; update the isActive test to render under VirtualViewportContext.Provider value={true}.

中文说明

[Suggestion] fullDetail 的点击抑制规则被编码了两次——一次在 clickablemouseTrackingEnabled && !fullDetail,约第 135 行),另一次在父级的 onToggle 守卫(if (!fullDetail) toggleThought(...),约第 345 行)——因为此处传给 useMouseEvents 的是硬编码的 { isActive: true }useMouseEvents 本身已独立应用 VP / 鼠标跟踪 / TTY 门控(enabled = isActive && isRawModeSupported && vpGateOpen && mouseTrackingEnabled && Boolean(stdout.isTTY)),因此传 { isActive: clickable } 并去掉 onToggle 上的守卫在行为上完全等价,且只需一处 !fullDetail 检查而非两处。(与第 183 行第 1 轮的那个话题不同:那条评论要求补上当时缺失的守卫;本条针对的是由此产生的双重编码的形态。)——失败场景:重复编码带来漂移风险:未来修改点击吞没策略时必须同步更新两处副本(相距约 200 行、跨两个组件);只改 clickable 会发出一个宣传点击却被静默忽略的提示,只改守卫则点击可用却不做任何宣传。新测试 'keeps the click handler active while the thought is pending' 在没有 VP Provider 的情况下断言 isActive === true,把当前编码钉死了。建议修复:此处传 { isActive: clickable },两处 !fullDetail 检查只保留一处;并把 isActive 测试改为在 VirtualViewportContext.Provider value={true} 下渲染。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +332 to +333
if (!expanded) {
const label = isPending ? pendingLabel : (completedLabel ?? t('Thinking'));

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] R5-2: The ThinkMessageProps.expanded doc comment (line 60) still says "When committed (not pending), whether to show the full reasoning.", but this diff changed the collapsed branch from if (!isPending && !expanded) to if (!expanded), so expanded now gates pending (streaming) thoughts too. — Failure scenario: a maintainer touching ThinkMessage reads the prop doc and concludes pending rendering ignores expanded — e.g. "simplifies" this branch back to if (!isPending && !expanded), or omits expanded when wiring a new pending render surface. That silently re-expands every streaming thought, breaking this PR's click-to-collapse-while-streaming behavior and its tests ('should advertise expansion while pending (streaming)' asserts the pending body is hidden when expanded defaults to false). Suggested fix: update the expanded doc at line 60 to "Whether to show the full reasoning. Applies to both pending (streaming) and committed thoughts; a collapsed thought renders only the header hint line." (the neighbouring clickable doc is also mildly dated and worth touching up in the same pass).

中文说明

[Suggestion] ThinkMessageProps.expanded 的文档注释(第 60 行)仍写着 "When committed (not pending), whether to show the full reasoning.",但本 diff 已把收起分支从 if (!isPending && !expanded) 改为 if (!expanded),因此 expanded 现在同样约束 pending(流式输出中)的思考块。——失败场景:维护者改动 ThinkMessage 时读到该 prop 文档,会误以为 pending 渲染不受 expanded 控制——例如把这个分支"简化"回 if (!isPending && !expanded),或在接入新的 pending 渲染面时漏传 expanded。这会让每个流式思考块静默地重新展开,破坏本 PR 的"流式期间可点击收起"行为及其测试('should advertise expansion while pending (streaming)' 断言了 expanded 缺省为 false 时 pending 正文被隐藏)。建议修复:把第 60 行 expanded 的文档更新为 "Whether to show the full reasoning. Applies to both pending (streaming) and committed thoughts; a collapsed thought renders only the header hint line."(相邻的 clickable 文档也略有陈旧,可一并润色)。

— qwen3.8-max via Qwen Code /review (v0.21.5)

).toBe(true);
});

describe('pending thought expansion keying', () => {

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] R5-3: The read side of the new expansion keying is untested: no test asserts that a pending thought whose key is in expandedHeadIds actually renders expanded — every pending-state test either mocks toggle, asserts hint text, or passes expanded directly. Probe-verified: re-gating resolvedThoughtExpanded on !isPending (in HistoryItemDisplay.tsx) leaves all 94 tests in the three touched component suites green, while a probe asserting that a pending head (sentinel in the set) and a pending tail (head id in the set) render expanded fails under the mutation and passes when reverted. — Failure scenario: a regression making clicks during streaming record state that never renders ships with a green suite (MainContent.test.tsx mocks HistoryItemDisplay — props spy only — so it cannot see the read side). Distinct from R3-1 (the consumption seam, now covered) and R3-4 (fullDetail propagation). Suggested fix: in HistoryItemDisplay.test.tsx, render a pending thought with thoughtHeadId={PENDING_THOUGHT_HEAD_ID} under a ThoughtExpandedProvider whose expandedHeadIds is new Set([PENDING_THOUGHT_HEAD_ID]) and assert the thought text is visible and the collapse hint shows (and, with an empty set, that it is collapsed).

中文说明

[Suggestion] 新展开键控的"读取侧"没有测试覆盖:没有任何测试断言"键在 expandedHeadIds 中的 pending 思考块确实渲染为展开"——现有 pending 用例要么 mock 了 toggle、要么只断言提示文案、要么直接传 expanded。探针验证:在 HistoryItemDisplay.tsx 中给 resolvedThoughtExpanded 重新加上 !isPending 门控后,三个组件测试套件的全部 94 个测试依旧为绿;而一个断言 pending 头部(集合中有哨兵键)与 pending 尾部(集合中有 head id)渲染为展开的探针,在该变异下失败、还原后通过。——失败场景:若回归使"流式期间的点击只写入状态却永不渲染展开",整套测试仍为绿(MainContent.test.tsx mock 了 HistoryItemDisplay——只有 props 探针——看不到读取侧)。与 R3-1(消费接缝,现已覆盖)和 R3-4(fullDetail 传递)是不同的缺口。建议修复:在 HistoryItemDisplay.test.tsx 中,以 thoughtHeadId={PENDING_THOUGHT_HEAD_ID} 渲染一个 pending 思考块,并把 ThoughtExpandedProviderexpandedHeadIds 设为 new Set([PENDING_THOUGHT_HEAD_ID]),断言思考正文可见且显示收起提示(空集合时则断言其收起)。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +1152 to +1153
it('keys a pending thought head off the sentinel in VP mode too', () => {
historyItemDisplayPropsSpy.mockClear();

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] R5-4: The new pending thought expansion keying describe holds five near-verbatim Static/VP test pairs that differ only by useTerminalBuffer: true (sentinel keying with empty history and against committed heads; split-tail keying off the head and off the most recent head; fullDetail forwarding), and the spy-extraction boilerplate (historyItemDisplayPropsSpy.mock.calls.map((c) => c[0]) + .find(...) + .toBeDefined()) is copied ~12 times. — Failure scenario: lockstep-maintenance cost: this PR is itself a prop-plumbing change through both render paths, and the next such change (or a rename of the spy/prop) must edit every copy or silently leaves one asserting a stale shape — review rounds R3-2/R3-5/R3-7 each demanded a missing twin that was answered with another copied block. Both paths genuinely need coverage; the duplication is in the encoding, not the scenarios. Suggested fix: extract a small findPendingProps(type) helper and run the scenario matrix with describe.each([{ vp: false }, { vp: true }]), threading useTerminalBuffer: vp into createUIState; keep the genuinely VP-specific ref-freshness test separate.

中文说明

[Suggestion] 新增的 pending thought expansion keying describe 里有五对几乎逐字相同的 Static/VP 测试,仅差 useTerminalBuffer: true(空历史与有已提交头部两种哨兵键控、尾部以 head 为键、尾部以最近 head 为键、fullDetail 传递),且探针提取样板(historyItemDisplayPropsSpy.mock.calls.map((c) => c[0]) + .find(...) + .toBeDefined())被复制了约 12 次。——失败场景:联动维护成本:本 PR 本身就是一次贯穿两条渲染路径的 prop 管线改动,下一次此类改动(或探针/prop 重命名)必须逐份修改,否则会静默留下某份断言着过期形状——评审第 R3-2/R3-5/R3-7 轮每次都因缺失孪生用例而追加一块复制代码。两条路径确实都需要覆盖;重复在于编码方式,而非场景本身。建议修复:抽出一个小工具函数 findPendingProps(type),用 describe.each([{ vp: false }, { vp: true }]) 跑场景矩阵,并把 useTerminalBuffer: vp 传入 createUIState;真正 VP 专属的 ref 新鲜度测试保持独立。

— qwen3.8-max via Qwen Code /review (v0.21.5)

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 5/100 轮)。改动内容与我反驳保留之处如下:

Round-5 review feedback addressed

Commit: affa5e119fix(cli): address round-5 review on thought expansion (#8443) (4 files, +209 / −238, net −29 lines). No base conflicts (--conflict false, no merge performed).

Findings and dispositions

  • [Suggestion] R5-1 — duplicated fullDetail click-suppression rule (HistoryItemDisplay.tsx) → Fixed. The !fullDetail check now lives in exactly one place: clickable. ClickableThinkMessage passes { isActive: clickable } to useMouseEvents, and the parent onToggle guard (if (!fullDetail) toggleThought(...)) was removed. Equivalence verified against useMouseEvents's gate (enabled = isActive && isRawModeSupported && vpGateOpen && mouseTrackingEnabled && Boolean(stdout.isTTY)): with isActive: clickable, the handler only arms where VP + mouse tracking + !fullDetail all hold, so clicks while fullDetail can never reach toggleThought — same observable behavior, one encoding instead of two. The hint text and the subscription can no longer drift apart. As suggested, the two isActive tests now render under VirtualViewportContext.Provider value={true}, and the fullDetail test (renamed disarms the click handler while fullDetail (Ctrl+O) is active) asserts the handler is disarmed via isActive === false instead of invoking the mocked handler directly.
  • [Suggestion] R5-2 — stale ThinkMessageProps.expanded doc comment (ConversationMessages.tsx) → Fixed. The expanded doc now reads "Whether to show the full reasoning. Applies to both pending (streaming) and committed thoughts; a collapsed thought renders only the header hint line.", matching the if (!expanded) branch. The adjacent clickable doc was touched up too: it now covers both the collapsed expand hint and the expanded collapse hint.
  • [Suggestion] R5-3 — read side of expansion keying untestedFixed. Added renders a pending thought expanded when its expansion key is recorded in HistoryItemDisplay.test.tsx: renders a pending gemini_thought head with thoughtHeadId={PENDING_THOUGHT_HEAD_ID} under a ThoughtExpandedProvider whose expandedHeadIds contains the sentinel (asserts the thought text is visible and the collapse hint shows) and with an empty set (asserts it is collapsed); same pair for a pending gemini_thought_content tail keyed off a committed head id. Mutation-probe verified: re-gating resolvedThoughtExpanded on !isPending makes exactly this new test fail; with the mutation reverted all suites are green.
  • [Suggestion] R5-4 — five near-verbatim Static/VP test pairs + copied spy boilerplate (MainContent.test.tsx) → Fixed. Extracted findHistoryItemDisplayProps(predicate) (reads the spy, asserts a match exists) and converted the four Static/VP pairs in pending thought expansion keying plus the fullDetail-forwarding pair in fullDetail wiring to describe.each([{ vp: false }, { vp: true }]) with useTerminalBuffer: vp threaded into the UI state. The genuinely VP-specific ref-freshness test and the Static-only committed-head test stay separate. Also converted the two tool-group fullDetail tests to the helper.

Review-level observations (no code action): the automated reviewer noted it could not review the Integration Tests (CLI, No Sandbox) job, and that the PR test plan says "57 tests pass" while it observed 17127. This round's changes are component/test-level and fully covered by the unit suites listed below; the touched behavior is not exercised only through the bundled CLI or integration harness, so no integration run was required.

Files changed

  • packages/cli/src/ui/components/HistoryItemDisplay.tsx
  • packages/cli/src/ui/components/HistoryItemDisplay.test.tsx
  • packages/cli/src/ui/components/MainContent.test.tsx
  • packages/cli/src/ui/components/messages/ConversationMessages.tsx

Verification

  • npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/MainContent.test.tsx src/ui/components/messages/ConversationMessages.test.tsx (in packages/cli) — passed, 3 files / 95 tests (94 pre-existing + the new R5-3 read-side test).
  • npx vitest run src/ui/components src/ui/hooks/useMouseEvents.test.tsx (in packages/cli) — 131 of 132 files passed, 2054 tests passed, 1 skipped. The 2 failures are Footer.test.tsx golden snapshots; reproduced identically on the clean pre-change HEAD via git stash (this runner is a Docker container, so the footer renders a docker | sandbox indicator the golden snapshots don't contain). Pre-existing and environment-specific, unrelated to this diff — the PR touches no Footer files.
  • npx vitest run src/ui/AppContainer.test.tsx src/ui/hooks (in packages/cli) — passed, 70 files / 1549 tests.
  • npm run typecheckpassed.
  • npm run lintpassed (full project; re-checked the reformatted file after Prettier).
  • npm run buildpassed.
  • npx prettier --check on the four changed files — passed (after a --write fix to MainContent.test.tsx, re-verified by tests + lint + typecheck).
  • Mutation probe for R5-3: temporarily re-gated resolvedThoughtExpanded on !isPending → the new test failed (only failing test); mutation reverted afterwards.
中文说明

已处理第 5 轮评审反馈

提交:affa5e119fix(cli): address round-5 review on thought expansion (#8443)(4 个文件,+209 / −238,净减 29 行)。无 base 冲突(--conflict false,未执行合并)。

各项反馈及处理

  • [Suggestion] R5-1 — fullDetail 点击抑制规则重复编码(HistoryItemDisplay.tsx)→ 已修复。 !fullDetail 检查现在只存在于一处:clickableClickableThinkMessageuseMouseEvents{ isActive: clickable },并移除了父级 onToggle 守卫(if (!fullDetail) toggleThought(...))。已对照 useMouseEvents 的门控逻辑验证等价性(enabled = isActive && isRawModeSupported && vpGateOpen && mouseTrackingEnabled && Boolean(stdout.isTTY)):传 isActive: clickable 后,只有 VP + 鼠标跟踪 + !fullDetail 同时成立时 handler 才会被激活,因此 fullDetail 期间点击永远到不了 toggleThought —— 可观察行为相同,编码从两处变为一处。提示文案与订阅不再可能互相漂移。按建议,两个 isActive 测试已改为在 VirtualViewportContext.Provider value={true} 下渲染;fullDetail 测试(更名为 disarms the click handler while fullDetail (Ctrl+O) is active)改为断言 handler 通过 isActive === false 被解除武装,而不再直接调用被 mock 的 handler。
  • [Suggestion] R5-2 — ThinkMessageProps.expanded 文档注释过期(ConversationMessages.tsx)→ 已修复。 expanded 的文档现在写的是 "Whether to show the full reasoning. Applies to both pending (streaming) and committed thoughts; a collapsed thought renders only the header hint line.",与 if (!expanded) 分支一致。相邻的 clickable 文档也一并润色:现在同时覆盖收起时的展开提示与展开时的收起提示。
  • [Suggestion] R5-3 — 展开键控的读取侧无测试已修复。 在 HistoryItemDisplay.test.tsx 中新增 renders a pending thought expanded when its expansion key is recorded:以 thoughtHeadId={PENDING_THOUGHT_HEAD_ID} 渲染 pending 的 gemini_thought 头部,ThoughtExpandedProviderexpandedHeadIds 包含哨兵时断言思考正文可见且显示收起提示;空集合时断言其收起;对以已提交 head id 为键的 pending gemini_thought_content 尾部做同样一组断言。变异探针验证:给 resolvedThoughtExpanded 重新加上 !isPending 门控后,恰好只有该新测试失败;还原变异后所有测试套件为绿。
  • [Suggestion] R5-4 — 五对几乎逐字相同的 Static/VP 测试 + 复制的探针样板(MainContent.test.tsx)→ 已修复。 抽出 findHistoryItemDisplayProps(predicate)(读取探针并断言存在匹配),并把 pending thought expansion keying 中的四对 Static/VP 测试与 fullDetail wiring 中的 fullDetail 传递一对,改为 describe.each([{ vp: false }, { vp: true }]),将 useTerminalBuffer: vp 传入 UI state。真正 VP 专属的 ref 新鲜度测试与 Static-only 的已提交头部测试保持独立。两个 tool-group fullDetail 测试也改用该工具函数。

评审级别的两点观察(无需代码动作):自动评审器表示无法审查 Integration Tests (CLI, No Sandbox) 这个 job,并指出 PR 测试计划写的是 "57 tests pass" 而它观察到 17127。本轮改动都在组件/测试层面,由下面列出的单元测试套件完全覆盖;被改动的行为并非只能通过打包后的 CLI 或集成测试框架验证,因此不需要跑集成测试。

变更文件

  • packages/cli/src/ui/components/HistoryItemDisplay.tsx
  • packages/cli/src/ui/components/HistoryItemDisplay.test.tsx
  • packages/cli/src/ui/components/MainContent.test.tsx
  • packages/cli/src/ui/components/messages/ConversationMessages.tsx

验证

  • npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/MainContent.test.tsx src/ui/components/messages/ConversationMessages.test.tsx(在 packages/cli 内)— 通过,3 个文件 / 95 个测试(94 个既有 + 新增的 R5-3 读取侧测试)。
  • npx vitest run src/ui/components src/ui/hooks/useMouseEvents.test.tsx(在 packages/cli 内)— 132 个文件中 131 个通过,2054 个测试通过,1 个跳过。仅有的 2 个失败是 Footer.test.tsx 的黄金快照;通过 git stash 在改动前的干净 HEAD 上复现了完全相同的失败(本 runner 是 Docker 容器,footer 会渲染出黄金快照中没有的 docker | 沙箱指示符)。属于既有且环境相关的失败,与本 diff 无关 —— 本 PR 未触碰任何 Footer 文件。
  • npx vitest run src/ui/AppContainer.test.tsx src/ui/hooks(在 packages/cli 内)— 通过,70 个文件 / 1549 个测试。
  • npm run typecheck通过
  • npm run lint通过(全项目;Prettier 重排后对受影响文件复查过)。
  • npm run build通过
  • 对四个变更文件执行 npx prettier --check通过(先对 MainContent.test.tsx 执行了一次 --write 修复,之后用测试 + lint + typecheck 重新验证)。
  • R5-3 变异探针:临时给 resolvedThoughtExpanded 加上 !isPending 门控 → 新测试失败(且是唯一失败的测试);随后已还原该变异。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 57 tests pass — this review observed 17128 passed.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan(非阻断):57 tests pass — this review observed 17128 passed

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +337 to +343
const lastCommittedThoughtHeadId = useMemo(() => {
for (let i = visibleHistory.length - 1; i >= 0; i--) {
const item = visibleHistory[i];
if (item && item.type === 'gemini_thought') return item.id;
}
return undefined;
}, [visibleHistory]);

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 new lastCommittedThoughtHeadId backward scan re-derives what the existing buildThoughtHeadIdMap(visibleHistory) pass — memoized two lines above with the same visibleHistory dep — already produces: the last gemini_thought head its forward pass visits is exactly the id this backward scan recomputes. The "what counts as a thought head" rule (item.type === 'gemini_thought') now lives in two places (historyUtils.ts and this component), and while both existing shapes in historyUtils.ts (buildThoughtHeadIdMap and the backward findLastUserItemIndex) have unit tests, this inline scan in a 599-line component has none. — Failure scenario: every history change pays two passes deriving the same thought-group structure, and the head-classification rule must change in two places in lockstep when the thought item taxonomy evolves.

Suggested fix: fold the value into the existing pass — e.g. have buildThoughtHeadIdMap return { map, lastHeadId }, or add a findLastThoughtHeadId(items) sibling next to findLastUserItemIndex in historyUtils.ts (with unit tests) — and consume it here instead of the inline memo.

中文说明

[Suggestion] 新增的 lastCommittedThoughtHeadId 反向扫描重复了既有 buildThoughtHeadIdMap(visibleHistory) 遍历(上方两行以相同 visibleHistory 依赖 memo 化):正向遍历见到的最后一个 gemini_thought head 正是该反向扫描重新计算的值。“什么算 thought head”的规则(item.type === 'gemini_thought')因此同时存在于 historyUtils.ts 与本组件两处;historyUtils.ts 中两种形态(buildThoughtHeadIdMap 与反向的 findLastUserItemIndex)均有单元测试,而 599 行组件内的这段内联扫描没有任何测试。— 失败场景:每次 history 变更都要付出两次推导同一 thought-group 结构的遍历;当 thought 条目分类演进时,head 判定规则必须在两处同步修改。

建议修复:将该值并入既有遍历——让 buildThoughtHeadIdMap 返回 { map, lastHeadId },或在 historyUtils.tsfindLastUserItemIndex 旁新增 findLastThoughtHeadId(items)(并补单元测试),此处改为消费它。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +136 to +139
const clickable =
useVirtualViewport(settings.merged.ui?.useTerminalBuffer) &&
mouseTrackingEnabled;
const isActive = !isPending;
mouseTrackingEnabled &&
!fullDetail;

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 forced-open click guard added here only covers fullDetail; its sibling forced-expansion input — the thoughtExpanded prop — is not covered. SessionPreview.tsx renders every thought with thoughtExpanded={true} and no fullDetail, inside AppContainer's real ThoughtExpandedProvider and VirtualViewportContext, so those thoughts stay clickable and the collapse hint this diff introduced still advertises "(click or ctrl+o to collapse)". — Failure scenario: probe-verified with a SessionPreview-exact render (VP mode + default-on mouse tracking): a full press+release fires toggle on the main session's shared expandedThoughtHeadIds set, but resolvedThoughtExpanded short-circuits on the forced prop, so the advertised click can never collapse — it only mutates state the preview never reflects. The pre-diff collapse hint was keyboard-only, so this dead-click advertisement is introduced by this diff; fullDetail got exactly this guard, its forced-open twin did not.

Suggested fix: treat prop-forced expansion like fullDetail — e.g. pass fullDetail={fullDetail || thoughtExpanded === true} to ClickableThinkMessage (or compute a forcedOpen value used for both the hint and the subscription), so SessionPreview gets the keyboard-only hint and a disarmed handler.

中文说明

[Suggestion] 此处新增的强制展开点击守卫只覆盖 fullDetail;其孪生强制展开输入——thoughtExpanded prop——未被覆盖。SessionPreview.tsxthoughtExpanded={true} 且无 fullDetail 渲染每个思考块,并位于 AppContainer 真实的 ThoughtExpandedProviderVirtualViewportContext 之内,因此这些思考块仍可点击,且本 diff 新引入的收起提示仍宣传 “(click or ctrl+o to collapse)”。— 失败场景:已用 SessionPreview 原样渲染探针验证(VP 模式 + 默认开启的鼠标跟踪):完整按下+释放会在主会话共享的 expandedThoughtHeadIds 集合上触发 toggle,但 resolvedThoughtExpanded 被强制 prop 短路,广告出来的点击永远无法真正收起——只会变更预览从不反映的状态。diff 之前收起提示仅提示键盘操作,因此“死点击”广告由本 diff 引入;fullDetail 恰好拿到了这个守卫,其强制展开孪生面没有。

建议修复:将 prop 强制展开与 fullDetail 同等对待——例如向 ClickableThinkMessagefullDetail={fullDetail || thoughtExpanded === true}(或计算 forcedOpen 后同时用于提示与订阅),使 SessionPreview 得到仅键盘的提示并解除点击处理。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +337 to +340
const lastCommittedThoughtHeadId = useMemo(() => {
for (let i = visibleHistory.length - 1; i >= 0; i--) {
const item = visibleHistory[i];
if (item && item.type === 'gemini_thought') return item.id;

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] No test covers the double-split state — a committed gemini_thought_content tail in history alongside a still-pending tail — so this head-only scan is unpinned there; every existing keying test populates history with gemini_thought heads only. — Failure scenario: mutation-verified: a one-line mutant accepting 'gemini_thought_content' in this scan survives all 31 existing MainContent.test.tsx tests, and a double-split probe then fails expected 43 to be 42 — the pending tail keys off the committed tail id instead of the head, so expanding the head (toggle 42) no longer expands the streaming tail. A thought exceeding 2×STREAM_PENDING_ITEM_MAX_CHARS reaches this state in production.

Suggested fix: add a case to the pending thought expansion keying describe (it runs for both vp variants automatically): history [{ id: 42, type: 'gemini_thought' }, { id: 43, type: 'gemini_thought_content' }] + pendingHistoryItems: [{ type: 'gemini_thought_content', … }]expect(pendingTail.thoughtHeadId).toBe(42).

中文说明

[Suggestion] 没有测试覆盖“双切分”状态——history 中同时存在已提交的 gemini_thought_content 尾块与仍 pending 的尾块,因此这段只认 head 的扫描在该状态下无测试钉住;现有所有 keying 测试的 history 只包含 gemini_thought head。— 失败场景:已做变异验证:让该扫描接受 'gemini_thought_content' 的单行变异可通过全部 31 个既有 MainContent.test.tsx 测试;双切分状态探针随即报 expected 43 to be 42——pending 尾块被键到已提交尾块 id 而非 head id,展开 head(toggle 42)不再展开流式尾块。超过 2×STREAM_PENDING_ITEM_MAX_CHARS 的思考在生产环境即进入该状态。

建议修复:在 pending thought expansion keying describe 中新增用例(describe.each 会自动覆盖两个 vp 变体):history [{ id: 42, type: 'gemini_thought' }, { id: 43, type: 'gemini_thought_content' }] + pendingHistoryItems: [{ type: 'gemini_thought_content', … }]expect(pendingTail.thoughtHeadId).toBe(42)

— qwen3.8-max via Qwen Code /review (v0.21.5)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +502 to +505
// Migrate a streamed thought's provisional pending expansion to its real
// committed head id when it commits (or drop it when passed `null`). See
// settlePendingThoughtExpansion in AppContainer.
settlePendingThoughtExpansion?: (committedHeadId: number | null) => void,

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] R7-1: The new settlePendingThoughtExpansion callback is appended as the 26th positional parameter of useGeminiStream, perpetuating a parameter list whose cost is already visible in the test harness rather than being generalised into a trailing options object. — Failure scenario: no runtime defect today (the AppContainer call site and the hook signature agree), but the concrete cost is the one this diff itself pays: useGeminiStream.test.tsx must reference parameter types by numeric index (Parameters<typeof useGeminiStream>[15], [20], [24], and now [25]) and pad the call with positional undefined placeholders. The next callback threaded through this hook repeats that dance, and adjacent optional params with identical types (slots 22/23, both React.RefObject<number>) are exactly the shape where an inserted/misordered argument binds to the wrong slot without a type error. Suggested fix: fold the trailing optional refs/callbacks into a single trailing options object so additions are name-keyed (e.g. opts?: { settlePendingThoughtExpansion?: ...; goalQueueRef?: ...; ... }); at minimum, do not append a parameter 27 positionally. No change is required inside this PR.

中文说明

[建议] 新增的 settlePendingThoughtExpansion 回调作为第 26 个位置参数追加到 useGeminiStream,延续了这套参数列表——其代价在测试脚手架中已清晰可见——而不是收敛为一个尾置 options 对象。— 失败场景:当前没有运行时缺陷(AppContainer 调用点与 hook 签名一致),但具体代价是本 diff 自身正在支付的:useGeminiStream.test.tsx 必须按数字下标引用参数类型(Parameters<typeof useGeminiStream>[15][20][24],现在是 [25]),并用位置 undefined 占位符补齐调用。下一个接入该 hook 的回调会重复这套操作;而相邻的同类型可选参数(第 22/23 位,均为 React.RefObject<number>)正是那种插入/错位的参数会绑到错误槽位且不产生类型错误的形状。建议修复:将尾部的可选 ref/回调收敛进单个尾置 options 对象,使新增项按名称键控(如 opts?: { settlePendingThoughtExpansion?: ...; goalQueueRef?: ...; ... });至少不要再以位置参数形式追加第 27 个参数。本 PR 内无需改动。

— qwen3.8-max via Qwen Code /review (v0.21.5)

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.

Declined for this PR. The finding itself states no change is required here, and folding ~26 positional parameters into a trailing options object is a standalone refactor of the hook signature, the AppContainer call site, and the test harness's Parameters<typeof useGeminiStream>[N] references — out of scope for this late-round bugfix. The new parameter follows the existing positional pattern, and the call site and signature were updated together (typecheck passes), so no slot mismatch is introduced; the options-object migration belongs in a follow-up refactor.

中文说明

本 PR 内拒绝处理。该 finding 本身已声明本 PR 内无需改动;将约 26 个位置参数收敛为尾置 options 对象是一次独立重构,涉及 hook 签名、AppContainer 调用点以及测试脚手架中的 Parameters<typeof useGeminiStream>[N] 引用——超出本轮后期 bugfix 的范围。新增参数沿用既有位置参数模式,且调用点与签名同步更新(typecheck 通过),不会引入槽位错位;options 对象迁移应放到后续独立重构中完成。

Comment thread packages/cli/src/ui/AppContainer.tsx Outdated
Comment on lines +798 to +800
const next = new Set(prev);
next.delete(PENDING_THOUGHT_HEAD_ID);
if (committedHeadId != null) next.add(committedHeadId);

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] R7-2: This reducer's set-update logic — the mechanism that keeps a click-expanded streaming thought expanded after it commits — has zero test coverage at any layer. A harness-validated test-efficacy probe found a surviving mutant: deleting next.delete(PENDING_THOUGHT_HEAD_ID); leaves every test green. Every test layer stubs the other side: the useGeminiStream tests pass a vi.fn() spy (pinning only invocation), the MainContent/HistoryItemDisplay tests inject static ThoughtExpandedProvider values, and AppContainer.test.tsx mocks useGeminiStream entirely. This is the still-uncovered half of the round-2 thread on this reducer: the call-site spy tests were added in later rounds, but the reducer unit test proposed in the same thread still does not exist. — Failure scenario: with the .delete() removed (e.g. a future refactor "simplifying" the migration), the -1 sentinel persists in expandedThoughtHeadIds after a thought commits, so the next streaming thought — keyed by the sentinel while pending — renders already expanded with no user action; deleting next.add(committedHeadId) ships the round-1 regression (a thought the user clicked open during streaming snaps collapsed the moment it commits) equally green. The harness confirmed the mutant survives with the full suite green, so either regression would ship silently. Suggested fix: extract the set-update into a small pure function and unit-test its three branches (no sentinel → returns prev unchanged; settle(id) → sentinel removed and id added; settle(null) → sentinel dropped):

export function settlePendingExpansion(
  prev: ReadonlySet<number>,
  committedHeadId: number | null,
): ReadonlySet<number> {
  if (!prev.has(PENDING_THOUGHT_HEAD_ID)) return prev;
  const next = new Set(prev);
  next.delete(PENDING_THOUGHT_HEAD_ID);
  if (committedHeadId != null) next.add(committedHeadId);
  return next;
}
中文说明

[建议] 该 reducer 的集合更新逻辑——让流式期间被点击展开的思考块在提交后保持展开的机制——在任何层级都没有测试覆盖。经 harness 校验的测试有效性探针发现存活变异体:删除 next.delete(PENDING_THOUGHT_HEAD_ID); 后所有测试仍全绿。每一层测试都把另一侧 stub 掉了:useGeminiStream 测试传入 vi.fn() 间谍(只钉住调用本身)、MainContent/HistoryItemDisplay 测试注入静态 ThoughtExpandedProvider 值、AppContainer.test.tsx 则整体 mock 掉 useGeminiStream。这是第 2 轮关于该 reducer 的评论中仍未覆盖的一半:调用点间谍测试已在后续轮次补上,但同一评论中提议的 reducer 单元测试至今不存在。— 失败场景:若删除该 .delete()(例如未来某次“简化”该迁移的重构),-1 哨兵会在思考提交后残留在 expandedThoughtHeadIds 中,于是下一个流式思考块——pending 期间以哨兵为键——会在用户未做任何操作时直接以展开状态渲染;而删除 next.add(committedHeadId) 则会以同样全绿的结果放行第 1 轮的回归(用户在流式期间点击展开的思考块在提交瞬间立刻折叠)。harness 已确认该变异体在整个套件全绿的情况下存活,因此两种回归都会悄无声息地进入发布。建议修复:将集合更新抽取为一个小型纯函数并单测其三个分支(无哨兵 → 原样返回 prevsettle(id) → 移除哨兵并加入 idsettle(null) → 丢弃哨兵),代码形状见上方英文部分。

— qwen3.8-max via Qwen Code /review (v0.21.5)

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 7/100 轮)。改动内容与我反驳保留之处如下:

Round-7 review response

Feedback dispositions

R7-1 — settlePendingThoughtExpansion appended as the 26th positional parameter of useGeminiStream (Suggestion) — Declined.
The finding itself states "No change is required inside this PR". Folding the ~26 positional parameters into a trailing options object is a standalone refactor spanning the hook signature, the AppContainer call site, and the test harness (all the Parameters<typeof useGeminiStream>[N] index references), which is out of scope for this bugfix PR and would balloon the diff in a late round. It is worth doing as its own follow-up refactor, reviewed on its own merits. The parameter added here follows the existing positional pattern, and the call site and signature were updated together in this diff (typecheck passes), so no slot mismatch is introduced.

R7-2 — reducer set-update logic has zero test coverage; surviving mutant (Suggestion) — Addressed.
Extracted the set-update into a pure settlePendingExpansion(prev, committedHeadId) function in ThoughtExpandedContext.tsx, next to the PENDING_THOUGHT_HEAD_ID sentinel it operates on. AppContainer's settlePendingThoughtExpansion callback now delegates to it. Added a colocated unit test ThoughtExpandedContext.test.tsx covering all three branches:

  • no sentinel in prev → returns prev by reference, unchanged
  • settle(id) → sentinel removed, committed id added, other entries preserved, prev not mutated
  • settle(null) → sentinel dropped, other entries preserved

The extraction also shrinks AppContainer: the inline updater and its comment block were removed, since the pure function's doc comment carries the same "why".

Mutant check: removing next.delete(PENDING_THOUGHT_HEAD_ID); — the exact surviving mutant named in the finding — now fails 2 of the 3 new tests, and removing next.add(committedHeadId) fails the committed-id assertion. Verified by temporarily applying the deletion mutation, running the new test file (2 failed), and restoring the file.

Conflict notes

None — --conflict false; no merge performed.

Verification

  • cd packages/cli && npx vitest run src/ui/contexts/ThoughtExpandedContext.test.tsx src/ui/hooks/useGeminiStream.test.tsx src/ui/AppContainer.test.tsx src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/MainContent.test.tsx src/ui/components/messages/ConversationMessages.test.tsx src/ui/utils/historyUtils.test.ts — passed (7 files, 453 tests)
  • Mutation probe: temporarily removed next.delete(PENDING_THOUGHT_HEAD_ID); and reran the new test file — 2 of 3 tests failed as expected; file restored, suite green again
  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • Integration tests: not run. The change is a pure-function extraction inside the React state layer plus unit tests; the behavior is not exercised through the bundled CLI or integration harness (this PR's integration suite was likewise skipped in CI).
中文说明

第 7 轮审查回应

反馈处理结论

R7-1 —— settlePendingThoughtExpansion 作为第 26 个位置参数追加到 useGeminiStream(建议) —— 拒绝。
该 finding 本身已声明"本 PR 内无需改动"。将约 26 个位置参数收敛为尾置 options 对象是一次独立重构,涉及 hook 签名、AppContainer 调用点以及测试脚手架(所有 Parameters<typeof useGeminiStream>[N] 数字下标引用),超出本 bugfix PR 的范围,且会在后期轮次大幅膨胀 diff。它值得作为独立的后续重构去做,并按其自身价值单独评审。本次新增的参数沿用既有位置参数模式,且调用点与签名在本 diff 中同步更新(typecheck 通过),因此不会引入槽位错位。

R7-2 —— reducer 集合更新逻辑零测试覆盖;存在存活变异体(建议) —— 已处理。
将集合更新抽取为纯函数 settlePendingExpansion(prev, committedHeadId),置于 ThoughtExpandedContext.tsx 中、紧邻其操作的 PENDING_THOUGHT_HEAD_ID 哨兵。AppContainer 的 settlePendingThoughtExpansion 回调现改为委托该函数。新增同目录单元测试 ThoughtExpandedContext.test.tsx,覆盖全部三个分支:

  • prev 中无哨兵 → 按引用原样返回 prev
  • settle(id) → 移除哨兵、加入已提交 id、保留其他条目,且不改动 prev
  • settle(null) → 丢弃哨兵、保留其他条目

此次抽取还让 AppContainer 变小:内联 updater 及其注释块被移除,因为纯函数的文档注释已承载同样的"为什么"。

变异体检验:删除 next.delete(PENDING_THOUGHT_HEAD_ID); —— 即 finding 中点名的存活变异体 —— 现在会导致 3 个新测试中的 2 个失败;删除 next.add(committedHeadId) 会使已提交 id 断言失败。已通过临时应用该删除变异、运行新测试文件(2 个失败)后恢复文件完成验证。

冲突说明

无 —— --conflict false;未执行任何合并。

验证

  • cd packages/cli && npx vitest run src/ui/contexts/ThoughtExpandedContext.test.tsx src/ui/hooks/useGeminiStream.test.tsx src/ui/AppContainer.test.tsx src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/MainContent.test.tsx src/ui/components/messages/ConversationMessages.test.tsx src/ui/utils/historyUtils.test.ts —— 通过(7 个文件,453 个测试)
  • 变异探针:临时删除 next.delete(PENDING_THOUGHT_HEAD_ID); 并重跑新测试文件 —— 3 个测试中 2 个按预期失败;恢复文件后套件重新全绿
  • npm run build —— 通过
  • npm run typecheck —— 通过
  • npm run lint —— 通过
  • 集成测试:未运行。本次改动是 React 状态层内的纯函数抽取加单元测试;该行为不通过打包后的 CLI 或集成测试框架执行(本 PR 的集成套件在 CI 中同样被跳过)。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +26 to +29
export function settlePendingExpansion(
prev: ReadonlySet<number>,
committedHeadId: number | null,
): ReadonlySet<number> {

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] R8-1: Click-vs-commit race — a press→release that straddles the thought's commit is a dead click (or a wrong-key toggle). On commit the thought moves between render sites with different keys: the VP key extractor yields p-${-item.id - 1} for pending items vs h-${item.id} for committed items (MainContent.tsx:109-114), and the non-VP path renders committed items in <Static> vs pending items in a separate live region. The pending ClickableThinkMessage therefore unmounts and a fresh committed instance mounts with pressRef.current === null. Any commit landing inside the ~50–150 ms press→release interval kills the click: the release either fires no toggle at all (the press record is lost in the remount), or — via the pre-commit subscriber — fires toggle(PENDING_THOUGHT_HEAD_ID) after settlePendingExpansion already migrated the sentinel: the collapse direction leaves {headId, sentinel}, so the thought stays expanded despite the click; the expand direction leaves a stray sentinel, so the committed thought renders collapsed despite the click.

Failure scenario: in VP mode with mouse tracking, the user presses on a streaming thought's line and the thought commits before release (the window sits exactly on the thinking→answer transition) → the click does nothing, or toggles the stale sentinel, and the expansion state ends up opposite to (or unchanged by) the user's intent. Probe-verified with flip checks: a commit-before-press control toggles the committed id and the frames flip correctly; reverting to pre-PR isActive: clickable && !isPending removes the armed surface. Introduced by this diff — pre-PR, pending thoughts never subscribed. The cost is an occasional dead/misfired click at the boundary; re-click or keyboard recovers, and a stray sentinel self-cleans at the next thought start's settle(null).

Suggested fix: allocate the thought head's history id when the thought starts and use it as the expansion key (and item key) through pending→committed — stable identity means no migration and the pressRef survives the transition; alternatively, document/accept the dead click.

中文说明

[建议] R8-1:点击与提交的竞态——跨越思考块提交时刻的"按下→释放"点击会成为死点击(或切换到错误的键)。提交时思考块在不同渲染位置之间迁移且键不同:VP 键提取器对 pending 项生成 p-${-item.id - 1}、对已提交项生成 h-${item.id}MainContent.tsx:109-114);非 VP 路径中已提交项渲染于 <Static>、pending 项渲染于独立的实时区域。因此 pending 的 ClickableThinkMessage 会卸载,新挂载的已提交实例 pressRef.current === null。任何落在约 50–150 ms 按下→释放区间内的提交都会使点击失效:释放事件要么完全不触发 toggle(press 记录随重挂载丢失),要么经由提交前的订阅者在 settlePendingExpansion 已迁移哨兵之后触发 toggle(PENDING_THOUGHT_HEAD_ID):收起方向残留 {headId, sentinel},点击后思考块仍保持展开;展开方向残留孤立哨兵,已提交的思考块反而渲染为收起。

失败场景:VP 模式 + 鼠标跟踪下,用户在流式思考块上按下鼠标,思考块在释放前提交(窗口恰在思考→回答的转换点)→ 点击无效或切换了过期哨兵,展开状态与用户意图相反(或毫无变化)。已通过探针验证并做翻转对照:提交先于按下的对照组可正确切换已提交 id、画面正常翻转;回退到 PR 前的 isActive: clickable && !isPending 则解除点击武装。该竞态由本 diff 引入——PR 前 pending 思考块从不订阅鼠标事件。代价为边界处偶发的失效/误触发点击;重新点击或键盘操作可恢复,孤立哨兵会在下一思考块开始时的 settle(null) 中自动清理。

建议修复:在思考块开始时预分配其历史 id,并在 pending→committed 全程以该 id 作为展开键(与列表项键)——身份稳定后无需迁移,pressRef 也能跨越转换存活;或者在文档中说明/接受该失效点击。

— qwen3.8-max via Qwen Code /review (v0.21.5)

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 AutoFix ran out of time before finishing (timeout (1080000ms)) (attempt 8/100) — it will retry on the next scan.

What I found before stopping:
Qwen failed during address-review: timeout (1080000ms).

See the Qwen Autofix agent step logs for model/tool output.

Run log: https://github.com/QwenLM/qwen-code/actions/runs/30978469269


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 9/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 9/100 轮)。改动内容与我反驳保留之处如下:

Round 8 review response — PR #8443

Feedback points and dispositions

R8-1 (rc:3718059310) — [Suggestion] Click-vs-commit race on pending thought expansion → accepted and documented (the reviewer's explicit fallback)

The finding is real and was probe-verified: a press→release that straddles a thought's commit can dead-click or toggle the stale sentinel, because the pending ClickableThinkMessage unmounts at commit (its pressRef is lost) while a fresh committed instance mounts.

Two code-level facts guided the disposition:

  1. Click-to-expand is VP-mode-only: clickable requires useVirtualViewport(...), which AppContainer provides as true only when useTerminalBuffer is on. The finding's non-VP branch (<Static> vs live region) is inert — no mouse subscription exists there at all.
  2. The suggested primary fix — pre-allocating the thought's history id at thought start and using it as a stable key across pending→committed — would not by itself close the race: VirtualizedListItem wraps committed items in StaticRender, so the live→static flip at commit remounts the content subtree even with a stable key. A complete fix would additionally require restructuring VirtualizedList's live→static transition plus an id-reservation mechanism in useHistoryManager (disturbing monotonic id allocation) — a core VP-infrastructure change disproportionate to the race's cost.

The reviewer explicitly offered "alternatively, document/accept the dead click". We take that option: the PENDING_THOUGHT_HEAD_ID doc comment now records the accepted race — its bounded window (one click duration at the commit instant, VP mouse mode only), the recovery path (re-click or keyboard), and the stray-sentinel self-clean via settle(null) at the next thought start.

Conflict: none (--conflict false; no merge performed).

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • Focused Vitest (packages/cli): src/ui/contexts/ThoughtExpandedContext.test.tsx, src/ui/components/HistoryItemDisplay.test.tsx, src/ui/components/MainContent.test.tsx — 3 files, 80 tests, all passed
  • Integration tests: not applicable — comment-only change, no behavior change
  • npm run generate:settings-schema: not required — no settings source changed
中文说明

第 8 轮审查回应 — PR #8443

反馈点与处置

R8-1 (rc:3718059310) — [建议] pending 思考块展开的点击与提交竞态 → 接受并文档化(审查者明确给出的备选方案)

该发现属实且已经过探针验证:跨越思考块提交时刻的"按下→释放"点击会成为死点击或切换到过期哨兵,因为 pending 的 ClickableThinkMessage 会在提交时卸载(其 pressRef 丢失),同时新挂载一个已提交实例。

两个代码层面的事实决定了本次处置:

  1. 点击展开仅在 VP 模式下生效:clickable 要求 useVirtualViewport(...),而 AppContainer 只在 useTerminalBuffer 开启时将其提供为 true。该发现中的非 VP 分支(<Static> 与实时区域)实际是惰性的——那里根本不存在鼠标订阅。
  2. 建议的主修复方案——在思考块开始时预分配其历史 id,并在 pending→committed 全程以该 id 作为稳定键——本身并不能消除竞态:VirtualizedListItem 会把已提交项包裹进 StaticRender,因此提交时 live→static 的切换即使键稳定也会重挂载内容子树。完整修复还需要重构 VirtualizedList 的 live→static 转换,并在 useHistoryManager 中引入 id 预留机制(破坏单调 id 分配假设)——这是与该竞态代价不相称的 VP 核心基础设施改动。

审查者明确给出了"或者在文档中说明/接受该失效点击"的备选方案。我们采纳该方案:PENDING_THOUGHT_HEAD_ID 的文档注释现已记录这一被接受的竞态——其有限窗口(提交瞬间约一次点击的时长,仅限 VP 鼠标模式)、恢复途径(重新点击或键盘操作),以及孤立哨兵在下一思考块开始时经 settle(null) 的自动清理。

冲突:无(--conflict false;未执行合并)。

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • 聚焦 Vitest(packages/cli):src/ui/contexts/ThoughtExpandedContext.test.tsxsrc/ui/components/HistoryItemDisplay.test.tsxsrc/ui/components/MainContent.test.tsx — 3 个文件、80 个测试全部通过
  • 集成测试:不适用——仅注释改动,无行为变化
  • npm run generate:settings-schema:不需要——未改动配置源

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 57 tests pass — this review observed 17138 passed.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan(非阻断):57 tests pass — this review observed 17138 passed

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +789 to +791
const settlePendingThoughtExpansion = useCallback(
(committedHeadId: number | null) => {
setExpandedThoughtHeadIds((prev) =>

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] R9-1: The reducer glue settlePendingThoughtExpansion — the mechanism that keeps a click-expanded streaming thought expanded after it commits — is exercised by no test at any layer. Each half is tested in isolation (the pure settlePendingExpansion in ThoughtExpandedContext.test.tsx; the hook's settle call timing in useGeminiStream.test.tsx via an injected spy), but nothing wires this real callback to the real state setter. — Failure scenario: probe-verified — the mutation settlePendingExpansion(prev, committedHeadId)settlePendingExpansion(prev, null) compiles cleanly and passes all 453 tests across the 7 relevant suites; shipped, it would make every thought the user clicked open during streaming collapse at commit — the exact regression this migration exists to prevent. The sibling mutation settlePendingExpansion(new Set(), committedHeadId) also survives (407/407) and would wipe every other expanded thought on each commit.

Suggested fix: add one integration test over this wiring — with the real provider state, record an expansion under PENDING_THOUGHT_HEAD_ID, invoke this callback with a committed head id, and assert the committed thought renders expanded while other set entries survive.

中文说明

[建议] R9-1:reducer 胶水 settlePendingThoughtExpansion —— 让流式期间点击展开的思考块在提交后保持展开的机制 —— 没有任何层级的测试覆盖。两半被隔离测试(ThoughtExpandedContext.test.tsx 中的纯函数 settlePendingExpansionuseGeminiStream.test.tsx 中通过注入 spy 测试 hook 的 settle 调用时机),但没有任何测试把这个真实回调与真实状态 setter 连接起来。— 失败场景:已探针验证 —— 变异 settlePendingExpansion(prev, committedHeadId)settlePendingExpansion(prev, null) 可编译并通过全部 7 个相关套件的 453 个测试;若带病上线,用户在流式期间点击展开的每个思考块都会在提交时折叠 —— 正是本迁移机制要防止的回归。兄弟变异 settlePendingExpansion(new Set(), committedHeadId) 同样存活(407/407),会在每次提交时清空所有其他已展开的思考块。

建议修复:为此接线添加一个集成测试 —— 使用真实 provider 状态,在 PENDING_THOUGHT_HEAD_ID 下记录一次展开,用已提交的 head id 调用此回调,并断言已提交的思考块渲染为展开状态且集合中其他条目保留。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +1718 to +1720
// A fresh thought begins; drop any stale provisional expansion key
// left over from a pending thought that never committed.
settlePendingThoughtExpansion?.(null);

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] R9-2: The if (startingNewThought) guard around this settle(null) call is pinned by no test — all three new settle tests stream exactly one Thought event per turn, so no test exercises a continuation Thought event with a settle spy attached. — Failure scenario: probe-verified — moving settlePendingThoughtExpansion?.(null) outside the guard passes all 186 useGeminiStream tests unchanged; a flip probe streaming two description-carrying Thought events showed settle calls [null, null, 1001] with the mutation vs [null, 1001] with the guard. Shipped without the guard: the model streams a continuation Thought chunk for the same thought while the user has it expanded → the sentinel is dropped mid-stream → the thought block collapses live while being read.

Suggested fix: add a test yielding two ServerGeminiEventType.Thought events for one thought and assert settle fires with null exactly once (first event only), then with the committed id at commit — e.g. expect(settle).toHaveBeenNthCalledWith(1, null) and expect(settle).toHaveBeenCalledTimes(2) after the commit.

中文说明

[建议] R9-2:此 settle(null) 调用外层的 if (startingNewThought) 守卫没有任何测试钉住 —— 三个新增 settle 测试均只流式输出一个 Thought 事件,因此没有测试在挂载 settle spy 的情况下执行续传 Thought 事件。— 失败场景:已探针验证 —— 把 settlePendingThoughtExpansion?.(null) 移出守卫后,全部 186 个 useGeminiStream 测试原样通过;翻转探针流式输出两个带 description 的 Thought 事件,变异下 settle 调用为 [null, null, 1001],守卫存在时为 [null, 1001]。若带病上线:模型为同一思考块续流一个 Thought 分块而用户已将其展开 → 哨兵在流式中途被丢弃 → 思考块在用户阅读时实时折叠。

建议修复:添加一个为一个思考块产出两个 ServerGeminiEventType.Thought 事件的测试,并断言 settle 仅以 null 触发一次(仅首个事件),随后在提交时以已提交 id 触发 —— 例如 expect(settle).toHaveBeenNthCalledWith(1, null),且提交后 expect(settle).toHaveBeenCalledTimes(2)

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment on lines +270 to +272
// fullDetail and `thoughtExpanded === true` both pin the thought open, so
// a click could never collapse it — ClickableThinkMessage disarms itself.
const forcedOpen = fullDetail || thoughtExpanded === true;

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] R9-4: forcedOpen misses the allExpanded (Ctrl+O) case on surfaces that don't forward it as fullDetail — specifically AgentChatContent, which passes neither fullDetail nor thoughtExpanded while agentHistoryAdapter.ts emits gemini_thought items. Ctrl+O force-expands those thoughts via context, but clickable stays true. — Failure scenario: probe-verified — in VP mode with mouse tracking, press Ctrl+O on an agent-view transcript: the handler stays armed and the header advertises "(click or ctrl+o to collapse)" (pre-PR the collapse hint was keyboard-only, so this false advertising is introduced by this diff). Clicks can never collapse the row while allExpanded dominates; they silently flip expandedHeadIds, and the accumulated invisible toggles surface when Ctrl+O turns off — state opposite to the user's repeated collapse clicks. This also breaks the invariant the added comment states ("a forced-open thought … can never collapse on click"). Probe flips with the fix below: disarmed handler, keyboard-only hint.

Suggested change
// fullDetail and `thoughtExpanded === true` both pin the thought open, so
// a click could never collapse it — ClickableThinkMessage disarms itself.
const forcedOpen = fullDetail || thoughtExpanded === true;
// fullDetail, Ctrl+O full-detail (`allExpanded`), and `thoughtExpanded ===
// true` all pin the thought open, so a click could never collapse it —
// ClickableThinkMessage disarms itself.
const forcedOpen = fullDetail || allExpanded || thoughtExpanded === true;

(or have AgentChatContent forward fullDetail={allExpanded} the way MainContent does)

中文说明

[建议] R9-4:forcedOpen 遗漏了未把 allExpanded(Ctrl+O)以 fullDetail 形式转发的渲染面 —— 具体是 AgentChatContent:它既不传 fullDetail 也不传 thoughtExpanded,而 agentHistoryAdapter.ts 会产出 gemini_thought 项。Ctrl+O 通过 context 强制展开这些思考块,但 clickable 仍为 true。— 失败场景:已探针验证 —— VP 模式 + 鼠标跟踪下,在 agent 视图会话上按 Ctrl+O:处理器保持武装,头部显示 "(click or ctrl+o to collapse)"(PR 前收起提示仅为键盘措辞,因此这一虚假宣传由本 diff 引入)。在 allExpanded 主导期间点击永远无法收起该行;点击只会静默翻转 expandedHeadIds,累积的不可见切换会在 Ctrl+O 关闭时浮现 —— 状态与用户反复点击"收起"的意图相反。这同时违反了新增注释所声明的不变量("强制展开的思考块不可能被点击收起")。应用下方修复后探针翻转:处理器解除武装、提示变为纯键盘措辞。

(或者让 AgentChatContentMainContent 一样转发 fullDetail={allExpanded}

— qwen3.8-max via Qwen Code /review (v0.21.5)

@wenshao

wenshao commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Review — PR #8443 (head 84c298e)

What this does

Makes a streaming thought block click-expandable, instead of only after it commits:

  • ClickableThinkMessage drops isActive = !isPending and arms the mouse handler off a single clickable predicate (VP mode + mouse tracking + not forced open); the measuring ref is now always attached.
  • ThinkMessage moves the collapsed one-liner from if (!isPending && !expanded) to if (!expanded), so a pending-and-collapsed thought renders the same hint line (with the pending icon + elapsed time), and the collapse hint is no longer gated on !isPending.
  • New PENDING_THOUGHT_HEAD_ID sentinel + pure settlePendingExpansion() migrate a "expanded while streaming" toggle onto the real head id once the thought commits; useGeminiStream calls it via a new (26th, optional) positional param wired from AppContainer.
  • MainContent keys pending thoughts: a pending gemini_thought head → sentinel; a pending gemini_thought_content tail (post-split) → findLastThoughtHeadId(visibleHistory), via a ref on the VP path so renderVirtualItem's identity stays stable.

The design is sound and the hard parts are the ones that got the attention: the head/tail split keying, the double-split case, VP-vs-Static parity, and the ref-freshness case are all covered by tests, and the press→release-straddles-commit race is explicitly documented as accepted rather than hand-waved. settlePendingExpansion being a pure, separately unit-tested function is the right call.

Worth noting the body-visibility semantics are unchanged: ThinkBody already returned null when !expanded on main, so this PR does not start or stop rendering reasoning text by itself — it only makes the toggle reachable while pending. Good.

Verification I ran

Against 84c298e in an isolated worktree:

  • vitest run HistoryItemDisplay.test.tsx ConversationMessages.test.tsx ThoughtExpandedContext.test.tsx historyUtils.test.ts MainContent.test.tsx126 passed
  • vitest run useGeminiStream.test.tsx186 passed (incl. the 3 new settlement tests; the vi.useFakeTimers() in the split test does not leak into the rest of the file)
  • eslint on all 7 changed source files → clean
  • tsc --noEmit not run (needs packages/core built first); CI covers it.

Findings

1. forcedOpen misses the allExpanded route — the guard is half-applied (medium)

HistoryItemDisplay.tsx:266-272

const resolvedThoughtExpanded =
  fullDetail || (thoughtExpanded ?? (allExpanded || expandedHeadIds.has(thoughtGroupHeadId)));
const forcedOpen = fullDetail || thoughtExpanded === true;

resolvedThoughtExpanded can be pinned open by three things — fullDetail, thoughtExpanded, and the context allExpanded — but forcedOpen only covers two. Inside MainContent this is invisible, because MainContent sets fullDetail = allExpanded (MainContent.tsx:137) so the two coincide. But HistoryItemDisplay has other callers under the same provider that never forward fullDetail:

  • agent-view/AgentChatContent.tsx:235,252
  • QuittingDisplay.tsx:26

There, with Ctrl+O on, the thought is force-expanded via allExpanded while forcedOpen is false → the handler stays armed and the hint now reads (click or ctrl+o to collapse). Clicking silently adds the head id to expandedHeadIds; the display doesn't move, and the thought flips state later when Ctrl+O is turned off. That is exactly the failure mode the PR's own test documents ("a click cannot silently record a per-item toggle that would flip once fullDetail turns off") — just via the other route.

Probe-verified on this head (VP context, allExpanded: true, no fullDetail prop): useMouseEvents opts isActive === true and the frame contains click.

Also note thoughtExpanded === false force-closes the thought and is equally un-toggleable, so === true is the wrong half of the predicate even ignoring allExpanded. Suggested:

const forcedOpen = fullDetail || thoughtExpanded !== undefined || allExpanded;

(Alternatively, forward fullDetail from AgentChatContent/QuittingDisplay — but the local predicate is the cheaper, non-leaky fix, and it makes the invariant "forcedOpen ⇔ a click cannot change resolvedThoughtExpanded" actually hold.)

2. (click or ctrl+o to collapse) overstates what ctrl+o does (medium-low)

ConversationMessages.tsx:353-355 — the collapse hint always names the key:

const collapseHint = clickable
  ? ` ${t('(click or {{keyHint}} to collapse)', ...)}`
  : ` ${t('({{keyHint}} to collapse)', ...)}`;

For a thought expanded via the per-head click set, ctrl+o cannot collapse it: turning full-detail on keeps it expanded, and turning it off falls back to expandedHeadIds.has(id) → still expanded. The PR's own test comment says as much ("A click-expanded thought cannot be collapsed by ctrl+o"), yet the rendered string still advertises it.

The inverse case is already correct (Ctrl+O-expanded ⇒ forcedOpen ⇒ hint is key-only), so the fix is symmetric: when the expansion came from the click set rather than allExpanded/fullDetail, render (click to collapse). That needs one more bit threaded into ThinkMessage (e.g. keyCanCollapse), so it's a judgement call whether it's worth it — but the current wording is a small, newly-reachable lie on the pending path.

3. Sentinel value collides numerically with the VP pending-item id (nit, latent)

ThoughtExpandedContext.tsx:26 sets PENDING_THOUGHT_HEAD_ID = -1, and the VP path stamps pending items id: -(i + 1) — so the first pending item's id is also -1. No bug today (MainContent renders pending items as {...item, id: 0} and thoughtGroupHeadId = thoughtHeadId ?? item.id), but two unrelated negative-id namespaces sharing -1 means a future refactor that drops the id: 0 override would appear to keep working for the first pending item and break for the rest. A distinctive constant (e.g. -1_000_001) removes the coincidence for free.

4. useGeminiStream is now at 26 positional parameters (nit)

The tests have to say Parameters<typeof useGeminiStream>[25] and renderTestHook threads five undefineds to reach the new arg. Not introduced here, and not this PR's job to fix, but this is the argument that should tip the hook to an options object — a mis-ordered positional here would typecheck in several slots.

5. New i18n key isn't in any locale (nit)

'(click or {{keyHint}} to collapse)' has no entry in packages/cli/src/i18n/locales/*.js. Consistent with the sibling hints ((click or … to expand), ({{keyHint}} to collapse) are also missing, and check-i18n doesn't flag source-keys-missing-from-en.js), so this is not a CI break — but that's now four always-visible strings that render English for every non-English user. Worth adding the set in one pass.

6. Minor

  • findLastThoughtHeadId reads visibleHistory, so a head hidden by /history collapse would leave a pending tail keyed to item.id (0). Effectively unreachable — a head committed in the current turn is always visible — but 0 is a shared key across pending items if it ever happens.
  • pendingLabel is computed unconditionally, including on the committed path where it's discarded. Trivial.
  • docs/design/ctrl-o-detail-expand/design.md:87,123 still describe the thought summary as (option+t to expand) and streaming as "实时显示". Already stale relative to main, but this PR is the natural moment to refresh the hint wording and add the click-while-pending affordance.

中文小结

整体设计站得住:sentinel + settlePendingExpansion 的 pending→committed 迁移是纯函数且单独有单测,head/tail 拆分、双拆分、VP/Static 双路径、ref 新鲜度都覆盖到了,press→release 跨越 commit 的竞态被明确记录为可接受而不是回避。本地跑过:126 + 186 个测试全绿,eslint 干净。

主要一条要改的是 #1forcedOpen 只挡了 fullDetailthoughtExpanded === true,漏了 context 里的 allExpanded。在 AgentChatContent / QuittingDisplay 这两个不传 fullDetail 的调用点,Ctrl+O 打开时思考块被强制展开、但点击处理仍然挂着,提示还会写 (click or ctrl+o to collapse)——点击会静默写进 expandedHeadIds,等 Ctrl+O 关掉时状态突然翻转。这正是 PR 自己的测试注释所防的那个坑,只是走了另一条路径。已实测确认(VP + allExpanded: true + 不传 fullDetailisActive === true 且帧里含 click)。建议改成 fullDetail || thoughtExpanded !== undefined || allExpanded

#2 是文案问题:被点击展开的思考块,ctrl+o 其实收不起来(开→仍展开,关→回落到 expandedHeadIds 仍展开),但提示照写。其余为 nit。

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 10/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 10/100 轮)。改动内容与我反驳保留之处如下:

Round-9 review feedback — all three findings addressed

Commit: fix(cli): address round-9 review on thought expansion (#8443)

R9-1 — settle wiring in AppContainer has no test (rc:3720331687) — Addressed

Added a Pending thought expansion settle wiring test to AppContainer.test.tsx.

Approach note: the suggested integration test was attempted first — real provider state, record an expansion under PENDING_THOUGHT_HEAD_ID via the real toggle, invoke the real settle callback with a committed head id, and assert the migrated set through the captured context. This harness does not propagate AppContainer-internal setState to the captured context consumer: the probe showed renderCount=2, ids=[] under every variation tried (act, async act, rerender, IS_REACT_ACT_ENVIRONMENT set before or after render) — the same limitation already documented in the Ctrl+O test in this file. Following that test's accepted precedent, the wiring is pinned in two layers instead:

  1. The real callback AppContainer passes as the last argument of useGeminiStream must be a function (the wiring seam exists).
  2. Its source must apply settlePendingExpansion(prev, committedHeadId) through the real setExpandedThoughtHeadIds setter (tolerating the bundler's (0, mod.fn) ESM-call interop form).

Probe-verified: both reviewer mutations — settlePendingExpansion(prev, null) and settlePendingExpansion(new Set(), committedHeadId) — make the new test fail. The pure migration behavior stays covered by ThoughtExpandedContext.test.tsx; the hook's settle-call timing by useGeminiStream.test.tsx.

R9-2 — startingNewThought guard around settle(null) pinned by no test (rc:3720331695) — Addressed

Added settles null only for the first Thought event of a continued thought to the Pending thought expansion settlement block in useGeminiStream.test.tsx.

The straightforward version (yield two consecutive Thought events) did not pin the guard — probe-verified: the guard-removal mutation survived it, because the throttled buffered flush merges adjacent thought chunks into a single handleThoughtEvent call. The final version uses fake timers to fire the 60ms flush while only part one is buffered (same pattern as the oversized-split test), so the continuation chunk reaches handleThoughtEvent with a non-empty thought buffer — the startingNewThought === false path. It asserts settle fires with null exactly once (first event), then with the committed head id at commit, toHaveBeenCalledTimes(2). Probe-verified: moving settlePendingThoughtExpansion?.(null) outside the guard fails the test ([null, null, committedId]).

R9-4 — forcedOpen misses allExpanded on surfaces without fullDetail forwarding (rc:3720331698) — Addressed

Applied the suggested one-line fix in HistoryItemDisplay.tsx:

const forcedOpen = fullDetail || allExpanded || thoughtExpanded === true;

with the comment updated accordingly. This disarms the click handler and switches the collapse hint to keyboard-only whenever Ctrl+O full-detail pins a thought open — including AgentChatContent (which forwards neither fullDetail nor thoughtExpanded) and any future surface. Added two regression tests mirroring the existing fullDetail pair: handler disarmed, and click wording dropped from the collapse hint when allExpanded is active without fullDetail forwarding. Probe-verified: both tests fail when the fix is reverted.

Conflict

None (--conflict false); origin/main was not merged.

Verification

  • npm run build — passed
  • npm run typecheck — passed (exit 0, no errors)
  • npm run lint — passed (exit 0)
  • Focused Vitest (packages/cli), all seven PR-related suites — 457 passed, 0 failed:
    • src/ui/AppContainer.test.tsx — 142 passed
    • src/ui/hooks/useGeminiStream.test.tsx — 187 passed
    • src/ui/components/HistoryItemDisplay.test.tsx — 46 passed
    • src/ui/components/MainContent.test.tsx — 33 passed
    • src/ui/components/messages/ConversationMessages.test.tsx — 22 passed
    • src/ui/utils/historyUtils.test.ts — 24 passed
    • src/ui/contexts/ThoughtExpandedContext.test.tsx — 3 passed
  • Re-run of the three directly touched suites on the committed state — 375 passed, 0 failed
  • Mutation probes (each reverted after confirming the test catches it):
    • settlePendingExpansion(prev, null) → R9-1 test fails ✓
    • settlePendingExpansion(new Set(), committedHeadId) → R9-1 test fails ✓
    • settle(null) moved outside the startingNewThought guard → R9-2 test fails ✓
    • forcedOpen without allExpanded → both R9-4 tests fail ✓
  • Settings source unchanged → npm run generate:settings-schema not required.
  • Integration tests not run: this round's changes are unit-covered UI behavior (click disarming) plus test additions, not behavior exercised only through the bundled CLI or integration harness.
中文说明

第 9 轮审查反馈 — 三项发现全部处理

提交:fix(cli): address round-9 review on thought expansion (#8443)

R9-1 — AppContainer 中的 settle 接线没有测试(rc:3720331687)— 已处理

AppContainer.test.tsx 中新增了 Pending thought expansion settle wiring 测试。

方法说明:首先尝试了建议的集成测试 —— 使用真实 provider 状态,通过真实 togglePENDING_THOUGHT_HEAD_ID 下记录一次展开,用已提交的 head id 调用真实 settle 回调,并通过捕获的 context 断言迁移后的集合。但此测试 harness 不会把 AppContainer 内部的 setState 传播到捕获的 context consumer:探针显示在所有尝试的变体下(act、async actrerender、在 render 前后设置 IS_REACT_ACT_ENVIRONMENT)均为 renderCount=2, ids=[] —— 与本文件中 Ctrl+O 测试已记录的局限相同。按照该测试已被接受的先例,接线改为两层钉住:

  1. AppContainer 作为 useGeminiStream 最后一个参数传入的真实回调必须是一个函数(接线缝隙存在)。
  2. 其源码必须通过真实的 setExpandedThoughtHeadIds setter 应用 settlePendingExpansion(prev, committedHeadId)(容忍 bundler 的 (0, mod.fn) ESM 调用 interop 形式)。

已探针验证:评审提出的两个变异 —— settlePendingExpansion(prev, null)settlePendingExpansion(new Set(), committedHeadId) —— 都会使新测试失败。纯函数的迁移行为仍由 ThoughtExpandedContext.test.tsx 覆盖;hook 的 settle 调用时机仍由 useGeminiStream.test.tsx 覆盖。

R9-2 — settle(null) 外层的 startingNewThought 守卫没有测试钉住(rc:3720331695)— 已处理

useGeminiStream.test.tsxPending thought expansion settlement 块中新增 settles null only for the first Thought event of a continued thought

直接版本(连续 yield 两个 Thought 事件)无法钉住守卫 —— 已探针验证:移除守卫的变异可以在该版本下存活,因为节流缓冲 flush 会把相邻的思考块分块合并成一次 handleThoughtEvent 调用。最终版本使用 fake timers,在只缓冲了第一段时触发 60ms flush(与超大分段测试相同的模式),使续传分块到达 handleThoughtEvent 时思考缓冲区非空 —— 即 startingNewThought === false 路径。断言 settle 仅以 null 触发一次(首个事件),随后在提交时以已提交 head id 触发,且 toHaveBeenCalledTimes(2)。已探针验证:把 settlePendingThoughtExpansion?.(null) 移出守卫会使测试失败([null, null, committedId])。

R9-4 — 未转发 fullDetail 的渲染面上 forcedOpen 遗漏 allExpanded(rc:3720331698)— 已处理

HistoryItemDisplay.tsx 中应用了建议的一行修复:

const forcedOpen = fullDetail || allExpanded || thoughtExpanded === true;

并同步更新了注释。只要 Ctrl+O 全详情把思考块钉在展开状态,就会解除点击处理器武装并把收起提示改为纯键盘措辞 —— 包括 AgentChatContent(它既不转发 fullDetail 也不转发 thoughtExpanded)以及未来任何渲染面。新增两个回归测试,镜像现有 fullDetail 测试对:allExpanded 生效且无 fullDetail 转发时,处理器被解除武装、收起提示去掉 click 措辞。已探针验证:回退修复后两个测试均失败。

冲突

无(--conflict false);未合并 origin/main

验证

  • npm run build — 通过
  • npm run typecheck — 通过(exit 0,无错误)
  • npm run lint — 通过(exit 0)
  • 聚焦 Vitest(packages/cli),PR 相关全部 7 个套件 — 457 通过,0 失败
    • src/ui/AppContainer.test.tsx — 142 通过
    • src/ui/hooks/useGeminiStream.test.tsx — 187 通过
    • src/ui/components/HistoryItemDisplay.test.tsx — 46 通过
    • src/ui/components/MainContent.test.tsx — 33 通过
    • src/ui/components/messages/ConversationMessages.test.tsx — 22 通过
    • src/ui/utils/historyUtils.test.ts — 24 通过
    • src/ui/contexts/ThoughtExpandedContext.test.tsx — 3 通过
  • 在已提交状态上重跑直接改动的 3 个套件 — 375 通过,0 失败
  • 变异探针(每次确认测试能捕获后均已回退):
    • settlePendingExpansion(prev, null) → R9-1 测试失败 ✓
    • settlePendingExpansion(new Set(), committedHeadId) → R9-1 测试失败 ✓
    • settle(null) 移出 startingNewThought 守卫 → R9-2 测试失败 ✓
    • forcedOpen 不含 allExpanded → R9-4 两个测试均失败 ✓
  • 设置源未改动 → 无需运行 npm run generate:settings-schema
  • 未运行集成测试:本轮改动是单测覆盖的 UI 行为(点击解除武装)加测试新增,并非只能通过打包 CLI 或集成测试框架验证的行为。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

📊 Takeover milestone — round 10/100, in the current window. Census: 9 pushed fix(es), 0 no-change review(s), 1 timeout(s), 0 rejected attempt(s), 0 other round(s) (crash / model error / gate error / infra), 1 base update(s).

This many rounds deserves a human look. Options: keep going (fine — nothing changes), split or reduce the PR if rounds keep accumulating, or release takeover (remove the autofix/takeover label or comment @qwen-code /takeover stop). Management continues unchanged unless you act.

中文说明

📊 接管里程碑 —— 第 10/100 轮(当前窗口)。统计:推送修复 9 次、审阅无需改动 0 次、超时 1 次、验证拒绝 0 次、其他轮次(崩溃/模型错误/门错误/infra)0 次、base 更新 1 次。

轮次到这个量值得人工看一眼。可选:继续(无需操作);若轮次持续累积,考虑拆分或缩减 PR;或释放接管(移除 autofix/takeover 标签或评论 @qwen-code /takeover stop)。不操作则托管照常继续。

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run.

中文说明

🔀 已更新 base:红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 57 tests pass — this review observed 17680 passed.

中文说明

已审查。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan(非阻断):57 tests pass — this review observed 17680 passed

— qwen3.8-max via Qwen Code /review (v0.21.5)

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 11/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 11/100 轮)。改动内容与我反驳保留之处如下:

Round summary — PR #8443 (addressing @wenshao's review of head 84c298e)

Critical-only mode is active; the deferred-feedback section for this round was empty, so the maintainer review was fully actionable. One commit: 422363649. No base-conflict resolution was needed (--conflict false, no merge performed).

Findings addressed

  1. forcedOpen misses routes — the guard is half-applied (medium) — Fixed. The allExpanded half was already addressed in the previous round (commit 10ad76665, after the reviewed head 84c298e); this round completes the invariant. forcedOpen now uses thoughtExpanded !== undefined instead of === true, so thoughtExpanded={false} (pinned closed) disarms the click handler exactly like true (pinned open). With this, the invariant "forcedOpen ⇔ a click cannot change resolvedThoughtExpanded" holds for every combination of fullDetail / allExpanded / thoughtExpanded — a click can only fire when resolvedThoughtExpanded resolves to the per-head click set. Two new tests cover the pinned-closed case (handler disarmed; expand hint drops the click wording).
  2. (click or ctrl+o to collapse) overstates what ctrl+o does (medium-low) — Fixed, cheaper than suggested. Once finding 1's invariant holds, a thought that is both clickable and expanded can only have been expanded via the per-head click set (every pinned-open route disarms clickable), and ctrl+o cannot collapse a click-expanded thought — so no keyCanCollapse bit needs threading into ThinkMessage. The clickable collapse hint is simply (click to collapse) now; the non-clickable hint keeps the key-only wording. Tests updated to assert the ctrl+o hint is absent in the clickable case.
  3. Sentinel collides numerically with the VP pending-item id (nit, latent) — Fixed. PENDING_THOUGHT_HEAD_ID changed from -1 to -1_000_001 so it can no longer coincide with the VP path's -(i + 1) pending-item namespace; the doc comment records the second namespace. All call sites and tests use the named constant, so the change is value-only.

Findings declined (with reasons)

  1. 26 positional parameters on useGeminiStream (nit) — Declined: the reviewer explicitly notes this is "not this PR's job to fix"; converting the hook to an options object is a refactor out of scope for this fix PR.
  2. New i18n keys aren't in any locale (nit) — Declined: not a CI break (npm run check-i18n passes — it does not flag source keys missing from locale files), and all four sibling hint keys are uniformly absent from all nine locales (pre-existing pattern). Machine-generating translations for nine locales risks inaccurate strings; this is best done as a dedicated i18n pass covering the final key set (this round's hint change also retires (click or {{keyHint}} to collapse) in favor of (click to collapse)).
  3. Minor items — 6a (findLastThoughtHeadId reading visibleHistory): declined — effectively unreachable per the finding's own analysis, and a fallback would be defensive code for a condition that cannot occur. 6b (pendingLabel computed unconditionally): declined — one template string plus one t() call per render is negligible, and hoisting it would duplicate the expression across both render branches. 6c (stale design doc): fixed — the thought row in the baseline table now describes the pending Thinking… summary line, click-to-expand while streaming, and expansion settlement on commit; the two screenshot captions' stale option+t hints were renamed to ctrl+o.

Verification

Commands actually run this round (all from a clean npm ci + build environment):

  • npm run build — passed
  • npm run typecheck — passed (tsc --noEmit across all packages)
  • npm run lint — passed (eslint . --ext .ts,.tsx && eslint integration-tests, zero findings)
  • npx vitest run in packages/cli (focused, touched files) — 459 passed: HistoryItemDisplay.test.tsx 48 (incl. 2 new), ConversationMessages.test.tsx 22 (2 updated assertions), ThoughtExpandedContext.test.tsx 3, MainContent.test.tsx 33, historyUtils.test.ts 24, useGeminiStream.test.tsx 187, AppContainer.test.tsx 142
  • npm run check-i18n — passed (extra precaution because an i18n key changed)
  • Integration tests after npm run bundle — not run: the touched behavior (TUI thought hints / click gating) is exercised by the unit/component tests above, not only through the bundled CLI or integration harness
  • npm run generate:settings-schema — not applicable (no settings source changed)

Note: an initial focused vitest run showed AppContainer.test.tsx and useGeminiStream.test.tsx failing to load with a vite packageEntryFailure (unresolvable @qwen-code/qwen-code-core entry) — a missing-build artifact issue, not a test failure; npm run build resolved it and both files then passed cleanly.

中文说明

轮次总结 — PR #8443(处理 @wenshao 针对 head 84c298e 的评审)

当前处于仅处理 Critical 的模式;本轮的延后反馈区为空,因此该维护者评审整体可处理。一个提交:422363649。无需解决与 base 的冲突(--conflict false,未执行合并)。

已处理的发现

  1. forcedOpen 漏掉路径 — 守卫只挡了一半(medium) — 已修复。allExpanded 那一半已在上一轮修复(提交 10ad76665,晚于被评审的 head 84c298e);本轮补全整个不变式。forcedOpen 现在使用 thoughtExpanded !== undefined 而非 === true,因此 thoughtExpanded={false}(强制收起)与 true(强制展开)一样会解除点击处理。至此,不变式「forcedOpen ⇔ 点击无法改变 resolvedThoughtExpanded」在 fullDetail / allExpanded / thoughtExpanded 的所有组合下都成立 — 只有当 resolvedThoughtExpanded 落到按 head 记录的点击集合时,点击才会生效。新增两个测试覆盖强制收起的场景(处理器被解除;展开提示去掉 click 文案)。
  2. (click or ctrl+o to collapse) 夸大了 ctrl+o 的作用(medium-low) — 已修复,且比建议的方案更省。发现 1 的不变式成立后,一个既 clickable 又处于展开状态的思考块,只可能是通过按 head 的点击集合展开的(所有强制展开路径都会解除 clickable),而 ctrl+o 收不起来点击展开的思考块 — 因此无需向 ThinkMessage 多穿一个 keyCanCollapse 位。clickable 时的收起提示现在就是 (click to collapse);非 clickable 时保持仅键盘的文案。测试更新为断言 clickable 场景下不出现 ctrl+o 提示。
  3. sentinel 与 VP pending 项 id 数值相撞(nit,潜在) — 已修复。PENDING_THOUGHT_HEAD_ID-1 改为 -1_000_001,不再可能与 VP 路径的 -(i + 1) pending 项命名空间重合;文档注释记录了第二个命名空间。所有调用点和测试都使用具名常量,因此改动只涉及取值。

已拒绝的发现(附理由)

  1. useGeminiStream 已有 26 个位置参数(nit) — 拒绝:评审者明确指出「not this PR's job to fix」;把 hook 改成 options 对象属于超出本修复 PR 范围的重构。
  2. 新 i18n key 不在任何 locale 中(nit) — 拒绝:不影响 CI(npm run check-i18n 通过 — 它不检查源码 key 缺失于 locale 文件的情况),且四个兄弟提示 key 在全部九个 locale 中一致缺失(既有模式)。为九个 locale 机器生成翻译有不准确的风险;最好作为专门的 i18n 批次统一补齐最终的 key 集合(本轮的提示改动也用 (click to collapse) 替换了 (click or {{keyHint}} to collapse))。
  3. 次要项 — 6a(findLastThoughtHeadIdvisibleHistory):拒绝 — 按该发现自身的分析实际上不可达,加回退属于为不可能发生的状态写防御代码。6b(pendingLabel 无条件计算):拒绝 — 每次渲染一个模板字符串加一次 t() 调用,开销可忽略;上移会造成两个渲染分支重复该表达式。6c(设计文档过期):已修复 — 基线表格中的思考行现在描述 pending 时的 Thinking… 摘要行、streaming 中点击展开实时观看、落定后展开状态迁移保留;两张截图说明中过期的 option+t 提示已更名为 ctrl+o

验证

本轮实际运行的命令(环境为干净的 npm ci + build):

  • npm run build — 通过
  • npm run typecheck — 通过(所有包 tsc --noEmit
  • npm run lint — 通过(eslint . --ext .ts,.tsx && eslint integration-tests,零发现)
  • packages/clinpx vitest run(聚焦受改动文件)— 459 通过:HistoryItemDisplay.test.tsx 48(含 2 个新增)、ConversationMessages.test.tsx 22(2 处断言更新)、ThoughtExpandedContext.test.tsx 3、MainContent.test.tsx 33、historyUtils.test.ts 24、useGeminiStream.test.tsx 187、AppContainer.test.tsx 142
  • npm run check-i18n — 通过(因改动了 i18n key 而额外确认)
  • npm run bundle 后的集成测试 — 未运行:受改动的行为(TUI 思考提示 / 点击门控)由上述单元/组件测试覆盖,并非只通过打包 CLI 或集成测试框架才能验证
  • npm run generate:settings-schema — 不适用(未改动 settings 源)

说明:首次聚焦 vitest 运行时 AppContainer.test.tsxuseGeminiStream.test.tsx 因 vite packageEntryFailure(无法解析 @qwen-code/qwen-code-core 入口)加载失败 — 这是缺少构建产物所致,并非测试失败;执行 npm run build 后两个文件均干净通过。

Deferred non-Critical feedback

Critical-only mode is active after 10 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (@qwen-code /retry starts a fresh counting window.)

中文说明

完成 10 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈;这些条目保持开放,留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后;达到预算的作者(如有)在下方点名。(评论 @qwen-code /retry 可开启新的计数窗口。)

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 AutoFix ran out of time before finishing (timeout (1080000ms)) (attempt 12/100) — it will retry on the next scan.

What I found before stopping:
Qwen failed during address-review: timeout (1080000ms).

See the Qwen Autofix agent step logs for model/tool output.

Run log: https://github.com/QwenLM/qwen-code/actions/runs/31029280757


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment @qwen-code /retry to re-arm. Until then future scans will skip this PR.

What I found before stopping:
Qwen failed during address-review: timeout (1080000ms).

See the Qwen Autofix agent step logs for model/tool output.

Run log: https://github.com/QwenLM/qwen-code/actions/runs/31037527888


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Test Plan (not a blocker): 57 tests pass — this review observed 17685 passed.

中文说明

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

Test Plan(非阻断):57 tests pass — this review observed 17685 passed

— qwen3.8-max via Qwen Code /review (v0.21.6)

Comment on lines +1681 to +1683
if (item.type === 'gemini_thought') {
settlePendingThoughtExpansion?.(committedId);
}

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] R11-1: The settle wiring assumes a single stream owns the hook's pending-thought slot, but the legacy concurrent ?btw path (allowConcurrentBtwDuringResponse — a supported, test-pinned path) runs a second stream through the same processGeminiStreamEvents/handleThoughtEvent/commitPendingThought handlers with the shared pendingThoughtItemRef and this settle callback, so a side-question stream can drop or mis-migrate the provisional expansion this PR introduces. (The sibling settle(null) call site at ~line 1720 is where the drop fires; both call sites are affected.)

Failure scenario: user in VP mouse mode click-expands a streaming thought (sentinel recorded). While it streams they submit ?btw .... Probe A — the btw answer starts with a Thought event: the btw stream's startingNewThought fires settlePendingThoughtExpansion?.(null) mid-flight, deleting the user's sentinel; when the main thought later commits, settle(committedId) finds nothing and the thought the user expanded commits collapsed — the exact regression this PR exists to prevent. Probe B — the btw answer starts with Content: the shared-slot commitPendingThought commits the main stream's partial head early and migrates the sentinel onto it; the remainder commits as a second head, stranding the expansion on the partial head. Probe-verified via settle-spy plus a walk through the real settlePendingExpansion reducer; a flip run with these two settle call sites reverted recorded zero settle calls, so the interference is introduced by this wiring.

Suggested fix: make settlement stream-owned — thread a per-stream ownership flag into processGeminiStreamEvents/handleThoughtEvent/addItemAndSettleThoughtHead and skip settlePendingThoughtExpansion on any stream that is not the primary turn.

中文说明

settle 接线假定只有一条流持有该 hook 的 pending-thought 槽位,但遗留的 ?btw 并发路径(allowConcurrentBtwDuringResponse —— 受支持且有测试固定的路径)会让第二条流复用相同的 processGeminiStreamEvents/handleThoughtEvent/commitPendingThought 处理器,共享 pendingThoughtItemRef 与该 settle 回调,因此旁路提问流可能丢弃或错误迁移本 PR 引入的临时展开状态。(另一个 settle(null) 调用点位于约 1720 行,是丢弃实际发生的位置;两个调用点都受影响。)

失败场景:用户在 VP 鼠标模式下点击展开正在流式输出的思考块(记录 sentinel)。流式进行中提交 ?btw ...。探针 A —— btw 回答以 Thought 事件开始:btw 流的 startingNewThought 在中途触发 settlePendingThoughtExpansion?.(null),删除用户的 sentinel;主思考随后提交时 settle(committedId) 找不到记录,用户展开的思考块在提交瞬间变为折叠 —— 正是本 PR 要防止的回归。探针 B —— btw 回答以 Content 开始:共享槽位的 commitPendingThought 提前提交主流的部分头部并把 sentinel 迁移到其上;剩余部分作为第二个头部提交,展开状态被搁浅在部分头部上。已通过 settle spy 加真实 settlePendingExpansion reducer 推演验证;将这两处 settle 调用点回退的对照运行未记录到任何 settle 调用,证明该干扰由本接线引入。

建议修复:让 settlement 按流隔离 —— 将每流所有权标志传入 processGeminiStreamEvents/handleThoughtEvent/addItemAndSettleThoughtHead,非主回合流跳过 settlePendingThoughtExpansion

— qwen3.8-max via Qwen Code /review (v0.21.6)

Comment on lines +5515 to +5520
expect(settlePendingThoughtExpansion).toBeTypeOf('function');
expect(
(
settlePendingThoughtExpansion as (...args: unknown[]) => unknown
).toString(),
).toMatch(

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] R11-2: This wiring test asserts on Function.prototype.toString() source text — the regex pins the exact identifier names (prev, committedHeadId, settlePendingExpansion) and the ESM-interop shape, and the callback is located via the last positional slot (mockedUseGeminiStream.mock.calls.at(-1)?.at(-1)) — so it pins the mechanism's source text, not its behavior.

Concrete cost: a behavior-neutral edit — renaming committedHeadId in AppContainer's settle callback, extracting the updater into a named helper, or appending a 27th positional parameter to useGeminiStream — fails this test with an opaque regex/position mismatch although production behavior is identical. The regex already carries a (?:\(0,\s*)?(?:[A-Za-z0-9_$]+\.)? carve-out for bundler ESM-interop forms, i.e. a transform difference nearly broke it once.

Suggested fix: pin behaviorally — e.g. export a factory from ThoughtExpandedContext (makeSettlePendingExpansion(setter)), unit-test it with a spy setter, and assert AppContainer passes an instance of it; or at minimum drop the identifier-name pins and assert only the load-bearing call structure.

中文说明

该接线测试对 Function.prototype.toString() 源码文本做断言 —— 正则固定了确切的标识符名(prevcommittedHeadIdsettlePendingExpansion)与 ESM interop 形态,且通过最后一个位置参数槽位(mockedUseGeminiStream.mock.calls.at(-1)?.at(-1))定位回调 —— 因此固定的是机制的源码文本,而非行为。

具体代价:行为无关的改动 —— 在 AppContainer 的 settle 回调中重命名 committedHeadId、把 updater 提取为具名辅助函数、或给 useGeminiStream 追加第 27 个位置参数 —— 都会让该测试以晦涩的正则/位置不匹配失败,而生产行为完全相同。正则中已有针对打包器 ESM interop 形态的 (?:\(0,\s*)?(?:[A-Za-z0-9_$]+\.)? 豁免项,说明转换差异曾险些使其失败。

建议修复:改为对行为断言 —— 例如从 ThoughtExpandedContext 导出工厂函数(makeSettlePendingExpansion(setter)),用 spy setter 对其单测,并断言 AppContainer 传入的是该工厂的实例;或至少去掉对标识符名的固定,只断言关键的调用结构。

— qwen3.8-max via Qwen Code /review (v0.21.6)

// closed), so a click could never change it — ClickableThinkMessage
// disarms itself. `allExpanded` matters on surfaces that don't forward it
// as `fullDetail` (e.g. AgentChatContent).
const forcedOpen = fullDetail || allExpanded || thoughtExpanded !== undefined;

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] R11-3: forcedOpen is a misleading name — it is also true when the thought is pinned closed (thoughtExpanded={false}), so it means "expansion state is pinned", not "forced open". The comment above correctly says the gate pins the state "(open or closed)", and this PR's own new test exercises forcedOpen === true with the thought rendered collapsed.

Concrete cost: today the value only gates clickable, so behavior is correct — but a future change that reuses forcedOpen where the name's meaning applies (e.g. expanded={forcedOpen || ...}, choosing the expanded header icon, or skipping collapse logic) would silently treat pinned-closed thoughts as open.

Suggested fix: rename to a state-neutral identifier such as expansionPinned in HistoryItemDisplayComponent and ClickableThinkMessage (prop declaration, destructure, and the clickable read site):

const expansionPinned = fullDetail || allExpanded || thoughtExpanded !== undefined;
中文说明

forcedOpen 命名有误导性 —— 当思考块被固定为收起thoughtExpanded={false})时它同样为 true,其含义是「展开状态被固定」,而非「强制展开」。上方注释正确地写明该门控将状态固定为「(展开或收起)」,且本 PR 新增的测试也在思考块渲染为收起时验证了 forcedOpen === true

具体代价:目前该值只门控 clickable,行为正确 —— 但未来若在名称含义适用的地方复用 forcedOpen(例如 expanded={forcedOpen || ...}、选择展开态头部图标、或跳过收起逻辑),会把被固定收起的思考块静默当作展开处理。

建议修复:在 HistoryItemDisplayComponentClickableThinkMessage 中(prop 声明、解构及 clickable 读取处)重命名为状态中立的标识符,例如 expansionPinned

const expansionPinned = fullDetail || allExpanded || thoughtExpanded !== undefined;

— qwen3.8-max via Qwen Code /review (v0.21.6)

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

⏸️ Takeover paused: this PR reached its round cap (100/100). Comment @qwen-code /takeover to re-arm a fresh window and continue management, or @qwen-code /takeover stop to release.

中文说明

⏸️ 托管已暂停:本 PR 达到轮次上限(100/100)。评论 @qwen-code /takeover 可重新武装、开启新窗口继续托管;或评论 @qwen-code /takeover stop 释放。

@QwenLM QwenLM deleted a comment from danialzivehdadr Aug 5, 2026
@QwenLM QwenLM deleted a comment from danialzivehdadr Aug 5, 2026
@QwenLM QwenLM deleted a comment from danialzivehdadr Aug 6, 2026
@qwen-code-dev-bot qwen-code-dev-bot added the autofix/needs-human The autofix loop stopped on this PR — a human must re-arm, split, merge, or close it label Aug 15, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🔓 Takeover auto-released: the autofix loop paused on this PR 9 day(s) ago (🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this ) and no re-arm followed, so the autofix/takeover label is removed to keep the managed pool honest. autofix/needs-human stays as the reminder that this PR needs a human decision: merge it, close it, or split/reduce it and comment @qwen-code /takeover to re-engage with a fresh round window.

中文说明

🔓 已自动释放接管:autofix 循环在 9 天前暂停于此 PR(🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this ),此后无人重新武装,现移除 autofix/takeover 标签以保持托管池真实可用。保留 autofix/needs-human 作为待办提醒 —— 本 PR 需要人工决策:合并、关闭,或拆分/缩小后评论 @qwen-code /takeover 以全新轮次窗口重新接管。

@qwen-code-dev-bot qwen-code-dev-bot removed the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Aug 15, 2026
@chiga0
chiga0 marked this pull request as draft August 19, 2026 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix/needs-human The autofix loop stopped on this PR — a human must re-arm, split, merge, or close it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants