Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,18 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, it, expect, vi } from 'vitest';
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { ToolConfirmationMessage } from './ToolConfirmationMessage.js';
import type {
SerializableConfirmationDetails,
ToolCallConfirmationDetails,
Config,
import {
type SerializableConfirmationDetails,
type ToolCallConfirmationDetails,
type Config,
ToolConfirmationOutcome,
} from '@google/gemini-cli-core';
import { renderWithProviders } from '../../../test-utils/render.js';
import { createMockSettings } from '../../../test-utils/settings.js';
import { useToolActions } from '../../contexts/ToolActionsContext.js';
import { act } from 'react';

vi.mock('../../contexts/ToolActionsContext.js', async (importOriginal) => {
const actual =
Expand Down Expand Up @@ -646,4 +648,63 @@ describe('ToolConfirmationMessage', () => {
expect(output).not.toContain('Invocation Arguments:');
unmount();
});

describe('ESCAPE key behavior', () => {
beforeEach(() => {
vi.useFakeTimers();
});

afterEach(() => {
vi.useRealTimers();
vi.restoreAllMocks();
});

it('should call confirm(Cancel) asynchronously via useEffect when ESC is pressed', async () => {
const mockConfirm = vi.fn().mockResolvedValue(undefined);

vi.mocked(useToolActions).mockReturnValue({
confirm: mockConfirm,
cancel: vi.fn(),
isDiffingEnabled: false,
});

const confirmationDetails: SerializableConfirmationDetails = {
type: 'info',
title: 'Confirm Web Fetch',
prompt: 'https://example.com',
urls: ['https://example.com'],
};

const { stdin, waitUntilReady, unmount } = renderWithProviders(
<ToolConfirmationMessage
callId="test-call-id"
confirmationDetails={confirmationDetails}
config={mockConfig}
getPreferredEditor={vi.fn()}
availableTerminalHeight={30}
terminalWidth={80}
/>,
);
await waitUntilReady();

stdin.write('\x1b');

// To assert that the confirmation happens asynchronously (via useEffect) rather than
// synchronously (directly inside the keystroke handler), we must run our assertion
// *inside* the act() block.
await act(async () => {
await vi.runAllTimersAsync();
expect(mockConfirm).not.toHaveBeenCalled();
});

// Now that the act() block has returned, React flushes the useEffect, calling handleConfirm.
expect(mockConfirm).toHaveBeenCalledWith(
'test-call-id',
ToolConfirmationOutcome.Cancel,
undefined,
);

unmount();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import type React from 'react';
import { useMemo, useCallback, useState } from 'react';
import { useEffect, useMemo, useCallback, useState } from 'react';
import { Box, Text } from 'ink';
import { DiffRenderer } from './DiffRenderer.js';
import { RenderInline } from '../../utils/InlineMarkdownRenderer.js';
Expand Down Expand Up @@ -79,6 +79,7 @@ export const ToolConfirmationMessage: React.FC<
callId,
expanded: false,
});
const [isCancelling, setIsCancelling] = useState(false);
const isMcpToolDetailsExpanded =
mcpDetailsExpansionState.callId === callId
? mcpDetailsExpansionState.expanded
Expand Down Expand Up @@ -183,7 +184,7 @@ export const ToolConfirmationMessage: React.FC<
return true;
}
if (keyMatchers[Command.ESCAPE](key)) {
handleConfirm(ToolConfirmationOutcome.Cancel);
setIsCancelling(true);
return true;
}
if (keyMatchers[Command.QUIT](key)) {
Expand All @@ -196,6 +197,20 @@ export const ToolConfirmationMessage: React.FC<
{ isActive: isFocused, priority: true },
);

// TODO(#23009): Remove this hack once we migrate to the new renderer.
// Why useEffect is used here instead of calling handleConfirm directly:
// There is a race condition where calling handleConfirm immediately upon
// keypress removes the tool UI component while the UI is in an expanded state.
// This simultaneously triggers setConstrainHeight, causing render two footers.
// By bridging the cancel action through state (isCancelling) and this useEffect,
// we delay handleConfirm until the next render cycle, ensuring setConstrainHeight
// resolves properly first.
useEffect(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is already code in AppContainer calling refreshStatic when appropriate on changes to constrainHeight. Please make these changes there instead of adding this logic here.
Also make sure we are only doing this when not in alternate buffer mode.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
Once I moved the handleConfirm to useEffect, the constrainHeight is actually set by handleGlobalKeypress in AppContainer.

if (isCancelling) {
handleConfirm(ToolConfirmationOutcome.Cancel);
}
}, [isCancelling, handleConfirm]);

const handleSelect = useCallback(
(item: ToolConfirmationOutcome) => handleConfirm(item),
[handleConfirm],
Expand Down
Loading