Repository navigation
fix(desktop): forward app hotkeys from browser panes via main-process chord index - #6233
andyst-dev wants to merge 5 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughBrowser panes now use shared canonical chord utilities. The renderer synchronizes registered application chords with the main process. Matching pane shortcuts are emitted through tRPC and re-dispatched as host-renderer keyboard events. Editing, find, and print shortcuts remain page-owned. ChangesBrowser pane hotkey forwarding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HotkeyRegistry
participant BrowserRouter
participant BrowserManager
participant PersistentWebview
participant HostRenderer
HotkeyRegistry->>BrowserRouter: setAppChords(chords)
BrowserRouter->>BrowserManager: setAppChords(chords)
BrowserManager->>BrowserManager: match before-input-event against appChords
BrowserManager->>BrowserRouter: emit app-hotkey(paneId, chord)
BrowserRouter->>PersistentWebview: deliver chord
PersistentWebview->>HostRenderer: dispatch synthetic keydown
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 adds a shared physical-key chord index and forwards registered application hotkeys from browser guest webContents into the renderer. It also adds the corresponding tRPC synchronization/subscription path, browser-pane redispatch, tests, and keyboard-system documentation, but accidentally removes the v2 listener required for the dedicated close-pane event.
Confidence Score: 4/5The Cmd/Ctrl+W regression must be fixed before merging because focused v2 browser panes now swallow the shortcut without closing. The main process still routes Cmd/Ctrl+W exclusively through close-pane, but the changed v2 hook removes the only subscriber that invokes the pane close action. Files Needing Attention: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/browser/browser-manager.ts | Adds renderer-synchronized chord matching and forwarding while retaining dedicated close/reload branches; the close branch now emits an event no longer consumed by v2. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts | Adds app-hotkey redispatch but replaces, rather than supplements, the dedicated close-pane subscription required by Cmd/Ctrl+W. |
| apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts | Re-exports shared chord utilities and mirrors rebuilt renderer chord indexes to the main process. |
| apps/desktop/src/shared/browser-pane-chords.ts | Introduces pure canonicalization, Electron-input matching, exclusions, and synthetic KeyboardEvent conversion helpers. |
| apps/desktop/src/lib/trpc/routers/browser/browser.ts | Adds observable-based hotkey subscriptions and a mutation for replacing the main-process chord index. |
| apps/desktop/src/shared/browser-pane-chords.test.ts | Covers physical-position matching, exclusions, modifier handling, and chord-to-event conversion. |
Sequence Diagram
sequenceDiagram
participant Guest as Browser guest webContents
participant Main as BrowserManager
participant IPC as Browser tRPC router
participant Renderer as v2 BrowserPane
participant Hotkeys as Renderer hotkey handlers
Guest->>Main: before-input-event
alt Cmd/Ctrl+W
Main->>Main: preventDefault()
Main->>IPC: emit close-pane
Note over IPC,Renderer: v2 close-pane subscriber was removed
else Other registered app chord
Main->>Main: match physical input.code
Main->>IPC: emit app-hotkey(chord)
IPC->>Renderer: onAppHotkey
Renderer->>Hotkeys: dispatch synthetic keydown
end
Prompt To Fix All With AI
### Issue 1
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts:86-96
**Close-pane event loses its subscriber**
When Cmd/Ctrl+W is pressed with a v2 browser pane focused, the main process prevents the default action, emits `close-pane`, and returns before `app-hotkey` forwarding, but this change replaces the pane's `onClosePane` subscription with `onAppHotkey`. The dedicated event is therefore unhandled and the shortcut is swallowed without closing the pane.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(desktop): forward app hotkeys from b..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const appHotkeySub = electronTrpcClient.browser.onAppHotkey.subscribe( | ||
| { paneId }, | ||
| { | ||
| onData: () => { | ||
| void ctxRef.current.actions.close(); | ||
| onData: (chord: string) => { | ||
| const event = dispatchChordEvent(chord); | ||
| if (event) { | ||
| window.dispatchEvent(event); | ||
| } | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
When Cmd/Ctrl+W is pressed with a v2 browser pane focused, the main process prevents the default action, emits close-pane, and returns before app-hotkey forwarding, but this change replaces the pane's onClosePane subscription with onAppHotkey. The dedicated event is therefore unhandled and the shortcut is swallowed without closing the pane.
Knowledge Base Used: Desktop App (Electron main + renderer)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts
Line: 86-96
Comment:
**Close-pane event loses its subscriber**
When Cmd/Ctrl+W is pressed with a v2 browser pane focused, the main process prevents the default action, emits `close-pane`, and returns before `app-hotkey` forwarding, but this change replaces the pane's `onClosePane` subscription with `onAppHotkey`. The dedicated event is therefore unhandled and the shortcut is swallowed without closing the pane.
**Knowledge Base Used:** [Desktop App (Electron main + renderer)](https://app.greptile.com/personal-org-6810/-/custom-context/knowledge-base/superset-sh/superset/-/docs/desktop-app.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/shared/browser-pane-chords.test.ts (1)
77-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest exclusions with registered excluded chords.
CHORDSdoes not containmeta+formeta+p. These assertions returnnullfrom the membership check even if the exclusion check fails. Use a set containing each excluded chord. Cover Meta and Ctrl variants, includingA,C,V,X,Z, andG.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/shared/browser-pane-chords.test.ts` around lines 77 - 88, Update the browser-pane chord exclusion tests to pass a chord set containing the relevant registered exclusions instead of relying on CHORDS, which lacks them. Extend coverage for both Meta and Ctrl variants of A, C, V, X, Z, and G, while preserving the existing find and print guest-handling assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts:
- Around line 86-93: Update the app hotkey forwarding in the onAppHotkey
subscription callback to dispatch the event through document or the focused pane
element instead of window. Preserve the existing dispatchChordEvent and event
guard so the bubbling keydown reaches global useHotkey listeners.
In `@apps/desktop/src/shared/browser-pane-chords.ts`:
- Around line 116-118: Expand BROWSER_PANE_EXCLUDED_CHORDS to include both Meta
and Ctrl variants of A, C, V, X, Z, F, P, and G. In
apps/desktop/src/shared/browser-pane-chords.test.ts lines 77-88, pass the
registered excluded chords into matchAppChord and assert each returns null.
- Around line 64-87: The inputToChord flow should ignore IME composition events.
Add isComposing?: boolean to BrowserPaneInput, return null before normalizeToken
and chord construction when input.isComposing is true, and add coverage for a
composing keyDown returning null.
---
Nitpick comments:
In `@apps/desktop/src/shared/browser-pane-chords.test.ts`:
- Around line 77-88: Update the browser-pane chord exclusion tests to pass a
chord set containing the relevant registered exclusions instead of relying on
CHORDS, which lacks them. Extend coverage for both Meta and Ctrl variants of A,
C, V, X, Z, and G, while preserving the existing find and print guest-handling
assertions.
🪄 Autofix
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 Plus
Run ID: 762ced90-3df7-4959-b708-26fe32b35f7d
📒 Files selected for processing (7)
apps/desktop/docs/KEYBOARD_SYSTEM.mdapps/desktop/src/lib/trpc/routers/browser/browser.tsapps/desktop/src/main/lib/browser/browser-manager.tsapps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.tsapps/desktop/src/shared/browser-pane-chords.test.tsapps/desktop/src/shared/browser-pane-chords.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Architecture diagram
sequenceDiagram
participant Reg as Renderer Hotkey Registry
participant TRPC as Browser tRPC Router
participant Main as BrowserManager (Main Process)
participant Shared as browser-pane-chords
participant Guest as Browser Pane Guest webContents
participant Host as Host Document (usePersistentWebview / useHotkey)
Note over Reg,Main: Renderer pushes its canonical chord index to main
Reg->>Reg: Rebuild chord index (overrides / layout / adaptive)
Reg->>TRPC: browser.setAppChords.mutate({ chords })
TRPC->>Main: setAppChords(chords)
Main->>Main: Replace appChords Set (empty until first push)
loop On registry rebuild (override / layout / preference changes)
Reg->>TRPC: setAppChords.mutate({ chords })
TRPC->>Main: setAppChords(chords)
end
Note over Guest,Host: Keydown while guest pane has focus
Guest->>Main: before-input-event (keyDown: input.code + modifiers)
Main->>Shared: inputToChord(input)
Shared-->>Main: Canonical chord by physical position (e.g. meta+b)
Note over Main,Shared: Physical code matching anchors chords across Dvorak and other layouts
alt Chord is meta/ctrl+w or meta/ctrl+r
Main->>Main: Emit close-pane / reload-pane (legacy TabView path)
else Chord excluded for panes (meta+f / meta+p / meta+g)
Main-->>Guest: No preventDefault: guest find / print / find-next wins
else Chord found in appChords index
Main->>Main: preventDefault, then emit app-hotkey for pane
Main-->>TRPC: onAppHotkey subscription emits chord
TRPC-->>Host: Chord over tRPC stream
Host->>Shared: dispatchChordEvent(chord)
Shared-->>Host: Synthetic KeyboardEvent init (code + modifiers)
Host->>Host: window.dispatchEvent(keydown)
Host->>Host: useHotkey resolves chord and fires handler
else No match (modifier-only / unregistered / index not yet pushed)
Main-->>Guest: Key falls through to guest page
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- keep onClosePane subscription in v2 pane (Cmd+W was swallowed after the dedicated close-pane event replaced app-hotkey forwarding) - dispatch synthetic keydowns on document (react-hotkeys-hook listens there, not window) - exclude guest-owned editing chords (meta/ctrl a c v x z) from the forwarded index, and test exclusions against registered chords - skip IME composition keydowns in inputToChord (isComposing) - catch setAppChords rejection during startup/teardown - move the fix note from Known gaps to Decision history
|
Thanks for the reviews — all actionable feedback is addressed in
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts`:
- Around line 87-96: Update pushAppChordsToMain to retain the latest chords when
setAppChords rejects because the IPC channel is unavailable, rather than only
warning and discarding the update. Add a pending chord-list retry mechanism and
trigger it when the electron-trpc channel becomes available, ensuring the most
recent registeredAppChords state is eventually sent while preserving successful
replacement behavior.
In `@apps/desktop/src/shared/browser-pane-chords.test.ts`:
- Around line 129-130: Update the test input construction in the chord test loop
so the Ctrl case sets the `control` property rather than `ctrl`, matching the
`inputToChord` field it reads. Keep the existing modifier behavior for other
keys and ensure the test exercises the Ctrl exclusion path.
🪄 Autofix
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 Plus
Run ID: e72a39ca-3a79-4967-9a45-4dbc5a9276bc
📒 Files selected for processing (5)
apps/desktop/docs/KEYBOARD_SYSTEM.mdapps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.tsapps/desktop/src/shared/browser-pane-chords.test.tsapps/desktop/src/shared/browser-pane-chords.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/docs/KEYBOARD_SYSTEM.md
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts
- apps/desktop/src/shared/browser-pane-chords.ts
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
- match guest editing shortcuts by produced key (input.key) so undo on non-QWERTY layouts (AZERTY Cmd+Z = physical KeyW) stays with the page instead of colliding with the app's CLOSE_PANE chord - catch close() rejection in the pane close subscription - retry setAppChords once after a short delay when the IPC channel is unavailable at startup, instead of dropping the update - fix ctrl-variant exclusion test to set input.control (the field inputToChord reads), which previously never exercised the ctrl path
|
Thanks — all round-2 feedback is addressed in
|
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
isGuestEditingShortcut matched the produced character regardless of shift, so the app's COPY_PATH (Cmd+Shift+C) was classified as guest copy and never forwarded from a browser pane. Shift-gated chords are not guest editing shortcuts (the guest's copy/paste/undo are plain Cmd/Ctrl+letter) — add the shift guard and a regression test. Also include paneId in the close-failure warning.
|
Thanks — all three points are addressed in
|
|
Thanks - fixed in The renderer-to-main chord sync now uses a dedicated latest-state synchronizer:
Added deterministic scheduler tests for transient recovery, stale-retry cancellation, in-flight replacement, and capped retry behavior. Validation:
|
|
Heads-up: this PR is now superseded by a refactor on main. Since it was opened, main introduced Per the repo's contribution guidance, the same end-state is already on main, so this PR is obsolete. Happy to close it, or I can leave it open if a maintainer wants to cherry-pick any surviving intent. |
|
Closing this as a duplicate: the same feature (forwarding app hotkeys from
Thanks for considering. I'll keep contributing on the superset desktop area. |
|
Superseded by #5494 (merged). See comment above. |
|
Thanks for the contribution @andyst-dev ! |
Fixes #6186
Bug
With a browser pane focused, every app shortcut dies: the pane is a guest
webContents(out-of-process<webview>), so a keydown while the guest has focus never reaches the host document wherereact-hotkeys-hooklistens. The only interception path is main'sbefore-input-event, and that handler hardcoded exactly two chords (⌘W / ⌘R matched by produced character viainput.key). ⌘B (TOGGLE_WORKSPACE_SIDEBAR), ⌘L (TOGGLE_SIDEBAR) and every other renderer-registered chord fell through to the page.Fix
Forward the app's full chord index to main and match there by physical position:
shared/browser-pane-chords.ts(new, pure — no Electron/DOM/zustand imports):normalizeToken/canonicalizeChord/eventToChordmoved from the renderer (re-exported fromresolveHotkeyFromEvent.tsso existing imports keep working), plusinputToChord(ElectronInput→ canonical chord viainput.code),matchAppChord(index lookup with a guest-owned exclusion list),tokenToCode/chordToEventInit(canonical chord → synthetic DOMKeyboardEventfor re-dispatch).browser-manager.ts(main):setAppChords()receives the renderer's canonical chord set;setupBeforeInputmatches on physical key position (input.code) instead ofinput.key, drops theshift/altandmeta||controlguards (the chord table is the gate now), keeps dedicatedclose-pane/reload-paneevents for the legacy TabView path (which has no globalCLOSE_PANEhotkey), and emitsapp-hotkey:<paneId>with the matched chord for everything else.browser.ts):setAppChordsmutation +onAppHotkeysubscription.resolveHotkeyFromEvent.ts: pushesregisteredAppChordskeys to main on every rebuild (overrides / layout / adaptive-toggle changes), so the main-side index never drifts.usePersistentWebview(v2): subscribes toonAppHotkeyand re-dispatches a syntheticKeyboardEvent, letting the normal focus-scopeduseHotkeyregistrations resolve it exactly as they do when the host document has focus.KEYBOARD_SYSTEM.md"Known gaps" row.This also fixes the Dvorak half of the report: the old handler matched whichever physical key prints "w" (
input.key), while the registry binds ⌘W to physicalKeyW— two different keys on non-QWERTY layouts. Matching oninput.codeanchors both sides to physical position.Validation
bun testonshared/browser-pane-chords.test.ts: 16/16 pass (⌘B match, Dvorak ⌘W anchoring, shift/alt chords, guest exclusion, modifier-only nulls,tokenToCode/chordToEventInitround-trips)tsc --noEmiton apps/desktop: 0 errorsSummary by cubic
Forward app hotkeys from focused browser panes by mirroring the renderer’s chord index to main and re-dispatching matches on the host document. Adds reliable retry/backoff for chord sync, restores dead shortcuts (⌘B/⌘L), and anchors to physical keys across layouts (e.g., Dvorak ⌘W).
before-input-eventby physicalcode(IME-safe), emitapp-hotkey:<paneId>, and keep dedicated ⌘W/⌘R close/reload events.input.key) and ignore shift-gated chords (e.g., ⌘⇧C), so undo on non‑QWERTY stays with the page while COPY_PATH still forwards.onAppHotkeyand dispatch a synthetic keydown ondocument; keeponClosePaneand catchclose()errors.Written for commit 275af17. Summary will update on new commits.
Summary by CodeRabbit