fix(web-shell): reduce streaming thought render jank - #9914
Conversation
E2E test reportScenario
Result
Additional verification
An intermittent development-mode frame spike around the terminal turn-completion transition remains outside this PR's scope. |
176a876 to
6e4e15c
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Thanks for the PR! One note up front: the head moved during this triage run — the branch was rebased onto current Template looks good ✓ — all required sections present, bilingual body complete, and a design doc committed under Problem: observed, with evidence. The body carries before/after numbers from a deterministic 5,000-char / 400-chunk folded-thinking scenario (frame-time P95 17 ms → 9 ms, ~21% faster sustained streaming, ~34% lower typing overhead) plus a detailed E2E report in the thread. Those are author-reported measurements from a macOS dev server, not re-run here — but the underlying problem (top-level Direction: aligned. Streaming-time render responsiveness is core Web Shell surface, and the reference product ships comparable fixes in this exact area (duplicated thinking text while streaming, collapsed thinking summaries staying readable). No scope concerns. Size: not applicable to the core-module gate — everything lives in Approach: the shape is right — gate the top-level subscription to structural changes using the store's change summary, move live-tail projection to a memoized boundary at the message list, and actually unmount collapsed detail subtrees instead of CSS-hiding them. Every edit serves the stated goal; no drive-by changes. The insight-marker path now does a full tail projection with stable-prefix identity reuse instead of bailing out, which keeps streaming correct when protocol JSON spans chunks. One implementation-level concern follows into code review. Risk: no elevated-risk paths matched the revert-history signal. Moving on to code review. 🔍 中文说明感谢贡献！先说明一点：本次 triage 运行期间 head 发生了移动 —— 分支被 rebase 到最新 模板完整 ✓ —— 所有必需小节齐全，中英双语完整，并在 问题：已观测、有证据。PR 正文提供了确定性场景（5,000 字符 / 400 chunk 折叠思考）的 before/after 数据（帧耗时 P95 17 ms → 9 ms、持续流式约快 21%、输入开销约降 34%），并在讨论串中附了详细 E2E 报告。这些是作者在 macOS 开发服务器上的自报数据，未在此重新执行 —— 但问题本身（顶层 方向：对齐。流式期间的渲染响应性是 Web Shell 的核心产品面，参考产品在完全相同的领域也有同类修复（流式期间思考文本重复、收起的思考摘要保持可读）。无范围问题。 规模：不适用核心模块门槛 —— 改动全部位于 方案：方向正确 —— 用 store 的 change summary 把顶层订阅限制为结构变化，把实时尾部投影移到消息列表的 memo 边界，收起的详情子树真正卸载而不是用 CSS 隐藏。每处改动都服务于既定目标，无顺手改动。insight 标记路径从直接退出改为完整尾部投影并复用稳定前缀对象标识，使协议 JSON 跨 chunk 时流式仍正确。一个实现层面的问题带入代码审查。 风险：未命中 revert 历史高风险路径。 进入代码审查。🔍 — 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 |
Code reviewIndependent baseline first: for "streamed thinking text wakes the whole app," I would (1) gate the app-level transcript subscription to structural changes using the store's change summary, (2) move live-tail consumption into a memoized boundary at the message list, and (3) unmount collapsed detail subtrees instead of CSS-hiding them. That is exactly what this PR does, and the execution is careful: The new render flow, for orientation: sequenceDiagram
participant P1 as Transcript store
participant P2 as structural snapshot hook
participant P3 as App baseline
participant P4 as LiveMessageList
participant P5 as MessageList
P1->>P2: streamed text append
P2--xP3: ignored while structure unchanged
P1->>P4: live throttled snapshot
P4->>P4: projectStreamingTailMessages onto baseline
P4->>P5: display messages
P1->>P2: structural change
P2->>P3: wake and refresh baseline
P3->>P4: new baseline props
One finding I'd want fixed before merge: The Non-blocking notes:
What's good: the unit tests pin the actual semantics rather than the plumbing — structural consumers stay asleep for pure tail appends and wake on the next structural change, collapsed groups render no detail subtree and restore current details on expand, and insight markers that span chunk boundaries project correctly with stable-prefix identity reuse (the renamed "projects…" tests replace the old "falls back…" expectations). The committed design doc under Test evidence (the PR's own CI, read via API — no PR code executed here)As of this pass, on the reviewed commit: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The unit suite had not finished on this commit at review time, so there are no green test results for the changed files yet — the finalize job updates the table above once CI settles. Security checks are green. Not verified here: the performance numbers (author-reported, macOS dev server only) and live Web Shell behavior — this run does not execute PR code. Sandboxed verification would settle the remaining claim: 中文说明代码审查 —— 先说独立基线：针对"流式思考文本唤醒整个应用"，我的做法是（1）用 store 的 change summary 把应用层的 transcript 订阅限制为结构变化，（2）把实时尾部消费移到消息列表处的 memo 边界，（3）收起的详情子树真正卸载而不是用 CSS 隐藏。这正是本 PR 的做法，且实现细致： 希望合并前修复一点： 非阻塞备注：收起与展开现在都是瞬时的 —— 移除 优点：单元测试钉住的是真实语义而非管线细节； 测试证据 —— 本运行未执行任何 PR 代码，以上为通过 API 读取的 PR 自身 CI：审查时该提交上的单元测试尚未完成，变更文件暂无绿色结果，表格由 finalize 任务在 CI 结束后更新；安全相关检查已通过。未在此验证：性能数字（作者自报、仅 macOS 开发服务器）与 Web Shell 实际运行行为。 沙箱验证可补齐剩余论断： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the design is right and the tests pin the mechanism, but the new Stepping back: this is a well-formed PR. The problem is real and was measured, not hypothesized; the approach matches what I'd propose independently; the committed design doc, the semantics-level test updates, and the clean mid-flight rebase onto current What keeps me at 2/5 is that the single finding sits exactly in this PR's area of responsibility — snapshot/render correctness — and contradicts the PR's own compatibility claim ("Stores without a change summary retain the existing behavior"). The failure mode is an infinite re-render loop, not a cosmetic bug, and the fix is small: fall back to the non-structural comparison when either summary is CI on the reviewed commit was still running at this pass (unit suite + web-shell visuals), so approval was not on the table in this run regardless. Once the fix above lands and CI is green on the new commit, this is on a clear path to approval — everything else in the review is non-blocking (lost expand animation, two-writer Requesting changes on that one item. 🙏 中文说明置信度：2/5 —— 设计方向正确、测试也钉住了机制，但新的 整体看：这是一个形态良好的 PR。问题真实且有测量数据支撑，不是假设；方案与我独立会提出的做法一致；提交的设计文档、语义层面的测试更新、以及运行中途干净地 rebase 到最新 停在 2/5 的原因是：唯一的问题恰恰落在本 PR 的职责范围内 —— 快照/渲染正确性 —— 并且与 PR 自己的兼容性声明（"没有 change summary 的 store 保留原有行为"）相矛盾。失败模式是无限重渲染循环而非外观问题，而修复很小：任一 summary 为 审查时该提交上的 CI 仍在运行（单元测试 + web-shell 视觉），因此本次运行本来也不到批准的时机。上述修复落地、新提交 CI 变绿后，这个 PR 就走在明确的批准路径上 —— 审查中的其余问题都不阻塞（展开动画丢失、 就这一项请求修改。🙏 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@ytahdn One fix needed before approval: the structuralOnly getSnapshot path returns a fresh object on every call for stores without getBlockChangeSummary (optional in the SDK interface) — that breaks the useSyncExternalStore caching invariant and drives an infinite re-render loop. Full detail and the suggested fix are in my review comment above. Everything else looks solid. 🙏
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. |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
静态审查（head ee3e0769，未运行任何测试/构建）
总体结论：设计成立、实现干净，两条机械（结构性快照 + 边界投影 + 折叠卸载）都站得住脚。未发现阻塞级问题。 重点：自动审查（bot）给出的那条 Critical 经逐行核对不可复现，详见下；我另外找到两条边界条件下的 Important，值得处理但不阻塞。
Overall: The design is sound and the implementation is clean; all three mechanisms (structural snapshot, boundary projection, collapse-unmount) hold up. No blocking issue found. Notably, the automated Critical does not reproduce on careful line-by-line verification (see below). I did find two Important edge cases worth fixing, but neither blocks.
🛑 关于 bot 的 Critical：structuralOnly getSnapshot 死循环 —— 不可复现 / Does not reproduce
Bot 声称：对缺少 getBlockChangeSummary（SDK 接口里是可选项）的 store，structuralOnly 的 getSnapshot 每次调用都返回新对象，破坏 useSyncExternalStore 缓存不变量，导致无限重渲染。
The bot claims that for a store lacking getBlockChangeSummary (optional in the SDK interface), the structuralOnly getSnapshot returns a fresh object every call, breaking the useSyncExternalStore caching invariant and driving an infinite re-render loop.
核对 head tree 后的结论：真实 store 永远提供该方法且恒返回已定义 summary，此场景不可达。
After checking the head tree: the real store always provides the method and always returns a defined summary, so this scenario is unreachable.
- 生产里唯一的
structuralOnly: true调用点是App.tsx:2290，其 store 来自useTranscriptStore()→DaemonStoreContext，而该 context 只由createDaemonTranscriptStore构造（DaemonSessionProvider.tsx:781/783）。 createDaemonTranscriptStore恒实现getBlockChangeSummary（store.ts:66），且 summary 在构造时即初始化为已定义对象（store.ts:36-40），其后只会被替换为新的已定义对象，从不为undefined。- 因此
isSameTranscriptStructure(cached?.blockChangeSummary, blockChangeSummary)中next === undefined永不为真；previous === undefined仅出现在缓存尚未建立时，已被!cached分支兜住重建。尾部追加时source与tailAppendBarrierRevision均不变（store.ts:167-173），blockIndexById引用亦保持（store.ts:209-210把它列为尾部追加前置条件）→changed === false→ 复用同一cached引用，useSyncExternalStore不会重渲染。新增的keeps structural consumers asleep for pure tail appends测试也锁住了这一点。
The only production structuralOnly: true call site is App.tsx:2290, whose store comes from useTranscriptStore() → DaemonStoreContext, built exclusively by createDaemonTranscriptStore (DaemonSessionProvider.tsx:781/783). That factory always implements getBlockChangeSummary (store.ts:66) and initializes the summary to a defined object at construction (store.ts:36-40); it is only ever replaced by further defined objects, never undefined. So next === undefined in isSameTranscriptStructure is never true, and previous === undefined only occurs before the cache exists, which the !cached branch already rebuilds. On tail appends both source and tailAppendBarrierRevision stay constant (store.ts:167-173) and blockIndexById keeps identity (store.ts:209-210 lists it as a tail-append precondition), so changed === false and the same cached reference is reused — no re-render. The new keeps structural consumers asleep for pure tail appends test pins this.
💡（可选加固，非阻塞）若想让接口层的"可选"不再是隐患，可在 getBlockChangeSummary 缺失时退化为逐块比较而不是恒 changed：(optional) hardening — if getBlockChangeSummary is absent, fall back to a block-by-block comparison instead of treating every call as changed. 仅为防御未来出现的非标准 store，当前无实际风险。
🎉 Praise
- barrier-revision 证明免去 O(transcript) 扫描：同
source+ barrier 相等 + revision 严格递增即可证明中间全为纯尾部追加，跳过前缀逐块比较。useMessages.test.ts用 Proxy 计数索引读取（indexedReads < 10）直接锁住"100 块历史不被扫描"，useAnimationFrameTranscriptBlocks.test.tsx用 revision↔text 配对锁住 blocks 与 summary 不撕裂。这是本 PR 的核心价值，测试也锁得很扎实。
The barrier-revision proof (same source + equal barrier + strictly increasing revision ⇒ all intermediate revisions are pure tail appends) skips the O(transcript) prefix scan, and the Proxy-based index-read counter plus revision↔text pairing tests lock the invariant firmly. - 折叠不吞进行中交互：
hasApprovalTool组走非摘要分支，含 pendingApproval 的组不参与 compact 合并（MessageList.tsxisForceExpandGroup），交互式 ask-user 表单在 App 层渲染；子树内无<input>/<textarea>，不存在"未提交输入被折叠丢弃"路径。
Collapsing cannot swallow in-flight interactions (approval tools bypass the summary branch; pending-approval groups are excluded from compact merging; interactive ask-user forms render at App level; no inputs live in the unmounted subtree).
🟡 Important（边界条件，建议修复，不阻塞）
[1] insight 分支在"尾部块无 id 可匹配的既有消息"时返回未 reconcile 的全量投影，后台 agent 卡片会闪回 pending
[1] The insight branch returns an unreconciled full projection when the tail block has no id-matchable prior message, flashing a completed background-agent card back to pending
位置 / Location: packages/web-shell/client/hooks/useMessages.ts:209-228（配合 :421、:435-443）
firstTailMessageIndex 用 message.id === before.id || startsWith(before.id + '-') 定位尾部块的第一条投影消息。但存在两类真实情形使尾部块在 previous.messages 中无可匹配 id，findIndex 返回 -1，stablePrefixLength 静默退化为 0：(a) 合并尾部——相邻 assistant/thought 块合并进保留首块 id 的前一条消息（transcriptToMessages.ts:598-617/661-678）；(b) 空文本尾部——空 streaming 块不产生消息（transcriptToMessages.ts:525），新建块首次 delta 前即此形态。
firstTailMessageIndex locates the tail block's first projected message via message.id === before.id || startsWith(before.id + '-'). In two real shapes the tail block has no id-matchable message and findIndex returns -1, silently degrading stablePrefixLength to 0: (a) merged tail — adjacent assistant/thought blocks merged into a prior message that keeps the first block's id; (b) empty-text tail — an empty streaming block produces no message, which is the shape right after a freshly created block.
此时 insight 分支返回全新全量投影（无一条复用 previous），而 useMessagesFromBlocks 因 reusedStreamingTail === true（:421）且 resolutions 未变直接 return messages（:435-443），跳过 reconciliation → 已 reconcile 为 completed 的后台 agent 卡片闪回 pending（pendingBackgroundAgentKey 重新非空，触发一次多余探针后自愈；在边界组件侧因标记只增不删，整个 insight 流式期间持续显示未 reconcile 投影，直到下一个结构性 revision）。
In that case the insight branch returns a fresh full projection (no prior reference reused), while useMessagesFromBlocks sees reusedStreamingTail === true (:421) with unchanged resolutions and returns messages directly (:435-443), skipping reconciliation — a background-agent card already reconciled to completed flashes back to pending (and on the boundary side, since the marker only accumulates, the unreconciled projection persists for the whole insight stream until the next structural revision).
建议修复 / Suggested fix（退化为调用方回退，两条调用链均自愈 / degrade to the caller fallback so both call chains self-heal）:
if (after.text.includes(INSIGHT_CONTENT_MARKER)) {
const tailPrefix = `${before.id}-`;
const firstTailMessageIndex = previous.messages.findIndex(
(message) =>
message.id === before.id || message.id.startsWith(tailPrefix),
);
// No id-matchable tail message (merged into an earlier message, or still
// empty at the baseline): a full projection would drop reconciled prefix
// messages while reusedStreamingTail skips re-reconciliation.
if (firstTailMessageIndex < 0) return undefined;
const messages = transcriptBlocksToLocalizedMessages(blocks, t);
for (let i = 0; i < firstTailMessageIndex; i += 1) { /* 原复用循环不变 / unchanged */ }
return messages;
}返回 undefined 后：hook 走 reuseUnchangedProjectedPrefix（marker bail → 全新消息且 reusedStreamingTail=false → reconciliation 正常应用）；边界组件回退已 reconcile 的 baselineMessages。现有新增测试只覆盖 findIndex 成功路径，建议补一条合并/空文本尾部 + resolutions 的用例。
After returning undefined, the hook path re-applies reconciliation and the boundary falls back to the reconciled baselineMessages. The new tests only cover the successful findIndex path; a merged/empty-tail + resolutions case would be worth adding.
[2] 折叠会卸载并重置 MCP App iframe，丢失应用内交互状态（超出设计声明范围）
[2] Collapsing unmounts and resets the MCP App iframe, discarding in-app interaction state (outside the documented scope)
位置 / Location: packages/web-shell/client/components/messages/ToolGroup.tsx:1814（{chatExpanded && ...}）、:1466（McpApp）、McpApp.tsx:246（sandbox="allow-scripts allow-forms" iframe）
详情子树现在条件挂载，而子树内可渲染 McpApp——带 AppBridge 的交互式沙箱 iframe。旧 CSS grid-template-rows: 0fr 折叠路径子树保持挂载、iframe 存活；新实现折叠即卸载 iframe。设计文档 Compatibility 一节只声明不再保留"hidden detail rows 内的本地展开状态"，iframe 内的表单/应用状态不属于"展开状态"，不在声明范围。
The detail subtree is now conditionally mounted, but it can render McpApp — an interactive AppBridge sandbox iframe. The old CSS grid-template-rows: 0fr path kept the subtree mounted so the iframe survived; the new implementation unmounts it on collapse. The design doc's Compatibility section only declares that local expanded state inside hidden detail rows is no longer preserved; in-iframe form/app state is not "expanded state" and is outside that declaration.
失败场景 / Failure scenario: 工具产出 MCP App（交互式 dashboard/查询表单）→ 组自动展开 → 用户在 iframe 应用内填写 → 用户折叠该组以查看上下文 → 再展开时 iframe 重新加载、AppBridge 重新握手，应用回到初始态，已输入数据静默丢失。ToolGroup.test.tsx:501-504 证明 MCP 组默认自动展开，但不阻止用户手动折叠。
A tool yields an MCP App (interactive dashboard/query form), the group auto-expands, the user fills in the in-app form, then collapses the group to read context; on re-expand the iframe reloads, AppBridge re-handshakes, the app resets to its initial state, and the entered data is silently lost.
建议（二选一）/ Suggested (either):
- 对含 MCP App 的组保留"挂载但隐藏"：
const keepMounted = hasMcpApp;条件改为{(chatExpanded || keepMounted) && ...}，折叠时加style={{ display: 'none' }}——iframe 文档存活且不参与布局/渲染，仍达成大部分性能目标。
Keep "mounted but hidden" for groups containing an MCP app (keepMounted = hasMcpApp, adddisplay: 'none'when collapsed) so the iframe document survives while still skipping layout/paint. - 若接受卸载，至少在文档 Compatibility 中补充"折叠含 MCP app 的组会重载其沙箱并丢弃应用内状态"。
If unmounting is accepted, at minimum document in Compatibility that collapsing a group with an MCP app reloads its sandbox and discards in-app state.
🟢 Nit
[3] messagesRef 双写者，父布局副作用可能用结构性基线覆盖子的实时投影
[3] messagesRef has two writers; the parent layout effect can overwrite the child's live projection with the structural baseline
位置 / Location: App.tsx:2721（父写）与 App.tsx:921（子写）。React 布局副作用子先父后，故同一次结构性 commit 中 LiveMessageList 先写投影值、App 随后以结构性基线覆盖。两个读取点均为事件处理器（App.tsx:6904 recap 锚点、App.tsx:9690 /copy），常态无害；唯一可构造场景是 insight 拆分 + 亚帧窗口内触发 /recap，锚点可能落到 insight 消息序列中部。代价极低。建议：LiveMessageList 恒挂载时父写冗余，可删父 useLayoutEffect 让子成为唯一权威（或仅在 messageListRef.current === null 时父写）。
React runs child layout effects before the parent, so on the same structural commit LiveMessageList writes the projection first and App then overwrites it with the structural baseline. Both readers are event handlers, so this is harmless in the common case; the only constructible scenario is insight-splitting plus a /recap inside a sub-frame window. Since LiveMessageList is always mounted, the parent write is redundant — consider removing it (or writing only when messageListRef.current === null).
[4] 测试残留对已删除类的空洞断言
[4] A leftover vacuous assertion on a removed class in tests
位置 / Location: ToolGroup.test.tsx:829-833。chatSummaryContentCollapsed 已随 CSS 折叠路径删除（全仓仅剩此一处字符串引用，非 styles. 引用，无编译风险），expect(...).not.toContain('chatSummaryContentCollapsed') 现恒为真、不再保护任何行为。建议删除或改为断言新语义（展开时 clip 存在、折叠时子树不存在）。
chatSummaryContentCollapsed was removed with the CSS collapse path (this is the only remaining string reference, not a styles. reference, so no compile risk); the not.toContain(...) assertion is now always true and protects nothing. Remove it or assert the new semantics instead.
💡 Suggestion
[5] insight 标记与 adapter 前缀集合无共享来源
[5] The insight marker and the adapter prefix set share no common source
位置 / Location: useMessages.ts:40（INSIGHT_CONTENT_MARKER = '"insight_'）vs transcriptToMessages.ts:1418-1422（INSIGHT_PREFIXES）。当前三个前缀都以 "insight_ 开头，语义一致、无现实失败；但若未来新增不以 "insight_ 开头的前缀，fast path 会把原始协议 JSON 当普通文本拼接流式渲染，直到下一个结构性变化才纠正，且无编译/测试报错。建议由 adapter 导出共享判定（如 mayContainInsightProtocol(text) 基于 INSIGHT_PREFIXES 派生），useMessages.ts 引用同一来源。
The three current prefixes all start with "insight_, so there is no present failure; but a future prefix not starting that way would let the fast path stream raw protocol JSON as plain text until the next structural change, with no compile/test signal. Consider exporting a shared predicate from the adapter and using it in useMessages.ts.
[6] 设计文档未提及"进入"动画也被移除
[6] The design doc does not mention that the enter animation was also removed
位置 / Location: docs/design/web-shell-collapsed-thinking-performance.md。文档说折叠瞬时（不等退场动画），但被删的 transition: grid-template-rows 180ms ease 是双向的——展开方向的进入动画也一并消失。这本身可接受（与性能目标一致），建议补一句"展开亦无动画"，避免后续读者误以为 enter transition 是遗漏。
The doc says collapse is immediate, but the removed transition was bidirectional — the expand/enter animation is gone too. That is fine, but a one-line note ("expand is also unanimated") would prevent a future reader from thinking the enter transition was dropped by accident.
结论 / Conclusion
无阻塞级问题；bot 的 Critical 不可复现。两条 Important（insight 边界 reconciliation、MCP iframe 状态）建议合入前处理，均为小改动。其余 Nit/Suggestion 可选。💬 评论。
No blocking issues; the automated Critical does not reproduce. The two Important findings (insight-edge reconciliation, MCP iframe state) are worth addressing before merge and are both small changes; the Nit/Suggestion items are optional. Comment.
|
Addressed the findings from the latest reviews in
The shared Insight-marker predicate suggestion is intentionally deferred: all current protocol prefixes match the existing marker and there is no present behavior defect. Validation: 541 App tests + 143 related tests, Web Shell build, typecheck, ESLint, and changed-file formatting all pass. |
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.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/App.tsx:923 — [review] no test pins that onTranscriptChange receives live blocks from LiveMessageListpackages/web-shell/client/hooks/useAnimationFrameTranscriptBlocks.test.tsx:161 — [review] no test re-queries the structural getSnapshot after a suppressed tail append
中文说明
已审查——无阻断问题。 建议见行内评论。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(testState.latestMessageListProps?.messages).toMatchObject([ | ||
| { id: 'assistant', role: 'assistant', content: 'ab' }, | ||
| ]); | ||
| expect(testState.latestMessageListProps?.transcriptBlockCount).toBe(1); |
There was a problem hiding this comment.
[Suggestion] This transcriptBlockCount assertion cannot discriminate the source it appears to pin: the fixture gives blocks and liveBlocks exactly one block each, and the harness mock returns testState.liveBlocks ?? testState.blocks to the structural and the live consumers alike, so the assertion passes whether LiveMessageList passes live.blocks.length or baselineBlocks.length. Verified by mutation: swapping in transcriptBlockCount={baselineBlocks.length} keeps all 541 App tests green. That matters because MessageList feeds this prop into the transcript-reload gate (MessageList.tsx:3703/3712); in the window where the live snapshot has observed a new block before the structural baseline catches up, the wrong source reads a stale count. Give liveBlocks a second block the baseline lacks and expect the count to be 2, so only the live source satisfies the assertion:
testState.liveBlocks = [
{ id: 'assistant', text: 'ab' },
{ id: 'tool', kind: 'tool' },
];
// ...
expect(testState.latestMessageListProps?.transcriptBlockCount).toBe(2);中文说明
这个 transcriptBlockCount 断言无法区分它看似锁定的来源：fixture 给 blocks 和 liveBlocks 各一个块，且 harness mock 对结构性与 live 两个消费者都返回 testState.liveBlocks ?? testState.blocks，因此无论 LiveMessageList 传入 live.blocks.length 还是 baselineBlocks.length，断言都会通过。已实测变异：把 transcriptBlockCount 改成 {baselineBlocks.length} 后 541/541 个 App 测试仍全部通过。这很重要，因为 MessageList 把该 prop 用于 transcript 重新加载门控（MessageList.tsx:3703/3712）；在 live 快照已看到新块而结构基线尚未跟上的窗口里，错误的来源会读到过期的计数。建议给 liveBlocks 一个基线没有的第二个块并断言计数为 2，使只有 live 来源能满足断言。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| act(() => container.querySelector('button')?.click()); | ||
| expect(container.querySelector('iframe')).toBe(iframe); | ||
| expect(iframe?.dataset['testState']).toBe('preserved'); |
There was a problem hiding this comment.
[Suggestion] The rewritten persistence test never asserts that the clip's inline display: none is cleared after re-expanding, leaving the visibility half of style={chatExpanded ? undefined : { display: 'none' }} unpinned. A regression that keeps the content hidden when expanded (e.g. an unconditional style={{ display: 'none' }}) would make MCP app content permanently invisible after the user expands the group, yet every assertion here would still pass: the iframe identity and dataset checks are display-blind, the only style.display assertion above checks the collapsed state, and sibling MCP tests use textContent, which includes hidden nodes. Verified: the style={{ display: 'none' }} mutant passes all 4 MCP tests, and adding the assertion below catches it (expected 'none' to be '').
| act(() => container.querySelector('button')?.click()); | |
| expect(container.querySelector('iframe')).toBe(iframe); | |
| expect(iframe?.dataset['testState']).toBe('preserved'); | |
| act(() => container.querySelector('button')?.click()); | |
| expect(container.querySelector('iframe')).toBe(iframe); | |
| expect(iframe?.dataset['testState']).toBe('preserved'); | |
| expect((content as HTMLElement | null)?.style.display).toBe(''); |
中文说明
重写后的持久化测试没有断言重新展开后 clip 的内联 display: none 被清除，style={chatExpanded ? undefined : { display: 'none' }} 的可见性一半因此未被锁定。若回归使内容在展开时仍隐藏（例如无条件 style={{ display: 'none' }}），用户展开后 MCP 应用内容将永久不可见，而本测试所有断言仍会通过：iframe 身份与 dataset 检查对 display 不敏感，上方唯一的 style.display 断言检查的是收起状态，兄弟 MCP 测试用的 textContent 包含隐藏节点。已实测：style={{ display: 'none' }} 变异体通过全部 4 个 MCP 测试；加上下方断言即可捕获（expected 'none' to be ''）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| }); | ||
|
|
||
| describe('useAnimationFrameTranscriptSnapshot', () => { | ||
| it('caches structural snapshots when the store has no change summary', () => { |
There was a problem hiding this comment.
[Suggestion] This summary-less store test performs no store update after mount, so the degradation guard is untested: a mutant making isSameTranscriptStructure(undefined, undefined) return true would classify every later notification of a summary-less store as tail-only and suppress it — the structural App consumer freezes at mount and the transcript stops updating — while this test still passes, because it only asserts renderCount === 1 and empty blocks immediately after mount (the subscribe callback is never even invoked). Verified: that mutant passes all 10 tests in this file, and extending the test as below catches it (expected 1 to be greater than 1). Push an update through the summary-less store after the mount assertions and assert the consumer wakes:
act(() => testStore.update([{ id: 'a' } as DaemonTranscriptBlock]));
// fire the pending frame (mock rAF as the sibling tests do)
expect(renderCount).toBeGreaterThan(1);
expect(latestBlocks).toHaveLength(1);This also keeps the design doc's "Stores without a change summary retain the existing behavior" honest.
中文说明
这个无 summary store 测试在挂载后不做任何 store 更新，退化防护因此未被测试：若变异使 isSameTranscriptStructure(undefined, undefined) 返回 true，无 summary store 之后的每次通知都会被归类为纯尾部追加而被抑制——结构性 App 消费者将冻结在挂载状态、transcript 停止更新——而本测试仍会通过，因为它只断言挂载后 renderCount === 1 和空 blocks（subscribe 回调根本不会触发）。已实测：该变异体通过本文件全部 10 个测试；按下方方式扩展测试即可捕获（expected 1 to be greater than 1）。在挂载断言之后向无 summary store 推入更新并断言消费者被唤醒。这同时守住设计文档中"没有 change summary 的 store 保持原有行为"的承诺。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| await act(async () => root.unmount()); | ||
| }); | ||
|
|
||
| it('keeps agent reconciliation when an insight tail has no projected message', async () => { |
There was a problem hiding this comment.
[Suggestion] The fixture does not produce the "no projected message" state the test name describes. The tail block is kind: 'thought' with text: '', and the adapter's thought case (transcriptToMessages.ts:648-683) has no empty-text guard — unlike the assistant case's if (!textBlock.text && !textBlock.usage) break; — so revision 1 projects an empty thinking message with id 'empty-thought'. Verified by probe: at revision 2 projectStreamingTailMessages returns a defined result — findIndex matches message.id === before.id at index 1 and the prefix-reuse branch runs — so the firstTailMessageIndex < 0 → return undefined fallback is covered only by the pure-function test 'falls back when an insight tail has no matching projected message', never at hook integration level. If the -1 guard's hook integration regresses (the guard is dropped or returns a stale array), this test still passes at firstTailMessageIndex === 1 despite its title claiming that coverage. Rename it to what it sets up (e.g. 'keeps agent reconciliation when an insight marker appears in a streaming tail'), or — if the fallback path was the intent — use an empty kind: 'assistant' tail, which genuinely projects no message.
中文说明
fixture 并未产生测试名所描述的"无投影消息"状态。尾部块是 kind: 'thought' 且 text: ''，而 adapter 的 thought 分支（transcriptToMessages.ts:648-683）没有空文本守卫——不同于 assistant 分支的 if (!textBlock.text && !textBlock.usage) break;——因此 revision 1 会投影出一条 id 为 'empty-thought' 的空 thinking 消息。探针实测：revision 2 时 projectStreamingTailMessages 返回有定义的结果——findIndex 在索引 1 处按 message.id === before.id 命中，走的是前缀复用分支——因此 firstTailMessageIndex < 0 → return undefined 回退只被纯函数测试 'falls back when an insight tail has no matching projected message' 覆盖，hook 集成层面并未覆盖。若 -1 守卫的 hook 集成发生回归（守卫被删除或返回过期数组），本测试在 firstTailMessageIndex === 1 下仍会通过，尽管其标题声称覆盖了该场景。建议改名为与实际场景一致的名称（如 'keeps agent reconciliation when an insight marker appears in a streaming tail'），或者如果本意是覆盖回退路径，改用空的 kind: 'assistant' 尾部——它确实不会投影出消息。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /takeover from 3 |
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. This window's round counter starts at 3 (the rounds this PR spent in review before takeover), so the Critical-only brake engages after 2 more change-producing round(s) instead of a full fresh 5. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本窗口轮次计数从 3 起算（即本 PR 托管前已进行的评审轮数），因此再经过 2 个产生改动的轮次即进入 Critical-only，而非重新计满 5 轮。移除 |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summaryRound 3 on PR #9914 (head Feedback points and dispositionsImplemented this round
Re-verified (fixed in an earlier commit)
Not resolved this round
Verification
中文说明本轮概要PR #9914 的第 3 轮（head 反馈点及处理本轮实现
复查确认（更早提交已修复）
本轮未解决
验证
🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈（门自动测量）：bite 检查无法验证测试侧修复，该解决仅以轮次摘要为凭。 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/模型 |
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.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/messages/ToolGroup.test.tsx:96 — [review] renderToolGroup grew to 9 positional parameters with an undefined filler chainpackages/web-shell/client/App.test.tsx:592 — [probe] The projectStreamingTailMessages mock is argument-blind, leaving the projection wiring unpinned
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/web-shell/client/App.test.tsx (findings in round 2; 1 more now); packages/web-shell/client/hooks/useMessages.test.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查——无阻断问题。 建议见行内评论。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/App.test.tsx（第 2 轮已出过发现，本轮又有 1 条）；packages/web-shell/client/hooks/useMessages.test.ts（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| options?.structuralOnly === true | ||
| ? testState.blocks | ||
| : (testState.liveBlocks ?? testState.blocks), |
There was a problem hiding this comment.
[Suggestion] The snapshot mock now branches on options?.structuralOnly, but no assertion depends on which option the App-level caller passes — every top-level blocks consumer is mocked to ignore its argument or sees no difference between blocks and liveBlocks. That leaves the App-level { structuralOnly: true } opt-in (App.tsx:2290), the wiring this PR exists to deliver, unpinned: if a future change drops that option, the top-level render path re-subscribes to every streamed tail append — exactly the jank this PR fixes — and the whole suite stays green. An A/B probe at this commit confirms it: dropping { structuralOnly: true } from App.tsx leaves all 684 tests of the four changed suites passing on both arms (Tests 684 passed (684)).
Record the options each call site passes into the mock and assert on them in the boundary test, e.g.:
// in the mock
useAnimationFrameTranscriptSnapshot: (options?: { structuralOnly?: boolean }) => {
testState.snapshotCallOptions.push(options);
return {
blocks:
options?.structuralOnly === true
? testState.blocks
: (testState.liveBlocks ?? testState.blocks),
};
},
// in the boundary test (declare/reset snapshotCallOptions in beforeEach)
expect(testState.snapshotCallOptions).toContainEqual({ structuralOnly: true }); // App-level call
expect(testState.snapshotCallOptions).toContainEqual(undefined); // LiveMessageList call中文说明
快照 mock 现在会根据 options?.structuralOnly 分支，但没有任何断言依赖 App 层调用者传入的选项——所有顶层 blocks 消费者要么被 mock 成忽略参数，要么对 blocks 与 liveBlocks 看不出差别。因此 App 层的 { structuralOnly: true } 订阅（App.tsx:2290）——本 PR 的核心接线——没有被任何测试锁定：未来若有改动去掉该选项，顶层渲染路径会重新订阅每一次流式尾部追加——正是本 PR 要修复的卡顿——而整个测试套件仍全绿。在本提交上的 A/B 探针证实了这一点：去掉 App.tsx 中的 { structuralOnly: true } 后，四个改动套件的 684 个测试在两臂上均全部通过（Tests 684 passed (684)）。
建议将各调用点传入的选项记录到 mock（如 push 进 testState.snapshotCallOptions，并在 beforeEach 中声明/重置），并在边界测试中分别断言 App 层调用带 { structuralOnly: true }、LiveMessageList 调用不带（见上方英文部分的 ts 代码块）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // An empty assistant block projects no message (unlike thought), so the | ||
| // insight append below hits the "no matching projected message" fallback. |
There was a problem hiding this comment.
[Suggestion] Swapping this fixture from an empty thought tail to an empty assistant tail makes the test genuinely hit the findIndex === -1 fallback — but it also silently moves the only reconciliation-plus-insight coverage off the matched path, where the tail block HAS a matching projected message and correctness depends on the insight branch reusing the committed, post-reconciliation prefix (messages[i] = previous.messages[i], useMessages.ts ~218-224) while the reconciledMessages memo trusts reusedStreamingTail. No remaining test combines a background-agent resolution with an insight append on that path. A probe at this commit shows the hole: with the prefix-reuse line removed, this changed test still passes (only the projection-level identity assertion elsewhere fails), while a matched-path variant displays the user-visible symptom — a completed background-agent tool group flickering back to pending (- "status": "completed" / + "status": "pending"); restoring the code turns the variant green (Tests 52 passed (52)). Keep the new fallback case and add back a matched-path variant (e.g. the previous kind: 'thought' tail, or a non-empty assistant tail), asserting the reconciled status: 'completed' tool group survives the insight append.
中文说明
将该 fixture 从空的 thought 尾部换成空的 assistant 尾部后，测试确实命中了 findIndex === -1 回退路径——但同时也把唯一“背景任务 reconciliation + insight 追加”的覆盖从匹配路径（尾部块存在 id 可匹配的已投影消息）上移走了；匹配路径的正确性依赖 insight 分支复用已提交、已 reconcile 的前缀（messages[i] = previous.messages[i]，useMessages.ts 约 218-224 行），且 reconciledMessages memo 信任 reusedStreamingTail。此后不再有测试在该路径上同时组合背景任务 resolution 与 insight 追加。在本提交上的探针显示了缺口：删除前缀复用行的变异下，本改动后的测试仍然通过（只有别处投影层的引用相等断言失败），而匹配路径变体展示了用户可见的症状——已完成的后台 agent 工具组闪回 pending（- "status": "completed" / + "status": "pending"）；恢复正确代码后该变体通过（Tests 52 passed (52)）。建议保留新的回退用例，同时补回一条匹配路径变体（如之前的 kind: 'thought' 尾部，或非空的 assistant 尾部），断言 insight 追加后已 reconcile 的 status: 'completed' 工具组仍然存在。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Address review summary — PR #9914 (round 4)Round outcome: both inline Suggestion findings resolved in code (test-only changes, no source changes). No Critical findings, no failed checks, no base conflict ( Feedback points and dispositions[rc:3846970159] App-level
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/App.test.tsx:595 — [probe] The projectStreamingTailMessages mock is argument-blind, leaving the projection wiring unpinned
中文说明
无阻断问题。LGTM！✅
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. Approving.
Scope (round 5 — blockers only per convergence posture):
Checked:
ToolGroup.tsxcollapse change: pending approvals are guarded by the!hasApprovalToolearly-return at line 1754, which bypasses the entire collapsible UI when a tool call awaits approval — the approval UI is always rendered regardless ofchatExpandedstate. Clean.useAnimationFrameTranscriptSnapshot({ structuralOnly: true }): for stores withoutgetBlockChangeSummarythe hook correctly degrades to existing behaviour (every update triggers a snapshot check), consistent with the design-doc statement "Stores without a change summary retain the existing behavior."projectStreamingTailMessagesInsight path: prefix-reuse loop correctly breaks on first id/role mismatch and falls back to fresh projection; thefirstTailMessageIndex < 0guard handles the no-matching-message case.LiveMessageListsplit:messagesRef.currentupdate moved touseLayoutEffect— the only reader is therecapcallback (user-triggered, never fires mid-commit). Clean.buildDisplayMessagesextracted from App-leveluseMemoto the same logic insideLiveMessageList— functionally equivalent, no behavioural change.
CI coverage gaps (disclosed):
Test (macos-latest, Node 22.x)— SKIPPED. PR description confirms macOS tested locally; CI has no evidence.Test (windows-latest, Node 22.x)— SKIPPED. PR description marks Windows N/A.Integration Tests (CLI, No Sandbox)— SKIPPED. Change is web-shell only; integration tests on CLI path are not in scope.
Ran: rung 0 only (CI result enumeration); no local toolchain available for rungs 1–3.
Reviewed with AI assistance.
|
Released in v0.22.2. |
5 participants
What this PR does
This change keeps pure streamed assistant and thinking text updates out of the top-level application render path while preserving live updates in the transcript. It also unmounts compact tool and thinking details while collapsed, and retains correct live projection when Insight protocol messages span multiple stream chunks.
Why it's needed
Long thinking output caused hidden transcript content and unrelated application surfaces to reconcile on every streamed chunk. Typing, starting actions, and collapsing activity groups could therefore feel sluggish during an active response.
Reviewer Test Plan
How to verify
Start a Web Shell session that produces sustained thinking output, keep the compact activity group collapsed, and type in the composer while streaming continues. Confirm that text remains live, typing and toolbar actions stay responsive, expanding restores the current tool and thinking details, and Insight progress or ready messages appear immediately even when their JSON marker spans chunks.
Evidence (Before & After)
In the deterministic 5,000-character / 400-chunk folded-thinking scenario, frame-time P95 improved from 17 ms to 9 ms. Sustained stream time improved by approximately 21%, and typing overhead improved by approximately 34%. The post-fix run completed in 5,057 ms with 120 ms typing overhead, 9 ms frame-time P95, and 24 ms maximum input latency.
Tested on
Environment (optional)
Local Vite development server with the daemon-backed Web Shell session, plus package unit tests and production builds.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 的改动
本次改动让纯粹的助手文本和思考文本流式增量不再进入顶层应用渲染路径，同时保持 transcript 内容实时更新。紧凑工具与思考详情在收起时会被卸载，并且当 Insight 协议消息跨多个流式 chunk 时仍能正确实时投影。
为什么需要
长时间思考输出此前会导致隐藏的 transcript 内容和无关应用区域在每个流式 chunk 上进行协调更新。因此，在响应进行期间，输入、启动操作以及收起活动组都可能出现卡顿。
Reviewer 测试计划
如何验证
启动一个会持续输出思考内容的 Web Shell 会话，保持紧凑活动组收起，并在流式输出期间继续在编辑器中输入。确认文本保持实时更新，输入和工具栏操作保持响应，展开后能够恢复当前工具与思考详情，并且即使 Insight progress 或 ready 的 JSON 标记跨 chunk，也会立即显示。
证据（修复前后）
在确定性的 5,000 字符 / 400 chunk 折叠思考场景中，帧耗时 P95 从 17 ms 降至 9 ms。持续流式处理时间约改善 21%，输入额外开销约改善 34%。修复后一次运行耗时 5,057 ms，输入额外开销 120 ms，帧耗时 P95 为 9 ms，最大输入延迟为 24 ms。
测试平台
环境（可选）
使用本地 Vite 开发服务器和 daemon 驱动的 Web Shell 会话，并运行包级单元测试与生产构建。
风险与范围
关联 Issue
N/A