diff --git a/packages/app/src/command-center/workspace-registration.tsx b/packages/app/src/command-center/workspace-registration.tsx index b1ebfc4fe..4963fd835 100644 --- a/packages/app/src/command-center/workspace-registration.tsx +++ b/packages/app/src/command-center/workspace-registration.tsx @@ -6,7 +6,10 @@ import { supportsDesktopPaneSplits, useIsCompactFormFactor } from "@/constants/l import { GIT_ACTION_ICONS } from "@/git/action-icons"; import { useGitActionRunner, useGitActions } from "@/git/use-actions"; import { useKeyboardShortcutOverrides } from "@/hooks/use-keyboard-shortcut-overrides"; -import { resolveShortcutKeysForAction } from "@/keyboard/keyboard-shortcuts"; +import { + resolveShortcutKeysForAction, + type ShortcutOverrides, +} from "@/keyboard/keyboard-shortcuts"; import { keyboardActionDispatcher } from "@/keyboard/keyboard-action-dispatcher"; import { useActiveWorkspaceSelection } from "@/stores/navigation-active-workspace-store"; import { useWorkspaceDirectory } from "@/stores/session-store-hooks"; @@ -36,9 +39,7 @@ function staticIcon(element: ReactElement | undefined): CommandCenterIcon | unde return StaticIcon; } -function resolveWorkspaceShortcuts( - overrides: Readonly>, -): WorkspaceCommandCenterShortcuts { +function resolveWorkspaceShortcuts(overrides: ShortcutOverrides): WorkspaceCommandCenterShortcuts { const platform = { isMac: getShortcutOs() === "mac", isDesktop: getIsElectron() }; return { newAgent: resolveShortcutKeysForAction("workspace-tab-new", overrides, platform) ?? undefined, diff --git a/packages/app/src/components/keyboard-shortcuts-dialog.tsx b/packages/app/src/components/keyboard-shortcuts-dialog.tsx index 770f1193d..b671416f6 100644 --- a/packages/app/src/components/keyboard-shortcuts-dialog.tsx +++ b/packages/app/src/components/keyboard-shortcuts-dialog.tsx @@ -8,7 +8,11 @@ import { Shortcut } from "@/components/ui/shortcut"; import { useKeyboardShortcutsStore } from "@/stores/keyboard-shortcuts-store"; import { formatShortcut } from "@/utils/format-shortcut"; import { getShortcutOs } from "@/utils/shortcut-platform"; -import { buildKeyboardShortcutHelpSections } from "@/keyboard/keyboard-shortcuts"; +import { + buildEffectiveBindings, + buildKeyboardShortcutHelpSections, +} from "@/keyboard/keyboard-shortcuts"; +import { useKeyboardShortcutOverrides } from "@/hooks/use-keyboard-shortcut-overrides"; const SNAP_POINTS: string[] = ["70%", "92%"]; @@ -42,9 +46,13 @@ export function KeyboardShortcutsDialog() { const shortcutOs = getShortcutOs(); const isMac = shortcutOs === "mac"; const isDesktopApp = getIsElectronRuntime(); + const { overrides } = useKeyboardShortcutOverrides(); + // Effective bindings, so a shortcut the user unassigned lists no keys here + // instead of advertising a default that no longer fires. + const bindings = useMemo(() => buildEffectiveBindings(overrides), [overrides]); const sections = useMemo( - () => buildKeyboardShortcutHelpSections({ isMac, isDesktop: isDesktopApp }), - [isDesktopApp, isMac], + () => buildKeyboardShortcutHelpSections({ isMac, isDesktop: isDesktopApp }, bindings), + [bindings, isDesktopApp, isMac], ); const visibleSections = useMemo(() => { const normalizedQuery = query.trim().toLocaleLowerCase(); diff --git a/packages/app/src/components/ui/normalize-display-chord.test.ts b/packages/app/src/components/ui/normalize-display-chord.test.ts new file mode 100644 index 000000000..99a3fe09a --- /dev/null +++ b/packages/app/src/components/ui/normalize-display-chord.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from "vitest"; +import { normalizeDisplayChord } from "./normalize-display-chord"; + +describe("normalizeDisplayChord", () => { + it("returns null when neither keys nor chord is given", () => { + expect(normalizeDisplayChord()).toBeNull(); + }); + + it("returns null for an empty keys array", () => { + // The trap: [] wraps to [[]], whose first element is truthy. + expect(normalizeDisplayChord(undefined, [])).toBeNull(); + }); + + it("returns null for an empty chord", () => { + expect(normalizeDisplayChord([])).toBeNull(); + }); + + it("returns null for a chord holding an empty combo", () => { + expect(normalizeDisplayChord([[]])).toBeNull(); + expect(normalizeDisplayChord([["mod", "K"], []])).toBeNull(); + }); + + it("wraps keys into a single-combo chord", () => { + expect(normalizeDisplayChord(undefined, ["mod", "K"])).toEqual([["mod", "K"]]); + }); + + it("passes a multi-step chord through unchanged", () => { + expect(normalizeDisplayChord([["mod", "K"], ["S"]])).toEqual([["mod", "K"], ["S"]]); + }); + + it("prefers chord over keys when both are given", () => { + expect(normalizeDisplayChord([["alt", "W"]], ["mod", "K"])).toEqual([["alt", "W"]]); + }); +}); diff --git a/packages/app/src/components/ui/normalize-display-chord.ts b/packages/app/src/components/ui/normalize-display-chord.ts new file mode 100644 index 000000000..6138999c2 --- /dev/null +++ b/packages/app/src/components/ui/normalize-display-chord.ts @@ -0,0 +1,28 @@ +import type { ShortcutKey } from "@/utils/format-shortcut"; + +/** + * The combos a `Shortcut` should render, or `null` when it should render + * nothing at all. + * + * `keys={[]}` is the trap this exists to close: it wraps to `[[]]`, whose first + * element is an empty array — truthy — so a first-element check waves it + * through and an empty badge pill renders. A shortcut with no keys, whether the + * user unassigned it or it ships without a default combo, has to produce no + * element at all. + */ +export function normalizeDisplayChord( + chord?: ShortcutKey[][], + keys?: ShortcutKey[], +): ShortcutKey[][] | null { + const combos = chord ?? (keys ? [keys] : []); + if (combos.length === 0) { + return null; + } + // A chord with an empty step cannot be drawn honestly: rendering it would + // show a blank pill, and dropping the step would advertise a shorter + // sequence than the one that actually fires. + if (combos.some((combo) => combo.length === 0)) { + return null; + } + return combos; +} diff --git a/packages/app/src/components/ui/shortcut.tsx b/packages/app/src/components/ui/shortcut.tsx index 5d4eb90e4..95be52a97 100644 --- a/packages/app/src/components/ui/shortcut.tsx +++ b/packages/app/src/components/ui/shortcut.tsx @@ -2,6 +2,7 @@ import React, { useMemo, type ReactElement } from "react"; import { Text, View, type StyleProp, type TextStyle, type ViewStyle } from "react-native"; import { StyleSheet } from "react-native-unistyles"; import { useKeyboardShortcutsAvailable } from "@/keyboard/availability"; +import { normalizeDisplayChord } from "@/components/ui/normalize-display-chord"; import { formatShortcut, type ShortcutKey } from "@/utils/format-shortcut"; import { getShortcutOs } from "@/utils/shortcut-platform"; @@ -17,9 +18,8 @@ export function Shortcut({ textStyle?: StyleProp; }): ReactElement | null { const shortcutsAvailable = useKeyboardShortcutsAvailable(); - const displayChord = chord ?? (keys ? [keys] : []); + const displayChord = normalizeDisplayChord(chord, keys); const shortcutOs = getShortcutOs(); - const singleCombo = displayChord[0]; const badgeStyle = useMemo(() => [styles.badge, style], [style]); const textCombinedStyle = useMemo(() => [styles.text, textStyle], [textStyle]); @@ -29,8 +29,11 @@ export function Shortcut({ return null; } - if (!singleCombo) { - return ; + const singleCombo = displayChord?.[0]; + // Render nothing, literally — an empty would still consume the + // parent's `gap` and leave a phantom slot where the badge used to be. + if (!displayChord || !singleCombo) { + return null; } if (displayChord.length === 1) { diff --git a/packages/app/src/hooks/use-keyboard-shortcut-overrides.ts b/packages/app/src/hooks/use-keyboard-shortcut-overrides.ts index a56627101..874b28122 100644 --- a/packages/app/src/hooks/use-keyboard-shortcut-overrides.ts +++ b/packages/app/src/hooks/use-keyboard-shortcut-overrides.ts @@ -1,16 +1,21 @@ -import { useCallback } from "react"; import AsyncStorage from "@react-native-async-storage/async-storage"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { type QueryClient, useQuery, useQueryClient } from "@tanstack/react-query"; +import type { ShortcutOverrides } from "@/keyboard/keyboard-shortcuts"; +import { + createShortcutOverrideStore, + type ShortcutOverrideStore, +} from "@/keyboard/shortcut-override-store"; const STORAGE_KEY = "@paseo:keyboard-shortcut-overrides"; const QUERY_KEY = ["keyboard-shortcut-overrides"]; -const EMPTY_OVERRIDES: Record = {}; +const EMPTY_OVERRIDES: ShortcutOverrides = {}; export interface UseKeyboardShortcutOverridesReturn { - overrides: Record; + overrides: ShortcutOverrides; isLoading: boolean; setOverride: (bindingId: string, comboString: string) => Promise; + clearOverride: (bindingId: string) => Promise; removeOverride: (bindingId: string) => Promise; resetAll: () => Promise; hasOverrides: boolean; @@ -25,48 +30,55 @@ export function useKeyboardShortcutOverrides(): UseKeyboardShortcutOverridesRetu gcTime: Infinity, }); - const setOverride = useCallback( - async (bindingId: string, comboString: string) => { - const prev = queryClient.getQueryData>(QUERY_KEY) ?? EMPTY_OVERRIDES; - const next = { ...prev, [bindingId]: comboString }; - queryClient.setQueryData>(QUERY_KEY, next); - await AsyncStorage.setItem(STORAGE_KEY, JSON.stringify(next)); - }, - [queryClient], - ); - - const removeOverride = useCallback( - async (bindingId: string) => { - const prev = queryClient.getQueryData>(QUERY_KEY) ?? EMPTY_OVERRIDES; - const { [bindingId]: _, ...next } = prev; - queryClient.setQueryData>(QUERY_KEY, next); - await AsyncStorage.setItem(STORAGE_KEY, JSON.stringify(next)); - }, - [queryClient], - ); - - const resetAll = useCallback(async () => { - queryClient.setQueryData>(QUERY_KEY, EMPTY_OVERRIDES); - await AsyncStorage.removeItem(STORAGE_KEY); - }, [queryClient]); - + const store = getStore(queryClient); const overrides = data ?? EMPTY_OVERRIDES; return { overrides, isLoading: isPending, - setOverride, - removeOverride, - resetAll, + setOverride: store.set, + clearOverride: store.clear, + removeOverride: store.remove, + resetAll: store.resetAll, hasOverrides: Object.keys(overrides).length > 0, }; } -async function loadOverridesFromStorage(): Promise> { +/** + * One store per query client. Every component that calls the hook has to share + * a single write queue -- a store per hook instance would give each its own + * queue and reintroduce the interleaving the store exists to prevent. + */ +const stores = new WeakMap(); + +function getStore(queryClient: QueryClient): ShortcutOverrideStore { + const existing = stores.get(queryClient); + if (existing) return existing; + + const created = createShortcutOverrideStore({ + cache: { + read: () => queryClient.getQueryData(QUERY_KEY) ?? EMPTY_OVERRIDES, + write: (next) => { + queryClient.setQueryData(QUERY_KEY, next); + }, + }, + storage: { + write: (serialized) => AsyncStorage.setItem(STORAGE_KEY, serialized), + remove: () => AsyncStorage.removeItem(STORAGE_KEY), + }, + onError: (err) => { + console.error("[KeyboardShortcutOverrides] Failed to save overrides:", err); + }, + }); + stores.set(queryClient, created); + return created; +} + +async function loadOverridesFromStorage(): Promise { try { const stored = await AsyncStorage.getItem(STORAGE_KEY); if (stored) { - return JSON.parse(stored) as Record; + return JSON.parse(stored) as ShortcutOverrides; } } catch (err) { console.error("[KeyboardShortcutOverrides] Failed to load overrides:", err); diff --git a/packages/app/src/hooks/use-keyboard-shortcuts.ts b/packages/app/src/hooks/use-keyboard-shortcuts.ts index 243c366af..6c5785b48 100644 --- a/packages/app/src/hooks/use-keyboard-shortcuts.ts +++ b/packages/app/src/hooks/use-keyboard-shortcuts.ts @@ -71,6 +71,7 @@ export function useKeyboardShortcuts({ const openProjectPickerAction = useOpenAddProject(); const activeWorkspaceSelection = useActiveWorkspaceSelection(); const keyboardWorkspaceSelectionRef = useRef(null); + const badgeModifierKeyRef = useRef(undefined); const publishBrowserShortcutPolicy = useCallback( (chordState?: ChordState) => { @@ -110,7 +111,13 @@ export function useKeyboardShortcuts({ // runtime should reveal the sidebar number badges (Alt on web, Cmd on // desktop Mac, Ctrl on desktop non-Mac). The store ORs altDown/cmdOrCtrlDown // to drive badge visibility, so we set the flag matching this runtime. - const badgeModifierKey = getWorkspaceIndexJumpModifierKey({ isMac, isDesktop: isDesktopApp }); + // Derived from the effective bindings: `null` when the user unassigned or + // rebound the jump shortcut, and no `event.key` ever equals null, so the + // badges simply never appear. + const badgeModifierKey = getWorkspaceIndexJumpModifierKey( + { isMac, isDesktop: isDesktopApp }, + bindings, + ); const setBadgeModifierDown = (down: boolean) => { const state = useKeyboardShortcutsStore.getState(); if (isDesktopApp) { @@ -120,6 +127,16 @@ export function useKeyboardShortcuts({ } }; + // The keyup listener matches the released key against the modifier derived + // when the effect last ran, so a modifier held while the jump binding + // changes can never be released -- the badges would stay up until a blur. + // Clear on change only: this effect also re-runs on every navigation, and + // clearing unconditionally would drop the badges mid Cmd+1, Cmd+2. + if (badgeModifierKeyRef.current !== badgeModifierKey) { + badgeModifierKeyRef.current = badgeModifierKey; + resetModifiers(); + } + const shouldHandle = () => { if (typeof document === "undefined") return false; if (document.visibilityState !== "visible") return false; diff --git a/packages/app/src/i18n/resources/ar.ts b/packages/app/src/i18n/resources/ar.ts index c29f45b3f..a231a7042 100644 --- a/packages/app/src/i18n/resources/ar.ts +++ b/packages/app/src/i18n/resources/ar.ts @@ -1913,11 +1913,15 @@ export const ar: TranslationResources = { searchPlaceholder: "البحث في الاختصارات", unavailableOnMobile: "اختصارات لوحة المفاتيح متاحة فقط على سطح المكتب", capturePrompt: "اضغط على الاختصار...", + unassigned: "غير معين", actions: { + menu: "إجراءات {{name}}", done: "منتهي", cancel: "يلغي", + bind: "ربط", rebind: "إعادة ربط", - reset: "إعادة ضبط", + clear: "مسح", + reset: "إعادة الضبط إلى الافتراضي", resetAll: "إعادة ضبط الكل", }, sections: { diff --git a/packages/app/src/i18n/resources/en.ts b/packages/app/src/i18n/resources/en.ts index 897107ebc..765dd2b6a 100644 --- a/packages/app/src/i18n/resources/en.ts +++ b/packages/app/src/i18n/resources/en.ts @@ -1923,11 +1923,15 @@ export const en = { searchPlaceholder: "Search shortcuts", unavailableOnMobile: "Keyboard shortcuts are only available on desktop", capturePrompt: "Press shortcut...", + unassigned: "Not set", actions: { + menu: "Actions for {{name}}", done: "Done", cancel: "Cancel", + bind: "Bind", rebind: "Rebind", - reset: "Reset", + clear: "Clear", + reset: "Reset to default", resetAll: "Reset all", }, sections: { diff --git a/packages/app/src/i18n/resources/es.ts b/packages/app/src/i18n/resources/es.ts index afbab3b4d..dd56db59b 100644 --- a/packages/app/src/i18n/resources/es.ts +++ b/packages/app/src/i18n/resources/es.ts @@ -1961,11 +1961,15 @@ export const es: TranslationResources = { searchPlaceholder: "Buscar atajos", unavailableOnMobile: "Los atajos de teclado solo están disponibles en el escritorio", capturePrompt: "Presione el acceso directo...", + unassigned: "Sin asignar", actions: { + menu: "Acciones para {{name}}", done: "Hecho", cancel: "Cancelar", + bind: "Asignar", rebind: "Reencuadernar", - reset: "Reiniciar", + clear: "Borrar", + reset: "Restablecer al valor predeterminado", resetAll: "Restablecer todo", }, sections: { diff --git a/packages/app/src/i18n/resources/fr.ts b/packages/app/src/i18n/resources/fr.ts index cc6e07d26..00b061a88 100644 --- a/packages/app/src/i18n/resources/fr.ts +++ b/packages/app/src/i18n/resources/fr.ts @@ -1964,11 +1964,15 @@ export const fr: TranslationResources = { searchPlaceholder: "Rechercher des raccourcis", unavailableOnMobile: "Les raccourcis clavier ne sont disponibles que sur le bureau", capturePrompt: "Appuyez sur le raccourci...", + unassigned: "Non défini", actions: { + menu: "Actions pour {{name}}", done: "Fait", cancel: "Annuler", + bind: "Attribuer", rebind: "Relier", - reset: "Réinitialiser", + clear: "Effacer", + reset: "Rétablir la valeur par défaut", resetAll: "Tout réinitialiser", }, sections: { diff --git a/packages/app/src/i18n/resources/ja.ts b/packages/app/src/i18n/resources/ja.ts index 0f90f923b..2ec9f2334 100644 --- a/packages/app/src/i18n/resources/ja.ts +++ b/packages/app/src/i18n/resources/ja.ts @@ -1929,11 +1929,15 @@ export const ja: TranslationResources = { searchPlaceholder: "ショートカットを検索", unavailableOnMobile: "キーボードショートカットはデスクトップでのみ利用できます", capturePrompt: "ショートカットを押してください...", + unassigned: "未設定", actions: { + menu: "{{name}} のアクション", done: "完了", cancel: "キャンセル", + bind: "割り当て", rebind: "再割り当て", - reset: "リセット", + clear: "クリア", + reset: "デフォルトに戻す", resetAll: "すべてリセット", }, sections: { diff --git a/packages/app/src/i18n/resources/ko.ts b/packages/app/src/i18n/resources/ko.ts index f1817f187..4aa353f60 100644 --- a/packages/app/src/i18n/resources/ko.ts +++ b/packages/app/src/i18n/resources/ko.ts @@ -1924,11 +1924,15 @@ export const ko: TranslationResources = { searchPlaceholder: "검색 단축키", unavailableOnMobile: "키보드 단축키는 데스크톱에서만 사용할 수 있습니다", capturePrompt: "단축키를 누르세요...", + unassigned: "설정되지 않음", actions: { + menu: "{{name}} 작업", done: "완료", cancel: "취소", + bind: "지정", rebind: "다시 지정", - reset: "재설정", + clear: "지우기", + reset: "기본값으로 재설정", resetAll: "모두 재설정", }, sections: { diff --git a/packages/app/src/i18n/resources/pt-BR.ts b/packages/app/src/i18n/resources/pt-BR.ts index f0b738156..b5dfbb388 100644 --- a/packages/app/src/i18n/resources/pt-BR.ts +++ b/packages/app/src/i18n/resources/pt-BR.ts @@ -1944,11 +1944,15 @@ export const ptBR: TranslationResources = { searchPlaceholder: "Pesquisar atalhos", unavailableOnMobile: "Atalhos de teclado estão disponíveis apenas no desktop", capturePrompt: "Pressione o atalho...", + unassigned: "Não definido", actions: { + menu: "Ações para {{name}}", done: "Concluído", cancel: "Cancelar", + bind: "Atribuir", rebind: "Reatribuir", - reset: "Redefinir", + clear: "Limpar", + reset: "Redefinir para o padrão", resetAll: "Redefinir tudo", }, sections: { diff --git a/packages/app/src/i18n/resources/ru.ts b/packages/app/src/i18n/resources/ru.ts index 6e720f610..2e387d70d 100644 --- a/packages/app/src/i18n/resources/ru.ts +++ b/packages/app/src/i18n/resources/ru.ts @@ -1951,11 +1951,15 @@ export const ru: TranslationResources = { searchPlaceholder: "Поиск сочетаний клавиш", unavailableOnMobile: "Сочетания клавиш доступны только на рабочем столе.", capturePrompt: "Нажмите ярлык...", + unassigned: "Не задано", actions: { + menu: "Действия для {{name}}", done: "Сделанный", cancel: "Отмена", + bind: "Привязать", rebind: "Перепривязка", - reset: "Перезагрузить", + clear: "Очистить", + reset: "Сбросить к значению по умолчанию", resetAll: "Сбросить все", }, sections: { diff --git a/packages/app/src/i18n/resources/zh-CN.ts b/packages/app/src/i18n/resources/zh-CN.ts index 4e8566e28..ab79c6826 100644 --- a/packages/app/src/i18n/resources/zh-CN.ts +++ b/packages/app/src/i18n/resources/zh-CN.ts @@ -1891,11 +1891,15 @@ export const zhCN: TranslationResources = { searchPlaceholder: "搜索快捷键", unavailableOnMobile: "键盘快捷键仅在桌面端可用", capturePrompt: "按下快捷键...", + unassigned: "未设置", actions: { + menu: "{{name}} 的操作", done: "完成", cancel: "取消", + bind: "绑定", rebind: "重新绑定", - reset: "重置", + clear: "清除", + reset: "重置为默认", resetAll: "全部重置", }, sections: { diff --git a/packages/app/src/keyboard/keyboard-shortcuts.test.ts b/packages/app/src/keyboard/keyboard-shortcuts.test.ts index e0bd68305..a3accef6c 100644 --- a/packages/app/src/keyboard/keyboard-shortcuts.test.ts +++ b/packages/app/src/keyboard/keyboard-shortcuts.test.ts @@ -3,12 +3,17 @@ import { buildKeyboardShortcutHelpSections, buildEffectiveBindings, getBindingIdForAction, + getDefaultKeysForAction, getWorkspaceIndexJumpModifierKey, + parseBindingChord, resolveKeyboardShortcut, + resolveShortcutKeysForAction, + UNASSIGNED_COMBO, type ChordState, type KeyboardShortcutContext, type KeyboardShortcutInput, type ParsedShortcutBinding, + type ShortcutOverrides, } from "./keyboard-shortcuts"; function keyboardInput(overrides: Partial): KeyboardShortcutInput { @@ -702,6 +707,8 @@ describe("keyboard-shortcut help sections", () => { }); describe("getWorkspaceIndexJumpModifierKey", () => { + const MAC_INDEX_BINDING = "workspace-navigate-index-cmd-digit-mac"; + it("uses Alt on web, regardless of OS", () => { expect(getWorkspaceIndexJumpModifierKey({ isMac: true, isDesktop: false })).toBe("Alt"); expect(getWorkspaceIndexJumpModifierKey({ isMac: false, isDesktop: false })).toBe("Alt"); @@ -714,4 +721,263 @@ describe("getWorkspaceIndexJumpModifierKey", () => { it("uses Ctrl on desktop non-Mac, not Meta or Alt", () => { expect(getWorkspaceIndexJumpModifierKey({ isMac: false, isDesktop: true })).toBe("Control"); }); + + it("derives the modifier from the effective binding, not the platform", () => { + const bindings = buildEffectiveBindings({ [MAC_INDEX_BINDING]: "Alt+Digit" }); + expect(getWorkspaceIndexJumpModifierKey({ isMac: true, isDesktop: true }, bindings)).toBe( + "Alt", + ); + }); + + it("suppresses the badges when the jump shortcut is unassigned", () => { + const bindings = buildEffectiveBindings({ [MAC_INDEX_BINDING]: UNASSIGNED_COMBO }); + expect(getWorkspaceIndexJumpModifierKey({ isMac: true, isDesktop: true }, bindings)).toBeNull(); + }); + + it("suppresses the badges when the jump shortcut is rebound to one concrete digit", () => { + // Capture can only ever produce a concrete digit, never the 1-9 wildcard, + // so the badges would advertise eight workspaces that no longer respond. + const bindings = buildEffectiveBindings({ [MAC_INDEX_BINDING]: "Cmd+3" }); + expect(getWorkspaceIndexJumpModifierKey({ isMac: true, isDesktop: true }, bindings)).toBeNull(); + }); + + it("suppresses the badges for a multi-step chord", () => { + const bindings = buildEffectiveBindings({ [MAC_INDEX_BINDING]: "Cmd+K Cmd+Digit" }); + expect(getWorkspaceIndexJumpModifierKey({ isMac: true, isDesktop: true }, bindings)).toBeNull(); + }); + + it("suppresses the badges when the combo needs a second modifier", () => { + const bindings = buildEffectiveBindings({ [MAC_INDEX_BINDING]: "Cmd+Shift+Digit" }); + expect(getWorkspaceIndexJumpModifierKey({ isMac: true, isDesktop: true }, bindings)).toBeNull(); + }); +}); + +function parseUnknownBindingChord() { + return parseBindingChord("Ctrl+Nonsense"); +} + +/** + * Effective bindings with one help row rewritten to ship without a default + * combo, standing in for a binding authored as `combo: ""`. `help.keys` is left + * as authored so the empty parsed chord is the only signal. + */ +function withoutDefaultCombo(helpId: string): ParsedShortcutBinding[] { + const bindings: ParsedShortcutBinding[] = []; + for (const binding of buildEffectiveBindings({})) { + if (binding.help?.id === helpId) { + bindings.push(Object.assign({}, binding, { combo: "", parsedChord: [] })); + } else { + bindings.push(binding); + } + } + return bindings; +} + +describe("unassigned shortcuts", () => { + const TAB_NEW_BINDING = "workspace-tab-new-ctrl-t-non-mac"; + const desktopNonMac = { isMac: false, isDesktop: true }; + + function findRow(sections: ReturnType, id: string) { + for (const section of sections) { + const row = section.rows.find((candidate) => candidate.id === id); + if (row) { + return row; + } + } + return null; + } + + describe("parseBindingChord", () => { + it("yields an empty chord for a binding that ships without a default combo", () => { + expect(parseBindingChord("")).toEqual([]); + }); + + it("parses a normal combo into one step", () => { + expect(parseBindingChord("Ctrl+T")).toHaveLength(1); + }); + + it("parses a chord into one step per combo", () => { + expect(parseBindingChord("Ctrl+W S")).toHaveLength(2); + }); + + it("still throws on an unparseable combo", () => { + expect(parseUnknownBindingChord).toThrow(); + }); + }); + + describe("matching", () => { + it("stops matching a shortcut the user unassigned", () => { + const bindings = buildEffectiveBindings({ [TAB_NEW_BINDING]: UNASSIGNED_COMBO }); + + const result = resolveShortcut({ + event: { key: "t", code: "KeyT", ctrlKey: true }, + context: desktopNonMac, + bindings, + }); + + expect(result.match).toBeNull(); + }); + + it("leaves other shortcuts firing when one is unassigned", () => { + const bindings = buildEffectiveBindings({ [TAB_NEW_BINDING]: UNASSIGNED_COMBO }); + + const result = resolveShortcut({ + event: { key: "t", code: "KeyT", ctrlKey: true, shiftKey: true }, + context: desktopNonMac, + bindings, + }); + + expect(result.match?.action).toBe("workspace.terminal.new"); + }); + + it("restores the default when the override is removed", () => { + const bindings = buildEffectiveBindings({}); + + const result = resolveShortcut({ + event: { key: "t", code: "KeyT", ctrlKey: true }, + context: desktopNonMac, + bindings, + }); + + expect(result.match?.action).toBe("workspace.tab.new"); + }); + + it("treats a stored empty combo as unassigned too", () => { + const bindings = buildEffectiveBindings({ [TAB_NEW_BINDING]: "" }); + + const result = resolveShortcut({ + event: { key: "t", code: "KeyT", ctrlKey: true }, + context: desktopNonMac, + bindings, + }); + + expect(result.match).toBeNull(); + }); + + it("keeps rebinding to a real combo working", () => { + const bindings = buildEffectiveBindings({ [TAB_NEW_BINDING]: "Ctrl+Y" }); + + expect( + resolveShortcut({ + event: { key: "y", code: "KeyY", ctrlKey: true }, + context: desktopNonMac, + bindings, + }).match?.action, + ).toBe("workspace.tab.new"); + expect( + resolveShortcut({ + event: { key: "t", code: "KeyT", ctrlKey: true }, + context: desktopNonMac, + bindings, + }).match, + ).toBeNull(); + }); + + it("falls back to the default for a non-string stored value", () => { + // Storage is unvalidated JSON, so a corrupt value must not throw. + const overrides = { [TAB_NEW_BINDING]: 42 } as unknown as ShortcutOverrides; + const bindings = buildEffectiveBindings(overrides); + + const result = resolveShortcut({ + event: { key: "t", code: "KeyT", ctrlKey: true }, + context: desktopNonMac, + bindings, + }); + + expect(result.match?.action).toBe("workspace.tab.new"); + }); + }); + + describe("resolveShortcutKeysForAction", () => { + it("returns the default keys when there is no override", () => { + expect(resolveShortcutKeysForAction("workspace-tab-new", {}, desktopNonMac)).toEqual([ + ["mod", "T"], + ]); + }); + + it("returns null when the user unassigned the shortcut", () => { + expect( + resolveShortcutKeysForAction( + "workspace-tab-new", + { [TAB_NEW_BINDING]: UNASSIGNED_COMBO }, + desktopNonMac, + ), + ).toBeNull(); + }); + + it("returns the override keys when the shortcut is rebound", () => { + expect( + resolveShortcutKeysForAction( + "workspace-tab-new", + { [TAB_NEW_BINDING]: "Ctrl+Y" }, + desktopNonMac, + ), + ).toEqual([["ctrl", "Y"]]); + }); + + it("falls back to the default keys for an unparseable override", () => { + // Matching falls back to the default here, so the display has to as well + // or it would advertise keys that do nothing. + expect( + resolveShortcutKeysForAction( + "workspace-tab-new", + { [TAB_NEW_BINDING]: "Ctrl+Nonsense" }, + desktopNonMac, + ), + ).toEqual([["mod", "T"]]); + }); + + it("falls back to the default keys for a non-string stored value", () => { + const overrides = { [TAB_NEW_BINDING]: 42 } as unknown as ShortcutOverrides; + expect(resolveShortcutKeysForAction("workspace-tab-new", overrides, desktopNonMac)).toEqual([ + ["mod", "T"], + ]); + }); + + it("returns null for an action with no binding on this platform", () => { + expect(resolveShortcutKeysForAction("message-input-send", {}, desktopNonMac)).toBeNull(); + }); + }); + + describe("help rows", () => { + it("lists no keys for an unassigned shortcut", () => { + const bindings = buildEffectiveBindings({ [TAB_NEW_BINDING]: UNASSIGNED_COMBO }); + const sections = buildKeyboardShortcutHelpSections(desktopNonMac, bindings); + + expect(findRow(sections, "workspace-tab-new")?.keys).toEqual([]); + }); + + it("keeps the row so the shortcut stays rebindable", () => { + const bindings = buildEffectiveBindings({ [TAB_NEW_BINDING]: UNASSIGNED_COMBO }); + const sections = buildKeyboardShortcutHelpSections(desktopNonMac, bindings); + + expect(findRow(sections, "workspace-tab-new")).not.toBeNull(); + expect(getBindingIdForAction("workspace-tab-new", desktopNonMac)).toBe(TAB_NEW_BINDING); + }); + + it("still lists the default keys when nothing is overridden", () => { + const sections = buildKeyboardShortcutHelpSections(desktopNonMac); + + expect(findRow(sections, "workspace-tab-new")?.keys).toEqual(["mod", "T"]); + expect(getDefaultKeysForAction("workspace-tab-new", desktopNonMac)).toEqual(["mod", "T"]); + }); + + // A binding authored with `combo: ""` has no default. The settings row uses + // this to hide Reset, which would otherwise be a dead button landing on the + // same "Not set" state Clear already produced. `help.keys` stays populated + // on purpose: it is hand-authored, so the empty parsed chord alone has to be + // what marks the binding default-less. + it("reports no default keys for a binding that ships without a combo", () => { + const bindings = withoutDefaultCombo("workspace-tab-new"); + + expect(getDefaultKeysForAction("workspace-tab-new", desktopNonMac, bindings)).toBeNull(); + }); + + it("lists no help keys for a binding that ships without a combo", () => { + const bindings = withoutDefaultCombo("workspace-tab-new"); + const sections = buildKeyboardShortcutHelpSections(desktopNonMac, bindings); + + expect(findRow(sections, "workspace-tab-new")?.keys).toEqual([]); + }); + }); }); diff --git a/packages/app/src/keyboard/keyboard-shortcuts.ts b/packages/app/src/keyboard/keyboard-shortcuts.ts index 3cec49723..e7e4032b0 100644 --- a/packages/app/src/keyboard/keyboard-shortcuts.ts +++ b/packages/app/src/keyboard/keyboard-shortcuts.ts @@ -5,8 +5,11 @@ import type { KeyboardShortcutPayload, MessageInputKeyboardActionKind, } from "@/keyboard/actions"; -import { type KeyCombo, parseChordString } from "@/keyboard/shortcut-string"; -import { chordStringToShortcutKeys } from "@/keyboard/shortcut-string"; +import { + chordStringToShortcutKeys, + type KeyCombo, + parseChordString, +} from "@/keyboard/shortcut-string"; export type { KeyCombo } from "@/keyboard/shortcut-string"; @@ -1064,8 +1067,30 @@ const SHORTCUT_BINDINGS: readonly ShortcutBinding[] = [ // --- Parse bindings at module load --- +/** + * The stored value meaning "the user deliberately unassigned this shortcut". + * Distinct from a missing key, which means "no override, use the default". + */ +export const UNASSIGNED_COMBO = null; + +/** + * Parse a binding's combo string into a chord. + * + * An empty combo yields an empty chord, which never matches any event — the + * matcher skips bindings whose first combo is missing. That is how both a + * user-unassigned shortcut and a binding authored without a default combo are + * represented: one state, not two. Authors who want a default-less binding + * write `combo: ""` and nothing else. + */ +export function parseBindingChord(combo: string): KeyCombo[] { + if (combo === "") { + return []; + } + return parseChordString(combo); +} + function parseBinding(binding: ShortcutBinding): ParsedShortcutBinding { - const parsedChord = parseChordString(binding.combo); + const parsedChord = parseBindingChord(binding.combo); const lastCombo = parsedChord.at(-1); if (binding.repeat === false && lastCombo) { lastCombo.repeat = false; @@ -1076,15 +1101,21 @@ function parseBinding(binding: ShortcutBinding): ParsedShortcutBinding { export const DEFAULT_BINDINGS: readonly ParsedShortcutBinding[] = SHORTCUT_BINDINGS.map(parseBinding); -export function buildEffectiveBindings(overrides: Record): ParsedShortcutBinding[] { +export type ShortcutOverrides = Record; + +export function buildEffectiveBindings(overrides: ShortcutOverrides): ParsedShortcutBinding[] { return DEFAULT_BINDINGS.map(function (binding) { const override = overrides[binding.id]; - if (override === undefined) { + if (override === UNASSIGNED_COMBO) { + return { ...binding, combo: "", parsedChord: [] }; + } + // Storage is unvalidated JSON, so anything can turn up here. + if (typeof override !== "string") { return binding; } let parsedChord: KeyCombo[]; try { - parsedChord = parseChordString(override); + parsedChord = parseBindingChord(override); } catch { return binding; } @@ -1394,45 +1425,110 @@ export function getBindingIdForAction( export function getDefaultKeysForAction( actionId: string, platform: { isMac: boolean; isDesktop: boolean }, + bindings: readonly ParsedShortcutBinding[] = DEFAULT_BINDINGS, ): ShortcutKey[] | null { - for (const binding of DEFAULT_BINDINGS) { + for (const binding of bindings) { if (binding.help?.id !== actionId) { continue; } if (!helpMatchesPlatform(binding.when, platform)) { continue; } + // `help.keys` is hand-authored, so it cannot be trusted to be empty for a + // binding that ships without a default combo. The parsed chord is derived + // from `combo`, so it is the single source of truth for "has no keys". + if (binding.parsedChord.length === 0) { + return null; + } return binding.help.keys; } return null; } +/** + * The keys to display for a shortcut: the user's override if they set one, + * the default otherwise, and `null` when the shortcut has no keys at all — + * either the user unassigned it or it ships without a default combo. + * + * The single resolver behind every display surface (hint badges, the command + * palette, and the settings rows). It validates an override the same way + * matching does, so what is shown is always what actually fires. + */ export function resolveShortcutKeysForAction( actionId: string, - overrides: Readonly>, + overrides: ShortcutOverrides, platform: { isMac: boolean; isDesktop: boolean }, ): ShortcutKey[][] | null { const bindingId = getBindingIdForAction(actionId, platform); - if (!bindingId) return null; - const override = overrides[bindingId]; - if (override) return chordStringToShortcutKeys(override); + if (bindingId === null) { + return null; + } + const defaultKeys = getDefaultKeysForAction(actionId, platform); - return defaultKeys ? [defaultKeys] : null; + const defaultChord = defaultKeys ? [defaultKeys] : null; + + const override = overrides[bindingId]; + if (override === UNASSIGNED_COMBO || override === "") { + return null; + } + // Storage is unvalidated JSON: a missing key and a corrupt value both mean + // "fall back to the default". + if (typeof override !== "string") { + return defaultChord; + } + try { + parseBindingChord(override); + } catch { + // Matching falls back to the default for an unparseable override, so the + // display has to as well or it would advertise keys that do nothing. + return defaultChord; + } + return chordStringToShortcutKeys(override); } /** * The `KeyboardEvent.key` whose hold reveals the sidebar workspace-jump number - * badges. It must match the modifier of the active `workspace.navigate.index` - * binding for this runtime, otherwise the badges appear for a modifier that - * does not actually jump: Alt on web, Cmd (Meta) on desktop Mac, Ctrl on - * desktop non-Mac. + * badges, or `null` when no badges should appear. + * + * It must match the modifier of the active `workspace.navigate.index` binding + * for this runtime, otherwise the badges appear for a modifier that does not + * actually jump. That binding is parameterized — its key is the `Digit` + * wildcard, which stands for any of 1-9 — so the badges are only honest when + * the effective binding is still a single combo built on that wildcard. + * Anything else (unassigned, rebound to one concrete digit, or a multi-step + * chord) yields `null`, because the 1-9 badges would be advertising more than + * the shortcut delivers. */ -export function getWorkspaceIndexJumpModifierKey(platform: { - isMac: boolean; - isDesktop: boolean; -}): "Alt" | "Meta" | "Control" { - if (!platform.isDesktop) return "Alt"; - return platform.isMac ? "Meta" : "Control"; +export function getWorkspaceIndexJumpModifierKey( + platform: { isMac: boolean; isDesktop: boolean }, + bindings: readonly ParsedShortcutBinding[] = DEFAULT_BINDINGS, +): "Alt" | "Meta" | "Control" | null { + const binding = bindings.find(function (candidate) { + return ( + candidate.action === "workspace.navigate.index" && + helpMatchesPlatform(candidate.when, platform) + ); + }); + if (!binding || binding.parsedChord.length !== 1) { + return null; + } + + const combo = binding.parsedChord[0]; + if (!combo || combo.code !== "Digit") { + return null; + } + // Exactly one modifier: holding it is what reveals the badges, so a combo + // needing a second one would show badges the user cannot act on. + const modifiers = [combo.mod, combo.meta, combo.ctrl, combo.alt, combo.shift]; + if (modifiers.filter(Boolean).length !== 1) { + return null; + } + + if (combo.mod) return platform.isMac ? "Meta" : "Control"; + if (combo.meta) return "Meta"; + if (combo.ctrl) return "Control"; + if (combo.alt) return "Alt"; + return null; } export function buildKeyboardShortcutHelpSections( @@ -1470,7 +1566,8 @@ export function buildKeyboardShortcutHelpSections( id: help.id, label: help.label, labelKey: SHORTCUT_HELP_LABEL_KEYS[help.id] ?? help.label, - keys: help.keys, + // An empty chord has no keys to show, whatever `help.keys` was authored as. + keys: binding.parsedChord.length === 0 ? [] : help.keys, ...(help.note ? { note: help.note } : {}), ...(SHORTCUT_HELP_NOTE_KEYS[help.id] ? { noteKey: SHORTCUT_HELP_NOTE_KEYS[help.id] } : {}), }); diff --git a/packages/app/src/keyboard/shortcut-override-store.test.ts b/packages/app/src/keyboard/shortcut-override-store.test.ts new file mode 100644 index 000000000..1f07ce046 --- /dev/null +++ b/packages/app/src/keyboard/shortcut-override-store.test.ts @@ -0,0 +1,183 @@ +import { describe, expect, it } from "vitest"; +import type { ShortcutOverrides } from "@/keyboard/keyboard-shortcuts"; +import { + createShortcutOverrideStore, + type ShortcutOverrideStore, +} from "@/keyboard/shortcut-override-store"; + +interface Harness { + store: ShortcutOverrideStore; + /** What the UI would render. */ + cached(): ShortcutOverrides; + /** What a relaunch would load. */ + persisted(): ShortcutOverrides | null; + errors: unknown[]; + /** Cache and storage transitions in the order they happened. */ + events: string[]; +} + +/** + * @param failWrites 1-based indexes of `storage.write` calls that reject. + */ +function createHarness( + options: { seed?: ShortcutOverrides; failWrites?: number[]; failRemove?: boolean } = {}, +): Harness { + const failWrites = new Set(options.failWrites ?? []); + let cache: ShortcutOverrides = options.seed ?? {}; + let stored: string | null = options.seed ? JSON.stringify(options.seed) : null; + let writeCount = 0; + const errors: unknown[] = []; + const events: string[] = []; + + const store = createShortcutOverrideStore({ + cache: { + read: () => cache, + write: (next) => { + cache = next; + events.push(`cache ${JSON.stringify(next)}`); + }, + }, + storage: { + write: async (serialized) => { + writeCount += 1; + const failed = failWrites.has(writeCount); + await settle(1); + if (failed) { + events.push(`write-failed ${serialized}`); + throw new Error("storage unavailable"); + } + stored = serialized; + events.push(`write ${serialized}`); + }, + // Slower than a write, so an unserialized reset would land last and + // delete whatever was written after it. + remove: async () => { + await settle(3); + if (options.failRemove) { + events.push("remove-failed"); + throw new Error("storage unavailable"); + } + stored = null; + events.push("remove"); + }, + }, + onError: (err) => errors.push(err), + }); + + return { + store, + cached: () => cache, + persisted: () => (stored === null ? null : (JSON.parse(stored) as ShortcutOverrides)), + errors, + events, + }; +} + +/** Deterministic stand-in for storage latency: yield `ticks` microtasks. */ +async function settle(ticks: number): Promise { + for (let i = 0; i < ticks; i += 1) await Promise.resolve(); +} + +describe("createShortcutOverrideStore", () => { + it("persists a rebind", async () => { + const h = createHarness(); + + await h.store.set("new-workspace", "Mod+Shift+N"); + + expect(h.cached()).toEqual({ "new-workspace": "Mod+Shift+N" }); + expect(h.persisted()).toEqual({ "new-workspace": "Mod+Shift+N" }); + expect(h.errors).toEqual([]); + }); + + it("distinguishes unassigned from absent", async () => { + const h = createHarness(); + + await h.store.clear("new-workspace"); + expect(h.persisted()).toEqual({ "new-workspace": null }); + + await h.store.remove("new-workspace"); + expect(h.persisted()).toEqual({}); + expect(h.cached()).not.toHaveProperty("new-workspace"); + }); + + it("reverts the cache and reports when the write fails", async () => { + const h = createHarness({ seed: { "new-workspace": "Mod+1" }, failWrites: [1] }); + + await h.store.clear("new-workspace"); + + expect(h.cached()).toEqual({ "new-workspace": "Mod+1" }); + expect(h.persisted()).toEqual({ "new-workspace": "Mod+1" }); + expect(h.errors).toHaveLength(1); + }); + + it("drops the whole map on reset", async () => { + const h = createHarness({ seed: { "new-workspace": "Mod+1" } }); + + await h.store.resetAll(); + + expect(h.cached()).toEqual({}); + expect(h.persisted()).toBeNull(); + }); + + it("restores the map when the reset fails", async () => { + const h = createHarness({ seed: { "new-workspace": "Mod+1" }, failRemove: true }); + + await h.store.resetAll(); + + expect(h.cached()).toEqual({ "new-workspace": "Mod+1" }); + expect(h.persisted()).toEqual({ "new-workspace": "Mod+1" }); + expect(h.errors).toHaveLength(1); + }); + + it("keeps the later of two changes to the same binding when the earlier write fails", async () => { + const h = createHarness({ failWrites: [1] }); + + // Both dispatched before either settles -- the settings row fires these + // without awaiting. + const first = h.store.set("new-workspace", "Mod+1"); + const second = h.store.set("new-workspace", "Mod+2"); + await Promise.all([first, second]); + + // The failed write must not roll its own stale value back over the one + // that landed, or the row would show Mod+1 and the relaunch would fire + // Mod+2. + expect(h.cached()).toEqual({ "new-workspace": "Mod+2" }); + expect(h.persisted()).toEqual({ "new-workspace": "Mod+2" }); + expect(h.errors).toHaveLength(1); + }); + + it("keeps a rebind made after Reset all", async () => { + const h = createHarness({ seed: { "new-workspace": "Mod+1" } }); + + const reset = h.store.resetAll(); + const rebind = h.store.set("open-settings", "Mod+2"); + await Promise.all([reset, rebind]); + + // The removal must not land after the write, or the new binding is gone on + // the next launch while the row still shows it. + expect(h.cached()).toEqual({ "open-settings": "Mod+2" }); + expect(h.persisted()).toEqual({ "open-settings": "Mod+2" }); + }); + + it("finishes each operation before starting the next", async () => { + const h = createHarness(); + + await Promise.all([h.store.set("a", "Mod+1"), h.store.set("b", "Mod+2")]); + + expect(h.events).toEqual([ + 'cache {"a":"Mod+1"}', + 'write {"a":"Mod+1"}', + 'cache {"a":"Mod+1","b":"Mod+2"}', + 'write {"a":"Mod+1","b":"Mod+2"}', + ]); + }); + + it("keeps running after a failed operation", async () => { + const h = createHarness({ failWrites: [1] }); + + await h.store.set("a", "Mod+1"); + await h.store.set("b", "Mod+2"); + + expect(h.persisted()).toEqual({ b: "Mod+2" }); + }); +}); diff --git a/packages/app/src/keyboard/shortcut-override-store.ts b/packages/app/src/keyboard/shortcut-override-store.ts new file mode 100644 index 000000000..de203cce4 --- /dev/null +++ b/packages/app/src/keyboard/shortcut-override-store.ts @@ -0,0 +1,116 @@ +import { UNASSIGNED_COMBO, type ShortcutOverrides } from "@/keyboard/keyboard-shortcuts"; + +const EMPTY_OVERRIDES: ShortcutOverrides = {}; + +/** A binding's stored value, or `undefined` to drop its override entirely. */ +export type ShortcutOverrideValue = ShortcutOverrides[string] | undefined; + +/** Where the override map is persisted. Async and able to fail. */ +export interface ShortcutOverrideStorage { + write(serialized: string): Promise; + remove(): Promise; +} + +/** What the UI renders from. Synchronous and always succeeds. */ +export interface ShortcutOverrideCache { + read(): ShortcutOverrides; + write(next: ShortcutOverrides): void; +} + +export interface ShortcutOverrideStore { + set(bindingId: string, comboString: string): Promise; + /** Unassign the shortcut: it keeps a row in Settings but matches nothing. */ + clear(bindingId: string): Promise; + /** Drop the override entirely, restoring the binding's default combo. */ + remove(bindingId: string): Promise; + resetAll(): Promise; +} + +export interface CreateShortcutOverrideStoreOptions { + storage: ShortcutOverrideStorage; + cache: ShortcutOverrideCache; + onError(err: unknown): void; +} + +/** + * Applies override changes to the cache optimistically and persists them, + * reverting the cache if the write fails. + * + * Operations are serialized: each one runs to completion before the next + * starts. Two things go wrong without that ordering, and both end with the + * user's shortcut changing on its own after a restart. Two changes to the same + * binding can interleave so the earlier one's failure rolls back a value the + * later one already persisted; and a Reset all can delete a rebind the user + * made after it, because `remove` and `write` race. Serializing also makes the + * rollback a whole-map restore again — nothing else can have touched the cache + * while the write was in flight. + * + * The cost is that the second of two rapid changes waits one storage + * round-trip before its row updates. Every change here comes from a settings + * row the user has to click through, so that queue is never more than a couple + * of operations deep. + */ +export function createShortcutOverrideStore({ + storage, + cache, + onError, +}: CreateShortcutOverrideStoreOptions): ShortcutOverrideStore { + let tail: Promise = Promise.resolve(); + + function enqueue(operation: () => Promise): Promise { + const run = tail.then(operation); + // A rejected tail would wedge every later operation, so the chain that the + // next caller waits on swallows failures. The returned promise does not. + tail = run.then(noop, noop); + return run; + } + + function commit(bindingId: string, value: ShortcutOverrideValue): Promise { + return enqueue(async () => { + const prev = cache.read(); + const next = withOverride(prev, bindingId, value); + cache.write(next); + try { + await storage.write(JSON.stringify(next)); + } catch (err) { + cache.write(prev); + onError(err); + } + }); + } + + return { + set: (bindingId, comboString) => commit(bindingId, comboString), + clear: (bindingId) => commit(bindingId, UNASSIGNED_COMBO), + remove: (bindingId) => commit(bindingId, undefined), + resetAll: () => + enqueue(async () => { + const prev = cache.read(); + cache.write(EMPTY_OVERRIDES); + try { + await storage.remove(); + } catch (err) { + cache.write(prev); + onError(err); + } + }), + }; +} + +/** + * `null` (unassigned) and absent (use the default) are distinct states, so + * dropping an override cannot go through a spread. + */ +export function withOverride( + overrides: ShortcutOverrides, + bindingId: string, + value: ShortcutOverrideValue, +): ShortcutOverrides { + if (value === undefined) { + const { [bindingId]: _removed, ...rest } = overrides; + return rest; + } + return { ...overrides, [bindingId]: value }; +} + +function noop(): void {} diff --git a/packages/app/src/screens/settings/keyboard-shortcuts-section.tsx b/packages/app/src/screens/settings/keyboard-shortcuts-section.tsx index b81dcb4ed..d0171cfe8 100644 --- a/packages/app/src/screens/settings/keyboard-shortcuts-section.tsx +++ b/packages/app/src/screens/settings/keyboard-shortcuts-section.tsx @@ -1,24 +1,34 @@ import { useCallback, useEffect, useMemo, useState } from "react"; import { useTranslation } from "react-i18next"; -import { View, Text } from "react-native"; +import { View, Text, type PressableStateCallbackType } from "react-native"; import { useIsFocused } from "@react-navigation/native"; -import { StyleSheet } from "react-native-unistyles"; +import { MoreHorizontal, Pencil, Undo2, X } from "lucide-react-native"; +import { StyleSheet, withUnistyles } from "react-native-unistyles"; +import type { Theme } from "@/styles/theme"; import { settingsStyles } from "@/styles/settings"; import { SettingsSection } from "@/screens/settings/settings-section"; import { Button } from "@/components/ui/button"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, +} from "@/components/ui/dropdown-menu"; import { Shortcut } from "@/components/ui/shortcut"; import { useKeyboardShortcutOverrides } from "@/hooks/use-keyboard-shortcut-overrides"; import { buildKeyboardShortcutHelpSections, getBindingIdForAction, + getDefaultKeysForAction, + resolveShortcutKeysForAction, type KeyboardShortcutHelpRow, } from "@/keyboard/keyboard-shortcuts"; import { - chordStringToShortcutKeys, comboStringToShortcutKeys, heldModifiersFromEvent, keyboardEventToComboString, } from "@/keyboard/shortcut-string"; +import type { ShortcutKey } from "@/utils/format-shortcut"; import { useKeyboardShortcutsStore } from "@/stores/keyboard-shortcuts-store"; import { getShortcutOs } from "@/utils/shortcut-platform"; import { getIsElectronRuntime } from "@/constants/layout"; @@ -27,6 +37,18 @@ import { getDesktopHost } from "@/desktop/host"; const EMPTY_CAPTURED_COMBOS: string[] = []; +const ThemedMoreHorizontal = withUnistyles(MoreHorizontal); +const ThemedPencil = withUnistyles(Pencil); +const ThemedUndo2 = withUnistyles(Undo2); +const ThemedX = withUnistyles(X); + +const foregroundColorMapping = (theme: Theme) => ({ color: theme.colors.foreground }); +const foregroundMutedColorMapping = (theme: Theme) => ({ color: theme.colors.foregroundMuted }); + +const bindLeadingIcon = ; +const clearLeadingIcon = ; +const resetLeadingIcon = ; + function ShortcutSequence({ chord, heldModifiers, @@ -53,32 +75,42 @@ function ShortcutSequence({ interface ShortcutRowContainerProps { row: KeyboardShortcutHelpRow; bindingId: string | null; - overrideCombo: string | undefined; + displayChord: ShortcutKey[][] | null; + hasOverride: boolean; + hasDefault: boolean; isCapturing: boolean; capturedCombos: string[]; heldModifiers: string | null; onStartCapture: (bindingId: string) => void; onSaveCapture: () => void; onCancelCapture: () => void; + onClearOverride: (bindingId: string) => void; onRemoveOverride: (bindingId: string) => void; } function ShortcutRowContainer({ row, bindingId, - overrideCombo, + displayChord, + hasOverride, + hasDefault, isCapturing, capturedCombos, heldModifiers, onStartCapture, onSaveCapture, onCancelCapture, + onClearOverride, onRemoveOverride, }: ShortcutRowContainerProps) { const handleRebind = useCallback(() => { if (bindingId) onStartCapture(bindingId); }, [bindingId, onStartCapture]); + const handleClear = useCallback(() => { + if (bindingId) onClearOverride(bindingId); + }, [bindingId, onClearOverride]); + const handleReset = useCallback(() => { if (bindingId) onRemoveOverride(bindingId); }, [bindingId, onRemoveOverride]); @@ -87,75 +119,204 @@ function ShortcutRowContainer({ ); } +function ShortcutRowKeys({ + displayChord, + isCapturing, + capturedCombos, + heldModifiers, +}: { + displayChord: ShortcutKey[][] | null; + isCapturing: boolean; + capturedCombos: string[]; + heldModifiers: string | null; +}) { + const { t } = useTranslation(); + + if (isCapturing) { + return ; + } + if (displayChord === null) { + return {t("settings.shortcuts.unassigned")}; + } + return ; +} + +function ShortcutActionsMenu({ + row, + bindLabel, + showClear, + showReset, + onRebind, + onClear, + onReset, +}: { + row: KeyboardShortcutHelpRow; + bindLabel: "bind" | "rebind"; + showClear: boolean; + showReset: boolean; + onRebind: () => void; + onClear: () => void; + onReset: () => void; +}) { + const { t } = useTranslation(); + const triggerStyle = useCallback( + ({ + pressed, + hovered, + open, + }: PressableStateCallbackType & { hovered?: boolean; open?: boolean }) => [ + styles.menuButton, + (hovered || open) && styles.menuButtonHovered, + pressed && styles.menuButtonPressed, + ], + [], + ); + + return ( + + + {({ hovered, open }) => ( + + )} + + + + {t(`settings.shortcuts.actions.${bindLabel}`)} + + {showClear && ( + + {t("settings.shortcuts.actions.clear")} + + )} + {showReset && ( + + {t("settings.shortcuts.actions.reset")} + + )} + + + ); +} + function ShortcutRow({ row, bindingId, - overrideCombo, + displayChord, + hasOverride, + hasDefault, isCapturing, capturedCombos, heldModifiers, onRebind, onDone, onCancel, + onClear, onReset, }: { row: KeyboardShortcutHelpRow; bindingId: string | null; - overrideCombo: string | undefined; + displayChord: ShortcutKey[][] | null; + hasOverride: boolean; + hasDefault: boolean; isCapturing: boolean; capturedCombos: string[]; heldModifiers: string | null; onRebind: () => void; onDone: () => void; onCancel: () => void; + onClear: () => void; onReset: () => void; }) { const { t } = useTranslation(); - const displayChord = useMemo( - () => (overrideCombo ? chordStringToShortcutKeys(overrideCombo) : [row.keys]), - [overrideCombo, row.keys], - ); const rowStyle = useMemo(() => [styles.row, isCapturing && styles.rowCapturing], [isCapturing]); + const isBindable = bindingId !== null; + const showDone = isCapturing && capturedCombos.length > 0; + const showClear = displayChord !== null; + // Reset restores the default, so it is only meaningful when there is a + // default to restore. A binding that ships without one would otherwise show a + // Reset that lands on the same "Not set" state Clear already produced. + const showReset = hasOverride && hasDefault; + // Nothing is bound in the unassigned state, so there is nothing to *re*-bind. + const bindLabel = displayChord === null ? "bind" : "rebind"; + return ( {t(row.labelKey)} + + + {isCapturing ? ( - - ) : ( - - )} - {bindingId !== null && ( <> - {isCapturing && capturedCombos.length > 0 ? ( + {showDone && ( - ) : null} - + )} + {isBindable && ( + + )} - )} - {overrideCombo !== undefined && !isCapturing && ( - + ) : ( + // Fixed slot, occupied or not, so the keys column keeps one rail on + // every row instead of sliding with whatever actions the row offers. + + {isBindable && ( + + )} + )} @@ -167,7 +328,7 @@ export function KeyboardShortcutsSection() { const [capturingBindingId, setCapturingBindingId] = useState(null); const [capturedCombos, setCapturedCombos] = useState([]); const [heldModifiers, setHeldModifiers] = useState(null); - const { overrides, hasOverrides, setOverride, removeOverride, resetAll } = + const { overrides, hasOverrides, setOverride, clearOverride, removeOverride, resetAll } = useKeyboardShortcutOverrides(); const setCapturingShortcut = useKeyboardShortcutsStore((s) => s.setCapturingShortcut); const capturing = useKeyboardShortcutsStore((s) => s.capturingShortcut); @@ -256,6 +417,10 @@ export function KeyboardShortcutsSection() { }, [capturing]); const handleResetAll = useCallback(() => void resetAll(), [resetAll]); + const handleClearOverride = useCallback( + (bindingId: string) => void clearOverride(bindingId), + [clearOverride], + ); const handleRemoveOverride = useCallback( (bindingId: string) => void removeOverride(bindingId), [removeOverride], @@ -288,18 +453,23 @@ export function KeyboardShortcutsSection() { > {section.rows.map(function (row, index) { - const bindingId = getBindingIdForAction(row.id, { - isMac, - isDesktop: isDesktopApp, - }); - const overrideCombo = bindingId ? overrides[bindingId] : undefined; + const platform = { isMac, isDesktop: isDesktopApp }; + const bindingId = getBindingIdForAction(row.id, platform); + const displayChord = resolveShortcutKeysForAction(row.id, overrides, platform); + // `in`, not a truthiness check: an unassigned shortcut stores + // null, and Reset has to stay available to undo it. + const hasOverride = bindingId !== null && bindingId in overrides; + // A binding authored with `combo: ""` has nothing to reset to. + const hasDefault = getDefaultKeysForAction(row.id, platform) !== null; return ( {index < section.rows.length - 1 && } @@ -343,11 +514,32 @@ const styles = StyleSheet.create((theme) => ({ alignItems: "center", gap: theme.spacing[2], }, + rowKeys: { + alignItems: "flex-end", + }, + menuSlot: { + width: 32, + height: 32, + }, + menuButton: { + width: 32, + height: 32, + borderRadius: theme.borderRadius.lg, + alignItems: "center", + justifyContent: "center", + }, + menuButtonHovered: { + backgroundColor: theme.colors.surface2, + }, + menuButtonPressed: { + backgroundColor: theme.colors.surface3, + }, capturingText: { fontSize: theme.fontSize.sm, color: theme.colors.foregroundMuted, }, - resetText: { + unassignedText: { + fontSize: theme.fontSize.sm, color: theme.colors.foregroundMuted, }, separator: { diff --git a/packages/desktop/e2e/keyboard-shortcut-unassign.spec.ts b/packages/desktop/e2e/keyboard-shortcut-unassign.spec.ts new file mode 100644 index 000000000..1d70dfca5 --- /dev/null +++ b/packages/desktop/e2e/keyboard-shortcut-unassign.spec.ts @@ -0,0 +1,139 @@ +import type { Page } from "@playwright/test"; +import { test, expect } from "../../app/e2e/support/fixtures"; +import { gotoAppShell, openSettings } from "../../app/e2e/support/helpers/app"; +import { openSettingsSection } from "../../app/e2e/support/helpers/settings"; + +// Settings > Keyboard Shortcuts is desktop-only (`desktopOnly` in +// settings-screen.tsx), and the gate reads `getIsElectronRuntime()`, which only +// checks for `window.paseoDesktop` -- so this belongs in the desktop suite even +// though no `.electron.*` module sits in the surface's import path. +const SHORTCUTS_ROW = "show-shortcuts"; + +/** + * The smallest bridge that makes the app believe it is Electron. The built-in + * daemon is left unmanaged so the app talks to the E2E daemon instead of trying + * to start one of its own. + */ +async function installDesktopBridge(page: Page): Promise { + await page.addInitScript(() => { + window.paseoDesktop = { + platform: "darwin", + events: { on: () => () => {} }, + invoke: async (command: string) => { + if (command === "get_desktop_settings") { + return { + releaseChannel: "stable", + daemon: { manageBuiltInDaemon: false, keepRunningAfterQuit: true }, + }; + } + return null; + }, + }; + }); +} + +async function openShortcutsSettings(page: Page) { + await installDesktopBridge(page); + await gotoAppShell(page); + await openSettings(page); + await openSettingsSection(page, "shortcuts"); + await expect(page.getByText("Show keyboard shortcuts", { exact: true })).toBeVisible({ + timeout: 30_000, + }); +} + +/** + * Every row action lives behind the row's actions menu, so which actions a row + * offers can only be asserted while that menu is open. + */ +async function openRowMenu(page: Page) { + await page.getByTestId(`shortcut-actions-${SHORTCUTS_ROW}`).click(); + await expect(page.getByTestId(`shortcut-bind-${SHORTCUTS_ROW}`)).toBeVisible(); +} + +async function closeRowMenu(page: Page) { + await page.keyboard.press("Escape"); + await expect(page.getByTestId(`shortcut-bind-${SHORTCUTS_ROW}`)).toHaveCount(0); +} + +test("unassigning a shortcut leaves it inert until it is reset", async ({ page }) => { + await openShortcutsSettings(page); + + const clear = page.getByTestId(`shortcut-clear-${SHORTCUTS_ROW}`); + const reset = page.getByTestId(`shortcut-reset-${SHORTCUTS_ROW}`); + const bind = page.getByTestId(`shortcut-bind-${SHORTCUTS_ROW}`); + const notSet = page.getByText("Not set", { exact: true }); + const dialog = page.getByTestId("keyboard-shortcuts-dialog"); + + // The shortcut fires before it is cleared, so the assertion after clearing + // measures the change rather than a shortcut that never worked. + await page.keyboard.press("Shift+?"); + await expect(dialog).toBeVisible({ timeout: 10_000 }); + await page.keyboard.press("Escape"); + await expect(dialog).not.toBeVisible({ timeout: 10_000 }); + + await openRowMenu(page); + await expect(clear).toBeVisible(); + await expect(reset).toHaveCount(0); + await expect(bind).toHaveText("Rebind"); + await clear.click(); + + await expect(notSet).toBeVisible(); + await openRowMenu(page); + await expect(clear).toHaveCount(0); + await expect(reset).toBeVisible(); + // Nothing is bound now, so the item stops offering to *re*-bind. + await expect(bind).toHaveText("Bind"); + await closeRowMenu(page); + + await page.keyboard.press("Shift+?"); + await expect(dialog).not.toBeVisible({ timeout: 5_000 }); + + // The unassignment has to survive a restart, or "cleared" is only a UI state. + // A reload lands back on the app shell, so Settings has to be reopened before + // the section is reachable. + await page.reload(); + await openSettings(page); + await openSettingsSection(page, "shortcuts"); + await expect(notSet).toBeVisible({ timeout: 30_000 }); + await page.keyboard.press("Shift+?"); + await expect(dialog).not.toBeVisible({ timeout: 5_000 }); + + await openRowMenu(page); + await page.getByTestId(`shortcut-reset-${SHORTCUTS_ROW}`).click(); + await expect(notSet).toHaveCount(0); + await openRowMenu(page); + await expect(page.getByTestId(`shortcut-clear-${SHORTCUTS_ROW}`)).toBeVisible(); + await expect(page.getByTestId(`shortcut-bind-${SHORTCUTS_ROW}`)).toHaveText("Rebind"); + await closeRowMenu(page); + + await page.keyboard.press("Shift+?"); + await expect(dialog).toBeVisible({ timeout: 10_000 }); + await page.keyboard.press("Escape"); + await expect(dialog).not.toBeVisible({ timeout: 10_000 }); +}); + +test("an unassigned shortcut lists no keys in the shortcuts cheat sheet", async ({ page }) => { + await openShortcutsSettings(page); + + await openRowMenu(page); + await page.getByTestId(`shortcut-clear-${SHORTCUTS_ROW}`).click(); + await expect(page.getByText("Not set", { exact: true })).toBeVisible(); + + // Reachable from the sidebar even with its own shortcut unassigned. + await gotoAppShell(page); + await page.getByTestId("sidebar-help").click(); + await expect(page.getByTestId("sidebar-help-menu")).toBeVisible(); + await page.getByTestId("sidebar-help-shortcuts").click(); + + const dialog = page.getByTestId("keyboard-shortcuts-dialog"); + await expect(dialog).toBeVisible({ timeout: 10_000 }); + + const row = dialog + .locator("div") + .filter({ hasText: /^Show keyboard shortcuts$/ }) + .first(); + await expect(row).toBeVisible(); + // No badge pill, blank or otherwise, for a shortcut with no keys. + await expect(dialog.getByText("?", { exact: true })).toHaveCount(0); +});