fix(cli): make an open context menu own the keyboard at the dispatch layer - #11573
bluefateludi wants to merge 7 commits into
Conversation
…layer KeypressContext's dispatch is a broadcast that discards handler return values, so an open right-click context menu cannot consume a key: the composer, the tool-approval dialog, and the agent tab bar all act on the same keystroke the user aimed at the menu. Worst case measured in the issue, Enter aimed at a menu item also confirms the approval dialog and Down+Enter can land on a persistent project-wide permission grant. Add an exclusive subscription tier to KeypressContext: while an exclusive handler is subscribed, ordinary handlers do not receive keys. The menu overlay subscribes exclusively while open, consumes up/down/return/escape, and dismisses itself on any other key, returning false so that dismissing keystroke falls through to the ordinary pipeline in the same dispatch (typing x dismisses the menu and types x) — mirroring the dismissal contract InputPrompt already implements and keeping the key from being dropped into the bare readline layer. Fixes QwenLM#11228
|
Thanks for the PR — this is well framed, and it picks up a design question that was deliberately left open rather than inventing one. Template looks good ✓ — every required section is present and substantively filled in. The only gap is the template's trailing Problem: observed, not theoretical. #11228 is open, and its own triage verified the root cause statically against Direction: aligned, and unusually explicitly so — the issue prescribes exactly this ("make an open menu exclusive at the dispatch layer … rather than adding a Size: core-module gate not applicable — all five files are Approach: the scope feels right, and this is the minimal shape of the fix — Risk: no elevated risk signals — none of the changed files match the revert-correlated path list. The real blast radius here is semantic rather than locational: a new dispatch tier that gates every ordinary Moving on to code review. 🔍 中文说明感谢贡献！这个 PR 描述得很清楚，而且它接手的是一个被有意留待人工决策的设计问题，不是自己新造一个。 模板完整 ✓ —— 所有必需章节都存在且有实质内容。唯一缺的是模板末尾的 问题： 是已观测到的 bug，不是理论性加固。#11228 仍处于 open 状态，其自身的 triage 已对照 方向： 对齐，而且对齐得非常明确——issue 本身就指定了这个方案（"在派发层让打开的菜单独占按键……而不是给每个消费者加一个 规模： 核心模块门禁不适用——五个文件全部位于单一包内的 方案： 范围合理，而且这是该修复的最小形态—— 风险： 无升级风险信号——改动文件均未命中与 revert 相关的路径清单。这里真正的影响面是语义层面而非位置层面的：新增的派发层会在任何模态界面 opt-in 时，屏蔽所有普通的 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI read the base tree at the reviewed commit and traced the new dispatch tier through its real consumers rather than only through the new tests. No Critical blockers found — the mechanism is sound, and it satisfies both constraints #11228 placed on any fix. What I verified:
Non-blocking, for the author to weigh:
The dispatch decision the whole PR turns on: sequenceDiagram
participant P1 as User
participant P2 as KeypressContext broadcast
participant P3 as ContextMenuOverlay exclusive tier
participant P4 as Ordinary handlers
P1->>P2: keypress while the menu is open
P2->>P3: route to the exclusive tier only
alt key is up, down, return or escape
P3-->>P2: true (consumed)
P2-->>P1: ordinary handlers never see it
else any other key, for example a printable character
P3->>P3: closeMenu()
P3-->>P2: false (declined)
P2->>P4: the same keystroke falls through
P4-->>P1: composer types it, quit and exit still work
end
Test evidenceEvidence carried: the PR's own CI, read through the API. This was an unattended CI run, so nothing was built, executed, or driven in a terminal — no terminal output is claimed, and I did not run the PR's tests. The honest headline: the checks that would substantiate this change have not reported yet. What is green is real but peripheral: the two TUI-renderer gates ( Nothing is red, so there is no failing-job log to quote. Not verified, and why: the four behaviours in the Reviewer Test Plan (Enter/Escape/Down acting on the menu only, a printable key dismissing and typing in one keystroke) were not exercised against a real terminal on this run. The PR's own Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle this: 中文说明代码审查我在被审查的 commit 上阅读了基线代码树，并把新的派发层沿着真实消费者追踪了一遍，而不只是看新增测试。未发现 Critical 阻塞项——机制是可靠的，并且满足了 #11228 对任何修复提出的两条约束。 已核实的内容：
非阻塞项，供作者权衡：
整个 PR 的关键派发决策（时序图见上，此处不重复）：菜单打开时按键只路由到独占层；up/down/return/escape 被消费，普通 handler 完全收不到；其他任何按键则先 测试证据本次证据：仅 PR 自身的 CI，通过 API 读取。 这是无人值守的 CI 运行，因此没有构建、没有执行、也没有在终端里驱动任何代码——不提供任何终端输出，也未自行运行 PR 的测试。 需要直说的一点：真正能支撑本改动的检查尚未出结果。 在被审查的 commit 上， 已经变绿的部分是真实的，但属于外围：两个 TUI 渲染门禁（ 没有任何检查变红，因此没有失败日志可引用。 未验证的部分及原因：Reviewer Test Plan 中的四个行为（Enter/Escape/Down 只作用于菜单、可打印字符在同一次按键内既关闭菜单又被输入）本次没有在真实终端上验证过。PR 自己的 CI 表格见上方标记区域（此处不重复）。 关于沙箱化验证：承重的断言是"菜单打开时按 Enter 与 Down+Enter 不再确认工具审批弹窗（也不再落到 'Always allow in this project'）"。diff 中没有任何东西钉住它：新测试探针的是一个合成的 bystander 而非 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the mechanism is right and I could not break it by reading, but #11228 was deliberately parked for a human design call, and this PR is that call. Before reading the diff I wrote down what I would do: a second subscriber set for exclusive handlers, The part I would push back on is scope of claim, not scope of code. Three entrances are described as live, and only one is: on On the code itself I am genuinely satisfied. I went looking for the two ways this class of fix usually breaks — a key path that routes around the new gate, and a dismissed keystroke that gets swallowed by a consumer still holding stale menu state — and neither is present. There is one dispatch point; the mouse channel is separate and ungated; What stops me approving is not doubt about the diff. It is that #11228 carries So: deferring rather than requesting changes. I found no blocking defect and I do not want the author to read this as a rejection — the work is good and the direction is the one the issue asked for. It needs a maintainer's sign-off on the protocol decision, and green CI, and ideally one test on the approval-dialog surface. 中文说明Confidence: 3/5 —— 机制是对的，我通过阅读没能把它推翻；但 #11228 是被有意留给人工做设计决策的，而这个 PR 正是那个决策本身。 在读 diff 之前我先写下了自己会怎么做：为独占 handler 增加第二个订阅集合； 我真正想提出的一点，是声明的范围而非代码的范围。PR 描述中有三个入口被说成是已生效的，而实际只有一个：在 就代码本身而言，我是真心满意的。我专门去找这类修复通常会出问题的两种方式——某条按键路径绕过了新的 gate，以及被拒绝的按键被仍持有过期菜单状态的消费者吞掉——两者都不存在。派发点只有一处；鼠标通道独立且未被 gate； 阻止我 approve 的，并不是对 diff 的怀疑。而是 #11228 带有 所以：选择 defer，而不是 request changes。我没有发现阻塞性缺陷，也不希望作者把这读成拒绝——工作是好的，方向也正是 issue 所要求的。它需要的是 maintainer 对协议决策的签字、绿色的 CI，以及最好在审批弹窗那个界面上补一个测试。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
⏸️ Deferring to @yiliang114 — needs a human call on this one. No blocking defect was found, so this is not a request for changes. Three things put it outside what the gate should settle on its own:
Because the author has read access only, |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- multiple exclusive subscribers' OR-ed decline semantics — already reported (comment 5619557750, non-blocking item 2)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; this diff's riskiest documented trigger lives in the Windows-only passthrough/pasteWorkaround path (KeypressContext.tsx:1277-1282).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; no interactive end-to-end lane exercised the real keypress dispatch.
— qwen3.8-max via Qwen Code /review (v0.23.2)
| closeMenu(); | ||
| return false; |
There was a problem hiding this comment.
[Suggestion] R1-1: The dismissal here is asynchronous, but the exclusive claim is released only by useKeypress's passive-effect cleanup, so a trailing up/down/return in the same stdin chunk is still routed to an already-dismissed menu and consumed instead of falling through. The contract this diff's own comment states — "it must keep flowing through the normal pipeline" — measured true only for the first key of a chunk.
KeypressContext dispatches a whole stdin chunk in one tick with no render between; ContextMenuContext.tsx:85-88 says so explicitly and nulls menuRef synchronously for exactly that reason. So with the menu open and a chunk carrying x then Enter, the first key takes this branch, closeMenu() queues setMenu(null) and returns false, and the composer types x. Then in the same synchronous tick broadcast('\r') still finds the overlay in exclusiveSubscribers — a passive effect cannot run inside the dispatch — with a closure that still sees the non-null menu, so it takes the return branch, executeIndex no-ops on the already-nulled menuRef, returns true, and broadcast returns before the ordinary loop. The trigger is documented in-tree: KeypressContext.tsx:1277-1282 records that Enter commonly shares a stdin chunk on Windows, and the kitty salvage loops broadcast inside a while.
Worth being precise about what this is not: it is not a regression, and it is not a lost submission. The A/B below shows the merge base loses the same keystroke one layer down — InputPrompt's own unchanged four-key gate eats it — and is worse in that window besides, because the base additionally executed a menu item on an Enter that arrived after dismissal. What survives is contract accuracy, which is why this is a Suggestion rather than a blocker.
Witness (A/B, the same probe file byte-identical in both arms, one stdin.write per row, fresh render per row):
CONTROL (no menu open), identical on both arms:
{"bystanderKeys":["x","\r"],"composer":{"called":["x","\r"],"swallowed":[],"submitted":["draft"]}}
row B 'x' + Enter, menu open
BASE {"bystanderKeys":["x","\r"],"itemOpenCalls":1}
PR {"bystanderKeys":["x"], "itemOpenCalls":0}
row C real composer path (BaseTextInput:218 + InputPrompt's menu gate)
BASE {"called":["x","\r"],"swallowed":["\r"],"submitted":[]}
PR {"called":["x"], "swallowed":[], "submitted":[]}
row D two Enters BASE bystanderKeys ["\r","\r"] / PR []
row E 'x' + Down BASE bystanderKeys ["x",Down] / PR ["x"]
row F 'x' + Esc identical on both arms (readline delivers a lone Esc after the commit)
Release the claim synchronously, the way closeMenu/executeIndex already release menuRef: add const releasedRef = useRef(false) in ActiveContextMenu — the component unmounts whenever menu goes null, so the ref is fresh per open and needs no reset — start the handler with if (!menu || releasedRef.current) return false;, and set releasedRef.current = true in the escape, return and dismiss branches, leaving up/down returning true on a live menu. Note this repairs the dispatch-layer contract, not the user-visible submission outcome, which InputPrompt's own stale gate limits independently.
Any fix here has to keep nulling menuRef so two Enters in one chunk still run the item at most once — ContextMenuContext.tsx:100-107 reads menuRef.current, nulls it, then calls current?.items[index]?.onSelect() under the comment "a double Enter must not run the item twice", pinned by ContextMenuContext.test.tsx:159 — and must not stop the first Enter or Escape from reaching executeIndex/closeMenu, which ContextMenuOverlay.test.tsx already pins. Please add a case to the key exclusivity block writing one chunk stdin.write('x\r') with the menu open and asserting the bystander received both keys and that no item's onSelect ran — it is red at this commit (expected [ 'x' ] to deeply equal [ 'x', '\r' ]) — plus a sibling '\r\r' case asserting the item ran exactly once and the bystander received one Enter; then remove the guard and confirm both go red, since the four existing tests each write a single key per stdin.write and so cannot see this window.
— qwen3.8-max via Qwen Code /review (v0.23.2)
There was a problem hiding this comment.
[Suggestion] R1-1: Still standing from round 1, at the severity round 1 ruled it. The dismissal here is asynchronous, but the exclusive claim is released only by useKeypress's passive-effect cleanup, so a trailing up/down/return in the same stdin chunk is still routed to an already-dismissed menu and consumed instead of falling through. The contract this diff's own comment states — "it must keep flowing through the normal pipeline" — measured true only for the first key of a chunk.
With the menu open and one stdin.write('x\r'), the first key takes the dismissal branch, closeMenu() queues setMenu(null) and returns false, so x reaches the composer correctly. The second key is broadcast in the same synchronous tick — no passive effect and no render can run inside it — so onKeypressRef.current still holds the closure with a non-null menu, the return branch matches, executeIndex no-ops on the already-nulled menuRef, and the handler returns true, so broadcast returns before the ordinary loop. The user's Enter never reaches the composer: the text is in the buffer, the prompt does not submit, and the menu is already gone so nothing suggests who took the key. The same window swallows a trailing arrow or Escape. This is routine under SSH/tmux batching, a busy event loop during streaming, or key auto-repeat, and the trigger is documented in-tree at KeypressContext.tsx:1277-1282.
Witness:
re-confirmed this round by two independent finder probes:
stdin.write('x\r') -> bystanderKeys ['x'] (the \r missing; expected ['x','\r'])
stdin.write('x\u001b') -> bystanderKeys ['x'] (the Escape eaten)
base-arm source proof (.qwen/tmp/review-pr-11573-base, built at 779cfe913):
grep -c "exclusive\|unmodified" .../ContextMenuOverlay.tsx -> 0
git show 779cfe913b:...KeypressContext.tsx -> 774: for (const handler of subscribers) {
775: handler(key); <- no exclusive tier
severity rests on a set-level argument rather than one row: the keys a stale closure can wrongly claim are
exactly {escape, up, down, return} — which is exactly the set InputPrompt.tsx:911-919's unchanged gate already
returns true for, so on the composer path the user-visible delta between arms is empty BY CONSTRUCTION.
Consistent with round 1's executed rows B, C and E.
behavioural A/B this round: not run — `review scratch-tree` was refused (CI runner's git-config `includeIf`),
so no probe file could be written into an isolated arm.
Decide the claim from live state rather than the render closure, and decline rather than consume once the menu is gone. ContextMenuContext already maintains the synchronous mirror (menuRef, cleared in closeMenu at :88 and guarded in executeIndex at :105); expose it (e.g. isMenuOpen(): boolean) and start the handler with if (!menu || !isMenuOpen()) return false;, which also retires the wrong-direction if (!menu) return true; sentinel at :51. An overlay-local equivalent is a dismissedRef set beside every closeMenu()/executeIndex() call and tested first; a fresh ref per mount is correct because ActiveContextMenu unmounts whenever the menu closes.
Three premises the fix rests on. The double-fire guard must survive: ContextMenuContext.tsx:100-107 reads menuRef.current, nulls it, then calls current?.items[index]?.onSelect() under the comment "a double Enter must not run the item twice" (pinned by ContextMenuContext.test.tsx:159), so any live-open predicate must be read before dispatch and must keep executeIndex as the only path that invokes onSelect. The dismissing keystroke itself must stay consumed: the existing 'an ordinary handler does not receive Escape aimed at the open menu' case asserts expect(bystanderKeys).toEqual([]), and broadcast resumes ordinary dispatch only on a literal false, so returning false for the Escape that closes the menu would leak Esc into keyMatchers[Command.ESCAPE] in AppContainer.tsx:4666-4672. And please fix this layer together with InputPrompt's gate (R2-3), or not at all: while that gate still swallows return by name, a released trailing Enter cannot reach Command.SUBMIT — but if the gate is made live or deleted first, a batched double-Enter would submit a draft the user never aimed at the model.
The witness this needs is a case in describe('key exclusivity') that issues ONE stdin.write('x\r') with the menu open and asserts expect(bystanderKeys).toEqual(['x', '\r']) plus expect(handlers.open).not.toHaveBeenCalled(); it is red at HEAD (expected [ 'x' ] to deeply equal [ 'x', '\r' ]) and must go red again if the live-state check is removed. A sibling 'x\u001b' case pins the Escape branch the same way, and a '\r\r' case should assert the item ran exactly once.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| if (!declined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-2: This fall-through is dispatched synchronously, but consumers that deactivate themselves while a menu is open are not re-subscribed yet, so the keystroke the new contract promises "must keep flowing" is dropped for them and the user has to press it twice.
The menu only opens in MainContent's virtual-viewport branch (ContentMouseController.tsx:335 is the sole production openMenu call site), and there viewportInteractive = !uiState.dialogsVisible && contextMenuOpen === null (MainContent.tsx:508-509) is passed as hasFocus to ScrollableList, whose useKeypress(..., { isActive: hasFocus }) (ScrollableList.tsx:80-103) owns pageup, pagedown, ctrl+home and ctrl+end. So a right-click unsubscribes the scroll list; the PageDown that dismisses the menu is then declined by the overlay and handed to a subscriber set that no longer contains it, because React has not committed the close or re-run the passive effects that would re-subscribe. The menu closes, the transcript does not scroll, and the user presses PageDown twice. The same shape sits one level up in AppContainer.tsx:4750-4760, which dismisses a completed btw side-question on Space only when !contextMenuOpenRef.current — a ref written from a useEffect (ContextMenuContext.tsx:92-94), so it is still true during the fall-through.
This is not a regression: on the base arm the menu never dismissed on PageDown, the gated consumer never received the key, and the btw card never cleared. What is new is the contract and the comment asserting fall-through, and no test can see either case, because the InnocentBystander in the new tests is isActive: true — subscribed throughout.
Witness (executed on both arms with the same probe file; the gated bystander is shaped exactly like ScrollableList, useKeypress(h, { isActive: menu === null }) inside ContextMenuProvider):
row G PageDown that dismisses the menu
PR {"menuClosedAfterFirst":true, "gatedAfterFirst":[], "alwaysAfterFirst":[PageDown]}
BASE {"menuClosedAfterFirst":false,"gatedAfterFirst":[], "gatedAll":[]}
-> the dismissing PageDown reaches the always-subscribed handler but not the gated one;
only the SECOND PageDown arrives (gatedAll has one entry)
row H control, no menu: {"gatedAll":[PageDown]} on both arms (the comparator can report a difference)
row I btw branch: PR {"handlerCalledWith":[" "," "],"clearedAfterFirst":[],"clearedAll":[" "]}
-> the global handler DOES receive the dismissing Space, but the btw branch does not fire on it
Stop deriving the keyboard gate from the menu now that the dispatch layer owns exclusivity: keep hasFocus={viewportInteractive} for the mouse path in MainContent and pass ScrollableList a separate keyboard-active flag of !uiState.dialogsVisible, so pageup/pagedown/ctrl+home/ctrl+end stay subscribed during the fall-through; and drop !contextMenuOpenRef.current from the btw Space branch in AppContainer, since Enter is already claimed at the dispatch layer. If the same-tick fall-through is deliberately limited to always-subscribed handlers, that is a fine decision too — but then say so in the ExclusiveKeypressOptions.exclusive doc and in ContextMenuOverlay's header comment rather than claiming the key must keep flowing.
One constraint on splitting that flag: ScrollableList.tsx:187 passes the same hasFocus to useMouseEvents(handleMouseEvent, { isActive: hasFocus, bypassVpGate: true }), and MainContent.tsx:503-505 records that "the scroll list goes quiet so clicks/keys don't leak into the content under the menu" — so wheel and scrollbar events must stay gated on contextMenuOpen === null, or they would reach the list under an open menu and fight ContentMouseController's contextMenuSize hit-testing. Please add a case to the key exclusivity block using a menu-gated bystander (const { menu } = useContextMenu(); useKeypress(h, { isActive: menu === null })) that writes PageDown, waits for the menu to disappear, and asserts the bystander received that same key — red at this commit — and then restore the single flag and confirm it goes red again.
— qwen3.8-max via Qwen Code /review (v0.23.2)
| * dismisses and types "x"; returning early would drop it into the bare | ||
| * readline layer). Dismissal is therefore reported back to KeypressContext so | ||
| * the same dispatch falls through to the ordinary handlers the moment the menu | ||
| * is gone. While the menu is open the subscription is exclusive: keys are |
There was a problem hiding this comment.
[Suggestion] R1-4: The design doc that owns this feature still prescribes the mechanism this diff replaces, and the user-facing shortcuts page still lists only two of the three ways the menu now closes.
docs/design/vp-native-mouse-parity.md:194-198 reads: "Keyboard: useKeypress({ isActive: open }) handles up/down (move selection), Enter (execute), Esc (close). While open, a contextMenuOpen flag from the context quiets the composer in InputPrompt following the existing agentTabBarFocused pattern, so arrow keys/Esc don't also edit the prompt." That is the per-consumer approach this change moves off, and it names neither the exclusive tier nor the new rule that any other key dismisses. The "Known limitation" section at :337-351 likewise attributes selection-clearing to Escape alone ("an Escape that closes the menu also clears the selection"), where after this diff every dismissing key triggers the same menu-close re-render. And docs/users/reference/keyboard-shortcuts.md:135 tells users the menu is "dismissed with Esc or by clicking outside it", omitting the dismiss-and-keep-typing behaviour the new tests pin.
The cost is the next contributor, not this one: an author adding another modal surface starts at that design doc, implements opt-out flags consumer by consumer as instructed, and every consumer that forgets the flag acts on the same keystroke — issue 11228 again.
Witness (every cited line read verbatim at the reviewed commit and quoted above; a markdown file has no runnable surface). The behaviour the shortcuts page omits is pinned by an executed test: ContextMenuOverlay.test.tsx > key exclusivity > a dismissing key falls through to ordinary handlers in the same keystroke passes (10 tests green in that file).
Two things to keep this in proportion. First, the per-consumer mechanism is not removed from the code — InputPrompt.tsx:911-925 and AppContainer's contextMenuOpenRef gates at :3872, :4670 and :4755 are all still there, deliberately, per the PR description — so the design doc is stale as guidance rather than as a description of deleted code, and saying which layer is load-bearing is the useful part of the update. Second, the missing zh-CN twin is the weakest element of this ask: only 10 of 458 files in docs/design/ carry one, and this file's gap predates the PR, so it can be split into follow-up work.
Concretely: rewrite the "Interaction while open -> Keyboard" bullet to state that the overlay subscribes with exclusive: true, that broadcast() routes keys to exclusive subscribers first and skips the ordinary set unless one returns false, and that any non-arrow/Enter/Esc key dismisses the menu and falls through in the same dispatch — noting InputPrompt's contextMenuOpen guard as the remaining belt-and-braces layer for the window before the overlay's effect subscribes. Widen the "Known limitation" section from Escape to any dismissing key, and add the dismiss-and-keep-typing line to the shortcuts page.
— qwen3.8-max via Qwen Code /review (v0.23.2)
There was a problem hiding this comment.
[Suggestion] R1-4: Still standing from round 1 — the design doc that owns this feature still prescribes the per-consumer mechanism this diff replaces, and the user-facing shortcuts page still lists only two of the three ways the menu now closes.
docs/design/vp-native-mouse-parity.md:194-198 still reads "Keyboard: useKeypress({ isActive: open }) handles up/down (move selection), Enter (execute), Esc (close). While open, a contextMenuOpen flag from the context quiets the composer in InputPrompt following the existing agentTabBarFocused pattern" — the per-consumer approach this change moves off, naming neither the exclusive tier nor the new rule that any other key dismisses. Its "Known limitation" section at :337-351 likewise attributes selection-clearing to Escape alone, where after this diff every dismissing key triggers the same menu-close re-render. And docs/users/reference/keyboard-shortcuts.md:135 tells users the menu is "dismissed with Esc or by clicking outside it", omitting the dismiss-and-keep-typing behaviour the new tests pin. The cost lands on the next contributor rather than this one: an author adding another modal surface starts at that design doc, implements opt-out flags consumer by consumer as instructed, and every consumer that forgets the flag acts on the same keystroke — issue 11228 again.
Witness:
every cited doc line read verbatim at the reviewed commit and quoted above; a markdown file has no runnable surface.
the behaviour the shortcuts page omits is pinned by an executed test:
key exclusivity > a dismissing key falls through to ordinary handlers in the same keystroke ✓ (Tests 11 passed (11))
nothing in the round-2 delta touched either doc:
git diff b018e4e53e..HEAD --name-only -> ContextMenuOverlay.tsx, ContextMenuOverlay.test.tsx
Worth keeping in proportion: the per-consumer mechanism is deliberately NOT removed from the code (InputPrompt.tsx:911-925 and AppContainer's contextMenuOpenRef gates at :3872, :4670 and :4755 all remain, per the PR description), so the design doc is stale as guidance rather than as a description of deleted code — saying which layer is load-bearing is the useful part of the update.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| if (handler(key) === false) { | ||
| declined = true; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-6: This compares against the literal false, but the widened return type's own JSDoc explicitly invites async handlers — and a pending Promise is never === false, so an async exclusive handler would silently consume every key it is handed.
KeypressHandler's new doc says the widening to unknown "keeps both sync and async handlers assignable", while broadcast never awaits handler(key). So the next modal surface that subscribes useKeypress(async (key) => { ...; return false; }, { isActive, exclusive: true }) — say one that must await a permission or config check before deciding — type-checks (Promise<false> is assignable to unknown), and at runtime declined stays false, broadcast returns at the gate, and the keystroke is dropped: the ordinary loop never runs, the composer never sees the key, and nothing raises. That is precisely the loss the dismissal contract exists to prevent.
This is latent, not live — the only production exclusive: true subscriber is ContextMenuOverlay.tsx:77, whose handler is (key: Key): boolean. But nothing static would catch it when someone adds one: eslint.config.js:63 uses ...tseslint.configs.recommended, and a sweep of the whole config for recommendedTypeChecked, no-misused-promises, no-floating-promises and strictTypeChecked found no matches.
Witness (runtime probe through the real KeypressProvider, PR arm):
PROBE-SYNC {"exclusiveCalls":1,"ordinaryCalls":1}
PROBE-ASYNC {"exclusiveCalls":1,"returnedIsPromise":true,
"returnedStrictEqualsFalse":false,"ordinaryCalls":0}
Make the synchronous requirement part of the type for the exclusive path so a violation is a compile error — overload useKeypress (and/or subscribe) so exclusive: true takes (key: Key) => boolean | void while exclusive?: false keeps the widened KeypressHandler. If overloads are too heavy for one call site, the minimum is to state "must be synchronous" in the ExclusiveKeypressOptions.exclusive JSDoc and drop the "keeps both sync and async handlers assignable" clause, which is what invites the broken shape.
Any narrowing has to stay on the exclusive: true overload only, never on KeypressHandler itself: the widened type must remain bidirectionally assignable for the ordinary path, because test mocks re-declare it locally as type KeypressHandler = (key: Key) => void; (AgentComposer.test.tsx:37) and capture real handlers into it. If you take the overload route, please add a // @ts-expect-error case to useKeypress.test.ts passing async () => false with exclusive: true, and confirm that removing the overload makes npm run typecheck go red on the now-unused directive.
— qwen3.8-max via Qwen Code /review (v0.23.2)
| if (exclusiveSubscribers.size > 0) { | ||
| let declined = false; | ||
| for (const handler of exclusiveSubscribers) { |
There was a problem hiding this comment.
[Suggestion] R1-7: Keys this tier consumes disappear from the general.debugKeystrokeLogging record, because the only per-key log in the CLI is the first statement of an ordinary subscriber that the early return below now skips.
A ripgrep sweep over all of packages/ finds exactly one [DEBUG] Keystroke site: AppContainer.tsx:4626-4629, the first statement of handleGlobalKeypress, before every guard. That handler is an ordinary subscriber (useKeypress(handleGlobalKeypress, { isActive: true }) at AppContainer.tsx:4901, with exclusive defaulting to false). The debugKeystrokeLogging sites inside this file (:340, :928, :1090-1215) cover only kitty-buffer accumulation, parse, recovery and overflow — never an ordinary dispatched key.
So with the setting on: right-click an OSC 8 link in the transcript and press Enter. The overlay executes the item — in production onSelect: () => openLink(url) (ContentMouseController.tsx:290-294), launching an external browser with a URL harvested from rendered model or tool output — and broadcast returns at the gate, so handleGlobalKeypress never runs and no keystroke line is written for that key. An engineer reproducing a "the browser opened by itself" or "the menu ate my key" report from the debug log finds a hole at exactly the decisive keystroke, and the same hole covers Escape and the arrows. AppContainer.tsx is not among this PR's five changed files and the broadcast hunk is additions-only, so every broadcast key was logged before this change; as far as we can tell this is the first time the setting has silently stopped recording a class of user input.
Witness (executed at the reviewed commit — in all three the ordinary handler's call list stays empty, i.e. handleGlobalKeypress never runs, so its log line never fires):
ContextMenuOverlay.test.tsx an ordinary handler does not receive Enter aimed at the open menu PASS
ContextMenuOverlay.test.tsx an ordinary handler does not receive Escape aimed at the open menu PASS
KeypressContext.test.tsx swallows the key for ordinary handlers when the exclusive handler
consumes it PASS
sweep: exactly 1 hit for "[DEBUG] Keystroke" across packages/ (AppContainer.tsx:4626-4629)
Log the key inside broadcast() immediately after noteInteraction() and before this gate, so the record covers every dispatched key regardless of who owns it — if (debugKeystrokeLogging) debugLogger.debug('[DEBUG] Keystroke:', JSON.stringify(key)); using the prop the provider already receives (:196, :203, genuinely populated from startInteractiveUI.tsx:239-241) — and drop the now-duplicated call in AppContainer.
Placing it there rather than earlier is what keeps the record clean: broadcast() is downstream of the protocol pre-filters, and the comment at :806-808 records that DA1/DA2, kitty queries and FOCUS_IN/OUT "early-return before reaching broadcast, so terminal protocol noise does not count as user activity" — so the relocated log sees strictly user input, not the raw byte stream the AppContainer call never saw either. Please extend the swallows the key for ordinary handlers when the exclusive handler consumes it case to render the provider with debugKeystrokeLogging={true} and a debugLogger.debug spy, asserting the keystroke was logged even though the ordinary handler was not called, then move the log back below the gate and confirm that assertion goes red while every existing one still passes.
— qwen3.8-max via Qwen Code /review (v0.23.2)
|
Thanks — the Critical is fixed in 794c379 (pushed), exactly along the suggested line. [Critical] R1-12 modified-Return loss. The menu's exclusive tier now claims Per your test request, the On the Suggestion-level items (R1-1 same-chunk trailing key, R1-2 re-subscribe fall-through, R1-4 design-doc drift, R1-5 arrow-consume pinning, R1-6 async-handler |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 synchronous fall-through vs. consumers that deactivate while a menu is open — already reported (comment 3982157537), KeypressContext.tsx:824, outside this round's incremental scope
- R1-6 literal
falsedecline check vs. async exclusive handlers — already reported (comment 3982157560), KeypressContext.tsx:820, outside this round's incremental scope - R1-7 debug-keystroke logging hole at the exclusive gate — already reported (comment 3982157569), KeypressContext.tsx:817, outside this round's incremental scope
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; no interactive end-to-end lane exercised the real keypress dispatch that this PR's new exclusive tier gates (~95 useKeypress call sites).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; the Windows-only passthrough/pasteWorkaround path feeding this dispatch tier (KeypressContext.tsx:1277-1282), which R1-1's own trigger cites, was never exercised on Windows.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| * readline layer). Dismissal is therefore reported back to KeypressContext so | ||
| * the same dispatch falls through to the ordinary handlers the moment the menu | ||
| * is gone. While the menu is open the subscription is exclusive: keys are | ||
| * routed here only, so the composer, approval dialogs, and tab bars cannot act |
There was a problem hiding this comment.
[Suggestion] R2-2: This docstring claims more than the tier delivers. While the menu is open, "keys are routed here only, so the composer, approval dialogs, and tab bars cannot act on the same keystroke" is true for the keys the overlay claims and false for every key it declines — and the decline path re-emits the key into the ordinary dispatch in the same tick, where name-only matchers still act on it.
Concretely: a tool confirmation is pending inline, the user right-clicks a transcript link, then presses Shift+Enter to start a second composer line. The overlay declines it and broadcast falls through to the ordinary subscribers, where useSelectionList.ts:357 (if (name === 'return') dispatch({ type: 'SELECT_CURRENT' })) has no modifier test, so index 0 — ToolConfirmationOutcome.ProceedOnce at ToolConfirmationMessage.tsx:251-253 — is selected and the pending tool call executes. The keystroke was aimed at the composer; the menu closes, no newline appears, and a command the user did not approve runs.
The coexistence premise is confirmed here rather than assumed, and it is the opposite of what a dialogsVisible reading suggests: AppContainer.tsx:3912-3955's disjunction has no term for a pending tool confirmation (its only confirmation terms are !!shellConfirmationRequest and !!confirmationRequest, and confirmationRequest is set only by the extension-consent flows at slashCommandProcessor.ts:399/:1463), so MainContent.tsx:550's isActive={!uiState.dialogsVisible} keeps ContentMouseController active and its deactivation cleanup never fires. Reachability caveat: ContentMouseController.tsx:285-323 bails at if (items.length === 0) return;, so this needs a link or a selection in the viewport.
This is filed as a Suggestion rather than a blocker because it is not a regression. At the merge base broadcast was a bare for (const handler of subscribers) handler(key) with no exclusive tier, so the same key reached useSelectionList too — and the base overlay additionally fired executeIndex on any Return, so base was strictly worse in every state. What is attributable to this diff is the contract sentence quoted above.
Witness:
base arm: git show 779cfe913b:packages/cli/src/ui/contexts/KeypressContext.tsx
768: const broadcast = (key: Key) => {
774: for (const handler of subscribers) {
775: handler(key);
777: }; <- no exclusive tier; every Return reached useSelectionList
base overlay handler: if (key.name === 'return') { executeIndex(selectedIndex); return; }
<- no modifier test, so base fired the item too
HEAD gate: AppContainer.tsx:3912 const dialogsVisible = ... 3918: !!confirmationRequest ||
<- no pending-tool-confirmation term
MainContent.tsx:550 isActive={!uiState.dialogsVisible}
useSelectionList.ts:357 if (name === 'return') {
end-to-end behavioural run: not run — `review scratch-tree` was refused (CI runner's git-config `includeIf`),
and the scenario additionally needs a real pending tool approval plus an OSC-8 link in the transcript.
Two parts, and the docstring is the part this diff owns: correct the claim at :31-33 so it describes what the tier actually guarantees — that the keys the menu claims are routed here only. Separately, narrow the name-only Enter matches that the decline path now feeds, e.g. useSelectionList.ts:357 → if (name === 'return' && <unmodified>), mirroring the predicate this diff added to the overlay. Declining is still the right overlay behaviour — consuming the modified Return instead would re-create the lost newline — so the code fix belongs at the consumers that match return by name alone.
Any tightening there must special-case the backslash rewrite: KeypressContext.tsx:1015-1027 synthesises { ...key, shift: true, sequence: '\r' } for a bare Enter typed after a literal backslash (text-buffer.ts:2670 documents the same form), so a bare !key.shift — and also routing it through keyMatchers[Command.SUBMIT], which requires shift: false, ctrl: false, command: false per keyBindings.ts:210-218 — would break "Enter selects" on that route. The usable discriminator is the sequence: the rewrite is the only route producing name === 'return' && shift === true && sequence === '\r', whereas a genuinely modified Return keeps its decoded CSI-u text and kittyProtocol: true.
Please add the case to ToolConfirmationMessage.test.tsx that renders a pending confirmation inside ContextMenuProvider with ContextMenuOverlay, opens the menu, writes the Shift+Enter CSI-u form, and asserts the outcome callback was not called at all (in particular not with ProceedOnce) — then remove the modifier condition and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| return true; | ||
| } | ||
| if (key.name === 'up') { | ||
| // Claim the menu's keys only without modifiers: a modified Return or |
There was a problem hiding this comment.
[Suggestion] R2-3: The benefit this comment promises does not materialise. Releasing a modified Return instead of consuming it does not get the newline into the composer, because InputPrompt's pre-existing context-menu gate swallows it one layer down. So the guard genuinely fixes the half of its own sentence about firing the highlighted item, and the other half — "consuming it here would drop the newline" — is still true of the released key.
Right-click a transcript link to open the menu, keep composing, press Shift+Enter to start a second line. The overlay declines the key (unmodified false) and closeMenu(); return false; re-emits it into the ordinary subscriber loop in the same synchronous dispatch. React has not re-rendered, so InputPrompt.handleInput still reads a non-null contextMenu from its closure; its gate at InputPrompt.tsx:911-919 matches key.name === 'return' with no modifier test and returns true, so BaseTextInput.tsx:218 (if (onKeypress?.(key)) { return; }) short-circuits before keyMatchers[Command.NEWLINE] at :239. The menu closes, no newline is inserted, and the keystroke is gone with no feedback. Identical for Ctrl+Enter, Alt+Enter, the backslash+CR rewrite, and Shift+Up/Shift+Down. The new test cannot see this: InnocentBystander is a bare useKeypress subscriber with no menu awareness, and its assertion is only bystanderKeys.length > 0.
Filed as a Suggestion rather than a blocker because it is not a regression. InputPrompt.tsx:911-925 is byte-identical at the merge base and the file is absent from this PR's diff, so at base Shift+Enter with a menu open lost its newline and fired the menu item; HEAD is strictly better. This also corrects round 1's R1-12, which asserted "before it, the key also reached the ordinary handlers, so the newline landed" — round 1's own A/B row C disproves that. What is left is that the round's stated goal is not met while the comment claims it is.
Witness:
PR arm (executed): npx vitest run src/ui/context-menu/ContextMenuOverlay.test.tsx
Tests 11 passed (11)
✓ key exclusivity > Shift+Enter dismisses the menu and falls through without firing the item
base arm: .qwen/tmp/review-pr-11573-base built at 779cfe913
grep -c "exclusive\|unmodified" .../ContextMenuOverlay.tsx -> 0
base handler: if (key.name === 'return') { executeIndex(selectedIndex); return; }
gate identity: git show 779cfe913b:packages/cli/src/ui/components/InputPrompt.tsx -> 911-925 identical to HEAD
git diff 779cfe913b..HEAD --name-only -> InputPrompt.tsx absent
round 1's A/B row C (real composer path): BASE {"called":["x","\r"],"swallowed":["\r"],"submitted":[]}
newline A/B on a real BaseTextInput/TextBuffer: not run — `review scratch-tree` was refused (the repository's
git config has an `includeIf` pointing at a missing CI credentials file), so no probe file
could be written into an isolated arm.
Either give InputPrompt.tsx:911-919 the same modifier test the overlay now applies, or delete that gate now that the exclusive tier owns the keyboard — while a menu is open the overlay only ever forwards keys it declined, so the gate's return true branch is reachable only for the keys it should not swallow. Until one of those lands, please soften this comment so it does not promise an outcome the code does not produce.
if (contextMenu !== null) {
const unmodified = !key.ctrl && !key.shift && !key.meta;
if (
unmodified &&
(key.name === 'up' ||
key.name === 'down' ||
key.name === 'return' ||
key.name === 'escape')
) {
return true;
}
closeContextMenu();
}Three premises the fix rests on. BaseTextInput.tsx:218 — if (onKeypress?.(key)) { return; } — means a truthy return from handleInput suppresses every later branch including keyMatchers[Command.NEWLINE], so the gate cannot simply be narrowed to return false without changing what the composer does next. keyBindings.ts:210-218 pins Command.SUBMIT to { key: 'return', ctrl: false, command: false, paste: false, shift: false }, so plain Enter must still not reach BaseTextInput.tsx:229 while a menu is open, or menu-execute also submits the draft — the behaviour this PR exists to remove. And before deleting the gate outright, check the screen-reader layout: ContextMenuOverlay is mounted only in DefaultAppLayout.tsx:167, while ScreenReaderAppLayout.tsx:49 renders MainContent (and therefore ContentMouseController, the only caller of openMenu) with no overlay at all, so in that layout this gate is the only thing keeping a bare arrow/Enter/Esc out of the composer while a menu is open.
The witness this needs is a composer-level test, not a bystander-level one: render InputPrompt (or BaseTextInput with a TextBuffer) inside ContextMenuProvider alongside ContextMenuOverlay, open the menu, write the Shift+Enter CSI-u form, and assert the buffer gained a newline while no item's onSelect fired — then remove the modifier condition from the gate and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
…context menu R2-1: the bystander recorder now records the decoded key identity (name + modifier flags) instead of the raw sequence, and the Shift+Enter test asserts exactly 'return+shift' reached the ordinary handler — a kitty-decoding regression that shreds the sequence now fails the test instead of passing on stray printables. R2-4: a table-driven block pins the other cells of the modifier matrix (Shift+Up, Shift+Down, Ctrl+Enter, Alt+Enter): each must dismiss the menu, fall through to ordinary handlers, and neither move the highlight nor fire the item. Verified red under both guard mutations (predicate narrowed to shift-only; unmodified dropped from the up/down branches). R1-5: the ArrowDown+Enter test now mounts a bystander and asserts zero leaked keys, so flipping a claim branch's consumption (return true -> false) turns it red.
|
Round-2 review: R1-12 confirmed fixed — thank you. No Critical findings remained, so per the ~5-round review policy only the three highest-value Suggestions were addressed in dde0ff3; the rest are deferred below. R2-1 (imprecise bystander assertion) — Addressed. The R2-4 (single-cell matrix witness) — Addressed with the requested table-driven block: Shift+Up, Shift+Down, Ctrl+Enter, and Alt+Enter each must dismiss the menu, fall through to ordinary handlers, and neither move the highlight nor fire the item. Red-verified under both mutations the review named: narrowing the predicate to R1-5 (arrow consumption unpinned) — Addressed. The Deferred Suggestions (happy to land in a follow-up if maintainers prefer):
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-4 design-doc and shortcuts-page drift — already reported (comment 3982157544), ContextMenuOverlay.tsx:31
- R2-2 tier docstring scope for declined keys — already reported (comment 3985096290), ContextMenuOverlay.tsx:32
- R2-3 released-key comment rationale — already reported (comment 3985096295), ContextMenuOverlay.tsx:56
- R1-1 same-chunk trailing key vs. asynchronous dismissal — already reported (comment 3982157528), ContextMenuOverlay.tsx:77
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; no interactive end-to-end lane exercised the real keypress dispatch that this PR's new exclusive tier gates (~95 useKeypress call sites).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; the Windows-only passthrough/pasteWorkaround path feeding this dispatch tier (KeypressContext.tsx:1277-1282), which R1-1's own trigger cites, was never exercised on Windows.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1d": none — I did not run a full npm run typecheck for the package (Vitest transpiles without type-checking, so a type error in the it.each readonly-tuple callba….
Convergence: round 3 posted 3 inline comment(s), 2 of them reported for the first time; the previous round posted 7 (4 new). Findings keep coming back to the same files: packages/cli/src/ui/context-menu/ContextMenuOverlay.test.tsx (findings in round 2; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const { lastFrame, stdin } = renderWithProviders( | ||
| <Scene> | ||
| <ContextMenuProvider> | ||
| <InnocentBystander onKey={(k) => bystanderKeys.push(k)} /> |
There was a problem hiding this comment.
[Suggestion] R3-1: The arrow-consumption witness added to this test splits Down and Enter across two stdin.write calls with a 20 ms await wait() between them, which is structurally unable to reach the batched single-chunk path. On that path the overlay executes the stale highlighted item, because handleKeypress reads selectedIndex from the render closure while broadcast dispatches a whole chunk with no render in between. The codebase already documents and defends this hazard for menu — ContextMenuContext.tsx:87-89 and :101-103, which is why menuRef exists — but selectedIndex has no mirror.
Concretely: right-click a hyperlink (the items are Open Link, which calls openLink(url), then Copy Link Address), then tap Down and Enter fast enough that the terminal, tmux or SSH read coalesces both sequences into one chunk. The browser launches at the URL and the clipboard copy the user asked for never happens. Holding Down to move several rows has the same shape, since every key in a chunk computes from the same stale index, so N presses move the highlight by one.
To be clear about scope: this is not a regression from your PR, and we are not asking you to fix it in this round. We ran the same probe against the merge base and it fails identically there, so the defect predates this branch — and your change strictly narrows the harm, because the base also leaked both keys to ordinary subscribers while yours does not. It is filed at Suggestion so it stays visible as follow-up work rather than blocking a test-only round.
Witness (A/B, one identical probe file run in both arms):
PR one-chunk stdin.write('\u001b[B\r') => {"open":1,"copy":0,"bystanderKeys":[]}
PR gapped '\u001b[B' +20ms+ '\r' => {"open":0,"copy":1,"bystanderKeys":[]}
BASE one-chunk stdin.write('\u001b[B\r') => {"open":1,"copy":0,"bystanderKeys":["down","return"]}
BASE gapped '\u001b[B' +20ms+ '\r' => {"open":0,"copy":1,"bystanderKeys":["down","return"]}
FIX-1 (indexRef mirror) one-chunk => {"open":0,"copy":1} <- probe flips; 23/23 tests green
A follow-up issue is the right home for it. The shape that fixed it in our probe was resolving the highlight through live state instead of the render closure — a selectedIndexRef mirrored in ContextMenuProvider beside the existing menuRef. The functional-updater alternative (setSelectedIndexState((i) => Math.min(len - 1, i + 1))) needs a setSelectedIndex signature change, and we did not run it.
Any such fix must not disturb ContextMenuContext.tsx:104-107 — const current = menuRef.current; menuRef.current = null; setMenu(null); current?.items[index]?.onSelect(); nulls the mirror before the async setMenu specifically so a same-chunk double Enter cannot run the item twice (measured: stdin.write('\r\r') gives {"open":1}). A fix that re-reads render state or drops the mirror reintroduces that double execution. The ref-mirror form we ran kept \r\r at {"open":1} with all 15 of your tests green, so the constraint is satisfiable.
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
[Suggestion] R3-1: Still standing, at the severity round 3 ruled it — and to be explicit about scope again, this is not a regression from your PR and it is not a request to widen this round. The code it cites has moved since round 3: the split witness is now at :182 (stdin.write('\u001b[B')) and :184 (stdin.write('\r')) in the ArrowDown + Enter executes the second item case.
Splitting Down and Enter across two writes with a 20 ms await wait() between them is structurally unable to reach the batched single-chunk path. On that path the overlay executes the stale highlighted item, because handleKeypress reads selectedIndex from the render closure while broadcast dispatches a whole chunk with no render in between. The codebase already documents and defends this hazard for menu — ContextMenuContext.tsx:87-89 and :101-103, which is why menuRef exists — but selectedIndex is still a plain useState (ContextMenuContext.tsx:67) with no mirror.
Concretely: right-click a hyperlink (items are Open Link, which calls openLink(url), then Copy Link Address), then tap Down and Enter fast enough that the terminal, tmux or SSH read coalesces both sequences into one chunk. The browser launches at the URL and the clipboard copy the user asked for never happens. Holding Down to move several rows has the same shape, since every key in a chunk computes from the same stale index, so N presses move the highlight by one.
Witness — measured by A/B in round 3 (one identical probe file run in both arms), and still valid at this commit, because git diff --name-only dde0ff3e6714..HEAD returns only this test file, so ContextMenuOverlay.tsx and ContextMenuContext.tsx are byte-identical to the tree that probe ran against:
PR one-chunk stdin.write('\^[[B\r') => {"open":1,"copy":0,"bystanderKeys":[]}
PR gapped '\^[[B' +20ms+ '\r' => {"open":0,"copy":1,"bystanderKeys":[]}
BASE one-chunk stdin.write('\^[[B\r') => {"open":1,"copy":0,"bystanderKeys":["down","return"]}
BASE gapped '\^[[B' +20ms+ '\r' => {"open":0,"copy":1,"bystanderKeys":["down","return"]}
FIX-1 (indexRef mirror) one-chunk => {"open":0,"copy":1} <- probe flips; 23/23 tests green
The BASE rows are why this is filed as follow-up rather than as a blocker: the probe fails identically on the merge base, so the defect predates the branch, and your change strictly narrows the harm — the base also leaked both keys to ordinary subscribers while yours does not. The probe was not re-run this round; the mechanism was re-confirmed statically (selectedIndex still bare state, handleKeypress still closing over it).
A follow-up issue is the right home for this. The shape that fixed it in our probe was resolving the highlight through live state instead of the render closure — a selectedIndexRef mirrored in ContextMenuProvider beside the existing menuRef. The functional-updater alternative (setSelectedIndexState((i) => Math.min(len - 1, i + 1))) needs a setSelectedIndex signature change and was not run.
Any such fix must not disturb ContextMenuContext.tsx:104-107 — const current = menuRef.current; menuRef.current = null; setMenu(null); current?.items[index]?.onSelect(); nulls the mirror before the async setMenu specifically so a same-chunk double Enter cannot run the item twice (measured: stdin.write('\r\r') gives {"open":1}). A fix that re-reads render state or drops the mirror reintroduces that double execution; the ref-mirror form we ran kept \r\r at {"open":1} with all tests green, so the constraint is satisfiable.
The acceptance criterion is the probe flipping: a single-chunk stdin.write('\u001b[B\r') with the menu open must yield {"open":0,"copy":1} instead of {"open":1,"copy":0}, while stdin.write('\r\r') stays at {"open":1}.
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
[Suggestion] R3-1: Still standing, at the severity round 3 ruled it — and, as in every round since, this is not a regression from your PR and not a request to widen this one. handleKeypress still reads selectedIndex from the render closure (ContextMenuOverlay.tsx:62,66,70) while broadcast dispatches a whole stdin chunk with no render between, so Down+Enter coalesced into one chunk executes the stale highlighted item. selectedIndex is still a plain useState with no ref mirror, unlike menu, which ContextMenuContext.tsx:87-89 documents and defends with menuRef for exactly this hazard.
The code this round added neither fixes nor blesses it: the new clamp test splits its chunks with await wait() and says so in the comment this is anchored to, asserting nothing about the single-chunk path. The cited production files are byte-identical to the tree round 3's A/B probe ran against — git diff --name-only 589707e491..HEAD returns only this test file — so that measurement still stands and was not re-run.
Concretely: right-click a hyperlink (the items are Open Link, which calls openLink(url), then Copy Link Address), then tap Down and Enter fast enough that the terminal, tmux or SSH read coalesces both sequences into one chunk. The browser launches at the URL and the clipboard copy the user asked for never happens. Holding Down to move several rows has the same shape — every key in a chunk computes from the same stale index, so N presses move the highlight by one.
Witness:
round-3 A/B, one identical probe file run in both arms; still valid at this commit because the
production files are unchanged (git diff --name-only 589707e491..HEAD => only this test file)
PR one-chunk Down+Enter => {"open":1,"copy":0,"bystanderKeys":[]}
PR gapped Down +20ms+ Enter => {"open":0,"copy":1,"bystanderKeys":[]}
BASE one-chunk Down+Enter => {"open":1,"copy":0,"bystanderKeys":["down","return"]} <- pre-existing
FIX-1 (indexRef mirror) => {"open":0,"copy":1} <- probe flips
witness: not re-run this round — the mechanism was re-confirmed statically against 918cb3cb17
A follow-up issue is still the right home for this. The shape that fixed it in the round-3 probe was resolving the highlight through live state instead of the render closure — a selectedIndexRef mirrored in ContextMenuProvider beside the existing menuRef. Any such fix must not disturb ContextMenuContext.tsx:104-107, which nulls the mirror before the async setMenu specifically so a same-chunk double Enter cannot run the item twice (measured: a doubled Return gives {"open":1}); a fix that re-reads render state or drops the mirror reintroduces that double execution. The ref-mirror form that was run kept the doubled Return at {"open":1} with all tests green, so the constraint is satisfiable.
Acceptance criterion: the probe flipping — a single-chunk Down+Enter with the menu open must yield {"open":0,"copy":1} instead of {"open":1,"copy":0}, while a doubled Return stays at {"open":1}.
— qwen3.8-max via Qwen Code /review (v0.23.3)
…ty fixture
R1-5 (round 3): no test pressed an unmodified Up while a menu is open, so
the up claim branch's return true was unpinned — flipping it to false left
all 15 tests green. Add 'an unmodified ArrowUp is consumed by the open
menu' using the legacy unparameterized sequence (the only Up that decodes
as a bare up under the kitty protocol). Verified by mutation: flipping
ContextMenuOverlay.tsx's up-branch return true to false turns exactly this
test red (bystander receives 'up'), and the restored file is 16/16 green.
R3-2 (round 3): ExclusiveScene was declared inside describe('key
exclusivity') while 'ArrowDown + Enter' spelled the identical tree inline
in the outer describe, so the fixture had two definitions that could drift
apart. Hoist ExclusiveScene to module scope beside Scene and MenuOpener
and use it in ArrowDown + Enter, leaving one definition.
|
Round 3 — both actionable items addressed in 589707e (test-only, no production change). R1-5 (up branch) — fixed. Added R3-2 (fixture duplication) — fixed. R3-1 (stale On convergence: agreed with land-and-defer — this round adds one test and hoists a fixture; nothing here widens behavior. If anything else comes up, we'd ask that it land as follow-up issues rather than further rounds on this branch. Note for the next review pass: this comment's anchor |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- declined modified Return still swallowed by the composer's pre-existing context-menu gate — already reported as R2-3 (comment 3985096295), ContextMenuOverlay.tsx:56
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; no interactive end-to-end lane exercised the real keypress dispatch this PR's new exclusive tier gates (~95 useKeypress call sites).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; the Windows-only passthrough/pasteWorkaround path feeding this dispatch tier (KeypressContext.tsx:1277-1282), which R1-1's own trigger cites, was never exercised on Windows — and Windows is the only OS the author's Test Plan marks as tested.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally; the unit suite ran on Linux only.
Convergence: round 4 posted 3 inline comment(s), 2 of them reported for the first time; the previous round posted 3 (2 new). Findings keep coming back to the same files: packages/cli/src/ui/context-menu/ContextMenuOverlay.test.tsx (findings in rounds 1, 3; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
— qwen3.8-max via Qwen Code /review (v0.23.3)
…ArrowUp clamp effect
|
Round-4 review: both Suggestions are addressed in 918cb3c (test-only, no production change). The review's convergence recommendation — batch the remaining fixes, verify, then push — is exactly what this commit does: both items landed together with the mutation witnesses below run before the push. R4-1 (single bystander could not tell all-subscribers fan-out from first-only) — Mutation confirmed as requested: truncating declined-path fan-out to the first ordinary subscriber in R4-2 (the new Up consumption witness pinned no effect) — added All three mutants named in the review turn exactly this test red (each: 1 failed | 16 skipped under
Pristine: 17/17 in the overlay file; combined run with These two land as the round's full set; per the convergence note we're keeping this branch at test-only since round 2 and asking that anything further land as follow-up issues rather than more rounds here. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; no interactive end-to-end lane exercised the real keypress dispatch this PR's new exclusive tier gates (~95 useKeypress call sites).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; the Windows-only passthrough/pasteWorkaround path feeding this dispatch tier (KeypressContext.tsx:1277-1282) was never exercised on Windows, and Windows is the only OS the author's Test Plan marks as tested.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally; the unit suite ran on Linux only.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/context-menu/ContextMenuOverlay.test.tsx:387 — [probe] post-close assertion does not pin delivery countpackages/cli/src/ui/context-menu/ContextMenuOverlay.test.tsx:386 — [probe] release test presses a declined key, so a leaked exclusive tier stays green
Convergence: round 5 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 3 (2 new). Findings keep coming back to the same files: packages/cli/src/ui/context-menu/ContextMenuOverlay.test.tsx (findings in rounds 3, 4; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
Round-5 review: R5-1 is addressed in 4885edd (test-only); R3-1 stays deferred at the severity and scope round 3 ruled. R5-1 (down clamp had no effect witness) — added Mutation confirmed as requested: replacing R3-1 (stale That closes every open item from this round. The convergence posture stands: this branch's last three commits are test-only, and we'd ask that anything further land as follow-up issues rather than more review rounds here. |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
@qwen-code /review |
1 similar comment
|
@qwen-code /review |
|
Housekeeping: merged upstream/main into the branch as d294a0a. No conflicts; the branch's own suites (ContextMenuOverlay, KeypressContext) re-verified green (139/139) plus build and typecheck clean. This should also clear the Lint & Static lane, whose failure was lint-gate freshness — main moved eslint.config.js / eslint.legacy-filenames.mjs (1e76897 / ae78d5b) after this branch's last merge, and the gate checks out the branch head against main's current config. No behavioral change; the diff remains the dispatch-tier exclusivity fix and its tests. |
|
Follow-up on the ubuntu What I verified:
Filed #11850 upstream with the full analysis and suggested deterministic refill hooks. Nothing branch-side to change — leaving the code as-is. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 same-chunk trailing key after asynchronous dismissal — already reported (comment 3982157528), ContextMenuOverlay.tsx:77
- R2-3 released modified Return swallowed by the composer's pre-existing context-menu gate — already reported (comment 3985096295), ContextMenuOverlay.tsx:56
- R1-4 design-doc and shortcuts-page drift — already reported (comment 3982157544), keyboard-shortcuts.md:135
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; no interactive end-to-end lane exercised the real keypress dispatch that this PR's new exclusive tier gates (~95 useKeypress call sites).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; the Windows-only passthrough/pasteWorkaround path feeding this dispatch tier (KeypressContext.tsx:1277-1282), which R1-1's own trigger cites, was never exercised on Windows, and Windows is the only OS the author's Test Plan marks as tested.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally; the unit suite ran on Linux only.
Not explored to full depth (tool budget reached): "agent 6b": did not verify whether the kitty protocol decoder in this build ever broadcasts key-*release* events (the enable flags live outside KeypressContext.tsx ); if i….
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/context-menu/ContextMenuOverlay.test.tsx:50 — [review] Test Plan and description verify two surfaces that cannot collide with an open menu at this commitpackages/cli/src/ui/context-menu/ContextMenuOverlay.test.tsx:247 — [review] arrow-consumption witness splits Down and Enter across two writes, so it cannot reach the batched single-chunk path
— qwen3.8-max via Qwen Code /review (v0.23.3)
This branch has not been deployed
3 participants
What this PR does
Makes the right-click context menu own the keyboard while it is open, enforced at the keypress dispatch layer rather than by teaching every key consumer about the menu. The keypress context now has an exclusive subscription tier: while an exclusive handler is subscribed, ordinary handlers do not receive keys at all. The menu overlay subscribes exclusively for exactly as long as it is open — it consumes up/down/return/escape, and on any other key it dismisses itself and declines the key, so that same keystroke falls through to the ordinary pipeline (typing "x" with the menu open dismisses the menu and types "x"). Before this change the dispatch was a pure broadcast, so with a menu open: Enter executed the menu item and also submitted the composer's half-typed draft or confirmed the tool-approval dialog (down+enter could land on "Always allow in this project" — a persistent project-wide permission grant written by a keystroke aimed at the menu); Escape dismissed the menu and also cancelled the running stream or wiped the composer draft; Down moved the menu highlight and also switched agent tabs.
Why it's needed
Entrance-by-entrance gating had not converged: on the teammate-tab PR three consecutive review rounds each closed one entrance of this family and left the next open. The durable fix the issue asks for is exclusivity at the dispatch layer, with two constraints this PR respects: the dismissing key must still fall through after the menu closes (returning early would drop it into the bare readline layer), and the mouse controller's own activation must not be gated (its menu-open branch is the only hover/click/dismiss path for the open menu). The existing per-consumer gating in the main input prompt and the app container is untouched — it stays correct and now redundant-safe.
Reviewer Test Plan
How to verify
Build and open the interactive CLI in a terminal with mouse support, trigger a tool confirmation, right-click the transcript to open the context menu, then press Enter / Escape / Down: each key must act on the menu only — the approval dialog must not confirm or cancel, the composer must not submit or clear, tabs must not switch. Then press a printable key: the menu must dismiss and the character must appear in the composer in the same keystroke. Close the menu and confirm keys reach the app normally again. Unit tests cover all four behaviors at the dispatch layer and at the overlay level with a bystandar handler.
Evidence (Before & After)
Non-trivial UI behavior; before/after TUI recording pending — unit tests included (3 dispatch-layer tests, 4 overlay exclusivity tests, all green along with the existing 167 keypress/context-menu tests and 452 consumer-surface tests: InputPrompt, AppContainer, AgentComposer, ToolConfirmationMessage).
Tested on
Environment (optional)
npm run dev + vitest unit tests, Windows sandbox none.
Risk & Scope
subscribe(handler)without options behaves exactly as before;KeypressHandler's return type widened fromvoidtounknown(source-compatible).Linked Issues
Fixes #11228