feat(desktop): toggle to disable adaptive keyboard-layout mapping#4078
Merged
Conversation
Adds an opt-in "Adaptive layout mapping" preference on the keyboard settings page. When off (default), shortcuts dispatch and display exactly as bound regardless of the OS input source — i.e. ⌘Z stays ⌘Z on QWERTZ instead of being remapped to physical KeyY. The preference is persisted in localStorage and read by the resolver index, the display hooks (useHotkeyDisplay, useFormatBinding), and the imperative getDispatchChord, all of which pass null for the layout map when adaptive is off.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new persisted keyboard preference toggles "adaptive layout mapping". Multiple hotkey hooks, utilities, and the settings UI now read this preference and conditionally apply the keyboard layout map (or ChangesAdaptive Layout Mapping
Sequence DiagramsequenceDiagram
actor User
participant Settings as Settings UI
participant Store as Keyboard<br/>Preferences Store
participant Hooks as Hotkey Hooks<br/>(useBinding, useHotkeyDisplay)
participant Utils as Hotkey Utils<br/>(resolveHotkeyFromEvent)
participant Display as Hotkey Display
User->>Settings: Toggle adaptive layout mapping
Settings->>Store: setAdaptiveLayoutEnabled(enabled)
Store->>Store: Persist to localStorage
activate Hooks
Hooks->>Store: Subscribe to adaptiveLayoutEnabled changes
Hooks->>Hooks: Derive layoutMap via useActiveLayoutMap()
deactivate Hooks
activate Utils
Utils->>Store: Subscribe to adaptiveLayoutEnabled changes
Utils->>Utils: Rebuild registeredAppChords index<br/>using activeLayoutMap()
deactivate Utils
rect rgba(100, 200, 100, 0.5)
Note over Hooks,Display: When adaptiveLayoutEnabled = true
Hooks->>Display: Pass current keyboard layout map
Display->>Display: Adapt hotkey display to layout
end
rect rgba(200, 100, 100, 0.5)
Note over Hooks,Display: When adaptiveLayoutEnabled = false
Hooks->>Display: Pass null (no layout map)
Display->>Display: Preserve authored hotkey chords
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 — no correctness bugs; two P2 style/performance findings only. All three code paths (resolver index, display hooks, imperative dispatch) consistently gate layout translation on the preference, and the store subscription correctly rebuilds state on toggle. Only P2 findings: unnecessary layout-store subscription when adaptive is off, and a deep import path in page.tsx. apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts — layout store subscription is active even when adaptive is disabled.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/hotkeys/stores/keyboardPreferencesStore.ts | New Zustand store with persist middleware, correctly partializing to only persist adaptiveLayoutEnabled (excludes the setter). Clean implementation. |
| apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts | Adds useActiveLayoutMap to gate layout translation on the preference; both hooks updated correctly, but the layout store is subscribed unconditionally causing unnecessary re-renders when adaptive is off. |
| apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts | Adds a useKeyboardPreferencesStore.subscribe listener to rebuild registeredAppChords on preference toggle; logic is symmetric with the existing overrides and layout subscribers. |
| apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts | getDispatchChord correctly reads the preference and passes null as the layout map when adaptive is off. |
| apps/desktop/src/renderer/hotkeys/stores/index.ts | Adds useKeyboardPreferencesStore to the stores barrel export — straightforward and correct. |
| apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx | Adds the preferences toggle UI and wires it to the store; imports useKeyboardPreferencesStore via a deep file path instead of the available renderer/hotkeys/stores barrel. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Pref[KeyboardPreferencesStore\nadaptiveLayoutEnabled] -->|subscribe| Resolver
Pref -->|useKeyboardPreferencesStore| UseActiveLayoutMap
LayoutStore[KeyboardLayoutStore\nmap] -->|subscribe| Resolver
LayoutStore -->|useKeyboardLayoutStore| UseActiveLayoutMap
OverridesStore[HotkeyOverridesStore\noverrides] -->|subscribe| Resolver
UseActiveLayoutMap{adaptive?\nlayoutMap : null} -->|layoutMap| UseHotkeyDisplay
UseActiveLayoutMap -->|layoutMap| UseFormatBinding
Resolver[resolveHotkeyFromEvent\nregisteredAppChords] -->|rebuild on any store change| ChordIndex[(chord → HotkeyId)]
GetDispatchChord[getDispatchChord\nimperative] -->|reads getState| Pref
GetDispatchChord -->|reads getState| LayoutStore
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/useHotkeyDisplay/useHotkeyDisplay.ts:12-16
**Unnecessary layout-store subscription when adaptive is off**
`useActiveLayoutMap` always subscribes to `useKeyboardLayoutStore`, so every keyboard-layout change (e.g. user switching input sources) triggers a re-render of every `useHotkeyDisplay`/`useFormatBinding` consumer even when adaptive layout is disabled and the return value is always `null`. Moving the selector condition inside the store subscription avoids these phantom re-renders.
```suggestion
function useActiveLayoutMap(): ReadonlyMap<string, string> | null {
const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled);
const layoutMap = useKeyboardLayoutStore((s) => (adaptive ? s.map : null));
return layoutMap;
}
```
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx:29
**Direct deep import instead of stores barrel**
`useKeyboardPreferencesStore` is now re-exported from `renderer/hotkeys/stores` (via the updated `stores/index.ts`), but the import here bypasses that barrel and reaches directly into the file. The rest of this file uses the top-level `renderer/hotkeys` barrel for related stores — using `renderer/hotkeys/stores` here would be more consistent and keeps internal file layout opaque to consumers.
```suggestion
import { useKeyboardPreferencesStore } from "renderer/hotkeys/stores";
```
Reviews (1): Last reviewed commit: "feat(desktop): add toggle to disable ada..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+12
to
+16
| function useActiveLayoutMap(): ReadonlyMap<string, string> | null { | ||
| const layoutMap = useKeyboardLayoutStore((s) => s.map); | ||
| const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled); | ||
| return adaptive ? layoutMap : null; | ||
| } |
There was a problem hiding this comment.
useActiveLayoutMap always subscribes to useKeyboardLayoutStore, so every keyboard-layout change (e.g. user switching input sources) triggers a re-render of every useHotkeyDisplay/useFormatBinding consumer even when adaptive layout is disabled and the return value is always null. Moving the selector condition inside the store subscription avoids these phantom re-renders.
Suggested change
| function useActiveLayoutMap(): ReadonlyMap<string, string> | null { | |
| const layoutMap = useKeyboardLayoutStore((s) => s.map); | |
| const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled); | |
| return adaptive ? layoutMap : null; | |
| } | |
| function useActiveLayoutMap(): ReadonlyMap<string, string> | null { | |
| const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled); | |
| const layoutMap = useKeyboardLayoutStore((s) => (adaptive ? s.map : null)); | |
| return layoutMap; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts
Line: 12-16
Comment:
**Unnecessary layout-store subscription when adaptive is off**
`useActiveLayoutMap` always subscribes to `useKeyboardLayoutStore`, so every keyboard-layout change (e.g. user switching input sources) triggers a re-render of every `useHotkeyDisplay`/`useFormatBinding` consumer even when adaptive layout is disabled and the return value is always `null`. Moving the selector condition inside the store subscription avoids these phantom re-renders.
```suggestion
function useActiveLayoutMap(): ReadonlyMap<string, string> | null {
const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled);
const layoutMap = useKeyboardLayoutStore((s) => (adaptive ? s.map : null));
return layoutMap;
}
```
How can I resolve this? If you propose a fix, please make it concise.| useHotkeyOverridesStore, | ||
| useRecordHotkeys, | ||
| } from "renderer/hotkeys"; | ||
| import { useKeyboardPreferencesStore } from "renderer/hotkeys/stores/keyboardPreferencesStore"; |
There was a problem hiding this comment.
useKeyboardPreferencesStore is now re-exported from renderer/hotkeys/stores (via the updated stores/index.ts), but the import here bypasses that barrel and reaches directly into the file. The rest of this file uses the top-level renderer/hotkeys barrel for related stores — using renderer/hotkeys/stores here would be more consistent and keeps internal file layout opaque to consumers.
Suggested change
| import { useKeyboardPreferencesStore } from "renderer/hotkeys/stores/keyboardPreferencesStore"; | |
| import { useKeyboardPreferencesStore } from "renderer/hotkeys/stores"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx
Line: 29
Comment:
**Direct deep import instead of stores barrel**
`useKeyboardPreferencesStore` is now re-exported from `renderer/hotkeys/stores` (via the updated `stores/index.ts`), but the import here bypasses that barrel and reaches directly into the file. The rest of this file uses the top-level `renderer/hotkeys` barrel for related stores — using `renderer/hotkeys/stores` here would be more consistent and keeps internal file layout opaque to consumers.
```suggestion
import { useKeyboardPreferencesStore } from "renderer/hotkeys/stores";
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…barrel Match how useHotkeyOverridesStore is exposed so the keyboard settings page can import both stores from the same module.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
9 tasks
Kitenite
added a commit
that referenced
this pull request
May 5, 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.
saddlepaddle
pushed a commit
that referenced
this pull request
May 6, 2026
) * feat(desktop): add toggle to disable adaptive keyboard-layout mapping Adds an opt-in "Adaptive layout mapping" preference on the keyboard settings page. When off (default), shortcuts dispatch and display exactly as bound regardless of the OS input source — i.e. ⌘Z stays ⌘Z on QWERTZ instead of being remapped to physical KeyY. The preference is persisted in localStorage and read by the resolver index, the display hooks (useHotkeyDisplay, useFormatBinding), and the imperative getDispatchChord, all of which pass null for the layout map when adaptive is off. * refactor(hotkeys): export useKeyboardPreferencesStore from top-level barrel Match how useHotkeyOverridesStore is exposed so the keyboard settings page can import both stores from the same module.
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
⌘Zstays⌘Zon QWERTZ instead of being remapped to physicalKeyY.useHotkeyDisplay/useFormatBinding), and imperativegetDispatchChordall read the preference and passnullfor the layout map when adaptive is off; resolver subscribes so toggling rebuilds the chord index live.Test plan
⌘Zfires the bound handler and renders as⌘Z(not⌘Y)bun run lintand existing hotkey tests still greenSummary by cubic
Adds an opt-in “Adaptive layout mapping” toggle in Keyboard settings to control layout-aware shortcuts. When off (default), shortcuts dispatch and display exactly as bound (e.g., ⌘Z stays ⌘Z on QWERTZ).
New Features
localStorage(zustandpersist).useHotkeyDisplay,useFormatBinding, andgetDispatchChordpass a null layout map when disabled.Refactors
useKeyboardPreferencesStorefrom the hotkeys barrel to simplify imports.Written for commit eb30829. Summary will update on new commits.
Summary by CodeRabbit