fix(web-shell): make tool summaries consistently collapsible - #10231
Merged
Conversation
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed bug, not theoretical. The scenario is described concretely (opening a compact summary while a shell tool is still running reveals an empty command card that cannot be closed), and I confirmed the mechanism exists on the base commit: Direction: aligned. This is Web Shell presentation behavior for the compact tool summaries, squarely inside the package's purpose. Claude Code's CHANGELOG has no entry for this exact issue, but it shows sustained attention to the same class of bug (e.g. "Fixed Focus view sections you expanded collapsing on their own during subagent tool activity"), so the area is clearly relevant. No auth/sandbox/contract surfaces touched. Size: not applicable — no core paths ( Approach: the scope feels right — three presentation rules (child rows stay collapsed until opened, every regular row toggleable, MCP Apps rendered standalone) share the same expand/collapse machinery, and fixing one without the others would leave the summary inconsistent. One honest question rather than a blocker: the file-summary count change (unique-file wording → operation counts) is a wording/semantics decision bundled in here. It's documented in the PR body and tested, so it's fine to carry, but it's the one part I'd expect a maintainer to consciously sign off on since it changes what a visible number means. Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，不是理论性问题。场景描述具体（在 shell 工具仍在运行时打开紧凑摘要，会出现一个无法关闭的空命令卡片），且我在 base 提交上确认了机制确实存在： 方向：对齐。这是 Web Shell 紧凑工具摘要的展示行为修复，完全在该包的职责范围内。Claude Code 的 CHANGELOG 没有针对此问题的条目，但同类问题一直受到关注（例如 "Fixed Focus view sections you expanded collapsing on their own during subagent tool activity"），说明该领域确实相关。未触及 auth/沙箱/对外契约等敏感面。 规模：不适用——未触及核心路径（ 方案：范围合理——三条展示规则（子行保持收起直到被打开、每个常规行都可切换、MCP App 独立渲染）共用同一套展开/收起机制，只修其中一个会让摘要行为仍然不一致。一个真诚的疑问（非阻塞）：文件摘要计数的改动（不重复文件数措辞 → 操作次数）是一个措辞/语义决策，被打包在本 PR 中。PR 正文已说明且有测试覆盖，携带没问题，但这是唯一建议 maintainer 有意识地确认的部分，因为它改变了可见数字的含义。 风险：无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewThe approach matches what I'd have proposed, and it's simpler than the alternative: instead of reworking the content-based expandability gate, the PR removes it entirely — Things I checked specifically:
No correctness blockers found. No AGENTS.md violations (tests colocated, comments updated where behavior changed, no unrelated churn). Test evidenceUnattended CI run — this review quotes the PR's own CI checks via the API; no PR code was built or executed here. At review time the Linux unit suite and the web-shell visual capture are still running; the table below is updated automatically once CI settles. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The new DOM/unit tests directly pin the component-level behavior (running shell stays collapsed and is toggleable, empty shell stays expandable after completion, contentless rows toggle, MCP App rows split into standalone segments, operation-count wording). Not verified: the author tested on macOS only, and nothing in the diff asserts that the same iframe instance survives a collapse/reopen cycle — that claim currently rests on the existing 中文说明代码审查实现与我独立提出的方案一致，且更简洁：PR 没有修补基于内容的可展开门控，而是直接将其移除—— 重点核对过：
未发现正确性阻塞项。未发现 AGENTS.md 违规（测试与源码同目录、行为变化处注释同步更新、无无关改动）。 测试证据无人值守 CI 运行——本节通过 API 引用 PR 自身的 CI 结果，未在此构建或运行任何 PR 代码。审查时 Linux 单测套件与 web-shell 视觉截图仍在运行，下表会在 CI 结束后自动更新。 新增的 DOM/单元测试直接钉住了组件级行为（运行中 shell 保持收起且可切换、空 shell 完成后仍可展开、无内容行可切换、MCP App 拆为独立行、操作次数措辞）。未验证：作者仅在 macOS 上测试，且 diff 中没有任何断言验证收起/重新打开后仍是同一个 iframe 实例——该声明目前依赖既有的 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — solid, minimal fix for a real observed bug; the one reservation is the untested iframe-retention claim, which is named in the review above with the lane that would settle it. Stepping back: the problem is real — I confirmed on the base commit that the content-based expandability gate made running tool rows non-toggleable and let the summary open an empty card that couldn't be closed, and the MCP App force-open effect did exactly what the PR describes. The fix is the minimal version of itself: it removes the gate rather than patching it, suppresses auto-expansion inside compact summaries, and splits MCP Apps into standalone rows using a small, memo-friendly helper. ToolGroup.tsx ends up 52 lines lighter. Every hunk serves a documented goal; the deleted tests cover scenarios that no longer exist, and the added ones pin the new behavior (toggle-while-running, toggle-after-completion respecting the user's choice, contentless rows, standalone app rows, operation-count wording). The one thing I'd have done differently is nothing material — the locate fallback trade-off (callId-first, no message-id fallback) is deliberate and correct for the split layout. So this is approving on merit, not for lack of reasons to say no. Approval is deferred until CI lands green on 中文说明置信度：4/5 —— 对一个真实观测到的 bug 给出了扎实、最小化的修复；唯一的保留意见是 iframe 状态保留这一声明尚无测试钉住，上文审查中已点名并给出了可补齐该验证的沙箱通道。 退一步看：问题是真实的——我在 base 提交上确认，基于内容的可展开门控使运行中的工具行不可切换，且摘要会打开一个无法关闭的空卡片；MCP App 的强制展开 effect 也确如 PR 所述。修复本身就是最小版本：直接移除门控而不是打补丁，在紧凑摘要内禁用自动展开，并用一个小巧、对 memo 友好的辅助函数把 MCP App 拆成独立行。ToolGroup.tsx 最终减少了 52 行。每个改动块都服务于已记录的目标；被删除的测试对应已不存在的场景，新增测试则钉住了新行为（运行中可切换、完成后切换尊重用户选择、无内容行、独立 App 行、操作次数措辞）。定位回退的取舍（callId 优先、不回退到消息 id）是有意为之，对拆分后的布局是正确的。 因此这是基于价值的认可，而不是"找不出反对理由"。批准将推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🖼️ 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 |
Contributor
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. |
Collaborator
tmux E2E test report (head
|
qwen-code-ci-bot
approved these changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qqqys
approved these changes
Aug 27, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Approving. Independent review at this head (db64d6669b — the PR's only commit, also the head my tmux verification report covered) found no blocking (Critical) issues. The earlier red lane was the concurrent EADDRINUSE flake in the serve suite (identical timing flakes hit unrelated PRs the same window); the re-run is green here: Test ubuntu 22m, Desktop Shell (both), web-shell E2E smoke. The bot has approved at this head; concur.
chiga0
approved these changes
Aug 27, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Scope: packages/web-shell/client/ — all 6 changed files reviewed. No platform-specific code; Windows/Linux paths not in scope.
CI disclosures:
Test (ubuntu-latest)✅ ·web-shell E2E Smoke✅ ·Desktop Shell (ubuntu/windows)✅Test (macos-latest)⚠️ SKIPPED ·Test (windows-latest)⚠️ SKIPPED — no platform-specific code in this diff.Integration Tests (CLI, No Sandbox)⚠️ SKIPPED — web-shell-only change, appropriate.review-prin_progress at time of review.
What I checked:
splitMcpAppToolGroups — split logic and id scheme:
- Correctly identifies multi-tool groups containing an MCP App and splits them at each MCP App boundary; single-tool groups pass through unchanged.
- First segment keeps the original
message.id; subsequent segments use${message.id}-${tools[0].callId}. No collision risk with daemon message ids (sequential integers vs. string callIds). - Returns
messagesunchanged (identity) when no split occurs — avoids unnecessary re-renders. ✓ mergeCompactToolGroupshas a defense-in-depth guard (!m.tools.some(getMcpAppDisplay)) that prevents any residual MCP App group from being compacted even if splitting somehow missed it. ✓
findDisplayItemIndex / displayItemMatchesLocateTarget — callId-first lookup:
- Old logic: messageId match first, then callId match. New logic: when callId provided, search by callId only; messageId bypassed entirely.
- For MCP Apps after split: calling
scrollToMessage('mixed', 'app')under the old code would have found theread,editsegment (first messageId match); new code correctly finds the standaloneapprow via callId. The DOM test exercises exactly this and passes. ✓ - For non-split tool groups in compact mode (merged under
summary-...id): callId match still finds the merged group. Behavior unchanged from caller's perspective. ✓ - When callId is provided for a non-tool message: returns -1. Correct; callId is only meaningful for tool calls, and no caller passes callId for plain messages.
ToolLine expandability — hasExpandableContent removal:
expandable = !isForcedExpandedmakes every regular tool row toggleable regardless of content. ✓shouldAutoExpandnow gated by!summaryOnly: child tool rows inside a compact summary no longer auto-expand when the summary is opened. This addresses the stated bug (empty running-tool card that couldn't be closed). ✓shouldAutoExpandstill applies for standalone non-summaryOnly rows (MCP App in main transcript auto-expands). ✓forceExpandableprop removal: TypeScript build is green, so no other callers remain.hasExpandableContent,hasDiffContent,hasEditContent,getWriteContentdeletions: all were used only withinhasExpandableContent. Build green confirms no external callers. ✓
showExpandedSummaryPanel condition:
- Old:
!isTodo && expanded && !detailView && (showDescriptionInDetail || result || failed) - New:
expanded && !detailView && (!isTodo || (!hasTodoList && !result)) - Empty todos (no list, no result) now show the summary panel when expanded — consistent with "every tool can be opened". Todos with content still route to
detailView. ✓
i18n changes:
- "Edited N files" → "Edited files N times" (EN/ZH). The unit test verifies the ZH variant; the EN variant follows the same template and is structurally identical. ✓
Pipeline ordering:
- Non-compact path:
splitMcpAppToolGroups(messages)applied first, then served asvalue. - Compact path:
splitMcpAppToolGroups→mergeCompactToolGroups. MCP App standalone groups are excluded from merging by theisMergedToolGroupguard. ✓ - Streaming thinking-tail fast path in non-compact mode:
splitMcpAppToolGroups(messages)called fresh — no stale cached split omitted. ✓
Test validity (new tests):
keeps MCP Apps standalone: exercises split + locate + flashing, including the negative assertion that the adjacentread,editgroup is not flashed. Meaningful. ✓keeps a running shell collapsed and lets the user toggle it: exercises the core bug fix — open a summary containing a running tool, verify it stays collapsed, then toggle it. ✓keeps an empty shell expandable after it completes: re-render from in_progress to completed while expanded; state is preserved; can collapse and re-expand. ✓lets a contentless tool expand and collapse(glob, todo_write): confirmsexpandable = !isForcedExpandedunconditionally. ✓expands an empty monitor inline when details are unavailable: updated to reflect new behavior (was "stays static", now "falls back to inline expansion"). ✓
One minor gap (not a blocker):
Split-segment ids (${message.id}-${callId}) are not handled by the turnIdOfMessageRow anchor resolver from #10086 (which resolves summary-* ids via summaryRunFirstMemberId). If a pagination anchor lands on a split MCP App segment while its turn collapses, the anchor falls back to dropping position instead of re-expanding. Trigger requires: MCP App in a large paginating turn + pagination mid-read. Low frequency; no data loss.
No blocking findings.
Reviewed with AI assistance.
This was referenced Aug 28, 2026
Collaborator
|
Released in v0.22.3. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Makes compact tool summaries predictable: opening a summary no longer automatically opens its running child tools, every regular tool row can be expanded or collapsed regardless of whether output is available, and file-related summary counts describe operations rather than unique files. MCP Apps are separated from neighboring tool aggregation, open by default, remain collapsible, and preserve their mounted iframe state while collapsed.
Why it's needed
While tools were still running, opening the parent summary could reveal an empty command card that could not be closed. The same content-based expandability rule affected other tool kinds. File summaries could also imply unique-file counts even when the same file was touched repeatedly, and MCP Apps lost their intended standalone presentation when mixed with ordinary tools.
Reviewer Test Plan
How to verify
Open a compact summary containing a running shell tool and another tool. Confirm the child rows remain collapsed, then expand and collapse each row even when it has no output. Complete the shell tool and confirm the user's chosen state is retained. Run repeated edit/read/search operations and confirm the summary uses operation counts. Place an MCP App between ordinary tools and confirm it renders as a standalone row, starts open, can be collapsed and reopened, and keeps the same iframe instance.
Evidence (Before & After)
Tested on
Environment (optional)
Local workspace build with targeted Vitest DOM/unit coverage, ESLint, and TypeScript type checking.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
让紧凑工具摘要的行为更可预期：打开摘要时不再自动展开正在运行的子工具；所有常规工具行无论是否已有输出都可展开和收起；文件类摘要统计改为操作次数，不再暗示不重复文件数。MCP App 不再与相邻常规工具聚合，默认展开、可收起，且收起时保留已挂载的 iframe 状态。
为什么需要
工具仍在运行时，打开父级摘要可能出现空的命令卡片，并且无法收起。其他工具类型也受到同一套基于内容的可展开规则影响。文件摘要还可能被理解为不重复文件数，即使同一文件被重复操作；MCP App 与常规工具混合时也会丢失原本的单独展示方式。
审阅者测试计划
如何验证
打开一个同时包含运行中 Shell 工具和其他工具的紧凑摘要，确认子工具仍保持收起；随后在没有输出时也能展开和收起每个工具行。让 Shell 工具完成，确认用户选择的状态被保留。重复执行编辑、读取和搜索操作，确认摘要使用操作次数。将 MCP App 放在常规工具之间，确认它以独立行展示、默认打开、可收起并重新打开，且始终使用同一个 iframe 实例。
证据（修复前后）
测试平台
环境（可选）
本地工作区构建，并执行了定向 Vitest DOM/单元测试、ESLint 和 TypeScript 类型检查。
风险与范围
关联 Issue
无。