Repository navigation
fix(terminal): render the IME caret and give the candidate anchor one owner - #17170
Conversation
Greptile SummaryThe PR consolidates IME preedit rendering, caret geometry, and candidate-anchor positioning in the terminal composition surface.
Confidence Score: 4/5The PR does not yet appear safe to merge because covered dim terminal text is still rendered at ordinary foreground intensity during active IME composition. The composition remainder is serialized to plain text and painted with one foreground color, so the previously reported loss of SGR-2 styling remains in the current xterm patch. Files Needing Attention: config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch and src/renderer/src/components/terminal-pane/terminal-ime-xterm-midline-preedit-tail.test.ts
|
| Filename | Overview |
|---|---|
| config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch | Adds the IME composition surface and geometry behavior, but the existing loss of dim styling in rendered row remainders remains outstanding. |
| src/renderer/src/lib/pane-manager/terminal-ime-candidate-anchor.ts | Consolidates deferred candidate-anchor clamping with lifecycle cleanup and no additional eligible issue identified. |
| src/renderer/src/components/terminal-pane/terminal-ime-xterm-midline-preedit-tail.test.ts | Covers composition-tail visibility and geometry extensively, though it does not establish preservation of per-cell dim styling. |
| config/reliability-gates.jsonc | Registers the new deterministic and rendered IME verification paths and documents native candidate-window coverage gaps. |
| pnpm-lock.yaml | Updates the patched xterm hash consistently across dependent package snapshots. |
Reviews (13): Last reviewed commit: "fix(terminal): keep both writers of the ..." | Re-trigger Greptile
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe patch reworks xterm.js IME handling around transaction state and deferred reconciliation. Merge Risk: 🟡 Moderate · up to The PR improves IME caret rendering, text preservation, placeholder masking, and candidate-anchor alignment, but it is not yet merge-ready because unresolved edge cases could prevent the terminal from building or leave committed text or placeholder masking in an incorrect state after interrupted or resumed composition. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The changes satisfy Full details: Out of Scope Changes checkExplanation Most changes support the linked IME objectives, tests, or reliability coverage. However, the Full details: Description checkExplanation The description provides detailed scope, rationale, linked issues, testing evidence, limitations, and AI disclosure. However, it does not include the required Visual Proof section with before-and-after UI evidence, and it omits several template sections such as Review and Checklist.
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch (2)
335-335: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRestore lifecycle state for update-only composition resumes.
When a later
compositionupdatearrives after the previouspendingCompositionfinalizer completes,_isComposingand_isAwaitingCompositionEndare bothfalse. This handler only togglesactive. The nextcompositionendreturnsfalseimmediately, so the resumed preedit is not accepted or finalized as a composition transaction. Restore the composition lifecycle state before accepting an update-only resume.
1159-1160: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDeclare the search index inside
_deleteAtKey.
_deleteAtKeyusesiwithout a lexical declaration. TypeScript reportsCannot find name 'i'. Addlet i = this._search(key)inside_deleteAtKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 70ad02c1-aca8-4109-9167-42e5d548ab59
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
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-midline-preedit-tail.test.tssrc/renderer/src/components/terminal-pane/terminal-ime-xterm-trailing-preedit-occlusion.test.tstests/e2e/terminal-ime-midline-occlusion-probe.tstests/e2e/terminal-korean-endofrow-preedit-cell-span.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
…-ime-overlay # Conflicts: # pnpm-lock.yaml
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)
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch (1)
576-576: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmit the pending session-end event before deferred reconciliation.
_endPendingCompositionSessionis never invoked. Each deferred transaction therefore emitsxterm-composition-session-endonly when_sendPendingCompositionsettles it. Listeners never receivedataPendingReconciliation: true.Call
_endPendingCompositionSession(pending)after storingpendingand before scheduling its finalizer. Preserve the intended cancellation behavior when the final input dispatch occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d117d634-3969-4d0b-ae5e-e48acc358ba5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
config/patches/@xterm__xterm@6.1.0-beta.287.patchconfig/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patchconfig/reliability-gates.jsoncsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-midline-preedit-tail.test.tssrc/renderer/src/components/terminal-pane/terminal-ime-xterm-resumed-preedit-visibility.test.tstests/e2e/terminal-ime-midline-occlusion-probe.tstests/e2e/terminal-ime-pane-arena.tstests/e2e/terminal-korean-endofrow-preedit-cell-span.spec.tstests/e2e/terminal-korean-midline-preedit-occlusion.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/terminal-korean-midline-preedit-occlusion.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
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)
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patch (1)
1124-1124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeclare
iin_deleteAtKey.The extracted method uses
iwithout a local binding. TypeScript will report an unresolved identifier. Useconst i = this._search(key).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5afe97d4-982e-484d-bae0-e2302f005e51
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
config/patches/@xterm__xterm@6.1.0-beta.287.patchconfig/patches/xterm-src/@xterm__xterm@6.1.0-beta.287.src.patchconfig/reliability-gates.jsoncsrc/main/daemon/headless-emulator.tssrc/main/daemon/terminal-cursor-line-context.test.tssrc/renderer/src/assets/terminal.csssrc/renderer/src/components/terminal-pane/terminal-ime-composer-placeholder-mask.test.tssrc/renderer/src/components/terminal-pane/terminal-ime-composer-placeholder-mask.tssrc/renderer/src/components/terminal-pane/terminal-ime-xterm-midline-preedit-tail.test.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.tssrc/shared/terminal-composer-draft.test.tssrc/shared/terminal-composer-draft.tssrc/shared/terminal-cursor-line-context.tstests/e2e/terminal-korean-midline-preedit-occlusion.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
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: 29b52aff-cdf7-4bc1-90e4-abbdae576142
📒 Files selected for processing (2)
src/renderer/src/components/terminal-pane/terminal-ime-composer-placeholder-mask.test.tssrc/renderer/src/components/terminal-pane/terminal-ime-composer-placeholder-mask.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
|
Fresh CI on head |
Holding this — one blocking production bug, plus a scope splitBlocking: the candidate-anchor slice has two owners and loses the race
This PR adds a second writer inside the patch. Measured with the patched terminal and the real anchor module in the same rig, cursor at the final column, cols=80, 8px cells, 2-cell preedit: xterm's handler is on the textarea; the anchor module's is on Both tests miss it. The shipped behaviour is not what the tests assert. And per AGENTS.md's reuse rule, this belongs in the module that already owns the property — as a renderer-only change, with no patch hunk at all. On the patch itself — the objection I came in with does not applyFor the record, since it shaped my initial read: What survives scrutiny is narrower but genuine: painting the caret inline after the preedit span, so it participates in the same flex row as the end-alignment, is legitimately hard from outside —
Scope — smaller than it looks, but still three changesReviewable non-test surface is 389 added lines, not 1250: ~700 additions are tests, 66 are gate prose, and the rest is the generated bundle and lockfile hash. Worth stating, because the raw number overstates the review burden. Still separable:
Two surviving mutations worth fixing
Verified cleanPatch pipeline ( One correction: the body cites the bundle as 7,392,151 bytes; it is 7,387,365. A stale number presented as verification evidence. Must-fix before merge
Residual, not blocking: the synthesized- |
Two of the four ledger rows describe contracts whose implementation is still open in #17170, and the placeholder-mask bullet with them. A reference doc that reads as current behaviour when it is really acceptance criteria is worse than no doc, so they are marked rather than removed - they are still the criteria that change has to meet. Also records the gap this checklist exists to compensate for: no PR-gate job runs a real input method. The unit gate is happy-dom, the e2e gate drives synthetic CDP compositions, the one workflow with ibus-hangul is nightly and gates nothing, and there is no macOS runner at all. Without that stated, the "native evidence" table reads like something CI enforces.
| + if (rowRemainder) { | ||
| + remainder = doc.createElement('span'); | ||
| + remainder.className = 'xterm-composition-remainder'; | ||
| + // Why: the view is nowrap, which collapses runs of spaces, so committed padding would draw | ||
| + // its trailing glyph cells to the left of where the grid has them. | ||
| + remainder.style.whiteSpace = 'pre'; | ||
| + remainder.textContent = rowRemainder; |
There was a problem hiding this comment.
When IME composition covers legitimate SGR-2-dim terminal content, _getRowRemainderText reduces those cells to plain text and the opaque overlay paints the remainder with the ordinary theme foreground, causing dim prompts, status text, and TUI content to become full-intensity until composition ends.
Knowledge Base Used: Restore Terminal IME Composition Handling
435ae80 to
dd8229a
Compare
…ment `textarea.style.left` has two writers: xterm's patched CompositionHelper and Orca's terminal-ime-candidate-anchor.ts. The anchor module listens on terminal.element, so within a composition event it writes after xterm's textarea listener and reverted the final-column clamp the patch had just applied. Moving the clamp into the anchor module and dropping the patch hunk does not fix it, and the rendered e2e caught that: CoreBrowserTerminal.ts:444 drives updateCompositionElements from onRender as well, so xterm re-asserts the textarea position on every repaint, with no composition event for that module to hear. The anchor survived only when no render happened to follow — measured as a flake at the final column, 1561.28px against a 1557px screen edge, the fully unclamped value. So both writers now compute the same clamp. The patch keeps it, because it is the writer on the render path and already holds cursorLeft, maxWidth and the preedit bounds. The anchor module applies the same one, so its composition-event write no longer reverts the correction in the window before the next render. Both halves are individually necessary and both are mutation-tested. Also restores _getRowRemainderText's expression from main: translateToString(true, x, line.length) and translateToString(false, x, getTrimmedLength()) are the same call, since upstream does endCol = min(endCol, getTrimmedLength()) under trimRight. Adds the two missing tests — one installing both anchor writers in a single rig, one driving a render under an open composition — plus disposal cleanup and clamp-bound coverage, and moves the Codex/Claude placeholder mask to a follow-up PR.
dd8229a to
0bb0b58
Compare
…edit Split out of stablyai#17170, which now carries only the xterm composition-overlay work. Codex and Claude draw an all-dim, full-row ghost placeholder. The opaque preedit overlay reproduces the committed row tail it covers, so without this the ghost is repeated to the right of the composing syllable instead of staying masked. The binding keys off the `.xterm-composition-remainder` class that stablyai#17170 adds and hides it through CSS while a composition owns a structurally verified placeholder row — bold prompt glyph plus a dimmed model footer below a blank gap for Codex, a frame line above the prompt for Claude. Arbitrary dim output, shell lookalikes, and any row carrying typed text keep their tail visible. readTerminalCursorLineContext moves from src/main/daemon to src/shared because the renderer now needs the same reader the daemon uses; the move is import-only. Depends on stablyai#17170.
…edit (#17377) Split out of #17170, which now carries only the xterm composition-overlay work. Codex and Claude draw an all-dim, full-row ghost placeholder. The opaque preedit overlay reproduces the committed row tail it covers, so without this the ghost is repeated to the right of the composing syllable instead of staying masked. The binding keys off the `.xterm-composition-remainder` class that #17170 adds and hides it through CSS while a composition owns a structurally verified placeholder row — bold prompt glyph plus a dimmed model footer below a blank gap for Codex, a frame line above the prompt for Claude. Arbitrary dim output, shell lookalikes, and any row carrying typed text keep their tail visible. readTerminalCursorLineContext moves from src/main/daemon to src/shared because the renderer now needs the same reader the daemon uses; the move is import-only. Depends on #17170.
… owner (stablyai#17170) * fix(terminal): render IME caret without placeholder overlap * fix(terminal): preserve dim mid-line composition tails * fix(terminal): keep IME caret visible at row edge * fix(terminal): harden IME overlay lifecycle and layout * test(terminal): type final-cell layout mock * fix(terminal): keep final-cell IME anchor on-screen * fix(terminal): bind IME masking to composer ownership * fix(terminal): bound IME placeholder session ownership * fix(terminal): track latest IME placeholder session * test(terminal): share IME session event fixture * fix(terminal): keep both writers of the IME candidate anchor in agreement `textarea.style.left` has two writers: xterm's patched CompositionHelper and Orca's terminal-ime-candidate-anchor.ts. The anchor module listens on terminal.element, so within a composition event it writes after xterm's textarea listener and reverted the final-column clamp the patch had just applied. Moving the clamp into the anchor module and dropping the patch hunk does not fix it, and the rendered e2e caught that: CoreBrowserTerminal.ts:444 drives updateCompositionElements from onRender as well, so xterm re-asserts the textarea position on every repaint, with no composition event for that module to hear. The anchor survived only when no render happened to follow — measured as a flake at the final column, 1561.28px against a 1557px screen edge, the fully unclamped value. So both writers now compute the same clamp. The patch keeps it, because it is the writer on the render path and already holds cursorLeft, maxWidth and the preedit bounds. The anchor module applies the same one, so its composition-event write no longer reverts the correction in the window before the next render. Both halves are individually necessary and both are mutation-tested. Also restores _getRowRemainderText's expression from main: translateToString(true, x, line.length) and translateToString(false, x, getTrimmedLength()) are the same call, since upstream does endCol = min(endCol, getTrimmedLength()) under trimRight. Adds the two missing tests — one installing both anchor writers in a single rig, one driving a render under an open composition — plus disposal cleanup and clamp-bound coverage, and moves the Codex/Claude placeholder mask to a follow-up PR.
…edit (stablyai#17377) Split out of stablyai#17170, which now carries only the xterm composition-overlay work. Codex and Claude draw an all-dim, full-row ghost placeholder. The opaque preedit overlay reproduces the committed row tail it covers, so without this the ghost is repeated to the right of the composing syllable instead of staying masked. The binding keys off the `.xterm-composition-remainder` class that stablyai#17170 adds and hides it through CSS while a composition owns a structurally verified placeholder row — bold prompt glyph plus a dimmed model footer below a blank gap for Codex, a frame line above the prompt for Claude. Arbitrary dim output, shell lookalikes, and any row carrying typed text keep their tail visible. readTerminalCursorLineContext moves from src/main/daemon to src/shared because the renderer now needs the same reader the daemon uses; the move is import-only. Depends on stablyai#17170.
ELI5
While an IME character is being assembled, Orca now keeps that preedit, its caret, the text underneath it, and the operating-system candidate anchor in one coherent xterm composition surface — even at the final terminal cell.
What changed
compositionupdatebut no newcompositionstart.terminal-ime-candidate-anchor.tsapply the same clamp, so the two writers oftextarea.style.leftagree instead of reverting each other.config/reliability-gates.jsonc.The Codex/Claude dim-placeholder masking that was previously in this PR is now #17377, stacked on this one. It is a different domain at a different layer, and its only coupling here is the
.xterm-composition-remainderclass this PR adds. That split took this PR from 18 files / +1,065 to 10 files / +701, excluding the generated bundle and lockfile.Root cause
The opaque xterm preedit view covered the renderer cursor but did not paint its own caret. Its mid-line box also covered committed grid cells while rendering only the preedit, so text under the box disappeared.
At the final column, flex end-alignment moves an over-wide preedit left so its caret stays visible. Native IMEs read xterm's hidden textarea rect to place the candidate window, so that rect has to move with it.
Two writers, and why the obvious fix is wrong
textarea.style.leftgenuinely has two writers: xterm's patchedCompositionHelper, and Orca'sterminal-ime-candidate-anchor.ts. The anchor module listens onterminal.element, so within a single composition event it fires after xterm's textarea listener. An earlier revision of this PR clamped only in the patch, and the anchor module reverted it.The obvious correction — move the clamp into the anchor module and drop the patch hunk — was tried, and it is wrong. The rendered e2e caught it:
CoreBrowserTerminal.ts:444is why:xterm re-asserts the textarea position on every render, not only on composition events. Renders come from PTY output and repaints that carry no composition event at all, so the anchor module — which only listens to
compositionstart/compositionupdate— structurally cannot be the last writer. Whether the anchor survived depended on whether a render happened to land after the last composition event, which is exactly the flake that showed up: two passing runs, then1561.28125, the fully unclamped value.So the fix is not to pick a winner. It is to remove the disagreement:
cursorLeft,maxWidth, and the preedit bounds at the moment it writes.Every path that writes
textarea.style.leftnow produces the same in-screen position, so no ordering can produce a wrong one. Both halves are load-bearing and both are mutation-tested below.Behaviour unchanged from
main_getRowRemainderTextkeepsmain's expression. An earlier revision rewrotetranslateToString(true, x, line.length)totranslateToString(false, x, getTrimmedLength()); upstream doesif (trimRight) endCol = Math.min(endCol, this.getTrimmedLength()), so the two are the same call. Verified as well as read: 1,200 differential comparisons across 15 row shapes (empty, padded, wide CJK, mixed) at every start column, all equal. The rewrite was churn on code already onmain, so it is reverted rather than declared.On the
IThemeServiceconstructor parameterReviewed as requested, because a constructor-signature hunk is the highest-conflict shape for an upstream xterm bump. Keeping it, for three reasons:
rawOptions.themeis the unresolved input and isundefinedfor any key the embedder did not set;colors.cursorin particular carries xterm's own default resolution.color.ensureContrastRatiooperates on xterm'sIColorobjects. Deriving the caret color in the renderer instead means reimplementing xterm's parse, default resolution, and contrast math — and getting it wrong whenevertheme.cursoris unset.src/browser/input/CompositionHelper.test.tsconstructs the class directly, and.npmignorestripssrc/**/*.test.tsfrom the published tarball, so the shipped patch has no hunk that could update that call. Removing the overload fails the upstream build withTS2554and the generator refuses to emit. This was found by running the regeneration, not by reading; the comment in the patch now records it.The theme service is therefore optional, and every color read keeps the stock fallback that six-argument path needs. One redundant rung was dropped: the view foreground no longer falls back through
rawOptions.theme?.foreground.Painting the caret inline after the preedit span does need the patch:
_renderCompositionViewcallsreplaceChildren()on every update.Exact-head verification
Published head:
0bb0b58ba7b.pnpm tc: passed.src/renderer/src/components/terminal-pane/andsrc/renderer/src/lib/pane-manager/: 478 files / 4,730 tests passed, 1 skipped.--workers=1, three consecutive runs: 4/4, 4/4, 4/4.pnpm run check:code-quality:changed: 0 findings.node config/scripts/regenerate-xterm-patches.mjs --check: passed. Bundled patch is 7,388,516 bytes. (The previous body said 7,392,151, which was never the size of any patch this branch produced.)pnpm install --frozen-lockfile: passed; lockfile hash0139c879….pnpm run check:reliability-gates: 99 gates valid.No regression to other IMEs
Run, not asserted:
terminal-ime-xterm-composition-cancel,terminal-ime-xterm-composition-deduplication,terminal-ime-xterm-consumed-key-commit,terminal-ime-hangul-terminating-digit,terminal-ime-composition-tracker,terminal-ios-hangul-preedit-coexistence,ime-composition-keyboard-event,terminal-complex-script,terminal-ime-xterm-transaction-events— 153 tests passed. These cover Chinese Pinyin (Sogou candidate selectors), Japanese romaji-to-kana, and Hangul/Hanja commit paths. All of them are inside the 478-file sweep above as well.Mutation results
Every line this PR adds or changes was broken, the suite re-run, and the line restored. The xterm harness mutates the upstream source, runs
npm run package, and copieslib/intonode_modules; the unmutated build of that harness is byte-identical to the shipped bundles, so the rig is faithful.Math.min(0, ...)droppedMath.max(0, ...)floor droppedNumber.isFinitewidth guard droppedmintomaxdispose()cleanup removedcompositionstartremovedjustifyContent: flex-enddropped_renderCompositionViewremoveddisplay: nonedroppedclassList.toggle('active', ...)to unconditionaladd#FFFTwo surviving mutations from the earlier review —
placeholder: !text && placeholderandhandleBlurnot clearing ownership — are both in the code that moved to #17377, and are killed there by two new tests.The tests that would have caught this
Two were missing, and each one corresponds to a real defect found during this round:
terminal-ime-xterm-midline-preedit-tail.test.tsnow installsinstallTerminalImeCandidateAnchoralongside the patched terminal, so both writers oftextarea.style.leftrun in one rig. Its absence is what let the original two-owner bug through: the old assertion checked624pxwhile never installing the other owner.keeps the candidate anchor inside the screen across a render, not just a composition eventdrives a repaint under an open composition with no composition event behind it. This is the arm that fails on the "move it to the anchor module" revision, and nothing in the previous suite exercised theonRenderpath.tests/e2e/terminal-ime-pane-arena.ts's initial-frame poll is raised from 15s to 30s. At default worker counts on a loaded machine that poll, not any assertion, is what times out first; it was observed failing two specs under concurrent load and passing all four serialized.What runs where
Stated plainly, because the automated evidence does not reach the operating system:
Linked issues
Fixes #16949
Fixes #17104
Shared acceptance checklist: #17172. Follow-up: #17377.
AI disclosure
AI-assisted implementation, test construction, and review were used. All claims above are tied to deterministic tests, rendered inspection, or explicitly named residual gaps.