Skip to content

Commit a37844e

Browse files
authored
Code review fixes for show question mark pr. (google-gemini#18480)
1 parent 6f1a5bf commit a37844e

11 files changed

Lines changed: 298 additions & 235 deletions

packages/cli/src/ui/components/Composer.test.tsx

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
import { describe, it, expect, vi } from 'vitest';
88
import { render } from '../../test-utils/render.js';
9-
import { Text } from 'ink';
9+
import { Box, Text } from 'ink';
1010
import { Composer } from './Composer.js';
1111
import { UIStateContext, type UIState } from '../contexts/UIStateContext.js';
1212
import {
@@ -598,4 +598,29 @@ describe('Composer', () => {
598598
);
599599
});
600600
});
601+
602+
describe('Shortcuts Hint', () => {
603+
it('hides shortcuts hint when a action is required (e.g. dialog is open)', () => {
604+
const uiState = createMockUIState({
605+
customDialog: (
606+
<Box>
607+
<Text>Test Dialog</Text>
608+
<Text>Test Content</Text>
609+
</Box>
610+
),
611+
});
612+
613+
const { lastFrame } = renderComposer(uiState);
614+
615+
expect(lastFrame()).not.toContain('ShortcutsHint');
616+
});
617+
618+
it('keeps shortcuts hint visible when no action is required', () => {
619+
const uiState = createMockUIState();
620+
621+
const { lastFrame } = renderComposer(uiState);
622+
623+
expect(lastFrame()).toContain('ShortcutsHint');
624+
});
625+
});
601626
});

packages/cli/src/ui/components/Composer.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,11 +136,11 @@ export const Composer = ({ isFocused = true }: { isFocused?: boolean }) => {
136136
flexDirection="column"
137137
alignItems={isNarrow ? 'flex-start' : 'flex-end'}
138138
>
139-
<ShortcutsHint />
139+
{!hasPendingActionRequired && <ShortcutsHint />}
140140
</Box>
141141
</Box>
142142
{uiState.shortcutsHelpVisible && <ShortcutsHelp />}
143-
<HorizontalLine width={uiState.terminalWidth} />
143+
<HorizontalLine />
144144
<Box
145145
justifyContent={
146146
settings.merged.ui.hideContextSummary

packages/cli/src/ui/components/InputPrompt.test.tsx

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4028,6 +4028,55 @@ describe('InputPrompt', () => {
40284028
});
40294029
});
40304030
});
4031+
4032+
describe('shortcuts help visibility', () => {
4033+
it.each([
4034+
{
4035+
name: 'terminal paste event occurs',
4036+
input: '\x1b[200~pasted text\x1b[201~',
4037+
},
4038+
{
4039+
name: 'Ctrl+V (PASTE_CLIPBOARD) is pressed',
4040+
input: '\x16',
4041+
setupMocks: () => {
4042+
vi.mocked(clipboardUtils.clipboardHasImage).mockResolvedValue(false);
4043+
vi.mocked(clipboardy.read).mockResolvedValue('clipboard text');
4044+
},
4045+
},
4046+
{
4047+
name: 'mouse right-click paste occurs',
4048+
input: '\x1b[<2;1;1m',
4049+
mouseEventsEnabled: true,
4050+
setupMocks: () => {
4051+
vi.mocked(clipboardUtils.clipboardHasImage).mockResolvedValue(false);
4052+
vi.mocked(clipboardy.read).mockResolvedValue('clipboard text');
4053+
},
4054+
},
4055+
])(
4056+
'should close shortcuts help when a $name',
4057+
async ({ input, setupMocks, mouseEventsEnabled }) => {
4058+
setupMocks?.();
4059+
const setShortcutsHelpVisible = vi.fn();
4060+
const { stdin, unmount } = renderWithProviders(
4061+
<InputPrompt {...props} />,
4062+
{
4063+
uiState: { shortcutsHelpVisible: true },
4064+
uiActions: { setShortcutsHelpVisible },
4065+
mouseEventsEnabled,
4066+
},
4067+
);
4068+
4069+
await act(async () => {
4070+
stdin.write(input);
4071+
});
4072+
4073+
await waitFor(() => {
4074+
expect(setShortcutsHelpVisible).toHaveBeenCalledWith(false);
4075+
});
4076+
unmount();
4077+
},
4078+
);
4079+
});
40314080
});
40324081

40334082
function clean(str: string | undefined): string {

packages/cli/src/ui/components/InputPrompt.tsx

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,9 @@ export const InputPrompt: React.FC<InputPromptProps> = ({
359359

360360
// Handle clipboard image pasting with Ctrl+V
361361
const handleClipboardPaste = useCallback(async () => {
362+
if (shortcutsHelpVisible) {
363+
setShortcutsHelpVisible(false);
364+
}
362365
try {
363366
if (await clipboardHasImage()) {
364367
const imagePath = await saveClipboardImage(config.getTargetDir());
@@ -403,7 +406,14 @@ export const InputPrompt: React.FC<InputPromptProps> = ({
403406
} catch (error) {
404407
debugLogger.error('Error handling paste:', error);
405408
}
406-
}, [buffer, config, stdout, settings]);
409+
}, [
410+
buffer,
411+
config,
412+
stdout,
413+
settings,
414+
shortcutsHelpVisible,
415+
setShortcutsHelpVisible,
416+
]);
407417

408418
useMouseClick(
409419
innerBoxRef,
@@ -553,6 +563,9 @@ export const InputPrompt: React.FC<InputPromptProps> = ({
553563
}
554564

555565
if (key.name === 'paste') {
566+
if (shortcutsHelpVisible) {
567+
setShortcutsHelpVisible(false);
568+
}
556569
// Record paste time to prevent accidental auto-submission
557570
if (!isTerminalPasteTrusted(kittyProtocol.enabled)) {
558571
setRecentUnsafePasteTime(Date.now());
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/**
2+
* @license
3+
* Copyright 2025 Google LLC
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
import { describe, it, expect, afterEach, vi } from 'vitest';
8+
import { renderWithProviders } from '../../test-utils/render.js';
9+
import { ShortcutsHelp } from './ShortcutsHelp.js';
10+
11+
describe('ShortcutsHelp', () => {
12+
const originalPlatform = process.platform;
13+
14+
afterEach(() => {
15+
Object.defineProperty(process, 'platform', {
16+
value: originalPlatform,
17+
});
18+
vi.restoreAllMocks();
19+
});
20+
21+
const testCases = [
22+
{ name: 'wide', width: 100 },
23+
{ name: 'narrow', width: 40 },
24+
];
25+
26+
const platforms = [
27+
{ name: 'mac', value: 'darwin' },
28+
{ name: 'linux', value: 'linux' },
29+
] as const;
30+
31+
it.each(
32+
platforms.flatMap((platform) =>
33+
testCases.map((testCase) => ({ ...testCase, platform })),
34+
),
35+
)(
36+
'renders correctly in $name mode on $platform.name',
37+
({ width, platform }) => {
38+
Object.defineProperty(process, 'platform', {
39+
value: platform.value,
40+
});
41+
42+
const { lastFrame } = renderWithProviders(<ShortcutsHelp />, {
43+
width,
44+
});
45+
expect(lastFrame()).toContain('shell mode');
46+
expect(lastFrame()).toMatchSnapshot();
47+
},
48+
);
49+
});

0 commit comments

Comments
 (0)