Proper STT connection fetching with new ai setting #1582
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemoves the db-script crate and its migration, converts transcript storage to word-level types with segmentation and hooks (useWords, useSegments, TranscriptView), threads model/base_url/api_key through listener/session actors, and simplifies local-stt plugin APIs (removing store-backed commands, Custom model, and event-driven startup). Adds tauri shell plugin to desktop2. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Desktop UI
participant Hook as useSTTConnection
participant Store as Persisted Store
participant Listener as Listener Actor
participant STT as STT Server
UI->>Hook: request STT Connection (provider, model)
alt local model
Hook->>Store: call localSttCommands.get_servers / start_server
Store-->>Hook: ServerInfo (url, health)
Hook-->>UI: Connection {model, base_url, api_key}
else remote provider
Hook-->>UI: Connection from provider config (base_url, api_key)
end
UI->>Listener: start(params {session_id, model, base_url, api_key}, onStreamResponse)
Listener->>STT: Create ListenClient(base_url, api_key, model)
STT-->>Listener: stream transcription chunks
Listener->>Listener: onStreamResponse(payload)
Listener->>Store: append/persist words to transcript[session_id]
UI->>Store: read words via useWords(session_id)
UI->>UI: group into segments via useSegments -> render TranscriptView
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Multiple subsystems changed (DB schema/types, new hooks, listener actor signatures, local-stt public API removal, desktop tauri plugin changes). The edits are heterogeneous and require cross-checking integration points (type migrations, persistence keys, plugin command removals, and listener start/callback wiring). Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (16)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/listener/src/actors/session.rs (1)
28-37: Critical: API key exposure via Debug trait.The
Debugderive onSessionParamswill expose theapi_keyfield in plain text whenever the struct is debug-formatted (e.g., in logs viatracing::debug!("{:?}", params)). This is a security risk.Apply this diff to implement a custom Debug that redacts the API key:
-#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, specta::Type)] +#[derive(Clone, serde::Serialize, serde::Deserialize, specta::Type)] pub struct SessionParams { pub session_id: String, pub languages: Vec<hypr_language::Language>, pub onboarding: bool, pub record_enabled: bool, pub model: String, pub base_url: String, pub api_key: String, } + +impl std::fmt::Debug for SessionParams { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SessionParams") + .field("session_id", &self.session_id) + .field("languages", &self.languages) + .field("onboarding", &self.onboarding) + .field("record_enabled", &self.record_enabled) + .field("model", &self.model) + .field("base_url", &self.base_url) + .field("api_key", &"***REDACTED***") + .finish() + } +}plugins/local-stt/src/ext.rs (1)
78-186: Avoid panics: replace unwrap() health reads with a bounded wait, and handle path discovery failures.Right after spawning the actors you do:
internal_health().await.map(|r| r.0).unwrap()andexternal_health()...unwrap(). These can beNoneif the actor isn’t ready yet, causing a panic.app_data_dir().unwrap()anddirs::home_dir().unwrap()can beNoneon some systems.Fix by adding a bounded wait for health and mapping failures into
crate::Error, and by avoiding unwraps on paths.Apply:
- let cache_dir = self.models_dir(); - let data_dir = self.app_handle().path().app_data_dir().unwrap().join("stt"); + let cache_dir = self.models_dir(); + let data_dir = self + .app_handle() + .path() + .app_data_dir() + .ok_or_else(|| crate::Error::ServerStartFailed("app_data_dir unavailable".into()))? + .join("stt"); @@ - let base_url = internal_health().await.map(|r| r.0).unwrap(); - Ok(base_url) + let base_url = wait_for_internal_health().await?; + Ok(base_url) @@ - let base_url = external_health().await.map(|v| v.0).unwrap(); - Ok(base_url) + let base_url = wait_for_external_health().await?; + Ok(base_url)Add helpers:
use std::time::Duration; async fn wait_for_internal_health() -> Result<String, crate::Error> { for _ in 0..50 { if let Some((url, _)) = internal_health().await { return Ok(url); } tokio::time::sleep(Duration::from_millis(100)).await; } Err(crate::Error::ServerStartFailed("internal server health timed out".into())) } async fn wait_for_external_health() -> Result<String, crate::Error> { for _ in 0..50 { if let Some((url, _)) = external_health().await { return Ok(url); } tokio::time::sleep(Duration::from_millis(100)).await; } Err(crate::Error::ServerStartFailed("external server health timed out".into())) }Also replace
current_dir(dirs::home_dir().unwrap())with a safer fallback:- .current_dir(dirs::home_dir().unwrap()) + .current_dir(dirs::home_dir().unwrap_or(std::env::current_dir().unwrap_or_else(|_| PathBuf::from("/"))))
🧹 Nitpick comments (6)
apps/desktop2/src/types/transcript.ts (1)
1-9: Eliminate type duplication by importing from bindings.The
Wordtype duplicates the type already defined inplugins/listener/js/bindings.gen.ts(line 161). This creates a maintenance risk if the types diverge.Apply this diff to import the type instead:
+import type { Word } from "../../../plugins/listener/js/bindings.gen"; + -export type Word = { - word: string; - start: number; - end: number; - confidence: number; - speaker: number | null; - punctuated_word: string | null; - language: string | null; -}; - export type Segment = { speaker: number | null; words: Word[]; startTime: number; endTime: number; };apps/desktop2/src/components/main/body/sessions/floating/listen.tsx (1)
48-57: Consider adding user feedback when store is unavailable.The store guard prevents errors, but clicking the button does nothing if the store is null. Consider logging or showing a toast to inform users why the action didn't proceed.
Example enhancement:
const handleClick = useCallback(() => { if (store) { start(tab.id, store); if (remote?.url) { openUrl(remote.url); } + } else { + console.error("Store not available, cannot start listening"); + // or show a toast notification } }, [start, remote, tab.id, store]);apps/desktop2/src/components/main/body/sessions/note-input/transcript.tsx (1)
64-68: Use cn utility for className composition per coding guidelines.The coding guidelines specify using the
cnutility with array format when composing Tailwind classes. Lines 67 and 88 use string literals instead.As per coding guidelines.
Apply this diff:
- <div className="relative h-full flex flex-col"> + <div className={cn(["relative h-full flex flex-col"])}> <div ref={scrollContainerRef} - className="flex-1 min-h-0 overflow-y-auto overflow-x-hidden pt-8 pb-6" + className={cn(["flex-1 min-h-0 overflow-y-auto overflow-x-hidden pt-8 pb-6"])} onScroll={handleScroll} > - <div className="px-8 text-[15px] leading-relaxed space-y-4"> + <div className={cn(["px-8 text-[15px] leading-relaxed space-y-4"])}> {segments.map((segment, idx) => ( - <div key={idx} className="space-y-1"> - <div className="text-xs font-medium text-gray-500"> + <div key={idx} className={cn(["space-y-1"])}> + <div className={cn(["text-xs font-medium text-gray-500"])}> Speaker {segment.speaker ?? "Unknown"} </div> - <div className="text-gray-800"> + <div className={cn(["text-gray-800"])}> {segment.words.map(word => word.punctuated_word || word.word).join(" ")} </div> </div> ))} </div> </div> {!isAtBottom && ( <Button onClick={scrollToBottom} size="sm" - className="absolute bottom-4 left-1/2 transform -translate-x-1/2 rounded-full shadow-lg bg-white hover:bg-gray-50 text-gray-700 border border-gray-200 z-10 flex items-center gap-1" + className={cn(["absolute bottom-4 left-1/2 transform -translate-x-1/2 rounded-full shadow-lg bg-white hover:bg-gray-50 text-gray-700 border border-gray-200 z-10 flex items-center gap-1"])} variant="outline" > <ChevronDownIcon size={14} /> - <span className="text-xs">Go to bottom</span> + <span className={cn(["text-xs"])}>Go to bottom</span> </Button> )} </div>Also applies to: 85-90
apps/desktop2/src/store/zustand/listener.ts (1)
54-61: Guard against duplicate start() calls (listener/interval leaks).If
start()is called twice, you’ll register multiple event handlers and intervals. Add a simple guard or stop+restart behavior.Apply:
- start: (sessionId: string, store: persisted.Store) => { + start: (sessionId: string, store: persisted.Store, params: SessionParams) => { set((state) => mutate(state, (draft) => { draft.loading = true; draft.sessionId = sessionId; }) ); + const curr = get(); + if (curr.status === "running_active") { + // Either no-op or call stop() first; choose policy explicitly. + return; + }plugins/local-stt/src/ext.rs (2)
87-93: Remove unreachable ServerType::Custom arm or mark unreachable.
tis either Internal or External; theCustomarm can’t be hit. Delete it or adddebug_assert!(false)to prevent silent success.- match t { - ServerType::Custom => Ok("".to_string()), + match t {
131-141: Tighten AM API key retrieval.Minor cleanup to avoid repeated clones and empty-string checks.
- let am_key = { - let state = self.state::<crate::SharedState>(); - let key = state.lock().await.am_api_key.clone(); - if key.clone().is_none() || key.clone().unwrap().is_empty() { - return Err(crate::Error::AmApiKeyNotSet); - } - key.clone().unwrap() - }; + let am_key = { + let state = self.state::<crate::SharedState>(); + state + .lock() + .await + .am_api_key + .clone() + .filter(|k| !k.is_empty()) + .ok_or(crate::Error::AmApiKeyNotSet)? + };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
Cargo.lockis excluded by!**/*.lockplugins/listener/js/bindings.gen.tsis excluded by!**/*.gen.tsplugins/local-stt/js/bindings.gen.tsis excluded by!**/*.gen.tsplugins/local-stt/permissions/autogenerated/commands/get_current_model.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/get_custom_api_key.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/get_custom_base_url.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/get_custom_model.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/get_external_server_status.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/get_local_model.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/get_provider.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/get_status.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/is_server_running.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/list_pro_models.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/list_supported_models_info.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/restart_server.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/set_current_model.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/set_custom_api_key.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/set_custom_base_url.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/set_custom_model.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/set_local_model.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/commands/set_provider.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/autogenerated/reference.mdis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/default.tomlis excluded by!plugins/**/permissions/**plugins/local-stt/permissions/schemas/schema.jsonis excluded by!plugins/**/permissions/**
📒 Files selected for processing (26)
Cargo.toml(0 hunks)apps/desktop2/src/components/main/body/sessions/floating/listen.tsx(1 hunks)apps/desktop2/src/components/main/body/sessions/note-input/index.tsx(2 hunks)apps/desktop2/src/components/main/body/sessions/note-input/transcript.tsx(2 hunks)apps/desktop2/src/components/main/body/sessions/transcript.tsx(0 hunks)apps/desktop2/src/hooks/useSegments.ts(1 hunks)apps/desktop2/src/hooks/useWords.ts(1 hunks)apps/desktop2/src/store/zustand/listener.ts(5 hunks)apps/desktop2/src/types/transcript.ts(1 hunks)crates/db-script/Cargo.toml(0 hunks)crates/db-script/src/lib.rs(0 hunks)crates/db-user/Cargo.toml(0 hunks)crates/db-user/src/lib.rs(0 hunks)owhisper/owhisper-interface/src/lib.rs(0 hunks)packages/db/src/schema.ts(1 hunks)plugins/listener/src/actors/listener.rs(2 hunks)plugins/listener/src/actors/session.rs(2 hunks)plugins/local-stt/Cargo.toml(0 hunks)plugins/local-stt/build.rs(0 hunks)plugins/local-stt/src/commands.rs(1 hunks)plugins/local-stt/src/error.rs(0 hunks)plugins/local-stt/src/events.rs(0 hunks)plugins/local-stt/src/ext.rs(2 hunks)plugins/local-stt/src/lib.rs(0 hunks)plugins/local-stt/src/model.rs(0 hunks)plugins/local-stt/src/store.rs(0 hunks)
💤 Files with no reviewable changes (14)
- Cargo.toml
- plugins/local-stt/src/error.rs
- crates/db-user/src/lib.rs
- apps/desktop2/src/components/main/body/sessions/transcript.tsx
- plugins/local-stt/src/events.rs
- owhisper/owhisper-interface/src/lib.rs
- plugins/local-stt/build.rs
- crates/db-script/Cargo.toml
- plugins/local-stt/src/store.rs
- crates/db-user/Cargo.toml
- crates/db-script/src/lib.rs
- plugins/local-stt/src/model.rs
- plugins/local-stt/src/lib.rs
- plugins/local-stt/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop2/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop2/.cursor/rules/style.mdc)
apps/desktop2/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the cn utility imported asimport { cn } from "@hypr/ui/lib/utils"
Always pass an array to cn when composing Tailwind classNames
Split cn array entries by logical grouping when composing Tailwind classNames
Files:
apps/desktop2/src/components/main/body/sessions/floating/listen.tsxapps/desktop2/src/components/main/body/sessions/note-input/transcript.tsxapps/desktop2/src/components/main/body/sessions/note-input/index.tsx
🧬 Code graph analysis (9)
apps/desktop2/src/types/transcript.ts (1)
plugins/listener/js/bindings.gen.ts (1)
Word(162-162)
apps/desktop2/src/components/main/body/sessions/floating/listen.tsx (1)
apps/desktop2/src/store/tinybase/persisted.ts (1)
Store(230-230)
apps/desktop2/src/hooks/useSegments.ts (2)
apps/desktop2/src/types/transcript.ts (1)
Segment(11-16)apps/desktop2/src/hooks/useWords.ts (1)
useWords(5-34)
apps/desktop2/src/hooks/useWords.ts (1)
apps/desktop2/src/types/transcript.ts (1)
Word(1-9)
plugins/local-stt/src/commands.rs (1)
plugins/local-stt/js/bindings.gen.ts (1)
SupportedSttModel(97-97)
apps/desktop2/src/components/main/body/sessions/note-input/transcript.tsx (2)
apps/desktop2/src/hooks/useSegments.ts (1)
useSegments(5-41)apps/desktop2/src/components/main/body/sessions/transcript.tsx (1)
TranscriptView(9-120)
plugins/local-stt/src/ext.rs (2)
plugins/local-stt/js/bindings.gen.ts (1)
SupportedSttModel(97-97)plugins/local-stt/src/commands.rs (1)
start_server(63-68)
apps/desktop2/src/store/zustand/listener.ts (2)
apps/desktop2/src/store/tinybase/persisted.ts (1)
Store(230-230)apps/desktop2/src/types/transcript.ts (1)
Word(1-9)
apps/desktop2/src/components/main/body/sessions/note-input/index.tsx (1)
apps/desktop2/src/components/main/body/sessions/note-input/transcript.tsx (1)
TranscriptView(18-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-14)
- GitHub Check: zizmor
🔇 Additional comments (13)
plugins/listener/src/actors/session.rs (1)
340-351: LGTM: STT configuration properly propagated.The explicit parameter flow from
SessionParamsthroughListenerArgscorrectly replaces the previous storage-backed approach. The three new fields (model,base_url,api_key) are consistently passed to the listener actor.plugins/listener/src/actors/listener.rs (2)
25-33: LGTM: Good security practice on ListenerArgs.
ListenerArgscorrectly omits theDebugderive, preventing accidental exposure of theapi_keyfield in logs. This is a good security practice for structs containing credentials.
155-164: Code configuration is secure; no credential logging detected.Verification confirms that
api_keyis not logged anywhere in the codebase. TheListenClientconfiguration at lines 155-164 correctly handles credentials, and error handling viaStreamStartFailedis appropriate.packages/db/src/schema.ts (1)
111-121: LGTM! Schema evolution to word-level granularity.The transcript schema has been updated to support word-level data with speaker identification and metadata. The structure aligns with the new Word type definitions and enables richer transcript rendering with confidence scores and punctuation.
apps/desktop2/src/components/main/body/sessions/note-input/index.tsx (2)
1-1: LGTM! Import needed for editorRef usage.The
useRefimport is correctly added for theeditorRefused on lines 16 and 23.
11-11: LGTM! TranscriptView properly replaces TranscriptEditorWrapper.The component replacement cleanly integrates the new segment-based transcript rendering with the existing note input flow.
Also applies to: 35-35
apps/desktop2/src/hooks/useSegments.ts (1)
5-41: LGTM! Clean segment derivation logic.The hook correctly groups consecutive words by speaker and properly memoizes the result. The algorithm handles edge cases (empty words, segment boundaries) correctly.
apps/desktop2/src/hooks/useWords.ts (1)
13-31: LGTM! setWords callback correctly memoized.The callback properly guards against missing store and correctly updates the transcript cell with stringified data.
apps/desktop2/src/components/main/body/sessions/note-input/transcript.tsx (2)
73-75: Fix speaker 0 displaying as "Unknown".The nullish coalescing operator
??treats0as truthy, butspeakerisnumber | null. A speaker with ID0would incorrectly display as "Speaker 0" initially, but the logic is actually correct. On second look, this is working as intended—0 ?? "Unknown"evaluates to0.Actually, reviewing more carefully: the template literal
Speaker {segment.speaker ?? "Unknown"}will correctly show "Speaker 0" for speaker 0, "Speaker 1" for speaker 1, and "Speaker Unknown" when speaker is null. The logic is correct.
18-97: LGTM! Well-structured transcript viewer with auto-scroll.The component correctly uses the segments hook, implements smooth auto-scroll behavior with user control, and renders speaker-segmented transcript data cleanly.
plugins/local-stt/src/commands.rs (1)
63-68: All callers correctly provide the required model parameter.The
start_serversignature change fromOption<SupportedSttModel>toSupportedSttModelhas been properly propagated to the auto-generated TypeScript bindings via Specta. The generated bindings file (plugins/local-stt/js/bindings.gen.tsline 53) correctly reflects the new signature:startServer(model: SupportedSttModel). The Rust call site (plugins/local-stt/src/commands.rs:67) passes the model parameter as expected. TypeScript type checking will enforce the requirement automatically for any frontend calls.plugins/local-stt/src/ext.rs (1)
23-24: Signature change verified: all call sites updated correctly.Rust trait (ext.rs:21-24) now requires
model: SupportedSttModel. Verification confirms:
- Rust command wrapper (commands.rs:67) correctly receives and passes the model parameter
- TypeScript bindings (bindings.gen.ts:53) correctly reflect the updated signature with model parameter
- No missing or incorrect call sites in local-stt plugin
- local-llm calls are unrelated (separate plugin/trait)
apps/desktop2/src/store/zustand/listener.ts (1)
5-12: Original review comment is incorrect. Word types are identical across plugin, app, and schema.The
Wordtype imported from@hypr/plugin-listener(defined inplugins/listener/js/bindings.gen.ts) is structurally identical to the localWordtype inapps/desktop2/src/types/transcript.ts, which also matches thetranscriptSchemainpackages/db/src/schema.ts. All three define the exact same shape:{ word, start, end, confidence, speaker | null, punctuated_word | null, language | null }. The code correctly stores plugin words directly to the persisted store without transformation, and there is no risk of type mismatch at runtime.Likely an incorrect or invalid review comment.
yujonglee
changed the title
No description provided.