fix: preserve nested approvals in collapsed groups

This commit is contained in:
Danny Avila 2026-07-26 17:43:26 -04:00
parent 51d423fb00
commit 68b764c14c
2 changed files with 201 additions and 16 deletions

View file

@ -24,6 +24,27 @@ interface ToolMeta {
hasOutput: boolean;
}
type ToolCallWithNestedContent = Agents.ToolCall & {
subagent_content?: TMessageContentParts[];
};
function hasPendingApprovalInPart(part: TMessageContentParts): boolean {
if (part.type !== ContentTypes.TOOL_CALL) {
return false;
}
const toolCall = part[ContentTypes.TOOL_CALL] as ToolCallWithNestedContent | undefined;
if (!toolCall) {
return false;
}
if (toolCall.approval != null && (toolCall.output?.length ?? 0) === 0) {
return true;
}
return (
Array.isArray(toolCall.subagent_content) &&
toolCall.subagent_content.some(hasPendingApprovalInPart)
);
}
function getToolMeta(part: TMessageContentParts): ToolMeta | null {
if (part.type !== ContentTypes.TOOL_CALL) {
return null;
@ -107,18 +128,12 @@ export default function ToolCallGroup({
const mcpIconMap = useMCPIconMap();
const rootRef = useRef<HTMLDivElement | null>(null);
const cancelLayoutReconcileRef = useRef<(() => void) | null>(null);
const retainedForPendingApprovalRef = useRef(false);
const count = parts.length;
const toolMetadata = useMemo(() => parts.map((p) => getToolMeta(p.part)), [parts]);
const hasPendingApproval = useMemo(
() =>
parts.some(({ part }) => {
if (part.type !== ContentTypes.TOOL_CALL) {
return false;
}
const toolCall = part[ContentTypes.TOOL_CALL] as Agents.ToolCall | undefined;
return toolCall?.approval != null && (toolCall.output?.length ?? 0) === 0;
}),
() => parts.some(({ part }) => hasPendingApprovalInPart(part)),
[parts],
);
const allCompleted = useMemo(
@ -234,18 +249,37 @@ export default function ToolCallGroup({
if (event.target !== event.currentTarget) {
return;
}
if (isExpanded || hasPendingApproval) {
if (isExpanded) {
return;
}
// Approval controls own unsent local form state. Keep unresolved cards
// mounted (the collapsed panel is inert/hidden) so collapsing a batch
// cannot erase decisions the reviewer already made.
if (hasPendingApproval) {
// Approval controls own unsent local form state. Keep unresolved cards
// mounted (the collapsed panel is inert/hidden) so collapsing a batch
// cannot erase decisions the reviewer already made.
retainedForPendingApprovalRef.current = true;
return;
}
retainedForPendingApprovalRef.current = false;
setShouldRenderBody(false);
notifyLayoutChange();
},
[hasPendingApproval, isExpanded, notifyLayoutChange],
);
useEffect(() => {
if (isExpanded) {
retainedForPendingApprovalRef.current = false;
return;
}
if (!hasPendingApproval && retainedForPendingApprovalRef.current) {
// A completed collapse transition retained this body only to preserve
// approval form state. Release it once the last approval resolves.
retainedForPendingApprovalRef.current = false;
setShouldRenderBody(false);
notifyLayoutChange();
}
}, [hasPendingApproval, isExpanded, notifyLayoutChange]);
/** Category-aware header verb: subagents and questions read as their own
* category (with tense), everything else is the generic "Used N tools". */
const resolveGroupLabel = (): string => {

View file

@ -83,14 +83,14 @@ const makePart = (
},
}) as unknown as TMessageContentParts;
const makePendingApprovalPart = (id: string): TMessageContentParts =>
const makeApprovalPart = (id: string, output = ''): TMessageContentParts =>
({
type: ContentTypes.TOOL_CALL,
[ContentTypes.TOOL_CALL]: {
id,
name: 'approval_probe',
args: {},
output: '',
output,
approval: {
actionId: 'action-1',
allowed_decisions: ['approve', 'reject'],
@ -98,6 +98,21 @@ const makePendingApprovalPart = (id: string): TMessageContentParts =>
},
}) as unknown as TMessageContentParts;
const makeSubagentPart = (
id: string,
subagentContent: TMessageContentParts[],
): TMessageContentParts =>
({
type: ContentTypes.TOOL_CALL,
[ContentTypes.TOOL_CALL]: {
id,
name: Constants.SUBAGENT,
args: {},
output: '',
subagent_content: subagentContent,
},
}) as unknown as TMessageContentParts;
const imageAttachment: TAttachment = {
filename: 'foo.png',
filepath: '/files/foo.png',
@ -228,8 +243,8 @@ describe('ToolCallGroup image hoisting', () => {
it('keeps unresolved approval bodies mounted while the group is collapsed', () => {
const approvalParts = [
{ part: makePendingApprovalPart('t1'), idx: 0 },
{ part: makePendingApprovalPart('t2'), idx: 1 },
{ part: makeApprovalPart('t1'), idx: 0 },
{ part: makeApprovalPart('t2'), idx: 1 },
];
renderGroup({
...baseProps,
@ -253,6 +268,142 @@ describe('ToolCallGroup image hoisting', () => {
expect(screen.getByTestId('approval-1')).toBeInTheDocument();
});
it('keeps deeply nested unresolved approval bodies mounted while the group is collapsed', () => {
const nestedApprovalParts = [
{
part: makeSubagentPart('parent', [
makeSubagentPart('child', [makeApprovalPart('grandchild')]),
]),
idx: 0,
},
{ part: makePart('sibling'), idx: 1 },
];
renderGroup({
...baseProps,
parts: nestedApprovalParts,
renderPart: (_p: TMessageContentParts, idx: number) => (
<div data-testid={`nested-${idx}`} key={idx}>
{'nested'}
</div>
),
});
const button = screen.getByRole('button', { name: 'Used 2 tools' });
const collapsible = button.nextElementSibling as HTMLElement;
fireEvent.click(button);
fireEvent.transitionEnd(collapsible);
expect(button).toHaveAttribute('aria-expanded', 'false');
expect(screen.getByTestId('nested-0')).toBeInTheDocument();
expect(screen.getByTestId('nested-1')).toBeInTheDocument();
});
it('does not retain a collapsed group for an already resolved nested approval', () => {
const nestedApprovalParts = [
{
part: makeSubagentPart('parent', [
makeSubagentPart('child', [makeApprovalPart('grandchild', 'done')]),
]),
idx: 0,
},
{ part: makePart('sibling'), idx: 1 },
];
renderGroup({
...baseProps,
parts: nestedApprovalParts,
renderPart: (_p: TMessageContentParts, idx: number) => (
<div data-testid={`resolved-nested-${idx}`} key={idx}>
{'nested'}
</div>
),
});
const button = screen.getByRole('button', { name: 'Used 2 tools' });
const collapsible = button.nextElementSibling as HTMLElement;
fireEvent.click(button);
fireEvent.transitionEnd(collapsible);
expect(screen.queryByTestId('resolved-nested-0')).not.toBeInTheDocument();
expect(screen.queryByTestId('resolved-nested-1')).not.toBeInTheDocument();
});
it('unmounts retained approval bodies after every approval in a collapsed group resolves', async () => {
const renderPart = (_p: TMessageContentParts, idx: number) => (
<div data-testid={`retained-${idx}`} key={idx}>
{'approval'}
</div>
);
const propsFor = (
firstOutput = '',
secondOutput = '',
): React.ComponentProps<typeof ToolCallGroup> => ({
...baseProps,
parts: [
{ part: makeApprovalPart('t1', firstOutput), idx: 0 },
{ part: makeApprovalPart('t2', secondOutput), idx: 1 },
],
renderPart,
});
const { rerender } = renderGroup(propsFor());
const button = screen.getByRole('button', { name: 'Used 2 tools' });
const collapsible = button.nextElementSibling as HTMLElement;
fireEvent.click(button);
fireEvent.transitionEnd(collapsible);
expect(screen.getByTestId('retained-0')).toBeInTheDocument();
rerender(
<RecoilRoot>
<ToolCallGroup {...propsFor('first done')} />
</RecoilRoot>,
);
expect(screen.getByTestId('retained-0')).toBeInTheDocument();
expect(screen.getByTestId('retained-1')).toBeInTheDocument();
rerender(
<RecoilRoot>
<ToolCallGroup {...propsFor('first done', 'second done')} />
</RecoilRoot>,
);
await waitFor(() => {
expect(screen.queryByTestId('retained-0')).not.toBeInTheDocument();
expect(screen.queryByTestId('retained-1')).not.toBeInTheDocument();
});
});
it('waits for an active collapse transition before unmounting resolved approval bodies', () => {
const renderPart = (_p: TMessageContentParts, idx: number) => (
<div data-testid={`transitioning-${idx}`} key={idx}>
{'approval'}
</div>
);
const propsFor = (output = ''): React.ComponentProps<typeof ToolCallGroup> => ({
...baseProps,
parts: [
{ part: makeApprovalPart('t1', output), idx: 0 },
{ part: makeApprovalPart('t2', output), idx: 1 },
],
renderPart,
});
const { rerender } = renderGroup(propsFor());
const button = screen.getByRole('button', { name: 'Used 2 tools' });
const collapsible = button.nextElementSibling as HTMLElement;
fireEvent.click(button);
rerender(
<RecoilRoot>
<ToolCallGroup {...propsFor('done')} />
</RecoilRoot>,
);
expect(screen.getByTestId('transitioning-0')).toBeInTheDocument();
expect(screen.getByTestId('transitioning-1')).toBeInTheDocument();
fireEvent.transitionEnd(collapsible);
expect(screen.queryByTestId('transitioning-0')).not.toBeInTheDocument();
expect(screen.queryByTestId('transitioning-1')).not.toBeInTheDocument();
});
it('reconciles layout after the group collapses from an expanded state', async () => {
renderGroup(baseProps);