feat(web-shell): improve thinking and tool progress display - #8872
feat(web-shell): improve thinking and tool progress display#8872ytahdn wants to merge 14 commits into
Conversation
|
Re-run at head Thanks for the PR! Template looks good ✓ Problem: unchanged — concrete, observable motivation rather than theory: compact mode used to swap tool groups for condensed cards (dropping the aggregate interaction), hidden thinking could fragment related tool activity, and elapsed labels restarted after a refresh because only client render time was recorded. Direction: aligned. Compact mode already exists; this replaces its rendering path instead of adding a parallel one, and the elapsed-label instability is a genuine defect. Thinking/tool-progress presentation remains an active area in agent UIs — claude-code's changelog carries recurring entries in this exact space. Size: 25 files — ~412 production lines (web-shell client + sdk-typescript daemon-ui timing), ~1022 test lines, 17 lines of design doc. The earlier pass's CLI settings schema / serve-route / vscode companion surface is gone from the net diff: the PR now preserves the existing Approach: the scope hangs together — thinking hidden + ordinary tool groups merged across it, all-active-foreground-tool summaries, tool-kind icons and spacing, and daemon-clock elapsed times all serve one goal ("quieter compact transcript, standard aggregate UX"), and the old condensed Risk: no elevated risk signals — no high-risk path matches in the changed file set. Gate passes; this round's verdict turns on the merge-tree build evidence in Stage 2/3. ⬇️ 中文说明在 head 感谢贡献！ 模板完整 ✓ 问题：不变——动机具体、可观察而非理论性：旧紧凑模式会把工具组换成压缩卡片（丢失聚合交互），隐藏思考可能拆散相关工具活动，刷新后耗时标签重新计时（此前只记录客户端渲染时间）。 方向：对齐。紧凑模式已经存在，本 PR 替换其渲染路径而非新增平行路径；耗时不稳定是真实缺陷。思考/工具进度展示仍是 agent UI 的活跃领域——claude-code 的 changelog 在这一领域有持续条目。 规模：25 个文件——约 412 生产行（web-shell client + sdk-typescript daemon-ui 计时），约 1022 测试行，17 行设计文档。上一轮涉及的 CLI settings schema / serve route / vscode companion 已从净 diff 中消失：PR 现在保留既有 方案：范围内聚——隐藏思考 + 跨隐藏思考合并普通工具组、展示全部活跃前台工具的摘要、工具类型图标与间距、daemon 时钟耗时，都服务于同一目标（"更安静的紧凑 transcript、标准聚合 UX"），且旧的压缩 风险：无升级风险信号——改动文件中无高风险路径命中。 Gate 通过；本轮结论取决于 Stage 2/3 中的合并树构建证据。⬇️ — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 11 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code review (re-review at
|
| File | What changed |
|---|---|
docs/design/web-shell-thinking-and-tool-progress.md |
New design doc: goals, mechanics, and the no-new-surface compatibility commitment |
packages/sdk-typescript/src/daemon/ui/transcript.ts |
Tracks serverUpdatedAt on text/tool blocks; terminator events stamp the end boundary without stealing the block's eventId |
packages/sdk-typescript/src/daemon/ui/types.ts |
Adds the optional serverUpdatedAt field to the block base type |
packages/sdk-typescript/test/unit/daemonUi.test.ts |
Pins replay pairs, thought-finalized-by-tool stamping, cancellation stamping, and the no-fabricated-pair negative |
packages/web-shell/client/App.test.tsx |
Ctrl+O persists the existing workspace setting; nested-session render-loop guard (#8934) |
packages/web-shell/client/adapters/messageTypes.ts |
Thinking messages gain startTime/endTime |
packages/web-shell/client/adapters/transcriptToMessages.ts |
getTranscriptTiming server/client pair logic; background-agent and permission-merge timing fixes |
packages/web-shell/client/adapters/transcriptToMessages.test.ts |
+279 lines pinning every timing branch, both permission orderings, cross-clock non-merge |
packages/web-shell/client/components/MessageItem.tsx |
Forwards thinking timing; tool-group spacing flag; memo comparator includes timing |
packages/web-shell/client/components/MessageItem.dom.test.tsx |
Spacing and timing-rerender coverage |
packages/web-shell/client/components/MessageList.tsx |
Compact merge across hidden thinking with standalone-group exclusions |
packages/web-shell/client/components/MessageList.dom.test.tsx |
+170 lines: hide/merge/boundary/agent-path DOM coverage |
packages/web-shell/client/components/MessageTimestamp.tsx |
Always-rendered row wrapper; optional larger tool-group spacing |
packages/web-shell/client/components/MessageTimestamp.module.css |
.toolGroupSpacing vertical rhythm |
packages/web-shell/client/components/MessageTimestamp.test.tsx |
Spacing behavior pinned |
packages/web-shell/client/components/dialogs/HelpDialog.tsx |
Documents the Ctrl+O shortcut |
packages/web-shell/client/components/dialogs/HelpDialog.test.tsx |
Asserts the shortcut appears in en and zh-CN |
packages/web-shell/client/components/messages/AssistantMessage.tsx |
ThinkingMessage consumes authoritative start/end; stable "Thought for Ns" |
packages/web-shell/client/components/messages/AssistantMessage.test.tsx |
Live→complete, replayed, and durationless states pinned |
packages/web-shell/client/components/messages/ToolGroup.tsx |
Multi-tool active summaries; removes the condensed compact card; icon per expanded row; duration fallback |
packages/web-shell/client/components/messages/ToolGroup.test.tsx |
Summary, timer continuation, fallback, and icon coverage |
packages/web-shell/client/components/messages/UserShellMessage.tsx |
Drops the compact hint variant |
packages/web-shell/client/components/messages/UserShellMessage.module.css |
Removes compact-only styles |
packages/web-shell/client/components/messages/tools/SubAgentPanel.test.tsx |
App mock updated for the remaining context imports |
packages/web-shell/client/components/messages/tools/ToolChrome.module.css |
Expanded-header active styling; icon no-shrink; compact styles removed |
Test evidence (the PR's own CI via API + the in-flight sandboxed verify — no PR code executed here)
CI on the reviewed commit is settled and fully green (zero failures, zero pending pull_request runs). One note that matters: ci.yml never sets QWEN_SKIP_PREPARE, so every npm ci lane runs the full build — including the SDK's assertBrowserSafeBundle budget check that failed the previous pass. The branch now carries the 186 KiB budget (it merged main after #8798), and the bare head builds within it.
| Check | Conclusion |
|---|---|
Test (ubuntu-latest, Node 22.x) |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Capture web-shell visuals (ubuntu-latest, Node 22.x) |
✅ success |
Post Coverage Comment (ubuntu-latest, 22.x) |
✅ success |
Classify PR / route |
✅ success |
Test (windows/macos), integration lanes |
⏭️ skipped (by design for this change class) |
Two signals with honest limits. Serve A/B (run at the earlier head 4c628e8) found no response deltas across 4 scenarios — consistent with a presentation-only change, but it predates the last two commits. The web-shell visual preview at this head reports no screenshot deltas while itself noting its mock scenarios never reach the compact/thinking-toggle UI — an empty result there is a coverage gap, not a clean bill of health. The PR body's test counts (320/320 etc.) remain the author's claim; the CI table above is the authoritative signal, and the author verified on macOS only.
The merge tree. The previous pass blocked because the stale merge ref carried the pre-#8798 185 KiB budget. That root cause is gone — the branch is synced past #8798. But main has moved 11 commits beyond the branch's last sync, and several of them touch packages/sdk-typescript/src/daemon/* (#8911 NDJSON buffer bound, #8933 restore shapes, v0.21.10) — so the exact merged combination has not been built anywhere yet. The sandboxed verification triggered by this re-run (run 31554742406) is in flight right now, building that merge tree; its report will post to the verify marker comment. Sandboxed verification would settle this: @qwen-code /verify — already running as part of this re-run — for the claim that the merged tree builds within the daemon browser bundle budget and that Ctrl+O merging and stable elapsed labels hold in a live session, neither of which the visuals scenario or the author's macOS-only pass can currently show.
中文说明
代码审查（3795fb38 复审）
head 自上次完整审查以来已变动（分支从 main 同步 + 4 个 PR 自有提交），因此重读了全部净 diff。maintainer 的两个拦截项保持已修复：P1（permission 合并前置空占位 startTime、mergeToolCall 仅有值时传播，两个方向均有回归测试固定服务端时间对）；P2（终结事件转发且剥离 eventId、已有 serverTimestamp 才盖 serverUpdatedAt、绝不伪造时间对，含负向测试）。时钟纪律保持：正区间才用服务端时间对，否则整体回退客户端时间对；进行中任务把 daemon 耗时投影到客户端时钟，无跨钟相减；不同时钟的相邻思考不合并。合并边界只合并普通工具组，审批待决/子代理/TodoWrite/AskUserQuestion 保持独立。head 提交是真实修复：运行中工具缺 startTime 时回退到挂载时钟（否则 NaN）——正是 doudouOUC 未决建议的落地，两个方向都有测试。旧压缩卡片路径完全删除（仅 ToolGroup.tsx 净 −91 行）。无 critical 拦截项；遗留非拦截项供 maintainer 知悉：M1（跨消费者时钟域契约，作者已独立确认并留待专项跟进）、R3-4（多工具摘要未透传 workspaceCwd）、R3-5（Ctrl+O 测试未钉关闭方向）、R4-2（测试助手硬编码非紧凑）、maintainer 遗留 P3（rejected permission 跨钟边界，仅边缘场景）。
测试证据
审查提交上的 CI 全绿且无在途 pull_request 运行。值得注意：ci.yml 从不设置 QWEN_SKIP_PREPARE，每个 npm ci 通道都执行完整构建——包括上一轮失败的那个 SDK 浏览器 bundle 预算断言；分支已带上 186 KiB 预算（#8798 之后合并了 main），裸 head 在预算内构建通过。Serve A/B（较早 head 4c628e8）无响应差异，但早于最后两个提交；视觉预览无截图差异、但其 mock 场景触达不到紧凑/思考开关 UI——空结果是覆盖缺口而非健康证明。PR 正文的测试数为作者自述，以上表为准，作者仅在 macOS 验证。
合并树：上一轮的根因（过期 merge ref 携带 185 KiB 旧预算）已消除，但 main 在分支最后同步之后又前进了 11 个提交，其中数个触及 packages/sdk-typescript/src/daemon/*——精确的合并组合尚未被任何构建验证过。本次重跑触发的沙箱验证（run 31554742406）正在构建该合并树，报告会发布在 verify 标记评论中；它将同时验证"合并树在 bundle 预算内构建"与"真实会话中 Ctrl+O 合并及耗时稳定"这两个视觉预览和作者 macOS 验证都无法覆盖的声明。
— Qwen Code · qwen3.8-max
Reviewed at 3795fb38e4ecff43bd267a53c1b9df11e7e4e8bd · re-run with @qwen-code /triage
|
Confidence: 3/5 — the code is ready and I would otherwise approve; the only thing I am waiting on is the merge-tree build verdict that is literally in flight. Stepping back: this is the cleanest this thread has looked. My independent proposal for the goal (filter thinking in the adapter, merge only groups that become adjacent, keep special boundaries, persist a server start/end pair and project live durations onto the client clock) matches what the PR does — I found no simpler path it missed. The maintainer's two blockers were fixed with mutation-resistant tests and I re-verified both in the current diff rather than trusting my prior notes. doudouOUC's one actionable finding is addressed by the head commit itself, with tests on both sides of the fallback. chiga0's review got honest answers — M1 (cross-consumer clock domains under skew) was independently confirmed by the author and deliberately deferred to a focused follow-up rather than waved away, which is the right call for a display-only edge that needs a cross-cutting contract. The remaining open threads are Suggestions that the autofix rounds pushed back on; at this point in the cycle that is a maintainer's call to make, not a gate's. So why not approve? The previous pass blocked on the merge tree failing to build, and the direct re-test of exactly that question — the sandboxed verify run this re-run triggered — has not landed yet. Since the branch last synced, main moved 11 commits, several touching ⏸️ Deferring to @yiliang114 — approval withheld pending the sandboxed verification report (run 31554742406, posting to the verify marker comment). This is a conditional hold, not a rethink: if that report lands green on the merge-tree build, the PR is approve-ready as-is — approve directly or re-trigger 中文说明置信度：3/5 —— 代码已就绪，若非唯一一件事我会直接批准：合并树构建的裁决正在路上。 整体看：这是本 thread 迄今最干净的状态。我对该目标的独立方案（适配器中过滤思考、只合并因此相邻的工具组、保留特殊边界、持久化服务端起止时间对并把进行中耗时投影到客户端时钟）与 PR 的做法一致，没有找到更简的被遗漏路径。maintainer 的两个拦截项已用抗变异测试修复，我在当前 diff 中重新核实而非沿用旧记录。doudouOUC 唯一可执行的发现由 head 提交本身解决且两侧都有测试。chiga0 的审查得到了诚实回应——M1（时钟偏斜下的跨消费者时钟域）被作者独立确认并有意留给专项跟进，这对于需要跨切面契约的纯展示边缘问题是正确处理。其余未决线程是 autofix 已回怼的 Suggestion 级别——此刻该由 maintainer 拍板，不构成 gate。 那为何不批？上一轮拦截于合并树构建失败，而对该问题的直接复测——本次重跑触发的沙箱 verify——尚未落地。分支最后同步之后 main 又前进了 11 个提交，其中数个触及 ⏸️ 转交 @yiliang114 —— 等待沙箱验证报告（run 31554742406，将发布在 verify 标记评论）后再批准。这是条件性搁置而非重新审议：若报告对合并树构建为绿，PR 即达可批状态——可直接批准或重新触发 — Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to @yiliang114 (re: @wenshao, deferred in the first pass) — re-run at head This PR escalates to maintainer awareness under the Stage 0 gate: it spans web-shell, sdk-typescript, the CLI settings surface, and the vscode companion schema with ~688 production lines, over the 500-line threshold for Status of the human call: @yiliang114 already re-reviewed this exact head and approved it. If that approval covers the escalated scope, no further gate action is needed — merge once the （按 Stage 0 gate 转交维护者：跨 web-shell、sdk-typescript、CLI 设置面与 vscode 伴随 schema、约 688 生产行，超过 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: did not trace external consumers of the sdk's projectDaemonTranscriptRecords (offline now: 0 projection) beyond the sdk itself — assessed and dropped anyway….
中文说明
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：did not trace external consumers of the sdk's projectDaemonTranscriptRecords (offline now: 0 projection) beyond the sdk itself — assessed and dropped anyway…。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 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. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 1/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31449545753 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review-response summary (round 2)Commit:
Fixed (resolved in code)
Deferred (replies recorded in
|
| Finding | Reason |
|---|---|
R1-5 orphaned ui.compactMode plumbing (settings schema, allowlist, hidden keys, zh labels) |
Removing the setting key is a retire-vs-keep product decision touching the settings schema (cross-package, CI-enforced regeneration); out of scope for this budget-limited round. |
R1-6 docs still say ui.compactMode is honored by the web shell |
Depends on the R1-5 retire-vs-keep decision; both should land together. |
R1-7 showThinking not settable on the exported WebShellTranscript surface |
The PR's design doc explicitly states no public prop is added; widening the public API is a maintainer call. Not a regression (thinking was never hideable there). |
| R1-8, R1-9, R1-11, R1-12, R1-13 | New test-coverage suggestions (thought-merge timing, SDK cancellation stamp, MessageList todo/question exclusions, durationless thinking pin, multi-delta thought stamp). Valid in principle; deferred under the round's time budget for a follow-up. |
Issue-level comments
- web-shell visual preview ("no screenshot changed"): informational; adding a visuals scenario for the hidden-thinking UI is a nice-to-have deferred with the test-coverage items above.
- Stage 0 gate escalation to @wenshao: acknowledged — the cross-package size gate leaves the final call to a maintainer; no bot action available.
- Failed check
review-address: that was the previous autofix round timing out (the budget warning above); this round completes that work.
Verification
Commands actually run in this round:
npm run build— passed (SDK/web-shell dist artifacts produced; the earlier test-collection errors were the missing SDK dist, cleared after building)npm run typecheck— passednpm run lint— passed (full repo; targetednpx eslinton the 5 changed files also clean)npx prettier --checkon the 5 changed files — passedvitestweb-shell full package suite (packages/web-shell) — 178 files / 3098 tests passedvitestfocused:transcriptToMessages.test.ts+ToolGroup.test.tsx+SubAgentPanel.test.tsx— 191 passed (re-run post-commit)vitestsdk-typescripttest/unit/daemonUi.test.ts(touched by the PR, untouched this round) — 293 passed- Regression pinning: temporarily reverted the
mergeToolCallfix → its new test failed; temporarily reverted the R1-4 guard → its new test failed; both fixes restored and the suite is green again. - Integration tests after
npm run bundle: not applicable — the changed behavior is fully exercised by the web-shell unit/DOM suites above.npm run generate:settings-schema: not applicable — no settings source changed this round.
中文说明
Autofix 评审响应总结（第 2 轮）
提交：fix(web-shell): address review feedback on tool timing and summaries (#8872)（位于 feat/optimize-web-shell-thinking-tools 分支）。
无需解决与基线的冲突（--conflict false；未合并 origin/main）。
comment-replies.json 中记录了回复，没有任何一项被静默丢弃。
已修复（在代码中解决）
| 发现 | 处理 | 变更 |
|---|---|---|
| R1-1 [Critical] 带 daemon 时间戳的工具合并进已批准权限占位条目时混用时钟计算耗时 | 已修复 | mergeToolCall 现在与 endTime 一并采纳 source.startTime，合并后的卡片保持同一时钟。新增回归测试 keeps merged permission placeholders on the tool server clock，临时回退该修复后测试失败（已验证）。 |
R1-3 运行中摘要的 count 被改为活跃工具数，而 i18n 文案仍按组内总数渲染（中文"共 N 个工具"） |
已修复 | 恢复 count: tools.length（组内总数），与未改动的中英文文案及改动前行为一致；同步更新了四处相关摘要测试断言。 |
R1-4 applyBackgroundAgentTaskUpdate 的局部 hasServerPair 缺少正耗时条件 |
已修复 | 补充 update.serverEndTime > block.serverTimestamp，与 hasServerTimingPair 及设计文档"仅当包含正耗时区间"一致。新增测试 falls back to client timing when a notification repeats the start stamp，移除该守卫后测试失败（已验证）。 |
R1-10 applyBackgroundAgentTaskUpdate 的服务器时钟分支无测试 |
以新测试解决 | uses the daemon clock pair for replayed background agent notifications 断言 startTime 与 endTime 均来自服务器值；上面的同戳用例锁定客户端回退。 |
R1-14 SubAgentPanel.test.tsx 的 App mock 缺少 TodoDetailContext |
已修复 | mock 现在同时导出两个 todo context，与 ToolGroup/PlanMessage 的兄弟 mock 保持一致。 |
R1-15 全局 Date.now spy 只在断言之后才恢复 |
已修复 | 用 try/finally 包裹断言，确保 spy 总是被恢复。 |
顺延（回复已记录在 comment-replies.json，线程保持打开）
| 发现 | 原因 |
|---|---|
R1-5 孤儿 ui.compactMode 管道（settings schema、白名单、隐藏键、中文标签） |
删除该设置键属于"退役还是保留"的产品决策，且涉及 settings schema（跨包、CI 强制重新生成）；超出本轮预算范围。 |
R1-6 文档仍声称 web shell 遵循 ui.compactMode |
依赖 R1-5 的退役/保留决策，两者应一起落地。 |
R1-7 导出的 WebShellTranscript 界面无法设置 showThinking |
本 PR 的设计文档明确"不新增公开 prop"；扩大公开 API 属于维护者决策。这也不是回归（该界面此前同样无法隐藏思考）。 |
| R1-8、R1-9、R1-11、R1-12、R1-13 | 新增测试覆盖类建议（思考合并计时、SDK 取消时间戳、MessageList 的 todo/提问排除、无时长思考锁定、多 delta 思考时间戳）。原则上成立；受本轮时间预算限制顺延至后续处理。 |
Issue 级评论
- web-shell 可视化预览（"没有截图变化"）：信息性评论；为隐藏思考界面新增 visuals 场景属于锦上添花，随上述测试覆盖项一并顺延。
- Stage 0 gate 转交 @wenshao：已知悉——跨包规模 gate 将最终决定权交给维护者；机器人无可执行操作。
- 失败检查
review-address：即上一轮 autofix 超时（见上方预算警告）；本轮已完成其遗留工作。
验证
本轮实际执行的命令：
npm run build— 通过（生成了 SDK/web-shell 的 dist 产物；此前的测试收集错误正是缺少 SDK dist，构建后消除）npm run typecheck— 通过npm run lint— 通过（全仓库；对 5 个改动文件的定向npx eslint亦无问题）- 对 5 个改动文件执行
npx prettier --check— 通过 vitestweb-shell 整包测试（packages/web-shell）— 178 个文件 / 3098 个测试全部通过vitest定向：transcriptToMessages.test.ts+ToolGroup.test.tsx+SubAgentPanel.test.tsx— 191 通过（提交后复跑）vitestsdk-typescripttest/unit/daemonUi.test.ts（本 PR 改动过、本轮未动）— 293 通过- 回归锁定验证：临时回退
mergeToolCall修复 → 对应新测试失败；临时回退 R1-4 守卫 → 对应新测试失败；恢复两处修复后测试套件重新全绿。 npm run bundle后的集成测试：不适用——上述改动的行为已完全由 web-shell 单元/DOM 测试覆盖。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-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): Context: PR #8872 (Web Shell thinking-visibility toggle +...: none — all checks above completed within budget.; Context: PR #8872 (Web Shell thinking-visibility toggle +...: none — all checks above completed within budget.; Context: PR #8872 (Web Shell thinking-visibility toggle +...: none — all checks above completed within budget.; Context: PR #8872 reuses Ctrl+O to toggle thinking visibi...: none — all planned checks complete. Should I mention that I did not run the test suite? My remit is review, not execution. No issue.; Context: PR #8872 reuses Ctrl+O to toggle thinking visibi...: none — no check was left unfinished at the budget ceiling; the one residual uncertainty (live ordering frequency of preparing-frame-before-permission) is stated…, and 2 more.
中文说明
未探索到全部深度（达到工具调用预算）：Context: PR #8872 (Web Shell thinking-visibility toggle +...：none — all checks above completed within budget.；Context: PR #8872 (Web Shell thinking-visibility toggle +...：none — all checks above completed within budget.；Context: PR #8872 (Web Shell thinking-visibility toggle +...：none — all checks above completed within budget.；Context: PR #8872 reuses Ctrl+O to toggle thinking visibi...：none — all planned checks complete. Should I mention that I did not run the test suite? My remit is review, not execution. No issue.；Context: PR #8872 reuses Ctrl+O to toggle thinking visibi...：none — no check was left unfinished at the budget ceiling; the one residual uncertainty (live ordering frequency of preparing-frame-before-permission) is stated…，另有 2 条。
— qwen3.8-max via Qwen Code /review (v0.21.9)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
The presentation half is solid — aggregation boundaries are correct (only consecutive ordinary tool groups separated solely by thinking merge; approval/todo/question/agent/message boundaries and subagent groups break the run; pending-approval groups force-expand), localStorage handling is safe (namespaced key, strict 'true'/'false' parse, synchronous read so no first-paint flash), and it is presentation-only. But the timing rework — the PR's core goal — has two confirmed gaps at this head, matching the open automated CHANGES_REQUESTED:
-
P1 (direction-blind startTime merge, approval-gated tools): the R1-1 fix added target.startTime = source.startTime ?? target.startTime, but permissionBlockToToolCall always sets startTime from block.createdAt (client clock). In the NORMAL tool-before-permission order the synthetic placeholder merges INTO the daemon-stamped tool and overwrites the server startTime; the terminal-state restore saves previousStatus/previousEndTime but no previousStartTime, leaving {clientStart, serverEnd}. When the client clock leads, elapsed goes negative and formatElapsed (seconds < 3 -> '') silently drops the duration label — on exactly the completed tools this PR exists to stabilize. The new regression test only covers the safe placeholder-first direction. Fix: make the permission branch direction-aware (clear the placeholder startTime when the existing tool already has one, or save/restore previousStartTime) and add a tool-first regression test.
-
P2 (thought->tool duration degrades on replay): clearActiveText takes no event, so upsertToolBlock's create path drops the stamped tool.update event; serverUpdatedAt stays == serverTimestamp (degenerate pair), fails hasServerTimingPair, falls back to the client pair which collapses to ~zero elapsed after refresh/resync. Live looks fine; replay shows no duration — the exact instability this PR removes. The text-delta path already forwards the event, so forward it from the create path too and pin with a daemonUi test.
Plus mutation-verified test gaps (merged tool list unpinned, inert showThinking flag in ToolGroup.test.tsx, hide-thinking dom test missing collapseCompletedTurns: false so turn collapse masks the feature, zero-assertion localStorage-unavailable test) and the orphaned ui.compactMode surface whose doc promise goes wrong the moment this merges (deferred is fine if tracked). CI green on head; nothing else blocks.
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Round 2 review feedback — all 8 findings addressedCommit: Critical
Suggestions
No finding was declined or escalated. VerificationCommands actually run (after the final restored state, before committing):
中文说明第 2 轮评审反馈 — 8 条全部处理提交： Critical
Suggestions
没有拒绝或升级任何发现。 验证实际执行的命令（在最终还原状态、提交之前）：
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/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-review at head 71a64b3 — both blockers are fixed with mutation-resistant tests. P1: the permission branch now nulls the placeholder's startTime before mergeToolCall, so the daemon-stamped startTime survives the tool-before-permission order; the new test asserts {startTime: 5000, endTime: 15000} and would fail on the old direction-blind code, and the placeholder-first direction stays pinned by the round-2 test. P2: clearActiveText now takes the event and all seven call sites forward it, including the upsertToolBlock create path; finalizeStreamingTextBlock stamps serverUpdatedAt from it, and the new daemonUi test pins {serverTimestamp: 1000, serverUpdatedAt: 6000} on a thought finalized by a tool update — fails pre-fix. All five test gaps are pinned (merged tool list via data-tool-ids, real showThinking consumption wired through WebShellTranscript, collapseCompletedTurns: false in the hide test, exact-string leak assertion, localStorage-unavailable assertions), and ui.compactMode is fully retired across schema/allowlist/HIDDEN_SETTING_KEYS/zh i18n/vscode schema/settings.md with negative tests (not.toHaveProperty, POST -> 400 disallowed_key).
One residual P3, display-only, follow-up not gate: a rejected permission resolution sets endTime from permBlock.updatedAt (client clock), so a tool-first + rejected order now yields a cross-clock pair whose negative elapsed silently drops the duration label on the failed card — edge-only. CI has no failures on head with the main test job still running; nothing blocks merge.
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — the PR could not be built - workflow run The PR could not be built because 中文 — 判定：❌ 不通过 · PR 构建失败由于 Install/build log— Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — the PR could not be built - workflow run The PR could not be built because 中文 — 判定：❌ 不通过 · PR 构建失败由于 Install/build log— Qwen Code · sandboxed verification |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no changes neededEvery point in the new feedback is already resolved at the current head ( Review findings (CHANGES_REQUESTED by @yiliang114, 06:17Z)The review was re-listed because it is newer than the last-evaluation cutoff, but it targeted the pre-round-3 head. All items were fixed by the two commits pushed after it —
Residual P3 (deferred, per reviewer)The approval review notes one remaining display-only edge: a rejected permission resolution sets Bot comments
DispositionNo code changes this round; no commits pushed. VerificationCommands actually run this round (all on head
中文说明Autofix 本轮：无需修改新反馈中的每一点都已在当前 head（ 评审意见（@yiliang114 的 CHANGES_REQUESTED，06:17Z）该评审被重新列出是因为它晚于上次评估截止时间，但它针对的是第 3 轮修复之前的 head。所有条目都已由其后推送的两个提交修复——
遗留 P3（按评审者意见推迟）批准评审中指出还剩一个纯展示面的边界情况：被拒绝的 permission 解决会用 机器人评论
处理结论本轮不做任何代码修改；不推送提交。 验证本轮实际执行的命令（均在 head
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
⛔ The sandboxed verification could not build this PR's merge tree, so it can't merge as-is — details and the traced root cause are in my Stage 2 comment above.
What failed: npm ci failed twice deterministically in the /verify sandbox — Browser daemon SDK bundle is 189487 bytes; expected <= 189440 (assertBrowserSafeBundle, packages/sdk-typescript/scripts/build.js). The failing tree is the current merge ref 5ebe3677, which predates #8798's bump of the daemon browser bundle budget from 185 KiB to 186 KiB by 17 minutes. Your PR's own CI can't see this because it checks out the bare PR head by design.
The fix (mechanical): update the branch from main. That rebuilds the merge ref against the current 186 KiB budget, under which the measured bundle fits with ~977 bytes of headroom. After updating, re-verify the build — #8798's own daemon-barrel additions land in the same budget — and if the combined bundle still exceeds 186 KiB, bump MAX_DAEMON_BROWSER_BUNDLE_BYTES with a comment line following the established pattern in that file.
The code review itself is clean (both prior timing blockers remain fixed, verified in the diff), and this is the only open blocker. 🙏
沙箱验证无法构建本 PR 的合并树，现状不可合入——细节与根因追溯见上方 Stage 2 评论。失败点：/verify 沙箱中 npm ci 两次确定性失败，daemon 浏览器 bundle 189487 字节超出当时上限 189440；原因是分支比 #8798（185→186 KiB 预算上调）早 17 分钟。修复是机械性的：从 main 更新分支，让 merge ref 按 186 KiB 预算重建；更新后重新验证构建，若仍超预算则按该文件惯例加注释提高预算。代码审查本身干净，这是唯一未决拦截项。
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Independent deep review — PR #8872 feat(web-shell): improve thinking and tool progress display
Verdict: COMMENT (no Critical/Major at head 71a64b3; 3 Minor + 5 Nit)
Reviewed blind first (metadata + full diff at head, AGENTS.md, full source of transcript.ts / transcriptToMessages.ts, plus every downstream consumer of DaemonTranscriptBlock timing), then cross-validated against the 3 prior automated rounds and @yiliang114's two reviews.
The engineering here is genuinely good. The core idea — keep a pair of daemon stamps per block, use it only when it spans a positive interval, and project the daemon-measured elapsed onto the client clock for live counters (block.updatedAt - elapsed) instead of subtracting across clocks — is the right shape, and the prior rounds' two blockers (direction-blind startTime merge; clearActiveText not forwarding the terminator event) are correctly fixed and pinned with mutation-resistant tests. serverUpdatedAt's blast radius is contained: I enumerated every read site across packages/sdk-typescript, packages/web-shell, and packages/webui, and there is no clone/equality/serialization helper that enumerates block fields (cloneTranscriptState spreads; webui only reads block.createdAt).
My findings are all in the layer below what the prior rounds covered: not whether a single card mixes clocks, but what happens when the projected values are compared across tools and messages by consumers outside this diff.
Findings
| # | Severity | Where | Summary |
|---|---|---|---|
| M1 | Minor | transcriptToMessages.ts:1052 |
startTime/endTime now carry two different clock domains across tools in the same view; two consumers outside this diff compare them across tools |
| M2 | Minor | transcript.ts:654 |
serverTimestamp flipped from last-write-wins to first-write-wins, silently changing two existing consumers (hover timestamp, goal setAt) |
| M3 | Minor | transcriptToMessages.ts:565 |
Thinking-bubble grouping is now driven by timing internals; a replayed thought run can split into two bubbles |
| N1 | Nit | AssistantMessage.tsx:241 |
New useEffect writing startTimeRef is unreachable-by-construction dead code; setFinishedAt(endTime) is a redundant render |
| N2 | Nit | transcriptToMessages.ts:125 |
startTime/endTime assigned unconditionally before the status switch |
| N3 | Nit | MessageList.tsx:293 |
mergeToolGroupsAcrossThinking also does the thinking hiding; the name advertises only the merge |
| N4 | Nit | App.tsx:5911 |
Ctrl+O is now a silent toggle — the removed compact mode had compact.enabled/compact.disabled toasts |
| N6 | Nit | transcript.ts:503 |
The serverTimestamp === undefined branch stamps the terminator's time into a field documented as "when the block was first observed" |
Cross-validation vs. existing reviews
Prior rounds: qwen-code-ci-bot R1 (1 Critical + 13 Suggestions), R2 (1 Critical + 7 Suggestions), R3 (merge-tree build blocker); @yiliang114 CHANGES_REQUESTED → APPROVED at this head.
| Category | Items |
|---|---|
| Overlap (independently reached, already fixed at head) | R1-1/R2-1 mixed-clock mergeToolCall — I traced the same permission path and confirm permissionToolCall.startTime = undefined + previousEndTime restore closes it in both orders. R1-4 positive-elapsed guard in applyBackgroundAgentTaskUpdate — present. R2-4 clearActiveText event forwarding — all 7 call sites forward; I verified the mechanism end-to-end (the next record's terminator supplies the later stamp, which is what makes the headline "replay-stable elapsed" claim actually true). R1-5/R1-6 ui.compactMode retirement — fully removed across schema / WEB_SHELL_SETTINGS / HIDDEN_SETTING_KEYS / zh i18n / vscode schema / settings.md, and safe for existing user configs (unknown keys only debug-log). R1-7 showThinking on WebShellTranscript — mirrored, memo deps complete. |
| Unique — mine | M1 (nobody traced utils/todos.ts:collectToolSpans or SubAgentPanel.tsx:110, the two places that compare timing across tools). M2 (R1-13 flagged the ??= as a test-coverage gap; nobody traced its effect on blockTime at transcriptToMessages.ts:377 and getLatestActiveGoalFromBlocks at App.tsx:554). M3, N1, N2, N3, N4, N6. |
| Unique — theirs | The entire test-coverage sweep (R1-8 … R1-15, R2-2 … R2-8) — thorough and correct; all now addressed. R3's merge-tree npm ci bundle-budget failure — I could not have found this from the diff alone; it is the real merge blocker. |
| Disagree / refine | @yiliang114's residual P3 (rejected-permission endTime = permBlock.updatedAt producing a cross-clock pair): I traced it and believe it is effectively unreachable, not merely edge-only. A cross-clock pair needs a server-clock startTime, which daemonToolBlockToToolCall only produces when the block is complete (getTranscriptTiming(block, true)); but a complete block means previousStatus is terminal, which triggers the existingPermission.endTime = previousEndTime restore. A non-terminal block yields a client-anchored updatedAt - elapsed start, so the pair stays same-clock. Worth a one-line comment rather than a follow-up issue — happy to be shown a path I missed. |
Structural audits (all 8 run)
- Handler parallelism — the 7
clearActiveTextcall sites all forward the event;propagateCancellationToInFlightTools's 3 call sites all forward. Complete. - Data-structure blast radius —
serverUpdatedAtread sites enumerated across all 3 packages; no field-enumerating helper. Contained. - Data provenance — traced daemon →
formatSseFrame(which always stamps_meta.serverTimestamp, so the "unstamped event advancesupdatedAtbut notserverUpdatedAt" non-monotonic-timer hazard is not reachable over SSE) →extractServerTimestamp→ reducer →getTranscriptTiming→ UI. Clean. - State field init matrix —
createTextBlockandupsertToolBlock's create path both seedserverUpdatedAt = serverTimestamp, so a fresh block never spuriously passes the strict>gate. Correct. - Replay/reconnect divergence — the offline projection path (
chat-record-transcript.ts→finalizeOfflineDaemonTranscriptState,now: 0) is SDK-only and never reaches web-shell, so theupdatedAt = 0fallback cannot produce a negative duration in the UI. Verified, no issue. - Project convention compliance — matches
AGENTS.md. Per its two-tier gate this touchespackages/cli/src/config/**and is cross-package, so it warrants maintainer awareness; as afeatit is not hard-blocked. Also noteAGENTS.md's "don't let review rounds balloon the PR" — this is round 4, so I would land only M1/M2 if anything, and defer the Nits. - Caller/consumer impact — see M1/M2; this is where my unique findings are.
- Sibling code consistency —
packages/webuihas an independentextractServerTimestampcall but renders no durations, so it needs no counterpart change. Correct to leave alone.
Merge blocker (not mine)
mergeable: CONFLICTING, and R3's sandbox npm ci failure stands. Update the branch from main per that comment before merging.
None of my findings should block. M1 is the one I would most like a maintainer to weigh in on, since it is a cross-cutting consequence of the new dual-clock model rather than a defect in this diff.
This review was generated by QoderWork AI
| @@ -997,6 +1049,7 @@ function daemonToolBlockToToolCall( | |||
| block.status === 'failed' || | |||
| block.status === 'cancelled' || | |||
| block.status === 'canceled'; | |||
| const timing = getTranscriptTiming(block, isComplete && !isBackgroundAgent); | |||
There was a problem hiding this comment.
[Minor] M1 — startTime/endTime now carry two clock domains, and two consumers compare them across tools.
getTranscriptTiming is per-block, and correctly avoids mixed-clock subtraction within one card (the design doc's stated goal). But it makes the domain per-tool: a completed tool with a valid server pair emits daemon-clock absolute values, while a tool without a pair — and every in-flight tool, via the block.updatedAt - elapsed projection — emits client-clock values. Before this PR every tool was uniformly block.createdAt/block.updatedAt, i.e. one domain. Two consumers outside this diff read these values as if they were commensurable:
packages/web-shell/client/utils/todos.ts:531—collectToolSpanspushes{start: tool.startTime, end: tool.endTime}for every tool into one array,sorts it, andsumToolTimeInWindowbinary-searches it against todo windows built frommessage.timestamp(which isblock.serverTimestamp ?? block.clientReceivedAt, itself mixed). With a remote daemon and non-trivial skew, spans in the "wrong" domain land outside their task window and per-task tool-time attribution is silently wrong. Note the sort itself also assumes one domain.packages/web-shell/client/components/messages/tools/SubAgentPanel.tsx:110—<SubToolTime timestamp={tool.startTime}>rendersformatTimestamp(startTime)as an absolute time-of-day. In a single panel, completed sub-tools now show the daemon's wall clock and in-flight ones the client's, so under skew the displayed times can read out of order.
Both are display-only and both need real clock skew to bite, hence Minor — but skew is exactly the scenario this PR was written for, so it seems worth an explicit decision rather than an accident of which tools happened to get a pair.
Options, roughly in increasing cost: (a) document at the ACPToolCall.startTime/endTime declaration that the domain is per-tool and that cross-tool comparison is unsound; (b) normalize at projection time — you already have both pairs on the block, so a single estimated serverClockOffset (median of clientReceivedAt - serverTimestamp over recent blocks) would let you emit everything in the client domain while keeping the daemon-measured durations; (c) carry the domain alongside the value so consumers like collectToolSpans can skip or convert.
This review was generated by QoderWork AI
There was a problem hiding this comment.
Confirmed with an independent probe: a 500 ms server-domain tool span is dropped from a server-domain todo window when the tool falls back to client timestamps, and nested subtool timestamps can appear out of order. Not fixed in this PR because the safe fix requires a cross-consumer clock-domain contract covering ACPToolCall, todo attribution, and SubAgentPanel. Choosing an offset heuristic or adding domain metadata here would be a speculative cross-package expansion late in the review cycle. Keeping this thread open for a focused follow-up.
| @@ -650,7 +651,8 @@ function appendTextDelta( | |||
| existing.updatedAt = state.now; | |||
| if (event.eventId !== undefined) existing.eventId = event.eventId; | |||
| if (event.serverTimestamp !== undefined) { | |||
| existing.serverTimestamp = event.serverTimestamp; | |||
| existing.serverTimestamp ??= event.serverTimestamp; | |||
There was a problem hiding this comment.
[Minor] M2 — the ??= silently changes serverTimestamp semantics for two existing consumers.
This flips merged text deltas from last-write-wins to first-write-wins. It is almost certainly the right change — the field's own JSDoc says "captured when the block was first observed", so the old = contradicted the documented contract, and the strict > in hasServerTimingPair depends on this. R1-13 already asked for a test pinning it.
What I did not see raised is that serverTimestamp has two pre-existing consumers whose output changes as a result, neither of which is about durations:
packages/web-shell/client/adapters/transcriptToMessages.ts:377—const blockTime = block.serverTimestamp ?? block.clientReceivedAtbecomes every message's hover timestamp. For a multi-delta assistant/thought block the tooltip previously drifted to the latest delta and now pins to the first. Better, but user-visible and unannounced.packages/web-shell/client/App.tsx:554—getLatestActiveGoalFromBlocksusesstatus.setAt ?? block.serverTimestamp ?? block.createdAt. Same shift.
Suggestion: call the semantics change out in the PR description / design doc (it is a public SDK field), and add one assertion on blockTime for a multi-delta block so the hover-timestamp behavior is pinned too — the currently-requested test only covers the reducer side.
This review was generated by QoderWork AI
There was a problem hiding this comment.
No code change. First-write-wins is intentional: serverTimestamp is documented as first observed, serverUpdatedAt carries the latest stamp, and the multi-delta reducer test pins the 1000 to 6000 pair. The hover timestamp now using the first observed stamp is the desired consequence. An additional adapter assertion would add coverage rather than correct behavior, so it is deferred under the late-review scope.
| target && | ||
| target.role === 'thinking' && | ||
| !needsNewContentMessage && | ||
| currentThinkingUsesServerPair === usesServerPair |
There was a problem hiding this comment.
[Minor] M3 — presentation grouping is now a function of timing internals.
I see the test does not merge adjacent thinking blocks from different clocks, so the split is deliberate and I understand the motivation: a merged bubble keeps ...target's startTime and takes the last block's endTime, so mixing pair-backed and fallback blocks would straddle clocks.
The consequence is that which bubbles the user sees now depends on whether each block happened to get a positive server pair. That is not rare on replay: a thought block only gets serverUpdatedAt from a later terminator event, so the final thought block of a turn (nothing after it, or the turn ended) keeps serverUpdatedAt === serverTimestamp, fails the strict >, and becomes usesServerPair === false — splitting off from the pair-backed thoughts before it. Two visually identical consecutive thoughts render as one bubble or two depending on transcript position.
Worth considering the alternative: keep the bubble merged and demote the whole merged range to the client pair when the segments disagree. That preserves the pre-PR grouping (a stable, purely structural property), costs authoritative timing only on the mixed runs, and keeps the "no mixed-clock subtraction" invariant. Right now a timing optimization is paying for itself with a layout change.
This review was generated by QoderWork AI
There was a problem hiding this comment.
Confirmed behavior: adjacent thoughts with server-pair versus fallback timing split, and the current regression test explicitly pins that result. Not changing it in this PR because both splitting and demoting the whole run to client timing preserve the no-mixed-clock invariant; choosing stable grouping versus authoritative per-segment timing is a product/design decision rather than a clear correctness fix. Keeping this open for that decision.
| const [translationOpen, setTranslationOpen] = useState(false); | ||
| const [translation, setTranslation] = useState<ThinkingTranslation>(); | ||
| const [translationLoading, setTranslationLoading] = useState(false); | ||
| const [translationThinking, setTranslationThinking] = useState(false); | ||
| const [translationError, setTranslationError] = useState(false); | ||
| const translationAbortRef = useRef<AbortController | undefined>(undefined); | ||
|
|
||
| useEffect(() => { | ||
| if (startTime !== undefined) startTimeRef.current = startTime; |
There was a problem hiding this comment.
[Nit] N1 — this effect can never affect a read, and the sibling setFinishedAt(endTime) is a wasted render.
startTimeRef.current is read in exactly one place, (startTime ?? startTimeRef.current) in thinkingDurationMs. So when startTime !== undefined the ref is not read, and when it is undefined this effect does not write. The assignment is unreachable-by-construction.
Same shape a few lines down: if (endTime !== undefined) { setFinishedAt(endTime); return; } writes state that effectiveFinishedAt = endTime ?? finishedAt never consults, costing an extra render per completed thought. It also leaves a stale server-clock value in finishedAt if endTime ever goes back to undefined (a thought that resumes streaming), which would then be subtracted from a client-clock start.
Dropping both — and initializing startTimeRef/finishedAt purely as the no-timing fallback — would make the "authoritative timing wins, refs are legacy fallback only" intent obvious.
This review was generated by QoderWork AI
There was a problem hiding this comment.
Not fixed. On the current transcript path the adapter supplies startTime and authoritative endTime does not regress, so this is redundant-render and fallback-state cleanup rather than an observed user-facing defect. Removing the fallback state now would be an unrelated late-stage refactor; it should be handled separately if profiling or a resumed-thought contract makes it necessary.
| block.serverTimestamp !== undefined && | ||
| update.serverEndTime !== undefined && | ||
| update.serverEndTime > block.serverTimestamp; | ||
| tool.startTime = hasServerPair ? block.serverTimestamp : block.createdAt; |
There was a problem hiding this comment.
[Nit] N2 — startTime/endTime are now assigned before the status switch, so they apply to statuses the switch ignores.
Previously endTime was set inside each terminal case. Now both are unconditional, and the switch has no default, so an update whose status is not completed/failed/cancelled/canceled leaves the tool in-flight while carrying an endTime (frozen duration) and a daemon-clock startTime.
Not reachable today — emitNotification in packages/core/src/agents/background-tasks.ts only fires on terminal transitions — but it is a live coupling: ToolGroup computes formatLiveElapsed(summaryNow - activeTool.startTime) with summaryNow = Date.now(), so a daemon-clock startTime on a still-active tool is precisely the mixed-clock subtraction the rest of this diff is designed to prevent. Moving the two assignments into the three terminal cases (or adding a terminal-status guard) keeps the invariant local instead of depending on a fact about a file in another package.
This review was generated by QoderWork AI
There was a problem hiding this comment.
Confirmed as a defensive gap, but the current production producer emits only completed, failed, or cancelled background notifications; an independent reachability check found no running notification path. Not fixed here to avoid adding handling for a currently unsupported future input. If the producer contract widens, the adapter should add a terminal-status guard and a producer-level test together.
| isForceExpandGroup(msg, pendingApproval) || | ||
| isStandaloneToolGroup(msg) | ||
| ) { | ||
| if (!isThinkingMessage(msg)) { |
There was a problem hiding this comment.
[Nit] N3 — the function name advertises the merge, but it is also the only thing that hides thinking.
mergeToolGroupsAcrossThinking drops thinking rows here and at the inner-loop continue. That works only because the caller invokes it exclusively when hideThinking is true. If merging is ever made conditional (a setting, a perf guard, an embedder opt-out), thinking silently becomes visible again — a behavior the name gives no hint about, and which no test would catch since the merge tests and the hide test are separate.
Suggest splitting: messages.filter((m) => !isThinkingMessage(m)) at the call site, and let this function only merge. It also makes the two behaviors independently testable, which is what R1-11 is asking for on the merge side.
This review was generated by QoderWork AI
There was a problem hiding this comment.
Not fixed. The helper is called only when thinking is hidden and intentionally filters plus merges in one pass. There is no conditional merge caller today, and the current tests independently cover hiding and merging. Splitting the pass for a hypothetical future opt-out would add code and traversal without changing current behavior.
| }, | ||
| ); | ||
| }, [reportError, setWorkspaceSetting, t]); | ||
| const handleToggleThinking = useCallback(() => { |
There was a problem hiding this comment.
[Nit] N4 — Ctrl+O became a silent toggle.
The compact mode this replaces surfaced compact.enabled / compact.disabled toasts (both i18n keys deleted in this PR). handleToggleThinking only sets state and writes localStorage.
On a transcript with no thinking blocks — a short turn, a tool-only turn, or any session with a model that emits no thoughts — pressing Ctrl+O produces zero visible change, so the key reads as broken. The Help dialog entry helps discovery but not confirmation. Reusing the existing notice mechanism with a "Thinking shown / hidden" message would preserve the affordance the old toggle had, and would also confirm that the preference was persisted (the writeShowThinking catch is silent, so a private-browsing user gets no signal that the choice will not survive reload).
This review was generated by QoderWork AI
There was a problem hiding this comment.
Not fixed. A toast is a UX enhancement rather than a correctness issue: the in-memory preference still changes when localStorage throws, and a toast could not truthfully confirm persistence in that case. Adding new localized copy and shortcut-toast behavior needs a product decision about noise and messaging, so it is kept out of this late review round.
| block.serverTimestamp = event.serverTimestamp; | ||
| if (event?.serverTimestamp !== undefined) { | ||
| if (block.serverTimestamp === undefined) { | ||
| block.serverTimestamp = event.serverTimestamp; |
There was a problem hiding this comment.
[Nit] N6 — this branch stamps the terminator's time into a field documented as the block's start.
Pre-existing rather than introduced here, but the PR now pins it with does not create a server timing pair from a stamped thought end only (serverTimestamp: 6_000 on a block with createdAt: 100_000), so it is worth naming.
When a block was created without a stamp and is finalized by a much later event, serverTimestamp — documented as "captured when the block was first observed", and consumed as the hover wall-clock at transcriptToMessages.ts:377 and as setAt at App.tsx:554 — becomes the block's end time. The comment two lines above says the intent is that "later finalize/status events … would skew message times", which is exactly what this branch does in the one case it applies to.
hasServerTimingPair protects the duration path (no serverUpdatedAt, so it falls back to the client pair — correct). The absolute-timestamp consumers are unprotected. Cheapest fix is to write the terminator time to serverUpdatedAt here as well and leave serverTimestamp unset, letting the pair gate reject it; alternatively just extend the comment to say the field is deliberately end-anchored in this case.
This review was generated by QoderWork AI
There was a problem hiding this comment.
Partially confirmed: in the mixed unstamped-start and stamped-terminator edge, hover time uses the terminator as the block start. The claimed goal setAt impact is not reachable because that helper reads status blocks while finalizeStreamingTextBlock handles assistant and thought blocks. Not fixed here because deciding whether to discard the sole daemon stamp from absolute display changes an existing timestamp fallback contract outside this PR. Keeping the real hover issue open for a focused follow-up.
|
🔀 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 将重新运行。 |
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 4/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31486007772 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #8872OutcomeThe single open blocker — the sandboxed-verification build failure on the merge tree — is resolved by updating the branch from Feedback points and dispositions
Changes
Conflict notesNone — the merge applied cleanly with no conflicts requiring resolution. Verification
中文说明处理评审总结 — PR #8872结果唯一未决的拦截项——合并树沙箱验证构建失败——已按评审者给出的方式通过从 反馈点与处理决定
变更内容
冲突说明无——合并干净完成，没有需要解决的冲突。 验证
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/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
| const runningDuration = | ||
| animateSummary && activeTool?.startTime !== undefined | ||
| ? formatLiveElapsed(summaryNow - activeTool.startTime) | ||
| : undefined; |
There was a problem hiding this comment.
[Suggestion] runningDuration no longer has a fallback when activeTool.startTime is undefined — Failure scenario: A tool call in a running tool group has startTime: undefined (e.g., older session data or a transcript that doesn't stamp startTime). The animateSummary boolean is true, but activeTool?.startTime !== undefined guard fails, so runningDuration is undefined. The elapsed time that was previously shown (via a Date.now()-based ref) is now absent. The startTime field on ACPToolCall (DaemonMessageToolCall) is declared as optional (startTime?: number), so the undefined case is reachable.
| const runningDuration = | |
| animateSummary && activeTool?.startTime !== undefined | |
| ? formatLiveElapsed(summaryNow - activeTool.startTime) | |
| : undefined; | |
| const liveStartedAtRef = useRef(Date.now()); | |
| const runningDuration = | |
| animateSummary && activeTool?.startTime !== undefined | |
| ? formatLiveElapsed(summaryNow - activeTool.startTime) | |
| : animateSummary | |
| ? formatLiveElapsed(summaryNow - liveStartedAtRef.current) | |
| : undefined; | |
| useEffect(() => { | |
| if (!animateSummary) return; | |
| liveStartedAtRef.current = Date.now(); | |
| }, [animateSummary, activeTool?.callId]); |
中文说明
当 activeTool.startTime 为 undefined 时，runningDuration 失去了回退逻辑。startTime 字段在 ACPToolCall 上声明为可选的，该情况在旧会话数据或未标记 startTime 的 transcript 中可能出现。建议恢复旧代码中的 Date.now() 回退逻辑，确保运行中的工具组始终显示耗时。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): Context: PR #8872 updates Web Shell compact mode (Ctrl+O ...: none — all checks I started were completed within budget.; Context: PR #8872 updates Web Shell compact mode (Ctrl+O ...: none — all checks above completed within budget.; Context: PR #8872 updates Web Shell compact mode (Ctrl+O ...: none — all checks above completed within budget.; Context: PR #8872 updates Web Shell compact mode (Ctrl+O ...: none — all checks I started were completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：Context: PR #8872 updates Web Shell compact mode (Ctrl+O ...：none — all checks I started were completed within budget.；Context: PR #8872 updates Web Shell compact mode (Ctrl+O ...：none — all checks above completed within budget.；Context: PR #8872 updates Web Shell compact mode (Ctrl+O ...：none — all checks above completed within budget.；Context: PR #8872 updates Web Shell compact mode (Ctrl+O ...：none — all checks I started were completed within budget.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| }; | ||
| appendBlock(state, block); | ||
| if (opts.clearActiveText !== false) clearActiveText(state); | ||
| if (opts.clearActiveText !== false) clearActiveText(state, undefined, event); |
There was a problem hiding this comment.
[Suggestion] R3-1: The serverUpdatedAt stamping through clearActiveText(state, undefined, event) is untested for six terminator kinds — shell, user-shell, permission request, permission resolve, status, prompt-cancelled (call sites at transcript.ts ~1083/1128/1170/1223/1278/1302) — and for the resync-required/peer-cancel propagateCancellationToInFlightTools call sites (:379, :447). The new tests assert serverUpdatedAt only for terminators delivered by text deltas, tool.update, and assistant.done reason cancelled. — Failure scenario: probe-verified mutation — dropping the event argument at the permission-request call site keeps all 301 SDK tests green while the cleared block gets a degenerate pair (serverUpdatedAt === serverTimestamp); hasServerTimingPair requires strict >, so replay falls back to client-clock durations for exactly those turns (the common permission-interrupted shape) — silently reintroducing the replay-time elapsed drift this PR exists to eliminate, with no CI signal. Fix: one representative reducer test per uncovered class (they share clearActiveText) — a stamped thought.text.delta then a permission/shell/status event with a later serverTimestamp, asserting the block ends with serverUpdatedAt: <terminator> — plus a serverUpdatedAt assertion in the existing state_resync_required test.
中文说明
经由 clearActiveText(state, undefined, event) 的 serverUpdatedAt 打点有六类终止事件未被测试覆盖——shell、user-shell、permission request、permission resolve、status、prompt-cancelled（调用点位于 transcript.ts ~1083/1128/1170/1223/1278/1302），resync-required/peer-cancel 的 propagateCancellationToInFlightTools 调用点（:379、:447）同样未覆盖。新测试只对 text delta、tool.update、reason 为 cancelled 的 assistant.done 这三种终止事件断言了 serverUpdatedAt。—— 失败场景：探针已验证的变异——在 permission-request 调用点丢掉 event 参数后全部 301 个 SDK 测试仍然全绿，但被清除的块会拿到退化时间对（serverUpdatedAt === serverTimestamp）；hasServerTimingPair 要求严格 >，于是回放时这些回合（常见的被权限请求打断的形态）会退回客户端时钟耗时——本 PR 要消除的回放耗时漂移被静默重新引入，且 CI 无任何信号。修复：每类未覆盖终止事件补一个代表性 reducer 测试（它们共用 clearActiveText）——先发一个带 serverTimestamp 的 thought.text.delta，再发一个更晚 serverTimestamp 的 permission/shell/status 事件，断言块最终以 serverUpdatedAt: <终止事件时间> 收尾；并在现有 state_resync_required 测试中补上对进行中工具获得 serverUpdatedAt 的断言。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| ) : ( | ||
| output && <pre className={styles.output}>{output}</pre> | ||
| )} | ||
| {output && <pre className={styles.output}>{output}</pre>} |
There was a problem hiding this comment.
[Suggestion] R3-2: This PR deletes both consumers of the compact.hint i18n key (CompactToolGroup in ToolGroup.tsx and the compact branch of UserShellMessage) but leaves the key defined in both locales (client/i18n.tsx:1447 en, :4208 zh-CN), with text describing behavior that no longer exists. — Failure scenario: a repo-wide grep matches only the two catalog definitions — nothing renders them; "Press Ctrl+O to show full tool output" / "按 Ctrl+O 显示完整工具输出" ship dead in the bundle and are maintained indefinitely for a UI that no longer exists, and Ctrl+O now hides thinking — so the wording misrepresents the shortcut if anyone resurrects the key. Fix: delete the two 'compact.hint' entries from client/i18n.tsx (the neighboring compact.enabled/compact.disabled/compact.saveFailed keys are still consumed by App.tsx and should stay).
中文说明
本 PR 删除了 compact.hint i18n 键的全部两个消费者（ToolGroup.tsx 中的 CompactToolGroup 与 UserShellMessage 的紧凑分支），但两种语言的键定义仍保留在 client/i18n.tsx:1447（en）与 :4208（zh-CN），文案描述的已是不存在的行为。—— 失败场景：全仓 grep 只命中这两处目录定义——没有任何地方渲染它们；"Press Ctrl+O to show full tool output" / "按 Ctrl+O 显示完整工具输出" 作为死代码随 bundle 发布，并为一个已不存在的 UI 长期维护；且 Ctrl+O 现在的行为是隐藏思考，一旦有人复用该键，文案会错误描述快捷键。修复：从 client/i18n.tsx 删除这两个 'compact.hint' 条目（相邻的 compact.enabled/compact.disabled/compact.saveFailed 仍被 App.tsx 使用，应保留）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (kind !== 'assistant') clearActiveAssistant(state, event); | ||
| if (kind !== 'thought') clearActiveThought(state, event); |
There was a problem hiding this comment.
[Suggestion] R3-3: Cross-kind text-delta finalization passes the raw successor event (with eventId) into clearActiveAssistant/clearActiveThought → finalizeStreamingTextBlock, which rewrites the finalized block's eventId to its successor's first-delta id — contradicting the strip-eventId invariant this same PR encodes in clearActiveText ("Terminator events close the streaming block but do not own its content… keeping the block's eventId, which anchors replay ordering"). The - lines show these clears previously passed no event, so this PR introduces the overwrite; the parent-keyed variants (~692/695) have the identical problem. — Failure scenario: probe-verified — after a stamped thought→assistant transition, two distinct blocks share one SSE cursor (the thought loses its own last-delta id and carries the assistant's); the added replay test blesses this state without asserting eventId, so deleting the clearActiveText strip while keeping this path raw leaves all tests green. No observable mis-sort today (stable sort + serverTimestamp/clientReceivedAt tie-breaks save it), but the replay-ordering anchor the PR deliberately preserves on every other terminator path it touches is silently lost on the most common one. Fix (probe-verified, 296 tests green): pass an eventId-stripped stamp at all four cross-kind clears, mirroring clearActiveText:
const stamp = { ...event, eventId: undefined };
if (kind !== 'assistant') clearActiveAssistant(state, stamp);
if (kind !== 'thought') clearActiveThought(state, stamp);中文说明
跨类型 text-delta 终结把带 eventId 的后继事件原样传给 clearActiveAssistant/clearActiveThought → finalizeStreamingTextBlock，将已终结块的 eventId 覆写为后继块首个 delta 的 id——与本 PR 在 clearActiveText 中明确编码的剥离 eventId 不变量相矛盾（"终止事件关闭流式块但不拥有其内容……保留块自己的 eventId，它是回放排序的锚点"）。diff 的 - 行显示这些清除调用此前不传事件，因此该覆写是本 PR 引入的；父键变体（~692/695）存在同样问题。—— 失败场景：探针已验证——带时间戳的 thought→assistant 转换后，两个不同的块共享同一个 SSE 游标（thought 丢失自己最后一个 delta 的 id，带上了 assistant 的 id）；新增回放测试未对 eventId 断言，因此删掉 clearActiveText 的剥离而保留本路径原样传事件，所有测试仍全绿。当前无可观察的排序错误（稳定排序 + serverTimestamp/clientReceivedAt 兜底救了它），但本 PR 在其触碰的每条终止路径上刻意保留的回放排序锚点，恰恰在最常见的一条路径上被静默丢弃。修复（探针已验证，296 个测试全绿）：四处跨类型清除调用都传剥离 eventId 的 stamp，与 clearActiveText 保持一致：
const stamp = { ...event, eventId: undefined };
if (kind !== 'assistant') clearActiveAssistant(state, stamp);
if (kind !== 'thought') clearActiveThought(state, stamp);— qwen3.8-max via Qwen Code /review (v0.21.9)
| const activeSummaries = foregroundActiveTools.map((tool) => | ||
| isAskUserQuestionToolName(tool.toolName) | ||
| ? t('toolGroup.summary.provideInformation') | ||
| : formatSingleToolSummary(tool, t), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-4: The new multi-tool running summary builds per-tool descriptions via formatSingleToolSummary(tool, t) without workspaceCwd, so pathForDisplay(path, undefined) falls back to bare basenames — while the single-tool summary (SingleToolSummary at ~696) and every expanded ToolLine header pass workspaceCwd and show cwd-relative paths for the identical tools. — Failure scenario: probe-measured with two parallel foreground tools on same-basename files — the collapsed group renders Running ReadFile i18n.tsx · Edit i18n.tsx · 2 tools, two different files indistinguishable and directory context lost, exactly in the busy parallel-tool case this change rewrites (and compact mode's group merging makes more likely). The pre-PR convention cited for the cwd-less call (formatCompletedToolSummary) renders no paths at all, so this branch newly introduces the degradation. Fix: thread the already-available workspaceCwd (a ToolGroup prop, in scope at the ~1571 call site) through:
function formatToolGroupSummary(
tools: DaemonMessageToolCall[],
t: (key: string, options?: Record<string, unknown>) => string,
duration?: string,
workspaceCwd?: string,
): string {
// ... in the multi-tool branch:
: formatSingleToolSummary(tool, t, workspaceCwd),(probe-verified: restores full cwd-relative paths in the joined summary)
中文说明
新的多工具运行摘要通过 formatSingleToolSummary(tool, t) 生成每个工具的描述时没有传 workspaceCwd，于是 pathForDisplay(path, undefined) 退化为裸文件名——而单工具摘要（~696 处的 SingleToolSummary）和每个展开的 ToolLine 头部都传了 workspaceCwd，同样的工具显示的是相对工作区的路径。—— 失败场景：探针实测两个并行的前台工具操作同名文件——折叠组渲染为 Running ReadFile i18n.tsx · Edit i18n.tsx · 2 tools，两个不同文件无法区分、目录上下文丢失，恰好发生在本次改动重写的多工具并行场景（紧凑模式的组合并使其更常见）。支持无 cwd 调用所引用的既有惯例（formatCompletedToolSummary）根本不渲染路径，因此该退化是本分支新引入的。修复：把调用点（~1571，ToolGroup 的 prop）已可用的 workspaceCwd 透传下去：
function formatToolGroupSummary(
tools: DaemonMessageToolCall[],
t: (key: string, options?: Record<string, unknown>) => string,
duration?: string,
workspaceCwd?: string,
): string {
// ... 多工具分支中：
: formatSingleToolSummary(tool, t, workspaceCwd),（探针已验证：联合摘要恢复完整的相对工作区路径）
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(settingsSetValue).toHaveBeenCalledWith( | ||
| 'workspace', | ||
| 'ui.compactMode', | ||
| true, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-5: This new test — the PR's sole coverage of the user-facing Ctrl+O binding (grep confirms the only key: 'o' dispatch in the client) — dispatches one keydown and asserts persistence only. The handler does two things: setCompactMode(next) and setWorkspaceSetting(...); the test observes only the second, and toggle-off is never checked. — Failure scenario: two regressions both keep this test green: (1) setCompactMode(next) dropped in handleToggleCompact — Ctrl+O persists the setting but the running UI never changes until reload; (2) the flip replaced with a hardcoded true — the first toggle is asserted, a second press never is. Fix: dispatch a second toggle and expect settingsSetValue to have been last called with ('workspace', 'ui.compactMode', false) — the handler flips off compactModeRef.current, so this is one line in the existing harness.
中文说明
这个新测试是本 PR 对用户可见 Ctrl+O 绑定的唯一覆盖（grep 确认这是整个 client 中唯一的 key: 'o' 派发），但它只派发一次按键且只断言持久化。处理器做两件事：setCompactMode(next) 与 setWorkspaceSetting(...)；测试只观察了第二件，且从未检查关闭方向。—— 失败场景：两种回归都能让该测试保持绿色：(1) handleToggleCompact 中丢掉 setCompactMode(next)——Ctrl+O 持久化了设置但运行中的 UI 直到刷新都不变；(2) 翻转被硬编码为 true——首次切换被断言，第二次按键从未被检查。修复：再派发一次切换并断言 settingsSetValue 最后一次被调用为 ('workspace', 'ui.compactMode', false)——处理器基于 compactModeRef.current 翻转，在现有测试桩中只需一行。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #8872Feedback triageOne inline finding was actionable this round (Critical-only mode deferred no items into the actionable sections; the deferred section carried no entries). [Suggestion]
|
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round summary (PR #8872) — no action takenRound outcome: no actionable work this round. This round ran in Critical-only mode (active after 5 change-producing review rounds). The workflow's feedback triage found no actionable items:
The only feedback listed was in the No commits were made; the branch head is unchanged ( 中文说明Autofix 轮次总结（PR #8872）— 本轮未采取任何操作本轮结果：本轮没有可处理的工作。 本轮在仅处理 Critical 的模式下运行（在完成 5 个产生改动的评审轮次后激活）。工作流的反馈分诊未发现任何可执行条目：
唯一列出的反馈位于 本轮未提交任何 commit；分支 head 保持不变（ Deferred non-Critical feedbackCritical-only mode is active after 5 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. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): Context: PR #8872 "feat(web-shell): improve thinking and ...: none — all checks above completed within budget.; Context: PR #8872 "feat(web-shell): improve thinking and ...: none — finished within budget (~20 calls).; Context: PR #8872 "feat(web-shell): improve thinking and ...: none — all checks above were completed within budget.; Context: PR #8872 "feat(web-shell): improve thinking and ...: none — all checks above completed within budget.; Context: PR #8872 "feat(web-shell): improve thinking and ...: none — all checks above completed within budget., and 2 more.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：Context: PR #8872 "feat(web-shell): improve thinking and ...：none — all checks above completed within budget.；Context: PR #8872 "feat(web-shell): improve thinking and ...：none — finished within budget (~20 calls).；Context: PR #8872 "feat(web-shell): improve thinking and ...：none — all checks above were completed within budget.；Context: PR #8872 "feat(web-shell): improve thinking and ...：none — all checks above completed within budget.；Context: PR #8872 "feat(web-shell): improve thinking and ...：none — all checks above completed within budget.，另有 2 条。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| ? t('toolGroup.summary.provideInformation') | ||
| : formatSingleToolSummary(tool, t), |
There was a problem hiding this comment.
[Suggestion] R3-4: The new multi-tool running summary builds per-tool descriptions via formatSingleToolSummary(tool, t) without workspaceCwd, so pathForDisplay(path, undefined) falls back to bare basenames — while the single-tool summary (SingleToolSummary ~696) and every expanded ToolLine header pass workspaceCwd and show cwd-relative paths for the identical tools. — Failure scenario: probe-measured with two parallel foreground tools on same-basename files — the collapsed group renders Running ReadFile i18n.tsx · Edit i18n.tsx · 2 tools, two different files indistinguishable and directory context lost, exactly in the busy parallel-tool case this change rewrites (and compact mode's group merging makes more likely). Still stands at this head. Fix: thread the already-available workspaceCwd (a ToolGroup prop, in scope at the ~1578 call site) through:
function formatToolGroupSummary(
tools: DaemonMessageToolCall[],
t: (key: string, options?: Record<string, unknown>) => string,
duration?: string,
workspaceCwd?: string,
): string {
// ... in the multi-tool branch:
: formatSingleToolSummary(tool, t, workspaceCwd),中文说明
新的多工具运行摘要通过 formatSingleToolSummary(tool, t) 生成每个工具的描述时没有传 workspaceCwd，于是 pathForDisplay(path, undefined) 退化为裸文件名——而单工具摘要（~696 处的 SingleToolSummary）和每个展开的 ToolLine 头部都传了 workspaceCwd，同样的工具显示的是相对工作区的路径。—— 失败场景：探针实测两个并行的前台工具操作同名文件——折叠组渲染为 Running ReadFile i18n.tsx · Edit i18n.tsx · 2 tools，两个不同文件无法区分、目录上下文丢失，恰好发生在本次改动重写的多工具并行场景（紧凑模式的组合并使其更常见）。该问题在当前 head 仍然存在。修复：把调用点（~1578，ToolGroup 的 prop）已可用的 workspaceCwd 透传进 formatToolGroupSummary，在多工具分支中调用 formatSingleToolSummary(tool, t, workspaceCwd)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| it('uses Ctrl+O and persists the existing workspace setting', async () => { | ||
| renderApp(); | ||
| await toggleCompactMode(); |
There was a problem hiding this comment.
[Suggestion] R3-5: This new test — the PR's sole coverage of the user-facing Ctrl+O binding (grep confirms the only key: 'o' dispatch in the client) — dispatches one keydown and asserts persistence only. The handler does two things: setCompactMode(next) and setWorkspaceSetting(...); the test observes only the second, and toggle-off is never checked. — Failure scenario: two regressions both keep this test green: (1) setCompactMode(next) dropped in handleToggleCompact — Ctrl+O persists the setting but the running UI never changes until reload; (2) the flip replaced with a hardcoded true — the first toggle is asserted, a second press never is. Still stands at this head. Fix: dispatch a second toggle and expect the setting to flip off — the handler flips off compactModeRef.current, so this is one line in the existing harness:
it('uses Ctrl+O and persists the existing workspace setting', async () => {
renderApp();
await toggleCompactMode();
await toggleCompactMode();
expect(settingsSetValue).toHaveBeenLastCalledWith(
'workspace',
'ui.compactMode',
false,
);
});中文说明
这个新测试是本 PR 对用户可见 Ctrl+O 绑定的唯一覆盖（grep 确认这是整个 client 中唯一的 key: 'o' 派发），但它只派发一次按键且只断言持久化。处理器做两件事：setCompactMode(next) 与 setWorkspaceSetting(...)；测试只观察了第二件，且从未检查关闭方向。—— 失败场景：两种回归都能让该测试保持绿色：(1) handleToggleCompact 中丢掉 setCompactMode(next)——Ctrl+O 持久化了设置但运行中的 UI 直到刷新都不变；(2) 翻转被硬编码为 true——首次切换被断言，第二次按键从未被检查。该问题在当前 head 仍然存在。修复：再派发一次切换并断言设置翻转为 false——处理器基于 compactModeRef.current 翻转，在现有测试桩中只需一行。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| <CompactModeContext.Provider value={false}> | ||
| <TranscriptRenderModeProvider value={entry.transcriptRenderMode}> |
There was a problem hiding this comment.
[Suggestion] R4-2: rerenderMessages hardcodes <CompactModeContext.Provider value={false}> while its paired mount() accepts a compactMode option; unlike transcriptRenderMode, which the helper deliberately persists on the mounted entry and restores, the compact mode chosen at mount is silently dropped on re-render. — Failure scenario: a compact-mode test (the exact suite this PR adds) that mounts with compactMode: true and then updates the list via rerenderMessages — e.g. simulating streaming tool/thinking arrivals — silently re-renders in non-compact mode: hidden thinking rows reappear and merged tool groups split back apart, so assertions pass or fail against the wrong mode. No current test exercises this yet (latent), but this diff adds both halves of the asymmetry. Fix: persist the mode on the mounted entry next to transcriptRenderMode and restore it here:
// mount(): push({ root, container, transcriptRenderMode, compactMode: opts.compactMode ?? false })
// rerenderMessages():
<CompactModeContext.Provider value={entry.compactMode}>中文说明
rerenderMessages 把 <CompactModeContext.Provider value={false}> 硬编码为 false，而与之配对的 mount() 接受 compactMode 选项；transcriptRenderMode 被有意保存在挂载条目上并在重渲染时恢复，compactMode 却没有——挂载时选择的紧凑模式在重渲染时被静默丢弃。—— 失败场景：一个以 compactMode: true 挂载、随后通过 rerenderMessages 更新列表（例如模拟流式到达工具/思考）的紧凑模式测试（正是本 PR 新增的测试套件）会静默地以非紧凑模式重渲染：被隐藏的思考行重新出现、已合并的工具组重新拆开，断言就会按错误的模式通过或失败。目前尚无测试触发该路径（潜在问题），但这一不对称的两半都是本 diff 新增的。修复：与 transcriptRenderMode 一样把 compactMode 保存在挂载条目上（push({ root, container, transcriptRenderMode, compactMode: opts.compactMode ?? false })），并在 rerenderMessages 中以 entry.compactMode 恢复。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action takenNo changes were made in this round, and nothing was committed. Why: the actionable sections of this round's feedback are empty — there are no new reviews, no inline comments, no issue-level comments, no failed checks, and no still-red checks to address. The PR is in Critical-only mode after five change-producing rounds; the feedback listed under "Deferred non-Critical feedback" is an audit record excluded from this round's scope, so it was intentionally left untouched (no code changes, no thread resolutions, no comment replies for those items). No base-conflict resolution was requested. Verification: no verification commands were run because no code was changed and no commit was made. 中文说明Autofix 审查轮次：未采取任何操作本轮未做任何改动，也没有提交。 原因： 本轮反馈的可执行区域为空 —— 没有新的 review、没有行内评论、没有 issue 级评论、没有失败的检查，也没有持续红灯的检查需要处理。该 PR 在经历 5 个产生改动的轮次后已进入仅处理 Critical 的模式；"Deferred non-Critical feedback"（已延后的非 Critical 反馈）中列出的反馈属于本轮范围之外的审计记录，因此有意保持原样（不对这些条目做代码改动、不解决其线程、也不回复评论）。本轮也没有要求解决与 base 分支的冲突。 验证： 由于没有改动任何代码、也没有产生提交，本轮未运行任何验证命令。 Deferred non-Critical feedbackCritical-only mode is active after 5 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. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 42 passed · 0 failed · 42 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：42 通过 · 0 失败 · 42 总计 Verification reportPR 8872 deep verification — round 2 (follow-up)Verdict: 中文摘要判定：
Previous-round finding status (follow-up round)The round-1 report was a build-failure notice (its snapshot reads as a status
Re-measurement (not diffed from the old report): rebuilt
Attribution: ScopeCentral claim — replay-stable timing: transcript blocks retain the first and Secondary claims — (a) compact mode hides thinking and merges only regular Out of scope by choice: browser-level visual styling, perf of the merge pass A/B — central claim (timing)Harness Scenario cells: completed tool with valid daemon pair (call-A), thinking block
13 head assertions + 3 base-control assertions (the base arm must exhibit the The block dump corroborates the mechanism at reducer level: head thought block A/B — secondary claims (compact merge + summary)One byte-identical vitest file (
Head 8/8; base fails exactly the four discriminating cells with behavioral Vacuity / mutation matrixEach mutant reverts one key hunk in a scratch worktree at HEAD (source only); the
Targeted gates (head)
Repo-wide gates were not run (not claimed); nothing outside the two packages is FindingsF1 — nit/advisory: browser daemon bundle margin is now 444 bytes (0.23% of cap). No other findings. The timing fallbacks were probed beyond the reported shapes Not covered
MethodologyOne container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-Review Overview (AI Generated)
PR: #8872 feat(web-shell): improve thinking and tool progress display
Previous review: #pullrequestreview-4904633884 at HEAD 71a64b3 (COMMENT, 3 Minor + 5 Nit)
Current HEAD: 3795fb3 (+1201/-250 across 25 files, 13 commits)
Merge / CI readiness — both previously reported blockers are cleared
| Item | At last review | Now |
|---|---|---|
mergeable |
CONFLICTING |
true — resolved by 4 main merges (ed03d38, 7669bfc, 183db61, 4c628e8, 5b7a2ec, 33aa860) |
merge-tree npm ci bundle-budget |
failing | no failing checks at 3795fb3; Test (ubuntu, Node 22.x), web-shell E2E Smoke, Desktop Shell (ubuntu/windows), Post Coverage Comment all SUCCESS |
mergeStateStatus |
— | still BLOCKED, but only because qwen-code-ci-bot's CHANGES_REQUESTED from 71a64b3 was never dismissed. @yiliang114's CHANGES_REQUESTED was self-dismissed at 71a64b3. No CI failure blocks merge. |
New commits since 71a64b3
Nine commits arrived; six are main merges. Only three carry logic:
| Commit | Scope |
|---|---|
e228244 fix(web-shell): preserve compact mode semantics |
Reverts the compact-mode retirement: restores ui.compactMode in settingsSchema.ts / workspace-settings allowlist / vscode schema / settings.md, restores Ctrl+O -> handleToggleCompact, drops the showThinking customization prop, renames mergeToolGroupsAcrossThinking back to mergeCompactToolGroups, adds Ctrl+O to HelpDialog. Net effect: App.tsx, i18n.tsx, customization.tsx, settingsSchema.ts all fall out of the diff entirely (zero net change vs. base). |
fab05f1 chore: revert unrelated ACP test formatting |
Drops the unrelated integration-tests/cli/acp-integration.test.ts churn. Good scope hygiene. |
3795fb3 fix(web-shell): restore running duration fallback |
ToolGroup.tsx — re-adds a live-duration anchor when activeTool.startTime is missing (answers @doudouOUC's note on 5b7a2ec). |
Key structural fact for this re-review: I fetched both timing-critical files at 3795fb3 and diffed them byte-for-byte against 71a64b3:
packages/web-shell/client/adapters/transcriptToMessages.ts -> IDENTICAL
packages/sdk-typescript/src/daemon/ui/transcript.ts -> IDENTICAL
packages/web-shell/client/utils/todos.ts -> IDENTICAL
packages/web-shell/client/components/messages/tools/SubAgentPanel.tsx -> IDENTICAL
So every finding rooted in the timing rework is unchanged at both producer and consumer sites, and the line numbers from the last review still resolve exactly. The two findings that did get fixed were fixed by e228244's revert, not by a targeted change.
Re-Review Status at HEAD 3795fb3
| Previous finding | Status | Evidence at current HEAD |
|---|---|---|
M1 [Minor] startTime/endTime mix two clock domains; consumers compare across tools |
✗ Still present | adapters/transcriptToMessages.ts:71-86 — getTranscriptTiming still returns raw daemon serverStart/serverEnd on the complete + server-pair path (L80) while the no-pair path returns client createdAt/updatedAt (L81). Producer applied at L1052. Both cross-tool consumers untouched: utils/todos.ts:531 collectToolSpans still pushes {start: startTime, end: endTime} from every group and spans.sort((a,b) => a.start - b.start) before the binary search; messages/tools/SubAgentPanel.tsx:110 still renders <SubToolTime timestamp={tool.startTime}>. @ytahdn reproduced it ("a 500 ms server-domain tool span is dropped from a server-domain todo window") and deferred. |
M2 [Minor] serverTimestamp flipped last-write-wins -> first-write-wins |
✗ Still present (author declined, by design) | daemon/ui/transcript.ts:654 existing.serverTimestamp ??= event.serverTimestamp;. Both affected consumers unchanged: transcriptToMessages.ts:377 blockTime = block.serverTimestamp ?? block.clientReceivedAt (hover time), and App.tsx:563 setAt: status.setAt ?? block.serverTimestamp ?? block.createdAt. Author's rationale (first-observed is the documented contract, serverUpdatedAt carries the latest) is reasonable — but the two consumers still silently changed meaning and no test pins either. |
| M3 [Minor] thinking-bubble grouping driven by timer internals; replay splits the last thought block | ✗ Still present | transcriptToMessages.ts:548 usesServerPair, gate at :565 currentThinkingUsesServerPair === usesServerPair, latch at :586. @ytahdn: "Confirmed behavior... the current regression test explicitly pins that result." |
N1 [Nit] dead startTimeRef effect + no-op setFinishedAt(endTime) |
✗ Still present | messages/AssistantMessage.tsx:250-252 writes startTimeRef.current that L281 (startTime ?? startTimeRef.current) can never read; :264 setFinishedAt(endTime) is unobservable because :277 effectiveFinishedAt = endTime ?? finishedAt. |
N2 [Nit] startTime/endTime assigned before the status switch |
✗ Still present | transcriptToMessages.ts:125-126, still above the switch (update.status) at L127. |
N3 [Nit] mergeToolGroupsAcrossThinking name hides that it also drops thinking |
✅ Fixed by e228244 |
MessageList.tsx:282 is back to mergeCompactToolGroups, with hiding delegated to the self-describing isHiddenInCompactMode (L266). @ytahdn replied "Not fixed" at 7669bfc, but the later revert resolved it anyway. |
N4 [Nit] Ctrl+O became a silent toggle |
✅ Fixed / moot by e228244 |
client/App.tsx now has zero net diff vs. base (absent from /pulls/8872/files); Ctrl+O -> handleToggleCompact is main's implementation again, and HelpDialog.tsx:87 documents the binding. My original premise was partly wrong: compact.enabled / compact.disabled are dead in main too, so no toast was lost. |
| N6 [Nit] termination time written into a field documented as first-observed | ✗ Still present | daemon/ui/transcript.ts:501-506 — the else at L504-505 now routes the later stamp to serverUpdatedAt, but L503 still seeds block.serverTimestamp from a terminator event when the block was never stamped. @ytahdn: "Partially confirmed: hover time uses the terminator as the block start." |
Score: 2 of 8 fixed (both Nits). All 3 Minors remain, acknowledged-and-deferred rather than disputed.
@yiliang114's residual P3 — resolved as a non-gate, and my disagreement stands
P3 (cross-clock endTime on permission denial) was filed inside the review @yiliang114 then self-dismissed (4903860852), explicitly scoped "display-only, follow-up not gate". No further discussion since, and the code is unchanged.
Re-verifying the reachability argument against 3795fb3: transcriptToMessages.ts:717 sets permissionToolCall.endTime = permBlock.updatedAt (client clock) on denial, but a cross-clock pair only forms if existingPermission.startTime is already server-clock. getTranscriptTiming only yields a server-clock startTime when complete === true (L79-81), i.e. block.status is terminal -> previousStatus is terminal -> isTerminalToolStatus(previousStatus) at L724 fires and L729-730 restores previousEndTime, discarding the client stamp. Still unreachable via the normal path — I continue to disagree that it is a live defect.
One caveat worth recording, because it links two open items: N2 is the single way a non-terminal tool can end up holding a server-clock startTime (L125 assigns it above the switch, so a hypothetical non-terminal background-agent notification leaves status untouched while startTime becomes daemon-clock). Fixing N2 therefore also closes P3 permanently instead of relying on the terminal-status coincidence. That makes N2 worth more than its Nit label suggests.
New findings introduced by the new commits
| # | Sev | Location | Issue |
|---|---|---|---|
| NM1 | Minor | ToolGroup.tsx:612 |
Multi-tool running summary calls formatSingleToolSummary(tool, t) with no workspaceCwd, so summary paths are absolute while the expanded rows of the same card (L697) pass workspaceCwd and render workspace-relative. Sibling-consistency divergence. |
| NN1 | Nit | ToolGroup.tsx:1496-1510 |
The liveStartedAtRef fallback is read during render but re-anchored in a post-paint effect. |
| NN2 | Nit | UserShellMessage.tsx / i18n.tsx |
compact.hint (en + zh) is now orphaned — this PR removed both consumers (CompactToolGroup in ToolGroup.tsx, and the compact branch of UserShellMessage.tsx). The redefinition is deliberate and documented in the new design doc ("Compact mode no longer switches message bodies to their old condensed cards"), so this is cleanup only, but compact mode also no longer surfaces any affordance telling users what Ctrl+O reveals. Matches qwen-code-ci-bot's R3-2, still open. |
Structural audits over the new commits
- Data provenance tracing —
serverUpdatedAt(daemon/ui/types.ts:829): unchanged since the last review, so the M1/M2/M3 provenance conclusions carry over verbatim. ThestartTime/endTimeaddition toDaemonThinkingMessage(messageTypes.ts:108-109) has exactly one producer (transcriptToMessages.ts:579), and both grouping branches populate it — no second path to diverge from. ✅ - State field initialization matrix — new
ThinkingMessageprops degrade cleanly when absent (startTime ?? timestamp ?? Date.now()), andareMessagesEqual'sthinkingcase was correctly extended withstartTime/endTime(MessageItem.tsx:335-336). I checked the paralleltool_groupcase for the same gap:areToolCallsEqualalready compares both (MessageItem.tsx:415-416). ✅ Residual nit:areMessagesEqual'stool_groupcase (L375-382) still ignorestimestamp, whiche228244newly propagates onto merged groups (MessageList.tsx:343) — a merged group whose tools are identical but whose anchor timestamp shifted will keep a stale hover tooltip. Marginal; not filed. - Replay/reconnect divergence — the newly reachable case is
3795fb3's live-duration fallback. It only engages whenactiveTool.startTimeis undefined, andgetTranscriptTiming's in-flight branch always emits a number, so replayed transcripts keep their real anchor and the fabricated one is confined to the live ACP path. Correctly bounded. ✅ M3 remains the outstanding replay divergence. - Handler parallelism / blast radius —
MessageList.tsx: hoistinguseWebShellCustomization()from L2748 to L2509 is safe (no conditional hooks in between).MessageTimestamp.tsxdropping itstimestamp === undefined && !copyTextearly return now wraps every message instyles.row; the early return sat after all hooks, so hook order is unaffected, and the accompanying+4CSS is intentional. Spot-checked only, per UI priority.CompactModeContextmoved fromToolGrouptoMessageItem/AssistantMessage, which raises the App-mock breakage risk R1-14 flagged — CI'sTest (ubuntu, Node 22.x)is green, so the mocks were updated.
Stale inline comments from my previous review
| Comment | State |
|---|---|
M1 transcriptToMessages.ts:1052, M2 transcript.ts:654, M3 :565, N1 AssistantMessage.tsx:251, N2 :125, N6 transcript.ts:503 |
Still VALID — identical file content, identical line numbers, all six lines still inside the current diff |
N3 MessageList.tsx:293 |
OBSOLETE — the flagged identifier no longer exists at HEAD |
N4 App.tsx:5911 |
ORPHANED — client/App.tsx left the PR's file set entirely (zero net diff) |
I have re-posted the six still-valid findings as fresh inline comments anchored to 3795fb3 so they are not lost behind the outdated markers.
Findings Summary
- Critical/Major: 0
- Minor: 4 (M1, M2, M3 carried over + NM1 new)
- Nit: 5 (N1, N2, N6 carried over + NN1, NN2 new)
Final Verdict
COMMENT — not APPROVE. The two mechanical blockers are genuinely gone (conflict resolved, CI green) and e228244/fab05f1 are real quality improvements: reverting the compact-mode retirement removed an entire class of orphaned-surface findings (R1-5/R1-6/R1-7/N3/N4) and shrank the PR from 38 files to 25. But the timing rework — this PR's stated core goal — is byte-for-byte unchanged since the last review, so all three Minors stand, with M1 the one I'd still gate on: it is confirmed-reproducible by the author and its two cross-tool consumers (todos.ts:531, SubAgentPanel.tsx:110) are silently wrong rather than degraded. A single normalization at the getTranscriptTiming boundary — project the daemon-measured interval onto the client clock for the complete path too, exactly as the in-flight branch at L86 already does — would close M1 at the producer and leave both consumers untouched. Everything else here is legitimately deferrable.
This review was generated by QoderWork AI
| block.status === 'failed' || | ||
| block.status === 'cancelled' || | ||
| block.status === 'canceled'; | ||
| const timing = getTranscriptTiming(block, isComplete && !isBackgroundAgent); |
There was a problem hiding this comment.
[Minor] M1 — still present at 3795fb3; unchanged at the producer and at both cross-tool consumers.
I diffed this file byte-for-byte against 71a64b3: it is identical, as are utils/todos.ts and messages/tools/SubAgentPanel.tsx. So nothing about M1 moved.
getTranscriptTiming (L71-86) still emits two incompatible domains for the complete case:
- L80 server pair ->
{startTime: serverStart, endTime: serverEnd}(daemon clock) - L81 no pair ->
{startTime: block.createdAt, endTime: block.updatedAt}(client clock)
The in-flight branch at L86 already does the right thing — block.updatedAt - elapsed projects the daemon-measured interval onto the client clock, so it is domain-safe. The complete branch is the only one that leaks absolute daemon time.
Both consumers that compare these values across tools are untouched:
utils/todos.ts:531collectToolSpanspushes{start: startTime, end: endTime}from every group, thenspans.sort((a, b) => a.start - b.start)feeds a binary search over todo windows. Mixed anchors mis-order the array, so the binary search can land in the wrong window and mis-attribute task duration. You reproduced exactly this ("a 500 ms server-domain tool span is dropped from a server-domain todo window").messages/tools/SubAgentPanel.tsx:110renders<SubToolTime timestamp={tool.startTime}>as an absolute wall-clock label, so sibling rows in one panel can read out of order under clock skew.
The fix is one line and needs no consumer changes — make complete domain-safe the same way the in-flight branch already is:
if (complete) {
if (!hasServerPair) {
return { startTime: block.createdAt, endTime: block.updatedAt };
}
// Keep the daemon-measured interval, but anchor it in the client domain so
// startTime/endTime stay comparable across tools (see collectToolSpans).
const elapsed = Math.max(0, serverEnd! - serverStart!);
return { startTime: block.updatedAt - elapsed, endTime: block.updatedAt };
}That preserves the replay-stable duration this PR exists to deliver while keeping every startTime/endTime in one domain. Alternatively, keep the raw daemon pair but add a timingDomain: 'server' | 'client' discriminator and make collectToolSpans bucket by domain — more invasive, and SubAgentPanel would still need a conversion.
This review was generated by QoderWork AI
| if (event.eventId !== undefined) existing.eventId = event.eventId; | ||
| if (event.serverTimestamp !== undefined) { | ||
| existing.serverTimestamp = event.serverTimestamp; | ||
| existing.serverTimestamp ??= event.serverTimestamp; |
There was a problem hiding this comment.
[Minor] M2 — still present at 3795fb3. Your rationale is sound; the gap is that the two changed consumers are still unpinned.
This file is byte-for-byte identical to 71a64b3, so existing.serverTimestamp ??= event.serverTimestamp stands.
I accept the design argument: serverTimestamp is documented as first observed and serverUpdatedAt now carries the latest stamp, so ??= makes the field match its own doc comment. Reclassifying from "silent change" to "intentional contract tightening" is fair.
What still concerns me is that two pre-existing consumers changed observable behavior with no test:
web-shell/client/adapters/transcriptToMessages.ts:377—blockTime = block.serverTimestamp ?? block.clientReceivedAtdrives the hover tooltip. A multi-delta assistant/thought block now shows its first delta time instead of its last.web-shell/client/App.tsx:563—setAt: status.setAt ?? block.serverTimestamp ?? block.createdAtfor goal state.
Both are defensible (arguably more correct), but nothing in the suite would notice a regression back to last-write-wins. A single daemonUi case sending two timestamped thought.text.deltas into one block and asserting {serverTimestamp: <first>, serverUpdatedAt: <second>} pins the new contract cheaply — this is also qwen-code-ci-bot's still-open R1-13.
This review was generated by QoderWork AI
| target && | ||
| target.role === 'thinking' && | ||
| !needsNewContentMessage && | ||
| currentThinkingUsesServerPair === usesServerPair |
There was a problem hiding this comment.
[Minor] M3 — still present at 3795fb3; unchanged, and still pinned by a test that encodes the symptom.
usesServerPair (L548), the grouping gate currentThinkingUsesServerPair === usesServerPair (L565), and the latch (L586) are all unchanged.
You confirmed the behavior. The concern I'd restate is the coupling direction: a presentation decision (does this thought join the previous bubble?) is now a function of timing plumbing (did the daemon happen to stamp a positive-elapsed serverTimestamp/serverUpdatedAt pair on this block?). Those are independent axes, so on replay a turn whose final thought block never received serverUpdatedAt splits into a second bubble that live rendering never showed — the same replay instability this PR is trying to remove, relocated from durations to bubble boundaries. The existing regression test asserting the split makes the coupling load-bearing rather than incidental.
Decoupling keeps both properties: group on transcript structure only (parentToolCallId, needsNewContentMessage, streaming state), and when merging blocks with heterogeneous timing just degrade the duration — e.g. drop endTime on the merged bubble, or accumulate only the same-domain deltas — instead of splitting the bubble. Note this becomes materially cheaper once M1 is normalized: with a single domain, usesServerPair stops being an observable distinction and the gate can simply be deleted.
This review was generated by QoderWork AI
| const translationAbortRef = useRef<AbortController | undefined>(undefined); | ||
|
|
||
| useEffect(() => { | ||
| if (startTime !== undefined) startTimeRef.current = startTime; |
There was a problem hiding this comment.
[Nit] N1 — still present at 3795fb3; both halves confirmed dead on re-read.
Re-verified against the current file (the compactMode change in e228244 did not touch this block):
- L250-252 writes
startTimeRef.current = startTime, but the only read is L281(startTime ?? startTimeRef.current)— the ref is consulted only whenstartTimeisundefined, which is exactly when the effect's guard prevents the write. The two conditions are mutually exclusive, so the assignment can never influence a render. TheuseRefinitializer at L239 already seeds the value correctly. - L263-265
setFinishedAt(endTime)is unobservable: L277effectiveFinishedAt = endTime ?? finishedAtshort-circuits onendTime, so the state write only costs a render pass.
Agreed this is cosmetic on the current transcript path, and "redundant-render, follow-up" is a fine disposition. Flagging only because both fragments read as intentional fallback wiring, so a future reader may build on a mechanism that does nothing. Deleting L250-252 and returning early at L263 (if (endTime !== undefined) return;) is behavior-preserving.
This review was generated by QoderWork AI
| block.serverTimestamp !== undefined && | ||
| update.serverEndTime !== undefined && | ||
| update.serverEndTime > block.serverTimestamp; | ||
| tool.startTime = hasServerPair ? block.serverTimestamp : block.createdAt; |
There was a problem hiding this comment.
[Nit -> worth more than a Nit] N2 — still present at 3795fb3, and it is the one thing keeping @yiliang114's P3 alive.
L125-126 still assign startTime/endTime above the switch (update.status) at L127, so a notification whose status is none of completed / failed / cancelled / canceled falls through the switch untouched while still having overwritten both timing fields — giving an in-flight tool a daemon-clock startTime. Agreed the current producer only emits terminal statuses, so it is defensive-only today.
The reason I'd raise the priority: this is the only path by which a non-terminal tool can hold a server-clock startTime, and that is precisely the precondition P3 needs.
I argued in the last round that P3 is unreachable, and re-verifying at 3795fb3 the argument holds: L717 stamps a client-clock endTime on denial, but the cross-clock pair only survives if the existing tool's startTime is server-clock; getTranscriptTiming only returns that when complete === true (L79-81), which implies a terminal block.status, which makes previousStatus terminal, which fires isTerminalToolStatus(previousStatus) at L724 and restores previousEndTime at L729-730 — discarding the client stamp. N2 is the sole hole in that chain.
So moving both assignments into the three terminal case arms turns P3 from "unreachable by coincidence" into "unreachable by construction", and closes this finding at the same time:
switch (update.status) {
case 'completed':
tool.status = 'completed';
applyTiming(tool, block, update);
break;
// ... same for 'failed' / 'cancelled' / 'canceled'
}That also aligns this helper with getTranscriptTiming's own rule that absolute server time is only valid once a block is complete. Also note the local hasServerPair here (L122-124) still omits the serverEnd > serverStart positive-elapsed requirement that hasServerTimingPair (L61-68) enforces — qwen-code-ci-bot's R1-4, still open.
This review was generated by QoderWork AI
| block.serverTimestamp = event.serverTimestamp; | ||
| if (event?.serverTimestamp !== undefined) { | ||
| if (block.serverTimestamp === undefined) { | ||
| block.serverTimestamp = event.serverTimestamp; |
There was a problem hiding this comment.
[Nit] N6 — still present at 3795fb3. Your "partially confirmed" is the accurate reading.
The else branch (L504-505) correctly routes a later stamp to serverUpdatedAt, so the field this PR adds behaves as documented. What remains is L502-503: when a block was never stamped, the terminator event's serverTimestamp is seeded into block.serverTimestamp, a field documented at daemon/ui/types.ts:826 as the first observed time of the block.
You confirmed the hover-time consequence (transcriptToMessages.ts:377 reads serverTimestamp and would show the terminator's time as the block's start) and correctly noted my goal-state claim does not hold here. Agreed it is pre-existing rather than introduced, and low-impact — a block reaching finalization with no prior stamp is already a degraded record.
Recording it because this PR is what makes the distinction load-bearing: with serverUpdatedAt now present, "first observed" vs. "latest observed" has real semantics, and a comment at L502 noting that this seed is a degraded-record fallback (not a true start) would stop a future reader from treating it as authoritative — especially since hasServerTimingPair deliberately rejects the resulting degenerate pair.
This review was generated by QoderWork AI
| const activeSummaries = foregroundActiveTools.map((tool) => | ||
| isAskUserQuestionToolName(tool.toolName) | ||
| ? t('toolGroup.summary.provideInformation') | ||
| : formatSingleToolSummary(tool, t), |
There was a problem hiding this comment.
[Minor] NM1 (new) — the multi-tool running summary drops workspaceCwd, so one card renders paths in two different formats.
formatToolGroupSummary (L590-594) takes (tools, t, duration) with no workspaceCwd, so this call reaches formatSingleToolSummary(tool, t) with the third parameter omitted. That parameter is threaded all the way down to getToolSummaryDescription(tool, workspaceCwd), which is what shortens absolute paths to workspace-relative ones.
The inconsistency is within a single card: the sibling render path at L697 calls formatSingleToolSummary(tool, t, workspaceCwd) with the cwd, so an expanded row shows src/foo.ts while the collapsed running summary directly above it shows /Users/<name>/<project>/src/foo.ts. Before this PR the summary only ever emitted a localized tool name, so no path formatting was involved and the divergence did not exist.
workspaceCwd is already in scope at the only call site (L1578, inside ToolGroup), so this is a parameter thread-through:
export function formatToolGroupSummary(
tools: ACPToolCall[],
t: ReturnType<typeof useI18n>['t'],
duration?: string,
workspaceCwd?: string,
): string {
// ...
: formatSingleToolSummary(tool, t, workspaceCwd),Independently found here; qwen-code-ci-bot raised the same thing as R3-4 and it is still open at 3795fb3. Secondary note on the same block: t('toolGroup.running', {...}) still passes count: tools.length (group total) while name now enumerates only the active foreground tools, so for a partially finished group the count no longer describes the listed names.
This review was generated by QoderWork AI
| const runningDuration = animateSummary | ||
| ? formatLiveElapsed(summaryNow - liveStartedAtRef.current) | ||
| ? formatLiveElapsed( | ||
| summaryNow - (activeTool?.startTime ?? liveStartedAtRef.current), |
There was a problem hiding this comment.
[Nit] NN1 (new, 3795fb3) — the fallback anchor is read during render but re-anchored after paint, so the first frame of a live group can show a duration that then jumps backwards.
(Anchored on the read at L1503; the paired re-anchor effect is L1507-1510.)
The fix itself is right — @doudouOUC's scenario (a running group whose activeTool.startTime is missing rendering no duration at all) is real, and getTranscriptTiming guarantees replayed in-flight blocks always carry a numeric startTime, so the fabricated anchor is correctly confined to the live ACP path where no better value exists. Good scoping.
The ordering is the issue. runningDuration (L1501-1505) reads liveStartedAtRef.current during render, but the effect at L1507-1510 re-anchors it after paint. On the render where animateSummary flips false -> true, or where activeTool?.callId changes, the value used is the previous anchor — for a group mounted earlier (completed tool, then a new tool starts in the same group) that is mount time, potentially minutes stale. Because a ref write does not trigger a re-render, the inflated figure survives until the next useSharedNow tick, so the user sees e.g. 3m 12s and then a jump back to 0s.
Deriving the anchor during render keeps it monotonic and removes the effect entirely:
const liveStartedAtRef = useRef(Date.now());
const liveAnchorKeyRef = useRef<string | undefined>(undefined);
const anchorKey = animateSummary ? activeTool?.callId : undefined;
if (anchorKey !== liveAnchorKeyRef.current) {
liveAnchorKeyRef.current = anchorKey;
if (anchorKey !== undefined) liveStartedAtRef.current = Date.now();
}Also worth a brief comment that this anchor is observation time, not tool start time: after a refresh mid-run it restarts at 0s, which is a deliberate trade (a slightly-low duration beats none) but is surprising without a note.
This review was generated by QoderWork AI
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： PR #8872 — review feedback round summaryAll eight actionable inline findings were verified against HEAD Findings and dispositionsM1 —
|
|
Closing this PR in favor of a clean replacement from the latest main. The implementation and scope changed substantially during review, so the replacement carries a single focused commit and a fresh description. |
6 participants
What this PR does
This PR updates Web Shell compact mode so Ctrl+O hides thinking rows and merges ordinary tool groups separated only by those rows into the standard expandable aggregate view. Agent, todo, question, approval, and message boundaries remain separate.
Active summaries describe every foreground tool until all tools finish. Expanded rows show tool-kind icons, and compact aggregate rows use clearer spacing and active styling.
The transcript retains the first and latest daemon timestamps for thinking and tools. Completed durations use a valid daemon timestamp pair, while legacy or partial records fall back to a consistent client-clock pair.
Why it's needed
Compact mode now presents a quieter transcript while preserving the normal aggregate tool interaction. Related tool activity stays together, concurrent progress remains informative, and elapsed labels remain stable when transcript records are replayed.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: compact mode used condensed output cards, related tools could remain fragmented around thinking rows, and elapsed labels restarted when transcript data was replayed.
After: compact mode hides thinking and keeps related ordinary tools in the standard aggregate UI, while elapsed labels use replay-stable timing.
Tested on
Environment (optional)
Local macOS workspace with Node.js 22. Relevant Web Shell tests passed 320/320, daemon UI reducer tests passed 296/296, and settings/route tests passed 59/59. Web Shell typecheck, ESLint, and git diff validation passed. The separate App test OOM harness fix is tracked in #8934.
Risk & Scope
Linked Issues
Related: #8934
中文说明
本 PR 的改动
本 PR 更新 Web Shell 紧凑模式：按 Ctrl+O 后隐藏思考行，并将仅被这些思考行分隔的普通工具组合并到标准的可展开聚合视图中。Agent、Todo、AskUserQuestion、审批和其他消息边界保持独立。
工具执行期间，摘要会持续展示所有活跃的前台工具，直到全部工具结束。展开后的每个工具行会显示工具类型图标，紧凑聚合行使用更清晰的间距和展开态样式。
Transcript 会保留 daemon 首次和最后一次事件时间戳。完成态使用有效的 daemon 时间对；旧记录或时间不完整的记录会整体回退到客户端时间。
为什么需要
更新后的紧凑模式可以提供更简洁的 transcript，同时保留正常的工具聚合交互。相关工具活动会保持在一起，并行执行进度更加清晰，transcript 重放时耗时也能保持稳定。
Reviewer 测试计划
验证方式
前后对比证据
改动前：紧凑模式使用压缩输出卡片，思考行之间的相关工具可能保持分散，transcript 重放时耗时会重新计时。
改动后：紧凑模式隐藏思考，并使用标准聚合界面展示相关普通工具；耗时使用可回放的稳定时间。
测试平台
环境（可选）
本地 macOS、Node.js 22。Web Shell 相关测试 320/320 通过，daemon UI reducer 测试 296/296 通过，settings/route 测试 59/59 通过；Web Shell typecheck、ESLint 和 git diff 检查通过。独立的 App 测试 OOM 测试桩修复由 #8934 跟踪。
风险与范围
关联 Issue
相关：#8934