Skip to content

fix(cli): sync terminal cursor with input caret for CJK IME - #1177

Open
ddddajie wants to merge 1 commit into
CodebuffAI:mainfrom
ddddajie:fix/1128-ime-cursor-position-v2
Open

fix(cli): sync terminal cursor with input caret for CJK IME#1177
ddddajie wants to merge 1 commit into
CodebuffAI:mainfrom
ddddajie:fix/1128-ime-cursor-position-v2

Conversation

@ddddajie

@ddddajie ddddajie commented Sep 1, 2026

Copy link
Copy Markdown

Summary

Recreates #1142 on top of the rewritten main history, as requested by the maintainer in the closed PR. Fixes #1128.

The CLI rendered a visual caret but did not move the real terminal hardware cursor to the input position. CJK IMEs use the terminal cursor position to anchor their composition and candidate windows, which caused the popup to appear at unrelated screen locations.

This change:

  • synchronizes the real terminal cursor with the MultilineInput caret
  • accounts for CJK terminal-cell width using string-width
  • handles tabs, wrapped visual lines, viewport offsets, and vertical scrolling
  • hides the hardware cursor when the input loses focus or unmounts
  • keeps the existing visual cursor behavior unchanged
  • centralizes OpenTUI 0.3.4 visual textBufferView.lineInfo access behind a guarded adapter while preserving wrapped-line semantics
  • keeps the line-info-change subscription stable instead of re-subscribing on ordinary caret/value changes

Tests

Regression coverage includes:

  • ASCII cursor positioning
  • CJK wide characters
  • mixed ASCII/CJK input
  • tab expansion
  • wrapped visual lines
  • viewport and vertical scroll offsets
  • cursor visibility on focus/unmount
  • component-level OpenTUI renderer cursor positioning after a CJK character
  • renderer-level cursor placement on a wrapped visual row

Validation from #1142:

  • bun test cli/src/components/__tests__/multiline-input.test.tsx — 81 passed
  • repeated focused test run (--rerun-each 3) — 243 passed
  • git diff --check — passed

CLI typecheck was blocked by pre-existing missing tar and react-dom/server typings; no errors were reported in the changed files.

Manual verification

Manually verified in Windows Terminal with Microsoft Pinyin. Candidate-window positioning followed the input caret correctly across normal CJK input, cursor movement, wrapped lines, and scrolling.

This branch is rebuilt directly from the current rewritten main and contains a single clean commit. Previous review context: #1142.

@codebuff-team

Copy link
Copy Markdown
Contributor

Good bug fix with a clear, verifiable cause: the visual caret was rendered but the terminal's real hardware cursor was never moved, so CJK IME candidate windows anchored to the wrong screen location.

Strengths:

  • calculateMultilineInputCursorPosition is extracted as a pure function and covered by unit tests for ASCII, CJK width (via string-width), tabs, wrapped lines, and scroll offsets — this is exactly the kind of coordinate-math logic that benefits from isolated tests rather than only integration coverage.
  • getVisualLineInfo replaces the previous unguarded as any cast on textBufferView.lineInfo with runtime validation (Array.isArray + type check), which is a real robustness improvement even independent of the IME fix.
  • Cursor visibility is correctly torn down on blur and unmount (renderer.setCursorPosition(0, 0, false)), avoiding a stuck hardware cursor.
  • The line-info-change subscription is scoped to focused and the sync callback, avoiding resubscription on every keystroke — addresses a plausible perf concern from the earlier review round.

Concerns for the maintainer to weigh during porting:

  • The adapter still depends on textRenderable.textBufferView.lineInfo, an internal OpenTUI 0.3.4 API not part of its public surface — the comment acknowledges this, but it means the guard only prevents crashes, not silent breakage if the internal shape changes meaning (e.g. lineStartCols staying an array of numbers but semantics shifting).
  • calculateMultilineInputCursorPosition's newline-counting logic (renderedPrefix.match(/\n/g)) assumes string-width treats \n as zero-width; this holds in the given test but is not explicitly asserted/commented, so a string-width version bump could silently break multi-line cursor placement without any test catching a width-counting change if it started counting newlines.
  • Manual verification is Windows Terminal + Microsoft Pinyin only; no coverage for iTerm2/Linux terminals mentioned, though that's likely acceptable given CI test breadth.

Overall this is exactly the sort of PR the private repo would want ported — clear cause, contained change, and thorough tests. Worth a maintainer's look.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CJK IME candidate window anchors to wherever text last changed on screen, not to the input caret — real terminal cursor is never moved

2 participants