From b211f30d95870edfa0798e15b074969c5bf5a3e7 Mon Sep 17 00:00:00 2001 From: Sehoon Shon Date: Tue, 17 Mar 2026 15:08:45 -0400 Subject: [PATCH] fix(cli): override j/k navigation in settings dialog to fix search input conflict (#22800) --- .../src/ui/components/SettingsDialog.test.tsx | 33 +++++++++++++++++-- .../cli/src/ui/components/SettingsDialog.tsx | 20 +++++++++++ .../components/shared/BaseSettingsDialog.tsx | 9 +++-- 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/ui/components/SettingsDialog.test.tsx b/packages/cli/src/ui/components/SettingsDialog.test.tsx index be99dfcc26..4a2fd6a854 100644 --- a/packages/cli/src/ui/components/SettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx @@ -52,6 +52,8 @@ enum TerminalKeys { RIGHT_ARROW = '\u001B[C', ESCAPE = '\u001B', BACKSPACE = '\u0008', + CTRL_P = '\u0010', + CTRL_N = '\u000E', } vi.mock('../../config/settingsSchema.js', async (importOriginal) => { @@ -357,9 +359,9 @@ describe('SettingsDialog', () => { up: TerminalKeys.UP_ARROW, }, { - name: 'vim keys (j/k)', - down: 'j', - up: 'k', + name: 'emacs keys (Ctrl+P/N)', + down: TerminalKeys.CTRL_N, + up: TerminalKeys.CTRL_P, }, ])('should navigate with $name', async ({ down, up }) => { const settings = createMockSettings(); @@ -397,6 +399,31 @@ describe('SettingsDialog', () => { unmount(); }); + it('should allow j and k characters to be typed in search without triggering navigation', async () => { + const settings = createMockSettings(); + const onSelect = vi.fn(); + const { lastFrame, stdin, waitUntilReady, unmount } = renderDialog( + settings, + onSelect, + ); + await waitUntilReady(); + + // Enter 'j' and 'k' in search + await act(async () => stdin.write('j')); + await waitUntilReady(); + await act(async () => stdin.write('k')); + await waitUntilReady(); + + await waitFor(() => { + const frame = lastFrame(); + // The search box should contain 'jk' + expect(frame).toContain('jk'); + // Since 'jk' doesn't match any setting labels, it should say "No matches found." + expect(frame).toContain('No matches found.'); + }); + unmount(); + }); + it('wraps around when at the top of the list', async () => { const settings = createMockSettings(); const onSelect = vi.fn(); diff --git a/packages/cli/src/ui/components/SettingsDialog.tsx b/packages/cli/src/ui/components/SettingsDialog.tsx index 82965bda71..994bde6ed3 100644 --- a/packages/cli/src/ui/components/SettingsDialog.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.tsx @@ -43,6 +43,8 @@ import { BaseSettingsDialog, type SettingsDialogItem, } from './shared/BaseSettingsDialog.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; +import { Command, KeyBinding } from '../key/keyBindings.js'; interface FzfResult { item: string; @@ -60,6 +62,11 @@ interface SettingsDialogProps { const MAX_ITEMS_TO_SHOW = 8; +const KEY_UP = new KeyBinding('up'); +const KEY_CTRL_P = new KeyBinding('ctrl+p'); +const KEY_DOWN = new KeyBinding('down'); +const KEY_CTRL_N = new KeyBinding('ctrl+n'); + // Create a snapshot of the initial per-scope state of Restart Required Settings // This creates a nested map of the form // restartRequiredSetting -> Map { scopeName -> value } @@ -336,6 +343,18 @@ export function SettingsDialog({ onSelect(undefined, selectedScope as SettingScope); }, [onSelect, selectedScope]); + const globalKeyMatchers = useKeyMatchers(); + const settingsKeyMatchers = useMemo( + () => ({ + ...globalKeyMatchers, + [Command.DIALOG_NAVIGATION_UP]: (key: Key) => + KEY_UP.matches(key) || KEY_CTRL_P.matches(key), + [Command.DIALOG_NAVIGATION_DOWN]: (key: Key) => + KEY_DOWN.matches(key) || KEY_CTRL_N.matches(key), + }), + [globalKeyMatchers], + ); + // Custom key handler for restart key const handleKeyPress = useCallback( (key: Key, _currentItem: SettingsDialogItem | undefined): boolean => { @@ -371,6 +390,7 @@ export function SettingsDialog({ onItemClear={handleItemClear} onClose={handleClose} onKeyPress={handleKeyPress} + keyMatchers={settingsKeyMatchers} footer={ showRestartPrompt ? { diff --git a/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx b/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx index d96646e8a5..804633fe15 100644 --- a/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx +++ b/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx @@ -19,7 +19,7 @@ import { TextInput } from './TextInput.js'; import type { TextBuffer } from './text-buffer.js'; import { cpSlice, cpLen, cpIndexToOffset } from '../../utils/textUtils.js'; import { useKeypress, type Key } from '../../hooks/useKeypress.js'; -import { Command } from '../../key/keyMatchers.js'; +import { Command, type KeyMatchers } from '../../key/keyMatchers.js'; import { useSettingsNavigation } from '../../hooks/useSettingsNavigation.js'; import { useInlineEditBuffer } from '../../hooks/useInlineEditBuffer.js'; import { formatCommand } from '../../key/keybindingUtils.js'; @@ -103,6 +103,9 @@ export interface BaseSettingsDialogProps { currentItem: SettingsDialogItem | undefined, ) => boolean; + /** Optional override for key matchers used for navigation. */ + keyMatchers?: KeyMatchers; + /** Available terminal height for dynamic windowing */ availableHeight?: number; @@ -134,10 +137,12 @@ export function BaseSettingsDialog({ onItemClear, onClose, onKeyPress, + keyMatchers: customKeyMatchers, availableHeight, footer, }: BaseSettingsDialogProps): React.JSX.Element { - const keyMatchers = useKeyMatchers(); + const globalKeyMatchers = useKeyMatchers(); + const keyMatchers = customKeyMatchers ?? globalKeyMatchers; // Calculate effective max items and scope visibility based on terminal height const { effectiveMaxItemsToShow, finalShowScopeSelector } = useMemo(() => { const initialShowScope = showScopeSelector;