Repository navigation
Conversation
xterm paints the preedit into an opaque .composition-view anchored to the cursor cell, so composing mid-line hid the character sitting there until the composition committed. Nothing reaches the pty while composing, so those cells still held the old text — repaint that row remainder just past the preedit so the composition reads as insertion pushing it right, matching macOS Terminal.app. The tail is its own element: xterm rewrites .composition-view's textContent on every compositionupdate, clips it, and flips it to direction: rtl. It is laid out cell by cell off the buffer so glyph advances cannot drift off the terminal grid, starts on the cell the preedit ends on rather than on the measured box (whose bidi marks left a ragged gap), and re-reads the row when a committed syllable echoes mid-composition — every Hangul syllable commits before the next one starts, so dropping the tail there hid it from the second syllable onwards. Masking needs an opaque backdrop, so a see-through terminalBackgroundOpacity theme falls back to the first opaque ancestor. Fixes stablyai#12545 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an IME composition tail overlay for terminal content hidden by the preedit box. The overlay resolves an opaque backdrop, measures cell widths through the terminal Unicode service, renders cell-aligned text, updates during composition and terminal output, and cleans up listeners and DOM state. Pane and preview terminal lifecycles now install and dispose the overlay. Tests cover rendering, positioning, redraws, colors, composition completion, and cleanup. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3360033d-562e-4090-981d-ad04fa306687
📒 Files selected for processing (7)
src/renderer/src/components/dashboard-popout/preview-terminal-compatibility.tssrc/renderer/src/lib/pane-manager/pane-dom-creation.tssrc/renderer/src/lib/pane-manager/pane-lifecycle.tssrc/renderer/src/lib/pane-manager/pane-manager-types.tssrc/renderer/src/lib/pane-manager/terminal-ime-composition-tail.test.tssrc/renderer/src/lib/pane-manager/terminal-ime-composition-tail.tssrc/shared/terminal-unicode-provider.ts
cursorY is viewport-relative but getLine indexes the whole buffer, so once output pushed baseY past zero the tail repainted a stale scrollback line over the cursor row. Read baseY + cursorY, from one helper both call sites share. isOpaqueColor only recognised alpha that ended in a bare number, so rgb(20 20 20 / 50%) and hsla(..., 0.5) passed as opaque and the mask let the covered cells show through. Parse the alpha argument itself — legacy comma and slash forms, percentage or number — across the rgb/hsl/hwb/lab families. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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: ae7763ad-ef60-43af-bf98-425c01b66043
📒 Files selected for processing (2)
src/renderer/src/lib/pane-manager/terminal-ime-composition-tail.test.tssrc/renderer/src/lib/pane-manager/terminal-ime-composition-tail.ts
…drop isOpaqueColor rejected #ffff and #ffffffff, which are fully opaque, so an alpha hex theme background fell through to the surface behind it. Read the alpha nibble or byte instead of the string length, and drop malformed hex, which CSS would ignore and leave the mask see-through. A translucent theme background is now layered over the first opaque surface behind it rather than swapped for one, so the mask matches the row it replaces instead of dropping a black block onto a light theme. Geometry comes off the cell grid rather than off .composition-view: xterm only assigns that element's position from compositionupdate, so the first composition of a terminal placed the tail in the top-left corner. The grid is re-measured on resize, and the tail stays hidden while the viewport is scrolled away from the cursor row, where there is nothing on screen to mask. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/lib/pane-manager/terminal-ime-composition-tail.ts (1)
270-273: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResnapshot the row on
compositionupdate.If the viewport moves after
compositionstart,rowTailremains populated.handleCompositionUpdatecallspaint(), andrenderTailCells()then uses that stale value whenreadCursorLine()returnsundefined. The tail becomes visible over an unrelated viewport row.
src/renderer/src/lib/pane-manager/terminal-ime-composition-tail.ts#L270-L273: CallresnapshotAndPaint()after updatingpreedit.src/renderer/src/lib/pane-manager/terminal-ime-composition-tail.test.ts#L218-L236: Keep composition active, move the viewport, dispatchcompositionupdate, and assert that the tail remains hidden.Proposed fix
const handleCompositionUpdate = (event: Event): void => { preedit = (event as CompositionEvent).data ?? '' - paint() + resnapshotAndPaint() }Based on PR objectives, hide the tail when the cursor row is outside the visible viewport.
🧹 Nitpick comments (1)
src/renderer/src/lib/pane-manager/terminal-ime-composition-tail.ts (1)
63-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the implementation comments.
Keep only the non-obvious constraint at each site.
src/renderer/src/lib/pane-manager/terminal-ime-composition-tail.ts#L63-L69: State the translucent-background layering rule in one concise comment.src/renderer/src/lib/pane-manager/terminal-ime-composition-tail.ts#L202-L207: State why cell-grid geometry is required in one concise comment.As per coding guidelines, comments must be concise and preferably one line.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cded5999-b67e-4ef1-bf3c-53a77ab5c169
📒 Files selected for processing (2)
src/renderer/src/lib/pane-manager/terminal-ime-composition-tail.test.tssrc/renderer/src/lib/pane-manager/terminal-ime-composition-tail.ts
compositionupdate only repainted, so a viewport that moved after compositionstart left rowTail populated and renderTailCells fell back to that stale snapshot — the tail stayed on screen over an unrelated row. Re-read the row instead, which hides it when the cursor row is no longer visible. Also trim the two block comments added with the cell-grid geometry down to the constraint each one exists to record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai Both findings from the outside-diff section of the last review are addressed in 13f7cfb.
Regression test:
Verified: 640 tests in One process note for anyone following along: these two landed in the review body rather than inline because GitHub only accepts inline comments on lines present in the PR diff, and the previous commit did not touch them. Reading |
|
Tip For best results, initiate chat on the files or code changes.
The shortened comments also address the style finding. Thank you for the detailed verification. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
@coderabbitai review |
|
|
@OrcaWin Could you take a look at this PR when you get a chance? Thanks! |
|
Closing — #12545 is fixed on main by #15014 ( Your PR is careful work and it was a genuinely close call, so it's worth saying exactly what decided it rather than leaving it as a preference. Both PRs aimed at the right thing. I went in assuming the floating overlay was itself the root cause — that mature terminals render preedit inline in the grid and we should too. That turned out to be wrong: two reference implementations both render inline and occlude the cells underneath identically. The real root cause is preedit being treated as an overwrite rather than an insertion, so rendering the covered tail is the correct fix at any layer. You and tonite31 both had that right. What decided it was geometry ownership. #13834 renders the tail inside xterm's own composition view, which is already placed, sized, clipped and font-matched at the cursor cell — so the tail's start is correct by construction. Yours re-derives the cell grid, font, clip and resize handling in the renderer, and computes the tail start as Your instinct about DOM text not advancing on the grid was also the sharpest thing either PR noticed, and it's directly why the merged version needed Thanks for it, and sorry it took this long to get a decision. |
Fixes #12545.
Summary
Composing Korean in the middle of a line hid the character sitting at the cursor for the whole
composition. xterm paints the preedit into
.composition-view, which upstream styles opaque(
background: #000; position: absolute; z-index: 1) and anchors to the cursor cell. Nothing reachesthe pty while composing, so that cell still holds its original character — the box simply covers it.
macOS Terminal.app renders the preedit as inserted text and keeps the following character visible;
iTerm2 and stock xterm hide it.
installTerminalImeCompositionTailrepaints the row remainder just past the preedit while acomposition is open, so the composition reads as an insertion pushing the tail right.
The overlay is its own element rather than a child of
.composition-view, which xterm rewrites onevery
compositionupdate, clips withoverflow: hidden, and flips todirection: rtl. Four detailsmatter:
width = columns × cellWidth, and positioned from the cell grid rather than from.composition-view. A plain text node advances on the font's own metrics, which runs tighter thanthe terminal grid and looked visibly cramped. xterm also only assigns the preedit box its geometry
on
compositionupdate, so reading that element would place a terminal's first composition in thetop-left corner. The grid is re-measured on
resize.emulator's own width tables (new
measureTerminalStringColumnsinshared/terminal-unicode-provider.ts). Measuring the preedit box instead inherits the bidi marksxterm wraps it in and leaves a ragged gap.
lands mid-composition. The tail re-reads the row (coalesced to one repaint per frame) instead of
being dropped — dropping it hid the tail from the second syllable onwards.
baseY + cursorY, sincecursorYis viewport-relative whilegetLineindexes the whole buffer. The tail hides entirely while the viewport is scrolled awayfrom the cursor row, and re-reads on every
compositionupdateso a viewport that movesmid-composition cannot leave a stale snapshot on screen.
Masking needs an opaque backdrop. A translucent
terminalBackgroundOpacitytheme is layered overthe first opaque surface behind it rather than swapped for one, so the mask matches the row it
replaces instead of dropping a black block onto a light theme.
Screenshots
Captured from the packaged app driven through Playwright (
electron-headful): type가나다라마,walk the cursor back three syllables, then open a composition on
바. Both frames are the same livestate — the "before" frame simply has the tail element suppressed, which is exactly the old
behavior.
Before —
다is hidden under the preedit box for the whole composition:After —
다is repainted just past the preedit, reading as an insertion pushing it right:The captured DOM state for the "after" frame: tail
textContent="다라마",display=block,left=440px, against the preedit box atleft=424px— the tail resumes exactly two cells (onedouble-width syllable) past where the preedit starts.
Testing
pnpm lintpnpm typecheckpnpm test— 45,482 passing. 9 failures in 3 files (check-root-directory-entries.test.mjs,codex-fetcher-pty-settle.test.ts,agent-exec-handler.test.ts) reproduce identically onorigin/mainwith none of this branch applied, so they are pre-existing and unrelated. Theroot-directory guard ones are a macOS-only artifact: the test shells out to
bash, and theguard script uses
declare -A, which macOS's bash 3.2 rejects.pnpm buildterminal-ime-composition-tail.test.tsruns 20 cases against the real emulator: mid-line repaint,end-of-row no-op, scrolled-buffer row read, viewport-scrolled-away hide, mid-composition viewport
move, cell-grid advance, preedit-end start column, first-composition placement, resize re-measure,
commit-echo re-read, composition-end cleanup, opaque / translucent / alpha-hex backdrops, and
listener + element teardown.
Every regression test was verified to fail against the pre-fix code before being kept.
Manually verified on macOS with the Korean 2-Set IME in the integrated terminal.
AI Review Report
Reviewed with Claude Code across the risks below. CodeRabbit also reviewed the branch; all five of
its findings are fixed and confirmed resolved.
and writes to a DOM element. No path handling, no shell invocation, no Electron main-process code,
no keyboard shortcut or accelerator handling, and no
metaKey/ctrlKeybranching — so none ofthe usual platform divergences are touched. It hangs off the DOM
composition*events, whichbehave the same on all three platforms. Manual verification is macOS-only (see Notes).
terminal.buffer.activein the renderer and writes nothing to the pty. Local, remote and SSHpanes take the identical path, and no round trip is added to composition handling. Installed in
both pane lifecycle (
pane-lifecycle.ts) and the dashboard popout preview(
preview-terminal-compatibility.ts), with a disposer stored on the pane sodisposePane()tearsit down.
compositionupdate: one buffer line read plus up tocolsspan elements.Geometry comes from the cell grid measured at
compositionstartand onresize, so no layoutread was added per
compositionupdate(keeping clear of Korean/CJK IME input lag in terminal panes — installTerminalImeCandidateAnchor forces layout on every compositionupdate #12211). Output-driven repaints fromonWriteParsedare coalesced to one per animation frame. Cleanup covers the three compositionlisteners, the
onWriteParsedandonResizedisposables, a pendingrequestAnimationFrame, andthe element itself.
z-index: 0against xterm's1) so a wideglyph never clips it, stops where the row's content stops rather than at the right margin, and
clamps to the screen width. Backdrop resolution walks to the first opaque ancestor and layers a
translucent theme over it, which was checked against both light and dark themes.
Flagged and changed as a result:
getLine(cursorY)ignoring scrollback (repainted a stale row onceoutput scrolled), translucent CSS colors passing as opaque (
rgb(… / 50%),hsla(…)), fullyopaque alpha hex being rejected (
#ffff,#ffffffff), andcompositionupdatenot resnapshotting(left a stale tail after a viewport move). Each has a regression test.
Security Audit
No input execution, path handling, IPC, auth, secrets, network access, or dependency changes.
textContent, neverinnerHTML, so terminal outputcannot inject markup.
style.backgroundImageinsidelinear-gradient(…). It can only get there afterisOpaqueColor(), whose functional-color pattern is anchored and forbids nested parentheses, sothe value cannot terminate the
linear-gradient()early.style.backgroundColordrops invalidcolors on its own. No injection path found. Malformed hex is now rejected outright rather than
treated as opaque, since CSS would ignore it and leave the mask see-through.
into the same window, for the duration of the composition. Nothing is persisted or transmitted.
No follow-up needed.
Notes
foreground/background for the duration of the composition.
(see above), but the IME stacks are not —
terminal-ime-e2e.ymlcovers Linux X11 withibus-hanguland would be the place to confirm it in CI.track-community-prexecuted, on the first commit. The remainingworkflows appear to be waiting on maintainer approval for a fork PR.