Repository navigation
fix(widget): harden voice after independent review - #155
Merged
Merged
Conversation
An independent review of the voice feature reproduced several real bugs that 507 green tests had missed, largely because the speechSynthesis test double encoded my own wrong assumption. Each fix below is pinned by a regression test (507 -> 541). Read aloud - Pause state now follows the utterance's pause / resume events. Chrome and Safari set `speechSynthesis.paused` only once the engine confirms, so reading the flag right after pause() left the UI on "Pause" with no way to resume. Confirmed against Chrome 153. The fake now confirms a pause asynchronously by default, and no longer clears `paused` on cancel(), which the spec says it must not. - The reader only cancels speech it started. `speechSynthesis` is a page global and the hook mounts even with voice off, so closing the chat silenced the host page's own speech on every embed. With two widgets, the first one's late "interrupted" error cancelled the second's queue and nothing played. - Stopping while paused resumes the engine, which cancel() leaves paused; otherwise the next reply queued in silence. - The controls keep keyboard focus. Swapping a bare button for a wrapper dropped focus to <body>, outside the dialog's focus trap, so Tab could leave the dialog. Dictation - Hold mode lasts as long as the hold. With single-utterance sessions the engine ends at the first pause, silently dropping the rest and, with auto-submit, sending half a message while the button was still down. A hold now spans sessions and auto-submit waits for the release. Toggle starts, including the assistive-technology click, are not bridged, since nothing guarantees their release. - Pressing again while the previous session winds down no longer duplicates the text: a refused start() must not move the dictation base. - Read-aloud now ends dictation, as dictation already ended read-aloud. Otherwise the mic transcribed the reply and auto-submit sent it as the visitor's message. - A request starting to load ends dictation, so auto-submit cannot clear text that useChat then refuses to send. - An error releases the session without waiting for `end`; an engine-side "aborted" no longer runs auto-submit; a voice error clears on send. Helpers - chunkSpeechText: 150 characters (about ten seconds, with room for numbers), CJK sentence marks, and a last-resort split for unbroken tokens. 560 characters of Japanese used to become one utterance. - stripAnnouncementFormatting keeps the punctuation after a URL, which now decides where speech pauses. - The `voice` option fails closed: the string "false" no longer switches the microphone on. - Visible focus ring on the mic while it is filled with the accent color. e2e: fakes behave like Chrome, plus hold-to-talk through the IIFE bundle with a real mouse press and release, confirming in Chromium that the click fired after the release carries a click count and does not restart dictation. Refs #54 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Hold mode now really does run from press to release, with auto-submit on release; say so, and explain why. Note that dictation and read-aloud end each other, that the widget only stops speech it started (relevant to sites that use speechSynthesis themselves), that the option fails closed, and that CJK sentence marks are recognized. The design doc records the reasoning behind each change, including the pause-state bug the first test double had hidden. Refs #54 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8 of 12 tasks
Contributor
size-limit report 📦
|
Deploying chat-widget with
|
| Latest commit: |
e7c162d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d93b5dfd.chat-widget-ejc.pages.dev |
| Branch Preview URL: | https://54-voice-review-fixes.chat-widget-ejc.pages.dev |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Follow-up to #153, which was merged before an independent review of it had finished. The review reproduced real bugs in the voice feature that are now on
main. This PR fixes them. Please merge it before the 1.16.0 release PR (#154), so voice never ships in the broken state. Nothing has been released yet: no tag contains the voice commits.Why this matters even for sites that never enable voice
speechSynthesisis a page-wide global, and the read-aloud hook mounts for every chat window whether voice is on or off. Its cleanup calledspeechSynthesis.cancel()unconditionally, so closing the chat cancels the host page's own speech on every existing embed. Only sites that use speech synthesis themselves would notice, but it breaks the promise that voice is "off by default" with no behaviour change. The reader now only ever cancels speech it started.What else the review found, all fixed here
Each fix is pinned by a regression test (widget suite 507 → 541).
speechSynthesis.pausedonly once the engine confirms, so reading it right afterpause()left the UI on "Pause" with no way to resume. 507 green tests missed it because my test double set the flag synchronously, encoding my own wrong assumption. I confirmed the real behaviour against Chrome 153. State now follows the utterance'spause/resumeevents, and the fake confirms a pause asynchronously by default.interruptederror cancelled the other's fresh queue, so nothing played.<body>when the read-aloud controls swapped, which also let Tab escape the dialog's focus trap.voice: "false"as a string switched the microphone on. The option now fails closed: onlytrueor an options object enables it.endleft the mic button stuck on; the focus ring was invisible on the accent-filled mic.Docs and the design doc are updated to match, notably the hold-mode section of
configuration/voice.md, which claimed "recording runs from press to release" when it did not.Left for later, deliberately
pause(). Theendanderrorvariants are handled; in the silent case the visitor can still press Stop.Related Issue
Refs #54 (closed by #153)
Type of Change
Checklist
pnpm testin bothwidget/andworker/): widget 541, worker 153, scripts 82pnpm lintinwidget/): 0 errors, the same 2 pre-existing warnings asmainpnpm format:checkinwidget/)pnpm typecheckinwidget/), anddocs:api:checkis cleancd widget && pnpm build)feat/,fix/,docs/,chore/): uses the repo's existing<issue>-<slug>patternScreenshots / Notes
Bundle size. The fixes add about 0.4 KB gzip to the embed bundle. That fits inside the headroom of the budgets raised in #153, so
.size-limit.jsonis unchanged. Voice now totals +4.30 KB gzip onclaudius.iife.jsagainst the pre-voicemain.Verification. All 15 desktop e2e tests pass locally against system Chrome 153. A new one drives hold-to-talk through the built IIFE bundle with a real
mouse.down()/mouse.up(). That confirms in Chromium that the click fired after a release carries a click count and so does not restart dictation, which was the one assumption the reviewer could not settle by reading the code.Still not verified, as in #153: real speech end to end (this machine has no microphone or TTS voices) and physical phones. One specific thing to try on an iPhone is hold mode: under the HTML spec a touch
pointerdownis not a user-activation event, so if WebKit gatesSpeechRecognition.start()on a user gesture, hold-to-talk could fail there with "Microphone access is blocked" even though toggle mode works. Only a device can tell.🤖 Generated with Claude Code