mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 16:03:20 +00:00
fix(ime): decide IME ownership once, at the shortcut matcher
Application shortcut dispatchers are capture-phase siblings on `window`. Returning early from one does not stop the others, and the terminal's guard deliberately does not preventDefault because the IME still needs the key. So guarding listeners one at a time cannot hold: a composing Mod+Alt+W still resolved `tab.closeAll` in the Terminal dispatcher and closed every editor tab mid-preedit. Move the decision to the matcher every dispatcher already shares. `keybindingMatchesAction` and `matchKeybindingDigitIndex` now refuse input carrying any IME ownership marker, and `KeybindingInput` carries those markers straight off the DOM event. The terminal is the one surface that must resolve a chord anyway, because input-source switching and deferred Enter are legitimate mid-composition; it opts out via `allowImeOwnedInput` through a single local wrapper, so its call sites are unchanged and ownership is still judged after the action is known. Plugin commands resolve against the low-level matcher and so did not inherit the refusal. They can be bound to any chord and run arbitrary code, so they get the same rule explicitly. The renderer's `isImeCompositionKeyDown` now imports the shared predicate rather than restating the three markers, so the two cannot drift. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
import {
|
||||
keybindingMatchesAction,
|
||||
keybindingMatchesAction as matchKeybindingActionAllowingIme,
|
||||
type KeybindingActionId,
|
||||
type KeybindingInput,
|
||||
type KeybindingMatchOptions,
|
||||
type KeybindingOverrides,
|
||||
@@ -7,6 +8,25 @@ import {
|
||||
} from '../../../../shared/keybindings'
|
||||
import type { WindowsShiftEnterEncoding } from './terminal-windows-shift-enter'
|
||||
|
||||
/**
|
||||
* The matcher refuses IME-owned input by default so no application dispatcher can act on a
|
||||
* keystroke the user is still composing. The terminal is the one surface that must resolve
|
||||
* it anyway: `terminalShortcutIsOwnedByIme` decides ownership *after* seeing which action a
|
||||
* chord names, because input-source switching and deferred Enter are legitimate mid-preedit.
|
||||
*/
|
||||
function keybindingMatchesAction(
|
||||
actionId: KeybindingActionId,
|
||||
input: KeybindingInput,
|
||||
platform: NodeJS.Platform,
|
||||
overrides?: KeybindingOverrides,
|
||||
options: KeybindingMatchOptions = {}
|
||||
): boolean {
|
||||
return matchKeybindingActionAllowingIme(actionId, input, platform, overrides, {
|
||||
...options,
|
||||
allowImeOwnedInput: true
|
||||
})
|
||||
}
|
||||
|
||||
export type TerminalShortcutEvent = {
|
||||
key: string
|
||||
code?: string
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import type { KeyboardEvent as ReactKeyboardEvent } from 'react'
|
||||
import { keybindingInputIsImeOwned } from '../../../shared/keybindings'
|
||||
|
||||
// The one place that decides a keystroke belongs to an IME.
|
||||
// See the IME Composition rules in AGENTS.md before changing any of this.
|
||||
@@ -97,11 +98,7 @@ export function isImeCompositionKeyDown(event: AnyKeyboardEvent): boolean {
|
||||
if (!nativeEvent) {
|
||||
return isDocumentImeCompositionActive()
|
||||
}
|
||||
if (
|
||||
nativeEvent.isComposing === true ||
|
||||
nativeEvent.keyCode === 229 ||
|
||||
nativeEvent.key === 'Process'
|
||||
) {
|
||||
if (keybindingInputIsImeOwned(nativeEvent)) {
|
||||
return true
|
||||
}
|
||||
// Why: an Escape the IME owns is marked above, so an unmarked one means this
|
||||
|
||||
@@ -77,6 +77,23 @@ describe('plugin command keybindings', () => {
|
||||
).toBeNull()
|
||||
})
|
||||
|
||||
// A plugin command can be bound to any chord and can run arbitrary plugin code, so it must
|
||||
// not resolve mid-composition. This resolver bypasses `keybindingMatchesAction`, so it does
|
||||
// not inherit that matcher's refusal.
|
||||
it('refuses to resolve a plugin command while an IME owns the keystroke', () => {
|
||||
const actionId = pluginCommandKeybindingActionId(command)
|
||||
const overrides = { [actionId]: ['Mod+Shift+T'] }
|
||||
const base = { key: 't', code: 'KeyT', control: true, meta: false, alt: false, shift: true }
|
||||
|
||||
expect(findPluginCommandForKeybinding([command], base, 'linux', overrides, true)).toBe(command)
|
||||
|
||||
for (const marker of [{ isComposing: true }, { keyCode: 229 }]) {
|
||||
expect(
|
||||
findPluginCommandForKeybinding([command], { ...base, ...marker }, 'linux', overrides, true)
|
||||
).toBeNull()
|
||||
}
|
||||
})
|
||||
|
||||
it('reports conflicts between plugin and built-in definitions', () => {
|
||||
const actionId = pluginCommandKeybindingActionId(command)
|
||||
const definitions = [...KEYBINDING_DEFINITIONS, pluginCommandKeybindingDefinition(command)]
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import type { ActivePluginCommand } from '@/store/plugin-panels'
|
||||
import {
|
||||
getEffectiveKeybindingsForDefinition,
|
||||
keybindingInputIsImeOwned,
|
||||
keybindingMatchesInput,
|
||||
type KeybindingDefinition,
|
||||
type KeybindingInput,
|
||||
@@ -60,6 +61,12 @@ export function findPluginCommandForKeybinding(
|
||||
overrides: KeybindingOverrides | undefined,
|
||||
hasActiveWorktree: boolean
|
||||
): ActivePluginCommand | null {
|
||||
// This resolves against `keybindingMatchesInput` directly rather than through
|
||||
// `keybindingMatchesAction`, so it does not inherit that matcher's IME refusal. A plugin
|
||||
// command can be bound to any chord and can do anything, so it gets the same rule.
|
||||
if (keybindingInputIsImeOwned(input)) {
|
||||
return null
|
||||
}
|
||||
for (const command of commands) {
|
||||
if (command.context === 'worktree' && !hasActiveWorktree) {
|
||||
continue
|
||||
|
||||
@@ -0,0 +1,130 @@
|
||||
// The shortcut matcher is the one place every dispatcher agrees on, so it is where IME
|
||||
// ownership is decided. Guarding the window listeners individually does not hold: they are
|
||||
// capture-phase siblings on `window`, returning early from one does not stop the others, and
|
||||
// the terminal's guard deliberately does not preventDefault (the IME still needs the key).
|
||||
//
|
||||
// The concrete failure this pins: a composing Mod+Alt+W resolved `tab.closeAll` in the
|
||||
// Terminal dispatcher and closed every editor tab in the worktree mid-preedit.
|
||||
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import {
|
||||
keybindingInputIsImeOwned,
|
||||
keybindingMatchesAction,
|
||||
matchKeybindingDigitIndex,
|
||||
type KeybindingInput
|
||||
} from './keybindings'
|
||||
|
||||
const TERMINAL_CONTEXT = { context: 'terminal', terminalShortcutPolicy: 'orca-first' } as const
|
||||
|
||||
const closeAllTabs = (over: Partial<KeybindingInput> = {}): KeybindingInput => ({
|
||||
key: 'w',
|
||||
code: 'KeyW',
|
||||
metaKey: true,
|
||||
altKey: true,
|
||||
ctrlKey: false,
|
||||
shiftKey: false,
|
||||
...over
|
||||
})
|
||||
|
||||
describe('keybindingInputIsImeOwned', () => {
|
||||
it.each([
|
||||
['isComposing', { key: 'w', isComposing: true }],
|
||||
['keyCode 229', { key: 'w', keyCode: 229 }],
|
||||
['the Process key', { key: 'Process' }]
|
||||
])('recognizes %s', (_marker, input) => {
|
||||
expect(keybindingInputIsImeOwned(input)).toBe(true)
|
||||
})
|
||||
|
||||
it('leaves an ordinary keystroke alone', () => {
|
||||
expect(keybindingInputIsImeOwned({ key: 'w', keyCode: 87, isComposing: false })).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('keybindingMatchesAction refuses input an IME owns', () => {
|
||||
it('matches a destructive chord the user actually pressed', () => {
|
||||
expect(
|
||||
keybindingMatchesAction('tab.closeAll', closeAllTabs(), 'darwin', undefined, TERMINAL_CONTEXT)
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
// Regression: this returned true, and Terminal.tsx's capture listener called
|
||||
// handleCloseAllFiles() while the user was mid-composition.
|
||||
it.each([
|
||||
['isComposing', closeAllTabs({ isComposing: true })],
|
||||
['keyCode 229', closeAllTabs({ keyCode: 229 })]
|
||||
])('refuses to close every tab on a chord marked by %s', (_marker, input) => {
|
||||
expect(
|
||||
keybindingMatchesAction('tab.closeAll', input, 'darwin', undefined, TERMINAL_CONTEXT)
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('refuses a marked chord on Windows and Linux too', () => {
|
||||
const ctrlAltW = closeAllTabs({ metaKey: false, ctrlKey: true, isComposing: true })
|
||||
expect(
|
||||
keybindingMatchesAction('tab.closeAll', ctrlAltW, 'win32', undefined, TERMINAL_CONTEXT)
|
||||
).toBe(false)
|
||||
expect(
|
||||
keybindingMatchesAction('tab.closeAll', ctrlAltW, 'linux', undefined, TERMINAL_CONTEXT)
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
// The terminal resolves first and judges ownership after, because input-source switching
|
||||
// and deferred Enter are legitimate mid-composition. Nothing else may set this.
|
||||
it('still resolves for a caller that owns the IME decision itself', () => {
|
||||
expect(
|
||||
keybindingMatchesAction(
|
||||
'tab.closeAll',
|
||||
closeAllTabs({ isComposing: true }),
|
||||
'darwin',
|
||||
undefined,
|
||||
{
|
||||
...TERMINAL_CONTEXT,
|
||||
allowImeOwnedInput: true
|
||||
}
|
||||
)
|
||||
).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe('matchKeybindingDigitIndex refuses input an IME owns', () => {
|
||||
// Cmd+1-9 on macOS.
|
||||
const selectWorkspace = (over: Partial<KeybindingInput> = {}): KeybindingInput => ({
|
||||
key: '2',
|
||||
code: 'Digit2',
|
||||
metaKey: true,
|
||||
altKey: false,
|
||||
ctrlKey: false,
|
||||
shiftKey: false,
|
||||
...over
|
||||
})
|
||||
|
||||
it('resolves an unmarked digit chord', () => {
|
||||
expect(matchKeybindingDigitIndex('workspace.selectByIndex', selectWorkspace(), 'darwin')).toBe(
|
||||
1
|
||||
)
|
||||
})
|
||||
|
||||
// Digits carry an IME meaning of their own (candidate selection), so a marked digit keydown
|
||||
// arriving while the modifier is still physically held is a live collision, not a hypothetical.
|
||||
it('returns null while the IME owns the digit', () => {
|
||||
expect(
|
||||
matchKeybindingDigitIndex(
|
||||
'workspace.selectByIndex',
|
||||
selectWorkspace({ isComposing: true }),
|
||||
'darwin'
|
||||
)
|
||||
).toBeNull()
|
||||
})
|
||||
|
||||
it('still resolves for a caller that owns the IME decision itself', () => {
|
||||
expect(
|
||||
matchKeybindingDigitIndex(
|
||||
'workspace.selectByIndex',
|
||||
selectWorkspace({ isComposing: true }),
|
||||
'darwin',
|
||||
undefined,
|
||||
{ allowImeOwnedInput: true }
|
||||
)
|
||||
).toBe(1)
|
||||
})
|
||||
})
|
||||
@@ -21,6 +21,14 @@ export type TerminalShortcutPolicy = 'orca-first' | 'terminal-first'
|
||||
export type KeybindingMatchOptions = {
|
||||
context?: KeybindingContext
|
||||
terminalShortcutPolicy?: TerminalShortcutPolicy
|
||||
/**
|
||||
* Resolve the action even when an IME owns the keystroke.
|
||||
*
|
||||
* Only for callers that make the ownership decision themselves and need to know what the
|
||||
* chord *would* do first — the terminal panes, which must still honour input-source
|
||||
* switching and deferred Enter mid-composition. Application dispatchers must never set it.
|
||||
*/
|
||||
allowImeOwnedInput?: boolean
|
||||
}
|
||||
|
||||
export type AgentTabActionId = `tab.newAgent.${TuiAgent}`
|
||||
@@ -170,6 +178,24 @@ export type KeybindingInput = {
|
||||
shiftKey?: boolean
|
||||
// Set only by the double-tap detector; always a physical token (never 'Mod').
|
||||
doubleTapModifier?: PhysicalModifierToken
|
||||
// IME ownership markers, read straight off the DOM event. See keybindingInputIsImeOwned.
|
||||
isComposing?: boolean
|
||||
keyCode?: number
|
||||
}
|
||||
|
||||
/**
|
||||
* True when an IME owns this keystroke, so no shortcut may resolve from it.
|
||||
*
|
||||
* This is the event-shape half of the renderer's `isImeCompositionKeyDown`, which layers a
|
||||
* document-wide composition flag on top; both must agree on these three markers, so the
|
||||
* renderer imports this rather than restating it.
|
||||
*
|
||||
* It lives at the matcher because guarding each window listener does not scale: the
|
||||
* listeners are capture-phase siblings on `window`, one guard does not stop the others, and
|
||||
* a chord that resolves `tab.closeAll` mid-composition closes the user's editor tabs.
|
||||
*/
|
||||
export function keybindingInputIsImeOwned(input: KeybindingInput): boolean {
|
||||
return input.isComposing === true || input.keyCode === 229 || input.key === 'Process'
|
||||
}
|
||||
|
||||
type ParsedKeybinding = {
|
||||
@@ -2143,6 +2169,9 @@ export function keybindingMatchesAction(
|
||||
if (!definition) {
|
||||
return false
|
||||
}
|
||||
if (!options.allowImeOwnedInput && keybindingInputIsImeOwned(input)) {
|
||||
return false
|
||||
}
|
||||
if (!keybindingIsActiveInContext(definition, options)) {
|
||||
return false
|
||||
}
|
||||
@@ -2173,6 +2202,9 @@ export function matchKeybindingDigitIndex(
|
||||
if (!definition || !keybindingIsActiveInContext(definition, options)) {
|
||||
return null
|
||||
}
|
||||
if (!options.allowImeOwnedInput && keybindingInputIsImeOwned(input)) {
|
||||
return null
|
||||
}
|
||||
const digit = digitFromInput(input, platform)
|
||||
if (!digit) {
|
||||
return null
|
||||
|
||||
Reference in New Issue
Block a user