From 8d968f3d715769496210453b2bb46d267a598fdd Mon Sep 17 00:00:00 2001 From: ChangeHow Date: Thu, 6 Aug 2026 01:33:31 +0800 Subject: [PATCH] fix(ui): enforce shortcut conflict rules --- .../chat/composer/ui/DraftTargetSelectors.tsx | 8 +- .../openchamber/KeyboardShortcutsSettings.tsx | 1 - .../ShortcutRecordingDialog.test.ts | 26 ++-- .../openchamber/ShortcutRecordingDialog.tsx | 116 +++++++++--------- packages/ui/src/components/ui/select.tsx | 6 +- .../ui/src/lib/i18n/messages/de.settings.ts | 20 +++ .../ui/src/lib/i18n/messages/en.settings.ts | 7 +- .../ui/src/lib/i18n/messages/es.settings.ts | 7 +- .../ui/src/lib/i18n/messages/fr.settings.ts | 5 +- .../ui/src/lib/i18n/messages/ja.settings.ts | 5 +- .../ui/src/lib/i18n/messages/ko.settings.ts | 5 +- .../ui/src/lib/i18n/messages/pl.settings.ts | 5 +- .../src/lib/i18n/messages/pt-BR.settings.ts | 5 +- .../ui/src/lib/i18n/messages/uk.settings.ts | 5 +- .../src/lib/i18n/messages/zh-CN.settings.ts | 5 +- .../src/lib/i18n/messages/zh-TW.settings.ts | 5 +- .../ui/src/lib/shortcuts/DOCUMENTATION.md | 6 +- packages/ui/src/lib/shortcuts/index.ts | 2 + packages/ui/src/lib/shortcuts/schema.test.ts | 22 ++++ packages/ui/src/lib/shortcuts/schema.ts | 25 ++++ 20 files changed, 184 insertions(+), 102 deletions(-) diff --git a/packages/ui/src/components/chat/composer/ui/DraftTargetSelectors.tsx b/packages/ui/src/components/chat/composer/ui/DraftTargetSelectors.tsx index 0d62e91f..32c5dbbb 100644 --- a/packages/ui/src/components/chat/composer/ui/DraftTargetSelectors.tsx +++ b/packages/ui/src/components/chat/composer/ui/DraftTargetSelectors.tsx @@ -148,7 +148,7 @@ export function DraftTargetSelectors(props: DraftTargetProps) { {projects.map((project) => ( - + {} ))} @@ -176,7 +176,7 @@ export function DraftTargetSelectors(props: DraftTargetProps) { {projectRootBranchOption ? ( {t('chat.chatInput.projectRoot')} - + {projectRootBranchOption.label} @@ -195,13 +195,13 @@ export function DraftTargetSelectors(props: DraftTargetProps) { {worktreeBranchOptions.map((option) => ( - + {option.pending ? '⏳ ' : ''}{option.label} ))} {selectedDirectory && !selectedBranchIsKnown ? ( - + {selectedBranchLabel} ) : null} diff --git a/packages/ui/src/components/sections/openchamber/KeyboardShortcutsSettings.tsx b/packages/ui/src/components/sections/openchamber/KeyboardShortcutsSettings.tsx index 15591474..bd36e40d 100644 --- a/packages/ui/src/components/sections/openchamber/KeyboardShortcutsSettings.tsx +++ b/packages/ui/src/components/sections/openchamber/KeyboardShortcutsSettings.tsx @@ -121,7 +121,6 @@ export const KeyboardShortcutsSettings: React.FC = () => { })} { diff --git a/packages/ui/src/components/sections/openchamber/ShortcutRecordingDialog.test.ts b/packages/ui/src/components/sections/openchamber/ShortcutRecordingDialog.test.ts index 75fba52f..962d42f8 100644 --- a/packages/ui/src/components/sections/openchamber/ShortcutRecordingDialog.test.ts +++ b/packages/ui/src/components/sections/openchamber/ShortcutRecordingDialog.test.ts @@ -10,28 +10,28 @@ function keyEvent(key: string, modifiers: Partial { test('previews modifiers and clears the preview when they are released', () => { const pressed = updateShortcutRecordingState(emptyState, keyEvent('Control', { ctrlKey: true, shiftKey: true }), 'keydown'); - expect(pressed.state.livePreview).toBe('mod+shift'); - expect(updateShortcutRecordingState(pressed.state, keyEvent('Control'), 'keyup').state.livePreview).toBeNull(); + expect(pressed.livePreview).toBe('mod+shift'); + expect(updateShortcutRecordingState(pressed, keyEvent('Control'), 'keyup').livePreview).toBeNull(); }); test('records up to two chords', () => { const first = updateShortcutRecordingState(emptyState, keyEvent('k', { ctrlKey: true }), 'keydown'); - const second = updateShortcutRecordingState(first.state, keyEvent('p', { ctrlKey: true }), 'keydown'); - const third = updateShortcutRecordingState(second.state, keyEvent('x', { ctrlKey: true }), 'keydown'); - expect(first.state.chords).toEqual(['mod+k']); - expect(second.state.chords).toEqual(['mod+k', 'mod+p']); - expect(third.state.chords).toEqual(['mod+k', 'mod+p']); + const second = updateShortcutRecordingState(first, keyEvent('p', { ctrlKey: true }), 'keydown'); + const third = updateShortcutRecordingState(second, keyEvent('x', { ctrlKey: true }), 'keydown'); + expect(first.chords).toEqual(['mod+k']); + expect(second.chords).toEqual(['mod+k', 'mod+p']); + expect(third.chords).toEqual(['mod+k', 'mod+p']); }); test('ignores repeat and IME events', () => { - expect(updateShortcutRecordingState(emptyState, { ...keyEvent('k', { ctrlKey: true }), repeat: true }, 'keydown').state).toEqual(emptyState); - expect(updateShortcutRecordingState(emptyState, { ...keyEvent('k', { ctrlKey: true }), isComposing: true }, 'keydown').state).toEqual(emptyState); + expect(updateShortcutRecordingState(emptyState, { ...keyEvent('k', { ctrlKey: true }), repeat: true }, 'keydown')).toEqual(emptyState); + expect(updateShortcutRecordingState(emptyState, { ...keyEvent('k', { ctrlKey: true }), isComposing: true }, 'keydown')).toEqual(emptyState); }); - test('uses Enter and Escape for dialog actions and Backspace to remove the final chord', () => { + test('records Enter and Escape while Backspace removes the final chord', () => { const state = { chords: ['mod+k', 'mod+p'], livePreview: null }; - expect(updateShortcutRecordingState(state, keyEvent('Enter'), 'keydown').action).toBe('save'); - expect(updateShortcutRecordingState(state, keyEvent('Escape'), 'keydown').action).toBe('cancel'); - expect(updateShortcutRecordingState(state, keyEvent('Backspace'), 'keydown').state.chords).toEqual(['mod+k']); + expect(updateShortcutRecordingState(emptyState, keyEvent('Enter'), 'keydown').chords).toEqual(['enter']); + expect(updateShortcutRecordingState(emptyState, keyEvent('Escape'), 'keydown').chords).toEqual(['escape']); + expect(updateShortcutRecordingState(state, keyEvent('Backspace'), 'keydown').chords).toEqual(['mod+k']); }); }); diff --git a/packages/ui/src/components/sections/openchamber/ShortcutRecordingDialog.tsx b/packages/ui/src/components/sections/openchamber/ShortcutRecordingDialog.tsx index 57c83458..68353904 100644 --- a/packages/ui/src/components/sections/openchamber/ShortcutRecordingDialog.tsx +++ b/packages/ui/src/components/sections/openchamber/ShortcutRecordingDialog.tsx @@ -10,13 +10,12 @@ import { import { Button } from '@/components/ui/button'; import { formatShortcutForDisplay, - getEffectiveShortcutCombo, - getEffectiveShortcutPrefix, - getShortcutConflict, + getShortcutBindingConflicts, isRiskyBrowserShortcut, keyToShortcutToken, normalizeCombo, type ShortcutActionId, + type ShortcutBindingConflict, type ShortcutCombo, type CustomizableShortcutAction, } from '@/lib/shortcuts'; @@ -39,11 +38,8 @@ interface ShortcutRecordingState { livePreview: ShortcutCombo | null; } -type ShortcutRecordingAction = 'cancel' | 'none' | 'save'; - interface ShortcutRecordingDialogProps { action: CustomizableShortcutAction | null; - actions: ReadonlyArray; overrides: Record; onSave: ( actionId: ShortcutActionId, @@ -53,6 +49,12 @@ interface ShortcutRecordingDialogProps { onOpenChange: (open: boolean) => void; } +function isCustomizableConflict( + conflict: ShortcutBindingConflict, +): conflict is ShortcutBindingConflict & { action: CustomizableShortcutAction } { + return conflict.action.customizable; +} + function getModifierPreview(event: RecordingKeyboardEvent): ShortcutCombo | null { const parts: string[] = []; if (event.metaKey || event.ctrlKey) parts.push('mod'); @@ -91,35 +93,29 @@ export function updateShortcutRecordingState( state: ShortcutRecordingState, event: RecordingKeyboardEvent, phase: 'keydown' | 'keyup', -): { action: ShortcutRecordingAction; state: ShortcutRecordingState } { - if (event.repeat || event.isComposing) return { action: 'none', state }; +): ShortcutRecordingState { + if (event.repeat || event.isComposing) return state; if (phase === 'keyup') { - return { action: 'none', state: { ...state, livePreview: getModifierPreview(event) } }; + return { ...state, livePreview: getModifierPreview(event) }; } - if (event.key === 'Escape') return { action: 'cancel', state }; - if (event.key === 'Enter') return { action: 'save', state }; if (event.key === 'Backspace') { - return { action: 'none', state: { chords: state.chords.slice(0, -1), livePreview: null } }; + return { chords: state.chords.slice(0, -1), livePreview: null }; } const chord = keyboardEventToCombo(event); if (chord) { return { - action: 'none', - state: { - chords: state.chords.length < 2 ? [...state.chords, chord] : state.chords, - livePreview: null, - }, + chords: state.chords.length < 2 ? [...state.chords, chord] : state.chords, + livePreview: null, }; } - return { action: 'none', state: { ...state, livePreview: getModifierPreview(event) } }; + return { ...state, livePreview: getModifierPreview(event) }; } export const ShortcutRecordingDialog: React.FC = ({ action, - actions, overrides, onSave, onOpenChange, @@ -136,26 +132,19 @@ export const ShortcutRecordingDialog: React.FC = ( }, [action]); const combo = normalizeCombo(recording.chords.join(' ')); - const conflicts = React.useMemo(() => { - if (!action || !combo) return []; - const result: Array<{ action: CustomizableShortcutAction; kind: 'exact' | 'prefix' }> = []; - for (const candidate of actions) { - if (candidate.id === action.id) continue; - const candidateCombo = candidate.id === 'switch_context_surface' - ? getEffectiveShortcutPrefix(candidate.id, overrides) - : getEffectiveShortcutCombo(candidate.id, overrides); - const kind = getShortcutConflict(combo, candidateCombo); - if (kind) result.push({ action: candidate, kind }); - } - return result; - }, [action, actions, combo, overrides]); - const prefixConflict = conflicts.find((conflict) => conflict.kind === 'prefix'); - const exactConflict = conflicts.find((conflict) => conflict.kind === 'exact'); + const conflicts = React.useMemo( + () => action && combo ? getShortcutBindingConflicts(action.id, combo, overrides) : [], + [action, combo, overrides], + ); + const protectedConflict = conflicts.find((conflict) => !conflict.action.customizable); + const customizableConflicts = conflicts.filter(isCustomizableConflict); + const prefixConflict = customizableConflicts.find((conflict) => conflict.kind === 'prefix'); + const exactConflict = customizableConflicts.find((conflict) => conflict.kind === 'exact'); const close = () => onOpenChange(false); - const save = () => { - if (!action || !combo || prefixConflict || exactConflict) return; - onSave(action.id, combo); + const confirm = () => { + if (!action || !combo || protectedConflict || prefixConflict) return; + onSave(action.id, combo, exactConflict?.action.id); close(); }; const handleRecordingEvent = (event: React.KeyboardEvent, phase: 'keydown' | 'keyup') => { @@ -168,7 +157,7 @@ export const ShortcutRecordingDialog: React.FC = ( return; } } - const result = updateShortcutRecordingState(recording, { + const nextRecording = updateShortcutRecordingState(recording, { altKey: event.altKey, ctrlKey: event.ctrlKey, isComposing: event.nativeEvent.isComposing, @@ -177,16 +166,21 @@ export const ShortcutRecordingDialog: React.FC = ( repeat: event.repeat, shiftKey: event.shiftKey, }, phase); - setRecording(action?.id === 'switch_context_surface' && result.state.chords.length > 1 - ? { ...result.state, chords: result.state.chords.slice(0, 1) } - : result.state); - if (result.action === 'cancel') close(); - if (result.action === 'save') save(); + setRecording(action?.id === 'switch_context_surface' && nextRecording.chords.length > 1 + ? { ...nextRecording, chords: nextRecording.chords.slice(0, 1) } + : nextRecording); }; return ( - - + { + if (!open) { + eventDetails.cancel(); + } + }} + > + {action ? t('settings.openchamber.keyboardShortcuts.dialog.title', { action: actionLabel(action) }) : ''} @@ -221,12 +215,16 @@ export const ShortcutRecordingDialog: React.FC = ( - {prefixConflict ? ( + {protectedConflict ? ( +

+ {t('settings.openchamber.keyboardShortcuts.error.internalConflict')} +

+ ) : prefixConflict ? (

{t('settings.openchamber.keyboardShortcuts.error.prefixConflict', { action: actionLabel(prefixConflict.action) })}

) : null} - {exactConflict && !prefixConflict ? ( + {exactConflict && !protectedConflict && !prefixConflict ? (

{t('settings.openchamber.keyboardShortcuts.error.exactConflict', { action: actionLabel(exactConflict.action) })}

@@ -237,17 +235,19 @@ export const ShortcutRecordingDialog: React.FC = (

) : null} - {exactConflict && !prefixConflict ? ( - - - - ) : null} + + + + ); diff --git a/packages/ui/src/components/ui/select.tsx b/packages/ui/src/components/ui/select.tsx index 0d4c19bb..7cc3b7f6 100644 --- a/packages/ui/src/components/ui/select.tsx +++ b/packages/ui/src/components/ui/select.tsx @@ -286,13 +286,17 @@ function SelectLabel({ function SelectItem({ className, children, + showSelectedBackground = true, ...props -}: React.ComponentProps) { +}: React.ComponentProps & { + showSelectedBackground?: boolean; +}) { return ( `. Each binding has one chor Contextual internal commands may deliberately share a sequence leader. The single-chord handler gets the first chance to handle the event; returning `false` lets the dispatcher start the sequence. The active file editor therefore owns `mod+s` for saving, while a mounted but unfocused editor yields `mod+s p`, `mod+s g`, and `mod+s l` to the draft target pickers and session list. -Runtime-specific commands may also share an exact binding when their handlers are mutually exclusive. `open_diff_panel` handles `mod+2` on desktop, while `switch_tab_2` handles it on mobile; each returns `false` outside its runtime so the dispatcher can try the next registered action. +The internal `switch_tab_*` bindings remain available to mobile handlers. Desktop numeric context-surface switching is resolved by the configurable `switch_context_surface` prefix before normal dispatcher matching and falls through on mobile. -The settings recorder also stops at two chords. It keeps the recording local until the user explicitly saves, allows an exact conflict to replace the previous assignment, and blocks prefix conflicts because they make dispatch ambiguous. +The settings recorder also stops at two chords and checks the complete schema, not only customizable actions. It keeps the recording local until the user clicks Confirm, allows an exact customizable conflict to replace the previous assignment, and blocks prefix conflicts because they make dispatch ambiguous. Internal bindings are authoritative: persisted overrides cannot change or unassign them, and recorder conflicts with them cannot be replaced. # Dispatching @@ -43,7 +43,7 @@ Shared `DropdownMenu` and `Select` can opt into this boundary with `disableGloba Terminal capture, Escape abort priming, and the shifted reverse-agent chord are input-boundary exceptions. They preserve their target-specific semantics and invoke the registered application handler rather than duplicating command behavior. -Local key handling remains appropriate for text editing, IME composition, menu and list navigation, dialog confirmation, terminal input, and other interactions that do not represent configurable application commands. +Local key handling remains appropriate for text editing, IME composition, menu and list navigation, dialog confirmation, terminal input, and other interactions that do not represent configurable application commands. The settings recorder treats Enter and Escape as recordable keys; only its explicit Confirm and Cancel buttons apply or discard a recording. # Adding shortcuts diff --git a/packages/ui/src/lib/shortcuts/index.ts b/packages/ui/src/lib/shortcuts/index.ts index 7ef4ab99..53f0f1b0 100644 --- a/packages/ui/src/lib/shortcuts/index.ts +++ b/packages/ui/src/lib/shortcuts/index.ts @@ -17,6 +17,7 @@ export { shortcutRegistry } from './registry'; export type { ShortcutHandler } from './registry'; export { getCustomizableShortcutActions, + getShortcutBindingConflicts, getEffectiveShortcutCombo, getEffectiveShortcutPrefix, getShortcutAction, @@ -24,6 +25,7 @@ export { } from './schema'; export type { CustomizableShortcutAction, + ShortcutBindingConflict, ShortcutActionId, ShortcutCategory, } from './schema'; diff --git a/packages/ui/src/lib/shortcuts/schema.test.ts b/packages/ui/src/lib/shortcuts/schema.test.ts index 463b92a0..a40512cc 100644 --- a/packages/ui/src/lib/shortcuts/schema.test.ts +++ b/packages/ui/src/lib/shortcuts/schema.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from 'bun:test'; import { getCustomizableShortcutActions, getEffectiveShortcutCombo, + getShortcutBindingConflicts, getShortcutAction, parseShortcut, SHORTCUT_SCHEMA, @@ -59,4 +60,25 @@ describe('shortcut schema', () => { expect(getEffectiveShortcutCombo('new_chat', { new_chat: 'mod+k' })).toBe('mod+k'); expect(getEffectiveShortcutCombo('new_chat', { new_chat: 'mod+k x y' })).toBe('mod+n'); }); + + test('keeps internal bindings authoritative over persisted overrides', () => { + expect(getEffectiveShortcutCombo('save_file', { save_file: 'mod+k' })).toBe('mod+s'); + expect(getEffectiveShortcutCombo('save_file', { save_file: '__unassigned__' })).toBe('mod+s'); + }); + + test('detects conflicts against customizable and internal bindings', () => { + const customizableConflict = getShortcutBindingConflicts('new_chat', 'mod+p') + .find((conflict) => conflict.action.id === 'open_command_palette'); + const internalConflict = getShortcutBindingConflicts('new_chat', 'mod+f') + .find((conflict) => conflict.action.id === 'find_in_file'); + const internalPrefixConflict = getShortcutBindingConflicts('new_chat', 'mod+s x') + .find((conflict) => conflict.action.id === 'save_file'); + + expect(customizableConflict?.kind).toBe('exact'); + expect(customizableConflict?.action.customizable).toBe(true); + expect(internalConflict?.kind).toBe('exact'); + expect(internalConflict?.action.customizable).toBe(false); + expect(internalPrefixConflict?.kind).toBe('prefix'); + expect(internalPrefixConflict?.action.customizable).toBe(false); + }); }); diff --git a/packages/ui/src/lib/shortcuts/schema.ts b/packages/ui/src/lib/shortcuts/schema.ts index 23ad1077..514b3e13 100644 --- a/packages/ui/src/lib/shortcuts/schema.ts +++ b/packages/ui/src/lib/shortcuts/schema.ts @@ -1,9 +1,11 @@ import { + getShortcutConflict, isValidShortcutCombo, normalizeCombo, parseShortcut, UNASSIGNED_SHORTCUT, type ShortcutCombo, + type ShortcutConflict, } from './bindings'; import { SHORTCUT_SCHEMA } from './config'; @@ -13,6 +15,10 @@ export type ShortcutAction = (typeof SHORTCUT_SCHEMA)[number]; export type ShortcutActionId = ShortcutAction['id']; export type ShortcutCategory = ShortcutAction['category']; export type CustomizableShortcutAction = Extract; +export type ShortcutBindingConflict = { + action: ShortcutAction; + kind: ShortcutConflict; +}; export function getShortcutAction(id: string): ShortcutAction | undefined { return SHORTCUT_SCHEMA.find((action) => action.id === id); @@ -30,6 +36,7 @@ export function getEffectiveShortcutCombo( ): ShortcutCombo { const action = getShortcutAction(actionId); if (!action) return ''; + if (!action.customizable) return action.defaultBinding; const override = overrides?.[actionId]; if (typeof override === 'string') { @@ -47,6 +54,7 @@ export function getEffectiveShortcutPrefix( ): ShortcutCombo { const action = getShortcutAction(actionId); if (!action) return ''; + if (!action.customizable) return action.defaultBinding; const override = overrides?.[actionId]; if (typeof override === 'string' && override.trim() !== '') { @@ -58,3 +66,20 @@ export function getEffectiveShortcutPrefix( return action.defaultBinding; } + +export function getShortcutBindingConflicts( + actionId: ShortcutActionId, + combo: ShortcutCombo, + overrides?: Record, +): ShortcutBindingConflict[] { + const conflicts: ShortcutBindingConflict[] = []; + for (const candidate of SHORTCUT_SCHEMA) { + if (candidate.id === actionId) continue; + const candidateCombo = candidate.id === 'switch_context_surface' + ? getEffectiveShortcutPrefix(candidate.id, overrides) + : getEffectiveShortcutCombo(candidate.id, overrides); + const kind = getShortcutConflict(combo, candidateCombo); + if (kind) conflicts.push({ action: candidate, kind }); + } + return conflicts; +}