fix(web-shell): settle parallel-agents collapse and unify agent detail transcript - #9640
Conversation
…l transcript - Show the subagent prompt as a user bubble inside the detail panel's transcript and let the whole message area scroll as one unit instead of pinning the prompt above a separate fixed-height box. - Collapse a parallel-agents group as soon as its agents are terminal, even while the main agent keeps responding, and bound the awaited-summary hold with a grace window so a missing narration cannot pin the turn open. - Return a stable empty artifacts array from useSessionArtifacts so a session without an artifacts endpoint stops re-running consumer effects every render, which previously cascaded into an infinite update loop. - Brighten a running subagent's summary text on hover alongside its icons.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: these read as observed issues, not theoretical hardening — the side-panel crash cites a concrete failure mode (React "Maximum update depth exceeded" when a session's artifact list cannot load), and the collapse/layout behaviors are specific and reproducible from the description. No linked issue or before/after capture is attached (Evidence says N/A), but the accompanying test updates encode regressions for each behavior, and the author is the web-shell domain maintainer, so this clears the existence bar. Direction: aligned. This is a continuation of the ongoing web-shell subagent UX work (agent detail panel, parallel-agents rendering, side-panel tabs) seen in recent merged PRs in this area. Nothing here touches auth, sandbox, telemetry, or public contracts. Size: not applicable — all changes are in Approach: scope feels right. Four fixes, but all are small, self-contained, and clustered in the same subagent UI surface; each carries its own tests. The PR itself calls out the one user-visible timing change (parallel groups now fold right after completion rather than at turn end) and a minor hover side effect — honest risk framing. No drive-by changes spotted in a skim of the diff structure. Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这些是已观测到的问题，不是理论性加固——侧边面板崩溃引用了具体的失败模式（会话产物列表无法加载时 React "Maximum update depth exceeded"），收起/布局行为也有具体、可按描述复现的场景。PR 未关联 issue、未附 before/after 截图（Evidence 写 N/A），但随附的测试更新为每个行为编码了回归用例，且作者是 web-shell 领域的维护者，通过问题存在性检查。 方向：对齐。这是近期已合并的 web-shell subagent UX 系列工作（agent 详情面板、并行智能体渲染、侧边面板标签页）的延续，不涉及 auth、sandbox、telemetry 或公共契约。 规模：不适用——全部改动位于 方案：范围合理。四项修复都小且自包含，集中在同一 subagent UI 面，各自带有测试。PR 自身也说明了唯一的用户可见时序变化（并行组在全部完成后立即收起而非整轮结束）和轻微的 hover 附带效果——风险描述诚实。diff 结构速览未发现顺手改动。 风险：无升级风险信号——改动文件均未命中与回滚相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI formed my independent proposal before reading the diff (drop the pinned prompt box and render the prompt as the transcript's first bubble; collapse groups once their agents are terminal and bound any narration-wait with a timeout; hoist the empty array to a module constant; one CSS line for hover). The PR lands on exactly those mechanisms in all four cases — no simpler path was missed. What I verified statically: Detail panel unification. Parallel-agents collapse. The deferral formula shrinks from "any latest-turn group while responding" to "only the group owning the awaited notification, and only while a summary narration is genuinely expected." The previously unbounded ordering wait (final answer on screen before the completion notification — the case that pinned groups open forever) now gets the same 5s grace the unmatched-completion latch already uses, keyed to the turn's own agent-notification id so a monitor banner mid-wait neither restarts nor re-arms it. The new memo/effect pair has no feedback loop: the hold-check passes a literal Infinite update loop. Module-level Hover. Tests were rewritten to encode the new semantics rather than just made to pass: the two old "defer while responding" tests were replaced with collapse-despite-responding tests, and new DOM tests pin the chronological repositioning ( Files changed (9 of 9)
Testing — the PR's own CI (this run executes no PR code)At review time the main unit suite (Test ubuntu-latest, ~30 min) and the Web-shell Visuals capture job were still in flight on the reviewed commit; build, desktop-shell, secret-scan, and CVE checks had already landed green. The macOS/Windows test and CLI-integration jobs show "skipped" by design — ci.yml gates them to merge_group, so ubuntu carries the PR suite. The table below reflects the fetch-time state; the finalize job rewrites it once CI settles. Not verified in this run: live-browser layout behavior (single scroll region, hover brightening) — CI runs no browser assertions for it, and the author's evidence is a dev:daemon run on macOS (author's claim, not independently re-run). Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remaining behavioural claim: 中文说明代码审查：先独立构思再看 diff，四项修复都落在最简机制上，没有遗漏更简路径。静态验证结论：
测试是按新语义重写的，不是简单改到通过：两个旧的"响应中延迟"测试被替换为"完成即收起"测试，新增 DOM 测试钉住时序回归（compareDocumentPosition）、4999/5000ms 宽限边界、监控通知不重启规则。未发现阻断问题，无规范违规。 测试证据：本次运行不执行任何 PR 代码，引用的是 PR 自身 CI。审查时主单测套件（ubuntu，约 30 分钟）与 Web-shell Visuals 截图任务仍在进行；构建、desktop-shell、密钥扫描、CVE 审计已通过。macOS/Windows 单测与 CLI 集成任务按 ci.yml 配置仅在 merge_group 触发，属设计内跳过。未验证项：浏览器实际布局效果（单滚动区、hover 变亮）——CI 无相应浏览器断言，作者的证据是 macOS 上的 dev:daemon 实测（作者自述，未独立复核）。上一段已给出可补齐该行为证据的沙箱验证触发方式。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, focused, well-tested; the only thing I can't attest from here is the pixel-level layout behavior in a real browser. Stepping back: this is four small fixes that each land on the minimal mechanism — a deleted helper and CSS block instead of a new layout system, a deferral formula that got strictly simpler, a module-level constant for the identity bug, and one CSS property. My independent proposal for each problem is what the PR did; I didn't find a cheaper path it missed. The crash fix is the clearest win: I traced the actual cascade (an The reservation, such as it is: "the panel scrolls as one unit" and "the summary brightens on hover" are CSS claims no test in this PR (or its CI) mechanically pins. The author verified live on macOS and the repo's Web-shell Visuals job captures this surface on every PR — between those, the residual risk is a visual nit, not a behavioral regression. The timing change (groups fold right after completion rather than at turn end) is intentional and disclosed in the PR's own risk section. If I'm maintaining this in six months, the comments answer the only non-obvious question (why the grace is keyed to the turn's own notification), and the DOM tests replay the whole decision table. That's a thank-the-author diff. Verdict: approve. CI is still running on the reviewed commit (unit suite + web-shell visuals), so approval is deferred until CI lands green on 中文说明整体回顾：四项小修复都落在最简机制上——删除 helper 与 CSS 块而非新增布局系统、严格变简单的延迟判定公式、模块级常量修同一性 bug、一行 CSS。我对每个问题的独立设想与 PR 实现一致，没有找到更便宜的遗漏路径。崩溃修复收益最明确：实际级联路径（以 唯一的保留意见："整体滚动"与"hover 变亮"是 CSS 层面的断言，本 PR 及其 CI 均无机械化断言钉住。作者已在 macOS 实测，且仓库的 Web-shell Visuals 任务会为每个 PR 截图该界面——两者结合，残余风险是视觉小瑕疵而非行为回归。时序变化（组在完成后立即折叠而非整轮结束）是有意的，且 PR 风险一节已如实披露。 六个月后维护这段代码时，注释回答了唯一不自明的问题（宽限为何以本轮自身通知为键），DOM 测试可回放整个决策表。这是值得感谢的 diff。 结论：通过。审查时该提交的 CI 仍在运行（单测套件 + web-shell 截图），因此批准推迟到 CI 在该提交上全绿后由收尾任务代为提交；若有检查变红则不会批准。 — 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 / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
…ent-panel-fixes # Conflicts: # packages/web-shell/client/components/MessageList.tsx
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. CI is pending (Test + Capture web-shell visuals in_progress), so posting a comment now.
Scope: 9 files changed, +402/-166. Reviewed all four fix areas against the implementation and tests.
Checked:
Fix 1 — SubagentDetail unified scroll (SubagentDetail.tsx, .css)
- Removing
hideFirstUserMessageand the<pre className={styles.prompt}\>block is consistent: the first user bubble now shows in the transcript and the fixed-height split region is gone. - Note: before the transcript loads there's a brief empty state (the old
rootTool.args.promptfallback is removed). Intentional per PR description; the integration test confirms nopre[class*="prompt"]remains.
Fix 2 — parallel-agents collapse (MessageList.tsx)
deferAutomaticCollapsesimplified tobackgroundSummaryAgentContext.key === displayItem.key && latestTurnAwaitsAgentSummary— no longer holds all latest-turn groups while responding. Matches new tests.- New
waitForOrderedNarration/orderedSummaryGraceExpiredstate: the threeturnAwaitsBackgroundSummarycall sites are consistent —latestTurnHoldsOrderedSummaryalways passestrue(detects the condition),latestTurnAwaitsAgentSummarypasses!orderedSummaryGraceExpired, and theapplyTurnCollapseoption threads the flag through. - Grace keyed on
latestTurnOrderedNotificationId(agent-completion notification ID, not global latest notification): monitor/shell banners mid-wait don't restart or re-arm the window. Verified by the new test at the end of the suite. backgroundSummaryAgentContextcorrectly stripped of the now-unusedagentNotificationIsLatestBackgroundandlatestBackgroundNotificationInLatestTurnfields, andlatestTurnStartIndexdropped from its deps.
Fix 3 — infinite update loop (useSessionArtifacts.ts)
EMPTY_ARTIFACTSconstant provides a stable reference whenloadedOwnerRef.current !== owner. Returning a fresh[]on every render was the root cause of the loop. New test validates reference stability across rerenders.
Fix 4 — hover (ToolChrome.module.css)
color: inheritadded alongside the existing-webkit-text-fill-color: currentColorrule on[aria-expanded='true']and:focus-visibleselectors. Correct: withoutcolorset, the fill would stay at the pre-hover value even whencurrentColoris referenced.
Cross-check: prior review (qwen-code-ci-bot, DISMISSED) was a plain LGTM with no specific findings; nothing to confirm or refute.
Unreviewed dimension: Test (ubuntu-latest, Node 22.x) and Capture web-shell visuals are still in_progress. The test job covers the changed MessageList, SubagentDetail, and useSessionArtifacts suites — will approve once it lands green.
Reviewed with AI assistance.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Approving per request — no blocking findings (reviewed in prior comment). Note: Test (ubuntu-latest, Node 22.x) is still in_progress; the code review itself found no issues.
Reviewed with AI assistance.
|
@qwen-code /verify |
|
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: 4829 passed · 0 failed · 4829 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：4829 通过 · 0 失败 · 4829 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9640 verification — fix(web-shell): settle parallel-agents collapse and unify agent detail transcriptVerdict: 中文摘要
Scope
A/B load-bearing proofEnvironment: base worktree at
Witnesses: The 9 base reds fail on intended expected-vs-actual values (e.g. Full-package gate on head: 193 files / 4013 tests, all pass ( Reviewer Test Plan walk-through
Mutation matrix (head)
No single-hunk survivor besides M4, so no combination row was needed (no layered guards hiding each other among these hunks). Every guard the PR introduces is load-bearing; M4's guard is correct but pinned only by my mid-wait fixture ( FindingsF1 — Suggestion (non-blocking, completeness): the monitor-non-restart test cannot observe a restarted grace. F2 — Observation: the pre-transcript prompt fallback is gone. Base showed the parent tool call's (truncated) prompt in the pinned Not covered
MethodologyContainer Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| hideSessionTimeline | ||
| hideFirstUserMessage | ||
| firstTurnMetrics={metrics} |
There was a problem hiding this comment.
[Suggestion] Removing hideFirstUserMessage here deletes the prop's only production setter, leaving MessageList's prop declaration (MessageList.tsx:125), its default, the first-user-message filter branch (~line 3448), and its memo deps (3356, 3485) as dead code. MessageList is not exported from the package's public entry (client/index.tsx), so no external embedder can set it — only DOM-test fixtures (MessageList.dom.test.tsx:365, 791) still pass it. The cost is an always-false dead switch plus an unreachable filter branch in a ~5,300-line component, and the DOM test exercises a path no production consumer can reach, so a future reader will infer that first-user-message hiding is a live mode worth preserving or building on. Consider deleting the prop, the filter branch and memo deps, and the DOM-test usage in this PR — or note that a near-term caller will use it.
中文说明
在此处移除 hideFirstUserMessage 后，该 prop 的唯一生产调用方被删除，MessageList 中的 prop 声明（MessageList.tsx:125）、默认值、首条用户消息过滤分支（约 3448 行）及其 memo 依赖（3356、3485）全部成为死代码。MessageList 并未从包的公开入口（client/index.tsx）导出，外部使用方无法设置该 prop——如今只有 DOM 测试 fixture（MessageList.dom.test.tsx:365、791）仍在传入它。代价是一个恒为 false 的死开关和一条不可达的过滤分支留在这个约 5,300 行的组件中，且 DOM 测试仍在覆盖一条生产环境不可达的路径，未来读者会误以为"隐藏首条用户消息"是一个值得保留或可以继续使用的有效模式。建议在本 PR 中删除该 prop、过滤分支、memo 依赖及 DOM 测试用法——或说明近期会有调用方使用它。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| useEffect(() => { | ||
| if (!latestTurnHoldsOrderedSummary) return; | ||
| const timer = setTimeout( | ||
| () => setOrderedSummaryGraceExpired(true), | ||
| UNMATCHED_AGENT_COMPLETION_GRACE_MS, | ||
| ); | ||
| return () => clearTimeout(timer); | ||
| }, [ |
There was a problem hiding this comment.
[Suggestion] The ordered-summary grace (latch state + reset effect + this timer effect) is a near-verbatim second copy of the unmatched-completion grace latch ~40 lines above, and the two copies already encode the isResponding gate differently (the older block guards inline inside its timer effect; this one folds !isResponding into its hold memo). UNMATCHED_AGENT_COMPLETION_GRACE_MS now bounds both waits while its name still claims exclusivity to the unmatched path. Any future tuning — lengthening the window for a slow summary narration, changing re-arm semantics — must then be applied in two places, a one-sided edit silently diverges the two latches, and editing the shared constant silently moves both windows. Consider extracting one small hook local to this file (latch + reset + timer) used by both graces, and renaming the constant to reflect its shared scope (e.g. BACKGROUND_SUMMARY_GRACE_MS).
中文说明
这个 ordered-summary 宽限机制（latch 状态 + 重置 effect + 此定时 effect）是约 40 行上方 unmatched-completion 宽限 latch 的近乎逐字复制，且两份副本对 isResponding 门控的编码方式已经不同（旧的一块在其定时 effect 内联守卫，这一块把 !isResponding 合并进了 hold memo）。UNMATCHED_AGENT_COMPLETION_GRACE_MS 现在同时约束两个等待，但其名称仍声称仅用于 unmatched 路径。未来任何调参——为较慢的总结叙述加长窗口、修改重新武装语义——都必须改两处，单边修改会让两个 latch 悄然分叉，而修改这个共享常量也会悄然同时移动两个窗口。建议抽取一个本文件内的小 hook（latch + 重置 + 定时器）供两个宽限共用，并将常量重命名为反映其共享作用域的名称（如 BACKGROUND_SUMMARY_GRACE_MS）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| * arrives cannot pin the turn (and its parallel-agents group) open forever. | ||
| */ | ||
| waitForOrderedNarration?: boolean; |
There was a problem hiding this comment.
[Suggestion] The new grace latch keys on agentNotificationsOnly=true (via latestTurnBackgroundSummaryState) while applyTurnCollapse evaluates the ordering rule with agentNotificationsOnly=false. A trailing monitor/shell-task banner landing after the final answer therefore keeps the turn-level ordering wait true, but latestTurnHoldsOrderedSummary requires sawAgentCompletion, so the latch never arms, orderedSummaryGraceExpired stays false, and the wait remains unbounded — while this JSDoc promises that a summary that never arrives "cannot pin the turn (and its parallel-agents group) open forever". A probe on this commit shows the split:
same turn, no trailing monitor: toggle=false, group=null -> turn folds normally
trailing monitor after final answer: toggle=true, group=false -> still pinned open at 30s (6x the 5s grace)
The pin itself predates this PR (manual collapse and a model reply to the banner both release it), so the minimal fix is to narrow this doc and the latch comment to matched agent completions; the fuller fix is to key the latch on the same all-notifications view applyTurnCollapse uses — the notification-id key already keeps monitor banners from re-arming it.
中文说明
新的宽限 latch 以 agentNotificationsOnly=true（经由 latestTurnBackgroundSummaryState）为键，而 applyTurnCollapse 以 agentNotificationsOnly=false 评估排序规则。因此，落在最终回答之后的 monitor/shell-task 横幅仍会让轮级排序等待为真，但 latestTurnHoldsOrderedSummary 要求 sawAgentCompletion，latch 永远不会武装，orderedSummaryGraceExpired 保持 false，该等待仍然无界——而这段 JSDoc 却承诺"永远不到达的总结不会把该轮（及其并行智能体组）无限期钉在展开状态"。在本次提交上的探测显示了这一差异：
同一轮、无尾部 monitor： toggle=false, group=null -> 轮正常折叠
最终回答后出现尾部 monitor： toggle=true, group=false -> 30 秒后仍钉在展开状态（5 秒宽限的 6 倍）
该钉住行为在本 PR 之前就存在（手动折叠或对横幅的模型回复都能解除），因此最小修复是收窄此处文档与 latch 注释，说明该界限仅适用于匹配的 agent 完成通知；更完整的修复是让 latch 使用与 applyTurnCollapse 一致的全通知视图——通知 id 键已能防止 monitor 横幅重新武装它。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // A monitor banner lands mid-wait. It is not the awaited agent summary, | ||
| // so it must neither restart the 5s bound nor re-arm an expired one; | ||
| // the group still collapses when the grace window closes. | ||
| rerenderMessages(c, [...settled, monitorNotificationMsg('monitor')], { | ||
| isResponding: false, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] This test cannot discriminate the regression it names: the monitor banner lands in a re-render immediately after the grace arms, with zero simulated time between the two renders, and no monitor arrival after expiry is ever exercised (the comment's "re-arm an expired one" half). A mutation probe on this commit confirms it — flipping the grace state memo's agentNotificationsOnly argument to false (making the grace key on the latest background notification of any kind) passes this test, the full file (128/128), and the whole web-shell suite, because both renders happen at simulated t=0 so the mutant's re-armed timer and the correct timer expire at the same instant. If that keying regresses later, repeated monitor banners arriving <5s apart restart the bound indefinitely and pin the turn open forever — the exact failure mode this PR fixes — while this test stays green. Advance ~3s of simulated time before the banner lands (and/or move a monitor arrival after expiry and assert the summary row does not reappear); verified that with the 3s advance the mutant fails (at t=7000 the group summary is still in the DOM where the test expects null) while the correct code passes:
act(() => vi.advanceTimersByTime(3_000));
rerenderMessages(c, [...settled, monitorNotificationMsg('monitor')], {
isResponding: false,
});中文说明
该测试无法判别它命名的回归：monitor 横幅在宽限武装后紧接着的一次重渲染中到达，两次渲染之间模拟时间为零，且从未测试过到期之后才到达的情况（即注释中"re-arm an expired one"的那一半）。在本次提交上做的变异探测证实了这一点——把宽限状态 memo 的 agentNotificationsOnly 参数翻转为 false（使宽限以任意类型的最新后台通知为键）后，本测试、整个文件（128/128）乃至整个 web-shell 套件全部通过，因为两次渲染都发生在模拟时间 t=0，变异代码重新武装的定时器与正确代码的定时器在同一时刻到期。如果该键控将来回归，间隔小于 5 秒反复到达的 monitor 横幅会无限重启该界限，把该轮永远钉在展开状态——正是本 PR 修复的失效模式——而本测试仍然是绿的。建议在横幅到达前先推进约 3 秒模拟时间（和/或把横幅到达移到到期之后，断言摘要行不再重新出现）；已验证：先推进 3 秒后，变异代码在 t=7000 时组摘要仍在 DOM 中（测试期望为 null）而失败，正确代码通过：
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
Released in v0.22.0. |
4 participants
What this PR does
Four web-shell UI fixes around subagents:
Why it's needed
The detail panel layout made the user message and the agent's activity feel disconnected from each other. Parallel agents stayed expanded and pinned at the bottom of the turn after completing, distorting transcript order while the main agent kept working, and one edge case (a completion notification landing after the final answer with no follow-up narration) kept the group expanded indefinitely. The update loop froze turn-collapse toggles across the whole chat once the side panel was open, and the running-agent row gave no hover feedback on its content.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A (UI changes; behavior verified via unit/DOM tests and live dev:daemon run, no screenshots captured).
Tested on
Environment (optional)
npm run dev:daemonin the worktree; relevant test suites pass: MessageList 263, parallel-agents/tool 124, useSessionArtifacts 10, App 490; typecheck, eslint and prettier clean on changed files.Risk & Scope
color: inherithover rule also brightens the running-agent summary text in the expanded state, a minor visual side effect.Linked Issues
N/A
中文说明
这个 PR 做了什么
四项 web-shell 关于 subagent 的 UI 修复：
为什么需要
详情面板布局让用户消息与 agent 活动显得割裂。并行智能体完成后仍保持展开并钉在本轮底部，扭曲了时间顺序，且存在一个边界情况（完成通知落在最终回答之后且无后续叙述）会让组无限期保持展开。无限循环在侧边面板打开后冻结了整段对话的折叠切换。运行中的 agent 行内容没有任何 hover 反馈。
Reviewer 验证计划
风险与范围
color: inherit的 hover 规则也会让展开态的运行中摘要文字变亮，属于轻微的视觉附带变化。