Skip to content

fix(terminal): correct macOS Hangul/CJK IME input - #1053

Closed
h1tTAKA wants to merge 1 commit into
crynta:mainfrom
h1tTAKA:fix/macos-hangul-cjk-ime
Closed

fix(terminal): correct macOS Hangul/CJK IME input#1053
h1tTAKA wants to merge 1 commit into
crynta:mainfrom
h1tTAKA:fix/macos-hangul-cjk-ime

Conversation

@h1tTAKA

@h1tTAKA h1tTAKA commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes Hangul/CJK IME input in the terminal on macOS. Composed text now reaches the PTY correctly, and Latin / symbols / IME switching keep working.

Closes #813.

Problem

macOS WKWebView surfaces IME input in two different ways depending on the OS / webview build, and the terminal only worked in neither:

  1. Composition events fire (compositionstart/update/end). Here xterm's own CompositionHelper assembles the syllable and emits the committed text through onData — it works, and anything extra we do corrupts it.
  2. No composition events — the composed stream arrives only as textarea input events (insertText / insertReplacementText / insertCompositionText) with isComposing false. xterm can't assemble a syllable from that, so Hangul reached the PTY broken.

Reproduction of case 2 (Korean 2-beolsik):

Typed Before After
안녕 ㅇ아... (leading jamo split off) 안녕
사과 ㅅ사과 사과

Fix

Detect the mode at runtime and never fight xterm:

  • On the first compositionstart, mark the slot native and get out of the way entirely — xterm owns IME, Latin, and symbols.
  • Until/unless that happens, reconstruct the committed stream from the input events (ime.ts), and drop xterm's spurious re-delivered printable onData so it can't double the leading jamo. Control sequences and bracketed paste carry control bytes and pass through.

Everything is gated on IS_MAC and the native flag, so non-macOS and composition-capable webviews run exactly xterm's normal path — zero behavior change there.

Testing

  • pnpm check-types, pnpm lint — clean.
  • pnpm test — full suite green; ime.test.ts covers the reconstruction core for both the replacement-event trace (안녕) and the composition-event trace ().
  • Manual on macOS (WKWebView, 2-beolsik): Hangul composition, spacing, no doubled jamo; Latin, digits, symbols (!@#$%^&*()_+-=~|?/), bracketed paste, IME language switching, and Enter / arrows / Backspace all correct.

Relationship to prior PRs

Builds on the input-reconstruction idea in #623 / #718. Those reconstruct unconditionally, which breaks on webview builds that DO fire composition events (double / scrambled output) and don't dedup xterm's re-delivery. This detects the mode and defers to xterm when it can handle IME itself, reconstructing only when it can't.

Summary by CodeRabbit

  • New Features

    • Improved macOS terminal input handling for IME-based languages.
    • Composition, replacement, deletion, and committed text now render correctly without duplicated characters or leaked intermediate symbols.
    • Supports more reliable Unicode-aware editing during terminal input.
  • Bug Fixes

    • Prevented duplicate IME text and visual flicker during composition.
    • Fixed deletion behavior for composed text, including edge cases involving empty input.

@h1tTAKA
h1tTAKA requested a review from crynta as a code owner July 27, 2026 14:25
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 070ae595-6d47-48e9-a4eb-7e19fcb5d04a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

macOS terminal input now reconstructs IME updates into PTY writes, tracks uncommitted composition state per slot, and filters duplicate printable deliveries. Tests cover Unicode deletion, replacement suppression, composition text, unsupported inputs, and Korean composition traces.

Changes

macOS IME input

Layer / File(s) Summary
IME reconstruction core
src/modules/terminal/lib/ime.ts, src/modules/terminal/lib/ime.test.ts
Adds Unicode-aware reconstruction for inserts, replacements, deletes, composition text, unsupported inputs, and Korean IME traces.
IME input wiring
src/modules/terminal/lib/rendererPool.ts
Tracks uncommitted IME units, attaches xterm textarea input handling, reconstructs IME updates, and writes them to the PTY.
Duplicate printable input filtering
src/modules/terminal/lib/rendererPool.ts
Suppresses duplicate printable macOS deliveries and resets IME state after terminal data handling.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address #813 by reconstructing macOS Korean IME input and preserving correct syllable composition.
Out of Scope Changes check ✅ Passed The new logic and tests stay focused on macOS IME handling with no clear unrelated changes.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and clearly summarizes the macOS IME input fix in the terminal.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@h1tTAKA
h1tTAKA force-pushed the fix/macos-hangul-cjk-ime branch from d3ae4ff to bf768e6 Compare July 27, 2026 14:27

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 220-225: Track each payload written by the IME handler around ta’s
input listener, then have the xterm onData path consume only the matching
deferred release, including the deleteContentBackward DEL case, instead of
filtering all printable data via isPrintableOnly; preserve unbracketed paste and
other non-IME printable delivery. Update both affected rendererPool.ts sites
(220-225 and 339-344) and extend ime.test.ts (61-83) to cover correlated
consumption and path-dependent non-IME input.
🪄 Autofix (Beta)

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: a20b827f-02a5-4b83-ac59-4fda945f6805

📥 Commits

Reviewing files that changed from the base of the PR and between e9ee1d6 and d3ae4ff.

📒 Files selected for processing (3)
  • src/modules/terminal/lib/ime.test.ts
  • src/modules/terminal/lib/ime.ts
  • src/modules/terminal/lib/rendererPool.ts

Comment thread src/modules/terminal/lib/rendererPool.ts
macOS WKWebView surfaces IME input in two different ways depending on the OS /
webview build:

1. It fires real composition events (compositionstart/update/end). Here xterm's
   own CompositionHelper assembles the syllable and emits the committed text
   through onData — it just works, and anything extra we do corrupts it.
2. It fires NO composition events and delivers the composed stream only as
   `input` events (insertText/insertReplacementText/insertCompositionText) with
   isComposing false. xterm can't assemble a syllable from that, so Hangul
   reached the PTY broken (leading jamo split off, e.g. 안녕 -> ㅇ아).

Handle both. On the first `compositionstart` we mark the slot native and get out
of the way entirely (xterm owns IME). Until/unless that happens, we reconstruct
the committed stream from the input events (ime.ts) and drop xterm's spurious
re-delivered printable onData so it can't double the jamo. Everything is gated
on IS_MAC and on the native flag, so non-macOS and composition-capable webviews
run exactly xterm's normal path.

Scoped to macOS. Unit tests cover the reconstruction core for both the
replacement and composition event traces.
@h1tTAKA
h1tTAKA force-pushed the fix/macos-hangul-cjk-ime branch from bf768e6 to ce67e85 Compare July 27, 2026 15:40
@h1tTAKA

h1tTAKA commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@sskys18

sskys18 commented Aug 3, 2026

Copy link
Copy Markdown

Tested this branch on macOS 26 / Apple Silicon (M1 Pro), rebased onto current main (24 commits ahead of your base, merges clean). Korean input works. pnpm check-types and pnpm test are green, and the three biome check complaints on rendererPool.ts are all pre-existing on main, so this adds none.

One thing I think is off in the delete branch of ime.ts:

if (inputType.startsWith("delete")) {
  return { send: DEL.repeat(Math.max(1, cpLen(unit))), unit: "" };
}

Backspace pressed during an active preedit looks like it reaches the PTY as two DELs, not one:

  1. keydown fires first. Backspace is keyCode 8, and in the non-composition model isComposing is false, so the guard in attachCustomKeyEventHandler does not take it. It is also not printable, so xterm 6's one-tick deferral for printable keydowns (Handle IME-transformed printable input xtermjs/xterm.js#6024) does not apply. xterm evaluates it and emits \x7f through onData.
  2. The new onData filter does not catch it: isPrintableOnly returns false for 0x7f, which is correct for ordinary backspace but means this DEL reaches the PTY.
  3. Then the textarea input event (deleteContentBackward) fires, imeReconstruct returns DEL.repeat(max(1, cpLen(unit))), and that DEL is written too.

For a one-code-point preedit unit that is 2 DELs for 1 keypress, so the character before the preedit gets eaten as well.

Since keydown always precedes input, the unit's first code point is already accounted for by xterm's own DEL, so the reconstruction only needs to erase the remainder:

// xterm's Backspace keydown already wrote one DEL before this input event
// fired, so only erase the remainder of the unit.
if (inputType.startsWith("delete")) {
  return { send: DEL.repeat(Math.max(0, cpLen(unit) - 1)), unit: "" };
}

Math.max(0, ...) also matters for the unit === "" case. Math.max(1, ...) there emits a DEL for a preedit that does not exist, which would double an ordinary backspace on any path where an input event still fires with an empty unit.

This changes two of the existing tests, since "always erases at least one code point on delete" locks in the extra DEL. I split it into three cases (single code point, multi code point, no active unit) and the suite stays green at 560.

Branch with the change on top of yours, if it is useful: https://github.com/sskys18/terax-ai/tree/fix/macos-hangul-ime

To be straight about confidence: this is derived from reading the event ordering and the isPrintableOnly predicate, not from a captured trace of a backspace during preedit. You already captured the real traces for the insert path, so you are better placed to confirm whether WKWebView actually emits deleteContentBackward there. If it does not fire while a preedit is active, the branch is dead code and the point is moot.

Happy to leave this with you either way. Getting #813 fixed matters more than whose PR does it.

@sskys18

sskys18 commented Aug 3, 2026

Copy link
Copy Markdown

Separate from the code: this PR is mergeStateStatus: BLOCKED for a mechanical reason, not a review one.

#1053  workflow runs: 0   check-runs: 0   mergeable: MERGEABLE   mergeState: BLOCKED
#1082  coverage, frontend, rust, rust-test (macos-latest), rust-test (windows-latest): all success  ->  merged

CI never ran on ce67e85. ci.yml triggers on pull_request: branches: [main], so the run is sitting behind the approval gate that GitHub applies to contributors without a merged PR. The required checks are therefore absent and the merge button stays locked regardless of how the code looks.

The useful part: that gate no longer applies to you. #1079 merged on 2026-08-01, so a fresh push to this branch should have workflows run automatically. A rebase onto current main (it merges clean, I checked) or an empty commit should be enough to get the checks to populate and move this out of BLOCKED.

Worth doing soon. #623 and #718 are both DIRTY now, and they were solving the same bug.

@crynta

crynta commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Thanks for the careful work and real macOS testing. We are consolidating this fix in #1112, whose author has been asked to incorporate this PRs important native-composition detection while keeping a narrower PTY path. I am closing this one as superseded so we have one canonical implementation. We really appreciate the investigation.

@crynta crynta closed this Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

I Can't use korean input(IME)

3 participants