Skip to content

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

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

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

Conversation

@ddddajie

Copy link
Copy Markdown

Summary

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 lines, viewport offsets, and vertical scrolling
  • hides the hardware cursor when the input loses focus or unmounts
  • keeps the existing visual cursor behavior unchanged

Tests

Added regression coverage for:

  • 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

Validation:

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

The full CLI test command is currently blocked by the pre-existing missing preload ../test/setup-scm-loader.ts.

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

Manual Windows Terminal / CJK IME verification was not completed.

@codebuff-team

Copy link
Copy Markdown
Contributor

Good instinct and unusually thorough test coverage (unit tests for calculateMultilineInputCursorPosition covering ASCII, CJK width, tabs, wrapping, scroll, plus renderer-level lifecycle tests for cursor visibility on focus/unmount). That alone puts this above most first-time-contributor PRs.

Concerns before this is portable as-is:

  1. syncHardwareCursor in multiline-input.tsx reaches into (textRef.current as any).textBufferView as TextBufferView — an any-cast into what looks like a private/internal OpenTUI renderable field, not part of its declared type surface. If TextBufferView or textBufferView isn't actually an exported/stable API, this will silently break on the next OpenTUI bump, and there's no fallback beyond the null check. Worth confirming this is intended public surface, or exposing it properly upstream.

  2. The line-info-change event listener is attached/detached each render via the useEffect dependency array [focused, displayValue, cursorPosition, lineInfo, syncHardwareCursor] — since syncHardwareCursor is recreated whenever renderer changes (stable) but the effect itself re-runs on every keystroke (displayValue/cursorPosition change every input), this re-subscribes the listener on every keystroke. Not necessarily wrong, but worth confirming there's no listener leak/thrash under rapid typing.

  3. The author states manual Windows Terminal / CJK IME verification — the actual scenario in CJK IME candidate window anchors to wherever text last changed on screen, not to the input caret — real terminal cursor is never moved #1128 — was not completed. Given this is fixing a rendering/terminal-integration bug that unit tests can only approximate (mocked TextBufferView, no real IME), this is the biggest gap. A porting maintainer will want to confirm the fix against a real terminal before merging.

Recommend keeping the calculation logic (calculateMultilineInputCursorPosition is clean and well-tested) but validating the textBufferView access is safe and doing the manual IME test pass before porting.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Aug 29, 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:needs-work Right idea, not mergeable as written

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