Repository navigation
Conversation
A mid-line composition renders the rest of the row inside `.composition-view` so the preedit reads as inserted text pushing the tail right (stablyai#12545). The tail came from `translateToString`, which returns characters and drops every cell attribute, and the view paints one themed colour over everything it covers — so any cell not drawn in the default foreground came back in it. The visible case is composing the first Hangul syllable into an empty agent CLI prompt. The syllable is still in the preedit, so nothing reached the pty and the CLI's dim placeholder is still on the row; the tail redraws it at full brightness and it reads as text the user had typed. Typing on clears it, and Enter submits only the syllable, which places the defect in the overlay rather than in the buffer. Latin input never shows it, because a committed character empties the placeholder before any tail is rendered. Read the cells instead, and emit the tail as runs that share a rendered style, each carrying the colour and flags the grid resolved for it: palette, RGB and default foregrounds, backgrounds, dim, bold, italic, inverse, invisible, underline, strikethrough, overline. A run that needs no override stays a bare text node and keeps inheriting the view's colour, so the default tail emits exactly the DOM it did before. The end column still comes from `getTrimmedLength()`, so the tail covers the columns it always did. The tail refresh keyed off the rendered text, which cannot see a repaint that recolours a row without changing it, so it now keys off text and styling together. The theme arrives through a setter rather than the constructor: widening the constructor needs a hunk in upstream's `CompositionHelper.test.ts`, which `.npmignore` strips from the published tarball and the generator therefore cannot carry. Before it is handed over, a tail renders as it does today. The DOM renderer's own `xterm-fg-*` and `xterm-dim` classes are not reusable here — they are scoped to the rows container and only generated while that renderer is the active one. Minimum contrast is not applied to a tail run. The renderer adjusts a cell against its own background; doing that here would need the same cache and still resolve against a different background, and leaving it out keeps every cell closer to the grid than one flat colour was.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe patch adds transaction-based IME composition handling across composition, keypress, input, blur, and disposal events. It prevents duplicate commits and handles cancellation and stale transactions. Composition overlays render styled terminal tail cells and update their positioning after repaints. CoreBrowserTerminal injects theme state and manages CompositionHelper lifecycle. SortedList deletion retries after pending cleanup changes sort keys. A test suite covers composition-tail styling and repaint behavior. Merge Risk: ⚪ Minimal · up to The change preserves each cell’s existing colors while rendering an active composition tail, without changing terminal input or network behavior. No actionable merge-blocking risk remains after normal checks and review. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch (1)
1157-1162: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard the tail refresh on overlay state, not on preedit text.
The refresh runs only when
this._compositionViewDatais truthy. An IME can sendcompositionupdatewith a single-character preedit and later a repaint, which is covered. But a composition whose current preedit string is empty while the view stays active keeps_compositionViewData === '', so a row repaint no longer re-renders the tail and the overlay keeps stale cells until the next composition event. Track a separate "tail rendered" flag or compare against_compositionRemainderKey !== undefinedinstead.♻️ Proposed guard
- if ( - this._compositionViewData && - this._getRowRemainder().key !== (this._compositionRemainderKey ?? '') - ) { + if ( + this._compositionViewData !== undefined && + this._getRowRemainder().key !== (this._compositionRemainderKey ?? '') + ) { this._renderCompositionView(this._compositionViewData); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3dacf67-3952-40dd-ba0d-69760c260b15
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
config/patches/@xterm__xterm@6.1.0-beta.287.patchconfig/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patchsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-preedit-tail-attributes.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
The trailing-blank case wrote the same data as the default-tail case above it and asserted a subset of it, under a comment about erasing to end of row that no escape sequence backed. It could not fail for the reason it named. Replaced with a row that carries a run of spaces inside the text. That pins the two things the tail's end column and its `white-space: pre` are for, and both arms bite: reverting the trimmed end column to the full row length fails it, as does letting the nowrap view collapse the inner run.
|
I tried this and then backed it out, because the state it protects against is not reachable. An empty preedit does not leave the overlay up. this._renderCompositionView(ev.data ?? '');
// Empty data is the IME saying the preedit is gone, so it hides the view again rather than
// leaving an orphaned overlay.
this._compositionView.classList.toggle('active', Boolean(ev.data));
That makes the two guards behaviourally identical in every reachable state. I wrote the change and a test for the empty-preedit repaint; the test failed against the fix for the same reason it failed without it, which is what sent me looking at the toggle. On the suggested diff specifically: Keeping the original guard. It is also worth noting the truthiness check predates this PR — only the comparison inside it is mine — so switching it would have put a second 7 MB regenerated bundle into a PR about tail colours, for no behaviour change. If you can name a path that reaches |
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: 785ed4c8-8f4b-4a8d-8852-101e70aeff8d
📒 Files selected for processing (1)
src/renderer/src/components/terminal-pane/terminal-ime-xterm-preedit-tail-attributes.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
The spaces case read `textContent`, which is unaffected by CSS whitespace handling, so dropping `white-space: pre` from the rendered tail left the whole file green while every glyph after the run moved a cell left. Assert the style alongside the text. Mutating the loaded bundle's `whiteSpace = 'pre'` to `'normal'` now takes the case down; before this it took nothing down.
|
Thanks for the fix and the clear before/after proof. Preserving the dim styling is definitely an improvement. Would it also be possible or should it be tracked separately to hide the placeholder as soon as Korean IME composition starts, matching the behavior after the first Latin character? That would address the original UX issue I noticed most directly. |
|
Separate, I think — the placeholder belongs to Codex. A composing syllable never reaches the pty, so its buffer is still empty and it keeps drawing the hint; after a Latin character it clears the hint itself. Orca draws the copy you see, but it can't tell a placeholder that should vanish from real text that must stay shifted right. And dropping the tail while composing blanks whatever sits under the overlay — the occlusion #12545 was filed for. Worth raising upstream on Codex if you want it tracked. |
|
Superseded by #17631. #17170 moved the pinned xterm to 6.1.0-beta.303 and reworked It also came out smaller: @lee-eojin — your question carried over to the new one, and thanks for the read here. |
ELI5
When you start typing a Korean syllable into an empty agent CLI prompt, the syllable is still being composed and has not reached the shell yet — so the CLI's grey hint text is still sitting on that row. Orca draws its own copy of that row next to the composing letter so the letter looks like it is pushing the text along, but it redraws the copy in the ordinary text colour. The grey hint stops looking grey and starts looking like something you already typed. It goes back to normal the instant the syllable lands. This makes Orca's copy keep the colours the row actually had.
What Changed
CompositionHelperbuilt the rendered row tail withtranslateToString, which returns characters and drops every cell attribute. It now reads the cells and emits the tail as runs that share a rendered style, each carrying what the grid resolved for it: palette, RGB and default foregrounds, backgrounds, dim, bold, italic, inverse, invisible, underline, strikethrough, overline.A run that needs no override stays a bare text node and keeps inheriting the view's colour, so a default tail emits exactly the DOM it did before. The end column still comes from
getTrimmedLength(), so the tail covers the columns it always did.The refresh that keeps an open tail current keyed off the rendered text, which cannot see a repaint that recolours a row without changing its characters. It now keys off text and styling together.
Files: the xterm source patch, its regenerated bundle patch, the
pnpm-lock.yamlhash bump, and one new test file.Why
The tail exists so a mid-line composition reads as inserted text pushing the row right rather than an opaque box hiding the character under the cursor (#12545). The view paints one themed colour over everything it covers:
That is correct for the preedit and for default cells. Any cell carrying its own attributes came back in the default foreground — a dim placeholder here, but equally a coloured prompt or a highlighted segment.
Reading the cells is the only way to render them faithfully; there is no smaller correct version. The DOM renderer's own
xterm-fg-*andxterm-dimclasses cannot be reused, because they are scoped to the rows container and only generated while that renderer is the active one.Linked Issue
Fixes #16025
Visual Proof
Before, both on v1.4.188 with the input empty, so an open composition is the only difference. Codex is the CLI; its empty-input placeholder is
Ask Codex to do anything.At rest — the placeholder is dim, which is how it is meant to read:
Composing
ㅇ— the placeholder is drawn as ordinary text:Measured rather than eyeballed. Glyph pixels in the
to do anythingspan — no block cursor and no jamo inside it — on the prompt row only, against the same background (luminance 52):Same string in the same font, and the glyph counts land within two of each other, so this is brightness alone: about 60% brighter for as long as the composition stays open.
After, this branch built and run with the same CLI, composing the same
ㅇ:And the same branch at rest, as a control that the ordinary case did not move:
Measured the same way — same span, same prompt-row restriction so the status line below cannot leak in:
Composing used to land 70 above its own at-rest baseline; it now lands 4 above, and p90 falls from 254 to 154. The at-rest column moving 116 → 118 is the control: a default row renders as it always did.
Worth saying plainly — before and after look nearly identical at a glance, because the glyphs are in the same places and only their brightness differs. That is the point. Nothing shifts, nothing disappears; the tail just stops being repainted.
For contrast, iTerm2 with the same CLI keeps the placeholder dim but has no rendered tail at all, so its preedit box hides the cells under it —
Askis gone and the row reads, Codex to do anything. That is the occlusion #12545 was about, and it is why reverting to "do not draw a tail" is not the fix.Testing
Regenerated per
docs/reference/xterm-patch-regeneration.md: source hunks edited in the generator's checkout,--write,pnpm install, then--checkreportsin sync (7406065 bytes). Source patch, bundle patch and the lockfile hash are in one commit, as that document requires.pnpm testpnpm lintpnpm typecheckpnpm buildtsgo --noEmit -p src/browser/tsconfig.jsonin the patched upstream treeOne file failed the first full run —
tests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.ts, which needs release tags my--depth 1clone did not have. Its own failure message names that cause. Aftergit fetch --tagsit passes 5/5.terminal-ime-xterm-preedit-tail-attributes.test.tspins seven cases: a dim tail stays dim, a palette colour survives, a default tail emits no run of its own, the tail splits where styling changes, bold and italic carry through, a recolour with identical characters still refreshes, and trailing blanks are still dropped. The third and sixth are the ones that would catch a regression in this change rather than restate it — one guards against styling cells that need none, the other against the text-only comparison the old refresh used.The existing
midline-preedit-tailandtrailing-preedit-occlusionsuites are unchanged and pass: the view keeps its own colour and the overrides sit on child runs, so what they assert still holds.Manual pass: reproduced on the shipped v1.4.188, then built and ran this branch and repeated it with a live 2-Set Korean IME, on macOS. Both states above are from those two runs, and the patched bundle is what the run loaded —
setThemeServiceis present innode_modules/@xterm/xterm/lib/xterm.jsand.mjs.AI Disclosure
Claude Opus 5 via Claude Code, used throughout: locating the cause in the xterm patch, writing the change and its tests, and running the gates above. Every measurement quoted here — the luminance figures, the
--checkbyte count, the suite results — was produced by running the command, not estimated. The reproduction and all four screenshots are mine, on my own machine.Review
Code review summary, per CONTRIBUTING:
getTrimmedLength()and the cell accessors are platform-neutral.cols. The previous code already walked the same span insidetranslateToString, and the refresh check now walks once where it used to walk once and compare, so the work is comparable. TheCellDatainstance is allocated per helper and reused per cell, so a render allocates nothing per cell. No timer, listener or layout read is added.eval. Cell text goes in throughtextContent, neverinnerHTML; the only attribute written isstyle, built from a closed set of enum branches with numeric colour channels, so nothing user-controlled reaches a CSS value.Known limitation, called out deliberately: minimum contrast is not applied to a tail run. The renderer adjusts a cell against its own background; doing that here would need the same cache and still resolve against a different background. Leaving it out keeps every cell closer to the grid than one flat colour was. Glad to add it if you would rather have the exact renderer path.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
The theme is handed over by a setter rather than injected. Adding
@IThemeServiceto the constructor needs a hunk in upstream'sCompositionHelper.test.ts, which.npmignorestrips from the published tarball —docs/reference/xterm-patch-regeneration.mdsays such a hunk fails the generator rather than being silently dropped.CoreBrowserTerminalhands the service over right after it creates the helper, which is where the existing patch already reaches in, andIThemeServiceis registered well before that point.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)