Repository navigation
feat(web-shell): honor voice hold mode #7839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
38292b8
feat(web-shell): honor voice hold mode
callmeYe 5e84197
fix(web-shell): finalize held voice after connecting
callmeYe 838d088
fix(web-shell): buffer voice while connecting
callmeYe cb41658
test(web-shell): cover voice buffer overflow
callmeYe 96da14b
Merge branch 'main' into codex/web-shell-voice-hold-mode
callmeYe 8d3c27c
test(web-shell): add negative test for mouse click guard in hold mode…
b441349
Merge branch 'main' into codex/web-shell-voice-hold-mode
qwen-code-dev-bot ed06d4a
test(web-shell): add negative test for mouse click guard in hold mode…
qwen-code-dev-bot 4d3460c
fix(web-shell): add recording-state pointercancel test and fix buffer…
4a96f39
test(web-shell): add tap-mode pointer and non-primary button guard te…
e61af2d
fix(web-shell): clear start timeout in deferred-stop path (#7839)
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| # Web Shell voice mode | ||
|
|
||
| ## Goal | ||
|
|
||
| Make the Web Shell voice control honor the workspace voice mode already returned by the daemon. | ||
|
|
||
| ## Behavior | ||
|
|
||
| | Mode | Start | Finish | Cancel | | ||
| | ------ | -------------------- | --------------------- | ---------------------- | | ||
| | `hold` | Primary pointer down | Matching pointer up | Pointer cancel | | ||
| | `tap` | Click while idle | Click while recording | Click while connecting | | ||
|
|
||
| The control captures the active pointer so releasing outside the button still finishes a hold. Keyboard-generated clicks retain the existing toggle behavior in either mode. | ||
|
|
||
| Once microphone access is ready, audio produced while the socket connects is buffered locally. The socket receives `start`, the buffered audio, and then a deferred `stop`, so releasing during connection does not discard captured speech. | ||
|
|
||
| The mode is loaded with the existing workspace voice status and refreshed through the existing settings-version signal. A status request remains fail-closed: the control stays hidden until the current workspace reports that voice is enabled. | ||
|
|
||
| ## Scope | ||
|
|
||
| This change only affects Web Shell input interaction. It does not change the daemon settings contract, transcription transport, model selection, or TUI voice behavior. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theisRecordingbranch render a<button>at the same tree position with nokey, 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 inhandlePointerDownsurvives, the subsequentpointerupis still delivered to it, andhandlePointerUp → stop()fires — including for a release outside the button, so the design guarantee holds. Theevent.detail !== 0click 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
pointerupon 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) toholds to start and releases to stop dictation.I did not apply the suggested refactor (a single stable button, or a document-level
pointeruplistener): 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 协调行为的理解有误,欢迎重新讨论。