Fix invalid Windows recording finalization#314
Merged
webadderall merged 1 commit intowebadderallorg:mainfrom Apr 22, 2026
Merged
Fix invalid Windows recording finalization#314webadderall merged 1 commit intowebadderallorg:mainfrom
webadderall merged 1 commit intowebadderallorg:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR strengthens video recording validation by introducing a minimum file-size threshold (1024 bytes) and tightening duration parsing logic. It adds Changes
Sequence Diagram(s)sequenceDiagram
participant FFmpeg
participant Mux as Mux Handler
participant Validator as validateRecordedVideo
participant FileSystem as File System
rect rgba(100, 150, 200, 0.5)
Note over FFmpeg,FileSystem: Success Path
FFmpeg->>Mux: FFmpeg mux completes
Mux->>Validator: validateRecordedVideo(mixedOutput)
Validator->>FileSystem: Check file size >= 1024 bytes
Validator->>FFmpeg: Parse duration from output
alt Valid Video
Validator->>Mux: Return validation result
Mux->>FileSystem: Move/finalize recorded video
end
end
rect rgba(200, 100, 100, 0.5)
Note over FFmpeg,FileSystem: Failure Path
alt FFmpeg mux fails
FFmpeg->>Mux: Error/stderr
Mux->>FileSystem: Cleanup intermediate file
Mux->>Mux: Rethrow error
else Validation fails
Validator->>Mux: Reject (too small or invalid duration)
Mux->>FileSystem: Cleanup intermediate file
Mux->>Mux: Rethrow error
end
end
sequenceDiagram
participant App as App/Hook
participant RegHandler as Register Handler
participant Validator as validateRecordedVideo
participant Recovery as Recovery Path
rect rgba(100, 150, 200, 0.5)
Note over App,Recovery: Normal Stop Path
App->>RegHandler: Stop recording requested
RegHandler->>Validator: validateRecordedVideo(finalPath)
Validator->>RegHandler: Valid recording + metadata
RegHandler->>App: Success with duration/fileSize
end
rect rgba(150, 200, 100, 0.5)
Note over App,Recovery: Recovery Path
App->>Recovery: Recover recorded video
Recovery->>Validator: validateRecordedVideo(candidate1)
alt Invalid
Validator->>Recovery: Reject candidate
Recovery->>Validator: validateRecordedVideo(candidate2)
else Valid
Validator->>Recovery: Return metadata
Recovery->>App: Return validated video
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
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)
electron/ipc/windowsCaptureSelection.ts (1)
34-40:⚠️ Potential issue | 🟠 MajorKeep
displayIdaligned with the returned bounds.When a requested display ID is finite but no longer present in
allDisplays,matchedDisplayfalls back toprimaryDisplaywhile Line 39 still returns the stale requested ID. That can send the Windows helper mismatcheddisplayIdand primary-display coordinates.🐛 Proposed fix
return { - displayId: requestedOrPrimaryDisplayId, + displayId: matchedDisplay.id, bounds: matchedDisplay.bounds, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/windowsCaptureSelection.ts` around lines 34 - 40, The returned displayId can be stale when requestedOrPrimaryDisplayId doesn't match any entry in allDisplays and matchedDisplay falls back to primaryDisplay; update the return to use the actual matched display's id so bounds and displayId always correspond (e.g., set displayId to matchedDisplay.id or String(matchedDisplay.id) to match the type used elsewhere) while keeping bounds: matchedDisplay.bounds; adjust the conversion to String only if the surrounding code expects a string.electron/ipc/register/recording.ts (1)
580-606:⚠️ Potential issue | 🟠 MajorPreserve Windows audio sidecar paths when returning a validated fallback.
Lines 587-588 clear
windowsSystemAudioPath/windowsMicAudioPathbefore Line 606 returns success. The renderer then callsmuxNativeWindowsRecording, but the mux handler sees no audio paths and finalizes video-only.🐛 Proposed fix
console.error('Failed to stop native Windows capture:', error) const fallbackPath = windowsCaptureTargetPath + const fallbackSystemAudioPath = windowsSystemAudioPath + const fallbackMicrophonePath = windowsMicAudioPath setWindowsNativeCaptureActive(false) setNativeScreenRecordingActive(false) setWindowsCaptureProcess(null) setWindowsCaptureTargetPath(null) setWindowsCaptureStopRequested(false) setWindowsCapturePaused(false) - setWindowsSystemAudioPath(null) - setWindowsMicAudioPath(null) setWindowsPendingVideoPath(null) if (fallbackPath) { try { await fs.access(fallbackPath) const validation = await validateRecordedVideo(fallbackPath) setWindowsPendingVideoPath(fallbackPath) recordNativeCaptureDiagnostics({ backend: 'windows-wgc', phase: 'stop', outputPath: fallbackPath, - systemAudioPath: windowsSystemAudioPath, - microphonePath: windowsMicAudioPath, + systemAudioPath: fallbackSystemAudioPath, + microphonePath: fallbackMicrophonePath, processOutput: windowsCaptureOutputBuffer.trim() || undefined, fileSizeBytes: validation.fileSizeBytes, error: String(error), }) return { success: true, path: fallbackPath } } catch { // File is absent or failed validation. } } + + setWindowsSystemAudioPath(null) + setWindowsMicAudioPath(null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/register/recording.ts` around lines 580 - 606, The code clears windowsSystemAudioPath and windowsMicAudioPath before returning a validated fallback, causing muxNativeWindowsRecording to not see audio sidecars; change the order so you preserve audio paths when returning the fallback: do not call setWindowsSystemAudioPath(null) or setWindowsMicAudioPath(null) before the success return in the block that validates fallbackPath (keep setting setWindowsPendingVideoPath(fallbackPath) and call recordNativeCaptureDiagnostics with the preserved windowsSystemAudioPath/windowsMicAudioPath), or move the two nulling calls to after the return/after muxing; reference the state setters setWindowsSystemAudioPath, setWindowsMicAudioPath, setWindowsPendingVideoPath and the validateRecordedVideo/recordNativeCaptureDiagnostics usage to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@electron/ipc/register/recording.ts`:
- Around line 580-606: The code clears windowsSystemAudioPath and
windowsMicAudioPath before returning a validated fallback, causing
muxNativeWindowsRecording to not see audio sidecars; change the order so you
preserve audio paths when returning the fallback: do not call
setWindowsSystemAudioPath(null) or setWindowsMicAudioPath(null) before the
success return in the block that validates fallbackPath (keep setting
setWindowsPendingVideoPath(fallbackPath) and call recordNativeCaptureDiagnostics
with the preserved windowsSystemAudioPath/windowsMicAudioPath), or move the two
nulling calls to after the return/after muxing; reference the state setters
setWindowsSystemAudioPath, setWindowsMicAudioPath, setWindowsPendingVideoPath
and the validateRecordedVideo/recordNativeCaptureDiagnostics usage to locate the
fix.
In `@electron/ipc/windowsCaptureSelection.ts`:
- Around line 34-40: The returned displayId can be stale when
requestedOrPrimaryDisplayId doesn't match any entry in allDisplays and
matchedDisplay falls back to primaryDisplay; update the return to use the actual
matched display's id so bounds and displayId always correspond (e.g., set
displayId to matchedDisplay.id or String(matchedDisplay.id) to match the type
used elsewhere) while keeping bounds: matchedDisplay.bounds; adjust the
conversion to String only if the surrounding code expects a string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 619d8972-3973-46b5-b61b-99fdc442ecd1
📒 Files selected for processing (7)
electron/ipc/recording/diagnostics.test.tselectron/ipc/recording/diagnostics.tselectron/ipc/recording/mac.tselectron/ipc/recording/windows.tselectron/ipc/register/recording.tselectron/ipc/windowsCaptureSelection.tssrc/hooks/useScreenRecorder.ts
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
2 participants
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
Testing
Notes
Summary by CodeRabbit
Bug Fixes
Tests