From ff99fdfdddfb9b5afcc347f1057341be2b208bc2 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 17:13:48 -0700 Subject: [PATCH 01/10] attempt fix --- apps/desktop/src/renderer/hotkeys/display.ts | 29 ++++++++++--- .../useRecordHotkeys/useRecordHotkeys.ts | 42 ++++++++++++------- .../hotkeys/utils/resolveHotkeyFromEvent.ts | 21 ++++++++-- 3 files changed, 70 insertions(+), 22 deletions(-) diff --git a/apps/desktop/src/renderer/hotkeys/display.ts b/apps/desktop/src/renderer/hotkeys/display.ts index 4eff9da0fdb..e5c40bd4346 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,6 +12,9 @@ const MODIFIER_DISPLAY: Record> = { linux: { meta: "Super", ctrl: "Ctrl", alt: "Alt", shift: "Shift" }, }; +// Keyed by canonical (event.code-normalized) tokens. Accepts both short and +// canonical arrow names so legacy registry strings (`up`) and recorder output +// (`arrowup`) both render correctly. const KEY_DISPLAY: Record = { enter: "↵", backspace: "⌫", @@ -21,9 +25,20 @@ const KEY_DISPLAY: Record = { down: "↓", left: "←", right: "→", + arrowup: "↑", + arrowdown: "↓", + arrowleft: "←", + arrowright: "→", space: "␣", slash: "/", + backslash: "\\", comma: ",", + period: ".", + semicolon: ";", + quote: "'", + backquote: "`", + minus: "-", + equal: "=", bracketleft: "[", bracketright: "]", }; @@ -39,12 +54,16 @@ export function formatHotkeyDisplay( 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 parts = keys.toLowerCase().split("+").map(normalizeToken); + const modifiers = parts + .map((p) => (p === "control" ? "ctrl" : p)) + .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]), + (p) => + p !== "control" && + !MODIFIER_ORDER.includes(p as (typeof MODIFIER_ORDER)[number]), ); if (!key) return { keys: ["Unassigned"], text: "Unassigned" }; diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts index 24514045bd2..ddadbfafb05 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts @@ -2,17 +2,27 @@ import { useEffect, useRef } from "react"; import { HOTKEYS, type HotkeyId, PLATFORM } from "../../registry"; import { useHotkeyOverridesStore } from "../../stores/hotkeyOverridesStore"; import type { Platform } from "../../types"; +import { + canonicalizeChord, + isIgnorableKey, + normalizeToken, +} from "../../utils/resolveHotkeyFromEvent"; // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- +// Matches the registry's modifier order (e.g. `meta+shift+l`, `meta+alt+up`) +// so recorded overrides are string-comparable with HOTKEYS[id].key. 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; + // Normalize via event.code (matches the library's matcher and registry tokens + // like `bracketleft`, `comma`, `slash`). Using event.key would diverge for + // Shift+digit (`@` vs `2`), Alt+letter on Mac (`¬` vs `l`), and non-US layouts. + 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); @@ -21,23 +31,25 @@ function captureHotkeyFromEvent(event: KeyboardEvent): string | null { // Reject meta on non-Mac if (PLATFORM !== "mac" && event.metaKey) return null; - 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 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 ordered = MODIFIER_ORDER.filter((m) => modifiers.includes(m)); + const ordered = MODIFIER_ORDER.filter((m) => modifiers.has(m)); return [...ordered, key].join("+"); } +// Reserved chords are matched after canonicalization, so use event.code-based +// tokens (e.g. `backslash`, not `\\`). const TERMINAL_RESERVED = new Set([ "ctrl+c", "ctrl+d", "ctrl+z", "ctrl+s", "ctrl+q", - "ctrl+\\", + "ctrl+backslash", ]); const OS_RESERVED: Record = { @@ -49,19 +61,21 @@ const OS_RESERVED: Record = { function checkReserved( keys: string, ): { reason: string; severity: "error" | "warning" } | null { - if (TERMINAL_RESERVED.has(keys)) + const canonical = canonicalizeChord(keys); + if (TERMINAL_RESERVED.has(canonical)) return { reason: "Reserved by terminal", severity: "error" }; - if (OS_RESERVED[PLATFORM].includes(keys)) + if (OS_RESERVED[PLATFORM].includes(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; } @@ -129,7 +143,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/utils/resolveHotkeyFromEvent.ts b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts index 880f9fb1c23..a26289baa25 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts @@ -33,13 +33,20 @@ 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 should 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/, ""); } +export function isIgnorableKey(normalized: string): boolean { + return !normalized || MODIFIERS.has(normalized) || LOCK_KEYS.has(normalized); +} + function normalizeChord(chord: string): string { const parts = chord.toLowerCase().split("+").map(normalizeToken); const mods: string[] = []; @@ -55,9 +62,17 @@ function normalizeChord(chord: string): string { return [...mods, ...keys].join("+"); } +// Canonical form for string comparison between recorded overrides and registry +// defaults. Tolerates differences in modifier order and token aliases (e.g. +// `meta+alt+up` vs `alt+meta+arrowup`, or `control` vs `ctrl`). +export function canonicalizeChord(chord: string): string { + return normalizeChord(chord); +} + 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"); From 1eeb15ff65c4516abd8ebe1f1d6e57c1d6715fb9 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 17:18:41 -0700 Subject: [PATCH 02/10] Test and doc --- ...0412-keyboard-recorder-ctrl-binding-fix.md | 355 ++++++++++++++++++ .../src/renderer/hotkeys/display.test.ts | 45 +++ .../useRecordHotkeys/useRecordHotkeys.test.ts | 145 +++++++ .../useRecordHotkeys/useRecordHotkeys.ts | 2 +- .../utils/resolveHotkeyFromEvent.test.ts | 153 ++++++++ 5 files changed, 699 insertions(+), 1 deletion(-) create mode 100644 apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md create mode 100644 apps/desktop/src/renderer/hotkeys/display.test.ts create mode 100644 apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts create mode 100644 apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts 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..ec468c27901 --- /dev/null +++ b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md @@ -0,0 +1,355 @@ +# Keyboard Recorder — Ctrl Binding & event.code Unification + +**Date:** 2026-04-12 +**Scope:** `apps/desktop/src/renderer/hotkeys/*` +**Status:** shipped + +## Problem + +User reported that the Settings → Keyboard recorder would not allow binding +chords that begin with Ctrl. Investigation found three compounding bugs in the +recorder and its comparison/display logic. + +--- + +## Bug 1 — Ctrl press auto-committed an invalid binding + +### Before + +`useRecordHotkeys.ts:13-15` filtered pure-modifier keydowns using lowercased +`event.key`: + +```ts +const key = event.key.toLowerCase(); +if (["shift", "ctrl", "alt", "meta", "dead", "unidentified"].includes(key)) + return null; +``` + +### Why it broke + +`KeyboardEvent.key` for the Control key is the string `"Control"`, not `"Ctrl"`. +Lowercased, that's `"control"` — which does **not** match `"ctrl"` in the +ignore list. Result: pressing Ctrl alone (the normal first half of any Ctrl +chord) passed the filter, `event.ctrlKey` was true, and +`captureHotkeyFromEvent` immediately returned and saved `ctrl+control` as the +binding before the user could press the second key. + +Shift/Alt/Meta worked because their `event.key` values lowercase to `"shift"`, +`"alt"`, `"meta"` — matching the filter. + +### Source citation + +[MDN KeyboardEvent.key values](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values#modifier_keys) +specifies: + +> `"Control"` — The Control (Ctrl) key. + +The repo already aliases `ControlLeft`/`ControlRight` → `ctrl` in +`resolveHotkeyFromEvent.ts` for `event.code` input, which hid the mismatch — it +only surfaces when someone uses `event.key`. + +### Fix + +Filter against the actual lowercased `event.key` value, plus other modifier-ish +keys that should never commit a binding on their own. + +```ts +// apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts +export const MODIFIERS = new Set(["meta", "ctrl", "control", "alt", "shift"]); +const LOCK_KEYS = new Set(["capslock", "numlock", "scrolllock"]); + +export function isIgnorableKey(normalized: string): boolean { + return !normalized || MODIFIERS.has(normalized) || LOCK_KEYS.has(normalized); +} +``` + +Also added `altgraph` (AltGr on European keyboards) coverage via the broader +rewrite below. + +--- + +## Bug 2 — Recorder and resolver used different key sources + +### Before + +- **Resolver** (`resolveHotkeyFromEvent.ts:59`, `eventToChord`): used + `event.code`, normalized via `CODE_ALIASES` (the same alias table as + `react-hotkeys-hook`). +- **Recorder** (`useRecordHotkeys.ts:13`): used `event.key`. +- **Registry defaults** (`registry.ts`): written in `event.code` form + (`bracketleft`, `comma`, `slash`, `backspace`, etc.). + +### Why it broke + +`event.key` depends on layout and other held modifiers; `event.code` is a +stable physical-key identifier. They diverge in common cases: + +| Chord pressed | Recorder saved (`event.key`) | Registry/library form (`event.code`) | Match? | +| ----------------- | ---------------------------- | ------------------------------------ | ------ | +| Ctrl+Shift+2 | `ctrl+shift+@` | `ctrl+shift+2` | ❌ | +| Alt+L on Mac | `alt+¬` | `alt+l` | ❌ | +| Ctrl+/ on DE kbd | `ctrl+-` | `ctrl+slash` | ❌ | +| Meta+[ | `meta+[` | `meta+bracketleft` | ❌ | + +The saved override string was fed into `useHotkeys(keys, …)`, which internally +re-parses via `mapCode`. Since `mapCode` is code-aware, an override string like +`ctrl+shift+@` would never match the `event.code` `"Digit2"`. + +### Source citation + +Upstream `react-hotkeys-hook` uses `event.code` by default +([`packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts`](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts)): + +```ts +// react-hotkeys-hook/packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts +const handler = useCallback( + (event: KeyboardEvent) => { + if (event.code === undefined) { + // Synthetic event (e.g., Chrome autofill). Ignore. + return + } + event.preventDefault() + event.stopPropagation() + setKeys((prev) => { + const newKeys = new Set(prev) + newKeys.add(mapCode(useKey ? event.key : event.code)) + return newKeys + }) + }, + [useKey], +) +``` + +And `parseHotkeys.ts` aliases `event.code` values like `ControlLeft` → `ctrl`, +`ShiftLeft` → `shift`, and strips `/key|digit|numpad/` so `KeyA` → `a`, +`Digit1` → `1`: + +```ts +// react-hotkeys-hook/packages/react-hotkeys-hook/src/lib/parseHotkeys.ts +const mappedKeys: Record = { + esc: 'escape', return: 'enter', + left: 'arrowleft', right: 'arrowright', up: 'arrowup', down: 'arrowdown', + ShiftLeft: 'shift', ShiftRight: 'shift', + AltLeft: 'alt', AltRight: 'alt', + MetaLeft: 'meta', MetaRight: 'meta', + OSLeft: 'meta', OSRight: 'meta', + ControlLeft: 'ctrl', ControlRight: 'ctrl', +} + +export function mapCode(key: string): string { + return (mappedKeys[key.trim()] || key.trim()).toLowerCase().replace(/key|digit|numpad/, '') +} +``` + +### Fix + +Export the existing `normalizeToken` (and `isIgnorableKey`) from +`resolveHotkeyFromEvent.ts` and reuse it in the recorder so both sides speak +the same language: + +```ts +// apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +import { + canonicalizeChord, + isIgnorableKey, + normalizeToken, +} from "../../utils/resolveHotkeyFromEvent"; + +function captureHotkeyFromEvent(event: KeyboardEvent): string | null { + if (event.code === undefined) return null; // matches upstream guard + const key = normalizeToken(event.code); + if (isIgnorableKey(key)) return null; + + const isFKey = /^f([1-9]|1[0-2])$/.test(key); + if (!isFKey && !event.ctrlKey && !event.metaKey) return null; + 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 ordered = MODIFIER_ORDER.filter((m) => modifiers.has(m)); + return [...ordered, key].join("+"); +} +``` + +--- + +## Bug 3 — String comparisons didn't canonicalize + +### Before + +```ts +// useRecordHotkeys.ts (before) +if (TERMINAL_RESERVED.has(keys)) // raw string lookup +if (OS_RESERVED[PLATFORM].includes(keys)) // raw string lookup +if (effective === keys) return id; // raw string comparison (conflict) +if (captured === defaultKey) resetOverride();// raw string comparison (reset-to-default) +``` + +### Why it broke + +The recorder produces strings in `MODIFIER_ORDER` (`meta+ctrl+alt+shift`), the +registry author orders them however reads naturally (`meta+alt+up`), and the +resolver sorts alphabetically (`alt+meta+arrowup`). All three forms mean the +same chord but fail `===` comparison. + +Examples that silently misbehaved: + +- User rebinds `PREV_WORKSPACE` to its own default (`meta+alt+up`). Recorder + saves `meta+alt+arrowup`; `captured === defaultKey` is false so we write an + override instead of resetting. +- User tries to bind `meta+alt+up` while `NEXT_WORKSPACE` already has + `meta+alt+down` — conflict check runs string `===` between canonical-ish + recorder output and registry-authored defaults, producing false negatives on + overlapping overrides. + +### Fix + +Introduce a single canonical form and use it for all comparisons. + +```ts +// apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts +export function canonicalizeChord(chord: string): string { + // sorts modifiers, normalizes control→ctrl, up→arrowup, bracketleft stays, etc. + return normalizeChord(chord); +} +``` + +Applied at the three comparison sites in `useRecordHotkeys.ts`: + +```ts +function checkReserved(keys: string) { + const canonical = canonicalizeChord(keys); + if (TERMINAL_RESERVED.has(canonical)) return { reason: "Reserved by terminal", severity: "error" }; + if (OS_RESERVED[PLATFORM].includes(canonical)) return { reason: "Reserved by OS", severity: "warning" }; + return null; +} + +function getHotkeyConflict(keys: string, excludeId: HotkeyId) { + 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 && canonicalizeChord(effective) === canonicalKeys) return id; + } + return null; +} + +// reset-to-default detection +if (canonicalizeChord(captured) === canonicalizeChord(defaultKey)) { + resetOverride(recordingId); +} else { + setOverride(recordingId, captured); +} +``` + +`TERMINAL_RESERVED` had `"ctrl+\\"` — canonicalization leaves the backslash +alone, but the recorder produces `ctrl+backslash` from `event.code === "Backslash"`. +Changed the constant to `"ctrl+backslash"` so the set membership check matches +what the recorder emits. + +--- + +## Display parity (`display.ts`) + +The display layer rendered symbols from a static table keyed on short names +(`up`, `comma`) and never normalized input. Overrides stored in canonical form +(`arrowup`) would render as `"ARROWUP"`. Fix: + +```ts +// apps/desktop/src/renderer/hotkeys/display.ts +import { normalizeToken } from "./utils/resolveHotkeyFromEvent"; + +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: "]", +}; + +export function formatHotkeyDisplay(keys: string | null, platform: Platform): HotkeyDisplay { + if (!keys) return { keys: ["Unassigned"], text: "Unassigned" }; + const parts = keys.toLowerCase().split("+").map(normalizeToken); + // …rest unchanged; uses KEY_DISPLAY[key] ?? key.toUpperCase() +} +``` + +--- + +## Decisions deliberately not taken + +### `mod` modifier alias + +Upstream `react-hotkeys-hook` supports `mod` as "meta on macOS, ctrl elsewhere" +([`parseHotkeys.ts` reserved list](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/parseHotkeys.ts)): + +```ts +const reservedModifierKeywords = ['shift', 'alt', 'meta', 'mod', 'ctrl', 'control'] +``` + +Our registry already stores per-platform bindings: + +```ts +// apps/desktop/src/renderer/hotkeys/registry.ts +QUICK_OPEN: { + key: { mac: "meta+p", windows: "ctrl+shift+p", linux: "ctrl+shift+p" }, + ... +} +``` + +Adding `mod` would duplicate that capability without simplifying existing +definitions. Skipped. + +--- + +## Testability + +Everything the fix touches is now isolated in **pure functions** that take +primitives and return primitives. All three bugs are testable without React or +a DOM: + +| Unit | Pure? | Inputs | Test harness | +| --------------------------------------------------- | ----- | ---------------------- | -------------------- | +| `normalizeToken(event.code \| alias)` | ✅ | string | `bun:test` direct | +| `isIgnorableKey(normalized)` | ✅ | string | `bun:test` direct | +| `canonicalizeChord(chord)` | ✅ | string | `bun:test` direct | +| `eventToChord(event)` / `resolveHotkeyFromEvent` | ✅ | `KeyboardEventInit` | `new KeyboardEvent` | +| `captureHotkeyFromEvent(event)` | ✅\* | `KeyboardEventInit` | `new KeyboardEvent` | +| `formatHotkeyDisplay(keys, platform)` | ✅ | string, `Platform` | `bun:test` direct | + +\* `captureHotkeyFromEvent` references `PLATFORM` (imported from `registry.ts`), +which is computed once from `navigator.platform`. To test non-Mac branches, +either export a `_captureHotkeyFromEventWithPlatform(event, platform)` variant +or module-mock `PLATFORM`. For the initial test pass we cover the current host +platform and construct events for the paths that don't depend on `PLATFORM`. + +The hook integration (`useRecordHotkeys`) is better covered with a smoke test +using `@testing-library/react` + `userEvent.keyboard`, but the pure helpers +cover all three bug classes deterministically. + +### Test file + +See co-located tests: +- `apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts` +- `apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts` +- `apps/desktop/src/renderer/hotkeys/display.test.ts` + +--- + +## Sources + +- [react-hotkeys-hook — GitHub](https://github.com/JohannesKlauss/react-hotkeys-hook) +- [`parseHotkeys.ts` (main)](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/parseHotkeys.ts) +- [`useRecordHotkeys.ts` (main)](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts) +- [MDN — KeyboardEvent.key values](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values) +- [MDN — KeyboardEvent.code values](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_code_values) +- VSCode's terminal hotkey forwarding pattern (referenced in + `apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:19-22` comment + citing `terminalInstance.ts:1116-1175`) 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/hooks/useRecordHotkeys/useRecordHotkeys.test.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts new file mode 100644 index 00000000000..bb5c62f0d85 --- /dev/null +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts @@ -0,0 +1,145 @@ +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", + code: init.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 ddadbfafb05..d09b665218a 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts @@ -16,7 +16,7 @@ import { // so recorded overrides are string-comparable with HOTKEYS[id].key. const MODIFIER_ORDER = ["meta", "ctrl", "alt", "shift"] as const; -function captureHotkeyFromEvent(event: KeyboardEvent): string | null { +export function captureHotkeyFromEvent(event: KeyboardEvent): string | null { // Normalize via event.code (matches the library's matcher and registry tokens // like `bracketleft`, `comma`, `slash`). Using event.key would diverge for // Shift+digit (`@` vs `2`), Alt+letter on Mac (`¬` vs `l`), and non-US layouts. 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..b0abe8a8b9e --- /dev/null +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts @@ -0,0 +1,153 @@ +import { describe, expect, it } from "bun:test"; +import { + canonicalizeChord, + isIgnorableKey, + normalizeToken, + resolveHotkeyFromEvent, +} from "./resolveHotkeyFromEvent"; + +// 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); + }); +}); + +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", () => { + 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(); + }); +}); From 56f3fe176a13380df9602422e1d6958357a9f462 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 17:32:09 -0700 Subject: [PATCH 03/10] fix(desktop): terminal & migration respect hotkey overrides - Rebuild the hotkey reverse index on override store changes so the terminal forwards the user's current bindings instead of frozen defaults. Fixes swallowed-keystroke on rebound-away defaults and dead-binding on new overrides. - Sanitize migrated overrides: canonicalize and drop malformed strings (`ctrl+control`, `ctrl+shift+@`, `meta+[`) that the pre-fix recorder could produce. - Document the meta-on-non-Mac policy (Windows OS intercept, Linux WM ownership). --- ...0412-keyboard-recorder-ctrl-binding-fix.md | 58 ++++++++- .../useRecordHotkeys/useRecordHotkeys.test.ts | 8 +- .../useRecordHotkeys/useRecordHotkeys.ts | 7 +- apps/desktop/src/renderer/hotkeys/migrate.ts | 35 ++++- .../hotkeys/utils/overrideSanitizer.test.ts | 59 +++++++++ .../utils/resolveHotkeyFromEvent.test.ts | 120 +++++++++++++++++- .../hotkeys/utils/resolveHotkeyFromEvent.ts | 37 +++++- 7 files changed, 304 insertions(+), 20 deletions(-) create mode 100644 apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts diff --git a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md index ec468c27901..81f359b2b60 100644 --- a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md +++ b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md @@ -283,6 +283,42 @@ export function formatHotkeyDisplay(keys: string | null, platform: Platform): Ho --- +## Follow-up bugs found during review + +### Bug 4 — Terminal swallowed hotkeys using stale defaults + +`resolveHotkeyFromEvent.ts` built its reverse index once at module load from +`HOTKEYS` defaults, ignoring user overrides. `terminal-runtime.ts:154` passes +`(event) => !isAppHotkey(event)` to xterm's `attachCustomKeyEventHandler`, so +the terminal's swallow/forward decision used frozen defaults. + +Failure modes after a user rebinds a hotkey from `meta+l` → `meta+y`: + +| In-terminal keystroke | Old behavior | +| ----------------------- | --------------------------------------------------- | +| `meta+y` (new binding) | not in map → xterm consumes → binding dead | +| `meta+l` (freed up) | still in map → xterm bails → bubbles → nothing fires → keystroke eaten | + +**Fix**: the reverse index is now a `let` rebound on every +`useHotkeyOverridesStore.subscribe` callback. Null overrides (explicit +unassignment) are dropped from the index so the terminal does not swallow +them. Tests in `resolveHotkeyFromEvent.test.ts` cover rebind, old-default +gone, and unassigned cases. + +### Bug 5 — Migration carried forward corrupt pre-fix overrides + +`migrate.ts` copied old overrides verbatim from the main-process tRPC store +into the new localStorage store. Any user who hit the pre-fix recorder could +have saved junk like `ctrl+control`, `ctrl+shift+@`, or `meta+[`, and the +migration would keep those broken strings around forever. + +**Fix**: `migrate.ts` now runs each migrated value through a sanitizer that +canonicalizes the chord and requires exactly one word-char key token. Invalid +entries are dropped with a count logged. `null` (explicit unassignment) is +preserved. Tests in `overrideSanitizer.test.ts`. + +--- + ## Decisions deliberately not taken ### `mod` modifier alias @@ -307,6 +343,21 @@ QUICK_OPEN: { Adding `mod` would duplicate that capability without simplifying existing definitions. Skipped. +### Allowing meta (Win/Super) recording on non-Mac + +The recorder rejects `event.metaKey` on Windows/Linux. We kept this and added +an explanatory comment. Reasoning: + +- **Windows** intercepts most `Win+*` chords at the OS level (`Win+R`, + `Win+E`, `Win+L`, `Win+Tab`, `Win+`) before Electron's renderer sees + them. Allowing binding would create silently-dead shortcuts. +- **Linux**'s Super key is WM-owned and behavior depends on distro/compositor + (GNOME overview, KDE app menu, tiling WM prefix, etc.). Same outcome. + +Users on those platforms should record ctrl-based chords instead. If we ever +want to loosen this on Linux specifically, replace the blanket reject with a +per-chord check against `OS_RESERVED` and a user-visible warning. + --- ## Testability @@ -337,9 +388,10 @@ cover all three bug classes deterministically. ### Test file See co-located tests: -- `apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts` -- `apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts` -- `apps/desktop/src/renderer/hotkeys/display.test.ts` +- `apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts` — token normalization, canonicalization, and the live override-aware reverse index (Bugs 1/3/4) +- `apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts` — migration validation (Bug 5) +- `apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts` — recorder capture (Bugs 1/2) +- `apps/desktop/src/renderer/hotkeys/display.test.ts` — display formatting parity --- diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts index bb5c62f0d85..da319b775b4 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts @@ -104,12 +104,8 @@ describe("captureHotkeyFromEvent — Bug 2: uses event.code, not event.key", () }); 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"); + 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", () => { diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts index d09b665218a..5b42e71f1d6 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts @@ -28,7 +28,12 @@ export function captureHotkeyFromEvent(event: KeyboardEvent): string | null { const isFKey = /^f([1-9]|1[0-2])$/.test(key); if (!isFKey && !event.ctrlKey && !event.metaKey) return null; - // Reject meta on non-Mac + // Reject meta (Win/Super) on non-Mac. + // Why: Windows intercepts most `Win+*` chords at the OS level (Win+R, + // Win+E, Win+L, Win+Tab, Win+) before Electron sees them, and the + // Super key on Linux is WM-owned (GNOME overview, KDE app menu, etc.). + // Allowing recording would silently create dead bindings that the app + // never receives. Users on those platforms should use ctrl-based chords. if (PLATFORM !== "mac" && event.metaKey) return null; const modifiers = new Set(); diff --git a/apps/desktop/src/renderer/hotkeys/migrate.ts b/apps/desktop/src/renderer/hotkeys/migrate.ts index 8712ce32ad7..e36faf9ea3b 100644 --- a/apps/desktop/src/renderer/hotkeys/migrate.ts +++ b/apps/desktop/src/renderer/hotkeys/migrate.ts @@ -7,6 +7,7 @@ import { electronTrpcClient } from "renderer/lib/trpc-client"; import { PLATFORM } from "./registry"; +import { canonicalizeChord, MODIFIERS } from "./utils/resolveHotkeyFromEvent"; const PLATFORM_MAP = { mac: "darwin", @@ -14,6 +15,24 @@ const PLATFORM_MAP = { linux: "linux", } as const; +/** + * A migrated override is only kept if, after canonicalization, it has exactly + * one non-modifier key token composed of word chars (letters/digits) — e.g. + * `ctrl+k`, `meta+shift+bracketleft`, `f12`. This drops pre-fix garbage like + * `ctrl+control`, `ctrl+shift+@`, or `meta+[` that the old recorder could + * produce and that would never match `event.code`-based dispatch. + */ +function sanitizeOverride(value: unknown): string | null | undefined { + if (value === null) return null; // explicit unassigned → preserve + if (typeof value !== "string" || !value.trim()) return undefined; + const canonical = canonicalizeChord(value); + const parts = canonical.split("+"); + const keys = parts.filter((p) => !MODIFIERS.has(p)); + if (keys.length !== 1) return undefined; + if (!/^[a-z0-9]+$/.test(keys[0])) return undefined; + return canonical; +} + export async function migrateHotkeyOverrides(): Promise { if (localStorage.getItem("hotkey-overrides")) { console.log("[hotkeys] Migration skipped — new store already exists"); @@ -29,12 +48,24 @@ export async function migrateHotkeyOverrides(): Promise { 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 }), ); 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); 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..86341b73013 --- /dev/null +++ b/apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "bun:test"; +import { canonicalizeChord, MODIFIERS } from "./resolveHotkeyFromEvent"; + +/** + * Mirrors the sanitizer in `migrate.ts` so we can unit-test the validation + * rules without mocking tRPC / localStorage. If the migrate.ts rules ever + * diverge from this, update both. + */ +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 parts = canonical.split("+"); + const keys = parts.filter((p) => !MODIFIERS.has(p)); + if (keys.length !== 1) return undefined; + if (!/^[a-z0-9]+$/.test(keys[0])) return undefined; + return canonical; +} + +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)", () => { + // canonicalizes to "ctrl" with no key token + 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 index b0abe8a8b9e..9b98ddd71c9 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts @@ -1,4 +1,6 @@ -import { describe, expect, it } from "bun:test"; +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { HOTKEYS } from "../registry"; +import { useHotkeyOverridesStore } from "../stores/hotkeyOverridesStore"; import { canonicalizeChord, isIgnorableKey, @@ -124,10 +126,124 @@ function ev(init: StubInit): KeyboardEvent { } as unknown as KeyboardEvent; } +describe("resolveHotkeyFromEvent — live override index", () => { + let originalOverrides: Record; + beforeEach(() => { + originalOverrides = useHotkeyOverridesStore.getState().overrides; + }); + afterEach(() => { + useHotkeyOverridesStore.setState({ overrides: originalOverrides }); + }); + + it("resolves a default binding when no override is set", () => { + // Pick any hotkey with a default key and construct its event. + const firstId = Object.keys(HOTKEYS)[0] as keyof typeof HOTKEYS; + const def = HOTKEYS[firstId].key; + if (!def) return; // skip if the default is unset + const event = buildEventFromChord(def); + expect(resolveHotkeyFromEvent(event)).toBe(firstId); + }); + + it("resolves a rebound chord after an override is saved", () => { + const id = Object.keys(HOTKEYS)[0] as keyof typeof HOTKEYS; + useHotkeyOverridesStore.setState({ + overrides: { [id]: "meta+shift+f10" }, + }); + const event = buildEventFromChord("meta+shift+f10"); + expect(resolveHotkeyFromEvent(event)).toBe(id); + }); + + it("does NOT resolve the old default after the user rebinds away from it", () => { + const id = Object.keys(HOTKEYS)[0] as keyof typeof HOTKEYS; + const oldDefault = HOTKEYS[id].key; + if (!oldDefault) return; + useHotkeyOverridesStore.setState({ + overrides: { [id]: "meta+shift+f10" }, + }); + const event = buildEventFromChord(oldDefault); + expect(resolveHotkeyFromEvent(event)).toBeNull(); + }); + + it("does NOT resolve a hotkey the user explicitly unassigned (null override)", () => { + const id = Object.keys(HOTKEYS)[0] as keyof typeof HOTKEYS; + const def = HOTKEYS[id].key; + if (!def) return; + useHotkeyOverridesStore.setState({ overrides: { [id]: null } }); + const event = buildEventFromChord(def); + 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 })), + resolveHotkeyFromEvent( + ev({ type: "keyup", code: "KeyP", metaKey: true }), + ), ).toBeNull(); }); diff --git a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts index a26289baa25..6f6afb510cb 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts @@ -1,16 +1,21 @@ 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. + * + * The reverse index is rebuilt whenever the user's override store changes, so + * consumers like the terminal's `attachCustomKeyEventHandler` always see the + * current effective bindings (not stale defaults). */ 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) @@ -82,9 +87,29 @@ 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, - ]), +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; + // `null` = user explicitly unassigned the shortcut → drop from index so + // the terminal does NOT swallow it. + if (hasOverride && override === null) continue; + const keys = override ?? HOTKEYS[id].key; + if (!keys) continue; + map.set(normalizeChord(keys), id); + } + return map; +} + +// Live reverse-index, rebuilt whenever overrides change. Using `let` because +// Zustand's `subscribe` mutates the binding on each store update. +let registeredAppChords = buildRegisteredAppChords( + useHotkeyOverridesStore.getState().overrides, ); + +useHotkeyOverridesStore.subscribe((state) => { + registeredAppChords = buildRegisteredAppChords(state.overrides); +}); From 1b60d0da4fe3406abd5311763f6e0ef384d24a8c Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 17:33:24 -0700 Subject: [PATCH 04/10] feat(desktop): allow meta (Win/Super) bindings on non-Mac with OS warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the blanket reject and extend OS_RESERVED for Windows shell intercepts (meta+d/e/l/r/tab) so users get a warning instead of a silent block. Linux WM configs vary too much to predict — trust the user and let them rebind if a chord doesn't fire. --- ...0412-keyboard-recorder-ctrl-binding-fix.md | 23 ++++++++++--------- .../useRecordHotkeys/useRecordHotkeys.ts | 23 +++++++++++-------- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md index 81f359b2b60..6b024cef4e4 100644 --- a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md +++ b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md @@ -343,20 +343,21 @@ QUICK_OPEN: { Adding `mod` would duplicate that capability without simplifying existing definitions. Skipped. -### Allowing meta (Win/Super) recording on non-Mac +### Meta (Win/Super) recording on non-Mac — now allowed with a warning -The recorder rejects `event.metaKey` on Windows/Linux. We kept this and added -an explanatory comment. Reasoning: +Originally we blanket-rejected `event.metaKey` on Windows/Linux because +Windows intercepts most `Win+*` chords (Win+R, Win+E, Win+L, Win+Tab) and the +Super key is WM-owned on Linux (GNOME overview, KDE menu, tiling prefixes). -- **Windows** intercepts most `Win+*` chords at the OS level (`Win+R`, - `Win+E`, `Win+L`, `Win+Tab`, `Win+`) before Electron's renderer sees - them. Allowing binding would create silently-dead shortcuts. -- **Linux**'s Super key is WM-owned and behavior depends on distro/compositor - (GNOME overview, KDE app menu, tiling WM prefix, etc.). Same outcome. +**Flipped that decision**: the blanket reject is paternalistic — users on +tiling WMs or with custom Windows configs often deliberately free up Super +for app use. We can't know their environment, and if a chord doesn't fire +they'll rebind. Instead: -Users on those platforms should record ctrl-based chords instead. If we ever -want to loosen this on Linux specifically, replace the blanket reject with a -per-chord check against `OS_RESERVED` and a user-visible warning. +- Recording meta-based chords is allowed on all platforms. +- `OS_RESERVED` was extended on Windows with the most common shell intercepts + (`meta+d/e/l/r/tab`). These surface a `"Reserved by OS"` warning at record + time instead of silently accepting a dead binding. --- diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts index 5b42e71f1d6..112a6cd9c03 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts @@ -28,14 +28,6 @@ export function captureHotkeyFromEvent(event: KeyboardEvent): string | null { const isFKey = /^f([1-9]|1[0-2])$/.test(key); if (!isFKey && !event.ctrlKey && !event.metaKey) return null; - // Reject meta (Win/Super) on non-Mac. - // Why: Windows intercepts most `Win+*` chords at the OS level (Win+R, - // Win+E, Win+L, Win+Tab, Win+) before Electron sees them, and the - // Super key on Linux is WM-owned (GNOME overview, KDE app menu, etc.). - // Allowing recording would silently create dead bindings that the app - // never receives. Users on those platforms should use ctrl-based chords. - if (PLATFORM !== "mac" && event.metaKey) return null; - const modifiers = new Set(); if (event.metaKey) modifiers.add("meta"); if (event.ctrlKey) modifiers.add("ctrl"); @@ -57,9 +49,22 @@ const TERMINAL_RESERVED = new Set([ "ctrl+backslash", ]); +// Chords that the OS / shell is likely to intercept before the renderer sees +// them. Binding is allowed (different Linux WM configs free many of these up), +// but the recorder emits a "Reserved by OS" warning so the user knows why a +// chord they just bound might not fire. const OS_RESERVED: Record = { mac: ["meta+q", "meta+space", "meta+tab"], - windows: ["alt+f4", "alt+tab", "ctrl+alt+delete"], + windows: [ + "alt+f4", + "alt+tab", + "ctrl+alt+delete", + "meta+d", // Show desktop + "meta+e", // Explorer + "meta+l", // Lock + "meta+r", // Run + "meta+tab", // Task view + ], linux: ["alt+f4", "alt+tab"], }; From 1e0cd32bfe013983fe7d958534e8166f14350873 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 20:22:43 -0700 Subject: [PATCH 05/10] =?UTF-8?q?refactor(desktop):=20unify=20event?= =?UTF-8?q?=E2=86=94chord=20matching=20via=20shared=20helpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit against react-hotkeys-hook and internal usages found two more consumers comparing via event.key, which breaks the same punctuation / layout / rebind cases fixed in the recorder: - utils/utils.ts isTerminalReservedEvent had its own event.key-based TERMINAL_RESERVED set with ctrl+\\. - Terminal/helpers.ts matchesKey used event.key to check the CLEAR_TERMINAL rebind — silently wrong for any punctuation rebind. Consolidation: - Export eventToChord and new matchesChord(event, chord) as the single canonical event↔chord matcher. - Export TERMINAL_RESERVED_CHORDS as the single source of truth. - Rewrite isTerminalReservedEvent around them. - Replace matchesKey() with matchesChord(). - Remove duplicated TERMINAL_RESERVED from useRecordHotkeys.ts. Also adds tests for eventToChord, matchesChord, and isTerminalReservedEvent parity. Updates plan doc with the cleanup. --- ...0412-keyboard-recorder-ctrl-binding-fix.md | 26 +++++ .../useRecordHotkeys/useRecordHotkeys.ts | 14 +-- apps/desktop/src/renderer/hotkeys/index.ts | 6 +- .../src/renderer/hotkeys/utils/index.ts | 5 +- .../utils/resolveHotkeyFromEvent.test.ts | 107 ++++++++++++++++++ .../hotkeys/utils/resolveHotkeyFromEvent.ts | 32 +++++- .../src/renderer/hotkeys/utils/utils.ts | 21 ++-- .../TabsContent/Terminal/helpers.ts | 14 +-- 8 files changed, 185 insertions(+), 40 deletions(-) diff --git a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md index 6b024cef4e4..7e14961f110 100644 --- a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md +++ b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md @@ -305,6 +305,32 @@ unassignment) are dropped from the index so the terminal does not swallow them. Tests in `resolveHotkeyFromEvent.test.ts` cover rebind, old-default gone, and unassigned cases. +### Bug 6 — Two more consumers were matching via `event.key` + +A post-refactor audit found the same event.key/event.code mismatch pattern in +two other places: + +1. `apps/desktop/src/renderer/hotkeys/utils/utils.ts::isTerminalReservedEvent` + — used `event.key.toLowerCase()` with its own local `TERMINAL_RESERVED` + set containing `ctrl+\\`. This happened to work for the letter chords + (`c`, `d`, `s`, `q`, `z`) but diverged from `useRecordHotkeys.ts`'s + canonical set which contains `ctrl+backslash`. +2. `…/Terminal/helpers.ts::matchesKey` — same event.key-based compare used + for the `CLEAR_TERMINAL` rebind check. Would silently fail if the user + rebound to any punctuation chord (e.g. `ctrl+shift+bracketleft`). + +**Fix**: + +- Exported `eventToChord` and introduced `matchesChord(event, chord)` in + `resolveHotkeyFromEvent.ts` as the single canonical event↔chord matcher. +- Exported `TERMINAL_RESERVED_CHORDS` as the single source of truth for + terminal-reserved chords (canonical form). +- Rewrote `isTerminalReservedEvent` and replaced `matchesKey` usage with + `matchesChord`. Removed the duplicated set from `useRecordHotkeys.ts`. + +New tests cover `eventToChord`, `matchesChord` (modifier order, arrow forms, +punctuation, negative cases), and `isTerminalReservedEvent` parity. + ### Bug 5 — Migration carried forward corrupt pre-fix overrides `migrate.ts` copied old overrides verbatim from the main-process tRPC store diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts index 112a6cd9c03..2858971cb53 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts @@ -6,6 +6,7 @@ import { canonicalizeChord, isIgnorableKey, normalizeToken, + TERMINAL_RESERVED_CHORDS, } from "../../utils/resolveHotkeyFromEvent"; // --------------------------------------------------------------------------- @@ -38,17 +39,6 @@ export function captureHotkeyFromEvent(event: KeyboardEvent): string | null { return [...ordered, key].join("+"); } -// Reserved chords are matched after canonicalization, so use event.code-based -// tokens (e.g. `backslash`, not `\\`). -const TERMINAL_RESERVED = new Set([ - "ctrl+c", - "ctrl+d", - "ctrl+z", - "ctrl+s", - "ctrl+q", - "ctrl+backslash", -]); - // Chords that the OS / shell is likely to intercept before the renderer sees // them. Binding is allowed (different Linux WM configs free many of these up), // but the recorder emits a "Reserved by OS" warning so the user knows why a @@ -72,7 +62,7 @@ function checkReserved( keys: string, ): { reason: string; severity: "error" | "warning" } | null { const canonical = canonicalizeChord(keys); - if (TERMINAL_RESERVED.has(canonical)) + if (TERMINAL_RESERVED_CHORDS.has(canonical)) return { reason: "Reserved by terminal", severity: "error" }; if (OS_RESERVED[PLATFORM].includes(canonical)) return { reason: "Reserved by OS", severity: "warning" }; 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/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/resolveHotkeyFromEvent.test.ts b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts index 9b98ddd71c9..41d530419b4 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts @@ -3,10 +3,14 @@ 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 @@ -267,3 +271,106 @@ describe("resolveHotkeyFromEvent", () => { ).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 6f6afb510cb..a2ece3c113a 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts @@ -74,7 +74,13 @@ export function canonicalizeChord(chord: string): string { return normalizeChord(chord); } -function eventToChord(event: KeyboardEvent): string | null { +/** + * Normalized canonical chord string for a KeyboardEvent, or null if it is a + * pure modifier / synthetic / ignorable press. Modifier order is sorted to + * match `canonicalizeChord`, so output is directly comparable with that + * function's output. + */ +export function eventToChord(event: KeyboardEvent): string | null { if (event.code === undefined) return null; const key = normalizeToken(event.code); if (isIgnorableKey(key)) return null; @@ -87,6 +93,30 @@ function eventToChord(event: KeyboardEvent): string | null { return [...mods, key].join("+"); } +/** + * True if a KeyboardEvent matches a chord string (in any form the registry or + * user might produce — `meta+alt+up`, `alt+meta+arrowup`, `control+k`, etc.). + */ +export function matchesChord(event: KeyboardEvent, chord: string): boolean { + const eventChord = eventToChord(event); + if (!eventChord) return false; + return eventChord === canonicalizeChord(chord); +} + +/** + * Terminal-reserved chords (sent straight to the PTY regardless of whether + * they would otherwise match an app hotkey). Stored in canonical form 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", +]); + function buildRegisteredAppChords( overrides: Record, ): Map { 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(); } From 3cd60265fb02a32e3cbc8913e0769846dd5d73d7 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 20:26:19 -0700 Subject: [PATCH 06/10] docs(desktop): rewrite hotkey fix plan for concise review --- ...0412-keyboard-recorder-ctrl-binding-fix.md | 511 +++++------------- 1 file changed, 133 insertions(+), 378 deletions(-) diff --git a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md index 7e14961f110..8b5620b33d5 100644 --- a/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md +++ b/apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md @@ -1,434 +1,189 @@ # Keyboard Recorder — Ctrl Binding & event.code Unification -**Date:** 2026-04-12 -**Scope:** `apps/desktop/src/renderer/hotkeys/*` -**Status:** shipped +**Date:** 2026-04-12 · **Scope:** `apps/desktop/src/renderer/hotkeys/*` (+ 1 terminal file) · **PR:** #3391 -## Problem +## TL;DR -User reported that the Settings → Keyboard recorder would not allow binding -chords that begin with Ctrl. Investigation found three compounding bugs in the -recorder and its comparison/display logic. +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 1 — Ctrl press auto-committed an invalid binding +| # | 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 | -### Before +## Key code changes -`useRecordHotkeys.ts:13-15` filtered pure-modifier keydowns using lowercased -`event.key`: +### Shared helpers (`utils/resolveHotkeyFromEvent.ts`) -```ts -const key = event.key.toLowerCase(); -if (["shift", "ctrl", "alt", "meta", "dead", "unidentified"].includes(key)) - return null; -``` - -### Why it broke - -`KeyboardEvent.key` for the Control key is the string `"Control"`, not `"Ctrl"`. -Lowercased, that's `"control"` — which does **not** match `"ctrl"` in the -ignore list. Result: pressing Ctrl alone (the normal first half of any Ctrl -chord) passed the filter, `event.ctrlKey` was true, and -`captureHotkeyFromEvent` immediately returned and saved `ctrl+control` as the -binding before the user could press the second key. - -Shift/Alt/Meta worked because their `event.key` values lowercase to `"shift"`, -`"alt"`, `"meta"` — matching the filter. - -### Source citation - -[MDN KeyboardEvent.key values](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values#modifier_keys) -specifies: - -> `"Control"` — The Control (Ctrl) key. - -The repo already aliases `ControlLeft`/`ControlRight` → `ctrl` in -`resolveHotkeyFromEvent.ts` for `event.code` input, which hid the mismatch — it -only surfaces when someone uses `event.key`. - -### Fix - -Filter against the actual lowercased `event.key` value, plus other modifier-ish -keys that should never commit a binding on their own. +Exposes the canonical normalizer and matcher used everywhere: ```ts -// apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts -export const MODIFIERS = new Set(["meta", "ctrl", "control", "alt", "shift"]); -const LOCK_KEYS = new Set(["capslock", "numlock", "scrolllock"]); - -export function isIgnorableKey(normalized: string): boolean { - return !normalized || MODIFIERS.has(normalized) || LOCK_KEYS.has(normalized); -} +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; ``` -Also added `altgraph` (AltGr on European keyboards) coverage via the broader -rewrite below. - ---- - -## Bug 2 — Recorder and resolver used different key sources - -### Before - -- **Resolver** (`resolveHotkeyFromEvent.ts:59`, `eventToChord`): used - `event.code`, normalized via `CODE_ALIASES` (the same alias table as - `react-hotkeys-hook`). -- **Recorder** (`useRecordHotkeys.ts:13`): used `event.key`. -- **Registry defaults** (`registry.ts`): written in `event.code` form - (`bracketleft`, `comma`, `slash`, `backspace`, etc.). - -### Why it broke - -`event.key` depends on layout and other held modifiers; `event.code` is a -stable physical-key identifier. They diverge in common cases: - -| Chord pressed | Recorder saved (`event.key`) | Registry/library form (`event.code`) | Match? | -| ----------------- | ---------------------------- | ------------------------------------ | ------ | -| Ctrl+Shift+2 | `ctrl+shift+@` | `ctrl+shift+2` | ❌ | -| Alt+L on Mac | `alt+¬` | `alt+l` | ❌ | -| Ctrl+/ on DE kbd | `ctrl+-` | `ctrl+slash` | ❌ | -| Meta+[ | `meta+[` | `meta+bracketleft` | ❌ | - -The saved override string was fed into `useHotkeys(keys, …)`, which internally -re-parses via `mapCode`. Since `mapCode` is code-aware, an override string like -`ctrl+shift+@` would never match the `event.code` `"Digit2"`. - -### Source citation - -Upstream `react-hotkeys-hook` uses `event.code` by default -([`packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts`](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts)): - -```ts -// react-hotkeys-hook/packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts -const handler = useCallback( - (event: KeyboardEvent) => { - if (event.code === undefined) { - // Synthetic event (e.g., Chrome autofill). Ignore. - return - } - event.preventDefault() - event.stopPropagation() - setKeys((prev) => { - const newKeys = new Set(prev) - newKeys.add(mapCode(useKey ? event.key : event.code)) - return newKeys - }) - }, - [useKey], -) -``` - -And `parseHotkeys.ts` aliases `event.code` values like `ControlLeft` → `ctrl`, -`ShiftLeft` → `shift`, and strips `/key|digit|numpad/` so `KeyA` → `a`, -`Digit1` → `1`: +Reverse index is now live (Bug 4): ```ts -// react-hotkeys-hook/packages/react-hotkeys-hook/src/lib/parseHotkeys.ts -const mappedKeys: Record = { - esc: 'escape', return: 'enter', - left: 'arrowleft', right: 'arrowright', up: 'arrowup', down: 'arrowdown', - ShiftLeft: 'shift', ShiftRight: 'shift', - AltLeft: 'alt', AltRight: 'alt', - MetaLeft: 'meta', MetaRight: 'meta', - OSLeft: 'meta', OSRight: 'meta', - ControlLeft: 'ctrl', ControlRight: 'ctrl', -} - -export function mapCode(key: string): string { - return (mappedKeys[key.trim()] || key.trim()).toLowerCase().replace(/key|digit|numpad/, '') -} +let registeredAppChords = buildRegisteredAppChords( + useHotkeyOverridesStore.getState().overrides, +); +useHotkeyOverridesStore.subscribe((state) => { + registeredAppChords = buildRegisteredAppChords(state.overrides); +}); ``` -### Fix +### Recorder (`hooks/useRecordHotkeys/useRecordHotkeys.ts`) -Export the existing `normalizeToken` (and `isIgnorableKey`) from -`resolveHotkeyFromEvent.ts` and reuse it in the recorder so both sides speak -the same language: +Bug 1 + 2 in one: ```ts -// apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts -import { - canonicalizeChord, - isIgnorableKey, - normalizeToken, -} from "../../utils/resolveHotkeyFromEvent"; - -function captureHotkeyFromEvent(event: KeyboardEvent): string | null { - if (event.code === undefined) return null; // matches upstream guard - const key = normalizeToken(event.code); - if (isIgnorableKey(key)) return null; - - const isFKey = /^f([1-9]|1[0-2])$/.test(key); - if (!isFKey && !event.ctrlKey && !event.metaKey) return null; - 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 ordered = MODIFIER_ORDER.filter((m) => modifiers.has(m)); - return [...ordered, key].join("+"); -} +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. -## Bug 3 — String comparisons didn't canonicalize +### Migration (`migrate.ts`) -### Before +Bug 5: sanitize each migrated value. Drops garbage (`ctrl+control`, +`ctrl+shift+@`, `meta+[`) and logs the count. Preserves `null` (explicit +unassignment). ```ts -// useRecordHotkeys.ts (before) -if (TERMINAL_RESERVED.has(keys)) // raw string lookup -if (OS_RESERVED[PLATFORM].includes(keys)) // raw string lookup -if (effective === keys) return id; // raw string comparison (conflict) -if (captured === defaultKey) resetOverride();// raw string comparison (reset-to-default) +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; ``` -### Why it broke - -The recorder produces strings in `MODIFIER_ORDER` (`meta+ctrl+alt+shift`), the -registry author orders them however reads naturally (`meta+alt+up`), and the -resolver sorts alphabetically (`alt+meta+arrowup`). All three forms mean the -same chord but fail `===` comparison. - -Examples that silently misbehaved: - -- User rebinds `PREV_WORKSPACE` to its own default (`meta+alt+up`). Recorder - saves `meta+alt+arrowup`; `captured === defaultKey` is false so we write an - override instead of resetting. -- User tries to bind `meta+alt+up` while `NEXT_WORKSPACE` already has - `meta+alt+down` — conflict check runs string `===` between canonical-ish - recorder output and registry-authored defaults, producing false negatives on - overlapping overrides. - -### Fix +### Terminal helpers -Introduce a single canonical form and use it for all comparisons. +Bug 6: `utils/utils.ts` and `Terminal/helpers.ts` now use `matchesChord` + +shared `TERMINAL_RESERVED_CHORDS`: ```ts -// apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts -export function canonicalizeChord(chord: string): string { - // sorts modifiers, normalizes control→ctrl, up→arrowup, bracketleft stays, etc. - return normalizeChord(chord); +// utils/utils.ts +export function isTerminalReservedEvent(event: KeyboardEvent): boolean { + const chord = eventToChord(event); + return chord != null && TERMINAL_RESERVED_CHORDS.has(chord); } -``` - -Applied at the three comparison sites in `useRecordHotkeys.ts`: -```ts -function checkReserved(keys: string) { - const canonical = canonicalizeChord(keys); - if (TERMINAL_RESERVED.has(canonical)) return { reason: "Reserved by terminal", severity: "error" }; - if (OS_RESERVED[PLATFORM].includes(canonical)) return { reason: "Reserved by OS", severity: "warning" }; - return null; -} - -function getHotkeyConflict(keys: string, excludeId: HotkeyId) { - 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 && canonicalizeChord(effective) === canonicalKeys) return id; - } - return null; -} - -// reset-to-default detection -if (canonicalizeChord(captured) === canonicalizeChord(defaultKey)) { - resetOverride(recordingId); -} else { - setOverride(recordingId, captured); -} +// Terminal/helpers.ts — was: matchesKey(event, keys) +if (clearKeys && matchesChord(event, clearKeys)) { … } ``` -`TERMINAL_RESERVED` had `"ctrl+\\"` — canonicalization leaves the backslash -alone, but the recorder produces `ctrl+backslash` from `event.code === "Backslash"`. -Changed the constant to `"ctrl+backslash"` so the set membership check matches -what the recorder emits. - ---- - -## Display parity (`display.ts`) - -The display layer rendered symbols from a static table keyed on short names -(`up`, `comma`) and never normalized input. Overrides stored in canonical form -(`arrowup`) would render as `"ARROWUP"`. Fix: - -```ts -// apps/desktop/src/renderer/hotkeys/display.ts -import { normalizeToken } from "./utils/resolveHotkeyFromEvent"; - -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: "]", -}; - -export function formatHotkeyDisplay(keys: string | null, platform: Platform): HotkeyDisplay { - if (!keys) return { keys: ["Unassigned"], text: "Unassigned" }; - const parts = keys.toLowerCase().split("+").map(normalizeToken); - // …rest unchanged; uses KEY_DISPLAY[key] ?? key.toUpperCase() -} -``` - ---- - -## Follow-up bugs found during review - -### Bug 4 — Terminal swallowed hotkeys using stale defaults - -`resolveHotkeyFromEvent.ts` built its reverse index once at module load from -`HOTKEYS` defaults, ignoring user overrides. `terminal-runtime.ts:154` passes -`(event) => !isAppHotkey(event)` to xterm's `attachCustomKeyEventHandler`, so -the terminal's swallow/forward decision used frozen defaults. - -Failure modes after a user rebinds a hotkey from `meta+l` → `meta+y`: - -| In-terminal keystroke | Old behavior | -| ----------------------- | --------------------------------------------------- | -| `meta+y` (new binding) | not in map → xterm consumes → binding dead | -| `meta+l` (freed up) | still in map → xterm bails → bubbles → nothing fires → keystroke eaten | - -**Fix**: the reverse index is now a `let` rebound on every -`useHotkeyOverridesStore.subscribe` callback. Null overrides (explicit -unassignment) are dropped from the index so the terminal does not swallow -them. Tests in `resolveHotkeyFromEvent.test.ts` cover rebind, old-default -gone, and unassigned cases. - -### Bug 6 — Two more consumers were matching via `event.key` +### Display (`display.ts`) -A post-refactor audit found the same event.key/event.code mismatch pattern in -two other places: +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`). -1. `apps/desktop/src/renderer/hotkeys/utils/utils.ts::isTerminalReservedEvent` - — used `event.key.toLowerCase()` with its own local `TERMINAL_RESERVED` - set containing `ctrl+\\`. This happened to work for the letter chords - (`c`, `d`, `s`, `q`, `z`) but diverged from `useRecordHotkeys.ts`'s - canonical set which contains `ctrl+backslash`. -2. `…/Terminal/helpers.ts::matchesKey` — same event.key-based compare used - for the `CLEAR_TERMINAL` rebind check. Would silently fail if the user - rebound to any punctuation chord (e.g. `ctrl+shift+bracketleft`). +## Library audit — nothing else missed -**Fix**: +Checked every `react-hotkeys-hook` usage against upstream docs: -- Exported `eventToChord` and introduced `matchesChord(event, chord)` in - `resolveHotkeyFromEvent.ts` as the single canonical event↔chord matcher. -- Exported `TERMINAL_RESERVED_CHORDS` as the single source of truth for - terminal-reserved chords (canonical form). -- Rewrote `isTerminalReservedEvent` and replaced `matchesKey` usage with - `matchesChord`. Removed the duplicated set from `useRecordHotkeys.ts`. +| 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 | -New tests cover `eventToChord`, `matchesChord` (modifier order, arrow forms, -punctuation, negative cases), and `isTerminalReservedEvent` parity. +Registry defaults already use event.code-canonical tokens (`bracketleft`, +`comma`, `slash`, `arrowup`). No hardcoded chord strings found outside +`hotkeys/` that need canonicalization. -### Bug 5 — Migration carried forward corrupt pre-fix overrides +## Decisions taken -`migrate.ts` copied old overrides verbatim from the main-process tRPC store -into the new localStorage store. Any user who hit the pre-fix recorder could -have saved junk like `ctrl+control`, `ctrl+shift+@`, or `meta+[`, and the -migration would keep those broken strings around forever. +- **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. -**Fix**: `migrate.ts` now runs each migrated value through a sanitizer that -canonicalizes the chord and requires exactly one word-char key token. Invalid -entries are dropped with a count logged. `null` (explicit unassignment) is -preserved. Tests in `overrideSanitizer.test.ts`. +## Testability ---- +Everything fixed is in pure functions over primitives. **62 tests across 4 +files**, no React/DOM harness needed (plain KeyboardEvent stubs): -## Decisions deliberately not taken +| 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) | -### `mod` modifier alias +Only untested branch: non-Mac `PLATFORM` path in the recorder's OS-reserved +warning. Would need module-mocking `PLATFORM`; not worth the harness. -Upstream `react-hotkeys-hook` supports `mod` as "meta on macOS, ctrl elsewhere" -([`parseHotkeys.ts` reserved list](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/parseHotkeys.ts)): +## Files changed -```ts -const reservedModifierKeywords = ['shift', 'alt', 'meta', 'mod', 'ctrl', 'control'] ``` - -Our registry already stores per-platform bindings: - -```ts -// apps/desktop/src/renderer/hotkeys/registry.ts -QUICK_OPEN: { - key: { mac: "meta+p", windows: "ctrl+shift+p", linux: "ctrl+shift+p" }, - ... -} +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 ``` -Adding `mod` would duplicate that capability without simplifying existing -definitions. Skipped. - -### Meta (Win/Super) recording on non-Mac — now allowed with a warning - -Originally we blanket-rejected `event.metaKey` on Windows/Linux because -Windows intercepts most `Win+*` chords (Win+R, Win+E, Win+L, Win+Tab) and the -Super key is WM-owned on Linux (GNOME overview, KDE menu, tiling prefixes). - -**Flipped that decision**: the blanket reject is paternalistic — users on -tiling WMs or with custom Windows configs often deliberately free up Super -for app use. We can't know their environment, and if a chord doesn't fire -they'll rebind. Instead: - -- Recording meta-based chords is allowed on all platforms. -- `OS_RESERVED` was extended on Windows with the most common shell intercepts - (`meta+d/e/l/r/tab`). These surface a `"Reserved by OS"` warning at record - time instead of silently accepting a dead binding. - ---- - -## Testability - -Everything the fix touches is now isolated in **pure functions** that take -primitives and return primitives. All three bugs are testable without React or -a DOM: - -| Unit | Pure? | Inputs | Test harness | -| --------------------------------------------------- | ----- | ---------------------- | -------------------- | -| `normalizeToken(event.code \| alias)` | ✅ | string | `bun:test` direct | -| `isIgnorableKey(normalized)` | ✅ | string | `bun:test` direct | -| `canonicalizeChord(chord)` | ✅ | string | `bun:test` direct | -| `eventToChord(event)` / `resolveHotkeyFromEvent` | ✅ | `KeyboardEventInit` | `new KeyboardEvent` | -| `captureHotkeyFromEvent(event)` | ✅\* | `KeyboardEventInit` | `new KeyboardEvent` | -| `formatHotkeyDisplay(keys, platform)` | ✅ | string, `Platform` | `bun:test` direct | - -\* `captureHotkeyFromEvent` references `PLATFORM` (imported from `registry.ts`), -which is computed once from `navigator.platform`. To test non-Mac branches, -either export a `_captureHotkeyFromEventWithPlatform(event, platform)` variant -or module-mock `PLATFORM`. For the initial test pass we cover the current host -platform and construct events for the paths that don't depend on `PLATFORM`. - -The hook integration (`useRecordHotkeys`) is better covered with a smoke test -using `@testing-library/react` + `userEvent.keyboard`, but the pure helpers -cover all three bug classes deterministically. - -### Test file - -See co-located tests: -- `apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts` — token normalization, canonicalization, and the live override-aware reverse index (Bugs 1/3/4) -- `apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts` — migration validation (Bug 5) -- `apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts` — recorder capture (Bugs 1/2) -- `apps/desktop/src/renderer/hotkeys/display.test.ts` — display formatting parity +## 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` (main)](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/parseHotkeys.ts) -- [`useRecordHotkeys.ts` (main)](https://raw.githubusercontent.com/JohannesKlauss/react-hotkeys-hook/main/packages/react-hotkeys-hook/src/lib/useRecordHotkeys.ts) -- [MDN — KeyboardEvent.key values](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values) +- [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) -- VSCode's terminal hotkey forwarding pattern (referenced in - `apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:19-22` comment - citing `terminalInstance.ts:1116-1175`) From 7ba18d4762612eccbfd9e02576505afd3665ff01 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 20:33:51 -0700 Subject: [PATCH 07/10] =?UTF-8?q?chore(desktop/hotkeys):=20deslop=20?= =?UTF-8?q?=E2=80=94=20tighten=20comments,=20remove=20dead=20wrappers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Merge canonicalizeChord / normalizeChord into one exported function (the wrapper was pointless indirection). - Drop section-banner comments (`// Helpers`, `// Hook`) and comments that restated code (`// Must include ctrl or meta…`). - Tighten JSDocs to convey intent in one line where possible. - display.ts: drop duplicate short arrow entries (`up: "↑"`) — the normalizeToken call aliases them to canonical names already. Extract isModifier type guard to kill the cast repetition. Net -35 LOC, same behavior, same 62 tests passing. --- apps/desktop/src/renderer/hotkeys/display.ts | 38 ++++++------- .../useRecordHotkeys/useRecordHotkeys.ts | 26 +++------ apps/desktop/src/renderer/hotkeys/migrate.ts | 17 +++--- .../hotkeys/utils/resolveHotkeyFromEvent.ts | 56 ++++++------------- 4 files changed, 51 insertions(+), 86 deletions(-) diff --git a/apps/desktop/src/renderer/hotkeys/display.ts b/apps/desktop/src/renderer/hotkeys/display.ts index e5c40bd4346..269f55a1183 100644 --- a/apps/desktop/src/renderer/hotkeys/display.ts +++ b/apps/desktop/src/renderer/hotkeys/display.ts @@ -12,19 +12,15 @@ const MODIFIER_DISPLAY: Record> = { linux: { meta: "Super", ctrl: "Ctrl", alt: "Alt", shift: "Shift" }, }; -// Keyed by canonical (event.code-normalized) tokens. Accepts both short and -// canonical arrow names so legacy registry strings (`up`) and recorder output -// (`arrowup`) both render correctly. +// 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: "←", @@ -44,27 +40,29 @@ const KEY_DISPLAY: Record = { }; 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("+").map(normalizeToken); - const modifiers = parts - .map((p) => (p === "control" ? "ctrl" : p)) - .filter((p) => - MODIFIER_ORDER.includes(p as (typeof MODIFIER_ORDER)[number]), - ); - const key = parts.find( - (p) => - p !== "control" && - !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.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts index 2858971cb53..303df45bf45 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts @@ -9,23 +9,18 @@ import { TERMINAL_RESERVED_CHORDS, } from "../../utils/resolveHotkeyFromEvent"; -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - -// Matches the registry's modifier order (e.g. `meta+shift+l`, `meta+alt+up`) -// so recorded overrides are string-comparable with HOTKEYS[id].key. +// 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; export function captureHotkeyFromEvent(event: KeyboardEvent): string | null { - // Normalize via event.code (matches the library's matcher and registry tokens - // like `bracketleft`, `comma`, `slash`). Using event.key would diverge for - // Shift+digit (`@` vs `2`), Alt+letter on Mac (`¬` vs `l`), and non-US layouts. + // 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; @@ -39,10 +34,9 @@ export function captureHotkeyFromEvent(event: KeyboardEvent): string | null { return [...ordered, key].join("+"); } -// Chords that the OS / shell is likely to intercept before the renderer sees -// them. Binding is allowed (different Linux WM configs free many of these up), -// but the recorder emits a "Reserved by OS" warning so the user knows why a -// chord they just bound might not fire. +// 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. const OS_RESERVED: Record = { mac: ["meta+q", "meta+space", "meta+tab"], windows: [ @@ -80,10 +74,6 @@ function getHotkeyConflict(keys: string, excludeId: HotkeyId): HotkeyId | null { return null; } -// --------------------------------------------------------------------------- -// Hook -// --------------------------------------------------------------------------- - interface UseRecordHotkeysOptions { onSave?: (id: HotkeyId, keys: string) => void; onCancel?: () => void; diff --git a/apps/desktop/src/renderer/hotkeys/migrate.ts b/apps/desktop/src/renderer/hotkeys/migrate.ts index e36faf9ea3b..be48fc84551 100644 --- a/apps/desktop/src/renderer/hotkeys/migrate.ts +++ b/apps/desktop/src/renderer/hotkeys/migrate.ts @@ -16,20 +16,17 @@ const PLATFORM_MAP = { } as const; /** - * A migrated override is only kept if, after canonicalization, it has exactly - * one non-modifier key token composed of word chars (letters/digits) — e.g. - * `ctrl+k`, `meta+shift+bracketleft`, `f12`. This drops pre-fix garbage like - * `ctrl+control`, `ctrl+shift+@`, or `meta+[` that the old recorder could - * produce and that would never match `event.code`-based dispatch. + * Drops pre-fix garbage (`ctrl+control`, `ctrl+shift+@`, `meta+[`) that the + * old recorder could produce and that would never match `event.code`-based + * dispatch. `null` is preserved as explicit unassignment; `undefined` means + * drop the entry. */ function sanitizeOverride(value: unknown): string | null | undefined { - if (value === null) return null; // explicit unassigned → preserve + if (value === null) return null; if (typeof value !== "string" || !value.trim()) return undefined; const canonical = canonicalizeChord(value); - const parts = canonical.split("+"); - const keys = parts.filter((p) => !MODIFIERS.has(p)); - if (keys.length !== 1) return undefined; - if (!/^[a-z0-9]+$/.test(keys[0])) return undefined; + 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/resolveHotkeyFromEvent.ts b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts index a2ece3c113a..4a81d329322 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts @@ -2,14 +2,10 @@ 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. - * - * The reverse index is rebuilt whenever the user's override store changes, so - * consumers like the terminal's `attachCustomKeyEventHandler` always see the - * current effective bindings (not stale defaults). + * 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; @@ -40,7 +36,7 @@ const CODE_ALIASES: Record = { export const MODIFIERS = new Set(["meta", "ctrl", "control", "alt", "shift"]); -// Lock keys should never commit a binding on their own. +// Lock keys must never commit a binding on their own. const LOCK_KEYS = new Set(["capslock", "numlock", "scrolllock"]); export function normalizeToken(token: string): string { @@ -52,7 +48,11 @@ export function isIgnorableKey(normalized: string): boolean { return !normalized || MODIFIERS.has(normalized) || LOCK_KEYS.has(normalized); } -function normalizeChord(chord: string): string { +/** + * 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[] = []; @@ -67,19 +67,7 @@ function normalizeChord(chord: string): string { return [...mods, ...keys].join("+"); } -// Canonical form for string comparison between recorded overrides and registry -// defaults. Tolerates differences in modifier order and token aliases (e.g. -// `meta+alt+up` vs `alt+meta+arrowup`, or `control` vs `ctrl`). -export function canonicalizeChord(chord: string): string { - return normalizeChord(chord); -} - -/** - * Normalized canonical chord string for a KeyboardEvent, or null if it is a - * pure modifier / synthetic / ignorable press. Modifier order is sorted to - * match `canonicalizeChord`, so output is directly comparable with that - * function's output. - */ +/** 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); @@ -93,21 +81,14 @@ export function eventToChord(event: KeyboardEvent): string | null { return [...mods, key].join("+"); } -/** - * True if a KeyboardEvent matches a chord string (in any form the registry or - * user might produce — `meta+alt+up`, `alt+meta+arrowup`, `control+k`, etc.). - */ +/** 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); } -/** - * Terminal-reserved chords (sent straight to the PTY regardless of whether - * they would otherwise match an app hotkey). Stored in canonical form so - * lookups via `eventToChord` / `canonicalizeChord` match directly. - */ +/** Sent straight to the PTY. Stored canonical so direct lookups work. */ export const TERMINAL_RESERVED_CHORDS = new Set([ "ctrl+c", "ctrl+d", @@ -124,22 +105,21 @@ function buildRegisteredAppChords( for (const id of Object.keys(HOTKEYS) as HotkeyId[]) { const hasOverride = id in overrides; const override = hasOverride ? overrides[id] : undefined; - // `null` = user explicitly unassigned the shortcut → drop from index so - // the terminal does NOT swallow it. + // 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(normalizeChord(keys), id); + map.set(canonicalizeChord(keys), id); } return map; } -// Live reverse-index, rebuilt whenever overrides change. Using `let` because -// Zustand's `subscribe` mutates the binding on each store update. +// 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); }); From e890e772ad4213813281a072cad5a4123b2f607c Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 20:42:54 -0700 Subject: [PATCH 08/10] fix(desktop/hotkeys): canonicalize reserved-chord tables + review cleanups Addresses PR review feedback: - Bug: OS_RESERVED[\"windows\"] had \"ctrl+alt+delete\" which never matched because canonicalization sorts modifiers alphabetically. Wrap both OS_RESERVED and TERMINAL_RESERVED_CHORDS in .map(canonicalizeChord) at build time so multi-modifier entries can't silently miss the reserved warning. OS_RESERVED values switched from array to Set. - Extract sanitizeOverride into utils/sanitizeOverride.ts so the test imports the real implementation instead of a near-copy. - Test stub: preserve explicit \`code: undefined\` so the synthetic-event guard in captureHotkeyFromEvent is actually exercised (not the empty-string branch). - Resolver tests: resolve a sample hotkey once at describe scope and throw if HOTKEYS has no bound defaults, instead of each test silently no-op-ing via \`if (!def) return\`. - Add regression test asserting canonicalizeChord(\"ctrl+alt+delete\") sorts to \"alt+ctrl+delete\". --- .../useRecordHotkeys/useRecordHotkeys.test.ts | 4 +- .../useRecordHotkeys/useRecordHotkeys.ts | 33 +++++++------- apps/desktop/src/renderer/hotkeys/migrate.ts | 17 +------ .../hotkeys/utils/overrideSanitizer.test.ts | 19 +------- .../utils/resolveHotkeyFromEvent.test.ts | 45 +++++++++++-------- .../hotkeys/utils/resolveHotkeyFromEvent.ts | 15 +++---- .../hotkeys/utils/sanitizeOverride.ts | 19 ++++++++ 7 files changed, 74 insertions(+), 78 deletions(-) create mode 100644 apps/desktop/src/renderer/hotkeys/utils/sanitizeOverride.ts diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts index da319b775b4..0dd60664767 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts @@ -22,7 +22,9 @@ interface StubInit { function ev(init: StubInit): KeyboardEvent { return { type: "keydown", - code: init.code ?? "", + // 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, diff --git a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts index 303df45bf45..8b52d9e552d 100644 --- a/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts +++ b/apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts @@ -36,20 +36,23 @@ export function captureHotkeyFromEvent(event: KeyboardEvent): string | null { // 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. -const OS_RESERVED: Record = { - mac: ["meta+q", "meta+space", "meta+tab"], - windows: [ - "alt+f4", - "alt+tab", - "ctrl+alt+delete", - "meta+d", // Show desktop - "meta+e", // Explorer - "meta+l", // Lock - "meta+r", // Run - "meta+tab", // Task view - ], - linux: ["alt+f4", "alt+tab"], +// 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( @@ -58,7 +61,7 @@ function checkReserved( const canonical = canonicalizeChord(keys); if (TERMINAL_RESERVED_CHORDS.has(canonical)) return { reason: "Reserved by terminal", severity: "error" }; - if (OS_RESERVED[PLATFORM].includes(canonical)) + if (OS_RESERVED[PLATFORM].has(canonical)) return { reason: "Reserved by OS", severity: "warning" }; return null; } diff --git a/apps/desktop/src/renderer/hotkeys/migrate.ts b/apps/desktop/src/renderer/hotkeys/migrate.ts index be48fc84551..b4407392265 100644 --- a/apps/desktop/src/renderer/hotkeys/migrate.ts +++ b/apps/desktop/src/renderer/hotkeys/migrate.ts @@ -7,7 +7,7 @@ import { electronTrpcClient } from "renderer/lib/trpc-client"; import { PLATFORM } from "./registry"; -import { canonicalizeChord, MODIFIERS } from "./utils/resolveHotkeyFromEvent"; +import { sanitizeOverride } from "./utils/sanitizeOverride"; const PLATFORM_MAP = { mac: "darwin", @@ -15,21 +15,6 @@ const PLATFORM_MAP = { linux: "linux", } as const; -/** - * Drops pre-fix garbage (`ctrl+control`, `ctrl+shift+@`, `meta+[`) that the - * old recorder could produce and that would never match `event.code`-based - * dispatch. `null` is preserved as explicit unassignment; `undefined` means - * drop the entry. - */ -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; -} - export async function migrateHotkeyOverrides(): Promise { if (localStorage.getItem("hotkey-overrides")) { console.log("[hotkeys] Migration skipped — new store already exists"); diff --git a/apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts b/apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts index 86341b73013..1ac907cc779 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts @@ -1,21 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { canonicalizeChord, MODIFIERS } from "./resolveHotkeyFromEvent"; - -/** - * Mirrors the sanitizer in `migrate.ts` so we can unit-test the validation - * rules without mocking tRPC / localStorage. If the migrate.ts rules ever - * diverge from this, update both. - */ -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 parts = canonical.split("+"); - const keys = parts.filter((p) => !MODIFIERS.has(p)); - if (keys.length !== 1) return undefined; - if (!/^[a-z0-9]+$/.test(keys[0])) return undefined; - return canonical; -} +import { sanitizeOverride } from "./sanitizeOverride"; describe("sanitizeOverride (migration validation)", () => { it("preserves an explicit unassignment (null)", () => { @@ -42,7 +26,6 @@ describe("sanitizeOverride (migration validation)", () => { }); it("drops pre-fix `ctrl+control` garbage (no real key)", () => { - // canonicalizes to "ctrl" with no key token expect(sanitizeOverride("ctrl+control")).toBeUndefined(); }); diff --git a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts index 41d530419b4..e2336f26946 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts @@ -108,6 +108,14 @@ describe("canonicalizeChord", () => { 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 { @@ -139,41 +147,40 @@ describe("resolveHotkeyFromEvent — live override index", () => { 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", () => { - // Pick any hotkey with a default key and construct its event. - const firstId = Object.keys(HOTKEYS)[0] as keyof typeof HOTKEYS; - const def = HOTKEYS[firstId].key; - if (!def) return; // skip if the default is unset - const event = buildEventFromChord(def); - expect(resolveHotkeyFromEvent(event)).toBe(firstId); + const event = buildEventFromChord(sampleDef.key); + expect(resolveHotkeyFromEvent(event)).toBe(sampleId); }); it("resolves a rebound chord after an override is saved", () => { - const id = Object.keys(HOTKEYS)[0] as keyof typeof HOTKEYS; useHotkeyOverridesStore.setState({ - overrides: { [id]: "meta+shift+f10" }, + overrides: { [sampleId]: "meta+shift+f10" }, }); const event = buildEventFromChord("meta+shift+f10"); - expect(resolveHotkeyFromEvent(event)).toBe(id); + expect(resolveHotkeyFromEvent(event)).toBe(sampleId); }); it("does NOT resolve the old default after the user rebinds away from it", () => { - const id = Object.keys(HOTKEYS)[0] as keyof typeof HOTKEYS; - const oldDefault = HOTKEYS[id].key; - if (!oldDefault) return; useHotkeyOverridesStore.setState({ - overrides: { [id]: "meta+shift+f10" }, + overrides: { [sampleId]: "meta+shift+f10" }, }); - const event = buildEventFromChord(oldDefault); + const event = buildEventFromChord(sampleDef.key); expect(resolveHotkeyFromEvent(event)).toBeNull(); }); it("does NOT resolve a hotkey the user explicitly unassigned (null override)", () => { - const id = Object.keys(HOTKEYS)[0] as keyof typeof HOTKEYS; - const def = HOTKEYS[id].key; - if (!def) return; - useHotkeyOverridesStore.setState({ overrides: { [id]: null } }); - const event = buildEventFromChord(def); + useHotkeyOverridesStore.setState({ + overrides: { [sampleId]: null }, + }); + const event = buildEventFromChord(sampleDef.key); expect(resolveHotkeyFromEvent(event)).toBeNull(); }); }); diff --git a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts index 4a81d329322..3cf565379d5 100644 --- a/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts +++ b/apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts @@ -88,15 +88,12 @@ export function matchesChord(event: KeyboardEvent, chord: string): boolean { return eventChord === canonicalizeChord(chord); } -/** Sent straight to the PTY. Stored canonical so direct lookups work. */ -export const TERMINAL_RESERVED_CHORDS = new Set([ - "ctrl+c", - "ctrl+d", - "ctrl+z", - "ctrl+s", - "ctrl+q", - "ctrl+backslash", -]); +/** 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, 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; +} From e8819688bdd8c731ee63d56fc51d7461e790f0b8 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 22:18:06 -0700 Subject: [PATCH 09/10] fix(desktop/hotkeys): re-sanitize localStorage for users who migrated pre-fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original guard short-circuited whenever \`hotkey-overrides\` existed in localStorage, which meant any user who ran the migration before the Bug 5 sanitizer shipped would keep their corrupt entries (\`ctrl+control\`, \`ctrl+shift+@\`, \`meta+[\`) forever. Gate migration/sanitization on a separate \`hotkey-overrides-sanitized-v1\` marker instead: - No marker + no store → tRPC migration with sanitizer. - No marker + has store → in-place re-sanitization of existing entries. - Marker present → done, skip. The marker is only set once per user; new bindings written by the fixed recorder can't be corrupt so no further passes are needed. --- apps/desktop/src/renderer/hotkeys/migrate.ts | 86 ++++++++++++++++---- 1 file changed, 69 insertions(+), 17 deletions(-) diff --git a/apps/desktop/src/renderer/hotkeys/migrate.ts b/apps/desktop/src/renderer/hotkeys/migrate.ts index b4407392265..454387aeb27 100644 --- a/apps/desktop/src/renderer/hotkeys/migrate.ts +++ b/apps/desktop/src/renderer/hotkeys/migrate.ts @@ -1,14 +1,25 @@ /** - * One-time migration from the old hotkey storage (main process JSON file via tRPC) - * to the new localStorage-based Zustand store. + * Migrates and sanitizes hotkey overrides on renderer boot. * - * No-op if the new store key already exists (Zustand persist creates it on first init). + * Two paths, gated by a single marker so each runs at most once per user: + * + * 1. Fresh install / never migrated — copy from the old main-process tRPC + * store into localStorage, running each value through `sanitizeOverride`. + * 2. Already migrated on a pre-sanitizer build — re-sanitize whatever is in + * localStorage in place, dropping garbage that the old recorder could + * produce (`ctrl+control`, `ctrl+shift+@`, `meta+[`). Without this pass, + * users who migrated before the sanitizer shipped would keep their + * corrupt entries forever because the original guard short-circuited on + * the presence of the `hotkey-overrides` key. */ import { electronTrpcClient } from "renderer/lib/trpc-client"; import { PLATFORM } from "./registry"; import { sanitizeOverride } from "./utils/sanitizeOverride"; +const STORE_KEY = "hotkey-overrides"; +const SANITIZED_MARKER_KEY = "hotkey-overrides-sanitized-v1"; + const PLATFORM_MAP = { mac: "darwin", windows: "win32", @@ -16,11 +27,45 @@ const PLATFORM_MAP = { } as const; export async function migrateHotkeyOverrides(): Promise { - if (localStorage.getItem("hotkey-overrides")) { - console.log("[hotkeys] Migration skipped — new store already exists"); + if (localStorage.getItem(SANITIZED_MARKER_KEY)) return; + + const existing = localStorage.getItem(STORE_KEY); + if (existing) { + resanitizeExisting(existing); + localStorage.setItem(SANITIZED_MARKER_KEY, "1"); return; } + await migrateFromTrpc(); + localStorage.setItem(SANITIZED_MARKER_KEY, "1"); +} + +function resanitizeExisting(raw: string): void { + try { + const parsed = JSON.parse(raw) as { + state?: { overrides?: Record }; + version?: number; + }; + const current = parsed.state?.overrides ?? {}; + const { cleaned, dropped } = sanitizeAll(current); + localStorage.setItem( + STORE_KEY, + JSON.stringify({ + state: { overrides: cleaned }, + version: parsed.version ?? 0, + }), + ); + if (dropped > 0) { + console.log( + `[hotkeys] Re-sanitized ${Object.keys(cleaned).length} override(s), dropped ${dropped} invalid`, + ); + } + } catch (error) { + console.log("[hotkeys] Re-sanitization failed:", error); + } +} + +async function migrateFromTrpc(): Promise { try { const oldState = await electronTrpcClient.uiState.hotkeys.get.query(); const oldPlatformKey = PLATFORM_MAP[PLATFORM]; @@ -30,19 +75,9 @@ export async function migrateHotkeyOverrides(): Promise { 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; - } - + const { cleaned, dropped } = sanitizeAll(oldOverrides); localStorage.setItem( - "hotkey-overrides", + STORE_KEY, JSON.stringify({ state: { overrides: cleaned }, version: 0 }), ); console.log( @@ -53,3 +88,20 @@ export async function migrateHotkeyOverrides(): Promise { console.log("[hotkeys] Migration failed, starting fresh:", error); } } + +function sanitizeAll(source: Record): { + cleaned: Record; + dropped: number; +} { + const cleaned: Record = {}; + let dropped = 0; + for (const [id, raw] of Object.entries(source)) { + const sanitized = sanitizeOverride(raw); + if (sanitized === undefined) { + dropped++; + continue; + } + cleaned[id] = sanitized; + } + return { cleaned, dropped }; +} From a3275eedf8c07f129d0e5e4ce7047a1543d0a36d Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 12 Apr 2026 22:28:28 -0700 Subject: [PATCH 10/10] fix(desktop/hotkeys): bump migration marker key so pre-sanitizer users re-migrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use a dedicated `hotkey-overrides-migrated-v2` marker separate from the Zustand persist key. Users who ran the migration on the pre-sanitizer build (~1 day window) will re-run once and get their corrupt entries (`ctrl+control`, `ctrl+shift+@`, `meta+[`) dropped. Mark set only on success paths (or when there's nothing to migrate), not in the catch branch — transient tRPC failures at boot retry next launch instead of silently giving up on the user's legacy bindings. --- apps/desktop/src/renderer/hotkeys/migrate.ts | 93 +++++--------------- 1 file changed, 22 insertions(+), 71 deletions(-) diff --git a/apps/desktop/src/renderer/hotkeys/migrate.ts b/apps/desktop/src/renderer/hotkeys/migrate.ts index 454387aeb27..72dbcd95a3f 100644 --- a/apps/desktop/src/renderer/hotkeys/migrate.ts +++ b/apps/desktop/src/renderer/hotkeys/migrate.ts @@ -1,24 +1,16 @@ /** - * Migrates and sanitizes hotkey overrides on renderer boot. + * One-time migration from the old hotkey storage (main process JSON file via tRPC) + * to the new localStorage-based Zustand store. * - * Two paths, gated by a single marker so each runs at most once per user: - * - * 1. Fresh install / never migrated — copy from the old main-process tRPC - * store into localStorage, running each value through `sanitizeOverride`. - * 2. Already migrated on a pre-sanitizer build — re-sanitize whatever is in - * localStorage in place, dropping garbage that the old recorder could - * produce (`ctrl+control`, `ctrl+shift+@`, `meta+[`). Without this pass, - * users who migrated before the sanitizer shipped would keep their - * corrupt entries forever because the original guard short-circuited on - * the presence of the `hotkey-overrides` key. + * 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 STORE_KEY = "hotkey-overrides"; -const SANITIZED_MARKER_KEY = "hotkey-overrides-sanitized-v1"; +const MIGRATION_MARKER_KEY = "hotkey-overrides-migrated-v2"; const PLATFORM_MAP = { mac: "darwin", @@ -27,81 +19,40 @@ const PLATFORM_MAP = { } as const; export async function migrateHotkeyOverrides(): Promise { - if (localStorage.getItem(SANITIZED_MARKER_KEY)) return; - - const existing = localStorage.getItem(STORE_KEY); - if (existing) { - resanitizeExisting(existing); - localStorage.setItem(SANITIZED_MARKER_KEY, "1"); - return; - } - - await migrateFromTrpc(); - localStorage.setItem(SANITIZED_MARKER_KEY, "1"); -} + if (localStorage.getItem(MIGRATION_MARKER_KEY)) return; -function resanitizeExisting(raw: string): void { - try { - const parsed = JSON.parse(raw) as { - state?: { overrides?: Record }; - version?: number; - }; - const current = parsed.state?.overrides ?? {}; - const { cleaned, dropped } = sanitizeAll(current); - localStorage.setItem( - STORE_KEY, - JSON.stringify({ - state: { overrides: cleaned }, - version: parsed.version ?? 0, - }), - ); - if (dropped > 0) { - console.log( - `[hotkeys] Re-sanitized ${Object.keys(cleaned).length} override(s), dropped ${dropped} invalid`, - ); - } - } catch (error) { - console.log("[hotkeys] Re-sanitization failed:", error); - } -} - -async function migrateFromTrpc(): Promise { 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, dropped } = sanitizeAll(oldOverrides); + 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( - STORE_KEY, + "hotkey-overrides", JSON.stringify({ state: { overrides: cleaned }, version: 0 }), ); + localStorage.setItem(MIGRATION_MARKER_KEY, "1"); console.log( `[hotkeys] Migrated ${Object.keys(cleaned).length} override(s)` + (dropped > 0 ? `, dropped ${dropped} invalid` : ""), ); } catch (error) { - console.log("[hotkeys] Migration failed, starting fresh:", error); - } -} - -function sanitizeAll(source: Record): { - cleaned: Record; - dropped: number; -} { - const cleaned: Record = {}; - let dropped = 0; - for (const [id, raw] of Object.entries(source)) { - const sanitized = sanitizeOverride(raw); - if (sanitized === undefined) { - dropped++; - continue; - } - cleaned[id] = sanitized; + // Marker intentionally not set — transient tRPC failures retry next boot. + console.log("[hotkeys] Migration failed, will retry next boot:", error); } - return { cleaned, dropped }; }