fix(terminal): send a composing cursor chord once, not twice - #14742
kunsanglee wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds IME-exempt chord detection and release-based recovery for terminal shortcuts. It records pending composing chords, reconstructs matching keyup events, and clears state during blur and cleanup. New fixtures and tests cover Korean, Kotoeri, Chinese, ABC, remapped commands, composition ordering, and cancellation. macOS drivers, probes, Swift utilities, and gated end-to-end suites trace input through the main process, renderer, composition layer, terminal, and PTY. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
src/renderer/src/components/terminal-pane/keyboard-handlers.ts (1)
74-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the explanatory comment blocks.
The coding guidelines require concise, brief comments and prefer one line. Lines 74-98, Lines 119-131, and Lines 1121-1128 are multi-paragraph narratives that walk through recorded trace behavior and code flow.
Keep the non-obvious facts, such as the Korean replay versus Japanese swallow distinction and the prototype-accessor warning at Lines 100-103. Move the recorded trace details and the TODO rationale into the trace fixture files or the PR description.
As per coding guidelines: "Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code."
Source: Coding guidelines
src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-exempt-chord-resolution.test.ts (1)
130-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-composing negative case to the gate table.
Both
it.eachtables forceisComposing: true. No row proves thatisImeExemptTerminalChordreturnsfalsewhen no composition is live. That is the condition which prevents the handler from arming a carry for a chord that already resolved on its own keydown. The behavior is covered indirectly atkeyboard-handlers.issue-12871-recorded-chord-traces.test.tsLine 759, but not pinned at the gate itself.💚 Proposed additional row
it.each([ ['Cmd+ArrowLeft', keyEvent({ key: 'ArrowLeft', code: 'ArrowLeft', metaKey: true })],Add a separate assertion after the tables:
it('does not remember an exempt chord pressed outside a composition', () => { expect( isImeExemptTerminalChord(keyEvent({ key: 'ArrowLeft', code: 'ArrowLeft', metaKey: true })) ).toBe(false) })src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.ts (3)
288-298: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDispose the
Terminalinunmount.
openRigcreates a realTerminaland callsterminal.open(container)for every test and everyit.eachrow.unmountremoves the DOM scope but never callsterminal.dispose(). xterm keeps internal listeners, observers, and render timers alive after the container is removed, so each row leaves one live instance behind for the rest of the file.♻️ Proposed change
unmount: () => { hook.unmount() + terminal.dispose() scope.remove() }Also applies to: 371-375
469-473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the committed text into the case data.
The runner hardcodes
'さ'as the commit payload for everycommitsAfterCapturecase. The flag type iscommitsAfterCapture?: true, so a future case with a different preedit would be committed asさwith no signal. Carry the text in the case instead.♻️ Proposed change
if (testCase.commitsAfterCapture) { // Held, not dropped: nothing yet, and the same expectations must hold once it commits. expect(rig.inputCalls).toEqual([]) - await commitComposition(rig.textarea, 'さ') + await commitComposition(rig.textarea, testCase.commitsAfterCapture) }Change the field type in
keyboard-handlers.issue-12871-in-app-chord-traces.tsto the committed string:/** Rows end mid-composition: the text the input source commits after the capture stops. */ commitsAfterCapture?: stringThen set
commitsAfterCapture: 'さ'inkeyboard-handlers.issue-12871-command-release-traces.tsand mirror the field in the localRecordedCasetype.
557-562: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the
findIndexrow lookups.Three tests locate a row by predicate and slice on the result, with no check that the row exists:
- Line 559:
findIndex(...) + 1. If the row is absent the index is-1and the slice is empty, so the test replays nothing and still passes.- Line 733:
findIndex(...)with no offset. If the row is absent,slice(0, -1)silently drops the last row instead of failing.- Line 799:
findIndex(...) + 1, same empty-slice outcome as Line 559.The file already guards case lookup by name in
caseNamedfor this reason. Apply the same discipline to the row lookups.💚 Proposed helper
function rowIndex(rows: RecordedRow[], match: (row: RecordedRow) => boolean): number { const index = rows.findIndex(match) if (index < 0) { throw new Error('recorded row not found in trace') } return index }Then replace each
findIndexcall, for example:const beforeFirstChord = japanese.rows.slice( 0, - japanese.rows.findIndex((row) => row.t === 'keydown' && row.code === 'MetaLeft') + rowIndex(japanese.rows, (row) => row.t === 'keydown' && row.code === 'MetaLeft') )Also applies to: 731-734, 797-800
tests/e2e/macos-input-source-driver.ts (1)
20-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the
execFileSynccalls.No call in this driver sets
timeout.swiftcompiles the script on each invocation, andosascriptblocks until System Events answers. If the machine denies Accessibility permission or a dialog steals focus, the call blocks the Playwright worker until the whole run times out, with no message that points at the driver.🛡️ Proposed change
+const EXEC_TIMEOUT_MS = 30_000 + export function selectInputSource(id: string): void { - execFileSync('swift', [SELECT_INPUT_SOURCE, id]) + execFileSync('swift', [SELECT_INPUT_SOURCE, id], { timeout: EXEC_TIMEOUT_MS }) }Apply the same option to
enableInputSource,focusApp,bounceFocus,typeKeyCodes,pressChordWithSeparateModifier, andpressChord.Also applies to: 52-93
tests/e2e/renderer-chord-event-probe.ts (1)
90-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
focusListenersand consider exporting a dispose helper.Lines 90-102 push the
compositionstart,compositionupdate,compositionend, andinputremovers intofocusListeners. The array then holds focus, visibility, and composition removers, so its name describes only part of its contents.disposestill removes everything, so behavior is correct.This module also exports no dispose function, unlike
tests/e2e/main-process-input-event-probe.ts, which exportsdisposeMainProcessInputProbe. The specs never release the renderer probe explicitly.♻️ Proposed change
- const focusListeners: (() => void)[] = [] + const stateListeners: (() => void)[] = []Rename the remaining
focusListenersreferences at Lines 87, 101, and 107, and add an exporteddisposeChordProbe(page)that mirrorsdisposeMainProcessInputProbe.tests/e2e/terminal-macos-kotoeri-chord-keyup-probe.spec.ts (1)
170-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
KEY.returnKeyand align the composition regex.Three small consistency items in this test:
- Lines 192 and 195 pass the raw key code
36.KEYis already imported and exportsreturnKey: 36.- Line 170 matches
/[\u3041-\u3093]/while Lines 78 and 85 match the same range as/[ぁ-ん]/. Use one form in this file.- Line 191 refers to "the contributor's flushLineToReader". The comment names no file, so it will not help a future reader. Name the spec that defines the helper, or drop the reference.
♻️ Proposed change
- pressChord(processId, 36) + pressChord(processId, KEY.returnKey) let bytes = await waitForTerminalImeBytes(orcaPage, reader, 5_000).catch(() => []) if (bytes.length === 0) { - pressChord(processId, 36) + pressChord(processId, KEY.returnKey) bytes = await waitForTerminalImeBytes(orcaPage, reader, 10_000).catch(() => []) }Also applies to: 190-196
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd96b167-aa74-461f-9213-25125b48408b
📥 Commits
Reviewing files that changed from the base of the PR and between 2b10767 and 8a0e40d58701a2342c5ba9624280b442cf33f38c.
📒 Files selected for processing (15)
src/renderer/src/components/terminal-pane/keyboard-handlers-ime-composing-chord.test.tsxsrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-command-release-traces.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-exempt-chord-resolution.test.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-in-app-chord-traces.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.tssrc/renderer/src/components/terminal-pane/terminal-shortcut-policy.tssrc/shared/keybindings.tstests/e2e/macos-input-source-driver.tstests/e2e/main-process-input-event-probe.tstests/e2e/post-modifier-chord.swifttests/e2e/renderer-chord-event-probe.tstests/e2e/terminal-macos-chord-input-pipeline-probe.spec.tstests/e2e/terminal-macos-ime-cursor-chord-native.spec.tstests/e2e/terminal-macos-kotoeri-chord-keyup-probe.spec.ts
| // 'Process' is Windows' report for a key an IME consumed (#12171): the produced key is | ||
| // genuinely unreportable, which is the same condition as 'Dead' above. | ||
| const PHYSICAL_CODE_FALLBACK_KEYS = new Set(['', 'Dead', 'Unidentified', 'Process']) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find keybinding match call sites and check for composition gating nearby.
rg -n -C 6 '\b(keybindingMatchesAction|matchesKeybinding)\s*\(' --type=ts -g '!**/*.test.*' -g '!**/*.spec.*'Repository: stablyai/orca
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- keybinding matcher definitions and references ---'
rg -n -C 5 'keybindingMatchesAction|matchesKeybinding|PHYSICAL_CODE_FALLBACK_KEYS|keybinding.*Action' src --glob '*.{ts,tsx,js,jsx}' || true
printf '%s\n' '--- composition-related guards near keyboard handlers ---'
rg -n -C 8 'isComposing|composition|KeyboardEvent|keydown|keyup' src/renderer/src src/shared --glob '*.{ts,tsx,js,jsx}' | head -n 1200 || true
printf '%s\n' '--- target file outline and relevant section ---'
ast-grep outline src/shared/keybindings.ts --lang typescript || true
sed -n '1560,1665p' src/shared/keybindings.tsRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- matcher implementation and fallback semantics ---'
rg -n -C 12 'PHYSICAL_CODE_FALLBACK_KEYS|function keybindingMatchesAction|const keybindingMatchesAction|export .*keybindingMatchesAction|function keybindingMatchesInput|isComposing' src/shared/keybindings.ts
printf '%s\n' '--- production call sites only ---'
rg -n '\bkeybindingMatchesAction\s*\(' src --glob '*.{ts,tsx,js,jsx}' --glob '!**/*.test.*' --glob '!**/*.spec.*' \
| sed -n '1,240p'
printf '%s\n' '--- production event handlers that invoke matcher ---'
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('src').rglob('*'):
if p.suffix not in {'.ts','.tsx','.js','.jsx'} or '.test.' in p.name or '.spec.' in p.name:
continue
s=p.read_text(errors='ignore').splitlines()
hits=[i for i,x in enumerate(s) if re.search(r'\bkeybindingMatchesAction\s*\(',x)]
if not hits: continue
print(f'\n## {p}')
for i in hits:
lo=max(0,i-12); hi=min(len(s),i+18)
print(f'-- lines {lo+1}-{hi} --')
print('\n'.join(f'{j+1}: {s[j]}' for j in range(lo,hi)))
PYRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- complete fallback and matching logic ---'
sed -n '1640,1815p' src/shared/keybindings.ts
sed -n '2160,2245p' src/shared/keybindings.ts
printf '%s\n' '--- composition checks in production shortcut paths ---'
rg -n -C 4 'isComposing|compositionstart|compositionend|key === .Process.|key === .Dead.' src --glob '*.{ts,tsx,js,jsx}' --glob '!**/*.test.*' --glob '!**/*.spec.*' | sed -n '1,320p'
printf '%s\n' '--- handlers with direct KeyboardEvent matching and no obvious target gate ---'
sed -n '1,210p' src/renderer/src/app-shell/use-global-keybindings.ts
sed -n '328,355p' src/renderer/src/components/editor/PdfViewer.tsx
sed -n '1,180p' src/renderer/src/components/dictation/use-hold-dictation-gesture.tsRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Process-specific tests and related issue history in source ---'
rg -n -C 10 "'Process'|Process|`#12171`|`#13033`" src tests --glob '*.{ts,tsx,js,jsx}' 2>/dev/null | sed -n '1,500p'
printf '%s\n' '--- all input/event type definitions used by global and browser/main dispatch ---'
rg -n -C 8 'type (ShortcutDispatchInput|.*Shortcut.*Input)|interface (ShortcutDispatchInput|.*Shortcut.*Input)|isComposing|keyCode' \
src/renderer/src/app-shell src/main/browser src/main/window src/shared --glob '*.{ts,tsx,js,jsx}' | sed -n '1,500p'
printf '%s\n' '--- exact global dispatch registration and browser event conversion ---'
rg -n -C 12 'dispatchShortcutInput|before-input-event|input:|KeyboardEvent|new KeyboardEvent|keyCode' \
src/renderer/src/app-shell src/main/browser src/main --glob '*.{ts,tsx,js,jsx}' | sed -n '1,650p'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Process references limited to keybinding sources/tests ---'
rg -n -C 8 'Process|PHYSICAL_CODE_FALLBACK_KEYS' src/shared/keybindings.ts src/shared/keybindings.test.ts src/renderer/src/components/terminal-pane src/renderer/src/lib/ime-composition-keyboard-event.ts
printf '%s\n' '--- action definitions and default bindings ---'
rg -n -C 3 'id:|default:|bindings:|keybinding' src/shared/keybindings.ts | sed -n '1,900p'
printf '%s\n' '--- global listener construction and input conversion ---'
rg -n -C 10 'addEventListener.*keydown|dispatchShortcutInput|ShortcutDispatchInput|toModifierDoubleTapEvent|input =' src/renderer/src/app-shell/use-global-keybindings.ts src/renderer/src/app-shell/app-command-handlers.ts
printf '%s\n' '--- browser guest input conversion only ---'
sed -n '280,440p' src/main/browser/browser-guest-ui.tsRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- compact default binding inventory ---'
python3 - <<'PY'
import re
p='src/shared/keybindings.ts'
lines=open(p, encoding='utf-8').read().splitlines()
for i,line in enumerate(lines):
if re.search(r"(default|bindings|keybinding).*(\+|['\"])", line, re.I):
print(f'{i+1}: {line[:240]}')
PY
printf '%s\n' '--- Process events in compact fixtures summary ---'
python3 - <<'PY'
from pathlib import Path
import json
for p in Path('src').rglob('*Process*'):
pass
for p in Path('src/renderer/src/components/terminal-pane/__fixtures__').glob('*.json'):
try: data=json.loads(p.read_text())
except Exception: continue
events=data if isinstance(data,list) else data.get('events',[])
found=[]
for e in events:
if isinstance(e,dict) and e.get('type')=='keydown' and e.get('key')=='Process':
found.append((e.get('code'), e.get('keyCode'), e.get('isComposing'), {k:e.get(k) for k in ('ctrlKey','metaKey','altKey','shiftKey') if e.get(k)}))
if found:
print(p.name, 'count=',len(found), 'samples=',found[:12])
PY
printf '%s\n' '--- app-level handlers and explicit composition guards ---'
rg -n 'keybindingMatchesAction|isImeOwnedKeyboardEvent|isImeCompositionKeyDown|isComposing|keyCode === 229' \
src/renderer/src/app-shell src/renderer/src/components/browser-pane src/renderer/src/components/editor \
src/renderer/src/components/settings src/renderer/src/components/sidebar src/renderer/src/components/right-sidebar \
--glob '*.{ts,tsx}' --glob '!**/*.test.*' --glob '!**/*.spec.*'Repository: stablyai/orca
Length of output: 13247
Gate IME-owned events before app-level keybinding matching. keybindingMatchesAction now maps key: 'Process' to code, but non-terminal handlers pass composing KeyboardEvent objects without a shared composition guard. A Windows IME event such as Process + KeyF can trigger a physical-code binding, including a user-defined binding. Use isImeOwnedKeyboardEvent before matching these shortcuts.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts (1)
22-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the IME policy comment brief.
Lines 22-34 use 13 lines to describe one predicate. Keep the non-obvious reason, but reduce this to a concise comment.
As per coding guidelines, “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 663d0930-893e-4d3d-a7a7-3d6d302737c4
📥 Commits
Reviewing files that changed from the base of the PR and between 8a0e40d58701a2342c5ba9624280b442cf33f38c and fd9d01b7422da05003d07b0e2316d45315c88907.
📒 Files selected for processing (7)
src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-command-release-traces.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-exempt-chord-resolution.test.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-in-app-chord-traces.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.tssrc/renderer/src/components/terminal-pane/terminal-ime-composition-route.tssrc/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-exempt-chord-resolution.test.ts
- src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-command-release-traces.ts
- src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-in-app-chord-traces.ts
- src/renderer/src/components/terminal-pane/keyboard-handlers.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/src/components/terminal-pane/keyboard-handlers.ts (1)
1098-1103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the added rationale comments.
These blocks run five to eight lines each and restate the reasoning at length. Keep the non-obvious claim and drop the narrative. For example, Line 1098-1103 reduces to: "A still-composing event means the IME consumed the key; an unmarked replay is already on its way."
As per coding guidelines: "Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code."
Also applies to: 1140-1147
Source: Coding guidelines
src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.ts (1)
746-750: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the
findIndexresult.If the fixture loses its
MetaLeftkeydown row,findIndexreturns-1andslice(0, -1)silently replays the whole trace except the last row. The test then exercises a different setup and can still pass. Assert the index first.🧪 Proposed fix
- const beforeFirstChord = japanese.rows.slice( - 0, - japanese.rows.findIndex((row) => row.t === 'keydown' && row.code === 'MetaLeft') - ) + const firstChordIndex = japanese.rows.findIndex( + (row) => row.t === 'keydown' && row.code === 'MetaLeft' + ) + expect(firstChordIndex).toBeGreaterThan(0) + const beforeFirstChord = japanese.rows.slice(0, firstChordIndex)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b012d8bf-2356-410d-9b63-12ddee897a10
📥 Commits
Reviewing files that changed from the base of the PR and between fd9d01b7422da05003d07b0e2316d45315c88907 and d72980b45f71261232c700802a65c39564e36f8b.
📒 Files selected for processing (3)
src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-exempt-chord-resolution.test.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-exempt-chord-resolution.test.ts
|
Review pass addressed in
Gate IME-owned events before app-level keybinding matching. The
The e2e helper duplication. Taken, and the divergence was worse than a naming drift. The literal left-to-right mark and the untimed poll are both fixed as described. Local verification after all three commits: |
|
Confirmed by hand on Thanks for carrying this through three rewrites of the ground underneath it — the revert, the release-keyed design, and now this. Re-recording rather than reasoning each time is what made the diagnosis hold up. Hope this one lands. |
stablyai#14730 fixed the order: a chord resolved mid-composition now waits for the syllable to commit instead of overtaking it. It still resolves that chord on the composing keydown, and on Korean 2-Set the platform then replays the same chord unmarked after keyup, so both copies fire. Cmd+Left hides it by being idempotent; Option+Left sends \x1bb twice and jumps two words. Recorded on stock macOS: the two input sources are indistinguishable while the key is down (both `code='ArrowLeft'`, `keyCode=229`, `isComposing=true`) and have separated by the time it comes up. Korean has committed and reports the composition over — its replay is on the way, so the press must stay silent. Japanese conversion swallowed the chord whole, with no commit and no replay, and is still composing at the release — nothing else will ever deliver it. So an exempt chord (Cmd/Option/Ctrl over ArrowLeft/ArrowRight/Backspace/Delete, never with Shift, which Japanese binds to resize a segment) is remembered on the composing keydown and decided on its release. A release that still reports itself composing runs the action; one that does not lets the replay answer. Cmd+Left delivers no arrow keyup at all, so the Command release ends that gesture. The snapshot is taken field by field because a KeyboardEvent keeps its fields as prototype accessors, and it carries the modifiers as they were at the press. Bytes still take stablyai#14730's deferral, now reached by both paths through one condition rather than two. Also read `code` rather than `key` for those chords while an IME owns the event — a CJK source rewrites `key` to 'Process' (stablyai#12171, stablyai#13033) — and add 'Process' to the keybinding matcher's physical-code fallback beside 'Dead'. keyboard-handlers-ime-composing-chord.test.tsx from stablyai#14730: its chord press now runs to the release, because that is where a swallowed chord is resolvable and what hardware delivers. Same assertions. Fixes stablyai#12871 Co-authored-by: hyeonho <prxyeo@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WjefjguNCzpY2kVq3Q69wH
…ce for Three independent reviews of the previous commit, run without sight of each other, agreed on where it overreached. Arming is now macOS-only. Both behaviours the release reads are stock-macOS captures: that a committing source replays the chord unmarked, and that Cmd+key delivers no keyup. An input source that commits without replaying — unrecorded for ibus and MS-IME — would arm, see a release that no longer reports itself composing, hand the chord to a replay that never comes, and drop it. That is worse than the ordering bug: stablyai#14730 delivered it late, this lost it. Elsewhere the deferred send stands, now pinned by a test on a Windows user agent. Reading the physical code moved out of the shared policy and into the snapshot, which is the only consumer that needed it. The policy is also called by the dashboard popout preview terminal, whose IME gate excludes modifier chords, so a composing Ctrl+Left there resolved to bytes with none of the recovery around it — the ordering bug reintroduced on a surface with no deferral. Direct calls into both trees confirmed it: `{key:'Process', code:'Backspace', ctrlKey:true, isComposing:true}` on win32 returned null before and `\x17` after. The snapshot stores `event.code` as its key instead, which is correct because the caller has already narrowed to four codes whose key is that same string. `'Process'` in the shared keybinding matcher's physical-code fallback goes with it: it short- circuited past a guard written on purpose for AltGr, and nothing needs it now. The arming keydown consumes the press again, as it did before this branch existed. Returning early skipped preventDefault and stopImmediatePropagation, so a global handler bound to the same remapped chord acted on the press while the release acted again — one gesture, two firings. The carry is keyed by code rather than a single slot, following the native-only tracker in the same pane. Two exempt keys can be down under one Cmd hold, and the Command release ends both; a single slot dropped the first without a trace. Also: selectAll joins switchInputSource in the release guard, since both arm the native-only tracker from a press; the file-search branch stops cutting off a keyup the comment three lines above it says must keep propagating; and the composition session events carry the name of the patch that emits them, since upstream xterm does not and a regenerated patch that drops them fails silently into an unbounded wait. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WjefjguNCzpY2kVq3Q69wH
The previous commit made the arming keydown consume the press, to stop a global
handler and this pane both acting on one remapped chord. It consumed too much:
the claim happened before anything decided whether this pane would answer.
`isImeExemptTerminalChord` accepts any of Cmd/Ctrl/Alt over the four codes, and
some of those resolve to nothing in the terminal policy. `Cmd+Alt+Left` is one,
and it is the default binding for worktree history, owned by a window handler
that mounts after this pane. Composing on macOS, the pane took that press,
stopped it reaching the global handler, and then answered for nothing at the
release. Worktree history back stopped working for the length of a composition —
no remap needed, and it worked on the parent commit.
The same early `preventDefault` also broke `switchInputSource`, whose whole
contract is that the OS keeps its default action while xterm is cut off.
One predicate now decides both ends: the press is claimed exactly when the
release would answer for it, so the two cannot drift. `switchInputSource` and
`selectAll` stay out of it — both arm the native-only tracker from a press, which
a release cannot do, so leaving them unclaimed keeps them on the keydown path
where they already work.
Also restores coverage for the snapshot's silent failure mode. The test dropped
in the simplification pass exercised the resolver, not the snapshot, so it could
not have caught a `{ ...event }` regression at all — that is the whole hazard,
since happy-dom keeps KeyboardEvent fields as own properties and Chromium does
not. `imeChordSnapshot` is exported and pinned directly against a prototype-only
object instead.
And records why the release stepping back is believed safe beyond the two
recorded input sources: the replay is Chromium redispatching a key the IME did
not consume, so a source that consumed one is still composing and is handled.
Closing the remaining gap would need a deadline on the replay, which is a second
timer able to fire the chord twice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WjefjguNCzpY2kVq3Q69wH
The claim predicate returned early on a file-search match without looking at the action, so a chord carrying both bindings was claimed on the press, and a release that found no selection fell through to selectAll — arming the native-only tracker from a keyup with no press left to spend it. Resolving the action first applies the exclusion to both paths. Two things about the recording harnesses, both of which could produce a wrong gesture rather than a wrong assertion: `pressChord` existed twice with the same name and different behaviour. The driver's folded the modifier into the target key's flags; the spec's local one sent it as its own key event. Three specs called it and two got the folded shape, which produces no modifier press or release at all — the exact difference that made the Cmd half of the gesture look like it had no end. They are now `pressChordWithFoldedModifier` and `pressChordAsTyped`, so a call site says which gesture it drives. The cursor-chord spec kept its own copies of the input-source constants, the key table, and four helpers whose bodies match the driver's. The key table had already drifted to a different name for Return. Deleted in favour of the shared ones; 83 lines, and nothing left to drift. Also: a literal left-to-right mark in the composition reader is now ``, so a whitespace-stripping tool cannot silently break composition reads, matching the sibling spec; and the assertion that a swallowed chord leaves the preedit alive reads once instead of polling, since the settle it follows means a late preedit would be a failure rather than a pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WjefjguNCzpY2kVq3Q69wH
b1d2c91 to
27f3cf9
Compare
|
Thanks for running it on real hardware — Trailer added in While checking which of your work is still here I found something you will want to know. Two of the three are intact: The third is not.
The Windows one is worth more now than when you wrote it. Review on this PR flagged that everything here is derived from two stock-macOS captures and nothing exercises win32 or linux, which is why the recovery is now gated to macOS — an IME that commits without replaying would otherwise lose the chord entirely rather than deliver it late. Your case is the shape that pins the non-mac side. Say the word and I will re-home all three against One thing I could not find: you mention Chinese fixtures, and I see none in |
|
Thanks for chasing down what survived — and yes please, re-home all three. The Chinese fixtures are on my fork, not on this branch: Two caveats before pulling it in:
On the Windows |
…mposition With committed text on the line and a syllable still composing, a cursor-movement chord relocated the composing character to wherever the cursor landed: Option+Left word-jump turned "가나 다라 마바[사]" into "가나 [사]다라 마바", and Cmd+Right moved the syllable to the line end. Reported with reproduction steps in stablyai#12871. Written originally in 8aec625 against keyboard-handlers-ime-deferred-input.test.tsx, which no branch still has. Rehomed onto keyboard-handlers-ime-composing-chord.test.tsx, whose harness is the same shape, and reshaped for where the chord now resolves: a marked press is remembered for its release rather than deferred to the commit, so the macOS cases run the whole Korean gesture — press, commit, unmarked release, the platform's replay — instead of stopping at the commit. - Option+ArrowLeft / Option+ArrowRight / Cmd+ArrowRight, each a resolver branch no recorded trace reaches - Ctrl+ArrowLeft on Windows, where no release takes part at all The Windows rows were transcribed from the same macOS session rather than captured on win32, so they pin the resolver's non-mac branch and claim no platform coverage. Cmd+ArrowLeft behind a multi-character Japanese preedit is left out: the existing 'holds the chord while the keydown is still marked composing' already commits 日本語 behind the same chord for the same bytes. Its note about the overwritten destination glyph moved onto that test instead of arriving as a second copy of it. Verified non-vacuous against a663f1b, the parent: all three macOS cases fail there with the chord byte twice over ('사', '\eb', '\eb') — the Korean duplicate itself. The Windows case passes there and fails against b849099, before the deferral landed, with the byte ahead of the commit.
The PR body no longer calls Chinese untested — it says Pinyin and Zhuyin are affected. This replaces that citation with measurement, at both layers. Every earlier Chinese capture is void twice over: it predates the Command release being honoured, and it was driven with the modifier folded into the target key's flags, which emits no modifier press or release at all and so could not see the end those gestures turn on. What the recordings say. Both sources swallow the chord and keep composing, like Kotoeri and unlike Korean, and the byte drains at the commit. A Cmd chord's arrow keydown does not reach Chromium's own input dispatch at all and its keyup exists nowhere; the Command release carries it, still marked composing, and both Cmd cells pass only because of that. Option resolves through the arrow's own keyup, as it always did. One gesture is deliberately left unpinned at the unit layer. Recorded here, Zhuyin Option+Left commits on the chord; checked by hand at the same keyboard, the preedit block stays up. Three synthesis regimes were tried against that — a flat 80ms with the modifier as a synthetic key event, then the hardware medians and a much slower run with it posted as a real flagsChanged — and all three committed on the press, so it is not a timing artifact this rig can drive out. Rather than freeze a recording no hand can reproduce, that fixture case is dropped rather than softened. Its e2e case stays, minus the intermediate-composition assertion: both behaviours put the same line on the pty, so the byte order is asserted and the disputed state is not. The e2e cases assert the line's shape rather than a particular reading, because which candidate Zhuyin commits adapts to use — one run committed 你好, another 妳好, and the byte order was the same contract in both. Rebased from 878ae5e with the expectations re-measured, as the mechanism moved on since: all three fixture cases now stop mid-composition and drain at the commit, so each carries the text its source committed and commitsAfterCapture holds that text rather than a bare flag — the rig had been committing さ for every case. Verified non-vacuous by removing the Command-release arm: both Cmd cases fail there alongside the Kotoeri one. The Option case is the half that already worked and passes either way, as its note says.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/macos-input-source-driver.ts (1)
26-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd explicit timeouts to the
execFileSynccalls.Every call blocks the worker with no upper bound. If
osascriptwaits on an Accessibility permission prompt, or ifswiftstalls while compiling, the run hangs instead of failing. The suite already requires that permission, so the prompt path is reachable. Pass atimeoutso a stuck helper surfaces as a test failure.♻️ Proposed change
+// Hand-run rig: a permission prompt or a stuck System Events target would otherwise block the +// worker with no upper bound. +const DRIVER_TIMEOUT_MS = 30_000 + export function selectInputSource(id: string): void { - execFileSync('swift', [SELECT_INPUT_SOURCE, id]) + execFileSync('swift', [SELECT_INPUT_SOURCE, id], { timeout: DRIVER_TIMEOUT_MS }) }Apply the same option to
enableInputSource,focusApp,bounceFocus,typeKeyCodes,pressChordWithSeparateModifier,pressChordWithFoldedModifier, andpressChordAsTyped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28a8fa0d-921e-4df3-b601-aad7fb00cdf2
📥 Commits
Reviewing files that changed from the base of the PR and between b1d2c91797d06635c15be4333b4fc9a10d1a59ee and 7e710f6.
📒 Files selected for processing (7)
src/renderer/src/components/terminal-pane/keyboard-handlers-ime-composing-chord.test.tsxsrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-chinese-chord-traces.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-command-release-traces.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-in-app-chord-traces.tssrc/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.tstests/e2e/macos-input-source-driver.tstests/e2e/terminal-macos-ime-cursor-chord-native.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.ts
- src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-command-release-traces.ts
- src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-in-app-chord-traces.ts
|
I audited the file's other 13 |
|
Re-homed both, authorship intact — GitHub attributes Your caveat about the expected bytes was right, and it cost nothing because you flagged it. All three Chinese cases stop mid-composition, so the byte was still queued and every one asserted The macOS cases from One I left out: The Zhuyin |
The positive control ran as a 10 s expect.poll immediately after the 700 ms settle, so it polled for a condition that must already hold. That hid a real failure two ways: a source that committed on the chord and then opened a fresh preedit satisfied the poll on the second one, which turns the byte assertion below into evidence about a different gesture; and when the composition was genuinely gone the run still burned the full timeout before reporting it. Read once instead — the shape the Kotoeri case already uses at the same point after the same settle.
…commit hole Review of the re-homing found three things worth changing, all in what the tests claim rather than in what they run. The macOS cases in keyboard-handlers-ime-composing-chord.test.tsx had a docstring narrating the whole gesture as though each step were asserted. Only the count is. Drop the marked press from those rows and they still pass — nothing is left that could fire twice — while dropping the arming makes them fail with the byte twice over. The docstring now says that, and points at the recorded-trace fixtures for which event carries the byte. commitsAfterCapture was read for truthiness, so a case committing the empty string would skip both the mid-gesture guard and the commit. Today that fails loudly, since every case expects a byte; a later case expecting none would have passed having asserted nothing. Read for presence instead. Its doc also now says plainly that the text is a label no assertion reads, so a wrong one misleads a reader rather than failing. The Chinese fixture claimed reproduction from a spec that checks those rows rather than producing them. The recorder, terminal-macos-chord-input-pipeline-probe.spec.ts, was never extended past Japanese, Korean and ABC, so these rows cannot be re-measured from this repo. Said so. And the deliberate Zhuyin Option+Left omission leaned on the e2e spec for that gesture's coverage without noting that no workflow sets ORCA_E2E_NATIVE_MACOS_KOREAN — so that gesture has no coverage that runs on its own, which the note now states. One review finding was not adopted: that the arrow keyup under a held Command contradicts the recorded "no keyup under Cmd". That claim belongs to the sources that swallow the chord, where the IME consumed the key. The Korean capture in keyboard-handlers.issue-12871-recorded-chord-traces.ts does carry an ArrowLeft keyup with metaKey set, right after compositionend. A comment now records the distinction where it was generalized.
…each A second review pass over the re-homed traces found six records claiming more than they measure. None of the assertions changed; the notes around them did. - The Zhuyin Cmd rows are byte-identical to the Kotoeri ones, so the header claim that each case pins the route it recorded was false for that case: replayed here it is Kotoeri under another name and cannot fail on its own. The identity is itself the measurement — a third input source producing the same shape — so it stays, said plainly, with the e2e cell named as the only place the real Zhuyin source is driven. - The Pinyin Cmd case justified itself by saying a recovery that disarmed on composition activity would drop its byte and pass everything else here. The Pinyin Option case carries the same compositionupdate rows and would fall with it. What the Cmd case adds is that route, not that property. - The multi-character Japanese preedit note read as though the glyph count were pinned. Shorten 日本語 to one character and the test still passes: where text lands on the grid is not visible to that harness. The note now says it records the reported symptom, not what the assertion reads. - The e2e header stated without qualification that both Chinese sources keep the preedit through the chord. Three of the four cells; the Zhuyin Option cell is the exception under this rig, which is why it opts out of the positive control. - `commitReturns: 1` was justified by "flushLineToReader's own two presses". It presses Return once and again only after a five-second wait fails. - The `cn-*.json` names have no path or digest, unlike the in-app family whose module carries a SHA-256 per file, and two of the three timing regimes cited for the Zhuyin omission were run out of tree. Both now say so rather than reading as things a reader could go and check.
stablyai#15017 gave the composing-chord deferral an owner and a ceiling, and stablyai#14743 moved MacOptionAsAlt into terminal-option-shortcut-policy.ts. Both landed on the lines this branch had rewritten. The deferral now goes through main's `deferredChordSender`, which blur and teardown can drop, and which abandons a chord after 10 s rather than waiting forever. The release-recovered path keeps its own `composing` argument, since a chord answered for at keyup has no live event to read it from. `runShortcutAction` gained main's Option-release arming. Its narrowed event type now carries the modifier flags the tracker reads; a snapshotted chord already held them. The arming is keydown-only in effect — the Option policy returns null for a composing event, which every recovered chord is.
|
On v1.4.188 the relocation is gone — #14730 shipped in v1.4.187. The double send is not: one Option+← over a composing syllable still moves the cursor two words, which is what this PR fixes. What worries me is that this looks stuck on something mechanical rather than on review. I would hate to see this one go quiet when it is the last piece of the #12871 chain still outstanding. Happy to re-push or rebase if that helps get the checks running. |
|
Confirmed, and thank you for checking a release build — that separates the two halves better than anything I can do from a branch. On the checks: a re-push won't clear it. Each push just adds two more runs in the same What I can add is that this isn't specific to this PR. My other two open PRs here — #13253 and #11094 — show the identical pattern: every Since my last comment the branch picked up four commits I never explained here:
Conflict-free against |
|
#12871 is fixed — the syllable stays put now, and I can confirm that on v1.4.192. The double send this branch was also carrying is still there, though. That half never had an issue of its own — it only existed inside this PR — which is why it read as superseded. Filed it as #17616 so it is tracked separately, and I am putting up a PR against current main that carries the commits here forward with authorship intact. @kunsanglee — the design and the measurements are yours, so say the word if you would rather take it yourself and I will close mine. |
5 participants
Summary
Supersedes #12732, which GitHub will not let me reopen — I force-pushed the rebase before reopening it, and that is a one-way door. Same branch, same author, one commit now.
Fixes #12871. Rebased onto #14730 and cut down to the one commit that is still missing from main.
Option+Left jumps two words instead of one, while a Korean syllable is composing.
Measured against
a663f1b, replaying this PR's recorded macOS trace — Korean 2-Set,사in the preedit,Option+←:\x1bbis one word back, so two of them are two words.Cmd+Leftis line-start and therefore idempotent, which is why the same double never shows there.#14730 fixed the order, and that half is done: the chord no longer overtakes the text it was typed after. It still resolves the chord on the composing keydown, and Korean 2-Set commits on that chord and lets the platform replay it unmarked after keyup. Both copies resolve.
One correction to the close comment: the Japanese half is not still outstanding. Replaying this PR's Kotoeri trace against
a663f1bproduces exactly one byte, after the commit. Removing the composing-event gate in #13282 is what let the marked keydown through, and #14730's deferral put it in the right place. That case is closed.What this adds
The two input sources are indistinguishable while the key is down — both recorded on stock macOS, Chrome 151, as
code='ArrowLeft',keyCode=229,isComposing=true. Nothing decidable is available at the keydown. By the release they have separated:isComposing: false. Acting is the double.So an exempt chord is remembered on the composing keydown and decided on its release: still composing means nothing else will deliver it, so run the action; not composing means the replay answers. Bytes take #14730's deferral either way, now reached from both paths through one condition instead of two.
"Exempt" is
Cmd/Option/CtrloverArrowLeft,ArrowRight,Backspace,Delete— never withShift, which Japanese conversion binds to resize the segment being converted.Two details that are not obvious and are load-bearing:
KeyboardEventkeeps its fields as prototype accessors, so{ ...event }is an empty object and the chord silently loses itscode. happy-dom keeps them as own properties and would go on passing, so the source comment is the only warning that survives.Cmd+←delivers no arrow keyup at all — recorded at Chromium's own input dispatch, whereOption+←and a bare←both deliver theirs. TheCommandrelease is the only event that ends that gesture, and it still reports the composition live.The remembered chord stores the physical
codeas its key, because a CJK source rewriteskeyto'Process'(#12171, #13033). That is safe precisely there and nowhere else: the caller has already narrowed to four codes whosekeyis that same string when nothing rewrote it.Arming is macOS-only. Both behaviours the release reads are stock-macOS captures. An input source that commits without replaying — unrecorded for ibus and MS-IME — would arm, find a release that no longer reports itself composing, hand the chord to a replay that never arrives, and drop it. That is worse than the bug #14730 fixed, which delivered late rather than not at all. Everywhere else the deferred send stands untouched, and a test on a Windows user agent pins it.
Not fixed here: the dashboard popout
preview-terminal-key-handler.tsis a second terminal keyboard surface with its own listeners, and its IME gate (isNativeTextKeydown) excludes modifier chords, so a Japanese conversion still swallowsCmd+Leftthere. #12871 remains open in the popout. An earlier revision of this branch leaked half a fix into it by putting the physical-code reading in the shared policy; that is why the reading now lives in the pane instead.One test from #14730 changed
keyboard-handlers-ime-composing-chord.test.tsxpressed the chord and never released it. Its press now runs to the release, with the same assertions, because that is where a swallowed chord becomes resolvable and it is what hardware delivers. Worth a look — it is the one place this PR touches someone else's just-landed test.About the size, and the missing checks
You flagged +2640/−31 with zero checks last time. The checks are structural: a fork branch does not run
pull_requestworkflows in this repo, and I have no push access here, so there is no version of this PR that arrives green. Local results are below.The line count did not come down much on the rebase, and it is worth saying why rather than trimming to make the number look better. The overlap with #14730 was 14 lines — the deferral itself. The bulk was never duplicated work:
The mechanism is 251 lines across three files. If you would rather the recording harnesses live outside this PR, say so and I will pull them — they are separable, and the trace files reference them only by name.
Verification
src/renderer/src/components/terminal-pane— 270 files, 3524 tests, all passing.tscclean on all three projects. oxlint and oxfmt clean on every file this PR touches. Reliability gate manifest passes for 84 gates; max-lines ratchet OK at 340.Every regression test added across the second and third commits was checked to fail without its fix: the non-mac one reads
[]instead of['\x1bb'], the two-chord one drops the first chord, the consumption one sees the press reach a later window listener, and the worktree-history one sees that listener get nothing.Widened once across
terminal-pane,dashboard-popout,sharedandlib— 1186 files, 12617 tests, all passing.One thing worth knowing for anyone reproducing locally: a git worktree nested inside the main checkout resolves
node_modulesupward and can serve an xterm patch hash the branch lockfile never asked for. That alone produced 62 failures here that had nothing to do with the code. Install inside the worktree before believing a red IME suite.