refactor(cli): split LiveAgentPanelBody + drop dead isPending/isWaitingForOtherApproval props

Two structural reviews from deepseek-v4-pro via /review:

1. **Hook-order footgun.** The `if (dialogOpen) return null` guard
   sat between the hook block and a substantial block of pure-render
   code; an extension that added `useMemo`/`useRef`/`useCallback`
   below the guard would crash with "Rendered fewer hooks than
   expected" the next time `dialogOpen` toggled. Extract the pure-
   render logic into `LiveAgentPanelBody` so the guard becomes the
   parent's last statement, and any future "add a hook to the body"
   refactor naturally lands in the inner component (hook-free
   today, hook-free for as long as it stays a presentational FC).

2. **Dead pass-through removed.** `isPending` and
   `isWaitingForOtherApproval` were dead in the subagent renderer
   (LiveAgentPanel + BackgroundTasksDialog own that surface) but
   ToolGroupMessage still computed `isWaitingForOtherApproval` and
   forwarded both into ToolMessage. Drop them from
   `ToolMessageProps`, drop the computation + forwarding in
   ToolGroupMessage, drop the test factory references. ToolGroupMessage
   keeps `isPending` on its own props for upstream caller compatibility
   (HistoryItemDisplay et al. forward it) but stops destructuring
   it; the doc comment now points at the surfaces that own that
   gating today.

Coverage: existing 115 cases across LiveAgentPanel /
BackgroundTasksDialog / BackgroundTasksPill / ToolMessage /
ToolGroupMessage all green; the panel split is purely structural
and the dead-prop removal was verified TS-clean before commit.
This commit is contained in:
wenshao 2026-05-07 19:49:03 +08:00
parent dbad3f9449
commit 93036b1d04
4 changed files with 31 additions and 49 deletions

View file

@ -279,9 +279,30 @@ export const LiveAgentPanel: React.FC<LiveAgentPanelProps> = ({
// in `bgTasksDialogOpen`), but we keep the internal gate so callers
// mounting the panel outside that layout still get the right
// behavior.
//
// The early-return is the LAST statement of this component on
// purpose — pure rendering moves to LiveAgentPanelBody so that
// future refactors which add a hook can't accidentally drop it
// below the `dialogOpen` guard (`Rendered fewer hooks than
// expected` is the canonical bug shape this guards against).
if (dialogOpen) return null;
return (
<LiveAgentPanelBody
snapshots={liveAgentSnapshots}
now={now}
maxRows={maxRows}
width={width}
/>
);
};
const visibleAgents: LivePanelEntry[] = liveAgentSnapshots
const LiveAgentPanelBody: React.FC<{
snapshots: AgentDialogEntry[];
now: number;
maxRows: number;
width: number | undefined;
}> = ({ snapshots, now, maxRows, width }) => {
const visibleAgents: LivePanelEntry[] = snapshots
.map((entry) => ({
...entry,
expired:

View file

@ -51,9 +51,9 @@ interface ToolGroupMessageProps {
/**
* True when this tool group is being rendered live (in
* `pendingHistoryItems`). False once it commits to Ink's `<Static>`.
* The subagent renderer uses this to suppress the live frame for
* foreground subagents (the pill+dialog handle live drill-down) while
* keeping the committed scrollback render unchanged.
* Currently consumed by upstream callers but not by the group body
* itself the subagent renderer used to gate its live frame on
* this; that gating moved to LiveAgentPanel + BackgroundTasksDialog.
*/
isPending?: boolean;
activeShellPtyId?: number | null;
@ -78,7 +78,10 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
availableTerminalHeight,
contentWidth,
isFocused = true,
isPending = false,
// `isPending` stays on the props interface for upstream compat
// (HistoryItemDisplay et al. forward it) but the group body no
// longer reads it. Skip the destructure so TS catches accidental
// re-introductions of dead state.
activeShellPtyId,
embeddedShellFocused,
memoryWriteCount,
@ -298,12 +301,6 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
isFocused &&
!toolAwaitingApproval &&
keyboardFocusedSubagentCallId === tool.callId;
// Show the waiting indicator only when this subagent genuinely has a
// pending confirmation AND another subagent holds the focus lock.
const isWaitingForOtherApproval =
isAgentWithPendingConfirmation(tool.resultDisplay) &&
focusedSubagentCallId !== null &&
focusedSubagentCallId !== tool.callId;
return (
<Box key={tool.callId} flexDirection="column" minHeight={1}>
<Box flexDirection="row" alignItems="center">
@ -328,8 +325,6 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
isAgentWithPendingConfirmation(tool.resultDisplay)
}
isFocused={isSubagentFocused}
isPending={isPending}
isWaitingForOtherApproval={isWaitingForOtherApproval}
/>
</Box>
{tool.status === ToolCallStatus.Confirming &&

View file

@ -103,8 +103,8 @@ vi.mock('../../utils/MarkdownDisplay.js', () => ({
}));
vi.mock('./ToolConfirmationMessage.js', () => ({
ToolConfirmationMessage: function MockToolConfirmationMessage() {
// Sentinel string lets `isPending && pendingConfirmation` tests
// assert the banner renders (instead of being suppressed).
// Sentinel string lets the focus-routed approval tests assert
// the banner renders (instead of being suppressed).
return <Text>MockApprovalPrompt</Text>;
},
}));
@ -315,9 +315,7 @@ describe('<ToolMessage />', () => {
status: 'running' | 'completed';
pendingConfirmation?: object;
};
isPending?: boolean;
isFocused?: boolean;
isWaitingForOtherApproval?: boolean;
}): ToolMessageProps => {
const resultDisplay = {
type: 'task_execution' as const,
@ -331,9 +329,7 @@ describe('<ToolMessage />', () => {
status: ToolCallStatus.Executing,
callId: 'gated-task-call',
forceShowResult: true, // mirror ToolGroupMessage's forceShowResult
isPending: overrides.isPending,
isFocused: overrides.isFocused,
isWaitingForOtherApproval: overrides.isWaitingForOtherApproval,
};
};
@ -347,7 +343,6 @@ describe('<ToolMessage />', () => {
taskPrompt: 'Search',
status: 'running',
},
isPending: true,
})}
/>,
StreamingState.Responding,
@ -371,7 +366,6 @@ describe('<ToolMessage />', () => {
taskPrompt: 'Already done',
status: 'completed',
},
isPending: false,
})}
/>,
StreamingState.Idle,
@ -392,7 +386,6 @@ describe('<ToolMessage />', () => {
status: 'running',
pendingConfirmation: {} as object,
},
isPending: true,
isFocused: true,
})}
/>,
@ -419,7 +412,6 @@ describe('<ToolMessage />', () => {
status: 'running',
pendingConfirmation: {} as object,
},
isPending: true,
isFocused: false,
})}
/>,

View file

@ -406,29 +406,8 @@ export interface ToolMessageProps extends IndividualToolCallDisplay {
* 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;
/**
* True when rendering inside `pendingHistoryItems` (live area), false once
* committed to `<Static>`. Subagents no longer paint an inline frame in
* either phase `LiveAgentPanel` (always-on roster) and
* `BackgroundTasksDialog` (Down-arrow detail) own that surface so this
* flag is purely informational at this layer; ToolGroupMessage still
* forwards it for non-subagent message types that key off live vs.
* committed rendering.
*/
isPending?: boolean;
/**
* Whether another subagent's approval currently holds the focus lock,
* blocking this one. Routed by `ToolGroupMessage`; vestigial for the
* subagent renderer (the queued marker reads the absence of `isFocused`
* directly), retained on the prop bag for call-site compatibility and
* future signaling needs.
*/
isWaitingForOtherApproval?: boolean;
}
export const ToolMessage: React.FC<ToolMessageProps> = ({
@ -446,11 +425,6 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({
config,
forceShowResult,
isFocused,
// isPending / isWaitingForOtherApproval flow into ToolMessage from
// ToolGroupMessage but no longer drive the subagent render path
// (LiveAgentPanel + BackgroundTasksDialog own that surface). They stay
// on the props interface so callers don't churn, but skipping the
// destructure here keeps the implementation honest about what's read.
executionStartTime,
}) => {
const settings = useSettings();