From 867f38e9dc764add02982bb65142b443ec50ad44 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 2 Aug 2026 00:29:21 -0700 Subject: [PATCH] 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 --- .../terminal-pane/terminal-shortcut-policy.ts | 22 ++- .../src/lib/ime-composition-keyboard-event.ts | 7 +- .../lib/plugin-command-keybindings.test.ts | 17 +++ .../src/lib/plugin-command-keybindings.ts | 7 + src/shared/keybindings-ime-ownership.test.ts | 130 ++++++++++++++++++ src/shared/keybindings.ts | 32 +++++ 6 files changed, 209 insertions(+), 6 deletions(-) create mode 100644 src/shared/keybindings-ime-ownership.test.ts diff --git a/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts b/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts index 57cc3eb4de6..29616c9c53d 100644 --- a/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts +++ b/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts @@ -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 diff --git a/src/renderer/src/lib/ime-composition-keyboard-event.ts b/src/renderer/src/lib/ime-composition-keyboard-event.ts index 78c4deaff16..4d64bddefef 100644 --- a/src/renderer/src/lib/ime-composition-keyboard-event.ts +++ b/src/renderer/src/lib/ime-composition-keyboard-event.ts @@ -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 diff --git a/src/renderer/src/lib/plugin-command-keybindings.test.ts b/src/renderer/src/lib/plugin-command-keybindings.test.ts index b4ddcb00dae..531357dd8db 100644 --- a/src/renderer/src/lib/plugin-command-keybindings.test.ts +++ b/src/renderer/src/lib/plugin-command-keybindings.test.ts @@ -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)] diff --git a/src/renderer/src/lib/plugin-command-keybindings.ts b/src/renderer/src/lib/plugin-command-keybindings.ts index 7d85093b581..fc471429013 100644 --- a/src/renderer/src/lib/plugin-command-keybindings.ts +++ b/src/renderer/src/lib/plugin-command-keybindings.ts @@ -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 diff --git a/src/shared/keybindings-ime-ownership.test.ts b/src/shared/keybindings-ime-ownership.test.ts new file mode 100644 index 00000000000..a9cceedfe2a --- /dev/null +++ b/src/shared/keybindings-ime-ownership.test.ts @@ -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 => ({ + 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 => ({ + 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) + }) +}) diff --git a/src/shared/keybindings.ts b/src/shared/keybindings.ts index 4fd980aa402..6fca09fbc41 100644 --- a/src/shared/keybindings.ts +++ b/src/shared/keybindings.ts @@ -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