-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(cli): prevent scroll snap-back and flicker in non-VP mode during multi-agent runs #5799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
wenshao
merged 5 commits into
QwenLM:main
from
chiga0:fix/non-vp-controls-height-roster
Jun 25, 2026
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c91c1bc
fix(cli): re-measure footer when LiveAgentPanel grows to stop non-VP …
78ba19d
test(cli): cover all isLiveAgentPanelVisibleEntry branches; condense …
aac3e92
refactor(cli): use canonical AgentTask.id instead of @deprecated agen…
b63d6d1
test(cli): add source-level guard for the controlsHeight measurement dep
7898399
Merge branch 'main' into fix/non-vp-controls-height-roster
wenshao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| /** | ||
| * Source-level regression guard for the one-line fix in #5798/#5799 that the | ||
| * behavioural tests cannot reach. | ||
| * | ||
| * The fix is: `liveAgentPanelLayoutKey` is listed in the dependency array of | ||
| * the `useLayoutEffect` that measures `controlsHeight` from `mainControlsRef`. | ||
| * Removing it silently re-introduces the non-VP overflow flicker (the footer | ||
| * stops being re-measured when the LiveAgentPanel grows). | ||
| * | ||
| * Why this is a source assertion rather than a render test: the behaviour only | ||
| * manifests on an in-place UPDATE of AppContainer, and ink-testing-library's | ||
| * `rerender` remounts AppContainer (re-running every mount effect regardless of | ||
| * its deps), while an external `setState` does not flush ink's reconciler. So a | ||
| * real AppContainer always re-measures on (re)mount in tests and the missing | ||
| * dependency is invisible to a render-based assertion — exactly why dropping it | ||
| * leaves the mechanism tests (which use a stand-in component) green. This guard | ||
| * pins the dependency directly, so a deps-array cleanup or an `exhaustive-deps` | ||
| * autofix cannot quietly delete the fix. | ||
| */ | ||
|
|
||
| import { describe, it, expect } from 'vitest'; | ||
| import { readFileSync } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
|
|
||
| const source = readFileSync( | ||
| join(import.meta.dirname, 'AppContainer.tsx'), | ||
| 'utf8', | ||
| ); | ||
|
|
||
| /** Extract the dependency array of the controls-height measurement effect. */ | ||
| function controlsHeightEffectDeps(): string { | ||
| const measureAt = source.indexOf('measureElement(mainControlsRef.current)'); | ||
| expect(measureAt).toBeGreaterThan(-1); | ||
| const depsOpen = source.indexOf('}, [', measureAt); | ||
| expect(depsOpen).toBeGreaterThan(-1); | ||
| const depsClose = source.indexOf(']);', depsOpen); | ||
| expect(depsClose).toBeGreaterThan(depsOpen); | ||
| return source.slice(depsOpen, depsClose); | ||
| } | ||
|
|
||
| describe('AppContainer controls-height measurement wiring', () => { | ||
| it('measures controls height from mainControlsRef', () => { | ||
| // Sanity: the effect we are guarding still exists and is shaped as expected. | ||
| expect(source).toContain('measureElement(mainControlsRef.current)'); | ||
| expect(source).toContain('setControlsHeight('); | ||
| }); | ||
|
|
||
| it('lists liveAgentPanelLayoutKey in the measurement effect dependencies', () => { | ||
| const deps = controlsHeightEffectDeps(); | ||
| // Confirm we located the right deps array before the key assertion. | ||
| expect(deps).toContain('terminalHeight'); | ||
| expect(deps).toContain('stickyTodosLayoutKey'); | ||
| // The fix: dropping this entry re-introduces the non-VP overflow flicker. | ||
| expect(deps).toContain('liveAgentPanelLayoutKey'); | ||
| }); | ||
|
|
||
| it('computes liveAgentPanelLayoutKey from the live agent roster', () => { | ||
| // The key must be derived from the roster + focus, not a constant. Match | ||
| // whitespace-tolerantly so prettier reformatting can't break the guard. | ||
| expect(source).toMatch( | ||
| /liveAgentPanelLayoutKey\s*=\s*getLiveAgentPanelLayoutKey\(\s*bgTaskEntries\s*,\s*bgLivePanelFocused\s*,?\s*\)/, | ||
| ); | ||
| }); | ||
| }); |
140 changes: 140 additions & 0 deletions
140
packages/cli/src/ui/components/background-view/liveAgentPanelLayout.measurement.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| /** | ||
| * Runtime reproduction of the non-VP overflow flicker root cause. | ||
| * | ||
| * AppContainer reserves room for the footer with | ||
| * availableTerminalHeight = terminalHeight - controlsHeight - ... | ||
| * where `controlsHeight` is measured from the controls box (which contains the | ||
| * LiveAgentPanel) inside a `useLayoutEffect` gated by a dependency array. The | ||
| * panel grows as agents launch, but the panel's only self-driven re-render is a | ||
| * per-second elapsed-time tick that never changes the roster. So unless the | ||
| * roster is part of the measurement effect's deps, the effect does not re-run | ||
| * when an agent launches: `controlsHeight` stays stale, `availableTerminalHeight` | ||
| * stays too large, the pending region overflows the terminal, and every repaint | ||
| * forces the view back to the bottom with a flicker. | ||
| * | ||
| * This test faithfully mirrors that exact measurement contract on a minimal | ||
| * component — real ink render, real `measureElement`, real | ||
| * `getLiveAgentPanelLayoutKey` — and shows that: | ||
| * - WITHOUT the roster key in the deps, a roster that grows leaves the | ||
| * measured controls height (and thus availableHeight) stale; and | ||
| * - WITH the roster key in the deps, the controls are re-measured and the | ||
| * reserved room shrinks to match — which is exactly the one-line fix | ||
| * applied in AppContainer. | ||
| */ | ||
|
|
||
| import { describe, it, expect } from 'vitest'; | ||
| import { useLayoutEffect, useRef, useState } from 'react'; | ||
| import { render } from 'ink-testing-library'; | ||
| import { Box, Text, measureElement, type DOMElement } from 'ink'; | ||
| import { getLiveAgentPanelLayoutKey } from './liveAgentPanelVisibility.js'; | ||
| import type { AgentDialogEntry } from '../../hooks/useBackgroundTaskView.js'; | ||
|
|
||
| const TERMINAL_HEIGHT = 24; | ||
|
|
||
| const agent = (id: string): AgentDialogEntry => | ||
| ({ | ||
| kind: 'agent', | ||
| id, | ||
| description: 'desc', | ||
| status: 'running', | ||
| startTime: 0, | ||
| abortController: new AbortController(), | ||
| }) as unknown as AgentDialogEntry; | ||
|
|
||
| /** | ||
| * Minimal stand-in for AppContainer's footer-measurement contract. The controls | ||
| * box renders one row per agent (so its real measured height grows with the | ||
| * roster), measures itself into `controlsHeight` via a useLayoutEffect, and | ||
| * reports the resulting availableHeight. `wireRosterDep` toggles whether the | ||
| * roster signal is part of the effect deps — i.e. buggy vs fixed. | ||
| */ | ||
| function ControlsMeasured({ | ||
| entries, | ||
| wireRosterDep, | ||
| report, | ||
| }: { | ||
| entries: readonly AgentDialogEntry[]; | ||
| wireRosterDep: boolean; | ||
| report: (availableHeight: number) => void; | ||
| }) { | ||
| const ref = useRef<DOMElement>(null); | ||
| const [controlsHeight, setControlsHeight] = useState(0); | ||
|
|
||
| const rosterKey = getLiveAgentPanelLayoutKey(entries, false); | ||
| const deps = wireRosterDep ? [rosterKey] : []; | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!ref.current) return; | ||
| const { height } = measureElement(ref.current); | ||
| setControlsHeight((prev) => (prev === height ? prev : height)); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, deps); | ||
|
|
||
| report(Math.max(0, TERMINAL_HEIGHT - controlsHeight)); | ||
|
|
||
| return ( | ||
| <Box flexDirection="column" ref={ref}> | ||
| <Text>main</Text> | ||
| {entries.map((e) => ( | ||
| <Text key={e.id}>{e.id} · running</Text> | ||
| ))} | ||
| </Box> | ||
| ); | ||
| } | ||
|
|
||
| async function measureGrowth( | ||
| wireRosterDep: boolean, | ||
| ): Promise<{ before: number; after: number }> { | ||
| let availableHeight = -1; | ||
| const report = (v: number) => { | ||
| availableHeight = v; | ||
| }; | ||
|
|
||
| const { rerender, unmount } = render( | ||
| <ControlsMeasured | ||
| entries={[]} | ||
| wireRosterDep={wireRosterDep} | ||
| report={report} | ||
| />, | ||
| ); | ||
| await new Promise((r) => setTimeout(r, 20)); | ||
| const before = availableHeight; | ||
|
|
||
| // Three agents launch → the controls box is now three rows taller. | ||
| rerender( | ||
| <ControlsMeasured | ||
| entries={[agent('a1'), agent('a2'), agent('a3')]} | ||
| wireRosterDep={wireRosterDep} | ||
| report={report} | ||
| />, | ||
| ); | ||
| await new Promise((r) => setTimeout(r, 20)); | ||
| const after = availableHeight; | ||
|
|
||
| unmount(); | ||
| return { before, after }; | ||
| } | ||
|
|
||
| describe('LiveAgentPanel growth → controls re-measurement', () => { | ||
| it('BUG: without the roster in the measurement deps, reserved room goes stale on growth', async () => { | ||
| const { before, after } = await measureGrowth(false); | ||
| // Footer was measured once with an empty roster and never again, so the | ||
| // reserved room does not shrink even though the panel grew by three rows. | ||
| expect(before).toBeGreaterThan(0); | ||
| expect(after).toBe(before); | ||
| }); | ||
|
|
||
| it('FIX: wiring the roster key into the deps re-measures, shrinking reserved room', async () => { | ||
| const { before, after } = await measureGrowth(true); | ||
| // The taller controls footprint is now reflected: less room is left for the | ||
| // main content, so it can no longer overflow the terminal. | ||
| expect(after).toBeLessThan(before); | ||
| expect(before - after).toBe(3); // exactly the three new agent rows | ||
| }); | ||
| }); |
139 changes: 139 additions & 0 deletions
139
packages/cli/src/ui/components/background-view/liveAgentPanelVisibility.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { describe, it, expect } from 'vitest'; | ||
| import { | ||
| getLiveAgentPanelLayoutKey, | ||
| isLiveAgentPanelVisibleEntry, | ||
| TERMINAL_VISIBLE_MS, | ||
| } from './liveAgentPanelVisibility.js'; | ||
| import type { | ||
| AgentDialogEntry, | ||
| DialogEntry, | ||
| } from '../../hooks/useBackgroundTaskView.js'; | ||
|
|
||
| function agentEntry( | ||
| overrides: Partial<AgentDialogEntry> = {}, | ||
| ): AgentDialogEntry { | ||
| return { | ||
| kind: 'agent', | ||
| id: 'a', | ||
| description: 'desc', | ||
| status: 'running', | ||
| startTime: 0, | ||
| abortController: new AbortController(), | ||
| ...overrides, | ||
| } as AgentDialogEntry; | ||
| } | ||
|
|
||
| function shellEntry(overrides: Partial<DialogEntry> = {}): DialogEntry { | ||
| return { | ||
| kind: 'shell', | ||
| shellId: 'bg_x', | ||
| command: 'sleep 60', | ||
| cwd: '/tmp', | ||
| status: 'running', | ||
| startTime: 0, | ||
| outputPath: '/tmp/x.out', | ||
| abortController: new AbortController(), | ||
| ...overrides, | ||
| } as DialogEntry; | ||
| } | ||
|
|
||
| describe('getLiveAgentPanelLayoutKey', () => { | ||
| it('changes when an agent is added (panel grows)', () => { | ||
| const before = getLiveAgentPanelLayoutKey([], false); | ||
| const after = getLiveAgentPanelLayoutKey([agentEntry({ id: 'a1' })], false); | ||
| expect(after).not.toBe(before); | ||
| }); | ||
|
|
||
| it('changes when an agent is removed (panel shrinks)', () => { | ||
| const two = getLiveAgentPanelLayoutKey( | ||
| [agentEntry({ id: 'a1' }), agentEntry({ id: 'a2' })], | ||
| false, | ||
| ); | ||
| const one = getLiveAgentPanelLayoutKey([agentEntry({ id: 'a1' })], false); | ||
| expect(one).not.toBe(two); | ||
| }); | ||
|
|
||
| it('changes when an agent status flips (running -> completed)', () => { | ||
| const running = getLiveAgentPanelLayoutKey( | ||
| [agentEntry({ id: 'a1', status: 'running' })], | ||
| false, | ||
| ); | ||
| const done = getLiveAgentPanelLayoutKey( | ||
| [agentEntry({ id: 'a1', status: 'completed', endTime: 1 })], | ||
| false, | ||
| ); | ||
| expect(done).not.toBe(running); | ||
| }); | ||
|
|
||
| it('changes when panel focus toggles (adds the navigation hint row)', () => { | ||
| const entries = [agentEntry({ id: 'a1' })]; | ||
| expect(getLiveAgentPanelLayoutKey(entries, true)).not.toBe( | ||
| getLiveAgentPanelLayoutKey(entries, false), | ||
| ); | ||
| }); | ||
|
|
||
| it('is STABLE across per-second elapsed-time ticks (no height change)', () => { | ||
| // The panel re-renders every second to refresh elapsed time, but that | ||
| // tick never touches the roster — the key must not churn, or AppContainer | ||
| // would needlessly re-measure the footer every second. | ||
| const entries = [ | ||
| agentEntry({ id: 'a1', status: 'running', startTime: 0 }), | ||
| agentEntry({ id: 'a2', status: 'running', startTime: 0 }), | ||
| ]; | ||
| const k1 = getLiveAgentPanelLayoutKey(entries, false); | ||
| const k2 = getLiveAgentPanelLayoutKey(entries, false); | ||
| expect(k2).toBe(k1); | ||
| }); | ||
|
|
||
| it('ignores non-agent entries (panel renders only agents)', () => { | ||
| const onlyShell = getLiveAgentPanelLayoutKey([shellEntry()], false); | ||
| const empty = getLiveAgentPanelLayoutKey([], false); | ||
| expect(onlyShell).toBe(empty); | ||
| }); | ||
| }); | ||
|
|
||
| // Guard the assumption the layout key relies on: a finished agent stays | ||
| // visible (so its row keeps occupying height) for the eviction window, and | ||
| // only then shrinks the panel — the "safe" direction the key intentionally | ||
| // does not track. | ||
| describe('isLiveAgentPanelVisibleEntry (eviction window)', () => { | ||
| it('returns false for non-agent entries', () => { | ||
| expect(isLiveAgentPanelVisibleEntry(shellEntry(), 1000)).toBe(false); | ||
| }); | ||
|
|
||
| it('keeps running agents visible unconditionally (no endTime)', () => { | ||
| expect( | ||
| isLiveAgentPanelVisibleEntry(agentEntry({ status: 'running' }), 1000), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it('keeps paused agents visible unconditionally (no endTime)', () => { | ||
| expect( | ||
| isLiveAgentPanelVisibleEntry(agentEntry({ status: 'paused' }), 1000), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false for a terminal agent missing endTime (guards NaN)', () => { | ||
| // nowMs - undefined would be NaN, and NaN <= window is false — assert the | ||
| // explicit endTime guard short-circuits before that comparison. | ||
| const entry = agentEntry({ status: 'completed' }); | ||
| expect(isLiveAgentPanelVisibleEntry(entry, 1000)).toBe(false); | ||
| }); | ||
|
|
||
| it('keeps a terminal agent visible within the window, evicts after', () => { | ||
|
chiga0 marked this conversation as resolved.
|
||
| const entry = agentEntry({ status: 'completed', endTime: 1000 }); | ||
| expect(isLiveAgentPanelVisibleEntry(entry, 1000)).toBe(true); | ||
| expect( | ||
| isLiveAgentPanelVisibleEntry(entry, 1000 + TERMINAL_VISIBLE_MS), | ||
| ).toBe(true); | ||
| expect( | ||
| isLiveAgentPanelVisibleEntry(entry, 1000 + TERMINAL_VISIBLE_MS + 1), | ||
| ).toBe(false); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.