fix(cli): make the Agent Team teammate tab transcript scrollable in VP mode - #9531
Conversation
…P mode In Virtualized History mode (ui.useTerminalBuffer, on by default) the app owns the whole screen, so there is no terminal-native scrollback. The main conversation view renders its transcript in a ScrollableList virtual viewport, but AgentChatContent rendered the teammate tab transcript through ink's <Static> unconditionally — content that scrolled off screen could not be reached by Page Up, Page Down, arrow keys, or the mouse wheel (#9507). Route the teammate transcript through the same ScrollableList viewport in VP mode: header + committed + live items + spinner live in one viewport pinned to the tail, with Page Up/Page Down, Shift+arrows, Ctrl+Home/End and wheel/scrollbar support. The committed/live split is preserved (items from an executing/confirming tool group onward stay on the interactive render path so approval dialogs keep receiving input). The legacy <Static> path for useTerminalBuffer: false is unchanged — native terminal scrollback already works there.
|
Re-running the gate on Template looks good ✓ — all nine headings present. Problem: observed, not theoretical. #9507 is still OPEN, filed from a real session with a five-step reproduction, and the PR carries a red-before/green-after component reproduction driven through the real Direction: aligned. In VP mode the app owns the whole screen, so there is no terminal-native scrollback to fall back on; the main conversation view already renders its transcript in a scrollable virtual viewport, and this brings the teammate tab to parity with a behaviour the product has already decided on. Size: not applicable for the core gate — all 14 files sit under Approach: the core of it is what I'd have written — branch on the same Risk: no elevated risk signals — Stage 1e matched none of the revert-correlated paths. One thing the gate cannot score: this is round 15 on a PR AGENTS.md says should have converged at round 5, and the review lane has already approved and then rejected the identical commit Moving on to code review. 🔍 中文说明在 模板完整 ✓ —— 九个必需标题齐全。 问题：已观测到的真实缺陷，不是理论性加固。#9507 仍处于 OPEN 状态，来自一次真实会话并附带五步复现；PR 里给出了修复前红、修复后绿的组件级复现，走的是真实的 方向：对齐。VP 模式下应用接管整个屏幕，没有终端原生回滚可以兜底；主对话视图本来就已经把转录渲染在可滚动的虚拟视口里，本 PR 让队友标签页对齐一个产品已经确立的行为。 规模：核心模块门禁不适用 —— 14 个文件全部位于 方案：核心部分就是我会写的方案 —— 按 风险：无升级风险信号 —— Stage 1e 未命中任何与 revert 相关的路径。 有一点门禁无法评分：按 AGENTS.md，本 PR 在第 5 轮就该收敛，而这是第 15 轮，并且评审线已经在同一个提交 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI wrote my independent proposal before reading the diff: branch Where we differ is the tail. My version would have stopped at the viewport and left the mouse alone; this PR also mounts Two Criticals still stand at
|
| File | What changed |
|---|---|
packages/cli/src/ui/components/agent-view/AgentChatContent.tsx |
The fix. New VP branch routing header, committed items, live items and spinner through one ScrollableList pinned to the tail; keeps the committed/pending split, mounts the selection and mouse controllers, gates the viewport on dialogs, shell focus, pending approvals and an open menu, and adds the one-shot pull-back that R13-1 turns into a trap. |
packages/cli/src/ui/components/agent-view/AgentComposer.tsx |
Exports getAgentComposerLayoutKey over streaming state, status row, queue length and input text, syncs it to context so the footer gets re-measured, and quiets Escape plus Shift+Tab while a menu is open. Also gives the tab bar focus handler the click-away fall-through. |
packages/cli/src/ui/contexts/AgentViewContext.tsx |
Carries the new layout key through state, actions and both memo dep arrays. Pure plumbing. |
packages/cli/src/ui/AppContainer.tsx |
Adds the layout key to the controls-height measure effect deps, so a growing agent footer no longer leaves the viewport stale-high. Six lines, one of them the dep. |
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx |
Computes inputActive from isFocused and the open menu, and threads it into the keypress handler, AskUserQuestionDialog and RadioButtonSelect. Stops one level short of the nested input, which is R14-1. |
packages/cli/src/ui/components/agent-view/AgentTabBar.tsx |
Deactivates the tab-switching keypress subscriber while a menu is open, so arrows aimed at the menu cannot switch away from a teammate awaiting approval. |
packages/cli/src/ui/selection/use-text-selection.tsx |
Clears the painted highlight on controller unmount. Shared with the main view; the painted range outlives the component, so without this a tab switch re-stamps the band over cells nobody selected. |
The 7 test files (1161 lines) cover the VP scroll round trip, the layout-key measurement and propagation guards, the context-menu gates, and the selection unmount teardown.
Testing
This is an unattended CI run — I did not build or execute any PR code. The evidence below is the PR's own CI on the reviewed commit, read through the API. No tmux session was driven (that path is local-invocation only).
Nothing is red. Fourteen legs are settled and one is still running, so this is not a complete picture and I am not presenting it as one. The three skipped legs are the ones I would most want for this change: Test (macos-latest) and Test (windows-latest) are skipped, so the suite only ran on Linux for a terminal-mouse and scroll-behaviour change, and Integration Tests (CLI, No Sandbox) is skipped as well.
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | success |
| Lint & Static (ubuntu-latest, Node 22.x) | success |
| Integration Tests (no-AK, No Sandbox) | success |
| TUI parity snapshots (ink vs opentui) | success |
| OpenTUI no-flicker gate | success |
| Desktop Shell (ubuntu-22.04) | success |
| Desktop Shell (windows-2022) | success |
| Classify PR | success |
| assign | success |
| label | success |
| Remind on force-push | success |
| Test (macos-latest, Node 22.x) | skipped |
| Test (windows-latest, Node 22.x) | skipped |
| Integration Tests (CLI, No Sandbox) | skipped |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | in_progress |
Green here means the tests pass. It does not mean they pin this change, and for R13-1 they demonstrably cannot: the fix does not exist yet, so the suite is green with the defect in place.
Sandboxed verification would settle both open claims, and I'd ask for it before merging. @qwen-code /verify for R13-1 specifically — whether a wheel burst in flight when an approval lands displaces the dialog is a timing claim about a 16 ms window, and no amount of diff reading settles it; the author has write access, so this is a direct run rather than a sponsored one. @qwen-code /tmux for the headline claim — the PR's own Test Plan concedes "a live multi-teammate terminal session was not exercised on this machine", and the component-level reproduction drives a real viewport but not a real teammate, a real provider, or a real approval arriving mid-scroll. That last one is precisely R13-1's trigger.
Not verified: live multi-agent session with real teammates (author's claim is component-level only, on Linux); macOS and Windows terminals (both suite legs skipped); the second entrance to R13-1's root cause — content growing below a pinned dialog — which round 14 traced end to end but never executed, and which I have not attempted to confirm either. Treat it as unconfirmed and, per the author's own point 4, do not let it justify a larger change here.
中文说明
代码审查
在读 diff 之前我先写了自己的独立方案：让 AgentChatContent 按 MainContent 用的同一个 uiState.useTerminalBuffer 开关分支，把转录交给已有的、钉在尾部的 ScrollableList，保留已提交/进行中拆分以让确认框留在可交互路径上，让 <Static> 分支逐字节不变；并且——因为 agent footer 的高度对 AppContainer 的控件高度测量是不可见的——像 getLiveAgentPanelLayoutKey 那样同步一个布局 key。这个 PR 做的正是这些，布局 key 也包括在内。就核心问题而言，方案与我的基线一致，关于滚动修复本身我没有更简的路可提。
分歧在尾部。我的版本会止步于视口、不去动鼠标；本 PR 还挂载了 ContentMouseController 和 TextSelectionController。这说得通——不管你愿不愿意，ScrollableList 自己就会开启 SGR 鼠标跟踪，所以不挂载它们就会让这个标签页悄悄失去 OSC 8 链接点击和右键菜单。但这正是 diff 从约 90 行生产代码变成 363 行的原因，也正是两条未决 Critical 都落在本 PR 原本不触及的共享文件里的原因。
两条 Critical 在 a5444d10 上依然成立——通过读代码复核，不是照抄线程结论
我没有直接采信第 14 轮的结论，而是在本次 head 上读了被引用的代码；这两个文件都在 PR diff 之外且与 main 逐字节一致，所以引用可以直接核对。
1. AgentChatContent.tsx:391（R13-1）——排队中的滚轮 flush 仍会把可应答的对话框滚出屏幕，且无法恢复。 成立，机制与描述完全一致：use-frame-coalesced-flush.ts 只在 cancel() 和卸载 effect 两处清掉 16 ms 定时器；ScrollableList.tsx:144-148 的 cancelPendingScroll() 唯一调用点是 :164 的 left-press 分支，焦点变化不会取消任何东西；applyPendingScroll（:121-136）依赖数组为空且不读焦点状态。本 PR 给 hasFocus 加了 !approvalPending（:401-406），这确实让视口静默了，但同时也关掉了 ScrollableList.tsx:103 的按键订阅和 :187 的鼠标订阅，而 approvalScrolledRef.current === true（:378-379）让回拉只生效一次，于是没有任何东西会重新钉住尾部。
结果是：滚轮回看前面的输出时审批落在 16 ms 窗口内，回拉先执行，随后排队的 flush 触发并把尾部移出 [renderRangeStart, renderRangeEnd]。ToolConfirmationMessage 连同它的 useKeypress 订阅一起卸载，pendingApprovals 只能由 onConfirm/clearPendingApprovals() 清除且没有超时，而所有能把它滚回来的输入都已关闭。唯一剩下的恢复路径是切走标签页再切回（AgentChatView.tsx:53 用 agentId 作为 ErrorBoundary 的 key），屏幕上没有任何提示。legacy <Static> 路径上不会发生这件事，所以这是回归而不是既有缺口。
作者记录的修法（在 ScrollableList.tsx 里于 hasFocus 丢失时调用 cancelPendingScroll()）方向正确，并且同时修掉 MainContent VP 路径上同样的潜在缺口。线程点明的约束必须保留：:378-379 的闩锁在真正发生滚动之前不能置位，否则「审批到达之前就已经向上滚动」的情形会重新把用户困住。
2. ToolConfirmationMessage.tsx:624（R14-1）——菜单门控在最后一层之前停住，因此一个本意选择右键菜单项的 Enter 会提交打了一半的答案。 成立。本 PR 计算 inputActive = isFocused && contextMenu === null（:71-77）并接入三个消费者，包括 :624 传给 AskUserQuestionDialog 的 isFocused={inputActive}。在该对话框内部，:357 是 { isActive: isFocused }（已门控），但嵌套的自定义回答输入框在 AskUserQuestionDialog.tsx:564 仍被硬编码为 isActive={true}，其正上方就是 onSubmit={handleCustomInputSubmit}。KeypressContext 广播分发并丢弃返回值，所以在 ask_user_question 这一类型上，用于选择菜单项的 Enter 同时会把草稿作为答案提交，teammate 的这一轮就会基于用户从未提交的内容继续。修复是一行 isActive={isFocused}，位于本 PR 未触及的文件中。不要重新强制 cursorVisible——TextInput.tsx:190 计算 shouldRenderCursor = isActive && cursorVisible，菜单打开时光标停止闪烁正是预期的可见信号。
R14-2 已修复。 229821035c 把 AppContainer 的注释改写为 Mirrors the LiveAgentPanel layout key (#5798).，源码守卫所切片的标识符不再出现在注释文本里；新的 agentViewMemoDeps() 辅助函数从 '\n [' 切到 '\n ],'——只覆盖依赖数组，不含其上方的对象字面量。相比原先「空过」的形态这是实质改进，两条新守卫（state memo 中的 agentComposerLayoutKey、actions memo 中的 setAgentComposerLayoutKey）在各自点名的突变下都会失败。
我核对过但不提的意见
use-text-selection.tsx的卸载清理是安全的。getBuffer是useCallback(..., [stdout])（:150-155），所以useEffect(() => () => getBuffer()?.setSelection(null), [getBuffer])确实如注释所说只在卸载时执行一次。把它限定在卸载而非eventsPaused上是正确的——暂停必须保留打开的菜单所提供的 Copy Selection 所需的选区。- 布局 key 没有引入新的每键抖动。
AgentComposer.tsx:147本来就在每次按键时把buffer.text同步进 context，所以agentComposerLayoutKey以同样节奏变化不产生额外开销。 - 与 MainContent VP 路径的对齐差异——省略的
measureAtFullHeight、未接线的selectionQueryRef（因此这个标签页上不会出现 Copy Selection）、没有<OverflowProvider>包裹——都已被报告过并带理由延后在案。其中OverflowProvider根本不是缺口：agent 视图的 legacy 路径本来也没有。这些我都不再重提。
测试
这是无人值守的 CI 运行——我没有构建或执行任何 PR 代码。 下面的证据是通过 API 读到的、本 PR 自己在被审查提交上的 CI 结果。没有驱动 tmux 会话（那条路径只适用于本地调用）。
没有红灯。14 条已经出结果，1 条仍在运行，所以这不是一张完整的图，我也不把它当作完整的图来呈现。被跳过的三条恰恰是这个改动最需要的：Test (macos-latest) 和 Test (windows-latest) 都被跳过，因此一个涉及终端鼠标与滚动行为的改动只在 Linux 上跑过测试套件，Integration Tests (CLI, No Sandbox) 同样被跳过。
这里的绿灯只说明测试通过，不说明测试钉住了这个改动。对 R13-1 而言它们显然钉不住：修复还不存在，所以带着缺陷套件也是绿的。
沙箱验证可以判定这两条未决主张，我会要求在合入前先做。 R13-1 用 @qwen-code /verify——审批落在飞行中的滚轮爆发时对话框是否被位移，是一个关于 16 ms 窗口的时序主张，读多少遍 diff 都无法判定；作者有写权限，所以这是直接运行而非 sponsored run。头部主张用 @qwen-code /tmux——PR 自己的测试计划承认「本机上没有实际跑过多 teammate 的终端会话」，而组件级复现驱动的是真实视口，不是真实 teammate、真实 provider，也不是一个在滚动过程中真实到达的审批。最后那一项正是 R13-1 的触发条件。
未验证：带真实 teammate 的实时多 agent 会话（作者的主张仅为 Linux 上的组件级验证）；macOS 与 Windows 终端（两条套件 leg 均被跳过）；R13-1 根因的第二个入口——内容在已钉住的对话框下方增长——第 14 轮端到端追踪过但从未执行，我也没有尝试确认。请把它当作未确认信息，并按作者自己的第 4 点，不要用它来为这里更大的改动辩护。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at a5444d10216c7d7847360519555cf7a2b41ff07e · re-run with @qwen-code /triage
|
Confidence: 3/5 — the scroll fix itself is right and I would merge its core without hesitation; what keeps this at 3 is two Criticals I re-verified as standing at this head, and a scope-and-convergence call that is not mine to make. Stepping back. The thing I asked myself in Stage 2 was whether I had a simpler path, and for the scroll fix I don't — it branches on the flag The part I'd argue with is what the review lane made this PR become. Mounting the mouse and selection controllers was a defensible answer to The exception is what blocks me, and I want to be precise about why I'm not approving. Both standing findings are cheap: five lines between them. But they are not cosmetic.
Merging those into main and deferring them to a follow-up is the one outcome I'd push back on hardest, because AGENTS.md's five-round rule says to land only Critical fixes at this point — it narrows the PR to Criticals, it does not license shipping them. Both recipes are already written down in their threads and both are small. If the size fuse is the obstacle, my answer is that five lines in two shared files is a much smaller risk than a known wedge, and What I genuinely cannot resolve from the diff, the tests, and the PR description is the convergence question the author raised, and it is a real one rather than a complaint. The lane approved commit I am deferring rather than acting, and I have not submitted a review. The bot's On the escalation mention: I could not resolve one, so there is no @mention below and I am not guessing a login. The deterministic resolver came back empty at every step — 中文说明Confidence: 3/5 —— 滚动修复本身是对的，其核心我会毫不犹豫地合入；之所以停在 3 分，是我在本次 head 上复核后确认仍然成立的两条 Critical，以及一个不属于门禁职权范围的范围/收敛判断。 退一步看。Stage 2 里我问自己有没有更简的路，就滚动修复而言没有——它按 我会争论的是评审线把这个 PR 变成的样子。挂载鼠标与选择控制器是对「 那一处正是拦住我的地方，我想把不批准的理由说准确。两条未决发现都很便宜：加起来五行。但它们不是表面问题。
把它们合进 main 再推给后续 PR，是我最坚决反对的一种结局，因为 AGENTS.md 的五轮规则说的是此时只落地 Critical 修复——它是把 PR 收窄到 Critical，而不是允许把 Critical 推出去。两条修法都已经写在各自的线程里，而且都很小。如果障碍是行数保险丝，我的看法是：两个共享文件里的五行，比一个已知的卡死风险小得多；而 我确实无法从 diff、测试和 PR 描述中解决的，是作者提出的收敛问题，而它是真问题，不是抱怨。评审线在 我选择延后而不是行动，并且没有提交评审。 机器人第 14 轮的 关于升级提及：我无法解析出任何人，所以下面没有 @mention，我也不会去猜一个 login。 确定性解析器在每一步都返回空—— — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.14)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory; 68 tests pass — this review observed 22032 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:271 — [review] D2-1 the VP spinner path is untested — deleting the spinner spread survives the whole new suite (mutant verified); deferred by the code-age rule: anchored on code…packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:263 — [review] R1-1 still stands — item-identity rebuild defeats memo(HistoryItemDisplay); author declined in round 1 as a separate rendering-performance changepackages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:346 — [review] R1-3 still stands — the third test does not pin the head-to-tail round trip its comment promises; author declined in round 1 as test-matrix expansionpackages/cli/src/ui/components/agent-view/AgentChatContent.tsx:60 — [review] R1-4 still stands — the key-must-not-encode-pending-ness invariant has no test driving a pending-to-committed transition; author declined in round 1packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:342 — [review] R1-5 still stands — the hasFocus/isActive gate is only tested in its all-false default; author declined in round 1 as a separate test-matrix expansionpackages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:97 — [review] R1-6 still stands — createUIState factory is a near-verbatim copy of MainContent.test.tsx's; author declined in round 1 as unrelated test-infrastructure refac…
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory; 68 tests pass — this review observed 22032 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Bring in the typecheck:integration and check:tui-dep-direction scripts added on main so the CI gate steps stop failing with "Missing script". Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
AppContainer's controls-height measure effect tracked only main-conversation state, so on the agent tab the Completed/Failed status row, queued messages and input growth never triggered a re-measure: controlsHeight stayed stale-high and the transcript viewport pushed the composer and tab bar past the terminal bottom. AgentComposer now derives a layout key from its height-shifting state (streaming state, status row, queued messages, input text) and syncs it to AgentViewContext; the measure effect depends on that key, mirroring liveAgentPanelLayoutKey from #5798. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Main renamed GeminiRespondingSpinner to RespondingSpinner; the merge kept the old name at the viewport's spinner item and broke typecheck. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Main renamed GeminiRespondingSpinner to RespondingSpinner; update the teammate-tab spinner usage and its test mock so the merged tree compiles. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
CI attribution —
Triggering a failed-jobs rerun; leaving the infra attribution to maintainers. |
|
CI triage (run 33249328264, head 4da1dae): re-running failed jobs. The web-shell E2E Smoke failures are collapsed-groups-persist and split-persist — this PR touches only packages/cli agent-view/AppContainer (0 web-shell files), and the same run lists 6 flaky specs on the shared runner. Test ubuntu failure attribution (7 suites outside the diff, 60m cap) is in the previous comment; Post Coverage Comment failed in 14s (infrastructure). Triggering --failed rerun. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-6 createUIState test-factory duplication (round 3 refiled it with gemini→memory key-drift evidence) — already reported (comment 3818835361), author declined in round 1 as unrelated test-infrastructure refactoring
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:348 — [probe] D3-1 agent VP path omits measureAtFullHeight — probe shows one clipped confirmation frame that self-heals next frame; deferred by the code-age rule (anchor unchang…packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:354 — [probe] D3-2 ScrollableList/TextSelectionController wiring duplicated near-verbatim from MainContent and already drifted; deferred by the code-age rule (anchor unchanged s…packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:271 — [review] D3-3 (re-deferral of D2-1) VP spinner branch untested — deleting the spinner spread survives the suite; deferred by the code-age rule
Convergence: round 3 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
) R3-1: the provider link of the #9507 layout-key chain was the only link without a source guard. Pin agentComposerLayoutKey in the state memo deps and setAgentComposerLayoutKey in the actions memo deps of AgentViewContext.tsx by extending app-container-controls-dep.test.ts in the PR's established guard style. Mutation-verified: dropping either entry from its deps array (while the object literal keeps it) turns exactly the matching new test red, and every pre-existing test stays green. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory.
2 Suggestion(s) were drafted inline past the resolved critical posting floor — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s); the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 4, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round:
packages/cli/src/ui/app-container-controls-dep.test.ts:110 — [review] R4-2: This guard comment states the staleness direction backwards. If the dep above is dropped and the agent footer grows (status row appears, messages queue, input wraps…packages/cli/src/ui/app-container-controls-dep.test.ts:152 — [review] R4-1: This comment describes a failure mechanism the code cannot have. setAgentComposerLayoutKey is a raw useState setter (AgentViewContext.tsx:165, passed unmodified…
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory。
2 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论——发布下限因首次发现速率连续 2 轮未下降而提前生效；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 4 轮，非阻断）——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
CI attribution — Shape: job-level 1h timeout kill. Every suite that completed before the kill passed:
The runner-health probe also reported its positive control inconclusive ("the injected always-failing test did not turn the run red — a dead runner, a collector that skipped it, or a reporter that dropped the failure"), i.e. the runner itself was sick. Same sick-runner/timeout signature seen repo-wide this window (#10159, #10394, #9260 prior attempts).
Triggering a failed-jobs rerun. |
|
CI attribution — run 33255831712 latest attempts (head
|
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 117 passed · 0 failed · 117 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：117 通过 · 0 失败 · 117 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #9531 verification — fix(cli): make the Agent Team teammate tab transcript scrollable in VP modeVerdict: 中文摘要
Central claim and A/B proofCentral claim: with The PR's own harness (
Flip: 1/3 → 3/3; the single flip is exactly the central-claim test. Witness: Independent boundary harness (
Head 5/5, base 2/5 — the flips are precisely the viewport-specific probes. Witness: Mutation matrix (on head, one hunk reverted at a time, restored after)
M3 classification — redundant defence, not a defect. FindingsNo blocking findings.
Not covered
MethodologyEnvironment: CI merge-ref checkout (depth 2), Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — non-deterministic tests (flakiness gate); job cancelled before the report - workflow run Flakiness gate: ❌ 1 of 4 changed test file(s) returned different results across identical re-runs (1 full round(s)) The job was cancelled before producing a report, but the deterministic flakiness gate had already observed run-to-run divergence — the per-round matrix is in the gate step output of the workflow run log. 中文 — 判定：❌ 不通过 · 测试结果不确定（抖动门）；作业在报告前被取消抖动门：❌ 1 of 4 changed test file(s) returned different results across identical re-runs (1 full round(s)) 作业在生成报告前被取消，但确定性抖动门已观测到轮间分歧——各轮矩阵见工作流运行日志中 gate step 的输出。 — Qwen Code · sandboxed verification |
|
Sandboxed verification: ❌ not passed — findings reported (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: 57 passed · 0 failed · 57 total Flakiness gate: ✅ 7 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：57 通过 · 0 失败 · 57 总计 抖动门：✅ 7 changed test file(s) x 5 identical rounds, no divergence Verification report<!-- qwen-triage:verify --> Sandboxed verification: ❌ not passed — findings reported (agent verdict) — round 7 Scripted assertions: 57 passed · 0 failed · 57 total Verified head 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）· 第 7 轮
Verification reportPR #9531 verification (round 7) — fix(cli): make the Agent Team teammate tab transcript scrollable in VP modeVerdict:
Previous-finding status (follow-up round)Round 6 verified head
Central claim + A/BCentral claim (carried): with Central claim (carried, round 6): on that VP path a teammate's pending approval stays mounted and answerable — the tail is pulled back into view when the approval arrives ( Central claim (round 7's delta): the pre-existing #5798 deps guard is no longer neutralised by this PR's comment prose — i.e. F4 is fixed. Control construction. Single-variable by construction and sha256-verified at every swap, restored with (a) The PR's own test file as a non-vacuity A/B
The four base reds, by name this round: (b) An independent end-to-end probe, and the two-halves matrixThe PR's own approval test mocks
Each half breaks a different shape, which is what the code comment claims ("Two halves, both required") and what a two-cell A/B against base cannot separate. Witness Head-arm cells, all as expected (23 scripted expects):
(c) The F4 fix, at the destination (round 7's delta)
Arm B is the positive control landed in the same file as the mutant; arm C is the proof that the mechanism of F4 is still live in the guard's design (→ N7). Witness Vacuity check on the PR's pinned tests (re-run at the new base)One production file reverted to its
Note the contrast with round 6: the Corrections
FindingsN7 (Informational, NEW evidence on a fixed trigger) — the deps guard still slices comments, so prose can still neutralise itReproducing command (arm C of # in packages/cli/src/ui/AppContainer.tsx: delete ` liveAgentPanelLayoutKey,` (line 4058)
# AND change line 4070 to ` // (#9507). Mirrors liveAgentPanelLayoutKey.`
cd packages/cli && npx --no-install vitest run src/ui/app-container-controls-dep.test.ts
# => 8/8 GREEN with the real dependency absentCommit F2 (Suggestion, stands) — "Copy Selection" can never appear on the teammate tabRe-measured: N5 (Suggestion, stands) — the teammate transcript is completely unscrollable while any approval is pending, with no time limit, and the description does not name itRe-measured at the destination: with one N3 (Suggestion, stands on a re-proven closure) — the selection teardown runs too late to stop the successor's first frame being stampedCarried with the closure evidence in the status table (three blobs identical to round 6's quoted hashes; render-test import closure contains no production caller; N6 (Informational, stands) — inside the viewport window, the approval dialog's own prompt can be clipped awayRe-measured: at N1 (Informational, stands — now directly measured) — three tests named "VP mode:" do not discriminate the VP pathThe base arm's five survivors are named in the status table; three carry the N2 (Informational, stands, unchanged) — the PR's new test file leaks renders10 renders by my census regex (8 Not covered
MethodologyEnvironment: CI merge-ref checkout at depth 2 in a token-free Evidence images
Flakiness gate logEvidence imagesHarness 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.
Partially reviewed — gaps disclosed.
25 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- committed-item identity churn defeats memo(HistoryItemDisplay) — AgentChatContent.tsx:282 — already reported (round-1 R1-1; author declined as a separate rendering-performance change)
- renderVirtualItem's dep array rebuilds the row callback on UI-only state changes — AgentChatContent.tsx:350 — already reported (round-11 deferral D11-2)
- the VP early return orphans AppContainer's historyRemountKey static-remount effect — AgentChatContent.tsx:383 — already reported (round-7 deferral D7-4, re-listed round 13)
- committed VP items get no per-item height cap — AgentChatContent.tsx:329 — already reported (round-9 deferral)
- the dialogsVisible gate's premise is false on the agent tab — AgentChatContent.tsx:388 — already reported (round-7 deferral D7-2, re-listed round 14)
- getAgentComposerLayoutKey omits both AgentFooter height inputs — AgentComposer.tsx:309 — already reported (round-7 deferral D7-1, re-listed round 14)
- three new comments invert the controlsHeight staleness direction — app-container-controls-dep.test.ts:109 (+2 locations) — already reported (round-4 R4-2 and round-5 deferral D5-1)
- the actions-memo guard pins a failure mode React excludes — app-container-controls-dep.test.ts:146 — already reported (round-4 R4-1, re-listed round 14)
- the HistoryItemDisplay mock records 3 of 9 forwarded props — AgentChatContent.test.tsx:85 — already reported (round-14 deferral)
- the spinner VP item is never constructed because every fixture returns COMPLETED — AgentChatContent.test.tsx:303 — already reported (round-2 deferral D2-1, re-listed rounds 3, 5, 6, 9 and 13)
- the mouse-controller isActive assertion is satisfied by the pre-menu render — AgentChatContent.test.tsx:534 — already reported (round-11 deferral D11-3, re-listed round 14)
- the pending-path test never performs the round trip its comment promises — AgentChatContent.test.tsx:447 — already reported (round-1 R1-3, re-listed round 13)
- the approval-latch reset has no test — AgentChatContent.tsx:374 — already reported (round-14 deferral)
- the approval-lockout case's fixture contains no approval item — AgentChatContent.test.tsx:549 — already reported (round-14 deferral)
- two of the four hasFocus terms are unpinned and getShellPids is stubbed empty — AgentChatContent.tsx:388 — already reported (round-1 R1-5, re-listed round 13)
- the createUIState fixture carries phantom keys and omits required members — AgentChatContent.test.tsx:202 — already reported (round-1 R1-6, with the same gemini-to-memory key-drift evidence)
- the fake core's event emitter is inert, so the approval re-render wiring is untested — AgentChatContent.test.tsx:291 — already reported (round-2 deferral D2-1, re-listed round 13)
- the closeMenu() click-away fall-through branch is untested — AgentComposer.tsx:222 — already reported (round-13 deferral)
- the Down-arrow control assertion is pre-satisfied by the isInputActive mount effect — AgentComposer.test.tsx:237 — already reported (round-13 deferral, re-listed round 14)
- the documented menu dismissal is unreachable while input is inactive — AgentComposer.tsx:211 — already reported (round-13 deferral noting this branch is pinned only through a seam production cannot reach)
- …and 5 more (see the run report)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: platform matrix — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the same suites ran on Linux and locally, so the gap is platform-specific behaviour.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory.
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:194 — [probe] Critical [fails-closed] [new-surface] Ctrl+F can focus a ShellInputPrompt that scrolled out of the render window, and the new !embeddedShellFocused term then kills…packages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:97 — [probe] the AGENT_HEADER sentinel is never read by any assertion, so the header virtual item is unpinnedpackages/cli/src/ui/components/agent-view/AgentChatContent.tsx:401 — [probe] the new showScrollbar pass-through has no read-back assertion; deleting it leaves 9/9 greenpackages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:280 — [probe] both legacy-mode cases render zero pending items, so the live-area-outside-Static invariant is never exercisedpackages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:479 — [probe] the only allExpanded:false case never renders a pending row, so fullDetail forwarding on that branch is unpinnedpackages/cli/src/ui/components/agent-view/AgentChatContent.tsx:207 — [probe] the Ctrl+F subscriber's new context-menu gate has no test; dropping the term leaves 9/9 greenpackages/cli/src/ui/components/agent-view/AgentTabBar.tsx:128 — [probe] gating the whole subscription also kills the printable branch that releases tab-bar focus, and its test pins only the going-quiet direction
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 25 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：platform matrix — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the same suites ran on Linux and locally, so the gap is platform-specific behaviour.
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 7 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.24.1)
…d answer input
Two review Criticals on the teammate tab's approval surface:
- ScrollableList left a wheel/drag flush armed across a focus loss, so the
timer fired after whatever took focus had pulled the tail into view and
scrolled a pending approval dialog back out of the render window, where
its keys and this list's were already dead.
- AskUserQuestionDialog hard-wired its nested custom-answer TextInput to
isActive={true}, so an Enter aimed at an open right-click menu still
submitted the half-typed draft as the answer to the pending question.
Clear the `Check lint gate freshness` failure: main moved the lint gate in c56d604 (#12135, .github/workflows/ci.yml) after this branch last incorporated it, so the required Lint & Static lane exited 1 in ~26s without running any linter. Merging main re-validates the branch under the current gate. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmu8nqzno6q
|
巡检处理了一下这条红的
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
23 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- AgentTabBar.tsx:128 — printable-character branch loses the tab-bar focus release when the whole subscriber is gated on the open menu — already reported (round-15 deferral list, probe-confirmed)
- AgentComposer.tsx:309 — getAgentComposerLayoutKey omits both AgentFooter height inputs — already reported (round-7 deferral D7-1, re-listed round 14)
- AgentChatContent.test.tsx:447 — the pending-path test never performs the round trip its comment promises — already reported (round-1 R1-3, re-listed round 13)
- AgentChatContent.tsx:207 — the Ctrl+F subscriber's new context-menu gate has no test — already reported (round-15 deferral list, probe-confirmed)
- AgentChatContent.tsx:389 — two of the four hasFocus terms are unpinned and getShellPids is stubbed empty — already reported (round-1 R1-5, re-listed round 13)
- AgentComposer.tsx:222 — the closeMenu() click-away fall-through branch is untested — already reported (round-13 deferral)
- app-container-controls-dep.test.ts:151 — the actions-memo guard pins a failure mode React excludes — already reported (round-4 R4-1, re-listed round 14)
- app-container-controls-dep.test.ts:109 — three new comments invert the controlsHeight staleness direction — already reported (round-4 R4-2 and round-5 deferral D5-1)
- AgentChatContent.test.tsx:534 — the mouse-controller isActive assertion is satisfied by the pre-menu render — already reported (round-11 deferral D11-3, re-listed round 14)
- AgentChatContent.test.tsx:85 — the HistoryItemDisplay mock records 3 of 9 forwarded props — already reported (round-14 deferral)
- AgentChatContent.tsx:100 — the VP early return orphans AppContainer's historyRemountKey static-remount effect — already reported (round-7 deferral D7-4, re-listed round 13)
- AgentChatContent.test.tsx:561 — the approval-lockout case's fixture contains no approval item — already reported (round-14 deferral)
- AgentChatContent.tsx:282 — committed-item identity churn defeats memo(HistoryItemDisplay) — already reported (round-1 R1-1; author declined as a separate rendering-performance change)
- AgentChatContent.tsx:388 — the dialogsVisible gate's premise is false on the agent tab — already reported (round-7 deferral D7-2, re-listed round 14)
- AgentComposer.test.tsx:224 — the Down-arrow control assertion is pre-satisfied by the isInputActive mount effect — already reported (round-13 deferral, re-listed round 14)
- AgentChatContent.test.tsx:287 — the approval-latch reset has no test — already reported (round-14 deferral)
- AgentChatContent.test.tsx:303 — the spinner VP item is never constructed because every fixture returns COMPLETED — already reported (round-2 deferral D2-1, re-listed rounds 3, 5, 6, 9 and 13)
- AgentChatContent.tsx:403 — TextSelectionController mounted without getAdditionalSelectableRects — already reported (round-3 deferral D3-2, re-listed R5-3, R6-2 and R9-5)
- AgentComposer.tsx:209 — the click-away fall-through assumes the tab bar releases focus on the same keypress — already reported (round-15 deferral at AgentTabBar.tsx:128)
- AgentChatContent.test.tsx:479 — the only allExpanded:false case never renders a pending row — already reported (round-15 deferral list, probe-confirmed)
- …and 3 more (see the run report)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": did not run an ink render of AgentFooter at a narrow width (40-60 columns) to measure the actual wrap threshold and confirm the 1-row growth empirically..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory; 68 tests pass — this review observed 32224 passed.
Deferred under the convergence posture (round 16, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/selection/use-text-selection.tsx:494 — [probe] no test pins that the unmount teardown clears the paint while eventsPaused is true; the conditional variant survives 42/42packages/cli/src/ui/components/agent-view/agentComposerLayout.measurement.test.tsx:26 — [probe] the header's stated reason for not testing the real AppContainer is measurably false, leaving the #9507 measure path with no runtime testpackages/cli/src/ui/components/agent-view/AgentTabBar.test.tsx:246 — [probe] the menu gates' close direction is unpinned; a one-way-latch rework survives 289 shipped testspackages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:24 — [probe] no fixture renders an item after splitIndex, so the "onward" half of the split contract is unfalsifiable as shippedpackages/cli/src/ui/app-container-controls-dep.test.ts:119 — [probe] the composer-key guard's regex is unanchored to the live assignment, unlike its sibling liveAgentPanelLayoutKey guard three tests up
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 23 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 4)"：did not run an ink render of AgentFooter at a narrow width (40-60 columns) to measure the actual wrap threshold and confirm the 1-row growth empirically.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：src/ui/components/agent-view/AgentChatContent.test.tsx — no such file or directory; src/ui/components/shared/ScrollableList.test.tsx — no such file or directory; 68 tests pass — this review observed 32224 passed。
收敛姿态下延后（第 16 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.24.1)
|
Latest authorized follow-up: R15-1 addressed in Validation: 199 tests across 13 files passed (agent-view, context-menu, useKeypress and ToolConfirmationMessage), plus focused ESLint and diff checks. The regression sends right-press + Shift+Tab + Right + Return in one stdin write, checks that no underlying handler or approval runs, then verifies menu activation and approval after dismissal. Empty-item right-clicks remain pass-through. No full build or live-terminal session was run. Scope: this explicitly authorized round changes 2 implementation files (+26/-4) and 1 test file (+85/-1); no dependencies or packages added. Baseline → current: implementation 1 file (+136/-3) → 11 (+392/-18); tests 1 (+338/-0) → 8 (+1357/-5). Historical growth and deferred suggestions remain recorded below. This pass does not claim to complete the broader input-arbitration work in #11228. CI follow-up: the first post-push Lint & Static job stopped at check-lint-gate-freshness because main had updated ci.yml (97cd563). Merged main without conflicts in 0f8f90a; the missing gate commit is now an ancestor. Re-ran the 13 focused test files (199 passing), focused ESLint, formatting and diff checks after the merge. Replacement CI is pending; no full build or live-terminal verification was run locally. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Scope: full diff reviewed (~2100 lines across 14 files). Cross-file callers of changed surface checked. NOT reviewed: macOS/Windows terminal behaviour (no host); live multi-teammate session (no sandbox); CI legs are outside this review's verdict.
No blocking findings.
Approval blockers: none.
What I checked
- VP mode rendering path (
AgentChatContent):virtualItems/renderVirtualItem/agentVpIsStatic/agentVpKeyExtractoragainst theScrollableListcontract; committed/pending split boundary;initialScrollIndexpinned toSCROLL_TO_ITEM_END. - Approval pending guard: two-halves sequence (scroll-to-tail first, then take focus) via
approvalScrolledRef; reset onapprovalPending === false. - Layout key chain:
getAgentComposerLayoutKey→setAgentComposerLayoutKey→AgentViewContextmemo deps →AppContainermeasurement effect deps — end-to-end. - Context menu gating:
useKeypress.tssame-tick guard (menu === null && isMenuOpen());ContextMenuContext.openMenusynchronous ref update beforesetMenu; gate propagation toAgentComposer,AgentTabBar,ToolConfirmationMessage,AskUserQuestionDialog. ScrollableListfocus-loss flush cancel:useEffectonhasFocusdispatchescancelPendingScrollbefore a pending burst can land after focus is taken.TextSelectionControllerunmount teardown:getBuffer()?.setSelection(null)— correctly clears the painted range on the per-stdout frame controller so a successor view doesn't inherit a stale highlight.AskUserQuestionDialogisActive={true}→isActive={isFocused}: change is sound; the nested input is now correctly gated along with the rest of the dialog.- Test validity: VP / legacy / approval / context-menu test paths;
agentComposerLayout.measurement.test.tsx(before/after re-measure);app-container-controls-dep.test.ts(source-level dep guard). One question below. - Cross-check: all prior criticals (R13–R15) are addressed at this head. The question below was not previously filed.
Question (unsettled — minor)
See inline comment at ToolConfirmationMessage.test.tsx line 1429.
Reviewed with AI assistance.
| { x: 0, y: 0 }, | ||
| ); | ||
| // AppContainer is outside the provider and relies on this callback. | ||
| expect(onMenuChange).toHaveBeenLastCalledWith(hasItems); |
There was a problem hiding this comment.
Question (minor): When hasItems === false, openMenu([], …) returns early without calling onMenuChange. At the point this assertion runs, onMenuChange has never been called — does toHaveBeenLastCalledWith(false) on a never-called mock pass in your vitest version, or would this fail?
If it fails, the assertion for the hasItems === false branch should be expect(onMenuChange).not.toHaveBeenCalled() (capturing the intent that an empty-items openMenu does NOT fire the app-level callback).
There was a problem hiding this comment.
Checked on 0f8f90ae70. At the moment that assertion runs, onMenuChange has already been called once with false, so toHaveBeenLastCalledWith(false) passes for a real reason rather than vacuously.
ContextMenuProvider has a mount effect — onMenuChange?.(menu !== null) at ContextMenuContext.tsx:98-100 — which fires on mount with menu === null, before any mouse event. Measured onMenuChange.mock.calls at the assertion: [[false],[true]] when hasItems is true, [[false]] when false. So the suggested expect(onMenuChange).not.toHaveBeenCalled() fails here: expected "spy" to not be called at all, but actually been called 1 times (Number of calls: 1).
The assertion is load-bearing. Mutating only the empty-items branch (ContextMenuContext.tsx:80) to fire onMenuChange?.(true) while keeping the early return — nothing else in the test changes — is caught by exactly this line: AssertionError: expected last "spy" call to have been called with [ false ], exit code 1.
Your instinct that something is off here is right, just not where the question pointed. Because the assertion runs inside a subscribed mouse handler, its failure escapes through KeypressContext.handleKeypress (KeypressContext.tsx:832) into the stdin stream's async emit, so vitest files it under "Unhandled Errors" (Errors 1 error, exit 1) while the case itself still prints a pass — vitest even warns "This might cause false positive tests". CI does go red, so the guardrail works; the failure is just not attributed to the case. Capturing it in the handler and rethrowing after the act() would fix the attribution (~5 lines, this test file only). I have not pushed that: review is at round 16 with the last round explicitly bounded to one, and a push now would invalidate the review run currently in flight on this head. Say the word if you want it in scope.
It has to stay inside the handler either way — it pins the synchronous mirror update within one stdin chunk, which is the point of R15-1. After the chunk the menu is already dismissed, so the last call is false in both branches and the assertion would stop discriminating.
中文
在 0f8f90ae70 上实测：断言执行时 onMenuChange 已被调用过一次（参数 false），所以 toHaveBeenLastCalledWith(false) 是真过而非空过。
原因是 ContextMenuProvider 的 mount effect（ContextMenuContext.tsx:98-100）挂载时就以 menu === null 调了一次 onMenuChange(false)，早于任何鼠标事件。实测 onMenuChange.mock.calls：hasItems=true → [[false],[true]]，hasItems=false → [[false]]。因此建议的 not.toHaveBeenCalled() 会失败（实际调用 1 次）。
这行断言有判别力：只把空 items 分支（ContextMenuContext.tsx:80）改成 onMenuChange?.(true)、保留 early return，其它都不动，就会被这一行抓到，退出码 1。
真正的小问题在归因：断言在订阅的鼠标 handler 内抛出，异常经 KeypressContext.handleKeypress（:832）落到 stdin 流的异步 emit，vitest 归到 "Unhandled Errors"（Errors 1 error、退出码 1），用例本身仍显示通过，vitest 自己也警告 "This might cause false positive tests"。CI 会红，护栏有效，只是失败没归到用例上。修法是 handler 内捕获、act() 之后重抛（约 5 行，只动这个测试文件）。本轮没推：评审已到第 16 轮、上一轮被明确限定为"仅一轮"，且现在推送会让当前 head 上正在跑的 review run 作废。需要纳入范围的话说一声。
无论如何断言都得留在 handler 里——它钉的是同一次 stdin chunk 内的同步镜像更新，这正是 R15-1 的要点；chunk 结束后菜单已关闭，两个分支的最后一次调用都是 false，断言就失去判别力了。
There was a problem hiding this comment.
The provider calls onMenuChange(false) from its mount effect before the test writes stdin. The empty-items openMenu adds no notification, so the last call remains that initial false. Both parameterized cases pass in the focused run (Vitest 3.2.7); changing this to not.toHaveBeenCalled() would reject the expected mount notification.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Picks up the current main so the lint-gate freshness check (step 8 of Lint & Static) passes again; no conflicts. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Scope
Tier: Standard — UI bug fix + contained feature (VP scroll for agent tabs). 19 files, +1627 −20.
Reviewed: all 19 changed files in the diff. Cross-file context: ScrollableList imperative ref shape (from diff context), AgentViewContext state/actions memo deps, getAgentComposerLayoutKey key contract, ContextMenuContext openMenu/closeMenu ref protocol, TextSelectionController unmount path.
Not reviewed: working tree unavailable — execution rung (Standard tier, rung 2) not run locally. Existing review history (12 rounds, qwen-code-ci-bot) documents 27/27 targeted test passes for this diff's test files.
Findings
No confirmed blockers.
R1-1 minor — confirmed · packages/cli/src/ui/components/messages/AskUserQuestionDialog.tsx:564
The isActive={true} → isActive={isFocused} fix is a genuine standalone bug: the custom-input field in the dialog was always active regardless of focus, accepting keystrokes from unrelated consumers. The fix is correct.
R1-2 minor — supported · packages/cli/src/ui/hooks/useKeypress.ts:26–29
The race guard if (menu === null && isMenuOpen()) return correctly handles the case where a right-click and a keypress share one stdin chunk: openMenu writes menuRef.current synchronously before setMenu, so isMenuOpen() returns true while React state still reads null. Guard fires only in that gap; once state propagates both sides agree and the guard is a no-op. The onMenuChange?.(true) ordering in ContextMenuContext.openMenu makes this safe.
R1-3 minor — confirmed · packages/cli/src/ui/selection/use-text-selection.tsx:495
useEffect(() => () => getBuffer()?.setSelection(null), [getBuffer]) correctly clears the painted selection on unmount. Without this, switching away from a teammate tab left the highlight band over whatever cells the successor tab rendered at those coordinates; no successor could repair it because clearSelection() early-returns on an empty own-selection. Correct fix.
Cross-check against existing reviews
Existing rounds (1–12, qwen-code-ci-bot):
-
D10-1 (Critical, deferred, fails-closed, new-surface) — "VP branch windows the pending region, so scrolling up unmounts a teammate's tool-approval UI." The current code addresses this with a two-part fix:
approvalScrolledRef+scrollRef.current?.scrollToEnd()whenapprovalPendingbecomes true, followed byhasFocus=falseto disable scroll keys. The bot classified this as "no wrong result certified" after 12 rounds including runtime probes. Cannot rule — would require running the approval+scroll interaction in a real terminal. Disclosed below. -
X12-1 measureAtFullHeight omitted from VP ScrollableList — downgraded from Critical to Suggestion by round-11 probe ("clip self-heals on the next frame"). Confirmed non-blocking.
-
X12-2 allItems rebuilt every tick — author declined.
keyExtractoruses stableitem.id, so key stability is maintained; the wrapper allocation is a performance cost, not a correctness bug. Confirmed non-blocking. -
X12-3 historyRemountKey's last VP-mode consumer removed —
historyRemountKeyis still destructured but unused in the VP path; two AppContainer comments referencing it may be stale. Documentation-only concern, no behavioral impact. Confirmed non-blocking. -
X12-4 through X12-6 (test coverage concerns, author declined) — excluded per taxonomy: no named reachable wrong behaviour, no named defect the gap hides.
-
X12-7 getAgentComposerLayoutKey omits height-shifting inputs — at the current head the function covers
streamingState,statusLabel,queuedMessageCount,inputText; the measurement test (agentComposerLayout.measurement.test.tsx) exercises all four and the comment inAgentComposer.tsxnames those four as the height-shifting parts of the footer. Cannot identify a missing input from the diff. Confirmed non-blocking at current head. -
D11-1 menu quiescence duplicated — duplication has no named cost per the exclusion list. Dropped.
-
D11-2 renderVirtualItem deps include availableTerminalHeight for all items — committed items don't use
availableTerminalHeightbut the callback memo still lists it, so any terminal resize re-generates the callback for committed items. Performance concern only, not a correctness bug. Confirmed non-blocking. -
R12-1 (test): AgentChatContent.test.tsx mounts eight ink trees without unmounting — stdout resize listeners accumulate past Node's MaxListeners of 10. Test hygiene issue, no production impact.
Needs a human
D10-1 (unsettled): The approval-pending scroll protection (approvalScrolledRef + scrollToEnd() before disabling hasFocus) relies on the imperative scrollRef.current?.scrollToEnd() call executing AFTER ScrollableList's hasFocus-loss effect cancels any pending scheduled scroll. Whether scrollToEnd() bypasses the pending-scroll queue or goes through it determines whether a user who had scrolled up before an approval arrives will see the dialog scrolled into view. Could not confirm without running the approval + scroll interaction locally. The bot's 12 rounds of testing found no repro but also no proof it works.
Approval blockers
- Execution rung not run — working tree unavailable; Standard tier requires rungs through 2. Execution evidence for D10-1 could not be produced.
Verdict
No confirmed blockers. Approval withheld: execution rung required by Standard tier could not be run (no working tree); D10-1 remains an unsettled potential interaction between scrollToEnd() and the pending-scroll cancellation on hasFocus loss.
The overall approach is sound: the composerLayoutKey propagation closes the height-measurement loop correctly, context-menu quiescence is applied consistently across all key subscribers, and the selection unmount teardown is a correct resource-lifecycle fix. The tests in agentComposerLayout.measurement.test.tsx are the strongest piece of this PR — they reproduce the root cause with real ink rendering and prove both the buggy and fixed states.
Reviewed with AI assistance.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at 5d9425b2, base 64c04538. The head moved twice while this review was in flight; both moves were merges of main, and none of AgentComposer.tsx, ContextMenuContext.tsx, useKeypress.ts or AppContainer.tsx changed between them, so every line reference below was re-read at 5d9425b2 rather than carried over.
Verdict: COMMENT — the blocking finding's provider half is fixed and I verified that in code; I could not confirm its consumer half within budget, and under the standard applied here an unconfirmed historical blocker is not an approve. No new Critical of my own.
What is fixed, verified at this head
The round-16 Critical was that every context-menu gate the diff adds reads the render-time contextMenu snapshot while openMenu wrote only React state and never the provider's synchronous mirror — so in the open direction each gate sat a full render behind the event that opened the menu, while closeMenu and executeIndex already nulled menuRef.current synchronously for exactly this reason.
openMenu in ContextMenuContext.tsx now does the symmetric thing:
if (items.length === 0) return;
menuRef.current = { items, position };
// The app-level key handler sits outside this provider.
onMenuChange?.(true);
setMenu(menuRef.current);
setSelectedIndexState(0);The mirror is written before the state update, isMenuOpen() is exposed on the context as a synchronous predicate reading that mirror, and onMenuChange fires immediately rather than waiting for the effect at line 99. AppContainer.tsx wires it to handleContextMenuChange. So the provider now offers precisely what the finding said was missing: a way to ask whether the menu is open without waiting for a render.
What I could not confirm
The two gates the finding named as consequential are still reading the render-time snapshot rather than that new predicate. In AgentComposer.tsx:
- line 142, the Escape subscriber:
isActive: streamingState === StreamingState.Responding && !agentShellFocused && contextMenu === null - line 171, the Shift+Tab subscriber:
isActive: !agentShellFocused && contextMenu === null, with a comment stating that this one "writes straight through to the agent runtime's tool-scheduling policy, so a Shift+Tab aimed at the menu would silently change the approval mode of the very teammate being decided about"
Both take contextMenu from useContextMenu(), which returns the state value, and neither handler body re-checks isMenuOpen() at dispatch time. useKeypress applies isActive through a useEffect keyed on it (lines 40-49), keeping only the handler itself in a ref — so a change to isActive takes effect after a render commits, which is the timing the original finding measured against a single coalesced stdin chunk.
The open question I ran out of budget on is whether handleContextMenuChange in AppContainer.tsx closes that path from above — for example by quieting a handler that would otherwise dispatch into these subscribers. I did not trace it, and I am not asserting either answer. What I can say is that the specific mechanism the finding described is still visible at these two call sites, that the consequences it measured were an approval-mode advance on a teammate awaiting approval, a Return approving a tool call the user never answered, Ctrl+C denying it and Escape cancelling the round, and that the thread being marked resolved does not by itself settle it.
The shape that would close this without depending on the app-level wiring is available now and is small: have those two subscribers consult isMenuOpen() at dispatch time — inside the handler, or through a ref-based predicate — instead of relying on isActive flipping after a render. The provider already exposes the synchronous answer, which is what made this hard before this commit.
Other state
One thread is unresolved and it is not a Critical: a question about whether expect(onMenuChange).toHaveBeenLastCalledWith(false) passes on a never-called mock in ToolConfirmationMessage.test.tsx:1429, where the empty-items branch returns early without invoking the callback. If it does not pass, the assertion for that branch should be not.toHaveBeenCalled(). Either way it is a test-intent point, and it is worth settling because an assertion that passes vacuously would not catch the branch it claims to cover.
reviewDecision still reads CHANGES_REQUESTED from the round-16 review at 397f3987, several heads back, and a maintainer's own review at 0f8f90ae was dismissed — so the decision state does not describe this head and needs a dismissal or re-request.
CI at this head is 11 passed, 19 skipped and 4 pending, with nothing failing. The pending set includes Test (ubuntu-latest, Node 22.x) and Lint & Static, and the first of those is the lane that would run the roughly 1,400 lines of new tests in this diff, so there is no executed signal for them yet. I did not wait on it.
|
@chiga0 @qqqys — offering this as the missing execution rung for D10-1. Answer: yes — a user who has scrolled up DOES get the approval tail pulled back into view. The 1. An existing test already pins this interaction
Crucially, the 2. That test has teeth (mutation)Commenting out 3. The actual effect ordering (measured, not reasoned)Temporary So the ordering chiga0 flagged as the risk is real: child cancel fires first, parent 4. Why the order can't matter
So the cancel has nothing to cancel on the All instrumentation was reverted; no commits, no pushes, no product-code change: What I could not establish
Not asking for an approval call here — just closing the "could not be produced" gap on D10-1's consumer half. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-reviewed at 5d9425b2 (delta since 0f8f90ae, which chiga0 approved): one new production commit da358660 plus a merge of main.
da358660 (preserve keys after input dismisses context menu) is small and correct: the inputDismissedMenuRef guard makes only an input-side dismissal release subsequent same-chunk keys, and the new test pins both directions — a fresh menu still owns Enter, and the Enter that executes a menu item cannot also submit the draft. No P0/P1.
Current state for the record: CI green at this head (15 pass, 0 fail, 1 pending = review lane); the only unresolved thread is chiga0's minor question on ToolConfirmationMessage.test.tsx:1429, which was answered with measured evidence (onMenuChange fires false from the provider's mount effect, so toHaveBeenLastCalledWith(false) passes for a real reason and not.toHaveBeenCalled() would fail) — it is his to resolve. His approve at 0f8f90ae was auto-dismissed by the push, so this PR needs one fresh non-author maintainer approve at 5d9425b2 to unblock.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Risk-driven review — PR #9531 (round 2)
Tier: Standard — UI bug fix, contained feature (VP scroll for agent tabs, context-menu quiescence, selection teardown)
Head reviewed: 5d9425b291738794f78c850fb3d66b83af988b8a
Base: 64c04538243ac0097f2219e865858cf70f5b03b8
Round-1 findings (R1-1 AskUserQuestionDialog.isActive, R1-2 useKeypress race guard, R1-3 TextSelectionController unmount teardown) and the prior execution-gap disclosure are already on record at this head (review 5314796664). This round resolves both items that blocked round-1 approval and integrates the qqqys cross-check.
Round-2 resolutions
D10-1 resolved — scrollToEnd() is independent of the pending-scroll queue
The round-1 unsettled item was: "does scrollToEnd() go through the pending-scroll queue, and can cancelPendingScroll() suppress it?"
scrollRef.current?.scrollToEnd() delegates through the useImperativeHandle chain to VirtualizedList.scrollToEnd() — a direct scroll-position update. The pending-scroll queue (pendingWheelDelta + pendingDragRow) is managed exclusively by handleMouseEvent and drained by useFrameCoalescedFlush; scrollToEnd() neither reads nor touches either ref. When approvalPending becomes true in one render cycle:
ScrollableList.useEffect([hasFocus])(child, fires first) cancels any queued wheel/drag intent viacancelPendingScroll().AgentChatContent.useEffect([approvalPending])(parent, fires second) callsscrollRef.current?.scrollToEnd()directly.
Step 2 arrives after step 1 and is unaffected by it. The dialog is brought into view regardless of prior scroll state.
qqqys Critical resolved — useKeypress.ts guard covers all subscribers
The concern was that the Shift+Tab approval-mode subscriber in AgentComposer reads contextMenu === null (render-time state) for isActive and so could misfire on a coalesced right-click + Shift+Tab stdin chunk before the render commits.
The useKeypress.ts change in this PR wraps EVERY subscriber's handler in onKeypressRef.current, which checks if (menu === null && isMenuOpen()) return before dispatching. isMenuOpen() reads menuRef.current synchronously — no render needed. Because ContextMenuContext.openMenu now writes menuRef.current before calling setMenu (the fix confirmed in both reviews), isMenuOpen() returns true the moment the menu opens, even before React state propagates. The guard therefore fires in the coalesced-stdin window and suppresses the Shift+Tab before it reaches the approval-mode subscriber. The isActive render-lag is present but irrelevant: the dispatch-time guard in the hook's wrapper layer is what closes the gap.
Minor note (from qqqys — test hygiene): ToolConfirmationMessage.test.tsx:1429 asserts expect(onMenuChange).toHaveBeenLastCalledWith(false) on the empty-items early-return path, where the callback is never called. If the assertion passes today it is vacuous (no last call = anything-or-nothing depending on jest mock state). Recommend replacing with expect(onMenuChange).not.toHaveBeenCalled() to make intent explicit. Non-blocking.
Verdict
Approve. No confirmed blocker. Both round-1 unsettled items are settled by code analysis at this head: scrollToEnd() is queue-independent, and the useKeypress.ts dispatch-time guard covers the approval-mode subscriber. The three round-1 Minors are correct confirmations of good fixes; none blocks. The known limitation — scroll locked during approval-pending — is explicitly documented in the PR description and is an accepted tradeoff.
Reviewed with AI assistance.
pomelo-nwu
left a comment
pomelo-nwu
left a comment
There was a problem hiding this comment.
Reviewed the current head. The teammate transcript now uses one virtual viewport in terminal-buffer mode, retains the legacy Static path, and keeps pending approval dialogs mounted at the tail while suspending viewport keys. Mouse selection, context-menu focus, and composer sizing have focused coverage; available checks are green. APPROVE. Minor follow-up: in the new hasItems === false test case, openMenu([]) returns before onMenuChange, so toHaveBeenLastCalledWith(false) in the mouse handler is not a useful assertion; please adjust that test when convenient.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
13 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- AgentTabBar.tsx:128 — printable-character branch loses the tab-bar focus release when the whole subscriber is gated on the open menu — already reported (round-15 deferral list, probe-confirmed; re-listed round 16)
- AgentChatContent.test.tsx:447 — the pending-path test never performs the head-to-tail round trip its comment promises — already reported (round-1 R1-3; re-listed rounds 13 and 16)
- AgentChatContent.tsx:207 — the Ctrl+F subscriber's new context-menu gate has no test — already reported (round-15 deferral list, probe-confirmed; re-listed round 16)
- AgentChatContent.test.tsx:287 — the approval-latch reset has no test — already reported (round-14 deferral; re-listed round 16)
- AgentChatContent.tsx:60 — the key-must-not-encode-pending-ness invariant has no transition test — already reported (round-1 R1-4, author declined; re-listed rounds 9 and 13)
- app-container-controls-dep.test.ts:151 — the actions-memo guard's stated rationale pins a failure mode React excludes — already reported (round-4 R4-1; re-listed rounds 14 and 16)
- AgentChatContent.test.tsx:303 — every fixture returns COMPLETED so the VP spinner item is never constructed — already reported (round-2 deferral D2-1; re-listed rounds 3, 5, 6, 9, 13 and 16)
- AgentChatContent.test.tsx:549 — the approval-lockout fixture registers an approval with no matching tool_call so no dialog ever mounts — already reported (round-14 deferral; re-listed round 16 at :561)
- agentComposerLayout.measurement.test.tsx:26 — the header's stated reason for not exercising the real AppContainer is measurably false — already reported (round-16 deferral list)
- AgentComposer.tsx:228 — the dismissal block's closeMenu() side effect has no test witness — already reported (round-13 deferral; re-listed round 16 at :222)
- AgentChatContent.test.tsx:291 — the fixture's event emitter is inert so the agent-event channel is never driven — already reported (round-2 deferral D2-1; re-listed rounds 3, 5, 6 and 9)
- AgentChatContent.tsx:398 — measureAtFullHeight omitted from the agent VP ScrollableList — already reported (round-3 deferral D3-1; re-listed rounds 5, 6, 10, 11 and 13; round-11 probe settled its severity)
- AgentChatContent.tsx:403 — neither selection controller receives a selectionQueryRef, so Copy Selection can never appear on this tab — already reported (rounds 10 and 11 duplicate lists, round-13 deferral D13-13; author answered on the R9-2…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; only the Linux unit run was covered locally.
Not explored to full depth (tool budget reached): chunk 7: none — no check was cut short.; "agent reverse-audit (round 4)": did not execute the three new ToolConfirmationMessage cases or the new ScrollableList case under mutation (drop inputActive / drop the focus-loss effect) …; "agent reverse-audit (round 1)": I did not execute AgentChatContent.test.tsx to observe the two findings red/green — both are argued from source ( ink/build/ink.js:341-348 , ink-testing-libr….
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): 68 tests pass — this review observed 33851, 595 passed.
Deferred under the convergence posture (round 17, not a blocker) — recorded, not requested in this round; 2 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:386 — [probe] Critical [fails-closed] [new-surface] A pending teammate approval taller than the viewport is…packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:194 — [probe] Critical [fails-closed] [new-surface] Ctrl+F while scrolled up focuses a shell row that is out…packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:388 — [probe] The teammate tab still does not scroll on a bare Up arrow,…packages/cli/src/ui/context-menu/ContextMenuContext.tsx:81 — [probe] openMenu notifies the out-of-provider mirror synchronously…packages/cli/src/ui/hooks/useKeypress.ts:26 — [probe] useKeypress now consumes the whole menu context, whose…packages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:317 — [probe] The nullable menuApi probe with optional chaining at both…packages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:528 — [probe] Every menu gate this diff adds on the teammate tab is…packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx:63 — [probe] The module-level menu probe is never reset, its guard is…packages/cli/src/ui/app-container-controls-dep.test.ts:111 — [review] The five new guards never pin that AgentComposer renders…packages/cli/src/ui/components/agent-view/AgentChatContent.test.tsx:276 — [probe] The fixture pins showScrollbar false, the opposite of the…packages/cli/src/ui/components/agent-view/AgentChatContent.tsx:332 — [probe] The agent VP path renders committed items with no height…
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 13 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; only the Linux unit run was covered locally.
未探索到全部深度（达到工具调用预算）：chunk 7：none — no check was cut short.；"agent reverse-audit (round 4)"：did not execute the three new ToolConfirmationMessage cases or the new ScrollableList case under mutation (drop inputActive / drop the focus-loss effect) …；"agent reverse-audit (round 1)"：I did not execute AgentChatContent.test.tsx to observe the two findings red/green — both are argued from source ( ink/build/ink.js:341-348 , ink-testing-libr…。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：68 tests pass — this review observed 33851, 595 passed。
收敛姿态下延后（第 17 轮，非阻断）——已记录，本轮不要求修改；其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 11 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.24.5)
|
Round at 1. @qqqys — your open question on the round-16 Critical's consumer half: the path is closed, and it closes inside
|
|
Released in v0.24.6. |
5 participants
What this PR does
When Virtualized History mode is on (
ui.useTerminalBuffer, the default), the teammate tab transcript is now rendered through the sameScrollableListvirtual viewport the main conversation view uses: header, committed items, live items, and the running spinner all live in one viewport pinned to the tail, so Page Up / Page Down, Shift+arrows, Ctrl+Home/End, and the mouse wheel can reach earlier output. The committed/live split is preserved — items from an executing/confirming tool group onward stay on the interactive render path, so teammate approval dialogs keep receiving input inside the viewport. WithuseTerminalBuffer: falsenothing changes: the transcript keeps flowing through<Static>into the terminal's native scrollback, which already worked there.Why it's needed
In VP mode the interactive UI takes over the whole terminal screen, so there is no terminal-native scrollback to fall back on.
AgentChatContentrendered the teammate tab transcript with ink's<Static>unconditionally, so once a teammate's output scrolled past the visible area it could not be retrieved by any means — Up arrow, Page Up/Page Down, and the mouse wheel all had no effect (#9507). The main conversation view does not have this problem because its transcript already renders in a scrollable virtual viewport; this brings the teammate tab to parity.Reviewer Test Plan
How to verify
Component-level reproduction and fix verification use the real
ScrollableList/VirtualizedListviewport machinery plus the real keypress pipeline (no viewport mocks). Both commands below are run frompackages/cli, so the paths are relative to that package:cd packages/cli && npx vitest run src/ui/components/agent-view/AgentChatContent.test.tsx. Before this PR, the VP-mode test fails because the renderer emits all 40 transcript lines at once and Page Up is a no-op; after this PR, the viewport starts pinned to the tail, Page Up reveals earlier output, the tail leaves the visible window, and the legacy (useTerminalBuffer: false) path still emits everything unchanged. Full agent-view regression (those two targets only, not the repo-wide suite):cd packages/cli && npx vitest run src/ui/components/agent-view/ src/ui/components/shared/ScrollableList.test.tsx— 68 tests pass.tsc --noEmitandeslintreport nothing new for the changed files (the remaining repo-wide tsc errors are pre-existing on pristinemainin this environment:acpAgent.ts,daemon-status.ts,workspace-extensions-controller.ts,native-audio-recorder.ts). Manual check: enable Agent Team (QWEN_CODE_ENABLE_AGENT_TEAM=1), create a team, have a teammate produce more output than fits on one screen, switch to its tab, and scroll back with Page Up.Evidence (Before & After)
Component-level frames from the reproduction test (VP mode, 40 transcript lines, viewport height 8):
Before (old always-
<Static>renderer — every line emitted, Page Up does nothing):After (viewport pinned to the tail on open):
After Page Up × N (earlier output is back in view, tail left the window):
Tested on
Linux ✅ is component-level verification (real viewport + keypress pipeline driven through ink-testing-library, red-before/green-after); a live multi-teammate terminal session was not exercised on this machine.
Environment (optional)
Node v24,
npx vitest runinpackages/clion Linux; no sandbox.Risk & Scope
<Static>— scroll behavior matches the main view, but any viewport-specific rendering quirks (height estimation, scrollbar) now apply to teammate transcripts too; the executing/confirming split keeps approval dialogs on the interactive path.ScrollableListgates both its keypress subscriber anduseMouseEventsonhasFocus, andAgentChatContentfeeds!approvalPendinginto that flag. That is intentional rather than an oversight: the pendingToolConfirmationMessageis rendered as a tail item inside the windowed viewport, so scrolling it out of[renderRangeStart, renderRangeEnd]would unmount the dialog together with itsuseKeypresssubscription, blocking the agent round with nothing on screen left to answer. The viewport is therefore scrolled to the tail once when the approval arrives, and only then are the keys taken away. Accepted for this PR; making the approval dialog independent of scroll position is separate work.<ContentMouseController>here is mounted without aselectionQueryRef(the main view passes one) — a missing convenience, not a broken one.useTerminalBuffer: falserendering is byte-for-byte the previous code path.Linked Issues
Fixes #9507
中文说明
这个 PR 做了什么
当 Virtualized History 模式开启（
ui.useTerminalBuffer，默认开启）时，队友标签页的转录现在通过与主对话视图相同的ScrollableList虚拟视口渲染：头部信息、已提交条目、进行中条目和运行中的 spinner 都位于同一个固定在尾部的视口内，因此 Page Up / Page Down、Shift+方向键、Ctrl+Home/End 以及鼠标滚轮都可以回看更早的输出。已提交/进行中的拆分逻辑被保留——从正在执行/等待确认的工具组开始的条目仍走可交互渲染路径，保证队友的审批对话框在视口内仍能接收输入。useTerminalBuffer: false时完全不变：转录继续通过<Static>流入终端原生回滚缓冲区，那条路径本来就没有问题。为什么需要
VP 模式下交互式界面接管整个终端屏幕，没有终端原生回滚可以兜底。
AgentChatContent无条件用 ink 的<Static>渲染队友标签页转录，所以一旦队友的输出滚出可见区域就无法通过任何方式找回——向上箭头、Page Up/Page Down、鼠标滚轮全部无效（#9507）。主对话视图没有这个问题，因为其转录本来就渲染在可滚动的虚拟视口里；本 PR 让队友标签页对齐这一行为。评审测试计划
如何验证
组件级复现与修复验证使用真实的
ScrollableList/VirtualizedList视口机制和真实按键管线（不 mock 视口）。下面两条命令都在packages/cli下执行，路径相对该 package：cd packages/cli && npx vitest run src/ui/components/agent-view/AgentChatContent.test.tsx。修复前 VP 模式测试失败，因为渲染器一次性输出全部 40 行转录且 Page Up 无效；修复后视口初始固定在尾部，Page Up 可以回看更早输出、尾部离开可见窗口，且 legacy（useTerminalBuffer: false）路径仍原样输出全部内容。完整 agent-view 回归（只跑这两个目标，不是全仓套件）：cd packages/cli && npx vitest run src/ui/components/agent-view/ src/ui/components/shared/ScrollableList.test.tsx，68 条测试全部通过。tsc --noEmit和eslint对改动文件无新增报错（本环境下仓库层面剩余的 tsc 错误在纯净main上同样存在：acpAgent.ts、daemon-status.ts、workspace-extensions-controller.ts、native-audio-recorder.ts）。手动验证方式：启用 Agent Team（QWEN_CODE_ENABLE_AGENT_TEAM=1），创建团队，让队友产生超过一屏的输出，切换到它的标签页，用 Page Up 回滚。证据（修复前后）
来自复现测试的组件级帧（VP 模式，40 行转录，视口高度 8）：
修复前（旧的纯
<Static>渲染——所有行一次性输出，Page Up 无效）：帧内同时包含AGENT_HEADER、user-msg-0到user-msg-39全部行，没有任何滚动能力。修复后（打开时视口固定在尾部）：帧内为
user-msg-32到user-msg-39。按若干次 Page Up 后（更早的输出回到视口，尾部离开窗口）：帧内为
user-msg-0到user-msg-7。测试平台
Linux ✅ 为组件级验证（真实视口 + 按键管线经 ink-testing-library 驱动，修复前红、修复后绿）；本机未跑真实的多队友终端会话。macOS / Windows⚠️ 未测试。
环境
Node v24，Linux 下
packages/cli内npx vitest run；无沙箱。风险与范围
<Static>——滚动行为与主视图对齐，但视口特有的渲染特性（高度估算、滚动条）也会作用于队友转录；执行中/确认中的拆分保证审批对话框仍在可交互路径上。ScrollableList的按键订阅与useMouseEvents都以hasFocus为门控，而AgentChatContent把!approvalPending传进了这个标志。这是有意设计而非疏漏：待处理的ToolConfirmationMessage是渲染在窗口化视口内部的尾部条目，一旦滚出[renderRangeStart, renderRangeEnd]就会连同它的useKeypress订阅一起被卸载，屏幕上不再有任何可回答的东西，却把 agent 这一轮阻塞住。因此审批到达时先把视口滚动到尾部一次，然后才收走按键。本 PR 接受这个权衡；让审批对话框不依赖滚动位置属于另一项工作。<ContentMouseController>没有传selectionQueryRef（主视图有传）——属于少一个便利项，不是坏了。useTerminalBuffer: false的渲染与原代码路径完全一致。关联 Issue
Fixes #9507