fix: stabilize Windows recording preview#588
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR centralizes Windows capture stop handling with a configurable timeout and single-settlement flow, adds tests for close/error/timeout cases, computes adaptive encoder bitrate and updates helper metadata, and clears source-scoped editor/audio-fallback state when switching sources. ChangesRecording Pipeline Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/ipc/recording/windows.test.ts (1)
28-59: 💤 Low valueConsider adding tests for fallback resolution and error event paths.
The current tests cover the primary scenarios (clean close with parsed path, timeout with kill). For more complete coverage, consider adding tests for:
- Resolution via
windowsCaptureTargetPathfallback whencode === 0but no match in buffer- Rejection on non-zero exit code
- Rejection when the process emits an
erroreventThese would strengthen confidence in the edge case handling.
💡 Example additional test cases
it("resolves via windowsCaptureTargetPath when buffer has no match but exit is clean", async () => { const proc = new FakeCaptureProcess(); setWindowsCaptureOutputBuffer("Some other output"); setWindowsCaptureTargetPath("C:\\Recordly\\fallback.mp4"); const stopped = waitForWindowsCaptureStop( proc as unknown as Parameters<typeof waitForWindowsCaptureStop>[0], 1000, ); proc.emit("close", 0); await expect(stopped).resolves.toBe("C:\\Recordly\\fallback.mp4"); }); it("rejects with buffer content on non-zero exit code", async () => { const proc = new FakeCaptureProcess(); setWindowsCaptureOutputBuffer("Encoder error: insufficient memory"); const stopped = waitForWindowsCaptureStop( proc as unknown as Parameters<typeof waitForWindowsCaptureStop>[0], 1000, ); proc.emit("close", 1); await expect(stopped).rejects.toThrow("Encoder error: insufficient memory"); });🤖 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 `@electron/ipc/recording/windows.test.ts` around lines 28 - 59, Add three unit tests to cover the missing edge cases for waitForWindowsCaptureStop: (1) a test that sets setWindowsCaptureOutputBuffer to a non-matching string and setWindowsCaptureTargetPath to a valid path, then emits proc.emit("close", 0) and asserts the promise resolves to the fallback path; (2) a test that sets a buffer containing an error message, emits proc.emit("close", nonZeroCode) and asserts the promise rejects with that buffer content; and (3) a test that emits an "error" event from the FakeCaptureProcess and asserts waitForWindowsCaptureStop rejects; reuse FakeCaptureProcess, setWindowsCaptureOutputBuffer, setWindowsCaptureTargetPath, and the same Parameters<typeof waitForWindowsCaptureStop>[0] cast pattern as in the existing tests.
🤖 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.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2067-2091: The resetSourceScopedEditorState function clears all
region/ID state but doesn't clear the undo/redo stack held in editorHistoryRef,
allowing Undo to restore stale source data; update resetSourceScopedEditorState
to also reset editorHistoryRef.current to an empty/initial history object (and
any related pointers like historyIndex/currentSnapshot if present) so the
undo/redo history is cleared when switching sources — locate
resetSourceScopedEditorState and editorHistoryRef to make this change.
---
Nitpick comments:
In `@electron/ipc/recording/windows.test.ts`:
- Around line 28-59: Add three unit tests to cover the missing edge cases for
waitForWindowsCaptureStop: (1) a test that sets setWindowsCaptureOutputBuffer to
a non-matching string and setWindowsCaptureTargetPath to a valid path, then
emits proc.emit("close", 0) and asserts the promise resolves to the fallback
path; (2) a test that sets a buffer containing an error message, emits
proc.emit("close", nonZeroCode) and asserts the promise rejects with that buffer
content; and (3) a test that emits an "error" event from the FakeCaptureProcess
and asserts waitForWindowsCaptureStop rejects; reuse FakeCaptureProcess,
setWindowsCaptureOutputBuffer, setWindowsCaptureTargetPath, and the same
Parameters<typeof waitForWindowsCaptureStop>[0] cast pattern as in the existing
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 614d4e4f-633d-4567-898a-d847938d2d6f
⛔ Files ignored due to path filters (1)
electron/native/bin/win32-x64/wgc-capture.exeis excluded by!**/*.exe
📒 Files selected for processing (6)
electron/ipc/recording/windows.test.tselectron/ipc/recording/windows.tselectron/native/bin/win32-x64/helpers-manifest.jsonelectron/native/wgc-capture/src/mf_encoder.cppsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/audio/useSourceAudioFallback.ts
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 (1)
src/components/video-editor/VideoEditor.tsx (1)
2067-2094:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the remaining source-scoped cursor state here too.
resetSourceScopedEditorStatestill leavescursorTelemetry,cursorTelemetrySourcePath, andpendingFreshRecordingAutoSuggestTelemetryCountRefintact. After the fresh-recording branches swapvideoPath, the preview and auto-suggest flow can keep using the previous source's telemetry until the async reload finishes, and the telemetry-count guard can suppress auto-suggest entirely if the next recording happens to have the same sample count.Suggested fix
const resetSourceScopedEditorState = useCallback(() => { setZoomRegions([]); setTrimRegions([]); setClipRegions([]); clipInitializedRef.current = false; autoFullTrackClipIdRef.current = null; autoFullTrackClipEndMsRef.current = null; setSpeedRegions([]); setAnnotationRegions([]); setAudioRegions([]); + setCursorTelemetry([]); + setCursorTelemetrySourcePath(null); setSourceAudioTrackSettingsByClip({}); setDefaultSourceAudioTrackSettings({}); setHasClipSourceAudio(false); setAutoCaptions([]); setAutoCaptionSettings((prev) => ({ ...prev, enabled: false })); setSelectedZoomId(null); setSelectedClipId(null); setSelectedAnnotationId(null); setSelectedAudioId(null); nextZoomIdRef.current = 1; nextClipIdRef.current = 1; nextAudioIdRef.current = 1; nextAnnotationIdRef.current = 1; nextAnnotationZIndexRef.current = 1; + pendingFreshRecordingAutoSuggestTelemetryCountRef.current = 0; + autoSuggestedVideoPathRef.current = null; resetEditorHistoryStack(editorHistoryRef.current); applyingHistoryRef.current = false; syncHistoryButtons(); }, [syncHistoryButtons]);🤖 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/components/video-editor/VideoEditor.tsx` around lines 2067 - 2094, resetSourceScopedEditorState currently does not clear cursor-related source-scoped state, causing telemetry and auto-suggest guards to persist across source swaps; update resetSourceScopedEditorState to also reset cursorTelemetry, cursorTelemetrySourcePath, and pendingFreshRecordingAutoSuggestTelemetryCountRef (e.g., set cursorTelemetry and cursorTelemetrySourcePath to null/undefined and pendingFreshRecordingAutoSuggestTelemetryCountRef.current to 0) so the new source starts with a clean cursor/telemetry state.
🤖 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/components/video-editor/VideoEditor.tsx`:
- Around line 2067-2094: resetSourceScopedEditorState currently does not clear
cursor-related source-scoped state, causing telemetry and auto-suggest guards to
persist across source swaps; update resetSourceScopedEditorState to also reset
cursorTelemetry, cursorTelemetrySourcePath, and
pendingFreshRecordingAutoSuggestTelemetryCountRef (e.g., set cursorTelemetry and
cursorTelemetrySourcePath to null/undefined and
pendingFreshRecordingAutoSuggestTelemetryCountRef.current to 0) so the new
source starts with a clean cursor/telemetry state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6245bd0c-96f9-457d-b2c3-8ede34b530eb
📒 Files selected for processing (2)
electron/ipc/recording/windows.test.tssrc/components/video-editor/VideoEditor.tsx
This was referenced May 24, 2026
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
Fixes the Windows recording/editing regressions seen during local beta testing:
Root Cause
The latest recording path can open the editor before background audio sidecars and session updates finish. During that refresh, the renderer could clear companion audio paths for the same source. Separately, when opening a new recording without a saved project, old source-scoped timeline state could survive long enough to affect playback and preview audio. The WGC helper also used a fixed bitrate that was too low for some higher-detail recordings.
Validation
recording-1779651005541.mp4; preview audio now continues past the previous 18-19s cutoff.npm run build:windows-capturenpm test -- electron/ipc/recording/windows.test.ts electron/ipc/paths/binaries.test.ts src/lib/exporter/exportBitrate.test.ts src/lib/exporter/sourceTrackRoutingPolicy.test.ts src/lib/exporter/sourceAudioFallback.test.tsnpx tsc --noEmit --pretty falsenpx biome check electron/ipc/recording/windows.ts electron/ipc/recording/windows.test.ts electron/native/bin/win32-x64/helpers-manifest.json src/components/video-editor/VideoEditor.tsx src/components/video-editor/audio/useSourceAudioFallback.ts --formatter-enabled=falsegit diff --checkwgc-capture.exeSHA256 matcheshelpers-manifest.json.Note
npm run smoke:packaged-binarieswas attempted but could not run locally because this checkout does not currently have a packagedrelease/app.asar.unpackeddirectory.Summary by CodeRabbit
New Features
Bug Fixes
Tests