Conversation
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughRefactors STT adapter surface by separating realtime and batch concerns: renames Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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
🧹 Nitpick comments (4)
owhisper/owhisper-client/src/adapter/soniox/mod.rs (1)
10-18: Consider returningResultinstead of panicking on invalid URL.Line 16 uses
.expect("invalid_api_base")which will panic if the URL fails to parse. While empty strings are handled gracefully, a malformed non-empty URL will cause a runtime panic. Consider propagating the error or using a fallback.- pub(crate) fn api_host(api_base: &str) -> String { + pub(crate) fn api_host(api_base: &str) -> Result<String, url::ParseError> { if api_base.is_empty() { - return DEFAULT_API_HOST.to_string(); + return Ok(DEFAULT_API_HOST.to_string()); } - let url: url::Url = api_base.parse().expect("invalid_api_base"); - url.host_str().unwrap_or(DEFAULT_API_HOST).to_string() + let url: url::Url = api_base.parse()?; + Ok(url.host_str().unwrap_or(DEFAULT_API_HOST).to_string()) }This would require updating callers to handle the
Result, but provides better error handling than panicking.owhisper/owhisper-client/src/adapter/argmax/live.rs (1)
45-75: Test relies on a local server and may be skipped in CI.The test connects to
ws://localhost:50060/v1which requires a local Argmax server to be running. Consider adding#[ignore]attribute or gating behind a feature flag to prevent CI failures when the server is unavailable.#[tokio::test] + #[ignore] // Requires local Argmax server async fn test_client() {owhisper/owhisper-client/src/adapter/deepgram/mod.rs (1)
14-27: Consider returningResultinstead of panicking on invalid URL.The
expect("invalid_api_base")on line 16 will panic ifapi_baseis not a valid URL. While this is a configuration error, panicking may be undesirable in library code. Consider returning aResultor validating earlier in the builder.If changing the return type is not feasible now, this is acceptable for internal use where the caller controls the input.
owhisper/owhisper-client/src/lib.rs (1)
119-125: Consider consistent validation forapi_key.
get_api_base()(line 60) panics if missing, butapi_keysilently defaults to an empty string here. For batch API calls, an empty API key will likely produce a cryptic authentication error from the remote service.Consider adding a
get_api_key()helper that panics (likeget_api_base) or returnsResult:+ fn get_api_key(&self) -> &str { + self.api_key.as_ref().expect("api_key is required") + } +} + impl<A: RealtimeSttAdapter + BatchSttAdapter> ListenClientBuilder<A> { pub fn build_batch(self) -> BatchClient<A> { let params = self.get_params(); let api_base = self.get_api_base().to_string(); - BatchClient::new(api_base, self.api_key.unwrap_or_default(), params) + BatchClient::new(api_base, self.get_api_key().to_string(), params) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
owhisper/owhisper-client/src/adapter/argmax/batch.rs(1 hunks)owhisper/owhisper-client/src/adapter/argmax/live.rs(1 hunks)owhisper/owhisper-client/src/adapter/argmax/mod.rs(1 hunks)owhisper/owhisper-client/src/adapter/deepgram.rs(0 hunks)owhisper/owhisper-client/src/adapter/deepgram/batch.rs(1 hunks)owhisper/owhisper-client/src/adapter/deepgram/live.rs(1 hunks)owhisper/owhisper-client/src/adapter/deepgram/mod.rs(1 hunks)owhisper/owhisper-client/src/adapter/mod.rs(2 hunks)owhisper/owhisper-client/src/adapter/soniox/batch.rs(6 hunks)owhisper/owhisper-client/src/adapter/soniox/live.rs(1 hunks)owhisper/owhisper-client/src/adapter/soniox/mod.rs(1 hunks)owhisper/owhisper-client/src/batch.rs(1 hunks)owhisper/owhisper-client/src/lib.rs(4 hunks)owhisper/owhisper-client/src/live.rs(5 hunks)plugins/listener/src/actors/listener.rs(3 hunks)
💤 Files with no reviewable changes (1)
- owhisper/owhisper-client/src/adapter/deepgram.rs
🧰 Additional context used
🧬 Code graph analysis (12)
owhisper/owhisper-client/src/adapter/argmax/batch.rs (6)
owhisper/owhisper-client/src/lib.rs (4)
adapter(50-57)api_base(35-38)api_key(40-43)params(45-48)owhisper/owhisper-client/src/adapter/soniox/batch.rs (1)
transcribe_file(329-341)owhisper/owhisper-client/src/adapter/deepgram/batch.rs (1)
transcribe_file(12-24)owhisper/owhisper-client/src/adapter/mod.rs (1)
transcribe_file(47-54)owhisper/owhisper-client/src/batch.rs (1)
transcribe_file(31-45)owhisper/owhisper-client/src/adapter/argmax.rs (1)
transcribe_file(39-49)
owhisper/owhisper-client/src/adapter/soniox/mod.rs (1)
owhisper/owhisper-client/src/lib.rs (1)
api_base(35-38)
owhisper/owhisper-client/src/batch.rs (1)
owhisper/owhisper-client/src/lib.rs (1)
adapter(50-57)
owhisper/owhisper-client/src/adapter/deepgram/batch.rs (1)
crates/audio-utils/src/lib.rs (3)
f32_to_i16_bytes(66-76)resample_audio(171-220)source_from_path(129-135)
owhisper/owhisper-client/src/live.rs (2)
owhisper/owhisper-client/src/adapter/argmax/live.rs (1)
ListenClientBuilder(58-66)owhisper/owhisper-client/src/lib.rs (1)
adapter(50-57)
plugins/listener/src/actors/listener.rs (1)
plugins/listener2/src/ext.rs (1)
owhisper_client(156-157)
owhisper/owhisper-client/src/adapter/deepgram/mod.rs (2)
owhisper/owhisper-client/src/lib.rs (2)
api_base(35-38)params(45-48)crates/whisper-local/src/model/mod.rs (1)
language(26-28)
owhisper/owhisper-client/src/adapter/soniox/batch.rs (2)
owhisper/owhisper-client/src/lib.rs (3)
adapter(50-57)api_base(35-38)params(45-48)owhisper/owhisper-client/src/adapter/soniox/mod.rs (1)
api_host(11-18)
owhisper/owhisper-client/src/adapter/argmax/live.rs (1)
owhisper/owhisper-client/src/lib.rs (1)
adapter(50-57)
owhisper/owhisper-client/src/adapter/deepgram/live.rs (2)
owhisper/owhisper-client/src/adapter/deepgram/mod.rs (3)
append_keyword_query(47-66)append_language_query(68-100)listen_endpoint_url(15-26)owhisper/owhisper-client/src/adapter/mod.rs (6)
supports_native_multichannel(24-24)build_ws_url(26-26)build_auth_header(28-28)keep_alive_message(30-30)finalize_message(32-32)parse_response(43-43)
owhisper/owhisper-client/src/adapter/soniox/live.rs (2)
owhisper/owhisper-client/src/adapter/mod.rs (7)
supports_native_multichannel(24-24)build_ws_url(26-26)build_auth_header(28-28)keep_alive_message(30-30)initial_message(34-41)parse_response(43-43)finalize_message(32-32)owhisper/owhisper-client/src/adapter/soniox/mod.rs (1)
ws_host(20-28)
owhisper/owhisper-client/src/lib.rs (2)
owhisper/owhisper-client/src/adapter/argmax/live.rs (1)
ListenClientBuilder(58-66)owhisper/owhisper-client/src/batch.rs (1)
new(21-29)
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
🔇 Additional comments (39)
plugins/listener/src/actors/listener.rs (3)
7-9: Imports correctly aligned with new realtime STT adapter APIThe import set (including
RealtimeSttAdapterand the concrete adapters) matches the refactor away fromSttAdapterand is consistent with how the adapters are used below.Please ensure
owhisper-clientis updated in lockfile/Cargo.toml so thatRealtimeSttAdapterand these adapters are available in the referenced version.
270-327: Generic bound change toRealtimeSttAdapterfor single‑channel path looks correctSwitching the generic from the old adapter trait to
RealtimeSttAdapterkeeps the function’s behavior intact while matching the newListenClient::builder().adapter::<A>()API; the rest of the connect/timeout/error‑handling logic is unchanged.After updating the dependency, run
cargo checkto confirm all realtime adapters (Argmax/Deepgram/Soniox) implementRealtimeSttAdapterand this generic still infers cleanly from call sites.
329-386: Dual‑channel helper correctly updated to useRealtimeSttAdapterThe dual‑channel spawn helper now also constrains
AbyRealtimeSttAdapter, matching the single‑channel path and thebuilder().adapter::<A>().build_dual()usage; no functional changes to connection or stream processing.Same as above, confirm via
cargo check/tests that all dual‑capable adapters implementRealtimeSttAdapterand thatbuild_dual()remains compatible with this trait.owhisper/owhisper-client/src/adapter/argmax/batch.rs (1)
1-20: LGTM!The
BatchSttAdapterimplementation forArgmaxAdapterfollows the delegation pattern consistently with other adapters in the codebase. The method signature matches the trait definition, and forwarding toself.inner.transcribe_fileis appropriate for a wrapper adapter.owhisper/owhisper-client/src/batch.rs (2)
7-20: LGTM!The trait bound update from
SttAdaptertoBatchSttAdapteris consistent with the refactoring goals. The generic parameter correctly constrains the adapter type for batch operations.
31-45: TheBatchSttAdaptertrait includesDefaultas a supertrait (pub trait BatchSttAdapter: Clone + Default + Send + Sync + 'static). The code at line 35 correctly callsA::default()and will compile without issues.owhisper/owhisper-client/src/adapter/soniox/mod.rs (1)
20-28: LGTM!The
ws_hostderivation logic correctly transformsapi.{domain}tostt-rt.{domain}with appropriate fallback toDEFAULT_WS_HOSTwhen the pattern doesn't match.owhisper/owhisper-client/src/adapter/soniox/batch.rs (6)
11-12: LGTM!The import changes correctly reference the new module structure with
SonioxAdapterfrom the parent module andBatchSttAdapterfrom the crate-level adapter module.
42-42: URL construction updated consistently.The URL format now uses
Self::api_host(api_base)which aligns with the new helper method pattern. Note that if theapi_hostmethod is refactored to returnResult(as suggested inmod.rs), this and other URL constructions will need error propagation.
100-109: LGTM!The inline derivation of
language_hintsfromparams.languagesis clean and straightforward. The ISO639 code extraction correctly maps the language objects to their string representations.
151-154: LGTM!URL construction pattern is consistent with other methods in this file.
251-254: LGTM!URL construction pattern is consistent.
328-341: LGTM!The
BatchSttAdapterimplementation correctly converts the generic path toPathBufto satisfy lifetime requirements, and usesBox::pinwith an async block to create theBatchFuture. This pattern is consistent with other adapter implementations likeDeepgramAdapter.owhisper/owhisper-client/src/live.rs (5)
12-12: LGTM!Import correctly updated to use
RealtimeSttAdapterfrom the crate root.
17-29: LGTM!Both
ListenClientandListenClientDualstruct definitions correctly updated to useRealtimeSttAdaptertrait bound, maintaining consistency with the refactored adapter architecture.
184-184: LGTM!The
implblock bound correctly updated toRealtimeSttAdapter.
222-222: LGTM!The
ListenClientDualimpl block bound correctly updated toRealtimeSttAdapter.
369-387: LGTM!Both helper functions
websocket_client_with_keep_aliveandextract_finalize_textcorrectly updated to useRealtimeSttAdapterbound. The logic remains unchanged, only the trait constraint is updated to match the renamed trait.owhisper/owhisper-client/src/adapter/argmax/live.rs (1)
5-33: LGTM!The
RealtimeSttAdapterimplementation cleanly delegates to the innerDeepgramAdapterwhile applying argmax-specific parameter adaptation viaSelf::adapt_params. The delegation pattern is appropriate for this wrapper adapter.owhisper/owhisper-client/src/adapter/argmax/mod.rs (3)
8-11: LGTM!The language constant is well-organized and covers the documented Parakeet v3 supported languages.
13-16: LGTM!Clean struct definition with appropriate derives. The inner
DeepgramAdaptercomposition enables delegation while allowing Argmax-specific behavior.
18-43: LGTM!The
adapt_paramslogic correctly handles model-specific language selection:
- Parakeet v2: English-only
- Parakeet v3: First supported language or English fallback
- Other models: First language or English fallback
The function always returns a single-language vector, which aligns with the adapter's purpose.
owhisper/owhisper-client/src/adapter/soniox/live.rs (4)
11-20: LGTM!The WebSocket URL construction correctly uses the
ws_hosthelper and builds a valid wss URL for the Soniox transcription endpoint.
30-74: LGTM!The
initial_messageimplementation properly constructs the Soniox configuration with appropriate defaults and optional fields. The early return with a warning for missing API keys is a reasonable approach.
76-101: Good error handling and token filtering.The response parsing correctly handles error messages, filters out control tokens (
<fin>,<end>), and returnsNonefor empty non-final responses. The logging for parse failures and errors is helpful for debugging.
204-221: LGTM!The
SpeakerIdenum with#[serde(untagged)]elegantly handles both numeric and string speaker IDs from the Soniox API. Theas_i32helper correctly extracts numeric values from either variant.owhisper/owhisper-client/src/adapter/deepgram/live.rs (2)
8-55: LGTM!The WebSocket URL construction is comprehensive, properly handling:
- Language and keyword query parameters via helper functions
- Multiple transcription options (diarization, punctuation, smart format, etc.)
- Scheme selection (ws vs wss) based on host for local development
57-80: LGTM!The auth header, keep-alive, finalize, and parse methods are correctly implemented. Using
owhisper_interface::ControlMessagefor serialization ensures consistency with the protocol.owhisper/owhisper-client/src/adapter/deepgram/mod.rs (4)
8-12: LGTM!The language constants and adapter struct are cleanly defined. The multi-language arrays match Deepgram's documented capabilities for Nova-2 and Nova-3 models.
29-45: LGTM!The
can_use_multilogic correctly validates that:
- At least 2 languages are provided
- The model supports multi-language (nova-2 or nova-3)
- All requested languages are in the model's supported set
47-66: LGTM!The keyword query construction correctly distinguishes between
keyterm(for nova-3/parakeet) andkeywords(for other models) based on Deepgram's API requirements.
68-100: LGTM!The language query construction handles all cases appropriately:
- 0 languages: enable auto-detection
- 1 language: set explicit language
- Multiple languages: use multi-language mode if supported, otherwise auto-detection with hints
owhisper/owhisper-client/src/adapter/deepgram/batch.rs (4)
1-9: LGTM!Imports are well-organized and appropriately scoped for the batch transcription functionality.
11-25: LGTM!Clean trait implementation that delegates to the internal async method while properly converting the file path to owned
PathBuf.
83-100: LGTM!HTTP request construction with proper auth header, content-type negotiation, and error handling is well-implemented.
104-141: LGTM!The audio decoding logic correctly handles:
- Blocking I/O via
spawn_blocking- Multi-channel to mono conversion with proper averaging
- Empty samples validation
- Error propagation with descriptive messages
Note:
resample_audio(decoder, sample_rate)with matching input/output rates is a no-op per the implementation, which is fine for format conversion purposes.owhisper/owhisper-client/src/adapter/mod.rs (1)
21-55: Clean trait separation.Good refactoring that clearly separates realtime streaming (
RealtimeSttAdapter) from batch transcription (BatchSttAdapter) capabilities. The consistent trait bounds (Clone + Default + Send + Sync + 'static) and theBatchFuturetype alias make the API ergonomic.owhisper/owhisper-client/src/lib.rs (2)
8-10: LGTM!Clean public API surface exposing both adapter traits alongside the concrete adapter implementations.
16-16: LGTM!Trait bound updates from
SttAdaptertoRealtimeSttAdapterare consistent across the builder struct and impl blocks.Also applies to: 34-34, 50-50
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
owhisper/owhisper-client/src/adapter/deepgram/live.rs (1)
90-91: Add#[ignore]attribute for CI compatibility.The test requires
DEEPGRAM_API_KEYenvironment variable and external API access. It will panic in CI environments where the key isn't set. This was suggested in a previous review.#[tokio::test] + #[ignore] // Requires DEEPGRAM_API_KEY environment variable async fn test_client() {
🧹 Nitpick comments (2)
owhisper/owhisper-client/src/adapter/deepgram/live.rs (2)
27-37: Consider making configurable query parameters part ofListenParams.Several query parameters are hardcoded (e.g.,
filler_words,interim_results,diarize,punctuate). If these need to be customizable per-client, consider adding them toListenParams. This is fine as-is if these defaults are intentional for all use cases.
130-132: Consider explicit handling or logging for non-transcript responses.Empty match arms silently discard errors and other response types. If debugging is needed later, consider at minimum a debug log statement.
- _ => {} + other => { + tracing::debug!("Received non-transcript response: {:?}", other); + } }, - _ => {} + Err(e) => { + tracing::warn!("Stream error: {:?}", e); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
owhisper/owhisper-client/src/adapter/deepgram/live.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
owhisper/owhisper-client/src/adapter/deepgram/live.rs (2)
owhisper/owhisper-client/src/adapter/deepgram/mod.rs (1)
listen_endpoint_url(15-26)owhisper/owhisper-client/src/adapter/mod.rs (6)
supports_native_multichannel(24-24)build_ws_url(26-26)build_auth_header(28-28)keep_alive_message(30-30)finalize_message(32-32)parse_response(43-43)
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: fmt
- GitHub Check: Pages changed - hyprnote
🔇 Additional comments (3)
owhisper/owhisper-client/src/adapter/deepgram/live.rs (3)
57-59: LGTM!The Deepgram authentication header format is correct.
61-75: LGTM!The keep-alive and finalize message implementations correctly serialize control messages. The
unwrap()calls are safe since these are simple, infallible enum serializations.
77-79: LGTM!Using
.ok()appropriately handles non-transcript WebSocket messages that won't parse asStreamResponse.
1 participant
No description provided.