Addresses the review feedback on #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>
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 memory limit, and the renderer
crashes silently with no file saved.
Two changes:
1. Stream chunks to disk (originally @Amanuel2x's contribution in #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#616
Supersedes #617
Co-Authored-By: Amanuel <amanuel@localboostnetworking.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
discardLatestPending() popped whichever batch happened to be at the
back of the queue. With a Stop → Record → Discard sequence, the
pending queue can have recording B's batch sitting in front of A's by
the time A's finalize callback resolves (because finalizeRecording
awaits fixWebmDuration), so the discard targets the wrong recording.
Tag each completed batch with the recording id supplied at
startSession() time and replace discardLatestPending() with
discardBatch(recordingId). takeNextBatch() now returns the full
{recordingId, samples} shape so prependBatch() can re-queue it on
write-failure without losing the id. The renderer already owns a
stable recordingId (Date.now() in useScreenRecorder) and the IPC
surface threads it through set-recording-state and
discard-cursor-telemetry.
Adds a regression test that mirrors FabLrc's scenario in PR #457:
two recordings finalize, A is discarded after B has already been
queued, and the buffer must drop A while keeping B intact.
Every consumer of /wallpapers/*.jpg — SettingsPanel, VideoPlayback,
frameRenderer — was doing async IPC round trips, useEffect dances, and
Promise.all for a value that is a build-time constant per process. Each
consumer showed briefly-empty or briefly-404ing state on first paint
until the handler's reply resolved.
The asset base URL depends only on process.defaultApp and
process.resourcesPath / __dirname — all available in preload at
context-bridge time. Compute once there, expose as a sync string.
- preload.ts resolves baseDir (process.resourcesPath packaged,
<appRoot>/public unpackaged) and emits assetBaseUrl synchronously.
- get-asset-base-path IPC handler + main-process branching deleted.
- getAssetPath() is now sync. Returns string, not Promise<string>.
Throws AssetBaseUnavailableError (new) when electronAPI.assetBaseUrl
is missing — catastrophic preload failure, not silent 404.
- resolveImageWallpaperUrl() sync; same sync throw semantics.
- SettingsPanel: Promise.all + useState + useEffect collapse to one
useMemo. First paint has real URLs, no 18× ERR_FILE_NOT_FOUND, no
flicker.
- VideoPlayback: wallpaper-resolve useEffect collapses to useMemo.
- frameRenderer.setupBackground: drops the await.
- electronAPI type decls updated in both .d.ts files.
- 35 unit tests updated to reflect sync signature + new
AssetBaseUnavailableError contract.
Silent-fallback behavior from getAssetPath (returning /relative when
electronAPI failed) is gone. Renderers now surface preload failures
instead of rendering 404s.
Address two issues raised during review:
P1 – When a recording is cancelled or restarted, setRecordingState(false)
enqueues its cursor batch but store-recorded-session is never called,
leaving a stale batch that contaminates the next recording's telemetry.
Add discardLatestPending() to the buffer and a discard-cursor-telemetry
IPC handler; the renderer now calls it on the discard path.
P2 – takeNextBatch() dequeued the batch before fs.writeFile, so a write
failure would permanently lose the telemetry. Wrap the write in
try/catch and re-insert the batch via prependBatch() on failure.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
huanld