fix(input): restore IME cursor positioning reverted in #4779 - #4993
Conversation
|
CI failure note: all three OS test jobs fail on the same two cases in
The test names self-describe as pins for a known upstream js-yaml limitation. The same two failures reproduce on the latest Lint, CodeQL, and Classify PR all pass. |
…nLM#4652) * feat(input): add useCursor hook for IME physical cursor tracking * refactor(input): optimize cursor positioning effect * feat(input): move setCursorPosition to render phase for immediate cursor positioning * fix(input): calculate absolute cursor position by walking yoga tree * fix(input): use addLayoutListener instead of useCursor for zero-jitter cursor positioning * perf(input): stable addLayoutListener subscription and skip redundant cursor updates * fix(input): revert lastPos dedup that broke cursorDirty one-shot flag * feat(input): use patch-package to expose Ink internals for IME cursor positioning * fix(input): address review feedback — prefixWidth, remove useBoxMetrics, pin ink version Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
QwenLM#4600 unified the approval-mode prompt styling into approvalModePromptStyle and removed the showAutoAcceptStyling / showYoloStyling constants, but left a dangling showYoloStyling reference in the prefixWidth calculation. This broke the tsc build (TS2304: Cannot find name showYoloStyling). npm run bundle (esbuild) skips type-checking, so the break only surfaced under npm run build / tsc --build. Replace it with approvalMode === ApprovalMode.YOLO, matching how the rest of the file branches on approval mode after QwenLM#4600. Both ternary arms stay 2 because every approval-mode prefix is a single char (getApprovalModePromptStyle returns prefix > or *), so the rendered prefix width and cursor positioning are unchanged.
370e480 to
45cd12e
Compare
|
@zzhenyao help review , sorry for that commit-revert |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
No issues found. LGTM! — qwen3-coder via Qwen Code /review
LGTM, thanks for restoring! |
| "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.
[Critical] "postinstall": "patch-package" will block all npm install runs if the Ink patch fails to apply (ink version bump, patch conflict, file permissions, etc.). This breaks CI pipelines, Docker builds, and fresh checkouts with no fallback.
| "postinstall": "patch-package", | |
| "postinstall": "patch-package || true" |
Alternatively, run patch-package only in CI with --error-on-fail, or move it to prepare where failures are more expected.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| : 2 // "! " = 2 chars | ||
| : commandSearchActive | ||
| ? 6 // "(r:) " (inner) + " " (outer) = 6 cols | ||
| : approvalMode === ApprovalMode.YOLO |
There was a problem hiding this comment.
[Suggestion] Dead ternary: both branches return 2, making the approvalMode === ApprovalMode.YOLO check meaningless. Reads as if YOLO needs a distinct width, but produces the same value as the default branch. A future change that adds a multi-character prefix for a new approval mode may update only one branch, causing a silent IME cursor misalignment.
| : approvalMode === ApprovalMode.YOLO | |
| : 2; // "> " or "* " = 2 chars |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (!node.parentNode) | ||
| return node['nodeName'] === 'ink-root' ? (node as DOMElement) : undefined; | ||
| return findRootNode(node.parentNode as Record<string, unknown>); | ||
| } |
There was a problem hiding this comment.
[Suggestion] findRootNode recurses up Ink's internal parentNode chain with no depth limit. If Ink's DOM structure ever changes unexpectedly (e.g., a virtual wrapper node introduced between the Box and ink-root, or a render bug creating a self-reference), this will crash with RangeError: Maximum call stack size exceeded — a hard CLI process crash with no recovery.
| } | |
| function findRootNode( | |
| node: (Record<string, unknown> & { parentNode?: unknown }) | null, | |
| depth = 0, | |
| ): DOMElement | undefined { | |
| if (!node || depth > 50) return undefined; | |
| // ... rest unchanged | |
| return findRootNode(node.parentNode as Record<string, unknown>, depth + 1); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| </Text> | ||
| ); | ||
|
|
||
| // Calculate prefix width for physical cursor positioning |
There was a problem hiding this comment.
[Suggestion] prefixWidth values are hardcoded (2 for default/YOLO, 6 for search modes) rather than derived from the actual prefix string. If shell/approval mode prefixes change (e.g., a custom approval mode with a 3-character prefix), the physical cursor X position will be silently misaligned. The IME popup will appear offset from the actual composition text.
Consider computing prefixWidth from the rendered prefix string using stringWidth() instead of maintaining a parallel hardcoded mapping.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // but BEFORE onRender() — yoga layout is fresh, terminal not yet written. | ||
| // 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); |
There was a problem hiding this comment.
[Suggestion] useRef(null) without a type parameter — TypeScript infers MutableRefObject<null>, so rootRef.current is typed as null even though React will assign a DOMElement at runtime. A future refactor could add incorrect null guards based on this type, silently breaking the feature.
| const rootRef = useRef(null); | |
| const rootRef = useRef<DOMElement | null>(null); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
@BenGuanRan heads up — this PR currently has merge conflicts with Conflicting files:
The rest merges cleanly. Thanks! 中文@BenGuanRan 提个醒 —— 这个 PR 目前和 冲突文件:
其余文件可以自动合并。谢谢! |
|
@copilot resolve the merge conflicts in this pull request |
1 similar comment
|
@copilot resolve the merge conflicts in this pull request |
|
@qwen-code /triage |
|
Thanks for the PR @BenGuanRan! Template: The PR body doesn't follow the PR template. The description is thorough, but it's missing the Reviewer Test Plan (with How to verify / Evidence / Tested on table), Risk & Scope, and Chinese translation sections. Would be great to fill those in when you get a chance — especially the Tested on table, since IME behavior varies a lot across platforms. Merge conflict: On direction: Clearly aligned. PR #4652 was a legitimate fix for IME cursor positioning (#3456), it was accidentally reverted in #4779, and this restores it. No question about whether this should ship. On approach: Scope is tight — two cherry-picks that do exactly what's described. One note for the code review: the Moving on to code review. 🔍 中文说明感谢 @BenGuanRan 的 PR！ 模板： PR 正文未使用 PR 模板。描述很详细，但缺少 Reviewer Test Plan（验证方法 / 证据 / 测试平台表）、Risk & Scope 和中文翻译。方便时请补上——尤其是 Tested on 表，因为 IME 行为在不同平台差异较大。 合并冲突： 方向： 明确对齐。#4652 是 #3456 的合理修复，在 #4779 中被误 revert，本 PR 将其恢复。合入没有问题。 方案： 范围紧凑——两个 cherry-pick 完全对应描述的目标。 代码审查的一个关注点： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewReviewed against the diff and the current state of
VerificationIME Testing LimitationIME cursor positioning requires a graphical terminal with an active input method (e.g. Pinyin, Japanese IME). This headless CI environment cannot test the actual IME behavior. The original PR author (@zzhenyao) confirmed the implementation works — manual verification on a real terminal with CJK IME is still needed before merge. Summary: Code review is clean. Build, typecheck, and unit tests all pass. The implementation is well-structured and follows project conventions. The merge conflict in 中文说明代码审查对照 diff 和
验证构建、类型检查和单元测试全部通过（详见上方英文部分输出）。 IME 测试限制IME 光标定位需要图形终端和激活的输入法（如拼音、日语 IME）。当前无头 CI 环境无法测试实际 IME 行为。原作者 (@zzhenyao) 已确认实现有效——合入前仍需在真实终端上用 CJK 输入法手动验证。 总结： 代码审查无问题。构建、类型检查和单元测试全部通过。实现结构良好，遵循项目规范。 — Qwen Code · qwen3.7-max |
Final AssessmentThis PR does the right thing — restoring a legitimate bug fix that was accidentally reverted. The IME cursor positioning implementation is well-structured, the What blocks approval:
What's clean: code quality, scope (no drive-by changes), approach ( 中文说明最终评估本 PR 做了正确的事——恢复被误 revert 的合理 bug 修复。IME 光标定位实现结构良好， 阻塞批准的原因：
无问题的部分： 代码质量、范围（无顺手改动）、方案（ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Merge conflict in package-lock.json needs resolving, and IME behavior needs a quick manual check on a real terminal before shipping. Code itself looks good. 🙏
# Conflicts: # package-lock.json
|
@qwen-code /triage |
|
Thanks for the PR @BenGuanRan! Template: The PR body doesn't follow the PR template — missing Reviewer Test Plan (How to verify / Evidence / Tested on table), Risk & Scope, and Chinese translation. The description is thorough though, so not blocking on this. Merge conflict: On direction: Clearly aligned. PR #4652 was a legitimate fix for IME cursor positioning (#3456), accidentally reverted in #4779. This restores it with two clean cherry-picks that preserve original authorship. No question about whether this should ship. On approach: Scope is tight — two cherry-picks, nothing extraneous. Moving on to code review. 🔍 中文说明感谢 @BenGuanRan 的 PR！ 模板： PR 正文未使用 PR 模板——缺少 Reviewer Test Plan（验证方法 / 证据 / 测试平台表）、Risk & Scope 和中文翻译。描述很详细，不因此阻塞。 合并冲突： 方向： 明确对齐。#4652 是 #3456 的合理修复，在 #4779 中被误 revert，本 PR 用两个干净的 cherry-pick 恢复并保留原作者。合入没有问题。 方案： 范围紧凑——两个 cherry-pick，没有多余内容。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewReviewed the diff against current
VerificationSmoke Test (tmux)CLI starts and responds correctly with the patched ink dependency. The devtools notice is benign. IME Testing LimitationIME cursor positioning requires a graphical terminal with an active input method (Pinyin, Japanese IME, etc.). This headless CI environment cannot test the actual IME behavior. The original author (@zzhenyao) confirmed the implementation works — manual verification on a real terminal with CJK IME is recommended before merge. Summary: Code review clean. Build, typecheck, and all 164 relevant tests pass. Implementation is well-structured and follows project conventions. Merge conflict resolved. Ready to merge pending manual IME verification. 中文说明代码审查对照 diff 和
验证结果
冒烟测试CLI 使用修补后的 ink 正常启动和响应。 IME 测试限制IME 光标定位需要有图形终端和活跃输入法（拼音、日文 IME 等）。无头 CI 环境无法测试实际 IME 行为。原作者 @zzhenyao 确认实现有效——建议合入前在真实终端上用 CJK 输入法手动验证。 总结： 代码审查通过。构建、类型检查和全部 164 个相关测试通过。合并冲突已解决。建议合入，待手动 IME 验证。 — Qwen Code · qwen3.7-max |
VerdictThis is a straightforward restoration of a previously merged feature that was accidentally reverted. The two cherry-picks are clean, the implementation is well-structured, and the scope is minimal — exactly what's needed to fix the regression. Build, typecheck, and all 164 relevant tests pass. The patch-package integration works correctly. The CLI starts up without issues on the patched ink. The only gap is IME testing — headless CI can't verify cursor positioning with an active input method. But @zzhenyao (original PR #4652 author) already confirmed LGTM, which carries weight since they wrote and tested the original implementation. Recommendation: Approve. The merge conflict has been resolved, code review is clean, and the risk is low — this is a cherry-pick of code that was already on 中文说明结论这是一个简单的恢复——将误 revert 的已合入功能还原回来。两个 cherry-pick 干净，实现结构合理，范围最小——恰好修复回归所需。 构建、类型检查和全部 164 个相关测试通过。patch-package 集成正常。CLI 在修补后的 ink 上正常启动。 唯一的缺口是 IME 测试——无头 CI 无法验证活跃输入法下的光标定位。但 @zzhenyao（原 PR #4652 作者）已确认 LGTM，作为原作者的确认有分量。 建议： 批准合入。合并冲突已解决，代码审查通过，风险低——这是之前已在 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — clean restoration of the IME cursor positioning. Build, typecheck, and all 164 relevant tests pass. Merge conflict resolved. Recommend manual IME spot-check before or after merge. ✅
Maintainer verification — local merge build + flagged-failure triageVerified the 3-way merge result (current 1. Build / typecheck / lint (merge result)
2. The
|
| 检查 | 结果 |
|---|---|
BaseTextInput.test.tsx（恢复的光标逻辑） |
✅ 3 通过 |
InputPrompt.test.tsx（prefixWidth 接线） |
✅ 160 通过 |
tsc --noEmit CLI —— 改动组件 + 打补丁后的 ink 导入 |
✅ 干净 |
npm run build core |
✅ 通过 |
eslint + prettier --check（3 个改动组件） |
✅ 干净 |
| CI | ✅ macOS / Windows / Linux 全绿；bot 已 approve |
2. ink 补丁有效
PR 附带 patches/ink+7.0.3.patch（由 patch-package postinstall 应用），用于暴露 ink/dom 与 ink/components/CursorContext，并把 ink 锁到精确的 7.0.3。验证：
- 对已安装的
ink@7.0.3执行git apply --reverse --check成功，即补丁的产物与包完全一致——且require.resolve('ink/dom')/require.resolve('ink/components/CursorContext')都能解析。（CI 会在 pristine 的 ink 上通过patch-package全新应用，且已绿。）
3. prefixWidth 重命名修复
InputPrompt.tsx 正确使用 approvalModePromptStyle 并把 prefixWidth 接到 BaseTextInput——不再有悬空的 showYoloStyling 引用（这正是 #4779 回滚所掩盖的 bug）。已对三个改动组件 grep 确认。
4. PR 标注的 14 个"无关"失败 —— 确认为既有且 flaky
PR 请维护者确认这些失败在 main 上也会复现。确实如此，而且是 flaky：
AuthDialog.test.tsx—— 失败数在相同命令的多次运行间波动：13（作者）→ 11 → 10 → 11。失败类型是vi.waitFor.timeout（如AuthDialog.test.tsx:154），即渲染计时超时，而非断言失败。本 PR 完全没有触及 auth dialog 的源码或测试，所以无论有没有它，跑的都是同一份代码——这些失败是既有的 flakiness，不是 fix(input): restore IME cursor positioning reverted in #4779 #4993 引起的。memoryDiagnostics.test.ts—— 这里 12/12 通过（作者说的"1 个失败"未复现——同样属于 flaky/环境性）。
所以这些 flag 的失败是一个独立的既有 flaky 测试问题，与本改动无关。不应阻塞合并（重跑 CI 即可消除；bot 重跑也报告相关测试全过）。
5. 范围说明 —— IME 行为
真正面向用户的效果（CJK 输入法的候选/合成文本渲染在光标处而非行首）是交互式终端行为，需要真实终端 + 输入法，因此我没有实跑它。这里通过以下方式覆盖：恢复的单元测试（BaseTextInput 光标逻辑、InputPrompt prefixWidth）、暴露光标 API 的有效 ink 补丁，以及"这是对已审阅且已合并的 #4652 的忠实恢复"这一事实。
关于合并状态
BLOCKED 看起来是一条 @wenshao 于 2026-06-11 留下的、已过时的 CHANGES_REQUESTED 评审（空正文），它早于这次 rebase/冲突解决以及 bot 之后的 approve。把那条评审 dismiss 掉或转为 approve 即可解锁。
结论
干净的恢复：能构建、类型检查、lint 通过，恢复的组件测试通过，ink 补丁有效，prefixWidth 重命名 bug 已修。被 flag 的 14 个失败确认为与本 PR 无关的既有 flaky vi.waitFor 超时。✅ 可以合并——为 flaky 的 auth 测试重跑 CI，并 dismiss 那条过时的 change-request。（IME 渲染本身按 PR 所述，最好用 CJK 输入法做一次手动确认。）
维护者 @wenshao 验证：三方合并构建 + BaseTextInput/InputPrompt 套件（3/160）+ ink 补丁有效性（reverse-check + resolve）+ 对 flag 失败的排查（AuthDialog flaky vi.waitFor 超时：多次运行 13/11/10；memoryDiagnostics 12/12）。环境：Darwin arm64，Node v22.22.2。PR head 55a57f5。
6 participants
Summary
Restores PR #4652 (IME physical cursor positioning) which was silently reverted as part of PR #4779. See issue #4987 for context — the revert was my mistake while resolving a merge conflict, and a merged feature should not be rolled back inside an unrelated PR without explanation.
What's restored
Two cherry-picks, both with the original authors preserved:
51cafe433— original PR feat(input): move physical cursor to visual cursor for IME input #4652 (@zzhenyao): addsBaseTextInputphysical-cursor positioning viaaddLayoutListener+CursorContext, theprefixWidthplumbing throughInputPrompt/AgentComposer, and thepatches/ink+7.0.3.patchthat exposesink/domandink/components/CursorContext(plus thepatch-packagepostinstall step).370e480b8— cherry-pick ofa19eec771by@LaZzyMan: fixes the danglingshowYoloStylingreference inprefixWidthafter PR fix(ui): distinguish auto approval mode indicators #4600 renamed it toapprovalModePromptStyle. This fix existed on a side branch but never landed on main — it's needed here because feat(stats): add interactive /stats dashboard with cross-session tracking #4779's revert hid the original bug.Verification
npm run buildandnpm run typecheckpass cleanly across all workspaces.npm run preflight(1 inmemoryDiagnostics.test.ts, 13 inAuthDialog.test.tsxwithvi.waitFortimeouts). These touch auth dialog rendering and heap-snapshot cleanup — unrelated to anything in this PR. Likely pre-existing onmain; please confirm during review.Test plan
main.Closes #4987.