feat(input): move physical cursor to visual cursor for IME input#4652
Conversation
…r cursor positioning
| // Calculate prefix width for physical cursor positioning | ||
| const prefixWidth = shellModeActive | ||
| ? reverseSearchActive | ||
| ? 5 // "(r:) " = 5 chars |
There was a problem hiding this comment.
[Critical] prefixWidth is off by one for (r:) modes.
The prefixNode JSX renders '(r:) ' (5 chars, including a trailing space inside the inner <Text>) plus the outer {' '} (1 more space), totaling 6 columns. But prefixWidth is set to 5 here (and again at the commandSearchActive branch).
This means the IME candidate window will be positioned 1 column too far left when reverse-search or command-search mode is active — the very modes where the user is most likely composing text.
The other branches are correct: "!" + {' '} = 2, "*" + {' '} = 2, ">" + {' '} = 2.
| ? 5 // "(r:) " = 5 chars | |
| ? 6 // "(r:) " (inner) + " " (outer) = 6 cols |
— qwen3.7-max via Qwen Code /review
| // addLayoutListener requires the root node (ink-root), not the component | ||
| // node. We find it by walking up the Ink DOM parent chain. | ||
| const rootRef = useRef(null); | ||
| useBoxMetrics(rootRef); |
There was a problem hiding this comment.
[Suggestion] useBoxMetrics(rootRef) is called but its return value is discarded. Internally, useBoxMetrics registers its own addLayoutListener on the root node (a second one, alongside the PR's own cursor listener) and calls setMetrics() / setHasMeasured() — two useState updates — on every layout commit. All of this work is wasted since the metrics are never read.
The ref is already populated by React via <Box ref={rootRef}> — no hook is needed for that. Removing useBoxMetrics eliminates the duplicate layout listener and avoids unnecessary re-renders of BaseTextInput on every Ink commit cycle.
| useBoxMetrics(rootRef); | |
| const rootRef = useRef(null); | |
| const cursorCtx = useContext(CursorContext); |
Also remove useBoxMetrics from the import:
| useBoxMetrics(rootRef); | |
| import { Box, Text } from 'ink'; |
— qwen3.7-max via Qwen Code /review
| "build:sdk:python": "python3 -m build packages/sdk-python", | ||
| "check-i18n": "npm run check-i18n --workspace=packages/cli", | ||
| "preflight": "npm run clean && npm ci && npm run format && npm run lint:ci && npm run build && npm run typecheck && npm run test:ci", | ||
| "postinstall": "patch-package", |
There was a problem hiding this comment.
[Suggestion] patch-package is added as a postinstall hook to apply patches/ink+7.0.3.patch, but ink is declared as "^7.0.3" (caret range). If npm resolves a newer Ink version (e.g. 7.0.4), patch-package will refuse to apply the patch because the filename is version-pinned to 7.0.3. The imports from 'ink/dom' and from 'ink/components/CursorContext' would then fail at runtime with ERR_PACKAGE_PATH_NOT_EXPORTED.
While the lockfile mitigates this in practice, consider pinning Ink to the exact version to make the coupling explicit:
"ink": "7.0.3"— qwen3.7-max via Qwen Code /review
…cs, pin ink version Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
|
@wenshao Thanks for the review. R1 Made the following changes:
|
|
Noticed two small issues while using the cli:
I can handle the first one. Are there plans to add shorter phrases? That would make keeping LoadingIndicator more useful. |
wenshao
left a comment
There was a problem hiding this comment.
All R1 findings addressed in this revision (prefixWidth corrected to 6 for (r:) modes, useBoxMetrics removed, ink pinned to exact 7.0.3). Build passes, 1279 tests pass, tsc + eslint clean. No high-confidence issues found.
— qwen3.7-max via Qwen Code /review
Verification ReportBranch: Test Results
Patch VerificationAfter {
".": { "types": "./build/index.d.ts", "default": "./build/index.js" },
"./dom": { "default": "./build/dom.js" },
"./components/CursorContext": { "default": "./build/components/CursorContext.js" }
}Code Review Notes
Caveats
Verdict✅ Ready to merge — patch applies cleanly, all imports resolve at runtime, typecheck is clean, build succeeds. The approach is sound and well-documented in the PR. Verified by wenshao |
|
Hi! @wenshao Thanks for your approval! If possible, could you also help invite another reviewer for this PR? |
|
Thanks for this PR, @zzhenyao! Template: The PR uses its own heading structure (Summary / Motivation / Changes / Design Decisions / Scope / Test Plan) instead of the project's PR template. The content quality is excellent — clear problem statement, before/after screenshots, design rationale, and verification steps are all present. Two small gaps: no "Tested on" OS matrix and no 中文说明 section. Not blocking, but please align with the template format for consistency — it helps reviewers scan PRs faster. On direction: This solves a real and painful problem for CJK users — IME candidate box appearing at the bottom-left instead of following the cursor is a daily friction point. Clearly aligned with the On approach: Scope feels right — only touches cursor positioning, doesn't restructure other components. The One question worth thinking about: if Ink 7.1+ ships with these exports natively, the patch becomes dead weight. Have you considered adding a comment in the patch file or a TODO noting the intent to drop it once upstream supports these exports? Not blocking — just helps the next person who sees Moving on to code review. 🔍 中文说明感谢 @zzhenyao 的贡献! 模板: PR 使用了自己的标题结构(Summary / Motivation / Changes / Design Decisions / Scope / Test Plan),未对齐项目的 PR 模板。内容质量很好——问题描述清晰、有前后对比截图、设计决策充分、验证步骤完整。两个小缺口:没有 "Tested on" 操作系统矩阵,没有中文说明段落。不作为阻塞项,但建议对齐模板格式。 方向: 这解决的是 CJK 用户的真实痛点——输入法候选框出现在终端左下角而非跟随光标,是日常高频摩擦。与 方案: 用 范围合理——只涉及光标定位,未重构其他组件。 一个值得思考的问题:如果 Ink 7.1+ 原生支持这些导出,patch 就变成了无用代码。是否考虑在 patch 文件中加个注释或 TODO,标注"上游支持后移除"的意图?不阻塞——只是帮助下一个看到 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
Code review: Read through the diff with an independent proposal in mind. The problem is clear — Ink hides the terminal cursor and renders its own visual cursor via inverted characters, so the physical cursor (and IME candidate box) defaults to bottom-left. The fix needs to call The implementation is clean and well-reasoned:
No critical blockers, no AGENTS.md violations. The reliance on Ink internals is a known tradeoff — the author explicitly mentions forking Ink as a fallback if this approach proves insufficient, which is the right escalation path. Testing: Built and tested the PR branch from source. Patch applies cleanly ( Caveat: IME candidate box positioning cannot be verified in headless tmux — it requires a real terminal session with a CJK input method active (e.g. macOS 搜狗/苹果拼音). The automated tests confirm the code compiles, builds, and basic input handling works. Full IME verification needs a human reviewer on macOS with a CJK IME. 中文说明代码审查: 带着独立方案阅读 diff。问题很清晰——Ink 隐藏终端光标并用反色字符渲染视觉光标,导致物理光标(和输入法候选框)默认停在左下角。修复方案需要用绝对屏幕坐标调用 实现干净且有充分理由:
无 critical blocker,无 AGENTS.md 违规。依赖 Ink 内部模块是已知取舍——作者明确提到如果此方案不够用可以 fork Ink,这是正确的升级路径。 测试: 从源码构建并测试 PR 分支。Patch 正确应用( 注意: 输入法候选框定位无法在 headless tmux 中验证——需要真实终端会话并激活 CJK 输入法(如 macOS 搜狗/苹果拼音)。自动化测试确认代码编译、构建、基本输入处理正常。完整的输入法验证需要 macOS 上使用 CJK 输入法的人工审查。 — Qwen Code · qwen3.7-max |
|
Stepping back — this is a well-scoped PR solving a real daily friction for CJK users. The before/after screenshots in the PR description are immediately relatable: anyone who's typed Chinese in a terminal TUI knows that "IME candidate box stuck at bottom-left" feeling. The approach is the right level of pragmatism. The code itself reads like someone who understands both React lifecycle subtleties and Ink's rendering pipeline. The My one reservation: From a maintenance perspective: I'd rather maintain this than not have it. The CJK input experience is meaningfully better, the code is contained to the right files, and the Ink dependency story is clear. R1 review caught the prefixWidth off-by-one and the unused LGTM, looks ready to ship. ✅ 中文说明退一步看——这是一个范围合理的 PR,解决了 CJK 用户的日常高频摩擦。PR 描述中的前后对比截图一看就懂:在终端 TUI 里打过中文的人都知道"输入法候选框卡在左下角"是什么体验。 方案处于正确的务实区间。用 代码本身体现了对 React 生命周期和 Ink 渲染管道的双重理解。 一个保留意见: 从维护角度看:有这个比没有好。CJK 输入体验有实质改善,代码收敛在正确的文件中,Ink 依赖策略清晰。R1 review 发现了 prefixWidth 偏差和未使用的 可以合并 ✅ — Qwen Code · qwen3.7-max |
pomelo-nwu
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
@pomelo-nwu Thanks for the review and approval! 🙏 |
Summary
addLayoutListenerinstead ofuseCursor. It fires aftercalculateLayout(), so cursor position reads the current yoga layout with zero jitter.useRef. Layout listener subscribes once and skips redundant updates.patch-packageto exposeink/domandink/components/CursorContext. Static import, no dynamic loading.Motivation
Fixes #3456
Qwen Code renders a visual cursor using inverted-color characters. The terminal physical cursor is hidden and defaults to the bottom-left corner. CJK input method users see the IME candidate box at bottom-left, far from where they type.
Before:
After:
Changes
packages/cli/src/ui/components/BaseTextInput.tsxaddLayoutListener+CursorContext, stableuseRefsubscription, cleanup on blur and unmountpackages/cli/src/ui/components/InputPrompt.tsxpackages/cli/src/ui/components/agent-view/AgentComposer.tsxpackage.jsonpatch-packagedependency andpostinstallscriptpatches/ink+7.0.3.patchink/domandink/components/CursorContextDesign Decisions
addLayoutListeneroveruseCursor: Ink render order:calculateLayout()→emitLayoutListeners()→onRender().useCursorreads layout during React's render phase, before yoga recomputes.addLayoutListenerfires aftercalculateLayout()— cursor position is always current, no jitter.useReffor stable subscriptio: Cursor state lives in a ref, updated each render.useEffectdeps are[cursorCtx]— the layout listener subscribes once. Skip the call when position hasn't changed. This removes the gap between cleanup and re-subscription.showCursor. CallsetCursorPosition(undefined)when blurred. Cleanup function clearscursorPositionon unmount — goodbye screen renders without stale cursor position.patch-packagefor internal module acces: Ink only exports its main entry. Node.js 22 enforces the exports map.patch-packageaddsink/dom(addLayoutListener) andink/components/CursorContext(setCursorPosition) to the exports field. Static imports work directly.setCursorPositionevery frame:log-update'scursorDirtyis a one-shot flag — set on call, reset on next render. Skip a call and the flag staysfalse, the next frame gets no cursor suffix, and the physical cursor falls to the end of output.Scope
Test Plan
How to Verify
Build and run
Patch applies correctly
npm install—patch-packageappliesink+7.0.3.patchautomaticallynode_modules/ink/package.jsonhasink/domandink/components/CursorContextin the exports fieldIME cursor positioning
Regression