feat: voice dictation (speech-to-text)#549
Closed
anandh8x wants to merge 8 commits into
Closed
Conversation
anandh8x
force-pushed
the
feat/voice-dictation
branch
from
7120a59 to
88f26c8
Compare
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Add STTConfig (provider/streaming/model/local-engine/language and related knobs) to the file config, with resolver merging, load-time validation of the provider enums, and writers (SetSTTProvider/SetSTTModel/SetSTTLocalEngine) so the TUI can persist a dictation choice.
anandh8x
force-pushed
the
feat/voice-dictation
branch
from
88f26c8 to
d92dc50
Compare
WalkthroughAdds STT configuration, dictation backends, verified local downloads, CLI wiring, and TUI voice-mode, model picker, and API-key flows. ChangesSpeech-to-Text dictation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant model as TUI model
participant controller as dictationController
participant Recorder
participant Transcriber
User->>model: Press Space
model->>controller: toggleDictation / startStreamingDictation
controller->>Recorder: StartStreaming()
Recorder-->>controller: audio chunks
controller->>Transcriber: StreamTranscribe(chunks)
Transcriber-->>controller: partial text
controller->>model: sttPartialMsg
User->>model: Release Space / Esc
model->>controller: stopDictation / cancelDictation
Transcriber-->>controller: final transcript
controller->>model: dictationTranscribedMsg
sequenceDiagram
participant User
participant model as TUI model
participant picker as STT model picker
participant download as dictation.EnsureLocalEngine
participant config as config.SetSTTLocalEngine
User->>model: /stt-model, select local variant
model->>picker: handleSTTDownloadSelection
picker->>download: download engine + model
download-->>picker: progress updates
picker->>model: sttDownloadProgressMsg
download-->>picker: EngineComponents
picker->>config: persist binary/model paths
picker->>model: dictationDownloadedMsg
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/view.go (1)
200-235: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMinor inconsistency: the non-tiny tier surfaces the
⬇download chip (L221-223), but the tiny tier (L207-209) only checksdictationStatusChip(), which is empty during a download (phase stays idle). On narrow terminals an in-flight model download shows no feedback at all. Likely acceptable for space reasons — flagging in case parity was intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/view.go` around lines 200 - 235, The tiny-tier branch in view.go is missing the in-flight model download state that the non-tiny path already handles via m.dictation.downloading and m.dictation.downloadStatus, so narrow terminals can show no feedback during downloads. Update the tierTiny logic to account for the download state, likely by prioritizing the same download chip before falling back to dictationStatusChip() or left. Use the existing symbols m.dictation.downloading, m.dictation.downloadStatus, and dictationStatusChip() to keep the behavior aligned across tiers.
🧹 Nitpick comments (11)
internal/dictation/download.go (1)
221-249: 🚀 Performance & Scalability | 🔵 TrivialUnauthenticated GitHub API calls — watch the 60 req/hr rate limit.
getJSON/resolveAssetnever send anAuthorizationheader.ListModels's pagination plus the 2 calls inEnsureLocalEngineand 2 more inEstimatedDownloadBytescan add up quickly for users sharing an office/CI IP, causing confusing 403s in an otherwise opt-in feature. Consider honoring aGITHUB_TOKENenv var (or a config field) to raise the limit, and surfacing rate-limit errors distinctly from generic HTTP failures.Also applies to: 598-646, 290-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dictation/download.go` around lines 221 - 249, The GitHub API requests in ListModels, getJSON, and resolveAsset are unauthenticated, which can quickly hit the shared 60 req/hr limit; update the request flow to attach an Authorization header when a token is available (for example from GITHUB_TOKEN or a config field) so callers can opt into higher limits. Make sure the same authenticated client behavior is used by the related calls in EnsureLocalEngine and EstimatedDownloadBytes, and distinguish rate-limit responses from generic HTTP failures in the returned errors.internal/tui/stt_model_picker.go (2)
164-169: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the model-list fetch.
context.Background()plus a nil*http.Client(→http.DefaultClient, no timeout) means a stalled GitHub round-trip leavessttModelsFetchedMsgunsent and the picker stuck on "loading…" indefinitely. Bound it withcontext.WithTimeout(and cancel) so a hung network falls back to the curated shortlist viahandleSTTModelsFetched.♻️ Suggested bound
func fetchSTTModelsCmd() tea.Cmd { return func() tea.Msg { - variants, err := dictation.ListModels(context.Background(), nil, "") + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + variants, err := dictation.ListModels(ctx, nil, "") return sttModelsFetchedMsg{variants: variants, err: err} } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/stt_model_picker.go` around lines 164 - 169, The model-list fetch in fetchSTTModelsCmd can hang indefinitely because it uses context.Background() with no timeout, so update it to create a bounded context with context.WithTimeout and defer cancel before calling dictation.ListModels. Keep the existing sttModelsFetchedMsg flow unchanged, but ensure a stalled request eventually returns so handleSTTModelsFetched can fall back to the curated shortlist instead of leaving the picker stuck loading.
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment refers to
sttModelValue, but the declared identifier issttValueSep. Fix the name so the comment describes the actual const.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/stt_model_picker.go` around lines 17 - 19, The comment is attached to the wrong identifier: it describes sttModelValue, but the declared constant in the picker code is sttValueSep. Update the declaration in stt_model_picker.go so the symbol name matches what the doc comment describes, or revise the comment to accurately document sttValueSep; use the nearby sttModelValue encoding logic as the reference point when renaming.internal/tui/dictation_download.go (1)
118-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
keyparameter is documented as significant ("A different key breaks the dedupe") but is discarded (_ = key); dedupe is purely text-based. The test only appears to exercise the key because the second call also changes the text. Either drop the parameter or actually key the dedupe on it to avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/dictation_download.go` around lines 118 - 124, The appendDictationNotice behavior is inconsistent with its API because the key argument is ignored and deduping in model.appendDictationNotice is currently based only on transcript text. Either remove the key parameter from appendDictationNotice and its callers if text-only dedupe is intended, or update the dedupe logic to use key as the identity so the documented behavior matches the implementation. Also adjust the related test expectations to reflect the chosen dedupe rule.internal/cli/dictation.go (1)
209-237: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winLoose substring matching in
providerMatchesrisks routing the wrong API key.
strings.Contains(f, provider)against Name/BaseURL/CatalogID/Kind can false-positive on unrelated configured providers whose name or base URL happens to contain another provider's identifier (e.g. a custom proxy named"my-openai-gateway"would matchprovider == "openai", and vice versa for cross-substrings). A false match sends one provider's stored key to a different cloud STT endpoint.Consider requiring an exact match on
CatalogID/Provider/ProviderKindfirst, and only falling back to substring matching onName/BaseURLas a last resort (or anchoring the substring check to a host/word boundary).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/dictation.go` around lines 209 - 237, The provider lookup in keyFromConfiguredProviders/providerMatches is too loose and can select the wrong API key. Update providerMatches to prefer exact equality for CatalogID, Provider, and ProviderKind first, and only use a constrained fallback for Name/BaseURL (for example, anchored/word-boundary matching or a stricter host-based check). Keep the same helper names so the fix stays localized to providerMatches and its call from keyFromConfiguredProviders.internal/cli/dictation_test.go (1)
1-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! Solid coverage of the factory's fallback/error branches and key precedence.
Given the substring-matching concern raised in
dictation.go(Lines 223-237), consider adding a regression test forproviderMatches/keyFromConfiguredProviderswith a deliberately colliding provider name (e.g. a configured provider named"openai-proxy"when resolving"openai") to lock in the intended behavior once addressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/dictation_test.go` around lines 1 - 96, Add a regression test around the provider lookup logic in dictation.go to cover substring collisions in provider matching. Use a deliberately overlapping configured provider name such as "openai-proxy" when resolving the "openai" provider, and assert that providerMatches/keyFromConfiguredProviders do not treat it as a valid match unless it is an exact intended match. Place the test alongside the existing factory/key resolution tests in dictation_test.go so the intended behavior stays locked in.internal/config/stt_test.go (1)
98-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on
TestSetSTTModelPersists, but consider adding coverage forSetSTTLocalEngineandSetSTTProvidertoo.A round-trip test for
SetSTTLocalEngine(setstreamProviderto a cloud value first, call it withstreaming=true, then assertSTTStreamProvider()resolves to local) would have caught the missingStreamProviderupdate flagged ininternal/config/writer.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/stt_test.go` around lines 98 - 120, Extend the STT config round-trip coverage beyond `TestSetSTTModelPersists` by adding tests for `SetSTTLocalEngine` and `SetSTTProvider` in `internal/config/stt_test.go`. In particular, add a case that first sets a cloud `streamProvider`, then calls `SetSTTLocalEngine` with `streaming=true`, and verifies `STTStreamProvider()` resolves to local so the missing `StreamProvider` update in `SetSTTLocalEngine` is caught. Also add a simple persistence check for `SetSTTProvider` to confirm the provider field is written and read back correctly through `ValidateFile`.internal/dictation/server.go (2)
126-132: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
(*net.Dialer).DialContextinstead ofnet.DialTimeout(golangcinoctx, lines 126 and 237).Both
portListeninganddialHealthChecktrip thenoctxlinter. FordialHealthCheckit's more than style: the 500msDialTimeoutignoresctx, so cancellation is only observed between dials, not during one. Threadingctxthrough anet.Dialer.DialContextfixes the lint and makes the poll promptly cancellable.♻️ Proposed change for dialHealthCheck
func dialHealthCheck(ctx context.Context, port int) error { addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(port)) deadline := time.Now().Add(5 * time.Second) + dialer := net.Dialer{Timeout: 500 * time.Millisecond} for { if ctx.Err() != nil { return ctx.Err() } - conn, err := net.DialTimeout("tcp", addr, 500*time.Millisecond) + conn, err := dialer.DialContext(ctx, "tcp", addr) if err == nil { _ = conn.Close() return nil } if time.Now().After(deadline) { return fmt.Errorf("port %d not listening after 5s", port) } time.Sleep(100 * time.Millisecond) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dictation/server.go` around lines 126 - 132, The `dialHealthCheck` helper should stop using `net.DialTimeout` and instead use `(*net.Dialer).DialContext` so the existing `ctx` is honored during the TCP probe. Update `dialHealthCheck` to accept and pass through the context, create a `net.Dialer` with the same timeout, and use `DialContext` when connecting to the localhost port; keep the close-and-return behavior unchanged.Source: Linters/SAST tools
78-115: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
m.muis held across the (up to 5s) health check.
EnsureRunninglocksm.mufor its whole body, includingm.cfg.healthCheckwhich polls for up to 5s. During that windowShutdown,URL,SetModelPath, and any concurrentEnsureRunningall block on the same mutex — so an exit requested during a cold start waits out the full startup poll. Consider capturing the spec under the lock, releasing it for the health check, then re-acquiring to publishproc/url(guarding against a racing starter).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dictation/server.go` around lines 78 - 115, The EnsureRunning method in dictation/server.go holds m.mu across the slow m.cfg.healthCheck call, which blocks Shutdown, URL, SetModelPath, and concurrent EnsureRunning calls during startup. Refactor EnsureRunning to only use m.mu while reading/updating shared state, capture the needed spec/proc data before releasing the lock, run healthCheck without the mutex, then re-lock to publish m.proc and m.url while checking for a racing starter and cleaning up any stale proc if another goroutine already initialized the server.internal/dictation/transcriber_local.go (1)
277-332: 🎯 Functional Correctness | 🔵 TrivialFamily-detection helpers pick a file via map iteration order, which is non-deterministic.
consistentQuantizedSet,singleModelOnnx,findContains, andfindOnnxMatchingall range over thefilesmap and return on the first (or, forsingleModelOnnx, effectively the last) match. If a model directory ever contains more than one candidate for the same role at the same quantization tier (e.g. a leftover checkpoint from a prior download), which file gets selected is non-deterministic across runs — a subtle, hard-to-reproduce bug report waiting to happen.Sorting keys before iterating (or preferring lexicographically-first/newest) would make selection deterministic at negligible cost.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dictation/transcriber_local.go` around lines 277 - 332, The file-selection helpers are relying on Go map iteration order, so selection is non-deterministic when multiple candidates exist. Update consistentQuantizedSet, singleModelOnnx, findContains, and findOnnxMatching to choose deterministically by sorting the file names (or using another explicit priority rule) before scanning, and keep the existing role/int8/token matching behavior unchanged. This will make the candidate picked for each role stable across runs even if multiple matching files are present.internal/dictation/transcriber_stream_test.go (1)
33-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead assertion variables — auth header and commit ordering aren't actually verified.
gotAuth(line 34) andcommitted(line 91) are captured but never asserted against — both are discarded via_ = xrather than checked. As written,TestDeepgramStreamTranscribedoesn't verify theAuthorization: Token ...header actually reaches the server, andTestOpenAIRealtimeStreamTranscribedoesn't verifyinput_audio_buffer.commitwas sent before completion — despite both variables clearly being set up to check exactly that.♻️ Suggested fix (Deepgram auth header)
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) { defer c.Close(websocket.StatusNormalClosure, "") // Read audio frames until CloseStream, then emit interim + final results. for { typ, data, err := c.Read(ctx) if err != nil { return } if typ == websocket.MessageText && strings.Contains(string(data), "CloseStream") { break } }Note: capturing the request header requires exposing
r.HeaderfromwsTestServerto the handler (it currently only passesr.Context()), or adding a small header-capturing wrapper handler here.♻️ Suggested fix (OpenAI Realtime commit assertion)
- _ = committed + if !committed { + t.Error("expected input_audio_buffer.commit before completion") + }Also applies to: 88-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dictation/transcriber_stream_test.go` around lines 33 - 73, `TestDeepgramStreamTranscribe` and `TestOpenAIRealtimeStreamTranscribe` set up `gotAuth` and `committed` but never assert them, so the tests don’t verify the intended behavior. Update the Deepgram test to capture the websocket request headers from `wsTestServer` (or a small wrapper) and assert the `Authorization: Token ...` value in `gotAuth`, and remove the `_ = gotAuth` discard. In the OpenAI Realtime test, assert that `input_audio_buffer.commit` is observed before the completion event using `committed`, and remove the `_ = committed` discard so the ordering is actually checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/app.go`:
- Around line 731-735: The STT download root is being computed from
config.UserConfigDir() instead of the injected userConfigPath source, which can
place downloads outside the same config tree used elsewhere. Update the logic in
the app setup around newDictationServerManager to derive sttDownloadRoot from
filepath.Dir(userConfigPath) instead, keeping it aligned with the rest of the
TUI configuration path handling.
In `@internal/config/writer.go`:
- Around line 214-245: SetSTTLocalEngine currently updates cfg.STT.Provider and
local engine paths but leaves cfg.STT.StreamProvider unchanged, so
buildStreamingTranscriber can still choose a cloud stream. Update
SetSTTLocalEngine to persist cfg.STT.StreamProvider = STTProviderLocal when
streaming is true, alongside the existing STTProviderLocal assignment and
writeConfigFile call.
In `@internal/dictation/download.go`:
- Around line 556-589: `EstimatedDownloadBytes` is using the default model
constant instead of the caller-selected model, so the download estimate can be
wrong for non-default choices. Update the model resolution path in
`EstimatedDownloadBytes` to use `opts.ModelAssetName` with the same fallback
behavior as `EnsureLocalEngine`, and pass that resolved value into the
`resolveAsset` call for the model so the confirm prompt reflects the actual
selected model.
In `@internal/dictation/recorder.go`:
- Around line 361-418: The EOF path in the dictation stream reader does not
trigger teardown, so the recording state and process handles can remain open
when the producer exits unexpectedly. In the reader goroutine around chunk
handling in recorder.go, ensure stop() is invoked on every exit path by
deferring it at the start of the goroutine; since stop() is guarded by stopOnce,
this will remain safe and idempotent while guaranteeing r.recording, stdout, and
proc are cleaned up when chunks closes or readFullOrEOF returns.
In `@internal/dictation/transcriber_local_stream.go`:
- Around line 41-50: Stop the capture path when local streaming setup fails in
StreamTranscribe. In localStreamingTranscriber.StreamTranscribe, if
l.manager.EnsureRunning or dialWithOneRestart returns an error, explicitly
cancel/stop the active recording or tap pipeline before returning so chunks is
no longer left undrained. Reuse the existing local streaming/session lifecycle
helpers around StreamTranscribe, EnsureRunning, and dialWithOneRestart to make
sure a startup failure tears down the mic path immediately.
In `@internal/dictation/transcriber_local.go`:
- Around line 96-116: The cancellation check in transcriber_local.go only
happens before launching the subprocess, so the sherpa-onnx command can still
run indefinitely once started. Update the transcription path in the local
transcriber (around the code using l.cfg.runOutput and parseSherpaOutput) to
pass ctx into runCommandOutput/runOutput and use a context-aware exec path so
cancellation and timeouts can terminate the transcription process. Keep the
existing error handling in TranscribeLocal, but make sure the command execution
itself observes ctx.
In `@internal/dictation/transcriber_openai_realtime.go`:
- Around line 54-78: The realtime transcription setup is still using the old
beta session event and header shape; update
openAIRealtimeTranscriber.StreamTranscribe to send a session.update event
instead of transcription_session.update, and move the model to
session.audio.input.transcription.model. Also remove the OpenAI-Beta:
realtime=v1 header from the websocket dial in StreamTranscribe, while keeping
the existing conversation.item.input_audio_transcription.* and
input_audio_buffer.committed handling unchanged.
In `@internal/tui/dictation_stream.go`:
- Around line 44-53: The tap goroutine in dictation_stream.go can block on
tapped <- chunk if StreamTranscribe exits early and the buffer fills. Update the
goroutine around tapped, chunks, and StreamTranscribe to listen for a session
shutdown signal and stop forwarding chunks when the streaming session ends; do
not rely only on ctx.Done() unless that context is definitely canceled on exit.
Ensure the goroutine terminates cleanly when the recorder or stream closes so
the capture path cannot stay alive.
In `@internal/tui/dictation_voice.go`:
- Around line 24-46: Gate Space handling in handleVoiceSpacePress on
dictation.eventTypesKnown so an initial Space press does not assume unsupported
capabilities before handleKeyboardEnhancements runs. Update the flow in
model.handleKeyboardEnhancements and handleVoiceSpacePress to treat the unknown
state separately, and only choose the toggle-vs-hold behavior after
eventTypesKnown has been set by the KeyboardEnhancementsMsg.
In `@internal/tui/dictation.go`:
- Around line 373-377: Stop the streaming recorder before resetting dictation
state in handleDictationTranscribed: the current order clears streamStop while
the recorder may still be active, which can leave the mic/process running and
cause errAlreadyRecording on the next attempt. Update the
handleDictationTranscribed flow in the model so the recorder stop action is
triggered before m.dictation.reset(), keeping the controller state cleanup after
the active stream has been stopped.
---
Outside diff comments:
In `@internal/tui/view.go`:
- Around line 200-235: The tiny-tier branch in view.go is missing the in-flight
model download state that the non-tiny path already handles via
m.dictation.downloading and m.dictation.downloadStatus, so narrow terminals can
show no feedback during downloads. Update the tierTiny logic to account for the
download state, likely by prioritizing the same download chip before falling
back to dictationStatusChip() or left. Use the existing symbols
m.dictation.downloading, m.dictation.downloadStatus, and dictationStatusChip()
to keep the behavior aligned across tiers.
---
Nitpick comments:
In `@internal/cli/dictation_test.go`:
- Around line 1-96: Add a regression test around the provider lookup logic in
dictation.go to cover substring collisions in provider matching. Use a
deliberately overlapping configured provider name such as "openai-proxy" when
resolving the "openai" provider, and assert that
providerMatches/keyFromConfiguredProviders do not treat it as a valid match
unless it is an exact intended match. Place the test alongside the existing
factory/key resolution tests in dictation_test.go so the intended behavior stays
locked in.
In `@internal/cli/dictation.go`:
- Around line 209-237: The provider lookup in
keyFromConfiguredProviders/providerMatches is too loose and can select the wrong
API key. Update providerMatches to prefer exact equality for CatalogID,
Provider, and ProviderKind first, and only use a constrained fallback for
Name/BaseURL (for example, anchored/word-boundary matching or a stricter
host-based check). Keep the same helper names so the fix stays localized to
providerMatches and its call from keyFromConfiguredProviders.
In `@internal/config/stt_test.go`:
- Around line 98-120: Extend the STT config round-trip coverage beyond
`TestSetSTTModelPersists` by adding tests for `SetSTTLocalEngine` and
`SetSTTProvider` in `internal/config/stt_test.go`. In particular, add a case
that first sets a cloud `streamProvider`, then calls `SetSTTLocalEngine` with
`streaming=true`, and verifies `STTStreamProvider()` resolves to local so the
missing `StreamProvider` update in `SetSTTLocalEngine` is caught. Also add a
simple persistence check for `SetSTTProvider` to confirm the provider field is
written and read back correctly through `ValidateFile`.
In `@internal/dictation/download.go`:
- Around line 221-249: The GitHub API requests in ListModels, getJSON, and
resolveAsset are unauthenticated, which can quickly hit the shared 60 req/hr
limit; update the request flow to attach an Authorization header when a token is
available (for example from GITHUB_TOKEN or a config field) so callers can opt
into higher limits. Make sure the same authenticated client behavior is used by
the related calls in EnsureLocalEngine and EstimatedDownloadBytes, and
distinguish rate-limit responses from generic HTTP failures in the returned
errors.
In `@internal/dictation/server.go`:
- Around line 126-132: The `dialHealthCheck` helper should stop using
`net.DialTimeout` and instead use `(*net.Dialer).DialContext` so the existing
`ctx` is honored during the TCP probe. Update `dialHealthCheck` to accept and
pass through the context, create a `net.Dialer` with the same timeout, and use
`DialContext` when connecting to the localhost port; keep the close-and-return
behavior unchanged.
- Around line 78-115: The EnsureRunning method in dictation/server.go holds m.mu
across the slow m.cfg.healthCheck call, which blocks Shutdown, URL,
SetModelPath, and concurrent EnsureRunning calls during startup. Refactor
EnsureRunning to only use m.mu while reading/updating shared state, capture the
needed spec/proc data before releasing the lock, run healthCheck without the
mutex, then re-lock to publish m.proc and m.url while checking for a racing
starter and cleaning up any stale proc if another goroutine already initialized
the server.
In `@internal/dictation/transcriber_local.go`:
- Around line 277-332: The file-selection helpers are relying on Go map
iteration order, so selection is non-deterministic when multiple candidates
exist. Update consistentQuantizedSet, singleModelOnnx, findContains, and
findOnnxMatching to choose deterministically by sorting the file names (or using
another explicit priority rule) before scanning, and keep the existing
role/int8/token matching behavior unchanged. This will make the candidate picked
for each role stable across runs even if multiple matching files are present.
In `@internal/dictation/transcriber_stream_test.go`:
- Around line 33-73: `TestDeepgramStreamTranscribe` and
`TestOpenAIRealtimeStreamTranscribe` set up `gotAuth` and `committed` but never
assert them, so the tests don’t verify the intended behavior. Update the
Deepgram test to capture the websocket request headers from `wsTestServer` (or a
small wrapper) and assert the `Authorization: Token ...` value in `gotAuth`, and
remove the `_ = gotAuth` discard. In the OpenAI Realtime test, assert that
`input_audio_buffer.commit` is observed before the completion event using
`committed`, and remove the `_ = committed` discard so the ordering is actually
checked.
In `@internal/tui/dictation_download.go`:
- Around line 118-124: The appendDictationNotice behavior is inconsistent with
its API because the key argument is ignored and deduping in
model.appendDictationNotice is currently based only on transcript text. Either
remove the key parameter from appendDictationNotice and its callers if text-only
dedupe is intended, or update the dedupe logic to use key as the identity so the
documented behavior matches the implementation. Also adjust the related test
expectations to reflect the chosen dedupe rule.
In `@internal/tui/stt_model_picker.go`:
- Around line 164-169: The model-list fetch in fetchSTTModelsCmd can hang
indefinitely because it uses context.Background() with no timeout, so update it
to create a bounded context with context.WithTimeout and defer cancel before
calling dictation.ListModels. Keep the existing sttModelsFetchedMsg flow
unchanged, but ensure a stalled request eventually returns so
handleSTTModelsFetched can fall back to the curated shortlist instead of leaving
the picker stuck loading.
- Around line 17-19: The comment is attached to the wrong identifier: it
describes sttModelValue, but the declared constant in the picker code is
sttValueSep. Update the declaration in stt_model_picker.go so the symbol name
matches what the doc comment describes, or revise the comment to accurately
document sttValueSep; use the nearby sttModelValue encoding logic as the
reference point when renaming.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41a537c6-388a-4260-a5ec-c11df4bbee29
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (54)
go.modinternal/cli/app.gointernal/cli/dictation.gointernal/cli/dictation_test.gointernal/config/resolver.gointernal/config/stt_test.gointernal/config/types.gointernal/config/validate.gointernal/config/writer.gointernal/dictation/dictation.gointernal/dictation/download.gointernal/dictation/download_smoke_test.gointernal/dictation/download_test.gointernal/dictation/listmodels_test.gointernal/dictation/local_smoke_test.gointernal/dictation/owner_unix.gointernal/dictation/owner_windows.gointernal/dictation/recorder.gointernal/dictation/recorder_test.gointernal/dictation/runner.gointernal/dictation/server.gointernal/dictation/server_test.gointernal/dictation/spill.gointernal/dictation/stream_smoke_test.gointernal/dictation/transcriber.gointernal/dictation/transcriber_cloud.gointernal/dictation/transcriber_cloud_test.gointernal/dictation/transcriber_deepgram.gointernal/dictation/transcriber_local.gointernal/dictation/transcriber_local_stream.gointernal/dictation/transcriber_local_test.gointernal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_stream_test.gointernal/providermodelcatalog/filter.gointernal/providermodelcatalog/stt_filter_test.gointernal/tui/autocomplete_test.gointernal/tui/commands.gointernal/tui/dictation.gointernal/tui/dictation_download.gointernal/tui/dictation_download_test.gointernal/tui/dictation_stream.gointernal/tui/dictation_test.gointernal/tui/dictation_voice.gointernal/tui/dictation_voice_test.gointernal/tui/keybindings.gointernal/tui/model.gointernal/tui/mouse.gointernal/tui/mouse_test.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/stt_key_prompt.gointernal/tui/stt_key_prompt_test.gointernal/tui/stt_model_picker.gointernal/tui/view.go
Comment on lines
+556
to
+589
| // EstimatedDownloadBytes resolves the total download size for the confirm prompt | ||
| // (engine + model) via the API, or a rough fallback on any resolution error. | ||
| func EstimatedDownloadBytes(ctx context.Context, opts DownloadOptions) int64 { | ||
| key := opts.platformKey | ||
| if key == "" { | ||
| key = platformKey() | ||
| } | ||
| suffix, ok := engineAssetSuffix[key] | ||
| if !ok { | ||
| return 0 | ||
| } | ||
| version := strings.TrimSpace(opts.EngineVersion) | ||
| if version == "" { | ||
| version = DefaultSherpaVersion | ||
| } | ||
| client := opts.HTTPClient | ||
| if client == nil { | ||
| client = http.DefaultClient | ||
| } | ||
| apiBase := opts.APIBase | ||
| if apiBase == "" { | ||
| apiBase = defaultAPIBase | ||
| } | ||
| const fallback = 126 << 20 // ~engine + model, if the API can't be reached | ||
| engine, err := resolveAsset(ctx, client, apiBase, version, "sherpa-onnx-", suffix) | ||
| if err != nil { | ||
| return fallback | ||
| } | ||
| model, err := resolveAsset(ctx, client, apiBase, modelReleaseTag, modelAssetName, "") | ||
| if err != nil { | ||
| return fallback | ||
| } | ||
| return engine.size + model.size | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
EstimatedDownloadBytes ignores opts.ModelAssetName — always sizes the default model.
resolveAsset is called with the package constant modelAssetName (moonshine-tiny) instead of the caller's selected model. EnsureLocalEngine (Lines 508-511) correctly falls back from opts.ModelAssetName to the const; this function doesn't. Since the doc comment states this drives the download confirm prompt, picking any non-default model (e.g. "Whisper large v3" at ~1GB) will still show the ~130MB default-model estimate — exactly the kind of silent trust-decision mismatch this file's own design notes warn against.
🐛 Proposed fix
const fallback = 126 << 20 // ~engine + model, if the API can't be reached
engine, err := resolveAsset(ctx, client, apiBase, version, "sherpa-onnx-", suffix)
if err != nil {
return fallback
}
- model, err := resolveAsset(ctx, client, apiBase, modelReleaseTag, modelAssetName, "")
+ modelName := opts.ModelAssetName
+ if modelName == "" {
+ modelName = modelAssetName
+ }
+ model, err := resolveAsset(ctx, client, apiBase, modelReleaseTag, modelName, "")
if err != nil {
return fallback
}
return engine.size + model.size📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // EstimatedDownloadBytes resolves the total download size for the confirm prompt | |
| // (engine + model) via the API, or a rough fallback on any resolution error. | |
| func EstimatedDownloadBytes(ctx context.Context, opts DownloadOptions) int64 { | |
| key := opts.platformKey | |
| if key == "" { | |
| key = platformKey() | |
| } | |
| suffix, ok := engineAssetSuffix[key] | |
| if !ok { | |
| return 0 | |
| } | |
| version := strings.TrimSpace(opts.EngineVersion) | |
| if version == "" { | |
| version = DefaultSherpaVersion | |
| } | |
| client := opts.HTTPClient | |
| if client == nil { | |
| client = http.DefaultClient | |
| } | |
| apiBase := opts.APIBase | |
| if apiBase == "" { | |
| apiBase = defaultAPIBase | |
| } | |
| const fallback = 126 << 20 // ~engine + model, if the API can't be reached | |
| engine, err := resolveAsset(ctx, client, apiBase, version, "sherpa-onnx-", suffix) | |
| if err != nil { | |
| return fallback | |
| } | |
| model, err := resolveAsset(ctx, client, apiBase, modelReleaseTag, modelAssetName, "") | |
| if err != nil { | |
| return fallback | |
| } | |
| return engine.size + model.size | |
| } | |
| // EstimatedDownloadBytes resolves the total download size for the confirm prompt | |
| // (engine + model) via the API, or a rough fallback on any resolution error. | |
| func EstimatedDownloadBytes(ctx context.Context, opts DownloadOptions) int64 { | |
| key := opts.platformKey | |
| if key == "" { | |
| key = platformKey() | |
| } | |
| suffix, ok := engineAssetSuffix[key] | |
| if !ok { | |
| return 0 | |
| } | |
| version := strings.TrimSpace(opts.EngineVersion) | |
| if version == "" { | |
| version = DefaultSherpaVersion | |
| } | |
| client := opts.HTTPClient | |
| if client == nil { | |
| client = http.DefaultClient | |
| } | |
| apiBase := opts.APIBase | |
| if apiBase == "" { | |
| apiBase = defaultAPIBase | |
| } | |
| const fallback = 126 << 20 // ~engine + model, if the API can't be reached | |
| engine, err := resolveAsset(ctx, client, apiBase, version, "sherpa-onnx-", suffix) | |
| if err != nil { | |
| return fallback | |
| } | |
| modelName := opts.ModelAssetName | |
| if modelName == "" { | |
| modelName = modelAssetName | |
| } | |
| model, err := resolveAsset(ctx, client, apiBase, modelReleaseTag, modelName, "") | |
| if err != nil { | |
| return fallback | |
| } | |
| return engine.size + model.size | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/dictation/download.go` around lines 556 - 589,
`EstimatedDownloadBytes` is using the default model constant instead of the
caller-selected model, so the download estimate can be wrong for non-default
choices. Update the model resolution path in `EstimatedDownloadBytes` to use
`opts.ModelAssetName` with the same fallback behavior as `EnsureLocalEngine`,
and pass that resolved value into the `resolveAsset` call for the model so the
confirm prompt reflects the actual selected model.
New internal/dictation package: platform audio recorders (shelling out to arecord/ffmpeg/parec/termux with discrete argv, no shell strings), an offline sherpa-onnx-offline transcriber with model-family auto-detection, a warm sherpa-onnx websocket streaming server manager (lazy start, health-checked reuse, one crash-restart, torn down on exit), and cloud transcribers (Groq/ OpenAI batch, Deepgram + OpenAI Realtime streaming). Includes the model downloader/browser with digest verification and progress reporting. Adds github.com/coder/websocket for the streaming clients.
Add IsSTTModel/IsSTTModelID (modality-metadata first, id heuristics as a fallback) so the dictation model picker can surface a provider's transcription models.
Add the /voice hold-to-record gesture (Space), the recording state machine, and a live audio-reactive waveform (real mic levels while streaming, an organic synthesized wave for batch). Add the /stt-model provider picker and the local model download chooser (flat list with a Live/Batch tag, a loading state, and a visible search filter), plus an inline API-key prompt for cloud providers. Turning /voice off releases the warm streaming server. Streaming inserts a live transcript region into the composer; batch inserts on stop.
Build the transcriber factory the TUI calls per recording (resolving cloud provider keys from configured providers, the credential store, then env), the warm sherpa-onnx streaming server manager, and the STT key status/save helpers; pass them into the TUI options and tear the server down on exit.
anandh8x
force-pushed
the
feat/voice-dictation
branch
from
d92dc50 to
14669ad
Compare
- Streaming teardown: stop the recorder and cancel the context when a streaming session ends via a transcriber error (not just a user stop), and let the audio tap goroutine exit on ctx.Done. An error path no longer leaves the mic running or makes the next attempt hit 'already recording'. - Recorder: the capture reader tears down on its own EOF/crash (defer stop), so a self-terminating device can't wedge the recorder with recording still true. - Offline transcribe: plumb ctx into the exec (exec.CommandContext) so a wedged sherpa-onnx-offline is cancellable rather than hanging the caller. - Config: SetSTTLocalEngine also resets streamProvider to local, so a leftover cloud value can't keep the live transcript on the cloud after switching to a downloaded local model. - CLI: derive the STT download root from userConfigPath so it stays in the same config tree when the config root is overridden. - Download: drop the unused EstimatedDownloadBytes (dead code). - Test: skip the Unix exec-bit assertion on Windows, fixing the Windows smoke run.
The test's waitFor predicate checked len(l.recorded()) == 2, which becomes true the instant Spawn returns (scheduler.go:307) — but incRuns() runs a beat later in the same goroutine (scheduler.go:285). On slower Windows CI the test read j.Runs before the increment landed, producing a spurious 'runs=1 skipped=1, want 2/1' failure. Wait for j.Runs == 2 directly instead of the launcher-recorded proxy.
…ted header CodeRabbit flagged that transcriber_openai_realtime.go was still using the beta event shape. Verified against OpenAI's current realtime-transcription guide: - transcription_session.update -> session.update with session.type=transcription - input_audio_format/input_audio_transcription flattened into the GA session.audio.input.format + session.audio.input.transcription nesting - OpenAI-Beta: realtime=v1 header removed (deprecated for GA endpoint) - Default model gpt-4o-transcribe -> gpt-realtime-whisper (the natively streaming model per the docs; gpt-4o-transcribe is for non-streaming) The fake test server's switch case is updated to match session.update. Closes the last unresolved CodeRabbit comment on PR 549.
anandh8x
force-pushed
the
feat/voice-dictation
branch
from
38fa758 to
dc2607d
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
anandh8x
dismissed
coderabbitai[bot]’s stale review
Addressed in commits 78d5f94, a5f0b8e, dc2607d: streaming teardown, recorder EOF, ctx-into-exec, streamProvider reset, sttDownloadRoot, tap-goroutine guard, eventTypesKnown gate, stop-before-reset, EstimatedDownloadBytes removed (dead code), OpenAI Realtime switched to GA session.update shape + deprecated header dropped. All 10 CodeRabbit comments resolved and verified against current PR code; CI green on all platforms including Windows.
Collaborator
Author
|
Closing in favor of a clean squashed PR. All changes, CI, and review fixes carry over. |
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
1 participant
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.
Speech-to-text dictation
Adds voice dictation to Zero. Run
/voiceto enter hold-to-record mode, then hold Space to dictate and release to transcribe (run/voiceagain to turn it off so Space types normally). Pick a backend with/stt-model.Backends
/stt-model; the engine binary is fetched once and shared across models. Family auto-detection covers the mainstream offline families (Whisper, transducer/Zipformer, Moonshine, Paraformer, SenseVoice, and the CTC families FireRedASR, NeMo, TeleSpeech, WeNet, Zipformer-CTC, Dolphin, Omnilingual — plus the encoder/decoder families Canary and FireRedASR-AED).Highlights
/stt-modeldownload chooser: recommended models first, Live/Batch tag per row, size + download state, and a search filter.stt.autoSubmitto fire straight at the agent)./voiceis turned off and on exit.Implementation notes
exec.Commandwith discrete argv (never a shell string).Testing
go build ./...,go vet ./..., and the config / dictation / cli / tui suites pass.Summary by CodeRabbit