feat(voice): add a push-to-talk key for voice input - #1608
Conversation
GPT 5.6 Review — 🔴 changes requested (blocking)GPT 5.6 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- website/src/pages/ChatPage.tsx:1915 -- PTT release discards the authoritative streaming final |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound feature, right layer for the binding — but the stuck-mic guarantee is built as consumer-side scaffolding against producer internals, and eager capture-on-keydown has unpriced side effects. Watch
Suggestions
[DESIGN-REVIEWED] 990ec04 |
UX Review (Fable 5) — 🟡 CONCERNSAdvisory UX-level review of The evidence is complete. The copy and states are unusually well-worked; the real findings are in defaults and control. Composing the review. UX-Verdict: CONCERNS Well-crafted copy and test strip, but the binding is default-on with no off switch, and on Windows/Linux the default key is unrestorable once changed. Watch
Suggestions
[UX-REVIEWED] 990ec04 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've reviewed the full diff exhaustively. This is a carefully engineered, heavily-reasoned frontend-only PR (localStorage-backed config, key state machine, streaming stop deferral, re-entrancy latches). Let me confirm my read of one last risk area — the pre- The logic is sound: pre- No blocking findings. FINDING — temp-screenshots/voice-ptt/01-ptt-hybrid-idle-dark.png:1 (and the 5 sibling PNGs) — six binary files under a [OPUS-REVIEWED] 990ec04 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
f853a0f to
c077385
Compare
|
Dispositions for the findings on
Two defects were stacked. Follow-on from the same finding — a startup that never settles — FIXED. Cleanup chained on the promise inherits its liveness, and the streaming path awaits a Rejected startup — FIXED. Evidence: three regression tests, each verified to fail against the prior code and pass after. The original race test was a false pass — it asserted a |
c077385 to
d8d1a2d
Compare
|
Dispositions for the findings on
Confirmed and worse than the summary suggests: with the hybrid default, a chord released under the 500 ms cutoff took the tap path, which latches recording on. On macOS that is how most special characters are typed (
The finding is right about the data loss. The suggested fix — disable push-to-talk while Not from this round, for completeness: the two findings on |
d8d1a2d to
ea576d1
Compare
|
Dispositions for the advisory verdicts on UX Review 🟡 CONCERNS — 3 fixed, 1 fixed (suggestion)Accent typists trigger dictation — FIXED. You and the GPT gate found this independently, and your framing named the everyday case the gate did not: The second half of that finding was a separate defect and is also fixed: with STT not set up, the key path called the same Test strip reacts to all typing on the page — FIXED. Correct: the capture-phase document listeners had no target filter, so editing the Language row below flashed the amber wrong-key state per keystroke. Mac vocabulary shipped to Windows/Linux — FIXED. Self-inflicted by the previous round: spelling the names out (the fix for an unreadable Suggestion — demote the AltGr note — DONE. It is rationale rather than an actionable problem, and amber on every non-mac visit turns alarm styling into wallpaper. Now muted helper text. Design Review 🟡 CONCERNS — accepted and deferredThe observation is correct and it is the sharpest comment on this PR: the stuck-mic guarantee is built in the consumer, against the producer's leaky startup internals. The consequences you name both hold — the mic-button path inherits the same reject-leaves-mic-open and never-settling- Your suggested direction is the right one: move the failure contract into the producer — Deferred rather than folded in, deliberately: it is a change to shared voice-input code on a path this PR does not otherwise touch, it needs its own tests against the real streaming module rather than a mock, and it would widen a diff that is already 30 files. Tracking it as the immediate follow-up to this PR, and the PR description now states plainly that the guard covers only the path this change adds. If you would rather see it in this PR than after it, say so and I will pull it in. Opus 5 Review — no verdict for this SHARecorded for completeness rather than answered: that lane failed closed on |
|
Dispositions for the GPT 5.6 findings on Note for whoever reads the checks rather than the logs: this round's verdict never made it into a PR comment — the review step emitted 1. BLOCKING —
|
ea576d1 to
bab1793
Compare
|
Disposition for the GPT 5.6 finding on (The verdict posted as a comment this time, unlike the previous five rounds.) BLOCKING —
|
06a98e5 to
e6b90dd
Compare
|
Disposition for the GPT 5.6 finding on BLOCKING —
|
e6b90dd to
10aca02
Compare
|
Prior reviewed SHA:
Correct and reachable, and I want to be specific about why, because the file argued the opposite. The comment claimed the pre- What actually happens when the key is released before
So the utterance is transcribed as silence. This is the normal case for a short push-to-talk tap (press, say "yes", release inside the ~2-3s Transcribe startup), not an edge case. The fix lives in Four tests in Rebase notes — three things worth naming:
One incidental change outside the feature: Verified on |
Hold a bare modifier to dictate, or tap it to latch recording on. The
default is right Option on macOS and Alt+Shift+Space elsewhere, which is
where Wispr Flow, VoiceInk and superwhisper all converge: a lone modifier
is the only key class that types nothing while held, emits no auto-repeat
and cannot collide with an editor chord -- and the composer almost always
has focus while dictating.
Settings -> Voice gains the key picker, a mode picker (press on/off, hold
to talk, or both), the tap/hold cutoff, and a live test strip. The strip
exists because two things cannot be answered from code or CI: whether a
given keyboard has the chosen key at all, and whether its release event
reaches the page. It is also the only discoverability surface available --
formatShortcut renders modifier+key chords, not a lone side-specific
modifier -- so without it "hold right Option to talk" is unguessable. It
watches keys only and never opens the microphone.
The binding is stored browser-local rather than in the server-side STT
config, for the same reason getPreferredMicId is: the right key depends
on the keyboard in front of you, and one account reaches the dashboard
from several machines. A server-side default would push the wrong key to
every other device.
ChatPage's toggleVoice is split into startVoice/stopVoice so the key
driver goes through the same STT availability gating and dictation
snapshot resets as the mic button -- calling voice.start() raw would skip
them and a key-started dictation would be rebuilt from stale
pre-dictation text. useVoiceInput now exports start/stop, because a
hold binding needs explicit start and stop; driving it through toggle
inverts the state under a race.
Four guards against the defining failure of a hold binding, a release
that never arrives: window blur, document visibility, a getModifierState
reconciliation on any later keystroke, and a hard duration cap.
The arming delay is load-bearing rather than a nuisance: it both
separates a tap from a hold and doubles as the getUserMedia pre-warm
window, so the opening word is not lost to a silent 50-200ms warmup that
Whisper otherwise hallucinates into a canned phrase.
Copy went through a first-run usability review, which caught the strip
contradicting itself -- a grey "never records anything" footnote under a
bright green "releasing stops recording". The reassurance now leads and
every verdict is in the conditional. Debug vocabulary (auto-repeat
counts, key-up, milliseconds) and the abbreviated "R / L" key names are
gone.
Two blocking findings from the review mirrors closed a stuck-microphone hole
that three of the four watchdogs could not reach:
- ChatPage's startVoice swallowed the start promise, so the guard meant to
stop a session whose async startup finished after the key was released
never ran in production at all. It also could not have worked: disarm
bumps the generation counter the guard compared against, so the test was
always false by the time a released hold resolved. The guard now keys on
the phase, and startVoice returns the promise.
- A startup can fail to settle AT ALL -- the streaming path awaits a `ready`
frame, and a socket that opens then goes silent leaves that await pending
forever, while the hard cap has already been cleared by the release. So a
disarm during startup no longer waits for anything: it calls cancel(),
which trips the streaming session's cancelled flag and closes the socket
(whose onclose settles the pending await) and releases the batch path's
warm mic. Nothing was captured yet, so discarding loses no audio.
- A REJECTED startup gets the same treatment: useStreamingStt builds its
AudioContext and worklet after getUserMedia and the socket handshake,
outside any try, and useVoiceInput's streaming branch re-raises -- so a
throw there leaves the mic open with no session to stop.
Each of the three is pinned by a test verified to fail against the code before
it. The original race test was a false pass: it counted stop() calls, which
release-time disarm satisfies either way, so it could not distinguish the bug
from the fix.
The i18n gate that CI runs reads INSIDE ALL-CAPS module constants, which the
local aggregate does not, and its added-lines check is zero-tolerance with no
baseline to raise. Of the four literals it caught, the getModifierState name
table is gone (the event's own modifier flags carry identical information for
these four families), the KeyboardEvent.code list is exempted by an enumerated
shape -- the eight side-specific modifier codes, spelled out rather than a
PascalCase wildcard that would also exempt 'Save' and 'Delete' -- and the
numpad keycap label was real copy and moved into the catalog.
Round 2 closed four more reachable defects, three of them found by the
advisory reviewers that never turn a check red:
- A bound bare modifier used as an ORDINARY modifier armed the trigger. On
macOS that is how you type most special characters, so `⌥e` for "é"
released under the 500ms cutoff and the hybrid tap path LATCHED recording
on; held slightly longer it started a hold outright. A non-matching keydown
now ends an armed press as a chord -- discarding while arming (nothing was
captured, and this is also what stops the release counting as a tap) and
committing while holding (a real utterance survives an accidental key).
- A keystroke must never raise a dialog. With STT not yet set up, the key
path called the same `setVoiceSetupOpen(true)` as the mic button, so a
keystroke that used to type a character threw an unsolicited modal. The
key path now starts silently; the button still explains itself.
- Releasing during a STREAMING startup discarded speech. useStreamingStt
connects its worklet and buffers PCM before the server's `ready` frame, so
a hold released during a slow handshake really does have audio in it.
Streaming now commits on that path (`streamStop()` sends the stop frame and
arms its own 8s force-cleanup, so the stuck-mic ceiling still holds); batch
still discards, because it has no recorder yet and nothing was captured.
- Spelling the key names out -- the fix for an unreadable `R ⌥` -- shipped Mac
vocabulary everywhere: Windows users were offered "Right Option ⌥" for a key
their keyboard labels Alt. Both platforms now have their own eight names.
Also from the UX review: the test strip's capture-phase document listeners had
no editable-target filter, so typing in the Language field below it flashed the
amber wrong-key state on every keystroke; and the AltGr note moved from an
always-visible amber warn box to muted helper text, since it is rationale rather
than an actionable problem and alarm styling on every non-mac visit is wallpaper.
KNOWN AND DEFERRED, per the design review: the stuck-mic guarantee is built in
the CONSUMER. useStreamingStt constructs its AudioContext and worklet after
getUserMedia and the socket handshake outside any try, and useVoiceInput's
streaming branch re-raises, so the mic-BUTTON path inherits the same
reject-leaves-mic-open and never-settling-handshake exposure unguarded. Sealing
that at the producer (try/finally around the post-getUserMedia build plus a
handshake timeout) fixes every caller and would let most of this hook's
sequence/phase machinery collapse. It is a separate change against shared voice
code with its own blast radius, so it is not folded in here.
Round 3 closed three more reachable defects, all found by the CI reviewer
reading the release and second-press paths again:
- A cold streaming release deleted the utterance. stopVoice() disarmed the
draining final on the premise that "the text is already in the composer" --
true only once a partial has landed. Before the server's first partial the
composer holds nothing and that final is the only copy, which is the
ordinary outcome of the first press of a session. The disarm is now gated on
frozenInputRef being set, i.e. on the premise it was already asserting.
- A second press could not cancel a startup still in flight. The
already-capturing test read voice.recording, false for the whole
getUserMedia + handshake window, so the press fell through and opened a
second start() that useVoiceInput's re-entrancy guard swallowed -- leaving
the first startup to go live against a user who had just pressed to switch
it off. Every start() now goes through one launcher that records what the
session was opened for ('hold' vs 'latch'), so a later press can end a
pending one and the settle handler can still tell an orphan from a latch.
- The test strip could never match the default Windows/Linux binding. A chord
arrives as separate keydowns and the strip kept whichever key came first, so
Alt+Shift+Space always read as "that's a different key" -- the one surface
whose job is to prove the shortcut works reported the shipped default as
broken. It now re-anchors onto the key that completes the binding.
Each fix is pinned by a test verified to fail against the pre-fix code.
Round 5 moved capture to the keydown, which is what the arming window should
have been doing all along.
`prewarm()` is a no-op on the streaming path, so waiting for the threshold put
the mic acquisition AND a ~2-3s Transcribe handshake in front of the opening
syllable -- the word the user starts on was simply not in the recording. Batch
was better only because `prewarm()` had already acquired the device; its recorder
still did not start until the threshold.
`start()` now runs on the keydown, and ONE session serves the whole gesture:
crossing the threshold promotes it to a hold, a tap in hybrid mode adopts it as
the latch, and a tap in hold-only mode or a chord discards it. Because the
session is opened before anyone knows what the press will become, `ownerRef`
records who holds it once the gesture RESOLVES ('gesture' while the key is down,
'latch' for a deliberate latch, null once torn down) -- and that, not
intent-at-open, is what the settle handler consults to tell a live session from
an orphan.
Nothing is transmitted for a discarded press: `useStreamingStt` buffers PCM
locally in a bounded window and only flushes it after the server's `ready`
frame, which lands well after the 500ms threshold has already resolved the
gesture, so `cancel()` drops the buffer unsent.
This also removes machinery rather than adding it -- `beginHold` no longer calls
`start()` (a second one would be swallowed by the producer's re-entrancy guard),
the tap-latch path no longer opens a session of its own, and `prewarm` leaves the
`VoiceControls` contract entirely.
Round 6 corrected the discriminator the pending-startup teardown uses. It asked
which TRANSPORT was in use; the question that actually matters is whether
capture has BEGUN. `useStreamingStt` flips `recording` true at the moment its
worklet is wired and PCM is buffering, and only then awaits the server's `ready`
frame -- so `recording` is exactly that boundary. Keying on `streamEnabled`
committed on the streaming path even when the release beat the permission grant,
where no socket exists, `stop()` is a no-op, and the startup would run to
completion and transmit audio for a press the user had already finished. All
three pending-teardown sites now commit only when `recording` is true and abort
otherwise, and `streamEnabled` leaves the `VoiceControls` contract.
The keydown change also stranded text on the discard path: a fast partial can
reach the composer before the press is revealed as a chord or a sub-threshold
tap, and the driver was wired to the hook's raw `cancel`, which drops the capture
without rolling the composer back. It now passes `cancelVoice`, the same
streaming rollback Esc uses, which removes the dictated region at the
frozenInputRef boundary and no-ops when nothing verifiably removable was written.
Round 7 moved the fix into the producer, which is where the last three findings
were actually pointing.
`useVoiceInput.start()` holds a re-entrancy latch for the whole async startup and
released it only in the `finally` after `await streamStart()`. Cancelling while
`getUserMedia` was still awaiting the permission dialog therefore left the latch
held for as long as that dialog stayed open -- nothing settles that await -- so
the next press hit `if (startingRef.current) return` and recorded NOTHING. On a
push-to-talk binding that is an ordinary sequence: press, release during the
first-run permission prompt, press again.
`cancel()` now releases the latch, and a per-startup generation makes that safe:
the abandoned startup can no longer clear a latch a newer press already holds,
claim ownership of a stream `useStreamingStt` has already bailed on, or run its
error teardown against the replacement session -- that path would otherwise null
the warm promise the new startup is awaiting, drop its ownership and surface a mic
error for a recording that is running fine. Releasing this startup's own tracks
stays unconditional, since an orphaned track is a live microphone.
This is the seam the previous three rounds kept compensating for in the consumer:
a startup that cannot be aborted before its socket exists. `usePushToTalk` needed
no change for it.
Round 8 closes the other half of the discard path. `cancelVoice` reconstructed the
dictated region as `frozen + separator + partial`, but `onPartial` writes it through
`spliceDictation`, which inserts at the snapshotted caret -- so for any dictation
started with the caret mid-draft the composer reads `before + partial + after` and
the append-only reconstruction failed its `startsWith` check. That fell through to
the deliberately conservative leave-unchanged branch, stranding the partial inside
the user's sentence. Acceptable for an Esc cancel, where the user knowingly started
a dictation; not acceptable now that a chord like the macOS dead key sequence opens
capture on the keydown, because the stranded word is one the user never asked to
dictate.
The rollback now reconstructs the region by calling the same `spliceDictation` the
write used, so the two cannot drift again, and it covers both the append and
mid-caret shapes. `frozenCaretRef` is cleared alongside `frozenInputRef`, since a
surviving caret would aim the next session's first splice at a position from the
discarded one.
Round 9 fixes the panel's own copy, which was mac-only prose rendered on every
platform. `ptt_heading_desc` promised "Right Option ⌥ works out of the box" and
`ptt_key_desc` offered "a key like Option, Control or Shift" -- both shown
unconditionally, so a Windows or Linux visitor read a claim about a key their
keyboard does not have, directly above a test strip saying "Press Alt + Shift +
Space". Same defect class already fixed for the key NAMES, so both strings get
the same platform split via a `PTT_COPY_KEY` module const (the i18n dynamic-key
gate only resolves direct indexing of a module-level const at the call site).
Two new keys across all ten hand-written locales plus the regenerated
pseudolocale.
Round 10 closes the same stuck-microphone hole in TOGGLE mode. The chord
reconciliation, the keyup handler and the blur/visibility guards all key off a
non-idle phase, but the toggle branch called `launch('latch')` on the keydown and
returned WITHOUT setting one -- so a toggle-mode press was invisible to every one
of them. Pressing the bound modifier and then another key (⌥ then E for `é`)
turned the microphone on and nothing in the gesture machinery could turn it off
again; only clicking the mic button could. That is the round-2 defect exactly,
surviving in the one mode whose arming path round 2 never touched.
Toggle presses are now armed like any other press -- capture still opens on the
keydown, and only the OWNERSHIP is deferred to the release, which is what makes
the press revocable. A joining key discards it, the release promotes it to the
latch, and a second press ends it as before. No hold timer is armed, since toggle
mode has no hold semantics.
Round 11 closes the last teardown site that still branched on the wrong signal.
`disarm`'s settled-hold path called `stop()` whenever `commit` was true, on the
assumption that "not pending" means the session is live. It does not when the
startup is one we never launched: a mic-button start leaves OUR `startPending`
false, so a press during its acquisition window falls through the second-press
guard and calls `start()` again, the producer's re-entrancy latch swallows it and
returns nothing, and `launch` reads that as a synchronous control and clears
`startPending`. Releasing then called `stop()` against a session whose capture had
not begun -- a no-op -- and the original startup went live afterwards with the
phase already back to idle and nothing watching it. That site now uses the same
`recording` discriminator as the other three, so it cancels instead.
One existing test had to be corrected rather than satisfied: its fake resolved
`start()` while leaving `recording` false, a state the producer never occupies
(`setRecording(true)` lands before the promise resolves). The same fake-fidelity
flaw was corrected for three other fakes in an earlier round; this was the last
one, and it was the reason the release path could look right while keying off a
value the fake never set.
10aca02 to
990ec04
Compare
|
Prior reviewed SHA:
Correct and reachable. Deferring the stop frame until the buffer flushes solved the lost-utterance problem, but I left everything else running: the worklet's Fixed by freezing capture at the moment the pending stop is recorded, which is what the deferral should have done from the start — defer the frame, never the end of capture:
The socket and the already-buffered PCM deliberately survive, because they are exactly what the flush still has to send. New test
The failure is Evidence it is not mine, in the order I checked it:
The shard is otherwise 8853 passed / 1 failed. Nothing in this PR can move it, and I have not touched it. Verified on |
|
Reviewed SHA
What I verified, in order: 1. The mechanism predates this PR, and this PR narrowed it. This PR split that into That change exists precisely to fix the case GPT is pointing at in its cold form: a short press against a cold stream where the release beats the first partial, where disarming would delete the whole utterance. So this PR strictly reduced the number of cases where speech is lost. It did not introduce the disarm. 2. GPT is still right about what remains. 3. The prescribed fix is not sufficient on its own. "Leave the draining final armed" cannot just be un-disarming, because 4. The sound fix is a flag split, and that is the decision I want from you. One boolean is currently doing two jobs: suppress the close-time append and suppress further composer updates. Correct behaviour needs them separated — keep drain-time I am not making that change unilaterally inside this PR: it rewrites pre-existing behaviour on four surfaces this feature does not own, on a path I cannot exercise by hand here (no microphone in this environment), and the tradeoff — may a late final overwrite text the user typed while the socket drained — is a product decision, not a mechanical one. Three ways forward, your call:
Everything else on this SHA is green, and the two earlier BLOCKINGs from this review lane are fixed and covered by tests. |
What
Adds a keyboard binding for voice dictation. Hold a bare modifier to talk; tap it to latch recording on. Default is right Option on macOS and Alt+Shift+Space elsewhere.
Settings → Voice → Speech-to-Text gains four rows: the shortcut key, how the key works, the tap/hold cutoff, and a live test strip.
Press on/offmode hides the cutoffLight theme and the released-key state are in
temp-screenshots/voice-ptt/.Why these choices
Why a bare modifier, and why the user picks which one. Wispr Flow, VoiceInk and superwhisper all converge on the same shape: hold one lone modifier, chosen by the user. A bare modifier is the only key class that types nothing while held, emits no auto-repeat, and cannot collide with an editor chord — and the composer almost always has focus while dictating. Their most popular default is fn/Globe, which Chromium does not deliver as a key event at all; the right-side modifiers are the closest reachable equivalent.
Why not the right Option default everywhere. On most Windows and Linux layouts the right Alt key is AltGr: it reports
ctrlKey && altKeyand composes characters. A lone left Alt reveals the window menu. Those platforms get the⌥⇧Spacechord instead, which is unclaimed on all three OSes (plain Alt+Space is the Windows system menu; adding Shift leaves that path).Why the binding is browser-local, not server config. Same reason
getPreferredMicIdis: the right key depends on the keyboard in front of you, and one account reaches the dashboard from several machines. A server-side default would push one machine's choice onto every other device. This also keeps the change frontend-only — no schema, no error-code contract, no migration.Why the test strip exists. Two things cannot be answered from code or from CI: whether a given keyboard has the chosen key at all, and whether its release event reaches the page. Only pressing the key on the keyboard in front of you settles either. A dropdown will happily list
Right Optionon a board that has none, so without the strip the first symptom of a bad choice is voice input silently never starting. It is also the only discoverability surface available —formatShortcutrenders modifier+key chords, not a lone side-specific modifier, so "hold right Option to talk" cannot be advertised in the shortcuts reference. It watches keys only and never opens the microphone.Why the arming delay is not just a delay. It separates a tap from a hold, and it doubles as the
getUserMediapre-warm window. Acquisition plus the first audio frame costs 50–200 ms on macOS, and a single-key press has no earlier moment to hide that in — so the opening word gets clipped, and Whisper hallucinates the silent warmup into a canned phrase. Arming callsprewarm()immediately andstart()only once the threshold passes, by which point the stream is live.Implementation notes for review
ChatPage'stoggleVoiceis split intostartVoice/stopVoice. That function carried the STT availability gate, thesttDisarmedRefreset and the streaming-manual-stop disarm. Callingvoice.start()raw from the key driver would skip them, and a key-started dictation would then be rebuilt from stale pre-dictation text. Both entry points now share one preamble.useVoiceInputnow exportsstart/stop. It previously exported onlytoggle; a hold binding needs explicit start and stop, and driving it throughtoggleinverts the state under a race.window.blur,document.visibilitychange, agetModifierStatereconciliation on any later keystroke, and a hard duration cap. A stuck-open microphone is the worst outcome here, so all four commit what was said rather than discarding it.SettingsStepper.valuewidened tonumber | stringso an already-localised duration ("0.5 seconds") can be displayed; the unit word cannot be split intosuffixbecause locales place and inflect it differently. The component only interpolates the value, so numeric state stays with the caller.Records indexed inline at thei18nT()call, which is the shapecheck-i18n-keys.mjsresolves statically. Declaring them inside the component pushed the dynamic-key count from 11 to 14.Copy went through a first-run usability review
A reviewer with no context on the implementation read only the screenshots and the English strings. It found a real defect: the strip contradicted itself — a small grey "never records anything" footnote sat four lines under a bright green "releasing stops recording". Verdict: a reader resolves that by distrusting both.
Fixed by leading with the reassurance and putting every verdict in the conditional ("in a real chat this would record while you hold"). Also folded in from that review:
auto-repeat ×0deleted — a debug counter that leaked into the UI, where×0reads as failure.R ⌥/L ⌥spelled out toRight Option ⌥everywhere. The abbreviation was worst exactly where the copy most needed to be unmistakable: the wrong-key warning. A test pins this, including a guard against the false pass where an unresolvedi18nTreturns the dotted catalog key.Hybrid→Both, with a per-mode explainer that follows the selection. A cold reader had no way to learn what "Hybrid" combined.modifier,threshold,trigger,the release is captured,key-up,ms.One recommendation was not taken: the reviewer wanted the cutoff row disabled-but-visible outside
Bothmode rather than hidden, arguing that a row appearing and disappearing reads as a bug. Hiding it was a deliberate product decision (the cutoff has no meaning in the other modes). The underlying concern is addressed instead by naming the dependency in the row's own description, so when it is visible the tie toBothis explicit rather than inferred.Not in this PR
globalShortcut.registerhas no key-up callback, so a true hold binding needs a native event monitor plus Accessibility permission on macOS. This is exactly whywebsite/electron/mochi/shortcuts.jsreserves avoiceInputaccelerator and deliberately leaves it unbound. That work would also unblock that dormant default.SearchEverywhereConfiginShortcutsPanelis the pattern to follow when it lands. A chord binding is reachable today only as the non-mac default, which is why the dropdown surfaces it as a read-only entry rather than hiding it.←+→) as a binding. Prototyped and dropped: they carry caret semantics in the composer, drive menus and listboxes, and are the primary navigation mechanism for screen-reader users. The first keypress also cannot be intercepted, because it is indistinguishable from real navigation until the second key arrives.Testing
62 new unit tests across
lib/pushToTalk.test.tsandhooks/usePushToTalk.test.ts, covering all three modes, auto-repeat suppression, the bare-modifier vs chordpreventDefaultdifference, terminal-target skipping, all four stuck-microphone guards, the release-during-async-start()race, live rebinding from another window, and per-field config coercion.Screenshots are produced by
website/scripts/capture-voice-ptt.mjs— the repo's gateway-free harness (real built SPA + route interception, no microphone). The keystrokes in it are real Playwright keyboard events, which is also what confirmsAltRightarrives withlocation=2.Local gates:
tsc,eslint, fullvitest, all i18n gates including the render-time gate,isort,flake8, and the dist-related backend tests. Two pre-existing failures were confirmed identical onmainand are not from this branch:src/i18n/format.test.ts(passes underTZ=UTC) and three mypy errors insrc/kiro_crew/hooks.py(os.listxattr/getxattr/setxattrare Linux-only in typeshed, so macOS reports them and CI's Linux runner does not — the file's diff here is empty).Review round 1 closed a stuck-microphone hole
Both CI review mirrors found blocking defects in the async-startup path, and both were real. Three of the four watchdogs could not reach the case.
ChatPage'sstartVoiceswallowed the start promise, so the handler meant to stop a session whose startup finished after the key was released never ran in production. It also could not have worked if it had:disarmbumps the generation counter the guard compared against, so the comparison was always false by the time a released hold resolved. The guard now keys on the phase, andstartVoicereturns the promise.readyframe; a socket that opens and then goes silent leaves that await pending forever, and the hard cap has already been cleared by the release. Any cleanup chained on the promise inherits its liveness. So a disarm during startup no longer waits for anything — it callscancel(), which trips the streaming session'scancelledflag and closes the socket (whoseonclosesettles the pending await) and releases the batch path's warm mic soacquireWarmrejects instead of handing back a stream. Nothing was captured yet, so discarding loses no audio.useStreamingSttbuilds itsAudioContextand worklet aftergetUserMediaand the socket handshake, outside anytry, anduseVoiceInput's streaming branch re-raises — so a throw there leaves the mic stream open with no session to stop.getUserMediawindow had the old hold's resolution kill the brand-new session. A per-call sequence number scopes the handler to the session it opened.Each of the four is pinned by a test verified to fail against the code before it. The original race test was a false pass: it counted
stop()calls, which release-time disarm satisfies either way, so it could not distinguish the bug from the fix.The i18n gate CI runs is stricter than the local aggregate
eslint.i18n.strict.config.jsreads INSIDE ALL-CAPS module constants; the locali18n:checkdoes not, and the[added-lines]check is zero-tolerance with no baseline to raise. Of the four literals it caught inlib/pushToTalk.ts:getModifierState('Alt'|'Control'|'Meta'|'Shift')name table is gone — aKeyboardEvent's own modifier flags carry identical information for those four families, sostillHeldreads them instead. Simpler, and no literals.KeyboardEvent.codelist is exempted by a newwords.excludeshape, enumerated rather than generalised:^(?:Alt|Control|Meta|Shift)(?:Left|Right)$. The obvious^[A-Z][a-zA-Z0-9]*$PascalCase wildcard was rejected because it would also exempt'Save','Delete'and'Done'— the single-word-copy class the config already names as its hardest false negative. Eight members of a closed DOM set cannot match prose.Review round 2 — the advisory reviewers found the worst bug
The two reviewers that never turn a check red found more than the blocking gates did. Four more reachable defects, all fixed:
⌥efor "é" releases Option under the 500 ms cutoff, and the hybrid tap path latched recording on. Held slightly longer, it started a hold outright. High frequency for anyone writing an accented language. A non-matching keydown now ends an armed press as a chord — discarding while arming (nothing was captured, and this is also what stops the release counting as a tap) and committing while holding, so a real utterance survives an accidental keypress.setVoiceSetupOpen(true)as the mic button, so a keystroke that used to type a character threw an unsolicited modal. The key path now starts silently; the button still explains itself. A passive binding must not interrupt.useStreamingSttconnects its worklet and buffers PCM before the server'sreadyframe, so a hold released during a slow handshake really does have audio in it. Streaming now commits on that path (streamStop()sends the stop frame and arms its own 8 s force-cleanup, so the stuck-mic ceiling still holds); batch still discards, because it has no recorder yet and nothing was captured. GPT's suggested fix here was to disable push-to-talk whenever streaming is on — that deletes the feature for the recommended STT configuration, so the branch was taken instead of the blanket.R ⌥handed Windows users "Right Option ⌥" for a key their keyboard labels Alt. Both platforms now have their own eight names. This one was self-inflicted by the previous round's copy fix.Two more from the UX review: the test strip's capture-phase document listeners had no editable-target filter, so typing in the Language field below it flashed the amber wrong-key state on every keystroke; and the AltGr note moved from an always-visible amber warn box to muted helper text, since it is rationale rather than an actionable problem.
Known and deferred — the design review is right about the layer
Design Review 🟡 CONCERNSobserves that the stuck-mic guarantee is built in the consumer, against the producer's leaky startup internals — and that the mic-button path therefore inherits the same reject-leaves-mic-open and never-settling-handshake exposure unguarded, while the new tests mockVoiceControlsand so pin nothing about the integration.That is accurate. The right fix is at the producer:
try/finallyarounduseStreamingStt's post-getUserMediabuild plus a handshake timeout, sostart()always either rejects promptly or tears down what it acquired. That fixes every caller and would let most of this hook's sequence/phase machinery collapse. It is a separate change against shared voice code with its own blast radius and its own tests, so it is not folded in here — this PR defends the path it adds, and the producer-side seal is the follow-up.One thing a reviewer on a real keyboard can settle that CI cannot
Whether the OS delivers auto-repeat for a held modifier, and whether both key-up events arrive when two keys are released in sequence, cannot be proven with synthetic events — Playwright emits no auto-repeat at all. The four watchdogs are written to be correct either way, but if you have a non-US layout or a keyboard without a right Option, pressing the test strip is the fastest way to find a hole.