fix(vscode-companion): don't override cursorPosition=0 to text.length - #2971
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] packages/vscode-ide-companion/src/webview/hooks/useCompletionTrigger.ts:270-271: the container-node path still uses cursorPosition = offset || text.length;, so a legitimate offset === 0 is still rewritten to text.length. That means the original bug can still reproduce when the selection starts on the contentEditable container boundary. Suggested fix: preserve 0 as a valid offset and only fall back to text.length when the offset cannot actually be computed.
— gpt-5.4 via Qwen Code /review
| cursorPosition === 0 && text.length > 0 ? text.length : cursorPosition; | ||
|
|
||
| const textBeforeCursor = text.substring(0, effectiveCursorPosition); | ||
| const textBeforeCursor = text.substring(0, cursorPosition); |
There was a problem hiding this comment.
[Critical] This patch removes effectiveCursorPosition, but the query extraction below still references it (text.substring(triggerPos + 1, effectiveCursorPosition)). The package now fails TypeScript compilation.
— gpt-5.4 via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: REQUEST_CHANGES (1 critical finding)
This PR correctly identifies and fixes the bug where cursorPosition === 0 was incorrectly overridden to text.length. The analysis in the PR description is accurate -- the walker already handles the "cursor not found" fallback, so the override was redundant and harmful.
However, the fix is incomplete: it removes the definition of effectiveCursorPosition but leaves a second reference to it on line 322, which will cause a TypeScript compilation error.
See inline comment for details.
| cursorPosition === 0 && text.length > 0 ? text.length : cursorPosition; | ||
|
|
||
| const textBeforeCursor = text.substring(0, effectiveCursorPosition); | ||
| const textBeforeCursor = text.substring(0, cursorPosition); |
There was a problem hiding this comment.
CRITICAL: Build-breaking -- effectiveCursorPosition is still referenced below
This change correctly removes the effectiveCursorPosition definition and updates textBeforeCursor to use cursorPosition directly. However, effectiveCursorPosition is still referenced on line 322:
const query = text.substring(triggerPos + 1, effectiveCursorPosition);This will cause a TypeScript compilation error (Cannot find name 'effectiveCursorPosition') and break the build.
The fix should also update line 322 to use cursorPosition:
| const textBeforeCursor = text.substring(0, cursorPosition); | |
| const query = text.substring(triggerPos + 1, cursorPosition); |
Additional note: This PR will have a merge conflict with current main, which has since wrapped the effectiveCursorPosition definition in Math.min(...) (to clamp against text.length for zero-width space handling) and added stripZeroWidthSpaces / shouldAllowCompletionQuery imports. A rebase onto main will be needed, at which point the Math.min clamp should also be evaluated -- the original cursorPosition === 0 override was the wrong fix, but the Math.min clamp for DOM offset vs stripped-text length may still be warranted independently.
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: COMMENT (no new findings)
This review re-examined PR #2971 at commit e3ecbc49.
Analysis
The fix correctly identifies and removes the erroneous effectiveCursorPosition override that rewrote a legitimate cursorPosition === 0 to text.length. The reasoning in the PR description is sound: the walker above already handles the "cursor not found" case, so cursorPosition === 0 unambiguously means the cursor is genuinely at position 0.
Already-discussed issue (no new comment posted)
The critical build-breaking issue — effectiveCursorPosition is removed at its definition site but still referenced on line 322 (text.substring(triggerPos + 1, effectiveCursorPosition)) — has already been flagged by both @wenshao and @DragonnZhang in prior review comments. No new inline comment is posted to avoid noise.
The fix for this dangling reference is straightforward: replace effectiveCursorPosition with cursorPosition on line 322.
CI Status
CI is failing on the Lint check. This should be addressed alongside the build-breaking fix above.
— qwen3-coder via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: COMMENT — The fix is logically correct but has two issues that should be addressed before merge.
What's good
The bug identification is spot-on. When cursorPosition === 0 and text is non-empty, the old code set effectiveCursorPosition = text.length, making textBeforeCursor span the entire text. This caused @// trigger detection to fire on content after the cursor — clearly wrong. The fix (removing the override) is correct since the walker's fallback already handles the "node not found" case.
Issue 1: Same bug exists on the element-node path (incomplete fix)
Line 270 has the same bug via JavaScript falsy evaluation:
cursorPosition = offset || text.length;When the cursor is at position 0 inside the container element (not a text node), offset is 0, which is falsy, so it falls through to text.length — reproducing the exact same incorrect behavior this PR fixes, just on a different code path.
Suggested fix:
cursorPosition = childIndex > 0 || inputElement.childNodes.length > 0 ? offset : text.length;Issue 2: Merge conflict with main
This PR no longer applies cleanly to main. Commit 04afc610e (fix: slash command completion not triggering after message submit) modified the same lines, wrapping effectiveCursorPosition with Math.min(...). A rebase onto current main is needed.
CI: Lint check is failing
The Lint CI check is failing. This may be related to the stale base branch.
|
@chinesepowered heads up — this PR has merge conflicts with
Are you still planning to push this forward? If so, please merge 中文@chinesepowered 提个醒 —— 这个 PR 和
请问还打算继续推进吗?如果是,麻烦 merge 最新 |
|
@chinesepowered Thanks for the cursor-position fix. Two things are blocking it:
Once rebuilt on current 中文说明@chinesepowered 感谢这个光标位置的修复。有两个卡点:
在当前 |
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. address-summary.mdMerge Conflict Resolution Summary — PR #2971Branch
Conflicted File
What ConflictedThe PR (HEAD) and main both modified the same block that computes PR side (HEAD)Removed the Main side (origin/main)Commit ResolutionCombined both intents:
Final code:// Clamp to text.length because the DOM cursor offset may exceed the
// stripped text length (e.g. after removing a leading zero-width space).
const clampedCursorPosition = Math.min(cursorPosition, text.length);
const textBeforeCursor = text.substring(0, clampedCursorPosition);Commit
Check the workflow run for full logs. |
e3ecbc4 to
790d594
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Rebuilt on current
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x).
The fix is logically correct — both the element-node path conditional and the removal of the cursorPosition===0 override properly preserve a legitimate offset of 0. Build passes locally. LGTM once CI is green. ✅
— qwen3.7-max via Qwen Code /review
|
Heads up for reviewers: the failing |
A cursor genuinely at position 0 was being rewritten to text.length in two places in useCompletionTrigger, so @// trigger detection scanned text after the cursor: - the container-node path used `offset || text.length`, so a valid offset of 0 fell through to text.length - the effectiveCursorPosition computation forced `cursorPosition === 0` to text.length Both paths now preserve a legitimate 0 while keeping the Math.min clamp that guards against the DOM offset exceeding the zero-width-space-stripped text length.
790d594 to
e95afd2
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No critical issues found. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/vscode-ide-companion/src/webview/hooks/useCompletionTrigger.ts |
No test coverage for the hook. Other hooks in the same directory (useWebViewMessages, useMessageSubmit, useImage) all have dedicated test files. The hook has 5 distinct cursor-position branches with no regression guard. |
Add useCompletionTrigger.test.ts covering: cursor at position 0 with children (the bug), empty container fallback, text-node walker found vs. not-found, Math.min clamp. |
packages/vscode-ide-companion/src/webview/App.tsx:961,1089 |
Same offset || text.length falsy-zero bug left unfixed in two other locations (slash-command trigger removal and cursor position calculation). Both would incorrectly rewrite a legitimate offset === 0 to text.length. |
Apply the same conditional pattern (childNodes.length > 0 ? offset : text.length) or file a follow-up issue. |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
|
Update for reviewers on the red check: the earlier
This PR's only change is |
Verification report — real-browser A/B against
|
after Backspace → then Enter |
main@88bd001 |
PR #2971 |
|---|---|---|
| completion menu | open — trigger=@ query="src" |
closed |
| messages submitted | 0 | 1 |
| completions inserted | src/index.ts |
none |
This is the part the PR undersells. CompletionMenu installs a document-level keydown listener that preventDefault()s Enter and calls onSelect, and InputForm.handleKeyDown deliberately yields Enter to it whenever completionActive. So the phantom menu does not merely look wrong — it swallows Enter, silently inserting a file path instead of sending your message.
The same thing happens with slash commands (x/help → Backspace → /help, caret 0 → main opens /help) and when deleting a selection back to the start (hello @src, Home, Shift+→×6, Delete). In all three, main opens a menu and the PR does not.
The fix discriminates on caret position, not on text — same @src, two caret indices:
2. ⚠️ The repro in the PR description does not reproduce
Repro: … type
hello @someoneinto the input, then press Home to put the cursor at position 0. Before the fix, autocomplete would incorrectly trigger on the@someonefurther right in the text.
It does not. The hook's only listener is:
inputElement.addEventListener('input', handleInput);and Home fires no input event. Measured directly: inputEventsFiredByHome = 0. handleInput never runs, nothing is re-evaluated, and main and this PR behave identically — the menu that opened while you typed @someone simply stays open on both.
The actual precondition is an input event that lands the caret at index 0: backspacing the first character, or deleting a selection that reaches the start of the text.
Please update the description and the Reviewer Test Plan. As written, a reviewer follows the steps, sees no difference between the two builds, and reasonably concludes the PR is a no-op. (The fix itself is fine — only the stated repro is wrong.)
3. The [Critical] container-node path — correctly fixed, but not reachable from the keyboard
The new childIndex > 0 || inputElement.childNodes.length > 0 ? offset : text.length does behave correctly when the container path is taken with a legitimate offset === 0:
However, I could not reach that branch with non-empty text through real keyboard input. Across five interaction batteries (select-all + Backspace, select-all + Delete, multiline select-all + Delete, backspace-first-char, delete-across-newline), range.startContainer === inputElement fired on 3 of 5, and in every case with:
startOffset: 0, childNodes: 1, childKinds: ["<BR>"], rawTextLen: 0
i.e. an empty composer, where main and this PR agree (0 || 0 === 0). In contentEditable="plaintext-only" Chromium keeps the caret inside a text node otherwise. Fig 3 above required placing the Range on the container node programmatically.
So this half of the change is defense-in-depth: correct, worth keeping, but it is not what fixes the user-visible bug. That is entirely the effectiveCursorPosition removal on the text-node path. Good to know for anyone weighing risk.
4. No regressions
Every one of these produced byte-identical hook state on main and on the PR:
| scenario | result (both builds) |
|---|---|
@src at start of line |
open, @, "src", 3 file items |
hello /he |
open, /, "he", /help |
see @src/index (path-like @ query) |
open, @, "src/index" |
@src hello (space in @ query) |
closed |
after-submit U+200B placeholder, then type /hel |
open, /, "hel" |
U+200B placeholder, caret before it, type /hel |
open, /, "hel" |
The last two matter: they are the #3609 regression. The Math.min(…, text.length) clamp is not just preserved, it is actually exercised — raw DOM text is "/hel" with startOffset: 5, while the stripped text.length is 4. Merging the two intents was done right.
5. Repo gates
eslint packages/vscode-ide-companion/src/webview/hooks/useCompletionTrigger.ts→ cleanprettier --checkon the same file → cleanvitest run src/webviewinpackages/vscode-ide-companion→ 26 files / 222 tests passed- No dangling
effectiveCursorPositionreferences remain anywhere inpackages/(the earlier build break is resolved) - CI red check verified as pre-existing.
Test (ubuntu-latest, Node 22.x)fails identically onmain— e.g. runs 29051234182 (de9452357) and 29023439987 (9bf7fc32b), same job. Not attributable to this PR. @chinesepowered's assessment is accurate.
Follow-ups (non-blocking)
a) This change has zero test coverage. There is no useCompletionTrigger.test.ts, and the only test file that mentions the hook — App.test.tsx — vi.mocks it away entirely. I proved the gap: with the buggy main version of the hook restored, the same 26 test files / 222 tests still pass. Nothing guards this, and nothing will stop it regressing.
A jsdom test is feasible despite the Selection dependency — build the DOM, place a Range on the text node at offset 0, dispatch new InputEvent('input'), assert the menu stays closed. That is exactly how Fig 3 was produced. Worth adding here or as an immediate follow-up.
b) The new ternary is redundant — it is exactly equivalent to cursorPosition = offset;:
range.setStart(el, k)throwsIndexSizeErrorwhenk > childNodes.length(verified in Chromium), sochildIndex > 0implieschildNodes.length > 0. The first disjunct can never change the result.- When
childNodes.length === 0,textContentis'', sotext.length === 0— andoffsetis0too, because the loop never runs. Both branches return0. Confirmed empirically: the empty-text container probe gives identical state on both builds.
Not worth blocking on, but the comment above it describes a fallback that cannot fire.
c) App.tsx:961 and App.tsx:1089 still carry the identical offset || text.length falsy-zero pattern (as @qwen-code-ci-bot noted). Same latent bug, same low reachability per §3. A follow-up issue is the right call — no need to grow this PR.
Verdict
LGTM — merge, once the description's repro steps are corrected. The behavioural change is real, correct, narrowly scoped, regression-free against the #3609 clamp, and it fixes a genuinely annoying failure mode (fix a typo at the start of a message that mentions a file → your Enter stops sending).
中文版
验证报告 —— 与 main 的真实浏览器 A/B 对比
我没有只看 diff，而是在真实浏览器里端到端跑了这个 PR。修复是正确的，而且它修掉的这个 bug 比描述里说的更严重。 建议合入 —— 但 PR 描述里给出的复现步骤实际上复现不出来，合入前应当更正（见第 2 节）。
本条评论取代我之前的 CHANGES_REQUESTED：container-node 路径已经处理了（见第 3 节）。
验证方法（测试台，点击展开）
这个 bug 存在于浏览器对 contentEditable="plaintext-only" 元素的 Selection/Range 语义中，jsdom 看不见它。因此：
- 用 esbuild 把真实的
useCompletionTriggerhook 打包了两份 —— 一份来自main@88bd001，一份来自本 PR（f943885）—— 挂载到一个与packages/webui/src/components/layout/InputForm.tsx结构一致的输入框上（相同的contentEditable="plaintext-only"div、相同的Enter分支、相同的completionActive判定）。 - 渲染的是
@qwen-code/webui里真实的CompletionMenu，因此它挂在document上的按键处理也在生效。 - 每个场景都由 Playwright 驱动真实 Chromium 键盘事件，并且两份构建都跑一遍。两份构建之间唯一的差异，就是本 PR 改动的那一个文件。
- 一个独立的探针监听器记录每次
input事件时window.getSelection()的真实返回值。hook 本身未作任何修改。
A/B 有效性的自检：
bundle_base.js : effectiveCursorPosition ×3, clampedCursorPosition ×0
bundle_pr.js : effectiveCursorPosition ×0, clampedCursorPosition ×3
1. Bug 真实存在 —— 而且它会吞掉你的回车键
复现步骤（一个很自然的场景）：输入 sSee @src，按 Home、→、Backspace —— 也就是把一条 @ 提及了文件的消息开头的错字删掉。
上面 Fig 1 两侧最终的文本完全相同（"See @src"），光标下标也完全相同（0）。在 main 上，尽管光标前面什么都没有，@ 补全菜单却是打开的。
此时按 Enter 想把消息发出去：
Backspace 之后再按 Enter |
main@88bd001 |
PR #2971 |
|---|---|---|
| 补全菜单 | 打开 —— trigger=@ query="src" |
关闭 |
| 实际发送的消息数 | 0 | 1 |
| 被插入的补全项 | src/index.ts |
无 |
这一点是 PR 描述低估了的。CompletionMenu 在 document 上注册了 keydown 监听，会对 Enter 调用 preventDefault() 并触发 onSelect；而 InputForm.handleKeyDown 在 completionActive 时会主动把 Enter 让给它。所以这个幽灵菜单不只是看起来不对 —— 它会吞掉 Enter，悄悄插入一个文件路径，而不是把消息发出去。
斜杠命令同理（x/help → Backspace → /help，光标在 0 → main 弹出 /help），删除一段一直选到开头的文本也同理（hello @src，Home，Shift+→×6，Delete）。这三种情况下，main 都会弹出菜单，本 PR 都不会。
Fig 2 说明修复的判据是光标位置而非文本内容：同样是 @src，两个不同的光标下标。
2. ⚠️ PR 描述里的复现步骤复现不出来
Repro: … 输入
hello @someone，然后按 Home 把光标移到位置 0。修复前，自动补全会错误地在光标右侧的@someone上触发。
不会。这个 hook 唯一的监听是：
inputElement.addEventListener('input', handleInput);而按 Home 不会触发 input 事件。实测：inputEventsFiredByHome = 0。handleInput 根本不会执行，什么都不会重新计算，main 与本 PR 的行为完全一致 —— 你输入 @someone 时打开的那个菜单，在两边都只是继续开着而已。
真正的前提条件是一次让光标落到下标 0 的 input 事件：退格删掉第一个字符，或者删除一段一直选到文本开头的选区。
请更新描述和 Reviewer Test Plan。按现在的写法，reviewer 照着做会发现两个版本毫无区别，从而合理地认为这个 PR 什么也没做。（修复本身没问题，只是复现步骤写错了。）
3. [Critical] container-node 路径 —— 修得对，但键盘操作到不了
新的 childIndex > 0 || inputElement.childNodes.length > 0 ? offset : text.length 在 container 路径确实取到合法 offset === 0 时行为是正确的（见 Fig 3）。
但是，我无法通过真实键盘输入、在文本非空的情况下走到这个分支。在五组交互中（全选 + Backspace、全选 + Delete、多行全选 + Delete、退格删首字符、跨换行删到开头），range.startContainer === inputElement 命中了 5 次中的 3 次，且每次都是：
startOffset: 0, childNodes: 1, childKinds: ["<BR>"], rawTextLen: 0
也就是空输入框，此时 main 与本 PR 结果一致（0 || 0 === 0）。在 contentEditable="plaintext-only" 下，其余情况 Chromium 都把光标保持在文本节点内。Fig 3 是通过程序化地把 Range 设到 container 节点上才构造出来的。
所以这一半改动属于纵深防御：正确、值得保留，但它并不是修复用户可见 bug 的那一半。真正修好的是文本节点路径上 effectiveCursorPosition 的移除。这一点对评估风险有帮助。
4. 无回归
以下每一项在 main 和本 PR 上产生的 hook 状态完全一致：
| 场景 | 结果（两个版本相同） |
|---|---|
行首 @src |
打开，@，"src"，3 个文件项 |
hello /he |
打开，/，"he"，/help |
see @src/index（路径形式的 @ 查询） |
打开，@，"src/index" |
@src hello（@ 查询里有空格） |
关闭 |
提交后的 U+200B 占位符，再输入 /hel |
打开，/，"hel" |
U+200B 占位符，光标在其之前，输入 /hel |
打开，/，"hel" |
最后两项很关键，它们正是 #3609 的回归点。Math.min(…, text.length) 这个 clamp 不只是被保留了，而且确实被触发到了 —— DOM 原始文本是 "/hel"、startOffset: 5，而剥离后的 text.length 是 4。两边意图的合并处理得是对的。
5. 仓库检查项
eslint改动文件 → 通过prettier --check同一文件 → 通过packages/vscode-ide-companion下vitest run src/webview→ 26 个文件 / 222 个测试全部通过packages/中不再有任何残留的effectiveCursorPosition引用（此前的编译失败已解决）- CI 红叉已确认为既有问题。
Test (ubuntu-latest, Node 22.x)在main上同样失败 —— 例如 29051234182（de9452357）与 29023439987（9bf7fc32b），同一个 job。与本 PR 无关，@chinesepowered 的判断是准确的。
后续项（不阻塞合入）
a) 这处改动没有任何测试覆盖。 不存在 useCompletionTrigger.test.ts，而唯一提到这个 hook 的测试文件 App.test.tsx 直接用 vi.mock 把它整个替换掉了。我验证了这个缺口：把 hook 换回 main 上有 bug 的版本，同样的 26 个测试文件 / 222 个测试依然全绿。没有任何东西守着它，也没有任何东西能阻止它再次退化。
尽管依赖 Selection，jsdom 测试仍然可行 —— 构造 DOM，在文本节点 offset 0 处放一个 Range，dispatchEvent(new InputEvent('input'))，断言菜单保持关闭。Fig 3 正是这么做出来的。建议在本 PR 或紧接的后续 PR 中补上。
b) 新的三元表达式是冗余的 —— 它与 cursorPosition = offset; 完全等价：
- 当
k > childNodes.length时range.setStart(el, k)会抛IndexSizeError（已在 Chromium 中验证），因此childIndex > 0蕴含childNodes.length > 0。第一个析取项永远不会改变结果。 - 当
childNodes.length === 0时，textContent为''，于是text.length === 0；而offset也是0，因为循环一次都不会执行。两个分支都返回0。已实测确认：空文本的 container 探针在两个版本上状态完全一致。
不值得因此阻塞，但它上面那段注释描述的 fallback 实际上永远不会发生。
c) App.tsx:961 与 App.tsx:1089 仍然带着一模一样的 offset || text.length falsy-zero 写法（正如 @qwen-code-ci-bot 指出的）。同样的潜在 bug，按第 3 节的结论同样难以触达。开一个后续 issue 是合适的做法，没必要把本 PR 撑大。
结论
LGTM —— 建议合入，前提是更正描述中的复现步骤。行为变更是真实的、正确的、范围收敛的，对 #3609 的 clamp 无回归，并且修掉了一个确实很烦人的失效场景（把一条提及文件的消息开头的错字删掉 → 你的 Enter 就发不出消息了）。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @chinesepowered! The fix itself looks well-reasoned — clear reproduction, root cause analysis, and a focused one-file change. But the PR body doesn't follow the PR template, which is required for all contributions.
Missing template sections:
## What this PR does— the "TLDR" section covers this, but please use the template heading## Why it's needed— the "Dive Deeper" section covers the technical detail, but the motivation heading is required- Under
## Reviewer Test Plan:### How to verify,### Evidence (Before & After),### Tested onsub-headings ## Risk & Scope## Linked Issues<details><summary>中文说明</summary>block (Chinese translation of the PR body)
The content is all there — it just needs to be restructured to match the template so reviewers can find information consistently. Please reformat the PR body to use the template headings. No code changes needed.
中文说明
感谢 PR，@chinesepowered！修复本身很有道理——复现清晰、根因分析到位、改动集中在一个文件。但 PR 描述没有遵循 PR 模板，这是所有贡献都需要遵守的。
缺少的模板章节：
## What this PR does— "TLDR" 部分覆盖了这部分内容，但请使用模板标题## Why it's needed— "Dive Deeper" 部分覆盖了技术细节，但动机标题是必需的## Reviewer Test Plan下的子标题：### How to verify、### Evidence (Before & After)、### Tested on## Risk & Scope## Linked Issues<details><summary>中文说明</summary>区块（PR 描述的中文翻译）
内容都有了——只需要按模板重新组织一下结构，方便 reviewer 一致地找到信息。请调整 PR 描述使用模板标题。不需要改代码。
— Qwen Code · qwen3.7-max
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
|
Thanks @wenshao for the review, and thanks for the triage note. I've reformatted the PR description to follow the required template — all the flagged sections are now present:
No code changes — the fix is unchanged from your approval. Could a maintainer re-run triage ( |
|
@qwen-code /triage |
|
Thanks for the PR, @chinesepowered! Template looks good ✓ — all required sections present (the earlier template concern has been addressed). Problem: This is a real, code-evidenced bug. The override Direction: Aligned — fixing autocomplete misfiring when the cursor is at position 0 of non-empty text is a clear user-facing improvement for the VS Code companion. Size: Not applicable — Approach: The scope feels exactly right. One focused fix, no drive-by refactors, no scope creep. The element-node path adjustment (preserving offset=0 when childNodes are present) is a thoughtful companion fix. Moving on to code review. 🔍 中文说明感谢 PR，@chinesepowered！ 模板完整 ✓ — 所有必需章节已到位（之前的模板问题已修复）。 问题：这是一个真实的、代码可证的 bug。 方向：对齐 — 修复光标在非空文本起始位置时自动补全误触发，对 VS Code 伴随插件的用户体验是明确的改善。 规模：不适用 — 方案：范围恰到好处。一个专注的修复，没有顺手重构，没有范围蔓延。元素节点路径的调整（当 childNodes 存在时保留 offset=0）是一个周到的配套修复。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
2a. Code ReviewIndependent proposal (before reading the diff): The bug is in the downstream override:
Comparison with the PR: The PR matches my proposal exactly, with a nice rename ( Element-node path ( The old
Text-node path (removal of override): The walker's Downstream references: All three uses of No critical issues. No AGENTS.md violations. The change is minimal, focused, and follows project conventions. 2b. Real-Scenario TestingNot applicable — tmux testing cannot be performed. This change is in
The tmux-based CLI testing workflow drives What was verified:
中文说明2a. 代码审查独立方案（阅读 diff 之前）： bug 在下游改写：
与 PR 对比： PR 与我的方案完全一致，并且做了一个漂亮的重命名（ 元素节点路径（ 旧的
文本节点路径（移除改写）： walker 的 下游引用： 无严重问题。 无 AGENTS.md 违规。改动最小化、专注，遵循项目规范。 2b. 真实场景测试不适用 — 无法进行 tmux 测试。 此改动位于
tmux CLI 测试流程驱动 已验证的内容：
— Qwen Code · qwen3.7-max |
|
This is a clean, well-reasoned bugfix. Let me walk through why I'm confident. The bug is real and code-provable. The override The fix is minimal and correct. Two changes, both addressing the same underlying pattern (JavaScript treating 0 as falsy in cursor-position logic):
No unrelated changes. No scope creep. No drive-by refactors. My independent proposal matched exactly. Before reading the diff, I would have made the same two changes. The PR adds a nice rename ( CI passes. The tmux-based interactive testing can't reach a VS Code webview, but the logic is straightforward enough that code review plus CI gives high confidence. The prior CHANGES_REQUESTED review (template formatting) has been addressed — the PR body now follows the template completely. Approving. ✅ 中文说明这是一个干净、合理的 bug 修复。让我说明为什么我有信心。 bug 是真实的且可以从代码证明。 改写 修复最小化且正确。 两处改动，都针对同一个底层模式（JavaScript 在光标位置逻辑中将 0 视为 falsy）：
没有无关改动。没有范围蔓延。没有顺手重构。 我的独立方案完全一致。 阅读 diff 之前，我会做出同样的两处修改。PR 还加了一个漂亮的重命名（ CI 通过。 基于 tmux 的交互式测试无法触及 VS Code webview，但逻辑足够直观，代码审查加 CI 可以提供高度信心。 之前的 CHANGES_REQUESTED 评审（模板格式）已被解决——PR 描述现在完全遵循模板。 批准。✅ — 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, looks ready to ship. ✅
6 participants
What this PR does
Fixes the VS Code companion autocomplete misfiring when the cursor sits at position 0 of non-empty input. The completion-trigger logic was rewriting a legitimate cursor position of
0into the full text length whenever the input was non-empty, which made trigger detection scan the entire input — including text after the cursor — for@and/characters. This PR removes that override so trigger detection only ever considers the text genuinely before the cursor.Why it's needed
When a user placed the cursor at the start of existing text (for example by pressing Home), the companion surfaced stale, incorrect autocomplete suggestions derived from content that appeared after the cursor rather than before it.
The walker that resolves the cursor's text node already handles the "cursor not found" case by setting
cursorPosition = text.length. So by the time the old override ran,cursorPosition === 0was unambiguous: it meant the walker did find the cursor and resolved its offset to 0 (cursor genuinely at the start of non-empty text). Rewriting0 → text.lengththen madetextBeforeCursorspan the whole string, so any@or/anywhere in the input falsely registered as a trigger.Before (the offending override):
After (trust the position the walker already computed):
The element-node path uses its own separate
offset || text.lengthfallback and is intentionally left unchanged; this PR only removes the text-node path's downstream override.Reviewer Test Plan
How to verify
hello @someoneinto the input.@autocomplete appears — before the fix it incorrectly triggered on the@someoneto the right of the cursor.@at position 0 (cursor moves to position 1) and confirm the@autocomplete now appears correctly.@//completions elsewhere in the text continue to work.Evidence (Before & After)
This is a logic change in trigger detection inside the webview (which needs a running VS Code extension host), so the observable behavior is described rather than screenshotted:
hello @someone→ autocomplete fires on the@someonelocated after the cursor (false trigger from stale content).Tested on
macOS: verified locally via typecheck/build plus a trace of the trigger-detection logic. Windows/Linux: not run locally; covered by CI, which is green across all three.
Environment (optional)
Verified on macOS with the package typecheck/build; interactive webview QA was not performed as it requires the VS Code extension host. No unit-test surface exists for this path beyond the compile check.
Risk & Scope
cursorPositionthe walker already resolved correctly.Linked Issues
None.
中文说明
本 PR 的作用
修复 VS Code 伴随插件在光标位于非空输入起始位置（position 0）时自动补全误触发的问题。补全触发逻辑此前会在输入非空时把合法的光标位置
0改写为文本总长度，导致触发检测扫描整段输入（包括光标之后的文本）来查找@和/字符。本 PR 移除该改写，使触发检测只考虑光标之前的文本。为什么需要
当用户把光标放在已有文本的起始处（例如按 Home 键）时，伴随插件会基于光标之后而非之前的内容，弹出过时且错误的自动补全建议。
解析光标所在文本节点的遍历逻辑本身已经处理了“未找到光标”的情况：此时会设置
cursorPosition = text.length。因此当旧的改写逻辑执行时，cursorPosition === 0的含义是明确的——它表示遍历确实找到了光标并将其偏移解析为 0（光标真实地位于非空文本的开头）。此时把0 → text.length改写，会使textBeforeCursor覆盖整段字符串，于是输入中任意位置的@或/都会被错误地识别为触发。修改前（有问题的改写）：
修改后（信任遍历已经计算好的位置）：
元素节点（element-node）路径使用它自己独立的
offset || text.length回退逻辑，本 PR 有意保持其不变；仅移除文本节点路径下游的这段改写。复核测试计划
如何验证
hello @someone。@自动补全——修复前它会错误地在光标右侧的@someone上触发。@（光标移动到 position 1），确认此时@自动补全能正确出现。@//补全依然工作。证据（修复前后对比）
这是 webview 内部触发检测的逻辑改动（需要运行中的 VS Code 扩展宿主环境），因此以行为描述代替截图：
hello @someone的 position 0 → 自动补全在光标之后的@someone上触发（由过时内容导致的误触发）。测试环境
macOS：本地通过 typecheck/build 以及触发检测逻辑的走查进行验证。Windows/Linux：未在本地运行，由 CI 覆盖，三平台均为绿色。
运行环境（可选）
在 macOS 上通过该包的 typecheck/build 验证；未进行交互式 webview 手动测试，因为它需要 VS Code 扩展宿主环境。除编译检查外，该路径没有现成的单元测试覆盖面。
风险与影响范围
cursorPosition。关联 Issue
无。