fix(desktop/hotkeys): adaptive-layout toggle now reaches every dispatch consumer#4091
Merged
Conversation
…h consumer The "Adaptive layout mapping" toggle in #4078 was wired into the resolver index, the display hooks, and `getDispatchChord`, but missed the actual hotkey registration in `useHotkey` and the recorder's conflict detector. Worse, every shipped registry default was a bare-string (physical) chord, which `bindingToDispatchChord` short-circuits — so flipping the toggle did nothing for the 75 built-in shortcuts regardless of any other fix. Net result: a Dvorak / QWERTZ user pressing the labeled-T key for `⌘T` saw the menu update but no hotkey fire. - Route every layout-map consumer through a single `useEffectiveLayoutMap` / `getEffectiveLayoutMap` chokepoint that gates by `adaptiveLayoutEnabled`. Adds a banner on `keyboardLayoutStore` warning future contributors not to read the raw store. - Re-author printable defaults in `registry.ts` as `mode: "logical"` via an `L()` helper so the toggle actually moves them on non-US layouts. Named-key chords (arrows, Enter, Backspace, Tab) stay as bare strings since they're layout-stable. - Flip `adaptiveLayoutEnabled` to default `true` to match macOS / VS Code / Cursor convention. Existing users who explicitly persisted a value keep it (zustand persist is write-on-set). - Update the keyboard settings copy to describe the toggle in user-facing terms ("match shortcuts to printed key labels" vs "anchor to physical positions"). - Add `registry.test.ts` locking in the logical-default authoring convention so a future PR can't silently revert printable chords to bare strings. - `apps/desktop/plans/20260505-hotkey-adaptive-toggle-smoke-test.md` documents the manual smoke test for non-US layouts.
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR implements an adaptive layout toggle for desktop hotkeys, defaulting enabled. A new ChangesAdaptive Layout Toggle Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
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 Summary
Confidence Score: 4/5Safe to merge — fix is correct and complete; only P2 documentation inconsistencies remain All five dispatch consumers are properly unified through the new chokepoint, the default flip is safe with persist middleware, and the regression test locks in the convention. The only findings are P2 documentation issues: a stale JSDoc in useBinding.ts, a warning banner that slightly overstates what's forbidden (import vs. raw map read), and a smoke-test doc that still says a helper needs to be added as a follow-up when it was already added in this PR. apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts (direct useKeyboardLayoutStore import contradicts new banner), apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts (stale JSDoc)
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/hotkeys/stores/keyboardPreferencesStore.ts | Adds useEffectiveLayoutMap / getEffectiveLayoutMap as the single chokepoint for toggle-gated layout access; flips default to true; correctly persists only adaptiveLayoutEnabled |
| apps/desktop/src/renderer/hotkeys/registry.ts | All 75 printable chord defaults migrated from bare strings to L() (v2 logical objects); layout-stable keys (arrows, Enter, F-keys) correctly left as bare strings |
| apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts | Resolver index now rebuilt via shared rebuild() helper using getEffectiveLayoutMap(); still directly imports useKeyboardLayoutStore for subscription (contradicts new banner warning) |
| apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts | Correctly routes getDispatchChord through getEffectiveLayoutMap(); JSDoc on useBinding still describes defaults as bare strings (now stale) |
| apps/desktop/src/renderer/hotkeys/registry.test.ts | New regression suite locks in logical-mode authoring convention for printable defaults and includes canary checks for letter, digit, and punctuation shapes |
| apps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.ts | Replaces raw useKeyboardLayoutStore read with useEffectiveLayoutMap(); registration now correctly gates on toggle state |
| apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts | Removes local useActiveLayoutMap helper and unifies both display hooks through useEffectiveLayoutMap() |
| apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts | Recorder conflict detector now reads through getEffectiveLayoutMap(); toggle state is correctly reflected on each keypress |
| apps/desktop/src/renderer/hotkeys/types.ts | PlatformKey and HotkeyDefinition.key correctly widened from string to ShortcutBinding; BindingMode doc reordered to reflect logical-first convention |
| apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts | Updated to extract chord from ShortcutBinding via parseBinding so tests work correctly now that defaults are v2 objects instead of bare strings |
| apps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.ts | Adds warning banner directing consumers to use useEffectiveLayoutMap instead of reading the store directly; no functional change |
| apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx | UI copy for Adaptive layout mapping clarified from technical jargon to user-facing language describing label-matching behaviour |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[KeyboardEvent] --> B{adaptiveLayoutEnabled?}
B -- "true (default)" --> C[getEffectiveLayoutMap\nreturns OS layout map]
B -- "false" --> D[getEffectiveLayoutMap\nreturns null]
C --> E[bindingToDispatchChord\ntranslates logical chord\nthrough OS map]
D --> F[bindingToDispatchChord\nfalls back to authored chord\nphysical mode]
E --> G[Dispatch consumers]
F --> G
G --> H[useHotkey\nregistration]
G --> I[resolveHotkeyFromEvent\nreverse index]
G --> J[useHotkeyDisplay\ndisplay hooks]
G --> K[getDispatchChord\nimperative dispatch]
G --> L[getHotkeyConflict\nrecorder detector]
Comments Outside Diff (2)
-
apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts, line 7-9 (link)Stale JSDoc after default-mode migration
The
useBindingJSDoc still says "bare chord string (legacy / shipped defaults, treated as physical mode)" — but every shipped default is now a v2 logical object viaL(). A developer reading this comment will get a wrong mental model of whatHOTKEYS[id].keyreturns.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts Line: 7-9 Comment: **Stale JSDoc after default-mode migration** The `useBinding` JSDoc still says "bare chord string (legacy / shipped defaults, treated as physical mode)" — but every shipped default is now a v2 logical object via `L()`. A developer reading this comment will get a wrong mental model of what `HOTKEYS[id].key` returns. How can I resolve this? If you propose a fix, please make it concise.
-
apps/desktop/plans/20260505-hotkey-adaptive-toggle-smoke-test.md, line 134-136 (link)Lines 134–136 say "consider a
useEffectiveLayoutMap()helper as a follow-up to make this unmissable," butuseEffectiveLayoutMap(and its imperative siblinggetEffectiveLayoutMap) are both implemented and exported in this very PR. The suggestion reads as if the helper still needs to be created.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/plans/20260505-hotkey-adaptive-toggle-smoke-test.md Line: 134-136 Comment: **Stale "follow-up" suggestion — helper already shipped in this PR** Lines 134–136 say "consider a `useEffectiveLayoutMap()` helper as a follow-up to make this unmissable," but `useEffectiveLayoutMap` (and its imperative sibling `getEffectiveLayoutMap`) are both implemented and exported in this very PR. The suggestion reads as if the helper still needs to be created. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts:7-9
**Stale JSDoc after default-mode migration**
The `useBinding` JSDoc still says "bare chord string (legacy / shipped defaults, treated as physical mode)" — but every shipped default is now a v2 logical object via `L()`. A developer reading this comment will get a wrong mental model of what `HOTKEYS[id].key` returns.
### Issue 2 of 3
apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts:3
**Direct `useKeyboardLayoutStore` import contradicts the new banner warning**
`keyboardLayoutStore.ts` now warns "Do not import this store directly from dispatch / display / recorder code." The resolver still imports it directly — albeit only to call `useKeyboardLayoutStore.subscribe(rebuild)`, not to read `.getState().map`. The warning should clarify "do not read `.map` directly" rather than "do not import," or the subscription should be encapsulated (e.g. a `subscribeToEffectiveLayoutMap` helper in `keyboardPreferencesStore`). As written, a future developer sees the import, reads the banner, and may incorrectly treat the resolver as a violation to be "fixed" by removing the subscription — which would silently break live layout-switch reactivity.
### Issue 3 of 3
apps/desktop/plans/20260505-hotkey-adaptive-toggle-smoke-test.md:134-136
**Stale "follow-up" suggestion — helper already shipped in this PR**
Lines 134–136 say "consider a `useEffectiveLayoutMap()` helper as a follow-up to make this unmissable," but `useEffectiveLayoutMap` (and its imperative sibling `getEffectiveLayoutMap`) are both implemented and exported in this very PR. The suggestion reads as if the helper still needs to be created.
Reviews (1): Last reviewed commit: "fix(desktop/hotkeys): make adaptive-layo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1,19 +1,13 @@ | |||
| import { HOTKEYS, type HotkeyId } from "../registry"; | |||
| import { useHotkeyOverridesStore } from "../stores/hotkeyOverridesStore"; | |||
| import { useKeyboardLayoutStore } from "../stores/keyboardLayoutStore"; | |||
There was a problem hiding this comment.
useKeyboardLayoutStore import contradicts the new banner warning
keyboardLayoutStore.ts now warns "Do not import this store directly from dispatch / display / recorder code." The resolver still imports it directly — albeit only to call useKeyboardLayoutStore.subscribe(rebuild), not to read .getState().map. The warning should clarify "do not read .map directly" rather than "do not import," or the subscription should be encapsulated (e.g. a subscribeToEffectiveLayoutMap helper in keyboardPreferencesStore). As written, a future developer sees the import, reads the banner, and may incorrectly treat the resolver as a violation to be "fixed" by removing the subscription — which would silently break live layout-switch reactivity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts
Line: 3
Comment:
**Direct `useKeyboardLayoutStore` import contradicts the new banner warning**
`keyboardLayoutStore.ts` now warns "Do not import this store directly from dispatch / display / recorder code." The resolver still imports it directly — albeit only to call `useKeyboardLayoutStore.subscribe(rebuild)`, not to read `.getState().map`. The warning should clarify "do not read `.map` directly" rather than "do not import," or the subscription should be encapsulated (e.g. a `subscribeToEffectiveLayoutMap` helper in `keyboardPreferencesStore`). As written, a future developer sees the import, reads the banner, and may incorrectly treat the resolver as a violation to be "fixed" by removing the subscription — which would silently break live layout-switch reactivity.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
pushed a commit
that referenced
this pull request
May 6, 2026
…h consumer (#4091) The "Adaptive layout mapping" toggle in #4078 was wired into the resolver index, the display hooks, and `getDispatchChord`, but missed the actual hotkey registration in `useHotkey` and the recorder's conflict detector. Worse, every shipped registry default was a bare-string (physical) chord, which `bindingToDispatchChord` short-circuits — so flipping the toggle did nothing for the 75 built-in shortcuts regardless of any other fix. Net result: a Dvorak / QWERTZ user pressing the labeled-T key for `⌘T` saw the menu update but no hotkey fire. - Route every layout-map consumer through a single `useEffectiveLayoutMap` / `getEffectiveLayoutMap` chokepoint that gates by `adaptiveLayoutEnabled`. Adds a banner on `keyboardLayoutStore` warning future contributors not to read the raw store. - Re-author printable defaults in `registry.ts` as `mode: "logical"` via an `L()` helper so the toggle actually moves them on non-US layouts. Named-key chords (arrows, Enter, Backspace, Tab) stay as bare strings since they're layout-stable. - Flip `adaptiveLayoutEnabled` to default `true` to match macOS / VS Code / Cursor convention. Existing users who explicitly persisted a value keep it (zustand persist is write-on-set). - Update the keyboard settings copy to describe the toggle in user-facing terms ("match shortcuts to printed key labels" vs "anchor to physical positions"). - Add `registry.test.ts` locking in the logical-default authoring convention so a future PR can't silently revert printable chords to bare strings. - `apps/desktop/plans/20260505-hotkey-adaptive-toggle-smoke-test.md` documents the manual smoke test for non-US layouts.
10 tasks
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
PR #4078 added an "Adaptive layout mapping" toggle but it didn't actually do anything for shipped shortcuts. Two stacked defects, plus a default-direction call:
useHotkey(the hook that wiresreact-hotkeys-hook) and the recorder's conflict detector read the layout map unconditionally. The toggle moved labels in Settings; it never moved which key fired the action.bindingToDispatchChordshort-circuits physical mode, so the layout map was never consulted regardless of toggle state — the 75 built-in shortcuts ignored the preference completely.adaptiveLayoutEnabled: falseshipped as the default, locking non-US users into physical-position dispatch as the out-of-the-box experience.User report context: a Dvorak user (Tony, in #ext-superset-aytuncext-mastra) installed the v1.8.1 release with the toggle, found that flipping it changed the Settings labels but not which keys actually fired, and confirmed the symptom for every shortcut.
Changes
useEffectiveLayoutMap()/getEffectiveLayoutMap()inkeyboardPreferencesStorethat gates byadaptiveLayoutEnabled. All five consumers (registration, resolver, display hooks, imperative dispatch, recorder conflict detector) route through it. Banner onkeyboardLayoutStorewarns against reading the raw store directly — that pattern was the root cause of feat(desktop): toggle to disable adaptive keyboard-layout mapping #4078's miss.registry.tsasmode: "logical"via a newL()helper. Named-key chords (arrows / Enter / Backspace / Tab) stay as bare strings — they're layout-stable.⌘T, regardless of layout. Existing users who explicitly persisted a value keep it; users who never touched the toggle pick up the new default on upgrade.registry.test.ts) locks in the logical-default authoring convention so a future PR can't silently revert printable chords to bare strings.apps/desktop/plans/20260505-hotkey-adaptive-toggle-smoke-test.md) documents the manual flow for non-US layouts.Migration story is clean: US users see no behavior change at all (identity layout map). Non-US users with no override get behavior closer to OS convention. Non-US users with custom overrides keep them.
Test plan
Verified manually on Dvorak with QWERTY-labeled hardware (Tests 1–7 in the smoke-test doc).
bun run typecheckpassesbunx biome check apps/desktop/src/renderer/hotkeyspassesbun test apps/desktop/src/renderer/hotkeys/registry.test.tspasses⌘Tfires on the K-cap (where "t" lives on Dvorak)⌘Tfires on the T-cap (physical KeyT, types "y" on Dvorak)Ctrl+Creaches PTY, app hotkeys don't leakFollow-ups (not blocking)
bun testsuite for hotkeys is broken at module init via a transitiveelectronTrpcClientimport (pre-existing on main). Worth a separate ticket so we can write a realuseHotkeyintegration test next time.Summary by CodeRabbit
New Features
Improvements