Fix: stream long recordings to disk and patch WebM duration on save (#616)#658
Conversation
Recordings longer than ~10 minutes silently fail to save (siddharthvaddem#616). The renderer buffers the whole WebM as a Blob[], then on stop makes several in-memory copies (fixWebmDuration -> arrayBuffer -> Buffer.from) before writing. A long 1080p recording duplicates hundreds of MB several times in the renderer, exceeds Electron's memory limit, and the renderer crashes silently with no file saved. Two changes: 1. Stream chunks to disk (originally @Amanuel2x's contribution in siddharthvaddem#617). Open an fs.WriteStream in the main process at recording start and send each ~1s ondataavailable chunk straight to disk over two new IPC calls (open-recording-stream, append-recording-chunk), so the renderer never holds more than a single chunk. A full in-memory fallback is preserved for environments where the IPC stream cannot open. 2. Patch the WebM Duration header on disk after the stream closes. Browser MediaRecorder writes WebM with no Duration element, so streamed files save with duration=N/A and the editor's seek bar, timeline, and any scrub/trim break. A new electron/recording/webm-duration.ts module rewrites the Duration element, writing to a temp file and renaming in place so a crash mid-write cannot corrupt the recording. Streaming is opt-in: the screen recorder and the browser-only webcam recorder stream to disk; native-capture webcam sidecars (Windows, macOS) keep buffering in-memory, since their finalize path reads the recorder blob directly to attach the webcam track. Verified: tsc --noEmit clean; biome clean; vitest 166/166. Closes siddharthvaddem#616 Supersedes siddharthvaddem#617 Co-Authored-By: Amanuel <amanuel@localboostnetworking.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds renderer-to-main chunked MediaRecorder streaming via preload IPC and a main-process RecordingStreamRegistry; session finalization skips re-writing streamed files and best-effort patches WebM duration headers on disk when durationMs is provided. ChangesRecording Chunk Streaming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 727e395fcf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| recorder.onstop = () => { | ||
| resolve(new Blob(chunks, { type: mimeType })); | ||
| if (streamFailed) { | ||
| // Not streaming, or the stream failed to open — return the full | ||
| // in-memory blob (the buffered fallback). | ||
| resolve(new Blob(fallbackChunks, { type: mimeType })); |
There was a problem hiding this comment.
ondataavailable offloads each chunk to event.data.arrayBuffer().then(...), but onstop resolves immediately without waiting for those async conversions/appends to finish. Finalization then calls storeRecordedSession, which closes and removes the write stream (screenWs.end() in electron/ipc/handlers.ts), so late appendRecordingChunk calls can arrive after close and be dropped, truncating the saved recording under load or with large final chunks.
Useful? React with 👍 / 👎.
|
|
||
| recorder.start(RECORDER_TIMESLICE_MS); | ||
| return { recorder, recordedBlobPromise }; | ||
| return { recorder, recordedBlobPromise, streaming: !streamFailed }; |
There was a problem hiding this comment.
The returned streaming flag is computed once at construction time (!streamFailed) before openRecordingStream resolves. If stream opening fails later, streamFailed flips and chunks are buffered in memory fallback, but finalize still treats this recorder as streamed and sends empty videoData; since no active stream exists in main, storeRecordedSessionFiles skips both stream-close and buffer-write paths, so the recording can be lost in exactly the failure case the fallback is meant to cover.
Useful? React with 👍 / 👎.
| window.electronAPI?.discardCursorTelemetry(activeRecordingId); | ||
| return; |
There was a problem hiding this comment.
When a recording is canceled/restarted, this early return bypasses storeRecordedSession, but write streams are only ended/deleted inside storeRecordedSessionFiles in electron/ipc/handlers.ts. For streamed recordings, that leaves entries in activeWriteStreams and open/partial files on disk for each discarded run, causing descriptor/resource leaks and stale artifacts over repeated cancels.
Useful? React with 👍 / 👎.
![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
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)
423-426: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftDiscard path doesn’t close active disk stream in main process.
At Line 423–426, discard returns before
storeRecordedSession, but main-side stream cleanup currently happens insidestoreRecordedSessionFiles. This can leakactiveWriteStreamsentries and leave partial files around.🤖 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 423 - 426, The discard branch returns early (when discardRecordingId.current === activeRecordingId) and calls window.electronAPI?.discardCursorTelemetry(activeRecordingId) but never triggers the main-process stream cleanup done in storeRecordedSessionFiles, which leaks entries in activeWriteStreams and leaves partial files; modify the discard path in useScreenRecorder (the block referencing discardRecordingId, activeRecordingId and window.electronAPI?.discardCursorTelemetry) to also invoke the main-process cleanup — either call the existing storeRecordedSessionFiles cleanup routine (or expose and call a new close/cleanup IPC like closeActiveWriteStream/cleanupRecording with the activeRecordingId) before returning so activeWriteStreams is closed and partial files are removed.
🤖 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 2157-2161: The IPC handler that opens a recording stream returns
success immediately after calling createWriteStream (using
resolveRecordingOutputPath, createWriteStream, activeWriteStreams, recordingId,
ws), but the stream can emit an asynchronous 'error' before it's actually
writable; change the handler to wait for the stream to emit 'open' (or an
'error') before returning success: create the write stream, attach one-time
listeners for 'open' to set activeWriteStreams and return { success: true } and
for 'error' to clean up and return { success: false, error: <message> } (do not
set activeWriteStreams until 'open' fires), ensuring the renderer only sees
success after the stream is confirmed writable.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 114-117: The code computes a one-time "streaming" snapshot which
can go stale and cause data loss; change all uses of the snapshot to compute
streaming at runtime from the current state (e.g., derive streaming as
Boolean(streamTarget) && !streamFailed && streamReady) instead of using the
precomputed "streaming" variable; update places that decide whether to
finalize/serialize fallback (including the finalize routine and any logic around
pendingChunks, streamReady, streamFailed, and streamTarget) to consult this
runtime-derived value so a late stream-open failure will fall back to
serializing the buffered blob.
- Around line 126-128: The appendRecordingChunk calls are fire-and-forget so
onstop can resolve before final writes complete, leading to truncated files;
update the code paths that push chunks (the loop over pendingChunks and the
other occurrences around lines 147-157 and 164-172) to collect the Promises
returned by window.electronAPI.appendRecordingChunk and await them (e.g., await
Promise.all(pendingChunks.map(chunk =>
window.electronAPI.appendRecordingChunk(streamTarget.recordingId, chunk))))
before allowing onstop to finish or before calling storeRecordedSession; ensure
appendRecordingChunk invocations are awaited and propagate/reject errors so
storeRecordedSession only runs after all append promises settle.
---
Outside diff comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 423-426: The discard branch returns early (when
discardRecordingId.current === activeRecordingId) and calls
window.electronAPI?.discardCursorTelemetry(activeRecordingId) but never triggers
the main-process stream cleanup done in storeRecordedSessionFiles, which leaks
entries in activeWriteStreams and leaves partial files; modify the discard path
in useScreenRecorder (the block referencing discardRecordingId,
activeRecordingId and window.electronAPI?.discardCursorTelemetry) to also invoke
the main-process cleanup — either call the existing storeRecordedSessionFiles
cleanup routine (or expose and call a new close/cleanup IPC like
closeActiveWriteStream/cleanupRecording with the activeRecordingId) before
returning so activeWriteStreams is closed and partial files are removed.
🪄 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: 4c762bce-992a-471d-836a-45251566cf2b
📒 Files selected for processing (6)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tselectron/recording/webm-duration.tssrc/hooks/useScreenRecorder.tssrc/lib/recordingSession.ts
Addresses the review feedback on siddharthvaddem#658 (CodeRabbit + Codex) and the structural notes from the quality pass. Correctness: - Compute the recorder's streaming state at finalize time, not at construction. A stream that fails to open is now reported as not-streamed, so its buffered chunks are saved as a complete in-memory fallback instead of being dropped (was total data loss on open failure). - Await every in-flight chunk write before onstop resolves, so the main process never closes the write stream while a final chunk is still in flight (was truncating the tail of a recording under load). - Open the disk write stream by awaiting its 'open' event, so a bad path or permission error rejects up front instead of being acknowledged as success and then silently dropping bytes. - Close the stream and remove the partial file when a streamed recording is discarded or fails, so cancelled/failed runs don't leak descriptors or orphan partial recordings. - Surface a mid-stream write failure as a rejected recording rather than saving a silently truncated file. Structure: - Extract the streaming concern into electron/ipc/recordingStream.ts (RecordingStreamRegistry) and src/hooks/recorderHandle.ts, out of the 2.8k-line handlers.ts and the screen-recorder hook. - Key write streams by output file name, removing the implicit recordingId/+1 contract that spanned the IPC boundary. - Collapse the duplicated screen/webcam finalize blocks into one helper and the repeated duration-validity guard into one check; patch the screen and webcam durations in parallel. Adds unit tests for the registry (real temp-dir fs) and the recorder handle state machine (open-failure fallback, in-order writes awaited before stop, mid-stream failure). Extends the vitest include glob to collect electron-side tests. Verified: tsc --noEmit clean; biome clean; vitest 180/180. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed CodeRabbit / Codex findings
Structural
Verified locally: One caveat unchanged from the PR description: still worth a desktop smoke-test of a real >10-min recording plus a native webcam before relying on it, since no unit test exercises the live Electron + MediaRecorder path end to end. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/recorderHandle.ts`:
- Around line 77-88: openPromise.then currently has no rejection handler which
can leave mode as "pending" on unexpected IPC failures; add a .catch on
openPromise that sets mode = "buffering", ensures streamOpened stays false,
preserves memoryChunks (or enqueues them as currently done when buffering), and
optionally logs the error so the code can proceed to finalizeBlob() safely;
update the promise chain around openPromise (the block referencing openPromise,
mode, streamOpened, memoryChunks, and enqueueWrite) to include this .catch and
make sure the error branch mirrors the successful-path transition to buffering.
🪄 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: fa0974de-e221-4710-b2f7-56db5c8ac597
📒 Files selected for processing (9)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/ipc/recordingStream.test.tselectron/ipc/recordingStream.tselectron/preload.tssrc/hooks/recorderHandle.test.tssrc/hooks/recorderHandle.tssrc/hooks/useScreenRecorder.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- vitest.config.ts
|
Re-review requested after @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3c5b8a65d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const webcamBlob = await activeWebcamRecorder.recordedBlobPromise.catch(() => null); | ||
| if (webcamBlob && webcamBlob.size > 0) { | ||
| fixedWebcamBlob = await fixWebmDuration(webcamBlob, duration); | ||
| if (!activeWebcamRecorder.isStreaming() && webcamBlob && webcamBlob.size > 0) { | ||
| const fixedWebcamBlob = await fixWebmDuration(webcamBlob, duration); | ||
| webcamVideoData = await fixedWebcamBlob.arrayBuffer(); | ||
| } else if (activeWebcamRecorder.isStreaming()) { |
There was a problem hiding this comment.
When the webcam recorder is using disk streaming and a chunk write fails, recordedBlobPromise is swallowed to null here, so webcam is omitted from storeRecordedSession while the screen path can still succeed. In that path savedToDisk becomes true later, so the finally block skips activeWebcamRecorder.discard(), leaving the webcam stream/file unclosed and orphaned after a partial write failure. Please either fail the whole save or explicitly discard the webcam stream when this branch is hit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/hooks/recorderHandle.ts (1)
77-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
openPromiserejection explicitly.Line 77 currently chains
void openPromise.then(...)without a rejection path; unexpected IPC-level rejection can leave this in a kinda cursed"pending"state and trigger unhandled rejection noise. Add a.catchthat flips to buffering fallback.nit: cleaner/fail-safe promise chain
- void openPromise.then((result) => { - if (result.success) { - streamOpened = true; - mode = "streaming"; - for (const chunk of memoryChunks) { - enqueueWrite(chunk); - } - memoryChunks.length = 0; - } else { - mode = "buffering"; - } - }); + void openPromise + .then((result) => { + if (result.success) { + streamOpened = true; + mode = "streaming"; + for (const chunk of memoryChunks) { + enqueueWrite(chunk); + } + memoryChunks.length = 0; + } else { + mode = "buffering"; + } + }) + .catch(() => { + mode = "buffering"; + });🤖 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/recorderHandle.ts` around lines 77 - 88, The promise openPromise is only handled with then(...) which leaves rejections unhandled; update the chain on the openPromise used around streamOpened/mode/memoryChunks to add a .catch(...) that sets mode = "buffering", ensures streamOpened is false, drains or preserves memoryChunks as before (do not enqueueWrite), and optionally logs the error; specifically modify the openPromise.then(...) callback for the open path (where enqueueWrite is called) to append a .catch that flips to the buffering fallback and prevents unhandled rejection noise.
🧹 Nitpick comments (1)
vitest.config.ts (1)
7-9: 💤 Low valueheads up: electron/ tests running in jsdom might get weird
most electron main-process code uses node APIs (fs, path, etc.) which jsdom doesn't provide natively. if these tests are passing (sounds like they are per the 180/180 report), you're probably mocking things properly — just noting that if future electron tests start acting cursed, you might want per-directory environment overrides:
// alternative: use vitest workspace or inline comments like // `@vitest-environment` nodelowkey fine to leave as-is if it works, just a heads up for the future.
🤖 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 `@vitest.config.ts` around lines 7 - 9, The current Vitest config sets environment: "jsdom" for all tests (see environment in vitest.config.ts) which can break Electron main-process tests that rely on Node APIs; to fix, either add per-directory/targeted overrides in the Vitest configuration to run tests under node for the electron folder (override environment for files matching "{electron}/**/*.{test,spec}..." or similar) or add inline file-level directives in failing electron tests (e.g., "`@vitest-environment` node") so main-process code uses Node APIs; update the include/exclude patterns if needed to ensure electron tests match the node-environment override.
🤖 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/hooks/recorderHandle.ts`:
- Around line 60-69: The writeChain.then block can reject if
api.appendRecordingChunk throws, leaving appendError null and causing
isStreaming() to report true; wrap the await api.appendRecordingChunk(fileName,
buffer) call in a try/catch inside the writeChain.then handler (the function
that currently sets appendError on result.success) so that any thrown error is
caught and assigned to appendError (e.g., appendError = caughtError or a new
Error with message), and do not rethrow so the promise chain stays
settled—update the handler around writeChain, appendError, and
api.appendRecordingChunk to normalize both rejection and { success:false } into
appendError.
---
Duplicate comments:
In `@src/hooks/recorderHandle.ts`:
- Around line 77-88: The promise openPromise is only handled with then(...)
which leaves rejections unhandled; update the chain on the openPromise used
around streamOpened/mode/memoryChunks to add a .catch(...) that sets mode =
"buffering", ensures streamOpened is false, drains or preserves memoryChunks as
before (do not enqueueWrite), and optionally logs the error; specifically modify
the openPromise.then(...) callback for the open path (where enqueueWrite is
called) to append a .catch that flips to the buffering fallback and prevents
unhandled rejection noise.
---
Nitpick comments:
In `@vitest.config.ts`:
- Around line 7-9: The current Vitest config sets environment: "jsdom" for all
tests (see environment in vitest.config.ts) which can break Electron
main-process tests that rely on Node APIs; to fix, either add
per-directory/targeted overrides in the Vitest configuration to run tests under
node for the electron folder (override environment for files matching
"{electron}/**/*.{test,spec}..." or similar) or add inline file-level directives
in failing electron tests (e.g., "`@vitest-environment` node") so main-process
code uses Node APIs; update the include/exclude patterns if needed to ensure
electron tests match the node-environment override.
🪄 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: c131ee22-9b2c-47a0-a447-6c7543da557d
📒 Files selected for processing (11)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/ipc/recordingStream.test.tselectron/ipc/recordingStream.tselectron/preload.tselectron/recording/webm-duration.tssrc/hooks/recorderHandle.test.tssrc/hooks/recorderHandle.tssrc/hooks/useScreenRecorder.tssrc/lib/recordingSession.tsvitest.config.ts
Addresses the CodeRabbit + Codex re-review of the prior commit.
- Normalize a rejected append (channel/handler error, not just a
{ success: false } result) into appendError, so the write queue never
rejects and isStreaming() stays consistent after a failure (CodeRabbit).
- Handle a rejected open-stream IPC the same as a failed open: fall back
to in-memory buffering instead of leaving the recorder stuck "pending"
with an unhandled rejection (CodeRabbit).
- Discard a streamed webcam whose write failed even when the screen save
succeeds. The cleanup gate is now per-recorder, so a webcam omitted from
a successful screen-only save no longer leaks its stream and partial
file (Codex).
Adds tests for the rejected-append and rejected-open paths.
Verified: tsc --noEmit clean; biome clean; vitest 182/182.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed
Verified locally: tsc clean, biome clean, vitest 182/182 (2 new tests). @coderabbitai full review |
|
🐇 ✨ ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36d7d2bdd0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fileName && api?.openRecordingStream | ||
| ? api.openRecordingStream(fileName) | ||
| : Promise.resolve({ success: false }); |
There was a problem hiding this comment.
If openRecordingStream exists but appendRecordingChunk is unavailable (for example during renderer/main version skew), this path still opens the stream and switches mode to "streaming". In that state enqueueWrite immediately returns on every chunk because appendRecordingChunk is missing, buffered chunks are cleared, and recordedBlobPromise resolves as if streaming succeeded, producing an empty/truncated saved recording instead of falling back to in-memory buffering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/recorderHandle.ts (1)
141-145: 💤 Low valuelowkey risky if called before
recordedBlobPromisesettlesif someone calls
discard()whileopenPromiseis still pending,streamOpenedis false and we return early — then the stream opens successfully afterward and gets orphaned. yes the docstring implies calling after the promise settles, but adding a defensive await makes this bulletproof.🛡️ defensive fix
async function discard(): Promise<void> { + await openPromise.catch(() => undefined); if (streamOpened && fileName && api?.closeRecordingStream) { await api.closeRecordingStream(fileName); } }🤖 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/recorderHandle.ts` around lines 141 - 145, discard() can return early if called before the recording/open promise settles, leaving a later-opened stream orphaned; to fix, have discard await the promise that signals the stream open (e.g., await openPromise or recordedBlobPromise) before checking streamOpened and fileName, then proceed to call api.closeRecordingStream(fileName) if streamOpened and fileName are set; update the discard function to await the appropriate Promise (openPromise/recordedBlobPromise) and keep the existing conditional and await of api.closeRecordingStream(fileName) to ensure no orphaned streams.
🤖 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.
Nitpick comments:
In `@src/hooks/recorderHandle.ts`:
- Around line 141-145: discard() can return early if called before the
recording/open promise settles, leaving a later-opened stream orphaned; to fix,
have discard await the promise that signals the stream open (e.g., await
openPromise or recordedBlobPromise) before checking streamOpened and fileName,
then proceed to call api.closeRecordingStream(fileName) if streamOpened and
fileName are set; update the discard function to await the appropriate Promise
(openPromise/recordedBlobPromise) and keep the existing conditional and await of
api.closeRecordingStream(fileName) to ensure no orphaned streams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04073d80-652d-47ae-8f96-672cab1bb530
📒 Files selected for processing (3)
src/hooks/recorderHandle.test.tssrc/hooks/recorderHandle.tssrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/recorderHandle.test.ts
- src/hooks/useScreenRecorder.ts
Codex re-review: if openRecordingStream exists but appendRecordingChunk does not (renderer/main version skew), the recorder would open the stream and switch to streaming mode, but every append silently no-ops and the save ends up empty. Require both IPC methods before streaming; otherwise fall back to in-memory buffering. Adds a regression test. Verified: tsc --noEmit clean; biome clean; vitest 183/183. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed tsc clean, biome clean, vitest 183/183. @coderabbitai full review |
|
🐇 ✨ ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/hooks/recorderHandle.ts (1)
141-145: 💤 Low valuesmall edge case around early cancellation
discard()checksstreamOpenedbefore callingcloseRecordingStream, which means if someone callsdiscard()whileopenPromiseis still pending and it later succeeds, the stream gets orphaned.in normal usage (discard after stop + await recordedBlobPromise), this is fine because
finalizeBlobawaitsopenPromisefirst. but if you ever need early cancellation (user hits cancel mid-recording before any chunks flush), you might want toawait openPromise.catch(() => undefined)at the top ofdiscard()too.probably not urgent since the main process will eventually GC the WriteStream, but worth noting if early cancellation becomes a feature.
safer discard for early cancellation (if needed)
async function discard(): Promise<void> { + await openPromise.catch(() => undefined); if (streamOpened && fileName && api?.closeRecordingStream) { await api.closeRecordingStream(fileName); } }🤖 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/recorderHandle.ts` around lines 141 - 145, The discard() function can orphan the recording stream if called before openPromise resolves; update discard() to first await openPromise.catch(() => undefined) (or otherwise await it safely) before checking streamOpened/fileName and calling api.closeRecordingStream, so that if openPromise later sets streamOpened and fileName the cleanup still runs; reference the discard(), openPromise, streamOpened, fileName and api.closeRecordingStream symbols when making this change.vitest.config.ts (1)
8-8: 💤 Low valuejsdom environment for electron/ tests — should be fine but worth noting
the electron/ tests now run under jsdom which works for the registry/IPC handler tests since they mock
fsand don't need actual Electron runtime. if future tests need real Electron APIs, consider a separate config or environment override. lowkey fine for now though.🤖 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 `@vitest.config.ts` at line 8, The current vitest include pattern "{src,electron}/**/*.{test,spec}..." causes electron/* tests to run under the default jsdom environment which is fine now but may break tests needing real Electron APIs; update vitest.config.ts to explicitly map the electron test files to an appropriate environment (e.g., use environmentMatch or a separate test config so electron/** uses "node" or an Electron-specific env) or split electron tests into a separate Vitest config so tests that require real Electron APIs don't run under jsdom; refer to the include pattern and the vitest config export to locate where to add the environmentMatch or alternate config.
🤖 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.
Nitpick comments:
In `@src/hooks/recorderHandle.ts`:
- Around line 141-145: The discard() function can orphan the recording stream if
called before openPromise resolves; update discard() to first await
openPromise.catch(() => undefined) (or otherwise await it safely) before
checking streamOpened/fileName and calling api.closeRecordingStream, so that if
openPromise later sets streamOpened and fileName the cleanup still runs;
reference the discard(), openPromise, streamOpened, fileName and
api.closeRecordingStream symbols when making this change.
In `@vitest.config.ts`:
- Line 8: The current vitest include pattern
"{src,electron}/**/*.{test,spec}..." causes electron/* tests to run under the
default jsdom environment which is fine now but may break tests needing real
Electron APIs; update vitest.config.ts to explicitly map the electron test files
to an appropriate environment (e.g., use environmentMatch or a separate test
config so electron/** uses "node" or an Electron-specific env) or split electron
tests into a separate Vitest config so tests that require real Electron APIs
don't run under jsdom; refer to the include pattern and the vitest config export
to locate where to add the environmentMatch or alternate config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7514b753-618e-4ff2-9bf0-14caaff81a5a
📒 Files selected for processing (11)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/ipc/recordingStream.test.tselectron/ipc/recordingStream.tselectron/preload.tselectron/recording/webm-duration.tssrc/hooks/recorderHandle.test.tssrc/hooks/recorderHandle.tssrc/hooks/useScreenRecorder.tssrc/lib/recordingSession.tsvitest.config.ts
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vitest.config.ts (1)
8-8: 💤 Low valuenit: electron tests probably don't need jsdom
the electron/ tests are likely pure node (no DOM), so running them under jsdom adds overhead. you could split environments by path pattern if perf becomes noticeable, but it's not blocking since tests pass.
// example if you ever care: test: { environmentMatchGlobs: [ ['electron/**', 'node'], ['src/**', 'jsdom'], ], }🤖 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 `@vitest.config.ts` at line 8, The electron tests under the include pattern "{src,electron}/**/*.{test,spec}..." are running in the default jsdom environment; update the vitest config to assign a Node environment for electron tests by adding a test.environmentMatchGlobs entry (e.g., map 'electron/**' -> 'node' and keep 'src/**' -> 'jsdom') so electron/* files run under Node and avoid jsdom overhead; modify the test config block to include environmentMatchGlobs accordingly.
🤖 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/recordingStream.ts`:
- Around line 83-86: In discard, don't swallow all unlink errors; after awaiting
this.endStream(fileName) catch unlink errors and only ignore those with err.code
=== 'ENOENT', rethrow any other error so delete failures surface to the caller
(update the error handler for unlink in the discard method to check err.code and
rethrow non-ENOENT errors) — this keeps the close-recording-stream flow from
reporting success when a real delete failed.
---
Nitpick comments:
In `@vitest.config.ts`:
- Line 8: The electron tests under the include pattern
"{src,electron}/**/*.{test,spec}..." are running in the default jsdom
environment; update the vitest config to assign a Node environment for electron
tests by adding a test.environmentMatchGlobs entry (e.g., map 'electron/**' ->
'node' and keep 'src/**' -> 'jsdom') so electron/* files run under Node and
avoid jsdom overhead; modify the test config block to include
environmentMatchGlobs accordingly.
🪄 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: 88726d70-fe66-49a9-8763-9c77cd13f11f
📒 Files selected for processing (11)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/ipc/recordingStream.test.tselectron/ipc/recordingStream.tselectron/preload.tselectron/recording/webm-duration.tssrc/hooks/recorderHandle.test.tssrc/hooks/recorderHandle.tssrc/hooks/useScreenRecorder.tssrc/lib/recordingSession.tsvitest.config.ts
| async discard(fileName: string, filePath: string): Promise<void> { | ||
| await this.endStream(fileName); | ||
| await unlink(filePath).catch(() => undefined); | ||
| } |
There was a problem hiding this comment.
Discard path hides real delete failures
lowkey risky: Line 85 swallows all unlink errors, so close-recording-stream can report success while a partial recording still exists on disk. Ignore only ENOENT; rethrow the rest.
Suggested fix
async discard(fileName: string, filePath: string): Promise<void> {
await this.endStream(fileName);
- await unlink(filePath).catch(() => undefined);
+ try {
+ await unlink(filePath);
+ } catch (error) {
+ const err = error as NodeJS.ErrnoException;
+ if (err.code !== "ENOENT") {
+ throw 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/recordingStream.ts` around lines 83 - 86, In discard, don't
swallow all unlink errors; after awaiting this.endStream(fileName) catch unlink
errors and only ignore those with err.code === 'ENOENT', rethrow any other error
so delete failures surface to the caller (update the error handler for unlink in
the discard method to check err.code and rethrow non-ENOENT errors) — this keeps
the close-recording-stream flow from reporting success when a real delete
failed.
|
@codex review (Final commit is |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
2 participants
What this fixes
Recordings longer than ~10 minutes silently fail to save (#616). The renderer buffers the whole WebM as a
Blob[], then on stop makes several in-memory copies (fixWebmDuration→arrayBuffer→Buffer.from) before writing. A long 1080p recording duplicates hundreds of MB several times in the renderer, exceeds Electron's renderer memory limit, and the renderer crashes silently — no file saved, no error surfaced.The fix (two parts)
1. Stream chunks to disk — originally @Amanuel2x's contribution in #617, preserved here with credit. An
fs.WriteStreamopens in the main process at recording start; each ~1sondataavailablechunk goes straight to disk over two new IPC calls (open-recording-stream,append-recording-chunk), so the renderer never holds more than a single chunk. A full in-memory fallback is preserved for environments where the IPC stream can't open.2. Patch the WebM Duration header on disk after the stream closes. Browser
MediaRecorderwrites WebM with no Duration element, so streamed files would otherwise save withduration=N/Aand the editor's seek bar, timeline, and any scrub/trim break. The pre-streaming code patched the in-renderer blob; with streaming the renderer never holds the bytes, so a newelectron/recording/webm-duration.tsmodule rewrites the Duration element on disk. It writes to a temp file and renames in place, so a crash mid-write can never corrupt the recording — the original survives intact. Any read/parse/write failure logs and returns a non-fatal result rather than throwing; the file is still playable, the patch is best-effort.Streaming is opt-in. The screen recorder and the browser-only webcam recorder stream to disk. Native-capture webcam sidecars (Windows, macOS) keep buffering in memory, because their finalize path reads the recorder blob directly to attach the webcam track — streaming those would silently drop the webcam. Making streaming opt-in (a recorder only streams when given a
recordingId+fileName) closes that gap.Verified
tsc --noEmitcleanbiome checkcleanvitest166/166 passingmain(5467796); single squashed commit.Not verified remotely (worth a smoke-test before/after merge)
I built and verified this from a remote session, so I could not run a real long recording end-to-end on a desktop. No unit test exercises the Electron
fs.WriteStreamIPC path or the native-webcam finalize path. Before relying on it, worth confirming on a desktop:Notes
Closes #616,Supersedes #617.Co-Authored-Byon the commit.0x8538067/0x549a966) against@fix-webm-duration/parser'ssrc/lib/sections.js— they're the length-descriptor-stripped form the parser uses as lookup keys, not the canonical 4-byte IDs.@siddharthvaddem — fresh single-commit PR off current
main, per your ask. Happy to adjust anything.Summary by CodeRabbit