fix(recording): publish Windows mic sidecar atomically - #785
fix(recording): publish Windows mic sidecar atomically#785AbhishekBarali wants to merge 1 commit into
Conversation
FFmpeg wrote the mic sidecar straight to its final path while the editor was already open. An unfinalized WAV advertises data size 0xFFFFFFFF, so the editor loaded a valid-looking but truncated track.
📝 WalkthroughWalkthroughThe recording pipeline now stages microphone sidecars with atomic publication, reports pending conversion state, validates WAV finalization, retries fallback discovery, cleans incomplete files, and continues background finalization after Windows mux errors. ChangesSidecar finalization and fallback handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Recorder
participant RecordingIPC
participant FFmpeg
participant Editor
Recorder->>RecordingIPC: start microphone sidecar conversion
RecordingIPC->>FFmpeg: write incomplete staging WAV
Editor->>RecordingIPC: request fallback paths
RecordingIPC-->>Editor: pending conversion state
FFmpeg-->>RecordingIPC: conversion completes
RecordingIPC->>RecordingIPC: write metadata and publish final sidecar
Editor->>RecordingIPC: retry fallback paths
RecordingIPC-->>Editor: finalized sidecar path
Possibly related PRs
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.
Actionable comments posted: 3
🤖 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 `@electron/ipc/recording/diagnostics.ts`:
- Around line 467-473: Update isUsableCompanionAudioFile so zero-byte companion
files remain eligible for recovery instead of being rejected by the stat.size
check; preserve the existing finalized-WAV validation for .wav files and the
unconditional eligibility of non-WAV files. Add a regression test covering a
zero-byte .wav companion returned by the fallback lookup.
In `@electron/ipc/register/recording.ts`:
- Around line 1791-1795: Update the conversion cleanup Promise.all near
moveFileWithOverwrite to also remove the metadata sidecar created from
sidecarPath. Delete the ${sidecarPath}.json artifact with forced, non-failing
cleanup alongside tempWebmPath, stagingSidecarPath, and sidecarPath, preserving
cleanup behavior on retries.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 1159-1169: Update the native Windows mux handling in the recording
flow around muxNativeWindowsRecording to inspect its resolved result and log an
error when success is false, in addition to retaining the existing catch for
rejected IPC calls. Ensure resolved mux failures include the returned error
details in the console.error output before proceeding.
🪄 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: 36a1517e-c377-49fb-a085-2d9b413a928b
📒 Files selected for processing (9)
electron/electron-env.d.tselectron/ipc/constants.tselectron/ipc/recording/diagnostics.test.tselectron/ipc/recording/diagnostics.tselectron/ipc/recording/prune.tselectron/ipc/register/recording.tselectron/ipc/utils.tssrc/components/video-editor/audio/useSourceAudioFallback.tssrc/hooks/useScreenRecorder.ts
| async function isUsableCompanionAudioFile(companionPath: string): Promise<boolean> { | ||
| const stat = await fs.stat(companionPath); | ||
| if (stat.size <= 0) { | ||
| return false; | ||
| } | ||
|
|
||
| return companionPath.toLowerCase().endsWith(".wav") ? isFinalizedWavFile(companionPath) : true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep zero-byte sidecars eligible for recovery.
Line 469 rejects zero-byte files before WAV finalization inspection. The fallback lookup therefore cannot return a zero-byte sidecar. This conflicts with the recovery contract in the PR objective.
Proposed fix
const stat = await fs.stat(companionPath);
- if (stat.size <= 0) {
+ if (stat.size < 0) {
return false;
}Add a regression test for a zero-byte .wav companion.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/diagnostics.ts` around lines 467 - 473, Update
isUsableCompanionAudioFile so zero-byte companion files remain eligible for
recovery instead of being rejected by the stat.size check; preserve the existing
finalized-WAV validation for .wav files and the unconditional eligibility of
non-WAV files. Add a regression test covering a zero-byte .wav companion
returned by the fallback lookup.
| await Promise.all([ | ||
| fs.rm(tempWebmPath, { force: true }).catch(() => undefined), | ||
| fs.rm(stagingSidecarPath, { force: true }).catch(() => undefined), | ||
| fs.rm(sidecarPath, { force: true }).catch(() => undefined), | ||
| ]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Remove the metadata sidecar during conversion cleanup.
Line 1763 can create ${sidecarPath}.json before moveFileWithOverwrite fails. This cleanup leaves that metadata behind. A later retry without metadata can then use stale timing data for the new sidecar.
Proposed fix
await Promise.all([
fs.rm(tempWebmPath, { force: true }).catch(() => undefined),
fs.rm(stagingSidecarPath, { force: true }).catch(() => undefined),
fs.rm(sidecarPath, { force: true }).catch(() => undefined),
+ fs.rm(`${sidecarPath}.json`, { force: true }).catch(() => undefined),
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await Promise.all([ | |
| fs.rm(tempWebmPath, { force: true }).catch(() => undefined), | |
| fs.rm(stagingSidecarPath, { force: true }).catch(() => undefined), | |
| fs.rm(sidecarPath, { force: true }).catch(() => undefined), | |
| ]); | |
| await Promise.all([ | |
| fs.rm(tempWebmPath, { force: true }).catch(() => undefined), | |
| fs.rm(stagingSidecarPath, { force: true }).catch(() => undefined), | |
| fs.rm(sidecarPath, { force: true }).catch(() => undefined), | |
| fs.rm(`${sidecarPath}.json`, { force: true }).catch(() => undefined), | |
| ]); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/register/recording.ts` around lines 1791 - 1795, Update the
conversion cleanup Promise.all near moveFileWithOverwrite to also remove the
metadata sidecar created from sidecarPath. Delete the ${sidecarPath}.json
artifact with forced, non-failing cleanup alongside tempWebmPath,
stagingSidecarPath, and sidecarPath, preserving cleanup behavior on retries.
| if (isNativeWindows) { | ||
| await window.electronAPI.muxNativeWindowsRecording(expectedDurationMs); | ||
| try { | ||
| await window.electronAPI.muxNativeWindowsRecording( | ||
| expectedDurationMs, | ||
| ); | ||
| } catch (muxError) { | ||
| console.error( | ||
| "Failed to mux native Windows recording audio:", | ||
| muxError, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log resolved mux failures.
muxNativeWindowsRecording returns success: false for mux errors. This catch only handles rejected IPC calls. A resolved failure proceeds without the required error log.
if (isNativeWindows) {
try {
- await window.electronAPI.muxNativeWindowsRecording(expectedDurationMs);
+ const muxResult =
+ await window.electronAPI.muxNativeWindowsRecording(expectedDurationMs);
+ if (!muxResult.success) {
+ console.error(
+ "Failed to mux native Windows recording audio:",
+ muxResult.error ?? muxResult.message,
+ );
+ }
} catch (muxError) {
console.error(
"Failed to mux native Windows recording audio:",
muxError,
);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isNativeWindows) { | |
| await window.electronAPI.muxNativeWindowsRecording(expectedDurationMs); | |
| try { | |
| await window.electronAPI.muxNativeWindowsRecording( | |
| expectedDurationMs, | |
| ); | |
| } catch (muxError) { | |
| console.error( | |
| "Failed to mux native Windows recording audio:", | |
| muxError, | |
| ); | |
| } | |
| if (isNativeWindows) { | |
| try { | |
| const muxResult = | |
| await window.electronAPI.muxNativeWindowsRecording( | |
| expectedDurationMs, | |
| ); | |
| if (!muxResult.success) { | |
| console.error( | |
| "Failed to mux native Windows recording audio:", | |
| muxResult.error ?? muxResult.message, | |
| ); | |
| } | |
| } catch (muxError) { | |
| console.error( | |
| "Failed to mux native Windows recording audio:", | |
| muxError, | |
| ); | |
| } |
🤖 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 1159 - 1169, Update the native
Windows mux handling in the recording flow around muxNativeWindowsRecording to
inspect its resolved result and log an error when success is false, in addition
to retaining the existing catch for rejected IPC calls. Ensure resolved mux
failures include the returned error details in the console.error output before
proceeding.
1 participant
Description
On Windows the microphone track is cut short — it typically stops 15-20 seconds in, even though the capture itself is complete.
Motivation
The mic is captured in the renderer and converted to a
.mic.wavsidecar by FFmpeg after capture stops. That took 3.2s for a 67s clip, but the editor opens immediately, and FFmpeg wrote directly to the final path.An unfinalized WAV declares a data size of 0xFFFFFFFF ("read to EOF"), so a mid-write read parses as a complete but shorter recording. Discovery accepted any file above zero bytes, so the editor loaded it.
Sampling the output during a real encode (bundled FFmpeg, same filter chain):
Editor startup lands inside that window, matching the reported 15-20 s. From an affected recording: capture stopped 09:21:12.922, sidecar finalized 09:21:16.138, finished file holds 67.26 s.
Type of Change
What changed
Testing Guide
npm test(997 pass, including 2 new regression tests),npx tsc --noEmit,npm run lint.A harness driving the real FFmpeg binary: the old path returned a truncated file on 20 of 20 samples (2.73-65.54 s apparent); the new path returned none, with the published file complete.
Verified at the file level on Windows 11, not yet through a long GUI session. macOS and Linux are untouched, as the new check applies only to WAV.
Summary by CodeRabbit