Add shortcut to toggle audio recording and REST API #1114
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces comprehensive audio device management capabilities across the Screenpipe application. The changes span multiple components, including the Tauri app, server, and audio processing modules. Key additions include global shortcuts for starting and stopping audio recording, new API endpoints for audio device control, and a shift from Changes
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
screenpipe-server/src/bin/screenpipe-server.rs (1)
368-368: Consider renamingsender_clonefor clarityThe variable
sender_cloneis now aDashMapdue to the change fromSegQueuetoDashMap. Renaming it to something likedevice_control_mapwould improve code readability and accurately reflect its purpose.Apply this diff to rename the variable:
-let sender_clone = audio_devices_control.clone(); +let device_control_map = audio_devices_control.clone();screenpipe-app-tauri/lib/api/index.ts (1)
33-33: Consistent use of double quotes in constructorChanging the
baseUrldefault value to use double quotes ensures consistency with common TypeScript conventions.Apply this diff for consistency:
-constructor(baseUrl: string = 'http://localhost:3030') { +constructor(baseUrl: string = "http://localhost:3030") {screenpipe-audio/src/bin/screenpipe-audio-forever.rs (1)
98-98: Consider enhancing error handling for the new parameter.While adding the new optional parameter is correct, consider adding explicit error handling for cases where device control might be required but not provided.
- None, + None.map(|ctrl| { + // Handle potential device control initialization errors + ctrl.map_err(|e| anyhow!("Failed to initialize device control: {}", e)) + }).transpose()?,screenpipe-app-tauri/app/page.tsx (2)
43-47: Consider caching audio devices to prevent frequent store access.The
getAudioDevicesfunction queries the store on every invocation. Consider caching the devices or using a React ref to optimize performance.+ const audioDevicesRef = React.useRef<string[]>([]); const getAudioDevices = async () => { + if (audioDevicesRef.current.length > 0) { + return audioDevicesRef.current; + } const store = await getStore(); const devices = (await store.get("audioDevices")) as string[]; + audioDevicesRef.current = devices; return devices; };
105-105: Remove or enhance console.log statement.The console.log statement should either be removed or enhanced with additional context for debugging purposes.
- console.log("audio-devices", devices); + console.debug("[Audio Shortcuts] Available devices:", devices);screenpipe-audio/src/stt.rs (1)
207-216: Device state check implementation looks robust.The implementation properly handles both active and missing device cases with appropriate debug logging. The early continue statements prevent unnecessary processing.
Consider adding metrics or structured logging to track skipped audio processing for better observability.
screenpipe-server/src/core.rs (1)
35-35: Great architectural improvement using DashMap!The transition from SegQueue to DashMap enhances concurrent device state management:
- Better support for multiple readers and writers
- More efficient state lookups
- Improved thread safety with built-in synchronization
Also applies to: 281-281
screenpipe-app-tauri/src-tauri/src/main.rs (1)
279-307: Consider extracting store updates to a reusable function.The audio shortcut registration is well-implemented, but the store update logic could be refactored for better reusability:
+fn update_audio_state(app: &AppHandle, enabled: bool) -> Result<(), String> { + let store = get_store(app, None).map_err(|e| e.to_string())?; + store.set("disableAudio", !enabled); + store.save().map_err(|e| e.to_string()) +} register_shortcut( app, &config.start_audio, config.is_disabled("start_audio"), |app| { - let store = get_store(app, None).unwrap(); - store.set("disableAudio", false); - store.save().unwrap(); + let _ = update_audio_state(app, true); let _ = app.emit("shortcut-start-audio", ()); }, )screenpipe-server/src/server.rs (1)
1624-1698: Consider extracting device validation to reduce duplication.The handlers are well-implemented but contain duplicated device validation logic. Consider refactoring:
+async fn validate_audio_device(device: &AudioDevice) -> Result<(), (StatusCode, JsonResponse<Value>)> { + let available_devices = list_audio_devices().await.map_err(|e| { + (StatusCode::INTERNAL_SERVER_ERROR, + JsonResponse(json!({ + "error": format!("failed to list audio devices: {}", e), + "success": false + }))) + })?; + + if !available_devices.contains(device) { + return Err((StatusCode::BAD_REQUEST, + JsonResponse(json!({ + "error": format!("device not found: {}", device.name), + "success": false + })))); + } + Ok(()) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Cargo.toml(1 hunks)screenpipe-app-tauri/app/page.tsx(3 hunks)screenpipe-app-tauri/components/settings/shortcut-row.tsx(3 hunks)screenpipe-app-tauri/components/settings/shortcut-section.tsx(1 hunks)screenpipe-app-tauri/lib/api/index.ts(2 hunks)screenpipe-app-tauri/lib/hooks/use-settings.tsx(3 hunks)screenpipe-app-tauri/src-tauri/src/main.rs(4 hunks)screenpipe-audio/Cargo.toml(1 hunks)screenpipe-audio/benches/record_and_transcribe_benchmark.rs(1 hunks)screenpipe-audio/src/bin/screenpipe-audio-forever.rs(1 hunks)screenpipe-audio/src/bin/screenpipe-audio.rs(1 hunks)screenpipe-audio/src/core.rs(1 hunks)screenpipe-audio/src/stt.rs(4 hunks)screenpipe-audio/tests/core_tests.rs(1 hunks)screenpipe-server/Cargo.toml(1 hunks)screenpipe-server/src/bin/screenpipe-server.rs(7 hunks)screenpipe-server/src/core.rs(6 hunks)screenpipe-server/src/server.rs(12 hunks)
🔇 Additional comments (26)
screenpipe-server/src/bin/screenpipe-server.rs (7)
39-39: DashMap imported for concurrent device control managementThe addition of
use dashmap::DashMap;correctly incorporatesDashMapfor thread-safe, concurrent access to audio device control states.
306-307: Switch fromSegQueuetoDashMapfor device controlReplacing
SegQueuewithDashMapallows for concurrent reads and writes to the audio device control map without explicit locking, enhancing performance and safety.
464-466: Cloning variables for scoped usage is appropriateCloning
vad_engineandrealtime_transcription_senderensures thread-safe access within the asynchronous task.
471-474: Passing clonedaudio_devices_control_recordingto recording functionPassing the cloned
audio_devices_control_recordingtostart_continuous_recordingaligns with the updated concurrency model.
493-493: Includingrealtime_transcription_sender_clonein parametersPassing
Arc::new(realtime_transcription_sender_clone)ensures that the real-time transcription sender is correctly shared across tasks.
533-535: Initialize broadcast channel for audio device updatesThe introduction of
audio_devices_txand cloning it for use in different parts of the application facilitates communication of audio device state changes.
552-572: Handle dynamic audio device state changes in spawned taskThe asynchronous task correctly listens to the broadcast channel and updates
audio_devices_control_for_spawnaccordingly. Error handling for unavailable devices and logging enhances reliability.screenpipe-audio/benches/record_and_transcribe_benchmark.rs (1)
28-28: Updatecreate_whisper_channelcall with new optional parameterAdding
Noneas the new argument aligns with the updated signature ofcreate_whisper_channel, ensuring the function is called correctly with all required parameters.screenpipe-app-tauri/lib/api/index.ts (4)
45-45: Properly handle API response success flagsThrowing an error when
data.successisfalseinlistPipesmethod ensures that failures are correctly propagated to the caller.
49-52: Log and rethrow errors inlistPipesmethodAdding error logging and rethrowing in the
catchblock aids in debugging and ensures that errors aren't silently swallowed.
54-80: ImplementstartAudiomethod with robust error handlingThe new
startAudiomethod correctly sends a POST request to the server and includes comprehensive error handling for both network and API-level errors.
81-104: 🛠️ Refactor suggestion
⚠️ Potential issueImprove device type detection in
stopAudiomethodThe current logic for determining the device type may fail if the device name does not include "(input)" or "(output)", or if these strings appear in the device name unintentionally. This could lead to incorrect device control commands being sent to the server.
Apply this diff to enhance the reliability of device type determination:
-const type = deviceName.includes("(input)") ? "Input" : "Output"; -const name = deviceName.replaceAll("(input)", "").replaceAll("(output)", "").trim(); +// Assume deviceName is an object with properties 'name' and 'type' +// Pass deviceName as { name: string, type: 'Input' | 'Output' } +const { name, type } = deviceName;Alternatively, ensure that the device type is provided explicitly when calling
stopAudio, and adjust the method signature accordingly.Likely invalid or redundant comment.
screenpipe-app-tauri/components/settings/shortcut-section.tsx (1)
54-69: LGTM! Audio shortcut components follow established patterns.The new ShortcutRow components for audio controls are well-integrated and maintain consistency with existing shortcut implementations.
screenpipe-audio/src/bin/screenpipe-audio.rs (1)
102-102: LGTM! Consistent implementation with robust error handling.The None parameter addition aligns with other files, and the existing timeout handling provides good reliability.
screenpipe-app-tauri/components/settings/shortcut-row.tsx (2)
92-93: LGTM! Audio shortcut integration looks good.The audio shortcut fields are properly integrated into the shortcut synchronization logic, maintaining consistency with the existing shortcut system.
Also applies to: 101-102, 112-113
179-179: Good addition of error logging.Error logging enhancement improves debugging capabilities for shortcut-related issues.
screenpipe-app-tauri/lib/hooks/use-settings.tsx (1)
90-91: Audio shortcut fields properly typed and initialized.The new audio shortcut fields are correctly added to the Settings type and initialized with empty strings in DEFAULT_SETTINGS.
Also applies to: 147-148
screenpipe-audio/src/stt.rs (1)
159-159: Verify thread safety of DashMap usage.The addition of
Arc<DashMap<AudioDevice, DeviceControl>>is a good choice for concurrent device state management. However, let's verify there are no potential deadlocks or race conditions in the codebase.Run this script to check for potential race conditions:
✅ Verification successful
DashMap usage is thread-safe and well-implemented
The implementation follows concurrent programming best practices:
- Thread-safe sharing through Arc wrapper
- Atomic operations using DashMap's internal synchronization
- Clean state management via broadcast channels
- No potential deadlocks or race conditions detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential race conditions in device control access # Look for places where DashMap is accessed without proper synchronization # Search for DashMap usage patterns rg -A 5 "DashMap|DeviceControl" # Search for potential race conditions in async contexts ast-grep --pattern 'async fn $name($params) { $$$ $map.insert($key, $value) $$$ await $$$ $map.remove($key) $$$ }'Length of output: 14091
screenpipe-audio/src/core.rs (1)
Line range hint
53-57: LGTM! Good addition of the Debug trait.Adding the Debug trait to DeviceControl struct enhances debugging capabilities, making it easier to inspect device states during development and troubleshooting.
screenpipe-server/src/core.rs (1)
295-304: Well-implemented device state management!The changes effectively handle device lifecycle:
- Proper iteration over active devices
- Skip already handled devices to prevent duplicates
- Clean removal of stopped devices
Also applies to: 314-315
screenpipe-app-tauri/src-tauri/src/main.rs (2)
79-80: LGTM! Good extension of shortcut configuration.The new audio shortcut fields maintain consistency with existing shortcut patterns.
143-150: Well-implemented shortcut initialization!The initialization follows the established pattern and properly handles defaults.
screenpipe-server/src/server.rs (2)
1610-1621: LGTM! Well-designed request/response structs.The structs are well-designed with:
- Proper serialization attributes
- Optional fields with defaults
- Clear and meaningful field names
1742-1743: LGTM! Well-structured API routes.The new audio control routes follow REST conventions and maintain consistency with existing endpoints.
Cargo.toml (1)
39-39: LGTM! Verify version compatibility.The addition of
dashmapas a workspace dependency is appropriate for concurrent device state management. Version 6.1.0 is stable and well-tested.Run this script to check for any newer versions with important updates:
✅ Verification successful
Version 6.1.0 is optimal ✓
The PR uses the latest stable version of dashmap, which is not yanked and actively maintained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check latest dashmap versions and changelogs curl -s https://crates.io/api/v1/crates/dashmap | jq -r '.versions[0] | {num:.num, yanked:.yanked}'Length of output: 141
screenpipe-audio/Cargo.toml (1)
68-68: LGTM! Consistent workspace dependency.The addition of
dashmapusing workspace inheritance ensures version consistency across the project.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
screenpipe-app-tauri/lib/api/index.ts (1)
83-110: Reduce code duplication in audio control methods.The
stopAudiomethod duplicates most of the logic fromstartAudio. Consider extracting the common functionality into a private helper method.Here's a suggested refactor:
+ private async controlAudio(deviceName: string, action: "start" | "stop"): Promise<void> { + if (!deviceName?.trim()) { + throw new Error("Device name cannot be empty"); + } + if (!deviceName.includes("(input)") && !deviceName.includes("(output)")) { + throw new Error("Invalid device name format. Must include '(input)' or '(output)'"); + } + try { + const type = deviceName.includes("(input)") ? "Input" : "Output"; + const name = deviceName.replaceAll("(input)", "").replaceAll("(output)", "").trim(); + const response = await fetch(`${this.baseUrl}/audio/${action}`, { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ + device_name: name, + device_type: type, + }), + }); + + if (!response.ok) { + throw new Error(`failed to ${action} audio: ${response.statusText}`); + } + + const data = await response.json(); + if (!data.success) { + throw new Error(`failed to ${action} audio: ${data.message}`); + } + } catch (error) { + console.error(`error ${action}ing audio:`, error); + throw error; + } + } + + async startAudio(deviceName: string): Promise<void> { + return this.controlAudio(deviceName, "start"); + } + + async stopAudio(deviceName: string): Promise<void> { + return this.controlAudio(deviceName, "stop"); + }screenpipe-server/src/bin/screenpipe-server.rs (2)
368-368: Consider parameterizing the sleep duration before inserting intosender_clone.The hardcoded 15-second delay might benefit from being configurable or documented to clarify its purpose, improving maintainability.
558-578: Handle potential errors fromrx.recv().awaitto ensure task reliability.If all senders are dropped,
rx.recv().awaitwill returnErr(RecvError::Closed), causing the loop to exit. Consider handling the error to maintain the task or explicitly break if intended.screenpipe-server/src/core.rs (1)
20-20: Remove unused import:HashSet.The
HashSetimport is unused and can be safely removed to clean up the codebase.Apply this diff to remove the unused import:
-use std::collections::{HashMap, HashSet}; +use std::collections::HashMap;🧰 Tools
🪛 GitHub Check: test-ubuntu
[warning] 20-20:
unused import:HashSet🪛 GitHub Check: test-linux
[warning] 20-20:
unused import:HashSet🪛 GitHub Check: test-macos
[warning] 20-20:
unused import:HashSetscreenpipe-server/src/server.rs (4)
1630-1667: Handle the result ofstate.audio_devices_tx.sendinstart_audio_device.The
sendmethod can return an error if there are no active subscribers or if the channel is full. Consider handling or logging the error to ensure that control messages are sent successfully.Apply this diff to handle the potential error:
- let _ = state.audio_devices_tx.send((device.clone(), control)); + if let Err(e) = state.audio_devices_tx.send((device.clone(), control)) { + error!("Failed to send control message: {}", e); + }
1670-1704: Handle the result ofstate.audio_devices_tx.sendinstop_audio_device.Similar to
start_audio_device, handle or log any errors returned by thesendmethod to ensure reliability.Apply this diff to handle the potential error:
- let _ = state.audio_devices_tx.send((device.clone(), DeviceControl { + if let Err(e) = state.audio_devices_tx.send((device.clone(), DeviceControl { is_running: false, is_paused: false, - })); + })) { + error!("Failed to send control message: {}", e); + }
1630-1667: Consider adding unit tests forstart_audio_devicehandler.Adding unit tests ensures the new API endpoint behaves as expected and helps prevent future regressions.
Would you like assistance in creating unit tests for this handler?
1670-1704: Consider adding unit tests forstop_audio_devicehandler.Unit tests will help verify the correctness of the
stop_audio_devicefunction and enhance code reliability.Do you want help in writing unit tests for this handler?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Cargo.toml(1 hunks)screenpipe-app-tauri/lib/api/index.ts(2 hunks)screenpipe-server/src/bin/screenpipe-server.rs(6 hunks)screenpipe-server/src/core.rs(6 hunks)screenpipe-server/src/server.rs(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🪛 GitHub Check: test-ubuntu
screenpipe-server/src/core.rs
[warning] 20-20:
unused import: HashSet
🪛 GitHub Check: test-linux
screenpipe-server/src/core.rs
[warning] 20-20:
unused import: HashSet
🪛 GitHub Check: test-macos
screenpipe-server/src/core.rs
[warning] 20-20:
unused import: HashSet
🪛 GitHub Actions: Linux Integration Test
screenpipe-server/src/server.rs
[error] 35-35: The name list_monitors is defined multiple times
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-windows
- GitHub Check: test-windows
🔇 Additional comments (18)
screenpipe-app-tauri/lib/api/index.ts (2)
45-45: LGTM! Enhanced error handling.The improved error handling with detailed logging will help with debugging issues.
Also applies to: 49-51
54-81: 🛠️ Refactor suggestionEnhance robustness and documentation of audio device handling.
Several improvements could make this method more robust and maintainable:
- The device type determination is fragile and relies on string patterns.
- The method lacks input validation and documentation.
Consider these improvements:
+ /** + * Starts audio recording/playback for the specified device. + * @param deviceName - The name of the audio device (must include "(input)" or "(output)") + * @throws {Error} If the device name is invalid or the API request fails + */ async startAudio(deviceName: string): Promise<void> { + if (!deviceName?.trim()) { + throw new Error("Device name cannot be empty"); + } + if (!deviceName.includes("(input)") && !deviceName.includes("(output)")) { + throw new Error("Invalid device name format. Must include '(input)' or '(output)'"); + } try { const type = deviceName.includes("(input)") ? "Input" : "Output"; const name = deviceName.replaceAll("(input)", "").replaceAll("(output)", "").trim();Let's verify the device name format assumptions across the codebase:
✅ Verification successful
Input validation for audio device names is essential and aligns with codebase patterns.
The review comment's concerns about device name format validation are valid. The codebase consistently uses "(input)" and "(output)" suffixes for device names across multiple components, making the suggested input validation and documentation crucial for maintaining this convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for patterns of device name usage to verify the format assumptions rg -A 2 "includes\(['\"].*input|output.*['\"]\)"Length of output: 15965
screenpipe-server/src/bin/screenpipe-server.rs (4)
39-39: LGTM!The import of
dashmap::DashMapis necessary for the updated audio device control implementation.
306-307: LGTM!Initialization of
audio_devices_controlwithDashMapenhances concurrent access and is correctly cloned for usage in different contexts.
470-478: LGTM!The parameters for
start_continuous_recordinghave been correctly updated to align with the newDashMapimplementation for audio device control.
539-541: LGTM!Creation of the
audio_devices_txbroadcast channel and its cloning are properly implemented for transmitting audio device control messages.screenpipe-server/src/core.rs (5)
6-6: LGTM!Importing
dashmap::DashMapis necessary for the new concurrent audio device control mechanism.
36-36: LGTM!The function signature of
start_continuous_recordinghas been appropriately updated to useDashMapfor audio device control.
124-124: LGTM!Passing
audio_devices_controltocreate_whisper_channelensures consistent audio device management across components.
297-297: LGTM!The updated
record_audiofunction signature correctly reflects the newDashMapusage.
311-331: LGTM!Iterating over
audio_devices_controlusingDashMapentries is appropriate for concurrent device state management.screenpipe-server/src/server.rs (7)
50-50: LGTM!Importing
tokio::sync::broadcastis necessary for implementing the audio device control broadcast channel.
65-65: LGTM!Updating
AppStatewithaudio_devices_txaligns with the new broadcast mechanism for audio device control.
804-804: LGTM!The
Serverstruct's inclusion ofaudio_devices_txis consistent with the updated architecture.
821-821: LGTM!Constructor parameters for
Server::newhave been appropriately updated.
835-835: LGTM!Initialization of
Serverwith the newaudio_devices_txaligns with the broadcast-based design.
859-859: LGTM!Passing
audio_devices_txintoAppStateensures that audio device control messages are managed correctly.
1615-1622: LGTM!The
AudioDeviceControlRequeststruct is properly defined to handle audio device control requests.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
screenpipe-server/src/bin/screenpipe-server.rs (2)
306-307: Consider adding a capacity hint to DashMap initialization.Since the number of audio devices is known in advance, you can optimize the
DashMapinitialization by providing an initial capacity hint to avoid reallocation.- let audio_devices_control = Arc::new(DashMap::new()); + let audio_devices_control = Arc::new(DashMap::with_capacity(all_audio_devices.len()));
539-541: Consider the broadcast channel capacity.The broadcast channel capacity of 100 might be insufficient for high-frequency device state changes, potentially leading to message drops. Consider:
- Making the capacity configurable
- Implementing a backpressure mechanism
- Logging when messages are dropped
screenpipe-server/src/server.rs (1)
1614-1626: Add documentation and validation for API structs.The request/response structs would benefit from:
- Documentation comments explaining their purpose and usage
- Validation attributes for the
device_namefield- Example usage in doc comments
+/// Request payload for controlling audio devices. +/// +/// # Example +/// ``` +/// let request = AudioDeviceControlRequest { +/// device_name: "Microphone".to_string(), +/// device_type: Some(DeviceType::Input), +/// }; +/// ``` #[derive(Deserialize)] pub struct AudioDeviceControlRequest { + #[serde(validate(length(min = 1)))] device_name: String, #[serde(default)] device_type: Option<DeviceType>, } +/// Response payload for audio device control operations. #[derive(Serialize)] pub struct AudioDeviceControlResponse { success: bool, message: String, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
screenpipe-app-tauri/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
screenpipe-server/src/bin/screenpipe-server.rs(6 hunks)screenpipe-server/src/server.rs(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-windows
- GitHub Check: test-ubuntu
- GitHub Check: test-windows
- GitHub Check: test-macos
- GitHub Check: test-linux
🔇 Additional comments (2)
screenpipe-server/src/bin/screenpipe-server.rs (2)
39-40: LGTM!The import of
dashmap::DashMapis correctly placed and aligns with the architectural change fromSegQueuetoDashMapfor concurrent device state management.
368-368: Review the 15-second delay before device control insertion.The arbitrary 15-second delay before inserting the device control state could cause issues:
- Users might attempt to control devices during this delay window
- The delay doesn't account for device initialization time variations
- No error handling if the device isn't ready after the delay
Consider either removing the delay or implementing a proper device readiness check.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
screenpipe-app-tauri/app/page.tsx (1)
126-145:⚠️ Potential issueAdd consistent error handling and debouncing to stop operation.
The stop operation needs the same safeguards as the start operation:
- Missing debouncing with
isProcessingRef- Inconsistent Promise handling (using forEach instead of Promise.all)
- No cleanup in finally block
listen("shortcut-stop-audio", async (event) => { + if (isProcessingRef.current) return; + isProcessingRef.current = true; try { const devices = await getAudioDevices(); const pipeApi = new PipeApi(); - devices.forEach((device) => { - pipeApi.stopAudio(device); - }); + await Promise.all(devices.map(device => pipeApi.stopAudio(device))); toast({ title: "audio stopped", description: "audio has been stopped", }); } catch (error) { console.error("error stopping audio:", error); toast({ title: "error stopping audio", description: error instanceof Error ? error.message : "unknown error occurred", variant: "destructive", }); + } finally { + isProcessingRef.current = false; } }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
screenpipe-app-tauri/app/page.tsx(3 hunks)screenpipe-server/src/core.rs(6 hunks)screenpipe-server/src/server.rs(13 hunks)screenpipe-server/tests/endpoint_test.rs(1 hunks)screenpipe-server/tests/tags_test.rs(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
screenpipe-app-tauri/app/page.tsx
[error] 101-101: Expected a property, an expression, or a method but instead found 'const isProcessingRef = React.useRef'.
Expected a property, an expression, or a method here.
(parse)
[error] 101-101: expected , but instead found (
Remove (
(parse)
[error] 101-101: expected , but instead found ;
Remove ;
(parse)
[error] 102-102: expected , but instead found (
Remove (
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-macos
- GitHub Check: test-ubuntu
- GitHub Check: test-windows
- GitHub Check: test-linux
- GitHub Check: test-windows
🔇 Additional comments (12)
screenpipe-app-tauri/app/page.tsx (4)
4-4: LGTM!The
getStoreimport is appropriately placed alongside the relateduseSettingsimport.
87-99: LGTM!The formatting changes improve code readability while maintaining the same functionality.
101-124: LGTM on the start audio implementation.The implementation includes proper debouncing and error handling with the
isProcessingRef.🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Expected a property, an expression, or a method but instead found 'const isProcessingRef = React.useRef'.
Expected a property, an expression, or a method here.
(parse)
[error] 101-101: expected
,but instead found(Remove (
(parse)
[error] 101-101: expected
,but instead found;Remove ;
(parse)
[error] 102-102: expected
,but instead found(Remove (
(parse)
101-145: Consider extracting shared audio operation logic.Both start and stop operations share similar patterns. Consider extracting the common logic into a shared function to improve maintainability.
+const handleAudioOperation = async ( + operation: 'start' | 'stop', + handler: (device: string) => Promise<void> +) => { + if (isProcessingRef.current) return; + isProcessingRef.current = true; + try { + const devices = await getAudioDevices(); + const pipeApi = new PipeApi(); + await Promise.all(devices.map(handler)); + toast({ + title: `audio ${operation}ed`, + description: `audio has been ${operation}ed`, + }); + } catch (error) { + console.error(`error ${operation}ing audio:`, error); + toast({ + title: `error ${operation}ing audio`, + description: error instanceof Error ? error.message : "unknown error occurred", + variant: "destructive", + }); + } finally { + isProcessingRef.current = false; + } +}; -listen("shortcut-start-audio", async () => { ... }), -listen("shortcut-stop-audio", async () => { ... }), +listen("shortcut-start-audio", () => + handleAudioOperation('start', device => pipeApi.startAudio(device)) +), +listen("shortcut-stop-audio", () => + handleAudioOperation('stop', device => pipeApi.stopAudio(device)) +),🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Expected a property, an expression, or a method but instead found 'const isProcessingRef = React.useRef'.
Expected a property, an expression, or a method here.
(parse)
[error] 101-101: expected
,but instead found(Remove (
(parse)
[error] 101-101: expected
,but instead found;Remove ;
(parse)
[error] 102-102: expected
,but instead found(Remove (
(parse)
screenpipe-server/tests/tags_test.rs (1)
34-34: LGTM! Channel size is appropriate for testing.The transition to broadcast channel with a buffer size of 1000 is suitable for test scenarios.
screenpipe-server/tests/endpoint_test.rs (1)
37-37: LGTM! Consistent with tags_test.rs implementation.The broadcast channel initialization maintains consistency across test files.
screenpipe-server/src/core.rs (3)
5-5: LGTM! DashMap is appropriate for concurrent device state management.Using DashMap provides better concurrent access patterns and state management capabilities compared to SegQueue.
35-35: LGTM! Parameter type update aligns with architectural changes.The transition to DashMap for audio_devices_control enhances concurrent device state management.
123-123: LGTM! Proper integration with whisper channel.The whisper channel correctly receives the audio_devices_control reference.
screenpipe-server/src/server.rs (3)
1614-1625: LGTM! Well-structured request/response types.The types are properly defined with appropriate optional fields and serialization attributes.
1796-1797: 🛠️ Refactor suggestionAdd rate limiting and authentication to audio endpoints.
The audio control endpoints need protection against abuse:
- Rate limiting to prevent DoS attacks
- Authentication to restrict access
- Request validation middleware
- .route("/audio/start", post(start_audio_device)) - .route("/audio/stop", post(stop_audio_device)) + .route("/audio/start", post(start_audio_device)) + .layer(RateLimitLayer::new(10, Duration::from_secs(60))) + .layer(AuthenticationLayer::new()) + .route("/audio/stop", post(stop_audio_device)) + .layer(RateLimitLayer::new(10, Duration::from_secs(60))) + .layer(AuthenticationLayer::new())Likely invalid or redundant comment.
1627-1665: 🛠️ Refactor suggestionImprove error handling and state validation.
The start handler needs improvements:
- Validate current device state before starting
- Handle broadcast send failures
- Update devices_status map
async fn start_audio_device( State(state): State<Arc<AppState>>, Json(payload): Json<AudioDeviceControlRequest>, ) -> Result<JsonResponse<AudioDeviceControlResponse>, (StatusCode, JsonResponse<Value>)> { let device = AudioDevice { name: payload.device_name.clone(), device_type: payload.device_type.unwrap_or(DeviceType::Input), }; + // Check if device is already running + if let Some(current_state) = state.devices_status.get(&device) { + if current_state.is_running { + return Ok(JsonResponse(AudioDeviceControlResponse { + success: true, + message: format!("device already running: {}", device.name), + })); + } + } let control = DeviceControl { is_running: true, is_paused: false }; - let _ = state.audio_devices_tx.send((device.clone(), control)); + if let Err(e) = state.audio_devices_tx.send((device.clone(), control.clone())) { + error!("Failed to send device control message: {}", e); + return Err((StatusCode::INTERNAL_SERVER_ERROR, + JsonResponse(json!({ + "error": "failed to start device", + "success": false + })))); + } + + // Update devices_status + state.devices_status.insert(device.clone(), control);Likely invalid or redundant comment.
neo773
changed the title
| @@ -155,6 +156,7 @@ pub async fn create_whisper_channel( | |||
| output_path: &PathBuf, | |||
| vad_sensitivity: VadSensitivity, | |||
| languages: Vec<Language>, | |||
| audio_devices_control: Option<Arc<DashMap<AudioDevice, DeviceControl>>>, | |||
There was a problem hiding this comment.
why is audio device control has to change speech to text code instead of audio device recording
There was a problem hiding this comment.
also does it work well with the audio streaming api
if i stop a device
There was a problem hiding this comment.
Yes, start_realtime_recording() gets checked by parent block if device is running.
|
will test this soon, questions:
|
Yeah, it's granular you can stop/start specific devices.
No, current logic checks for both audio and video I think for it to turn red. |
Screen.Recording.2025-01-10.at.10.16.39.AM.movissues i noticed, not sure if related to this PR:
|
|
testing agai now |
|
seem to work\ one issue which seem only dev i think is stop button does not work sometimes, when starting the app with bun tauri dev would it be possible to add in the status some kind of green/red or thing saying which devices are being recorded (monitor and audio deviec)? |
I think it should be doable, the entire modal needs a redesigned UI/UX imo, current one looks very MVPish/Alpha. As for stop button I'm not sure, the code diff doesn't really delve into that parts. it's happened to me before too (before this PR). It's also hard to reproduce |
|
/approve |
|
@louis030195: The claim has been successfully added to reward-all. You can visit your dashboard to complete the payment. |
/closes #1065
/claim #1065
a.mp4
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates
dashmapdependency across multiple packages.