fix(cli): prevent scroll snap-back and flicker in non-VP mode during multi-agent runs - #5799
Conversation
…overflow flicker (QwenLM#5798) In non-VP mode the dynamic (non-<Static>) region is repainted in place; once it is taller than the terminal, every repaint forces the view back to the bottom with a flicker. The pending region is supposed to be clipped to availableTerminalHeight = terminalHeight - controlsHeight - ..., but controlsHeight was measured by a useLayoutEffect whose dependency list did not reflect the LiveAgentPanel roster. The panel renders inside mainControlsRef and grows as agents launch, yet its only self-driven re-render is a per-second elapsed-time tick that never changes the roster. So controlsHeight went stale on agent launch, availableTerminalHeight was left too large, and the pending region overflowed the terminal. Add getLiveAgentPanelLayoutKey (mirrors the existing stickyTodosLayoutKey) and feed it into the controlsHeight measurement effect's deps, so the footer is re-measured exactly on height-affecting roster changes (agent add/remove/status, panel focus) and stays stable across the elapsed-time tick. All growth events already flow through BackgroundTaskViewState.entries, which AppContainer consumes, so this adds no extra render churn. Time-based eviction only shrinks the panel (safe over-reservation) and is intentionally not tracked. Tests: unit-test the key (changes on growth/shrink/status/focus, stable across ticks) and a runtime reproduction using real ink render + measureElement showing the reserved room goes stale without the dep and re-measures with it. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! (Re-triage — PR body has been updated since the initial review.) Template looks good ✓ — all required sections present: Reviewer Test Plan (with How to verify, Evidence, Tested on, Environment), Risk & Scope, Linked Issues, bilingual details. On direction: clear, real bug fix. Scroll snap-back and flicker in non-VP mode during multi-agent runs is a genuine user-facing TUI regression. Directly aligned with the CLI's core interactive experience and the On approach: minimal and pattern-following. One production line (adding a deps-array entry) mirroring the existing Moving on to code review. 🔍 中文说明感谢贡献!(重新审查——PR 正文已根据初次审查反馈更新。) 模板完整 ✓ — 所有必需章节齐全:Reviewer Test Plan(含 How to verify、Evidence、Tested on、Environment)、Risk & Scope、Linked Issues、中英双语。 方向:明确的真实 bug 修复。非 VP 模式下多 agent 运行时的滚动回弹和闪烁是用户可感知的 TUI 回归,与 CLI 核心交互体验和 方案:最小化且遵循既有模式。生产代码仅一行(在依赖数组中添加一个条目),复用了 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Verification reportAutomated testsNew tests (run from repo root): Regression — existing suites that touch the same surfaces: How the bug is isolated
How to verify manually (the live symptom)
Note on test scopeA full |
chiga0
left a comment
There was a problem hiding this comment.
Overview
Final Verdict: LGTM — Clean, minimal, well-documented fix for a precise root cause. The layout key pattern mirrors the existing stickyTodosLayoutKey and correctly captures only height-affecting roster changes while staying stable across per-second elapsed-time ticks.
Key Observations
The root cause analysis is spot-on: the footer measurement useLayoutEffect's dependency array did not reflect the LiveAgentPanel roster, so controlsHeight went stale on agent launch. The fix is a single dependency addition in AppContainer.tsx plus a well-designed utility function.
The design decision to NOT track time-based eviction (the 8s window after agent completion) is correct — eviction only shrinks the panel, so a stale, slightly too-large reservation is the safe direction (over-clip, not overflow).
Additional Audit Coverage
- Layout key stability: String concatenation of
agentId:statuspairs is efficient for typical roster sizes (<20 agents). The|delimiter prevents collision between adjacent entries. Focus flag prefix (f/_) ensures focus toggles trigger re-measurement. - Test quality: The runtime reproduction test (
liveAgentPanelLayout.measurement.test.tsx) is exceptional — it demonstrates both the bug (without the dep) and the fix (with the dep) using real ink render +measureElement. The unit tests cover all edge cases: add/remove/status/focus/tick-stability/non-agent-filtering. isLiveAgentPanelVisibleEntryeviction window test: Guards the assumption that finished agents stay visible forTERMINAL_VISIBLE_MS, validating the safe-direction design decision.- No render churn: The layout key is derived from
bgTaskEntriesandbgLivePanelFocused, both already consumed byAppContainer. No new subscriptions or re-renders introduced.
This review was generated by QoderWork AI
|
Code review: No issues found. The production change is a single dependency-array addition in The source-level regression guard ( Tests: 42/42 pass.
Typecheck: Build: Smoke test (tmux)Basic CLI operation works under the PR build. The actual bug (multi-agent non-VP scroll flicker) requires interactive background agents + content overflow + scroll-up — cannot be scripted in headless 中文说明代码审查: 未发现问题。生产代码改动仅一行——在 源码级回归守护测试( 测试:42/42 通过。
类型检查: 构建: 冒烟测试(tmux): 基本 CLI 功能正常。实际 bug(多 agent 非 VP 滚动闪烁)需要交互式后台 agent + 内容溢出 + 向上滚动——无法在无头 — Qwen Code · qwen3.7-max |
|
Clean bug fix, ready to ship. The root cause is precisely identified (stale Independent proposal matches: I would have taken the same approach. A roster-derived string key in the deps array is the minimum viable fix, and the PR goes further by including both a runtime integration test that proves the bug and fix end-to-end, and a source-assertion test that prevents silent removal of the dep. All 42 tests pass. Typecheck clean. Build clean. Smoke test clean. Two independent maintainer verifications (mutation testing + live TUI repro) confirm the same conclusion. Approving. ✅ 中文说明干净的 bug 修复,可以合入。根因已精确定位(LiveAgentPanel 增长时 独立方案一致:我会采取同样的方法。在依赖数组中使用 roster 派生的字符串 key 是最小可行修复,PR 更进一步,既包含端到端证明 bug 和修复的运行时集成测试,又包含防止依赖被静默移除的源码断言测试。 42 个测试全部通过。类型检查干净。构建干净。冒烟测试干净。两位独立维护者验证(变异测试 + 真实 TUI 复现)确认相同结论。 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
chiga0
left a comment
There was a problem hiding this comment.
No issues found. Downgraded from Approve to Comment: self-PR; CI still running.
The fix is minimal and well-targeted — adds a roster-derived layout key to the footer measurement effect's deps so the controls are re-measured when the LiveAgentPanel grows, without triggering on the per-second elapsed-time tick. The design choice to not track time-based eviction (safe direction) is sound. Tests faithfully reproduce the bug mechanism and validate the fix.
— qwen3.7-max via Qwen Code /review
…duplicated rationale Address review on QwenLM#5799: - AppContainer: condense the call-site comment to a cross-reference; the full rationale lives in getLiveAgentPanelLayoutKey's JSDoc (was duplicated 3x). - liveAgentPanelVisibility.test: add the missing branches for isLiveAgentPanelVisibleEntry — non-agent → false, running/paused → true, terminal agent missing endTime → false (guards NaN from nowMs - undefined). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the review! Addressed everything:
Test count is now 13 across the two new files (11 + 2); existing suites still green. |
…tId in layout key Address @wenshao's review on QwenLM#5799: agentId is a @deprecated synonym for the canonical TaskBase.id ("Always equals id"). Switch getLiveAgentPanelLayoutKey to read entry.id, and update both new test factories to populate id. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
✅ Local verification report — PR #5799Verdict: LGTM — recommend merge. I built the real binary, ran the full test suite, mutation-tested the new tests, and reproduced the issue's exact multi-agent scenario in a live TUI. The fix is correct, minimal, and well-targeted, and the new tests are non-vacuous (mutation-verified). Verified at PR head 1. Build & boot (real binary)
2. Tests / typecheck / lint
3. Mutation testing — are the new tests real?I mutated the source and re-ran the tests to prove they actually catch the regression:
4. Real-binary TUI reproduction of the issue scenarioI built two bundles — the PR (fixed) and a reverted-fix (buggy), confirmed byte-different — and drove the real binary in Key lines from the run (agents launch first, then the response streams to fill the terminal):
5. Correctness notes
🇨🇳 中文版(完整对应)✅ 本地验证报告 — PR #5799结论:LGTM,建议合并。 我构建了真实二进制、跑了完整测试套件、对新增测试做了变异测试,并在真实 TUI 中复现了该 issue 的多 agent 场景。修复正确、最小化、定位精准,新增测试经变异验证确实有效(非空过)。 验证基于 PR head 1. 构建与启动(真实二进制)
2. 测试 / 类型检查 / Lint
3. 变异测试 — 新增测试是真的有效吗?我对源码做了变异并重跑测试,以证明它们确实能捕获该回归:
4. 真实二进制 TUI 场景复现我构建了两个 bundle——PR 版(fixed)与还原修复版(buggy),并确认两者字节不同——在 运行中的关键画面(先启动 agent,随后响应流式输出填满终端):
5. 正确性说明
Verification: isolated worktree @ |
✅ Maintainer local-verification report — PR #5799Verdict: LGTM — recommend merge. The fix is correct, minimal, well-targeted, and backed by tests that genuinely exercise the mechanism (confirmed by mutation testing). Built & verified locally against PR head Root cause (re-confirmed against the code)
The fixA pure Verification performed
Mutation testing — proves the tests aren't tautological:
The constant-key kill is the important one: it shows Design notes (all correct / non-blocking)
Environment notes (not PR issues)
I deliberately did not run a manual TUI flicker A/B: this is a layout-measurement bug, and the shipped 🇨🇳 中文版(完整对应)✅ 维护者本地验证报告 — PR #5799结论:LGTM,建议合并。 修复正确、最小化、目标精准,且配套测试真正覆盖了该机制(已通过变异测试验证)。在隔离 worktree 中基于 PR head 根因(已对照代码再次确认)
修复方案新增纯函数 已执行的验证
变异测试 —— 证明测试不是空转:
常量 key 那次最关键:它证明 设计说明(均正确 / 不阻塞)
环境说明(非本 PR 问题)
我特意没有做人工 TUI 闪烁 A/B 对比:这是一个布局测量类 bug,PR 自带的 Verified locally by the maintainer (@wenshao) in an isolated worktree against PR head |
Address @wenshao's review on QwenLM#5799: the one-line fix (liveAgentPanelLayoutKey in the controlsHeight measurement-effect deps) had no regression guard — the behavioural tests exercise the mechanism on a stand-in component, and removing the dep left all of them green. A render-based guard against the real AppContainer is not feasible: ink-testing-library's rerender remounts AppContainer (re-running mount effects regardless of deps) and an external setState does not flush ink's reconciler, so the missing-dependency update path is unobservable. Add a source-level assertion instead — it fails if liveAgentPanelLayoutKey is dropped from the measurement effect's dependency array (verified: removing the dep turns the guard red), so a deps cleanup or exhaustive-deps autofix can't silently revert the fix. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Clean, focused bug fix — adds a roster-derived layout key to the footer measurement effect's deps so controlsHeight re-measures when the LiveAgentPanel grows. All 16 new tests pass. Deterministic analysis (tsc + eslint) clean on changed files. 9-agent review + reverse audit found no high-confidence issues.
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: review-pr.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Clean, minimal fix — adds a roster-derived layout key to the controls-height measurement effect's dependency array so controlsHeight re-measures when the LiveAgentPanel grows. The per-second elapsed tick correctly stays stable (no re-measurement churn). All 16 new tests pass, deterministic analysis clean (tsc + eslint, 0 findings).
— qwen3.7-max via Qwen Code /review
What this PR does
Keeps the footer height reservation in sync with the always-on LiveAgentPanel as background agents launch and finish, so the main content area never overflows the terminal during multi-agent runs. It adds a small roster-derived layout key (mirroring the existing sticky-todos layout key) and feeds it into the footer-measurement effect, so the controls are re-measured exactly when the panel can grow — and stays quiet across the panel's per-second elapsed-time tick.
Why it's needed
In non-VP mode (the default), when output plus the footer exceeds the terminal height while background agents are running, the view cannot be scrolled up: any scroll-up snaps straight back to the bottom and the screen flickers, clearing up only once the run finishes. The footer is measured into
controlsHeightand subtracted from the available height for the main content, but the measurement only re-ran on a fixed set of dependencies that did not include the agent roster. The LiveAgentPanel renders inside the measured controls box and grows as agents launch, yet its only self-driven re-render is a per-second elapsed-time tick that never changes the roster — socontrolsHeightwent stale on agent launch, the reserved room was left too large, and the dynamic region overflowed the terminal, forcing the view back to the bottom on every repaint. Fixes #5798.Reviewer Test Plan
How to verify
Automated — from the repo root:
The runtime reproduction (
liveAgentPanelLayout.measurement.test.tsx) mirrors the exact measurement contract on a minimal component using real ink render, realmeasureElement, and the real layout-key helper: without the roster in the measurement deps a roster that grows from 0 → 3 agents leaves the reserved room unchanged (after === before, i.e. stale → would overflow); with it, the controls are re-measured and the reserved room shrinks by exactly the three new rows (before - after === 3). The unit tests pin the key's contract (changes on agent add/remove/status/focus, stable across the per-second tick) and cover every branch of the visibility predicate.Manual (the live symptom): on a short terminal (~20 rows) in non-VP mode, run a flow that launches several background agents and produces tall pending output, then try to scroll up while they run. Before: the view snaps back to the bottom and flickers each second; after: the footer footprint is reserved correctly and the snap-back / flicker is gone.
Evidence (Before & After)
Before: scroll-up snaps back to the bottom with per-second flicker during a multi-agent run (overflowing dynamic region). After: the dynamic region stays within the viewport and scrolling/redraw is stable. A reviewer can confirm via the automated reproduction above (the stale-vs-re-measured assertion is the programmatic Before/After); the maintainer CI smoke test on this PR also confirms no startup/regression impact.
Tested on
Verified locally on macOS (unit + runtime-reproduction tests, plus the existing AppContainer / LiveAgentPanel / DefaultAppLayout suites). Windows/Linux not run locally — covered by CI.
Environment (optional)
Local:
npx vitest runon macOS. No runtime sandbox needed; the change is TUI layout logic exercised by ink-testing-library and real ink render.Risk & Scope
availableTerminalHeight: besides the pending-region clip (the fix), the only other consumer is the embedded-shell pty sizing. So with a foreground shell running and background agents changing the panel at the same time, the pty is now resized to track the panel (previously the height was stale during agent runs and the pty did not follow). This is more correct, not a regression, and is bounded by roster-change events (not the per-second tick).Linked Issues
Fixes #5798
中文说明
这个 PR 做了什么
让底部 footer 的高度预留与常驻的 LiveAgentPanel(随后台 agent 启动/结束而增减行数)保持同步,从而在多 agent 运行时主内容区不再溢出终端。做法是新增一个由 roster 派生的 layout key(仿照已有的 sticky-todos layout key),接入 footer 高度测量的 effect 依赖,使控件区在 panel 可能变高时被重新测量,同时对 panel 每秒的计时刷新保持不变。
为什么需要
非 VP 模式(默认)下,当输出加 footer 超过终端高度且有后台 agent 在运行时,页面无法上滚:一上滚就被拽回底部并闪烁,直到运行结束才恢复。footer 被测量进
controlsHeight并从主内容可用高度里扣除,但该测量只在一组固定依赖变化时重跑,而这组依赖不含 agent roster。LiveAgentPanel 渲染在被测量的控件盒内、随 agent 启动而变高,但它唯一的自驱动重渲染是每秒计时 tick(不改变 roster)——于是 agent 启动时controlsHeight失准偏小,预留空间偏大,动态区溢出终端,每次重绘都把视图顶回底部。修复 #5798。Reviewer Test Plan
如何验证
自动化(仓库根目录):见上方
vitest命令,13 个测试全过。运行时复现(liveAgentPanelLayout.measurement.test.tsx)用真实 ink 渲染 + 真实measureElement+ 真实 layout-key 函数,在最小组件上复刻测量契约:未接入 roster 依赖时,roster 从 0→3 个 agent 但预留高度不变(after === before,失准→会溢出);接入后控件被重测,预留高度精确减少 3 行(before - after === 3)。单测固定 key 的契约(roster 增删/状态/focus 时变化,每秒 tick 时不变),并覆盖可见性判定的所有分支。手动(真实现象):约 20 行高的短终端、非 VP 模式下,运行会启动多个后台 agent 且产生较高 pending 输出的流程,运行中尝试上滚。Before:视图被拽回底部并每秒闪烁;After:footer 占位正确预留,拽回/闪烁消失。
Evidence(Before & After)
Before:多 agent 运行中上滚被拽回底部、每秒闪烁(动态区溢出)。After:动态区保持在视口内,滚动/重绘稳定。可通过上方自动化复现确认(stale-vs-重测 的断言即程序化的 Before/After);本 PR 上 maintainer CI 的 smoke test 也确认无启动/回归影响。
测试平台
本地仅在 macOS 验证(单测 + 运行时复现,以及既有 AppContainer / LiveAgentPanel / DefaultAppLayout 套件)。Windows/Linux 本地未跑,由 CI 覆盖。
环境(可选)
本地:macOS 上
npx vitest run。无需运行时沙箱;改动为 TUI 布局逻辑,由 ink-testing-library 与真实 ink 渲染覆盖。风险与范围
availableTerminalHeight的下游:除 pending 区裁剪(即本修复)外,唯一的另一个消费者是嵌入式 shell 的 pty 尺寸。因此当"前台 shell 运行"与"后台 agent 改变 panel"同时发生时,pty 现在会随 panel 同步 resize(此前 agent 运行期高度失准、pty 不跟随)。这是更正确的行为、非回归,且频率受 roster 变化事件限制(非每秒 tick)。关联 Issue
Fixes #5798