refactor(desktop/hotkeys): drop layout-map dispatch, single matchByTypedKey toggle#4124
Open
Kitenite wants to merge 1 commit into
Open
refactor(desktop/hotkeys): drop layout-map dispatch, single matchByTypedKey toggle#4124Kitenite wants to merge 1 commit into
Kitenite wants to merge 1 commit into
Conversation
…pedKey toggle Two prior PRs (#4078, #4091) tried to wire an "Adaptive layout mapping" toggle on top of a layout-translated dispatch path. Tony (Dvorak) hit the toggle stuck on adaptive across both releases and only recovered after a downgrade-then-upgrade cycle. The most likely cause was the tRPC `keyboardLayout.changes` subscription getting wedged: when its `map` stayed null, `useEffectiveLayoutMap` collapsed to null in both toggle states and the toggle effectively did nothing. This drops the entire layout-translation infrastructure and follows VS Code's `keyboard.dispatch: 'code' | 'keyCode'` model — one boolean that picks the event field to match against. Bindings are bare chord strings; the toggle never reshapes them. - `matchByTypedKey: false` (default): match `event.code`. ⌘T fires on physical KeyT on every layout. The QWERTY-position behavior most Electron apps ship with. - `matchByTypedKey: true`: match `event.key.toLowerCase()`. ⌘T fires on whichever key types 't' (physical KeyK on Dvorak). Removed - `main/lib/keyboardLayout.ts` and its tRPC router (native-keymap is no longer a runtime dependency) - `keyboardLayoutStore` (renderer mirror) and its subscription retry - `binding.ts`: `parseBinding`, `serializeBinding`, `bindingToDispatchChord`, `translateLogicalChord`, `BindingMode`, `ParsedBinding`, the `L()` registry helper, and the v2 ShortcutBinding object form - `glyphForCode` / layout-map-aware `formatHotkeyDisplay` — display renders the chord text as authored (⌘T renders as ⌘T everywhere; users who want a different label rebind via Settings) Migrations - `keyboard-preferences` v1 → v2 maps `adaptiveLayoutEnabled` → `matchByTypedKey` so existing users keep their prior choice - `hotkey-overrides` migrate coerces stored v2 binding objects to bare chord strings, so anyone with stuck localStorage from #4091 hydrates clean without the downgrade dance - The whole tRPC sub class of "stuck IPC" failures is gone with the store Recorder honors the toggle: when on, captures `event.key` so newly recorded chords round-trip through dispatch. Named keys (Enter, F-keys, arrows) always use `event.code` since their `event.key` form lower-cases to the same token. Net: 1465 deletions, 568 additions; 75 hotkey tests pass including new Dvorak coverage that locks in both toggle states.
Contributor
📝 WalkthroughWalkthroughThis PR removes native-keymap dependency and simplifies the hotkey system from layout-aware bindings to string-based chords. It replaces keyboard layout detection and adaptive display logic with a user preference (matchByTypedKey) that controls whether hotkeys use physical keys (event.code) or typed characters (event.key). The system now treats bindings as simple strings rather than structured objects. ChangesKeyboard Hotkey System Simplification
Sequence DiagramsequenceDiagram
participant User
participant Renderer as Renderer Process
participant Store as Preferences Store
participant Utils as Chord Utils
participant Display as Display Engine
Note over User,Display: OLD FLOW (Layout-Aware)
User->>Renderer: Press key
Renderer->>Renderer: Load layout map from keyboardLayoutStore
Renderer->>Utils: eventToKeyCode → layout map lookup
Utils->>Renderer: Dispatch chord via layoutMap
Renderer->>Display: formatHotkeyDisplay(binding, platform, layoutMap)
Display->>User: Platform-adapted glyph (layout-specific)
Note over User,Display: NEW FLOW (String-Based)
User->>Renderer: Press key
Renderer->>Store: Check matchByTypedKey preference
Store->>Renderer: Return preference (true/false)
Renderer->>Utils: eventToChord(event, matchByTypedKey)
alt matchByTypedKey = true
Utils->>Utils: Use event.key (typed character)
else matchByTypedKey = false
Utils->>Utils: Use event.code (physical key)
end
Utils->>Renderer: Return chord string (e.g., "meta+z")
Renderer->>Display: formatHotkeyDisplay(chord, platform)
Display->>User: Platform glyph (chord as-authored, no layout adaptation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR replaces the layout-translation infrastructure (tRPC
Confidence Score: 3/5Safe to merge for users on the default physical-key path; enabling the toggle breaks NAVIGATE_BACK and NAVIGATE_FORWARD. The core simplification is clean and migrations are correct, but the apps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.ts — the
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.ts | Introduces strictMatch filter + useKey toggle for react-hotkeys-hook; strictMatch compares eventToChord output (which returns literal chars like [) against canonicalizeChord output (which keeps word forms like bracketleft), silently breaking any punctuation-key binding when matchByTypedKey is on. |
| apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts | Rewires chord resolution to use matchByTypedKey toggle; retains a now-unnecessary subscription to useKeyboardPreferencesStore that was explicitly called out in comments as having no effect on the index. |
| apps/desktop/src/renderer/hotkeys/stores/hotkeyOverridesStore.ts | Adds v2 migration that coerces stored { version: 2, mode, chord } objects to bare chord strings on hydrate; logic is sound. |
| apps/desktop/src/renderer/hotkeys/stores/keyboardPreferencesStore.ts | Replaces adaptiveLayoutEnabled with matchByTypedKey; v1→v2 migration correctly maps the old boolean forward. |
| apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts | Recorder now captures a single chord via chordTerminalForCapture (event.code or event.key depending on toggle); NAMED_TOKENS set is duplicated from the old NAMED_KEYS in binding.ts — logic is correct. |
| apps/desktop/src/renderer/hotkeys/registry.ts | Drops L() helper and v2 ShortcutBinding objects; all entries now use bare chord strings. Mechanical change, correct. |
| apps/desktop/src/renderer/hotkeys/display.ts | Removes layout-map glyph lookup and glyphForCode; formatHotkeyDisplay now renders the chord as authored. Straightforward simplification. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx | Switches from getDispatchChord to getBinding for synthesizing the keyboard event; works for letter chords but inherits the punctuation mismatch bug for any punctuation-keyed binding. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
KE[KeyboardEvent] --> ETC[eventToChord]
ETC --> MTK{matchByTypedKey}
MTK -- false --> CODE[normalizeToken from event.code]
MTK -- true --> TTK[typedKeyToken from event.key]
CODE --> SM[strictMatch filter in useHotkey]
TTK --> SM
SM -- match --> RHH[react-hotkeys-hook callback fires]
SM -- mismatch --> IGNORE[event suppressed]
subgraph Index
OVR[hotkeyOverridesStore] --> BUILD[buildRegisteredAppChords]
BUILD --> IDX[canonicalized chord map]
end
IDX --> RES[resolveHotkeyFromEvent]
ETC --> RES
RES --> TERM[terminal forwarding decision]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.ts:33-40
**`strictMatch` always suppresses `bracketleft`/`bracketright` bindings when `matchByTypedKey` is on**
`eventToChord` uses `typedKeyToken` when `matchByTypedKey = true`, which returns the literal character `"["` for a Cmd+[ press. Meanwhile `canonicalizeChord("meta+bracketleft")` produces `"meta+bracketleft"` (the word form is unchanged by `normalizeToken` because `"bracketleft"` contains `"ket"` not `"key"`). These strings can never be equal, so `strictMatch` returns `true` for every Cmd+[ event and the handler is suppressed unconditionally. `NAVIGATE_BACK` and `NAVIGATE_FORWARD` are silently dead for any user who enables the toggle. The same mismatch applies to all other punctuation chords stored in word form (`slash`, `comma`, `period`, `semicolon`, `backquote`, `backslash`, `minus`, `equal`). The existing test suite only exercises letter/named-key round-trips and does not catch this.
### Issue 2 of 2
apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts:162
**Stale subscription with no effect**
The inline comment on line 150–152 explicitly states that `matchByTypedKey` does not affect the index (registered chords are stored as written; only the event-side conversion in `eventToChord` shifts). Despite that, `useKeyboardPreferencesStore.subscribe(rebuild)` is still wired up, causing a full index rebuild on every preference change for no observable benefit. This is leftover wiring from the old code path.
Reviews (1): Last reviewed commit: "refactor(desktop/hotkeys): drop layout-m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
33
to
+40
| const callerIgnore = options?.ignoreEventWhen; | ||
| // Strict match: react-hotkeys-hook with `useKey: true` matches on either | ||
| // event.key OR event.code (its matcher is additive — see dist/index.js | ||
| // lines 117-131). To get clean "typed character only" semantics we | ||
| // suppress events whose `eventToChord` form (which obeys the toggle) | ||
| // doesn't equal the bound chord. | ||
| const strictMatch = chord | ||
| ? (e: KeyboardEvent) => { |
There was a problem hiding this comment.
strictMatch always suppresses bracketleft/bracketright bindings when matchByTypedKey is on
eventToChord uses typedKeyToken when matchByTypedKey = true, which returns the literal character "[" for a Cmd+[ press. Meanwhile canonicalizeChord("meta+bracketleft") produces "meta+bracketleft" (the word form is unchanged by normalizeToken because "bracketleft" contains "ket" not "key"). These strings can never be equal, so strictMatch returns true for every Cmd+[ event and the handler is suppressed unconditionally. NAVIGATE_BACK and NAVIGATE_FORWARD are silently dead for any user who enables the toggle. The same mismatch applies to all other punctuation chords stored in word form (slash, comma, period, semicolon, backquote, backslash, minus, equal). The existing test suite only exercises letter/named-key round-trips and does not catch this.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.ts
Line: 33-40
Comment:
**`strictMatch` always suppresses `bracketleft`/`bracketright` bindings when `matchByTypedKey` is on**
`eventToChord` uses `typedKeyToken` when `matchByTypedKey = true`, which returns the literal character `"["` for a Cmd+[ press. Meanwhile `canonicalizeChord("meta+bracketleft")` produces `"meta+bracketleft"` (the word form is unchanged by `normalizeToken` because `"bracketleft"` contains `"ket"` not `"key"`). These strings can never be equal, so `strictMatch` returns `true` for every Cmd+[ event and the handler is suppressed unconditionally. `NAVIGATE_BACK` and `NAVIGATE_FORWARD` are silently dead for any user who enables the toggle. The same mismatch applies to all other punctuation chords stored in word form (`slash`, `comma`, `period`, `semicolon`, `backquote`, `backslash`, `minus`, `equal`). The existing test suite only exercises letter/named-key round-trips and does not catch this.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| useHotkeyOverridesStore.subscribe(rebuild); | ||
| useKeyboardLayoutStore.subscribe(rebuild); | ||
| useKeyboardPreferencesStore.subscribe(rebuild); |
There was a problem hiding this comment.
The inline comment on line 150–152 explicitly states that matchByTypedKey does not affect the index (registered chords are stored as written; only the event-side conversion in eventToChord shifts). Despite that, useKeyboardPreferencesStore.subscribe(rebuild) is still wired up, causing a full index rebuild on every preference change for no observable benefit. This is leftover wiring from the old code path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts
Line: 162
Comment:
**Stale subscription with no effect**
The inline comment on line 150–152 explicitly states that `matchByTypedKey` does not affect the index (registered chords are stored as written; only the event-side conversion in `eventToChord` shifts). Despite that, `useKeyboardPreferencesStore.subscribe(rebuild)` is still wired up, causing a full index rebuild on every preference change for no observable benefit. This is leftover wiring from the old code path.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts (1)
148-162: ⚡ Quick win
useKeyboardPreferencesStore.subscribe(rebuild)is a no-op rebuild.The block comment correctly notes the toggle doesn't affect the registered-chord index —
buildRegisteredAppChordsonly readsoverrides. Subscribing the preferences store torebuildtherefore just recomputes the same map on every toggle without changing anything. Drop the second subscription to remove the contradiction with the comment and avoid spurious work.♻️ Suggested fix
useHotkeyOverridesStore.subscribe(rebuild); -useKeyboardPreferencesStore.subscribe(rebuild);If a future change makes the preference participate in index construction, re-add the subscription at that point.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts` around lines 148 - 162, The rebuild function and registeredAppChords are only derived from useHotkeyOverridesStore.overrides via buildRegisteredAppChords, so subscribing the unrelated useKeyboardPreferencesStore to rebuild is a no-op and causes unnecessary recomputation; remove the useKeyboardPreferencesStore.subscribe(rebuild) call (leave useHotkeyOverridesStore.subscribe(rebuild) and the rebuild function intact) and only re-add a subscription to useKeyboardPreferencesStore if/when buildRegisteredAppChords starts reading keyboard preferences.apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts (1)
65-115: 💤 Low valueHarden test isolation: pin
matchByTypedKeyfor the event.code suite.The "chord uses event.code, not event.key" suite assumes
matchByTypedKey === falsebut never asserts/sets it. If another test (or future test file run before this one) leaves the store intrue, assertions likectrl+shift+2would silently change toctrl+shift+@. Cheap to make explicit:♻️ Suggested isolation
describe("captureHotkeyFromEvent — chord uses event.code, not event.key", () => { + let original: boolean; + beforeEach(() => { + original = useKeyboardPreferencesStore.getState().matchByTypedKey; + useKeyboardPreferencesStore.setState({ matchByTypedKey: false }); + }); + afterEach(() => { + useKeyboardPreferencesStore.setState({ matchByTypedKey: original }); + }); + it("Ctrl+Shift+2 chord is ctrl+shift+2 (not ctrl+shift+@)", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts` around lines 65 - 115, The tests in the "chord uses event.code, not event.key" suite assume matchByTypedKey is false but never set; add deterministic pinning by capturing the original value of matchByTypedKey, setting matchByTypedKey = false in a beforeAll for this describe block, and restoring the original value in an afterAll to avoid cross-test leakage; do this near the tests that call captureHotkeyFromEvent and ev so the suite always uses event.code regardless of global test order.apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts (1)
9-23: 💤 Low valueLGTM!
The split between reactive
useBinding(zustand selector closure) and imperativegetBinding(one-shotgetState) is correct. Theid in state.overridescheck correctly preserves explicit unbinds (null) versus falling through to the registry default.Optionally, the resolution body is duplicated; a small helper
resolve(state, id)would let both share the body without changing semantics:♻️ Optional DRY refactor
+function resolve( + state: { overrides: Record<string, string | null> }, + id: HotkeyId, +): string | null { + if (!id) return null; + if (id in state.overrides) return state.overrides[id] ?? null; + return HOTKEYS[id]?.key ?? null; +} + export function useBinding(id: HotkeyId): string | null { - return useHotkeyOverridesStore((state) => { - if (!id) return null; - if (id in state.overrides) return state.overrides[id] ?? null; - return HOTKEYS[id]?.key ?? null; - }); + return useHotkeyOverridesStore((state) => resolve(state, id)); } export function getBinding(id: HotkeyId): string | null { - const state = useHotkeyOverridesStore.getState(); - if (!id) return null; - if (id in state.overrides) return state.overrides[id] ?? null; - return HOTKEYS[id]?.key ?? null; + return resolve(useHotkeyOverridesStore.getState(), id); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts` around lines 9 - 23, Create a small shared resolver to remove the duplicated resolution logic: extract the body that checks for falsy id, tests "id in state.overrides" and returns state.overrides[id] ?? null or HOTKEYS[id]?.key ?? null into a function (e.g., resolveBinding(state, id) or resolve(state, id)), then call that helper from both useBinding (inside the zustand selector) and getBinding (using useHotkeyOverridesStore.getState()) so behavior and semantics (preserving explicit null unbinds) remain identical.apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts (1)
7-9: ⚡ Quick winTighten
idtoHotkeyIdand drop the type cast.
id: stringplusid as Parameters<typeof useBinding>[0]defeats the purpose ofuseBinding'sHotkeyIdconstraint and lets arbitrary strings reachuseBinding. The siblinguseHotkey(line 25) correctly typesid: HotkeyId— please align here so misuse is caught at compile time.♻️ Proposed fix
import { useMemo } from "react"; import { formatHotkeyDisplay } from "../../display"; -import { PLATFORM } from "../../registry"; +import { PLATFORM, type HotkeyId } from "../../registry"; import type { HotkeyDisplay } from "../../types"; import { useBinding } from "../useBinding"; -export function useHotkeyDisplay(id: string): HotkeyDisplay { - const chord = useBinding(id as Parameters<typeof useBinding>[0]); +export function useHotkeyDisplay(id: HotkeyId): HotkeyDisplay { + const chord = useBinding(id); return useMemo(() => formatHotkeyDisplay(chord, PLATFORM), [chord]); }As per coding guidelines: "Avoid
anytype in TypeScript — use explicit type annotations for type safety".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts` around lines 7 - 9, Change the useHotkeyDisplay signature to accept HotkeyId (not string) and remove the cast to Parameters<typeof useBinding>[0]; specifically, update the parameter type on useHotkeyDisplay to HotkeyId so callers are type-checked, then call useBinding(id) directly (no "as" cast) and keep the useMemo call unchanged (formatHotkeyDisplay(chord, PLATFORM)). This aligns useHotkeyDisplay with the sibling useHotkey and ensures useBinding's HotkeyId constraint is enforced at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/hotkeys/types.ts`:
- Around line 31-37: The docstring for ShortcutBinding claims "null means
explicitly unassigned" but the exported alias is just string, causing a doc/type
mismatch; remove the misleading "null means explicitly unassigned" clause from
the comment above ShortcutBinding (keep the type as `string`) and ensure the
null semantics remain only at the usage sites (PlatformKey.*,
HotkeyDefinition.key, StoredBinding) rather than in this alias so code and docs
stay consistent.
In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts`:
- Around line 99-108: typedKeyToken incorrectly returns an empty string for a
single whitespace event.key (e.g., Space) because the current guard rejects lone
whitespace and then normalizeToken trims it to "", so update typedKeyToken to
treat single-character whitespace the same as non-printable keys by returning
codeFallback for single-char keys that are whitespace; in practice, change the
conditional around key.length === 1 to return the lowercase key when it's a
printable non-whitespace char and return codeFallback when it's a single
whitespace character (leave the existing normalizeToken(key) path for multi-char
named keys like "Enter" / "ArrowUp"), referencing the typedKeyToken function and
normalizeToken to locate the logic to change.
In `@apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx`:
- Around line 227-233: Update the copy in the keyboard settings paragraph that
currently says "Use Rebind below..." to reference the actual UI affordance:
change it to mention the "Press a key…" recording control (or the "Press a key…"
control and "Reset" button) instead of "Rebind"; locate the paragraph JSX in the
keyboard settings page and replace the phrase so it accurately points users to
the "Press a key…" recorder below.
---
Nitpick comments:
In `@apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts`:
- Around line 9-23: Create a small shared resolver to remove the duplicated
resolution logic: extract the body that checks for falsy id, tests "id in
state.overrides" and returns state.overrides[id] ?? null or HOTKEYS[id]?.key ??
null into a function (e.g., resolveBinding(state, id) or resolve(state, id)),
then call that helper from both useBinding (inside the zustand selector) and
getBinding (using useHotkeyOverridesStore.getState()) so behavior and semantics
(preserving explicit null unbinds) remain identical.
In
`@apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts`:
- Around line 7-9: Change the useHotkeyDisplay signature to accept HotkeyId (not
string) and remove the cast to Parameters<typeof useBinding>[0]; specifically,
update the parameter type on useHotkeyDisplay to HotkeyId so callers are
type-checked, then call useBinding(id) directly (no "as" cast) and keep the
useMemo call unchanged (formatHotkeyDisplay(chord, PLATFORM)). This aligns
useHotkeyDisplay with the sibling useHotkey and ensures useBinding's HotkeyId
constraint is enforced at compile time.
In
`@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts`:
- Around line 65-115: The tests in the "chord uses event.code, not event.key"
suite assume matchByTypedKey is false but never set; add deterministic pinning
by capturing the original value of matchByTypedKey, setting matchByTypedKey =
false in a beforeAll for this describe block, and restoring the original value
in an afterAll to avoid cross-test leakage; do this near the tests that call
captureHotkeyFromEvent and ev so the suite always uses event.code regardless of
global test order.
In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts`:
- Around line 148-162: The rebuild function and registeredAppChords are only
derived from useHotkeyOverridesStore.overrides via buildRegisteredAppChords, so
subscribing the unrelated useKeyboardPreferencesStore to rebuild is a no-op and
causes unnecessary recomputation; remove the
useKeyboardPreferencesStore.subscribe(rebuild) call (leave
useHotkeyOverridesStore.subscribe(rebuild) and the rebuild function intact) and
only re-add a subscription to useKeyboardPreferencesStore if/when
buildRegisteredAppChords starts reading keyboard preferences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10c960cd-75fb-473c-8f4f-3b744fa78f47
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
apps/desktop/package.jsonapps/desktop/runtime-dependencies.tsapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/keyboardLayout.tsapps/desktop/src/main/lib/keyboardLayout.tsapps/desktop/src/renderer/hotkeys/display.test.tsapps/desktop/src/renderer/hotkeys/display.tsapps/desktop/src/renderer/hotkeys/hooks/index.tsapps/desktop/src/renderer/hotkeys/hooks/useBinding/index.tsapps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.tsapps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.tsapps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.tsapps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.tsapps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.tsapps/desktop/src/renderer/hotkeys/index.tsapps/desktop/src/renderer/hotkeys/registry.test.tsapps/desktop/src/renderer/hotkeys/registry.tsapps/desktop/src/renderer/hotkeys/stores/hotkeyOverridesStore.tsapps/desktop/src/renderer/hotkeys/stores/index.tsapps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.tsapps/desktop/src/renderer/hotkeys/stores/keyboardPreferencesStore.tsapps/desktop/src/renderer/hotkeys/types.tsapps/desktop/src/renderer/hotkeys/utils/binding.test.tsapps/desktop/src/renderer/hotkeys/utils/binding.tsapps/desktop/src/renderer/hotkeys/utils/index.tsapps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.tsapps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsxapps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx
💤 Files with no reviewable changes (10)
- apps/desktop/src/renderer/hotkeys/utils/binding.ts
- apps/desktop/src/renderer/hotkeys/utils/binding.test.ts
- apps/desktop/src/lib/trpc/routers/keyboardLayout.ts
- apps/desktop/src/main/lib/keyboardLayout.ts
- apps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.ts
- apps/desktop/package.json
- apps/desktop/runtime-dependencies.ts
- apps/desktop/src/lib/trpc/routers/index.ts
- apps/desktop/src/renderer/hotkeys/utils/index.ts
- apps/desktop/src/renderer/hotkeys/index.ts
Comment on lines
31
to
+37
| /** | ||
| * How a binding identifies a key: | ||
| * - `logical`: matches the produced character — same printed letter on | ||
| * every layout, even when it lives on different physical keys. Default | ||
| * for shipped registry entries (`⌘Z` always fires on the labeled-Z | ||
| * key) and for new user-recorded printable bindings, when adaptive | ||
| * layout mapping is enabled. | ||
| * - `physical`: matches `event.code` — same physical key on every | ||
| * layout regardless of what's printed on it. Used when adaptive | ||
| * layout mapping is off, or for explicit position-anchored bindings. | ||
| * - `named`: stable named keys (Enter, ArrowUp, F1-F12, ...). Used | ||
| * automatically for non-printable keys regardless of preference. | ||
| * A keyboard shortcut, e.g. `"meta+shift+p"`. `null` means explicitly unassigned. | ||
| * | ||
| * Storage: bare chord string. (Older builds wrote `{ version: 2, mode, chord }` | ||
| * objects; `hotkeyOverridesStore` migrates those to bare strings on hydrate.) | ||
| */ | ||
| export type BindingMode = "physical" | "logical" | "named"; | ||
|
|
||
| /** | ||
| * Stored as a bare chord string for legacy storage (implicitly physical | ||
| * or named, decided by `defaultModeForChord`) or a v2 object for explicit | ||
| * modes. Shipped defaults use the v2 object form for printable chords — | ||
| * see the `L()` helper in `registry.ts`. | ||
| */ | ||
| export type ShortcutBinding = | ||
| | string | ||
| | { | ||
| version: 2; | ||
| mode: BindingMode; | ||
| /** Canonical form, e.g. "meta+shift+p", "ctrl+slash". */ | ||
| chord: string; | ||
| }; | ||
|
|
||
| /** Normalized view of a binding, regardless of stored form. */ | ||
| export interface ParsedBinding { | ||
| mode: BindingMode; | ||
| chord: string; | ||
| } | ||
| export type ShortcutBinding = string; |
Contributor
There was a problem hiding this comment.
ShortcutBinding doc/type mismatch.
The docstring states `null` means explicitly unassigned but the type alias is just string. The null semantics live on the use sites (PlatformKey.*, HotkeyDefinition.key, StoredBinding), not on this alias. Pick one: tighten the doc, or include null in the type.
📝 Suggested fix (drop the misleading null clause)
/**
- * A keyboard shortcut, e.g. `"meta+shift+p"`. `null` means explicitly unassigned.
+ * A keyboard shortcut chord, e.g. `"meta+shift+p"`. Use-sites that allow
+ * "explicitly unassigned" wrap this in `ShortcutBinding | null`.
*
* Storage: bare chord string. (Older builds wrote `{ version: 2, mode, chord }`
* objects; `hotkeyOverridesStore` migrates those to bare strings on hydrate.)
*/
export type ShortcutBinding = string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * How a binding identifies a key: | |
| * - `logical`: matches the produced character — same printed letter on | |
| * every layout, even when it lives on different physical keys. Default | |
| * for shipped registry entries (`⌘Z` always fires on the labeled-Z | |
| * key) and for new user-recorded printable bindings, when adaptive | |
| * layout mapping is enabled. | |
| * - `physical`: matches `event.code` — same physical key on every | |
| * layout regardless of what's printed on it. Used when adaptive | |
| * layout mapping is off, or for explicit position-anchored bindings. | |
| * - `named`: stable named keys (Enter, ArrowUp, F1-F12, ...). Used | |
| * automatically for non-printable keys regardless of preference. | |
| * A keyboard shortcut, e.g. `"meta+shift+p"`. `null` means explicitly unassigned. | |
| * | |
| * Storage: bare chord string. (Older builds wrote `{ version: 2, mode, chord }` | |
| * objects; `hotkeyOverridesStore` migrates those to bare strings on hydrate.) | |
| */ | |
| export type BindingMode = "physical" | "logical" | "named"; | |
| /** | |
| * Stored as a bare chord string for legacy storage (implicitly physical | |
| * or named, decided by `defaultModeForChord`) or a v2 object for explicit | |
| * modes. Shipped defaults use the v2 object form for printable chords — | |
| * see the `L()` helper in `registry.ts`. | |
| */ | |
| export type ShortcutBinding = | |
| | string | |
| | { | |
| version: 2; | |
| mode: BindingMode; | |
| /** Canonical form, e.g. "meta+shift+p", "ctrl+slash". */ | |
| chord: string; | |
| }; | |
| /** Normalized view of a binding, regardless of stored form. */ | |
| export interface ParsedBinding { | |
| mode: BindingMode; | |
| chord: string; | |
| } | |
| export type ShortcutBinding = string; | |
| /** | |
| * A keyboard shortcut chord, e.g. `"meta+shift+p"`. Use-sites that allow | |
| * "explicitly unassigned" wrap this in `ShortcutBinding | null`. | |
| * | |
| * Storage: bare chord string. (Older builds wrote `{ version: 2, mode, chord }` | |
| * objects; `hotkeyOverridesStore` migrates those to bare strings on hydrate.) | |
| */ | |
| export type ShortcutBinding = string; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/hotkeys/types.ts` around lines 31 - 37, The
docstring for ShortcutBinding claims "null means explicitly unassigned" but the
exported alias is just string, causing a doc/type mismatch; remove the
misleading "null means explicitly unassigned" clause from the comment above
ShortcutBinding (keep the type as `string`) and ensure the null semantics remain
only at the usage sites (PlatformKey.*, HotkeyDefinition.key, StoredBinding)
rather than in this alias so code and docs stay consistent.
Comment on lines
+99
to
+108
| /** When `matchByTypedKey` is on, we use the typed character (`event.key`) | ||
| * for printable keys but fall back to the normalized `event.code` for | ||
| * non-printable keys (Enter, ArrowUp, F-keys, …) since `event.key` for | ||
| * those is "Enter" / "ArrowUp" / "F1", which lowercase identically. */ | ||
| function typedKeyToken(event: KeyboardEvent, codeFallback: string): string { | ||
| const key = (event.key ?? "").toLowerCase(); | ||
| if (key.length === 1 && /\S/.test(key)) return key; | ||
| if (key.length > 0) return normalizeToken(key); | ||
| return codeFallback; | ||
| } |
Contributor
There was a problem hiding this comment.
typedKeyToken returns "" for Space when matchByTypedKey is on.
event.key for Space is " " (single whitespace char). The first guard skips it (/\S/.test(" ") is false), so we fall to normalizeToken(" "), where the leading .trim() reduces it to "". The resulting chord becomes e.g. "meta+" and silently fails to match any binding. Not exercised by the current default registry, but reachable through user overrides (e.g., ctrl+space) and the recorder.
🐛 Suggested fix — fall back to codeFallback for single-char whitespace
function typedKeyToken(event: KeyboardEvent, codeFallback: string): string {
const key = (event.key ?? "").toLowerCase();
if (key.length === 1 && /\S/.test(key)) return key;
- if (key.length > 0) return normalizeToken(key);
+ if (key.length > 1) return normalizeToken(key);
return codeFallback;
}This routes Space (and any other lone-whitespace event.key) through the codeFallback ("space"), matching the behavior the surrounding comment promises for non-printable keys.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** When `matchByTypedKey` is on, we use the typed character (`event.key`) | |
| * for printable keys but fall back to the normalized `event.code` for | |
| * non-printable keys (Enter, ArrowUp, F-keys, …) since `event.key` for | |
| * those is "Enter" / "ArrowUp" / "F1", which lowercase identically. */ | |
| function typedKeyToken(event: KeyboardEvent, codeFallback: string): string { | |
| const key = (event.key ?? "").toLowerCase(); | |
| if (key.length === 1 && /\S/.test(key)) return key; | |
| if (key.length > 0) return normalizeToken(key); | |
| return codeFallback; | |
| } | |
| /** When `matchByTypedKey` is on, we use the typed character (`event.key`) | |
| * for printable keys but fall back to the normalized `event.code` for | |
| * non-printable keys (Enter, ArrowUp, F-keys, …) since `event.key` for | |
| * those is "Enter" / "ArrowUp" / "F1", which lowercase identically. */ | |
| function typedKeyToken(event: KeyboardEvent, codeFallback: string): string { | |
| const key = (event.key ?? "").toLowerCase(); | |
| if (key.length === 1 && /\S/.test(key)) return key; | |
| if (key.length > 1) return normalizeToken(key); | |
| return codeFallback; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts` around
lines 99 - 108, typedKeyToken incorrectly returns an empty string for a single
whitespace event.key (e.g., Space) because the current guard rejects lone
whitespace and then normalizeToken trims it to "", so update typedKeyToken to
treat single-character whitespace the same as non-printable keys by returning
codeFallback for single-char keys that are whitespace; in practice, change the
conditional around key.length === 1 to return the lowercase key when it's a
printable non-whitespace char and return codeFallback when it's a single
whitespace character (leave the existing normalizeToken(key) path for multi-char
named keys like "Enter" / "ArrowUp"), referencing the typedKeyToken function and
normalizeToken to locate the logic to change.
Comment on lines
227
to
233
| <p className="text-xs text-muted-foreground"> | ||
| Match shortcuts to the labels on your keyboard (e.g. ⌘Z always fires | ||
| on the key labeled "Z" — physical KeyY on QWERTZ). When off, | ||
| shortcuts are anchored to physical key positions and ignore the | ||
| current input source. | ||
| When on, ⌘T fires on whichever key types "t" on your layout (the | ||
| "t"-labeled key on Dvorak, etc.). When off (default), shortcuts | ||
| match the same physical key on every layout — the QWERTY-position | ||
| behavior most desktop apps ship with. Use Rebind below if neither | ||
| default fits your muscle memory. | ||
| </p> |
Contributor
There was a problem hiding this comment.
Copy nit: "Rebind" doesn't match any button label on this page.
The description tells users to "Use Rebind below if neither default fits your muscle memory," but rows expose a "Press a key…" recording target and a "Reset" button — there is no "Rebind" affordance. Consider rewording for accuracy:
✏️ Suggested wording
- When on, ⌘T fires on whichever key types "t" on your layout (the
- "t"-labeled key on Dvorak, etc.). When off (default), shortcuts
- match the same physical key on every layout — the QWERTY-position
- behavior most desktop apps ship with. Use Rebind below if neither
- default fits your muscle memory.
+ When on, ⌘T fires on whichever key types "t" on your layout (the
+ "t"-labeled key on Dvorak, etc.). When off (default), shortcuts
+ match the same physical key on every layout — the QWERTY-position
+ behavior most desktop apps ship with. Click any shortcut below to
+ rebind it if neither default fits your muscle memory.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <p className="text-xs text-muted-foreground"> | |
| Match shortcuts to the labels on your keyboard (e.g. ⌘Z always fires | |
| on the key labeled "Z" — physical KeyY on QWERTZ). When off, | |
| shortcuts are anchored to physical key positions and ignore the | |
| current input source. | |
| When on, ⌘T fires on whichever key types "t" on your layout (the | |
| "t"-labeled key on Dvorak, etc.). When off (default), shortcuts | |
| match the same physical key on every layout — the QWERTY-position | |
| behavior most desktop apps ship with. Use Rebind below if neither | |
| default fits your muscle memory. | |
| </p> | |
| <p className="text-xs text-muted-foreground"> | |
| When on, ⌘T fires on whichever key types "t" on your layout (the | |
| "t"-labeled key on Dvorak, etc.). When off (default), shortcuts | |
| match the same physical key on every layout — the QWERTY-position | |
| behavior most desktop apps ship with. Click any shortcut below to | |
| rebind it if neither default fits your muscle memory. | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx`
around lines 227 - 233, Update the copy in the keyboard settings paragraph that
currently says "Use Rebind below..." to reference the actual UI affordance:
change it to mention the "Press a key…" recording control (or the "Press a key…"
control and "Reset" button) instead of "Rebind"; locate the paragraph JSX in the
keyboard settings page and replace the phrase so it accurately points users to
the "Press a key…" recorder below.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two prior PRs (#4078, #4091) tried to wire an "Adaptive layout mapping" toggle on top of a layout-translated dispatch path. Tony (Dvorak) hit the toggle stuck on adaptive across both, and only recovered after a downgrade-then-upgrade cycle. Most likely cause: the tRPC
keyboardLayout.changessubscription getting wedged — when itsmapstayednull,useEffectiveLayoutMapcollapsed tonullin both toggle states and the toggle effectively did nothing.This drops the entire layout-translation infrastructure and follows VS Code's
keyboard.dispatch: 'code' | 'keyCode'model — one boolean that picks which event field to match against. Bindings are bare chord strings; the toggle never reshapes them.matchByTypedKey: false(default): matchevent.code. ⌘T fires on physical KeyT on every layout — the QWERTY-position behavior most Electron apps ship with.matchByTypedKey: true: matchevent.key.toLowerCase(). ⌘T fires on whichever key types 't' (physical KeyK on Dvorak).Removed
main/lib/keyboardLayout.tsand its tRPC router —native-keymapis no longer a runtime dependencykeyboardLayoutStore(renderer mirror) and its retry-with-backoff subscriptionbinding.ts:parseBinding,serializeBinding,bindingToDispatchChord,translateLogicalChord,BindingMode,ParsedBinding, theL()registry helper, and the v2 ShortcutBinding object formglyphForCode/ layout-map-awareformatHotkeyDisplay— display renders the chord text as authored (⌘T everywhere; users who want a different label rebind via Settings)Net: 1465 deletions, 568 additions across 30 files.
Migrations (no user action needed)
keyboard-preferencesv1 → v2 mapsadaptiveLayoutEnabled→matchByTypedKeyso existing users keep their prior choicehotkey-overridesmigrate coerces stored v2 binding objects to bare chord strings, so anyone with stuck localStorage from fix(desktop/hotkeys): adaptive-layout toggle now reaches every dispatch consumer #4091 hydrates clean without the downgrade danceRecorder
Honors the toggle: when on, captures
event.keyso newly recorded chords round-trip through dispatch in the same frame. Named keys (Enter, F-keys, arrows) always useevent.codesince theirevent.keyform lower-cases to the same token.Test plan
bun test src/renderer/hotkeys— 75 pass, including new Dvorak coverage ineventToChordandcaptureHotkeyFromEventthat locks in both toggle statesbun run typecheck— cleanbun run lint— cleanRelates to
Summary by cubic
Replaced the layout-mapped hotkey system with a single
matchByTypedKeytoggle that switches matching between physical keys (event.code, default) and typed characters (event.key). This removes the layout map/IPC path, simplifies bindings to plain strings, and resolves the “toggle stuck” issues seen on non-US layouts.Refactors
matchByTypedKeytoggle:event.code(QWERTY-position behavior).event.key.toLowerCase()(follows the typed letter, e.g. Dvorak).native-keymap.event.code.Migration
adaptiveLayoutEnabledis migrated tomatchByTypedKey.Written for commit 726bbd7. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor