diff --git a/src/main/browser/browser-guest-shortcut-precedence.test.ts b/src/main/browser/browser-guest-shortcut-precedence.test.ts new file mode 100644 index 00000000000..9b442763ee0 --- /dev/null +++ b/src/main/browser/browser-guest-shortcut-precedence.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, it, vi } from 'vitest' + +vi.mock('electron', async () => { + const { EventEmitter } = await import('node:events') + return { + BrowserWindow: class { + webContents = new (class extends EventEmitter { + send = vi.fn() + reloadIgnoringCache = (): void => this.send('reloadIgnoringCache') + })() + }, + screen: { getCursorScreenPoint: vi.fn(() => ({ x: 0, y: 0 })) }, + webContents: { fromId: vi.fn() } + } +}) + +import { BrowserWindow } from 'electron' +import { + findKeybindingConflicts, + isKeybindingInterceptedByMainProcess, + KEYBINDING_DEFINITIONS, + type KeybindingOverrides +} from '../../shared/keybindings' +import { setupGuestShortcutForwarding } from './browser-guest-shortcut-forwarding' + +const CHORD = 'Mod+Alt+Shift+1' + +function rendererMessagesForGuestChord(keybindings: KeybindingOverrides): unknown[][] { + const guest = new BrowserWindow().webContents + const renderer = new BrowserWindow().webContents + const cleanup = setupGuestShortcutForwarding({ + browserTabId: 'tab-1', + guest, + resolveRenderer: () => renderer, + resolveWorkspaceId: () => 'workspace-1', + shouldForwardDictationShortcut: () => true, + getKeybindings: () => keybindings + }) + const isMac = process.platform === 'darwin' + const event: Electron.Event = { preventDefault: vi.fn(), defaultPrevented: false } + const input: Electron.Input = { + type: 'keyDown', + code: 'Digit1', + key: '1', + meta: isMac, + control: !isMac, + alt: true, + shift: true, + isAutoRepeat: false, + isComposing: false, + location: 0, + modifiers: [] + } + guest.emit('before-input-event', event, input) + cleanup() + return vi.mocked(renderer.send).mock.calls +} + +describe('browser guest precedence for main-process shortcuts', () => { + it('reports a browser binding as conflicting exactly when the guest lets the intercepted action win', () => { + const guestDispatchedBrowserActions = KEYBINDING_DEFINITIONS.filter( + (definition) => + definition.scope === 'browser' && + rendererMessagesForGuestChord({ [definition.id]: [CHORD] }).length > 0 + ) + expect(guestDispatchedBrowserActions.length).toBeGreaterThan(0) + + const mismatches: string[] = [] + for (const browser of guestDispatchedBrowserActions) { + const alone = rendererMessagesForGuestChord({ [browser.id]: [CHORD] }) + for (const intercepted of KEYBINDING_DEFINITIONS.filter( + isKeybindingInterceptedByMainProcess + )) { + const shared = { [browser.id]: [CHORD], [intercepted.id]: [CHORD] } + const browserWins = + JSON.stringify(rendererMessagesForGuestChord(shared)) === JSON.stringify(alone) + const reported = + findKeybindingConflicts(process.platform, shared, { + includeMainProcessInterception: true + }).length > 0 + if (reported === browserWins) { + mismatches.push(`${browser.id} vs ${intercepted.id}`) + } + } + } + expect(mismatches).toEqual([]) + }) +}) diff --git a/src/renderer/src/components/settings/shortcut-definition-catalog.ts b/src/renderer/src/components/settings/shortcut-definition-catalog.ts index f219ac04503..9ed736a5f58 100644 --- a/src/renderer/src/components/settings/shortcut-definition-catalog.ts +++ b/src/renderer/src/components/settings/shortcut-definition-catalog.ts @@ -44,7 +44,9 @@ export function buildShortcutDefinitionCatalog(options: { ignoredActionIds: ignoredConflictActionIds, // Plugin defaults are external additions to Orca's conflict-free static // registry, so surface their collisions even before the user customizes one. - relevantActionIds: pluginDefinitions.map((definition) => definition.id) + relevantActionIds: pluginDefinitions.map((definition) => definition.id), + // Why: warn when main claims a custom chord before it reaches the renderer action. + includeMainProcessInterception: true } ) for (const conflict of conflicts) { diff --git a/src/renderer/src/components/settings/shortcut-groups.test.ts b/src/renderer/src/components/settings/shortcut-groups.test.ts index 906d70ce5cb..af69731d932 100644 --- a/src/renderer/src/components/settings/shortcut-groups.test.ts +++ b/src/renderer/src/components/settings/shortcut-groups.test.ts @@ -62,4 +62,18 @@ describe('shortcut groups', () => { expect.stringContaining('Go to File') ]) }) + + it('reports a saved terminal override that main intercepts first', () => { + const catalog = buildShortcutDefinitionCatalog({ + disabledTuiAgents: [], + pluginCommands: [], + keybindings: { 'terminal.splitRight': ['Mod+Alt+ArrowRight'] }, + platform: 'darwin', + missionControlConflictMessage: 'Blocked by Mission Control.' + }) + + expect(catalog.conflictByAction.get('terminal.splitRight')).toEqual([ + expect.stringContaining('Worktree History Forward') + ]) + }) }) diff --git a/src/shared/keybindings-conflicts.test.ts b/src/shared/keybindings-conflicts.test.ts index a8aad3836a2..b8c4ffba541 100644 --- a/src/shared/keybindings-conflicts.test.ts +++ b/src/shared/keybindings-conflicts.test.ts @@ -5,7 +5,8 @@ import { findKeybindingActionsForBinding, findKeybindingConflicts, getEffectiveKeybindingsForAction, - keybindingMatchesAction + keybindingMatchesAction, + KEYBINDING_DEFINITIONS } from './keybindings' describe('keybindings', () => { @@ -196,4 +197,97 @@ describe('keybindings', () => { actionIds: expect.arrayContaining(['sidebar.explorer.toggle', 'worktree.palette']) }) }) + + const findInterceptionConflicts = ( + platform: NodeJS.Platform, + overrides?: Parameters[1], + options: Parameters[2] = {} + ) => + findKeybindingConflicts(platform, overrides, { + ...options, + includeMainProcessInterception: true + }) + + it('keeps main-process interception out of the loader and write conflict check', () => { + expect( + findKeybindingConflicts('darwin', { 'terminal.splitRight': ['Mod+Alt+ArrowRight'] }) + ).toEqual([]) + }) + + it('reports terminal bindings that lose their chord to main-process shortcuts', () => { + expect( + findInterceptionConflicts('darwin', { 'terminal.splitRight': ['Mod+Alt+ArrowRight'] }) + ).toContainEqual({ + binding: 'Mod+Alt+ArrowRight', + actionIds: expect.arrayContaining(['worktree.history.forward', 'terminal.splitRight']) + }) + expect( + findInterceptionConflicts('darwin', { 'terminal.splitDown': ['Mod+Alt+ArrowLeft'] }) + ).toContainEqual({ + binding: 'Mod+Alt+ArrowLeft', + actionIds: expect.arrayContaining(['worktree.history.back', 'terminal.splitDown']) + }) + expect( + findInterceptionConflicts('darwin', { 'terminal.splitRight': ['Mod+P'] }) + ).toContainEqual({ + binding: 'Mod+P', + actionIds: expect.arrayContaining(['worktree.quickOpen', 'terminal.splitRight']) + }) + expect(findInterceptionConflicts('linux', { 'terminal.splitRight': ['Alt+2'] })).toContainEqual( + { + binding: 'Alt+2', + actionIds: expect.arrayContaining(['tab.selectByIndex', 'terminal.splitRight']) + } + ) + }) + + it('reports main-process shortcuts against renderer actions in other scopes', () => { + expect( + findInterceptionConflicts('linux', { + 'workspace.create': ['Mod+Shift+N'], + 'tab.newTerminal': ['Mod+Shift+N'] + }) + ).toContainEqual({ + binding: 'Mod+Shift+N', + actionIds: expect.arrayContaining(['workspace.create', 'tab.newTerminal']) + }) + }) + + it('still lets the two digit ranges share a modifier', () => { + expect( + findInterceptionConflicts('linux', { + 'workspace.selectByIndex': ['Alt+1'], + 'tab.selectByIndex': ['Alt+1'] + }) + ).toEqual([]) + }) + + it('leaves the dictation chord to other scopes, since main lets it through while voice is off', () => { + expect(findInterceptionConflicts('darwin', { 'terminal.splitRight': ['Mod+E'] })).toEqual([]) + }) + + it('checks browser bindings only against main-process shortcuts a focused guest forwards first', () => { + expect( + findInterceptionConflicts('darwin', { 'browser.back': ['Mod+Alt+ArrowLeft'] }) + ).toContainEqual({ + binding: 'Mod+Alt+ArrowLeft', + actionIds: expect.arrayContaining(['worktree.history.back', 'browser.back']) + }) + expect( + findInterceptionConflicts('darwin', { + 'browser.focusAddressBar': ['Mod+L'], + 'sidebar.right.toggle': ['Mod+L'] + }) + ).toEqual([]) + }) + + it('keeps every default binding conflict-free on all platforms', () => { + for (const platform of ['darwin', 'linux', 'win32'] as const) { + expect( + findInterceptionConflicts(platform, undefined, { + relevantActionIds: KEYBINDING_DEFINITIONS.map((definition) => definition.id) + }) + ).toEqual([]) + } + }) }) diff --git a/src/shared/keybindings/effective.ts b/src/shared/keybindings/effective.ts index c8e8a879e4f..28e7e33ecf8 100644 --- a/src/shared/keybindings/effective.ts +++ b/src/shared/keybindings/effective.ts @@ -96,6 +96,43 @@ export function isKeybindingPotentialTerminalConflict(definition: KeybindingDefi return !isKeybindingAllowedInTerminal(definition) } +// Dictation passes through when voice is off; tests pin the other main and guest routing priorities. +const MAIN_PROCESS_INTERCEPTED_ACTIONS: Partial< + Record +> = { + 'worktree.history.back': 'beforeBrowserChords', + 'worktree.history.forward': 'beforeBrowserChords', + 'floatingTerminal.toggle': 'beforeBrowserChords', + 'zoom.in': 'beforeBrowserChords', + 'zoom.out': 'beforeBrowserChords', + 'zoom.reset': 'beforeBrowserChords', + 'app.settings': 'afterBrowserChords', + 'app.forceReload': 'afterBrowserChords', + 'worktree.palette': 'afterBrowserChords', + 'sidebar.left.toggle': 'afterBrowserChords', + 'sidebar.right.toggle': 'afterBrowserChords', + 'worktree.quickOpen': 'afterBrowserChords', + 'workspace.create': 'afterBrowserChords', + 'workspace.delete': 'afterBrowserChords', + 'workspace.openBoard': 'afterBrowserChords', + 'view.tasks': 'afterBrowserChords', + 'dashboard.toggle': 'afterBrowserChords', + 'tab.previousRecent': 'beforeBrowserChords', + 'workspace.selectByIndex': 'afterBrowserChords', + 'tab.selectByIndex': 'afterBrowserChords', + 'tab.openQuickCommandsMenu': 'afterBrowserChords' +} + +export function isKeybindingInterceptedByMainProcess(definition: KeybindingDefinition): boolean { + return MAIN_PROCESS_INTERCEPTED_ACTIONS[definition.id] !== undefined +} + +export function isKeybindingInterceptedBeforeBrowserChords( + definition: KeybindingDefinition +): boolean { + return MAIN_PROCESS_INTERCEPTED_ACTIONS[definition.id] === 'beforeBrowserChords' +} + export function keybindingIsActiveInContext( definition: KeybindingDefinition, options: KeybindingMatchOptions = {} diff --git a/src/shared/keybindings/formatting.ts b/src/shared/keybindings/formatting.ts index c9e2b35ac43..9ad2daff8f7 100644 --- a/src/shared/keybindings/formatting.ts +++ b/src/shared/keybindings/formatting.ts @@ -9,7 +9,12 @@ import type { } from './types' import { KEYBINDING_DEFINITIONS, isDigitIndexActionId, isKeybindingActionId } from './definitions' import { parseKeybinding } from './parser' -import { getEffectiveKeybindingsForAction, getEffectiveKeybindingsForDefinition } from './effective' +import { + getEffectiveKeybindingsForAction, + getEffectiveKeybindingsForDefinition, + isKeybindingInterceptedBeforeBrowserChords, + isKeybindingInterceptedByMainProcess +} from './effective' import { isDoubleTapBinding } from './normalization' import { getKeybindingConflictIdentity, keybindingConflictIdentities } from './matching' @@ -154,6 +159,7 @@ export function findKeybindingConflictsForDefinitions( customizedActions.add(actionId) } } + const scopes = new Set(definitions.map((definition) => definition.scope)) for (const definition of definitions) { if (ignoredActionIds.has(definition.id)) { continue @@ -164,6 +170,17 @@ export function findKeybindingConflictsForDefinitions( // Why: native menu accelerators can consume global chords, so check custom bindings against both the menu bucket and scope. groups.add(definition.scope) } + // Why: main consumes these before renderer handlers run, but a focused browser guest runs its own chords ahead of the after-browser ones. + if ( + options.includeMainProcessInterception && + isKeybindingInterceptedByMainProcess(definition) + ) { + for (const scope of scopes) { + if (scope !== 'browser' || isKeybindingInterceptedBeforeBrowserChords(definition)) { + groups.add(scope) + } + } + } for (const group of groups) { for (const identity of keybindingConflictIdentities(definition.id, binding, platform)) { const conflictKey = `${group}\u0000${identity}` @@ -183,7 +200,16 @@ export function findKeybindingConflictsForDefinitions( const seenConflictKeys = new Set() return Array.from(owners.values()) - .filter(({ actionIds }) => actionIds.size > 1 && setIntersects(actionIds, customizedActions)) + .filter( + ({ actionIds }) => + actionIds.size > 1 && + // Why: the two digit ranges may share a modifier on purpose (see tab.selectByIndex's definition). + !( + options.includeMainProcessInterception && + Array.from(actionIds).every(isDigitIndexActionId) + ) && + setIntersects(actionIds, customizedActions) + ) .map(({ binding, actionIds }) => ({ binding, actionIds: Array.from(actionIds) diff --git a/src/shared/keybindings/types.ts b/src/shared/keybindings/types.ts index edb66379feb..29753ee5dfc 100644 --- a/src/shared/keybindings/types.ts +++ b/src/shared/keybindings/types.ts @@ -204,4 +204,6 @@ export type KeybindingConflict = { export type FindKeybindingConflictOptions = { ignoredActionIds?: Iterable relevantActionIds?: Iterable + /** Settings-only diagnostic; the loader and writes keep the scope-only check so saved overrides still load. */ + includeMainProcessInterception?: boolean } diff --git a/src/shared/window-shortcut-policy-interception.test.ts b/src/shared/window-shortcut-policy-interception.test.ts new file mode 100644 index 00000000000..47573cf293d --- /dev/null +++ b/src/shared/window-shortcut-policy-interception.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from 'vitest' +import { isKeybindingInterceptedByMainProcess, KEYBINDING_DEFINITIONS } from './keybindings' +import { getWindowShortcutActionId, resolveWindowShortcutAction } from './window-shortcut-policy' + +describe('main-process interception list', () => { + it.each(['darwin', 'linux', 'win32'] as const)( + 'names exactly the actions resolveWindowShortcutAction routes on %s, minus dictation', + (platform) => { + const routed = KEYBINDING_DEFINITIONS.filter((definition) => { + const action = resolveWindowShortcutAction( + { + type: 'keyDown', + key: '1', + code: 'Digit1', + meta: platform === 'darwin', + control: platform !== 'darwin', + alt: true, + shift: true + }, + platform, + { [definition.id]: ['Mod+Alt+Shift+1'] } + ) + return action !== null && getWindowShortcutActionId(action) === definition.id + }) + + expect( + routed.map((definition) => definition.id).filter((id) => id !== 'voice.dictation') + ).toEqual( + KEYBINDING_DEFINITIONS.filter(isKeybindingInterceptedByMainProcess).map( + (definition) => definition.id + ) + ) + } + ) +}) diff --git a/tests/e2e/keybinding-terminal-conflict-diagnostic.spec.ts b/tests/e2e/keybinding-terminal-conflict-diagnostic.spec.ts new file mode 100644 index 00000000000..fe962a72b37 --- /dev/null +++ b/tests/e2e/keybinding-terminal-conflict-diagnostic.spec.ts @@ -0,0 +1,47 @@ +import { mkdirSync, writeFileSync } from 'node:fs' +import path from 'node:path' +import { test, expect } from './helpers/orca-app' +import { waitForSessionReady } from './helpers/store' + +const CONFLICTING_FILE = `${JSON.stringify( + { version: 1, keybindings: { 'terminal.splitRight': ['Mod+Alt+ArrowRight'] } }, + null, + 2 +)}\n` + +test.describe('Terminal shortcut conflicting with a chord main intercepts', () => { + test('keeps the saved override and reports the conflict in Settings', async ({ + orcaPage, + electronApp + }) => { + await waitForSessionReady(orcaPage) + + const isolatedHome = await electronApp.evaluate(({ app }) => app.getPath('home')) + mkdirSync(path.join(isolatedHome, '.orca'), { recursive: true }) + writeFileSync(path.join(isolatedHome, '.orca', 'keybindings.json'), CONFLICTING_FILE) + + const savedOverride = await orcaPage.evaluate(async () => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + await store.getState().updateSettings({ uiLanguage: 'en' }) + await store.getState().reloadKeybindings() + store.getState().openSettingsPage() + return store.getState().keybindings['terminal.splitRight'] + }) + expect(savedOverride).toEqual(['Mod+Alt+ArrowRight']) + + const searchInput = orcaPage.getByPlaceholder('Search settings') + await expect(searchInput).toBeVisible() + await searchInput.fill('shortcuts') + await expect(orcaPage.getByRole('heading', { name: 'Shortcuts', exact: true })).toBeVisible() + + await orcaPage.getByRole('button', { name: /Conflicts/ }).click() + await expect(orcaPage.getByText('Split terminal right', { exact: true })).toBeVisible() + await expect(orcaPage.getByText('Worktree History Forward', { exact: true })).toBeVisible() + await expect( + orcaPage.getByText(/conflicts with .*Worktree History Forward/).first() + ).toBeVisible() + }) +})