Skip to content

fix(terminal): keep a composition tail in the colours its cells had - #17631

Open
ethznn wants to merge 5 commits into
stablyai:mainfrom
ethznn:fix/ime-preedit-tail-attributes-v2
Open

ethznn wants to merge 5 commits into
stablyai:mainfrom
ethznn:fix/ime-preedit-tail-attributes-v2

Conversation

@ethznn

@ethznn ethznn commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

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

CompositionHelper built the rendered row tail with translateToString, 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.yaml hash 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:

this._compositionView.style.color = this._optionsService.rawOptions.theme?.foreground ?? '#FFF';

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-* and xterm-dim classes 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

Recaptured per review. The earlier pair used Codex's Ask Codex to do anything, which the composer-placeholder mask now hides outright, so those two shots compared a build without that mask to one with it. These use a row the mask does not recognise — terminal-composer-draft.ts matches exactly Ask Codex to do anything, Ask a follow-up question and a Try " prefix — so the tail is rendered in both builds and the only difference is this change.

The row is written straight to the shell, which keeps the repro free of any CLI:

printf '\033[2mdim hint\033[0m \033[33myellow prompt\033[0m\033[22D'; read -r

read holds the prompt off the row, and the cursor ends on the first cell. A ㄱ is then composed and left uncommitted.

Before — v1.4.210. Chosen rather than an older release because it carries this branch's base: _renderCompositionRemainder from #19367 and the placeholder mask from #17377 are both in it, and only this change is missing. At rest, then composing:

v1.4.210 at rest: dim hint in grey, yellow prompt in yellow

v1.4.210 composing: the whole tail is repainted in the default foreground

After — this branch, same row, same key:

This branch at rest

This branch composing: dim stays dim and the yellow run stays yellow

Measured from those four images rather than eyeballed. Glyph pixels only (luminance > 95); the coloured run is counted as pixels with r - b > 35 and r >= g > b:

build state hint (SGR 2) yellow prompt (SGR 33)
v1.4.210 at rest rgb(136,138,143), luminance 137 rgb(202,175,115), 1,113 coloured pixels
v1.4.210 composing rgb(218,219,220), luminance 218 0 coloured pixels
this branch at rest grey rgb(201,174,114), 1,255 coloured pixels
this branch composing rgb(136,138,141), luminance 137 rgb(203,176,116), 1,057 coloured pixels

Two readings, one per attribute. Dim: the row's own at-rest luminance is 137, and on v1.4.210 a composition lifts it to 218 — SGR 2 is simply gone. On this branch it reads 137 while composing, the same number it has at rest. Colour: the yellow run contributes about eleven hundred coloured pixels at rest and none at all while composing on v1.4.210, because every cell comes back as default foreground; on this branch the count and the mean survive the composition.

The at-rest rows are the control — they are identical in both builds, so nothing in the ordinary path moved.

For contrast, iTerm2 with a mid-line composition keeps the row's colours but renders no tail at all, so its preedit box hides the cells under it. That is the occlusion #12545 was about, and it is why "stop drawing a tail" is not the fix here.

Testing

Re-cut onto main at bd5177801b (20 Sep). #19367 had landed in the meantime and reworked the same function, so the patch files conflicted in twenty places. Rather than hand-merge a patch, the merge was done in source form, the way docs/reference/xterm-patch-regeneration.md prescribes: three versions of CompositionHelper.ts materialised in the generator's checkout — the fork point, this branch, and current main — then git merge-file --diff3, seven source conflicts resolved by hand, and the patch regenerated from the result with --write, pnpm install, --check.

The resolution keeps #19367's structure. _renderCompositionRemainder stays a separate renderer, so a row repaint leaves the preedit and caret nodes in place; the styled runs moved inside it; _compositionViewData, which that PR removed, was not reintroduced; and the refresh is keyed on text and styling together, since a repaint that only recolours a row leaves its characters identical.

Command Result
regenerate-xterm-patches.mjs --check in sync with the pinned upstream build (1,362,743 bytes)
pnpm test src/renderer/src/components/terminal-pane 474 files, 4,582 passed, 1 skipped — includes #19367's own suites
terminal-ime-xterm-preedit-tail-attributes.test.ts 11 passed
pnpm typecheck clean
pnpm lint clean
pnpm test (full) 9,223 files / 86,289 tests passed; 10 files / 21 tests failed, none of them this branch's (below)
pnpm build desktop and renderer complete; build:computer-macos fails on this machine (below)

The full-run failures are this machine, not this branch. Eight of the ten files fail identically on pristine bd5177801b: skill-recipe-shell (system bash is 3.2.57, which errors on an empty array under set -u), workflow-ref-reachability (git init --ref-format=reftable needs git 2.45; this machine has 2.40.1), the two real-Claude-CLI proofs, browser-manager-viewport-ownership, repro-13767-shell-ready-marker-lost-to-exec, e2e-worker-env-isolation and relay-region-correction. The remaining two — browser-cookie-import-partition.electron and cross-version-wire/orchestration-delivery-downgrade — pass in isolation on this branch, so they are full-suite context rather than content. This branch's diff is the two patch files, the lockfile and one renderer test; none of the ten is reachable from it.

pnpm build stops in build:native at the macOS universal step, lipo: same architectures (arm64) found in ... x86_64-apple-macosx/release/orca-computer-use-macos. Reproduced on pristine main with the same command, so it is the toolchain here rather than the change; no native code is touched by this PR.

terminal-ime-xterm-preedit-tail-attributes.test.ts pins eleven 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, the underline carries its own line style and colour rather than a plain underline, an underlined blank gets something to draw the line under, an inner run of spaces survives with the tail's white-space asserted rather than only its text, a wide CJK tail walks by cell pairs, and an inverse tail takes its pair from the theme.

The last two are the coverage asked for in review, and both discriminate: a mutant that advances one cell at a time renders 한 글 tail and fails the wide case, and a mutant that ignores isInverse() fails the inverse one. Of the original nine, the third and sixth are the ones that would catch a regression rather than restate it — one guards against styling cells that need none, the other against the text-only comparison the old refresh used.

The new file finds the tail by .xterm-composition-remainder rather than by child index, so neither the caret span #17170 added nor #19367's spacing spans move it.

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

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.

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 --check byte 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:

  • Cross-platform. No platform branch is touched. The tail path runs wherever a composition opens, so Windows and Linux IMEs get the same fix. getTrimmedLength() and the cell accessors are platform-neutral.
  • Remote / SSH. Rendering only; nothing crosses the wire and no PTY bytes change. A remote session renders from the same local buffer.
  • Agents and integrations. Any TUI that styles the row benefits; the CLI in the repro is Codex, and Claude Code and Cursor Agent place placeholders the same way. Nothing keys off which agent is running.
  • Performance. One extra pass over the row tail per composition render, bounded by cols. The previous code already walked the same span inside translateToString, and the refresh check now walks once where it used to walk once and compare, so the work is comparable. The CellData instance is allocated per helper and reused per cell, so a render allocates nothing per cell. No timer, listener or layout read is added.
  • UI quality. Default cells emit byte-identical DOM, so the common tail cannot regress. Dim is applied as half opacity on the resolved foreground, matching how the renderer fades a dim cell.
  • Security. No new input sink, no network, no filesystem, no eval. Cell text goes in through textContent, never innerHTML; the only attribute written is style, built from a closed set of enum branches with numeric colour channels, so nothing user-controlled reaches a CSS value.
  • Backwards compatibility. CompositionHelper already takes @IThemeService on this version, and the resolver returns '' when no theme is present, so a partial construction path renders the tail the way it renders today rather than throwing.

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

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

This replaces #16024, which was cut against xterm 6.1.0-beta.287 and cannot be rebased: #17170 moved the pin to beta.303 and reworked the same function, so the patch file that PR edits no longer exists. Its review is still worth reading — a CodeRabbit pass, and a question from @lee-eojin about hiding the placeholder outright rather than recolouring it. The answer to the latter is that the terminal cannot tell a placeholder that should vanish from real text that must stay shifted right, and dropping the tail while composing reintroduces the occlusion #12545 was filed for.

Re-cut against beta.303 the change got smaller. CompositionHelper already takes @IThemeService there, so the colours resolve through the service the view itself already uses and the earlier version's setter workaround is gone.

Overlap with other open work

#19367 merged on 19 Sep and this is re-cut onto it, so the two no longer collide. Two neighbours are still open on the same overlay:

This PR stays narrower than both: the committed tail's own cell attributes, nothing else. It leaves the view background as _opaqueViewBackground(), does not decide whether a placeholder row is masked, and does not touch preedit geometry. #19742 reports that view background painting over a TUI input box's own colour — a different defect in the same overlay, and not this one.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@greptile-apps

greptile-apps Bot commented Aug 31, 2026 •

Copy link
Copy Markdown

Greptile Summary

The PR updates the patched xterm composition helper so a rendered composition tail retains the terminal cells’ resolved styling and refreshes after style-only repaints.

  • Serializes foreground, background, emphasis, visibility, decoration, and inverse attributes into styled tail runs.
  • Preserves unstyled tail runs as bare text nodes.
  • Adds coverage for styled runs, style-only refreshes, whitespace, and underline variants and colors.
  • Regenerates the packaged xterm patch and updates its pnpm patch hash.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch Adds attribute-aware composition-tail serialization, including the underline variants and colors omitted by the previously reported implementation.
config/patches/@xterm__xterm@6.1.0-beta.303.patch Regenerates the packaged xterm patch with the source-patch behavior.
src/renderer/src/components/terminal-pane/terminal-ime-xterm-preedit-tail-attributes.test.ts Exercises default and styled tail rendering, style-only refreshes, whitespace preservation, and underline fidelity.
pnpm-lock.yaml Updates references to the regenerated xterm patch hash.

Reviews (2): Last reviewed commit: "fix(terminal): carry the underline's sty..." | Re-trigger Greptile

@coderabbitai

coderabbitai Bot commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 75237299-676a-467a-bff4-12883939a353

📥 Commits

Reviewing files that changed from the base of the PR and between a41fa88259212bccda8d9654545a46e9617b867f and 195949947516b93f2f2bf3f392fd820e1fff8f46.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • config/patches/@xterm__xterm@6.1.0-beta.303.patch
  • config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch
🚧 Files skipped from review as they are similar to previous changes (1)

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


📝 Walkthrough

Walkthrough

The patch expands CompositionHelper with transaction-based finalization, IME key and input reconciliation, cancellation, blur, disposal handling, lifecycle events, and inferred sessions. It updates composition rendering with a styled preedit caret and cell-styled row remainder. It adds theme-aware colors, repaint synchronization, whitespace preservation, grapheme-width correction, and textarea anchoring. New tests verify styled, default, refreshed, underline, and whitespace-containing preedit tails.

Priority: ➖ Normal

Severity of issue fixed: Medium

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The patch changes SortedList to track object identities and deleted indices after sort-key mutation. The summary provides no connection between this behavior and composition-tail rendering or issue … Remove or split the unrelated SortedList changes. Separate the broader IME lifecycle and reconciliation changes unless the PR provides concrete evidence that each change is required for issue #16025.
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #16025 requires composition-tail rendering to preserve cell styling without changing the terminal buffer or submitted input. The PR reads cell attributes and emits styled runs for colors, dim, b…
Title check ✅ Passed The title clearly and concisely describes the primary change: preserving composition-tail cell colors during terminal input composition.
Description check ✅ Passed The description is complete and matches the repository template. It explains the user impact, implementation, rationale, linked issue, visual proof, testing results and limitations, AI use, compatibil…
Full details: Out of Scope Changes check

Explanation

The patch changes SortedList to track object identities and deleted indices after sort-key mutation. The summary provides no connection between this behavior and composition-tail rendering or issue #16025. The patch also includes broad IME lifecycle, reconciliation, and forwarding changes beyond style preservation. The available evidence does not establish that these changes are required for the linked issue.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)


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.

@pullfrog pullfrog 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.

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes

  • Styled tail runs — CompositionHelper._getRowRemainder() now walks the row's cells and groups the remainder into runs that share a resolved style (fg/bg mode, RGB and palette resolution, dim, bold, italic, inverse, invisible, decorations). Default-styled runs stay bare text nodes so the common tail emits byte-identical DOM.
  • Styling-aware refresh key — the repaint path in updateCompositionElements keys off text and styling together (_compositionRemainderKey) rather than text alone, so a repaint that only recolours the row re-renders the tail.
  • Regenerated patch + lockfile — the xterm bundle patch and the pnpm-lock.yaml hash were regenerated per the xterm-patch workflow.
  • Tests — a new happy-dom suite pins dim, palette, default-no-run, run splitting, bold/italic, recolour-without-text-change, and whitespace preservation.

I checked _tailCellStyle against upstream DomRendererRowFactory at the pinned commit: inverse swap, bold-brighten, RGB/palette resolution, dim-as-half-opacity-foreground, and invisible cells all match the renderer's behavior, and the end column (getTrimmedLength) reproduces what translateToString(true, …) resolved to. The recolour test is a real regression guard — it fails against the old text-only comparison.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

The new commit carries the underline decoration through faithfully instead of collapsing it to a plain underline.

  • Underline style variants — _tailCellStyle now maps cell.extended.underlineStyle (single/double/curly/dotted/dashed) to text-decoration-style and moves the decoration list to text-decoration-line so the two compose.
  • Underline colour — SGR 58 colours are emitted as text-decoration-color in both RGB and palette form, including bold-brighten for palette colours < 8.
  • Underlined/overlined blanks — a ' ' under an underline/overline is swapped for \xa0, matching DomRendererRowFactory.
  • Tests — two new cases pin the wavy style + independent colour and the non-breaking-space swap, and the missing semicolon was fixed.

I checked the new _tailCellStyle decoration block against upstream DomRendererRowFactory at the pinned commit (6.1.0-beta.303): the \xa0 swap condition, the xterm-underline-1..5 → solid/double/wavy/dotted/dashed mapping, the RGB channel extraction, the palette bold-brighten, and the default-colour short-circuit all match the renderer exactly. Both prior review threads are addressed. The test file passes (9 tests).

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 48d7a2b0-e6f6-40b9-b075-2a19b88ee8fa

📥 Commits

Reviewing files that changed from the base of the PR and between 7db8a41e86596f9985a9abf5957ca816aa28b28f and a41fa88259212bccda8d9654545a46e9617b867f.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • config/patches/@xterm__xterm@6.1.0-beta.303.patch
  • config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch

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

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review (d048782), the branch was rebased onto current main, which already carries the IME preedit-renderer rework, and the tail-styling logic was re-fit to the new helper boundary. No behavior change from the approved revision.

  • Re-fit the runs logic to current main — main now owns _renderCompositionView(data) / _renderCompositionRemainder(...) plus CompositionPreeditRenderer; the branch moved its runs rendering from the old _renderCompositionView(data, rowRemainder) / _compositionViewData shape into _renderCompositionRemainder(rowRemainder) and added the _compositionRemainderKey reset in _resetCompositionView.
  • Tail styling unchanged — _getRowRemainder() and _tailCellStyle() are byte-identical to the previously approved revision; the styling-aware refresh key is preserved, and the empty-runs path still removes the remainder.
  • Regenerated patch + lockfile — bundle patch and pnpm-lock.yaml hash regenerated (dd0ccc59… → db5518de…) for the new source patch; the test file is unchanged and still 9 cases.

I verified CompositionPreeditRenderer, _preeditRenderer, _styleCompositionCaret, and the text-only _renderCompositionRemainder(data) all exist in base main, so they are base context rather than this PR's contribution. I checked the re-fitted _renderCompositionRemainder/_getRowRemainder paths: _renderCompositionView clears and recreates the remainder, the repaint path only replaces its children while keeping the preedit and caret nodes, and the Math.min(line.length, getTrimmedLength()) end column reproduces the old translateToString(true, …) coverage. Both prior threads are resolved.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review (a41fa882), one commit landed.

  • Underline style read through the guarded accessor — _tailCellStyle now reads cell.getUnderlineStyle() instead of cell.extended.underlineStyle, with an expanded comment explaining the representation-independent read. Behavior-neutral on parser-produced input: loadCell hands a reused CellData a zeroed extended blueprint when HAS_EXTENDED is clear, and _processUnderline attaches extended attributes even to a plain SGR 4, so the two forms agree here. The accessor additionally reports SINGLE for an underline-flag cell lacking extended attrs, which the mapping already collapsed to a plain solid underline.
  • Regenerated patch artifacts — the source hunk, the rebuilt bundle patch, and the pnpm-lock.yaml hash bump (dd0ccc59… → e28ed5f9…) land in the same commit, per the xterm patch workflow.

The new commit only changes a read that both representations resolve identically for today's inputs; no test ships because a reverted mutant survives any assertion. I confirmed the change is confined to the source patch's tail path, and the getUnderlineStyle() accessor exists on the pinned ICellData/IAttributeData. Prior feedback is addressed; no open threads.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@nwparker nwparker 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.

Thanks for this — the re-cut onto the current pin was the right call, and the write-up made it cheap to check. The code I have no concerns about; both remaining items are evidence, not correctness.

What I verified, on an isolated copy of your branch:

  • regenerate-xterm-patches.mjs --check reports in sync (1362743 bytes), matching your figure exactly.
  • pnpm install --frozen-lockfile succeeds, so the lockfile hash and the regenerated patch agree. Only @xterm/xterm's hash moved; the four addon hashes are untouched, which is right.
  • main has not moved under config/patches/ since your fork point, so despite the branch being well behind, there is nothing to rebase — merge is clean.
  • Renderer suite 4581 passed / 0 failed; the new tail-attributes file 9/9; typecheck clean.

On the per-keystroke cost, since this runs on every composition update: the loop is bounded by cols (start → min(line.length, getTrimmedLength())), never the scrollback; it loads into a reused CellData rather than allocating per cell; adjacent same-style cells merge into one run, so a dim placeholder row stays a single span; and the length-prefixed key means a repaint that changes nothing rebuilds nothing. No timer, listener or layout read added. That satisfies our bounded-work rule.

Two things before I merge:

  1. Your CI has never run. This is a fork PR and both workflow runs were sitting in action_required, so only the AI reviewer executed and the xterm patch sync gate never fired. I have approved them — my local --check is a proxy, not the gate.

  2. The before/after screenshots are confounded. The composer-placeholder mask that hides the whole tail landed in a commit that is not in v1.4.188 but is in your base. So the two shots compare a build without that mask to one with it, and the literal repro in your issue is already suppressed on main today. The fix is still right — that mask is deliberately narrow, and anything it does not recognise is still repainted in the default foreground. Could you recapture against a styled row that is not a recognised placeholder: a coloured prompt, a highlighted segment, or arbitrary dim output?

One coverage note, cheap insurance rather than a blocker: there is no wide-CJK cell and no inverse-video case among the nine. I checked both by reading the source and they look correct, but a wide-character tail is the kind of thing that breaks quietly.

On the neighbours, for your information: #16230 targets a pin that no longer exists in the repo, so it cannot merge in any form and needs a re-cut the way you did here. #19592 is substantially superseded by what has since landed on main. Yours is the only one of the three that is mergeable and in sync, so it goes first — and I would rather keep it narrow than consolidate them.

Re-running CI now; I will merge on green once the screenshot is updated.

@ethznn

ethznn commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Both done — screenshots recaptured and the two missing cases added.

Screenshots. You were right that the old pair was confounded; the mask hides that row outright now. The new pair uses a row the mask cannot recognise, written straight to the shell so no CLI is involved:

printf '\033[2mdim hint\033[0m \033[33myellow prompt\033[0m\033[22D'; read -r

Before is v1.4.210 rather than an older release, because it carries this branch's base — _renderCompositionRemainder from #19367 and the mask from #17377 are both in it, and only this change is missing. I checked that against the tag rather than assuming: translateToString is still the tail's source there and neither _tailCellStyle nor the run type exists.

Measured from the four images, glyph pixels only:

build state hint (SGR 2) yellow prompt (SGR 33)
v1.4.210 at rest luminance 137 1,113 coloured pixels
v1.4.210 composing luminance 218 0
this branch composing luminance 137 1,057

The two composing frames, since they are the contrast that matters — v1.4.210 first, then this branch, same row and same key:

v1.4.210 composing: the whole tail is repainted in the default foreground

This branch composing: dim stays dim and the yellow run stays yellow

Two attributes, two readings: dim is simply gone on 1.4.210 while composing, and the yellow run contributes no coloured pixel at all because every cell comes back as default foreground. On this branch the composing row reads the same 137 it has at rest, and the colour survives. Body updated with all four images and the at-rest controls.

Coverage. Added a wide-CJK case and an inverse case in 8a1edd08f8, and checked that both discriminate rather than restate:

  • advancing one cell at a time instead of by getWidth() renders 한 글 tail, which fails the wide case — the zero-width continuation read as another character, which is the quiet break you had in mind;
  • ignoring isInverse() fails the inverse case, since an inverse cell on default colours carries neither colour and the pair has to come from the theme.

terminal-pane is 474 files / 4,582 passed with those two in.

On the neighbours — agreed, and thanks for checking them. Happy to keep this narrow.

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review (19594994), one commit landed, and it changes only the test file. It answers the human review's coverage note about a wide-CJK cell and an inverse-video cell.

  • Wide CJK tail — 한글 tail under SGR 2 pins that the walk advances by cell.getWidth(), so a syllable's zero-width continuation is skipped rather than read back as a padded space. It fails against a one-cell-advance mutant, which renders 한 글 tail.
  • Inverse tail — SGR 7 on default colours pins that the pair comes from the theme (background as color, foreground as backgroundColor) rather than from the cell's own fields, which are both default here. It fails against an isInverse()-ignoring mutant, which emits no run.
  • Test helpers — two constants for either CSS colour form.

Both new cases pass locally alongside the existing nine (11/11 in terminal-ime-xterm-preedit-tail-attributes.test.ts). The new commit does not touch the source patch, the bundle patch, or the lockfile, so nothing in the shipped behaviour moved. Both prior pullfrog threads are resolved and neither is re-raised.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

ethznn and others added 5 commits September 24, 2026 01:03
CompositionHelper built the rendered row tail from translateToString, which
returns characters and drops every cell attribute, so a dim placeholder or a
coloured prompt came back in the default foreground and read as entered text.

The tail is now emitted 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 style and colour, strikethrough
and overline. A run that needs no override stays a bare text node, so a default
tail emits exactly the DOM it did before.

Re-cut onto stablyai#19367, which split the remainder into its own renderer so a row
repaint keeps the preedit and caret nodes. The runs go inside that renderer and
the refresh is keyed on text and styling together, since a repaint that only
recolours a row leaves its characters identical.

Fixes stablyai#16025
…cessor

`getUnderlineStyle()` reports SINGLE for a cell that carries the underline flag
without extended attributes, which the raw `extended.underlineStyle` field calls
NONE. Everything the parser writes reaches the tail with extended attributes
attached — `_processUnderline` clones them even for a plain SGR 4 — so the two
readings agree on today's inputs and no test distinguishes them. The accessor is
the representation-independent form, which is the one this should have used.
Two cases the nine did not reach. The wide one walks by `getWidth()`, so a
regression that advanced a cell at a time would read each syllable's zero-width
continuation as another character; the mutant that does so renders `한 글  tail`
and the test fails on it. The inverse one takes its pair from the theme rather
than from the cell, since an inverse cell on default colours carries neither;
the mutant that ignores `isInverse()` fails it too.
… its own

Two cases the existing wide and inverse tests do not reach. The wide one ends a
dim run on the far cell of a syllable that also ends the row, and counts the
tail's child nodes rather than its text: a continuation cell emitted as a run of
its own adds an empty node `textContent` cannot see. The inverse one puts SGR 7
over an explicit truecolour pair, where reading the cell's fields straight
through renders the text unswapped and so identically to the same cell without
SGR 7 — the theme-coloured case cannot tell those apart, because an inverse cell
on default colours carries no pair to mis-read.

Drops the `as HTMLElement[]` casts the file used to read the tail's runs; a
narrowing filter does the same job, and the changed-lines casting gate rejects
them. The two casts left — xterm's private render service and the canvas context
WidthCache needs — carry a line-specific SAFETY rationale.
…does not claim

The screenshots this change shipped with compared a build without Orca's
composer-placeholder mask to one with it, so they showed the mask's effect and
not the tail's painting. The mask is deliberately narrow — it hides the tail only
for a row whose text is one of three exact stock placeholder strings behind a
bold prompt glyph — and everything it does not recognise was still repainted in
the overlay's own colour. That is the case with no rendered evidence.

Adds one: a coloured shell-style prompt beside arbitrary dim output, composed
over through CDP, asserted on the colours the browser resolves rather than on a
picture of them. The prompt segments use truecolour SGR so the expected values
are exact numbers instead of whatever palette the runner's theme carries, and
the spec asserts the tail is `visible` first, so a row that ever did become a
recognised placeholder fails loudly instead of passing vacuously.

Routed onto the IME e2e lane, which `config/patches/` changes already select.

Co-authored-by: ethznn <ethznn@users.noreply.github.com>
@nwparker
nwparker force-pushed the fix/ime-preedit-tail-attributes-v2 branch from 8a1edd0 to a60beee Compare September 24, 2026 08:21
@nwparker

Copy link
Copy Markdown
Contributor

I picked this up to finish it — and I owe you a correction first. I briefed my review on a stale read and described you as inactive. You had pushed 8a1edd0 with a wide-CJK case and an inverse case hours earlier. Sorry; that was my error, and those tests are kept.

Your commits are preserved with your authorship, rebased onto current main. Two commits added on top.

Two test cases yours could not catch.

Your wide-CJK test asserts the text, which passes even when a wide character's continuation cell is emitted as its own empty run — textContent cannot see an empty node. The new case asserts tail.childNodes is exactly 2, and pins the boundary between the last ASCII cell and the first wide one. Reverting x += cell.getWidth() || 1 to x += 1 now fails both wide tests: expected 'ab한 글 ' to be 'ab한글'.

Your inverse test uses default colours, where the cell carries no pair to mis-read, so a straight-through read passes it. The new case puts SGR 7 over explicit truecolour foreground and background. Stubbing isInverse() to false fails it on exactly the discriminator: expected [ '#c86432', … ] to include '#0a141e'.

Rendered evidence replacing the screenshots. Yours compared a v1.4.188 build against your base, and the composer-placeholder mask that hides the whole tail landed between those two points — so the two shots differed by the mask, not by your fix, and the literal repro is already suppressed on main. The new spec composes over a truecolour shell-style prompt, which the mask does not claim, and asserts getComputedStyle values rather than pixels. It checks the row is actually visible before the colour assertions, so that same confound fails loudly instead of passing vacuously. Registered on the route your config/patches/ changes already select, so it gates this PR rather than sitting unrun.

One thing I changed that was not a defect: the file's ten as HTMLElement[] casts. The changed-lines casting gate rejects them, and it failed with 14 findings before I replaced them with a narrowing filter. The two unavoidable ones now carry SAFETY: rationales.

terminal-ime-xterm-preedit-tail-attributes.test.ts — 13 passed
src/renderer/src/components/terminal-pane — 479 files, 4606 tests passed
regenerate-xterm-patches.mjs --check — in sync, 1362743 bytes
pnpm tc — clean; changed-code quality gate — 0 findings
e2e spec — 1 passed; 1 failed with the fix reverted

Your CI had also never run — the workflow runs sat unapproved since you opened this, so only the automated reviewer ever executed. Approved now.

Thank you for the re-cut onto the current pin and for coming back to add those cases. The bounded-work shape here is right: one row, a reused cell, adjacent same-style cells merged into one run.

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review (8a1edd08), two test-only commits landed; no production code moved. Both answer the human review's requests and the shippable behavior is unchanged.

  • Rendered-colour e2e arm — tests/e2e/terminal-ime-preedit-tail-paint.spec.ts (plus its probe) drives a composition over a coloured, dim shell-style row written straight to the emulator and asserts the resolved getComputedStyle colours of each tail run. The row deliberately avoids the three recognized composer placeholders, and the spec first asserts the tail is visible, so a masked row fails loudly instead of passing vacuously.
  • Wider unit coverage — terminal-ime-xterm-preedit-tail-attributes.test.ts gains a wide-cell run-boundary case (asserts two child nodes so a per-cell walk's empty continuation run is detectable) and an inverse case over an explicit truecolour pair (asserts the swapped values exactly, which the theme-coloured case cannot). Both fail against the mutants the commit describes.
  • Cast cleanup — the suite's as HTMLElement[] casts are replaced by a narrowing runsOf() helper, with SAFETY rationale on the two remaining casts.
  • Docs and lane routing — a "Preedit tail attributes (#17631)" section in the IME regression checklist, and the new spec routed onto the existing IME e2e lane that config/patches/ already selects.

I read the full diff end-to-end (including the patch body and the e2e probe/spec) and confirmed the delta is confined to tests, docs, and the e2e routing list. The unit suite passes locally (13/13), oxlint is clean on the changed files, and the new e2e files typecheck cleanly (they are absent from typecheck:e2e's pre-existing errors). The rendered arm is a genuine regression guard: on the pre-fix text-only tail the remainder is a single unstyled node, so styled count is 0 and the colour assertions do not hold. Prior feedback is addressed and no Pullfrog threads are open.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

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.

[Bug]: A composing syllable redraws the row tail in the default foreground, so a dim placeholder reads as entered text

2 participants