Conversation
- Rebuild the hotkey reverse index on override store changes so the terminal forwards the user's current bindings instead of frozen defaults. Fixes swallowed-keystroke on rebound-away defaults and dead-binding on new overrides. - Sanitize migrated overrides: canonicalize and drop malformed strings (`ctrl+control`, `ctrl+shift+@`, `meta+[`) that the pre-fix recorder could produce. - Document the meta-on-non-Mac policy (Windows OS intercept, Linux WM ownership).
…ning Drop the blanket reject and extend OS_RESERVED for Windows shell intercepts (meta+d/e/l/r/tab) so users get a warning instead of a silent block. Linux WM configs vary too much to predict — trust the user and let them rebind if a chord doesn't fire.
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:
📝 WalkthroughWalkthroughRecorder, resolver, display, and migration logic unified to use KeyboardEvent.code with shared token normalization and canonical chord formatting; overrides are indexed live and sanitized at migration; terminal-reserved checks and display rendering use canonical forms; tests and plan doc added. Changes
Sequence Diagram(s)sequenceDiagram
participant Keyboard as KeyboardEvent
participant Recorder as Recorder (captureHotkeyFromEvent)
participant Normalizer as normalizeToken / eventToChord
participant Canon as canonicalizeChord
participant Store as HotkeyOverridesStore
participant Builder as buildRegisteredAppChords
participant Resolver as resolveHotkeyFromEvent
participant Terminal as Terminal handler
participant UI as formatHotkeyDisplay
Keyboard->>Recorder: keydown (event.code + modifier flags)
Recorder->>Normalizer: normalizeToken(event.code)
Normalizer->>Canon: canonicalizeChord(tokens)
Canon->>Store: request overrides / check reserved/conflicts
Store->>Builder: overrides changed -> buildRegisteredAppChords(overrides)
Builder->>Resolver: provide live registeredAppChords
Resolver->>Canon: compare canonical forms (matchesChord)
Resolver->>Terminal: isTerminalReservedEvent(event)
Resolver->>UI: resolved chord -> formatHotkeyDisplay
UI-->>Keyboard: render hotkey text
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 fixes five compounding bugs in the desktop keyboard-shortcut recorder and resolver, with the primary user-reported issue being that Ctrl-based chords could not be recorded in Settings → Keyboard. Key changes:
Additional: The blanket Confidence Score: 4/5Safe to merge — all five stated bugs are correctly fixed with 45 new passing tests; only P2 findings remain. The core bug fixes are sound and well-tested. The three findings are all P2: (1)
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Recorder as useRecordHotkeys
participant Cap as captureHotkeyFromEvent
participant Canon as canonicalizeChord
participant Store as useHotkeyOverridesStore
participant Index as registeredAppChords (let)
participant Terminal as terminal attachCustomKeyEventHandler
participant Resolver as resolveHotkeyFromEvent
User->>Recorder: keydown (e.g. Ctrl+Shift+2)
Recorder->>Cap: captureHotkeyFromEvent(event)
Note over Cap: Uses event.code → normalizeToken()<br/>isIgnorableKey() guards lone modifiers
Cap-->>Recorder: "ctrl+shift+2"
Recorder->>Canon: canonicalizeChord("ctrl+shift+2")
Canon-->>Recorder: "ctrl+shift+2" (sorted)
Recorder->>Recorder: checkReserved / getHotkeyConflict (canonicalized)
Recorder->>Store: setOverride(id, "ctrl+shift+2")
Store-->>Index: subscribe callback fires
Index->>Index: buildRegisteredAppChords(overrides)
Note over Index: normalizeChord(override) → sorted keys in map
User->>Terminal: keydown (same chord)
Terminal->>Resolver: resolveHotkeyFromEvent(event)
Resolver->>Resolver: eventToChord() → mods.sort() → "ctrl+shift+2"
Resolver->>Index: registeredAppChords.get("ctrl+shift+2")
Index-->>Resolver: HotkeyId
Resolver-->>Terminal: HotkeyId (forward to app, not swallowed)
Reviews (1): Last reviewed commit: "feat(desktop): allow meta (Win/Super) bi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 4
🧹 Nitpick comments (2)
apps/desktop/src/renderer/hotkeys/migrate.ts (1)
10-34: Extract the sanitizer into a shared, side-effect-free utility.
overrideSanitizer.test.tscurrently has to fork this logic becausesanitizeOverride()is local here. That makes the migration path and its test easy to drift apart, and it also pulls pure chord helpers fromresolveHotkeyFromEvent.ts, which now has module-load store subscription side effects.As per coding guidelines
**/*.test.{ts,tsx}: Co-locate tests with their source files; tests should be in the same directory as the component/utility they test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hotkeys/migrate.ts` around lines 10 - 34, Extract sanitizeOverride into a new side-effect-free utility module and update tests to import it: move the function (including its use of canonicalizeChord and MODIFIERS) out of apps/desktop/src/renderer/hotkeys/migrate.ts into a new file (e.g., hotkeys/sanitizeOverride.ts) that re-exports a pure sanitizeOverride(value: unknown): string | null | undefined; update overrideSanitizer.test.ts to colocate with and import the new utility, and adjust migrate.ts to import sanitizeOverride from the new module so resolveHotkeyFromEvent.ts no longer needs to be imported (avoiding its module-load side effects).apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md (1)
401-403: Pin upstream source links to a commit SHA for doc stability.These citations point to
main, which can drift and make this plan harder to verify later. Prefer permalink URLs pinned to a specific commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md` around lines 401 - 403, Replace the three upstream links that point to "main" with permalinks pinned to a specific commit SHA to ensure doc stability: update the GitHub repository link and the two raw file links (`parseHotkeys.ts` and `useRecordHotkeys.ts`) so they reference the exact commit SHA (e.g., change "/main/" to "/commit/<SHA>/" or use the raw file URL that includes the commit hash) rather than the branch name.
🤖 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/20260412-keyboard-recorder-ctrl-binding-fix.md`:
- Line 165: The snippet that returns null when PLATFORM !== "mac" &&
event.metaKey is contradictory with the later policy allowing Meta on non-Mac
with OS-reserved warnings; update the behavior so the check in the keyboard
handling code (the PLATFORM constant and the event.metaKey gate) either permits
Meta on non-Mac and defers to the new OS-reserved warning flow, or explicitly
mark the existing condition as historical/commented and add a clear note linking
it to the legacy behavior described later (the section that explains allowing
Meta with OS-reserved warnings). Ensure you modify the code branch that
currently short-circuits on event.metaKey (or its surrounding comment) so the
plan is internally consistent with the later policy text.
In
`@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.ts`:
- Around line 22-33: The test stub function ev currently coerces undefined
init.code into an empty string which prevents exercising the undefined code
path; update ev (the KeyboardEvent stub used in useRecordHotkeys.test) to
preserve a missing code field (e.g., set code: init.code as unknown as string |
undefined or omit the defaulting ?? "") so that when tests construct ev({}) the
event.code can be undefined and the undefined guard in the assertion will be
exercised. Ensure only code's defaulting is removed (you can keep other fields'
coercions).
In
`@apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts`:
- Around line 56-67: OS_RESERVED entries are raw literals so checkReserved
compares canonicalized input against non-canonical table entries (e.g.,
"ctrl+alt+delete" vs "alt+ctrl+delete"), so canonicalization never matches;
update the reserved-chord handling by canonicalizing each chord in the
OS_RESERVED map (and the other nearby reserved-chord table) with the same
canonicalization routine used by checkReserved before storing or comparing, or
precompute a canonicalized version of those arrays when the module initializes
so checkReserved can reliably detect reserved chords (refer to OS_RESERVED and
the checkReserved function).
In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.ts`:
- Around line 138-145: The test should not early-return when the first HOTKEYS
entry lacks a default key; instead locate a hotkey with a bound key and assert
it exists before using it. Replace the current first-entry logic with a search
like Object.entries(HOTKEYS).find(([, hotkey]) => hotkey.key) and add an expect
that the found entry is defined, then use its key to build the event with
buildEventFromChord and assert resolveHotkeyFromEvent(event) equals the found
id; apply the same change to the other test block covering lines 156-174 to
avoid silent no-ops.
---
Nitpick comments:
In `@apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md`:
- Around line 401-403: Replace the three upstream links that point to "main"
with permalinks pinned to a specific commit SHA to ensure doc stability: update
the GitHub repository link and the two raw file links (`parseHotkeys.ts` and
`useRecordHotkeys.ts`) so they reference the exact commit SHA (e.g., change
"/main/" to "/commit/<SHA>/" or use the raw file URL that includes the commit
hash) rather than the branch name.
In `@apps/desktop/src/renderer/hotkeys/migrate.ts`:
- Around line 10-34: Extract sanitizeOverride into a new side-effect-free
utility module and update tests to import it: move the function (including its
use of canonicalizeChord and MODIFIERS) out of
apps/desktop/src/renderer/hotkeys/migrate.ts into a new file (e.g.,
hotkeys/sanitizeOverride.ts) that re-exports a pure sanitizeOverride(value:
unknown): string | null | undefined; update overrideSanitizer.test.ts to
colocate with and import the new utility, and adjust migrate.ts to import
sanitizeOverride from the new module so resolveHotkeyFromEvent.ts no longer
needs to be imported (avoiding its module-load side effects).
🪄 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: 4b6691f6-4d2e-4caa-930c-a350bb7aff0a
📒 Files selected for processing (9)
apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.mdapps/desktop/src/renderer/hotkeys/display.test.tsapps/desktop/src/renderer/hotkeys/display.tsapps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.test.tsapps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.tsapps/desktop/src/renderer/hotkeys/migrate.tsapps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.tsapps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.test.tsapps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts
Audit against react-hotkeys-hook and internal usages found two more consumers comparing via event.key, which breaks the same punctuation / layout / rebind cases fixed in the recorder: - utils/utils.ts isTerminalReservedEvent had its own event.key-based TERMINAL_RESERVED set with ctrl+\\. - Terminal/helpers.ts matchesKey used event.key to check the CLEAR_TERMINAL rebind — silently wrong for any punctuation rebind. Consolidation: - Export eventToChord and new matchesChord(event, chord) as the single canonical event↔chord matcher. - Export TERMINAL_RESERVED_CHORDS as the single source of truth. - Rewrite isTerminalReservedEvent around them. - Replace matchesKey() with matchesChord(). - Remove duplicated TERMINAL_RESERVED from useRecordHotkeys.ts. Also adds tests for eventToChord, matchesChord, and isTerminalReservedEvent parity. Updates plan doc with the cleanup.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md (2)
18-18: Clarify Bug 1 fix description for accuracy.The current description "Filter against
"control"(the lowercasedevent.keyname)" could be misleading, as the actual fix switches fromevent.keytoevent.codevianormalizeTokenandisIgnorableKey. Consider rewording to clarify that the solution uses normalizedevent.codetokens, not lowercasedevent.keyvalues.💡 Suggested rewording
-| 1 | Lone Ctrl auto-committed `ctrl+control` before the user pressed key 2 | Filter against `"control"` (the lowercased `event.key` name) + lock keys + altgraph | +| 1 | Lone Ctrl auto-committed `ctrl+control` before the user pressed key 2 | Use `normalizeToken(event.code)` + `isIgnorableKey` to filter modifiers, lock keys, and synthetic events |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md` at line 18, Update the Bug 1 description to state that the fix uses normalized event.code tokens rather than lowercased event.key values: mention normalizeToken and isIgnorableKey are used to convert and filter based on event.code (plus lock keys and AltGraph) instead of filtering against the lowercased event.key string; keep the note that it prevents lone Ctrl auto-commit but replace "Filter against `\"control\"` (the lowercased `event.key` name)" with wording that explicitly references normalized event.code handling via normalizeToken and isIgnorableKey.
153-168: Add language identifier to fenced code block.The code block should specify a language identifier for proper markdown formatting. Since this is a file listing rather than executable code, use
textor leave the language identifier empty.📝 Suggested fix
-``` +```text apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md (this doc) apps/desktop/src/renderer/hotkeys/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md` around lines 153 - 168, In the markdown file apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md update the fenced code block that contains the file listing so it includes a language identifier (use "text" or leave it blank) after the opening triple backticks; locate the triple-backtick block that starts before the line "apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md (this doc)" and change it to "```text" (or "```") to ensure proper markdown formatting.
🤖 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/20260412-keyboard-recorder-ctrl-binding-fix.md`:
- Line 138: Update the mismatched test count string in the markdown: locate the
sentence containing "Everything fixed is in pure functions over primitives. **62
tests across 4" and replace the "62 tests across 4 files" fragment with the
accurate summary ("45 passing tests across four new files") or the verified
current test count; ensure the surrounding phrasing and bold markup remain
consistent with the PR objectives section.
---
Nitpick comments:
In `@apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md`:
- Line 18: Update the Bug 1 description to state that the fix uses normalized
event.code tokens rather than lowercased event.key values: mention
normalizeToken and isIgnorableKey are used to convert and filter based on
event.code (plus lock keys and AltGraph) instead of filtering against the
lowercased event.key string; keep the note that it prevents lone Ctrl
auto-commit but replace "Filter against `\"control\"` (the lowercased
`event.key` name)" with wording that explicitly references normalized event.code
handling via normalizeToken and isIgnorableKey.
- Around line 153-168: In the markdown file
apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md update the
fenced code block that contains the file listing so it includes a language
identifier (use "text" or leave it blank) after the opening triple backticks;
locate the triple-backtick block that starts before the line
"apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md (this doc)"
and change it to "```text" (or "```") to ensure proper markdown formatting.
🪄 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: 9207fafa-72b7-4395-81c5-2f03015b0747
📒 Files selected for processing (1)
apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.md
- Merge canonicalizeChord / normalizeChord into one exported function (the wrapper was pointless indirection). - Drop section-banner comments (`// Helpers`, `// Hook`) and comments that restated code (`// Must include ctrl or meta…`). - Tighten JSDocs to convey intent in one line where possible. - display.ts: drop duplicate short arrow entries (`up: "↑"`) — the normalizeToken call aliases them to canonical names already. Extract isModifier type guard to kill the cast repetition. Net -35 LOC, same behavior, same 62 tests passing.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts (1)
71-74: Consider strengthening the event.code check.The check
event.code === undefinedcatches undefined values, but synthetic events or unusual edge cases might produce an empty string. TheisIgnorableKeyon line 74 handles empty strings via!normalized, so this is safe, but the early return could be more explicit.export function eventToChord(event: KeyboardEvent): string | null { - if (event.code === undefined) return null; + if (!event.code) return null; const key = normalizeToken(event.code); if (isIgnorableKey(key)) return null;🤖 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 71 - 74, The early guard in eventToChord checks only for event.code === undefined which misses empty-string or falsy values; update the guard to treat any falsy or non-string code as invalid (e.g., check if !event?.code or typeof event.code !== 'string') before calling normalizeToken, referencing the eventToChord function and the normalizeToken/isIgnorableKey flow so empty or non-string codes short-circuit consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts`:
- Around line 71-74: The early guard in eventToChord checks only for event.code
=== undefined which misses empty-string or falsy values; update the guard to
treat any falsy or non-string code as invalid (e.g., check if !event?.code or
typeof event.code !== 'string') before calling normalizeToken, referencing the
eventToChord function and the normalizeToken/isIgnorableKey flow so empty or
non-string codes short-circuit consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2654ee11-aec8-4223-9518-f919926af969
📒 Files selected for processing (4)
apps/desktop/src/renderer/hotkeys/display.tsapps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.tsapps/desktop/src/renderer/hotkeys/migrate.tsapps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/hotkeys/hooks/useRecordHotkeys/useRecordHotkeys.ts
…anups Addresses PR review feedback: - Bug: OS_RESERVED[\"windows\"] had \"ctrl+alt+delete\" which never matched because canonicalization sorts modifiers alphabetically. Wrap both OS_RESERVED and TERMINAL_RESERVED_CHORDS in .map(canonicalizeChord) at build time so multi-modifier entries can't silently miss the reserved warning. OS_RESERVED values switched from array to Set. - Extract sanitizeOverride into utils/sanitizeOverride.ts so the test imports the real implementation instead of a near-copy. - Test stub: preserve explicit \`code: undefined\` so the synthetic-event guard in captureHotkeyFromEvent is actually exercised (not the empty-string branch). - Resolver tests: resolve a sample hotkey once at describe scope and throw if HOTKEYS has no bound defaults, instead of each test silently no-op-ing via \`if (!def) return\`. - Add regression test asserting canonicalizeChord(\"ctrl+alt+delete\") sorts to \"alt+ctrl+delete\".
… pre-fix The original guard short-circuited whenever \`hotkey-overrides\` existed in localStorage, which meant any user who ran the migration before the Bug 5 sanitizer shipped would keep their corrupt entries (\`ctrl+control\`, \`ctrl+shift+@\`, \`meta+[\`) forever. Gate migration/sanitization on a separate \`hotkey-overrides-sanitized-v1\` marker instead: - No marker + no store → tRPC migration with sanitizer. - No marker + has store → in-place re-sanitization of existing entries. - Marker present → done, skip. The marker is only set once per user; new bindings written by the fixed recorder can't be corrupt so no further passes are needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (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/src/renderer/hotkeys/migrate.ts">
<violation number="1" location="apps/desktop/src/renderer/hotkeys/migrate.ts:35">
P2: Only mark re-sanitization complete when it actually succeeds; current logic suppresses future retries after a failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…s re-migrate Use a dedicated `hotkey-overrides-migrated-v2` marker separate from the Zustand persist key. Users who ran the migration on the pre-sanitizer build (~1 day window) will re-run once and get their corrupt entries (`ctrl+control`, `ctrl+shift+@`, `meta+[`) dropped. Mark set only on success paths (or when there's nothing to migrate), not in the catch branch — transient tRPC failures at boot retry next launch instead of silently giving up on the user's legacy bindings.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 13, 2026
…on, terminal override respect (superset-sh#3391) * attempt fix * Test and doc * fix(desktop): terminal & migration respect hotkey overrides - Rebuild the hotkey reverse index on override store changes so the terminal forwards the user's current bindings instead of frozen defaults. Fixes swallowed-keystroke on rebound-away defaults and dead-binding on new overrides. - Sanitize migrated overrides: canonicalize and drop malformed strings (`ctrl+control`, `ctrl+shift+@`, `meta+[`) that the pre-fix recorder could produce. - Document the meta-on-non-Mac policy (Windows OS intercept, Linux WM ownership). * feat(desktop): allow meta (Win/Super) bindings on non-Mac with OS warning Drop the blanket reject and extend OS_RESERVED for Windows shell intercepts (meta+d/e/l/r/tab) so users get a warning instead of a silent block. Linux WM configs vary too much to predict — trust the user and let them rebind if a chord doesn't fire. * refactor(desktop): unify event↔chord matching via shared helpers Audit against react-hotkeys-hook and internal usages found two more consumers comparing via event.key, which breaks the same punctuation / layout / rebind cases fixed in the recorder: - utils/utils.ts isTerminalReservedEvent had its own event.key-based TERMINAL_RESERVED set with ctrl+\\. - Terminal/helpers.ts matchesKey used event.key to check the CLEAR_TERMINAL rebind — silently wrong for any punctuation rebind. Consolidation: - Export eventToChord and new matchesChord(event, chord) as the single canonical event↔chord matcher. - Export TERMINAL_RESERVED_CHORDS as the single source of truth. - Rewrite isTerminalReservedEvent around them. - Replace matchesKey() with matchesChord(). - Remove duplicated TERMINAL_RESERVED from useRecordHotkeys.ts. Also adds tests for eventToChord, matchesChord, and isTerminalReservedEvent parity. Updates plan doc with the cleanup. * docs(desktop): rewrite hotkey fix plan for concise review * chore(desktop/hotkeys): deslop — tighten comments, remove dead wrappers - Merge canonicalizeChord / normalizeChord into one exported function (the wrapper was pointless indirection). - Drop section-banner comments (`// Helpers`, `// Hook`) and comments that restated code (`// Must include ctrl or meta…`). - Tighten JSDocs to convey intent in one line where possible. - display.ts: drop duplicate short arrow entries (`up: "↑"`) — the normalizeToken call aliases them to canonical names already. Extract isModifier type guard to kill the cast repetition. Net -35 LOC, same behavior, same 62 tests passing. * fix(desktop/hotkeys): canonicalize reserved-chord tables + review cleanups Addresses PR review feedback: - Bug: OS_RESERVED[\"windows\"] had \"ctrl+alt+delete\" which never matched because canonicalization sorts modifiers alphabetically. Wrap both OS_RESERVED and TERMINAL_RESERVED_CHORDS in .map(canonicalizeChord) at build time so multi-modifier entries can't silently miss the reserved warning. OS_RESERVED values switched from array to Set. - Extract sanitizeOverride into utils/sanitizeOverride.ts so the test imports the real implementation instead of a near-copy. - Test stub: preserve explicit \`code: undefined\` so the synthetic-event guard in captureHotkeyFromEvent is actually exercised (not the empty-string branch). - Resolver tests: resolve a sample hotkey once at describe scope and throw if HOTKEYS has no bound defaults, instead of each test silently no-op-ing via \`if (!def) return\`. - Add regression test asserting canonicalizeChord(\"ctrl+alt+delete\") sorts to \"alt+ctrl+delete\". * fix(desktop/hotkeys): re-sanitize localStorage for users who migrated pre-fix The original guard short-circuited whenever \`hotkey-overrides\` existed in localStorage, which meant any user who ran the migration before the Bug 5 sanitizer shipped would keep their corrupt entries (\`ctrl+control\`, \`ctrl+shift+@\`, \`meta+[\`) forever. Gate migration/sanitization on a separate \`hotkey-overrides-sanitized-v1\` marker instead: - No marker + no store → tRPC migration with sanitizer. - No marker + has store → in-place re-sanitization of existing entries. - Marker present → done, skip. The marker is only set once per user; new bindings written by the fixed recorder can't be corrupt so no further passes are needed. * fix(desktop/hotkeys): bump migration marker key so pre-sanitizer users re-migrate Use a dedicated `hotkey-overrides-migrated-v2` marker separate from the Zustand persist key. Users who ran the migration on the pre-sanitizer build (~1 day window) will re-run once and get their corrupt entries (`ctrl+control`, `ctrl+shift+@`, `meta+[`) dropped. Mark set only on success paths (or when there's nothing to migrate), not in the catch branch — transient tRPC failures at boot retry next launch instead of silently giving up on the user's legacy bindings.
This was referenced Apr 13, 2026
Closed
saddlepaddle
added a commit
that referenced
this pull request
Apr 13, 2026
…workspace Widen PlatformKey and HotkeyDefinition so hotkey entries can register with a null chord per platform. Downstream consumers were already null-safe from #3391 (useBinding, buildRegisteredAppChords, formatHotkeyDisplay, sanitizeOverride, HotkeyMenuShortcut), so the schema widening is the only structural change needed. Re-introduce PREV_TAB, NEXT_TAB, PREV_WORKSPACE, NEXT_WORKSPACE as registered-but-unbound entries so users who want tab/workspace neighbor navigation can rebind them in Settings → Keyboard. PR #3403 removed these to free Cmd+Alt+Arrow for directional pane focus; this restores the hotkey IDs (and their v1/v2 handlers) without claiming any default chord. Users with pre-#3403 overrides for these IDs will transparently get their bindings back since the override is preserved in localStorage. - Null-guard canonicalizeChord(defaultKey) in useRecordHotkeys so recording a new chord for an unbound hotkey no longer throws. - Replace the force-cast in resolveHotkeyFromEvent.test.ts sample picker with a type predicate so sampleDef.key narrows to string honestly instead of lying about the widened schema.
8 tasks
saddlepaddle
added a commit
that referenced
this pull request
Apr 13, 2026
…kspace (#3422) * feat(desktop/hotkeys): allow unbound defaults; restore PREV/NEXT tab+workspace Widen PlatformKey and HotkeyDefinition so hotkey entries can register with a null chord per platform. Downstream consumers were already null-safe from #3391 (useBinding, buildRegisteredAppChords, formatHotkeyDisplay, sanitizeOverride, HotkeyMenuShortcut), so the schema widening is the only structural change needed. Re-introduce PREV_TAB, NEXT_TAB, PREV_WORKSPACE, NEXT_WORKSPACE as registered-but-unbound entries so users who want tab/workspace neighbor navigation can rebind them in Settings → Keyboard. PR #3403 removed these to free Cmd+Alt+Arrow for directional pane focus; this restores the hotkey IDs (and their v1/v2 handlers) without claiming any default chord. Users with pre-#3403 overrides for these IDs will transparently get their bindings back since the override is preserved in localStorage. - Null-guard canonicalizeChord(defaultKey) in useRecordHotkeys so recording a new chord for an unbound hotkey no longer throws. - Replace the force-cast in resolveHotkeyFromEvent.test.ts sample picker with a type predicate so sampleDef.key narrows to string honestly instead of lying about the widened schema. * fix(desktop/hotkeys): restore prevIndex in v2 PREV_WORKSPACE handler
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 14, 2026
…kspace (superset-sh#3422) * feat(desktop/hotkeys): allow unbound defaults; restore PREV/NEXT tab+workspace Widen PlatformKey and HotkeyDefinition so hotkey entries can register with a null chord per platform. Downstream consumers were already null-safe from superset-sh#3391 (useBinding, buildRegisteredAppChords, formatHotkeyDisplay, sanitizeOverride, HotkeyMenuShortcut), so the schema widening is the only structural change needed. Re-introduce PREV_TAB, NEXT_TAB, PREV_WORKSPACE, NEXT_WORKSPACE as registered-but-unbound entries so users who want tab/workspace neighbor navigation can rebind them in Settings → Keyboard. PR superset-sh#3403 removed these to free Cmd+Alt+Arrow for directional pane focus; this restores the hotkey IDs (and their v1/v2 handlers) without claiming any default chord. Users with pre-superset-sh#3403 overrides for these IDs will transparently get their bindings back since the override is preserved in localStorage. - Null-guard canonicalizeChord(defaultKey) in useRecordHotkeys so recording a new chord for an unbound hotkey no longer throws. - Replace the force-cast in resolveHotkeyFromEvent.test.ts sample picker with a type predicate so sampleDef.key narrows to string honestly instead of lying about the widened schema. * fix(desktop/hotkeys): restore prevIndex in v2 PREV_WORKSPACE handler
5 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
Fixes the user-reported bug that the keyboard settings recorder wouldn't let you bind shortcuts starting with Ctrl, plus four related issues found during the dig.
Bugs fixed
ctrl+control—event.keyfor the Control key lowercases to"control", not"ctrl", so the pure-modifier filter missed it and immediately saved a broken chord before the user could press the second key.event.keywhile registry + dispatch useevent.code. Shift+digit, Alt+letter on Mac, and non-US layouts silently recorded unmatchable strings (ctrl+shift+@vsctrl+shift+2,alt+¬vsalt+l). Unified onevent.codevia sharednormalizeTokenhelper.===, someta+alt+up≠meta+alt+arrowupeven though they're the same chord. AddedcanonicalizeChordand applied at all three sites.REGISTERED_APP_CHORDSwas a module-load-time constant built from defaults only, so after any rebind the terminal would (a) swallow the old default into the void and (b) let xterm consume the new binding. Now rebuilds on every override-store change.Related decisions
OS_RESERVEDfor Windows with common shell intercepts (meta+d/e/l/r/tab) so users get a warning instead of a silent block.modalias: skipped — the registry already uses per-platform{mac, windows, linux}bindings.See
apps/desktop/plans/20260412-keyboard-recorder-ctrl-binding-fix.mdfor the full write-up with upstreamreact-hotkeys-hooksource citations and MDN references.Tests
45 passing tests across 4 new files covering pure helpers (
normalizeToken,canonicalizeChord,isIgnorableKey,formatHotkeyDisplay), recorder capture (captureHotkeyFromEvent), live override-aware resolver, and migration sanitizer.Test plan
ctrl+shift+2(notctrl+shift+@)meta+bracketleft(notmeta+[)Summary by cubic
Fixes Ctrl-based shortcut recording and unifies hotkey matching across record/resolve/display; the terminal now respects live overrides. Allows Meta (Win/Super) bindings on Windows/Linux with an OS-reserved warning, and re-migrates/sanitizes old overrides to drop corrupt entries.
Bug Fixes
event.code, ignore lone modifiers/lock keys, and compare via a canonical chord; arrows/punctuation render correctly; reserved lists canonicalized (e.g.,alt+ctrl+deletewarns) and includectrl+backslash.hotkey-overrides-migrated-v2so users who migrated pre-sanitizer re-run once; canonicalizes overrides, preservesnull(unassign), and drops malformed strings; marker set only on success or when nothing to migrate.Refactors
eventToChord,matchesChord,canonicalizeChord,normalizeToken,TERMINAL_RESERVED_CHORDS);OS_RESERVEDswitched to canonicalizedSets; rewroteisTerminalReservedEventand replaced terminalmatchesKeywithmatchesChord.formatHotkeyDisplay, add punctuation/arrow mappings, treatcontrolasctrl; tightened comments/JSDocs.Written for commit a3275ee. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation
Migration
Tests