Repository navigation
Conversation
xterm's composition view is an opaque DOM overlay that sits on the cell the renderer draws the cursor in, so a composing Hangul syllable had no cursor at all — it came back only on commit or when the pane lost focus and the overlay closed. Mirror the cursor in DOM at the end of the preedit, matching the pane's cursorStyle, cursorBlink and cell size. The caret is an in-flow element: a pseudo-element computed correctly but never painted inside this overlay, and an absolutely positioned one falls outside its box. xterm gives the view `direction: rtl` with an isolate, under which the first child is the rightmost, so placement follows the resolved direction. Also cache the Cursor Agent anchor scan, which a Hangul IME repeated per jamo, and stop re-focusing a pane terminal that already holds focus — React effects re-activate the active pane on unrelated renders, and re-focusing a textarea mid-composition can drop the preedit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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; 4 remain after this review. 📝 WalkthroughWalkthroughThe PR adds pane activation focus helpers that skip redundant terminal focus calls. It adds a DOM caret for xterm IME composition views with block, bar, underline, RTL, theme color, and blink support. It integrates the caret into IME anchor handling. It caches resolved Cursor Agent anchors by cursor and viewport state and invalidates the cache after terminal writes or cursor movement. Tests cover focus decisions and anchor cache behavior. Merge Risk: ⚪ Minimal · up to This localized renderer change restores the cursor during CJK preedit and reduces redundant work without changing transport, permissions, or external interfaces; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is mostly complete. It includes the main required sections, linked issue, visual proof, testing details, AI disclosure, review notes, and known gaps. The Agent skill boundary and checklist sections are not included, but these omissions do not prevent review of the change. Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation All changes are related to issue
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d7a03ff2-8feb-4acb-b4f5-a3fb0ea9bc76
📒 Files selected for processing (7)
src/renderer/src/assets/terminal.csssrc/renderer/src/lib/pane-manager/pane-active-focus.test.tssrc/renderer/src/lib/pane-manager/pane-active-focus.tssrc/renderer/src/lib/pane-manager/pane-manager.tssrc/renderer/src/lib/pane-manager/terminal-ime-candidate-anchor.test.tssrc/renderer/src/lib/pane-manager/terminal-ime-candidate-anchor.tssrc/renderer/src/lib/pane-manager/terminal-ime-preedit-caret.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
… caret comment Two comment blocks had grown over the same rule saying the same thing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r colour The caret inherited the composition overlay's foreground, which happens to equal the cursor colour in both default themes and diverges in every other one — a green cursor in classic or a blue one in One Dark drew a preedit caret that matched neither. xterm's resolved theme already carries the configured colour with terminalCursorOpacity applied, so the caret reads that and falls back to the stylesheet only for a pane with no theme. Verified live: at opacity 0.4 the caret paints rgba(255,255,255,0.4) while the overlay's own foreground stays opaque. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ELI5
While you are typing a Korean syllable in a terminal, the cursor used to vanish — it came back only when you finished the syllable, or when you clicked away. It was never gone, just hidden underneath the little box that shows the half-typed character. This draws the cursor back, at the end of that box, looking exactly like the one it stands in for.
What Changed
The preedit caret. xterm renders an in-flight composition into
.composition-view, an opaque DOM overlay positioned over the cell the renderer draws the cursor in. The cursor is painted on the WebGL/canvas layer, so it cannot show through.terminal-ime-preedit-caret.tsmirrors it in DOM at the end of the preedit, reading the pane's owncursorStyle(block / bar / underline),cursorBlink, and cell metrics so it matches rather than invents a look.Two implementation notes worth keeping, both recorded as comments:
::aftercomputed correctly but never painted inside this overlay, and an absolutely positioned one falls outside its box and is not painted eitherdirection: rtlwithunicode-bidi: isolate, under which the first child is the rightmost. Placement follows the resolved direction rather than assuming append puts the caret lastAnchor scan caching.
resolveCursorAgentImeAnchorwalks visible rows bottom-up reading up tocolscells per row. A Hangul IME firescompositionupdateper jamo, so that O(rows×cols) walk ran two to three times per typed character. The result is derived from the cursor position and the viewport contents: the cursor is cheap to compare so it is the cache key, and the contents invalidate by event (onWriteParsed/onCursorMove).Redundant focus.
setActivePanecomputedchangedbut used it only to gateonActivePaneChange;terminal.focus()ran unconditionally, so React effects re-activating the already-active pane refocused it — 21 calls over 30s of typing.pane-active-focus.tsskips the call when the target textarea already holds focus.Why
The missing cursor is the user-visible one. The other two are waste that this investigation surfaced, and they belong here because they are the same code path.
Being straight about their size: the anchor scan measured 45–96µs per jamo, and re-focusing an already-focused element is a no-op in a single-pane layout. Neither is something a user feels today. The focus one is a live hazard the moment focus can actually move mid-composition (split panes, chat↔terminal), which is why it is fixed rather than left.
Linked Issue
Fixes #16949
Visual Proof
The caret is a new visible element. Verified in the running app while a syllable composed:
viewX + 14— immediately right of the preeditorca-ime-preedit-caret-blink, 1.2s stepBefore, the same probe found no cursor element at all for the whole composition.
Testing
pane-active-focus.test.tsis new and covers the focus decision including the regression it guards.terminal-ime-candidate-anchor.test.tsgained cases for cache invalidation on write and on cursor move, so a stale anchor cannot come back.The caret's placement and geometry were verified by driving a composition over CDP and reading the element's rect and computed style, then screenshotting it. That is not automated here: it needs a live xterm with a real composition overlay, which the node-environment suite does not provide.
Platform: developed and measured on Windows 11. The overlay behavior is xterm's own, so the missing cursor should reproduce identically on macOS and Linux, but the fix is unverified there by me.
pnpm testcould not run in my environment — theensure-native-runtime --runtime=nodeprestep fails to rebuildnode-ptyfor Node 24 — so vitest was invoked directly.AI Disclosure
Written with Claude Opus 5 (Claude Code). The geometry figures above were read from the running app over CDP.
Review
Cross-platform — no platform branch. The caret reads xterm options and cell metrics, both platform-neutral.
currentColortracks the overlay's own foreground, which xterm keeps legible against its ground, so it works in both themes.SSH / remote — renderer-local throughout. No transport, pty, or execution-host code is touched.
Agents / integrations — the Cursor Agent anchor branch is unchanged in behavior, only cached. No provider-specific naming introduced.
Backwards compatibility —
PaneManager.setActivePanekeeps its signature, its return, and itsonActivePaneChangefiring condition. The only behavior change is skippingfocus()when the target already has it; a terminal without an open textarea, or one whose focus is elsewhere, is focused exactly as before.Performance — the anchor cache removes the repeated buffer walk.
getComputedStylein the caret path runs once per composition update, not per frame.Security — no new IPC, no new input path, no parsing of user content.
Known gaps
terminal.cssare scoped under.pane-manager-root, a class that no longer exists in the DOM — including two IME fixes (.xterm-helpers { left: 0 }for candidate-window placement, andcaret-color: transparentfor a stale Windows caret), which are therefore inert today. The new rule here is scoped to.xterm-containerto match the actual DOM. Re-enabling the dead ones is its own change with its own regression risk and is left alone.