fix(terminal): make macOS Korean/CJK IME input reliable (forward replacements + compensate xterm's dropped insertText) - #1112
Conversation
On macOS (WKWebView) the Korean/CJK IME drives composition through `input` events — `insertText` for a new glyph and `insertReplacementText` as the in-progress syllable is refined (ㅇ → 아 → 안) — and never fires the standard composition* events. xterm forwards `insertText` to the PTY but silently drops `insertReplacementText`, so only the first jamo of each syllable reached the shell: typing "안녕" arrived as "ㅇㄴ". Bridge the gap on the terminal textarea: track the last uncommitted glyph and, on `insertReplacementText`, erase it with DEL and write the refined syllable so the shell mirrors what the IME shows. `insertText` is still handled by xterm; we only record it so the next replacement knows how many code points to erase. This is the remaining half of crynta#147 (the keydown guard from crynta#196 stopped the duplicate raw keys but never restored the dropped replacements). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… input Two follow-up fixes on top of the insertReplacementText bridge: 1. Track only the last code point of each IME event as the pending glyph. A replacement can carry a commit plus a new composition in one event (jamo carry-over: "간" + ㅏ → "가나"); erasing the full previous event data on the next refinement swallowed the committed character. 2. Forward insertText that xterm drops during fast typing. xterm's _inputEvent refuses insertText while _keyDownSeen is set (keydown seen, keyup pending) or _keyPressHandled is stale. The macOS IME fires input BEFORE keydown, so a syllable committed right after another key — most visibly the first syllable after a space — never reached the PTY. ASCII input still goes through the keydown/keypress path untouched; only non-ASCII (IME) text is compensated, so nothing double-writes. Verified by logging the WKWebView event stream while typing: the drops correlate exactly with _keyDownSeen/_keyPressHandled at input time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe terminal renderer adds macOS WebKit IME handling. It forwards dropped non-ASCII text, replaces previously sent glyphs during composition, and clears pending state on composition end or blur. Tests cover Hangul composition, key states, leaf changes, and ignored events. ChangesIME input handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/modules/terminal/lib/rendererPool.ts (1)
250-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the IME commentary and remove prohibited punctuation.
Keep one or two lines that explain why the native input bridge is necessary. Remove the step-by-step event narrative and the em dash characters.
As per coding guidelines,
**/*.{ts,tsx,rs}requires minimal why-focused comments, and**/*.{ts,tsx,rs,md,json}prohibits em dashes and emojis.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/terminal/lib/rendererPool.ts` around lines 250 - 259, Shorten the comment above the native input bridge to one or two lines that briefly explain its purpose for macOS WebKit IME input. Remove the detailed event sequence and all em dash characters, emojis, and other unnecessary commentary while preserving the why-focused explanation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/terminal/lib/rendererPool.ts`:
- Around line 268-321: Add an event-level regression test for the input
listeners in the terminal renderer flow, covering PTY write order for dropped
non-ASCII insertText, pending-glyph replacement, jamo carry-over, and ensuring
ASCII insertText is not duplicated. Also verify that compositionend and blur
clear pending glyph state so subsequent replacements do not erase stale
characters.
- Around line 264-320: Reset the slot-level IME state when ownership changes,
since the persistent listener’s pendingGlyph can survive leaf replacement
without compositionend or blur. Update the bindSlot acquisition/rebind and
detachment or reuse paths to clear pendingGlyph whenever slot.currentLeafId is
reassigned or released, while preserving the existing input-event tracking
behavior.
---
Nitpick comments:
In `@src/modules/terminal/lib/rendererPool.ts`:
- Around line 250-259: Shorten the comment above the native input bridge to one
or two lines that briefly explain its purpose for macOS WebKit IME input. Remove
the detailed event sequence and all em dash characters, emojis, and other
unnecessary commentary while preserving the why-focused explanation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 502dee07-821f-4072-a04c-dfa5d7ccf85e
📒 Files selected for processing (1)
src/modules/terminal/lib/rendererPool.ts
…sion tests Address review feedback: - Move the WebKit IME decision logic out of createSlot into imeBridge.ts, a DOM-free pure function that returns exactly what to write to the PTY. rendererPool now only wires DOM events and reads xterm's key flags. - Reset pending IME state when the slot rebinds to a different leaf, so a glyph composed in one pane can never trigger a stray DEL in another (slots are pooled across panes and DOM blur is not guaranteed). - Add imeBridge.test.ts locking the invariants: in-place syllable refinement, jamo carry-over (간 + ㅏ → 가나) never erasing the committed character, forwarding of insertText xterm drops (key held / stale keypress after space), ASCII never double-written, and state cleanup on compositionend, blur, and leaf rebind. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the thorough investigation, tests, and for preserving #718 authorship. After comparing the competing IME PRs, this looks like the best base. Before merge, could you please address three remaining contracts:
Please keep the surgical bridge approach and avoid broad onData filtering. CI is green, and once these invariants are covered this should be close. |
Summary
This builds on #718 (included here with original authorship, rebased onto current
main) and adds two follow-up fixes discovered while dogfooding Korean input. Together they take Korean typing in the terminal from "first jamo only" to fully reliable at real typing speed.Fixes #147's remaining half, fixes #1105.
1. Rebase of #718 — bridge
insertReplacementText(author: @kmsdoit)On macOS WKWebView the Korean IME drives composition through
inputevents (insertText/insertReplacementText) and never firescomposition*events, so xterm silently drops every refinement — typing 안녕 arrived as ㅇㄴ. #718's bridge erases the previously-sent glyph with DEL and writes the refined syllable. Rebased becausecreateSlot()has since movedattachWebglout of that spot.2. Jamo carry-over swallowed the committed syllable
A single replacement event can carry a commit plus a new composition: 간 + ㅏ → one event with data 가나 (the batchim migrates to the next syllable). Tracking the full event data as the pending glyph meant the next refinement erased two code points — deleting the already-committed 가. Now only the last code point is tracked; earlier code points are committed and must never be erased again.
3. xterm drops committed syllables during fast typing
_inputEventin xterm refusesinsertTextwhile_keyDownSeenis set (keydown seen, keyup still pending) or_keyPressHandledis stale-true. The macOS IME firesinputbeforekeydown, so at real typing speed the previous key's keyup hasn't landed yet and the committed syllable never reaches the PTY. The most visible case: the first syllable right after a space (space's keyup lands ~30ms later), e.g. 것 같아 → 것 ㅏ같아 minus a glyph.I verified this by logging the full WKWebView event stream while typing — the drops correlate exactly with
_keyDownSeen/_keyPressHandledatinputtime:The fix replicates xterm's exact drop condition and forwards the data over the PTY bridge itself — but only for non-ASCII data. ASCII (space, English) still reaches the PTY through the keydown/keypress path, so nothing double-writes; Hangul never fires keypress, so the stale
_keyPressHandledflag must not veto it.Testing
tsc --noEmitandbiome checkclean on the touched file🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor