Repository navigation
Conversation
2-Set Korean ends the composition on a cursor chord, and the platform then replays that press unmarked. Both copies resolve to the same action, so the chord held for the composition fires on commit and the replay fires straight through: one `Option+←` walks the cursor two words. `Cmd+←` hides it because `\x01` is idempotent. A held chord now owes one absorb credit, spent on the replay that follows. The identity is the one the Enter path already uses — `code` plus `timeStamp` — because Chromium keeps the original timeStamp when it re-dispatches an event. Measured on stock macOS: both ArrowLeft keydowns of a single Option+← over a preedit report timeStamp 23884.000. A genuine second press carries its own and is not absorbed. Japanese and Chinese conversions swallow the chord instead of replaying it, so the credit is never spent there and goes when the deferral settles, rather than waiting to eat a later press. stablyai#14730 and stablyai#15017 fixed the ordering this shares a file with — the syllable no longer travels to the cursor destination, which is what stablyai#12871 was about. The count was never part of that issue. Fixes stablyai#17616
Greptile SummaryThe PR adds identity-scoped replay absorption for terminal cursor chords deferred during IME composition.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.ts | Adds bounded, identity-keyed state for deferred chord replay absorption and lifecycle cleanup. |
| src/renderer/src/components/terminal-pane/terminal-keyboard-event-handlers.ts | Passes chord identity into deferral and suppresses a matching non-composing replay before terminal input is sent. |
| src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.test.ts | Covers one-time absorption, identity mismatches, expiry, abandonment, and replay timing after composition. |
| src/renderer/src/components/terminal-pane/terminal-keyboard-event-handlers-focus.test.ts | Updates the keyboard-handler test double for the new replay-absorption method. |
Sequence Diagram
sequenceDiagram
participant OS as macOS IME
participant Handler as Keyboard handler
participant Sender as Deferred chord sender
participant PTY as Terminal PTY
OS->>Handler: keydown (composing, code + timestamp)
Handler->>Sender: defer(identity, send)
OS->>Sender: compositionend
Sender->>PTY: send chord once
OS->>Handler: redispatched keydown (same identity)
Handler->>Sender: absorbRedispatchedChord(identity)
Sender-->>Handler: true
Handler--xPTY: duplicate suppressed
Reviews (3): Last reviewed commit: "docs(terminal): record that the replay i..." | Re-trigger Greptile
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between ac9948deadacc3f5079efd518c1709fb657273ac and d7ce5b4. 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe deferred IME chord sender now tracks held chords by Merge Risk: 🔵 Low · up to The PR prevents duplicate cursor-chord sends, but a bounded lifecycle edge could rarely suppress or misorder later terminal input if identical events are reused around cancellation. It is mergeable with explicit owner awareness or follow-up on generation-safe cleanup. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description includes all required sections, explains the problem and solution, links issue Full details: Linked Issues checkExplanation The implementation directly addresses issue
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e0552dc-6cdb-4263-8592-77e7b7cec2f8
📒 Files selected for processing (4)
src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.test.tssrc/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.tssrc/renderer/src/components/terminal-pane/terminal-keyboard-event-handlers-focus.test.tssrc/renderer/src/components/terminal-pane/terminal-keyboard-event-handlers.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Chord identity + absorb credit —
defernow takes{ code, timeStamp }, records a single-use absorb credit keyed by that identity, andabsorbRedispatchedChordconsumes it exactly once. - Keydown handler wiring — non-composing
sendInputkeydowns consultabsorbRedispatchedChordbeforesendResolvedInput, so the platform's unmarked replay of a committing chord is dropped while a genuine second press (owntimeStamp) passes through. - Tests — four new sender cases pin the absorb-once behavior, the different-
timeStamp/different-codepassthrough, and credit release on both settle and abandon.
The mechanism mirrors the already-shipped Enter path (terminal-ime-deferred-newline.ts), including the code+timeStamp identity and the ordering where the replayed keydown arrives before the setTimeout(0) deferred send. Targeted tests, tc:web, and the changed-files quality gate all pass.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Credit outlives the composition —
settle(formerlystopWaiting) now clears the hold and arms aTERMINAL_IME_CHORD_REPLAY_WINDOW_MS(1s) expiry instead of forgetting the chord immediately, so the unmarked replay that lands after the commit can still spend its credit. - State machine folded into the map —
DeferredChordStatenow carries thestopWaitingsettle closure andexpiryTimer, replacing the oldpendingStopsset;absorbRedispatchedChordforgets the state as soon as it is spent-and-settled, andcancelPendingcollects closures and clears timers. - Tests pinned to the real ordering — six sender cases model the replay-after-commit sequence, the still-held spend, different-
code/timeStamppassthrough, and credit release on spend, settle, and abandon.
The ordering assumption from the prior review (replay before the deferred-send macrotask) was wrong, and this delta fixes exactly that: the credit is now measured against the correct sequence, and both orderings are handled. Targeted tests and check:code-quality:changed pass.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Not taking it — keep yours. The keyboard file was split by #16335 after I last touched it, so my version would need re-homing across several new modules to do what your 131 lines do inside the sender that already owns the wait. Could the commit carry The timeStamp measurement your commit message cites comes from the probes in #14742, and the two-word jump is what those recordings were taken to pin down. I ran #14742's recorded traces against The part worth raising: the property the fix rests on — Chromium keeping the original Happy to port the portable fixtures whenever it's useful — before or after this merges, your call. |
The credit died with the composition that issued it, which is exactly when it was needed: on 2-Set Korean the chord ends the composition, the commit releases the held chord, and only then does the platform replay the same press. The absorb therefore found nothing and the chord still went twice — the fix did not work. Hold the credit past the commit instead, on its own short window. That is safe because the identity carries the press's own timeStamp, so no later press can spend it; the window only stops a credit that is never claimed — Japanese and Chinese conversions swallow the chord and never replay it — from sitting in the map for the life of the pane. The tests missed this because none of them ran the real order. One absorbed straight after `defer`, before any commit, and another asserted the credit was gone after `compositionend` — pinning the defect as intended behaviour. Both are replaced by cases that dispatch the commit first, and reintroducing the old `forget` on settle now fails two of them. Also pins that a credit is spent while the chord is still held: with the entry still in the map, a check that never decremented would swallow every replay of that press, and no case caught that. Co-Authored-By: kunsanglee <85242378+kunsanglee@users.noreply.github.com>
The absorb keys on Chromium preserving an event's `timeStamp` across a re-dispatch. That was measured by hand, and nothing in the tree asserts it: the unit tests hand the absorb a matching timeStamp, which pins what the credit does given the premise rather than the premise, and a recorded trace would not close it either — a frozen fixture asserts the value written into it and keeps passing if the field stops being preserved. Only real hardware can fail on it, and the macOS IME specs that could carry the assertion do not run in CI. Written down so the observation is not load-bearing and undocumented. Co-Authored-By: kunsanglee <85242378+kunsanglee@users.noreply.github.com>
ac9948d to
d7ce5b4
Compare
|
Added, on both commits — thank you, and for running the traces against it rather than taking my word for the behaviour. The Cmd+ArrowRight cell is the useful one there: your fixture expects nothing where this sends On the premise — you are right, and it is the part of this I am least comfortable with. Recorded it in d7ce5b4 rather than leave Yes to the fixtures, and after this merges if that suits you — the four cells that cannot match today need timeStamp in the recording, which means re-capturing on hardware, and that is a change to your traces rather than a port of them. I would rather that land as its own thing with your name on it than get folded in here. |
This branch has not been deployed
2 participants
ELI5
When you press
Option+←while a Korean syllable is still being typed, the cursor jumps two words instead of one. Orca holds the arrow back until the syllable lands — that part is right — but macOS then hands the same press over a second time, and Orca sends it again. This makes it keep the second copy instead of sending it.What Changed
A chord held for a live composition now owes one absorb credit, and the keydown path spends it on the replay that follows.
The identity is the one the Enter path already uses —
Pick<KeyboardEvent, 'code' | 'timeStamp'>— because Chromium keeps the originaltimeStampwhen it re-dispatches an event. A genuine second press carries its owntimeStampand is not absorbed.Three files, and nothing outside the composing-chord path moves.
Why
2-Set Korean treats a cursor chord as a commit trigger: the composition ends on the chord, and the platform then replays that press unmarked. Both copies resolve to the same action, so the held chord fires on
compositionendand the replay fires straight through:Cmd+←hides it because\x01is idempotent — two of them land where one does.Option+←does not.Japanese and Chinese conversions swallow the chord inside a live preedit instead of committing and replaying it, so nothing is replayed there and the credit is simply never spent. It goes when the deferral settles, so an unspent credit cannot sit waiting to eat a later press.
#14730 and #15017 fixed the ordering — the syllable no longer travels to the cursor destination, which is what #12871 was about. The count was never part of that issue and is not addressed by either: #15017 discards a timed-out chord rather than sending it, which does not deduplicate.
Linked Issue
Fixes #17616
Visual Proof
가나 다라 마바사with사held in the preedit, oneOption+←:다라— two words마바사— one wordThe identity this relies on is measured rather than assumed. Logging
keydownon a text input under 2-Set Korean, oneOption+←over a preedit produces twoArrowLeftkeydowns reporting the sametimeStamp:That is the replay, and it is what the credit is keyed on.
Testing
Reproduced by hand on v1.4.192, macOS, 2-Set Korean, and the measurement above is from the same machine.
terminal-ime-deferred-chord.test.tsgains four cases:timeStamp, and a differentcode, both go through — a genuine second press is not eatencompositionendThe second is the one that would catch this change going wrong rather than restate it: absorbing too eagerly would swallow real input, which is worse than the bug.
AI Disclosure
Claude Opus 5 via Claude Code, used throughout: reading the existing Enter path, writing the change and its tests, and running the gates. The reproduction and the
timeStampmeasurement are mine on my own machine, and every figure quoted here came from running the command rather than an estimate.Review
Code review summary, per CONTRIBUTING:
Maplookup on the keydown path that already resolves a shortcut action, and one small entry per held chord, removed when the deferral settles. No timer, listener or layout read is added.codestring and a numerictimeStamp.defertakes the identity as a new first argument; the only callers are in this repo and both are updated. Behaviour with no replay is what it was.Known limitation, deliberately out of scope: a chord swallowed by a Japanese or Chinese conversion still produces no byte until the composition ends. That is a different defect with a different fix — it needs the chord recovered from the key's release — and it has no issue of its own yet.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
The double send was found and analysed by @kunsanglee in #14742, whose first commit describes the mechanism exactly. That PR was closed as superseded because #12871 — the ordering defect — is genuinely fixed; the count half had no issue of its own, which is why #17616 exists now. This is the narrow fix for that issue alone and does not carry #14742's broader release-keyed recovery, which remains the right shape for the swallowed-chord case and is theirs to land.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)