Skip to content

VC-301: route app shortcuts out of focused Browser pages - #884

Merged
hussainph merged 4 commits into
mainfrom
volli/VC-301-k-and-app-navigation-shortcuts-do-not-fire-when
Oct 18, 2026
Merged

hussainph merged 4 commits into
mainfrom
volli/VC-301-k-and-app-navigation-shortcuts-do-not-fire-when

Conversation

@hussainph

@hussainph hussainph commented Oct 18, 2026 •

Copy link
Copy Markdown
Owner

What changed

A Browser Tab's page is its own native WebContentsView, so the renderer's window keydown listeners never saw a key pressed inside it. With a page focused, ⌘K, ⌘1–⌘9 and ⌃⌘+arrow did nothing, and ⌘W went through to the menu's Close Window role (A15 / VC-301).

Main (main/browser/app-shortcuts.ts, new, pure, 100% covered and added to the coverage gate): browserKeyRoute(input) sorts each page key into page (the page keeps it), swallow (the app owns it but has nothing to do) or app. tab-host.ts adds one before-input-event listener per page. The listener acts only when the page is on screen and has native focus. CDP key events also reach this hook, so an agent pressing ⌘W into a page nobody is focused on still goes to the page and never closes a tab. index.ts wires the new optional deliverAppShortcut dep to main/browser/app-shortcut-relay.ts, which focuses the app renderer and then hands the shortcut to every window subscribed to the desktop-only browserShortcuts.subscribe entry (see Transport below).

The app owns this deliberately small set:

Chord Effect
⌘K, ⌘1–⌘9, ⌃⌘+arrow Replayed into the renderer as a window keydown. The listener that already owns the chord makes the decision, guards included. use-command-palette-shortcut.ts and its terminal guard are untouched. First, the split-view pane that draws the page is focused, because a click into a native page never reaches the renderer, so the split view's focused pane is otherwise whichever pane was last clicked in the app.
⌘W Closes this page's own tab, routed by opaque tab id.
⌘L Returns focus to the app, on this tab's Address field (with the address selected).

Every other key stays with the page. That includes all editing chords (⌘B/C/V/Z/A/P), Escape, Tab/Shift+Tab, and ⌘[ / ⌘], which mean outdent and indent in page editors. Main can't tell whether a page field has focus, so the set holds only chords that a page in a real browser doesn't get to keep either. ⌘K is the ticket's one explicit exception. A held ⌘W/⌘L fires once, and its repeats are swallowed so they can't reach the menu's Close Window.

Renderer:

  • components/browser/browser-app-shortcuts.ts (new, 100%, gated) holds a registry keyed by tab id, plus the replay.
  • BrowserPane registers its own close and Address field while it's visible, and takes a new optional onClose prop. Every surface passes the close its tab strip's × already uses:
    • ticket-detail passes closeTab(tab).
    • browser-preview passes the same api.close call as its × button.
    • home-surface's browser branch of handleClose is pulled out into a closeBrowser(tabId) callback, unchanged, so both paths share it.
  • A tab with no mounted pane does nothing. ⌘W can't close another pane: the key names its tab, and the split view's focused pane is never consulted.
  • BrowserPane also takes onFocusPane. Home wires it to focusHomePane(pane) and the ticket workspace to focusTicketPane(pane).
  • hooks/use-browser-app-shortcuts.ts is mounted once in AppShell. When runBrowserAppShortcut reports that the app did nothing, the hook calls the desktop-only browserShortcuts.focusPage entry, which hands keyboard focus back to the page. "Did nothing" means one of: a replayed chord no listener preventDefault-ed (for example ⌘K under the terminal guard, or ⌃⌘+arrow with no split), a pane with no close, or a tab no pane draws. On the main side, BrowserTabHost.focusPage stays quiet for a tab that is gone or off screen.

Docs: a Browser Tab section in reference/keyboard-shortcuts.mdx.

Choices for the maintainer to confirm

  • ⌘L for focus return. The ticket asked for a "focus-return" shortcut without naming a key. ⌘L is the browser's own "leave the page" chord, and Escape and Tab can't be taken from pages (dialogs, forms).
  • ⌘W closes the page's tab. The app has no close-tab chord anywhere else. Leaving ⌘W alone in a page closes the whole window, which is the wrong-pane failure taken to its extreme.
  • Not routed: ⌘[ / ⌘] history, ⌥⌘B, ⌘P, ⌘T, and the DevTools view. In DevTools, ⌘K clears the console. Each is easy to add to the matcher if wanted.

Transport (docs/BOUNDARIES.md rule 5)

Both crossings are desktop-only tier entries, client-local and host-placed like hosts.*, served from the host's one handler map. No raw IPC channel is added.

Entry Kind Was
browserShortcuts.subscribe subscription of BrowserAppShortcutEvent the volli:browser-app-shortcut push
browserShortcuts.focusPage mutation { tabId } → null the volli:browser-focus-page invoke
  • The wire types live in @volli/shared (browser-app-shortcut.ts).
  • In host-core, BrowserShortcutsPort is a structural port. hostd has none, so both entries answer unavailable there.
  • In session-rpc, both procedures are built with zod validators and the subscription's schema is published. pnpm check:protocol-schema --base origin/main reports two client-local additions and passes.
  • In desktop main, app-shortcut-relay.ts is gated at 100%, and index.ts wires the port.
  • In the renderer, followBrowserAppShortcuts follows the stream over the Session RPC client. If the stream ends in error, it follows it again after a second; a shortcut is a press, not state, so nothing is lost.
  • Both earlier raw channels are gone from contract.ts, placement.ts, ipc-descriptors.ts, the preload and the browser IPC handlers.

Why this shape

Replaying the chord keeps one meaning for each shortcut: the renderer predicates stay the single source. app-shortcuts.test.ts checks every replayed chord against isCommandPaletteKeyEvent, projectIndexForKeyEvent and splitShortcutForKeyEvent. Only the two tab-specific actions are named, because only they need to know where the key came from.

Known limitations

  • Main claims the key, and moves focus to the app, before the renderer decides. A chord the renderer then ignores gets focus handed back to the page. Focus does leave the page briefly, and the page never receives that keydown. The page would have no use for it, because main only claims chords the app owns.
  • "Ignored" means "no listener called preventDefault". That is the renderer's own definition of handled. use-project-shortcuts.ts calls preventDefault for ⌘N even when there is no Nth project, exactly as it does in the app today, so that case keeps focus in the app rather than returning it to the page. I left that hook alone.

Browser overlays (acceptance)

  • Covered: overlays the page draws itself, such as DOM modals, menus and popovers. They live in the same page WebContents, so the same before-input-event hook sees their keys.
  • Not affected: app overlays over a Browser Tab, such as the palette, menus and dialogs. While one is up, the native plane is detached and replaced by a captured still (existing behaviour), keyboard focus is already in the renderer, and the app's own listeners run unchanged.
  • Not covered:
    • native OS popups that Chromium shows for a page, such as a <select> popup menu or a JS alert. These take keys outside the WebContents.
    • the DevTools view, which is unrouted as noted above.

For the maintainer

  • Transport. Per docs/BOUNDARIES.md rule 5, no raw IPC was added (see Transport below).
  • Wording. The docs say "Address field" to match the existing Browser Tabs guide. Code comments say "address bar".
  • No single test runs from main's delivery all the way to the renderer hook. Each hop is tested on its own: the relay, the host-core handler, the router served over both the IPC and the WebSocket contract links, and the renderer's follower.

Verification

  • Focused vitest: src/main/browser/, src/main/ipc-descriptors.test.ts, src/ipc/placement.test.ts, src/renderer/src/components/browser/ and src/renderer/src/hooks/use-split-shortcuts.browser.test.tsx. That is 22 files and 1025 tests, all passing. They include:
    • tab-host before-input tests: keys are routed while the page is focused and on screen; left alone when unfocused (CDP), staged, or with no deliver dep; repeats are swallowed; the page's own tab is named when two are shown; focusPage returns focus to a shown page and does nothing for a hidden or missing tab.
    • use-split-shortcuts.browser.test.tsx: three panes, with the app's focused pane on the left and the page in the middle. ⌃⌘→ from the page lands on the right pane through the real split listener and workspace store. A control case without the pane focus lands one pane short. Unsplit, the chord is reported as unclaimed.
    • browser-pane.shortcuts.test.tsx: ⌘W closes the pane's own tab and not the other visible pane; ⌘W with no close is reported as not acted on; a replay focuses the pane's own split pane; ⌘L focuses and selects the right Address field; hidden or unmounted panes don't answer.
    • browser-app-shortcuts.test.ts: the acted/ignored results, and the order (pane focus before keydown).
  • Transport: desktop-router.test.ts covers the stream and the hand-back over both contract links, plus input bounds. host-handlers.test.ts covers unavailable on hostd and the port pass-through. The fixtures are desktop-ipc, handler-projection and session-handlers.test-support. app-shortcut-relay.test.ts checks focus before delivery, fan-out, unsubscribe, and a listener that throws. browser-app-shortcuts.test.ts checks re-follow on error and the stop.
  • Scoped coverage at 100/100/100/100 for app-shortcuts.ts, app-shortcut-relay.ts, browser-app-shortcuts.ts, session-rpc's desktop-router.ts and host-core's handlers/host-handlers.ts.
  • Typecheck is clean for @volli/shared, @volli/host-core, @volli/session-rpc, apps/desktop and the root config.
  • vp run typecheck (apps/desktop, all four configs) and vp check: clean.
  • Not driven live. volli-drive sends synthetic CDP input to an accessory app that never takes focus, and this fix acts only on native page focus, so the drive can't exercise it. The native repro in the ticket comments (vc322-browser-boundary.mjs --native-window, PR fix(a11y): restore dialog focus and record release blockers (VC-322) #579, not on main) is the right live check.

A Browser Tab's page is its own native WebContents, so the renderer's
window keydown listeners never saw keys pressed in it: ⌘K, ⌘1–⌘9 and
⌃⌘+arrow did nothing, and ⌘W fell through to the menu's Close Window.

Main now listens to before-input-event on each page and, only while the
page holds native focus on screen, takes a small app-owned set
(browser/app-shortcuts.ts, a pure tested matcher):

- ⌘K, ⌘1–⌘9, ⌃⌘+arrow are replayed into the renderer as a window keydown,
  so the existing listener and its guards (the ⌘K terminal guard
  included) decide, unchanged.
- ⌘W closes the page's own tab and ⌘L returns focus to its address bar,
  routed by the opaque tab id to the pane that draws it, never to the
  split view's focused pane.

Everything else, including every editing chord, Escape and Tab, stays
the page's. Focus moves back to the app renderer before delivery.
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: f184fccf-642d-4a06-91ac-e82eb45c365e
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Review follow-up on #884.

- A click into a native page never reaches the renderer, so the split
  view's focused pane was whichever pane was last clicked in the app and
  ⌃⌘+arrow from a page moved from the wrong place. A replayed chord now
  focuses the pane drawing the page first (BrowserPane onFocusPane, wired
  by Home and the ticket workspace), proven against the real split
  listener and workspace store.
- runBrowserAppShortcut now reports whether the app acted: a replayed
  chord no listener preventDefault-ed (⌘K under the terminal guard,
  ⌃⌘+arrow unsplit), a close with no close, or a tab no pane draws. The
  hook then asks main to give keyboard focus back to the page through
  the new client-local volli:browser-focus-page command
  (BrowserTabHost.focusPage, quiet for a gone or off-screen tab).
- Drop runBrowserAppShortcut's test-only target parameter.
docs/BOUNDARIES.md rule 5: no new raw IPC. The two raw channels this PR
added are now desktop-only entries (DESKTOP_ENTRIES, client-local,
host-placed like hosts.*), served from the host's one handler map:

- browserShortcuts.subscribe: every app shortcut a focused Browser page
  hands back (was the volli:browser-app-shortcut push).
- browserShortcuts.focusPage: focus back to a page whose shortcut did
  nothing (was volli:browser-focus-page).

The wire types move to @volli/shared; host-core gains a structural
BrowserShortcutsPort (hostd has none, so both answer unavailable);
session-rpc builds both procedures with zod validators and publishes
their schemas; desktop main's new app-shortcut-relay focuses the window
then feeds every subscriber, and wires the port. The renderer follows
the stream over the Session RPC client and re-follows after a beat if
it ends in error. Both raw channels are deleted from contract,
placement, descriptors, preload and the browser IPC handlers.

Behaviour and the review-round regression tests are unchanged.
@hussainph
hussainph merged commit bd99838 into main Oct 18, 2026
23 checks passed
hussainph added a commit that referenced this pull request Oct 18, 2026
…browser-tab-blank-loading-surfa

VC-302: theme the native Browser Tab blank/loading plane (stacked on #884 — merge after it)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant