fix(terminal): send a composing cursor chord once, not twice - #12732
kunsanglee wants to merge 18 commits into
Conversation
Typing "가나" and pressing Cmd+Backspace leaves "나" on the line. "가" has reached the pty while "나" is still in the preedit, so the line-kill byte goes out synchronously, erases what the shell has, and only then does xterm flush "나" from its timer. xterm guards this in CompositionHelper.keydown by flushing the preedit before a non-composition key runs, but Orca's window-level capture handler calls stopImmediatePropagation, so xterm never sees the keydown. The compositionend path sends the committed glyph from a timer, so it is asynchronous regardless. keyboard-handlers already defers for this reason, but only for Enter. Widen it to every sendInput action while a composition is pending. Enter keeps deferredNewlineSender, which also owns the Windows redispatch ledger; other keys call sendTerminalInputAfterComposition directly so they do not leave absorb credits nothing will consume. Also fixes Cmd+Delete, Cmd+Arrow, Ctrl+Backspace, and the Alt meta sequences, which shared the same unsequenced path on all platforms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughTerminal shortcut policy now normalizes selected IME-owned navigation and editing chords by physical key code. Keyboard handling suppresses non-exempt IME events and recovers swallowed exempt chords on keyup through the shared shortcut-action path. Native-only actions remain excluded. Scoped keyup listeners are registered and cleaned up with the keyboard hook. New tests cover composing chords, recorded Korean and Japanese traces, pane commands, terminal output, replay behavior, and text-field isolation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Heads up — I hit the user-visible side of this on macOS and filed #12871 before finding your PR: with I've opened #12872 as a draft that carries your commit unchanged ( Not meant to supersede this PR — the fix is yours and I hope it lands as-is. Happy to rebase mine to a test-only follow-up once this merges, or for you to cherry-pick the tests into this branch directly if you'd rather keep it to one PR; equally fine with closing mine on request. |
…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+Left moved the syllable to the line start. With a Japanese preedit the relocated text also overwrote the glyph at the destination cell. Reported with reproduction steps in stablyai#12871. The ordering fix already covers these chords; this pins the previously untested resolver branches so the symptom cannot quietly return: - Option+ArrowLeft / ArrowRight word jumps (\eb / \ef) - Cmd+ArrowRight line-end jump (\x05) - a multi-character Japanese preedit committing before the chord byte - Ctrl+ArrowLeft word jump on Windows (\eb) Verified non-vacuous: against the pre-fix keyboard-handlers.ts all five new cases fail with the chord byte recorded ahead of the commit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/keyboard-handlers-ime-deferred-input.test.tsx (1)
230-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the test rationale comments.
Keep each comment to one concise line. Preserve the required ordering rationale and remove the detailed outcome examples.
src/renderer/src/components/terminal-pane/keyboard-handlers-ime-deferred-input.test.tsx#L230-L234: replace with a one-line explanation that IME text must commit before cursor movement.src/renderer/src/components/terminal-pane/keyboard-handlers-ime-deferred-input.test.tsx#L282-L284: replace with a one-line explanation that the complete preedit must commit before movement.As per coding guidelines, comments must be concise and preferably one line.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 685badd9-1685-4ac3-8a14-123a0dcd84e9
📒 Files selected for processing (1)
src/renderer/src/components/terminal-pane/keyboard-handlers-ime-deferred-input.test.tsx
|
Cherry-picked your test commit onto this branch as I verified the five cases independently rather than taking the claim on faith: against Two scope notes, so the coverage isn't read as wider than it is. The Windows The Japanese case pins ordering with a multi-character commit, but the harness commits synchronously, so it doesn't exercise a preedit that outlives Happy for you to close #12872 whenever suits you, and thanks for filing #12871 with the reproduction — the relocation framing is clearer than mine. |
|
thanks for reporting. looking into it |
1 similar comment
|
thanks for reporting. looking into it |
Conflict in keyboard-handlers.ts: stablyai#12462 added the terminalModifierKeyDownObserved field to the Windows Enter chord claim while this branch re-nested the same condition to cover non-Enter keys. Kept both — the Enter branch keeps upstream's claim logic verbatim, now inside the widened composition guard.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Heads-up: this can't land as written any more. 17b3dff merged to main and removes the first-party IME composition layer — Not a criticism of the approach; the deferral mechanism itself turned out to be the problem. On 1.4.171 we could make Orca emit a Shift+Enter payload to the PTY with no Enter pressed — 10/10 deterministic, landing 203-218 ms after the keydown, which was the 200 ms fallback firing straight through the pending-composition gate that the event path checked. #12890 independently reported the same thing from the Japanese/Chinese side, where phrase-level preedits stay open far longer than the bound. The symptom you're targeting is real and still open (#12871 has a good reproduction). The shape that survives is having the chord path consult composition state directly rather than sequencing bytes behind a timer — Happy to look at a rebased version. |
Upstream stablyai#13128 (17b3dff) returned IME composition ownership to xterm and removed the deferred-newline layer this branch built on. Both this branch's production change and its test file targeted modules that no longer exist, so the merge takes upstream wholesale and drops the stale test.
|
Following nwparker's notes here and on #12871, I've started on the rework for the new architecture — recording real Korean/Japanese IME traces on stock macOS (the relocation case and the Japanese-overwrite variant he asked for), rebuilding the tests from Flagging here mainly to avoid duplicated effort: if you're already reworking this PR for the new architecture, let me know and I'll shape my branch to complement yours — tests-first, like last time. Otherwise I'll fold the chord-path fix into the same PR — either way I'll cite this PR's ordering analysis in the description, and once it's up, additive commits are welcome on my branch if you spot gaps. |
|
Yes, I'm on it — and I have a measurement you should see before you build the chord-path gate, because it says that gate is wrong on macOS. I merged main into this branch, dropped everything that built on the removed layer, and rebuilt the fix as the guidance describes: exempt modifier chords over ArrowLeft/ArrowRight/Backspace/Delete from Composing The IME ends the composition itself when the Cmd chord arrives, and macOS then redispatches the arrow unmarked. That second copy was never IME-owned, so it already resolves the chord. Replaying that trace end to end,
So for this gesture main is already correct — commit first, then the cursor move — and exempting the marked keydown adds a second firing. For Two things I got wrong that are worth stating, since they're the reason I didn't catch this from tests: Hand-built event shapes can show what a given input produces, but never that the input arrives alone. My probe fed a single Also, Two more measured, both from the same replay rig: Queued bytes flush on a cancelled composition, not just a commit. On the local Windows ConPTY path What I don't have is the same trace for Your traces settle that, so I'd rather not guess ahead of them. Concretely: you take the traces and the tests, I'll hold this PR at the merge with main and hand over the replay rig and the two findings above. Whichever branch the fix lands on is fine by me — happy for it to be yours. |
|
I have both input sources recorded now, and they behave in opposite ways. That turns out to be the whole story here, so I'd hold off on a plain chord-path exemption — it fixes Japanese and breaks Korean. All traces are stock macOS, Chrome 151, recorded through a page that logs Korean 2-Set — the chord is not droppedComposing
That unmarked copy was never IME-owned, so it already resolves normally:
Left column is right in every row. The exemption doubles all three; Japanese — the chord really is droppedPreedit No
Three chords, zero calls on main. With the exemption each fires exactly once and lands after the commit, because there is no redispatch to double it. Why this can't be gated at keydownThe marked keydown is identical in both: So the shape that works for both is: resolve on the marked keydown, and swallow the unmarked redispatch of the same physical key when it comes. Japanese gains the chord it was losing; Korean drops back to one firing. That is exactly what I'm building that now, mirroring the Enter gesture rather than inventing a second mechanism. Two things I'd want a second opinion on: With the carry in place, the Japanese chord queues in And the queue drains on a cancelled composition too, not just a commit. Measured: compose, Traces and the replay rig are yours either way — say the word and I'll push the rig as a test on this branch, or hand it over for yours. Neither of us should be guessing at this layer, and I nearly shipped the exemption on the strength of hand-written fixtures that had no redispatch in them. |
A modifier chord pressed during a composition arrives identically on both input sources tested — code='ArrowLeft', keyCode=229, isComposing=true — but they answer it in opposite ways, recorded on stock macOS: Korean 2-Set commits the syllable, emits compositionend, and the platform replays the chord unmarked after keyup. That replay already reached the shell, so main is correct here today. Japanese conversion swallows it: no commit, no replay, still composing at keyup. Cmd/Option+Arrow and Cmd+Backspace produced no bytes at all. Exempting the marked keydown alone fixes Japanese and breaks Korean, where Option+Left's \x1bb would go out twice and jump two words. So the exemption is paired with a redispatch ledger that retires the replayed copy, mirroring useImeEnterGestureOwnership — including its animation-frame expiry, since macOS delivers keyup before the replay. Chord matching for these four physical keys now reads event.code, and the keybinding lookup reads the same resolved key, so a remapped chord cannot lose to the built-in byte while an input source rewrites event.key. Ordering needs no timer: xterm's patched input() queues the bytes behind the preedit and flushes them with the commit.
Three defects, two of them introduced by the previous commit. Claiming happened only on the byte path, but widening the keybinding lookup is what let a remapped chord resolve mid-composition — and a remap produces a pane command, not bytes. The replay then ran it again: two panes closed, or two clears, from one press. Claim now covers every resolved action. Pinned by replaying the recorded Korean trace with terminal.clear remapped onto Mod+Backspace, which counts 2 without the fix. The carry expired on an animation frame, so any slow task landing between keyup and the replay let the chord fire twice. It now expires when the last chord modifier comes up, which is the gesture's own boundary in both traces and does not depend on timing at all. The trace replay now spaces rows by a full task rather than a microtask, so a regression back to a timer would fail it. A gesture interrupted by focus loss never releases its modifier, leaving the carry armed to swallow an ordinary chord later; blur now resets it, as both siblings in this effect already do. The keyup listener is scoped like the keydown one so another pane's release cannot retire this pane's carry. Also narrows the physical-key substitution to IME-owned events — an X11 keysym remap preserves code while changing key, and reading code there would have overridden it on ordinary presses.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
kunsanglee
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.ts (2)
45-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a trace with a rewritten
keyvalue.Every case sets
keyequal to the physical key name, for example"key": "ArrowLeft"with"code": "ArrowLeft". SophysicalChordKeyinterminal-shortcut-policy.tsreturnsevent.keyunchanged, and the normalization branch at Lines 152-153 of that file never runs in this suite.That branch is the one the PR description calls out for Windows, where an IME-consumed key arrives as
key: 'Process'. It also contains the native-event spread defect flagged onterminal-shortcut-policy.tsLines 147-153. A case with"key": "Process"and"code": "ArrowLeft"dispatched as a realKeyboardEventwould catch it.The PR description also lists composition cancellation as in scope. No case here cancels a composition. Add a trace where the user presses Escape mid-preedit and then presses an exempt chord, so the ledger state after cancellation is pinned.
754-782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect recorded cases by name, not by array index.
CASES[2]is "Korean 2-Set, Cmd+Backspace" andCASES[0]is "Korean 2-Set, Cmd+ArrowLeft". Both tests depend on the specific gesture: the first needs a Backspace chord to match theMod+Backspaceremap, and the second needs the trailing modifier keyup that releases the carry. If someone inserts or reorders a case in the 545-line fixture block above, these tests silently exercise the wrong trace and may still pass.♻️ Proposed refactor
+function caseNamed(name: string): RecordedCase { + const found = CASES.find((testCase) => testCase.name === name) + if (!found) { + throw new Error(`recorded case not found: ${name}`) + } + return found +} + describe('recorded macOS chord traces during an IME composition', () => {it('runs a remapped pane command once across the replay', async () => { + const backspaceCase = caseNamed('Korean 2-Set, Cmd+Backspace') const rig = openRig({ keybindings: { 'terminal.clear': ['Mod+Backspace'] } }) - await replay(rig.textarea, CASES[2].rows) + await replay(rig.textarea, backspaceCase.rows)it('releases the carry so a later ordinary chord still sends', async () => { + const arrowCase = caseNamed('Korean 2-Set, Cmd+ArrowLeft') const rig = openRig() - await replay(rig.textarea, CASES[0].rows) - expect(rig.inputCalls).toEqual(CASES[0].expectCalls) + await replay(rig.textarea, arrowCase.rows) + expect(rig.inputCalls).toEqual(arrowCase.expectCalls)- expect(rig.inputCalls).toEqual([...CASES[0].expectCalls, CASES[0].expectCalls[0]]) + expect(rig.inputCalls).toEqual([...arrowCase.expectCalls, arrowCase.expectCalls[0]])
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef380aae-9012-49e3-86f3-c0240261e196
📒 Files selected for processing (6)
src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-composing-chord.test.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-chord-redispatch.test.tssrc/renderer/src/components/terminal-pane/terminal-ime-chord-redispatch.tssrc/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts
|
Our recordings agree with yours layer-for-layer — and the redispatch find explains the one gap in ours: we recorded through the in-app boundary probe, where Orca's window-level capture consumes the unmarked redispatch above the textarea, so we saw its effect (exactly one On your two questions: (1) Japanese queuing until the phrase commits seems strictly better than today's drop — deferred-but-delivered preserves the ordering contract, and "inert until conversion ends" is normal IME modality; worth one line in the PR notes. (2) The cancelled-composition drain feels like the real hazard: a queued For the rig: push it on this branch — tests belong with the behavior change they verify. I'll rework my five recorded scenarios into fixtures asserting the both-worlds invariants (exactly one movement byte, strictly after the commit, destination glyph intact) and send them your way as a commit to cherry-pick, like last time — or as a follow-up tests PR if you'd rather keep this one lean. |
The two input sources are indistinguishable while the key is down and
separate by the time it comes up: Korean has ended its composition and a
platform replay is on the way, Japanese is still composing and nothing
else will ever deliver the chord.
So yield every IME-owned keydown as main does, and resolve the chord from
its release only when the composition is still live. Nothing is swallowed
and no state is carried between events, which retires the redispatch
ledger and the whole class of gaps it had to cover.
Also stop rebuilding the chord event with a spread. A real KeyboardEvent
keeps its properties as prototype accessors, so `{ ...event }` produced an
empty object and every keybinding lookup silently missed in production
while the plain-object test doubles passed.
The Japanese trace is replaced with the full capture, including the
modifier presses and releases that hand transcription had dropped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mu6j33LNsd4Mvv8a1NuFoo
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-recorded-chord-traces.test.ts (1)
413-433: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelect the traces by name instead of by array index.
Both tests address a specific recorded trace:
CASES[3]is the Japanese multi-chord trace, andCASES[2]is the KoreanCmd+Backspacetrace. The assertions depend on those exact rows. If somebody inserts or reorders a case inCASES, these tests keep passing against the wrong trace, or they fail with an unclear reason.Look the trace up by its
namefield so the intent stays explicit.♻️ Proposed refactor
+const caseNamed = (name: string): RecordedCase => { + const found = CASES.find((c) => c.name === name) + if (!found) { + throw new Error(`unknown recorded case: ${name}`) + } + return found +} + it('runs a remapped pane command from a swallowed release', async () => { const rig = openRig({ keybindings: { 'terminal.clear': ['Mod+Backspace'] } }) - await replay(rig.textarea, CASES[3].rows) + await replay(rig.textarea, caseNamed('Japanese, a bare arrow and four chords across one live preedit').rows) expect(rig.clearPaneCalls).toBe(1) @@ it('runs a remapped pane command once when the platform replays instead', async () => { const rig = openRig({ keybindings: { 'terminal.clear': ['Mod+Backspace'] } }) - await replay(rig.textarea, CASES[2].rows) + await replay(rig.textarea, caseNamed('Korean 2-Set, Cmd+Backspace').rows)Adjust the type name to whatever the recorded-case type is called in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 66b0e72b-a3f0-473d-8d28-2ed7116f7f2f
📒 Files selected for processing (4)
src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-composing-chord.test.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.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/components/terminal-pane/keyboard-handlers.issue-12871-composing-chord.test.ts
- src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts
Review found the release's own fields are the wrong ones to read, in both directions, and both reproduce. A KeyboardEvent's modifier flags describe the moment it fired, so letting Cmd up before the arrow leaves the arrow's keyup with metaKey: false. The exemption then misses and the chord is lost outright — the bug this change exists to fix, surviving in whichever order the user happens to let go. The mirror case: a key that went down with no composition in sight has already resolved from its own keydown, but a composition starting while it is held makes its release look swallowed and sends it a second time. Measured as \x1bb twice, which is two words instead of one. So keep the chord from the keydown that yielded, and let the release answer for that press: same key, composition still live, modifiers as they were when it went down. Nothing is swallowed and the carry cannot outlive its key, since an ordinary press of the same code drops it. The release also now runs the file-search and floating-panel guards the keydown path applies, matching on the press while suppressing on the real event, and Shift is excluded from the exemption as its comment always claimed — Japanese conversion binds Shift+arrow to resize a segment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu6j33LNsd4Mvv8a1NuFoo
A chord interrupted by Cmd+Tab or Spotlight delivers no keyup here, and nothing dropped what it left behind. A later bare arrow moving the preedit caret then matched that carry by code and sent the missing chord's bytes to a shell the user never aimed at — measured as a stray \x1bb. Three closures, matching what the sibling tracker in this effect already does: any press of the same code supersedes the carry whoever owns it, the release spends it before any gate can refuse it, and window blur drops it outright. Also record at imeChordSnapshot why it must stay field by field. The prototype-accessor trap moved here when the policy stopped seeing native events, and this is the only copy of a real KeyboardEvent left. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu6j33LNsd4Mvv8a1NuFoo
|
Pushed. The chord-path gate is in, but not the shape I described above — the carry-and-swallow mirror of What changedResolving on the marked keydown and swallowing the redispatch works on the traces, and that is exactly the problem: it is a decision made where the two input sources are still identical, defended afterwards by suppressing an event. Every gap it had came from the suppressing half — a carry surviving a focus change inside the window, a The decision does not have to be made there. At the chord's keyup the two have already separated: That holds on all four recorded traces, 8/8 chords. So the pane yields every IME-owned keydown exactly as main does, remembers the chord it yielded, and resolves that remembered press when the key comes up still composing. Korean's release finds the composition already over and does nothing — the replay resolves it, as it does on main today. Nothing is swallowed, so the entire class of gaps above does not exist. Ablation against the recorded tests: drop the exemption and Japanese goes back to Four defects worth naming, because tests did not catch themA spread over a native event is empty. The first revision built the chord's keybinding lookup with Modifier flags describe the moment the event fired. Releasing The mirror of it. A key that went down with no composition in sight has already resolved from its keydown; a composition starting while it is held made its release look swallowed and sent it again. Measured as A carry with no release to spend it. A chord interrupted by All four are regression tests now, each failing on the revision before its fix. @ethznn — on the cancelled compositionMeasured rather than assumed, and I landed on the opposite of your suggestion, so it is worth disagreeing with if you still see it the other way. Compose Your PTY-layer scenarios are still very welcome as a commit to cherry-pick. The both-worlds invariants you described are the right assertions, and the rig is on this branch now ( What this does not fixA kitty-protocol pane is half covered. The policy returns The dashboard popout preview terminal drops IME-owned events before reaching the policy, so it still has this bug on that surface. No traces for it either. Korean's correctness rests on one ordering — Traces are macOS, two input sources. Windows and Linux are routed via |
Review found the chord's keybinding lookup was reimplementing something the matcher already has one word away. PHYSICAL_CODE_FALLBACK_KEYS holds the keys whose produced value the platform cannot report — '', 'Dead', 'Unidentified' — and 'Process' is exactly that case: Windows' report for a key an IME consumed. Adding it there deletes the synthetic chord event and its fourteen renamed call sites, and fixes the same blind spot on every other surface rather than in the terminal pane alone. `chordKey` stays for the byte fallbacks, which read the key directly. Also from review: the release path's floating-panel and file-search guards now have the tests their comments were asserting without, Shift exclusion has the composing Cmd+Shift+Arrow negative it arrived without, a one-caller wrapper is inlined, and two comments stop pointing at a symbol deleted two commits ago. The idle assertion added earlier is dropped. It was restored on the claim that terminal-shortcut-policy.test.ts leaves `code` empty for these chords; it does not, and for a non-composing event the substitution short-circuits before reading `code` at all, so both fixtures took the same branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu6j33LNsd4Mvv8a1NuFoo
|
One follow-up, and it touches a file outside the terminal pane, so it should be an explicit decision rather than something to notice in the diff. The chord's keybinding lookup was reimplementing a mechanism this repo already has. const PHYSICAL_CODE_FALLBACK_KEYS = new Set(['', 'Dead', 'Unidentified'])
The tradeoff worth naming: this widens from one surface to all of them. It also retires the defect I described above: with nothing copying a native event any more, there is no spread left to be empty. The byte fallbacks still resolve the physical key themselves, gated on IME ownership so an ordinary press follows Three gaps closed alongside it, all cases where a comment asserted a contract nothing enforced. The release path's floating-panel and file-search guards now have tests — each fails if the guard is removed, and without them a chord remapped onto And a correction to my own earlier reasoning. I had restored an assertion here on the claim that Suite is green at 25,431 renderer and shared tests. |
Three reviewers independently reached the same keyup. Suppressing it buys nothing: by the time the release is consumed the action has already run, so there is no second handler left to race. It costs, though — cut off at window capture the keyup never reaches xterm's own _keyUp, which clears the flags its input path reads, nor any window listener registered after this one. The release now forwards preventDefault and drops the rest. Separately, a composition can live in a rename field or the search input, and that field can unmount while the key is still down. The release then arrives with the terminal as its target and walks past the editable guard, sending a chord aimed at the field to the shell. Refusing to arm from such a press closes that, where refusing at the release cannot. Three invariants the earlier commit messages asserted had no test behind them: the same-code supersede, the blur release, and spending the carry before a guard can refuse it. One test covering all three passed as long as any one worked, so it is now three, each failing only for its own path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu6j33LNsd4Mvv8a1NuFoo
|
Settled at The release no longer consumes the keyup. The carry is never armed from a press aimed at a text field. A composition can live in a rename field or the search input, and that field can unmount while the key is still down. The release then arrives with the terminal as its target, walks past the editable guard, and sends a chord aimed at the field to the shell. Guarding at the release cannot close that — only refusing to arm can. Three invariants my own commit messages asserted had nothing behind them. The same-code supersede, the blur release, and spending the carry before a guard can refuse it. One test covered all three, which meant it passed as long as any one of them worked and pinned none. It is three tests now, each failing only for its own path. That is the general shape of what review caught here: not wrong reasoning so much as reasoning stated in a comment and nowhere else. Every fix in this PR has been mutation-checked — reverted, watched to fail, restored. Full renderer and shared suites: 25,435 passing. Where it landed, for anyone picking this up cold: The pane yields every IME-owned keydown exactly as main does, and remembers the chord it yielded. When that key comes up with the composition still live, it resolves the remembered press — the input source ate the chord and nothing else will deliver it. When the composition has already ended, it does nothing, because the platform's unmarked replay is on its way and resolves it as main does today. Korean and Japanese answer a mid-composition chord in opposite ways and are indistinguishable at the keydown; the release is the first moment they differ. Known bounds, all in the description: a kitty-protocol pane still loses the Option chords (xterm will not encode them during a composition either, and I have no trace saying what it should receive instead); the popout preview terminal drops IME events before reaching the policy; holding a swallowed chord repeats nothing; and the traces are macOS, two input sources — Windows, Linux and Chinese are reasoned about, not measured. Happy to split the |
|
On the cancelled composition — you're right and I was wrong. I was picturing the queued byte as aimed at the preedit, but nothing in the exempt set is: the preedit never enters the shell's line buffer, so Korean ordering, second surface — on your note that Korean's correctness rests on Kotoeri: a state where the release you key on does not exist I ran a native e2e spec against The preedit-survives assertion passes first, so the IME does swallow the chord — it is the recovery that does not fire. A capture-phase probe at and then nothing — no keyup at any position, with The variable I'd look at: your Japanese capture is a converted phrase ( What I have — two commits on
Cherry-pick whichever is useful, or leave the e2e one until after — no need to reply about it either way. |
|
Correction: the conversion-state hypothesis I posted above is wrong. I recorded the A/B and it refutes it — a fully converted, uncommitted What the ten states actually separate is whether the IME ends the composition in response to the chord:
2-Set treats Those rows were driven with synthesized key events, so I re-checked by hand on the same build: typing on a real keyboard, So the release path is gated on the IME conceding the chord. Korean concedes, which is why it works there. On this machine Japanese and Chinese never do, which is where I cannot get the recovery to fire. One thing I can't reconcile, and it may well be my setup: your Japanese capture has Seven trace JSONs (window / document / textarea probes, |
…e rig A second recording of the same gestures, taken inside the app rather than on a bare page, replayed through the rig already on this branch. Both cases are negatives: the bare-page rows pin where the chord bytes come from, and these pin the keys around them, which is where a release-keyed recovery misfires. The Korean case carries a plain ArrowRight pressed one beat after the chord — no bare-page case has one — so a carry left armed past its own release has something of the same `code` to answer for. Its \x05 is not asserted: the app's own window capture consumed the platform's unmarked replay above the probe, so the press that delivers the byte was never recorded. The third recording of that session, a Kotoeri Cmd+ArrowLeft, is not replayed. Its chord press is followed by no arrow keyup anywhere, so replaying it could only assert that no byte is produced. That is raised on the PR instead.
The unit rig replays recorded traces; this is the layer those traces came from, with the real OS input methods deciding everything and the bytes read off the pty. A platform change — redispatch timing, chord swallowing — fails here first. Korean 2-Set and the ABC control pass on this branch. The Kotoeri case does not: the chord byte never reaches the pty, so it is currently a failing pre-merge finding rather than a regression guard. Kept unmodified rather than relaxed, because relaxing it would erase the evidence.
|
Both of your commits are on the branch at The composition state is not what decides it. I rebuilt the bare-page recorder to probe the same three positions your in-app probe used — Unconverted The release arrives, at every position, still composing. That is exactly the shape the recovery keys on. Converted So converted-phrase versus unconverted-single-character is not the discriminator. Both compose, both swallow the chord, both deliver the release. What that leaves is the surface. Your capture and mine differ in a second way I should have named when I first read yours: yours is inside the Electron app at the xterm helper textarea, mine is a bare page in Chrome. With the composition state now controlled on my side, that difference is the only one still standing between a release that arrives and one that does not. Which means your finding survives intact and only its explanation changes. The Kotoeri byte genuinely does not reach the pty on That is the next capture, and your e2e spec is the harness for it — which is the main reason I took it onto the branch rather than leaving it. It carries a failing expectation, and I would rather ship it failing than relax it, for the reason you gave: relaxing it erases the evidence. Two things I have not done and am not claiming. I have not run the in-app probe yet, so I do not know what consumes the keyup there. And the bare-page rig is a scratch harness, not committed — if the in-app run turns up something worth pinning I will bring the relevant rows in as fixtures rather than the rig. |
The release-keyed recovery never fires for Cmd chords in-app while the bare-page recording of the same input source and the same preedit delivers that release. This records the difference instead of reasoning about it: four listener positions, plus focus ownership and the live target on every row, so "never dispatched" can be told apart from "dispatched somewhere else". What it measured. Cmd+Left during a Kotoeri さ preedit produces the keydown at all four positions and no keyup at any of them, while a bare ArrowLeft pressed right after produces both, still composing, with the window still focused. The ABC control has no IME anywhere and shows the same split: Cmd loses its keyup, Option keeps it. Option+Left during the same preedit reaches the pty as さ\x1bb — the recovery does work, on the modifier that still reports a release. So the discriminator is the Command modifier, not the composition state, and not the input source. Kept as the capture harness rather than a gate: it asserts only that rows were recorded, and writes the traces under test-results. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu6j33LNsd4Mvv8a1NuFoo
|
Correction, and the exact rule. My last comment said the OS never delivers the chord keyup and that the IME decides it. Both are wrong. I reran the whole ladder with a human typing on real hardware instead of System Events, and the two behaviours separate cleanly. Option chords are queued, not lost. Kotoeri,
The mechanism's own latency is negligible; what is unbounded is the middle row. The gap between pressing the chord and seeing it take effect is exactly however long the composition lasts — that is the Command chords are dropped, and it is not the IME. An arrow keyup that still carries the Command flag never reaches The two that survived are the useful part. In both, the human happened to release Command before the arrow, so the keyup carried Since you remember the press rather than re-reading the release, that release does resolve the remembered chord. So During a Japanese composition the Command byte never reaches the pty at all, at chord time or at flush. The complete pty write list for that segment is Synthesis is vindicated. Every human cell matches its synthetic counterpart exactly — rung 1 records One thing for the maintainers rather than for you. You flagged that a queued chord "looks inert until conversion ends". The measurement says that invisible interval is not a fixed cost but an unbounded one: it lasts as long as the user keeps composing, so a long phrase means a longer silence before the cursor jumps. Deferred-and-delivered still beats dropped, and I would not change it on my own reading. But Korean gets commit-in-place for free — its Traces from the human run — five rungs, 65 chords classified individually, with the clock alignment — are yours if useful, and I can add the Kotoeri |
macOS delivers no keyup for a key released while Command is held, so the release-keyed recovery for stablyai#12871 could never fire for a Cmd chord. Recorded at Chromium's own input dispatch as well as in the page: Cmd+Left arrives as a keydown with nothing after it, while Option+Left and a bare arrow both deliver their keyup, and defaultPrevented is false throughout — nothing in the app consumes it, it is never made. The Command key's own release does arrive, still marked as composing, and it ends the same gesture. Letting it answer for a pending Cmd chord fixes that half without touching the Option half or the design. Korean cannot double-fire: it commits on the chord, so its arrow keyup arrives first and spends the carry, and both releases report the composition already over. Earlier captures missed this because System Events folds a modifier into the target key's flags and never produces its own press or release. The new probe posts it as its own key event, the way a hand types it, and the contributor's native spec now drives its chords the same way. Verified in a dev build: the pty receives e38195010a — さ, \x01, newline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu6j33LNsd4Mvv8a1NuFoo
|
Your correction lands, and it left one piece on the table that turns the order-dependence into a fix. The Command key's own release is delivered, and the composition is still live on it. You showed the arrow keyup survives only when the human happens to lift Command first. I went looking for what marks the end of the gesture in the other 14 cases, and it is No arrow keyup between them, exactly as you measured. But the last row is the same signal the recovery already waits for, just carried by the other key in the chord. So the carry accepts either release: function releasesPendingImeChord(chord: PendingImeChord, event: KeyboardEvent): boolean {
return chord.code === event.code || (chord.metaKey === true && event.key === 'Meta')
}That is the entire logic change. Why my earlier captures could not see this. Your Kotoeri case passes. With Korean cannot double-fire through the new path, for two reasons rather than one. It commits on the chord, so its arrow keyup does arrive — after One more thing worth having on the record. I also measured a layer below the page, at Chromium's Yes to both offers. The five-rung human traces are useful, particularly the Chinese cells — the PR body no longer says Chinese is untested, it now says Pinyin and Zhuyin are affected on your measurement, and a trace would let that be pinned rather than cited. And please do add the Kotoeri On the unbounded interval before a Japanese phrase flushes: I agree that is a product call rather than a mechanism bug, and I would rather @nwparker make it explicitly than have it fall out of the queue. Worth noting that it is now the only remaining asymmetry between the input sources here, since the Command half is no longer one. |
|
Both delivered, on
Three fixture cases, all measured on
The Command cells are hand-checked. Pinyin preedit live, One case I recorded and then dropped, deliberately. Zhuyin Rig 26/26, the Chinese e2e suite 9/9, oxlint and Unrelated observation, in case it is not just this machine: |
|
Both landed here. On the Zhuyin Your Korean observation: not your machine, and not this branchReproduced, then bisected.
So it has never passed. The cause is in the harness, not the product:
let newlineIndex = pending.indexOf(0x0a)
while (newlineIndex >= 0) { ... }The It stays green in CI because the spec is gated behind Fixing it means either terminating the capture some other way for chords that emit no newline, or asserting at the renderer the way the sibling spec does at Thanks for running it with my files reverted. That framing is what made the bisect one step instead of three. |
|
Took it separately as promised: #13277, off A plain Return follows the chord for the row whose bytes do not end a line, and the expectation carries the LF it produces. Unmodified is safe because the chord already committed the preedit — the assertion's leading Also dropped the 2/2 on three consecutive runs here, against the reproduced failure at all three commits I bisected. |
|
I will read above shortly |
main reverted stablyai#13128 (`17cfc968cf`, PR stablyai#13282), which this branch was built on. Two conflicts, both from that revert. `terminal-pane/AGENTS.md` — took the deletion. The file was stablyai#13128's and the revert removed it; this branch only reformatted one line of it. `keyboard-handlers.ts` — took main's restored file and reapplied this branch's additions on top: - No IME gate in `resolveTerminalKeyboardShortcutAction`. This branch had narrowed stablyai#13128's blanket gate; with the blanket gate reverted away, adding the narrowed form back would newly suppress IME-owned events main routes through here (the Windows composing-Enter path among them). - The keydown yield is scoped to the exempt chords. stablyai#13128 yielded on every IME-owned keydown; now only a chord the IME can swallow yields, and every other IME-owned key keeps main's restored path. - The composing-Enter deferral moved out of `runShortcutAction` into a caller-supplied callback. It reads the live keydown and its `imeProcessEnter`; a chord recovered from a release has neither, and is never Enter. KNOWN RED, and not fixable by merging: 14 cases in `keyboard-handlers.issue-12871-recorded-chord-traces.test.ts`. The revert changed where terminal shortcut bytes go. Before, they went through `pane.terminal.input(...)` — xterm held them behind a live preedit and released them at commit, which is the standing safety argument in this branch's own comments. Now they go through `sendCapturedTerminalInput` → `transport.sendInput(...)`, straight to the pty with nothing holding them. So a swallowed chord recovered during a live composition lands AHEAD of the text still being composed, and the suite's spies watch a sink the bytes no longer reach. The redesign rides main's own `sendTerminalInputAfterComposition` (`terminal-ime-deferred-newline.ts`), which waits for the composition to commit. Its open decisions are enumerated in `.loop/spec/fix-ime-deferred-terminal-shortcut-input/` and land in a separate commit, so this one stays what it is: taking the revert. The other five red IME suites under `terminal-pane/` are main's own — 62 failures on `upstream/main` untouched, the same 62 here.
A chord the IME swallows is recovered from its release, and until now the bytes went out there. They no longer travel through xterm: stablyai#13282 moved shortcut input onto the transport, which reaches the pty directly, so a release that fires mid-preedit puts the chord ahead of the text still being composed. With 가나 on the line and 다 in the preedit, Cmd+ArrowLeft yielded 다가나 — the cursor moved to line start and the syllable landed there. The release now defers through sendTerminalInputAfterComposition, with no time bound. The 200ms fallback is right for a newline, where arriving late still means arriving; a cursor chord sent while the preedit is open is the corruption the wait exists to prevent, and a Japanese conversion holds its candidate window open far longer than that. An abandoned composition drops the chord instead, which costs one keypress rather than a mangled line. Routing the chord back through terminal.input() was the smaller change and does not work: on the patched xterm this tree builds, a byte handed to input() during a live composition is dropped outright rather than queued behind the preedit. The recordings that show it queued predate that. A chord remapped onto a pane command keeps firing immediately. Deferring it too would let the commit land after the pane had already cleared or closed, which loses the text rather than ordering it. No platform check: what arms the recovery is a release that still reports itself composing, which an input source either produces or does not. Gating it to macOS was tried and moved nothing measurable, so it would have been an untested claim. The trace rig was watching terminal.input and terminal.onData, so every chord assertion had been reading an empty channel since stablyai#13282. Both records now follow the bytes down either route, and expectEmitted compares as a joined stream because the captures were taken when xterm flushed the preedit and the chord as one payload while the transport writes each on its own. Two captures stop mid-composition, where the contract is now "held, not sent". They assert that, then drive the commit the recording does not contain and check that the chord lands after it. The appended commit is marked as authored so it cannot be read as captured. Two expectations went stale when an earlier merge changed the bundled xterm, and both are about what xterm emits rather than what this handler sends. It no longer commits a session it never saw start, so a capture that opens mid-gesture flushes nothing; and it now commits a cancelled composition from its own buffer instead of the cleared helper textarea. The cancel case stops pinning whether that text arrives and pins that the chord is last either way. The three macOS e2e specs this branch adds are hand-run — no workflow calls them and they need real input sources — and now say so in a header line, so a red local run is not read as a regression of this change. Not touched: the two symptoms stablyai#13282 gave for the revert (full-width punctuation arriving as ASCII, an invisible Korean preedit). Both live in xterm's composition rendering, which nothing here goes near. Local: src/renderer/src/components/terminal-pane is 251 files / 3230 tests, all passing. tsc clean on all three projects, oxlint and oxfmt clean, reliability gate manifest passes for 73 gates, max-lines ratchet OK at 352. Measured after installing this worktree's own dependencies. Worktrees here resolve node_modules from the enclosing checkout, which was serving an xterm patch hash this lockfile does not ask for; the 62 failures seen before that install were entirely that mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
sorry I might not have looked. Let me look now |
|
Heads-up: I landed a narrower fix for the ordering half of this in #14730 ( Your diagnosis is what identified it — the recorded traces and the two-defect split in your description are what I worked from, and the PR body says so. What I shipped is only Defect 2: hold a Defect 1 is still unfixed and still yours. I did not touch the swallowed-chord recovery: it is Japanese-only, I could not reproduce it, and as you point out, acting on a release that is not still composing double-sends on Korean where the platform already replays. That reasoning is the part of this PR I could not have derived myself. Two things that may help if you rebase:
Also worth knowing: CI has never run on this branch ( If you would rather rebase onto |
|
Thanks again @kunsanglee ! Like the msg above states, the 2nd part couldn't be reproduced... |
kunsanglee
changed the title
|
Continued in #14742 — GitHub refuses to reopen a PR whose head branch was force-pushed after closing, and I pushed the rebase before trying to reopen. Same branch, rebased onto #14730 and down to one commit. Short version of what changed: the Japanese half you found unreproducible really is fixed on main, and I was wrong to leave it framed as outstanding. What is still there is a Korean one — |
5 participants
Summary
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.Also here: read
coderather thankeyfor those chords while an IME owns the event, because a CJK source rewriteskeyto'Process'(#12171, #13033), and add'Process'to the keybinding matcher's physical-code fallback beside'Dead'— same condition, an unreportable produced key.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 224 lines in
keyboard-handlers.tsand 60 interminal-shortcut-policy.ts. 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, 3513 tests, all passing.src/sharedandsrc/renderer/src/lib— 877 files, 8754 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.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.