From f3c5b8a65d554216ba7e2182571f7b5c675705a2 Mon Sep 17 00:00:00 2001 From: neurot1cal Date: Tue, 26 May 2026 16:09:39 -0700 Subject: [PATCH] fix: harden streaming lifecycle and lift it out of the IPC god-module 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) --- electron/electron-env.d.ts | 8 +- electron/ipc/handlers.ts | 139 ++++++++---------- electron/ipc/recordingStream.test.ts | 84 +++++++++++ electron/ipc/recordingStream.ts | 147 ++++++++++++++++++++ electron/preload.ts | 11 +- src/hooks/recorderHandle.test.ts | 201 +++++++++++++++++++++++++++ src/hooks/recorderHandle.ts | 140 +++++++++++++++++++ src/hooks/useScreenRecorder.ts | 118 +++------------- vitest.config.ts | 2 +- 9 files changed, 658 insertions(+), 192 deletions(-) create mode 100644 electron/ipc/recordingStream.test.ts create mode 100644 electron/ipc/recordingStream.ts create mode 100644 src/hooks/recorderHandle.test.ts create mode 100644 src/hooks/recorderHandle.ts diff --git a/electron/electron-env.d.ts b/electron/electron-env.d.ts index cbe43be..b86c4ff 100644 --- a/electron/electron-env.d.ts +++ b/electron/electron-env.d.ts @@ -81,14 +81,12 @@ interface Window { message?: string; error?: string; }>; - openRecordingStream: ( - recordingId: number, - fileName: string, - ) => Promise<{ success: boolean; error?: string }>; + openRecordingStream: (fileName: string) => Promise<{ success: boolean; error?: string }>; appendRecordingChunk: ( - recordingId: number, + fileName: string, chunk: ArrayBuffer, ) => Promise<{ success: boolean; error?: string }>; + closeRecordingStream: (fileName: string) => Promise<{ success: boolean; error?: string }>; getRecordedVideoPath: () => Promise<{ success: boolean; path?: string; diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index f10cbbc..d367728 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -1,6 +1,6 @@ import { type ChildProcessWithoutNullStreams, spawn } from "node:child_process"; import { EventEmitter } from "node:events"; -import { createWriteStream, constants as fsConstants, type WriteStream } from "node:fs"; +import { constants as fsConstants } from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -42,6 +42,7 @@ import { requestMacCursorAccessibilityAccess } from "../native-bridge/cursor/rec import type { CursorRecordingSession } from "../native-bridge/cursor/recording/session"; import { patchWebmDurationOnDisk } from "../recording/webm-duration"; import { registerNativeBridgeHandlers } from "./nativeBridge"; +import { RecordingStreamRegistry, registerRecordingStreamHandlers } from "./recordingStream"; const PROJECT_FILE_EXTENSION = "openscreen"; const SHORTCUTS_FILE = path.join(app.getPath("userData"), "shortcuts.json"); @@ -266,6 +267,30 @@ function resolveRecordingOutputPath(fileName: string): string { return path.join(RECORDINGS_DIR, parsedPath.base); } +function isValidDurationMs(value: number | undefined): value is number { + return typeof value === "number" && Number.isFinite(value) && value > 0; +} + +/** + * Finalize a single recording file: if it was streamed to disk, flush and close + * the stream; otherwise (a short recording, or the stream failed to open and the + * renderer fell back to in-memory buffering) write the buffered bytes. Returns + * whether the file was streamed, which the caller uses to decide whether the + * WebM duration needs patching on disk. + */ +async function finalizeRecordingFile( + registry: RecordingStreamRegistry, + fileName: string, + filePath: string, + videoData?: ArrayBuffer, +): Promise { + const streamed = await registry.finalize(fileName); + if (!streamed && videoData && videoData.byteLength > 0) { + await fs.writeFile(filePath, Buffer.from(videoData)); + } + return streamed; +} + async function getApprovedProjectSession( project: unknown, projectFilePath?: string, @@ -2142,46 +2167,11 @@ export function registerIpcHandlers( }, ); - // Streaming chunk writers — keyed by recordingId. Chunks are appended directly - // to disk as they arrive from ondataavailable so the renderer never holds the - // full video in memory. - const activeWriteStreams = new Map(); - - ipcMain.handle( - "open-recording-stream", - async ( - _, - recordingId: number, - fileName: string, - ): Promise<{ success: boolean; error?: string }> => { - 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) }; - } - }, - ); - - ipcMain.handle( - "append-recording-chunk", - async ( - _, - recordingId: number, - chunk: ArrayBuffer, - ): Promise<{ success: boolean; error?: string }> => { - const ws = activeWriteStreams.get(recordingId); - if (!ws) return { success: false, error: "No active stream for recordingId " + recordingId }; - return new Promise((resolve) => { - ws.write(Buffer.from(chunk), (err) => { - if (err) resolve({ success: false, error: err.message }); - else resolve({ success: true }); - }); - }); - }, - ); + // On-disk write streams for in-progress recordings, keyed by output file name. + // Chunks are appended as they arrive from ondataavailable so the renderer + // never buffers the full video in memory (the #616 fix). + const recordingStreams = new RecordingStreamRegistry(); + registerRecordingStreamHandlers(ipcMain, recordingStreams, resolveRecordingOutputPath); ipcMain.handle("store-recorded-session", async (_, payload: StoreRecordedSessionInput) => { try { @@ -2203,56 +2193,37 @@ export function registerIpcHandlers( : Date.now(); const cursorCaptureMode = normalizeCursorCaptureMode(payload.cursorCaptureMode); const screenVideoPath = resolveRecordingOutputPath(payload.screen.fileName); - - // 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); - let screenStreamed = false; - if (screenWs) { - await new Promise((resolve, reject) => - screenWs.end((err?: Error | null) => (err ? reject(err) : resolve())), - ); - activeWriteStreams.delete(createdAt); - screenStreamed = true; - } else if (payload.screen.videoData && payload.screen.videoData.byteLength > 0) { - await fs.writeFile(screenVideoPath, Buffer.from(payload.screen.videoData)); - } + const screenStreamed = await finalizeRecordingFile( + recordingStreams, + payload.screen.fileName, + screenVideoPath, + payload.screen.videoData, + ); let webcamVideoPath: string | undefined; let webcamStreamed = false; if (payload.webcam) { webcamVideoPath = resolveRecordingOutputPath(payload.webcam.fileName); - const webcamWs = activeWriteStreams.get(createdAt + 1); // webcam stream keyed as recordingId+1 - if (webcamWs) { - await new Promise((resolve, reject) => - webcamWs.end((err?: Error | null) => (err ? reject(err) : resolve())), - ); - activeWriteStreams.delete(createdAt + 1); - webcamStreamed = true; - } else if (payload.webcam.videoData && payload.webcam.videoData.byteLength > 0) { - await fs.writeFile(webcamVideoPath, Buffer.from(payload.webcam.videoData)); - } + webcamStreamed = await finalizeRecordingFile( + recordingStreams, + payload.webcam.fileName, + webcamVideoPath, + payload.webcam.videoData, + ); } - // Streamed files lack the WebM Duration header (renderer no longer holds the - // blob to patch). Patch on disk so the editor's seek bar and timeline work. - // Best-effort: log on failure but don't block, since the file is still playable. - if ( - screenStreamed && - typeof payload.durationMs === "number" && - Number.isFinite(payload.durationMs) && - payload.durationMs > 0 - ) { - await patchWebmDurationOnDisk(screenVideoPath, payload.durationMs); - } - if ( - webcamStreamed && - webcamVideoPath && - typeof payload.durationMs === "number" && - Number.isFinite(payload.durationMs) && - payload.durationMs > 0 - ) { - await patchWebmDurationOnDisk(webcamVideoPath, payload.durationMs); + // Streamed files lack the WebM Duration header (the renderer no longer holds + // the blob to patch). Patch on disk so the editor's seek bar and timeline + // work. Best-effort and independent per file, so the patches run together. + if (isValidDurationMs(payload.durationMs)) { + const patches: Promise[] = []; + if (screenStreamed) { + patches.push(patchWebmDurationOnDisk(screenVideoPath, payload.durationMs)); + } + if (webcamStreamed && webcamVideoPath) { + patches.push(patchWebmDurationOnDisk(webcamVideoPath, payload.durationMs)); + } + await Promise.all(patches); } const session: RecordingSession = webcamVideoPath diff --git a/electron/ipc/recordingStream.test.ts b/electron/ipc/recordingStream.test.ts new file mode 100644 index 0000000..776fcf1 --- /dev/null +++ b/electron/ipc/recordingStream.test.ts @@ -0,0 +1,84 @@ +import { mkdtemp, readFile, rm, stat } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { RecordingStreamRegistry } from "./recordingStream"; + +describe("RecordingStreamRegistry", () => { + let dir: string; + const pathFor = (name: string) => path.join(dir, name); + + beforeEach(async () => { + dir = await mkdtemp(path.join(tmpdir(), "openscreen-stream-")); + }); + + afterEach(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + it("streams chunks to disk in order and reports streamed on finalize", async () => { + const registry = new RecordingStreamRegistry(); + await registry.open("rec.webm", pathFor("rec.webm")); + await registry.append("rec.webm", Buffer.from("hello ")); + await registry.append("rec.webm", Buffer.from("world")); + + const streamed = await registry.finalize("rec.webm"); + + expect(streamed).toBe(true); + expect(await readFile(pathFor("rec.webm"), "utf8")).toBe("hello world"); + // A second finalize has nothing to close. + expect(await registry.finalize("rec.webm")).toBe(false); + }); + + it("reports not-streamed when no stream was opened", async () => { + const registry = new RecordingStreamRegistry(); + expect(await registry.finalize("missing.webm")).toBe(false); + expect(registry.has("missing.webm")).toBe(false); + }); + + it("rejects open when the target path is not writable (open is awaited, not assumed)", async () => { + const registry = new RecordingStreamRegistry(); + // Parent directory does not exist, so createWriteStream emits 'error' on open. + await expect( + registry.open("rec.webm", path.join(dir, "does-not-exist", "rec.webm")), + ).rejects.toThrow(); + // A failed open must not register a stream the renderer would treat as live. + expect(registry.has("rec.webm")).toBe(false); + }); + + it("rejects append when no stream is open", async () => { + const registry = new RecordingStreamRegistry(); + await expect(registry.append("rec.webm", Buffer.from("x"))).rejects.toThrow( + /No active recording stream/, + ); + }); + + it("discard closes the stream and removes the partial file", async () => { + const registry = new RecordingStreamRegistry(); + await registry.open("rec.webm", pathFor("rec.webm")); + await registry.append("rec.webm", Buffer.from("partial")); + + await registry.discard("rec.webm", pathFor("rec.webm")); + + expect(registry.has("rec.webm")).toBe(false); + await expect(stat(pathFor("rec.webm"))).rejects.toThrow(); + // Nothing left to finalize after a discard. + expect(await registry.finalize("rec.webm")).toBe(false); + }); + + it("discard tolerates a missing file", async () => { + const registry = new RecordingStreamRegistry(); + await expect(registry.discard("never.webm", pathFor("never.webm"))).resolves.toBeUndefined(); + }); + + it("opening the same file twice replaces the prior stream", async () => { + const registry = new RecordingStreamRegistry(); + await registry.open("rec.webm", pathFor("rec.webm")); + await registry.append("rec.webm", Buffer.from("first")); + await registry.open("rec.webm", pathFor("rec.webm")); + await registry.append("rec.webm", Buffer.from("second")); + await registry.finalize("rec.webm"); + + expect(await readFile(pathFor("rec.webm"), "utf8")).toBe("second"); + }); +}); diff --git a/electron/ipc/recordingStream.ts b/electron/ipc/recordingStream.ts new file mode 100644 index 0000000..3dce5b9 --- /dev/null +++ b/electron/ipc/recordingStream.ts @@ -0,0 +1,147 @@ +import { createWriteStream, type WriteStream } from "node:fs"; +import { unlink } from "node:fs/promises"; +import type { IpcMain } from "electron"; + +/** + * Owns the lifecycle of on-disk write streams for in-progress recordings, keyed + * by the recording's output file name. Browser MediaRecorder chunks are appended + * here as they arrive so a long recording never buffers the whole video in the + * renderer (the #616 fix). + * + * The file name is the key because it is the one value the renderer and main + * process already exchange and it is globally unique per recording, so there is + * no derived/offset key to keep in sync across the IPC boundary. + */ +export class RecordingStreamRegistry { + private readonly streams = new Map(); + + /** + * Open a write stream and resolve only once the OS confirms it is writable. + * Resolving on the `open` event (rather than on `createWriteStream` returning) + * means a bad path or permission error rejects here instead of surfacing as a + * silent chunk drop later, so the renderer's fallback can take over. + */ + async open(fileName: string, filePath: string): Promise { + await this.endStream(fileName); + + const ws = createWriteStream(filePath, { flags: "w" }); + await new Promise((resolve, reject) => { + const onError = (error: Error) => reject(error); + ws.once("error", onError); + ws.once("open", () => { + ws.removeListener("error", onError); + resolve(); + }); + }); + // Keep a listener for the stream's lifetime so a late error logs rather + // than crashing the main process with an unhandled 'error' event. Per-write + // failures still surface through the `append` callback below. + ws.on("error", (error) => { + console.error(`[recording-stream] ${fileName}:`, error); + }); + + this.streams.set(fileName, ws); + } + + has(fileName: string): boolean { + return this.streams.has(fileName); + } + + /** Append a chunk; rejects if no stream is open or the write fails. */ + async append(fileName: string, chunk: Buffer): Promise { + const ws = this.streams.get(fileName); + if (!ws) { + throw new Error(`No active recording stream for ${fileName}`); + } + await new Promise((resolve, reject) => { + ws.write(chunk, (error) => (error ? reject(error) : resolve())); + }); + } + + /** + * Flush and close the stream, keeping the file. Returns whether a stream was + * open — i.e. whether the recording was streamed to disk (true) or needs its + * in-memory buffer written by the caller (false). + */ + async finalize(fileName: string): Promise { + const ws = this.streams.get(fileName); + if (!ws) { + return false; + } + this.streams.delete(fileName); + await new Promise((resolve, reject) => { + ws.end((error?: Error | null) => (error ? reject(error) : resolve())); + }); + return true; + } + + /** + * Close the stream (if any) and delete the partial file. Used when a streamed + * recording is discarded or fails before a successful save, so cancelled runs + * don't leak file descriptors or orphan partial recordings on disk. + */ + async discard(fileName: string, filePath: string): Promise { + await this.endStream(fileName); + await unlink(filePath).catch(() => undefined); + } + + private async endStream(fileName: string): Promise { + const ws = this.streams.get(fileName); + if (!ws) { + return; + } + this.streams.delete(fileName); + await new Promise((resolve) => ws.end(() => resolve())); + } +} + +/** + * Register the streaming IPC handlers. Thin wrappers that translate the + * registry's throw-on-failure contract into the `{ success, error }` shape the + * renderer expects. + */ +export function registerRecordingStreamHandlers( + ipcMain: IpcMain, + registry: RecordingStreamRegistry, + resolveRecordingOutputPath: (fileName: string) => string, +): void { + ipcMain.handle( + "open-recording-stream", + async (_, fileName: string): Promise<{ success: boolean; error?: string }> => { + try { + await registry.open(fileName, resolveRecordingOutputPath(fileName)); + return { success: true }; + } catch (error) { + return { success: false, error: String(error) }; + } + }, + ); + + ipcMain.handle( + "append-recording-chunk", + async ( + _, + fileName: string, + chunk: ArrayBuffer, + ): Promise<{ success: boolean; error?: string }> => { + try { + await registry.append(fileName, Buffer.from(chunk)); + return { success: true }; + } catch (error) { + return { success: false, error: String(error) }; + } + }, + ); + + ipcMain.handle( + "close-recording-stream", + async (_, fileName: string): Promise<{ success: boolean; error?: string }> => { + try { + await registry.discard(fileName, resolveRecordingOutputPath(fileName)); + return { success: true }; + } catch (error) { + return { success: false, error: String(error) }; + } + }, + ); +} diff --git a/electron/preload.ts b/electron/preload.ts index a7283d8..4b69740 100644 --- a/electron/preload.ts +++ b/electron/preload.ts @@ -64,11 +64,14 @@ contextBridge.exposeInMainWorld("electronAPI", { storeRecordedSession: (payload: StoreRecordedSessionInput) => { return ipcRenderer.invoke("store-recorded-session", payload); }, - openRecordingStream: (recordingId: number, fileName: string) => { - return ipcRenderer.invoke("open-recording-stream", recordingId, fileName); + openRecordingStream: (fileName: string) => { + return ipcRenderer.invoke("open-recording-stream", fileName); }, - appendRecordingChunk: (recordingId: number, chunk: ArrayBuffer) => { - return ipcRenderer.invoke("append-recording-chunk", recordingId, chunk); + appendRecordingChunk: (fileName: string, chunk: ArrayBuffer) => { + return ipcRenderer.invoke("append-recording-chunk", fileName, chunk); + }, + closeRecordingStream: (fileName: string) => { + return ipcRenderer.invoke("close-recording-stream", fileName); }, getRecordedVideoPath: () => { diff --git a/src/hooks/recorderHandle.test.ts b/src/hooks/recorderHandle.test.ts new file mode 100644 index 0000000..3551b7d --- /dev/null +++ b/src/hooks/recorderHandle.test.ts @@ -0,0 +1,201 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createRecorderHandle } from "./recorderHandle"; + +type ElectronAPI = Window["electronAPI"]; + +const tick = () => new Promise((resolve) => setTimeout(resolve, 0)); +const decode = (buffer: ArrayBuffer) => new TextDecoder().decode(new Uint8Array(buffer)); + +/** Minimal MediaRecorder stand-in the tests can drive directly. */ +class FakeMediaRecorder { + ondataavailable: ((event: BlobEvent) => void) | null = null; + onstop: (() => void) | null = null; + onerror: (() => void) | null = null; + state: "inactive" | "recording" = "inactive"; + + start(): void { + this.state = "recording"; + } + + stop(): void { + this.state = "inactive"; + this.onstop?.(); + } + + emit(data: Blob): void { + this.ondataavailable?.({ data } as BlobEvent); + } +} + +function stubElectronAPI(api: Partial): void { + window.electronAPI = api as unknown as ElectronAPI; +} + +function driver(handle: { recorder: MediaRecorder }): FakeMediaRecorder { + return handle.recorder as unknown as FakeMediaRecorder; +} + +describe("createRecorderHandle", () => { + beforeEach(() => { + vi.stubGlobal("MediaRecorder", FakeMediaRecorder); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + window.electronAPI = undefined as unknown as ElectronAPI; + }); + + it("streams chunks to disk in arrival order and resolves an empty blob", async () => { + const appended: string[] = []; + const openRecordingStream = vi.fn(async () => ({ success: true })); + const appendRecordingChunk = vi.fn(async (_fileName: string, buffer: ArrayBuffer) => { + appended.push(decode(buffer)); + return { success: true }; + }); + stubElectronAPI({ openRecordingStream, appendRecordingChunk }); + + const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }, "rec.webm"); + const fake = driver(handle); + + fake.emit(new Blob(["a"])); // arrives before open resolves -> buffered + await tick(); // open resolves -> buffered chunk flushes, mode becomes streaming + fake.emit(new Blob(["b"])); + fake.emit(new Blob(["c"])); + fake.stop(); + + const blob = await handle.recordedBlobPromise; + expect(openRecordingStream).toHaveBeenCalledWith("rec.webm"); + expect(appended).toEqual(["a", "b", "c"]); + expect(blob.size).toBe(0); + expect(handle.isStreaming()).toBe(true); + }); + + it("falls back to a complete in-memory blob when the stream fails to open", async () => { + const openRecordingStream = vi.fn(async () => ({ success: false, error: "nope" })); + const appendRecordingChunk = vi.fn(async () => ({ success: true })); + stubElectronAPI({ openRecordingStream, appendRecordingChunk }); + + const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }, "rec.webm"); + const fake = driver(handle); + + fake.emit(new Blob(["a"])); + await tick(); // open resolves false -> buffering, keep everything in memory + fake.emit(new Blob(["bc"])); + fake.stop(); + + const blob = await handle.recordedBlobPromise; + expect(appendRecordingChunk).not.toHaveBeenCalled(); + expect(handle.isStreaming()).toBe(false); + expect(blob.size).toBe(3); + expect(decode(await blob.arrayBuffer())).toBe("abc"); + }); + + it("waits for in-flight chunk writes before stop resolves (no truncation)", async () => { + let releaseAppend: () => void = () => undefined; + const appendGate = new Promise((resolve) => { + releaseAppend = resolve; + }); + const appendRecordingChunk = vi.fn(async () => { + await appendGate; + return { success: true }; + }); + stubElectronAPI({ + openRecordingStream: vi.fn(async () => ({ success: true })), + appendRecordingChunk, + }); + + const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }, "rec.webm"); + const fake = driver(handle); + + await tick(); // open resolves + fake.emit(new Blob(["a"])); // write blocks on the gate + fake.stop(); + + let resolved = false; + void handle.recordedBlobPromise.then(() => { + resolved = true; + }); + await tick(); + expect(resolved).toBe(false); // must not resolve while the write is in flight + + releaseAppend(); + await handle.recordedBlobPromise; + expect(resolved).toBe(true); + expect(appendRecordingChunk).toHaveBeenCalledTimes(1); + }); + + it("rejects when a chunk fails to write mid-stream", async () => { + stubElectronAPI({ + openRecordingStream: vi.fn(async () => ({ success: true })), + appendRecordingChunk: vi.fn(async () => ({ success: false, error: "disk full" })), + closeRecordingStream: vi.fn(async () => ({ success: true })), + }); + + const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }, "rec.webm"); + const fake = driver(handle); + + await tick(); + fake.emit(new Blob(["a"])); + fake.stop(); + + await expect(handle.recordedBlobPromise).rejects.toThrow(/disk full/); + expect(handle.isStreaming()).toBe(false); + }); + + it("buffers in memory and never opens a stream when no file name is given", async () => { + const openRecordingStream = vi.fn(async () => ({ success: true })); + stubElectronAPI({ + openRecordingStream, + appendRecordingChunk: vi.fn(async () => ({ success: true })), + }); + + const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }); + const fake = driver(handle); + + fake.emit(new Blob(["xy"])); + await tick(); + fake.stop(); + + const blob = await handle.recordedBlobPromise; + expect(openRecordingStream).not.toHaveBeenCalled(); + expect(handle.isStreaming()).toBe(false); + expect(blob.size).toBe(2); + }); + + it("discard closes the disk stream for a streamed recording", async () => { + const closeRecordingStream = vi.fn(async () => ({ success: true })); + stubElectronAPI({ + openRecordingStream: vi.fn(async () => ({ success: true })), + appendRecordingChunk: vi.fn(async () => ({ success: true })), + closeRecordingStream, + }); + + const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }, "rec.webm"); + const fake = driver(handle); + await tick(); + fake.emit(new Blob(["a"])); + fake.stop(); + await handle.recordedBlobPromise; + + await handle.discard(); + expect(closeRecordingStream).toHaveBeenCalledWith("rec.webm"); + }); + + it("discard is a no-op when the stream never opened", async () => { + const closeRecordingStream = vi.fn(async () => ({ success: true })); + stubElectronAPI({ + openRecordingStream: vi.fn(async () => ({ success: false })), + appendRecordingChunk: vi.fn(async () => ({ success: true })), + closeRecordingStream, + }); + + const handle = createRecorderHandle({} as MediaStream, { mimeType: "video/webm" }, "rec.webm"); + const fake = driver(handle); + await tick(); + fake.stop(); + await handle.recordedBlobPromise; + + await handle.discard(); + expect(closeRecordingStream).not.toHaveBeenCalled(); + }); +}); diff --git a/src/hooks/recorderHandle.ts b/src/hooks/recorderHandle.ts new file mode 100644 index 0000000..3c63120 --- /dev/null +++ b/src/hooks/recorderHandle.ts @@ -0,0 +1,140 @@ +const RECORDER_TIMESLICE_MS = 1000; + +export type RecorderHandle = { + recorder: MediaRecorder; + /** + * Resolves once the recording has fully drained. For a streamed recording the + * blob is empty (the bytes are already on disk); for an in-memory recording it + * holds the full WebM. Rejects if a chunk failed to write to disk mid-stream, + * so a truncated recording surfaces as an error instead of a silent partial save. + */ + recordedBlobPromise: Promise; + /** + * Whether the recording's bytes went to disk via the streaming path. Computed + * at finalize time rather than construction, so a stream that fails to open is + * correctly reported as not-streamed and its in-memory fallback is used. + */ + isStreaming: () => boolean; + /** + * Close the disk stream (if one opened) and delete its partial file. Called + * when a recording is discarded or fails before a successful save, so cancelled + * runs don't leak the stream or orphan a partial file. No-op for in-memory + * recorders. + */ + discard: () => Promise; +}; + +/** + * Wrap a MediaRecorder, optionally streaming its chunks to disk. + * + * When `fileName` is given, chunks are written to disk in arrival order through + * the main process as they arrive, so a long recording never buffers the whole + * video in the renderer (the #616 fix). Until the disk stream confirms it is + * open, chunks are held in memory; if the open fails, that buffer becomes a + * complete in-memory fallback so nothing is lost. Native-capture webcam sidecars + * omit `fileName` and always buffer in memory, since their finalize path reads + * the blob directly to attach the webcam track. + */ +export function createRecorderHandle( + stream: MediaStream, + options: MediaRecorderOptions, + fileName?: string, +): RecorderHandle { + const recorder = new MediaRecorder(stream, options); + const mimeType = options.mimeType || "video/webm"; + const api = window.electronAPI; + + // Chunks held in memory: everything before the stream opens, plus everything + // when not streaming at all. On a successful open these flush to disk and are + // dropped; on open failure they remain as the complete fallback recording. + const memoryChunks: Blob[] = []; + let mode: "pending" | "streaming" | "buffering" = fileName ? "pending" : "buffering"; + let streamOpened = false; + let appendError: Error | null = null; + + // Serialize chunk writes so they land on disk in arrival order, and so stop + // can await every in-flight write before the main process closes the stream + // (otherwise a late chunk arrives after close and truncates the recording). + let writeChain: Promise = Promise.resolve(); + const enqueueWrite = (chunk: Blob) => { + writeChain = writeChain.then(async () => { + if (appendError || !fileName || !api?.appendRecordingChunk) { + return; + } + const buffer = await chunk.arrayBuffer(); + const result = await api.appendRecordingChunk(fileName, buffer); + if (!result.success) { + appendError = new Error(result.error ?? "Failed to write recording chunk to disk"); + } + }); + }; + + const openPromise: Promise<{ success: boolean; error?: string }> = + fileName && api?.openRecordingStream + ? api.openRecordingStream(fileName) + : Promise.resolve({ success: false }); + + void openPromise.then((result) => { + if (result.success) { + streamOpened = true; + mode = "streaming"; + for (const chunk of memoryChunks) { + enqueueWrite(chunk); + } + memoryChunks.length = 0; + } else { + mode = "buffering"; + } + }); + + const recordedBlobPromise = new Promise((resolve, reject) => { + recorder.ondataavailable = (event: BlobEvent) => { + if (!event.data || event.data.size === 0) { + return; + } + if (mode === "streaming") { + enqueueWrite(event.data); + } else { + // "pending" (stream not open yet) or "buffering" (not streaming). + memoryChunks.push(event.data); + } + }; + + recorder.onerror = () => { + reject(new Error("Recording failed")); + }; + + recorder.onstop = () => { + resolve(finalizeBlob()); + }; + }); + + async function finalizeBlob(): Promise { + // Wait for the open attempt to settle so its flush (or fallback switch) has + // been applied, then for every queued write to land, so we never resolve + // while chunks are still in flight to the about-to-close disk stream. + await openPromise.catch(() => undefined); + await writeChain; + if (appendError) { + throw appendError; + } + if (mode === "streaming") { + return new Blob([], { type: mimeType }); + } + return new Blob(memoryChunks, { type: mimeType }); + } + + async function discard(): Promise { + if (streamOpened && fileName && api?.closeRecordingStream) { + await api.closeRecordingStream(fileName); + } + } + + recorder.start(RECORDER_TIMESLICE_MS); + return { + recorder, + recordedBlobPromise, + isStreaming: () => mode === "streaming" && !appendError, + discard, + }; +} diff --git a/src/hooks/useScreenRecorder.ts b/src/hooks/useScreenRecorder.ts index 665ae5e..d6c03f1 100644 --- a/src/hooks/useScreenRecorder.ts +++ b/src/hooks/useScreenRecorder.ts @@ -13,6 +13,7 @@ import { } from "@/lib/nativeWindowsRecording"; import type { CursorCaptureMode, RecordedVideoAssetInput } from "@/lib/recordingSession"; import { requestCameraAccess } from "@/lib/requestCameraAccess"; +import { createRecorderHandle, type RecorderHandle } from "./recorderHandle"; const TARGET_FRAME_RATE = 60; const MIN_FRAME_RATE = 30; @@ -34,7 +35,6 @@ const DEFAULT_HEIGHT = 1080; const CODEC_ALIGNMENT = 2; -const RECORDER_TIMESLICE_MS = 1000; const BITS_PER_MEGABIT = 1_000_000; const CHROME_MEDIA_SOURCE = "desktop"; const RECORDING_FILE_PREFIX = "recording-"; @@ -74,12 +74,6 @@ type UseScreenRecorderReturn = { setCursorCaptureMode: (mode: CursorCaptureMode) => void; }; -type RecorderHandle = { - recorder: MediaRecorder; - recordedBlobPromise: Promise; - streaming: boolean; -}; - type NativeWindowsRecordingHandle = { recordingId: number; finalizing: boolean; @@ -93,90 +87,6 @@ type NativeMacRecordingHandle = { paused: boolean; }; -function createRecorderHandle( - stream: MediaStream, - options: MediaRecorderOptions, - recordingId?: number, - fileName?: string, -): RecorderHandle { - const recorder = new MediaRecorder(stream, options); - const mimeType = options.mimeType || "video/webm"; - - // Stream chunks to disk only when a target (recordingId + fileName) is given. - // The main screen recorder and the browser-only webcam recorder pass a target - // so long recordings never buffer the whole video in the renderer (the #616 - // fix). Native-capture webcam sidecars omit the target and buffer in-memory, - // because their finalize path reads recordedBlobPromise directly to attach the - // webcam file; an empty streamed blob would silently drop their webcam track. - const streamTarget = - recordingId !== undefined && fileName !== undefined ? { recordingId, fileName } : null; - - const pendingChunks: ArrayBuffer[] = []; - let streamReady = false; - let streamFailed = streamTarget === null; - - if (streamTarget) { - const streamOpenPromise = - window.electronAPI?.openRecordingStream?.(streamTarget.recordingId, streamTarget.fileName) ?? - Promise.resolve({ success: false }); - - streamOpenPromise.then((result) => { - if (result.success) { - streamReady = true; - for (const chunk of pendingChunks) { - void window.electronAPI.appendRecordingChunk(streamTarget.recordingId, chunk); - } - pendingChunks.length = 0; - } else { - streamFailed = true; - } - }); - } - - const fallbackChunks: Blob[] = []; - - const recordedBlobPromise = new Promise((resolve, reject) => { - recorder.ondataavailable = (event: BlobEvent) => { - 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 && streamTarget) { - void window.electronAPI.appendRecordingChunk(streamTarget.recordingId, buf); - } else { - pendingChunks.push(buf); - } - }); - }; - - recorder.onerror = () => { - reject(new Error("Recording failed")); - }; - - recorder.onstop = () => { - 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 })); - } else { - // Streaming succeeded — the main process already has the data on disk. - resolve(new Blob([], { type: mimeType })); - } - }; - }); - - recorder.start(RECORDER_TIMESLICE_MS); - return { recorder, recordedBlobPromise, streaming: !streamFailed }; -} - export function useScreenRecorder(): UseScreenRecorderReturn { const t = useScopedT("editor"); const [recording, setRecording] = useState(false); @@ -418,6 +328,10 @@ export function useScreenRecorder(): UseScreenRecorderReturn { window.electronAPI?.setRecordingState(false); void (async () => { + // Set once the recording is safely stored. Until then any disk stream + // is still open, so the finally block closes it and removes the partial + // file on the discard or error paths. + let savedToDisk = false; try { const screenBlob = await activeScreenRecorder.recordedBlobPromise; if (discardRecordingId.current === activeRecordingId) { @@ -425,16 +339,17 @@ export function useScreenRecorder(): UseScreenRecorderReturn { return; } // When streaming succeeded the blob is empty — the data is already on disk. - if (!activeScreenRecorder.streaming && screenBlob.size === 0) { + if (!activeScreenRecorder.isStreaming() && screenBlob.size === 0) { return; } const screenFileName = `${RECORDING_FILE_PREFIX}${activeRecordingId}${VIDEO_FILE_EXTENSION}`; const webcamFileName = `${RECORDING_FILE_PREFIX}${activeRecordingId}${WEBCAM_FILE_SUFFIX}${VIDEO_FILE_EXTENSION}`; - // Only fix duration / convert to ArrayBuffer if we have in-memory data. + // Only fix duration / convert to ArrayBuffer for in-memory data; + // streamed recordings are patched on disk by the main process. let screenVideoData: ArrayBuffer = new ArrayBuffer(0); - if (!activeScreenRecorder.streaming && screenBlob.size > 0) { + if (!activeScreenRecorder.isStreaming() && screenBlob.size > 0) { const fixedScreenBlob = await fixWebmDuration(screenBlob, duration); screenVideoData = await fixedScreenBlob.arrayBuffer(); } @@ -442,10 +357,10 @@ export function useScreenRecorder(): UseScreenRecorderReturn { let webcamVideoData: ArrayBuffer | undefined; if (activeWebcamRecorder) { const webcamBlob = await activeWebcamRecorder.recordedBlobPromise.catch(() => null); - if (!activeWebcamRecorder.streaming && webcamBlob && webcamBlob.size > 0) { + if (!activeWebcamRecorder.isStreaming() && webcamBlob && webcamBlob.size > 0) { const fixedWebcamBlob = await fixWebmDuration(webcamBlob, duration); webcamVideoData = await fixedWebcamBlob.arrayBuffer(); - } else if (activeWebcamRecorder.streaming) { + } else if (activeWebcamRecorder.isStreaming()) { webcamVideoData = new ArrayBuffer(0); } } @@ -468,6 +383,8 @@ export function useScreenRecorder(): UseScreenRecorderReturn { console.error("Failed to store recording session:", result.message); return; } + // store-recorded-session has flushed and closed the disk streams. + savedToDisk = true; if (result.session) { await window.electronAPI.setCurrentRecordingSession(result.session); @@ -479,6 +396,13 @@ export function useScreenRecorder(): UseScreenRecorderReturn { } catch (error) { console.error("Error saving recording:", error); } finally { + if (!savedToDisk) { + // Discarded, or failed before a successful save — close any + // dangling disk streams and remove their partial files so a + // cancelled or failed run doesn't leak a descriptor or orphan. + await activeScreenRecorder.discard().catch(() => undefined); + await activeWebcamRecorder?.discard().catch(() => undefined); + } if (finalizingRecordingId.current === activeRecordingId) { finalizingRecordingId.current = null; } @@ -1418,7 +1342,6 @@ export function useScreenRecorder(): UseScreenRecorderReturn { ? { audioBitsPerSecond: systemAudioTrack ? AUDIO_BITRATE_SYSTEM : AUDIO_BITRATE_VOICE } : {}), }, - activeRecordingId, `${RECORDING_FILE_PREFIX}${activeRecordingId}${VIDEO_FILE_EXTENSION}`, ); screenRecorder.current.recorder.addEventListener( @@ -1433,7 +1356,6 @@ export function useScreenRecorder(): UseScreenRecorderReturn { webcamRecorder.current = createRecorderHandle( webcamStream.current, { mimeType, videoBitsPerSecond: Math.min(videoBitsPerSecond, BITRATE_BASE) }, - activeRecordingId + 1, `${RECORDING_FILE_PREFIX}${activeRecordingId}${WEBCAM_FILE_SUFFIX}${VIDEO_FILE_EXTENSION}`, ); } diff --git a/vitest.config.ts b/vitest.config.ts index 9108f69..5a52a9b 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -5,7 +5,7 @@ export default defineConfig({ test: { globals: true, environment: "jsdom", - include: ["src/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}"], + include: ["{src,electron}/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}"], exclude: ["src/**/*.browser.test.{ts,tsx}"], }, resolve: {