feat(web-shell): honor voice hold mode - #7839
Conversation
|
|
|
Thanks for the PR! Template looks good ✓ Problem: this is a real, observed gap — the daemon already persists and returns Direction: squarely aligned. This is honoring an existing daemon contract, not adding a new surface. No CHANGELOG reference needed — the daemon-side mode support is already shipped; this completes the loop on the Web Shell side. Size: not applicable — no core paths touched ( Approach: the scope feels right. Pointer-event handlers for hold, click preserved for tap and keyboard, a Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个真实的、可观测的缺口——daemon 已经通过 方向：完全对齐。这是在履行已有的 daemon 协议，不是新增功能面。无需 CHANGELOG 引用——daemon 侧的 mode 支持已经发布，这个 PR 在 Web Shell 侧补完了闭环。 规模：不适用——未触及核心路径（仅 方案：范围合理。hold 模式用 Pointer Event 处理，tap 模式和键盘保留 click， 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: given the problem (Web Shell ignores the daemon's Comparison with the diff: the PR does exactly this, and does it cleanly. A few observations:
No critical blockers. No AGENTS.md violations (ESM, strict TS, Test evidenceCI on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Real-scenario testing: N/A — this is a browser-side Web Shell interaction change (Pointer Events on a mic button), not a TUI surface. tmux capture would not exercise it. The PR's own focused DOM tests (hold start/release, release-while-connecting, pointer cancel, tap toggle, keyboard activation) cover the interaction model; real-device browser verification is noted by the author as out of scope. 中文说明代码审查独立方案： 给定问题（Web Shell 忽略 daemon 的 与 diff 对比： PR 完全这样做了，而且很干净。几个观察：
无关键阻塞。无 AGENTS.md 违规（ESM、严格 TS、 测试证据
真实场景测试：不适用——这是浏览器端 Web Shell 交互变更（麦克风按钮上的 Pointer Events），不是 TUI 界面。tmux 捕获无法演练它。PR 自身的聚焦 DOM 测试（hold 开始/释放、连接中释放、指针取消、tap 兼容、键盘触发）覆盖了交互模型；真实设备浏览器验证被作者标注为范围外。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, minimal implementation that exactly matches what the problem calls for; CI still pending. Stepping back: the daemon already speaks The code reads well. The Every change in the diff is necessary for the stated goal. No drive-by edits, no scope creep. If I had to maintain this in six months, I'd thank the author. Approval deferred until CI lands green on 中文说明置信度：4/5 —— 干净、最小的实现，完全匹配问题所需；CI 仍在运行。 退一步看：daemon 已经能说 代码读起来很舒服。 diff 中的每个改动都是目标所必需的。没有顺手改动，没有范围蔓延。如果六个月后我来维护，我会感谢作者。 审批推迟到 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
发现一个会导致 hold 模式录音静默丢失的阻塞问题，详见行内评论。
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
连接期间释放的问题仍未真正解决：当前实现会完成空转写，但没有保留用户在连接期间说出的音频，详见行内评论。
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
复核了当前 head 838d0885559fb190d1a5e5275370a538741fb632f3，未发现新的阻塞问题。
此前两条 P1 在当前实现中已经实质修复：连接期间会先本地缓冲 PCM，socket 打开后按 start → buffered PCM → deferred stop 发送；release 会立即停止音频图，abort/cleanup 也会清除 pending stop，避免后续误提交。现有未解决项是缓冲区溢出路径缺少测试，属于非阻塞 Suggestion。
验证证据：
- 隔离 worktree 运行
VoiceButton.test.tsx与useVoiceCapture.test.tsx：31/31 通过 git diff --check upstream/main...838d0885通过- CI 的 Linux lint / format / build / tests 均通过；Web Shell E2E 在 checkout 阶段被取消，尚未执行到本 PR 的浏览器 smoke，因此不归因于该改动
| holdPointerIdRef.current = event.pointerId; | ||
| try { | ||
| event.currentTarget.setPointerCapture(event.pointerId); | ||
| } catch { |
There was a problem hiding this comment.
[Critical] Pointer capture is set on the icon <button>, but when ws.onopen fires and status transitions to recording, React unmounts the icon button and mounts a structurally different pill button. Per the Pointer Events spec (§5.3), removing a captured element from the DOM implicitly releases its pointer capture. The subsequent pointerup fires on the document or the new pill button — which was never in the capture path — so handlePointerUp never fires. The synthesized click is suppressed by the event.detail !== 0 guard in handleClick. stop() is never called and recording continues indefinitely.
The design doc states: "releasing outside the button still finishes a hold" — this guarantee is not achieved. The tests mask this by manually dispatching pointerup on the re-queried (new) button element after act(() => root.render(...)), which does not reflect what a real browser delivers.
Failure scenario: User presses and holds the mic icon in hold mode → start() fires → WebSocket opens → status becomes recording → React swaps the icon button for the pill button → pointer capture is implicitly released → user releases the pointer → pointerup fires elsewhere → stop() is never called → recording runs until external termination.
Consider either (a) rendering a single stable <button> element whose children change conditionally — preserving DOM identity and pointer capture across status transitions — or (b) attaching a pointerup/pointercancel listener to document when a hold starts, and routing release/cancel through that listener instead of relying on per-button React event handlers.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Declined as a false positive — leaving this thread open for a maintainer to confirm.
The finding's load-bearing premise is that when status transitions to recording, React unmounts the icon <button> and mounts a structurally different pill <button>, which (per Pointer Events §5.3) would implicitly release pointer capture. That unmount does not happen here. Both the idle/connecting branch and the isRecording branch render a <button> at the same tree position with no key, so React's reconciler keeps the same DOM node and only patches its className, children, and handlers — implicit capture release is triggered by node removal, which never occurs.
I verified this empirically rather than arguing it from the spec: a DOM-identity check across the idle→recording transition returns the identical node (expect(recordingButton).toBe(idleButton) passes). Because the captured element stays in the DOM, the capture set in handlePointerDown survives, the subsequent pointerup is still delivered to it, and handlePointerUp → stop() fires — including for a release outside the button, so the design guarantee holds. The event.detail !== 0 click guard is irrelevant to this path; the pointer handlers, not the click handler, drive hold mode.
This also means the existing test is faithful to a real browser, not masking the issue: the re-queried button it dispatches pointerup on is the same captured node a real browser would target. To lock the invariant in, I added that one-line DOM-identity assertion (with a short why-comment) to holds to start and releases to stop dictation.
I did not apply the suggested refactor (a single stable button, or a document-level pointerup listener): both would add real complexity to fix a defect that does not exist. Happy to revisit if I've misread React's reconciliation here.
中文说明
拒绝此项，判定为误报 —— 线程保持打开，留待 maintainer 确认。
该结论的关键前提是：当状态切换到 recording 时，React 会卸载图标 <button> 并挂载一个结构不同的 pill <button>，从而（依据 Pointer Events §5.3）隐式释放指针捕获。但这里并不会发生这种卸载。idle/connecting 分支与 isRecording 分支都在同一树位置、且没有 key 渲染一个 <button>，因此 React 的协调器会保留同一个 DOM 节点，只更新它的 className、children 和事件处理函数 —— 隐式释放捕获是由节点被移除触发的，而这里从未发生移除。
我没有仅凭规范推断，而是做了实证验证：在 idle→recording 切换前后做 DOM 同一性检查，返回的是完全相同的节点（expect(recordingButton).toBe(idleButton) 通过）。由于被捕获的元素始终留在 DOM 中，handlePointerDown 中设置的捕获得以保留，随后的 pointerup 仍会派发到该节点，handlePointerUp → stop() 会触发 —— 即使在按钮外释放也成立，因此设计保证成立。event.detail !== 0 的 click 守卫与此路径无关；hold 模式由指针处理函数驱动，而非 click 处理函数。
这也意味着现有测试忠实反映了真实浏览器，并未掩盖问题：测试重新查询到的、并在其上派发 pointerup 的按钮，正是真实浏览器会命中的同一个被捕获节点。为锁定这一不变量，我在 holds to start and releases to stop dictation 中加入了这一行 DOM 同一性断言（以及一条简短的 why 注释）。
我没有采用建议中的重构（单一稳定按钮，或 document 级的 pointerup 监听器）：两者都会为一个并不存在的缺陷增加实实在在的复杂度。如果我对 React 协调行为的理解有误，欢迎重新讨论。
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Reconcile the workspace-scoped voice refactor (#7754) with the hold-mode PCM buffering and deferred-stop work: port the buffer/overflow/finalize logic onto the snapshot-based capture lifecycle, keep the required target prop wired through the button tests, and assert the icon/pill button reuses one DOM node so pointer capture survives the recording transition.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review round summaryMerged Conflict resolution (
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3659694979] Missing negative test for
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: coverage-check — prompt-matching failed despite 12/12 agents doing substantive work. Not reviewed: the linked-issue fidelity pass, the line-by-line correctness pass, the security pass, the code-quality pass, the performance pass, the test-coverage pass, the open-ended audit (attacker mindset), the open-ended audit (oncall mindset), the open-ended audit (maintainer mindset), the removed-behavior audit, the cross-file consistency pass, the build-and-test check — its prompt was built, but no agent on record was launched with it.
中文说明
已审查。 建议见行内评论。 未审查：coverage-check — prompt-matching failed despite 12/12 agents doing substantive work。 未审查：关联 issue 一致性检查、逐行正确性检查、安全检查、代码质量检查、性能检查、测试覆盖检查、开放式审计（攻击者视角）、开放式审计（值班排障视角）、开放式审计（后续维护者视角）、删除行为审计、跨文件一致性检查、构建与测试验证——它的 prompt 已构建，但没有任何 agent 有记录用它启动过。
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3660318181] Missing negative test for
|
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedrc:3660598975 — Missing negative test for
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedInline suggestions
Review-level comments
ConflictNo conflict ( Verification
中文说明已处理的审查反馈行内建议
审查级别评论
冲突无冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action requiredThe latest review round contains no actionable feedback:
No code changes were made this round. 中文说明无需处理最新一轮审查不包含可操作的反馈：
本轮未做任何代码改动。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3661629148] [Critical] Deferred stop path does not clear the 60-second start timeoutDecision: Fixed. The finding is valid. When Changes:
No conflict resolution was needed ( Verification
中文说明已处理的评审反馈[rc:3661629148] [Critical] 延迟停止路径未清除 60 秒启动超时决定： 已修复。 该发现有效。当 更改：
无需解决冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Local runtime validation — real browser + real
|
| Layer | What actually ran |
|---|---|
| Browser | headless Chromium (Playwright) with --use-fake-device-for-media-stream — real getUserMedia, real AudioContext/ScriptProcessor, real Pointer Events, real setPointerCapture |
| App | the real Web Shell (vite dev, packages/web-shell/client), full <App>, PR head e61af2d |
| Daemon | real node packages/cli/dist/index.js serve --port 4171 --workspace …, real /voice/stream WS, real voice-ws.ts → real transcribeVoiceAudio |
| Provider | local OpenAI-compatible ASR stub that returns the received audio length as the transcript (ASR OK · <ms> ms audio · <bytes> PCM bytes), so the composer text is a receipt for what the daemon really got |
| Voice mode | real workspace setting (general.voice.mode), served by the daemon's GET /workspace/voice |
A/B was done by materialising the two production files from the merge base (3209b89) and swapping them in through a vite alias, so the same app, same daemon, same gesture runs pre- and post-PR. A third arm (hookbefore) keeps the PR's VoiceButton and restores only the pre-PR useVoiceCapture, isolating the buffering/deferred-stop change. A tiny TCP proxy in front of the daemon delays the WebSocket upgrade on demand to reproduce "released while still connecting".
1. Hold mode works; on main the same gesture is broken
PR head, press → hold 1.2 s → release, wire trace from the browser:
t=1917 pointerdown → state=connecting
t=2001 state=recording hasPointerCapture(1)=true (same DOM node as the idle mic button)
t=1924 ws sent {"type":"start"}
t=2173..3192 ws sent 5 × 8192 B PCM
t=3247 pointerup → ws sent {"type":"stop"}
t=3254 ws recv {"type":"final","text":"ASR OK · 1280 ms audio · 40960 PCM bytes"}
→ inserted into the composer
Pre-PR arm, identical gesture: pointerdown does nothing (still idle after 15 s of holding), the release's click starts a capture that never stops — 24 s later the pill still reads 0:24 and PCM is still streaming to the daemon.
Two claims that only a real browser can check, both confirmed:
- Release outside the button — released 500 px away over
DIV._chatPane_…;{"type":"stop"}is still sent and the transcript arrives.setPointerCaptureis doing its job. - The icon → pill swap keeps the same DOM node — I tagged the idle mic node with a
data-*attribute before pressing; after the transition the tag is on the recording pill andhasPointerCapture(1)is stilltrue. React reuses the element, so capture survives.
2. Release while the socket is still connecting (buffering + deferred stop)
WS upgrade delayed 1.8 s, hold 0.9 s and release while state=connecting:
PR head on open → {"type":"start"} → 3 × 8192 B buffered PCM → {"type":"stop"}
→ "ASR OK · 768 ms audio · 24576 PCM bytes" ✅ speech preserved
pre-PR hook release → socket closed immediately, 0 frames sent, 0 ASR calls ❌ speech discarded
Also verified in the same setup:
- Touch
pointercancel(dispatched through CDPInput.dispatchTouchEvent→ a genuine browserpointercancel, not a synthetic JS event):{"type":"abort"}is sent, the socket closes, 0 ASR calls, nothing inserted. - A stray quick click in hold mode (60 ms press, socket still connecting):
start+stopare sent on open and the capture completes; the trailingclickdoes not abort it. With theevent.detail !== 0guard removed, the same gesture aborts the socket 750 ms before it opens and the capture is lost — the guard has real teeth.
3. Tap mode and keyboard are unchanged
- Tap: a 1 s press leaves the control idle (pointer handlers correctly inert), the click starts, recording continues after release, the second click stops → transcript. Byte-for-byte the same on both arms.
- Keyboard in hold mode: Enter starts, Enter stops → transcript. Same on both arms.
- No console errors or page errors in any post-PR run.
4. Static checks + do the new tests have teeth?
client/voice/**suites: 80/80 pass (VoiceButton30,useVoiceCapture23, plus the 27 pre-existing voice tests).tsc -p tsconfig.json --noEmit: clean.- Full package: 2339/2350 pass. The 11 failures are
build-artifact.test.ts(10 — needsvite build --config vite.lib.config.tsto have been run first) and oneutils/composerTag.test.tsSVG-inlining assertion; the PR touches no file in either suite. - ESLint on
client/voiceat the merge base and at PR head is identical modulo one advisory from a@vitest/eslint-pluginnewer than the lockfile pins (1.6.24vs1.3.4; the rule does not exist in1.3.4) — not a CI failure.
Mutation testing — I broke each production hunk in turn and re-ran the PR's two suites:
| Mutant | Killed by the PR's tests |
|---|---|
drop the detail !== 0 click guard |
✅ 2 tests |
pointerdown stops checking mode === 'hold' |
✅ 1 |
pointerup accepts any pointerId |
✅ 1 |
| releasing the hold no longer stops | ✅ 2 |
| accept any mouse button | ✅ 1 |
stop() while connecting → abort (pre-PR) |
✅ 3 |
| buffered PCM not flushed on open | ✅ 1 |
| deferred stop no longer clears the start timeout | ✅ 1 |
| audio graph connected despite a pending stop | ✅ 1 |
| remove the pre-open buffer limit | ✅ 1 |
remove setPointerCapture |
❌ 0 of 53 |
finalize() no longer releases the mic |
❌ 0 of 53 |
The two survivors are the two things jsdom cannot see, and the browser harness kills both:
- no
setPointerCapture→ releasing outside the button leaves the capture running forever (state stillrecordingafter 25 s); - no
teardownAudio()infinalize()→ with a 3 s ASR latency the browser keeps sending microphone frames after{"type":"stop"}(12+ extra frames vs 0 on the shipped code).
Neither is a defect in the PR — the shipped code is correct — but a small Playwright case for "release outside the control" would protect the feature's core guarantee.
5. Two edge cases worth a look (non-blocking)
A. In hold mode, a mouse click can no longer stop or abort a running capture.
handleClick returns for every pointer-generated click when the mode is hold, including when a capture is already recording/connecting. Reproduced: start with Enter (allowed in hold mode), then click the recording pill → still recording 1.5 s later; on main the same click stops it and inserts the transcript. The same applies if a pointerup is ever lost (window switch, context menu). This is also why the two existing "disabled composer can stop/abort dictation" tests had to be switched to tap in this PR — in hold they no longer hold.
Suggested fix: instead of gating on the mode, suppress only the click that belongs to the hold gesture, e.g. set suppressNextClickRef.current = true in handlePointerDown and consume it in handleClick; unrelated clicks then keep working.
B. After a deferred stop, nothing bounds the wait for the socket.
stop() now calls clearTranscribeTimeout() on the deferred path (e61af2d) and ws.onopen is the only place a timer is re-armed. If the upgrade never completes, the control stays on "Starting…" indefinitely — I held the upgrade for 90 s and it was still connecting at 78 s, with no error and no way to abort by mouse (finding A). Pre-PR the 60 s start timeout fired and surfaced "Voice capture timed out while starting.", which is recoverable. The microphone is already released, so this is a UI dead-end rather than a privacy issue.
Suggested fix: re-arm a bounded timer when setting stopWhenConnectedRef.current = true (fail the capture if the socket has not opened by then) instead of clearing the timeout outright.
Minor: MAX_BUFFERED_PCM_BYTES equals exactly 60 s of audio and the 60 s start timeout fires ~160 ms earlier, so in practice the overflow branch is only reachable if timers are throttled. Harmless, just noting it is defensive rather than load-bearing.
Also note that the voice mode is cached per status query key, so a mode change written outside the daemon (editing settings.json, TUI /voice tap) only takes effect after the status is re-fetched — a page reload in my run. That is pre-existing behaviour shared with enabled, not something this PR changed; the PR description's "refreshed through the existing settings-version signal" is only true for changes that flow through the daemon's own settings path.
Nothing here blocks the merge from my side.
中文版
本地运行时验证 —— 真实浏览器 + 真实 qwen serve 守护进程
这个 PR 的关键部分（指针捕获、pointercancel、hold 手势尾随的 click）恰恰是 jsdom 无法表达的，因此我没有只做代码审阅，而是在 Linux 上完整跑通了真实链路。结论：功能与描述一致，tap 与键盘路径没有回归。发现两个本 PR 新引入的边界问题，均不阻塞合并，详见文末。
验证链路（路径中没有 mock）
| 层 | 实际运行的东西 |
|---|---|
| 浏览器 | 无头 Chromium（Playwright）+ --use-fake-device-for-media-stream：真实 getUserMedia、真实 AudioContext/ScriptProcessor、真实 Pointer Events、真实 setPointerCapture |
| 应用 | 真实 Web Shell（vite dev，packages/web-shell/client），完整 <App>，PR head e61af2d |
| 守护进程 | 真实 node packages/cli/dist/index.js serve --port 4171 --workspace …，真实 /voice/stream WS，真实 voice-ws.ts → 真实 transcribeVoiceAudio |
| 供应商 | 本地 OpenAI 兼容 ASR stub，把实际收到的音频长度作为转写结果返回（ASR OK · <ms> ms audio · <bytes> PCM bytes），因此输入框里的文字就是守护进程真实收到内容的凭证 |
| 语音模式 | 真实 workspace 设置（general.voice.mode），由守护进程 GET /workspace/voice 下发 |
A/B 方式：从 merge base（3209b89）取出两个生产文件作为“改前”版本，通过 vite alias 换入，从而同一个应用、同一个守护进程、同一个手势跑改前/改后。第三组 hookbefore 保留 PR 的 VoiceButton、只回退 useVoiceCapture，用于单独隔离缓冲/延迟停止这一处改动。守护进程前面加了一个极小的 TCP 代理，可按需延迟 WebSocket 升级，用来复现“socket 还在连接时就松手”。
1. hold 模式可用；main 上同样的手势是坏的
改后（按住 1.2 s 后松手）的浏览器实际时序：
t=1917 pointerdown → state=connecting
t=2001 state=recording hasPointerCapture(1)=true （与空闲态麦克风按钮是同一个 DOM 节点）
t=1924 ws sent {"type":"start"}
t=2173..3192 ws sent 5 × 8192 B PCM
t=3247 pointerup → ws sent {"type":"stop"}
t=3254 ws recv {"type":"final","text":"ASR OK · 1280 ms audio · 40960 PCM bytes"}
→ 已插入输入框
改前同样手势：pointerdown 无反应（按住 15 s 仍是 idle），松手产生的 click 启动了一次永远不会停止的采集 —— 24 秒后录音条仍显示 0:24，PCM 仍在持续发往守护进程。
两个只有真实浏览器才能验证的点，均已确认：
- 在按钮外松手：在 500 px 外的
DIV._chatPane_…上释放，{"type":"stop"}仍然发出并拿到转写结果，setPointerCapture生效。 - 图标 → 录音条切换复用同一个 DOM 节点：按下前给空闲麦克风节点打了
data-*标记，切换后该标记出现在录音条上，且hasPointerCapture(1)仍为true，说明 React 复用了元素、指针捕获得以保留。
2. 连接过程中松手（缓冲 + 延迟停止）
WS 升级延迟 1.8 s，按住 0.9 s 后在 state=connecting 时松手：
改后 socket 打开后 → {"type":"start"} → 3 × 8192 B 缓冲 PCM → {"type":"stop"}
→ "ASR OK · 768 ms audio · 24576 PCM bytes" ✅ 语音未丢失
改前 hook 松手 → 立即关闭 socket，0 帧、0 次 ASR 调用 ❌ 语音被静默丢弃
同一环境下另外确认：
- 触摸
pointercancel（通过 CDPInput.dispatchTouchEvent触发，是浏览器真实产生的pointercancel，不是 JS 合成事件）：发出{"type":"abort"}，socket 关闭，0 次 ASR 调用，输入框无内容。 - hold 模式下的误点击（按下 60 ms，socket 仍在连接）：socket 打开后照常发送
start+stop并完成采集，尾随的click没有把它中止；若移除event.detail !== 0这道判断，同样的手势会在 socket 打开前 750 ms 就中止连接、采集丢失 —— 这道判断确有作用。
3. tap 模式与键盘无回归
- tap：按住 1 s 控件仍为 idle（指针处理正确地不生效），click 才开始采集，松手后继续录音，第二次 click 停止并插入转写。改前/改后表现完全一致。
- hold 模式键盘：Enter 开始、Enter 停止并插入转写，改前/改后一致。
- 改后各场景均无 console error / page error。
4. 静态检查与新增测试的“咬合力”
client/voice/**用例：80/80 通过（VoiceButton30、useVoiceCapture23，加上既有 27 个语音用例）。tsc -p tsconfig.json --noEmit：无错误。- 整包：2339/2350 通过；11 个失败来自
build-artifact.test.ts（10 个，需先执行vite build --config vite.lib.config.ts）和utils/composerTag.test.ts的 SVG 内联断言，本 PR 未触碰这两个用例涉及的任何文件。 client/voice的 ESLint 在 merge base 与 PR head 上一致，唯一差异来自本机@vitest/eslint-plugin版本高于 lockfile（1.6.24vs1.3.4，该规则在1.3.4中不存在）—— 不会导致 CI 失败。
变异测试：逐个破坏生产代码改动点，再跑 PR 自带的两个测试文件：
| 变异 | 是否被 PR 的测试杀死 |
|---|---|
移除 detail !== 0 点击判断 |
✅ 2 个用例 |
pointerdown 不再判断 mode === 'hold' |
✅ 1 |
pointerup 接受任意 pointerId |
✅ 1 |
| 松手不再停止采集 | ✅ 2 |
| 接受任意鼠标键 | ✅ 1 |
连接中的 stop() 回退为 abort（改前行为） |
✅ 3 |
| socket 打开后不再冲刷缓冲 PCM | ✅ 1 |
| 延迟停止不再清理启动超时 | ✅ 1 |
| 已有待停止请求时仍然接上音频图 | ✅ 1 |
| 移除预连接缓冲上限 | ✅ 1 |
移除 setPointerCapture |
❌ 53 个用例全部通过 |
finalize() 不再释放麦克风 |
❌ 53 个用例全部通过 |
两个存活变异恰好是 jsdom 看不见的部分，而浏览器验证都能杀死：
- 移除
setPointerCapture→ 在按钮外松手后采集永不停止（25 s 后仍为recording）； finalize()不释放麦克风 → 把 ASR 延迟设为 3 s 后，浏览器在{"type":"stop"}之后仍继续发送麦克风数据（12+ 帧，而线上代码为 0 帧）。
这两点都不是本 PR 的缺陷（当前实现是正确的），但补一个“在控件外松手”的 Playwright 用例可以守住该特性最核心的保证。
5. 两个值得关注的边界问题（不阻塞）
A. hold 模式下，鼠标点击无法再停止/中止正在进行的采集。
只要模式为 hold，handleClick 会对所有指针产生的点击直接返回，包括当前已经处于 recording/connecting 的情况。复现：用 Enter 开始（hold 模式允许），再用鼠标点击录音条 → 1.5 s 后仍在录音；main 上同样的点击会停止并插入转写。若 pointerup 丢失（切换窗口、右键菜单等）也是同样结果。这也是本 PR 不得不把既有两个“输入框禁用时仍可停止/中止听写”的用例改成 tap 的原因 —— 在 hold 下它们已不成立。
建议：不要按模式一刀切，而只屏蔽属于本次 hold 手势的那一次点击，例如在 handlePointerDown 里设置 suppressNextClickRef.current = true，在 handleClick 中消费它；其他点击即可恢复正常。
B. 延迟停止之后，等待 socket 的时间没有任何上界。
stop() 在延迟停止分支中调用了 clearTranscribeTimeout()（e61af2d），而重新装定时器只发生在 ws.onopen。如果升级始终不完成，控件会一直停在“Starting…”：我把升级挂起 90 s，78 s 时仍为 connecting，既没有报错，也无法用鼠标中止（见问题 A）。改前有 60 s 启动超时兜底，会提示 “Voice capture timed out while starting.”，用户可以恢复。此时麦克风已经释放，所以这是 UI 死角而非隐私问题。
建议：在设置 stopWhenConnectedRef.current = true 时重新装一个有界定时器（超时仍未打开则失败），而不是直接清除。
补充：MAX_BUFFERED_PCM_BYTES 正好等于 60 s 音频，而 60 s 启动超时会早约 160 ms 触发，因此溢出分支实际上只在定时器被节流时才可达 —— 无害，只是说明它是防御性的。
另外，语音模式按状态查询 key 缓存，绕过守护进程写入的模式变更（直接改 settings.json、TUI /voice tap）需要重新拉取状态后才生效（我的验证里是刷新页面）。这与 enabled 的既有行为一致，并非本 PR 引入；PR 描述中“通过既有 settings-version 信号刷新”只对经由守护进程设置路径的变更成立。
从我这边看，没有阻塞合并的问题。
|
Thanks for the real-browser and real-daemon validation. This confirms the core PR behavior, including release outside the button and DOM identity preserving pointer capture. I classify findings A and B as non-blocking follow-up suggestions: A concerns mixing a keyboard-started capture with a later mouse click rather than the requested hold gesture, and B concerns bounding a stalled deferred WebSocket upgrade. This PR is already through six review rounds, so per repository guidance I am keeping the merge diff scoped to Critical fixes and deferring both rather than widening it further. No code change in this round. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no code changeNo code change this round. The new feedback is @wenshao's real-browser + real-daemon validation of PR #7839, which confirms the feature works as described (hold gesture, release outside the button, pointer-capture DOM identity, buffered/deferred stop, Feedback triageFinding A — in hold mode a mouse click can no longer stop/abort a running capture (
Finding B — after a deferred stop, nothing bounds the wait for the socket (
The reviewer's remaining notes (the Why no change rather than an escalationThese are not open judgment calls awaiting a maintainer: @callmeYe (maintainer) has already decided in the thread to classify both as non-blocking follow-ups and to make no code change this round, citing the repository's review-round guidance. There is no inline comment to resolve or reply to (both points were raised as issue-level comments), so no review thread is left dangling. The deferral is recorded here and is already visible in the PR thread. Suggested follow-up (out of scope for this PR)One follow-up issue covering both hardening items for the web-shell voice feature:
VerificationNo code change was made this round, so no build/typecheck/lint/test run was performed (per the workflow, those run after a code change). The decision is a triage of explicitly non-blocking feedback that the maintainer has already deferred.
中文说明Autofix 审阅轮次 —— 本轮不做代码改动本轮不做代码改动。新反馈是 @wenshao 对 PR #7839 做的真实浏览器 + 真实守护进程验证，确认功能与描述一致（hold 手势、在按钮外松手、指针捕获的 DOM 同一性、缓冲/延迟停止、 反馈分类问题 A —— hold 模式下鼠标点击无法再停止/中止正在进行的采集（
问题 B —— 延迟停止之后，等待 socket 的时间没有任何上界（
审阅者的其余说明（ 为什么是“不改动”而非“升级给维护者”这些并不是等待维护者裁决的开放判断：@callmeYe（维护者）已在讨论串中决定把两者都归为不阻塞的后续改进，并本轮不做代码改动，理由是本仓库的审阅轮次指引。没有需要 resolve 或回复的行内评论（两点都是以 issue 级评论提出的），因此不会留下悬空的审阅线程。该推迟决定记录于此，并且在 PR 讨论串中已可见。 建议的后续改进（不在本 PR 范围内）针对 web-shell 语音功能，建一个后续 issue 覆盖这两项加固：
验证本轮未做代码改动，因此未运行 build/typecheck/lint/测试（按工作流，这些在代码改动之后运行）。本决定是对明确不阻塞、且维护者已决定推迟的反馈所做的分类。
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
|
Released in v0.21.1. |
7 participants
What this PR does
The Web Shell now honors the workspace voice interaction mode returned by the daemon. In hold mode, pressing the microphone starts capture, releasing the same pointer finishes capture and begins transcription, and a cancelled pointer gesture aborts capture. Tap mode keeps the existing click-to-start and click-to-stop behavior, while keyboard-generated activation remains usable in both modes.
Why it's needed
The daemon already persists and returns
holdortap, but the Web Shell previously ignored that value and always behaved like a tap toggle. As a result, hosts could configure hold-to-talk without changing the browser interaction.Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: both configured modes used click-to-start and click-to-stop behavior in the Web Shell.
After: hold mode follows press-to-start and release-to-finish semantics, while tap mode preserves the previous toggle behavior. Focused DOM coverage exercises hold start/release, release while connecting, pointer cancellation, tap compatibility, and keyboard activation.
Tested on
Environment (optional)
macOS with Node.js 22.17.0. Verified with the focused Web Shell voice-control tests, Web Shell lint and typecheck, and the repository build.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
Web Shell 现在会消费 daemon 返回的 workspace 语音交互模式。在 hold 模式下，按下麦克风开始采集，释放同一个指针结束采集并进入转写，指针手势被取消时则中止采集。tap 模式继续保持原有的点击开始、再次点击停止行为，同时两种模式都保留键盘触发能力。
为什么需要
daemon 已经能够持久化并返回
hold或tap，但 Web Shell 之前忽略该值，始终按 tap 切换方式工作。因此宿主即使配置了按住说话，也无法改变浏览器端交互。Reviewer Test Plan
如何验证
前后对比证据
修改前：Web Shell 中两种配置模式都使用点击开始、点击停止行为。
修改后：hold 模式遵循按下开始、释放完成语义，tap 模式保留原来的切换行为。聚焦 DOM 测试覆盖了 hold 开始和释放、连接中释放、指针取消、tap 兼容以及键盘触发。
测试平台
环境
macOS，Node.js 22.17.0。已完成 Web Shell 语音控件聚焦测试、Web Shell lint 和 typecheck，以及仓库构建。
风险与范围
关联 Issue
无。