pro model rollout 2 #1372
pro model rollout 2 #1372
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughServer health reporting switched to a typed ServerHealth enum; local STT plugin and desktop STT UI were refactored — model selection, download-status polling, and licensing gating added; app window lifecycle now drives server shutdown; multiple timing/defaults and small model/data transformations adjusted across audio/transcription components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Desktop STT UI
participant JS as local-stt JS bindings
participant Plugin as local-stt Rust plugin
participant Srv as Local STT Servers
rect #E8F3FF
User->>UI: Click model entry
UI->>JS: setCurrentModel / restart request
JS->>Plugin: invoke set_current_model / restart
Plugin->>Srv: stop internal/external servers
Plugin->>Srv: start servers for selected model
Plugin-->>JS: return status
JS-->>UI: update UI/status
end
sequenceDiagram
autonumber
participant Tauri as App runtime
participant Events as plugins/local-stt events
participant Plugin as local-stt Rust plugin
rect #F4FFF0
Tauri->>Events: RunEvent::WindowEvent(label, CloseRequested/Destroyed)
Events->>Events: parse label -> if Main
alt Main window
Events->>Plugin: log "events: stopping servers"
Events->>Plugin: terminate internal/external
Events->>Plugin: cancel download tasks
else Not main / parse fail
Events->>Events: ignore / log parse error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (7)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
yujonglee
force-pushed
the
pro-models-rollout-2
branch
from
ab3fac5 to
2f34cb1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/settings/components/ai/stt-view-local.tsx (1)
270-276: Fix incorrect type cast and avoid double server restart on selection.Enabling Pro models now exercises this path for AM models; casting to
WhisperModelis wrong and the extra stop/start introduces avoidable flapping and race with the plugin’s own restart insidesetCurrentModel.Apply this diff to rely solely on
setCurrentModeland use the correct type:- onClick={() => { - if (model.downloaded && !disabled) { - setSelectedSTTModel(model.key as WhisperModel); - localSttCommands.setCurrentModel(model.key as WhisperModel); - localSttCommands.stopServer(null); - localSttCommands.startServer(null); - } - }} + onClick={() => { + if (model.downloaded && !disabled) { + setSelectedSTTModel(model.key); + // setCurrentModel internally stops current server (if any) and starts the one for the selected model + void localSttCommands.setCurrentModel(model.key as SupportedSttModel); + } + }}Follow-up: After this change,
WhisperModelimport becomes unused; see next comment to remove it.
🧹 Nitpick comments (9)
apps/desktop/src/components/settings/components/ai/stt-view-local.tsx (3)
227-227: Redundantdisabled={false}prop — can be dropped.
disableddefaults to falsey. Removing it reduces noise without changing behavior.Apply this diff:
- disabled={false}
214-221: Update header copy: models are now interactive.“Available soon” is misleading if entries are clickable when downloaded.
Apply this diff:
- <h3>Pro Models (Available soon)</h3> + <h3>Pro Models</h3>
261-262: Remove redundantkeyon inner div.The list item already has a
keyon theModelEntrycomponent; React ignoreskeyon non-list children.Apply this diff:
- <div - key={model.key} + <divplugins/local-stt/src/ext.rs (3)
308-309: Avoid holding the state lock across.await— refactor to prevent lock contention.
server.health().awaitis awaited whileguardholds thetokio::Mutex. This can throttle other callers and is a common footgun. Take the servers out, drop the lock, perform health checks, then reacquire to mutate.Apply this refactor scoped to
get_servers:- async fn get_servers(&self) -> Result<HashMap<ServerType, Option<String>>, crate::Error> { - let state = self.state::<crate::SharedState>(); - let mut guard = state.lock().await; - - let internal_url = if let Some(server) = &guard.internal_server { - if server.health().await { - Some(server.base_url.clone()) - } else { - if let Some(server) = guard.internal_server.take() { - server.terminate().ok(); - guard.internal_server = None; - } - None - } - } else { - None - }; - - let external_url = if let Some(server) = &guard.external_server { - if server.health().await { - Some(server.base_url.clone()) - } else { - if let Some(server) = guard.external_server.take() { - server.terminate().ok(); - guard.external_server = None; - } - None - } - } else { - None - }; - - Ok([ - (ServerType::Internal, internal_url), - (ServerType::External, external_url), - ] - .into_iter() - .collect()) - } + async fn get_servers(&self) -> Result<HashMap<ServerType, Option<String>>, crate::Error> { + let state = self.state::<crate::SharedState>(); + // Take the servers out to avoid holding the lock across awaits + let (mut internal, mut external) = { + let mut guard = state.lock().await; + (guard.internal_server.take(), guard.external_server.take()) + }; + + // Check internal health without holding the lock + let internal_url = if let Some(server) = internal.take() { + if server.health().await { + let url = Some(server.base_url.clone()); + // Put it back + let mut guard = state.lock().await; + guard.internal_server = Some(server); + url + } else { + let _ = server.terminate(); + None + } + } else { + None + }; + + // Check external health without holding the lock + let external_url = if let Some(server) = external.take() { + if server.health().await { + let url = Some(server.base_url.clone()); + // Put it back + let mut guard = state.lock().await; + guard.external_server = Some(server); + url + } else { + let _ = server.terminate(); + None + } + } else { + None + }; + + Ok([(ServerType::Internal, internal_url), (ServerType::External, external_url)] + .into_iter() + .collect()) + }
314-318: Redundant= Noneafter.take().
.take()already sets the Option toNone; the explicit assignment is unnecessary.Apply this diff:
- if let Some(server) = guard.internal_server.take() { - server.terminate().ok(); - guard.internal_server = None; - } + if let Some(server) = guard.internal_server.take() { + server.terminate().ok(); + }
329-333: Same redundancy for external server.Drop the extra assignment after
.take().Apply this diff:
- if let Some(server) = guard.external_server.take() { - server.terminate().ok(); - guard.external_server = None; - } + if let Some(server) = guard.external_server.take() { + server.terminate().ok(); + }plugins/local-stt/src/events.rs (3)
26-44: Avoid blocking the runtime withblock_in_place+block_on.Running async shutdown inside
block_in_placeon the event loop can stall other events. Prefer spawning an async task.Apply this diff:
- tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { - let mut guard = state.lock().await; - if let Some(server) = guard.internal_server.take() { - let _ = server.terminate(); - guard.internal_server = None; - } - if let Some(server) = guard.external_server.take() { - let _ = server.terminate(); - guard.external_server = None; - } - for (_, (task, token)) in guard.download_task.drain() { - token.cancel(); - task.abort(); - } - }); - }); + tauri::async_runtime::spawn({ + let state = state.clone(); + async move { + let mut guard = state.lock().await; + if let Some(server) = guard.internal_server.take() { + let _ = server.terminate(); + } + if let Some(server) = guard.external_server.take() { + let _ = server.terminate(); + } + for (_, (task, token)) in guard.download_task.drain() { + token.cancel(); + task.abort(); + } + } + });Repeat the same pattern for the Focused(true) branch (see next comment).
46-51: Use async spawn for startup too.Same rationale as shutdown; avoid blocking on focus events.
Apply this diff:
- tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { - let _ = app.start_server(None).await; - }); - }); + tauri::async_runtime::spawn({ + let app = app.clone(); + async move { + let _ = app.start_server(None).await; + } + });
11-12: Potential double-fire on CloseRequested and Destroyed.Both events may trigger on a single close. The code is idempotent (thanks to
take()), but you’ll log twice. Consider handling one (e.g.,Destroyed) or gating with a boolean in state if duplicate logs are noisy.Would you like me to add a simple “shutdown_initiated” flag to
SharedStateand guard the log/cleanup?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/desktop/src/components/settings/components/ai/stt-view-local.tsx(1 hunks)plugins/local-stt/src/events.rs(1 hunks)plugins/local-stt/src/ext.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/settings/components/ai/stt-view-local.tsxplugins/local-stt/src/events.rsplugins/local-stt/src/ext.rs
🔇 Additional comments (3)
apps/desktop/src/components/settings/components/ai/stt-view-local.tsx (1)
7-7: KeepWhisperModelimport – it’s still used
The typeWhisperModelis referenced later in this file (lines 272–273:model.key as WhisperModel), so removing the import would break the code. No changes needed here.Likely an incorrect or invalid review comment.
plugins/local-stt/src/ext.rs (1)
274-275: Good addition: explicit shutdown log.The “ext: stopping servers” trace gives much-needed visibility for shutdown paths.
plugins/local-stt/src/events.rs (1)
10-53: Window-event–driven shutdown looks good.Shifting shutdown to
WindowEventtied to the main window aligns lifecycle to user intent and removes reliance onExit*run events. The cleanup sequence is clear.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
plugins/local-stt/src/ext.rs (1)
274-275: Prefer structured log with context (server_type) for easier tracingIncluding the server_type clarifies whether you’re stopping a specific server or all, and avoids ambiguity with the similarly-worded log in events.rs.
- tracing::info!("ext: stopping servers"); + tracing::info!(?server_type, "ext: stopping server(s)");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/desktop/src/components/settings/components/ai/stt-view-local.tsx(1 hunks)plugins/local-stt/src/events.rs(1 hunks)plugins/local-stt/src/ext.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/components/settings/components/ai/stt-view-local.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/local-stt/src/events.rsplugins/local-stt/src/ext.rs
⏰ 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: build (macos, aarch64-apple-darwin, macos-latest)
- GitHub Check: build (windows, x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (1)
plugins/local-stt/src/events.rs (1)
10-21: Main-window gating and label parsing look correctParsing the window label into HyprWindow and early-returning for non-Main windows is a clean guard.
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 (1)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
79-81: Server availability check ignores new ServerHealth shape.Using
servers.external || servers.internaltreats any object as truthy, even if unhealthy. With the new enum-based health API, explicitly check for a healthy/ready state.- const servers = await localSttCommands.getServers(); - const isServerAvailable = servers.external || servers.internal; - return isDownloaded && isServerAvailable; + const servers = await localSttCommands.getServers(); + const isHealthy = (s: any) => + s?.health === "healthy" || s?.status === "healthy" || s?.ready === true; + const isServerAvailable = isHealthy(servers.external) || isHealthy(servers.internal); + return isDownloaded && Boolean(isServerAvailable);
♻️ Duplicate comments (1)
plugins/local-stt/src/ext.rs (1)
35-35: Don’t await health() while holding the state lock; take/drop/reinsert and terminate unhealthy handlesAwaiting network I/O under the mutex can block other operations and risks deadlocks/starvation. Also, per PR goals, stale/unhealthy handles should be cleared. Use take → drop lock → check → reinsert-or-terminate.
- async fn get_servers(&self) -> Result<HashMap<ServerType, ServerHealth>, crate::Error> { - let state = self.state::<crate::SharedState>(); - let guard = state.lock().await; - - let internal_url = if let Some(server) = &guard.internal_server { - let status = server.health().await; - status - } else { - ServerHealth::Unreachable - }; - - let external_url = if let Some(server) = &guard.external_server { - server.health().await - } else { - ServerHealth::Unreachable - }; - - Ok([ - (ServerType::Internal, internal_url), - (ServerType::External, external_url), - ] - .into_iter() - .collect()) - } + async fn get_servers(&self) -> Result<HashMap<ServerType, ServerHealth>, crate::Error> { + let state = self.state::<crate::SharedState>(); + // Take handles to avoid awaiting while holding the mutex. + let (internal_opt, external_opt) = { + let mut guard = state.lock().await; + (guard.internal_server.take(), guard.external_server.take()) + }; + + // Check internal + let internal_health = match internal_opt { + Some(server) => { + let h = server.health().await; + if h != ServerHealth::Unreachable { + let mut g = state.lock().await; + g.internal_server = Some(server); + } else { + let _ = server.terminate(); + } + h + } + None => ServerHealth::Unreachable, + }; + + // Check external + let external_health = match external_opt { + Some(server) => { + let h = server.health().await; + if h != ServerHealth::Unreachable { + let mut g = state.lock().await; + g.external_server = Some(server); + } else { + let _ = server.terminate(); + } + h + } + None => ServerHealth::Unreachable, + }; + + Ok([ + (ServerType::Internal, internal_health), + (ServerType::External, external_health), + ] + .into_iter() + .collect()) + }#!/bin/bash # Audit for any remaining patterns that await health() while a lock is held. rg -nP -C3 '(lock\(\)\.await)(?s:.{0,400})health\(\)\.await' --glob 'plugins/local-stt/src/**'Also applies to: 304-327
🧹 Nitpick comments (17)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (2)
160-164: Simplify disable condition; redundant conjunction.
meetingEnded && isEnhancePendingis equivalent toisEnhancePendingbecausemeetingEnded = isEnhancePending || nonEmptySession. Use the simpler predicate.- disabled: isOnboarding - ? !anySttModelExists.data || (meetingEnded && isEnhancePending) - : !modelDownloaded.data || (meetingEnded && isEnhancePending), + disabled: isOnboarding + ? !anySttModelExists.data || isEnhancePending + : !modelDownloaded.data || isEnhancePending,
73-82: Optional: make the query key model-aware to avoid stale cache sharing.If the user switches STT models while inactive, tying the key to the current model avoids cross-model cache confusion (even though you poll). For example:
- queryKey: ["check-stt-model-downloaded"], + queryKey: ["check-stt-model-downloaded", await localSttCommands.getCurrentModel()],Note: This requires lifting model retrieval so the key is stable (e.g., a separate
useQueryfor current model, then use its value in this key).crates/transcribe-whisper-local/src/service/streaming.rs (1)
134-138: Centralize the 400ms default to avoid future driftUse a shared constant instead of a magic number so future tuning is one-line. Optionally clamp user-provided values to a sane range to protect VAD behavior on extreme inputs.
Apply this diff within the function:
- .unwrap_or(Duration::from_millis(400)); + .unwrap_or(Duration::from_millis(DEFAULT_REDEMPTION_MS));Add this at module scope:
const DEFAULT_REDEMPTION_MS: u64 = 400; // Why: keep server/client defaults in lockstep; tweak in one place during tuning.Optionally (only if you want guardrails):
- .map(|ms| Duration::from_millis(ms)) + .map(|ms| Duration::from_millis(ms.clamp(50, 3_000)))plugins/listener/src/fsm.rs (1)
583-591: Replace magic numbers forredemption_time_mswith shared named constantsThe 60 ms/400 ms values are hard-coded in multiple crates (listener FSM, owhisper-client, transcribe-whisper-local). Defining shared constants (e.g. in
owhisper-interfaceor a commonconstantsmodule) and importing them everywhere will keep values in sync and clarify intent.Affected locations:
plugins/listener/src/fsm.rs(around line 588)owhisper/owhisper-client/src/lib.rs(around line 86)crates/transcribe-whisper-local/src/service/streaming.rs(around line 137)Suggested changes:
Define constants in a shared module (e.g.
owhisper-interface/src/constants.rs):// owhisper-interface/src/constants.rs pub const REDEMPTION_ONBOARDING_MS: u64 = 60; // snappier onboarding UX pub const REDEMPTION_DEFAULT_MS: u64 = 400; // balanced latency/stabilityUpdate listener FSM:
// plugins/listener/src/fsm.rs use owhisper_interface::constants::{REDEMPTION_ONBOARDING_MS, REDEMPTION_DEFAULT_MS}; Ok(owhisper_client::ListenClient::builder() .api_base(conn.base_url) .api_key(conn.api_key.unwrap_or_default()) .params(owhisper_interface::ListenParams {
redemption_time_ms: Some(if is_onboarding { 60 } else { 400 }),
redemption_time_ms: Some(if is_onboarding {REDEMPTION_ONBOARDING_MS} else {REDEMPTION_DEFAULT_MS}), ..Default::default() }) .build_dual())
- Update owhisper-client default:
// owhisper/owhisper-client/src/lib.rs use owhisper_interface::constants::REDEMPTION_DEFAULT_MS; // … .append_pair( "redemption_time_ms",
¶ms.redemption_time_ms.unwrap_or(400).to_string(),
¶ms.redemption_time_ms.unwrap_or(REDEMPTION_DEFAULT_MS).to_string(),
- Update transcribe-whisper-local fallback:
// crates/transcribe-whisper-local/src/service/streaming.rs use owhisper_interface::constants::REDEMPTION_DEFAULT_MS; // … .map(|ms| Duration::from_millis(ms))
- .unwrap_or(Duration::from_millis(400));
- .unwrap_or(Duration::from_millis(REDEMPTION_DEFAULT_MS));
This optional refactor will ensure all crates use the same tuned defaults and improve code clarity.
owhisper/owhisper-client/src/lib.rs (1)
85-87: IntroduceDEFAULT_REDEMPTION_MSin owhisper-client and audit cross-crate defaultsTo eliminate the magic
400literal inowhisper-clientand ensure a single source of truth, add:// at the top of owhisper/owhisper-client/src/lib.rs const DEFAULT_REDEMPTION_MS: u64 = 400; // default handshake timingThen update the append call:
- .append_pair( - "redemption_time_ms", - ¶ms.redemption_time_ms.unwrap_or(400).to_string(), - ); + .append_pair( + "redemption_time_ms", + ¶ms.redemption_time_ms.unwrap_or(DEFAULT_REDEMPTION_MS).to_string(), + );Additionally, we’ve observed other default values elsewhere in the workspace:
•
plugins/listener/src/fsm.rs:
redemption_time_ms: Some(if is_onboarding { 60 } else { 400 })
•crates/transcribe-whisper-local/src/service/streaming.rs:
.unwrap_or(Duration::from_millis(400))
•crates/transcribe-moonshine/src/service/streaming.rs:
params.redemption_time_ms.unwrap_or(500)
•crates/db-user/src/config_types.rs:
redemption_time_ms: Some(500)Please review whether all of these should align on a single default (e.g.
400 ms), and if so, consider movingDEFAULT_REDEMPTION_MSinto a shared location (for example, inowhisper-interface) so every crate references the same constant.crates/am/src/types.rs (2)
22-30: No TS/TSX references found; backward-compat aliasing remains recommendedThe search across all
.ts/.tsxfiles for PascalCase literals(Ready|Initializing|Uninitialized|Unloaded)returned no matches, so your front-end code is already using the new lowercase wire format. However, to guard against external clients or services that may still send PascalCase, it’s advisable to emit lowercase while accepting both formats during the transition.Suggested patch (apply to every variant in
crates/am/src/types.rs):pub enum ServerStatusType { - Ready, - Initializing, - Uninitialized, - Unloaded, + #[serde(alias = "Ready")] Ready, + #[serde(alias = "Initializing")] Initializing, + #[serde(alias = "Uninitialized")] Uninitialized, + #[serde(alias = "Unloaded")] Unloaded, }• No TS/TSX usages found for the old PascalCase values.
• Still add#[serde(alias = ...)]attributes to ensure any external callers using the old casing continue to work.
33-45: ModelState rename_all to lowercase – no direct TS/TSX usage foundOur audit of the TypeScript/TSX client code shows no literal string comparisons or references to the old-cased
ModelStatevariants:• Ran:
rg -n -C2 '\b(Unloading|Unloaded|Loading|Loaded|Prewarming|Prewarmed|Downloading|Downloaded)\b' \ --glob '*.ts' --glob '*.tsx'Result: only matches inside UI/localization text (e.g. “Loading…”, “Downloading models…”), not against
ModelStateenum values.• No
.tsor.tsxfiles perform exact matches on PascalCase ModelState names.Recommendation (optional):
• You can merge the#[serde(rename_all = "lowercase")]change as-is—TypeScript consumers aren’t breaking.
• For other clients (mobile, Python, Go, etc.) that may still emit/expect PascalCase, consider adding:pub enum ModelState { - Unloading, + #[serde(alias = "Unloading")] Unloading, … }This is a low-cost way to provide a transparent, backward-compatible rollout.
• If you maintain additional consumer code outside this repo, manually verify whether it needs adjustment or can rely on these aliases.plugins/local-stt/js/bindings.gen.ts (1)
31-33: Bindings match Rust types; consider whether Partial is still neededReturn type and
ServerHealthunion align with the Rust enum. If the backend always returns both keys, you can narrow toRecord<ServerType, ServerHealth>to simplify consumers; if not, keepingPartialis fine.Also applies to: 61-61
plugins/local-stt/src/server/external.rs (5)
12-30: Health mapping: tighten state handling and avoid unwrap-style flowCurrent mapping returns Unreachable for anything not Loading/Loaded. If hypr_am adds/renames states (e.g., Ready/Uninitialized/Error), this will silently degrade to Unreachable. Also, the unwrap after is_err is safe but a bit clunky.
Consider matching on the Result and on model_state explicitly. If hypr_am exposes additional “ready-like” states, include them; otherwise keep the two you have.
- pub async fn health(&self) -> ServerHealth { - let res = self.client.status().await; - if res.is_err() { - tracing::error!("{:?}", res); - return ServerHealth::Unreachable; - } - - let res = res.unwrap(); - - if res.model_state == hypr_am::ModelState::Loading { - return ServerHealth::Loading; - } - - if res.model_state == hypr_am::ModelState::Loaded { - return ServerHealth::Ready; - } - - ServerHealth::Unreachable - } + pub async fn health(&self) -> ServerHealth { + match self.client.status().await { + Ok(res) => match res.model_state { + hypr_am::ModelState::Loading => ServerHealth::Loading, + hypr_am::ModelState::Loaded => ServerHealth::Ready, + // If hypr_am later adds a "Ready" variant, include it: + // hypr_am::ModelState::Ready => ServerHealth::Ready, + _ => ServerHealth::Unreachable, + }, + Err(e) => { + tracing::error!("{:?}", e); + ServerHealth::Unreachable + } + } + }
32-35: Blocking sleep before kill can stall async callersterminate is sync, so std::thread::sleep blocks the calling thread (possibly a Tokio worker) for 250ms. If this is called on an async path, it can cause head-of-line blocking.
Two options:
- Keep sync API but offload the delay to a non-async mechanism (e.g., try_wait loop).
- Or add an async terminate_async alongside terminate to use tokio::time::sleep.
- std::thread::sleep(std::time::Duration::from_millis(250)); + // Poll for a short grace period without blocking a reactor for too long + let start = std::time::Instant::now(); + while start.elapsed() < std::time::Duration::from_millis(250) { + if let Ok(Some(_)) = self.child.try_wait() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(25)); + }
60-64: Preemptively killing a fixed port risks collateral damageHard-coding port 50060 and unconditionally killing the process on that port can terminate unrelated services. Prefer:
- Attempting an ephemeral/available port first, or
- Killing only known prior PIDs you spawned, or
- Making the port configurable.
- let port = 50060; + // Prefer a configurable or dynamically selected port; fall back to 50060 + let port = std::env::var("HYPR_LOCAL_STT_PORT") + .ok() + .and_then(|s| s.parse::<u16>().ok()) + .unwrap_or(50060); - - if port_killer::kill(port).is_ok() { - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - } + // Only clear the port if we know we owned the prior process (tracked elsewhere).
88-95: Stderr severity: consider warn/error for better signalStderr lines are currently logged at info, which can hide issues in noisy logs. Treat Stderr as warn to improve observability without being overly loud.
- Some(tauri_plugin_shell::process::CommandEvent::Stderr(bytes)) => { + Some(tauri_plugin_shell::process::CommandEvent::Stderr(bytes)) => { if let Ok(text) = String::from_utf8(bytes) { let text = text.trim(); if !text.is_empty() { - tracing::info!("{}", text); + tracing::warn!("{}", text); } } }
118-133: Startup probe: downgrade log level to reduce chatterThese readiness logs run every start and on transient failures. Using debug for non-ready paths keeps info logs cleaner while still traceable during diagnostics.
- Ok(_) => { - tracing::info!("Server is ready and responding"); - } + Ok(_) => { + tracing::info!("Server is ready and responding"); + } Err(e) => { - // Server may need initialization, which happens after this function returns - // Just log the status check result - tracing::info!("Server status check: {:?} (may need initialization)", e); + tracing::debug!("Server status check: {:?} (may need initialization)", e); }apps/desktop/src/components/settings/components/ai/stt-view-local.tsx (4)
59-65: Remove debug log from polling queryThis query refetches every 1s; console.log will spam DevTools and can impact performance.
queryFn: async () => { - const servers = await localSttCommands.getServers(); - console.log(servers); - return servers; + return await localSttCommands.getServers(); },
72-97: Duplicate/proliferated download checks; centralize model listDownload status here mixes Whisper and AM models, while ProModelsSection performs its own download checks. This duplicates work every 3s and widens the query surface.
Consider scoping this query to Whisper models only and letting ProModelsSection own AM checks (or vice versa) with a shared constant of SupportedSttModel keys.
- const sttModelDownloadStatus = useQuery({ + const sttModelDownloadStatus = useQuery({ queryKey: ["stt-model-download-status"], queryFn: async () => { - const models = [ - "QuantizedTiny", - "QuantizedTinyEn", - "QuantizedBase", - "QuantizedBaseEn", - "QuantizedSmall", - "QuantizedSmallEn", - "QuantizedLargeTurbo", - "am-parakeet-v2", - "am-whisper-large-v3", - ]; + // Whisper-only here; AM models are handled within ProModelsSection + const models = [ + "QuantizedTiny", + "QuantizedTinyEn", + "QuantizedBase", + "QuantizedBaseEn", + "QuantizedSmall", + "QuantizedSmallEn", + "QuantizedLargeTurbo", + ] as const; const statusChecks = await Promise.all( - models.map(model => localSttCommands.isModelDownloaded(model as SupportedSttModel)), + models.map(model => localSttCommands.isModelDownloaded(model)), ); - return models.reduce((acc, model, index) => ({ - ...acc, - [model]: statusChecks[index], - }), {} as Record<SupportedSttModel, boolean>); + return models.reduce((acc, model, index) => { + acc[model] = statusChecks[index]; + return acc; + }, {} as Record<typeof models[number], boolean>); }, refetchInterval: REFETCH_INTERVALS.downloadStatus, });
246-253: Section header UX polishMinor: consider “green for ready” to align with common status semantics. Current blue pulse for ready may read as “in-progress.”
- (status === "ready") - ? "bg-blue-300 animate-pulse" + (status === "ready") + ? "bg-green-400"
256-267: Pro models disabled state tied to license validity: align with hook contractuseLicense.getLicense returns a valid license or null. Checking getLicense.data?.valid is redundant; getLicense.data being truthy already implies validity.
- disabled={!getLicense.data?.valid} + disabled={!getLicense.data}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(1 hunks)apps/desktop/src/components/settings/components/ai/stt-view-local.tsx(8 hunks)crates/am/src/model.rs(1 hunks)crates/am/src/types.rs(2 hunks)crates/transcribe-whisper-local/src/service/streaming.rs(1 hunks)owhisper/owhisper-client/src/lib.rs(1 hunks)plugins/listener/src/fsm.rs(1 hunks)plugins/local-stt/js/bindings.gen.ts(2 hunks)plugins/local-stt/src/commands.rs(2 hunks)plugins/local-stt/src/ext.rs(3 hunks)plugins/local-stt/src/server/external.rs(4 hunks)plugins/local-stt/src/server/internal.rs(2 hunks)plugins/local-stt/src/server/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/transcribe-whisper-local/src/service/streaming.rsplugins/local-stt/src/server/mod.rsowhisper/owhisper-client/src/lib.rsplugins/listener/src/fsm.rscrates/am/src/model.rsplugins/local-stt/js/bindings.gen.tsplugins/local-stt/src/server/internal.rscrates/am/src/types.rsapps/desktop/src/components/editor-area/note-header/listen-button.tsxplugins/local-stt/src/commands.rsapps/desktop/src/components/settings/components/ai/stt-view-local.tsxplugins/local-stt/src/ext.rsplugins/local-stt/src/server/external.rs
🧬 Code Graph Analysis (8)
plugins/local-stt/src/server/mod.rs (1)
plugins/local-stt/js/bindings.gen.ts (1)
ServerHealth(61-61)
crates/am/src/model.rs (1)
plugins/local-stt/js/bindings.gen.ts (1)
AmModel(58-58)
plugins/local-stt/src/server/internal.rs (2)
plugins/local-stt/js/bindings.gen.ts (1)
ServerHealth(61-61)plugins/local-stt/src/server/external.rs (1)
health(12-30)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
packages/utils/src/contexts/ongoing-session.tsx (1)
useOngoingSession(32-46)
plugins/local-stt/src/commands.rs (1)
plugins/local-stt/js/bindings.gen.ts (2)
ServerHealth(61-61)ServerType(62-62)
apps/desktop/src/components/settings/components/ai/stt-view-local.tsx (3)
apps/desktop/src/components/settings/components/ai/shared.tsx (2)
SharedSTTProps(91-98)STTModel(55-61)plugins/local-stt/js/bindings.gen.ts (3)
ServerHealth(61-61)SupportedSttModel(64-64)WhisperModel(66-66)apps/desktop/src/hooks/use-license.ts (1)
useLicense(9-105)
plugins/local-stt/src/ext.rs (2)
plugins/local-stt/js/bindings.gen.ts (2)
ServerHealth(61-61)ServerType(62-62)plugins/local-stt/src/commands.rs (1)
get_servers(102-106)
plugins/local-stt/src/server/external.rs (2)
plugins/local-stt/js/bindings.gen.ts (1)
ServerHealth(61-61)plugins/local-stt/src/server/internal.rs (3)
health(57-65)health(121-123)terminate(67-70)
⏰ 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-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (13)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
62-71: Centralized ongoing-session selectors look good.Consolidating status/id/actions at the top removes duplication and keeps the component’s source of truth tight. Using selector objects with shallow comparison is appropriate here.
crates/transcribe-whisper-local/src/service/streaming.rs (1)
134-138: Default redemption_time_ms lowered to 400ms — consistent with the rest of the stackChange aligns server default with client (owhisper-client) and listener FSM. Good move to reduce E2E latency.
plugins/listener/src/fsm.rs (1)
583-591: Onboarding=60ms and default=400ms — nice latency winSwitch from 70/500 to 60/400 matches the server/client defaults and should tighten responsiveness during onboarding.
owhisper/owhisper-client/src/lib.rs (1)
85-87: Client default redemption_time_ms -> 400ms: aligns with server/listenerConsistent defaults across the stack remove surprising behavior when params are omitted.
crates/am/src/types.rs (1)
22-30: Added Eq/PartialEq and lowercase serialization for ServerStatusType — OKDeriving Eq/PartialEq is helpful for comparisons. Lowercase serde aligns with newer enums introduced elsewhere (e.g., ServerHealth).
crates/am/src/model.rs (1)
86-91: Verify WhisperLargeV3 CRC32 checksum against the published tarI wasn’t able to compute the CRC32 locally (the tar wasn’t present), so please download the current
openai_whisper-large-v3-v20240930_626MB.tarfrom your S3 bucket and confirm that its CRC32 matches1964673816. A stale checksum here will cause the installer to delete a valid download, breaking installs.• Location: crates/am/src/model.rs lines 86–91
• Expected checksum: 1964673816
• Action: computecrc32of the tar (e.g. via Python’szlib.crc32orcksum -o3) and ensure it matches.plugins/local-stt/src/server/mod.rs (1)
14-22: ServerHealth enum shape and serialization look correctDerives and
#[serde(rename_all = "lowercase")]align with the generated TS union and specta export. No issues.plugins/local-stt/src/server/internal.rs (1)
9-9: Import of ServerHealth is appropriateMatches the new return type usage in this module.
plugins/local-stt/src/commands.rs (1)
5-7: Verify ServerHealth shape in TS/desktop callsitesPlease double-check that all TypeScript and desktop consumers correctly handle the new return type
Partial<{ [key in ServerType]: ServerHealth }>, and no longer assume an array of servers or URL strings:• apps/desktop/src/components/settings/components/ai/stt-view-local.tsx
– Inspect how theserversobject is used afterawait localSttCommands.getServers()
– Ensure you’re not iterating overserversas if it were an array or accessing properties like.url
– Confirm the UI logic instead readsservers.internal/servers.external(or handles missing keys)• apps/desktop/src/components/editor-area/note-header/listen-button.tsx
– You’re already checkingservers.external || servers.internalfor availability; verify this still aligns with the new numeric/metric values inServerHealthNo URL-based patterns (e.g.
server.url,includes(…),startsWith(…)) were found in other callsites, but please audit any remaining legacy usage across the desktop and web layers.plugins/local-stt/src/ext.rs (1)
14-15: Import of ServerHealth in ext aligns with the new contractNo issues; keeps the trait/impl in sync with server module types.
apps/desktop/src/components/settings/components/ai/stt-view-local.tsx (3)
122-127: Model filtering logic looks goodShows default Small by default and reveals other Whisper variants only when downloaded. Keeps UI uncluttered.
386-414: Good event handling: stopPropagation on button clicksPrevents accidental card selection when clicking “Show in Finder” or during download actions.
144-157: The imports forarchandplatformin this component are synchronous (they come from@tauri-apps/plugin-os), so using them insideuseMemowith an empty dependency array is safe and will not “always evaluate to false.” No changes needed here—please disregard the async-lifting suggestion.
1 participant
Summary by cubic
Enables selecting Pro STT models in settings and makes the local STT server lifecycle more reliable by starting on focus, stopping on main window close, and cleaning up unhealthy servers.
New Features
Bug Fixes