diff --git a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md new file mode 100644 index 00000000000..8b5620b33d5 --- /dev/null +++ b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md @@ -0,0 +1,189 @@ +# Keyboard Recorder — Ctrl Binding & event.code Unification + +**Date:** 2026-04-12 · **Scope:** `apps/desktop/src/renderer/hotkeys/*` (+ 1 terminal file) · **PR:** #3391 + +## TL;DR + +User couldn't bind Ctrl-based shortcuts in Settings → Keyboard. Root cause: the +recorder filtered modifier keys using the wrong string (`"ctrl"` vs the actual +`event.key === "Control"`). Investigation surfaced a cluster of related bugs +all rooted in the recorder using `event.key` while the rest of the system +(registry, library dispatch, resolver) uses `event.code`. Consolidated on +`event.code` via shared helpers. + +## What was broken + +| # | Bug | Fix | +|---|------------------------------------------------------------------------------|--------------------------------------------------------------| +| 1 | Lone Ctrl auto-committed `ctrl+control` before the user pressed key 2 | Filter against `"control"` (the lowercased `event.key` name) + lock keys + altgraph | +| 2 | Recorder used `event.key`, resolver/registry use `event.code` → Shift+digit, Alt+letter on Mac, punctuation, non-US layouts silently unmatchable | Unified recorder on `event.code` via shared `normalizeToken` | +| 3 | `===` string compares missed equivalent chords (`meta+alt+up` ≠ `alt+meta+arrowup`) | Added `canonicalizeChord`; apply at conflict/reset/reserved lookups | +| 4 | Terminal forwarding used a frozen default-only reverse index → rebinds swallowed, freed defaults eaten | Reverse index subscribes to override store and rebuilds on change; `null` overrides drop from index | +| 5 | Migration blindly copied old corrupt overrides into localStorage | Sanitizer canonicalizes and drops entries that don't parse to one word-char key | +| 6 | Terminal helpers (`isTerminalReservedEvent`, `matchesKey`) used event.key and a duplicated `TERMINAL_RESERVED` | Exported `eventToChord` + `matchesChord` + `TERMINAL_RESERVED_CHORDS` as single source of truth; deleted duplicate | + +## Key code changes + +### Shared helpers (`utils/resolveHotkeyFromEvent.ts`) + +Exposes the canonical normalizer and matcher used everywhere: + +```ts +export function normalizeToken(token: string): string; // code/key → canonical +export function isIgnorableKey(normalized: string): boolean; // modifier + lock keys +export function canonicalizeChord(chord: string): string; // stable compare form +export function eventToChord(event: KeyboardEvent): string | null; +export function matchesChord(event: KeyboardEvent, chord: string): boolean; +export const TERMINAL_RESERVED_CHORDS: Set; // canonical form +export const MODIFIERS: Set; +``` + +Reverse index is now live (Bug 4): + +```ts +let registeredAppChords = buildRegisteredAppChords( + useHotkeyOverridesStore.getState().overrides, +); +useHotkeyOverridesStore.subscribe((state) => { + registeredAppChords = buildRegisteredAppChords(state.overrides); +}); +``` + +### Recorder (`hooks/useRecordHotkeys/useRecordHotkeys.ts`) + +Bug 1 + 2 in one: + +```ts +if (event.code === undefined) return null; // synthetic / autofill guard +const key = normalizeToken(event.code); // event.code, not event.key +if (isIgnorableKey(key)) return null; // catches Control/Shift/Alt/Meta/lock +const isFKey = /^f([1-9]|1[0-2])$/.test(key); +if (!isFKey && !event.ctrlKey && !event.metaKey) return null; +// …emit in registry MODIFIER_ORDER to stay string-comparable with defaults. +``` + +Bug 3: `canonicalizeChord` on both sides of every comparison (reset-to-default, +conflict detection, reserved-list lookup). `TERMINAL_RESERVED_CHORDS` imported +from the shared module — no more duplicate. + +### Migration (`migrate.ts`) + +Bug 5: sanitize each migrated value. Drops garbage (`ctrl+control`, +`ctrl+shift+@`, `meta+[`) and logs the count. Preserves `null` (explicit +unassignment). + +```ts +const canonical = canonicalizeChord(value); +const keys = canonical.split("+").filter((p) => !MODIFIERS.has(p)); +if (keys.length !== 1) return undefined; +if (!/^[a-z0-9]+$/.test(keys[0])) return undefined; +return canonical; +``` + +### Terminal helpers + +Bug 6: `utils/utils.ts` and `Terminal/helpers.ts` now use `matchesChord` + +shared `TERMINAL_RESERVED_CHORDS`: + +```ts +// utils/utils.ts +export function isTerminalReservedEvent(event: KeyboardEvent): boolean { + const chord = eventToChord(event); + return chord != null && TERMINAL_RESERVED_CHORDS.has(chord); +} + +// Terminal/helpers.ts — was: matchesKey(event, keys) +if (clearKeys && matchesChord(event, clearKeys)) { … } +``` + +### Display (`display.ts`) + +Runs each chord part through `normalizeToken` and extends `KEY_DISPLAY` to +cover both short (`up`) and canonical (`arrowup`) arrow names plus common +punctuation (`backslash`, `semicolon`, `quote`, `period`, `minus`, `equal`). + +## Library audit — nothing else missed + +Checked every `react-hotkeys-hook` usage against upstream docs: + +| Option | Our use | +|---------------------------------|---------------------------------------------| +| `useKey` (default false → code) | default (matches our `event.code` path) | +| `splitKey` / `sequenceSplitKey` | not used (no `,` multi-binds, no `>` chords)| +| `mod` alias | skipped — per-platform registry covers it | +| `scopes` / `HotkeysProvider` | not used (global `*` scope) | +| `keyup` / `keydown` | default (keydown only) | +| `preventDefault` | default false; callbacks handle when needed | +| `ignoreModifiers` | not used | +| `enableOnFormTags: true` | set in our `useHotkey` helper | + +Registry defaults already use event.code-canonical tokens (`bracketleft`, +`comma`, `slash`, `arrowup`). No hardcoded chord strings found outside +`hotkeys/` that need canonicalization. + +## Decisions taken + +- **Meta (Win/Super) on non-Mac — kept allowed.** Originally blocked; flipped + after review. Power users on tiling WMs / custom Windows configs can bind + Super-based chords. Extended `OS_RESERVED` on Windows with common shell + intercepts (`meta+d/e/l/r/tab`) so users get a "Reserved by OS" *warning* + instead of a silent block. +- **`mod` alias — skipped.** Registry's per-platform `{mac,windows,linux}` + covers the same ground without adding a parsing rule. +- **Migration: dropping invalid entries is better than carrying them.** + Silent corruption is worse than a visible drop count in console. + +## Testability + +Everything fixed is in pure functions over primitives. **62 tests across 4 +files**, no React/DOM harness needed (plain KeyboardEvent stubs): + +| File | Covers | +|-------------------------------------------------|-----------------------------------------------------| +| `utils/resolveHotkeyFromEvent.test.ts` | `normalizeToken`, `isIgnorableKey`, `canonicalizeChord`, `eventToChord`, `matchesChord`, live override index, `isTerminalReservedEvent` parity | +| `utils/overrideSanitizer.test.ts` | migration validation (Bug 5) | +| `hooks/useRecordHotkeys/useRecordHotkeys.test.ts` | recorder capture — all 3 bug classes | +| `display.test.ts` | display formatting parity (short + canonical forms) | + +Only untested branch: non-Mac `PLATFORM` path in the recorder's OS-reserved +warning. Would need module-mocking `PLATFORM`; not worth the harness. + +## Files changed + +``` +apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md (this doc) +apps/desktop/src/renderer/hotkeys/ + display.ts + display.test.ts (new) + migrate.ts + hooks/useRecordHotkeys/useRecordHotkeys.ts + hooks/useRecordHotkeys/useRecordHotkeys.test.ts (new) + utils/resolveHotkeyFromEvent.ts + utils/resolveHotkeyFromEvent.test.ts (new) + utils/utils.ts + utils/overrideSanitizer.test.ts (new) + utils/index.ts (barrel) + index.ts (barrel) +apps/desktop/src/renderer/screens/.../Terminal/helpers.ts +``` + +## Test plan (manual QA) + +- [ ] macOS: Settings → Keyboard → Record, press Cmd alone → no auto-commit, still recording +- [ ] Press Ctrl alone → no auto-commit (was the reported bug) +- [ ] Press Ctrl+Shift+2 → captures `ctrl+shift+2`, not `ctrl+shift+@` +- [ ] Press Meta+[ → captures `meta+bracketleft` +- [ ] Rebind a hotkey, press the new chord inside a terminal pane → fires +- [ ] Press the OLD default of a rebound hotkey in terminal → not swallowed +- [ ] Unassign (Backspace while recording) → old chord no longer swallowed in terminal +- [ ] Rebind CLEAR_TERMINAL to `ctrl+shift+bracketleft`, press it → clears (Bug 6) +- [ ] Windows: try binding Win+R → allowed with "Reserved by OS" warning + +## Sources + +- [react-hotkeys-hook GitHub](https://github.com/JohannesKlauss/react-hotkeys-hook) +- [`parseHotkeys.ts`](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/parseHotkeys.ts) — upstream modifier table + `mapCode` +- [`useRecordHotkeys.ts`](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts) — upstream uses `event.code` by default and guards `event.code === undefined` +- [`useHotkeys` docs](https://react-hotkeys-hook.vercel.app/docs/api/use-hotkeys) — all options reviewed +- [MDN — KeyboardEvent.key values](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values) (`"Control"` not `"Ctrl"`) +- [MDN — KeyboardEvent.code values](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_code_values) diff --git a/apps/desktop/src/renderer/hotkeys/display.test.ts b/apps/desktop/src/renderer/hotkeys/display.test.ts new file mode 100644 index 00000000000..9e56bbac551 --- /dev/null +++ b/apps/desktop/src/renderer/hotkeys/display.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from "bun:test"; +import { formatHotkeyDisplay } from "./display"; + +describe("formatHotkeyDisplay", () => { + it("formats a mac chord with modifier glyphs and no separator", () => { + const result = formatHotkeyDisplay("meta+shift+n", "mac"); + expect(result.text).toBe("⌘⇧N"); + expect(result.keys).toEqual(["⌘", "⇧", "N"]); + }); + + it("formats a windows chord with named modifiers and `+` separators", () => { + const result = formatHotkeyDisplay("ctrl+shift+n", "windows"); + expect(result.text).toBe("Ctrl+Shift+N"); + }); + + it("renders short arrow aliases and canonical arrow names identically", () => { + const short = formatHotkeyDisplay("meta+alt+up", "mac"); + const canonical = formatHotkeyDisplay("alt+meta+arrowup", "mac"); + expect(short.text).toBe("⌘⌥↑"); + expect(canonical.text).toBe("⌘⌥↑"); + }); + + it("renders punctuation tokens with their character", () => { + expect(formatHotkeyDisplay("meta+bracketleft", "mac").text).toBe("⌘["); + expect(formatHotkeyDisplay("meta+comma", "mac").text).toBe("⌘,"); + expect(formatHotkeyDisplay("ctrl+backslash", "linux").text).toBe("Ctrl+\\"); + expect(formatHotkeyDisplay("ctrl+slash", "linux").text).toBe("Ctrl+/"); + }); + + it("treats `control` as `ctrl`", () => { + const result = formatHotkeyDisplay("control+k", "windows"); + expect(result.text).toBe("Ctrl+K"); + }); + + it("returns Unassigned for null or chords with no key token", () => { + expect(formatHotkeyDisplay(null, "mac")).toEqual({ + keys: ["Unassigned"], + text: "Unassigned", + }); + expect(formatHotkeyDisplay("meta", "mac")).toEqual({ + keys: ["Unassigned"], + text: "Unassigned", + }); + }); +}); diff --git a/apps/desktop/src/renderer/hotkeys/display.ts b/apps/desktop/src/renderer/hotkeys/display.ts index 4eff9da0fdb..269f55a1183 100644 --- a/apps/desktop/src/renderer/hotkeys/display.ts +++ b/apps/desktop/src/renderer/hotkeys/display.ts @@ -4,6 +4,7 @@ */ import type { HotkeyDisplay, Platform } from "./types"; +import { normalizeToken } from "./utils/resolveHotkeyFromEvent"; const MODIFIER_DISPLAY: Record> = { mac: { meta: "⌘", ctrl: "⌃", alt: "⌥", shift: "⇧" }, @@ -11,41 +12,57 @@ const MODIFIER_DISPLAY: Record> = { linux: { meta: "Super", ctrl: "Ctrl", alt: "Alt", shift: "Shift" }, }; +// Keyed by canonical (event.code-normalized) tokens. normalizeToken aliases +// the short forms (`up` → `arrowup`, `esc` → `escape`) so only canonical +// names need entries here. const KEY_DISPLAY: Record = { enter: "↵", backspace: "⌫", delete: "⌦", escape: "⎋", tab: "⇥", - up: "↑", - down: "↓", - left: "←", - right: "→", + arrowup: "↑", + arrowdown: "↓", + arrowleft: "←", + arrowright: "→", space: "␣", slash: "/", + backslash: "\\", comma: ",", + period: ".", + semicolon: ";", + quote: "'", + backquote: "`", + minus: "-", + equal: "=", bracketleft: "[", bracketright: "]", }; const MODIFIER_ORDER = ["meta", "ctrl", "alt", "shift"] as const; +type Modifier = (typeof MODIFIER_ORDER)[number]; + +const isModifier = (p: string): p is Modifier => + (MODIFIER_ORDER as readonly string[]).includes(p); /** - * Format a key string into display symbols. - * e.g. "meta+shift+n" on mac → { keys: ["⌘", "⇧", "N"], text: "⌘⇧N" } + * Format a chord string into display symbols. + * e.g. `"meta+shift+n"` on mac → `{ keys: ["⌘", "⇧", "N"], text: "⌘⇧N" }` */ export function formatHotkeyDisplay( keys: string | null, platform: Platform, ): HotkeyDisplay { if (!keys) return { keys: ["Unassigned"], text: "Unassigned" }; - const parts = keys.toLowerCase().split("+"); - const modifiers = parts.filter((p) => - MODIFIER_ORDER.includes(p as (typeof MODIFIER_ORDER)[number]), - ); - const key = parts.find( - (p) => !MODIFIER_ORDER.includes(p as (typeof MODIFIER_ORDER)[number]), - ); + + const parts = keys + .toLowerCase() + .split("+") + .map(normalizeToken) + .map((p) => (p === "control" ? "ctrl" : p)); + + const modifiers = parts.filter(isModifier); + const key = parts.find((p) => !isModifier(p)); if (!key) return { keys: ["Unassigned"], text: "Unassigned" }; const modSymbols = MODIFIER_ORDER.filter((m) => modifiers.includes(m)).map( diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts new file mode 100644 index 00000000000..0dd60664767 --- /dev/null +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts @@ -0,0 +1,143 @@ +import { describe, expect, it } from "bun:test"; +import { captureHotkeyFromEvent } from "./useRecordHotkeys"; + +/** + * Covers the three regressions fixed in + * apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md + * + * Note: `captureHotkeyFromEvent` reads `PLATFORM` via registry.ts, which in a + * Bun test runtime without a DOM navigator resolves to "mac". The meta-on- + * non-Mac branch is exercised indirectly via review, not here. + */ + +// Bun test runtime has no DOM KeyboardEvent; stub just what the code reads. +interface StubInit { + code?: string | undefined; + key?: string; + ctrlKey?: boolean; + metaKey?: boolean; + altKey?: boolean; + shiftKey?: boolean; +} +function ev(init: StubInit): KeyboardEvent { + return { + type: "keydown", + // Preserve explicit `undefined` so the captureHotkeyFromEvent guard + // against synthetic events (no event.code) is actually exercised. + ...("code" in init ? { code: init.code } : { code: "" }), + key: init.key ?? "", + ctrlKey: !!init.ctrlKey, + metaKey: !!init.metaKey, + altKey: !!init.altKey, + shiftKey: !!init.shiftKey, + preventDefault() {}, + stopPropagation() {}, + } as unknown as KeyboardEvent; +} + +describe("captureHotkeyFromEvent — Bug 1: lone Ctrl must not auto-commit", () => { + it("returns null when only Control is pressed", () => { + expect( + captureHotkeyFromEvent(ev({ code: "ControlLeft", ctrlKey: true })), + ).toBeNull(); + expect( + captureHotkeyFromEvent(ev({ code: "ControlRight", ctrlKey: true })), + ).toBeNull(); + }); + + it("returns null for every other lone modifier", () => { + expect( + captureHotkeyFromEvent(ev({ code: "ShiftLeft", shiftKey: true })), + ).toBeNull(); + expect( + captureHotkeyFromEvent(ev({ code: "AltLeft", altKey: true })), + ).toBeNull(); + expect( + captureHotkeyFromEvent(ev({ code: "MetaLeft", metaKey: true })), + ).toBeNull(); + }); + + it("ignores lock keys even if Ctrl is also held", () => { + expect( + captureHotkeyFromEvent(ev({ code: "CapsLock", ctrlKey: true })), + ).toBeNull(); + expect( + captureHotkeyFromEvent(ev({ code: "NumLock", ctrlKey: true })), + ).toBeNull(); + }); +}); + +describe("captureHotkeyFromEvent — Bug 2: uses event.code, not event.key", () => { + it("Ctrl+Shift+2 produces ctrl+shift+2 (not ctrl+shift+@)", () => { + const captured = captureHotkeyFromEvent( + ev({ code: "Digit2", key: "@", ctrlKey: true, shiftKey: true }), + ); + expect(captured).toBe("ctrl+shift+2"); + }); + + it("Alt+L on Mac (where event.key is `¬`) produces alt+l via event.code", () => { + // This event would only pass the `must include ctrl/meta` gate with ctrl + // also held; the point of the test is that we read `code`, not `key`. + const captured = captureHotkeyFromEvent( + ev({ code: "KeyL", key: "¬", ctrlKey: true, altKey: true }), + ); + expect(captured).toBe("ctrl+alt+l"); + }); + + it("Ctrl+[ produces ctrl+bracketleft (registry form)", () => { + const captured = captureHotkeyFromEvent( + ev({ code: "BracketLeft", key: "[", ctrlKey: true }), + ); + expect(captured).toBe("ctrl+bracketleft"); + }); + + it("Ctrl+/ produces ctrl+slash", () => { + const captured = captureHotkeyFromEvent( + ev({ code: "Slash", key: "/", ctrlKey: true }), + ); + expect(captured).toBe("ctrl+slash"); + }); + + it("Meta+Alt+ArrowUp produces meta+alt+arrowup", () => { + const captured = captureHotkeyFromEvent( + ev({ code: "ArrowUp", key: "ArrowUp", metaKey: true, altKey: true }), + ); + expect(captured).toBe("meta+alt+arrowup"); + }); + + it("F-keys are accepted without requiring a modifier", () => { + expect(captureHotkeyFromEvent(ev({ code: "F1", key: "F1" }))).toBe("f1"); + expect(captureHotkeyFromEvent(ev({ code: "F12", key: "F12" }))).toBe("f12"); + }); + + it("requires ctrl or meta for non-F-keys", () => { + expect( + captureHotkeyFromEvent(ev({ code: "KeyA", key: "a", shiftKey: true })), + ).toBeNull(); + expect( + captureHotkeyFromEvent(ev({ code: "KeyA", key: "a", altKey: true })), + ).toBeNull(); + }); + + it("returns null when event.code is undefined (synthetic / autofill events)", () => { + expect( + captureHotkeyFromEvent(ev({ code: undefined, ctrlKey: true })), + ).toBeNull(); + }); +}); + +describe("captureHotkeyFromEvent — modifier ordering", () => { + it("emits modifiers in MODIFIER_ORDER (meta, ctrl, alt, shift)", () => { + const captured = captureHotkeyFromEvent( + ev({ + code: "KeyK", + key: "k", + metaKey: true, + ctrlKey: true, + altKey: true, + shiftKey: true, + }), + ); + expect(captured).toBe("meta+ctrl+alt+shift+k"); + }); +}); diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts index 24514045bd2..8b52d9e552d 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts @@ -2,74 +2,81 @@ import { useEffect, useRef } from "react"; import { HOTKEYS, type HotkeyId, PLATFORM } from "../../registry"; import { useHotkeyOverridesStore } from "../../stores/hotkeyOverridesStore"; import type { Platform } from "../../types"; - -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - +import { + canonicalizeChord, + isIgnorableKey, + normalizeToken, + TERMINAL_RESERVED_CHORDS, +} from "../../utils/resolveHotkeyFromEvent"; + +// Matches the registry's written modifier order (`meta+alt+up`) so recorded +// strings stay visually aligned with defaults. Canonicalization handles +// reordering at compare time. const MODIFIER_ORDER = ["meta", "ctrl", "alt", "shift"] as const; -function captureHotkeyFromEvent(event: KeyboardEvent): string | null { - const key = event.key.toLowerCase(); - if (["shift", "ctrl", "alt", "meta", "dead", "unidentified"].includes(key)) - return null; +export function captureHotkeyFromEvent(event: KeyboardEvent): string | null { + // event.code (not event.key) so Shift+2 records as `2`, Alt+L on Mac as + // `l`, and non-US layouts produce stable tokens matching the registry. + if (event.code === undefined) return null; + const key = normalizeToken(event.code); + if (isIgnorableKey(key)) return null; - // Must include ctrl or meta (or be F1-F12) const isFKey = /^f([1-9]|1[0-2])$/.test(key); if (!isFKey && !event.ctrlKey && !event.metaKey) return null; - // Reject meta on non-Mac - if (PLATFORM !== "mac" && event.metaKey) return null; + const modifiers = new Set(); + if (event.metaKey) modifiers.add("meta"); + if (event.ctrlKey) modifiers.add("ctrl"); + if (event.altKey) modifiers.add("alt"); + if (event.shiftKey) modifiers.add("shift"); - const modifiers: string[] = []; - if (event.metaKey) modifiers.push("meta"); - if (event.ctrlKey) modifiers.push("ctrl"); - if (event.altKey) modifiers.push("alt"); - if (event.shiftKey) modifiers.push("shift"); - - const ordered = MODIFIER_ORDER.filter((m) => modifiers.includes(m)); + const ordered = MODIFIER_ORDER.filter((m) => modifiers.has(m)); return [...ordered, key].join("+"); } -const TERMINAL_RESERVED = new Set([ - "ctrl+c", - "ctrl+d", - "ctrl+z", - "ctrl+s", - "ctrl+q", - "ctrl+\\", -]); - -const OS_RESERVED: Record = { - mac: ["meta+q", "meta+space", "meta+tab"], - windows: ["alt+f4", "alt+tab", "ctrl+alt+delete"], - linux: ["alt+f4", "alt+tab"], +// Chords the OS / shell is likely to intercept. Binding is allowed (Linux +// WM configs vary), but the recorder emits a warning so the user knows why +// a chord they just bound might not fire. Canonicalized at build time so +// multi-modifier entries (e.g. `ctrl+alt+delete` → `alt+ctrl+delete`) match. +const OS_RESERVED: Record> = { + mac: new Set(["meta+q", "meta+space", "meta+tab"].map(canonicalizeChord)), + windows: new Set( + [ + "alt+f4", + "alt+tab", + "ctrl+alt+delete", + "meta+d", // Show desktop + "meta+e", // Explorer + "meta+l", // Lock + "meta+r", // Run + "meta+tab", // Task view + ].map(canonicalizeChord), + ), + linux: new Set(["alt+f4", "alt+tab"].map(canonicalizeChord)), }; function checkReserved( keys: string, ): { reason: string; severity: "error" | "warning" } | null { - if (TERMINAL_RESERVED.has(keys)) + const canonical = canonicalizeChord(keys); + if (TERMINAL_RESERVED_CHORDS.has(canonical)) return { reason: "Reserved by terminal", severity: "error" }; - if (OS_RESERVED[PLATFORM].includes(keys)) + if (OS_RESERVED[PLATFORM].has(canonical)) return { reason: "Reserved by OS", severity: "warning" }; return null; } function getHotkeyConflict(keys: string, excludeId: HotkeyId): HotkeyId | null { const { overrides } = useHotkeyOverridesStore.getState(); + const canonicalKeys = canonicalizeChord(keys); for (const id of Object.keys(HOTKEYS) as HotkeyId[]) { if (id === excludeId) continue; const effective = id in overrides ? overrides[id] : HOTKEYS[id].key; - if (effective === keys) return id; + if (effective && canonicalizeChord(effective) === canonicalKeys) return id; } return null; } -// --------------------------------------------------------------------------- -// Hook -// --------------------------------------------------------------------------- - interface UseRecordHotkeysOptions { onSave?: (id: HotkeyId, keys: string) => void; onCancel?: () => void; @@ -129,7 +136,7 @@ export function useRecordHotkeys( } const defaultKey = HOTKEYS[recordingId].key; - if (captured === defaultKey) { + if (canonicalizeChord(captured) === canonicalizeChord(defaultKey)) { resetOverride(recordingId); } else { setOverride(recordingId, captured); diff --git a/apps/desktop/src/renderer/hotkeys/index.ts b/apps/desktop/src/renderer/hotkeys/index.ts index af4d7c87c28..9874b054093 100644 --- a/apps/desktop/src/renderer/hotkeys/index.ts +++ b/apps/desktop/src/renderer/hotkeys/index.ts @@ -10,4 +10,8 @@ export type { HotkeyDisplay, Platform, } from "./types"; -export { isTerminalReservedEvent, resolveHotkeyFromEvent } from "./utils"; +export { + isTerminalReservedEvent, + matchesChord, + resolveHotkeyFromEvent, +} from "./utils"; diff --git a/apps/desktop/src/renderer/hotkeys/migrate.ts b/apps/desktop/src/renderer/hotkeys/migrate.ts index 8712ce32ad7..72dbcd95a3f 100644 --- a/apps/desktop/src/renderer/hotkeys/migrate.ts +++ b/apps/desktop/src/renderer/hotkeys/migrate.ts @@ -2,11 +2,15 @@ * One-time migration from the old hotkey storage (main process JSON file via tRPC) * to the new localStorage-based Zustand store. * - * No-op if the new store key already exists (Zustand persist creates it on first init). + * Marker key is bumped (`-v2`) so users who migrated on the pre-sanitizer + * build re-run once and get their corrupt entries dropped. */ import { electronTrpcClient } from "renderer/lib/trpc-client"; import { PLATFORM } from "./registry"; +import { sanitizeOverride } from "./utils/sanitizeOverride"; + +const MIGRATION_MARKER_KEY = "hotkey-overrides-migrated-v2"; const PLATFORM_MAP = { mac: "darwin", @@ -15,28 +19,40 @@ const PLATFORM_MAP = { } as const; export async function migrateHotkeyOverrides(): Promise { - if (localStorage.getItem("hotkey-overrides")) { - console.log("[hotkeys] Migration skipped — new store already exists"); - return; - } + if (localStorage.getItem(MIGRATION_MARKER_KEY)) return; try { const oldState = await electronTrpcClient.uiState.hotkeys.get.query(); const oldPlatformKey = PLATFORM_MAP[PLATFORM]; const oldOverrides = oldState?.byPlatform?.[oldPlatformKey]; if (!oldOverrides || Object.keys(oldOverrides).length === 0) { + localStorage.setItem(MIGRATION_MARKER_KEY, "1"); console.log("[hotkeys] Migration skipped — no old overrides found"); return; } + const cleaned: Record = {}; + let dropped = 0; + for (const [id, raw] of Object.entries(oldOverrides)) { + const sanitized = sanitizeOverride(raw); + if (sanitized === undefined) { + dropped++; + continue; + } + cleaned[id] = sanitized; + } + localStorage.setItem( "hotkey-overrides", - JSON.stringify({ state: { overrides: oldOverrides }, version: 0 }), + JSON.stringify({ state: { overrides: cleaned }, version: 0 }), ); + localStorage.setItem(MIGRATION_MARKER_KEY, "1"); console.log( - `[hotkeys] Migrated ${Object.keys(oldOverrides).length} override(s)`, + `[hotkeys] Migrated ${Object.keys(cleaned).length} override(s)` + + (dropped > 0 ? `, dropped ${dropped} invalid` : ""), ); } catch (error) { - console.log("[hotkeys] Migration failed, starting fresh:", error); + // Marker intentionally not set — transient tRPC failures retry next boot. + console.log("[hotkeys] Migration failed, will retry next boot:", error); } } diff --git a/apps/desktop/src/renderer/hotkeys/utils/index.ts b/apps/desktop/src/renderer/hotkeys/utils/index.ts index fe7386d7999..00b72d7c88c 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/index.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/index.ts @@ -1,2 +1,5 @@ -export { resolveHotkeyFromEvent } from "./resolveHotkeyFromEvent"; +export { + matchesChord, + resolveHotkeyFromEvent, +} from "./resolveHotkeyFromEvent"; export { isTerminalReservedEvent } from "./utils"; diff --git a/apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts b/apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts new file mode 100644 index 00000000000..1ac907cc779 --- /dev/null +++ b/apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from "bun:test"; +import { sanitizeOverride } from "./sanitizeOverride"; + +describe("sanitizeOverride (migration validation)", () => { + it("preserves an explicit unassignment (null)", () => { + expect(sanitizeOverride(null)).toBeNull(); + }); + + it("drops empty / non-string values", () => { + expect(sanitizeOverride(undefined)).toBeUndefined(); + expect(sanitizeOverride("")).toBeUndefined(); + expect(sanitizeOverride(" ")).toBeUndefined(); + expect(sanitizeOverride(42)).toBeUndefined(); + expect(sanitizeOverride({})).toBeUndefined(); + }); + + it("canonicalizes valid chords", () => { + expect(sanitizeOverride("meta+k")).toBe("meta+k"); + expect(sanitizeOverride("shift+ctrl+k")).toBe("ctrl+shift+k"); + expect(sanitizeOverride("meta+alt+up")).toBe("alt+meta+arrowup"); + }); + + it("accepts multi-char key tokens (bracketleft, f12)", () => { + expect(sanitizeOverride("meta+bracketleft")).toBe("meta+bracketleft"); + expect(sanitizeOverride("f12")).toBe("f12"); + }); + + it("drops pre-fix `ctrl+control` garbage (no real key)", () => { + expect(sanitizeOverride("ctrl+control")).toBeUndefined(); + }); + + it("drops chords with single-char punctuation keys (pre-fix event.key output)", () => { + expect(sanitizeOverride("ctrl+shift+@")).toBeUndefined(); + expect(sanitizeOverride("meta+[")).toBeUndefined(); + expect(sanitizeOverride("alt+¬")).toBeUndefined(); + }); + + it("drops chords with only modifiers", () => { + expect(sanitizeOverride("ctrl+shift")).toBeUndefined(); + expect(sanitizeOverride("meta")).toBeUndefined(); + }); +}); diff --git a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts new file mode 100644 index 00000000000..e2336f26946 --- /dev/null +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts @@ -0,0 +1,383 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { HOTKEYS } from "../registry"; +import { useHotkeyOverridesStore } from "../stores/hotkeyOverridesStore"; +import { + canonicalizeChord, + eventToChord, + isIgnorableKey, + matchesChord, + normalizeToken, + resolveHotkeyFromEvent, + TERMINAL_RESERVED_CHORDS, +} from "./resolveHotkeyFromEvent"; +import { isTerminalReservedEvent } from "./utils"; + +// Minimal stub — the renderer references `navigator` only at import time. +// Bun's test runtime doesn't have a DOM navigator by default; registry.ts +// detects platform via `navigator.platform` and falls back to "mac" when +// navigator is undefined. We only assert platform-agnostic behavior here. + +describe("normalizeToken", () => { + it("maps code aliases to canonical names", () => { + expect(normalizeToken("ControlLeft")).toBe("ctrl"); + expect(normalizeToken("ControlRight")).toBe("ctrl"); + expect(normalizeToken("MetaLeft")).toBe("meta"); + expect(normalizeToken("ShiftRight")).toBe("shift"); + expect(normalizeToken("AltLeft")).toBe("alt"); + expect(normalizeToken("OSLeft")).toBe("meta"); + }); + + it("strips key/digit/numpad prefixes from event.code", () => { + expect(normalizeToken("KeyA")).toBe("a"); + expect(normalizeToken("KeyZ")).toBe("z"); + expect(normalizeToken("Digit1")).toBe("1"); + expect(normalizeToken("Digit0")).toBe("0"); + expect(normalizeToken("Numpad5")).toBe("5"); + }); + + it("lowercases physical key names and keeps punctuation tokens", () => { + expect(normalizeToken("BracketLeft")).toBe("bracketleft"); + expect(normalizeToken("BracketRight")).toBe("bracketright"); + expect(normalizeToken("Comma")).toBe("comma"); + expect(normalizeToken("Slash")).toBe("slash"); + expect(normalizeToken("Backslash")).toBe("backslash"); + expect(normalizeToken("Semicolon")).toBe("semicolon"); + }); + + it("aliases short arrow names to canonical", () => { + expect(normalizeToken("up")).toBe("arrowup"); + expect(normalizeToken("down")).toBe("arrowdown"); + expect(normalizeToken("left")).toBe("arrowleft"); + expect(normalizeToken("right")).toBe("arrowright"); + expect(normalizeToken("esc")).toBe("escape"); + expect(normalizeToken("return")).toBe("enter"); + }); + + it("canonicalizes arrow event.code to the same as short form", () => { + expect(normalizeToken("ArrowUp")).toBe("arrowup"); + expect(normalizeToken("ArrowDown")).toBe("arrowdown"); + }); +}); + +describe("isIgnorableKey", () => { + it("rejects empty normalized keys", () => { + expect(isIgnorableKey("")).toBe(true); + }); + + it("rejects every modifier alias", () => { + for (const m of ["meta", "ctrl", "control", "alt", "shift"]) { + expect(isIgnorableKey(m)).toBe(true); + } + }); + + it("rejects lock keys", () => { + expect(isIgnorableKey("capslock")).toBe(true); + expect(isIgnorableKey("numlock")).toBe(true); + expect(isIgnorableKey("scrolllock")).toBe(true); + }); + + it("allows regular letters, digits, and punctuation", () => { + expect(isIgnorableKey("a")).toBe(false); + expect(isIgnorableKey("1")).toBe(false); + expect(isIgnorableKey("bracketleft")).toBe(false); + expect(isIgnorableKey("arrowup")).toBe(false); + }); +}); + +describe("canonicalizeChord", () => { + it("sorts modifiers alphabetically and preserves the key", () => { + expect(canonicalizeChord("meta+alt+up")).toBe("alt+meta+arrowup"); + expect(canonicalizeChord("shift+ctrl+k")).toBe("ctrl+shift+k"); + }); + + it("treats `control` and `ctrl` as the same modifier", () => { + expect(canonicalizeChord("control+k")).toBe("ctrl+k"); + expect(canonicalizeChord("Control+K")).toBe("ctrl+k"); + }); + + it("normalizes key aliases across equivalent chord spellings", () => { + expect(canonicalizeChord("meta+alt+up")).toBe( + canonicalizeChord("alt+meta+arrowup"), + ); + expect(canonicalizeChord("ctrl+shift+bracketleft")).toBe( + canonicalizeChord("shift+ctrl+bracketleft"), + ); + }); + + it("is idempotent", () => { + const once = canonicalizeChord("meta+shift+l"); + expect(canonicalizeChord(once)).toBe(once); + }); + + // Regression: OS_RESERVED had `ctrl+alt+delete` written non-canonical, which + // meant the "Reserved by OS" warning never fired for that chord. Fix wraps + // the table in `.map(canonicalizeChord)`. Assert the canonical form here so + // future additions can't silently break the warning the same way. + it("sorts all modifiers alphabetically (ctrl+alt+delete → alt+ctrl+delete)", () => { + expect(canonicalizeChord("ctrl+alt+delete")).toBe("alt+ctrl+delete"); + }); +}); + +interface StubInit { + type?: string; + code?: string; + ctrlKey?: boolean; + metaKey?: boolean; + altKey?: boolean; + shiftKey?: boolean; +} +function ev(init: StubInit): KeyboardEvent { + return { + type: init.type ?? "keydown", + code: init.code ?? "", + key: "", + ctrlKey: !!init.ctrlKey, + metaKey: !!init.metaKey, + altKey: !!init.altKey, + shiftKey: !!init.shiftKey, + } as unknown as KeyboardEvent; +} + +describe("resolveHotkeyFromEvent — live override index", () => { + let originalOverrides: Record; + beforeEach(() => { + originalOverrides = useHotkeyOverridesStore.getState().overrides; + }); + afterEach(() => { + useHotkeyOverridesStore.setState({ overrides: originalOverrides }); + }); + + // Resolve once so registry reorders / removals surface as a test failure + // here instead of silently skipping the cases below. + const sampleEntry = ( + Object.entries(HOTKEYS) as [keyof typeof HOTKEYS, { key: string }][] + ).find(([, hotkey]) => !!hotkey.key); + if (!sampleEntry) throw new Error("HOTKEYS has no bound default"); + const [sampleId, sampleDef] = sampleEntry; + + it("resolves a default binding when no override is set", () => { + const event = buildEventFromChord(sampleDef.key); + expect(resolveHotkeyFromEvent(event)).toBe(sampleId); + }); + + it("resolves a rebound chord after an override is saved", () => { + useHotkeyOverridesStore.setState({ + overrides: { [sampleId]: "meta+shift+f10" }, + }); + const event = buildEventFromChord("meta+shift+f10"); + expect(resolveHotkeyFromEvent(event)).toBe(sampleId); + }); + + it("does NOT resolve the old default after the user rebinds away from it", () => { + useHotkeyOverridesStore.setState({ + overrides: { [sampleId]: "meta+shift+f10" }, + }); + const event = buildEventFromChord(sampleDef.key); + expect(resolveHotkeyFromEvent(event)).toBeNull(); + }); + + it("does NOT resolve a hotkey the user explicitly unassigned (null override)", () => { + useHotkeyOverridesStore.setState({ + overrides: { [sampleId]: null }, + }); + const event = buildEventFromChord(sampleDef.key); + expect(resolveHotkeyFromEvent(event)).toBeNull(); + }); +}); + +/** + * Turns a chord string (e.g. `meta+shift+f10`, `ctrl+bracketleft`) into a + * KeyboardEvent stub with matching `event.code` and modifier flags. + */ +function buildEventFromChord(chord: string): KeyboardEvent { + const parts = chord.toLowerCase().split("+"); + const mods = { + metaKey: parts.includes("meta"), + ctrlKey: parts.includes("ctrl") || parts.includes("control"), + altKey: parts.includes("alt"), + shiftKey: parts.includes("shift"), + }; + const key = parts.find( + (p) => !["meta", "ctrl", "control", "alt", "shift"].includes(p), + ); + const code = chordKeyToCode(key ?? ""); + return { + type: "keydown", + code, + key: "", + ...mods, + } as unknown as KeyboardEvent; +} + +// Inverse of normalizeToken for the tokens the registry uses. Only needs to +// cover what tests exercise. +function chordKeyToCode(key: string): string { + if (/^[a-z]$/.test(key)) return `Key${key.toUpperCase()}`; + if (/^[0-9]$/.test(key)) return `Digit${key}`; + if (/^f([1-9]|1[0-2])$/.test(key)) return key.toUpperCase(); + switch (key) { + case "arrowup": + case "up": + return "ArrowUp"; + case "arrowdown": + case "down": + return "ArrowDown"; + case "arrowleft": + case "left": + return "ArrowLeft"; + case "arrowright": + case "right": + return "ArrowRight"; + case "bracketleft": + return "BracketLeft"; + case "bracketright": + return "BracketRight"; + case "comma": + return "Comma"; + case "slash": + return "Slash"; + case "backslash": + return "Backslash"; + case "backspace": + return "Backspace"; + case "space": + return "Space"; + case "tab": + return "Tab"; + default: + return key; + } +} + +describe("resolveHotkeyFromEvent", () => { + it("returns null for non-keydown events", () => { + expect( + resolveHotkeyFromEvent( + ev({ type: "keyup", code: "KeyP", metaKey: true }), + ), + ).toBeNull(); + }); + + it("returns null for pure modifier presses", () => { + expect( + resolveHotkeyFromEvent(ev({ code: "ControlLeft", ctrlKey: true })), + ).toBeNull(); + }); + + it("returns null for unbound chords", () => { + expect( + resolveHotkeyFromEvent( + ev({ + code: "KeyZ", + ctrlKey: true, + shiftKey: true, + altKey: true, + metaKey: true, + }), + ), + ).toBeNull(); + }); +}); + +describe("eventToChord", () => { + it("normalizes punctuation via event.code", () => { + expect(eventToChord(ev({ code: "BracketLeft", ctrlKey: true }))).toBe( + "ctrl+bracketleft", + ); + expect(eventToChord(ev({ code: "Slash", metaKey: true }))).toBe( + "meta+slash", + ); + }); + + it("returns null for pure modifiers and lock keys", () => { + expect(eventToChord(ev({ code: "ControlLeft", ctrlKey: true }))).toBeNull(); + expect(eventToChord(ev({ code: "CapsLock" }))).toBeNull(); + }); +}); + +describe("matchesChord", () => { + it("matches events regardless of modifier order in the chord string", () => { + const event = ev({ code: "KeyK", ctrlKey: true, shiftKey: true }); + expect(matchesChord(event, "ctrl+shift+k")).toBe(true); + expect(matchesChord(event, "shift+ctrl+k")).toBe(true); + }); + + it("matches short vs canonical arrow forms", () => { + const event = ev({ code: "ArrowUp", metaKey: true, altKey: true }); + expect(matchesChord(event, "meta+alt+up")).toBe(true); + expect(matchesChord(event, "alt+meta+arrowup")).toBe(true); + }); + + it("matches punctuation rebinds via event.code (bracket, backslash)", () => { + expect( + matchesChord( + ev({ code: "BracketLeft", ctrlKey: true, shiftKey: true }), + "ctrl+shift+bracketleft", + ), + ).toBe(true); + expect( + matchesChord(ev({ code: "Backslash", ctrlKey: true }), "ctrl+backslash"), + ).toBe(true); + }); + + it("does NOT match when key differs", () => { + expect(matchesChord(ev({ code: "KeyK", ctrlKey: true }), "ctrl+j")).toBe( + false, + ); + }); + + it("does NOT match when modifiers differ", () => { + expect(matchesChord(ev({ code: "KeyK", ctrlKey: true }), "meta+k")).toBe( + false, + ); + }); + + it("does NOT match a bare modifier press", () => { + expect( + matchesChord(ev({ code: "ControlLeft", ctrlKey: true }), "ctrl+k"), + ).toBe(false); + }); +}); + +describe("isTerminalReservedEvent", () => { + it.each([ + "ctrl+c", + "ctrl+d", + "ctrl+z", + "ctrl+s", + "ctrl+q", + "ctrl+backslash", + ])("detects %s", (chord) => { + const codeMap: Record = { + "ctrl+c": "KeyC", + "ctrl+d": "KeyD", + "ctrl+z": "KeyZ", + "ctrl+s": "KeyS", + "ctrl+q": "KeyQ", + "ctrl+backslash": "Backslash", + }; + expect( + isTerminalReservedEvent(ev({ code: codeMap[chord], ctrlKey: true })), + ).toBe(true); + }); + + it("ignores non-reserved ctrl chords", () => { + expect(isTerminalReservedEvent(ev({ code: "KeyK", ctrlKey: true }))).toBe( + false, + ); + }); + + it("ignores reserved letter when extra modifier is held", () => { + expect( + isTerminalReservedEvent( + ev({ code: "KeyC", ctrlKey: true, shiftKey: true }), + ), + ).toBe(false); + }); + + it("TERMINAL_RESERVED_CHORDS stays in canonical form", () => { + for (const chord of TERMINAL_RESERVED_CHORDS) { + expect(canonicalizeChord(chord)).toBe(chord); + } + }); +}); diff --git a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts index 880f9fb1c23..3cf565379d5 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts @@ -1,16 +1,17 @@ import { HOTKEYS, type HotkeyId } from "../registry"; +import { useHotkeyOverridesStore } from "../stores/hotkeyOverridesStore"; /** - * Resolves a KeyboardEvent to a registered {@link HotkeyId}, or `null` if the - * chord is not bound. Uses the same `event.code` normalization as - * react-hotkeys-hook (its internal `K` function) so the reverse index - * cannot drift from the matcher. + * KeyboardEvent → registered {@link HotkeyId}, or `null` if unbound. Uses the + * same `event.code` normalization as react-hotkeys-hook so the reverse index + * can't drift from the matcher. Index reflects current overrides, not frozen + * defaults — see {@link registeredAppChords}. */ export function resolveHotkeyFromEvent(event: KeyboardEvent): HotkeyId | null { if (event.type !== "keydown") return null; const chord = eventToChord(event); if (!chord) return null; - return REGISTERED_APP_CHORDS.get(chord) ?? null; + return registeredAppChords.get(chord) ?? null; } // Mirrors react-hotkeys-hook's alias table (react-hotkeys-hook/dist/index.js:3-19) @@ -33,14 +34,25 @@ const CODE_ALIASES: Record = { ControlRight: "ctrl", }; -const MODIFIERS = new Set(["meta", "ctrl", "control", "alt", "shift"]); +export const MODIFIERS = new Set(["meta", "ctrl", "control", "alt", "shift"]); -function normalizeToken(token: string): string { +// Lock keys must never commit a binding on their own. +const LOCK_KEYS = new Set(["capslock", "numlock", "scrolllock"]); + +export function normalizeToken(token: string): string { const aliased = CODE_ALIASES[token.trim()] ?? token.trim(); return aliased.toLowerCase().replace(/key|digit|numpad/, ""); } -function normalizeChord(chord: string): string { +export function isIgnorableKey(normalized: string): boolean { + return !normalized || MODIFIERS.has(normalized) || LOCK_KEYS.has(normalized); +} + +/** + * Stable form for comparing chord strings. Tolerates modifier order and + * aliases: `meta+alt+up` ≡ `alt+meta+arrowup` ≡ `control+alt+arrowup`. + */ +export function canonicalizeChord(chord: string): string { const parts = chord.toLowerCase().split("+").map(normalizeToken); const mods: string[] = []; const keys: string[] = []; @@ -55,9 +67,11 @@ function normalizeChord(chord: string): string { return [...mods, ...keys].join("+"); } -function eventToChord(event: KeyboardEvent): string | null { +/** KeyboardEvent → canonical chord (comparable to {@link canonicalizeChord} output), or null for pure modifier / synthetic presses. */ +export function eventToChord(event: KeyboardEvent): string | null { + if (event.code === undefined) return null; const key = normalizeToken(event.code); - if (!key || MODIFIERS.has(key)) return null; + if (isIgnorableKey(key)) return null; const mods: string[] = []; if (event.metaKey) mods.push("meta"); if (event.ctrlKey) mods.push("ctrl"); @@ -67,9 +81,42 @@ function eventToChord(event: KeyboardEvent): string | null { return [...mods, key].join("+"); } -const REGISTERED_APP_CHORDS: Map = new Map( - (Object.keys(HOTKEYS) as HotkeyId[]).map((id) => [ - normalizeChord(HOTKEYS[id].key), - id, - ]), +/** True if `event` produces `chord` (tolerating modifier order / aliases). */ +export function matchesChord(event: KeyboardEvent, chord: string): boolean { + const eventChord = eventToChord(event); + if (!eventChord) return false; + return eventChord === canonicalizeChord(chord); +} + +/** Sent straight to the PTY. Canonicalized at build time so lookups via `eventToChord` / `canonicalizeChord` match directly. */ +export const TERMINAL_RESERVED_CHORDS = new Set( + ["ctrl+c", "ctrl+d", "ctrl+z", "ctrl+s", "ctrl+q", "ctrl+backslash"].map( + canonicalizeChord, + ), +); + +function buildRegisteredAppChords( + overrides: Record, +): Map { + const map = new Map(); + for (const id of Object.keys(HOTKEYS) as HotkeyId[]) { + const hasOverride = id in overrides; + const override = hasOverride ? overrides[id] : undefined; + // Explicit unassignment (null override) must drop from the index — else + // the terminal's isAppHotkey check would swallow the freed chord. + if (hasOverride && override === null) continue; + const keys = override ?? HOTKEYS[id].key; + if (!keys) continue; + map.set(canonicalizeChord(keys), id); + } + return map; +} + +// Reassigned on each override-store change; `let` is required so the +// subscribe callback can replace the reference the resolver reads. +let registeredAppChords = buildRegisteredAppChords( + useHotkeyOverridesStore.getState().overrides, ); +useHotkeyOverridesStore.subscribe((state) => { + registeredAppChords = buildRegisteredAppChords(state.overrides); +}); diff --git a/apps/desktop/src/renderer/hotkeys/utils/sanitizeOverride.ts b/apps/desktop/src/renderer/hotkeys/utils/sanitizeOverride.ts new file mode 100644 index 00000000000..217ee469c38 --- /dev/null +++ b/apps/desktop/src/renderer/hotkeys/utils/sanitizeOverride.ts @@ -0,0 +1,19 @@ +import { canonicalizeChord, MODIFIERS } from "./resolveHotkeyFromEvent"; + +/** + * Validates a migrated override string. Drops pre-fix garbage + * (`ctrl+control`, `ctrl+shift+@`, `meta+[`) that the old recorder could + * produce and that would never match `event.code`-based dispatch. + * + * - Returns the canonicalized chord on success. + * - Returns `null` to preserve an explicit unassignment. + * - Returns `undefined` to signal the caller should drop the entry. + */ +export function sanitizeOverride(value: unknown): string | null | undefined { + if (value === null) return null; + if (typeof value !== "string" || !value.trim()) return undefined; + const canonical = canonicalizeChord(value); + const keys = canonical.split("+").filter((p) => !MODIFIERS.has(p)); + if (keys.length !== 1 || !/^[a-z0-9]+$/.test(keys[0])) return undefined; + return canonical; +} diff --git a/apps/desktop/src/renderer/hotkeys/utils/utils.ts b/apps/desktop/src/renderer/hotkeys/utils/utils.ts index effddc7c255..dd5628a3478 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/utils.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/utils.ts @@ -1,16 +1,11 @@ -/** Check if a KeyboardEvent matches a terminal-reserved chord */ -const TERMINAL_RESERVED = new Set([ - "ctrl+c", - "ctrl+d", - "ctrl+z", - "ctrl+s", - "ctrl+q", - "ctrl+\\", -]); +import { + eventToChord, + TERMINAL_RESERVED_CHORDS, +} from "./resolveHotkeyFromEvent"; +/** True if the event matches a chord the terminal must always receive. */ export function isTerminalReservedEvent(event: KeyboardEvent): boolean { - if (!event.ctrlKey || event.metaKey || event.altKey || event.shiftKey) - return false; - const key = event.key.toLowerCase(); - return TERMINAL_RESERVED.has(`ctrl+${key}`); + const chord = eventToChord(event); + if (!chord) return false; + return TERMINAL_RESERVED_CHORDS.has(chord); } diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts index 5abc578d44d..a89141003ec 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts @@ -11,6 +11,7 @@ import { Terminal as XTerm } from "@xterm/xterm"; import { getBinding, isTerminalReservedEvent, + matchesChord, resolveHotkeyFromEvent, } from "renderer/hotkeys"; import type { DetectedLink } from "renderer/lib/terminal/links"; @@ -438,17 +439,6 @@ export function setupPasteHandler( * * Returns a cleanup function to remove the handler. */ -function matchesKey(event: KeyboardEvent, keys: string): boolean { - const parts = keys.toLowerCase().split("+"); - const modifiers = new Set(parts.slice(0, -1)); - const key = parts[parts.length - 1]; - if (modifiers.has("meta") !== event.metaKey) return false; - if (modifiers.has("ctrl") !== event.ctrlKey) return false; - if (modifiers.has("alt") !== event.altKey) return false; - if (modifiers.has("shift") !== event.shiftKey) return false; - return event.key.toLowerCase() === key; -} - export function setupKeyboardHandler( xterm: XTerm, options: KeyboardHandlerOptions = {}, @@ -589,7 +579,7 @@ export function setupKeyboardHandler( // CLEAR_TERMINAL is handled here (xterm needs to call onClear) const clearKeys = getBinding("CLEAR_TERMINAL"); - if (clearKeys && matchesKey(event, clearKeys)) { + if (clearKeys && matchesChord(event, clearKeys)) { if (event.type === "keydown" && options.onClear) { options.onClear(); }