feat(desktop/hotkeys): v1 directional pane focus + best-effort v1 override migrator#3460
Conversation
Restores keyboard pane navigation to v1 workspaces with the same Cmd+Alt+Arrow chords v2 uses, via a standalone spatial neighbor walker over v1's MosaicNode<string> tree (no cross-package coupling to v2). Also lets the recorder accept alt-only chords on Mac and warns when bound alt-only chords could mask typing special characters.
The v1 recorder stored chords from event.key (layout-dependent raw glyphs like "meta+,", "ctrl+shift+@", "meta+alt+ª"), while the new store matches on event.code names. Extends sanitizeOverride with a token rewrite pre-pass covering US-ANSI punctuation, shifted glyphs, and macOS Option dead-keys; gates the Option dead-key table behind a navigator.keyboard-based US-layout probe so non-US Mac users aren't silently rebound to the wrong physical key.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThese changes enhance hotkey handling on macOS by detecting keyboard layouts, supporting Mac Option modifier for hotkeys, and improving sanitization of override strings. Additionally, directional pane navigation with spatial awareness is introduced alongside corresponding hotkeys. Changes
Sequence DiagramsequenceDiagram
actor User
participant WorkspacePage as Workspace Page
participant Focus as Focus Handler
participant Spatial as Spatial Navigator
participant Store as Focus Store
User->>WorkspacePage: Press hotkey (e.g., Ctrl+Left)
WorkspacePage->>Focus: moveFocusDirectional("left")
Focus->>Spatial: getSpatialNeighborMosaicPaneId(root, currentPaneId, "left")
Spatial->>Spatial: Locate mosaic path to currentPaneId
Spatial->>Spatial: Traverse ancestor splits aligned with left axis
Spatial->>Spatial: Descend perpendicular split to select edge pane
Spatial-->>Focus: Return neighbor paneId or null
Focus->>Store: setFocusedPane(neighborPaneId)
Store->>WorkspacePage: Update focused pane state
WorkspacePage->>User: Render focus highlight on neighbor pane
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR delivers two well-scoped hotkey improvements for the desktop app: directional pane focus for v1 workspaces (matching the v2 behaviour added in #3403) and a best-effort v1 override migrator that handles the Key changes:
Confidence Score: 4/5Safe to merge; the only open item is a minor UX gap where The spatial navigation algorithm is correct (traced through multiple 2×2 layouts manually), the migrator is well-tested against real production data with 64 passing parametric cases, type-check and lint are clean, and the one-time migration marker design handles transient failures gracefully. The only gap is the recorder's No files require special attention beyond the P2 note on Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App boot] --> B{migration marker set?}
B -- yes --> Z[skip]
B -- no --> C[fetch v1 overrides via tRPC]
C --> D{any overrides?}
D -- no --> E[set marker, log skip]
D -- yes --> F{PLATFORM === mac?}
F -- yes --> G[isUSCompatibleLayout\ngetLayoutMap probe]
F -- no --> H[assumeUSMacLayout = true]
G --> I{US-compatible?}
I -- yes --> H
I -- no --> J[assumeUSMacLayout = false]
H & J --> K[sanitizeOverride per entry]
K --> L{result?}
L -- string --> M[include in cleaned map]
L -- null --> M
L -- undefined --> N[drop / increment dropped counter]
M & N --> O[write to localStorage hotkey-overrides]
O --> P[set migration marker]
subgraph sanitizeOverride
K1[split by +] --> K2[ALWAYS_SAFE_REWRITES]
K2 --> K3{assumeUSMacLayout?}
K3 -- yes --> K4[MAC_US_DEAD_KEYS]
K3 -- no --> K5[pass through]
K4 & K5 --> K6[canonicalizeChord]
K6 --> K7{single non-modifier matching a-z0-9+?}
K7 -- yes --> K8[return canonical]
K7 -- no --> K9[return undefined]
end
|
| const keys = canonical.split("+").filter((p) => !MODIFIERS.has(p)); | ||
| if (keys.length !== 1 || !/^[a-z0-9]+$/.test(keys[0])) return undefined; | ||
| return canonical; |
There was a problem hiding this comment.
Regex rejects multi-word code names used outside the rewrite tables
The final guard !/^[a-z0-9]+$/.test(keys[0]) passes for all names currently produced by the rewrite tables (bracketleft, arrowup, pagedown, etc.). However, if a v1 chord ever stored a hyphenated or otherwise non-alphanumeric code name (e.g. a future key like numpad-decimal from an obscure layout), it would be silently dropped even though canonicalizeChord might have accepted it. The current table coverage makes this a theoretical concern, but a comment explaining why [a-z0-9]+ is sufficient given the known code-name vocabulary would help the next person extending ALWAYS_SAFE_REWRITES.
…rride migrator (superset-sh#3460) * feat(desktop/hotkeys): directional pane focus for v1 + Mac alt modifier Restores keyboard pane navigation to v1 workspaces with the same Cmd+Alt+Arrow chords v2 uses, via a standalone spatial neighbor walker over v1's MosaicNode<string> tree (no cross-package coupling to v2). Also lets the recorder accept alt-only chords on Mac and warns when bound alt-only chords could mask typing special characters. * feat(desktop/hotkeys): best-effort migrator for v1 event.key overrides The v1 recorder stored chords from event.key (layout-dependent raw glyphs like "meta+,", "ctrl+shift+@", "meta+alt+ª"), while the new store matches on event.code names. Extends sanitizeOverride with a token rewrite pre-pass covering US-ANSI punctuation, shifted glyphs, and macOS Option dead-keys; gates the Option dead-key table behind a navigator.keyboard-based US-layout probe so non-US Mac users aren't silently rebound to the wrong physical key.
Two blocking issues Codex found on the conflict resolution: 1. apps/desktop/src/renderer/hotkeys/migrate.ts — the fork guard that short-circuited when localStorage already had a hotkey-overrides entry bypassed upstream superset-sh#3460's new sanitizeOverride / detectUSLayout logic. The whole point of the `-v2` marker bump is to re-sanitize existing localStorage overrides that a pre-sanitizer build wrote with corrupt values, so skipping them defeats the migration. Restructure: instead of an early return, parse the existing localStorage overrides, re-run them through sanitizeOverridesMap with the US-layout probe, and write the cleaned map back. Still avoids overwriting with stale tRPC data (the fork's original intent). 2. apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/ $workspaceId/page.tsx — the v1 FOCUS_PANE_{LEFT,RIGHT,UP,DOWN} hotkeys were missing `enabled: isActive`. Every other useHotkey in this file already has that guard because KeepAliveWorkspaces keeps inactive WorkspacePage instances mounted; without the guard, a user who rebinds the FOCUS_PANE_* ids would see the hotkey fire in hidden background workspaces as well.
chore(upstream): PR5 hotkeys — v1 directional pane focus + Cmd+Alt+Arrow restoration (superset-sh#3460 superset-sh#3472)
-s ours merge to record that upstream commits a3e34bf through de70163 (13 commits) are semantically already present on origin/main via the PR1-6 cherry-pick series (PRs #176, #177, #178, #179, #180, #182), plus fork-adaptation fixes layered on top. This merge target is de70163 specifically (not upstream/main) so newer upstream commits (9fff075 and later) remain visible in future behind counts. Upstream commits covered by this audit merge: - a3e34bf fix(desktop): restore cmd+click requirement for v1 terminal file links (superset-sh#3457) [PR1/#176] - 57557f8 fix(desktop): gate v2 workspace children on collection readiness (superset-sh#3464) [PR1/#176] - 4ee2e61 fix(desktop): use native clipboard for copy path in v2 sidebar (superset-sh#3462) [PR1/#176] - 87d6e93 feat(desktop): close settings with Escape key (superset-sh#3466) [PR1/#176] - 9c7f5f4 chore(desktop): auto-restart host-service on bundle change in dev (superset-sh#3461) [PR1/#176] - 93140d9 fix(mcp): accept MCP resource URL as valid OAuth audience (superset-sh#3459) [PR2/#177] - be9e000 fix(desktop): drive tray menu off events, fetch real org name (superset-sh#3458) [PR2/#177] - c5f791e feat(v2): unify workspace delete through host-service (superset-sh#3443) [PR3/#178] - 2c24d93 feat(desktop): paginated branch picker with checkout + open actions (superset-sh#3397) [PR4/#179] - 2bf1049 feat(desktop/hotkeys): v1 directional pane focus + best-effort v1 override migrator (superset-sh#3460) [PR5/#180] - 1294a7d feat(desktop/hotkeys): restore Cmd+Alt+Arrow for tab/workspace nav (superset-sh#3472) [PR5/#180] - de70163 feat(desktop): v2 review tab first pass — PR info, checks, comments (superset-sh#3463) [PR6/#182] Intentionally skipped (version bump, fork has independent versioning): - 1e23353 chore(desktop): bump version to 1.5.5 (superset-sh#3473) Fork-adaptation fixes layered on top of the cherry-picks: - PR1: host-service-coordinator alias import fix, settings Escape selector narrowing (role-based + popper wrapper), Escape close uses replace navigation - PR2: dual quit mode preservation (requestQuit "release"/"stop"), trayUpdateToken guard for stale async fetchHostInfo results - PR4: ChangesHeader.normalizeBranchName regex rewrite (lint false positive), worktree add uses fullRef for remote-tracking refs, syncTimedOut reset on pendingId change, GIT_REFS.md barrel example fix - PR5: migrate.ts re-sanitize of existing localStorage overrides (v2 marker bump intent), FOCUS_PANE_* enabled:isActive for KeepAliveWorkspaces, CATEGORY_ORDER merges Navigation (upstream) and Browser (fork) - PR6: normalizeThreadsToComments flattens all thread.comments (not just first), CommentPane overrides <a> (openUrl) and <img> (SafeImage), zero-badge suppression, pr-null comments gate Fork features verified intact (Explore agent audit of combined 36d4de4..35d95f3 range): - BROWSER_RELOAD / BROWSER_HARD_RELOAD hotkeys - dual quit mode menu in tray - v1 terminal cold-restore + retry reconnect (out of range but unaffected) - KeepAliveWorkspaces (FOCUS_PANE_* gated on isActive) - useCommandPalette + addMemoTab in v2 workspace - host-service-coordinator rename alias pattern
Summary
Two related hotkey fixes bundled together:
Restore directional pane focus in v1 workspaces. PR feat(desktop): Cmd+Alt+Arrow moves focus between v2 panes #3403 wired
Cmd+Alt+Arrowspatial pane focus for v2 but skipped v1, leaving v1 with no keyboard pane navigation at all. Reimplements the spatial neighbor walker against v1'sMosaicNode<string>tree (no cross-package coupling to v2'sLayoutNode) and addsFOCUS_PANE_LEFT/RIGHT/UP/DOWNhandlers to the v1 workspace route. Also teaches the recorder thatOptionis a legitimate app modifier on Mac (⌥⌫ etc.) and warns when an alt-only chord could mask typing special characters.Best-effort migrator for v1 hotkey overrides. The v1 recorder stored chords from
event.key— layout-dependent raw glyphs likemeta+,,ctrl+shift+@,meta+alt+ª— while the new store matches onevent.codenames. The previoussanitizeOverridewas strict (/^[a-z0-9]+$/) and silently dropped most real-world v1 entries. Extends it with a token rewrite pre-pass covering:,→comma,[→bracketleft, etc.)@→2,_→minus,?→slash, etc.)ª→9,å→a,•→8, etc.)The Mac Option table is gated behind a
navigator.keyboard.getLayoutMap()probe — on non-US Mac layouts (whereOption+Qproduces•instead ofœ), the glyphs fall through and drop rather than silently rebind to the wrong physical key.Coverage
Validated against three real leveldb / app-state dumps captured from the
indigo-pentaceratops(v1.4.7),tray-polling-fix, andhotkeys-fixesworktrees — every captured chord is locked in as a parametric test case. 20/20 entries from the v1.4.7 blob now migrate successfully (17 rewritten + 3nullunassigns). 64 tests total inoverrideSanitizer.test.ts.Test plan
bun test apps/desktop/src/renderer/hotkeys/utils/overrideSanitizer.test.ts— 64 passbun run --filter=@superset/desktop typecheck— cleanbun run lint— cleanCmd+Alt+Arrowwalks to the spatially adjacent pane, edges are no-opslocalStorage.getItem('hotkey-overrides')contains the rewritten entriesSummary by cubic
Restores Cmd+Alt+Arrow directional pane focus in v1 workspaces and migrates v1 hotkey overrides to the v2 format so users keep their shortcuts. Adds Mac-specific recording logic and a safety warning for alt-only chords.
New Features
FOCUS_PANE_LEFT/RIGHT/UP/DOWNand no-ops at edges.Migration
event.keychords (e.g., "meta+,", "ctrl+shift+@", "meta+alt+ª") toevent.codenames.navigator.keyboard.getLayoutMap()).Written for commit 1b3099d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements