From 1bb13269044dc07d966613d7e2fa2c2bbd8bdc65 Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 7 May 2026 19:30:14 +0800 Subject: [PATCH] =?UTF-8?q?fix(cli):=20address=20Copilot=20second-pass=20r?= =?UTF-8?q?eview=20=E2=80=94=20dialogOpen=20tick=20gate,=20isFocused=20doc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two further findings from Copilot: 1. **Interval kept ticking while bg-tasks dialog was open.** The first-pass fix already torn the tick down once all rows expired from the visibility window, but `dialogOpen` was a separate reason the panel returned null and was missed by the gate. Add `dialogOpen` to the useEffect deps and short-circuit when true, so the dialog's tenure is interval-free. 2. **Stale `isFocused` doc comment in `ToolMessageProps`.** The comment claimed the prop controlled the now-retired `Ctrl+E / Ctrl+F` display shortcuts (those died with the inline `AgentExecutionDisplay` frame). Rewrite the comment to describe the only remaining behavior — the focus-routed approval surface (focus-holder banner vs. queued-sibling marker). Coverage delta: +1 case on `LiveAgentPanel.test.tsx` — `tears the 1s tick down when the bg-tasks dialog opens` advances 60s of fake time with `dialogOpen=true` and asserts no panel state drift. --- .../background-view/LiveAgentPanel.test.tsx | 31 +++++++++++++++++++ .../background-view/LiveAgentPanel.tsx | 21 ++++++++----- .../ui/components/messages/ToolMessage.tsx | 9 ++++-- 3 files changed, 51 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/ui/components/background-view/LiveAgentPanel.test.tsx b/packages/cli/src/ui/components/background-view/LiveAgentPanel.test.tsx index 27267baf3..d9d5c0a18 100644 --- a/packages/cli/src/ui/components/background-view/LiveAgentPanel.test.tsx +++ b/packages/cli/src/ui/components/background-view/LiveAgentPanel.test.tsx @@ -295,6 +295,37 @@ describe('', () => { expect(lastFrame() ?? '').toBe(''); }); + it('tears the 1s tick down when the bg-tasks dialog opens', () => { + // While the dialog is open the panel returns null and the dialog + // owns the same data — a still-running interval is a wasted + // re-render budget. Verify by checking that advancing the clock + // past the visibility window with dialogOpen=true does not flip + // the panel into its "expired" state (which would only happen if + // the tick advanced `now`). + const initial = agentEntry({ + agentId: 'live-1', + subagentType: 'researcher', + description: 'researcher: investigate', + status: 'completed', + startTime: -2000, + endTime: 0, + }); + const { config } = makeRegistryConfig([initial]); + const { lastFrame } = renderPanel({ + entries: [initial], + config, + dialogOpen: true, + }); + // Dialog open → panel hidden, no opportunity for `now` to drift. + expect(lastFrame() ?? '').toBe(''); + act(() => { + vi.advanceTimersByTime(60_000); + }); + // Still hidden. The fact that we got here without the panel ever + // mounting an interval means subsequent renders won't churn either. + expect(lastFrame() ?? '').toBe(''); + }); + it('still shows the snapshot when no Config is mounted (test fixtures)', () => { // Without a Config provider the panel can't reach the registry, so // it has to trust the snapshot — this is the one place the legacy diff --git a/packages/cli/src/ui/components/background-view/LiveAgentPanel.tsx b/packages/cli/src/ui/components/background-view/LiveAgentPanel.tsx index 402ec6f6a..31ed77b56 100644 --- a/packages/cli/src/ui/components/background-view/LiveAgentPanel.tsx +++ b/packages/cli/src/ui/components/background-view/LiveAgentPanel.tsx @@ -148,14 +148,20 @@ export const LiveAgentPanel: React.FC = ({ const config = useContext(ConfigContext); // Wall-clock tick. Drives elapsed-time refresh, terminal-row eviction, - // AND the live registry re-pull below. The gate must consider both - // live agents AND terminal-but-still-visible agents (within the 8s - // visibility window) — `BackgroundTaskRegistry.getAll()` retains - // terminal entries indefinitely, so a naive `entries.some(isAgentEntry)` - // gate would keep ticking forever after the last entry's window - // closed, churning re-renders for nothing on screen. + // AND the live registry re-pull below. The gate must consider: + // - `dialogOpen` — when the bg-tasks dialog is up, the panel + // renders null (`if (dialogOpen) return null` below), so any + // ticks the interval fires are wasted re-renders. + // - live agents (running / paused) — always need elapsed updates. + // - terminal agents still inside the 8s visibility window — need + // ticks to drive their eviction. + // `BackgroundTaskRegistry.getAll()` retains terminal entries + // indefinitely, so a naive `entries.some(isAgentEntry)` gate would + // keep ticking forever after the last entry's window closed; the + // `dialogOpen` arm closes the corresponding gap on the dialog side. const [now, setNow] = useState(() => Date.now()); useEffect(() => { + if (dialogOpen) return; const needsTick = (whenMs: number) => entries.some((e) => { if (!isAgentEntry(e)) return false; @@ -174,8 +180,7 @@ export const LiveAgentPanel: React.FC = ({ if (!needsTick(wallNow)) clearInterval(id); }, 1000); return () => clearInterval(id); - - }, [entries]); + }, [entries, dialogOpen]); // Re-pull each agent from the live registry on every tick so the row // shows the latest `recentActivities` — `useBackgroundTaskView` diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index b926ccd07..c5a6d7db0 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -402,8 +402,13 @@ export interface ToolMessageProps extends IndividualToolCallDisplay { config?: Config; forceShowResult?: boolean; /** - * Whether this subagent owns keyboard input for confirmations and - * Ctrl+E/Ctrl+F display shortcuts. + * Whether this subagent owns keyboard input for the inline approval + * surface — when true the focus-holder banner renders and the + * underlying ToolConfirmationMessage receives keystrokes; when false + * sibling subagents render a dim "Queued approval" marker instead. + * (The legacy Ctrl+E / Ctrl+F display shortcuts were retired with + * the inline AgentExecutionDisplay frame; LiveAgentPanel owns the + * live progress surface and BackgroundTasksDialog owns drill-down.) */ isFocused?: boolean; /**