mirror of
https://github.com/stablyai/orca.git
synced 2026-10-18 08:03:22 +00:00
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 <help@stably.ai>
This commit is contained in:
@@ -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<typeof vi.fn>
|
||||
let guestOnMock: ReturnType<typeof vi.fn>
|
||||
let guestOffMock: ReturnType<typeof vi.fn>
|
||||
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<Electron.Input>): 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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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')
|
||||
}
|
||||
})
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user