Conversation
📝 WalkthroughWalkthroughThis change implements comprehensive microphone device selection and management across the application. It introduces new UI controls for selecting microphones, updates plugin commands to get and set the selected device, adds device validation and switching support in the backend, and synchronizes configuration and localization to support this feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (MicControlWithDropdown)
participant Plugin (listener)
participant Config DB
participant Audio Layer
User->>UI (MicControlWithDropdown): Opens mic device dropdown
UI->>Plugin (listener): getSelectedMicrophoneDevice()
Plugin (listener)->>Config DB: Read selected_microphone_device
Plugin (listener)->>Audio Layer: List available devices
Plugin (listener)-->>UI: Return device list + current selection
User->>UI (MicControlWithDropdown): Selects device
UI->>Plugin (listener): setSelectedMicrophoneDevice(deviceName)
Plugin (listener)->>Audio Layer: Validate deviceName
Plugin (listener)->>Config DB: Update selected_microphone_device
Plugin (listener)->>Audio Layer: (If session active) Switch device live
Plugin (listener)-->>UI: Confirm update
Possibly related issues
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)error: failed to load source for dependency Caused by: Caused by: Caused by: 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{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".
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (8)
✨ 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: 10
🧹 Nitpick comments (7)
apps/desktop/src/locales/ko/messages.po (1)
302-302: Korean translations needed for new microphone device selection featuresMultiple new message entries related to microphone device selection have been added but remain untranslated (empty
msgstr ""). These include device loading states, selection prompts, and access descriptions.Consider getting Korean translations for the new microphone device selection features to maintain localization completeness:
- "Loading available devices..."
- "Microphone Device"
- "Select microphone device"
- "System Default"
- And other related entries
Also applies to: 546-546, 715-717, 723-725, 761-763, 765-765, 769-771, 923-923, 927-927, 931-931, 985-987, 993-995, 1053-1053, 1057-1060
crates/audio/src/errors.rs (1)
25-27: Remove unused empty Error enum.This empty enum appears to be unused and should be removed to avoid confusion.
-#[derive(thiserror::Error, Debug)] -pub enum Error {} -apps/desktop/src/components/settings/views/sound.tsx (2)
84-92: Remove console.log statements before production.Debug logging should be removed or replaced with proper error tracking.
- console.log("Attempting to call getSelectedMicrophoneDevice..."); try { const result = await listenerCommands.getSelectedMicrophoneDevice(); - console.log("Device query result:", result); return result; } catch (error) { - console.error("Device query failed:", error); throw error; }Also applies to: 101-101, 104-104, 106-106
102-102: Use optional chaining for cleaner code.- if (result && result.startsWith("DEVICES:")) { + if (result?.startsWith("DEVICES:")) {Also applies to: 118-118
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
383-383: Use optional chaining as suggested by static analysis.- if (parsedData && typeof parsedData === 'object' && parsedData.devices) { + if (parsedData?.devices) {- if (parsedData && typeof parsedData === 'object' && parsedData.selected) { + if (parsedData?.selected) {Also applies to: 409-409
crates/audio/src/mic.rs (2)
136-136: Use proper logging in error callbacks.- |err| eprintln!("Audio stream error: {}", err), + |err| tracing::error!("Audio stream error: {}", err),Also applies to: 168-168, 201-201
101-102: Consider making sample rate configurable.The hardcoded 16kHz sample rate might not be optimal for all use cases. Some devices or speech recognition systems might work better with different rates.
Consider adding a parameter or configuration option:
- pub fn sample_rate(&self) -> u32 { - 16000 // Standard sample rate for speech + pub fn sample_rate(&self) -> u32 { + self.target_sample_rate.unwrap_or(16000) // Default to 16kHz for speech }Also applies to: 344-344, 378-379
📜 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 (24)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(3 hunks)apps/desktop/src/components/settings/views/general.tsx(1 hunks)apps/desktop/src/components/settings/views/sound.tsx(2 hunks)apps/desktop/src/locales/en/messages.po(19 hunks)apps/desktop/src/locales/ko/messages.po(19 hunks)crates/audio/Cargo.toml(1 hunks)crates/audio/src/errors.rs(1 hunks)crates/audio/src/lib.rs(2 hunks)crates/audio/src/mic.rs(1 hunks)crates/db-user/src/config_types.rs(2 hunks)plugins/db/js/bindings.gen.ts(1 hunks)plugins/listener/Cargo.toml(1 hunks)plugins/listener/build.rs(1 hunks)plugins/listener/js/bindings.gen.ts(1 hunks)plugins/listener/permissions/autogenerated/commands/get_selected_microphone_device.toml(1 hunks)plugins/listener/permissions/autogenerated/commands/set_selected_microphone_device.toml(1 hunks)plugins/listener/permissions/autogenerated/reference.md(3 hunks)plugins/listener/permissions/default.toml(1 hunks)plugins/listener/permissions/schemas/schema.json(3 hunks)plugins/listener/src/commands.rs(2 hunks)plugins/listener/src/error.rs(1 hunks)plugins/listener/src/ext.rs(5 hunks)plugins/listener/src/fsm.rs(5 hunks)plugins/listener/src/lib.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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".
**/*.{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".
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
plugins/listener/build.rsplugins/listener/src/error.rscrates/db-user/src/config_types.rsapps/desktop/src/components/settings/views/general.tsxplugins/db/js/bindings.gen.tsplugins/listener/js/bindings.gen.tsplugins/listener/src/lib.rscrates/audio/src/errors.rsplugins/listener/src/ext.rsplugins/listener/src/fsm.rscrates/audio/src/lib.rsapps/desktop/src/components/editor-area/note-header/listen-button.tsxplugins/listener/src/commands.rsapps/desktop/src/components/settings/views/sound.tsxcrates/audio/src/mic.rs
🧬 Code Graph Analysis (3)
plugins/listener/src/fsm.rs (2)
crates/audio/src/lib.rs (2)
from_mic_device(82-95)from_mic(73-80)plugins/listener/src/ext.rs (11)
state(172-172)state(276-276)state(283-283)state(293-293)state(303-303)state(314-314)state(325-325)state(336-336)state(347-347)state(358-358)state(369-369)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (5)
packages/ui/src/components/ui/button.tsx (1)
Button(37-89)packages/ui/src/lib/utils.ts (1)
cn(4-6)apps/desktop/src/components/sound-indicator.tsx (1)
SoundIndicator(6-31)packages/ui/src/components/ui/popover.tsx (3)
Popover(85-85)PopoverTrigger(85-85)PopoverContent(85-85)packages/ui/src/components/ui/spinner.tsx (1)
Spinner(58-58)
apps/desktop/src/components/settings/views/sound.tsx (1)
packages/ui/src/components/ui/select.tsx (5)
Select(174-174)SelectTrigger(174-174)SelectValue(174-174)SelectContent(174-174)SelectItem(174-174)
🪛 Biome (1.9.4)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx
[error] 377-377: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 403-403: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/desktop/src/components/settings/views/sound.tsx
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 118-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci
🔇 Additional comments (23)
plugins/listener/Cargo.toml (1)
55-55: LGTM! Clean dependency addition.The
uuiddependency with thev4feature is properly added to support unique identification for microphone device management functionality.crates/audio/Cargo.toml (1)
17-18: LGTM! Appropriate dependency enhancements.The addition of the "sync" feature to
tokioand the newtokio-streamdependency properly support the expanded asynchronous audio input handling and streaming capabilities.plugins/listener/build.rs (1)
2-3: LGTM! Proper command registration.The new commands
get_selected_microphone_deviceandset_selected_microphone_deviceare properly added to the COMMANDS array to support microphone device selection functionality.plugins/listener/src/error.rs (1)
19-20: LGTM! Proper error handling addition.The
NoneUsererror variant is correctly implemented following the established pattern and supports user context validation for microphone device operations.plugins/listener/permissions/default.toml (1)
4-5: LGTM! Proper permission configuration.The new permissions
allow-get-selected-microphone-deviceandallow-set-selected-microphone-deviceare correctly added to provide controlled access to the microphone device selection functionality.plugins/listener/permissions/autogenerated/commands/get_selected_microphone_device.toml (1)
1-14: LGTM! Permission configuration follows expected pattern.The permission definitions are correctly structured with proper schema reference and both allow/deny entries for the
get_selected_microphone_devicecommand.apps/desktop/src/components/settings/views/general.tsx (1)
129-130: LGTM! Proper field preservation with safe access patterns.The mutation correctly preserves the
selected_microphone_devicefield using optional chaining and null fallback, maintaining consistency with the existing codebase pattern.crates/db-user/src/config_types.rs (2)
47-47: LGTM! Field addition follows established patterns.The
selected_microphone_devicefield is properly typed asOption<String>and aligns with the microphone device selection feature.
59-59: LGTM! Default implementation correctly updated.The Default implementation properly initializes the new field to
None, maintaining consistency with other optional fields.plugins/listener/permissions/autogenerated/commands/set_selected_microphone_device.toml (1)
1-14: LGTM! Permission configuration follows expected pattern.The permission definitions are correctly structured with proper schema reference and both allow/deny entries for the
set_selected_microphone_devicecommand.plugins/db/js/bindings.gen.ts (1)
154-154: LGTM! Type definition correctly reflects backend changes.The
selected_microphone_devicefield is properly typed asstring | nullto match the RustOption<String>type, and the generated binding is consistent with the backend structure.plugins/listener/src/lib.rs (1)
31-32: Clean API refactor for device selection confirmedWe’ve verified that both new commands are fully implemented and registered:
plugins/listener/src/commands.rsdefinespub async fn get_selected_microphone_deviceandpub async fn set_selected_microphone_device- Extension methods exist in
plugins/listener/src/ext.rs- Commands are registered in
plugins/listener/build.rsand exposed via JS bindings and permission schemasNo further action needed—approving the changes.
crates/audio/src/lib.rs (2)
7-7: Good practice: explicit exports over wildcardExplicitly exporting only
AudioErrorandErrorinstead of using wildcard imports improves code clarity and prevents accidental exposure of internal types.
82-95: Well-implemented device-specific constructorThe new
from_mic_devicemethod properly handles device selection with appropriate fallback to default behavior. The implementation follows the existing constructor pattern and correctly initializes all fields.plugins/listener/permissions/autogenerated/reference.md (1)
7-8: Auto-generated permissions correctly reflect new commandsThe permission documentation has been properly updated to include the new microphone device selection commands with appropriate allow/deny variants following the established naming conventions.
Also applies to: 115-136, 401-422
plugins/listener/js/bindings.gen.ts (1)
10-15: TypeScript bindings correctly implement new device selection APIThe generated bindings properly expose the new microphone device selection commands with appropriate TypeScript types. The
string | nullreturn type for the getter andnullparameter/return types for the setter follow established conventions.crates/audio/src/errors.rs (1)
1-23: LGTM! Well-structured error handling for audio operations.The
AudioErrorenum provides comprehensive coverage of audio-related errors with proper use ofthiserrorfor automatic conversions fromcpalerrors.apps/desktop/src/locales/en/messages.po (1)
1-1234: LGTM! Localization updates for microphone device selection.The localization file properly includes all necessary translation entries for the new microphone device selection UI.
plugins/listener/permissions/schemas/schema.json (1)
333-344: LGTM! Permission schema properly extended for microphone device management.The new permissions follow the established pattern and are correctly included in the default permission set.
Also applies to: 465-476, 514-517
apps/desktop/src/components/settings/views/sound.tsx (1)
97-112: Add error handling for JSON parsing.The JSON.parse call could throw an exception if the response format is invalid.
queryFn: async () => { const result = deviceQuery.data; - console.log("Processing device query result:", result); if (result && result.startsWith("DEVICES:")) { const devicesJson = result.substring(8); - console.log("Devices JSON:", devicesJson); - const devices = JSON.parse(devicesJson) as string[]; - console.log("Parsed devices:", devices); - return devices; + try { + const parsed = JSON.parse(devicesJson); + return parsed.devices || []; + } catch (error) { + console.error("Failed to parse device JSON:", error); + return []; + } } return []; },Likely an incorrect or invalid review comment.
plugins/listener/src/commands.rs (1)
41-67: LGTM! Thorough device validation with clear error messages.The validation logic properly checks device availability before setting it, with informative error messages.
plugins/listener/src/ext.rs (1)
46-117: Excellent error handling and fallback logic!The enhanced
list_microphone_devicesimplementation provides robust error handling with appropriate fallbacks:
- Gracefully handles device enumeration failures
- Falls back to default device when enumeration fails
- Provides detailed logging for debugging
- Never panics and always returns a valid result
This is a significant improvement over typical device enumeration code.
plugins/listener/src/fsm.rs (1)
103-127: Good integration of device selection from config.The implementation correctly:
- Retrieves the selected device from user configuration
- Falls back to default device if none selected
- Provides clear logging for debugging
- Removes the delay for faster device switching
The comment explaining the delay removal is helpful for future maintainers.
| pub async fn get_selected_microphone_device<R: tauri::Runtime>( | ||
| app: tauri::AppHandle<R>, | ||
| ) -> Result<Vec<String>, String> { | ||
| app.list_microphone_devices() | ||
| ) -> Result<Option<String>, String> { | ||
| // Get the currently selected device from config | ||
| let selected_device = app.get_selected_microphone_device() | ||
| .await | ||
| .map_err(|e| e.to_string()) | ||
| .map_err(|e| e.to_string())?; | ||
|
|
||
| // Always return device list with selected device info | ||
| if let Ok(devices) = app.list_microphone_devices().await { | ||
| tracing::info!("Available devices: {:?}, Selected: {:?}", devices, selected_device); | ||
|
|
||
| // Create a combined response that includes both device list and selected device | ||
| let mut response = std::collections::HashMap::new(); | ||
| response.insert("devices".to_string(), devices); | ||
| response.insert("selected".to_string(), vec![selected_device.unwrap_or_default()]); | ||
|
|
||
| let response_json = serde_json::to_string(&response).unwrap_or_default(); | ||
| return Ok(Some(format!("DEVICES:{}", response_json))); | ||
| } | ||
|
|
||
| // Fallback to just selected device | ||
| Ok(selected_device) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify and clarify the return format.
The current implementation mixes different response formats which is confusing. Consider returning a structured response consistently.
Consider returning a proper struct:
#[derive(serde::Serialize)]
pub struct MicrophoneDeviceInfo {
pub devices: Vec<String>,
pub selected: Option<String>,
}This would eliminate the need for the "DEVICES:" prefix hack and make the API cleaner.
🤖 Prompt for AI Agents
In plugins/listener/src/commands.rs around lines 6 to 29, the function returns
inconsistent response formats by mixing a prefixed JSON string and an optional
string. Refactor the function to always return a serialized struct
MicrophoneDeviceInfo containing a devices vector and an optional selected
string, eliminating the "DEVICES:" prefix and simplifying the API. Define the
struct with serde::Serialize, populate it with the device list and selected
device, serialize it to JSON, and return it consistently as
Ok(Some(json_string)).
| let mut response = std::collections::HashMap::new(); | ||
| response.insert("devices".to_string(), devices); | ||
| response.insert("selected".to_string(), vec![selected_device.unwrap_or_default()]); | ||
|
|
||
| let response_json = serde_json::to_string(&response).unwrap_or_default(); | ||
| return Ok(Some(format!("DEVICES:{}", response_json))); |
There was a problem hiding this comment.
Improve error handling for JSON serialization.
Using unwrap_or_default() could hide serialization errors.
- let response_json = serde_json::to_string(&response).unwrap_or_default();
- return Ok(Some(format!("DEVICES:{}", response_json)));
+ let response_json = serde_json::to_string(&response)
+ .map_err(|e| format!("Failed to serialize device info: {}", e))?;
+ return Ok(Some(format!("DEVICES:{}", response_json)));🤖 Prompt for AI Agents
In plugins/listener/src/commands.rs around lines 19 to 24, the code uses
unwrap_or_default() on the result of serde_json::to_string, which can silently
ignore serialization errors. Replace unwrap_or_default() with proper error
handling by propagating or logging the serialization error instead of defaulting
to an empty string, ensuring that serialization failures are not hidden.
apps/desktop/src/components/editor-area/note-header/listen-button.tsx
Outdated
Show resolved
Hide resolved
apps/desktop/src/components/editor-area/note-header/listen-button.tsx
Outdated
Show resolved
Hide resolved
| StateEvent::MicrophoneDeviceChanged(device_name) => { | ||
| // Handle device change during active session with atomic restart | ||
| if let Some(session_id) = self.session_id.clone() { | ||
| tracing::info!("🎤 ATOMIC microphone device switch to: {:?}", device_name); | ||
|
|
||
| // Step 1: Update config atomically | ||
| if let Err(e) = self.switch_microphone_device_lightweight(device_name.clone()).await { | ||
| tracing::error!("❌ Config update failed: {:?}", e); | ||
| return Handled; | ||
| } | ||
|
|
||
| // Step 2: Atomic restart - no delays, fast cleanup | ||
| tracing::info!("⚡ Atomic audio restart"); | ||
|
|
||
| // Quick cleanup - abort all tasks immediately | ||
| if let Some(tx) = self.silence_stream_tx.take() { | ||
| let _ = tx.send(()); | ||
| } | ||
| if let Some(mut tasks) = self.tasks.take() { | ||
| tasks.abort_all(); | ||
| // Don't wait for tasks to finish - abort and move on | ||
| } | ||
|
|
||
| // Reset channels but keep session_id | ||
| self.mic_muted_tx = None; | ||
| self.mic_muted_rx = None; | ||
| self.speaker_muted_tx = None; | ||
| self.speaker_muted_rx = None; | ||
| self.session_state_tx = None; | ||
|
|
||
| // Step 3: Immediate restart with new device (no delays) | ||
| match self.setup_resources(&session_id).await { | ||
| Ok(_) => { | ||
| tracing::info!("✅ ATOMIC microphone switch SUCCESS"); | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("❌ ATOMIC switch FAILED: {:?} - going inactive", e); | ||
| return Transition(State::inactive()); | ||
| } | ||
| } | ||
| } | ||
| Handled | ||
| } |
There was a problem hiding this comment.
Add user notification for device switching failures.
When device switching fails (line 627), the session transitions to inactive but users aren't notified. They might think the app crashed or froze.
Err(e) => {
- tracing::error!("❌ ATOMIC switch FAILED: {:?} - going inactive", e);
+ tracing::error!("❌ ATOMIC switch FAILED: {:?} - going inactive", e);
+ // TODO: Emit error event to notify UI
+ let _ = SessionEvent::DeviceSwitchError {
+ reason: format!("Failed to switch microphone: {}", e)
+ }.emit(&self.app);
return Transition(State::inactive());
}🤖 Prompt for AI Agents
In plugins/listener/src/fsm.rs around lines 591 to 633, when the device
switching fails at line 627 and the session transitions to inactive, add a user
notification to inform them of the failure. Modify the error handling block to
trigger a user-facing alert or message before returning the Transition to
inactive state, ensuring users are aware of the issue instead of assuming the
app crashed or froze.
| if let Some(tx) = self.silence_stream_tx.take() { | ||
| let _ = tx.send(()); | ||
| } | ||
| if let Some(mut tasks) = self.tasks.take() { | ||
| tasks.abort_all(); | ||
| // Don't wait for tasks to finish - abort and move on | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider waiting for critical tasks to prevent resource leaks.
Aborting tasks without joining them could leave resources (file handles, network connections) in an inconsistent state. While speed is important, ensuring clean shutdown prevents issues.
if let Some(mut tasks) = self.tasks.take() {
tasks.abort_all();
- // Don't wait for tasks to finish - abort and move on
+ // Wait briefly for critical tasks to clean up
+ let _ = tokio::time::timeout(
+ Duration::from_millis(100),
+ async {
+ while let Some(_) = tasks.join_next().await {}
+ }
+ ).await;
}📝 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.
| if let Some(tx) = self.silence_stream_tx.take() { | |
| let _ = tx.send(()); | |
| } | |
| if let Some(mut tasks) = self.tasks.take() { | |
| tasks.abort_all(); | |
| // Don't wait for tasks to finish - abort and move on | |
| } | |
| if let Some(tx) = self.silence_stream_tx.take() { | |
| let _ = tx.send(()); | |
| } | |
| if let Some(mut tasks) = self.tasks.take() { | |
| tasks.abort_all(); | |
| // Wait briefly for critical tasks to clean up | |
| let _ = tokio::time::timeout( | |
| Duration::from_millis(100), | |
| async { | |
| while let Some(_) = tasks.join_next().await {} | |
| } | |
| ).await; | |
| } |
🤖 Prompt for AI Agents
In plugins/listener/src/fsm.rs around lines 606 to 612, the current code aborts
tasks without waiting for their completion, which may cause resource leaks.
Modify the code to await the completion of all aborted tasks after calling
abort_all(), ensuring a clean shutdown by properly releasing resources like file
handles and network connections before proceeding.
| found_device = Some(device); | ||
| break; | ||
| } | ||
| // Try case-insensitive match as fallback | ||
| if name.to_lowercase() == device_name.to_lowercase() { | ||
| found_device = Some(device); | ||
| break; | ||
| } | ||
| // Try partial match for similar names (e.g., "MacBook Pro Microphone" contains "MacBook") | ||
| if name.to_lowercase().contains(&device_name.to_lowercase()) || | ||
| device_name.to_lowercase().contains(&name.to_lowercase()) { | ||
| found_device = Some(device); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine device matching logic to prevent unintended matches.
The current partial matching is too permissive. For example, searching for "Book" could match "MacBook Pro Microphone" which might not be intended.
Consider this approach:
- Try exact match first (current)
- Try case-insensitive exact match (current)
- For partial matches, require the search term to be a complete word/token
// Try partial match for similar names (e.g., "MacBook Pro Microphone" contains "MacBook")
- if name.to_lowercase().contains(&device_name.to_lowercase()) ||
- device_name.to_lowercase().contains(&name.to_lowercase()) {
+ // Only match if the device_name appears as a complete token
+ let name_lower = name.to_lowercase();
+ let search_lower = device_name.to_lowercase();
+ if name_lower.split_whitespace().any(|word| word == search_lower) ||
+ name_lower.starts_with(&format!("{} ", search_lower)) ||
+ name_lower.ends_with(&format!(" {}", search_lower)) ||
+ name_lower.contains(&format!(" {} ", search_lower)) {
found_device = Some(device);
break;
}📝 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.
| if name == device_name { | |
| found_device = Some(device); | |
| break; | |
| } | |
| // Try case-insensitive match as fallback | |
| if name.to_lowercase() == device_name.to_lowercase() { | |
| found_device = Some(device); | |
| break; | |
| } | |
| // Try partial match for similar names (e.g., "MacBook Pro Microphone" contains "MacBook") | |
| if name.to_lowercase().contains(&device_name.to_lowercase()) || | |
| device_name.to_lowercase().contains(&name.to_lowercase()) { | |
| found_device = Some(device); | |
| break; | |
| } | |
| } | |
| if name == device_name { | |
| found_device = Some(device); | |
| break; | |
| } | |
| // Try case-insensitive match as fallback | |
| if name.to_lowercase() == device_name.to_lowercase() { | |
| found_device = Some(device); | |
| break; | |
| } | |
| // Only match if the device_name appears as a complete token | |
| let name_lower = name.to_lowercase(); | |
| let search_lower = device_name.to_lowercase(); | |
| if name_lower.split_whitespace().any(|word| word == search_lower) || | |
| name_lower.starts_with(&format!("{} ", search_lower)) || | |
| name_lower.ends_with(&format!(" {}", search_lower)) || | |
| name_lower.contains(&format!(" {} ", search_lower)) { | |
| found_device = Some(device); | |
| break; | |
| } |
🤖 Prompt for AI Agents
In crates/audio/src/mic.rs between lines 58 and 73, the partial matching logic
for device names is too broad, causing unintended matches like "Book" matching
"MacBook Pro Microphone." To fix this, keep the exact and case-insensitive exact
matches as is, but update the partial match to only succeed if the search term
matches a whole word or token within the device name. Implement this by
splitting the device name and search term into tokens and checking if any token
matches the search term exactly in a case-insensitive manner.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (3)
374-422: Extract duplicated device parsing logic into a helper function.The device data parsing logic is duplicated between
microphoneDevicesandselectedDevicequeries. This violates DRY principle and could lead to maintenance issues.
360-367: Improve error handling to provide user feedback.The current error handling silently fails by returning
null. Users won't know if device enumeration failed due to permissions, API errors, or other issues.
424-442: Add user feedback for device switching errors.The mutation logs errors to console but doesn't inform users when device switching fails. Users need to know if their action succeeded.
crates/audio/src/mic.rs (3)
78-98: Replaceeprintln!with proper logging framework.Using
eprintln!for logging bypasses the application's logging configuration and always outputs to stderr.
67-73: Refine device matching logic to prevent unintended matches.The current partial matching is too permissive. For example, searching for "Book" could match "MacBook Pro Microphone" which might not be intended.
389-392: Wait for thread cleanup to prevent resource leaks.The
Dropimplementation sends a shutdown signal but doesn't wait for the thread to finish.
🧹 Nitpick comments (1)
plugins/listener/src/ext.rs (1)
45-114: Consider reducing logging verbosity for production environments.While the extensive logging is helpful for debugging device enumeration issues, the current implementation logs at info level for every device found and various fallback scenarios. This could be noisy in production environments with many audio devices.
Consider adjusting log levels:
- tracing::info!("Found input device: '{}'", name); + tracing::debug!("Found input device: '{}'", name); - tracing::info!( + tracing::debug!( "Successfully enumerated {} microphone devices out of {} total devices", device_names.len(), device_count );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(3 hunks)crates/audio/src/mic.rs(1 hunks)plugins/listener/src/commands.rs(2 hunks)plugins/listener/src/ext.rs(5 hunks)plugins/listener/src/fsm.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/listener/src/commands.rs
- plugins/listener/src/fsm.rs
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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".
**/*.{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".
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
plugins/listener/src/ext.rscrates/audio/src/mic.rsapps/desktop/src/components/editor-area/note-header/listen-button.tsx
🧬 Code Graph Analysis (1)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (4)
packages/ui/src/components/ui/button.tsx (1)
Button(37-89)packages/ui/src/lib/utils.ts (1)
cn(4-6)apps/desktop/src/components/sound-indicator.tsx (1)
SoundIndicator(6-31)packages/ui/src/components/ui/popover.tsx (3)
Popover(85-85)PopoverTrigger(85-85)PopoverContent(85-85)
🪛 Biome (1.9.4)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx
[error] 377-377: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 403-403: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci
🔇 Additional comments (1)
plugins/listener/src/ext.rs (1)
14-21: LGTM! Well-designed trait extensions for microphone device management.The new async methods provide comprehensive microphone device selection functionality with clear separation of concerns.
2 participants
No description provided.