feat(cli): Open OSC 8 links with a plain click in VP mode - #10979
Conversation
Virtual viewport turns on SGR mouse tracking, which makes the terminal stop handling OSC 8 hyperlink clicks — so links could not be opened at all in that mode. Read the URL from the composited frame cell under the pointer and open it on a plain left click, deferring the open by the multi-click window so drag-to-select and double/triple-click word/line selection keep their priority. A right-click raises an in-app context menu (Open Link / Copy Link Address / Copy Selection). Gating is unchanged for other cohorts: non-VP and mouse-tracking-off users keep native terminal link handling (the controller is inert outside VP+mouse), and the OpenTUI renderer has its own link path and is not wired into this one.
E2E test report (re-run against committed head `63735d9f6f`)Harness: `integration-tests/terminal-capture/vp-link-click.ts` — drives a real SGR mouse click at the link cell against a fake model server, with a `BROWSER` wrapper recording the opened URL. Result: `pass: true`, `linkOpened: true` (exit 0). The `BROWSER` wrapper recorded `https://example.com/\` from the plain left click. Flow exercised (5 screenshots captured):
Other gates on the same head:
Environment: macOS, bundled `dist/cli.js` (`npm run build && npm run bundle`). Windows/Linux are covered by CI, not verified locally. |
|
Thanks for the PR! Template looks good ✓ — every section is filled in, and the design doc under Problem: observed, and independently corroborated by the repo itself. This isn't a hypothetical — Direction: aligned. The reference agent's CHANGELOG treats this as an active area rather than a fringe one — mouse click support in menus, clickable hyperlinks in rendered markdown, right-click context menus, and a dedicated Size: production logic 1007 lines, tests 1334, integration harness 238, docs 436, generated schema 4. The only core-path file touched is Approach: the implementation looks well-reasoned, and it reuses rather than reinvents — The stated rationale for a plain click is that "modifier state never reaches the app through SGR mouse reports." That holds for Cmd — Super isn't in the protocol — and the code comment in That matters because of what the plain click costs. Shift is already taken — it's the documented "hand the mouse back to the terminal" bypass — but Ctrl and Alt are free. A Ctrl-click opener would restore link opening at a fraction of the behavioural cost, whereas a plain click changes what an ordinary left click does on every link in every transcript, and needs the multi-click window, the same-cell drag guard and the delayed-open timer to keep selection gestures winning. Those guards look correct to me, but they're complexity that only exists because the opener is unmodified. The reference agent reached for Cmd/Ctrl-click for its clickable file attachments. Was Ctrl-click considered and rejected for a reason that isn't in the description — a terminal that swallows Ctrl+click, or a collision I haven't spotted? Related, and also a question rather than a blocker: the opt-out here is Risk: no elevated signals — nothing in the diff matches the revert-correlated path list. Two things I'll be reading carefully in the code pass: Esc ownership moves to the overlay while the menu is open, with Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ —— 每个章节都填写了, 问题: 已观测到的问题,而且仓库自身就能佐证。这不是假设—— 方向: 对齐。参考 agent 的 CHANGELOG 把这块当作活跃领域而非边缘需求——菜单的鼠标点击支持、渲染 markdown 中的可点击超链接、右键上下文菜单,以及专门的 规模: 生产代码 1007 行,测试 1334 行,集成测试脚手架 238 行,文档 436 行,生成的 schema 4 行。唯一触及核心路径的文件是 方案: 实现思路严谨,而且是复用而非重造——从 selection 栈里抽出 「以单击作为触发方式」的理由是「修饰键状态从不会通过 SGR 鼠标上报传到应用」。这对 Cmd 成立——Super 不在协议里—— 这一点之所以重要,在于单击的代价。Shift 已经被占用了——它是文档里「把鼠标交还给终端」的旁路——但 Ctrl 和 Alt 是空闲的。用 Ctrl+单击作为触发方式,能以小得多的行为代价恢复链接点击;而单击会改变所有 transcript 里每一个链接上普通左键单击的含义,并且需要多击窗口、同格拖拽守卫和延迟打开计时器来保证选择手势优先。这些守卫在我看来是正确的,但它们的存在本身就是因为触发方式不带修饰键。参考 agent 对可点击的文件附件用的就是 Cmd/Ctrl+单击。是否考虑过 Ctrl+单击并因为某个描述里没写的原因否决了它——比如某个终端会吞掉 Ctrl+单击,或者有我还没发现的冲突? 相关的一点,同样是问题而非阻碍:这里的退出方式是 风险: 无升级信号——diff 里没有文件命中与 revert 相关的路径清单。代码审查阶段我会重点看两处:菜单打开期间 Esc 的所有权转移给了 overlay,而 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head 63735d9f.
- No review history or threads to reconcile — this pass is the full review. Author evidence (189 unit tests, the terminal-capture SGR harness with screenshots, macOS local; typecheck/lint clean) is consistent with what I checked in the code.
- Security surface: the only sinks for a model-supplied OSC 8 URL are the existing, strictly-validated
openBrowserSecurely(http(s); rejects e.g. hostlesshttps://, with the rejection path degrading to a clipboard copy) andcopyToClipboard— no raw exec/spawn of URLs, and non-http schemes are copied, never opened. - Lifecycle: the deferred open is cancelled by any new press, a scroll, and both deactivate and unmount (same teardown closes the menu); resize closes the menu; while open, pointer and keyboard have a single owner — RowMouseController, TextInputMouseController, the scroll list and the global Esc handlers all check the mirrored menu state, and the selection controller pauses without clearing so "Copy Selection" still sees what it offered. The click-vs-toggle suppression in HistoryItemDisplay and the double/triple
near-rule mirroring of the selection controller are deliberate and consistent with their comments. - Non-VP and mouse-tracking-off paths are untouched (gated on
isActive/VP), and the OpenTUI renderer has no import of the new modules. - CI facts on this head: 12 checks pass, zero assertion failures; Test/Lint/review-pr/triage still in flight on a PR opened ~2 h ago, and the single red is the
Dependency CVE auditdying on a registry503 Service Unavailablefrom the audit endpoint — the PR touches no manifest or lockfile, and the same audit passed on another PR an hour earlier. Per the channel convention the call is on the review itself; a re-run should clear that leg.
Code reviewThe design here is sound and the implementation is careful about the things that usually go wrong in this corner — deferred opens, teardown, and pointer ownership are all handled deliberately rather than accidentally. Two correctness findings, both one-line fixes, and one design question carried over from the gate. 1.
|
| File | What changed |
|---|---|
packages/cli/src/ui/context-menu/ContentMouseController.tsx |
new, 431 — the whole feature: press/release anchoring, multi-click chain, delayed open, right-click menu build, menu pointer ownership |
packages/cli/src/ui/context-menu/ContextMenuContext.tsx |
new, 164 — menu state, menuRef mirror, openMenu/closeMenu/executeIndex, onMenuChange callback for AppContainer |
packages/cli/src/ui/context-menu/ContextMenuOverlay.tsx |
new, 105 — absolute-positioned Ink overlay, arrow-key highlight, Enter executes, Esc closes, rows padded to the longest label |
packages/cli/src/ui/utils/hyperlink-at.ts |
new, 79 — OSC 8 URL extraction from a frame cell's styles, handling BEL/ST/C1 and tmux DCS wrappers |
packages/cli/src/ui/selection/use-text-selection.tsx |
70/11 — adds eventsPaused and selectionQueryRef, exports MULTI_CLICK_MS, swaps inline wide-char logic for snapWideChar |
packages/cli/src/ui/selection/selection-coords.ts |
new 22 — snapWideChar extracted so every per-cell lookup shares one implementation |
packages/cli/src/ui/components/MainContent.tsx |
26/2 — mounts the controller, threads the selection query ref, quiets ScrollableList while the menu is open |
packages/cli/src/ui/AppContainer.tsx |
21/1 — wraps App in the provider, mirrors open state into a ref, gates the global Esc and btw-item branches on it |
packages/cli/src/ui/components/InputPrompt.tsx |
new 23 — menu owns up/down/Enter/Esc; any other key dismisses it and falls through the normal pipeline |
packages/cli/src/ui/components/HistoryItemDisplay.tsx |
21/4 — think-block toggle suppressed when the click lands on a link (finding 2 above) |
packages/cli/src/ui/components/shared/RowMouseController.tsx |
8/1 — isActive gated on the menu being closed so a menu click can't select a row underneath |
packages/cli/src/ui/components/shared/TextInputMouseController.tsx |
8/1 — same gating so a menu click can't move the composer cursor |
packages/cli/src/ui/layouts/DefaultAppLayout.tsx |
new 5 — mounts the overlay last so it paints over the transcript |
packages/cli/src/config/settingsSchema.ts |
2/2 — the only core-path file: both ui.useTerminalBuffer and ui.mouseTracking descriptions rewritten |
packages/vscode-ide-companion/schemas/settings.schema.json |
2/2 — generated schema regenerated to match |
packages/cli/src/ui/context-menu/ContentMouseController.test.tsx |
new, 579 — 33 cases: drag, multi-click, scroll invalidation, scheme fallback, clipboard-failure attribution, menu lifetime |
packages/cli/src/ui/context-menu/ContextMenuContext.test.tsx |
new, 197 — state transitions, bounds-checked executeIndex, safe-outside-provider behaviour |
packages/cli/src/ui/context-menu/ContextMenuOverlay.test.tsx |
new, 167 — render-when-open, Esc closes, Enter executes, arrow navigation, absolute positioning |
packages/cli/src/ui/selection/use-text-selection.test.tsx |
186/1 — pause-vs-deactivate semantics, selection survival, query ref lifecycle |
packages/cli/src/ui/utils/hyperlink-at.test.ts |
new, 140 — every OSC 8 terminator form, tmux DCS wrapper, wide-char snapping, out-of-bounds and null frames |
packages/cli/src/ui/components/HistoryItemDisplay.test.tsx |
new 16 — toggle suppressed on a link click |
packages/cli/src/ui/components/shared/RowMouseController.test.tsx |
42/2 — quieted while the menu is open |
packages/cli/src/ui/components/MainContent.test.tsx |
new 4 — controller mounted with the expected props |
integration-tests/terminal-capture/vp-link-click.ts |
new, 238 — drives a real SGR click at the link cell and records the browser open |
docs/design/vp-native-mouse-parity.md |
new, 358 — design doc, committed under docs/design/ per AGENTS.md |
docs/users/configuration/settings.md |
34/34 — both setting rows rewritten to describe the new behaviour |
docs/users/reference/keyboard-shortcuts.md |
3/1 — the mouse section no longer tells users links are unavailable |
docs/users/support/troubleshooting.md |
3/3 — same correction |
Testing
This is an unattended CI run, so I did not build or execute anything from this PR — the evidence below is the PR's own CI, read through the API for the reviewed commit. No tmux capture: that path is local-invocation only, and on the CI path the live-behaviour signal comes from the lane named below.
Final CI results for 63735d9 (auto-updated by the triage finalize job after CI completed):
| Check | Conclusion |
|---|---|
Lint & Static (ubuntu-latest, Node 22.x) |
🚫 cancelled |
Classify PR |
✅ success |
Dependency CVE audit |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Integration Tests (no-AK, No Sandbox) |
✅ success |
OpenTUI no-flicker gate |
✅ success |
Secret scan (TruffleHog) |
✅ success |
Test (ubuntu-latest, Node 22.x) |
✅ success |
TUI parity snapshots (ink vs opentui) |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。
The one red check is transient infrastructure noise, not this PR. Dependency CVE audit failed because the npm registry's audit endpoint returned 503 Service Unavailable on POST https://registry.npmjs.org/-/npm/v1/security/audits/quick, which npm surfaces as audit endpoint returned an error. Two independent reasons to discount it: this PR changes no dependency manifest — there is no package.json or package-lock.json anywhere in the 28 files — so it cannot have altered CVE exposure; and the same check is green on main at 60161cb6. The advisories the log does list (diff GHSA-73rr-hh4g-fpgx, uuid GHSA-w5hq-g745-h8pq) are low and moderate, below this job's --audit-level=high threshold, and are standing repo condition rather than anything introduced here.
Not verified, and this is the real gap: the two unit-test lanes that would cover Windows and macOS are skipped, and that is by design rather than an accident of this PR — test_macos and test_windows in .github/workflows/ci.yml carry an if restricted to merge_group, schedule and workflow_dispatch, with the pull_request trigger deliberately removed. The PR body says "Windows/Linux are covered by CI, not verified locally", but on a pull request only the Linux lane runs; Windows and macOS coverage arrives from the nightly on main, i.e. after merge. The author's own Tested-on table marks macOS ✅ and Windows/Linux Lint & Static plus Test (ubuntu) were still in flight when I read them.
The author's E2E and unit-test results quoted in the PR body are the author's claim; I have not re-run them and am not presenting them as evidence.
Sandboxed verification would settle this, and the author has write access so both lanes are available:
@qwen-code /tmux— the central claim is a TUI surface: that a plain click opens the link, a right click raises the menu, and drag plus double/triple-click still select rather than open. Nothing in the diff or the unit suite demonstrates that against a real terminal, and the terminal-capture harness the author ran is not one of the checks CI executed here (tmux-testingandverifyare bothskipped).@qwen-code /verify— for findings 1 and 2 specifically, because both are green-suite defects: no test dispatches two inputs in one tick, and no test clicks the trailing half of a fullwidth character inside a think-block link. An A/B run against the base build would show whether the guards actually hold rather than merely pass.
中文说明
代码审查
整体设计是可靠的,实现也在这类改动最容易出错的地方——延迟打开、销毁清理、指针所有权——做了有意识的处理,而不是碰巧没出问题。两个正确性问题,都是一行可修,外加一个从闸门阶段带过来的设计问题。
1. executeIndex 在同一个输入 chunk 内可能把同一项执行两次。 它清了 React state 却没清同步镜像 menuRef.current。紧邻上方的 closeMenu 恰恰先清了镜像,注释还点名了这个隐患:「KeypressContext 可能在一个 tick 内派发整个 stdin chunk,中间没有渲染」。但对称的情形——一次 executeIndex 紧接另一次 executeIndex——没有防护,于是第二次调用仍读到旧镜像并再次触发 onSelect()。两条路径:同一 chunk 里两个 \r(overlay 的按键处理跑两次,闭包里的 menu 都非空);或在菜单项上双击(handleMouse 在函数开头捕获的 openMenuState 仍视为打开,inMenu 仍为真,于是再调一次 executeIndex)。「Open Link」就会打开浏览器两次。在 executeIndex 首行加 menuRef.current = null; 即可,与 closeMenu 保持一致。建议补一个在单次 stdin.write 里派发 \r\r 的回归测试——33 个控制器测试和 12 个 context 测试都没有覆盖同一 tick 内的双输入,所以修与不修测试都是绿的。
2. 第三处 hyperlinkAtCell 调用没有做宽字符吸附。 本 PR 新增 snapWideChar,其不变式是「点击右半格必须解析到左格,任何逐格查询都是如此」,并在两处 mapPoint 都用了它。但 HistoryItemDisplay.tsx 的 ClickableThinkMessage 直接把未经吸附的 layoutRowForEvent 结果传给 hyperlinkAtCell。于是点击全角字符的右半格时,这里查不到 URL 而调用 onToggle(),而做了吸附的 ContentMouseController 同时打开了链接——正是该处新注释声称要防止的「两件事同时发生」。这是可达的:ThinkBody 经由 MarkdownDisplay 渲染,展开的思考块确实带 OSC 8 链接,而中文链接标签对本项目受众很常见。一行修复:查询前把坐标过一遍 snapWideChar。顺带说明,坐标系本身是对的:terminalToGrid 与 layoutRowForEvent 使用完全相同的 frameAnchor 修正,所以 col/row 是合法的帧网格坐标,缺的只是宽字符吸附。
3. 以单击作为触发方式(设计问题,非缺陷)。 理由写的是修饰键状态无法通过 SGR 上报到达应用。这对 Cmd 成立,代码注释也正确限定为 Cmd;但 packages/cli/src/ui/utils/mouse.ts 已经从 SGR 和 X11 的 button code 解出 shift、meta/Alt、ctrl,mouse.test.ts 也固定了该行为,而 packages/cli/src/ui/ 下目前无人读取这三个字段。Ctrl+单击可以在不改变「transcript 里普通左键单击含义」的前提下恢复链接点击,并让多击窗口、同格拖拽守卫和延迟打开计时器从「承重结构」变成「不必要」。这些守卫写得是对的,问题在于它们是否本该存在。
已核查且认为可靠的部分(记录下来免得重复走一遍):Esc 不会被卡住——ContextMenuProvider 位于 AppContainer 唯一且无条件的返回树中,只随 AppContainer 一起卸载;控制器在失活与卸载两条路径都调用 closeMenu(),经 state 触发 onMenuChange(false) 清掉镜像 ref;待打开计时器在同两条路径取消,堵住了「销毁后计时器仍打开链接」的口子。协议处理保守——只有 ^https?:// 才进 openBrowserSecurely,mailto/ftp/ssh 等一律降级为剪贴板复制;.catch() 保证伪造的无 host https:// 不会抛出未处理拒绝;extractUrlFromOsc8Code 在所有 C0/C1 控制符处终止而非只在三种合法终止符处,畸形封装无法把控制字符夹带进 URL。没有新增鼠标模式管理——菜单打开时切到 'any' 交给 useMouseEvents 现有的按流引用计数,取最高请求级别且仅在级别真变化时写转义序列,无抖动。eventsPaused 与 isActive 不冗余——暂停保留选区以便「Copy Selection」有内容可复制,失活才清除,且拖拽进行中时的 left-release 仍会收尾。Copy Selection 在菜单打开时快照文本而非执行时重算,因为菜单打开期间帧仍在流式刷新。复用做得干净:snapWideChar 从 selection 栈抽出而非复制,MULTI_CLICK_MS 导出共享以免延迟窗口与多击窗口漂移,openBrowserSecurely/copyToClipboard 复用而非重写。文档也做了修正而非留旧:main 上三处「关掉 mouseTracking 才能恢复链接」的说明都更新了,生成的 companion schema 在同一 diff 里同步。
测试
本次为无人值守 CI 运行,因此我没有构建或执行本 PR 的任何代码——以下证据是 PR 自身的 CI,通过 API 针对被审查的 commit 读取。没有 tmux 抓取:那条路径仅限本地调用,CI 路径上的实时行为信号来自下方点名的验证通道。
唯一的红色检查是临时性基础设施噪声,与本 PR 无关。 Dependency CVE audit 失败的原因是 npm registry 的 audit 端点对 POST https://registry.npmjs.org/-/npm/v1/security/audits/quick 返回 503 Service Unavailable,npm 将其表现为 audit endpoint returned an error。两条独立理由可以排除它:本 PR 未改动任何依赖清单——28 个文件里没有 package.json 或 package-lock.json——因此不可能改变 CVE 暴露面;且同一检查在 main 的 60161cb6 上是绿的。日志中确实列出的公告(diff GHSA-73rr-hh4g-fpgx、uuid GHSA-w5hq-g745-h8pq)为 low 与 moderate,低于该作业的 --audit-level=high 阈值,属仓库既有状况而非本 PR 引入。
未经验证,而这正是真正的缺口: 覆盖 Windows 和 macOS 的两条单测通道是 skipped,而且是设计如此而非本 PR 的偶然——.github/workflows/ci.yml 里 test_macos 与 test_windows 的 if 限定为 merge_group、schedule、workflow_dispatch,pull_request 触发已被刻意移除。PR 描述写「Windows/Linux 由 CI 覆盖,未在本地验证」,但在 pull request 上只有 Linux 通道会跑;Windows 与 macOS 的覆盖来自 main 的 nightly,也就是合并之后。作者自己的 Tested-on 表格标记 macOS ✅、Windows/Linux Lint & Static 与 Test (ubuntu) 仍在进行中。
PR 描述中引用的 E2E 与单测结果是作者的声明;我没有重跑,也不将其作为证据呈现。
沙箱验证可以定论此事,且作者有写权限,两条通道均可用:
@qwen-code /tmux—— 核心主张是 TUI 界面行为:单击打开链接、右键弹出菜单、拖拽与双击/三击仍然是选择而非打开。diff 与单测都无法证明这在真实终端上成立,而作者跑过的 terminal-capture 脚手架并不在 CI 此次执行的检查之列(tmux-testing与verify均为skipped)。@qwen-code /verify—— 专门针对上面两个问题,因为二者都是「测试绿着的缺陷」:没有测试在同一 tick 内派发两次输入,也没有测试点击思考块链接中全角字符的右半格。针对 base build 的 A/B 运行能说明这些守卫是否真的成立,而不只是通过。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at 63735d9f6fad626ac54201563307095e21e6f6e5 · re-run with @qwen-code /triage
|
Confidence: 3/5 — the feature is well built and the problem is real, but there are two correctness defects the test suite structurally cannot see, and the choice of opener gesture rests on a rationale that only half holds. Both belong to a human, not to this gate. Stepping back: this is good work, and I want to be clear that 3/5 is genuine reservation rather than a policy cap — neither the fork-refactor guardrail nor a Stage 0 escalation applies here. Before reading the diff I'd have proposed reading the URL out of the composited frame cell under the pointer, gating the open behind a modifier, and adding a small in-app menu for right-click. The PR's frame-cell reading is the best idea in it and better than what I'd have done — I'd have instinctively reached for markdown re-parsing and column arithmetic, which is strictly worse, because by the time the frame is composited wrapping, wide characters and link extents are all already resolved. Choosing to hit-test at that layer is the right call, and What gives me pause is that the guard complexity is compensation. The multi-click window, the same-cell jitter tolerance, the deferred 400ms open, the mirrored The two defects are small in isolation. What bothers me is the pattern: 1334 lines of tests, 33 cases in the controller suite alone, genuinely good coverage of drag and multi-click and menu lifetime — and neither defect has a case, because both need something the suite never does. Finding 1 needs two inputs in one tick; finding 2 needs a click on the trailing half of a fullwidth character. If I were maintaining this in six months I'd thank you for the lifecycle handling — closing the menu and cancelling the pending-open timer on deactivate and unmount and resize is the thing that usually gets missed, and the orphaned-timer-opens-a-link hole is closed properly. I'd curse the un-snapped third Two housekeeping notes. I'm deferring rather than requesting changes, because the feature is worth landing and nothing here is a reason to rethink it. Concretely, what would move me to approve:
I'd also like No maintainer resolved deterministically for an @mention: the PR carries no area label, so the owner map had nothing to match, and the only existing review is from a bot account. Not assigning anyone rather than guessing a login. 中文说明Confidence: 3/5 —— 功能做得扎实,问题也是真实的,但存在两个测试套件在结构上看不见的正确性缺陷,而触发手势的选择所依据的理由只成立一半。这两件事该由人来判断,不该由闸门决定。 退一步看:这是一份好的工作,我想说清楚 3/5 是真实的保留意见,而不是政策性封顶——fork refactor 护栏和 Stage 0 上报在这里都不适用。 在读 diff 之前,我自己的方案是:从指针所在的合成帧单元格里读出 URL,用一个修饰键来限定打开动作,再为右键加一个小的应用内菜单。本 PR「读帧单元格」这一点是它最好的想法,也比我原本会做的更好——我会本能地去重新解析 markdown 并做列运算,而那严格更差,因为帧合成完成时,折行、宽字符和链接范围都已经被解析好了。选择在这一层做命中测试是正确的,而 让我犹豫的是:这些守卫的复杂度是一种补偿。多击窗口、同格抖动容忍、400ms 延迟打开、从 selection 控制器镜像过来的 这两个缺陷单独看都很小。让我不安的是这个模式:1334 行测试,光控制器套件就有 33 个用例,对拖拽、多击、菜单生命周期的覆盖确实很好——但两个缺陷都没有用例,因为它们都需要套件从未做过的事。问题 1 需要同一 tick 内的两次输入;问题 2 需要点击一个全角字符的右半格。而 如果六个月后由我来维护这段代码,我会感谢你在生命周期上的处理——在失活、卸载、resize 三条路径上都关闭菜单并取消待打开计时器,这通常正是最容易漏掉的部分,而「孤儿计时器打开链接」这个口子被正确堵住了。我也会骂一次那个没做吸附的第三处 两点流程说明。 我选择推迟而非要求修改,因为这个功能值得合并,这里没有任何内容需要推倒重来。具体地说,能让我转为 approve 的是:
合并前我也希望针对最终 head 跑一次 没有确定性地解析出可以 @ 的维护者:本 PR 没有领域标签,owner 映射无从匹配,而唯一已有的审查来自机器人账号。宁可不指派,也不猜一个 login。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
chiga0
left a comment
There was a problem hiding this comment.
Review — Round 1
No blocking findings. Approval withheld: reviewer account is the PR author (chiga0).
Scope
Reviewed: ContentMouseController.tsx (431 lines, new) · ContextMenuContext.tsx (164, new) · ContextMenuOverlay.tsx (105, new) · hyperlink-at.ts (79, new) · use-text-selection.tsx (paused/selectionQueryRef additions) · selection-coords.ts (snapWideChar extraction) · MainContent.tsx · AppContainer.tsx · InputPrompt.tsx · RowMouseController.tsx · TextInputMouseController.tsx.
NOT reviewed: docs/design/vp-native-mouse-parity.md (358-line design doc) · integration test vp-link-click.ts (238 lines) · unit test files. No working tree was available, so execution rungs (mutation probe, non-vacuity checks) were not run.
Checked — clean
-
contextMenuSize()vsContextMenuOverlaygeometry —width = longest + 4(2 border + 2 padding chars);height = items.length + 2(2 border rows). Overlay encodes the same:Textbody is${label.padEnd(longest)}inside aborderStyle="round"box. Consistent. -
hyperlink-at.tsURL extraction —extractUrlFromOsc8Codesearches for]8;, skips the params section to the second;, then reads until any C0/C1 control. Terminating on the full control-character range (not only BEL/ESC) prevents a hostile envelope from smuggling control bytes into the extracted URL. -
snapWideCharrefactoring — inline wide-char snap extracted fromTextSelectionControllerintoselection-coords.ts, re-used identically inContentMouseController.mapPointandhyperlink-at.hyperlinkAtCell. Both call sites pass the right frame/point. -
eventsPausedlogic inTextSelectionController— pausing ignores press/move/scroll while letting an in-flightleft-releasefinish its drag; the existing selection is preserved so the menu's Copy Selection item sees the live range. -
Mouse consumer gating —
RowMouseControllerandTextInputMouseControllerboth gate oncontextMenu === null, preventing a click on the overlay from also selecting a list row or repositioning the composer cursor underneath it. -
AppContainerEsc gating — reading a ref in the always-active keypress handler is the right mechanism; it avoids a stale-closure dependency on state, andonMenuChangekeeps it current without a re-render cycle. -
clampMenuPositionusingframeHeight— intentional and correct: the overlay paints inside the composited frame (not the raw terminal viewport), soframeHeightis the right bound when the frame overflows the terminal.
R1-1 — Minor · ContextMenuContext.tsx:101
closeMenu nulls menuRef.current synchronously before the async setMenu(null) to protect against a second keypress arriving in the same stdin batch before a React render. executeIndex does not apply the same guard:
const executeIndex = useCallback((index: number) => {
const item = menuRef.current?.items[index]; // menuRef.current still live
setMenu(null); // async — no render yet
if (item) item.onSelect();
}, []);The comment on closeMenu explicitly names this risk ("KeypressContext can dispatch a whole stdin chunk in one tick with no render between"). If two Enter keypresses arrive before the next render, executeIndex fires twice; on the second call menuRef.current is still non-null and onSelect() runs again (browser opens twice for Open Link). Fix mirrors closeMenu:
const executeIndex = useCallback((index: number) => {
const current = menuRef.current;
menuRef.current = null; // synchronous guard, same as closeMenu
setMenu(null);
current?.items[index]?.onSelect();
}, []);R1-2 — Question · InputPrompt.tsx:919
handleInput returns true for ↑/↓/Enter/Esc when the context menu is open, which is correct — it stops those keys from reaching vim/readline/shortcuts. But does return true also stop dispatch to independently subscribed useKeypress handlers (i.e. ContextMenuOverlay)? If so, menu keyboard navigation breaks silently: the keys are consumed before the overlay sees them, the menu never moves or closes, and only a mouse click or a non-navigation keystroke can dismiss it.
The design clearly intends the overlay to have fired first (no closeContextMenu() call on these keys, implying the overlay already handled them). But the priority guarantee isn't visible from the diff. Could you point to where the keypress system ensures the overlay's subscription fires before InputPrompt.handleInput?
Unreviewed dimensions
- Integration test and unit test files: not reviewed in depth.
- No working tree: execution rungs (mutation probe on
extractUrlFromOsc8Code, non-vacuity checks on new unit tests) were not run.
Reviewed with AI assistance.
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. Two Major findings to fix before merge (both one-liners, both already raised on the ci-bot's stage-2 review — I verified each independently at head and confirm they hold). Approval not recorded from this account (author); this is a COMMENT review.
R1-1 — executeIndex can run the same item twice on one buffered chunk (confirmed). ContextMenuContext.tsx:101-105: it reads menuRef.current, calls setMenu(null), then onSelect() — but never nulls the mirror. closeMenu directly above does clear it, and its own comment establishes the hazard ("KeypressContext can dispatch a whole stdin chunk in one tick with no render between"). The symmetric executeIndex-after-executeIndex case is unguarded: two \r in one chunk (key repeat over a highlighted row), or two left-presses on a row before React re-renders, each read the stale mirror and fire again — "Open Link" launches the browser twice. Fix: menuRef.current = null; as the first line of executeIndex. Regression gap: the existing executeIndex right after closeMenu in the same tick is a no-op test pins only the close→execute order, not execute→execute — a \r\r single-write test would have caught this; the suite is green with or without the defect.
R1-2 — ClickableThinkMessage doesn't snap wide characters before its link lookup (confirmed). HistoryItemDisplay.tsx calls hyperlinkAtCell(frame, col, row) on the un-snapped layoutRowForEvent result, while the PR's own new snapWideChar invariant says a click on the trailing half of a fullwidth cell must resolve to the leading cell for any per-cell lookup. A click on the right half of a CJK link label inside an expanded think block therefore sees no URL in the think-message guard and calls onToggle() — while ContentMouseController, which does snap, arms the open. That is exactly the toggle+open double action the guard exists to prevent. Reachable for this project's audience (CJK labels, links in think bodies via MarkdownDisplay). Fix: route the point through snapWideChar before the lookup.
Checked (my own pass, at head 63735d9f):
hyperlink-at.tsextraction: marker-search parse handles plain/ST/C1/tmux-DCS forms; terminating on every C0/C1/DEL keeps a malformed envelope from smuggling controls — reviewed against the emitter contract and matches.- Gesture state machine: press anchor + same-cell jitter, drag-breaks-chain, scroll-invalidates anchor and pending open,
lastClick.count !== 1release gate, delayed open cancelled by any press/scroll/teardown — I traced double/triple/drag-away/drag-back and each lands where the comment says. - Ownership:
AppContainerref-mirror (outside the provider) gates the always-active global handler;InputPromptconsumes ↑/↓/Enter/Esc and closes+falls through on any other key; overlay mounts only while open; menu closes on deactivate, unmount and resize; selection pauses (not clears) with in-flightleft-releasestill finishing. MULTI_CLICK_MSshared between the two controllers; geometrycontextMenuSizevs overlay rendering (border + padded label) consistent,itemIndexOfmatches.- Scheme policy: only
https?://reachesopenBrowserSecurely(catch→clipboard fallback); everything else copies with a hint — no arbitrary-scheme launching. - Settings descriptions updated in
settingsSchema.tsand regeneratedsettings.schema.json(2 strings) — in sync.
Ran (linux, head 63735d9f): the 8 changed/new unit files → 183/183 passed · mutation probe: deleting the count !== 1 open-guard in ContentMouseController.tsx turns exactly does not open when the click completes a double-click chain red (source restored byte-identical).
Not covered: the terminal-capture E2E (vp-link-click.ts) not run here — needs build+bundle and a real PTY; it was macOS-only locally per the PR, and the ci-bot observed the ubuntu/lint lanes still in flight. The design question the gate raised (plain click vs Ctrl/Alt-click — mouse.ts already decodes the modifier bits at lines 112-114, nothing in ui/ consumes them) stands for the author to answer in the description; I verified the plumbing facts and it does change how much of the guard surface is necessary.
Reviewed with AI assistance.
executeIndex cleared menu state asynchronously but left the synchronous menuRef mirror live, so two Enter keypresses dispatched in one stdin chunk (before React re-renders) ran the item's onSelect() twice — opening the browser twice for "Open Link". Null the mirror before the async setMenu, mirroring the guard closeMenu already applies.
chiga0
left a comment
There was a problem hiding this comment.
Review — Round 2
No blocking findings. No open questions. Approval withheld: reviewer account is the PR author (chiga0).
Round 1 findings — status at new head ea606b4e
| ID | Prior finding | Status |
|---|---|---|
| R1-1 | executeIndex double-execution on same-tick Enter |
✅ Fixed — commit e03f4d4a adds menuRef.current = null before setMenu(null), identical pattern to closeMenu. Comment matches exactly. |
| R1-2 | return true in InputPrompt.handleInput might block ContextMenuOverlay.useKeypress |
✅ Not a bug — see below. |
R1-2 resolution
After reading KeypressContext.tsx at this head, the two systems are independent:
ContextMenuOverlaycallsuseKeypress(handler, { isActive: menu !== null }), which subscribes toKeypressContext.subscribers(aSet<KeypressHandler>).InputPromptpasseshandleInputasonKeypress={handleInput}to an Ink text component (line 2349) — this is an entirely separate Ink prop callback, not auseKeypresssubscriber.broadcast()(KeypressContext, line 768) iteratesfor (const handler of subscribers)with no short-circuit:KeypressHandlerreturnsvoid, there is no cancellation mechanism.
So return true from handleInput signals to the Ink text component "don't echo this character" — it has zero effect on whether ContextMenuOverlay.useKeypress fires. Both handlers see every broadcasted key independently. No bug.
Unreviewed dimensions (unchanged from round 1)
- Integration test
vp-link-click.tsand unit test files not reviewed in depth. - No working tree: execution rungs not run.
Reviewed with AI assistance.
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Re-reviewed at head ea606b4e (my earlier approval sat on 63735d9f and was dismissed by the pushes; this replaces it after re-verifying the delta).
e03f4d4acloses a real double-fire I had not flagged on the first pass: KeypressContext broadcasts a whole stdin chunk with no render between the two Enters, andexecuteIndexcleared only the async state, leaving the synchronousmenuRefmirror live — a double Enter on "Open Link" ranonSelect()twice. The fix nulls the mirror before dispatch, mirroringcloseMenu's existing guard, with a same-tick red test (executeIndex(0)twice →onSelectonce, menu closed). Ordering subtleties hold: the capturedcurrentkeeps the item list for the first call, and a re-open from insideonSelectstarts from the fresh null mirror.ea606b4eis a pure merge of main: all fifteen files this PR owns are byte-identical across it, and the only two overlapping files (settingsSchema.ts,InputPrompt.tsx) shifted solely from main-side lands (#10817's held-expiry setting, #10929's live-buffer Enter fix) — this PR's hunks and the context-menu wiring verified present at the new head.- Carried forward from the prior pass, unchanged by the delta: the URL sinks remain strictly-validated
openBrowserSecurelyand clipboard copy (no raw exec of model-supplied targets), the deferred-open lifecycle cancels on press/scroll/deactivate/unmount, and while the menu is open pointer and keyboard have a single owner across RowMouseController, TextInputMouseController, the scroll list, and the global Esc handler. - Threads: the two follow-up threads opened after my first approval (chiga0's, on the double-Enter hazard this fix then closed) are both resolved; no other review tickets stand.
- CI facts: fresh run on the merged head, 4 pass / zero fail so far, nine lanes in flight including Test and the CVE audit that 503'd on the registry endpoint earlier today — per the channel convention the call is on the review itself.
|
Addressing the stage-2 code review (posted against 1 — 2 — the third let cell = row[x];
if (cell && cell.value === '' && x > 0 && row[x - 1]?.fullWidth) {
cell = row[x - 1];
}that is the same judgement 3 — plain click vs Ctrl+click (design). The factual correction is right, and the design doc already accounts for it ( On the coarser opt-out: agreed that |
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the feature diff at 63735d9 across correctness and security.
Security side is clean: the click-to-open path only forwards http(s):// URLs to openBrowserSecurely (argv-array launch, no shell), every other scheme degrades to a clipboard copy, OSC 8 URI extraction terminates on C0/C1 control characters, and the context menu never renders the raw attacker-controlled URL.
Correctness side had one finding: executeIndex() cleared menu state via setMenu(null) but left the synchronous menuRef mirror live, so two Enter keys in one stdin chunk could run the selected item twice. Fixed in e03f4d4 (mirror nulled before invoking onSelect) — verified at ea606b4. Selection logic and controller teardown checked with no regressions.
Changes since are main merges only. Test (ubuntu) is green on the current head. Nice work.
qqqys
left a comment
There was a problem hiding this comment.
Reviewed at head 27c2e06b. All historical blocking items are verified fixed or resolved at this head, and my independent Critical-only pass finds no blocking defects.
Historical blockers — status verified at this head's code:
executeIndexsame-tick double-fire (stage-2/3 finding, yiliang114's pass, author R1-1): fixed ine03f4d4aand present at this head —ContextMenuContext.executeIndexcaptures the menu, nulls the synchronousmenuRefmirror before the asyncsetMenu(null), then dispatches, exactly mirroringcloseMenu's guard; the same-tick regression test (twoexecuteIndex(0)in oneact(), author-verified red pre-fix) is in the suite.- Wide-char snap at the third
hyperlinkAtCellcall site (stage-3 finding 2): resolved as a false positive — verified at this head thathyperlinkAtCellitself snaps the trailing spacer half of a fullWidth cell to the leading cell (hyperlink-at.ts,cell.value === '' && row[x-1]?.fullWidth → row[x-1]), the same judgementsnapWideCharapplies, so a click on the trailing half of a think-block link resolves the URL and suppresses the toggle; the unit suite covers the snap. The primary click path additionally routes throughsnapWideCharinmapPoint. - Green
Test (ubuntu-latest)+Lint & Staticon the final head (stage-3 precondition 4): both lanes pass at this head. The opener-gesture rationale (stage-3 point 3) is a product-direction call, documented in the design doc with the corrected SGR facts — not a code blocker.
Critical-only pass on the feature: the only sinks for a model-supplied OSC 8 URL are core's strictly-validated openBrowserSecurely (http(s) only, argv launch, .catch degrading to clipboard) and clipboard copy for every other scheme; extractUrlFromOsc8Code terminates on all C0/C1 controls so no control character can ride into the extracted URL. The deferred-open lifecycle cancels on any subsequent press or scroll and on deactivate/unmount/resize (no orphaned timer opening a link, no stranded invisible menu executable by Enter). Pointer and keyboard ownership while the menu is open is single-owner: RowMouseController, TextInputMouseController and ClickableThinkMessage all quiet on contextMenu !== null, the menu's own hover/execute paths are bounds-checked, and scroll/outside-press dismiss it. Multi-click arbitration mirrors TextSelectionController's near rule so a double/triple-click release never opens the link under it. The delta since the prior approval at ea606b4e is main-side only (four opentui input-prompt files), this PR's files byte-identical.
CI at this head: no failing or cancelled checks; review-pr is the only pending lane, which does not gate this review per policy.
|
Round-3 disposition against the stage-3 approve-list (my earlier replies were written against the pre-merge 1 — 2 — let cell = row[x];
if (cell && cell.value === '' && x > 0 && row[x - 1]?.fullWidth) {
cell = row[x - 1];
}that is the same judgement For corroboration: 3 — opener gesture. Decision: keep the plain click. The description now states the honest tradeoff instead of a protocol claim — ⌘ is invisible to SGR, Ctrl/Alt are decodable, and plain click was chosen for discoverability + WezTerm/Kitty parity + not consuming a modifier — and I've added a sentence owning the cost (the guard surface, and that an ordinary left click now acts on every link in the transcript) rather than hiding it. Right-click carries Open Link / Copy for the modifier-preferring cohort. Moving to Ctrl/Alt would delete the guards but also move the gesture away from what the non-VP terminal path already does and reintroduce the "why doesn't a normal click work" surprise this PR exists to remove, so I'm not reverting the gesture here. 4 — CI on the final head. On |
tmux E2E report — head
|
|
Released in v0.23.1. |
What this PR does
In virtual-viewport (VP) mode, a plain left click now opens the OSC 8 hyperlink sitting under the pointer, and a right click raises an in-app context menu offering Open Link, Copy Link Address, and Copy Selection. Drag-to-select and double/triple-click word/line selection keep working.
Why it's needed
VP mode turns on SGR mouse tracking so the app owns the pointer. That also stops the terminal from handling its own OSC 8 hyperlinks and right-click menu, which is why links in the transcript were completely unclickable in VP mode. The opener is a plain click. ⌘ cannot be detected at all — the SGR mouse protocol carries only Shift/Ctrl/Alt, never Super — and although Ctrl/Alt are decodable, plain click is the deliberate choice: it is the most discoverable gesture, matches the WezTerm/Kitty convention, and consumes no modifier that terminals and user commands already reserve. The open is deferred by the multi-click window so it can never hijack a double/triple-click selection. That tradeoff is accepted on purpose: it does add a multi-click window, a same-cell drag guard and a delayed-open timer, and it makes an ordinary left click act on every link in the transcript — because click-to-act matches the text-selection metaphor, and the right-click menu carries the same Open Link / Copy actions for anyone who would rather reach a link behind a modifier.
Reviewer Test Plan
How to verify
Turn on VP (
ui.useTerminalBuffer) with mouse tracking on, then ask the model for a markdown link. A single click on the link opens it in the browser for http/https, or copies it to the clipboard with a hint for other schemes; a press-and-hold drag still selects text; double/triple click still selects a word/line; right-click over a link or an active selection shows the menu, and Esc / click-away / arrow+Enter all drive it. Withui.mouseTrackingoff, or outside VP, the terminal handles links natively exactly as before, and the OpenTUI renderer (which has its own link path) is untouched.Evidence (Before & After)
Before: clicking an OSC 8 link in VP did nothing — SGR tracking had suppressed the terminal's native link handling. After: an end-to-end terminal-capture harness drives a real SGR click at the link cell and observes the browser-open record (
linkOpened: true), plus the right-click → Escape menu flow, with screenshots captured. 189 unit tests across the new and changed modules pass; typecheck and lint are clean.Manual verification across terminals
A user ran the built CLI in four macOS terminals. Clicking a transcript link opened it in every terminal that supports OSC 8 + xterm mouse reporting; the default macOS Terminal.app is the sole failure, and that is a terminal-capability limit rather than a regression introduced here.
Apple_Terminalcase insupportsHyperlinks()→ no OSC 8 is emitted (links render as plain text), and Terminal.app implements no SGR mouse reporting, so a click never reaches the app — neither in-app nor native OSC 8 clicking is availableTested on
Environment
Bundled
dist/cli.js(npm run build && npm run bundle); E2E via the terminal-capture harness with a fake model server and aBROWSERwrapper; unit tests via vitest. Windows/Linux are covered by CI, not verified locally.Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
在虚拟视口(VP)模式下,单击左键现在会打开光标所在处的 OSC 8 超链接;右键会弹出一个应用内上下文菜单,提供「打开链接 / 复制链接地址 / 复制所选内容」。拖拽选择文本、双击/三击选词选行均保持不变。
为什么需要
VP 模式会开启 SGR 鼠标追踪,让应用接管指针。这同时也让终端不再处理它自己的 OSC 8 超链接和右键菜单——这正是 VP 模式下转录内容里的链接完全点不动的原因。打开手势选为单击:⌘ 根本无法探测——SGR 鼠标协议只携带 Shift/Ctrl/Alt,不含 Super;而 Ctrl/Alt 虽然能解出,单击仍是刻意之选——它最易发现、与 WezTerm/Kitty 的约定一致,且不占用任何终端与用户命令已保留的修饰键。打开动作按多击窗口延迟,使其永远不会抢占双击/三击的选择。这一取舍是有意接受的:它确实带来了多击窗口、同格拖拽守卫与延迟打开计时器,也让一次普通左键作用于整个 transcript 的每个链接——之所以接受,是因为"点击即作用于指针下之物"与文本选择的隐喻一致,且右键菜单同样提供 Open Link / Copy,为偏好以修饰键触达链接的用户保留了入口。
如何验证
开启 VP(
ui.useTerminalBuffer)并保持鼠标追踪开启,然后让模型输出一个 markdown 链接。单击链接:http/https 会在浏览器打开,其他协议会复制到剪贴板并给出提示;按住拖拽仍能选中文本;双击/三击仍能选中词/行;在链接或已有选区上右键弹出菜单,Esc / 点击空白 / 方向键+回车均可操作。关闭ui.mouseTracking,或在 VP 之外,终端会像以前一样原生处理链接;OpenTUI 渲染器(有自己独立的链接链路)不受影响。证据(前 vs 后)
前:在 VP 下点击 OSC 8 链接毫无反应——SGR 追踪压制了终端原生的链接处理。后:端到端 terminal-capture 测试在链接单元格注入真实 SGR 单击并观察到浏览器打开记录(
linkOpened: true),并覆盖了右键 → Esc 菜单流程,同时捕获了截图。新增及改动模块的 189 个单元测试全部通过;typecheck 与 lint 均干净。跨终端手动验证
用户在 macOS 上分别用四个终端跑了构建后的 CLI。除 macOS 默认「终端」外,所有支持 OSC 8 + xterm 鼠标上报的终端点击链接都能打开;默认终端点不动是终端能力所限,而非本次改动引入的回归。
supportsHyperlinks()无Apple_Terminal分支 → 不输出 OSC 8(链接渲染为纯文本),且 Terminal.app 不支持 SGR 鼠标上报,点击事件根本不进应用——应用内与原生 OSC 8 点击均不可用环境
打包后的
dist/cli.js(npm run build && npm run bundle);E2E 通过 terminal-capture 测试配合假模型服务与BROWSER包装脚本;单元测试用 vitest。Windows/Linux 由 CI 覆盖,未在本地验证。风险与范围