Fix macOS Sidecar microphone duplication#641
Closed
webadderall wants to merge 2 commits into
Closed
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds label-token heuristics to identify and exclude display-route audio devices from microphone selection and applies this filter in web hooks and the native ScreenCaptureKit recorder before resolving microphone device IDs. ChangesDisplay Audio Device Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useMicrophoneDevices.ts (1)
73-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the raw labels for the permission check.
getAvailableMicrophoneDevices()now fills empty labels with a fallback name, soneedsLabelPermissioncan never become true here. On browsers that hide device labels untilgetUserMedia()runs, this will leave the hook stuck with synthetic names and skip the re-enumeration path entirely.Suggested fix
- let allDevices = await navigator.mediaDevices.enumerateDevices(); - let audioInputs = getAvailableMicrophoneDevices(allDevices); + let allDevices = await navigator.mediaDevices.enumerateDevices(); + let audioInputs = getAvailableMicrophoneDevices(allDevices); - const needsLabelPermission = - audioInputs.length > 0 && audioInputs.every((device) => !device.label.trim()); + const rawAudioInputs = allDevices.filter( + (device): device is MediaDeviceInfo => device.kind === "audioinput", + ); + const needsLabelPermission = + rawAudioInputs.length > 0 && + rawAudioInputs.every((device) => !device.label.trim());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useMicrophoneDevices.ts` around lines 73 - 83, The permission-check uses getAvailableMicrophoneDevices which replaces empty labels, so needsLabelPermission will never detect hidden labels; instead, compute needsLabelPermission directly from the raw enumerateDevices() result by filtering devices with kind === 'audioinput' and checking device.label.trim() on that raw array before calling getAvailableMicrophoneDevices; keep the existing flow using hasRequestedMicrophoneLabels and permissionStream and only call getUserMedia() / re-enumerate if the raw-label check indicates permission is needed.src/hooks/useScreenRecorder.ts (1)
1523-1544:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the sanitized mic id in the fallback path too.
This only fixes the ID for
startNativeScreenRecording(). IfmicrophoneFallbackRequiredis returned later, the browser fallback still callscreateProcessedMicrophoneConstraints(microphoneDeviceId, ...)with the stale saved selection, so a filtered Sidecar/display device can still break microphone capture on that branch.Suggested fix
- let nativeMicrophoneDeviceId = microphoneDeviceId; + let resolvedMicrophoneDeviceId = microphoneDeviceId; if (microphoneEnabled) { try { const devices = await navigator.mediaDevices.enumerateDevices(); const availableMicrophones = getAvailableMicrophoneDevices(devices); const mic = availableMicrophones.find( (device) => device.deviceId === microphoneDeviceId, ); - nativeMicrophoneDeviceId = mic?.deviceId; + resolvedMicrophoneDeviceId = mic?.deviceId; micLabel = mic?.label || undefined; } catch { // Fall through — native process will use the default mic } } @@ - microphoneDeviceId: nativeMicrophoneDeviceId, + microphoneDeviceId: resolvedMicrophoneDeviceId, microphoneLabel: micLabel, }, );And then use
resolvedMicrophoneDeviceIdwhen creating the browser fallback constraints as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useScreenRecorder.ts` around lines 1523 - 1544, The sanitized microphone device id obtained for native recording isn't reused for the browser fallback path; update the logic in useScreenRecorder.ts so the resolved/sanitized id (e.g., nativeMicrophoneDeviceId or a new resolvedMicrophoneDeviceId) that you set before calling window.electronAPI.startNativeScreenRecording is also passed into createProcessedMicrophoneConstraints(...) when handling microphoneFallbackRequired, and use the same micLabel variable for consistency; ensure createProcessedMicrophoneConstraints and the microphoneFallbackRequired branch reference that resolved id instead of the original microphoneDeviceId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/hooks/useMicrophoneDevices.ts`:
- Around line 73-83: The permission-check uses getAvailableMicrophoneDevices
which replaces empty labels, so needsLabelPermission will never detect hidden
labels; instead, compute needsLabelPermission directly from the raw
enumerateDevices() result by filtering devices with kind === 'audioinput' and
checking device.label.trim() on that raw array before calling
getAvailableMicrophoneDevices; keep the existing flow using
hasRequestedMicrophoneLabels and permissionStream and only call getUserMedia() /
re-enumerate if the raw-label check indicates permission is needed.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 1523-1544: The sanitized microphone device id obtained for native
recording isn't reused for the browser fallback path; update the logic in
useScreenRecorder.ts so the resolved/sanitized id (e.g.,
nativeMicrophoneDeviceId or a new resolvedMicrophoneDeviceId) that you set
before calling window.electronAPI.startNativeScreenRecording is also passed into
createProcessedMicrophoneConstraints(...) when handling
microphoneFallbackRequired, and use the same micLabel variable for consistency;
ensure createProcessedMicrophoneConstraints and the microphoneFallbackRequired
branch reference that resolved id instead of the original microphoneDeviceId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4153d067-8fa4-4389-897f-fdbe3542c0c6
📒 Files selected for processing (6)
electron/native/ScreenCaptureKitRecorder.swiftelectron/native/bin/darwin-arm64/recordly-screencapturekit-helperelectron/native/bin/darwin-x64/recordly-screencapturekit-helpersrc/hooks/useMicrophoneDevices.test.tssrc/hooks/useMicrophoneDevices.tssrc/hooks/useScreenRecorder.ts
Collaborator
Author
|
Closing this for now while we re-evaluate the root cause. The current patch targets bad microphone device selection, but the more likely issue is simultaneous active audio capture paths on macOS. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests