feat(desktop): layout-aware keyboard shortcuts + dual-mode bindings#3839
Merged
Conversation
navigator.keyboard.layoutchange doesn't fire for macOS input-source switches; native-keymap hooks the OS-level notification VSCode uses, giving reliable on-the-fly layout updates. Replaces the hand-rolled US-layout fingerprint heuristic with authoritative layout data from main, exposed to the renderer via tRPC observable.
Merge utils/utils.ts into resolveHotkeyFromEvent.ts (one helper, shared deps), route all top-level barrel imports through nested barrels, and flag migrate.ts + MAC_US_DEAD_KEYS for sunset removal once the v1→v2 window closes.
Adds ShortcutBinding versioned shape (string for legacy/physical, v2 object for logical/named) plus parseBinding/serializeBinding/ bindingsEqual helpers. All current bindings still match by event.code — no behavior change. Sets up commit 2 to add layout-aware logical-mode dispatch.
Logical-mode bindings ("the key labeled P, regardless of layout") are
translated through the current keymap to the equivalent event.code-based
chord at registration time. On Dvorak, logical meta+p registers as
meta+r so react-hotkeys-hook's event.code matcher fires when the user
presses physical KeyR (which prints P on Dvorak).
Translation re-runs on layout change for both useHotkey's react-hotkeys-
hook registration and resolveHotkeyFromEvent's reverse index used by
terminal forwarding.
…icts
Recorder now captures both event.code (codeChord) and event.key
(keyChord) plus a classification ("named" | "fkey" | "printable").
F-keys and named keys force "named" mode regardless of preference;
printable bindings honor preferredMode (defaults to "logical" so the
recorded chord follows the printed character).
Settings page passes preferredMode: "logical" and shows a small
"Letter / Position / Named" badge per binding so users can see how
each one matches.
Cross-mode conflict detection translates each binding to its dispatch
chord (event.code form, via the current keymap for logical bindings)
before comparing, so a logical and a physical binding that resolve to
the same physical key on this layout collide as expected.
Mode (physical / logical / named) is an implementation detail. The badge was useful while debugging Phase 2 but adds clutter for users — the displayed glyph and recording behavior already convey what matters.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OS keyboard-layout support: main-process native-keymap reader and change events, a tRPC router/subscription, renderer layout store, new binding types and translation/serialize utilities, layout-aware recording/display/hooks, many tests and docs, and removes legacy sanitizer/migration/detect-US utilities. Changes
Sequence Diagram(s)sequenceDiagram
actor OS
participant NativeKeymap as NativeKeymap
participant Main as Main Process
participant tRPC as tRPC Router
participant Renderer as Renderer
participant Store as Layout Store
OS->>NativeKeymap: keyboard layout change
NativeKeymap-->>Main: onDidChangeKeyboardLayout
Main->>Main: getKeyboardLayoutSnapshot()
Main->>tRPC: emit layout snapshot (changes observable)
tRPC-->>Renderer: subscription payload
Renderer->>Store: applySnapshot(map, layoutId)
Store-->>Renderer: layout state updated
sequenceDiagram
actor User
participant Browser as Browser Event
participant Hook as useHotkey Hook
participant Binding as Binding Utils
participant Layout as KeyboardLayout Store
participant Display as formatHotkeyDisplay
User->>Browser: press key
Browser-->>Hook: KeyboardEvent (key, code)
Hook->>Layout: get layoutMap
Hook->>Binding: bindingToDispatchChord(storedBinding, layoutMap)
alt binding.mode == "logical" and layoutMap present
Binding->>Hook: translated dispatch chord
else
Hook-->>Hook: use physical/named chord
end
Hook->>Display: formatHotkeyDisplay(chord, layoutMap)
Display-->>Hook: formatted display
Hook-->>User: show hotkey UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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)
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 hand-rolled US-layout heuristic with Two P1 bugs affect non-US layout users with logical bindings:
Confidence Score: 4/5Safe to merge for US-layout users; two P1 bugs break logical-binding display and programmatic dispatch on non-US layouts. Core architecture (main-process keymap service, tRPC bridge, binding translation, recorder, conflict detection) is solid and well-tested. Two P1 issues exist for the non-US layout + logical binding combination: useHotkeyDisplay shows wrong glyphs, and SearchBarTrigger programmatic dispatch fails. Both are two-line fixes following the same pattern useHotkey already implements correctly. apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts and apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts | Passes raw logical chord to formatHotkeyDisplay without translating to physical first — shows wrong glyph on non-US layouts for logical bindings (P1) |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx | dispatchHotkeyEvent uses logical chord (code=KeyP on Dvorak) instead of translated physical chord (code=KeyR), programmatic trigger fails for logical bindings on non-US layouts (P1) |
| apps/desktop/src/renderer/hotkeys/utils/binding.ts | New binding utilities (parseBinding, serializeBinding, translateLogicalChord, bindingsEqual) — well-tested; minor inconsistency in named-key early-return and duplicated NAMED_KEYS set |
| apps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.ts | Correctly translates logical bindings via translateLogicalChord before registering with react-hotkeys-hook; adds IME/AltGr suppression via ignoreEventWhen |
| apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts | Captures both codeChord and keyChord; resolveCapturedBinding correctly forces F-key/named to named mode; cross-mode conflict detection via dispatchChordFor is correct |
| apps/desktop/src/main/lib/keyboardLayout.ts | New main-process service wrapping native-keymap — lazy init, snapshot cache, EventEmitter for layout-change notifications, graceful EMPTY fallback on load failure |
| apps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.ts | Process-lifetime Zustand store subscribed to main-process layout changes via tRPC; correctly primed with initial snapshot by the subscription observable |
| apps/desktop/src/lib/trpc/routers/keyboardLayout.ts | New tRPC router exposing get (query) and changes (subscription); subscription primes subscriber with current snapshot; cleanup via returned unsubscribe function is correct |
| apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts | Added IME composition guard, AltGr suppression, logical-binding translation in buildRegisteredAppChords, and isTerminalReservedEvent helper — all correct |
| apps/desktop/src/renderer/hotkeys/migrate.ts | Replaces hand-rolled detectUSLayout with native-keymap query; inlines isUSCompatibleKeymap with SUNSET comment; migration logic unchanged |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts
Line: 7-16
Comment:
**Wrong glyph for logical bindings on non-US layouts**
`parseBinding(binding).chord` for a logical binding (e.g. `"meta+p"`) is passed directly to `formatHotkeyDisplay`. That function calls `glyphForCode("p", layoutMap)`, which resolves to `canonicalToScanCode("p") → "KeyP"`, then does `layoutMap.get("KeyP")`. On Dvorak, `"KeyP"` prints `"l"`, so the settings page shows `"⌘L"` instead of the correct `"⌘P"`. `useHotkey` correctly translates first (`translateLogicalChord("meta+p", dvorakMap) → "meta+r"`, which maps to `"p"` on Dvorak, yielding `"⌘P"`), but `useHotkeyDisplay` skips that step — so the Settings list and every `HotkeyLabel` rendering a logical binding on a non-US layout will show the wrong key glyph.
Fix: resolve the chord the same way `useHotkey` does before passing to `formatHotkeyDisplay`.
```ts
const parsed = binding ? parseBinding(binding) : null;
const chord =
parsed?.mode === "logical"
? (translateLogicalChord(parsed.chord, layoutMap) ?? parsed.chord)
: (parsed?.chord ?? null);
return useMemo(
() => formatHotkeyDisplay(chord, PLATFORM, layoutMap),
[chord, layoutMap],
);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx
Line: 42-47
Comment:
**Programmatic dispatch fires wrong event for logical bindings on non-US layouts**
`dispatchHotkeyEvent(parseBinding(binding).chord)` passes the raw **logical** chord (e.g. `"meta+p"`) to `dispatchHotkeyEvent`, which sets `code = keyToCode("p") = "KeyP"`. On Dvorak, `useHotkey` registers react-hotkeys-hook with the **translated physical** chord `"meta+r"` (via `translateLogicalChord`). Because react-hotkeys-hook matches by `event.code`, the dispatched `code="KeyP"` event does not match the `"KeyR"` registration — the QUICK_OPEN handler never fires when the user clicks the button on a non-US layout.
```ts
const handleClick = useCallback(() => {
const binding = getBinding("QUICK_OPEN");
if (binding) {
const parsed = parseBinding(binding);
const layoutMap = useKeyboardLayoutStore.getState().map;
const chord =
parsed.mode === "logical"
? (translateLogicalChord(parsed.chord, layoutMap) ?? parsed.chord)
: parsed.chord;
dispatchHotkeyEvent(chord);
}
}, []);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hotkeys/utils/binding.ts
Line: 3-25
Comment:
**Duplicate named-key set with `NAMED_CODES` in `useRecordHotkeys.ts`**
`NAMED_KEYS` here and `NAMED_CODES` in `useRecordHotkeys.ts` serve similar roles but are defined independently. They diverge: `NAMED_KEYS` includes aliases (`"return"`, `"esc"`, `"up"`, `"down"`, `"left"`, `"right"`) absent from `NAMED_CODES`. A future edit to one set that misses the other will silently produce inconsistent `defaultModeForChord` / classification results. Consider exporting `NAMED_KEYS` from `binding.ts` and reusing it in `useRecordHotkeys.ts`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hotkeys/utils/binding.ts
Line: 118-120
Comment:
**`translateLogicalChord` returns un-canonicalized chord for named / F-keys**
When the key is in `NAMED_KEYS` or is an F-key, the function returns the original `chord` argument unchanged (preserving input modifier order). For all other keys it returns `parts.join("+")` from a `canonicalizeChord`-sorted split. Callers in `buildRegisteredAppChords` and `dispatchChordFor` always apply `canonicalizeChord` to the result, so there is no observable bug today — but it is a latent footgun for any future caller that compares without canonicalizing.
```ts
// Canonicalize before the early-return too
if (NAMED_KEYS.has(key) || isFunctionKey(key)) return canonicalizeChord(chord);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore(desktop): drop mode badge from key..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts (1)
232-236:⚠️ Potential issue | 🟠 MajorOnly use Backspace/Delete for unassign on the bare keypress.
This branch also catches
meta+Backspace,ctrl+Delete, etc., so those named bindings can never be recorded even though the new binding model supports them. Restrict unassign to the unmodified keypress, or move it to an explicit UI action.Minimal fix
- if (event.key === "Backspace" || event.key === "Delete") { + if ( + (event.key === "Backspace" || event.key === "Delete") && + !event.metaKey && + !event.ctrlKey && + !event.altKey && + !event.shiftKey + ) { setOverride(recordingId, null); optionsRef.current?.onUnassign?.(recordingId); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts` around lines 232 - 236, The unassign branch in useRecordHotkeys is triggered for any Backspace/Delete keypress even when modifiers are held, blocking recording of bindings like Meta+Backspace or Ctrl+Delete; update the condition in the handler that checks event.key === "Backspace" || event.key === "Delete" to also require no modifier keys (e.g. event.metaKey, event.ctrlKey, event.altKey, event.shiftKey are all false) before calling setOverride(recordingId, null) and optionsRef.current?.onUnassign?.(recordingId); ensure this logic lives inside the same function in useRecordHotkeys so only a bare Backspace/Delete unassigns and modified key combos can still be recorded.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts (1)
4-6: Prefer tsconfig alias imports for consistency.These new imports can be switched to alias-based paths to match repository conventions and keep import style uniform.
As per coding guidelines: “
apps/desktop/**/*.{ts,tsx}: Use alias as defined intsconfig.jsonwhen possible”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts` around lines 4 - 6, Update the three imports to use the repo's tsconfig path aliases instead of relative paths: replace the import of useKeyboardLayoutStore, the type HotkeyDisplay, and the parseBinding utility with their aliased module paths (keep the same symbol names: useKeyboardLayoutStore, HotkeyDisplay, parseBinding) so the file useHotkeyDisplay.ts imports via the configured tsconfig aliases rather than "../../stores/keyboardLayoutStore", "../../types", and "../../utils/binding".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/plans/20260427-keyboard-layout-plan.md`:
- Line 4: The branch name in the plan is truncated: replace the broken branch
identifier `keyboard-shortcut-analysi` with the correct, full branch name (e.g.,
`keyboard-shortcut-analysis` or the intended branch) in the document so the
Branch line reflects the true branch name; update the `**Branch:**` value in the
file to the corrected branch string.
- Line 325: The acceptance criteria line "No new runtime deps" contradicts Phase
1 which adds the runtime dependency native-keymap; update the acceptance wording
to reflect the addition (e.g., "Adds native-keymap as a new runtime dependency"
or relax to "Allows one new runtime dep: native-keymap") so Phase 1 and the
acceptance criteria are consistent—search for the phrase "No new runtime deps"
and the Phase 1 section referencing native-keymap to make the edit.
- Around line 227-252: The fenced code blocks in the markdown (showing the
keyboard layout architecture diagram and the listed source files) are missing
language tags and trigger MD040; update each triple-backtick fence to include an
appropriate language (e.g., text or ts) for the blocks that contain plain
diagrams or TypeScript paths. Locate the fences around the diagram and the file
references (mentions of apps/desktop/src/main/lib/keyboardLayout.ts,
apps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.ts,
apps/desktop/src/lib/trpc/routers/keyboardLayout.ts, and display.ts →
formatHotkeyDisplay) and add the language tag after the opening ``` so the
linter recognizes them.
In `@apps/desktop/src/renderer/hotkeys/display.ts`:
- Around line 77-80: The code currently unconditionally calls v.toUpperCase(),
which can expand some single glyphs (e.g., "ß" → "SS") and break keycap
rendering; change the logic to first compute the uppercased form (const upper =
v.toUpperCase()) and only use upper if it remains a single code unit
(upper.length === 1); otherwise fall back to the original single-character glyph
(v) or null as appropriate so you never return a multi-character string for the
key display (update the expression that currently returns v.length === 1 ?
v.toUpperCase() : null).
In
`@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts`:
- Around line 86-96: The code currently treats any single non-whitespace
event.key as a logical token, which allows separator characters like "+" to
produce malformed logical chords (e.g., "meta++"); instead, detect unencodable
glyphs (at minimum the chord separator "+", and any other characters that would
collide with the joiner) when building keyChord from produced and if detected,
revert to physical mode: set keyChord = codeChord and set classification (or the
value used to indicate mode) to "physical". Apply the same change in both places
that build logical keyChord (the block using produced at lines ~86-96 and the
similar block at ~103-111) so resolveCapturedBinding no longer receives
malformed logical bindings.
In `@apps/desktop/src/renderer/hotkeys/migrate.ts`:
- Around line 27-39: The current isUSCompatibleKeymap heuristic is too narrow
and can falsely allow non-US remaps; update the logic in isUSCompatibleKeymap to
either (a) check navigator.keyboard/layoutId or the platform-specific layout
identifier against an explicit allowlist of known US-compatible macOS layouts,
or (b) validate every event.code key that MAC_US_DEAD_KEYS relies on by
comparing unshifted[code] for all keys present in MAC_US_DEAD_KEYS rather than
only KeyA/KeyQ/KeyW/KeyZ/Semicolon/Quote so sanitizeOverride(..., {
assumeUSMacLayout: true }) only runs when the full set matches; reference the
isUSCompatibleKeymap function, MAC_US_DEAD_KEYS, and sanitizeOverride to locate
and change the gate logic.
In `@apps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.ts`:
- Around line 38-43: The subscription error handler currently only logs errors
so the store's map can remain null permanently; change the handler on
electronTrpcClient.keyboardLayout.changes.subscribe to attempt recovery: call
the existing applySnapshot path or a new bootstrap function (e.g.,
bootstrapKeyboardLayout/loadKeyboardLayout) to fetch the latest keyboard layout
once on error, then schedule/resume a re-subscribe retry with backoff (or at
least a fixed retry) so the store's map is set instead of staying null; ensure
the handler replaces the console.error with logic that: (1) invokes the same
snapshot application used on successful onData, (2) triggers a one-time fetch of
keyboardLayout via electronTrpcClient.keyboardLayout.query (or the existing
bootstrap function) to populate map immediately, and (3) re-establishes the
subscription (using electronTrpcClient.keyboardLayout.changes.subscribe and
applySnapshot) with exponential or capped retry to avoid permanent failure.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx`:
- Around line 43-45: The click handler currently takes
parseBinding(binding).chord and dispatches it directly, which fails on non‑US
layouts; instead, after getBinding("QUICK_OPEN") and parseBinding(binding), pass
the parsed chord through the same layout-aware translation used for hotkey
registration (the code path that converts logical binding tokens into dispatch
form / event.code tokens) and then call dispatchHotkeyEvent with that translated
dispatch-chord. Locate where hotkeys are registered (the layout-aware
translation function used there) and reuse it so SearchBarTrigger synthesizes
the same dispatch chord the runtime expects rather than the raw parsed chord.
In `@apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx`:
- Around line 286-289: The conflict dialog is calling
formatHotkeyDisplay(parseBinding(pendingConflict.binding).chord, PLATFORM)
without the layoutMap, causing printable bindings to render as US glyphs; update
that invocation to pass the current layoutMap (e.g., formatHotkeyDisplay(...,
PLATFORM, layoutMap)) so parseBinding/pendingConflict display uses the active
key layout; ensure any other nearby calls in the same reassignment/dialog
rendering (references: formatHotkeyDisplay, parseBinding, pendingConflict,
PLATFORM, HOTKEYS) also receive layoutMap for consistent layout-aware rendering.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts`:
- Around line 232-236: The unassign branch in useRecordHotkeys is triggered for
any Backspace/Delete keypress even when modifiers are held, blocking recording
of bindings like Meta+Backspace or Ctrl+Delete; update the condition in the
handler that checks event.key === "Backspace" || event.key === "Delete" to also
require no modifier keys (e.g. event.metaKey, event.ctrlKey, event.altKey,
event.shiftKey are all false) before calling setOverride(recordingId, null) and
optionsRef.current?.onUnassign?.(recordingId); ensure this logic lives inside
the same function in useRecordHotkeys so only a bare Backspace/Delete unassigns
and modified key combos can still be recorded.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts`:
- Around line 4-6: Update the three imports to use the repo's tsconfig path
aliases instead of relative paths: replace the import of useKeyboardLayoutStore,
the type HotkeyDisplay, and the parseBinding utility with their aliased module
paths (keep the same symbol names: useKeyboardLayoutStore, HotkeyDisplay,
parseBinding) so the file useHotkeyDisplay.ts imports via the configured
tsconfig aliases rather than "../../stores/keyboardLayoutStore", "../../types",
and "../../utils/binding".
🪄 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: a0084a12-af81-4370-9c07-ab4557c63f61
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
apps/desktop/package.jsonapps/desktop/plans/20260427-keyboard-layout-plan.mdapps/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/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/migrate.tsapps/desktop/src/renderer/hotkeys/stores/hotkeyOverridesStore.tsapps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.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/detectUSLayout.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/hotkeys/utils/sanitizeOverride.tsapps/desktop/src/renderer/hotkeys/utils/utils.tsapps/desktop/src/renderer/lib/terminal/line-edit-translations.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 (2)
- apps/desktop/src/renderer/hotkeys/utils/utils.ts
- apps/desktop/src/renderer/hotkeys/utils/detectUSLayout.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 30 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/hotkeys/display.ts">
<violation number="1" location="apps/desktop/src/renderer/hotkeys/display.ts:79">
P3: Uppercasing can expand some single-character layout glyphs (for example `ß` → `SS`), causing multi-character hotkey labels despite the single-glyph guard.</violation>
</file>
<file name="apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts">
<violation number="1" location="apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts:12">
P2: Logical-mode bindings are displayed without layout translation, so displayed keys can be wrong on non-US layouts.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx:45">
P1: The trigger dispatches `parseBinding(binding).chord` directly, which skips logical->dispatch translation and can break Search click activation for logical bindings on non-US keyboard layouts.</violation>
</file>
<file name="apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts">
<violation number="1" location="apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts:92">
P1: The `+` character passes both `length === 1` and `/\S/` checks but will produce a malformed chord like `meta++` since `+` is the separator in chord syntax. This binding can't be round-tripped by `canonicalizeChord` or `parseBinding`. Filter out `+` here and let `keyChord` fall back to `codeChord`.</violation>
<violation number="2" location="apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts:93">
P1: Don't store shifted symbol glyphs as logical chord tokens. Shortcuts like Ctrl+Shift+2 record as `ctrl+shift+@`, which cannot be translated by the unshifted layout map and won't dispatch as the pressed key.</violation>
<violation number="3" location="apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts:168">
P2: Mirror the hotkey registration fallback here. Returning `null` skips conflict detection for logical chords that still register with `parsed.chord`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The conflict dialog called formatHotkeyDisplay without the layout map, so a captured chord shown there used US glyphs even on German/QWERTZ. Added a useFormatChord hook for the "format an arbitrary chord, before it's saved" case and used it here.
…patch
A logical binding records the produced character ("z"), not a scan-code
token. useHotkey was already translating to event.code form before
registering with react-hotkeys-hook, but useHotkeyDisplay /
useFormatChord were passing the literal "z" to the keymap-based glyph
lookup, which treats it as the canonical token for KeyZ — rendering
"⌘Y" on QWERTZ for what should display as "⌘Z".
Extract bindingToDispatchChord into utils/binding.ts and use it from
every site that needs to resolve a binding to its event.code chord
(useHotkey, useHotkeyDisplay, useFormatBinding, conflict detection,
terminal forwarding reverse index). Renames useFormatChord →
useFormatBinding now that it accepts a binding shape rather than a
raw chord.
Manual test plan covering Phase 0–2: smoke regression on US, layout- aware display, logical bindings (the Phase 2 payoff), edge cases (IME, AltGr, terminal forwarding, migration), storage shape, and triage notes for failure modes.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts (1)
85-95:⚠️ Potential issue | 🟠 MajorFall back to physical mode when the logical glyph contains the chord separator.
The check at line 91 allows
"+"through since it's a single non-whitespace character. When a user recordsMeta++(meta plus the plus key),keyChordbecomes"meta++", which cannot be parsed correctly because+is the chord separator. The comment on line 89-90 says "caller falls back to physical," butresolveCapturedBindingdoesn't implement this fallback.Proposed fix
if (classification === "printable") { const produced = (event.key ?? "").toLowerCase(); // Single printable char only — strings like "Dead", "Process" or // multi-char IME output stay on codeChord (caller falls back to // physical when forced into logical mode). - if (produced.length === 1 && /\S/.test(produced)) { + if (produced.length === 1 && /\S/.test(produced) && produced !== "+") { keyChord = [...ordered, produced].join("+"); + } else { + // Unencodable glyph — force physical fallback + keyChord = codeChord; } }And in
resolveCapturedBinding:export function resolveCapturedBinding( captured: CapturedHotkey, preferredMode: "physical" | "logical", ): ParsedBinding { if (captured.classification === "fkey" || captured.classification === "named") return { mode: "named", chord: captured.codeChord }; - const mode: BindingMode = preferredMode; - const chord = mode === "logical" ? captured.keyChord : captured.codeChord; - return { mode, chord }; + // Fall back to physical if keyChord couldn't encode the logical glyph + if (preferredMode === "logical" && captured.keyChord !== captured.codeChord) { + return { mode: "logical", chord: captured.keyChord }; + } + return { mode: "physical", chord: captured.codeChord }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts` around lines 85 - 95, The recorded logical glyph can contain the chord separator "+" (e.g., pressing Meta+Plus), so update useRecordHotkeys: in the printable branch (where produced is derived) ensure produced.length === 1 && /\S/.test(produced) && produced !== "+" (or more generally: does not include the chord separator) before building keyChord, so we don't create ambiguous keyChord strings; and update resolveCapturedBinding to detect when a logical glyph contains the separator ("+") and in that case fall back to using the physical/code-based binding (i.e., treat the capture as non-printable and resolve via the codeChord path). Ensure you reference/use useRecordHotkeys, produced, keyChord, codeChord, classification and resolveCapturedBinding when making these changes.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/hotkeys/utils/binding.test.ts (1)
170-175: Minor: Test name may be misleading.The test is named "preserves modifier order from the input chord" but
canonicalizeChordsorts modifiers alphabetically. The test actually verifies that modifiers are canonicalized correctly (alt+meta+shift+...), not that input order is preserved.Suggested rename
- it("preserves modifier order from the input chord", () => { - // Verify modifiers stay in their input order; canonicalizeChord sorts them + it("canonicalizes modifier order in output chord", () => { + // Verify modifiers are sorted alphabetically by canonicalizeChord const result = translateLogicalChord("alt+meta+shift+p", dvorakMap); expect(result).toBe("alt+meta+shift+r"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/utils/binding.test.ts` around lines 170 - 175, Rename the test to reflect that modifiers are canonicalized rather than preserved; update the test name from "preserves modifier order from the input chord" to something like "canonicalizes modifier order (alphabetical) via canonicalizeChord" and ensure it still calls translateLogicalChord with the same inputs (translateLogicalChord and canonicalizeChord are the relevant symbols) so the assertion expecting "alt+meta+shift+r" remains correct.apps/desktop/src/renderer/hotkeys/utils/binding.ts (1)
4-26: Consider consolidatingNAMED_KEYSwithNAMED_CODESfromuseRecordHotkeys.ts.Both sets serve similar purposes (identifying named/special keys) but have different entries.
NAMED_KEYSincludes aliases like"return","esc","up","down", whileNAMED_CODESdoesn't. If the aliasing is intentional (to handle both raw and normalized tokens), consider extracting a shared constant or documenting the distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/utils/binding.ts` around lines 4 - 26, NAMED_KEYS in binding.ts largely overlaps with NAMED_CODES in useRecordHotkeys.ts but contains aliases ("return", "esc", "up", "down") causing duplication and potential divergence; consolidate by extracting a single shared constant (e.g., NAMED_KEY_SET) that includes all normalized names and aliases or clearly document/justify the alias differences, then update binding.ts's NAMED_KEYS and useRecordHotkeys.ts's NAMED_CODES to import and use that shared constant (or replace one with the other) so both tokenization paths use the same canonical set.apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts (1)
25-34: Consider extractinguseFormatBindingto its own file.The coding guidelines recommend one component per file. While
useFormatBindingis closely related touseHotkeyDisplay, consider extracting it touseFormatBinding.tsfor consistency.As per coding guidelines: "Do not create multi-component files; use one component per file."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts` around lines 25 - 34, Extract the hook useFormatBinding into its own file named useFormatBinding.ts: move the exported function useFormatBinding(binding: ShortcutBinding | null): HotkeyDisplay into the new file and keep its imports (useKeyboardLayoutStore, bindingToDispatchChord, formatHotkeyDisplay, PLATFORM, useMemo) at the top of that file; update the original file to import useFormatBinding from the new module and ensure the function signature and behavior (calling useKeyboardLayoutStore, computing chord with bindingToDispatchChord, and memoizing formatHotkeyDisplay) remain identical so existing callers of useFormatBinding continue to work.apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts (1)
144-155: Consider combining subscriptions to avoid redundant rebuilds.When both stores update in quick succession (e.g., during initialization), this triggers two separate rebuilds. Consider consolidating into a single subscription or debouncing if this becomes a performance concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts` around lines 144 - 155, Currently two separate subscriptions (useHotkeyOverridesStore.subscribe and useKeyboardLayoutStore.subscribe) each call buildRegisteredAppChords and assign registeredAppChords, causing redundant rebuilds; consolidate by creating a single coordinated updater that reads both stores and sets registeredAppChords (e.g., a function updateRegisteredAppChords that calls buildRegisteredAppChords with useHotkeyOverridesStore.getState().overrides and useKeyboardLayoutStore.getState().map) and subscribe that function to both stores, or add a short debounce around updateRegisteredAppChords to coalesce rapid successive updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts`:
- Around line 85-95: The recorded logical glyph can contain the chord separator
"+" (e.g., pressing Meta+Plus), so update useRecordHotkeys: in the printable
branch (where produced is derived) ensure produced.length === 1 &&
/\S/.test(produced) && produced !== "+" (or more generally: does not include the
chord separator) before building keyChord, so we don't create ambiguous keyChord
strings; and update resolveCapturedBinding to detect when a logical glyph
contains the separator ("+") and in that case fall back to using the
physical/code-based binding (i.e., treat the capture as non-printable and
resolve via the codeChord path). Ensure you reference/use useRecordHotkeys,
produced, keyChord, codeChord, classification and resolveCapturedBinding when
making these changes.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts`:
- Around line 25-34: Extract the hook useFormatBinding into its own file named
useFormatBinding.ts: move the exported function useFormatBinding(binding:
ShortcutBinding | null): HotkeyDisplay into the new file and keep its imports
(useKeyboardLayoutStore, bindingToDispatchChord, formatHotkeyDisplay, PLATFORM,
useMemo) at the top of that file; update the original file to import
useFormatBinding from the new module and ensure the function signature and
behavior (calling useKeyboardLayoutStore, computing chord with
bindingToDispatchChord, and memoizing formatHotkeyDisplay) remain identical so
existing callers of useFormatBinding continue to work.
In `@apps/desktop/src/renderer/hotkeys/utils/binding.test.ts`:
- Around line 170-175: Rename the test to reflect that modifiers are
canonicalized rather than preserved; update the test name from "preserves
modifier order from the input chord" to something like "canonicalizes modifier
order (alphabetical) via canonicalizeChord" and ensure it still calls
translateLogicalChord with the same inputs (translateLogicalChord and
canonicalizeChord are the relevant symbols) so the assertion expecting
"alt+meta+shift+r" remains correct.
In `@apps/desktop/src/renderer/hotkeys/utils/binding.ts`:
- Around line 4-26: NAMED_KEYS in binding.ts largely overlaps with NAMED_CODES
in useRecordHotkeys.ts but contains aliases ("return", "esc", "up", "down")
causing duplication and potential divergence; consolidate by extracting a single
shared constant (e.g., NAMED_KEY_SET) that includes all normalized names and
aliases or clearly document/justify the alias differences, then update
binding.ts's NAMED_KEYS and useRecordHotkeys.ts's NAMED_CODES to import and use
that shared constant (or replace one with the other) so both tokenization paths
use the same canonical set.
In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts`:
- Around line 144-155: Currently two separate subscriptions
(useHotkeyOverridesStore.subscribe and useKeyboardLayoutStore.subscribe) each
call buildRegisteredAppChords and assign registeredAppChords, causing redundant
rebuilds; consolidate by creating a single coordinated updater that reads both
stores and sets registeredAppChords (e.g., a function updateRegisteredAppChords
that calls buildRegisteredAppChords with
useHotkeyOverridesStore.getState().overrides and
useKeyboardLayoutStore.getState().map) and subscribe that function to both
stores, or add a short debounce around updateRegisteredAppChords to coalesce
rapid successive updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ebfedc7-85a3-45e7-9df4-e4b8dfecc875
📒 Files selected for processing (12)
apps/desktop/plans/20260428-keyboard-qa-plan.mdapps/desktop/src/renderer/hotkeys/hooks/index.tsapps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.tsapps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/index.tsapps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.tsapps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.tsapps/desktop/src/renderer/hotkeys/index.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.tsapps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/index.ts
- apps/desktop/plans/20260428-keyboard-qa-plan.md
- apps/desktop/src/renderer/hotkeys/hooks/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/hotkeys/utils/index.ts
- apps/desktop/src/renderer/hotkeys/index.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx
- apps/desktop/src/renderer/hotkeys/hooks/useHotkey/useHotkey.ts
Contributor
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/plans/20260428-keyboard-qa-plan.md">
<violation number="1" location="apps/desktop/plans/20260428-keyboard-qa-plan.md:57">
P2: The QA assertion for `⌘,` being layout-invariant is incorrect; on layouts like French AZERTY, comma is not on the same physical key, so the displayed glyph may change.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts (2)
201-205:⚠️ Potential issue | 🟠 MajorDon't use modified Delete/Backspace as the unassign shortcut.
This early return swallows
Meta+Backspace,Ctrl+Delete, etc. beforecaptureHotkeyFromEvent, so those named bindings can never be recorded even thoughNAMED_KEYSandresolveCapturedBindingsupport them. Restrict unassign to the unmodified keys or move it behind capture parsing.Possible fix
- if (event.key === "Backspace" || event.key === "Delete") { + if ( + (event.key === "Backspace" || event.key === "Delete") && + !event.metaKey && + !event.ctrlKey && + !event.altKey && + !event.shiftKey + ) { setOverride(recordingId, null); optionsRef.current?.onUnassign?.(recordingId); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts` around lines 201 - 205, The current early-return treats any Backspace/Delete as an unassign and therefore swallows modified combos like Meta+Backspace or Ctrl+Delete before they reach captureHotkeyFromEvent; change the logic so unassign only triggers for unmodified Backspace/Delete (i.e., require no modifier keys) or alternatively perform captureHotkeyFromEvent first and then treat an unmodified Backspace/Delete binding as an unassign; update the block that calls setOverride(recordingId, null) and optionsRef.current?.onUnassign?.(recordingId) to check modifier keys (event.metaKey, event.ctrlKey, event.altKey, event.shiftKey) or move it after captureHotkeyFromEvent/resolveCapturedBinding/NAMED_KEYS handling so modified Delete/Backspace are allowed to be recorded.
46-60:⚠️ Potential issue | 🟠 MajorCheck for AltGraph before recording ctrl+alt modifier combinations.
The comment on lines 48–49 correctly identifies that AltGr masquerades as
ctrl+alton Linux/international layouts, but the recorder does not actually guard against this. Other parts of the hotkeys system (resolveHotkeyFromEvent.ts,useHotkey.ts) explicitly checkevent.getModifierState?.("AltGraph"), butcaptureHotkeyFromEventdoes not. This causes AltGr key presses to be incorrectly recorded asctrl+altchords. Bail out or suppress thealtmodifier when AltGraph is detected, consistent with the rest of the system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts` around lines 46 - 60, The handler incorrectly records AltGr as ctrl+alt; update the AltGraph handling in this block by using event.getModifierState?.("AltGraph") to detect AltGraph and suppress it: change the altIsAppModifier computation (currently using PLATFORM === "mac" && event.altKey) to take AltGraph into account (e.g., treat event.altKey as not-app-modifier when getModifierState("AltGraph") is true), and when building the modifiers Set (the code that adds "alt") skip adding "alt" if AltGraph is detected (or return null early to bail out if that matches the behavior in resolveHotkeyFromEvent/useHotkey). Ensure the checks reference isFKey, altIsAppModifier, and the modifiers Set so the recorded chord matches the rest of the hotkey system.
♻️ Duplicate comments (3)
apps/desktop/plans/20260427-keyboard-layout-plan.md (2)
4-4:⚠️ Potential issue | 🟡 MinorBranch name appears truncated.
keyboard-shortcut-analysilooks incomplete; verify the full branch name matches the PR branch for traceability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260427-keyboard-layout-plan.md` at line 4, The branch name in the plan file is truncated: replace the string `keyboard-shortcut-analysi` with the full branch name used for the PR (e.g., `keyboard-shortcut-analysis` or the exact PR branch) so the Branch: line accurately matches the source branch for traceability; update the Branch value in the file where `keyboard-shortcut-analysi` appears to the correct full branch name.
227-252:⚠️ Potential issue | 🟡 MinorAdd language tags to fenced code blocks.
The architecture diagram and file path blocks should specify a language (e.g.,
text) to satisfy markdown linting (MD040).🧹 Suggested fix
-``` +```text ┌──────────────────────────────────────────────┐ ... └──────────────────────────────────────────────┘Also applies to lines 266-268, 274-276, and 282-284. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@apps/desktop/plans/20260427-keyboard-layout-plan.mdaround lines 227 - 252,
Add language tags (e.g., "text") to the fenced code blocks containing the ASCII
architecture diagram and the file-path blocks (the block starting with
"┌──────────────────────────────────────────────┐" and the smaller file
path/code blocks that reference apps/desktop/src/... and display.ts →
formatHotkeyDisplay) so markdown lint MD040 is satisfied; update each
triple-backtick fence to ```text for those blocks (also apply the same change to
the other similar fenced blocks mentioned in the comment).</details> </blockquote></details> <details> <summary>apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts (1)</summary><blockquote> `30-36`: _⚠️ Potential issue_ | _🟠 Major_ **Don't keep the `"+"` fallback in logical mode.** When `event.key` is unencodable, `captureHotkeyFromEvent` copies `codeChord` into `keyChord`, but `resolveCapturedBinding` still returns `mode: "logical"`. `Meta+Shift+=` therefore gets stored as logical `meta+shift+equal`, which will later translate as the `=` key on other layouts instead of the `+` glyph the user actually recorded. Represent “no logical chord” explicitly and downgrade those captures to physical. <details> <summary>Possible fix</summary> ```diff export interface CapturedHotkey { /** Modifiers + canonical(event.code). Always meaningful. */ codeChord: string; - /** Modifiers + lowercased event.key for printable letters/digits/punctuation; - * identical to codeChord for named keys / F-keys. */ - keyChord: string; + /** Null when the produced glyph cannot be encoded safely in chord syntax. */ + keyChord: string | null; classification: "named" | "fkey" | "printable"; } @@ - let keyChord = codeChord; + let keyChord: string | null = codeChord; if (classification === "printable") { const produced = (event.key ?? "").toLowerCase(); + keyChord = null; if (produced.length === 1 && /\S/.test(produced) && produced !== "+") { keyChord = [...ordered, produced].join("+"); } } @@ if (captured.classification === "fkey" || captured.classification === "named") return { mode: "named", chord: captured.codeChord }; - const mode: BindingMode = preferredMode; - const chord = mode === "logical" ? captured.keyChord : captured.codeChord; - return { mode, chord }; + if (preferredMode === "logical" && captured.keyChord) { + return { mode: "logical", chord: captured.keyChord }; + } + return { mode: "physical", chord: captured.codeChord }; } ``` </details> Also applies to: 69-79, 86-94 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts` around lines 30 - 36, The code currently copies codeChord into keyChord when event.key is unencodable, but resolveCapturedBinding still returns mode: "logical", causing recorded glyphs like "+" to be misinterpreted as the physical "=" on other layouts; update captureHotkeyFromEvent so that when event.key is unencodable you do NOT set a logical fallback (represent absence of a logical chord explicitly—e.g. set keyChord to null/undefined or a dedicated flag) and ensure resolveCapturedBinding downgrades such captures to mode: "physical" (or returns mode: "physical" when keyChord is empty); change handling around CapturedHotkey.codeChord, CapturedHotkey.keyChord, captureHotkeyFromEvent and resolveCapturedBinding so logical mode is only used when a true logical key is encodable. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@apps/desktop/plans/20260427-keyboard-layout-plan.md:
- Line 32: The documentation currently includes local dev paths (e.g.,
~/workplace/native-keymap/src/keyboard_mac.mm:182-189 and
~/workplace/vscode/src/vs/platform/keyboardLayout/electron-main/keyboardLayoutMainService.ts:48-60)
which are not accessible to others; update the paragraph to replace those local
filesystem references with public GitHub URLs pointing to the equivalent
files/lines or, if exact line links are unstable, use generic repository + file
references (repo name, path, and function/symbol names such as
kTISNotifySelectedKeyboardInputSourceChanged, keyboard_mac.mm, and
keyboardLayoutMainService.ts) so readers can find the implementations without
local paths.
Outside diff comments:
In
@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts:
- Around line 201-205: The current early-return treats any Backspace/Delete as
an unassign and therefore swallows modified combos like Meta+Backspace or
Ctrl+Delete before they reach captureHotkeyFromEvent; change the logic so
unassign only triggers for unmodified Backspace/Delete (i.e., require no
modifier keys) or alternatively perform captureHotkeyFromEvent first and then
treat an unmodified Backspace/Delete binding as an unassign; update the block
that calls setOverride(recordingId, null) and
optionsRef.current?.onUnassign?.(recordingId) to check modifier keys
(event.metaKey, event.ctrlKey, event.altKey, event.shiftKey) or move it after
captureHotkeyFromEvent/resolveCapturedBinding/NAMED_KEYS handling so modified
Delete/Backspace are allowed to be recorded.- Around line 46-60: The handler incorrectly records AltGr as ctrl+alt; update
the AltGraph handling in this block by using
event.getModifierState?.("AltGraph") to detect AltGraph and suppress it: change
the altIsAppModifier computation (currently using PLATFORM === "mac" &&
event.altKey) to take AltGraph into account (e.g., treat event.altKey as
not-app-modifier when getModifierState("AltGraph") is true), and when building
the modifiers Set (the code that adds "alt") skip adding "alt" if AltGraph is
detected (or return null early to bail out if that matches the behavior in
resolveHotkeyFromEvent/useHotkey). Ensure the checks reference isFKey,
altIsAppModifier, and the modifiers Set so the recorded chord matches the rest
of the hotkey system.
Duplicate comments:
In@apps/desktop/plans/20260427-keyboard-layout-plan.md:
- Line 4: The branch name in the plan file is truncated: replace the string
keyboard-shortcut-analysiwith the full branch name used for the PR (e.g.,
keyboard-shortcut-analysisor the exact PR branch) so the Branch: line
accurately matches the source branch for traceability; update the Branch value
in the file wherekeyboard-shortcut-analysiappears to the correct full branch
name.- Around line 227-252: Add language tags (e.g., "text") to the fenced code
blocks containing the ASCII architecture diagram and the file-path blocks (the
block starting with "┌──────────────────────────────────────────────┐" and the
smaller file path/code blocks that reference apps/desktop/src/... and display.ts
→ formatHotkeyDisplay) so markdown lint MD040 is satisfied; update each
triple-backtick fence to ```text for those blocks (also apply the same change to
the other similar fenced blocks mentioned in the comment).In
@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts:
- Around line 30-36: The code currently copies codeChord into keyChord when
event.key is unencodable, but resolveCapturedBinding still returns mode:
"logical", causing recorded glyphs like "+" to be misinterpreted as the physical
"=" on other layouts; update captureHotkeyFromEvent so that when event.key is
unencodable you do NOT set a logical fallback (represent absence of a logical
chord explicitly—e.g. set keyChord to null/undefined or a dedicated flag) and
ensure resolveCapturedBinding downgrades such captures to mode: "physical" (or
returns mode: "physical" when keyChord is empty); change handling around
CapturedHotkey.codeChord, CapturedHotkey.keyChord, captureHotkeyFromEvent and
resolveCapturedBinding so logical mode is only used when a true logical key is
encodable.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `dc987add-6d11-433b-b14b-84bbb898e8a9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 16f0da83e2001f3fc4f366d0f1b03f2dbea34803 and 1326becb2e91173a6f22a44d4ff78a2c4b4f39ee. </details> <details> <summary>📒 Files selected for processing (13)</summary> * `apps/desktop/plans/20260427-keyboard-layout-plan.md` * `apps/desktop/plans/20260428-keyboard-qa-plan.md` * `apps/desktop/src/renderer/hotkeys/display.test.ts` * `apps/desktop/src/renderer/hotkeys/display.ts` * `apps/desktop/src/renderer/hotkeys/hooks/index.ts` * `apps/desktop/src/renderer/hotkeys/hooks/useBinding/index.ts` * `apps/desktop/src/renderer/hotkeys/hooks/useBinding/useBinding.ts` * `apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts` * `apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts` * `apps/desktop/src/renderer/hotkeys/index.ts` * `apps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.ts` * `apps/desktop/src/renderer/hotkeys/utils/binding.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * apps/desktop/src/renderer/hotkeys/hooks/useBinding/index.ts </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (4)</summary> * apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/SearchBarTrigger/SearchBarTrigger.tsx * apps/desktop/src/renderer/hotkeys/hooks/index.ts * apps/desktop/plans/20260428-keyboard-qa-plan.md * apps/desktop/src/renderer/hotkeys/stores/keyboardLayoutStore.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
native-keymap's Linux backend uses libxkbfile and Xlib via pkg-config; node-gyp fails on Ubuntu runners without those headers. Add the apt install before each `bun run install:deps` step on Linux: - ci.yml Test job - ci.yml Build job - build-desktop.yml Build - Linux (x64) macOS jobs are unaffected (native-keymap uses Cocoa frameworks there).
validate:native-runtime flagged native-keymap in the main bundle as an unexpected external require — it wasn't on the runtime allowlist. Adding it to externalizedRuntimeModules wires up everything in one place: keeps the bundle externalization, materializes the module into apps/desktop/node_modules during build, includes the .node binary in the package, and unpacks it from asar so the native binding is loadable at runtime. Same pattern as better-sqlite3 / node-pty.
… doc Three shipped/active plans cover overlapping ground (April refactor retrospective, the dual-mode plan, the QA plan). Replace them with a single canonical reference at apps/desktop/docs/KEYBOARD_SYSTEM.md covering: architecture, public API, binding model, layout-aware translation, recording flow, cross-cutting guards (AltGr / IME / PTY- reserved), brief decision history, and known gaps. Move the three shipped plans to plans/done/ per AGENTS.md convention. 20260409-tui-hotkey-forwarding.md stays in plans/ — v1 terminal migration is still remaining work.
10 tasks
This was referenced May 8, 2026
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
native-keymap(replaces hand-rolled US-layout heuristic) — picks up macOS input-source switches reliably.ShortcutBindingv2): logical (the key labeled "P"), physical (event.code), and named, with translation through the current keymap at registration time so logical bindings work across layouts (e.g. logicalmeta+pregisters asmeta+ron Dvorak).Test plan
Summary by cubic
Make desktop hotkeys layout-aware and add dual‑mode bindings so shortcuts display and fire correctly on any keyboard layout. Removes the old v1→v2 migration and adds reliable live OS layout updates via
native-keymap.New Features
native-keymapin main, streamed to the renderer overtRPC. Shows the printed glyph for each physical key and updates on input‑source changes.ShortcutBindingv2 with modes: physical, logical, and named. Logical chords translate through the current keymap for dispatch and display (e.g., logicalmeta+pregisters/renders asmeta+ron Dvorak). Shipped defaults stay physical; the recorder defaults to logical for printable keys and forces named for F‑keys/special keys.Bug Fixes & Cleanup
native-keymap. v1 string bindings continue to work.KEYBOARD_SYSTEM.md; moved prior plans to done/.native-keymapas a runtime‑external with asar‑unpack, and install Linuxlibx11-dev/libxkbfile-devto fix node‑gyp builds.Written for commit e9b1d20. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores