Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe listener plugin is refactored to support both single-channel (microphone-only) and dual-channel (microphone and speaker) audio modes. The unified Changes
Sequence DiagramsequenceDiagram
participant Pipeline
participant Listener as ListenerActor
participant ChannelSender
participant Recorder
rect rgb(200, 220, 240)
Note over Pipeline,ChannelSender: Single Mode (Mic Only)
Pipeline->>Recorder: Audio (mic data)
Pipeline->>Listener: AudioSingle(mic_bytes)
Listener->>ChannelSender: Route via Single variant
ChannelSender->>Listener: Send MixedMessage<Bytes>
end
rect rgb(240, 200, 220)
Note over Pipeline,ChannelSender: Dual Mode (Mic + Speaker)
Pipeline->>Recorder: Audio (mixed mic/spk data)
Pipeline->>Listener: AudioDual(mic_bytes, spk_bytes)
Listener->>ChannelSender: Route via Dual variant
ChannelSender->>Listener: Send MixedMessage<(Bytes, Bytes)>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 (2)
plugins/listener/src/actors/source.rs (1)
375-405: Single/Dual dispatch semantics are correct; consider de-duping mic cloning in Single modeThe new behavior looks right:
- Recorder: mic-only in
ChannelMode::Single, mixed mic+spk inChannelMode::Dual.- Listener:
AudioSingle(mic only) for single-channel,AudioDual(separate mic/spk) for dual-channel.One small optimization in the Single branch: you currently do
mic.to_vec()twice (once indirectly viaaudio_for_recordingand once again when building the Bytes). You can allocate/copy once and reuse:- if let Some(cell) = registry::where_is(RecorderActor::name()) { - let actor: ActorRef<RecMsg> = cell.into(); - let audio_for_recording = if mode == ChannelMode::Single { - mic.to_vec() - } else { - Self::mix(mic.as_ref(), spk.as_ref()) - }; + if let Some(cell) = registry::where_is(RecorderActor::name()) { + let actor: ActorRef<RecMsg> = cell.into(); + let audio_for_recording = if mode == ChannelMode::Single { + // Single mode: record mic only, and reuse this Vec later when encoding. + mic.to_vec() + } else { + Self::mix(mic.as_ref(), spk.as_ref()) + }; if let Err(e) = actor.cast(RecMsg::Audio(audio_for_recording)) { tracing::error!(error = ?e, "failed_to_send_audio_to_recorder"); } } @@ - let result = if mode == ChannelMode::Single { - let audio_bytes = f32_to_i16_bytes(mic.to_vec().iter().copied()); - actor.cast(ListenerMsg::AudioSingle(audio_bytes)) + let result = if mode == ChannelMode::Single { + // If you keep `audio_for_recording` as the mic Vec in the branch above, + // you can pass an iterator over it here instead of creating a second Vec. + let audio_bytes = f32_to_i16_bytes(mic.iter().copied()); + actor.cast(ListenerMsg::AudioSingle(audio_bytes))Or equivalently, bind
let mic_vec = mic.to_vec();once in the Single arm, passmic_vecto the recorder, and then usemic_vec.iter().copied()for byte conversion.This keeps the new logic but avoids an extra allocation and copy on the hot path.
plugins/listener/src/actors/listener.rs (1)
18-27: Mode‑aware ListenerMsg + ChannelSender wiring looks correct; consider logging on mismatched modeThe restructuring around
ListenerMsg::{AudioSingle, AudioDual}andChannelSender::{Single, Dual}looks solid:
- In Single mode you only ever push
MixedMessage<Bytes, _>into a single‑channel stream.- In Dual mode you only ever push
MixedMessage<(Bytes, Bytes), _>into a dual‑channel stream.- The
if let ChannelSender::Single/Dual(..)guards ensure you can’t accidentally send the wrong payload shape into a given channel.One minor improvement: if
AudioSingleis received whilestate.txisChannelSender::Dual(or vice versa), the message is just dropped. That’s probably “impossible” under normal flow, but adding atracing::warn!in the non‑matching case would make any future wiring/Mode‑Change bugs much easier to diagnose without changing runtime behavior for the happy path.Also applies to: 44-54, 106-117
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/listener/src/actors/listener.rs(5 hunks)plugins/listener/src/actors/mod.rs(1 hunks)plugins/listener/src/actors/source.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/listener/src/actors/source.rs (1)
crates/audio-utils/src/lib.rs (1)
f32_to_i16_bytes(66-76)
plugins/listener/src/actors/listener.rs (3)
owhisper/owhisper-interface/src/lib.rs (2)
default(130-132)default(152-161)owhisper/owhisper-interface/src/stream.rs (2)
default(66-76)default(91-102)owhisper/owhisper-client/src/live.rs (1)
builder(106-108)
⏰ 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: fmt
🔇 Additional comments (2)
plugins/listener/src/actors/mod.rs (1)
13-16: SAMPLE_RATE alignment on macOS looks goodSwitching macOS
SAMPLE_RATEto 16 kHz to match the non-macOS path and the listen-side configuration is consistent with the rest of this PR and with typical STT expectations. I don’t see any functional issues with this change; just be aware this slightly changes the effective frontend behavior for existing macOS users.plugins/listener/src/actors/listener.rs (1)
195-206: Review comment verification complete: concern about ListenParams.channels is invalidThe review identified a potential issue with
ListenParams.channelsnot being set to 2 in dual-channel mode. However, verification shows this concern is unfounded.Key finding: The
build_dual()method callsself.build_request(2)with a hardcoded channel count of 2, and thebuild_request()function accepts achannelsparameter and passes it tobuild_uri(channels)to construct the request URI. This meansbuild_dual()internally controls the channel count via the client builder—it does not depend on or use thechannelsfield inListenParams.The code is correct as-is. The
build_listen_params()function can safely leavechannelsat its default value (1) becausebuild_dual()overrides this internally when constructing the dual-channel client.
1 participant
No description provided.