fix: stream recording chunks to disk to support recordings longer than 10 minutes#617
Open
Amanuel2x wants to merge 1 commit into
Open
fix: stream recording chunks to disk to support recordings longer than 10 minutes#617Amanuel2x wants to merge 1 commit into
Amanuel2x wants to merge 1 commit into
Conversation
Recordings longer than ~10 minutes would silently fail to save. The entire recording was held in memory as a Blob[] array, then on stop fixWebmDuration() and arrayBuffer() created additional copies before the data was sent over IPC to be written. For an 18+ minute 1080p recording this meant 400-600MB being duplicated 3-4x in the renderer process, exceeding Electron's memory limits and causing a silent crash with no file written to disk. Fix: open an fs.WriteStream on the main process as soon as recording starts, and send each 1-second ondataavailable chunk directly to disk via IPC instead of accumulating chunks in RAM. On stop, the stream is closed and the session manifest is written — no in-memory blob needed. A full in-memory fallback is preserved for environments where the IPC stream is unavailable.
Contributor
📝 WalkthroughWalkthroughThis PR implements disk-backed recording streaming to prevent out-of-memory crashes for long recordings. Instead of buffering the entire WebM blob in renderer memory, chunks are now streamed directly to disk via IPC as they arrive, keeping memory usage constant regardless of recording duration. ChangesRecording Stream Persistence
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
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. 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: 3
🧹 Nitpick comments (1)
src/hooks/useScreenRecorder.ts (1)
117-119: 💤 Low valuenit: chunk append errors are silently ignored.
if
appendRecordingChunkfails mid-recording (disk full, stream error), the promise rejection is swallowed byvoid. the recording continues but data is lost with no feedback.lowkey risky for long recordings — at minimum consider logging failures so they're debuggable. could also set a flag to trigger fallback behavior.
Also applies to: 142-143
🤖 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 117 - 119, The code currently uses "void window.electronAPI.appendRecordingChunk(recordingId, chunk)" inside the pendingChunks loop (and similarly at lines ~142-143), which swallows promise rejections and hides failures; update the logic in useScreenRecorder.ts to await or handle appendRecordingChunk's promise and surface errors: wrap the per-chunk calls (or the Promise.all of them) in try/catch or attach .catch handlers that log the error (e.g., console.error or an existing logger) and set a recording error flag/state (e.g., recordingFailed or setRecordingError) so the UI or fallback behavior can react when appendRecordingChunk(recordingId, chunk) fails; ensure you update both the pendingChunks loop and the later occurrence that uses appendRecordingChunk.
🤖 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/handlers.ts`:
- Around line 2081-2089: The handler that opens a write stream (using
resolveRecordingOutputPath, createWriteStream and activeWriteStreams with
recordingId) can fail with ENOENT if the recordings directory doesn't exist;
before creating the write stream call fs.mkdir (or fs.promises.mkdir) on the
parent directory of resolveRecordingOutputPath(fileName) with { recursive: true
} (await if using promises) inside the try block, then create the stream and set
activeWriteStreams; keep the existing try/catch and return shape but ensure the
directory creation is awaited and errors propagate into the existing error
return.
- Around line 2130-2141: The finalize path leaks file streams when a recording
is discarded: update finalizeScreenRecording (and/or cancelRecording flow) to
explicitly close and cleanup any activeWriteStreams entry for the recordingId
(use the same logic as in the existing write/close block that calls screenWs.end
and activeWriteStreams.delete) before bailing out, and also remove any partial
screenVideoPath (or ensure stream end is awaited) so open handles and orphaned
.webm files are cleaned up; reference activeWriteStreams,
finalizeScreenRecording, cancelRecording, discardRecordingId.current,
storeRecordedSession and discardCursorTelemetry when adding this cleanup or
creating a dedicated IPC abortRecording handler.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 104-166: The returned streaming flag is captured before
streamOpenPromise resolves, causing a race and data loss; fix by determining
streaming only when recording stops and exposing that final value (or including
it with the resolved blob). Concretely, update recorder.onstop to set a
finalStreaming boolean (based on streamFailed) before resolving
recordedBlobPromise, and change the hook's return value to either provide a
getter/function for streaming or have recordedBlobPromise resolve to an object
like { blob, streaming }; then update finalizeRecording to consume the final
streaming state from that getter/object instead of reading the early-captured
streaming variable. Ensure you touch streamOpenPromise,
streamFailed/streamReady, recorder.onstop, recordedBlobPromise, and the place
that checks activeScreenRecorder.streaming in finalizeRecording.
---
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 117-119: The code currently uses "void
window.electronAPI.appendRecordingChunk(recordingId, chunk)" inside the
pendingChunks loop (and similarly at lines ~142-143), which swallows promise
rejections and hides failures; update the logic in useScreenRecorder.ts to await
or handle appendRecordingChunk's promise and surface errors: wrap the per-chunk
calls (or the Promise.all of them) in try/catch or attach .catch handlers that
log the error (e.g., console.error or an existing logger) and set a recording
error flag/state (e.g., recordingFailed or setRecordingError) so the UI or
fallback behavior can react when appendRecordingChunk(recordingId, chunk) fails;
ensure you update both the pendingChunks loop and the later occurrence that uses
appendRecordingChunk.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f44ef88-365b-4fb5-b52e-82313de502a5
📒 Files selected for processing (4)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/hooks/useScreenRecorder.ts
Comment on lines
+2081
to
+2089
| try { | ||
| const filePath = resolveRecordingOutputPath(fileName); | ||
| const ws = createWriteStream(filePath, { flags: "w" }); | ||
| activeWriteStreams.set(recordingId, ws); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| return { success: false, error: String(error) }; | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
Missing directory creation before opening write stream.
createWriteStream will throw ENOENT if RECORDINGS_DIR doesn't exist. Native recording flows call fs.mkdir(RECORDINGS_DIR, { recursive: true }) before starting, but the browser-based recording flow doesn't — it relies on this handler being called first.
🐛 Proposed fix
ipcMain.handle(
"open-recording-stream",
async (
_,
recordingId: number,
fileName: string,
): Promise<{ success: boolean; error?: string }> => {
try {
+ await fs.mkdir(RECORDINGS_DIR, { recursive: true });
const filePath = resolveRecordingOutputPath(fileName);
const ws = createWriteStream(filePath, { flags: "w" });
activeWriteStreams.set(recordingId, ws);
return { success: true };
} catch (error) {
return { success: false, error: String(error) };
}
},
);📝 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.
Suggested change
| try { | |
| const filePath = resolveRecordingOutputPath(fileName); | |
| const ws = createWriteStream(filePath, { flags: "w" }); | |
| activeWriteStreams.set(recordingId, ws); | |
| return { success: true }; | |
| } catch (error) { | |
| return { success: false, error: String(error) }; | |
| } | |
| }, | |
| try { | |
| await fs.mkdir(RECORDINGS_DIR, { recursive: true }); | |
| const filePath = resolveRecordingOutputPath(fileName); | |
| const ws = createWriteStream(filePath, { flags: "w" }); | |
| activeWriteStreams.set(recordingId, ws); | |
| return { success: true }; | |
| } catch (error) { | |
| return { success: false, error: String(error) }; | |
| } | |
| }, |
🤖 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/handlers.ts` around lines 2081 - 2089, The handler that opens a
write stream (using resolveRecordingOutputPath, createWriteStream and
activeWriteStreams with recordingId) can fail with ENOENT if the recordings
directory doesn't exist; before creating the write stream call fs.mkdir (or
fs.promises.mkdir) on the parent directory of
resolveRecordingOutputPath(fileName) with { recursive: true } (await if using
promises) inside the try block, then create the stream and set
activeWriteStreams; keep the existing try/catch and return shape but ensure the
directory creation is awaited and errors propagate into the existing error
return.
Comment on lines
+2130
to
+2141
|
|
||
| // Close the streaming write stream if one was used; otherwise fall back to | ||
| // writing the full buffer (short recordings that never opened a stream). | ||
| const screenWs = activeWriteStreams.get(createdAt); | ||
| if (screenWs) { | ||
| await new Promise<void>((resolve, reject) => | ||
| screenWs.end((err?: Error | null) => (err ? reject(err) : resolve())), | ||
| ); | ||
| activeWriteStreams.delete(createdAt); | ||
| } else if (payload.screen.videoData && payload.screen.videoData.byteLength > 0) { | ||
| await fs.writeFile(screenVideoPath, Buffer.from(payload.screen.videoData)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if discardCursorTelemetry is the only cleanup called on discard
rg -n "discardCursorTelemetry" --type ts -C 3Repository: siddharthvaddem/openscreen
Length of output: 1541
🏁 Script executed:
#!/bin/bash
# Find the discard-cursor-telemetry handler in electron handlers
rg -n "discard-cursor-telemetry" --type ts -A 15 electron/Repository: siddharthvaddem/openscreen
Length of output: 1152
🏁 Script executed:
#!/bin/bash
# Also check what happens with activeWriteStreams when recording is discarded
rg -n "activeWriteStreams" --type ts -B 3 -A 3Repository: siddharthvaddem/openscreen
Length of output: 3504
🏁 Script executed:
#!/bin/bash
# Check if there's a cancel/cleanup handler for recordings
rg -n "cancelRecording\|abort.*[Rr]ecord" --type ts -A 5Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Search for the discard-cursor-telemetry handler definition
rg -n '"discard-cursor-telemetry"' electron/ipc/handlers.ts -A 20Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Also check the structure around the storeRecordedSessionFiles handler to understand the flow better
rg -n '"store-recorded-session"' electron/ipc/handlers.ts -B 5 -A 3Repository: siddharthvaddem/openscreen
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Search for discard-cursor-telemetry in all files
rg -n '"discard-cursor-telemetry"' . --type tsRepository: siddharthvaddem/openscreen
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check the useScreenRecorder hook to see full flow on discard
rg -n "discardRecordingId.current" src/hooks/useScreenRecorder.ts -B 5 -A 10Repository: siddharthvaddem/openscreen
Length of output: 5815
Streams definitely leak when recording is cancelled/discarded.
When cancelRecording is called, it sets discardRecordingId.current and triggers finalization. In finalizeScreenRecording, if the recording is being discarded (line 416-418 in useScreenRecorder.ts), it bails out early without calling storeRecordedSession. That handler is the only place activeWriteStreams gets cleaned up—so the write streams stay open indefinitely, and partial .webm files are orphaned on disk.
The discardCursorTelemetry call looks like an incomplete attempt at cleanup; the handler doesn't even exist in the codebase.
You'll need either:
- A dedicated IPC to abort/cleanup a recording stream by
recordingId, or - Close streams directly when discard happens (maybe in a new handler or as part of the same finalize flow)
Without this, cancelled recordings will accumulate open file handles and disk clutter.
🤖 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/handlers.ts` around lines 2130 - 2141, The finalize path leaks
file streams when a recording is discarded: update finalizeScreenRecording
(and/or cancelRecording flow) to explicitly close and cleanup any
activeWriteStreams entry for the recordingId (use the same logic as in the
existing write/close block that calls screenWs.end and
activeWriteStreams.delete) before bailing out, and also remove any partial
screenVideoPath (or ensure stream end is awaited) so open handles and orphaned
.webm files are cleaned up; reference activeWriteStreams,
finalizeScreenRecording, cancelRecording, discardRecordingId.current,
storeRecordedSession and discardCursorTelemetry when adding this cleanup or
creating a dedicated IPC abortRecording handler.
Comment on lines
+104
to
+166
|
|
||
| // Try to open a disk-backed stream. Falls back to in-memory if IPC unavailable. | ||
| const streamOpenPromise = | ||
| window.electronAPI?.openRecordingStream?.(recordingId, fileName) ?? | ||
| Promise.resolve({ success: false }); | ||
|
|
||
| const pendingChunks: ArrayBuffer[] = []; | ||
| let streamReady = false; | ||
| let streamFailed = false; | ||
|
|
||
| streamOpenPromise.then((result) => { | ||
| if (result.success) { | ||
| streamReady = true; | ||
| for (const chunk of pendingChunks) { | ||
| void window.electronAPI.appendRecordingChunk(recordingId, chunk); | ||
| } | ||
| pendingChunks.length = 0; | ||
| } else { | ||
| streamFailed = true; | ||
| } | ||
| }); | ||
|
|
||
| const fallbackChunks: Blob[] = []; | ||
|
|
||
| const recordedBlobPromise = new Promise<Blob>((resolve, reject) => { | ||
| recorder.ondataavailable = (event: BlobEvent) => { | ||
| if (event.data && event.data.size > 0) { | ||
| chunks.push(event.data); | ||
| if (!event.data || event.data.size === 0) return; | ||
|
|
||
| if (streamFailed) { | ||
| fallbackChunks.push(event.data); | ||
| return; | ||
| } | ||
|
|
||
| void event.data.arrayBuffer().then((buf) => { | ||
| if (streamFailed) { | ||
| fallbackChunks.push(new Blob([buf], { type: mimeType })); | ||
| return; | ||
| } | ||
| if (streamReady) { | ||
| void window.electronAPI.appendRecordingChunk(recordingId, buf); | ||
| } else { | ||
| pendingChunks.push(buf); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| recorder.onerror = () => { | ||
| reject(new Error("Recording failed")); | ||
| }; | ||
|
|
||
| recorder.onstop = () => { | ||
| resolve(new Blob(chunks, { type: mimeType })); | ||
| if (streamFailed) { | ||
| // Streaming failed — return full in-memory blob as fallback. | ||
| resolve(new Blob(fallbackChunks, { type: mimeType })); | ||
| } else { | ||
| // Streaming succeeded — main process already has the data on disk. | ||
| resolve(new Blob([], { type: mimeType })); | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| recorder.start(RECORDER_TIMESLICE_MS); | ||
| return { recorder, recordedBlobPromise }; | ||
| return { recorder, recordedBlobPromise, streaming: !streamFailed }; |
Contributor
There was a problem hiding this comment.
Race condition: streaming flag doesn't reflect actual outcome.
The streaming flag is captured at line 166 as !streamFailed, but at that point the streamOpenPromise hasn't resolved yet — so streamFailed is always false and streaming is always true initially.
If the IPC stream fails to open (permissions, disk error, etc.), the fallback logic in ondataavailable correctly buffers to fallbackChunks, and onstop correctly returns the fallback blob. But then in finalizeRecording, the check at line 430:
if (!activeScreenRecorder.streaming && screenBlob.size > 0) {...is false because streaming was captured as true. The in-memory data gets thrown away and an empty ArrayBuffer(0) is sent instead, resulting in data loss.
kinda cursed timing issue — maybe make streaming a getter that checks the final state, or resolve the promise before returning.
🐛 One possible fix: defer flag evaluation to onstop
- const fallbackChunks: Blob[] = [];
+ let usedFallback = false;
+ const fallbackChunks: Blob[] = [];
const recordedBlobPromise = new Promise<Blob>((resolve, reject) => {
recorder.ondataavailable = (event: BlobEvent) => {
if (!event.data || event.data.size === 0) return;
if (streamFailed) {
fallbackChunks.push(event.data);
+ usedFallback = true;
return;
}
void event.data.arrayBuffer().then((buf) => {
if (streamFailed) {
fallbackChunks.push(new Blob([buf], { type: mimeType }));
+ usedFallback = true;
return;
}
// ... rest unchanged
});
};
recorder.onstop = () => {
- if (streamFailed) {
+ if (usedFallback) {
resolve(new Blob(fallbackChunks, { type: mimeType }));
} else {
resolve(new Blob([], { type: mimeType }));
}
};
});
recorder.start(RECORDER_TIMESLICE_MS);
- return { recorder, recordedBlobPromise, streaming: !streamFailed };
+ // streaming is determined by whether we fell back to in-memory
+ // We need to expose this info; simplest: check blob size in finalizeRecording
+ // OR return a promise/getter. For now, returning true and relying on blob.size check.
+ return { recorder, recordedBlobPromise, streaming: true };Then update finalizeRecording to check blob size regardless of streaming flag:
-if (!activeScreenRecorder.streaming && screenBlob.size === 0) {
+if (screenBlob.size === 0 && !activeScreenRecorder.streaming) {
return;
}
+// If blob has data, always process it (fallback case)
+let screenVideoData: ArrayBuffer = new ArrayBuffer(0);
+if (screenBlob.size > 0) {
+ const fixedScreenBlob = await fixWebmDuration(screenBlob, duration);
+ screenVideoData = await fixedScreenBlob.arrayBuffer();
+}🤖 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 104 - 166, The returned
streaming flag is captured before streamOpenPromise resolves, causing a race and
data loss; fix by determining streaming only when recording stops and exposing
that final value (or including it with the resolved blob). Concretely, update
recorder.onstop to set a finalStreaming boolean (based on streamFailed) before
resolving recordedBlobPromise, and change the hook's return value to either
provide a getter/function for streaming or have recordedBlobPromise resolve to
an object like { blob, streaming }; then update finalizeRecording to consume the
final streaming state from that getter/object instead of reading the
early-captured streaming variable. Ensure you touch streamOpenPromise,
streamFailed/streamReady, recorder.onstop, recordedBlobPromise, and the place
that checks activeScreenRecorder.streaming in finalizeRecording.
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.
Fixes #616
Problem
Recordings longer than ~10 minutes silently fail to save. The entire recording was buffered in the renderer as a
Blob[]array, then on stopfixWebmDuration()+.arrayBuffer()+Buffer.from()created 3–4 additional in-memory copies before the data was written to disk. For an 18-minute 1080p recording this exceeds 400–600 MB in the renderer process, causing a silent crash with no file saved and no error shown.Solution
Open an
fs.WriteStreamon the main process the moment recording starts, and pipe each 1-secondondataavailablechunk directly to disk via two new IPC calls (open-recording-stream,append-recording-chunk). On stop the stream is closed and the session manifest is written — the renderer never holds more than a single 1-second chunk in memory.A full in-memory fallback is preserved for cases where the IPC stream fails to open.
Changes
src/hooks/useScreenRecorder.ts—createRecorderHandlestreams chunks to disk via IPC instead of accumulating them inBlob[]; stop path skipsarrayBuffer()when streaming succeededelectron/ipc/handlers.ts— addsopen-recording-streamandappend-recording-chunkhandlers;storeRecordedSessionFilescloses the write stream instead of callingwriteFilewhen streaming was usedelectron/preload.ts— exposes the two new IPC callselectron/electron-env.d.ts— type declarations for the two new IPC callsTesting
tsc --noEmit— no type errorsvitest --run— 151/151 tests passingSummary by CodeRabbit
Release Notes