fix(terminal): dispose a deferred IME chord instead of leaving it armed - #15017
Conversation
The chord held for a live composition waits with `fallbackMs: null`, so nothing but compositionend can end it. `sendTerminalInputAfterComposition` returned void, so nobody could stop it either: the two listeners it puts on the terminal element outlived the pane, and a later composition on that element flushed the stale chord. Return a disposer from the helper and put a sender in front of it that owns every pending chord, so blur and pane teardown drop them the way the Enter path already clears its state. The sender also bounds the wait — generous enough for a conversion candidate window, and it discards rather than sends, because a chord arriving mid-preedit is the corruption the wait exists to prevent. The Enter path needs none of this: its 200 ms fallback always runs, so its listeners cannot outlive it. Pinned so that stays true. Fixes STA-4476
📝 WalkthroughWalkthroughThe change adds explicit disposal for deferred newline input and introduces a deferred IME chord sender. Chords send after 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
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: 0d473490-bcdc-491a-b99e-5211778c6b26
📒 Files selected for processing (6)
src/renderer/src/components/terminal-pane/keyboard-handlers-ime-composing-chord.test.tsxsrc/renderer/src/components/terminal-pane/keyboard-handlers.tssrc/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-ime-deferred-newline.test.tssrc/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 3 remain after this review.
| const finish = (): void => { | ||
| if (done) { | ||
| return | ||
| } | ||
| stopWaiting() | ||
| // xterm flushes the committed glyph after compositionend. | ||
| window.setTimeout(send, 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the disposer cancel the queued send.
After finish() runs, done is already true. A later disposer call returns from stopWaiting() and cannot clear the timer on Line 55. If blur or effect cleanup occurs after compositionend but before that timer runs, the stale chord still sends.
Return a disposer that also clears the post-composition send timer. Add a regression test that disposes in this interval.
Proposed fix
let done = false
+let sendTimer: number | undefined
const finish = (): void => {
if (done) {
return
}
stopWaiting()
- window.setTimeout(send, 0)
+ sendTimer = window.setTimeout(send, 0)
}
-return stopWaiting
+return () => {
+ stopWaiting()
+ if (sendTimer !== undefined) {
+ window.clearTimeout(sendTimer)
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const finish = (): void => { | |
| if (done) { | |
| return | |
| } | |
| stopWaiting() | |
| // xterm flushes the committed glyph after compositionend. | |
| window.setTimeout(send, 0) | |
| let done = false | |
| let sendTimer: number | undefined | |
| const finish = (): void => { | |
| if (done) { | |
| return | |
| } | |
| stopWaiting() | |
| // xterm flushes the committed glyph after compositionend. | |
| sendTimer = window.setTimeout(send, 0) | |
| } | |
| return () => { | |
| stopWaiting() | |
| if (sendTimer !== undefined) { | |
| window.clearTimeout(sendTimer) | |
| } | |
| } |
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.
…ng (stablyai#15017) The chord held for a live composition waits with `fallbackMs: null`, so nothing but compositionend can end it. `sendTerminalInputAfterComposition` returned void, so nobody could stop it either: the two listeners it puts on the terminal element outlived the pane, and a later composition on that element flushed the stale chord. Return a disposer from the helper and put a sender in front of it that owns every pending chord, so blur and pane teardown drop them the way the Enter path already clears its state. The sender also bounds the wait — generous enough for a conversion candidate window, and it discards rather than sends, because a chord arriving mid-preedit is the corruption the wait exists to prevent. The Enter path needs none of this: its 200 ms fallback always runs, so its listeners cannot outlive it. Pinned so that stays true. Fixes STA-4476
Resolves STA-4476. Follow-up to #14730 (
a663f1b), which is where the defect came from.The defect is worse than "a leak"
#14730 deferred a cursor chord until the composing syllable commits, and passed
fallbackMs: null— no timer — because a fallback that fires mid-preedit reintroduces the very corruption the wait prevents.But
finish()was the only thing that removed the two listeners, and with no timer the only callers offinish()were those listeners themselves. Ifcompositionendnever arrives, they are never detached.They are not merely stranded — they stay armed. The listeners live on the terminal element, which outlives the deferral, so the next composition on that pane fires the stale chord.
createCapturedInputSender's guards catch a rebound pty but not the same-pane/same-pty case, which is the common one.Measured against the pre-fix
keyboard-handlers.ts:\x01arriving after unmount is the stale send. An earlier ordering of the same tests showed the other half directly:expected 2 to be 0— two listeners retained per chord, permanently.The fix
sendTerminalInputAfterCompositionnow returns a disposer, and a smallterminal-ime-deferred-chord.ts(51 lines) owns every pending chord.finish()split intostopWaiting()(detach, no send) andfinish()(stopWaiting()then send). That is the only behaviour change to the existing path.cancelPending()is called from the two places that already clear this class of state: the native-only blur handler and the effect teardown, right besidedeferredNewlineSender.clearRedispatchedEnters().TERMINAL_IME_DEFERRED_CHORD_ABANDON_MS = 10_000, which discards rather than sends.That last point is a deliberate divergence from the ticket's literal "bounded fallback": a fallback that fires at any bound reintroduces #12871 in a narrow window. Discarding preserves #14730's accepted trade — a dropped chord costs one keypress, an early one costs a line — while making the trade bounded instead of unbounded.
Why all three and not fewer: a disposer alone still strands a deferral when the pane stays alive and focused, which is reachable because
finishAfterPendingCompositiononly completes when the pending-composition count is zero, and the composition route'sdispose()zeroes that count without dispatching the session-end that would re-check it. A ceiling alone leaves the stale-chord window open for 10 s. Cancel-on-new-composition fixes neither teardown nor the never-ending case.keyboard-handlers.tsmoves 14 lines; the bookkeeping lives in its own module. Nomax-linesdisable added and nothing bumped.The Enter path is clean — checked, not assumed
deferredNewlineSender.defercalls the helper with no options, sofallbackMsresolves to 200 and the timer is unconditional:finish()always runs, so those listeners cannot outlive 200 ms whatever the IME does. Addeddetaches its listeners on the fallback, not only on compositionendto pin it, since that timer is now the only thing keeping the path safe.#12871 stays fixed
The four original cases in
keyboard-handlers-ime-composing-chord.test.tsxare unchanged and green, andtests/e2e/terminal-korean-composing-chord-order.spec.tshas a zero-byte diff againstorigin/main. I ran that e2e — 1 passed,다before\x01at the pty.Verification
npm run typecheckclean; oxlint and oxfmt clean on all six touched filesterminal-korean-composing-chord-order.spec.ts: 1 passedOne note for the record
The investigating agent measured 67 pre-existing failures in this tree and baselined them as unrelated. They were an artifact of its own worktree (missing
node_modules, producingTypeError: … reading 'dimensions'under happy-dom). Re-run in a healthy checkout the same suite is fully green, and the e2e it could not build runs fine. Nothing in that baseline claim affects this change, but the number should not be quoted from the ticket.