mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-11 06:31:01 -07:00
Fix cluster of bugs in the settings dialog. (#17628)
This commit is contained in:
@@ -307,6 +307,26 @@ describe('SettingsDialog', () => {
|
||||
// Use snapshot to capture visual layout including indicators
|
||||
expect(output).toMatchSnapshot();
|
||||
});
|
||||
|
||||
it('should use almost full height of the window but no more when the window height is 25 rows', async () => {
|
||||
const settings = createMockSettings();
|
||||
const onSelect = vi.fn();
|
||||
|
||||
// Render with a fixed height of 25 rows
|
||||
const { lastFrame } = renderDialog(settings, onSelect, {
|
||||
availableTerminalHeight: 25,
|
||||
});
|
||||
|
||||
// Wait for the dialog to render
|
||||
await waitFor(() => {
|
||||
const output = lastFrame();
|
||||
expect(output).toBeDefined();
|
||||
const lines = output!.split('\n');
|
||||
|
||||
expect(lines.length).toBeGreaterThanOrEqual(24);
|
||||
expect(lines.length).toBeLessThanOrEqual(25);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('Setting Descriptions', () => {
|
||||
@@ -1072,6 +1092,87 @@ describe('SettingsDialog', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('Restart and Search Conflict Regression', () => {
|
||||
it('should prioritize restart request over search text box when showRestartPrompt is true', async () => {
|
||||
vi.mocked(getSettingsSchema).mockReturnValue(TOOLS_SHELL_FAKE_SCHEMA);
|
||||
const settings = createMockSettings();
|
||||
const onRestartRequest = vi.fn();
|
||||
|
||||
const { stdin, lastFrame, unmount } = renderDialog(settings, vi.fn(), {
|
||||
onRestartRequest,
|
||||
});
|
||||
|
||||
// Wait for initial render
|
||||
await waitFor(() => expect(lastFrame()).toContain('Show Color'));
|
||||
|
||||
// Navigate to "Enable Interactive Shell" (second item in TOOLS_SHELL_FAKE_SCHEMA)
|
||||
act(() => {
|
||||
stdin.write(TerminalKeys.DOWN_ARROW);
|
||||
});
|
||||
|
||||
// Wait for navigation to complete
|
||||
await waitFor(() =>
|
||||
expect(lastFrame()).toContain('● Enable Interactive Shell'),
|
||||
);
|
||||
|
||||
// Toggle it to trigger restart required
|
||||
act(() => {
|
||||
stdin.write(TerminalKeys.ENTER);
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
expect(lastFrame()).toContain(
|
||||
'To see changes, Gemini CLI must be restarted',
|
||||
);
|
||||
});
|
||||
|
||||
// Press 'r' - it should call onRestartRequest, NOT be handled by search
|
||||
act(() => {
|
||||
stdin.write('r');
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
expect(onRestartRequest).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should hide search box when showRestartPrompt is true', async () => {
|
||||
vi.mocked(getSettingsSchema).mockReturnValue(TOOLS_SHELL_FAKE_SCHEMA);
|
||||
const settings = createMockSettings();
|
||||
|
||||
const { stdin, lastFrame, unmount } = renderDialog(settings, vi.fn());
|
||||
|
||||
// Search box should be visible initially (searchPlaceholder)
|
||||
expect(lastFrame()).toContain('Search to filter');
|
||||
|
||||
// Navigate to "Enable Interactive Shell" and toggle it
|
||||
act(() => {
|
||||
stdin.write(TerminalKeys.DOWN_ARROW);
|
||||
});
|
||||
|
||||
await waitFor(() =>
|
||||
expect(lastFrame()).toContain('● Enable Interactive Shell'),
|
||||
);
|
||||
|
||||
act(() => {
|
||||
stdin.write(TerminalKeys.ENTER);
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
expect(lastFrame()).toContain(
|
||||
'To see changes, Gemini CLI must be restarted',
|
||||
);
|
||||
});
|
||||
|
||||
// Search box should now be hidden
|
||||
expect(lastFrame()).not.toContain('Search to filter');
|
||||
|
||||
unmount();
|
||||
});
|
||||
});
|
||||
|
||||
describe('String Settings Editing', () => {
|
||||
it('should allow editing and committing a string setting', async () => {
|
||||
let settings = createMockSettings({ 'a.string.setting': 'initial' });
|
||||
|
||||
@@ -601,6 +601,7 @@ export function SettingsDialog({
|
||||
(key: Key, _currentItem: SettingsDialogItem | undefined): boolean => {
|
||||
// 'r' key for restart
|
||||
if (showRestartPrompt && key.sequence === 'r') {
|
||||
saveRestartRequiredSettings();
|
||||
setShowRestartPrompt(false);
|
||||
setModifiedSettings(new Set());
|
||||
setRestartRequiredSettings(new Set());
|
||||
@@ -609,80 +610,90 @@ export function SettingsDialog({
|
||||
}
|
||||
return false;
|
||||
},
|
||||
[showRestartPrompt, onRestartRequest],
|
||||
[showRestartPrompt, onRestartRequest, saveRestartRequiredSettings],
|
||||
);
|
||||
|
||||
// Calculate effective max items and scope visibility based on terminal height
|
||||
const { effectiveMaxItemsToShow, showScopeSelection } = useMemo(() => {
|
||||
// Only show scope selector if we have a workspace
|
||||
const hasWorkspace = settings.workspace.path !== undefined;
|
||||
const { effectiveMaxItemsToShow, showScopeSelection, showSearch } =
|
||||
useMemo(() => {
|
||||
// Only show scope selector if we have a workspace
|
||||
const hasWorkspace = settings.workspace.path !== undefined;
|
||||
|
||||
if (!availableTerminalHeight) {
|
||||
return {
|
||||
effectiveMaxItemsToShow: Math.min(MAX_ITEMS_TO_SHOW, items.length),
|
||||
showScopeSelection: hasWorkspace,
|
||||
};
|
||||
}
|
||||
// Search box is hidden when restart prompt is shown to save space and avoid key conflicts
|
||||
const shouldShowSearch = !showRestartPrompt;
|
||||
|
||||
// Layout constants
|
||||
const DIALOG_PADDING = 2; // Top and bottom borders
|
||||
const SETTINGS_TITLE_HEIGHT = 1;
|
||||
const SEARCH_BOX_HEIGHT = 3;
|
||||
const SCROLL_ARROWS_HEIGHT = 2;
|
||||
const SPACING_HEIGHT = 2;
|
||||
const SCOPE_SELECTION_HEIGHT = 4;
|
||||
const BOTTOM_HELP_TEXT_HEIGHT = 1;
|
||||
const RESTART_PROMPT_HEIGHT = showRestartPrompt ? 1 : 0;
|
||||
const ITEM_HEIGHT = 3; // Label + description + spacing
|
||||
|
||||
const currentAvailableHeight = availableTerminalHeight - DIALOG_PADDING;
|
||||
|
||||
const baseFixedHeight =
|
||||
SETTINGS_TITLE_HEIGHT +
|
||||
SEARCH_BOX_HEIGHT +
|
||||
SCROLL_ARROWS_HEIGHT +
|
||||
SPACING_HEIGHT +
|
||||
BOTTOM_HELP_TEXT_HEIGHT +
|
||||
RESTART_PROMPT_HEIGHT;
|
||||
|
||||
// Calculate max items with scope selector
|
||||
const heightWithScope = baseFixedHeight + SCOPE_SELECTION_HEIGHT;
|
||||
const availableForItemsWithScope = currentAvailableHeight - heightWithScope;
|
||||
const maxItemsWithScope = Math.max(
|
||||
1,
|
||||
Math.floor(availableForItemsWithScope / ITEM_HEIGHT),
|
||||
);
|
||||
|
||||
// Calculate max items without scope selector
|
||||
const availableForItemsWithoutScope =
|
||||
currentAvailableHeight - baseFixedHeight;
|
||||
const maxItemsWithoutScope = Math.max(
|
||||
1,
|
||||
Math.floor(availableForItemsWithoutScope / ITEM_HEIGHT),
|
||||
);
|
||||
|
||||
// In small terminals, hide scope selector if it would allow more items to show
|
||||
let shouldShowScope = hasWorkspace;
|
||||
let maxItems = maxItemsWithScope;
|
||||
|
||||
if (hasWorkspace && availableTerminalHeight < 25) {
|
||||
// Hide scope selector if it gains us more than 1 extra item
|
||||
if (maxItemsWithoutScope > maxItemsWithScope + 1) {
|
||||
shouldShowScope = false;
|
||||
maxItems = maxItemsWithoutScope;
|
||||
if (!availableTerminalHeight) {
|
||||
return {
|
||||
effectiveMaxItemsToShow: Math.min(MAX_ITEMS_TO_SHOW, items.length),
|
||||
showScopeSelection: hasWorkspace,
|
||||
showSearch: shouldShowSearch,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
effectiveMaxItemsToShow: Math.min(maxItems, items.length),
|
||||
showScopeSelection: shouldShowScope,
|
||||
};
|
||||
}, [
|
||||
availableTerminalHeight,
|
||||
items.length,
|
||||
settings.workspace.path,
|
||||
showRestartPrompt,
|
||||
]);
|
||||
// Layout constants based on BaseSettingsDialog structure:
|
||||
// 4 for border (2) and padding (2)
|
||||
const DIALOG_PADDING = 4;
|
||||
const SETTINGS_TITLE_HEIGHT = 1;
|
||||
// 3 for box + 1 for marginTop + 1 for spacing after
|
||||
const SEARCH_SECTION_HEIGHT = shouldShowSearch ? 5 : 0;
|
||||
const SCROLL_ARROWS_HEIGHT = 2;
|
||||
const ITEMS_SPACING_AFTER = 1;
|
||||
// 1 for Label + 3 for Scope items + 1 for spacing after
|
||||
const SCOPE_SECTION_HEIGHT = hasWorkspace ? 5 : 0;
|
||||
const HELP_TEXT_HEIGHT = 1;
|
||||
const RESTART_PROMPT_HEIGHT = showRestartPrompt ? 1 : 0;
|
||||
const ITEM_HEIGHT = 3; // Label + description + spacing
|
||||
|
||||
const currentAvailableHeight = availableTerminalHeight - DIALOG_PADDING;
|
||||
|
||||
const baseFixedHeight =
|
||||
SETTINGS_TITLE_HEIGHT +
|
||||
SEARCH_SECTION_HEIGHT +
|
||||
SCROLL_ARROWS_HEIGHT +
|
||||
ITEMS_SPACING_AFTER +
|
||||
HELP_TEXT_HEIGHT +
|
||||
RESTART_PROMPT_HEIGHT;
|
||||
|
||||
// Calculate max items with scope selector
|
||||
const heightWithScope = baseFixedHeight + SCOPE_SECTION_HEIGHT;
|
||||
const availableForItemsWithScope =
|
||||
currentAvailableHeight - heightWithScope;
|
||||
const maxItemsWithScope = Math.max(
|
||||
1,
|
||||
Math.floor(availableForItemsWithScope / ITEM_HEIGHT),
|
||||
);
|
||||
|
||||
// Calculate max items without scope selector
|
||||
const availableForItemsWithoutScope =
|
||||
currentAvailableHeight - baseFixedHeight;
|
||||
const maxItemsWithoutScope = Math.max(
|
||||
1,
|
||||
Math.floor(availableForItemsWithoutScope / ITEM_HEIGHT),
|
||||
);
|
||||
|
||||
// In small terminals, hide scope selector if it would allow more items to show
|
||||
let shouldShowScope = hasWorkspace;
|
||||
let maxItems = maxItemsWithScope;
|
||||
|
||||
if (hasWorkspace && availableTerminalHeight < 25) {
|
||||
// Hide scope selector if it gains us more than 1 extra item
|
||||
if (maxItemsWithoutScope > maxItemsWithScope + 1) {
|
||||
shouldShowScope = false;
|
||||
maxItems = maxItemsWithoutScope;
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
effectiveMaxItemsToShow: Math.min(maxItems, items.length),
|
||||
showScopeSelection: shouldShowScope,
|
||||
showSearch: shouldShowSearch,
|
||||
};
|
||||
}, [
|
||||
availableTerminalHeight,
|
||||
items.length,
|
||||
settings.workspace.path,
|
||||
showRestartPrompt,
|
||||
]);
|
||||
|
||||
// Footer content for restart prompt
|
||||
const footerContent = showRestartPrompt ? (
|
||||
@@ -695,7 +706,8 @@ export function SettingsDialog({
|
||||
return (
|
||||
<BaseSettingsDialog
|
||||
title="Settings"
|
||||
searchEnabled={true}
|
||||
borderColor={showRestartPrompt ? theme.status.warning : undefined}
|
||||
searchEnabled={showSearch}
|
||||
searchBuffer={searchBuffer}
|
||||
items={items}
|
||||
showScopeSelector={showScopeSelection}
|
||||
|
||||
@@ -50,6 +50,8 @@ export interface BaseSettingsDialogProps {
|
||||
// Header
|
||||
/** Dialog title displayed at the top */
|
||||
title: string;
|
||||
/** Optional border color for the dialog */
|
||||
borderColor?: string;
|
||||
|
||||
// Search (optional feature)
|
||||
/** Whether to show the search input. Default: true */
|
||||
@@ -107,6 +109,7 @@ export interface BaseSettingsDialogProps {
|
||||
*/
|
||||
export function BaseSettingsDialog({
|
||||
title,
|
||||
borderColor,
|
||||
searchEnabled = true,
|
||||
searchPlaceholder = 'Search to filter',
|
||||
searchBuffer,
|
||||
@@ -390,7 +393,7 @@ export function BaseSettingsDialog({
|
||||
return (
|
||||
<Box
|
||||
borderStyle="round"
|
||||
borderColor={theme.border.default}
|
||||
borderColor={borderColor ?? theme.border.default}
|
||||
flexDirection="row"
|
||||
padding={1}
|
||||
width="100%"
|
||||
|
||||
Reference in New Issue
Block a user