mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
fix(keybindings): report conflicts with shortcuts the main process consumes first (#10780)
* fix(keybindings): report conflicts between terminal shortcuts and chords that fire in terminals Actions that reach a focused terminal now share a conflict bucket, and the Shortcuts pane judges the bindings as written in the file instead of the map the loader already stripped. * fix(keybindings): flag only the ignored binding and say it was ignored The other claimant keeps its own default and never stopped working, so a warning on its row blamed an untouched action. Adds the e2e regression that seeds a conflicting file inside the isolated E2E home. * test(keybindings): capture the shortcuts pane at each stage of the conflict report * test(keybindings): assert the untouched action carries no warning Visibility alone would still pass if a regression warned on both rows. * fix(keybindings): narrow the ignored-binding guard and tighten its negative test Review follow-ups: - Only warn when the file asked for a binding and the loader took it away. The guard now also requires the action to be absent from the active map, so an override that survived is never labelled "was ignored". - The negative case used view.tasks, which has no default binding, so the conflict assertion passed without exercising the terminal bucket at all. It now uses worktree.quickOpen and asserts the definition exists, is not terminal-reachable, and has a non-empty default list. * fix(keybindings): compare main-process shortcuts against every scope - List the actions resolveWindowShortcutAction consumes in main and put them in every scope's conflict bucket, so a binding elsewhere on the same chord is reported instead of silently losing it. - Browser bindings are compared only against the actions a focused guest forwards ahead of browser chords. - Dictation stays out, since main lets its chord through while voice is off (the default). - The two digit ranges may still share a modifier. - Drop the allowInTerminal terminal-bucket rule. - Explain overrides the loader drops on a later pass. - Pin the list to the resolver and the guest order with tests. * fix: report main-intercepted shortcuts in Settings without dropping saved overrides Main-process interception now only feeds the Settings conflict report. The loader, write path and web normalization keep main's scope-only check, so existing keybindings.json overrides load exactly as before. --------- Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> Co-authored-by: Neil <neil@stably.ai>
This commit is contained in:
@@ -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([])
|
||||
})
|
||||
})
|
||||
@@ -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) {
|
||||
|
||||
@@ -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')
|
||||
])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<typeof findKeybindingConflicts>[1],
|
||||
options: Parameters<typeof findKeybindingConflicts>[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([])
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<KeybindingActionId, 'beforeBrowserChords' | 'afterBrowserChords'>
|
||||
> = {
|
||||
'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 = {}
|
||||
|
||||
@@ -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<string>()
|
||||
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)
|
||||
|
||||
@@ -204,4 +204,6 @@ export type KeybindingConflict = {
|
||||
export type FindKeybindingConflictOptions = {
|
||||
ignoredActionIds?: Iterable<KeybindingActionId>
|
||||
relevantActionIds?: Iterable<KeybindingActionId>
|
||||
/** Settings-only diagnostic; the loader and writes keep the scope-only check so saved overrides still load. */
|
||||
includeMainProcessInterception?: boolean
|
||||
}
|
||||
|
||||
@@ -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
|
||||
)
|
||||
)
|
||||
}
|
||||
)
|
||||
})
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user