From b8293d5acd1b6132490ba6308ee79c30e601f6e7 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Mon, 14 Sep 2026 15:01:27 +0200 Subject: [PATCH 1/3] fix(macos): settle the native stop on the helper's exit, not its first error The macOS stop resolved on `recording-stopped` and rejected on the first `"event":"error"` anywhere in the helper's buffered output, replaying the whole take to find one. Since #655 the helper raises non-terminal errors on purpose (`writer-failed-during-capture`, and `capture-stopped-with-error` before it), stops itself, and only then reports a terminal outcome. So the stop rejected before the helper had been told to stop, and three cases went wrong: - Capture stopped on its own but the file was finalized: the take was thrown away with an error although `recording-stopped` followed. - The helper died without a word (killed, crashed): nothing noticed until the user pressed Stop, which then waited the full 30 s for a `close` that had already happened and put the whole helper log in the toast. - Cancel/Restart after a failed stop left the .mp4 behind, because deleting it depended on the stop having worked. The stop now lives in electron/recording/nativeMacCaptureStop.ts, the twin of nativeWindowsCaptureStop.ts, and settles when the helper exits. The helper always exits after `stop` once it has said its last word, so `close` is the one moment the output is complete and the file is no longer being written. It resolves instead of rejecting: - `recording-stopped` wins; an earlier interruption becomes a warning the HUD shows, and the take opens. - A terminal error reports the interruption's message when there was one ("the video file could not be written"), not the raw NSError behind it. - Exit 0 with no word keeps the old meaning, except after an interruption, where it only means the command pipe closed. - An exit the output drain already recorded settles at once. - On timeout a helper that already reported a finalized file still counts as a success; one that has not is not killed, because this helper has no shutdown ceiling and a long finishWriting has not been measured. The live-take watch from #655 now also fires when the helper process closes mid-take, and ignores the exit a stop in flight causes. `stop` is only written to a helper that can still hear it, and the command pipe has an error listener. A discard removes the take's files whatever the stop returned. Salvaging the fragments of a take whose writer died is left to a follow-up. --- electron/electron-env.d.ts | 2 + electron/ipc/handlers.ts | 164 +++++----- .../ipc/nativeMacMidCaptureErrorWatch.test.ts | 33 ++ electron/ipc/nativeMacMidCaptureErrorWatch.ts | 29 +- .../recording/nativeMacCaptureStop.test.ts | 290 ++++++++++++++++++ electron/recording/nativeMacCaptureStop.ts | 289 +++++++++++++++++ src/hooks/useScreenRecorder.ts | 3 + 7 files changed, 719 insertions(+), 91 deletions(-) create mode 100644 electron/recording/nativeMacCaptureStop.test.ts create mode 100644 electron/recording/nativeMacCaptureStop.ts diff --git a/electron/electron-env.d.ts b/electron/electron-env.d.ts index 6de7a3cc4..0fce60828 100644 --- a/electron/electron-env.d.ts +++ b/electron/electron-env.d.ts @@ -179,6 +179,8 @@ interface Window { session?: import("../src/lib/recordingSession").RecordingSession; message?: string; discarded?: boolean; + /** The take ended before it was stopped, but its recording was kept. */ + warning?: string; error?: string; }>; attachNativeMacWebcamRecording: (payload: { diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index 4e9ba5429..3cd452afb 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -81,6 +81,11 @@ import { findPipeWireCursorHelperPath } from "../native-bridge/cursor/recording/ import type { CursorRecordingSession } from "../native-bridge/cursor/recording/session"; import { toHelperRect } from "../native-bridge/helperCoordinates"; import { scoreDeviceNameMatch } from "../recording/deviceNameMatching"; +import { + type NativeMacCaptureExit, + sendNativeMacStopCommand, + waitForNativeMacCaptureStop, +} from "../recording/nativeMacCaptureStop"; import { isSalvageableFragmentedCapture, NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES, @@ -765,6 +770,13 @@ let nativeMacCursorRecordingStartMs = 0; let nativeMacPauseStartedAtMs: number | null = null; let nativeMacPauseRanges: Array<{ startMs: number; endMs: number }> = []; let nativeMacIsPaused = false; +/** + * How each macOS helper exited, recorded by its output drain on `close` — the + * point at which its output has been read in full. + */ +const nativeMacCaptureExits = new WeakMap(); +/** True while stop-native-mac-recording runs: the take is ending on purpose. */ +let nativeMacStopInFlight = false; // Global frame of the region captured by the SCK helper (see getSelectedSourceBounds). let activeMacCaptureBounds: Rectangle | null = null; let linuxNativeCaptureSession: LinuxNativeCaptureSession | null = null; @@ -1488,14 +1500,14 @@ function inspectNativeMacCaptureOutput() { function attachNativeMacCaptureOutputDrain( proc: ChildProcessWithoutNullStreams, - onErrorDuringCapture: () => void, + onTakeEnded: () => void, ) { let lineBuffer = ""; - // Hooked here rather than on `nativeMacCaptureEvents`, which the stop wait + // Hooked here rather than on `nativeMacCaptureEvents`, which the start wait // replays from the buffer: the drain sees each line once, live. - const watchForMidCaptureError = createNativeMacMidCaptureErrorWatch( - () => nativeMacCaptureProcess === proc, - onErrorDuringCapture, + const watchLiveTake = createNativeMacMidCaptureErrorWatch( + () => nativeMacCaptureProcess === proc && !nativeMacStopInFlight, + onTakeEnded, ); const drain = (chunk: Buffer) => { const text = chunk.toString(); @@ -1507,21 +1519,32 @@ function attachNativeMacCaptureOutputDrain( const event = tryParseNativeHelperEvent(line.trim()); if (event) { dispatchNativeMacHelperEvent(event); - watchForMidCaptureError(event); + watchLiveTake(event); } } }; - const cleanup = () => { + // Registered right after spawn, before the stop wait can listen for `close`, so + // the wait always finds the exit recorded when its own listener runs. + const onClose = (code: number | null, signal: NodeJS.Signals | null) => { + nativeMacCaptureExits.set(proc, { code, signal }); proc.stdout.off("data", drain); proc.stderr.off("data", drain); - proc.off("close", cleanup); - proc.off("error", cleanup); + watchLiveTake.exited(); }; proc.stdout.on("data", drain); proc.stderr.on("data", drain); - proc.once("close", cleanup); - proc.once("error", cleanup); + proc.once("close", onClose); + // A ChildProcess `error` with no listener throws in the main process. + proc.on("error", (error) => { + console.warn("[native-sck] helper process error:", error); + }); + // `sendNativeMacStopCommand` checks the pipe first, but the helper can still die + // between that check and the write. The main-process guard would swallow the + // EPIPE; a listener here keeps it from being raised as uncaught at all. + proc.stdin.on("error", (error) => { + console.warn("[native-sck] helper command pipe error:", error); + }); } function waitForNativeMacCaptureStart(proc: ChildProcessWithoutNullStreams) { @@ -1571,64 +1594,6 @@ function waitForNativeMacCaptureStart(proc: ChildProcessWithoutNullStreams) { }); } -function waitForNativeMacCaptureStop(proc: ChildProcessWithoutNullStreams) { - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - cleanup(); - reject( - new Error( - `Timed out waiting for native macOS capture to stop. Output path: ${ - nativeMacCaptureTargetPath ?? "unknown" - }. Output: ${nativeMacCaptureOutput.trim()}`, - ), - ); - }, 30_000); - - const inspect = (event: Record) => { - if (event.event === "recording-stopped") { - cleanup(); - resolve(String(event.screenPath ?? nativeMacCaptureTargetPath ?? "")); - return; - } - if (event.event === "error") { - cleanup(); - reject(new Error(String(event.message ?? event.code ?? "Native macOS capture failed"))); - } - }; - - const onOutput = (event: Record) => inspect(event); - const onClose = (code: number | null) => { - if (code === 0 && nativeMacCaptureTargetPath) { - cleanup(); - resolve(nativeMacCaptureTargetPath); - return; - } - cleanup(); - reject( - new Error( - nativeMacCaptureOutput.trim() || - `Native macOS capture exited with code=${code ?? "unknown"}`, - ), - ); - }; - const onError = (error: Error) => { - cleanup(); - reject(error); - }; - const cleanup = () => { - clearTimeout(timer); - nativeMacCaptureEvents.off("helper-event", onOutput); - proc.off("close", onClose); - proc.off("error", onError); - }; - - nativeMacCaptureEvents.on("helper-event", onOutput); - proc.once("close", onClose); - proc.once("error", onError); - inspectNativeMacCaptureOutput(); - }); -} - function setCurrentRecordingSessionState(session: RecordingSession | null) { currentRecordingSession = session; currentVideoPath = session?.screenVideoPath ?? null; @@ -2821,6 +2786,7 @@ export function registerIpcHandlers( nativeMacPauseStartedAtMs = null; nativeMacPauseRanges = []; nativeMacIsPaused = false; + nativeMacStopInFlight = false; activeMacCaptureBounds = null; const cursorStartTimeMs = Date.now(); @@ -2836,8 +2802,9 @@ export function registerIpcHandlers( stdio: ["pipe", "pipe", "pipe"], }); nativeMacCaptureProcess = proc; - // Drives the renderer's own stop, the same one the tray's Stop Recording - // sends: it clears the HUD and surfaces the helper's error as the result. + // When the take ends without the user — the helper reported an error or + // exited — this drives the renderer's own stop, the same one the tray's Stop + // Recording sends: it clears the HUD and surfaces the result. attachNativeMacCaptureOutputDrain(proc, () => { const hudWindow = getMainWindow(); if (hudWindow && !hudWindow.isDestroyed()) { @@ -3200,15 +3167,18 @@ export function registerIpcHandlers( return { success: false, error: "Native macOS capture is not running." }; } + nativeMacStopInFlight = true; try { completeNativeMacCursorPauseRange(); - const stoppedPathPromise = waitForNativeMacCaptureStop(proc); - proc.stdin.write("stop\n"); - const stoppedPath = await stoppedPathPromise; - const screenVideoPath = stoppedPath || preferredPath; - if (!screenVideoPath) { - throw new Error("Native macOS capture did not return an output path."); - } + // Listen before sending, so a helper that stops at once cannot slip past. + const stopResultPromise = waitForNativeMacCaptureStop({ + proc, + targetPath: preferredPath, + readOutput: () => nativeMacCaptureOutput, + readExit: () => nativeMacCaptureExits.get(proc) ?? null, + }); + sendNativeMacStopCommand(proc); + const stopResult = await stopResultPromise; if (cursorCaptureMode === "editable-overlay") { await stopCursorRecording(); @@ -3217,12 +3187,40 @@ export function registerIpcHandlers( } if (discard) { pendingCursorRecordingData = null; - await Promise.all([ - fs.rm(screenVideoPath, { force: true }), - fs.rm(`${screenVideoPath}.cursor.json`, { force: true }), - ]); + // Discarding does not wait on the stop having worked: a take thrown away + // after its writer died used to stay behind as an orphan .mp4. + const discardedPath = stopResult.ok ? stopResult.screenVideoPath : preferredPath; + if (discardedPath) { + await Promise.all([ + fs.rm(discardedPath, { force: true }), + fs.rm(`${discardedPath}.cursor.json`, { force: true }), + ]); + } + if (!stopResult.ok) { + console.warn("[native-sck] discarded a take whose stop did not complete", { + reason: stopResult.reason, + message: stopResult.message, + }); + } return { success: true, discarded: true }; } + if (!stopResult.ok) { + pendingCursorRecordingData = null; + console.error("Failed to stop native macOS recording:", { + reason: stopResult.reason, + message: stopResult.message, + helperExited: stopResult.exited, + output: nativeMacCaptureOutput.trim(), + }); + return { success: false, error: stopResult.message }; + } + const screenVideoPath = stopResult.screenVideoPath; + if (stopResult.warning) { + console.warn("[native-sck] the take ended before it was stopped; its recording was kept", { + warning: stopResult.warning, + path: screenVideoPath, + }); + } if (cursorCaptureMode === "editable-overlay") { compactPendingCursorTelemetryPauseRanges(nativeMacPauseRanges); @@ -3250,12 +3248,14 @@ export function registerIpcHandlers( path: screenVideoPath, session, message: "Native macOS recording session stored successfully", + ...(stopResult.warning ? { warning: stopResult.warning } : {}), }; } catch (error) { console.error("Failed to stop native macOS recording:", error); await stopCursorRecording(); return { success: false, error: error instanceof Error ? error.message : String(error) }; } finally { + nativeMacStopInFlight = false; nativeMacCaptureProcess = null; nativeMacCaptureTargetPath = null; nativeMacCaptureRecordingId = null; diff --git a/electron/ipc/nativeMacMidCaptureErrorWatch.test.ts b/electron/ipc/nativeMacMidCaptureErrorWatch.test.ts index c22b9c0e9..c6958cd4b 100644 --- a/electron/ipc/nativeMacMidCaptureErrorWatch.test.ts +++ b/electron/ipc/nativeMacMidCaptureErrorWatch.test.ts @@ -46,4 +46,37 @@ describe("createNativeMacMidCaptureErrorWatch", () => { expect(onError).not.toHaveBeenCalled(); }); + + /** Killed or crashed: no error line ever comes, only the process closing. */ + it("fires when the helper exits in the middle of a take", () => { + const onTakeEnded = vi.fn(); + const watch = createNativeMacMidCaptureErrorWatch(() => true, onTakeEnded); + + watch({ event: "recording-started" }); + watch.exited(); + + expect(onTakeEnded).toHaveBeenCalledTimes(1); + }); + + it("leaves an exit before recording started to the start wait", () => { + const onTakeEnded = vi.fn(); + const watch = createNativeMacMidCaptureErrorWatch(() => true, onTakeEnded); + + watch.exited(); + + expect(onTakeEnded).not.toHaveBeenCalled(); + }); + + /** The exit a stop causes is the stop working, not the take ending under the user. */ + it("ignores the exit of a take that is already being stopped", () => { + const onTakeEnded = vi.fn(); + let live = true; + const watch = createNativeMacMidCaptureErrorWatch(() => live, onTakeEnded); + + watch({ event: "recording-started" }); + live = false; + watch.exited(); + + expect(onTakeEnded).not.toHaveBeenCalled(); + }); }); diff --git a/electron/ipc/nativeMacMidCaptureErrorWatch.ts b/electron/ipc/nativeMacMidCaptureErrorWatch.ts index 4c62f6b7e..6c786d3f3 100644 --- a/electron/ipc/nativeMacMidCaptureErrorWatch.ts +++ b/electron/ipc/nativeMacMidCaptureErrorWatch.ts @@ -1,24 +1,35 @@ /** - * Decides, one live helper event at a time, when the macOS helper raised an - * error mid-take. + * Decides, as the macOS helper speaks and when it exits, whether a take ended + * without the user stopping it, so the HUD can stop with it. * * The start and stop waits only listen while they are pending, so an error the * helper raises between them (a dead writer, a stream that stopped) used to sit * in the buffer until the user pressed stop, while the HUD counted on for - * minutes (issue #621). Before `recording-started` the start wait owns the - * error, and a helper that is no longer the current process has nobody to stop. + * minutes (issue #621). A helper that dies without a word — killed, crashed — is + * the same take ending, and only its process `close` shows it. + * + * Before `recording-started` the start wait owns both. A take that is no longer + * live — another helper replaced it, or a stop is already running — has nothing + * left to stop. */ export function createNativeMacMidCaptureErrorWatch( - isCurrentProcess: () => boolean, - onError: () => void, + isLiveTake: () => boolean, + onTakeEnded: () => void, ) { let recordingStarted = false; - return (event: Record) => { + const watch = (event: Record) => { if (event.event === "recording-started") { recordingStarted = true; } - if (event.event === "error" && recordingStarted && isCurrentProcess()) { - onError(); + if (event.event === "error" && recordingStarted && isLiveTake()) { + onTakeEnded(); + } + }; + /** The helper process closed. */ + const exited = () => { + if (recordingStarted && isLiveTake()) { + onTakeEnded(); } }; + return Object.assign(watch, { exited }); } diff --git a/electron/recording/nativeMacCaptureStop.test.ts b/electron/recording/nativeMacCaptureStop.test.ts new file mode 100644 index 000000000..83150de1e --- /dev/null +++ b/electron/recording/nativeMacCaptureStop.test.ts @@ -0,0 +1,290 @@ +import type { ChildProcessWithoutNullStreams } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { PassThrough, Writable } from "node:stream"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + type NativeMacCaptureExit, + readNativeMacHelperEvents, + readNativeMacStopOutcome, + sendNativeMacStopCommand, + waitForNativeMacCaptureStop, +} from "./nativeMacCaptureStop"; + +const TARGET = "/rec/recording-1.mp4"; + +/** What the helper prints, one JSON event per line. */ +function line(event: Record) { + return `${JSON.stringify(event)}\n`; +} + +const started = line({ event: "recording-started", width: 3840, height: 2160 }); +const stopped = line({ event: "recording-stopped", screenPath: TARGET }); +const writerDied = line({ + event: "error", + code: "writer-failed-during-capture", + message: "Recording stopped: the video file could not be written (video append: -11800).", +}); +const writerFailed = line({ + event: "error", + code: "writer-failed", + message: "Error Domain=AVFoundationErrorDomain Code=-11800", +}); +const captureStopped = line({ + event: "error", + code: "capture-stopped-with-error", + message: "The display was disconnected.", +}); + +/** + * Stands in for openscreen-screencapturekit-helper. `exitCode`/`signalCode` are + * real `ChildProcess` properties the code under test reads, and `exit()` records + * the exit the way the output drain in handlers.ts does before `close` reaches + * any later listener. + */ +class FakeHelper extends EventEmitter { + stdout = new PassThrough(); + stderr = new PassThrough(); + stdin: Writable; + written: string[] = []; + exitCode: number | null = null; + signalCode: NodeJS.Signals | null = null; + recordedExit: NativeMacCaptureExit | null = null; + + constructor() { + super(); + const written = this.written; + this.stdin = new Writable({ + write(chunk, _encoding, callback) { + written.push(chunk.toString()); + callback(); + }, + }); + } + + exit(code: number | null, signal: NodeJS.Signals | null = null) { + this.exitCode = code; + this.signalCode = signal; + this.recordedExit = { code, signal }; + this.emit("close", code, signal); + } +} + +function asProc(helper: FakeHelper) { + return helper as unknown as ChildProcessWithoutNullStreams; +} + +let helper: FakeHelper; + +beforeEach(() => { + vi.useFakeTimers(); + helper = new FakeHelper(); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +function waitForStop(readOutput: () => string, timeoutMs?: number) { + return waitForNativeMacCaptureStop({ + proc: asProc(helper), + targetPath: TARGET, + readOutput, + readExit: () => helper.recordedExit, + timeoutMs, + }); +} + +describe("readNativeMacHelperEvents", () => { + it("reads an event glued behind stderr text", () => { + const events = readNativeMacHelperEvents(`2026-09-14 helper log line${stopped}`); + expect(events).toEqual([{ event: "recording-stopped", screenPath: TARGET }]); + }); + + it("skips plain text and objects that are not events", () => { + expect(readNativeMacHelperEvents('warming up\n{"not":"an event"}\n{broken\n')).toEqual([]); + }); +}); + +describe("readNativeMacStopOutcome", () => { + it("takes the path the helper reported", () => { + expect(readNativeMacStopOutcome(started + stopped, null, "/elsewhere.mp4")).toEqual({ + ok: true, + screenVideoPath: TARGET, + }); + }); + + it("falls back to the requested path when the report names none", () => { + expect( + readNativeMacStopOutcome( + line({ event: "recording-stopped" }), + { code: 0, signal: null }, + TARGET, + ), + ).toEqual({ ok: true, screenVideoPath: TARGET }); + }); + + /** + * The shape that used to lose a finalized take: capture stopped on its own, the + * helper finished the file anyway, and the stop rejected on the earlier error. + */ + it("keeps a take whose capture stopped on its own but whose file was finalized", () => { + expect(readNativeMacStopOutcome(started + captureStopped + stopped, null, TARGET)).toEqual({ + ok: true, + screenVideoPath: TARGET, + warning: + "Recording ended early (The display was disconnected). The part recorded until then was saved.", + }); + }); + + it("says why the take ended rather than quoting the raw writer error", () => { + expect(readNativeMacStopOutcome(started + writerDied + writerFailed, null, TARGET)).toEqual({ + ok: false, + reason: "helper-failed", + message: "Recording stopped: the video file could not be written (video append: -11800).", + exited: false, + }); + }); + + /** The replay that used to reject a stop before the helper had even been told to stop. */ + it("does not settle on an interruption alone while the helper is still running", () => { + expect(readNativeMacStopOutcome(started + writerDied, null, TARGET)).toBeNull(); + }); + + it("keeps the old behaviour for a helper that exits 0 without a word", () => { + expect(readNativeMacStopOutcome(started, { code: 0, signal: null }, TARGET)).toEqual({ + ok: true, + screenVideoPath: TARGET, + }); + }); + + /** Exit 0 after an interruption only means the command pipe closed; nothing was finalized. */ + it("does not call exit 0 after an interruption a finished recording", () => { + expect( + readNativeMacStopOutcome(started + writerDied, { code: 0, signal: null }, TARGET), + ).toEqual({ + ok: false, + reason: "helper-failed", + message: "Recording stopped: the video file could not be written (video append: -11800).", + exited: true, + }); + }); + + it("reports a killed helper by its signal instead of dumping its log", () => { + expect(readNativeMacStopOutcome(started, { code: null, signal: "SIGKILL" }, TARGET)).toEqual({ + ok: false, + reason: "helper-failed", + message: "The recorder stopped unexpectedly (signal SIGKILL).", + exited: true, + }); + }); + + it("refuses a finalized report with no path to point at", () => { + expect( + readNativeMacStopOutcome( + line({ event: "recording-stopped" }), + { code: 0, signal: null }, + null, + ), + ).toMatchObject({ ok: false, reason: "helper-failed" }); + }); +}); + +describe("waitForNativeMacCaptureStop", () => { + it("waits for the helper to exit even when an error is already buffered", async () => { + let output = started + writerDied; + let settled = false; + const pending = waitForStop(() => output).then((result) => { + settled = true; + return result; + }); + + await vi.advanceTimersByTimeAsync(1_000); + expect(settled).toBe(false); + + output += writerFailed; + helper.exit(0); + + await expect(pending).resolves.toMatchObject({ + ok: false, + message: "Recording stopped: the video file could not be written (video append: -11800).", + exited: true, + }); + }); + + it("resolves with the finalized path once the helper exits", async () => { + let output = started; + const pending = waitForStop(() => output); + + output += stopped; + helper.exit(0); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: TARGET }); + }); + + /** Node never re-emits `close`, so a dead helper must not cost the whole timeout. */ + it("settles immediately when the helper has already exited", async () => { + helper.exit(null, "SIGKILL"); + + const result = await waitForStop(() => started); + + expect(result).toEqual({ + ok: false, + reason: "helper-failed", + message: "The recorder stopped unexpectedly (signal SIGKILL).", + exited: true, + }); + expect(vi.getTimerCount()).toBe(0); + }); + + it("keeps a finalized recording when the helper never exits", async () => { + const pending = waitForStop(() => started + stopped, 30_000); + + await vi.advanceTimersByTimeAsync(30_000); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: TARGET }); + }); + + it("reports a timeout, without claiming the helper exited, when it never finishes", async () => { + const pending = waitForStop(() => started, 30_000); + + await vi.advanceTimersByTimeAsync(30_000); + + await expect(pending).resolves.toEqual({ + ok: false, + reason: "stop-timeout", + message: "The recorder did not finish saving in time.", + exited: false, + }); + }); + + it("reports a process error as a helper failure", async () => { + const pending = waitForStop(() => started); + + helper.emit("error", new Error("spawn EACCES")); + + await expect(pending).resolves.toEqual({ + ok: false, + reason: "helper-failed", + message: "spawn EACCES", + exited: false, + }); + }); +}); + +describe("sendNativeMacStopCommand", () => { + it("tells a running helper to stop", () => { + expect(sendNativeMacStopCommand(asProc(helper))).toBe(true); + expect(helper.written).toEqual(["stop\n"]); + }); + + it("does not write to a helper that already exited", () => { + helper.exit(null, "SIGKILL"); + expect(sendNativeMacStopCommand(asProc(helper))).toBe(false); + expect(helper.written).toEqual([]); + }); + + it("does not write to a command pipe that is already closed", () => { + helper.stdin.destroy(); + expect(sendNativeMacStopCommand(asProc(helper))).toBe(false); + }); +}); diff --git a/electron/recording/nativeMacCaptureStop.ts b/electron/recording/nativeMacCaptureStop.ts new file mode 100644 index 000000000..b07e5135d --- /dev/null +++ b/electron/recording/nativeMacCaptureStop.ts @@ -0,0 +1,289 @@ +import type { ChildProcessWithoutNullStreams } from "node:child_process"; + +/** + * Stopping a native macOS (ScreenCaptureKit) recording, as a unit that can be + * tested — the macOS twin of `nativeWindowsCaptureStop.ts`, which exists for + * the same reason: `electron/ipc/handlers.ts` cannot be loaded from a test. + * + * # Why the stop settles on the helper's exit, not on its first error + * + * The helper speaks two kinds of error. `writer-failed-during-capture` and + * `capture-stopped-with-error` say that the take ended on its own; the helper + * then shuts down by itself (issue #621, PR #655) and still reports how that + * went, with `recording-stopped` or `writer-failed`. The stop used to reject on + * the first `"event":"error"` in the whole buffered output, so a take whose + * capture stopped but whose file was finalized was thrown away with an error, + * and a take whose writer died was rejected before the helper had finished + * writing anything down. + * + * The helper always exits once it has received `stop` and finished (its command + * loop runs `await recorder.stop(); exit(0)`), after its last word. So the one + * moment at which the output is complete and the file is no longer being + * written is the process's `close`. That is also where Windows settles. + */ + +/** Outer bound on a stop. Unchanged from the wait this replaced. */ +export const NATIVE_MAC_CAPTURE_STOP_TIMEOUT_MS = 30_000; + +/** How the helper process ended, as Node's `close` reports it. */ +export type NativeMacCaptureExit = { + code: number | null; + signal: NodeJS.Signals | null; +}; + +export type NativeMacCaptureStopResult = + | { + ok: true; + screenVideoPath: string; + /** Why the take ended before the user stopped it, when it did. */ + warning?: string; + } + | { + ok: false; + reason: "helper-failed" | "stop-timeout"; + message: string; + /** False when the helper was still running when the wait gave up. */ + exited: boolean; + }; + +/** + * Errors after which the helper stops itself and still reports a terminal + * outcome. They explain a stop; they never settle one. + */ +const INTERRUPTION_ERROR_CODES = new Set([ + "writer-failed-during-capture", + "capture-stopped-with-error", +]); + +type HelperEvent = Record; + +function parseHelperEvent(line: string): HelperEvent | null { + const candidates = [line]; + // stdout and stderr drain into one buffer chunk by chunk, so a stderr line can + // end up glued onto the front of a JSON event. + const braceIndex = line.indexOf("{"); + if (braceIndex > 0) { + candidates.push(line.slice(braceIndex)); + } + for (const candidate of candidates) { + try { + const parsed = JSON.parse(candidate); + if (parsed && typeof parsed === "object" && typeof parsed.event === "string") { + return parsed as HelperEvent; + } + } catch { + // Not an event; the helper log also carries plain text. + } + } + return null; +} + +export function readNativeMacHelperEvents(output: string): HelperEvent[] { + const events: HelperEvent[] = []; + for (const line of output.split(/\r?\n/)) { + const trimmed = line.trim(); + if (!trimmed) { + continue; + } + const event = parseHelperEvent(trimmed); + if (event) { + events.push(event); + } + } + return events; +} + +/** `Array.prototype.findLast`, which the project's ES library target predates. */ +function lastWhere(events: HelperEvent[], matches: (event: HelperEvent) => boolean) { + for (let index = events.length - 1; index >= 0; index -= 1) { + if (matches(events[index])) { + return events[index]; + } + } + return undefined; +} + +function messageOf(event: HelperEvent) { + if (typeof event.message === "string" && event.message.trim()) { + return event.message.trim(); + } + return typeof event.code === "string" ? event.code : "Native macOS capture failed"; +} + +function describeExit(exit: NativeMacCaptureExit) { + if (exit.signal) { + return `The recorder stopped unexpectedly (signal ${exit.signal}).`; + } + if (exit.code === 0) { + return "The recorder exited without reporting a finished recording."; + } + return `The recorder stopped unexpectedly (exit code ${exit.code ?? "unknown"}).`; +} + +/** + * What the helper's output says about a stop, or null while it has not said + * enough yet. `exit` is null while the helper is still running. + */ +export function readNativeMacStopOutcome( + output: string, + exit: NativeMacCaptureExit | null, + targetPath: string | null, +): NativeMacCaptureStopResult | null { + const events = readNativeMacHelperEvents(output); + const isInterruption = (event: HelperEvent) => + typeof event.code === "string" && INTERRUPTION_ERROR_CODES.has(event.code); + const stopped = lastWhere(events, (event) => event.event === "recording-stopped"); + const errors = events.filter((event) => event.event === "error"); + const interruption = errors.find(isInterruption); + const failure = lastWhere(errors, (event) => !isInterruption(event)); + const exited = exit !== null; + + // A finalized file outranks anything said before it: this is the take the + // user would otherwise lose. + if (stopped) { + const reportedPath = + typeof stopped.screenPath === "string" && stopped.screenPath ? stopped.screenPath : null; + const screenVideoPath = reportedPath ?? targetPath; + if (!screenVideoPath) { + return { + ok: false, + reason: "helper-failed", + message: "Native macOS capture did not return an output path.", + exited, + }; + } + if (!interruption) { + return { ok: true, screenVideoPath }; + } + const reason = messageOf(interruption).replace(/[.\s]+$/, ""); + return { + ok: true, + screenVideoPath, + warning: `Recording ended early (${reason}). The part recorded until then was saved.`, + }; + } + + if (failure) { + // The interruption is what the user can act on ("the video file could not be + // written"); the terminal error that follows it is the raw NSError behind it. + return { + ok: false, + reason: "helper-failed", + message: messageOf(interruption ?? failure), + exited, + }; + } + + if (!exit) { + return null; + } + + // A helper that exits 0 without a word finalized the file it was asked for — + // unless it had already said the take ended, in which case exit 0 only means + // its command pipe closed. + if (exit.code === 0 && !exit.signal && targetPath && !interruption) { + return { ok: true, screenVideoPath: targetPath }; + } + return { + ok: false, + reason: "helper-failed", + message: interruption ? messageOf(interruption) : describeExit(exit), + exited: true, + }; +} + +/** + * Waits for the helper to finish a stop and says how it went. + * + * Resolves rather than rejects, like the Windows wait: the caller needs to tell + * a helper failure apart from a timeout, and a discard has to proceed either + * way. + */ +export function waitForNativeMacCaptureStop(options: { + proc: ChildProcessWithoutNullStreams; + /** Path we asked the helper to write, used when it exits 0 without saying so. */ + targetPath: string | null; + /** The accumulated helper output; read lazily so late chunks are included. */ + readOutput: () => string; + /** + * How the helper exited, once its output drain has seen `close`. Read from the + * drain rather than from `proc.exitCode`, because `exit` can fire before the + * last output chunk has been read. + */ + readExit: () => NativeMacCaptureExit | null; + timeoutMs?: number; +}): Promise { + const { proc, targetPath, readOutput, readExit } = options; + const timeoutMs = options.timeoutMs ?? NATIVE_MAC_CAPTURE_STOP_TIMEOUT_MS; + + const settleFromExit = (exit: NativeMacCaptureExit): NativeMacCaptureStopResult => + readNativeMacStopOutcome(readOutput(), exit, targetPath) ?? { + ok: false, + reason: "helper-failed", + message: describeExit(exit), + exited: true, + }; + + // The helper may already be gone: killed, crashed, or exited after stopping + // itself. Node never re-emits `close`, so waiting for one would burn the whole + // timeout on a recorder that has nothing left to say. + const alreadyExited = readExit(); + if (alreadyExited) { + return Promise.resolve(settleFromExit(alreadyExited)); + } + + return new Promise((resolve) => { + const onClose = (code: number | null, signal: NodeJS.Signals | null) => { + cleanup(); + resolve(settleFromExit(readExit() ?? { code, signal })); + }; + const onError = (error: Error) => { + cleanup(); + resolve({ + ok: false, + reason: "helper-failed", + message: error.message, + exited: proc.exitCode !== null || proc.signalCode !== null, + }); + }; + const cleanup = () => { + clearTimeout(timer); + proc.off("close", onClose); + proc.off("error", onError); + }; + + const timer = setTimeout(() => { + cleanup(); + // A helper that already reported a finalized file but has not exited still + // left a complete recording behind. It is not killed here: one that has not + // reported may be inside a slow finishWriting, and this helper has no + // shutdown ceiling that would make killing it safe. + const outcome = readNativeMacStopOutcome(readOutput(), null, targetPath); + resolve( + outcome ?? { + ok: false, + reason: "stop-timeout", + message: "The recorder did not finish saving in time.", + exited: false, + }, + ); + }, timeoutMs); + + proc.once("close", onClose); + proc.once("error", onError); + }); +} + +/** + * Sends `stop` to a helper that can still hear it. Returns whether it was sent. + * + * A helper that already exited has closed its command pipe, and writing to it + * raises EPIPE or ERR_STREAM_DESTROYED on `proc.stdin`. + */ +export function sendNativeMacStopCommand(proc: ChildProcessWithoutNullStreams): boolean { + if (proc.exitCode !== null || proc.signalCode !== null || !proc.stdin.writable) { + return false; + } + proc.stdin.write("stop\n"); + return true; +} diff --git a/src/hooks/useScreenRecorder.ts b/src/hooks/useScreenRecorder.ts index 8960548b2..68a50d2c1 100644 --- a/src/hooks/useScreenRecorder.ts +++ b/src/hooks/useScreenRecorder.ts @@ -767,6 +767,9 @@ export function useScreenRecorder(): UseScreenRecorderReturn { clearNativeRecordingState(); return true; } + if (result.warning) { + toast.warning(result.warning); + } if (webcamResult.asset && result.path) { const attachResult = await window.electronAPI.attachNativeMacWebcamRecording({ From f667d237f4cbf9924c53a2206d61bc9f71bdb646 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Mon, 14 Sep 2026 15:20:25 +0200 Subject: [PATCH 2/3] fix(macos): keep a timed-out helper out of the next take, and say why a take ended Review of the stop rewrite found three defects. A helper whose stop timed out is deliberately left running, but its drain kept appending to the shared output buffer and dispatching on the shared event bus. If the next take started and then failed or was killed, the old helper's late `recording-stopped` was in that take's buffer and outranked its failure: the stop reported success and opened the previous take's file. The same late output could also reject the next take's start. Each helper's output is now kept on its own, the stop reads only its helper's output, and only the current helper feeds the shared buffer and bus. This predates the rewrite; the rewrite still carried it. The "recording ended early" warning was toasted in the HUD, which the editor switch closes a moment later, so nobody saw it. It now travels with the recording hand-off: `get-current-recording-session` returns it for the file it was kept in, the editor shows it when it imports the recording, and the CLI returns it in `warnings` instead of reporting a cut-short take as a clean run. Also: the discard targets are a tested function instead of inline handler code; the fixtures use the messages the helper really prints; and two comments said things the Swift helper cannot do (a self-stopped helper stays alive until it is sent `stop`, and exit 0 without a word is stdin EOF, which the app never causes). --- electron/electron-env.d.ts | 2 + electron/ipc/handlers.ts | 58 ++++++++++++++----- .../recording/nativeMacCaptureStop.test.ts | 58 ++++++++++++++++--- electron/recording/nativeMacCaptureStop.ts | 27 +++++++-- src/cli/CliRecordRunner.tsx | 3 + src/components/ai-edition/NewEditorShell.tsx | 2 +- .../ai-edition/recordingImport.test.ts | 30 +++++++++- src/components/ai-edition/recordingImport.ts | 13 ++++- src/hooks/useScreenRecorder.ts | 3 - 9 files changed, 161 insertions(+), 35 deletions(-) diff --git a/electron/electron-env.d.ts b/electron/electron-env.d.ts index 0fce60828..838ef6e77 100644 --- a/electron/electron-env.d.ts +++ b/electron/electron-env.d.ts @@ -319,6 +319,8 @@ interface Window { success: boolean; session?: RecordingSession | null; canceled?: boolean; + /** Why this recording ended before it was stopped, when it did. */ + warning?: string; }>; findRecordingCamera: (videoPath: string) => Promise<{ success: boolean; diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index 3cd452afb..d270e8b77 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -83,6 +83,7 @@ import { toHelperRect } from "../native-bridge/helperCoordinates"; import { scoreDeviceNameMatch } from "../recording/deviceNameMatching"; import { type NativeMacCaptureExit, + nativeMacDiscardTargets, sendNativeMacStopCommand, waitForNativeMacCaptureStop, } from "../recording/nativeMacCaptureStop"; @@ -775,6 +776,18 @@ let nativeMacIsPaused = false; * point at which its output has been read in full. */ const nativeMacCaptureExits = new WeakMap(); +/** + * Each macOS helper's own output. The shared `nativeMacCaptureOutput` belongs to + * the current take; a helper whose stop timed out keeps talking after the next + * take has started, and its late `recording-stopped` must not settle that take. + */ +const nativeMacCaptureOutputs = new WeakMap(); +/** + * Why the last macOS take ended before it was stopped, keyed by the file it was + * kept in. Handed to whatever opens that recording next — the editor or the CLI — + * because the HUD that ran the stop closes as the editor opens. + */ +let nativeMacRecordingWarning: { screenVideoPath: string; message: string } | null = null; /** True while stop-native-mac-recording runs: the take is ending on purpose. */ let nativeMacStopInFlight = false; // Global frame of the region captured by the SCK helper (see getSelectedSourceBounds). @@ -1511,14 +1524,22 @@ function attachNativeMacCaptureOutputDrain( ); const drain = (chunk: Buffer) => { const text = chunk.toString(); - nativeMacCaptureOutput += text; + nativeMacCaptureOutputs.set(proc, (nativeMacCaptureOutputs.get(proc) ?? "") + text); + // Only the current take's helper feeds the shared buffer and event bus that the + // start wait, the microphone check and the diagnostics bundle read. + const isCurrent = nativeMacCaptureProcess === proc; + if (isCurrent) { + nativeMacCaptureOutput += text; + } lineBuffer += text; const lines = lineBuffer.split(/\r?\n/); lineBuffer = lines.pop() ?? ""; for (const line of lines) { const event = tryParseNativeHelperEvent(line.trim()); if (event) { - dispatchNativeMacHelperEvent(event); + if (isCurrent) { + dispatchNativeMacHelperEvent(event); + } watchLiveTake(event); } } @@ -2787,6 +2808,7 @@ export function registerIpcHandlers( nativeMacPauseRanges = []; nativeMacIsPaused = false; nativeMacStopInFlight = false; + nativeMacRecordingWarning = null; activeMacCaptureBounds = null; const cursorStartTimeMs = Date.now(); @@ -3174,7 +3196,7 @@ export function registerIpcHandlers( const stopResultPromise = waitForNativeMacCaptureStop({ proc, targetPath: preferredPath, - readOutput: () => nativeMacCaptureOutput, + readOutput: () => nativeMacCaptureOutputs.get(proc) ?? "", readExit: () => nativeMacCaptureExits.get(proc) ?? null, }); sendNativeMacStopCommand(proc); @@ -3187,15 +3209,11 @@ export function registerIpcHandlers( } if (discard) { pendingCursorRecordingData = null; - // Discarding does not wait on the stop having worked: a take thrown away - // after its writer died used to stay behind as an orphan .mp4. - const discardedPath = stopResult.ok ? stopResult.screenVideoPath : preferredPath; - if (discardedPath) { - await Promise.all([ - fs.rm(discardedPath, { force: true }), - fs.rm(`${discardedPath}.cursor.json`, { force: true }), - ]); - } + await Promise.all( + nativeMacDiscardTargets(stopResult, preferredPath).map((target) => + fs.rm(target, { force: true }), + ), + ); if (!stopResult.ok) { console.warn("[native-sck] discarded a take whose stop did not complete", { reason: stopResult.reason, @@ -3210,11 +3228,14 @@ export function registerIpcHandlers( reason: stopResult.reason, message: stopResult.message, helperExited: stopResult.exited, - output: nativeMacCaptureOutput.trim(), + output: (nativeMacCaptureOutputs.get(proc) ?? "").trim(), }); return { success: false, error: stopResult.message }; } const screenVideoPath = stopResult.screenVideoPath; + nativeMacRecordingWarning = stopResult.warning + ? { screenVideoPath, message: stopResult.warning } + : null; if (stopResult.warning) { console.warn("[native-sck] the take ended before it was stopped; its recording was kept", { warning: stopResult.warning, @@ -4254,9 +4275,14 @@ export function registerIpcHandlers( }); ipcMain.handle("get-current-recording-session", () => { - return currentRecordingSession - ? { success: true, session: currentRecordingSession } - : { success: false }; + if (!currentRecordingSession) { + return { success: false }; + } + const warning = + nativeMacRecordingWarning?.screenVideoPath === currentRecordingSession.screenVideoPath + ? nativeMacRecordingWarning.message + : undefined; + return { success: true, session: currentRecordingSession, ...(warning ? { warning } : {}) }; }); // returns the webcam path (if any) for a given screen video by diff --git a/electron/recording/nativeMacCaptureStop.test.ts b/electron/recording/nativeMacCaptureStop.test.ts index 83150de1e..9855b7c2d 100644 --- a/electron/recording/nativeMacCaptureStop.test.ts +++ b/electron/recording/nativeMacCaptureStop.test.ts @@ -4,6 +4,7 @@ import { PassThrough, Writable } from "node:stream"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { type NativeMacCaptureExit, + nativeMacDiscardTargets, readNativeMacHelperEvents, readNativeMacStopOutcome, sendNativeMacStopCommand, @@ -17,12 +18,19 @@ function line(event: Record) { return `${JSON.stringify(event)}\n`; } +// The messages as the Swift helper really prints them: the interruption embeds the +// writer's NSError, and a stream the system stopped reports its NSError alone. +const WRITER_DIED_MESSAGE = + 'Recording stopped: the video file could not be written (video append: Error Domain=AVFoundationErrorDomain Code=-11800 "The operation could not be completed" UserInfo={NSLocalizedFailureReason=An unknown error occurred (-16364)}).'; +const CAPTURE_STOPPED_MESSAGE = + 'Error Domain=com.apple.ScreenCaptureKit.SCStreamErrorDomain Code=-3815 "The stream was stopped by the system." UserInfo={NSLocalizedDescription=The stream was stopped by the system.}'; + const started = line({ event: "recording-started", width: 3840, height: 2160 }); const stopped = line({ event: "recording-stopped", screenPath: TARGET }); const writerDied = line({ event: "error", code: "writer-failed-during-capture", - message: "Recording stopped: the video file could not be written (video append: -11800).", + message: WRITER_DIED_MESSAGE, }); const writerFailed = line({ event: "error", @@ -32,7 +40,7 @@ const writerFailed = line({ const captureStopped = line({ event: "error", code: "capture-stopped-with-error", - message: "The display was disconnected.", + message: CAPTURE_STOPPED_MESSAGE, }); /** @@ -131,8 +139,7 @@ describe("readNativeMacStopOutcome", () => { expect(readNativeMacStopOutcome(started + captureStopped + stopped, null, TARGET)).toEqual({ ok: true, screenVideoPath: TARGET, - warning: - "Recording ended early (The display was disconnected). The part recorded until then was saved.", + warning: `Recording ended early (${CAPTURE_STOPPED_MESSAGE}). The part recorded until then was saved.`, }); }); @@ -140,7 +147,7 @@ describe("readNativeMacStopOutcome", () => { expect(readNativeMacStopOutcome(started + writerDied + writerFailed, null, TARGET)).toEqual({ ok: false, reason: "helper-failed", - message: "Recording stopped: the video file could not be written (video append: -11800).", + message: WRITER_DIED_MESSAGE, exited: false, }); }); @@ -164,7 +171,7 @@ describe("readNativeMacStopOutcome", () => { ).toEqual({ ok: false, reason: "helper-failed", - message: "Recording stopped: the video file could not be written (video append: -11800).", + message: WRITER_DIED_MESSAGE, exited: true, }); }); @@ -206,7 +213,7 @@ describe("waitForNativeMacCaptureStop", () => { await expect(pending).resolves.toMatchObject({ ok: false, - message: "Recording stopped: the video file could not be written (video append: -11800).", + message: WRITER_DIED_MESSAGE, exited: true, }); }); @@ -288,3 +295,40 @@ describe("sendNativeMacStopCommand", () => { expect(sendNativeMacStopCommand(asProc(helper))).toBe(false); }); }); + +describe("nativeMacDiscardTargets", () => { + it("removes the file the helper finalized", () => { + expect(nativeMacDiscardTargets({ ok: true, screenVideoPath: "/rec/a.mp4" }, TARGET)).toEqual([ + "/rec/a.mp4", + "/rec/a.mp4.cursor.json", + ]); + }); + + /** The orphan: a take thrown away after its writer died. */ + it("removes the requested file when the stop failed", () => { + expect( + nativeMacDiscardTargets( + { ok: false, reason: "helper-failed", message: WRITER_DIED_MESSAGE, exited: true }, + TARGET, + ), + ).toEqual([TARGET, `${TARGET}.cursor.json`]); + }); + + it("removes the requested file when the stop timed out", () => { + expect( + nativeMacDiscardTargets( + { ok: false, reason: "stop-timeout", message: "timed out", exited: false }, + TARGET, + ), + ).toEqual([TARGET, `${TARGET}.cursor.json`]); + }); + + it("has nothing to remove when no file was ever requested", () => { + expect( + nativeMacDiscardTargets( + { ok: false, reason: "helper-failed", message: "x", exited: true }, + null, + ), + ).toEqual([]); + }); +}); diff --git a/electron/recording/nativeMacCaptureStop.ts b/electron/recording/nativeMacCaptureStop.ts index b07e5135d..42e665f47 100644 --- a/electron/recording/nativeMacCaptureStop.ts +++ b/electron/recording/nativeMacCaptureStop.ts @@ -178,9 +178,11 @@ export function readNativeMacStopOutcome( return null; } - // A helper that exits 0 without a word finalized the file it was asked for — - // unless it had already said the take ended, in which case exit 0 only means - // its command pipe closed. + // Exit 0 with no terminal word is the command loop reaching stdin EOF without a + // `stop` — nothing in the app closes that pipe, and `stop()` always reports + // before its exit(0). It keeps the meaning the stop wait gave it before this + // module (the requested file), except after an interruption, when nothing + // finalized that file. if (exit.code === 0 && !exit.signal && targetPath && !interruption) { return { ok: true, screenVideoPath: targetPath }; } @@ -224,9 +226,9 @@ export function waitForNativeMacCaptureStop(options: { exited: true, }; - // The helper may already be gone: killed, crashed, or exited after stopping - // itself. Node never re-emits `close`, so waiting for one would burn the whole - // timeout on a recorder that has nothing left to say. + // The helper may already be gone: killed or crashed. (One that stopped itself + // stays alive until it is sent `stop`.) Node never re-emits `close`, so waiting + // for one would burn the whole timeout on a recorder that has nothing left to say. const alreadyExited = readExit(); if (alreadyExited) { return Promise.resolve(settleFromExit(alreadyExited)); @@ -287,3 +289,16 @@ export function sendNativeMacStopCommand(proc: ChildProcessWithoutNullStreams): proc.stdin.write("stop\n"); return true; } + +/** + * The files a discarded take leaves behind, whatever its stop returned: a take + * thrown away after its writer died used to stay on disk as an orphan .mp4, + * because removing it depended on the stop having worked. + */ +export function nativeMacDiscardTargets( + result: NativeMacCaptureStopResult, + targetPath: string | null, +): string[] { + const screenVideoPath = result.ok ? result.screenVideoPath : targetPath; + return screenVideoPath ? [screenVideoPath, `${screenVideoPath}.cursor.json`] : []; +} diff --git a/src/cli/CliRecordRunner.tsx b/src/cli/CliRecordRunner.tsx index e19c451f9..f588caa4c 100644 --- a/src/cli/CliRecordRunner.tsx +++ b/src/cli/CliRecordRunner.tsx @@ -284,6 +284,9 @@ export function CliRecordRunner() { cursorDataPath: `${session.screenVideoPath}.cursor.json`, durationMs, ...(request?.projectOut ? { projectData: buildDefaultProject(session) } : {}), + // The take ended before it was stopped but was kept: a script must be + // able to tell that apart from a clean run. + ...(sessionResult.warning ? { warnings: [sessionResult.warning] } : {}), }); } catch (error) { await fail(error); diff --git a/src/components/ai-edition/NewEditorShell.tsx b/src/components/ai-edition/NewEditorShell.tsx index 681a88031..fabfc3acb 100644 --- a/src/components/ai-edition/NewEditorShell.tsx +++ b/src/components/ai-edition/NewEditorShell.tsx @@ -380,7 +380,7 @@ export function NewEditorShell() { void (async () => { if (!window.electronAPI) return; try { - if (await importPendingRecording()) { + if (await importPendingRecording((warning) => toast.warning(warning))) { toast.success("Recording added to a new project"); return; } diff --git a/src/components/ai-edition/recordingImport.test.ts b/src/components/ai-edition/recordingImport.test.ts index 123c954af..9bc7c5557 100644 --- a/src/components/ai-edition/recordingImport.test.ts +++ b/src/components/ai-edition/recordingImport.test.ts @@ -55,6 +55,7 @@ const realActions = { function stubElectronApi( screenVideoPath: string | null, cursorCaptureMode: "editable-overlay" | "system" = "editable-overlay", + warning?: string, ) { let session: { screenVideoPath: string; @@ -63,7 +64,7 @@ function stubElectronApi( } | null = screenVideoPath ? { screenVideoPath, createdAt: 0, cursorCaptureMode } : null; const api = { getCurrentRecordingSession: vi.fn(async () => - session ? { success: true, session } : { success: false }, + session ? { success: true, session, ...(warning ? { warning } : {}) } : { success: false }, ), setCurrentRecordingSession: vi.fn(async (next: typeof session) => { session = next; @@ -93,6 +94,33 @@ describe("importPendingRecording", () => { expect(createProject).not.toHaveBeenCalled(); }); + // The HUD that stopped a take which ended on its own closes as the editor opens, + // so the editor has to be the one that says the recording was cut short. + it("hands over why the take ended early once the import succeeded", async () => { + stubElectronApi( + "/recordings/recording-1.mp4", + "editable-overlay", + "Recording ended early (stream stopped). The part recorded until then was saved.", + ); + const onWarning = vi.fn(); + + await expect(importPendingRecording(onWarning)).resolves.toBe(true); + + expect(onWarning).toHaveBeenCalledTimes(1); + expect(onWarning).toHaveBeenCalledWith( + "Recording ended early (stream stopped). The part recorded until then was saved.", + ); + }); + + it("says nothing about a take that was stopped normally", async () => { + stubElectronApi("/recordings/recording-1.mp4"); + const onWarning = vi.fn(); + + await importPendingRecording(onWarning); + + expect(onWarning).not.toHaveBeenCalled(); + }); + it("imports the recording into a new project and consumes the hand-off", async () => { const api = stubElectronApi("C:\\recordings\\recording-1.mp4"); diff --git a/src/components/ai-edition/recordingImport.ts b/src/components/ai-edition/recordingImport.ts index 975a4a144..39b0923bd 100644 --- a/src/components/ai-edition/recordingImport.ts +++ b/src/components/ai-edition/recordingImport.ts @@ -297,7 +297,14 @@ export async function maybeSaveFreshRecordingAutoZooms( * reopening the most recent project. Throws if the import itself fails, leaving * the session in place so a later mount can retry it. */ -export async function importPendingRecording(): Promise { +export async function importPendingRecording( + /** + * Called once the import succeeded, with why the take ended before it was + * stopped when it did. The HUD that ran the stop closes as the editor opens, so + * the editor is the only window left to say it. + */ + onWarning?: (message: string) => void, +): Promise { const api = window.electronAPI; if (!api) return false; @@ -305,6 +312,7 @@ export async function importPendingRecording(): Promise { const screenPath = result.success ? result.session?.screenVideoPath : undefined; if (!screenPath) return false; const cursorCaptureMode = result.success ? result.session?.cursorCaptureMode : undefined; + const warning = result.success ? result.warning : undefined; const label = screenPath.split(/[\\/]/).pop() || "Recording"; await useProjectStore.getState().createProject(`Recording ${new Date().toLocaleString()}`); @@ -348,5 +356,8 @@ export async function importPendingRecording(): Promise { if (latest) { await maybeSaveFreshRecordingAutoZooms(latest); } + if (warning) { + onWarning?.(warning); + } return true; } diff --git a/src/hooks/useScreenRecorder.ts b/src/hooks/useScreenRecorder.ts index 68a50d2c1..8960548b2 100644 --- a/src/hooks/useScreenRecorder.ts +++ b/src/hooks/useScreenRecorder.ts @@ -767,9 +767,6 @@ export function useScreenRecorder(): UseScreenRecorderReturn { clearNativeRecordingState(); return true; } - if (result.warning) { - toast.warning(result.warning); - } if (webcamResult.asset && result.path) { const attachResult = await window.electronAPI.attachNativeMacWebcamRecording({ From 949141c1823e706343cdc0c493e46157815be7b9 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Mon, 14 Sep 2026 16:21:11 +0200 Subject: [PATCH 3/3] fix(macos): listen for errors on the helper's output pipes too The macOS drain listened for errors on the process and on stdin, but not on stdout or stderr. The main-process guard swallows only EPIPE, ECONNRESET and ERR_STREAM_DESTROYED, so any other output-stream error would reach uncaughtException and take the main process down. The Windows drain already guards all three pipes; this mirrors it (CodeRabbit review on #661). --- electron/ipc/handlers.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index d270e8b77..5837502da 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -1566,6 +1566,14 @@ function attachNativeMacCaptureOutputDrain( proc.stdin.on("error", (error) => { console.warn("[native-sck] helper command pipe error:", error); }); + // The output pipes too, as the Windows drain does: the guard only swallows a few + // codes, and any other stream error would take the main process down. + proc.stdout.on("error", (error) => { + console.warn("[native-sck] helper stdout error:", error); + }); + proc.stderr.on("error", (error) => { + console.warn("[native-sck] helper stderr error:", error); + }); } function waitForNativeMacCaptureStart(proc: ChildProcessWithoutNullStreams) {