fix(terminal): hold a cursor chord until the composing syllable commits - #14730
Merged
Merged
Conversation
The composed glyph reaches the pty from the composition session-end handler, which runs after the chord's keydown. Only Enter was held for that, so every other chord went straight out on the transport and overtook the text it was typed after: with 가나 on the line, typing 가나다 and pressing Cmd+Left left 다가나, the composing 다 landed at the cursor's destination. Defer any sendInput chord while a composition is live or its session has not yet flushed. Korean 2-Set shows the shape most clearly — the platform replays the chord unmarked after keyup, so isComposing is already false while the session is still pending. No fallback timer on this path. A newline arriving late still arrives, which is what that timer is for; a chord arriving mid-preedit is the corruption the wait exists to prevent, and a conversion can hold its candidate window open for seconds. Dropping the chord costs one keypress, firing early costs a line. Pane commands are unaffected: they are not sendInput actions. Fixes #12871
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change defers non-Enter terminal shortcuts during IME composition until 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
The unit coverage asserts the handler's ordering against a synthetic transport. This asserts it where it is actually observable: the committed glyph and the chord reach the pty by two different routes, and only their merged order is visible to the shell. Verified to discriminate — against keyboard-handlers.ts from main the same spec reads 01 eb8ba4 0a, the chord ahead of the syllable, which is the reported corruption byte-for-byte.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/terminal-korean-composing-chord-order.spec.ts (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the file header comment.
Keep one concise comment that states the PTY ordering contract. Remove issue history and test implementation details.
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: 4232e88c-e139-43b9-8ea5-7aedca70c006
📒 Files selected for processing (1)
tests/e2e/terminal-korean-composing-chord-order.spec.ts
… the Enter path Nesting the new branch inside the Enter condition re-indented the whole Enter block, which is the kind of diff that can silently change it. Keeping them as sibling conditions leaves the Enter path out of the diff entirely.
Cmd+Left resolves to \x01 only under the macOS branch of the shortcut policy, so on a Linux shard the chord produced no byte and the spec passed by measuring nothing — it failed in CI for that reason, not for the behaviour under test. Pinning the platform is the established pattern for these specs, and expectImePlatformPolicy fails loudly if the override does not take.
kunsanglee
added a commit
to kunsanglee/orca
that referenced
this pull request
Aug 15, 2026
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
kunsanglee
added a commit
to kunsanglee/orca
that referenced
this pull request
Aug 15, 2026
…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
This was referenced Aug 17, 2026
This was referenced Aug 31, 2026
Contributor
|
thanks for reporting. looking into it |
Contributor
|
thanks for reporting. looking into it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #12871. Reproduced by hand on macOS with 2-Set Korean, then pinned at the pty by a test that fails on
main.The bug
The composed glyph reaches the pty from the composition session-end handler, which runs after the chord's keydown. Only Enter was held for that; every other chord went straight out on the transport and overtook the text it was typed after.
With
가나on the line, typing가나다and pressingCmd+Left:On screen:
다가나. QWERTY keys to reproduce with 한 active:r k s k e k, thenCmd+Left.Why Korean shows it even though
isComposingis false. The platform replays the chord unmarked after keyup, so by the time it resolvesisComposing === false— but xterm has not yet emitted the session end that writes the syllable, sohasPendingTerminalImeCompositionis still true. Gating on either condition covers that shape and the still-marked one.The change
+13 / −1 in production code, and the single deletion is a comma on an import line.
A sibling condition rather than a nested one, deliberately: nesting the new branch inside the Enter condition re-indented the entire Enter block, which is exactly the diff shape that can silently alter it. As written, the Enter path does not appear in the diff at all — that is the regression argument, and it is checkable rather than asserted.
No fallback timer on this path. The 200 ms fallback is right for a newline — arriving late still means arriving. It is wrong here: a conversion can hold its candidate window open for seconds, so the timer would fire mid-preedit and reproduce exactly the corruption the wait exists to prevent. A composition that never ends drops the chord instead, costing one keypress rather than a mangled line.
sendTerminalInputAfterCompositiongrows afallbackMs: nulloption for that.Pane commands are unaffected — a chord remapped onto
terminal.clearorclosePaneis not asendInputaction and never reaches this branch. Waiting would let committed text land after the pane had cleared.Evidence it discriminates
Both layers were run against
keyboard-handlers.tsrestored frommain:Unit — 3 of 4 fail:
chord must not reach the pty while the glyph is pending: expected [ '\x01' ] to deeply equal []E2E, at the pty — real composition via CDP, chord pressed mid-preedit, bytes read where the two routes merge:
main01 eb8ba4 0a— chord firsteb8ba4 01 0a— syllable firstThat is the reported corruption byte-for-byte, and its absence afterwards.
The e2e pins the renderer to macOS via
applyImePlatformPolicy. That was not cosmetic:Cmd+Leftresolves to\x01only under the macOS branch of the shortcut policy, so on a Linux shard the chord produced no byte and the spec passed by measuring nothing. It failed in CI for that reason before the pin, which is the failure modeexpectImePlatformPolicyexists to make loud.Review evidence
shouldSuppressTerminalImeKeyboardEventreturns early onisComposing === trueatuse-terminal-pane-lifecycle.ts:1048, before the non-Latin chord arm at:1096. Different scenario (non-Latin layout, no composition), correctly separated.composeHangulSyllableandcommitImeTextcome from.Scope
Narrower than #12732, which fixes this and a second defect: a chord swallowed outright by a Japanese conversion, recovered at keyup. That one is real and still unfixed, but it is Japanese-only, I have not reproduced it, and getting it wrong double-sends on Korean where the platform already replays. This is the half verified end to end.
No regression
src/main/files (daemon, pty-subprocess, updater) fail under full-suite load; the same four fail worse on unmodifiedmain(2 files / 6 tests, versus 1 file / 5 on this branch), so they are pre-existing load-dependent flakes unrelated to this path