fix(cli): keep keypress handlers current - #5421
Conversation
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hey @tt-a1i, thanks for the fix — stale keypress handlers are a real pain, and the ref pattern looks like a solid approach.
The PR body doesn't follow the PR template though. Several required sections are missing:
- What this PR does / Why it's needed — the "Summary" section covers the what but there's no motivation explaining why the stale-closure bug matters or what user-visible symptom it causes
- Reviewer Test Plan — needs "How to verify", "Evidence (Before & After)", and the "Tested on" table
- Risk & Scope — what could break, what's out of scope
- Linked Issues — reference the flaky test or the PR #5415 investigation that surfaced this
- Chinese translation (
<details>中文说明</details>)
Could you update the PR description to match the template? It helps reviewers (and future-you reading git blame) understand the context quickly. The actual code change looks reasonable — just the framing that needs work. 🙏
中文说明
@tt-a1i 你好，感谢修复——keypress handler 闭包过期是个真实痛点，ref 模式看起来方向正确。
不过 PR 描述没有按照 PR 模板 来写，缺少几个必填部分：
- What this PR does / Why it's needed — "Summary" 说了做了什么，但没有解释为什么要改、用户看到了什么症状
- Reviewer Test Plan — 需要 "How to verify"、"Evidence (Before & After)" 和 "Tested on" 表格
- Risk & Scope — 可能影响什么、什么不在范围内
- Linked Issues — 关联到发现这个问题的 PR #5415 或相关 issue
- 中文翻译 (
<details>中文说明</details>)
请按模板更新一下描述就好，代码改动本身看起来没问题。🙏
— Qwen Code · qwen3.7-max
|
updated the PR description to match the template. code is unchanged. |
|
|
||
| onKeypressRef.current = onKeypress; | ||
|
|
||
| const handleKeypress = useCallback<KeypressHandler>((key) => { |
There was a problem hiding this comment.
[Critical] The core fix (ref + useCallback) ensures the latest onKeypress is always invoked even when its identity changes across renders — but useKeypress.test.ts has no test that exercises this behavior. Every existing test passes a single vi.fn() that never changes identity. If a future refactor silently reverts to the stale-closure behavior, the hook's own unit tests will still pass; the regression would only surface in an unrelated component test.
Consider adding a regression test:
it('always invokes the latest onKeypress callback after re-render', () => {
const first = vi.fn();
const second = vi.fn();
const { rerender } = renderHook(
({ handler }) => useKeypress(handler, { isActive: true }),
{ initialProps: { handler: first }, wrapper },
);
rerender({ handler: second });
act(() => stdin.pressKey({ name: 'a', sequence: 'a' }));
expect(first).not.toHaveBeenCalled();
expect(second).toHaveBeenCalledTimes(1);
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
added a focused hook-level regression test for rerendered handlers.
|
|
||
| const wait = (ms = 50) => new Promise((resolve) => setTimeout(resolve, ms)); | ||
| const clean = (value: string | undefined) => stripAnsi(value ?? ''); | ||
| const waitForFrame = async ( |
There was a problem hiding this comment.
[Suggestion] waitForFrame is functionally identical to vi.waitFor from vitest, which is already used 17+ times in neighboring test files (e.g., HooksManagementDialog.test.tsx). The custom helper adds ~20 lines of duplicate infrastructure.
Replace with vi.waitFor and delete the helper:
await vi.waitFor(() => {
expect(clean(lastFrame())).toContain('❯ 4.');
}, { interval: 10 });— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
done, switched the dialog waits to vi.waitFor and removed the custom helper.
| expect(clean(lastFrame())).toContain('❯ 4.'); | ||
| await waitForFrame(() => { | ||
| expect(clean(lastFrame())).toContain('❯ 4.'); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] After the Ctrl+P/N round-trip, the assertion only checks ❯ 4. — it doesn't verify that the custom input text ('jk') is still present. Data loss during navigation would go undetected.
| }); | |
| await waitForFrame(() => { | |
| const frame = clean(lastFrame()); | |
| expect(frame).toContain('❯ 4.'); | |
| expect(frame).toContain('jk'); | |
| }); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
done, the Ctrl+P/N round-trip now also checks that the custom input text is still there.
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual, test plan included. On direction: this fixes a real timing bug where On approach: the scope is tight — +55/-11 across two files. The fix uses the idiomatic React pattern (ref for latest handler + stable Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必填章节齐全，双语，测试计划已包含。 方向：修复了一个真实的时序 bug —— macOS 上 方案：范围紧凑 —— 两个文件 +55/-11。使用标准 React 模式（ref 存最新 handler + 稳定的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading diff): The problem is stale keypress handlers during the render→effect window. The idiomatic fix is to store the handler in a ref updated on each render, and subscribe/unsubscribe with a stable wrapper via Comparison with PR: The PR's approach matches my proposal exactly. The implementation is clean:
The No blockers found. No correctness issues, no security concerns, no AGENTS.md violations. The code is minimal, idiomatic, and well-scoped. Test ResultsUnit tests for both affected files pass on PR branch and on main: ESLint: clean. Prettier: clean. Real-Scenario TestingThis fix targets a sub-frame React hook subscription timing issue that manifests only under specific React scheduling conditions on macOS CI. It's not reproducible through manual TUI interaction — the timing window is between a render pass and the subsequent 中文说明代码审查独立方案（读 diff 前）： 问题是 render→effect 窗口内过期的 keypress handler。标准修复是用 ref 存最新 handler，用 与 PR 对比： PR 方案和我的提案完全一致。实现干净：
无阻塞问题。 无正确性问题，无安全隐患，无 AGENTS.md 违反。代码精简、规范、范围恰当。 测试结果PR 分支和 main 上两个受影响文件的单元测试均通过：32 passed / 1 skipped。ESLint 和 Prettier 均通过。 真实场景测试此修复针对的是 React hook 订阅的亚帧级时序问题，仅在 macOS CI 的特定 React 调度条件下出现。通过手动 TUI 操作无法复现 —— 时序窗口在 render pass 和后续 useEffect cleanup/setup 周期之间，对正常速度打字的人类用户不可见。单元测试（特别是用 — Qwen Code · qwen3.7-max |
|
This is a clean, well-scoped fix for a real timing bug. The ref+stable-callback pattern is exactly what I'd write independently. The test improvement (replacing fixed waits with polling) is a genuine quality upgrade — it makes the test both faster and more robust. Two files, 55 additions, 11 deletions, zero waste. Lint, format, and all 32 tests pass. The prior Approving. ✅ 中文说明这是一个干净、范围恰当的时序 bug 修复。ref + 稳定 callback 的模式和我自己会写的一模一样。测试改进（用轮询替换固定等待）是真正的质量提升 —— 让测试更快也更稳健。 两个文件，55 行新增，11 行删除，零浪费。Lint、格式化和全部 32 个测试通过。之前的 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-reviewed at HEAD a626497f73 — the fix is correct. The latest-ref + stable useCallback pattern keeps the subscription stable while always dispatching to the current onKeypress, removing the re-subscribe-per-render window that let a rapid Ctrl+P/N keypress hit a stale/absent handler. Public signature is unchanged, so all useKeypress callers benefit with no behavior change. typecheck clean; useKeypress + AskUserQuestionDialog tests pass (32, incl. the de-flaked Ctrl-nav test); CI green (48 checks).
No new issues. The 3 items from my earlier review remain open (already inline, all test-quality — the implementation itself is sound):
useKeypress.ts:29— the core fixed behavior (latest handler after identity change) has no focused hook-level unit test. The AskUserQuestionDialog integration test does exercise it, but arenderHookregression test would stop a silent revert to the stale-closure behavior.AskUserQuestionDialog.test.tsx:16—waitForFrameduplicates vitest'svi.waitFor(used 17+ times in neighboring tests); prefer it over the custom helper.AskUserQuestionDialog.test.tsx:356— the post-Ctrl+P/N assertion checks❯ 4.but not that the custom input ('jk') survived the round-trip.
— claude-opus-4-8 via Claude Code /qreview
✅ Local verification — PR #5421
|
| Hook | subscribe calls |
unsubscribe calls |
original registration reports |
|---|---|---|---|
| base (pre-PR) | 2 | 1 | [1] — stale |
| PR | 1 | 0 | [2] — current |
The base re-subscribes on every render, and the function the dispatcher holds goes stale in the window between render-commit and the effect re-running. The PR subscribes once, and that single registration always calls the latest closure via the ref. That window is the real-terminal win: a keypress arriving there hits a stale handler on base, a current one on the PR.
3 · Important nuance — the new test passes on the unfixed hook too
I ran the PR's own regression test (keeps bare k/j in custom input while Ctrl+P/N still navigates options) against the base hook 10× → 10/10 pass. Because the new waitForFrame + await wait() give the base hook's effect time to re-subscribe between keystrokes, the test does not fail on the unfixed hook. So:
- the test change de-flakes the previously-flaky test (that's the real value of the polling), and
- the hook change fixes the real-terminal race that a key-by-key awaited test can't reproduce.
Both are good; they fix different layers.
4 · Real terminal (tmux PTY) — real component + real useKeypress (PR)
4 → ❯ 4. > Type something... (Other selected, custom input active)
j → ❯ 4. > j (TYPED, not consumed as vim-down)
k → ❯ 4. > jk (TYPED)
C-p → ❯ 3. Green | 4. jk (Ctrl+P navigates up, value preserved)
C-n → ❯ 4. > jk (Ctrl+N back to custom input)
Exactly the contract in the test name — end-to-end through the real keypress pipeline.
5 · Honesty note — the raw race is not cleanly tmux-reproducible
Firing 4jk as a single un-paced write lands on ❯ 1. Red with an empty custom input on both base and PR — because no React render happens between the bytes, so even the PR's ref can't update. Paced input works on both. The base↔PR difference lives only in the narrow render→effect window, which send-keys timing can't reliably hit — which is why §2's hook-level A/B is the proper proof, not a tmux A/B.
6 · Regression sweep (other useKeypress consumers)
useKeypress + TextInput + RadioButtonSelect + AskUserQuestionDialog → 43 passed.
(InputPrompt.test.tsx fails to load in my sandbox on an unrelated ink@7.0.3 Missing "./dom" specifier — pre-existing dep skew via BaseTextInput, fails identically with and without the PR.)
Minor, non-blocking
- The ref is assigned in the render body (
onKeypressRef.current = onKeypress). Fine for Ink (synchronous renderer, no StrictMode double-render); if the TUI ever moves to concurrent React, prefer assigning it inuseLayoutEffect. - As shown in §3, the shipped integration test guards the timing flake but passes on the unfixed hook — a hook-level test like the §2 probe would be a stronger regression guard for the handler-currency behavior itself. Optional.
中文版（验证报告）
✅ 本地验证 — PR #5421 fix(cli): keep keypress handlers current
结论：LGTM，可以合并。 我把 PR 应用到当前 main 上，在 hook 层用确定性 A/B 证明了修复机制，并通过真实 tmux PTY 驱动了真实的 AskUserQuestionDialog（真实 KeypressProvider/useKeypress）。
关于合并范围：分支里有两个 commit —— keypress 修复，以及一个已经在
main里的 cron commit(#5230)的副本 —— 所以 GitHub 的 diff（和真实合并）就是这 2 个文件。我只 cherry-pick 了 keypress commit；两个改动文件在分支基线和当前main之间逐字节一致（0 行 diff），因此这就是真实的净合并内容。
这个 PR 改了什么
useKeypress.ts：只subscribe一个稳定包装函数一次，通过onKeypressRef.current读取最新 handler；不再直接订阅onKeypress闭包（那会导致每次渲染都 unsubscribe + 重新 subscribe）。这是经典的 latest-ref / "useEvent" 模式。AskUserQuestionDialog.test.tsx：把固定的await wait(150)换成轮询waitForFrame(...)，并加强断言（输入j后画面含j，输入k后含jk）。
针对的 bug：在对话框里，裸 j/k 在普通分支是 vim 导航（SELECTION_DOWN/UP），但选了 "Other" 之后必须当作文本输入。如果 handler 在两次渲染之间变“陈旧”，它仍以为没选 "Other"，于是被输入的 j/k 会被当成光标导航吞掉。
验证（全部为真实执行）
-
测试通过、无 flake：两个测试文件 32 passed / 1 skipped；对话框完整套件跑 5 次，每次
17 passed / 1 skipped，稳定。 -
hook 层确定性 A/B（mock 掉 dispatcher 的
subscribe/unsubscribe，捕获挂载时注册的函数，再以新闭包重渲染后调用它）：Hook subscribeunsubscribe最初注册的函数返回 base（修复前） 2 1 [1]— 陈旧PR 1 0 [2]— 最新base 每次渲染都重订阅，在“渲染提交 → effect 重新执行”之间 dispatcher 持有的函数是陈旧的；PR 只订阅一次，该注册始终通过 ref 调用最新闭包。这个窗口就是真实终端里的区别所在。
-
关键细节 —— 新测试在“未修复”的 hook 上也通过：把 PR 自带的回归测试在 base hook 上跑 10 次 → 10/10 通过。因为新的
waitForFrame+await wait()给了 base 的 effect 足够时间在两次按键之间重订阅。所以：测试改动消除了原本的测试 flake；hook 改动修的是真实终端里的竞态（逐键 await 的测试无法复现）。两者修的是不同层面。 -
真实终端（tmux PTY，PR）：
4→ 行 4 自定义输入激活；j→❯ 4. > j（被输入，未被当导航吞）；k→❯ 4. > jk；Ctrl+P→❯ 3. Green（向上导航，值保留）；Ctrl+N→❯ 4. > jk。完全符合测试名所述契约。 -
诚实说明 —— 原始竞态无法在 tmux 干净复现：把
4jk作为一次性无间隔写入，base 和 PR 都落到❯ 1. Red且自定义输入为空 —— 因为字节之间没有 React 渲染，PR 的 ref 也无法更新。有间隔的输入两者都正常。base↔PR 的差异只存在于很窄的“渲染→effect”窗口，send-keys的时序无法稳定命中 —— 所以正确的证明是 §2 的 hook 层 A/B，而不是 tmux A/B。 -
回归扫描：
useKeypress+TextInput+RadioButtonSelect+AskUserQuestionDialog→ 43 passed。（InputPrompt.test.tsx在我的沙箱里因无关的ink@7.0.3缺少"./dom"导出而加载失败 —— 经由BaseTextInput的既有依赖版本错配，带不带 PR 都一样失败。）
次要、不阻塞
- ref 是在渲染体里赋值（
onKeypressRef.current = onKeypress）。对 Ink（同步渲染、无 StrictMode 双渲染）没问题；若将来 TUI 迁到 concurrent React，建议改到useLayoutEffect里赋值。 - 如 §3 所示，随 PR 提交的集成测试守的是“时序 flake”，在未修复的 hook 上也会通过 —— 像 §2 那样的 hook 层测试能更强地守住“handler 始终最新”这一行为本身。可选。
3 participants
What this PR does
This keeps keypress subscriptions stable while still dispatching to the latest handler from the current render. It also tightens the AskUserQuestionDialog regression test so bare
j/kstay in the custom input, while Ctrl+P/Ctrl+N still move the selected option.Why it's needed
While checking #5415, the macOS test run exposed a timing issue where AskUserQuestionDialog could receive input through a stale keypress callback. In that state, typing
jafter moving to the custom input could be handled as list navigation instead of text input. Keeping the subscribed wrapper stable removes that post-render/pre-effect stale handler window without changing the keypress context API.Reviewer Test Plan
How to verify
Confirm that AskUserQuestionDialog keeps the custom input selected when bare
j/kare typed, and that Ctrl+P/Ctrl+N still navigate choices. The targeted test covers that regression together with the existinguseKeypressbehavior.Evidence (Before & After)
Before: the macOS CI run seen while checking #5415 failed the AskUserQuestionDialog custom input test because
jmoved selection back toBlueinstead of staying onOther. After: the targeted AskUserQuestionDialog and useKeypress test files pass locally, and this PR's CI passed on macOS, Windows, and Linux.Tested on
Environment (optional)
Node 22 via
npx -p node@22.Commands run locally:
npx -p node@22 node node_modules/vitest/vitest.mjs run --coverage.enabled=false packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx packages/cli/src/ui/hooks/useKeypress.test.tsnpx prettier --check packages/cli/src/ui/hooks/useKeypress.ts packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsxnpx eslint packages/cli/src/ui/hooks/useKeypress.ts packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsxgit diff --checkRisk & Scope
useKeypressnow keeps one subscribed wrapper per active registration and reads the latest handler from a ref, so any issue would likely show up around subscription ordering or active/inactive toggles.Linked Issues
Related: #5415
中文说明
What this PR does
这个 PR 让 keypress 订阅保持稳定，同时仍然把事件分发给当前 render 里的最新 handler。它也收紧了 AskUserQuestionDialog 的回归测试，确认普通
j/k会留在自定义输入框里，而 Ctrl+P/Ctrl+N 仍然能移动选项。Why it's needed
检查 #5415 时，macOS 测试暴露出一个时序问题：AskUserQuestionDialog 可能通过旧的 keypress callback 收到输入。在这种状态下，移动到自定义输入框后输入
j，有机会被旧逻辑当成列表导航，而不是文本输入。稳定订阅 wrapper 可以去掉 render 之后、effect 更新之前这段 stale handler 窗口，同时不改 keypress context API。Reviewer Test Plan
How to verify
确认 AskUserQuestionDialog 在输入普通
j/k时仍然选中自定义输入框，同时 Ctrl+P/Ctrl+N 仍然可以切换选项。目标测试覆盖了这个回归，也覆盖了现有useKeypress行为。Evidence (Before & After)
Before：检查 #5415 时看到的 macOS CI 在 AskUserQuestionDialog 自定义输入测试失败，因为
j把选中项移动回了Blue，没有留在Other。After：本地目标 AskUserQuestionDialog 和 useKeypress 测试通过，本 PR 的 CI 也已经在 macOS、Windows、Linux 通过。Tested on
Environment (optional)
通过
npx -p node@22使用 Node 22。本地运行过：
npx -p node@22 node node_modules/vitest/vitest.mjs run --coverage.enabled=false packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx packages/cli/src/ui/hooks/useKeypress.test.tsnpx prettier --check packages/cli/src/ui/hooks/useKeypress.ts packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsxnpx eslint packages/cli/src/ui/hooks/useKeypress.ts packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsxgit diff --checkRisk & Scope
useKeypress现在对每个 active registration 保持一个订阅 wrapper，并从 ref 读取最新 handler；如果有问题，大概率会出现在订阅顺序或 active/inactive 切换上。Linked Issues
Related: #5415
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.