Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR extracts audio mixing logic into reusable utilities within the Changes
Sequence DiagramsequenceDiagram
participant Source as Source<br/>(source.rs)
participant Recorder as Recorder<br/>(recorder.rs)
participant Writers as WAV Writers<br/>(main/mic/spk)
Source->>Source: Capture audio<br/>(mic, speaker)
alt Single Audio Path
Source->>Recorder: RecMsg::AudioSingle(Arc<[f32]>)
Recorder->>Writers: Write to main writer
else Dual Audio Path
Source->>Recorder: RecMsg::AudioDual(Arc<[f32]>, Arc<[f32]>)
rect rgb(200, 220, 240)
Note over Recorder,Writers: Main writer path
Recorder->>Recorder: Mix mic + speaker
Recorder->>Writers: Write mixed to main writer
end
rect rgb(220, 240, 200)
Note over Recorder,Writers: Debug mode separate paths
alt Debug mode enabled
Recorder->>Writers: Write mic to writer_mic
Recorder->>Writers: Write speaker to writer_spk
end
end
end
Recorder->>Recorder: Flush if due
Recorder->>Writers: Periodic flush
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 |
![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
🧹 Nitpick comments (1)
plugins/listener/src/actors/recorder.rs (1)
123-146: Consider simplifying the per-channel writer access pattern.The outer check
if st.writer_mic.is_some()on Line 131 is redundant since the innerif let Some(...)patterns already handle the None case. Since both per-channel writers are created together (Lines 77-96), you can simplify this:- if st.writer_mic.is_some() { - if let Some(ref mut writer_mic) = st.writer_mic { - for s in mic.iter() { - writer_mic.write_sample(*s)?; - } - } - - if let Some(ref mut writer_spk) = st.writer_spk { - for s in spk.iter() { - writer_spk.write_sample(*s)?; - } + if let Some(ref mut writer_mic) = st.writer_mic { + for s in mic.iter() { + writer_mic.write_sample(*s)?; + } + } + + if let Some(ref mut writer_spk) = st.writer_spk { + for s in spk.iter() { + writer_spk.write_sample(*s)?; } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/audio-utils/src/lib.rs(1 hunks)crates/transcribe-aws/Cargo.toml(1 hunks)crates/transcribe-aws/src/lib.rs(2 hunks)crates/transcribe-deepgram/Cargo.toml(1 hunks)crates/transcribe-deepgram/src/service.rs(2 hunks)crates/ws-utils/src/lib.rs(2 hunks)plugins/listener/src/actors/recorder.rs(6 hunks)plugins/listener/src/actors/source.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
crates/transcribe-aws/src/lib.rs (1)
crates/audio-utils/src/lib.rs (1)
mix_audio_pcm16le(102-127)
crates/transcribe-deepgram/src/service.rs (1)
crates/audio-utils/src/lib.rs (1)
mix_audio_pcm16le(102-127)
crates/audio-utils/src/lib.rs (1)
crates/aec/benches/aec_bench.rs (1)
mic_sample(19-20)
crates/ws-utils/src/lib.rs (1)
crates/audio-utils/src/lib.rs (2)
bytes_to_f32_samples(78-85)mix_audio_f32(91-100)
plugins/listener/src/actors/recorder.rs (2)
crates/audio-utils/src/lib.rs (2)
std(37-37)mix_audio_f32(91-100)crates/audio-utils/src/vorbis.rs (2)
decode_vorbis_to_wav_file(111-150)encode_wav_to_vorbis_file(152-175)
⏰ 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). (5)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (15)
crates/transcribe-aws/Cargo.toml (1)
10-10: LGTM!The workspace dependency addition aligns with the PR objective to use shared audio mixing utilities.
crates/transcribe-deepgram/Cargo.toml (1)
7-7: LGTM!The dependency is declared in both
[dev-dependencies]and[dependencies]sections. Ifhypr-audio-utilsis only used in production code (as suggested by the service.rs changes), the dev-dependencies entry may be redundant.Also applies to: 15-15
crates/transcribe-aws/src/lib.rs (1)
31-31: LGTM!The refactor correctly replaces the local mixing implementation with the shared
mix_audio_pcm16leutility fromhypr-audio-utils, improving code maintainability and reducing duplication.Also applies to: 92-92
crates/transcribe-deepgram/src/service.rs (1)
11-11: LGTM!Consistent with the refactor in other modules, this correctly adopts the shared
mix_audio_pcm16leutility for dual-audio mixing.Also applies to: 75-75
crates/ws-utils/src/lib.rs (1)
11-11: LGTM!The refactor correctly replaces the local
mix_audio_channelsfunction with the sharedmix_audio_f32utility, which is appropriate since the data is already in f32 format at this point in the pipeline.Also applies to: 119-119
plugins/listener/src/actors/source.rs (1)
378-386: LGTM!The refactor to Arc-based audio payloads is efficient and correct. Using
Arc::cloneavoids expensive buffer copies while sharing data between actors. The error handling is appropriate.crates/audio-utils/src/lib.rs (3)
87-89: LGTM!The
mix_sample_f32function correctly clamps the mixed sample to the valid audio range [-1.0, 1.0] to prevent distortion.
91-100: LGTM!The
mix_audio_f32function correctly handles slices of different lengths by zero-padding shorter inputs and delegates mixing logic tomix_sample_f32.
102-127: Callers do not validate input buffer lengths; odd-length buffers from clients will silently lose data.Verification reveals that both callers (
crates/transcribe-aws/src/lib.rs:92andcrates/transcribe-deepgram/src/service.rs:75) receivemicandspeakerbuffers from untrusted JSON input without any length validation. If a client sends odd-length buffers in theDualAudiomessage,mix_audio_pcm16lewill silently discard the final incomplete sample. Consider adding buffer length validation at the deserialization layer or documenting this behavior in the function's contract.plugins/listener/src/actors/recorder.rs (6)
4-4: LGTM!The switch to
Arc<[f32]>for audio payloads is efficient and avoids unnecessary copying of large buffers between actors.Also applies to: 15-16
26-27: LGTM!The per-channel writer initialization correctly creates separate WAV files for mic and speaker in debug mode. The files are created in the session-specific directory with appropriate naming conventions.
Also applies to: 77-96
115-122: LGTM!The
AudioSinglehandling correctly writes samples directly to the main writer and triggers periodic flushing.
157-159: LGTM!The cleanup logic correctly finalizes all writers (main, mic, and speaker) using the
finalize_writerhelper, ensuring all data is properly flushed and written to disk.Also applies to: 219-227
191-196: LGTM!The
is_debug_modefunction correctly checks both compile-time debug assertions and theHYPRNOTE_DEBUGenvironment variable for enabling debug features.
198-217: LGTM!The flushing helpers (
flush_if_dueandflush_all) correctly implement periodic flushing with a 1-second interval, ensuring data durability without excessive I/O overhead. All three writers are properly coordinated.
1 participant
No description provided.