-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: prevent double recording when device and macOS app active (#3244) #3537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…top (BasedHardware#3244) Signed-off-by: Rishi Jat <[email protected]>
|
@aaravgarg please have a look when you get a chance. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to prevent duplicate recordings when both a Bluetooth device and macOS system audio are active. The changes introduce guards to coordinate recording, giving system audio precedence on desktop platforms. My review identifies a critical race condition and a high-severity code duplication issue. I've provided suggestions to resolve these problems, ensuring the state management is robust and the code is more maintainable.
| if (recordingState == RecordingState.deviceRecord && _recordingDevice != null) { | ||
| debugPrint("Stopping device recording to start system audio"); | ||
| await stopStreamDeviceRecording(cleanDevice: false); | ||
| } | ||
|
|
||
| updateRecordingState(RecordingState.initialising); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces a critical race condition. During await stopStreamDeviceRecording(cleanDevice: false);, the recordingState remains RecordingState.deviceRecord. If another operation (e.g., streamDeviceRecording) is triggered during this await, it will see the old state and might start conflicting operations, leading to unpredictable behavior.
To fix this, the state should be updated to RecordingState.initialising before any await calls. This acts as a lock to prevent other operations from starting. I've suggested inlining the relevant parts of stopStreamDeviceRecording to correctly manage the state transitions without causing UI flickers or incorrect states.
| if (recordingState == RecordingState.deviceRecord && _recordingDevice != null) { | |
| debugPrint("Stopping device recording to start system audio"); | |
| await stopStreamDeviceRecording(cleanDevice: false); | |
| } | |
| updateRecordingState(RecordingState.initialising); | |
| if (recordingState == RecordingState.deviceRecord && _recordingDevice != null) { | |
| debugPrint("Stopping device recording to start system audio"); | |
| updateRecordingState(RecordingState.initialising); | |
| // Inlining `stopStreamDeviceRecording` to manage state correctly and avoid race conditions. | |
| await _cleanupCurrentState(); | |
| await _socket?.stop(reason: 'stop stream device recording'); | |
| } else { | |
| updateRecordingState(RecordingState.initialising); | |
| } |
| if (PlatformService.isDesktop && | ||
| recordingState == RecordingState.systemAudioRecord) { | ||
| debugPrint("Skipping device recording - system audio is active"); | ||
| if (device != null) _updateRecordingDevice(device); // Still track device | ||
| return; | ||
| } | ||
|
|
||
| if (device != null) _updateRecordingDevice(device); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's code duplication here. The call if (device != null) _updateRecordingDevice(device); appears both inside the new if block (line 730) and after it (line 734). This can lead to maintenance issues if one instance is updated and the other is forgotten.
To improve maintainability and remove redundancy, you can move this call to before the if block and remove the other two instances. This ensures the device is updated regardless of the recording state, which matches the current logic but in a cleaner way.
| if (PlatformService.isDesktop && | |
| recordingState == RecordingState.systemAudioRecord) { | |
| debugPrint("Skipping device recording - system audio is active"); | |
| if (device != null) _updateRecordingDevice(device); // Still track device | |
| return; | |
| } | |
| if (device != null) _updateRecordingDevice(device); | |
| if (device != null) _updateRecordingDevice(device); | |
| // Prevent device recording if system audio is active (desktop only) | |
| if (PlatformService.isDesktop && | |
| recordingState == RecordingState.systemAudioRecord) { | |
| debugPrint("Skipping device recording - system audio is active"); | |
| return; | |
| } |
|
Just a note regarding the Gemini bot review: its comments are not applicable to this provider. stopStreamDeviceRecording() is already the central, correct stop routine, and inlining cleanup as suggested would duplicate logic and break consistency with the rest of the codebase. The duplication note is valid, but the suggested patch is incorrect. The current fix keeps changes minimal and aligned with existing patterns. |
/claim #3244
Description
Fixes #3244 - Prevents duplicate recordings when both a Bluetooth device and macOS system audio are recording simultaneously.
Problem
When a Bluetooth device is connected and macOS system audio recording is started (or vice versa), both sources send audio streams to the backend, creating duplicate conversations.
Solution
Implemented priority-based recording coordination where system audio takes precedence on desktop platforms:
Both guards are scoped to desktop platforms only (
PlatformService.isDesktop), ensuring zero impact on mobile.Changes
Modified: app/lib/providers/capture_provider.dart
Fixes fix double recording when both device and macOS app is on ($100) #3244