mic selector #1062
mic selector #1062
Conversation
📝 WalkthroughWalkthroughThis change introduces a comprehensive microphone device selection feature across the desktop application and backend. It adds UI components for selecting and switching microphone devices, extends backend APIs and configuration to persist and update the selected device, and implements robust device enumeration and switching logic. Localization and permissions are updated to support the new functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Desktop)
participant Backend (Listener Plugin)
participant Audio Layer
User->>UI (Desktop): Open mic device dropdown/select device
UI (Desktop)->>Backend (Listener Plugin): setSelectedMicrophoneDevice(deviceName)
Backend (Listener Plugin)->>Audio Layer: Validate device
Audio Layer-->>Backend (Listener Plugin): Validation result
Backend (Listener Plugin)->>Backend (Listener Plugin): Update config and/or switch device (if session active)
Backend (Listener Plugin)-->>UI (Desktop): Success/Error response
UI (Desktop)-->>User: Show toast/feedback, update UI
sequenceDiagram
participant UI (Desktop)
participant Backend (Listener Plugin)
UI (Desktop)->>Backend (Listener Plugin): getSelectedMicrophoneDevice()
Backend (Listener Plugin)->>Backend (Listener Plugin): Retrieve config, enumerate devices
Backend (Listener Plugin)-->>UI (Desktop): Return selected device and device list
UI (Desktop)-->>User: Show current selection in UI
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ 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). (1)
✨ 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
🔭 Outside diff range comments (3)
crates/audio/src/errors.rs (1)
25-27: Remove the unused emptyErrorenum.The empty
Errorenum serves no purpose and could cause confusion with the newAudioErrorenum. Consider removing it to keep the error module clean.-#[derive(thiserror::Error, Debug)] -pub enum Error {} -crates/audio/src/lib.rs (1)
101-101: Handle speaker creation errors in the constructor.The
from_speakerconstructor usesunwrap()onSpeakerInput::new(), which could panic if speaker initialization fails. Consider returning aResultor at least documenting the panic condition.- pub fn from_speaker(sample_rate_override: Option<u32>) -> Self { + pub fn from_speaker(sample_rate_override: Option<u32>) -> Result<Self, AudioError> { Self { source: AudioSource::RealtimeSpeaker, mic: None, - speaker: Some(SpeakerInput::new(sample_rate_override).unwrap()), + speaker: Some(SpeakerInput::new(sample_rate_override)?), data: None, - } + }.into() }plugins/listener/src/ext.rs (1)
197-201: Use the selected microphone device for permission checks.The permission check and request methods use the default device instead of the user's selected device. This could cause incorrect permission status if the selected device requires different permissions.
For
check_microphone_access:#[cfg(not(target_os = "macos"))] { - // Use default device for permission check - let mut mic_sample_stream = hypr_audio::AudioInput::from_mic().stream(); + // Use selected device for permission check + let selected_device = self.get_selected_microphone_device().await.ok().flatten(); + let mut mic_sample_stream = hypr_audio::AudioInput::from_mic_device(selected_device).stream(); let sample = mic_sample_stream.next().await; Ok(sample.is_some()) }For
request_microphone_access:#[cfg(not(target_os = "macos"))] { - // Use default device for permission request - let mut mic_sample_stream = hypr_audio::AudioInput::from_mic().stream(); + // Use selected device for permission request + let selected_device = self.get_selected_microphone_device().await.ok().flatten(); + let mut mic_sample_stream = hypr_audio::AudioInput::from_mic_device(selected_device).stream(); mic_sample_stream.next().await; }Also applies to: 228-230
🧹 Nitpick comments (8)
plugins/listener/Cargo.toml (1)
19-19: Remove duplicateduuidentry to avoid unnecessary rebuilds
uuidis declared in both[dev-dependencies](line 19) and[dependencies](line 55).
Because Cargo unifies feature sets across sections, the dev-dependency entry is redundant and slightly slows incremental builds.-[dev-dependencies] -… -uuid = { workspace = true }Keep only the
[dependencies]declaration withfeatures = ["v4"].Also applies to: 55-55
apps/docs/data/i18n.json (1)
4-6: Consider adding a CI check for translation count driftThe manual
total/missingcounters are easy to desynchronise from the real key set.
A tiny script in CI that recomputes these numbers from the JSON files would eliminate future inconsistencies.Also applies to: 9-10
plugins/listener/src/fsm.rs (1)
600-645: Consider refactoring to reduce code duplication.The device change handler duplicates cleanup logic from
cleanup_resources()method (lines 618-631 vs 419-436). Consider calling the existing method to maintain DRY principle.Apply this refactor:
// 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; + self.cleanup_resources().await;apps/desktop/src/components/editor-area/note-header/listen-button.tsx (2)
340-364: Add type safety for parsed data.The function parses JSON but doesn't validate the structure. Consider adding type guards to ensure data integrity.
+interface DeviceData { + devices: string[]; + selected?: string[]; +} + +function isDeviceData(data: unknown): data is DeviceData { + return ( + typeof data === 'object' && + data !== null && + 'devices' in data && + Array.isArray((data as DeviceData).devices) && + (data as DeviceData).devices.every(d => typeof d === 'string') + ); +} + -function parseDeviceData(result: string | null | undefined): { devices: string[]; selected?: string[] } | null { +function parseDeviceData(result: string | null | undefined): DeviceData | null { if (!result || !result.startsWith("DEVICES:")) { return null; } const devicesJson = result.substring(8); try { const parsedData = JSON.parse(devicesJson); // Check if it's the new format with devices and selected - if (parsedData && typeof parsedData === "object" && parsedData.devices) { + if (isDeviceData(parsedData)) { return parsedData; } // Fallback to old format (array of devices) if (Array.isArray(parsedData)) { return { devices: parsedData }; } return null; } catch (e) { console.error("Failed to parse device data:", e); return null; } }
427-431: Optimize refetch calls.Both success and error handlers call the same three refetch methods. Consider using React Query's invalidation pattern for better performance.
Instead of manual refetches, use query invalidation:
+import { useQueryClient } from "@tanstack/react-query"; + function MicControlWithDropdown({ isMuted, onMuteClick, disabled, }: { isMuted?: boolean; onMuteClick: () => void; disabled?: boolean; }) { const { t } = useLingui(); + const queryClient = useQueryClient(); // ... existing code ... const updateSelectedDevice = useMutation({ mutationFn: (deviceName: string | null) => listenerCommands.setSelectedMicrophoneDevice(deviceName), onSuccess: (_, deviceName) => { const displayName = deviceName === null ? t`System Default` : deviceName; sonnerToast.success(t`Microphone switched to ${displayName}`, { duration: 2000, }); - // Force immediate refetch to ensure UI updates instantly - deviceQuery.refetch(); - microphoneDevices.refetch(); - selectedDevice.refetch(); + // Invalidate all microphone-related queries + queryClient.invalidateQueries({ queryKey: ["microphoneDeviceInfo"] }); + queryClient.invalidateQueries({ queryKey: ["microphoneDevices"] }); + queryClient.invalidateQueries({ queryKey: ["selectedMicrophoneDevice"] }); }, onError: (error, deviceName) => { const displayName = deviceName === null ? t`System Default` : deviceName; sonnerToast.error(t`Failed to switch to ${displayName}`, { description: t`Please try again or check your microphone permissions.`, duration: 4000, }); - // Even on error, refresh to show correct state - deviceQuery.refetch(); - microphoneDevices.refetch(); - selectedDevice.refetch(); + // Refresh state even on error + queryClient.invalidateQueries({ queryKey: ["microphoneDeviceInfo"] }); + queryClient.invalidateQueries({ queryKey: ["microphoneDevices"] }); + queryClient.invalidateQueries({ queryKey: ["selectedMicrophoneDevice"] }); }, });Also applies to: 440-443
apps/desktop/src/components/settings/views/sound.tsx (1)
102-102: Use optional chaining as suggested by static analysis.Static analysis correctly identifies opportunities for optional chaining.
-if (result && result.startsWith("DEVICES:")) { +if (result?.startsWith("DEVICES:")) {-if (result && result.startsWith("DEVICES:")) { +if (result?.startsWith("DEVICES:")) {Also applies to: 118-118
apps/desktop/src/locales/ko/messages.po (1)
302-304: Korean translations need to be added.All new microphone device-related message IDs have been added but Korean translations are missing (empty msgstr). This is expected but should be tracked for completion.
Would you like me to create an issue to track the missing Korean translations for the microphone device features?
Also applies to: 599-606, 732-743, 782-797, 1024-1029, 1098-1104
crates/audio/src/mic.rs (1)
408-428: Consider simplifying the thread shutdown logic.The current implementation spawns a new thread just to join the audio thread with a timeout. This adds unnecessary complexity.
Consider using a simpler approach with a direct timeout check or async runtime facilities if available.
📜 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 (26)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(4 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(32 hunks)apps/desktop/src/locales/ko/messages.po(32 hunks)apps/docs/data/i18n.json(1 hunks)crates/audio/Cargo.toml(1 hunks)crates/audio/src/errors.rs(1 hunks)crates/audio/src/lib.rs(3 hunks)crates/audio/src/mic.rs(2 hunks)crates/db-user/src/config_types.rs(2 hunks)plugins/db/js/bindings.gen.ts(1 hunks)plugins/db/src/ext.rs(2 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.rscrates/db-user/src/config_types.rsapps/desktop/src/components/settings/views/general.tsxplugins/listener/src/error.rsplugins/listener/src/lib.rsplugins/db/js/bindings.gen.tscrates/audio/src/errors.rsplugins/db/src/ext.rsplugins/listener/js/bindings.gen.tscrates/audio/src/lib.rsplugins/listener/src/commands.rsapps/desktop/src/components/editor-area/note-header/listen-button.tsxplugins/listener/src/ext.rsapps/desktop/src/components/settings/views/sound.tsxcrates/audio/src/mic.rsplugins/listener/src/fsm.rs
🧬 Code Graph Analysis (1)
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/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 (4)
- GitHub Check: ci
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (24)
crates/audio/Cargo.toml (1)
17-18: Verify feature unification for Tokio and newtokio-streamcrateAdding the
syncfeature and the extratokio-streamdependency is fine, but both crates rely on Tokio’s version/feature unification at workspace level.
Please runcargo tree -e features | rg tokioto confirm that:
- No other crate pulls incompatible feature sets (e.g.
io-util,time) that could silently turn on the heavyfullfeature group.tokio-streamresolves to the same Tokio version; a mismatched minor patch will cause duplicated compile artefacts.If a mismatch appears, bump the workspace‐pinned version or add the missing features here instead of pulling them in transitively.
plugins/listener/build.rs (1)
2-3: Command list update looks goodThe new command strings follow the existing naming convention and keep the array in logical group order.
No further action needed.plugins/listener/src/error.rs (1)
19-20: Variant addition is consistent
NoneUsercleanly extends the enum and is automatically covered by the customSerializeimpl.
No issues spotted.plugins/listener/permissions/default.toml (1)
4-5: LGTM! Clean permission additions.The new permissions align with the microphone device management functionality and follow the established naming convention.
plugins/listener/src/lib.rs (1)
31-32: LGTM! Command registration follows established patterns.The new microphone device management commands are properly registered with the correct type parameters and placement.
plugins/listener/permissions/autogenerated/commands/set_selected_microphone_device.toml (1)
1-14: LGTM! Autogenerated permissions file is properly structured.The file follows the standard format for Tauri command permissions with both allow and deny entries correctly configured.
crates/db-user/src/config_types.rs (1)
59-59: LGTM! Default value correctly updated.The default implementation properly initializes the new field to None, maintaining consistency with the field type.
apps/desktop/src/components/settings/views/general.tsx (1)
129-130: LGTM! Safe field access with backwards compatibility.The optional chaining and null coalescing operators properly handle both the new
selected_microphone_devicefield and the legacyselected_template_idfield, ensuring robust migration support.plugins/listener/permissions/autogenerated/commands/get_selected_microphone_device.toml (1)
1-14: LGTM! Permission configuration is properly structured.The TOML file correctly defines both allow and deny permissions for the
get_selected_microphone_devicecommand with appropriate identifiers and descriptions.plugins/db/js/bindings.gen.ts (1)
154-154: LGTM! Type definition correctly supports microphone device selection.The addition of the optional
selected_microphone_devicefield properly supports the new microphone device selection feature with appropriate null handling.plugins/db/src/ext.rs (2)
20-23: LGTM! Trait method signature is consistent with existing patterns.The async method signature follows the established pattern in the trait and properly complements the existing
db_get_configmethod.
139-146: LGTM! Implementation follows established patterns and has proper error handling.The implementation correctly follows the same pattern as other methods in the trait with appropriate state management, locking, database reference validation, and error handling.
plugins/listener/permissions/autogenerated/reference.md (3)
7-8: LGTM! Default permission set correctly updated.The new microphone device selection permissions are properly added to the default permission set.
115-136: LGTM! Permission table entries are correctly formatted.The
get_selected_microphone_devicepermission entries follow the established documentation format with proper identifiers and descriptions.
401-422: LGTM! Permission table entries are correctly formatted.The
set_selected_microphone_devicepermission entries follow the established documentation format with proper identifiers and descriptions.plugins/listener/js/bindings.gen.ts (1)
10-15: LGTM! Command bindings are properly implemented.The new microphone device selection methods have correct signatures and properly invoke the corresponding Tauri plugin commands. The return types appropriately handle null values for device selection.
plugins/listener/permissions/schemas/schema.json (1)
333-476: Permission definitions look good!The new microphone device selection permissions follow the existing pattern and are properly integrated into the default permission set.
plugins/listener/src/ext.rs (1)
44-114: Excellent error handling and device enumeration!The improved
list_microphone_devicesimplementation with comprehensive error handling, detailed logging, and fallback logic is well-designed and robust.plugins/listener/src/fsm.rs (4)
52-54: LGTM!The getter method for session ID is implemented correctly.
103-121: LGTM!The microphone device selection logic is well-implemented with appropriate logging for debugging.
127-130: Good documentation of the delay removal rationale.The comment clearly explains why the delay was removed and provides context with the commit reference.
416-437: LGTM!The lightweight cleanup method effectively separates audio resource cleanup from session management, enabling fast device switching.
apps/desktop/src/locales/en/messages.po (1)
302-304: LGTM!All new microphone device-related translations are properly added with correct English text.
Also applies to: 599-606, 732-743, 782-797, 1024-1029, 1098-1104
crates/audio/src/mic.rs (1)
59-120: Well-implemented device selection logic!The fallback strategy from exact match → case-insensitive match → word-boundary partial match → default device is robust and user-friendly.
| pub telemetry_consent: bool, | ||
| pub save_recordings: Option<bool>, | ||
| pub selected_template_id: Option<String>, | ||
| pub selected_microphone_device: Option<String>, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify database migration for field rename.
The field rename from selected_template_id to selected_microphone_device is a breaking change that requires careful migration handling.
🏁 Script executed:
#!/bin/bash
# Verify migration strategy for the field rename
echo "Checking for database migrations related to the field rename..."
fd -t f -e sql . | xargs grep -l "selected_template_id\|selected_microphone_device" 2>/dev/null || echo "No SQL migration files found"
echo "Checking for references to the old field name..."
rg "selected_template_id" -A 2 -B 2 --type rust --type ts --type tsxLength of output: 493
🏁 Script executed:
#!/bin/bash
# Check for a migrations directory and SQL files referencing the old/new field
if [ -d migrations ]; then
echo "Searching SQL migrations in migrations/ for field rename references..."
fd -t f -e sql migrations | xargs grep -Hl "selected_template_id\|selected_microphone_device" 2>/dev/null \
|| echo "No SQL migration references found in migrations/"
else
echo "No migrations/ directory found"
fi
# Search across source for any lingering references to the old field name
echo "Searching codebase for 'selected_template_id' occurrences..."
rg "selected_template_id" --glob "*.rs" --glob "*.sql" --glob "*.ts" --glob "*.tsx" \
|| echo "No references to selected_template_id found in codebase"Length of output: 1069
Action Required: Update lingering references and add migration
The rename from selected_template_id to selected_microphone_device breaks code in several places and no SQL migration exists to preserve user data. Please:
• Add a database migration that renames the selected_template_id column to selected_microphone_device (or backfills data into the new field)
• Update all code references to the old field:
plugins/db/js/bindings.gen.ts: removeselected_template_idfromConfigGeneralor regenerate bindingsapps/desktop/src/components/editor-area/index.tsx: replaceconfig.general.selected_template_idwithconfig.general.selected_microphone_deviceapps/desktop/src/components/settings/views/general.tsx: updateselected_template_id→selected_microphone_deviceapps/desktop/src/components/settings/views/templates.tsx: same replacements in both usage and initializers
Once these are in place, verify end-to-end that settings load correctly and existing user configurations aren’t lost.
🤖 Prompt for AI Agents
In crates/db-user/src/config_types.rs at line 47, the field renamed from
selected_template_id to selected_microphone_device requires a database migration
to rename the corresponding column or backfill data to preserve user settings.
Additionally, update all code references to selected_template_id in
plugins/db/js/bindings.gen.ts by regenerating bindings or removing the old
field, and replace selected_template_id with selected_microphone_device in
apps/desktop/src/components/editor-area/index.tsx,
apps/desktop/src/components/settings/views/general.tsx, and
apps/desktop/src/components/settings/views/templates.tsx in both usage and
initialization. After these changes, test thoroughly to ensure settings load
correctly and no user data is lost.
| const microphoneDevices = useQuery({ | ||
| queryKey: ["microphoneDevices"], | ||
| 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; | ||
| } | ||
| return []; | ||
| }, | ||
| enabled: micPermissionStatus.data === true && deviceQuery.data !== undefined, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract shared device parsing logic.
The device parsing logic is duplicated between sound.tsx and listen-button.tsx. Consider extracting it to a shared utility.
Create a shared utility file:
// utils/microphone-devices.ts
export interface DeviceData {
devices: string[];
selected?: string[];
}
export function parseDeviceData(result: string | null | undefined): DeviceData | null {
if (!result || !result.startsWith("DEVICES:")) {
return null;
}
const devicesJson = result.substring(8);
try {
const parsedData = JSON.parse(devicesJson);
// Check if it's the new format with devices and selected
if (parsedData && typeof parsedData === "object" && parsedData.devices) {
return parsedData;
}
// Fallback to old format (array of devices)
if (Array.isArray(parsedData)) {
return { devices: parsedData };
}
return null;
} catch (e) {
console.error("Failed to parse device data:", e);
return null;
}
}Then use it in both components:
+import { parseDeviceData } from "@/utils/microphone-devices";
const microphoneDevices = useQuery({
queryKey: ["microphoneDevices"],
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;
- }
- return [];
+ return parseDeviceData(result)?.devices || [];
},
enabled: micPermissionStatus.data === true && deviceQuery.data !== undefined,
});Also applies to: 340-364
🧰 Tools
🪛 Biome (1.9.4)
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In apps/desktop/src/components/settings/views/sound.tsx around lines 97 to 112
and also lines 340 to 364, the device parsing logic is duplicated. Extract this
logic into a shared utility file (e.g., utils/microphone-devices.ts) with a
function that safely parses the device data string and returns a structured
object. Replace the inline parsing code in sound.tsx and listen-button.tsx with
calls to this new utility function to avoid duplication and improve
maintainability.
|
@plyght can you make it compile |
|
@yujonglee will fix everything 🫡 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/audio/src/lib.rs (1)
115-128: LGTM: Consistent error handling in stream creation.The
streammethod now properly propagates errors from both microphone and speaker stream creation, addressing the previous review comment about inconsistent error handling.apps/desktop/src/utils/microphone-devices.ts (1)
11-35: LGTM: Robust parsing with backward compatibility.The
parseDeviceDatafunction handles both new and legacy formats gracefully, with proper error handling and fallback logic. This addresses the previous review comment about extracting shared device parsing logic.plugins/listener/src/fsm.rs (1)
438-474: Method correctly addresses past review feedback.The method has been renamed from
switch_microphone_device_lightweighttoupdate_microphone_device_config, which better reflects its current implementation. However, the TODO comment indicates incomplete functionality.The comment indicates this is still incomplete as noted in previous reviews - it only updates config without implementing actual hot-swapping.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(3 hunks)apps/desktop/src/components/settings/views/sound.tsx(2 hunks)apps/desktop/src/locales/en/messages.po(32 hunks)apps/desktop/src/locales/ko/messages.po(32 hunks)apps/desktop/src/utils/microphone-devices.ts(1 hunks)crates/audio/src/errors.rs(1 hunks)crates/audio/src/lib.rs(3 hunks)crates/audio/src/mic.rs(2 hunks)crates/db-user/src/config_types.rs(2 hunks)plugins/listener/Cargo.toml(1 hunks)plugins/listener/js/bindings.gen.ts(1 hunks)plugins/listener/src/commands.rs(2 hunks)plugins/listener/src/error.rs(1 hunks)plugins/listener/src/ext.rs(6 hunks)plugins/listener/src/fsm.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/listener/Cargo.toml
- crates/db-user/src/config_types.rs
- plugins/listener/js/bindings.gen.ts
- apps/desktop/src/components/editor-area/note-header/listen-button.tsx
- plugins/listener/src/commands.rs
- apps/desktop/src/locales/ko/messages.po
- plugins/listener/src/ext.rs
- apps/desktop/src/locales/en/messages.po
🧰 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/error.rscrates/audio/src/errors.rsplugins/listener/src/fsm.rscrates/audio/src/lib.rscrates/audio/src/mic.rsapps/desktop/src/utils/microphone-devices.tsapps/desktop/src/components/settings/views/sound.tsx
🧬 Code Graph Analysis (1)
apps/desktop/src/utils/microphone-devices.ts (1)
plugins/listener/js/bindings.gen.ts (1)
MicrophoneDeviceInfo(80-83)
🪛 Biome (1.9.4)
apps/desktop/src/components/settings/views/sound.tsx
[error] 89-89: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (26)
plugins/listener/src/error.rs (2)
17-18: LGTM: Proper error propagation from audio crate.The
AudioErrorvariant correctly uses the#[from]attribute to enable automatic conversion fromhypr_audio::AudioError, facilitating clean error propagation.
21-22: LGTM: Clear semantic error variant.The
NoneUservariant provides a clear, specific error case for user-related failures.crates/audio/src/errors.rs (1)
1-25: LGTM: Comprehensive and well-structured error handling.The
AudioErrorenum provides excellent coverage of audio-related error scenarios with:
- Proper use of
thiserror::Errorderive macro- Clear, descriptive error messages
- Appropriate error wrapping from underlying crates (
cpal,anyhow)- Good separation of concerns between different error types
This establishes a solid foundation for error handling throughout the audio crate.
crates/audio/src/lib.rs (3)
7-7: LGTM: Focused error export.Narrowing the export from
errors::*to justAudioErrorprovides a cleaner, more focused API.
82-95: LGTM: Well-implemented device selection method.The
from_mic_devicemethod provides flexible device selection with proper fallback to default when no device is specified.
97-104: LGTM: Proper error handling in speaker creation.The method now returns
Result<AudioInput, AudioError>and properly propagates errors fromSpeakerInput::new.apps/desktop/src/utils/microphone-devices.ts (2)
1-4: LGTM: Interface matches backend bindings.The
MicrophoneDeviceInfointerface correctly matches the TypeScript bindings from the plugin, ensuring type consistency across the application.
37-58: LGTM: Comprehensive type guards.The type guard functions provide thorough validation of the data structures with proper type checking for all required fields.
apps/desktop/src/components/settings/views/sound.tsx (3)
108-116: LGTM: Well-structured device selection mutation.The mutation properly handles the device selection update with appropriate mapping of "default" to
nulland triggers refetch on success.
118-130: LGTM: Robust selected device logic.The
getSelectedDevicefunction properly handles edge cases including missing devices and devices no longer available in the list.
155-199: LGTM: Well-implemented microphone device selection UI.The conditional rendering of the device selection section is appropriately gated on microphone permission, and the dropdown implementation includes proper loading states and device mapping.
plugins/listener/src/fsm.rs (6)
52-54: LGTM - Simple accessor method.Clean implementation for exposing the current session ID.
103-121: Good device selection implementation with proper fallback.The logic correctly retrieves the selected microphone device from config and falls back to default when no device is specified. The logging provides good visibility into device selection.
416-436: Well-designed cleanup method for atomic device switching.The
cleanup_resourcesmethod provides fast resource cleanup while preserving session context, which is ideal for atomic device switching. The approach of aborting tasks and cleaning audio state while keeping session_id is sound.
574-574: LGTM - Clean event enum extension.Adding
MicrophoneDeviceChanged(Option<String>)variant is a clean way to handle device change events.
600-631: Robust atomic device switching implementation.The event handling provides proper atomic restart logic:
- Updates config atomically
- Performs fast cleanup via
cleanup_resources- Immediately restarts with new device
- Handles errors gracefully by transitioning to inactive state
This addresses the performance concern about device switching being laggy.
127-131: Confirm absence of audio‐stability tests; manual validation requiredI ran a repo-wide search for any references to “audio stability,” timing, or delay and found no tests or code depending on this 50 ms startup delay.
• File to check:
plugins/listener/src/fsm.rs(lines 127–131)
• No existing tests guard against audio glitches on rapid device switchesPlease manually verify that removing the delay doesn’t introduce audio dropouts when switching devices. If instability appears, consider reintroducing a small delay or adding automated tests to cover fast switching scenarios.
crates/audio/src/mic.rs (9)
1-11: LGTM - Comprehensive imports for audio implementation.All imports are necessary for the cpal-based audio input implementation with proper async support.
28-55: Excellent device validation with timeout protection.The
validate_devicemethod provides robust device testing with a 500ms timeout, preventing indefinite hangs when testing problematic devices. This is crucial for UX in device selection scenarios.
57-118: Comprehensive device matching strategy with good fallback behavior.The device selection logic implements multiple matching strategies:
- Exact name match
- Case-insensitive match
- Word-boundary partial match
The fallback to default device with detailed logging is excellent for debugging device issues.
135-229: Good separation of stream building by sample format.The separate functions for each sample format (f32, i16, u16) provide clean separation and type safety. The error callback logging is appropriate.
231-310: Proper error handling addresses past review feedback.The audio sample send operations now include proper error logging as suggested in previous reviews, preventing silent audio loss.
350-403: Clean thread-based stream implementation addresses past review feedback.The implementation successfully removes the unused
_stream_handlefield mentioned in past reviews and uses a dedicated thread approach. The synchronous setup before thread spawning is good practice.
410-430: Robust cleanup with timeout protection.The Drop implementation includes timeout protection to prevent indefinite blocking during cleanup, which is essential for responsive UI behavior during device switching.
432-442: Correct futures Stream implementation.The poll_next implementation properly forwards the tokio receiver's poll results, maintaining the async stream contract.
313-323: u16 normalization implementation is correct.The current code
fn normalize_u16_sample(sample: u16) -> f32 { (sample as f32 - 32768.0) / 32768.0 }follows the standard procedure for unsigned 16-bit PCM: subtract the midpoint (32768) to center around zero, then divide by 32768.0 to scale to –1.0…+1.0. No changes required.
|
@yujonglee could you help debug some issues i'm having with the selector? |
2 participants
No description provided.