Skip to content

fix(terminal): preserve ANSI background under IME preedit - #16230

Open
Noobear wants to merge 1 commit into
stablyai:mainfrom
Noobear:Noobear/fix-macos-ime-background
Open

Noobear wants to merge 1 commit into
stablyai:mainfrom
Noobear:Noobear/fix-macos-ime-background

Conversation

@Noobear

@Noobear Noobear commented Aug 24, 2026

Copy link
Copy Markdown

Summary

  • paint xterm's opaque IME composition overlay with the cursor cell's effective ANSI background instead of the terminal-wide theme background
  • resolve RGB, indexed palette, default, and inverse cell colors while preserving the existing mid-line tail masking behavior
  • regenerate the canonical xterm patch and lockfile hash, and add unit plus Electron regression coverage for styled prompt rows

Why

The composition overlay intentionally stays opaque so committed cells hidden beneath it remain readable through the rendered row tail. On a prompt row with its own ANSI background, however, the overlay used the terminal theme background and produced a visible seam behind the active Korean preedit.

Validation

  • pnpm install --frozen-lockfile --ignore-scripts
  • 27 related renderer test files: 342 passed
  • focused xterm IME tests on macOS arm64: 17 passed
  • macOS arm64 Electron e2e: 3 passed
    • mid-line Korean tail remains readable
    • styled ANSI background matches the opaque overlay
    • end-of-row composition remains unchanged
  • node config/scripts/regenerate-xterm-patches.mjs --check on macOS arm64
  • pnpm typecheck:web
  • targeted oxlint for the changed TypeScript tests/probe

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The patch updates CompositionHelper to track deferred IME transactions, reconcile composition, input, and keypress data, and handle cancellation and disposal. CoreBrowserTerminal routes input through the helper and emits accepted-transaction events. Composition overlays now use opaque colors derived from the cursor cell and support trailing preedit positioning. Unit and end-to-end tests cover ANSI, inverse, indexed-color, and Korean preedit cases.

Merge Risk: 🟡 Moderate · up to 79fe4

An empty IME cancellation can leave the terminal composing state stuck, causing subsequent user input to be withheld instead of sent. The PR is not merge-ready until this bounded input-loss risk is fixed or explicitly accepted.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the change, rationale, and validation, but it omits required template sections including Linked Issue, Visual Proof, and checklist confirmations. Use the repository template headings, add a linked issue, provide visual proof or an explicit N/A reason, and complete the required testing and review checklists.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the terminal IME fix and the preservation of ANSI backgrounds under the preedit overlay.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch (1)

342-348: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Settle an empty compositionend for the active transaction.

If a composition starts and then ends with empty data before the textarea or selection changes, _compositionEndBelongsToCurrentTransaction stays false. The deferred callback then returns without clearing _isComposing. Later input events without a keydown are stored in _compositionInputData and are not sent. Treat this current-transaction case as cancellation after deferred inspection.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b2d16bf0-8970-41ec-807c-638f63ee822f

📥 Commits

Reviewing files that changed from the base of the PR and between afd76a4 and 79fe456.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • config/patches/@xterm__xterm@6.1.0-beta.287.patch
  • config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch
  • src/renderer/src/components/terminal-pane/terminal-ime-xterm-trailing-preedit-occlusion.test.ts
  • tests/e2e/terminal-ime-midline-occlusion-probe.ts
  • tests/e2e/terminal-korean-midline-preedit-occlusion.spec.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

This branch has not been deployed

No deployments
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.

1 participant