UI for Mic Selector #1160
UI for Mic Selector #1160
Conversation
📝 WalkthroughWalkthroughThe changes refactor the audio control UI in the listen button component by introducing separate Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ListenButton
participant MicrophoneSelector
participant SpeakerButton
participant DeviceQuery
User->>ListenButton: Interacts with audio controls
ListenButton->>MicrophoneSelector: Render with mute toggle and device selection
MicrophoneSelector->>DeviceQuery: Fetch available microphones
DeviceQuery-->>MicrophoneSelector: Return device list
User->>MicrophoneSelector: Selects device or toggles mute
MicrophoneSelector-->>ListenButton: Notify of mute/device change
ListenButton->>SpeakerButton: Render with mute toggle
User->>SpeakerButton: Toggles speaker mute
SpeakerButton-->>ListenButton: Notify of speaker mute state
Possibly related PRs
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/desktop/src/locales/en/messages.po (1)
583-583: Fix the merge conflict marker.There's an unresolved git merge conflict marker in the translation string. This will cause issues in the application.
Apply this fix:
-msgstr "Enable Integration<<<<<<< HEAD" +msgstr "Enable Integration"
🧹 Nitpick comments (1)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
335-335: Consider adding error handling for audio mutations.The mutation calls don't handle potential errors, which could leave the UI in an inconsistent state if device operations fail.
Add error handling for better UX:
-onToggleMuted={() => toggleMicMuted.mutate()} +onToggleMuted={() => { + toggleMicMuted.mutate(undefined, { + onError: (error) => { + sonnerToast.error("Failed to toggle microphone"); + } + }); +}}Also applies to: 339-339
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(3 hunks)apps/desktop/src/locales/en/messages.po(6 hunks)apps/desktop/src/locales/ko/messages.po(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
⏰ 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 (7)
apps/desktop/src/locales/ko/messages.po (1)
260-261: LGTM! Source reference updates are correct.The line number updates properly reflect the code changes in
listen-button.tsxafter the UI refactoring. These maintenance updates ensure accurate linkage between translation entries and source code.Also applies to: 875-875, 944-944, 956-956, 1000-1000, 1098-1098, 1102-1102
apps/desktop/src/locales/en/messages.po (1)
260-261: LGTM! Source reference updates are correct.The line number updates properly reflect the code changes in
listen-button.tsxafter the UI refactoring.Also applies to: 875-875, 944-944, 956-956, 1000-1000, 1098-1098, 1102-1102
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (5)
3-13: LGTM! Import updates are clean and used.All new icon imports are properly utilized throughout the component for the UI refactoring.
332-341: LGTM! Layout refactoring improves component separation.The change from
justify-betweentogap-2with flex-1 components provides better visual balance and separates microphone and speaker controls appropriately.
372-372: LGTM! Icon updates are consistent.The updated icons (
PauseandSquare) provide better visual consistency with the new UI design.Also applies to: 380-380
496-523: LGTM! SpeakerButton implementation is clean.The simplified speaker button component follows the same pattern as the microphone button and provides consistent styling.
406-410: Fix potential infinite loop in useEffect.The
selectedDevicedependency in the useEffect could cause an infinite loop sinceselectedDeviceis set within the effect.Apply this fix to prevent potential infinite loops:
useEffect(() => { if (!selectedDevice && devices.length > 0) { setSelectedDevice(devices[0]); } -}, [devices, selectedDevice]); +}, [devices]);Likely an incorrect or invalid review comment.
| function MicrophoneSelector({ | ||
| isMuted, | ||
| onToggleMuted, | ||
| disabled, | ||
| }: { | ||
| isMuted?: boolean; | ||
| onToggleMuted: () => void; | ||
| disabled?: boolean; | ||
| }) { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| const [selectedDevice, setSelectedDevice] = useState<string>(""); | ||
|
|
||
| const { data: devices = [], isLoading } = useQuery({ | ||
| queryKey: ["microphone-devices"], | ||
| queryFn: () => listenerCommands.listMicrophoneDevices(), | ||
| refetchOnWindowFocus: false, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!selectedDevice && devices.length > 0) { | ||
| setSelectedDevice(devices[0]); | ||
| } | ||
| }, [devices, selectedDevice]); | ||
|
|
||
| const Icon = isMuted ? MicOffIcon : MicIcon; | ||
|
|
||
| return ( | ||
| <div className="flex-1 min-w-0"> | ||
| <Popover open={isOpen} onOpenChange={setIsOpen}> | ||
| <div className="flex -space-x-px"> | ||
| <Button | ||
| variant="outline" | ||
| className="rounded-r-none flex-1 min-w-0 h-10" | ||
| disabled={disabled} | ||
| onClick={onToggleMuted} | ||
| > | ||
| <Icon | ||
| className={cn( | ||
| "w-4 h-4 flex-shrink-0", | ||
| isMuted ? "text-neutral-500" : "", | ||
| disabled && "text-neutral-300", | ||
| )} | ||
| /> | ||
| {!disabled && <SoundIndicator input="mic" size="long" />} | ||
| </Button> | ||
|
|
||
| <PopoverTrigger asChild> | ||
| <Button | ||
| variant="outline" | ||
| className="rounded-l-none px-0.5 flex-shrink-0" | ||
| disabled={disabled} | ||
| > | ||
| <ChevronDown className="w-4 h-4" /> | ||
| </Button> | ||
| </PopoverTrigger> | ||
| </div> | ||
|
|
||
| <PopoverContent className="w-64 p-0" align="end"> | ||
| <div className="p-2"> | ||
| <div className="mb-2 px-2"> | ||
| <span className="text-sm font-medium">Microphone</span> | ||
| </div> | ||
|
|
||
| {isLoading | ||
| ? ( | ||
| <div className="p-4 text-center"> | ||
| <div className="animate-spin rounded-full h-4 w-4 border-b-2 border-neutral-600 mx-auto"></div> | ||
| <p className="text-sm text-neutral-500 mt-2">Loading devices...</p> | ||
| </div> | ||
| ) | ||
| : devices.length === 0 | ||
| ? ( | ||
| <div className="p-4 text-center"> | ||
| <p className="text-sm text-neutral-500">No microphones found</p> | ||
| </div> | ||
| ) | ||
| : ( | ||
| <div className="space-y-1"> | ||
| {devices.map((device) => { | ||
| const isSelected = device === selectedDevice; | ||
| return ( | ||
| <Button | ||
| key={device} | ||
| variant="ghost" | ||
| className={cn( | ||
| "w-full justify-start text-left h-8 px-2", | ||
| isSelected && "bg-neutral-100", | ||
| )} | ||
| onClick={() => { | ||
| setSelectedDevice(device); | ||
| setIsOpen(false); | ||
| }} | ||
| > | ||
| <Icon className="w-3 h-3 mr-2 flex-shrink-0" /> | ||
| <span className="text-sm truncate flex-1">{device}</span> | ||
| {isSelected && <Check className="w-3 h-3 ml-auto flex-shrink-0 text-green-600" />} | ||
| </Button> | ||
| ); | ||
| })} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </PopoverContent> | ||
| </Popover> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Device selection logic is incomplete.
The selectedDevice state is managed but never used to actually change the microphone device. This creates a misleading UI where users can select devices but the selection has no effect.
Add device switching functionality:
const handleStopWithTemplate = () => {
const actualTemplateId = selectedTemplate === "auto" ? null : selectedTemplate;
onStop(actualTemplateId);
};
+
+const switchMicrophone = useMutation({
+ mutationFn: (deviceId: string) => listenerCommands.setMicrophoneDevice(deviceId),
+});
onClick={() => {
setSelectedDevice(device);
+ switchMicrophone.mutate(device);
setIsOpen(false);
}}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/components/editor-area/note-header/listen-button.tsx around
lines 388 to 494, the selectedDevice state is updated on user selection but not
used to switch the actual microphone input device. To fix this, implement logic
that applies the selectedDevice to change the active microphone input. This
likely involves calling a function or command to switch the microphone device
whenever selectedDevice changes, ensuring the UI selection affects the actual
audio input source.
This commit introduces several improvements to the microphone and speaker controls in the listen-button component: - Adds a `MicrophoneSelector` component that allows users to select the active microphone device and toggle the microphone mute state. - Replaces the previous `AudioControlButton` component with a `SpeakerButton` that handles the speaker mute state. - Updates the button icons to use the more appropriate `Pause` and `Square` icons for the pause and stop actions, respectively. - Ensures consistent styling and layout for the microphone and speaker controls. These changes aim to provide a more intuitive and user-friendly experience for managing audio input and output during the recording process.
ComputelessComputer
force-pushed
the
mic-selector-ui
branch
from
a65b0fe to
88619df
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
crates/audio/src/lib.rs (2)
21-39: Add error handling for audio output operationsThe
to_speakermethod silently ignores errors fromOutputStream::try_default()andDecoder::new().Consider returning a
Resultto propagate errors:-pub fn to_speaker(bytes: &'static [u8]) -> std::sync::mpsc::Sender<()> { +pub fn to_speaker(bytes: &'static [u8]) -> Result<std::sync::mpsc::Sender<()>, crate::Error> { use rodio::{Decoder, OutputStream, Sink}; let (tx, rx) = std::sync::mpsc::channel(); std::thread::spawn(move || { - if let Ok((_, stream)) = OutputStream::try_default() { + let (_, stream) = OutputStream::try_default()?; let file = std::io::Cursor::new(bytes); - if let Ok(source) = Decoder::new(file) { + let source = Decoder::new(file)?; let sink = Sink::try_new(&stream).unwrap(); sink.append(source); let _ = rx.recv_timeout(std::time::Duration::from_secs(3600)); sink.stop(); - } - } }); - tx + Ok(tx) }
41-56: Add error handling for silence methodSimilar to
to_speaker, thesilencemethod ignores errors fromOutputStream::try_default().Consider propagating errors:
-pub fn silence() -> std::sync::mpsc::Sender<()> { +pub fn silence() -> Result<std::sync::mpsc::Sender<()>, crate::Error> { use rodio::{source::Zero, OutputStream, Sink}; let (tx, rx) = std::sync::mpsc::channel(); std::thread::spawn(move || { - if let Ok((_, stream)) = OutputStream::try_default() { + let (_, stream) = OutputStream::try_default()?; let sink = Sink::try_new(&stream).unwrap(); sink.append(Zero::<f32>::new(1, 16000)); let _ = rx.recv(); sink.stop(); - } }); - tx + Ok(tx) }
🧹 Nitpick comments (2)
crates/audio/src/mic.rs (2)
14-19: Remove unnecessary commentThe comment "Information about an audio input device" is redundant - it describes "what" the struct is, which is already clear from the name.
Remove the comment:
-/// Information about an audio input device #[derive(Debug, Clone)] pub struct AudioDeviceInfo {
348-365: Potential busy-loop inread_allmethodThe
read_allmethod polls in a tight loop without yielding, which could consume excessive CPU.Consider adding a yield or using a different approach:
pub fn read_all(&mut self) -> rodio::buffer::SamplesBuffer<f32> { let mut samples = Vec::new(); let mut cx = std::task::Context::from_waker(futures_util::task::noop_waker_ref()); + let mut consecutive_empty = 0; // Drain all available samples while let std::task::Poll::Ready(Some(sample)) = self.receiver.poll_next_unpin(&mut cx) { samples.push(sample); + consecutive_empty = 0; } + + // Avoid tight loop if no samples available + if consecutive_empty > 0 { + std::thread::yield_now(); + } rodio::buffer::SamplesBuffer::new( self.config.channels(), self.config.sample_rate().0, samples, ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)apps/desktop/src-tauri/src/lib.rs(1 hunks)crates/audio/Cargo.toml(1 hunks)crates/audio/src/errors.rs(1 hunks)crates/audio/src/lib.rs(1 hunks)crates/audio/src/mic.rs(1 hunks)crates/onnx/src/lib.rs(2 hunks)crates/vad/Cargo.toml(1 hunks)crates/vad/src/error.rs(1 hunks)crates/vad/src/lib.rs(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- Cargo.toml
- crates/audio/Cargo.toml
- crates/vad/src/lib.rs
- crates/vad/src/error.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🪛 GitHub Actions: .github/workflows/fmt.yaml
apps/desktop/src-tauri/src/lib.rs
[error] 18-24: dprint formatting check failed. File is not formatted according to dprint rules.
⏰ 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). (3)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci
🔇 Additional comments (6)
crates/onnx/src/lib.rs (2)
2-2: LGTM - Import is properly used.The
LogLevelimport is correctly utilized in the session builder configuration.
12-12: LGTM - Logging configuration is appropriate.Setting the ONNX runtime log level to
Erroreffectively reduces verbosity by suppressing info, warning, and debug messages, which aligns with the coordinated logging changes across the application.crates/vad/Cargo.toml (2)
8-8: Confirmhypr-onnxexposesload-dynamicfeatureIn crates/vad/Cargo.toml (around line 8) you’ve added:
[features] load-dynamic = ["hypr-onnx/load-dynamic"]Since there’s no local
hypr-onnxcrate in this workspace, please verify that the externalhypr-onnxversion you depend on actually defines aload-dynamicfeature in itsCargo.toml(e.g. on crates.io or in its upstream repo). Without that, enablingload-dynamicwill break CI builds run with--all-features.
10-12: All directort/ndarrayreferences now go throughhypr-onnxre-exports
A search for\bort::and\bndarray::incrates/vad/srconly turned up:
- Nested imports in
use hypr_onnx::{ ndarray::{…}, ort::{…}, };- Calls to
hypr_onnx::ort::inputs!and error conversions viahypr_onnx::{ort, ndarray}No standalone
use ndarray::…oruse ort::…remain. No further action required.crates/audio/src/lib.rs (1)
76-76: Add error handling instead of usingunwrap()The
unwrap()call will panic if no input device is found. This violates the coding guideline about error handling.Apply this diff to propagate the error:
-pub fn from_mic() -> Self { +pub fn from_mic() -> Result<Self, crate::Error> { Self { source: AudioSource::RealtimeMic, - mic: Some(MicInput::new().unwrap()), + mic: Some(MicInput::new()?), speaker: None, data: None, - } + }.into() }You'll also need to update the return type to
Result<Self, crate::Error>.Likely an incorrect or invalid review comment.
crates/audio/src/errors.rs (1)
1-8: LGTM!The error enum is well-structured with appropriate error variants for audio input handling.
| let env_filter = EnvFilter::try_from_default_env().unwrap_or_else(|_| { | ||
| EnvFilter::new("info") | ||
| .add_directive("ort::logging=error".parse().unwrap()) | ||
| }); |
There was a problem hiding this comment.
Fix formatting issue flagged by pipeline.
The logging configuration logic is correct - it maintains the default "info" level while specifically setting "ort::logging" to "error" level, which coordinates well with the ONNX crate changes. However, the pipeline indicates a formatting issue that needs to be resolved.
Please run the formatter to fix the dprint formatting issue:
dprint fmt🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/lib.rs around lines 20 to 23, the code formatting
does not comply with the project's style guidelines, causing a pipeline
formatting error. To fix this, run the dprint formatter tool by executing
"dprint fmt" in the project root directory to automatically correct the
formatting issues in this file.
| let (device, config) = { | ||
| let device_guard = self | ||
| .current_device | ||
| .try_read() | ||
| .expect("Failed to read device"); | ||
| let config_guard = self | ||
| .current_config | ||
| .try_read() | ||
| .expect("Failed to read config"); | ||
| (device_guard.clone(), config_guard.clone()) | ||
| }; |
There was a problem hiding this comment.
Replace expect() with proper error handling
Using try_read().expect() can panic if the lock is held by another thread.
Consider using async read or handling the error:
-// Clone current device and config synchronously using try_read
-let (device, config) = {
- let device_guard = self
- .current_device
- .try_read()
- .expect("Failed to read device");
- let config_guard = self
- .current_config
- .try_read()
- .expect("Failed to read config");
- (device_guard.clone(), config_guard.clone())
-};
+// Clone current device and config
+let (device, config) = {
+ match (self.current_device.try_read(), self.current_config.try_read()) {
+ (Ok(device_guard), Ok(config_guard)) => {
+ (device_guard.clone(), config_guard.clone())
+ }
+ _ => {
+ // Fall back to blocking read if try_read fails
+ let device_guard = self.current_device.blocking_read();
+ let config_guard = self.current_config.blocking_read();
+ (device_guard.clone(), config_guard.clone())
+ }
+ }
+};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/audio/src/mic.rs around lines 159 to 169, replace the use of
try_read().expect() which can cause a panic if the lock is held by another
thread. Instead, handle the error returned by try_read() gracefully by matching
on the Result and implementing appropriate error handling or fallback logic, or
consider using an async read lock if applicable to avoid blocking and panics.
| Ok(_) => {} | ||
| Err(e) => { | ||
| if e.is_full() { | ||
| tracing::warn!("Audio buffer full, dropping samples"); | ||
| } | ||
| // If disconnected, the stream will be cleaned up | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing backpressure for audio buffer
When the buffer is full, samples are being dropped with only a warning log. This could lead to audio discontinuities.
Consider implementing a backpressure mechanism or using a larger buffer:
match tx.try_send(samples) {
Ok(_) => {}
Err(e) => {
if e.is_full() {
- tracing::warn!("Audio buffer full, dropping samples");
+ // Apply backpressure by blocking briefly to allow consumer to catch up
+ tracing::warn!("Audio buffer full, applying backpressure");
+ std::thread::sleep(std::time::Duration::from_micros(100));
+ // Try once more after brief pause
+ let _ = tx.try_send(e.into_inner());
}
// If disconnected, the stream will be cleaned up
}
}Alternatively, consider increasing the buffer size in line 154 from 64 to a larger value based on expected sample rates.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| match tx.try_send(samples) { | |
| Ok(_) => {} | |
| Err(e) => { | |
| if e.is_full() { | |
| tracing::warn!("Audio buffer full, dropping samples"); | |
| } | |
| // If disconnected, the stream will be cleaned up | |
| } | |
| } | |
| match tx.try_send(samples) { | |
| Ok(_) => {} | |
| Err(e) => { | |
| if e.is_full() { | |
| // Apply backpressure by blocking briefly to allow consumer to catch up | |
| tracing::warn!("Audio buffer full, applying backpressure"); | |
| std::thread::sleep(std::time::Duration::from_micros(100)); | |
| // Try once more after brief pause | |
| let _ = tx.try_send(e.into_inner()); | |
| } | |
| // If disconnected, the stream will be cleaned up | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/audio/src/mic.rs around lines 276 to 284, the current code drops audio
samples when the buffer is full, causing potential audio discontinuities. To fix
this, implement a backpressure mechanism by either blocking or retrying the send
operation until there is space in the buffer, or increase the buffer size at
line 154 from 64 to a larger value suitable for the expected sample rates to
reduce the chance of overflow.
|
Will take some UI code and work on separate PR |
implemented UI for #1062
please take on the Rust B/E from here :) @yujonglee
This is part 1 of 2 in a stack made with GitButler: