fix(cli): avoid agent composer unmount reset - #5302
Conversation
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @tt-a1i — thanks for the patch! Before anyone dives into the code, could you reformat the PR body to match the pull request template? The current body uses free-form headings (Summary / Notes / Test Plan / AI Assistance Disclosure) and is missing the sections reviewers rely on to triage:
- What this PR does and Why it's needed — the motivation and the bug being fixed (refs #5199) in prose.
- Reviewer Test Plan —
How to verify,Evidence (Before & After)with actual before/after TUI output, and theTested onOS matrix. - Risk & Scope — main risk / tradeoff, what's out of scope, and any breaking changes.
- Linked Issues —
Refs #5199(orFixes #5199if it closes it). - The 中文说明
<details>block with the full translation.
Once the body is reshaped I'll re-run triage and we can get into the actual change. 🙏
中文说明
@tt-a1i 你好，感谢提交！在开始代码评审之前，麻烦按照 PR 模板 重新整理一下 PR 正文。当前正文使用了自由标题（Summary / Notes / Test Plan / AI Assistance Disclosure），缺少评审所需的关键章节：
- What this PR does 与 Why it's needed：用文字说明动机以及要修复的 bug（关联 #5199）。
- Reviewer Test Plan：包含
How to verify、Evidence (Before & After)的前后 TUI 输出，以及Tested on操作系统矩阵。 - Risk & Scope：主要风险/权衡、不在范围内的内容、破坏性变更。
- Linked Issues：
Refs #5199（若关闭则用Fixes #5199）。 - 中文说明
<details>区块：完整对应英文正文的翻译。
整理好后我会重新跑 triage 并进入代码评审。🙏
— Qwen Code · qwen3.7-max
✅ Maintainer verification — real-TUI (tmux) + tests + mutationVerified on a clean build of the PR head ( What the change does
Root cause (
|
| Check | Result |
|---|---|
Build real qwen binary (npm ci + build) |
✅ exit 0 |
Declared suite — agent-view/ |
✅ 42 passed (incl. new AgentComposer.test.tsx) |
Declared suite — InputPrompt onTabConsumerChange (#4171) |
✅ 6 passed (incl. "reports false on unmount even if a Tab consumer was active" → confirms the InputPrompt cleanup is kept) |
| Mutation test (deterministic) | ✅ see below |
| Dataflow safety | ✅ agentInputBufferText has zero production readers |
| Real‑TUI arena A/B (tmux) | ✅ no regression on FIXED; PRE‑FIX also clean |
1. Mutation test — proves the new test is non‑vacuous. Reverting only AgentComposer.tsx to base (re‑adding the cleanup) while keeping the PR's test makes it FAIL:
expect(setAgentInputBufferText).not.toHaveBeenCalled()
Number of calls: 1
1st spy call: [ "" ] ← the cleanup fires setAgentInputBufferText('') on unmount
Restoring the PR version → passes. So the test guards precisely the removed line.
2. Dataflow — why this is strictly safe. An exhaustive search shows agentInputBufferText is written but never read in production: the only occurrences are the context definition (AgentViewContext.tsx) and mock objects in InputPrompt.test.tsx. AgentTabBar/InputPrompt do not read it. So removing the unmount reset cannot regress any consumer, and the "stale last‑synced value persists" note is moot in practice. (Minor follow‑up for later, out of scope here: the value is now write‑only — the whole sync effect could eventually be dropped.)
3. Real‑TUI (tmux), real arena with 2 in‑process agents (qwen3.7-max + qwen3.7-plus, both completed). I drove the exact teardown surface from the crash stack: focus a tab → switch into an agent tab (mounts AgentComposer) → type a draft (non‑empty buffer) → 12 rapid tab switches (each key={activeView} switch = one passive unmount/mount cycle) → app teardown.
- FIXED: 0 React errors/warnings across the whole lifecycle.
- PRE‑FIX (cleanup restored + rebuilt): identical sequence plus an extreme 100‑keystroke burst → also 0 errors, process stayed alive.
Honest note on reproducing the raw #185
The crash is a nested‑update loop that needs a teardown storm (the reporter's embedding host / concurrent state churn). Under manual — or even burst — TUI keystrokes, each unmount settles and React 18 treats setState on an unmounted fiber as a no‑op, so #185 does not reproduce by hand on either build. The decisive behavioral difference is therefore the unit mutation test above; the fix removes exactly the setState in the reported stack and is provably safe (no readers). Net: a legitimate, well‑targeted defensive fix.
🇨🇳 中文版（点击展开）
✅ 维护者验证 —— 真实 TUI（tmux）+ 测试 + 变异测试
在隔离 worktree 中对 PR head（ba0cd9a9d4）全新构建后验证。结论：正确、最小化、低风险 —— 建议合并。
改动做了什么
- 从
AgentComposer的 buffer 同步 effect 中移除卸载清理函数return () => setAgentInputBufferText('')（AgentComposer.tsx）。 - 更新
agentInputBufferText的文档注释（改为"最后同步值"，不再是"在 main 上为空"）。 - 刻意不触碰
InputPrompt——#4171的 tab‑consumer 清理保持原样（比#5228更窄）。
根因（#5199 → React #185）
React 错误 #185 = "Maximum update depth exceeded…"（超过最大更新深度）（已对照 React 的 codes.json 核实）。它由 getRootForUpdatedFiber → throwIfInfiniteUpdateLoopDetected 抛出。issue 的堆栈起源于一个 passive 卸载清理（commitPassiveUnmountOnFiber → commitHookEffectListUnmount → dispatchSetState）—— 正好对应被删除的 return () => setAgentInputBufferText('')：在子组件卸载期间向顶层 AgentViewProvider（挂载于 gemini.tsx）发起一次 setState。移除它即把该 setState 从 agent 视图的卸载路径中清除。
证据
| 检查项 | 结果 |
|---|---|
构建真实 qwen 二进制（npm ci + build） |
✅ exit 0 |
声明测试 —— agent-view/ |
✅ 42 通过（含新增 AgentComposer.test.tsx） |
声明测试 —— InputPrompt onTabConsumerChange（#4171） |
✅ 6 通过（含 "reports false on unmount even if a Tab consumer was active" → 证实 InputPrompt 的清理被保留） |
| 变异测试（确定性） | ✅ 见下 |
| 数据流安全性 | ✅ agentInputBufferText 在生产代码中零读取方 |
| 真实 TUI arena A/B（tmux） | ✅ FIXED 无回归；PRE‑FIX 同样干净 |
1. 变异测试 —— 证明新测试非空过场。 仅把 AgentComposer.tsx 还原到 base（重新加回 cleanup）、保留 PR 的测试，测试即失败：
expect(setAgentInputBufferText).not.toHaveBeenCalled()
调用次数: 1
第 1 次 spy 调用: [ "" ] ← 卸载时 cleanup 触发了 setAgentInputBufferText('')
还原 PR 版本 → 通过。所以该测试精确守护了被删除的那一行。
2. 数据流 —— 为什么严格安全。 穷举搜索表明 agentInputBufferText 在生产代码中只写不读：全部出现位置仅为 context 定义（AgentViewContext.tsx）与 InputPrompt.test.tsx 里的 mock 对象。AgentTabBar/InputPrompt 都不读取它。因此移除卸载 reset 不可能让任何消费者回归，PR 里"残留的最后同步值会保留"的担忧在实际中也不成立。（后续可选、本 PR 范围之外：该值现在已是只写状态，整个同步 effect 将来可一并移除。）
3. 真实 TUI（tmux），2 个 in‑process agent 的真实 arena（qwen3.7-max + qwen3.7-plus，均完成）。我复现了崩溃堆栈所指的精确卸载场景：聚焦 tab → 切入某个 agent tab（挂载 AgentComposer）→ 输入草稿（buffer 非空）→ 12 次快速切换 tab（每次 key={activeView} 切换 = 一次 passive 卸载/重挂载循环）→ 退出 app。
- FIXED： 整个生命周期 0 个 React 错误/警告。
- PRE‑FIX（恢复 cleanup 并重新构建）：相同序列 外加 100 次极端连续按键突发 → 同样 0 错误，进程存活。
关于复现原始 #185 的诚实说明
该崩溃是一个嵌套更新循环，需要一次卸载风暴（报告者的嵌入宿主 / 并发状态 churn）才能触发。在手动 —— 甚至突发 —— 的 TUI 按键下，每次卸载都会 settle，且 React 18 会把对已卸载 fiber 的 setState 视为 no‑op，所以 #185 在两个构建上都无法手动复现。因此决定性的行为差异来自上面的单元变异测试；该修复移除的正是报告堆栈中的那次 setState，且可证明安全（无读取方）。总体：一个合理、精准的防御性修复。
Verification method: worktree build + declared test plan + source‑revert mutation test + exhaustive dataflow grep + tmux A/B on real binaries (fixed vs cleanup‑restored).
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: this is a clean, well-motivated crash fix. Issue #5199 reports React error #185 ("Cannot update a component while rendering a different component"), and the stack trace points precisely to On approach: the scope is admirably minimal — two lines removed from the cleanup, a doc comment update, and a regression test. Deliberately narrower than the closed PR #5228 (doesn't touch Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是一个干净且有充分动机的崩溃修复。Issue #5199 报告了 React error #185（"无法在渲染另一个组件时更新当前组件"），堆栈精确指向 方案：范围极小——删除两行清理代码、更新文档注释、添加回归测试。有意比已关闭的 PR #5228 更窄（不触碰 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe change is clean and precisely targeted. Three files touched, each change justified:
No correctness bugs, no security concerns, no AGENTS.md violations. The diff is as minimal as a crash fix can be. Test Results
Real-Scenario Test (tmux)The crash (React #185) is a non-deterministic unmount-ordering issue that can't be reproduced in CLI starts cleanly, responds normally, no errors. The unit test provides the deterministic behavioral guarantee: reverting the fix makes the test fail with 中文说明代码审查改动干净且精准。涉及三个文件，每处改动都有充分理由：
无正确性 bug、无安全隐患、无 AGENTS.md 违规。diff 作为崩溃修复已尽可能精简。 测试结果
真实场景测试（tmux）崩溃（React #185）是一个非确定性的卸载时序问题，在 CLI 正常启动，响应正常，无错误。单元测试提供了确定性的行为保障：回滚修复会使测试失败 — Qwen Code · qwen3.7-max |
Final AssessmentThis is exactly the kind of PR you want to see: a tight, well-reasoned crash fix with clear evidence. The root cause analysis is solid — the #5199 stack trace shows The regression test is non-vacuous: reverting the fix makes it fail with the exact The one thing that makes this even more confidence-inspiring is the maintainer's independent verification (@wenshao's comment): mutation testing, exhaustive dataflow analysis showing Minor observation for a future cleanup (out of scope for this PR): Verdict: approve. Clean crash fix, minimal diff, solid test, thorough independent verification. 中文说明最终评估这正是你希望看到的 PR：一个紧凑、推理充分的崩溃修复，附带清晰的证据。 根因分析扎实——#5199 堆栈显示 回归测试非空验证：回滚修复会使测试以 PR 移除的精确 更增添信心的是维护者的独立验证（@wenshao 的评论）：变异测试、穷举数据流分析表明 对未来清理的小观察（不在本 PR 范围内）： 结论：批准。 干净的崩溃修复，最小 diff，扎实的测试，充分的独立验证。 — Qwen Code · qwen3.7-max |
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. ✅
qwen-code-ci-bot
added
category/ui
3 participants
What this PR does
Removes the
useEffectcleanup inAgentComposerthat reset the parentAgentViewContextinput-buffer state (setAgentInputBufferText('')) when the composer unmounted. The component keeps syncing the active agent's buffer text to context on mount and on every update (setAgentInputBufferText(buffer.text)); only the unmount-time reset is dropped. Because that reset is gone,agentInputBufferTextis now a "last-synced" value rather than something guaranteed to be cleared on unmount, so the field's doc comment inAgentViewContext.tsxis updated to say "Last synced text from the active agent tab's input buffer." A unit test is added asserting that unmountingAgentComposerdoes not call the parent input-buffer setter.The change is deliberately narrower than the closed PR #5228: it does not touch
InputPrompt.tsx, so the existing #4171 tab-consumer cleanup stays in place.Why it's needed
Issue #5199 reports a crash with minified React error #185 ("Cannot update a component while rendering a different component"). The stack trace shows the failure originating from
commitHookEffectListUnmount→dispatchSetState: a child component'suseEffectcleanup was callingsetStateon a parent context provider that was unmounting in the same commit phase.AgentComposer's cleanup did exactly that — it calledsetAgentInputBufferText('')on the parentAgentViewContextduring unmount — which could surface the React #185 error and crash the render. Removing the cleanup eliminates the parent-setState-during-unmount path. The buffer text is internal plumbing (used to coordinate agent-tab state, not directly rendered), so dropping the forced reset on unmount has no functional downside.Reviewer Test Plan
How to verify
Repro (from the issue): the crash occurs when an
AgentComposerunmounts while its parentAgentViewContextprovider is unmounting in the same React commit (e.g. tearing down the agent view), producing React error #185.setAgentInputBufferText('')on the unmounting parent context).does not reset the parent input-buffer state during unmount) rendersAgentComposer, then unmounts it and assertssetAgentInputBufferTextis not called during unmount.Verification commands (run by the author):
npx vitest run packages/cli/src/ui/components/agent-view/AgentComposer.test.tsxnpx vitest run packages/cli/src/ui/components/agent-viewnpx vitest run packages/cli/src/ui/components/InputPrompt.test.tsx -t "onTabConsumerChange reporting"npx eslint packages/cli/src/ui/components/agent-view/AgentComposer.tsx packages/cli/src/ui/components/agent-view/AgentComposer.test.tsx packages/cli/src/ui/contexts/AgentViewContext.tsxnpx prettier --check packages/cli/src/ui/components/agent-view/AgentComposer.tsx packages/cli/src/ui/components/agent-view/AgentComposer.test.tsx packages/cli/src/ui/contexts/AgentViewContext.tsxnpm run build --workspace=packages/corenpm run build --workspace=packages/acp-bridgenpm run typecheck --workspace=packages/cligit diff --check upstream/main...HEADEvidence (Before & After)
This is a crash fix, not a layout/widget change. The user-observable effect is a render crash, not a visible UI element:
agentInputBufferTextis internal state used to coordinate agent-tab behavior and is not text drawn on screen.AgentViewContextunmounts in the same commit could throw minified React error fix e2e #185 and crash the render (see the stack trace in Minified React error #185; visit https://react.dev/errors/185 for the full message or use the non-minified dev environment for full errors and additional helpful warnings. file:///E:/V-pan-FIX/C-pan/Users/nasAdmin/.cherrystudio/install/global/node #5199).setStateis removed, so that crash path no longer exists; the regression is locked in by the new unit test.No screenshot was captured: the symptom is an intermittent React-internal crash (error #185 overlay/stack trace) triggered by a specific unmount-ordering commit, not a deterministic on-screen state, so a meaningful Before/After screenshot could not be reliably reproduced. The behavior is instead covered by the added unit test.
Tested on
✅ tested ·⚠️ not tested · N/A
Environment (optional)
Unit tests only (npm workspaces). No special environment required.
Risk & Scope
AgentComposerand a doc comment inAgentViewContext. The only behavior change is that the active agent's input-buffer text is no longer force-cleared in context on composer unmount (it retains its last-synced value).InputPrompt.tsx(the Tab 键在windows上存在冲突，当输入框为空时，Tab会同时触发输入预测和权限模式变更，应当修改为一次键盘事件只对应一个行为 #4171 tab-consumer cleanup is intentionally preserved). No unrelated refactors, public API changes, or UI redesigns.Linked Issues
Refs #5199
中文说明
这个 PR 做了什么
移除了
AgentComposer中的useEffect清理逻辑——该逻辑在组件卸载（unmount）时会重置父级AgentViewContext的输入缓冲状态（调用setAgentInputBufferText('')）。组件仍然会在挂载时以及每次更新时把当前 active agent 的缓冲文本同步到 context（setAgentInputBufferText(buffer.text)），仅去掉了卸载时的那次重置。由于不再重置，agentInputBufferText现在是一个"最近一次同步的值"，而不是"卸载后保证被清空"的值，因此同步更新了AgentViewContext.tsx中该字段的文档注释为"Last synced text from the active agent tab's input buffer."（active agent tab 输入缓冲的最近同步文本）。同时新增了一个单元测试，断言卸载AgentComposer时不会再调用父级的输入缓冲 setter。此改动有意比已关闭的 PR #5228 更小：它不触碰
InputPrompt.tsx，因此现有的 #4171 tab-consumer 清理逻辑保持原样。为什么需要它
Issue #5199 报告了一个崩溃，错误为压缩后的 React error #185（"Cannot update a component while rendering a different component"，无法在渲染另一个组件时更新当前组件）。堆栈显示该错误源自
commitHookEffectListUnmount→dispatchSetState：即一个子组件的useEffect清理函数，在同一个 commit 阶段对正在卸载的父级 context provider 调用了setState。AgentComposer的清理逻辑正是如此——它在卸载时对父级AgentViewContext调用了setAgentInputBufferText('')——这会触发 React #185 错误并导致渲染崩溃。移除该清理逻辑就消除了"卸载期间对父级 setState"的路径。缓冲文本属于内部状态（用于协调 agent tab 的行为，并非直接渲染到界面上的文本），因此去掉卸载时的强制重置不会带来功能上的副作用。Reviewer Test Plan（审阅者测试计划）
如何验证
复现（来自 issue）：当
AgentComposer在与其父级AgentViewContextprovider 处于同一个 React commit 阶段一起卸载时（例如销毁 agent 视图），会出现崩溃，报 React error #185。setAgentInputBufferText('')）。does not reset the parent input-buffer state during unmount）会渲染AgentComposer，再将其卸载，并断言卸载期间不会调用setAgentInputBufferText。验证命令（由作者执行）：
npx vitest run packages/cli/src/ui/components/agent-view/AgentComposer.test.tsxnpx vitest run packages/cli/src/ui/components/agent-viewnpx vitest run packages/cli/src/ui/components/InputPrompt.test.tsx -t "onTabConsumerChange reporting"npx eslint packages/cli/src/ui/components/agent-view/AgentComposer.tsx packages/cli/src/ui/components/agent-view/AgentComposer.test.tsx packages/cli/src/ui/contexts/AgentViewContext.tsxnpx prettier --check packages/cli/src/ui/components/agent-view/AgentComposer.tsx packages/cli/src/ui/components/agent-view/AgentComposer.test.tsx packages/cli/src/ui/contexts/AgentViewContext.tsxnpm run build --workspace=packages/corenpm run build --workspace=packages/acp-bridgenpm run typecheck --workspace=packages/cligit diff --check upstream/main...HEADEvidence（修复前后对比）
这是一个崩溃修复，而非布局/控件改动。用户可观察到的是渲染崩溃，而不是某个可见的界面元素：
agentInputBufferText是用于协调 agent tab 行为的内部状态，并不会作为文本绘制到屏幕上。AgentViewContext在同一个 commit 阶段一起卸载时，可能抛出压缩版 React error fix e2e #185 并导致渲染崩溃（见 Minified React error #185; visit https://react.dev/errors/185 for the full message or use the non-minified dev environment for full errors and additional helpful warnings. file:///E:/V-pan-FIX/C-pan/Users/nasAdmin/.cherrystudio/install/global/node #5199 中的堆栈）。setState被移除，因此该崩溃路径不复存在；并由新增的单元测试锁定该回归。未提供截图：该症状是一个由特定卸载时序的 commit 触发的、间歇性的 React 内部崩溃（error #185 的报错/堆栈），而非确定性的屏幕状态，因此无法稳定复现出有意义的前后对比截图。改为通过新增单元测试来覆盖该行为。
Tested on（已测试平台）
✅ 已测试 ·⚠️ 未测试 · N/A
Environment（环境，可选）
仅单元测试（npm workspaces）。无需特殊环境。
Risk & Scope（风险与范围）
AgentComposer以及AgentViewContext中的一处文档注释。唯一的行为变化是：composer 卸载时不再强制清空 context 中 active agent 的输入缓冲文本（它会保留最近一次同步的值）。InputPrompt.tsx（有意保留 Tab 键在windows上存在冲突，当输入框为空时，Tab会同时触发输入预测和权限模式变更，应当修改为一次键盘事件只对应一个行为 #4171 的 tab-consumer 清理逻辑）。没有无关的重构、公共 API 改动或 UI 重新设计。Linked Issues（关联 issue）
Refs #5199
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.