From c3cc32af352a5cb68c5a79864557baef1c5f75da Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Mon, 4 May 2026 16:11:23 -0700 Subject: [PATCH] fix(shortcuts): emit ui:shortcutConsumed from all browser-guest chord forwards Without this, the stuck-badge regression the PR fixes still reproduces when focus is inside a webview guest: browser-guest-ui.ts forwards 5+ shortcut IPCs (worktreeHistoryNavigate, toggleWorktreePalette, openQuickOpen, openNewWorkspace, jumpToWorktreeIndex, plus the browser-specific Cmd+T/W/L/R/F, Cmd+Shift+B/[/], Cmd+Alt+[/], Ctrl+PageUp/Down) but never called emitConsumedShortcut. Consolidate emit via an IIFE-thunk so every matched branch emits once, before the action send, honoring the main->renderer ordering contract. Also tighten createMainWindow.ts zoom-changed ordering and update the JSDoc on emitConsumedShortcut to accurately describe trigger conditions and FIFO delivery. Co-authored-by: Orca --- src/main/browser/browser-guest-ui.test.ts | 130 +++++++++++++++++++++- src/main/browser/browser-guest-ui.ts | 127 +++++++++++++-------- src/main/index.ts | 6 + src/main/window/createMainWindow.ts | 3 +- src/main/window/emit-consumed-shortcut.ts | 22 +++- 5 files changed, 235 insertions(+), 53 deletions(-) diff --git a/src/main/browser/browser-guest-ui.test.ts b/src/main/browser/browser-guest-ui.test.ts index 5e8d4cb7d58..37b9a52df26 100644 --- a/src/main/browser/browser-guest-ui.test.ts +++ b/src/main/browser/browser-guest-ui.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { screenGetCursorScreenPointMock } = vi.hoisted(() => ({ screenGetCursorScreenPointMock: vi.fn(() => ({ x: 0, y: 0 })) @@ -9,7 +9,7 @@ vi.mock('electron', () => ({ webContents: { fromId: vi.fn() } })) -import { setupGuestContextMenu } from './browser-guest-ui' +import { setupGuestContextMenu, setupGuestShortcutForwarding } from './browser-guest-ui' describe('setupGuestContextMenu', () => { const browserTabId = 'tab-1' @@ -217,3 +217,129 @@ describe('setupGuestContextMenu', () => { }) }) }) + +describe('setupGuestShortcutForwarding', () => { + const browserTabId = 'tab-1' + let rendererSendMock: ReturnType + let guestOnMock: ReturnType + let guestOffMock: ReturnType + const originalPlatform = process.platform + + function makeGuest() { + return { + on: guestOnMock, + off: guestOffMock + } as unknown as Electron.WebContents + } + + function makeRenderer() { + return { send: rendererSendMock } as unknown as Electron.WebContents + } + + beforeEach(() => { + rendererSendMock = vi.fn() + guestOnMock = vi.fn() + guestOffMock = vi.fn() + // Why: resolveWindowShortcutAction branches on platform; pin darwin so the + // Cmd+J chord below resolves to toggleWorktreePalette deterministically. + Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true }) + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true }) + }) + + function triggerBeforeInput(input: Partial): void { + const handler = guestOnMock.mock.calls.find((call) => call[0] === 'before-input-event')?.[1] as + | ((event: { preventDefault: () => void }, input: Electron.Input) => void) + | undefined + + expect(handler).toBeTypeOf('function') + handler!({ preventDefault: vi.fn() }, { + type: 'keyDown', + key: 'j', + code: 'KeyJ', + meta: false, + control: false, + alt: false, + shift: false, + ...input + } as Electron.Input) + } + + it('emits ui:shortcutConsumed before the forwarded action IPC for Cmd+J', () => { + const guest = makeGuest() + const renderer = makeRenderer() + + setupGuestShortcutForwarding({ + browserTabId, + guest, + resolveRenderer: () => renderer + }) + + triggerBeforeInput({ key: 'j', code: 'KeyJ', meta: true }) + + const channels = rendererSendMock.mock.calls.map((call) => call[0]) + expect(channels).toContain('ui:shortcutConsumed') + expect(channels).toContain('ui:toggleWorktreePalette') + // Why: the consumed-shortcut signal must precede the action so the + // renderer's useModifierHint overlay clears before the action handler runs. + expect(channels.indexOf('ui:shortcutConsumed')).toBeLessThan( + channels.indexOf('ui:toggleWorktreePalette') + ) + }) + + it('emits ui:shortcutConsumed before ui:worktreeHistoryNavigate on Cmd+Alt+Left', () => { + const guest = makeGuest() + const renderer = makeRenderer() + + setupGuestShortcutForwarding({ + browserTabId, + guest, + resolveRenderer: () => renderer + }) + + // Why: Cmd+Alt+Left is the Alt-exempt worktreeHistoryNavigate chord, + // structurally different from the modifier-chord ladder covered above. + triggerBeforeInput({ key: 'ArrowLeft', code: 'ArrowLeft', meta: true, alt: true }) + + const channels = rendererSendMock.mock.calls.map((call) => call[0]) + expect(channels).toContain('ui:shortcutConsumed') + expect(channels).toContain('ui:worktreeHistoryNavigate') + expect(channels.indexOf('ui:shortcutConsumed')).toBeLessThan( + channels.indexOf('ui:worktreeHistoryNavigate') + ) + }) + + it('preventDefaults Cmd+Alt+Left without emitting when renderer is unavailable', () => { + const guest = makeGuest() + const preventDefaultMock = vi.fn() + + setupGuestShortcutForwarding({ + browserTabId, + guest, + resolveRenderer: () => null + }) + + const handler = guestOnMock.mock.calls.find((call) => call[0] === 'before-input-event')?.[1] as + | ((event: { preventDefault: () => void }, input: Electron.Input) => void) + | undefined + expect(handler).toBeTypeOf('function') + + handler!({ preventDefault: preventDefaultMock }, { + type: 'keyDown', + key: 'ArrowLeft', + code: 'ArrowLeft', + meta: true, + control: false, + alt: true, + shift: false + } as Electron.Input) + + // Why: preventDefault fires unconditionally so Chromium/guest doesn't + // handle the chord as its own navigation. No IPC sends because renderer + // is null — emitConsumedShortcut no-ops on undefined. + expect(preventDefaultMock).toHaveBeenCalledTimes(1) + expect(rendererSendMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/browser/browser-guest-ui.ts b/src/main/browser/browser-guest-ui.ts index 313404888bd..a448f6fdf4b 100644 --- a/src/main/browser/browser-guest-ui.ts +++ b/src/main/browser/browser-guest-ui.ts @@ -7,6 +7,7 @@ import { isWindowShortcutModifierChord, resolveWindowShortcutAction } from '../../shared/window-shortcut-policy' +import { emitConsumedShortcut } from '../window/emit-consumed-shortcut' type ResolveRenderer = (browserTabId: string) => Electron.WebContents | null @@ -234,6 +235,12 @@ export function setupGuestShortcutForwarding(args: { // only by suppressing the event here too. event.preventDefault() const renderer = resolveRenderer(browserTabId) + // Why: the chord's non-modifier key never reaches the renderer's + // window-level listener when focus is inside a guest webContents, so + // useModifierHint cannot self-clear. Emit the consumed-shortcut signal + // so the renderer's single ui:shortcutConsumed subscriber clears the + // modifier-hint overlay. emitConsumedShortcut no-ops on undefined. + emitConsumedShortcut(renderer ?? undefined) renderer?.send('ui:worktreeHistoryNavigate', action.direction) return } @@ -250,6 +257,7 @@ export function setupGuestShortcutForwarding(args: { ) { event.preventDefault() const renderer = resolveRenderer(browserTabId) + emitConsumedShortcut(renderer ?? undefined) renderer?.send('ui:switchTabAcrossAllTypes', input.code === 'BracketRight' ? 1 : -1) return } @@ -260,6 +268,7 @@ export function setupGuestShortcutForwarding(args: { if (isTerminalTabSwitchChord(input)) { event.preventDefault() const renderer = resolveRenderer(browserTabId) + emitConsumedShortcut(renderer ?? undefined) renderer?.send('ui:switchTerminalTab', input.code === 'PageDown' ? 1 : -1) return } @@ -277,53 +286,81 @@ export function setupGuestShortcutForwarding(args: { return } - if (input.code === 'KeyB' && input.shift) { - renderer.send('ui:newBrowserTab') - } else if (input.code === 'KeyT' && !input.shift) { - // Why: once focus is inside a browser guest, Cmd/Ctrl+T should extend - // the current browser workspace with another internal page instead of - // creating a sibling Orca terminal tab. The renderer still decides - // whether that means "new page in this workspace" or "new workspace" - // based on the current active surface. - renderer.send('ui:newBrowserTab') - } else if (input.code === 'KeyL' && !input.shift) { - // Why: the address bar lives in the renderer chrome, not the guest - // page. Forward Cmd/Ctrl+L out of the guest so the active BrowserPane - // can focus its own input just like a standalone browser would. - renderer.send('ui:focusBrowserAddressBar') - } else if (input.code === 'KeyR' && input.shift) { - // Why: Cmd/Ctrl+Shift+R is the browser convention for hard reload - // (bypass cache). The guest would handle it natively, but Orca's webview - // reloadIgnoringCache() call must come from the renderer side so it goes - // through the same parked-webview ref that owns the guest surface. - renderer.send('ui:hardReloadBrowserPage') - } else if (input.code === 'KeyR' && !input.shift) { - // Why: same as above for soft reload — Cmd/Ctrl+R must be forwarded so - // the renderer can call reload() on its own webview ref rather than - // relying on the guest's built-in shortcut, which may not reach the - // parked-webview eviction logic. - renderer.send('ui:reloadBrowserPage') - } else if (input.code === 'KeyF' && !input.shift) { - // Why: Cmd/Ctrl+F must be forwarded out of the guest so the renderer can - // open its own find-in-page bar and call webview.findInPage(). Letting the - // guest handle it natively would open Chromium's built-in find UI inside - // the guest frame, which is invisible behind Orca's chrome. - renderer.send('ui:findInBrowserPage') - } else if (input.code === 'KeyW' && !input.shift) { - renderer.send('ui:closeActiveTab') - } else if (input.shift && (input.code === 'BracketRight' || input.code === 'BracketLeft')) { - renderer.send('ui:switchTab', input.code === 'BracketRight' ? 1 : -1) - } else if (action?.type === 'toggleWorktreePalette') { - renderer.send('ui:toggleWorktreePalette') - } else if (action?.type === 'openQuickOpen') { - renderer.send('ui:openQuickOpen') - } else if (action?.type === 'openNewWorkspace') { - renderer.send('ui:openNewWorkspace', action.tab) - } else if (action?.type === 'jumpToWorktreeIndex') { - renderer.send('ui:jumpToWorktreeIndex', action.index) - } else { + // Why: resolve which forwarded-action IPC this chord maps to BEFORE + // actually sending it. This lets us emit the consumed-shortcut signal + // ONCE, immediately before the action send, for every matched branch + // (honoring the main→renderer ordering requirement documented on + // emitConsumedShortcut). The unmatched null return exits without emit + // so main doesn't claim it consumed a chord it's going to let through. + const dispatch = ((): (() => void) | null => { + if (input.code === 'KeyB' && input.shift) { + return () => renderer.send('ui:newBrowserTab') + } + if (input.code === 'KeyT' && !input.shift) { + // Why: once focus is inside a browser guest, Cmd/Ctrl+T should extend + // the current browser workspace with another internal page instead of + // creating a sibling Orca terminal tab. The renderer still decides + // whether that means "new page in this workspace" or "new workspace" + // based on the current active surface. + return () => renderer.send('ui:newBrowserTab') + } + if (input.code === 'KeyL' && !input.shift) { + // Why: the address bar lives in the renderer chrome, not the guest + // page. Forward Cmd/Ctrl+L out of the guest so the active BrowserPane + // can focus its own input just like a standalone browser would. + return () => renderer.send('ui:focusBrowserAddressBar') + } + if (input.code === 'KeyR' && input.shift) { + // Why: Cmd/Ctrl+Shift+R is the browser convention for hard reload + // (bypass cache). The guest would handle it natively, but Orca's + // webview reloadIgnoringCache() call must come from the renderer side + // so it goes through the same parked-webview ref that owns the guest. + return () => renderer.send('ui:hardReloadBrowserPage') + } + if (input.code === 'KeyR' && !input.shift) { + // Why: same as above for soft reload — Cmd/Ctrl+R must be forwarded so + // the renderer can call reload() on its own webview ref rather than + // relying on the guest's built-in shortcut, which may not reach the + // parked-webview eviction logic. + return () => renderer.send('ui:reloadBrowserPage') + } + if (input.code === 'KeyF' && !input.shift) { + // Why: Cmd/Ctrl+F must be forwarded out of the guest so the renderer + // can open its own find-in-page bar and call webview.findInPage(). + // Letting the guest handle it natively would open Chromium's built-in + // find UI inside the guest frame, which is invisible behind Orca's + // chrome. + return () => renderer.send('ui:findInBrowserPage') + } + if (input.code === 'KeyW' && !input.shift) { + return () => renderer.send('ui:closeActiveTab') + } + if (input.shift && (input.code === 'BracketRight' || input.code === 'BracketLeft')) { + return () => renderer.send('ui:switchTab', input.code === 'BracketRight' ? 1 : -1) + } + if (action?.type === 'toggleWorktreePalette') { + return () => renderer.send('ui:toggleWorktreePalette') + } + if (action?.type === 'openQuickOpen') { + return () => renderer.send('ui:openQuickOpen') + } + if (action?.type === 'openNewWorkspace') { + return () => renderer.send('ui:openNewWorkspace', action.tab) + } + if (action?.type === 'jumpToWorktreeIndex') { + return () => renderer.send('ui:jumpToWorktreeIndex', action.index) + } + return null + })() + if (!dispatch) { return } + // Why: the chord's non-modifier key never reaches the renderer's + // window-level listener when focus is inside a guest webContents, so + // useModifierHint cannot self-clear. Emit the consumed-shortcut signal + // before the action send (single call covers every matched branch above). + emitConsumedShortcut(renderer) + dispatch() // Why: preventDefault stops the guest page from also processing the chord // (e.g. Cmd+T opening a browser-internal new-tab page). event.preventDefault() diff --git a/src/main/index.ts b/src/main/index.ts index 4f4c5169246..862055ddaf6 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -478,6 +478,12 @@ app.whenReady().then(async () => { emitConsumedShortcut(mainWindow?.webContents) mainWindow?.webContents.send('ui:toggleRightSidebar') }, + // Why: intentionally does NOT call emitConsumedShortcut. The Appearance + // submenu items (statusBarVisible, showTasksButton, showTitlebarAgentActivity) + // have no keyboard accelerators — they're menu-click only — so there is no + // intercepted chord and no modifier hint to clear. If any appearance item + // gains an accelerator in the future, add + // `emitConsumedShortcut(mainWindow?.webContents)` at the top of this callback. onToggleAppearance: (key) => { if (!store) { return diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index 17950069404..fddf4e76d3b 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -546,10 +546,11 @@ export function createMainWindow( // before-input-event fires, but still emit Electron's zoom command. We // reroute that command to terminal zoom so zoom-out remains reachable. event.preventDefault() - emitConsumedShortcut(mainWindow.webContents) if (zoomDirection === 'in') { + emitConsumedShortcut(mainWindow.webContents) mainWindow.webContents.send('terminal:zoom', 'in') } else if (zoomDirection === 'out') { + emitConsumedShortcut(mainWindow.webContents) mainWindow.webContents.send('terminal:zoom', 'out') } }) diff --git a/src/main/window/emit-consumed-shortcut.ts b/src/main/window/emit-consumed-shortcut.ts index cd44e8da22c..e0b2f29d7b1 100644 --- a/src/main/window/emit-consumed-shortcut.ts +++ b/src/main/window/emit-consumed-shortcut.ts @@ -1,12 +1,19 @@ import type { WebContents } from 'electron' -export const MAIN_CONSUMED_SHORTCUT_CHANNEL = 'ui:shortcutConsumed' +const MAIN_CONSUMED_SHORTCUT_CHANNEL = 'ui:shortcutConsumed' /** - * Notify the renderer that main just consumed a platform-modifier chord - * (Cmd/Ctrl+X) via `before-input-event` `preventDefault()` or a native menu - * accelerator. Sent alongside any forwarded action IPC so the renderer's - * `useModifierHint` hook can clear pending number-badge state. + * Notify the renderer that main is forwarding a shortcut-action IPC so the + * renderer's `useModifierHint` hook can clear any pending number-badge state. + * + * Fires whenever main dispatches a shortcut-action IPC to the renderer: + * - an intercepted platform-modifier chord (`before-input-event` + + * `preventDefault()`), OR + * - any menu-item activation that calls `emitConsumedShortcut` in its click + * handler — this covers both keyboard accelerators AND mouse clicks on the + * same menu item, because click handlers don't distinguish the two. The + * renderer's clear is idempotent (no-op when no hint is visible), so + * spurious emits from mouse-triggered menu clicks are harmless. * * Why this exists: when main intercepts a Cmd-chord, the non-modifier key's * keydown never reaches the renderer's window-level listener, so the hook's @@ -15,6 +22,11 @@ export const MAIN_CONSUMED_SHORTCUT_CHANNEL = 'ui:shortcutConsumed' * preventDefault'd chord, so `onKeyUp`-based clearing is unreliable too. * A single authoritative signal from main is the stable fix; individual IPC * handlers in the renderer then do not need to remember to dispatch a clear. + * + * Ordering: callers MUST invoke this BEFORE sending the action IPC so the + * renderer clears the hint before the action handler runs. This relies on + * FIFO delivery on the IPC pipe between main and the renderer (ordering + * preserved across channels for a single WebContents). */ export function emitConsumedShortcut(webContents: WebContents | undefined): void { webContents?.send(MAIN_CONSUMED_SHORTCUT_CHANNEL)