fix(recording): start webcam recorder after native macOS capture begins#85
Conversation
On the native macOS path the webcam MediaRecorder was started while the ScreenCaptureKit helper was still spinning up (permission checks, stream construction, first-frame latency — ~1s). The webcam sidecar therefore contained ~1s of pre-roll footage from before the screen/mic recording began, and since the editor and exporter align both files at t=0, the webcam PiP lagged the voice/screen by that startup latency in both preview and export. Start the webcam recorder only after startNativeMacRecording resolves, which happens when the helper emits recording-started (first video frame written), so both recordings share the same time origin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe native macOS recording startup flow handles missing webcam streams, discards recordings when countdowns become inactive, and creates the webcam recorder after the native recording-started event. ChangesNative recording synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
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 (1)
src/hooks/useScreenRecorder.ts (1)
1014-1051: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNative macOS recording leaks if webcam recorder creation throws.
startNativeMacRecordingresolves at line 1014, butcreateRecorderHandleat line 1029 runs afterward. IfcreateRecorderHandlethrows (e.g.,MediaRecorderconstructor orrecorder.start()fails), the catch block at line 1049 logs and rethrows without stopping the now-running native recording. SincenativeMacRecording.currentis never assigned (line 1036 is unreachable), no cleanup path — the unmount effect (line 716),cancelRecording, orstartRecording's catch — will find or stop the orphaned native recording.🔒 Proposed fix: stop native recording in the catch block if it was started
const result = await window.electronAPI.startNativeMacRecording(request); if (!result.success || !result.recordingId) { throw new Error(result.error ?? "Native macOS capture failed."); } + const nativeRecordingStarted = true; if (!isCountdownRunActive(countdownRunToken)) { await window.electronAPI.stopNativeMacRecording(true); return true; } // Start the webcam recorder only after the helper has emitted // recording-started (startNativeMacRecording resolves on that event). // Starting it earlier bakes the helper's capture startup latency // (~1s of pre-roll) into the webcam file, desyncing it from the // screen/mic recording that the editor and exporter align at t=0. if (webcamEnabled && webcamStream.current) { nativeWebcamRecorder = createRecorderHandle(webcamStream.current, { mimeType: selectMimeType(), videoBitsPerSecond: BITRATE_BASE, }); } recordingId.current = result.recordingId; nativeMacRecording.current = { recordingId: result.recordingId, finalizing: false, paused: false, }; webcamRecorder.current = nativeWebcamRecorder; accumulatedDurationMs.current = 0; segmentStartedAt.current = Date.now(); allowAutoFinalize.current = true; setRecording(true); setPaused(false); setElapsedSeconds(0); return true; } catch (error) { console.error("Native macOS capture failed:", error); + if (nativeRecordingStarted) { + await window.electronAPI.stopNativeMacRecording(true).catch(() => undefined); + } throw error; }Note:
nativeRecordingStartedshould be declared aslet nativeRecordingStarted = false;before thetryblock and set totrueafter the success check.🤖 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 1014 - 1051, Track whether native recording started in the recording startup flow by declaring nativeRecordingStarted before the try block and setting it true after the startNativeMacRecording success check. In the catch block, stop the native recording with stopNativeMacRecording(true) when that flag is set, then log and rethrow the original error.
🧹 Nitpick comments (1)
src/hooks/useScreenRecorder.ts (1)
946-960: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
waitForWebcamReadyinstead of duplicating its logic.The inline webcam-ready wait at lines 948-959 is identical to the
waitForWebcamReadyhelper at lines 779-796. The Windows path already calls the helper (line 830). Extracting this reduces duplication across three call sites (macOS, Windows, browser capture at lines 1261-1272).♻️ Proposed refactor
if (webcamEnabled) { - if (!webcamReady.current) { - await new Promise<void>((resolve) => { - const interval = setInterval(() => { - if (webcamReady.current) { - clearInterval(interval); - resolve(); - } - }, 50); - setTimeout(() => { - clearInterval(interval); - resolve(); - }, 5000); - }); - } + await waitForWebcamReady(); if (!isCountdownRunActive(countdownRunToken)) { return true; }🤖 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 946 - 960, Replace the inline webcam-ready polling block in the webcamEnabled recording flow with the existing waitForWebcamReady helper, preserving the current conditional check and timeout behavior provided by that helper. Do not duplicate the interval or timeout logic at this call site.
🤖 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/useScreenRecorder.ts`:
- Around line 1014-1051: Track whether native recording started in the recording
startup flow by declaring nativeRecordingStarted before the try block and
setting it true after the startNativeMacRecording success check. In the catch
block, stop the native recording with stopNativeMacRecording(true) when that
flag is set, then log and rethrow the original error.
---
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 946-960: Replace the inline webcam-ready polling block in the
webcamEnabled recording flow with the existing waitForWebcamReady helper,
preserving the current conditional check and timeout behavior provided by that
helper. Do not duplicate the interval or timeout logic at this call site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1000923-2481-41dc-9cc3-7c06b3b9fff0
📒 Files selected for processing (1)
src/hooks/useScreenRecorder.ts
…n fails The webcam RecorderHandle is now created after startNativeMacRecording resolves, so the native helper is already recording when createRecorderHandle runs. If it throws (e.g. MediaRecorder rejects the mime type / stream), the surrounding catch only logs and rethrows, leaving the helper recording with no handle to finalize it. Stop the native recording before rethrowing so a failed webcam sidecar tears down the whole capture cleanly, as it did when creation ran earlier. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Problem
On the native macOS capture path, recordings made with the webcam enabled are audibly out of sync: the voice/screen lead the webcam PiP by about a second, in both the editor preview and exports.
Root cause
startNativeMacRecordingIfAvailablecreated the webcamRecorderHandle(whose constructor callsMediaRecorder.start()) beforewindow.electronAPI.startNativeMacRecording(request). That IPC call only resolves once the ScreenCaptureKit helper emitsrecording-started— i.e. after permission checks, stream construction, and first-frame latency. Measured on an M1 Pro, that startup takes ~1.03s (diagnostic: sessioncreatedAtvs helperrecording-startedtimestamps).So the webcam webm contains ~1s of pre-roll footage from before the screen/mic recording began. The editor and exporter align both files at t=0, which shifts the webcam behind the voice/screen by exactly the helper's startup latency. The screen recording itself (video + system audio + mic) is internally in sync — only the webcam sidecar is offset.
Fix
Start the webcam recorder only after
startNativeMacRecordingresolves, so both recordings share the same time origin. Residual offset is now just IPC + MediaRecorder start latency (tens of ms) instead of the helper's full startup time.Note: the native Windows path (
startNativeWindowsRecordingIfAvailable) has the same pattern and likely the same skew; I left it untouched since I can only verify on macOS.Testing
tsc --noEmit,biome check, and the full vitest suite (352 tests) pass.🤖 Generated with Claude Code
Summary by CodeRabbit