Skip to content

feat(cli): --follow-windows — automatic window switching for multi-window demos - #190

Closed
PeterTakahashi wants to merge 6 commits into
getopenscreen:mainfrom
PeterTakahashi:feat/follow-windows
Closed

feat(cli): --follow-windows — automatic window switching for multi-window demos#190
PeterTakahashi wants to merge 6 commits into
getopenscreen:mainfrom
PeterTakahashi:feat/follow-windows

Conversation

@PeterTakahashi

@PeterTakahashi PeterTakahashi commented Jul 28, 2026

Copy link
Copy Markdown

Note

Stacked on #176 (headless CLI). This branch contains #176's commits until it merges; the new work here is the last commit (31d7c418). Happy to rebase once #176 lands.

Problem

A recording can capture one window or the whole screen — but real product demos move between windows: type in the terminal, switch to the editor, open the browser, check the app. Recording a single window can't show that; recording the entire screen loses the polish that makes OpenScreen worth using.

Approach: record the display, move the camera

Instead of switching the capture target mid-recording (encoder dimension changes, per-platform native work), this records the whole display plus a window-focus timeline, then turns that timeline into zoom regions at export. The existing zoom spring animates the camera between windows — a cinematic pan/zoom rather than a hard cut, exactly like manually-authored zooms:

openscreen record --display 0 --follow-windows --project demo.openscreen
# ...work normally: terminal → editor → browser...
openscreen export demo.openscreen -o demo.mp4 --follow-windows

Implementation

  • openscreen-macos-focus-helper (new Swift executable in the existing SwiftPM package): samples the frontmost app's focused window via NSWorkspace + CGWindowListCopyWindowInfo — window names/bounds come from the Screen Recording permission the capture pipeline already holds, so no new permission prompts. NDJSON on change + 1 s heartbeat, same protocol family as the cursor helper.
  • record --follow-windows: electron/cli/focusSampler.ts spawns the helper when capture starts and writes a <video>.focus.json sidecar (samples + display geometry), sibling to the existing .cursor.json.
  • export --follow-windows: src/lib/windowFocus/focusToZoomRegions.ts — a pure, unit-tested converter: ≥1.5 s dwell filter (ignores passing-through focus), brief-detour merging, 6% framing margin, 5× scale cap, and near-fullscreen windows produce no region so the camera pulls back to full frame. Generated regions never overlap manual zooms, and it composes with --auto-zoom (cursor-dwell zooms fill the gaps).

The sidecar format and converter are platform-neutral; only the sampler is macOS-specific for now, mirroring the capture pipeline's mac-first history. The converter is deliberately reusable for a future GUI toggle ("Follow windows" in the editor).

Tested (macOS 26.6, Apple Silicon)

  • Helper standalone: frontmost tracking across app switches, window resize animations captured, concurrent instances OK
  • record --follow-windows: sidecar written with real focus timelines; diagnostics emitted when sampling yields nothing
  • End-to-end: 20 s display recording → export produced framed zooms on two windows and correctly pulled back for a near-fullscreen window between them
  • 7 new unit tests for the converter (dwell/merge/fullscreen-skip/scale-cap/overlap/display filtering); full suite 450 passing; lint + tsc clean

🤖 Generated with Claude Code

https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J

Summary by CodeRabbit

  • New Features
    • Added a headless command-line interface for recording, exporting, captioning, source discovery, project packaging, and project information.
    • Added machine-readable NDJSON progress/completion events and a new cli npm script to launch the app.
    • Added voiceover mixing/replacement for MP4 exports and focus-based automatic zooming (including macOS follow-windows).
    • Added macOS multi-window recording with offline composition.
  • Documentation
    • Added comprehensive CLI docs with examples and NDJSON contracts.
  • Bug Fixes
    • Improved portable project support by resolving media paths via project-sibling basenames.
  • Tests
    • Added CLI argument parsing and focus/zoom generation test coverage.

PeterTakahashi and others added 5 commits July 28, 2026 03:04
Adds a CLI so scripts, CI pipelines, and AI coding agents can drive
OpenScreen end-to-end without a visible window:

  openscreen record --duration 20 --project demo.openscreen --json
  openscreen export demo.openscreen -o demo.mp4 --audio voice.m4a --json
  openscreen info demo.openscreen --json

Design: CLI mode boots Electron without HUD/tray/menu and drives a hidden
runner window (windowType=cli-export|cli-record) that reuses the existing
pipelines unchanged — VideoExporter/GifExporter for export (the approach
already proven by the HEADLESS=true e2e test) and useScreenRecorder for
recording (native SCK/WGC helpers, browser fallback). registerIpcHandlers
is reused with inert window callbacks, so the 2900-line handler module is
untouched.

- electron/cli/args.ts: pure argv parser + unit tests; skips leading
  Chromium switches (AppImage --no-sandbox) before subcommand detection
- electron/cli/cliMain.ts: stdio protocol (NDJSON with --json, TTY-aware
  progress otherwise), SIGINT/SIGTERM/stdin stop, EPIPE-safe writes,
  exit codes 0/1/2, console rerouted to stderr, SwiftShader fallback
- src/cli/CliExportRunner.tsx: loads .openscreen via the native bridge,
  rebuilds the editor's exporter config deterministically (1280x720
  reference preview box, overridable via --preview-size), optional
  voiceover mix (--audio/--audio-mode/--audio-offset) that copies video
  packets and re-renders audio offline via OfflineAudioContext+mediabunny
- src/cli/CliRecordRunner.tsx: source pick by display index or window
  title, mic device by label, --duration/signal/stdin stop, optional
  ready-to-export --project output
- useScreenRecorder: expose startRecordingImmediately() (skips countdown)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J
Actionable findings:
- voiceoverMix: duck the original bed to 0.4 gain by default in "mix" mode
  so the unity-gain sum cannot hard-clip; skip buffering the whole MP4 in
  "replace" mode; cancel the mediabunny Output on remux failure
- CliRecordRunner: a stop signal arriving before capture starts is now
  remembered and applied the moment recording begins; add a 30s watchdog
  so a silently failed start no longer hangs the CLI; move ref sync out
  of render into an effect
- cliMain: guard window-all-closed with the finished flag so teardown
  after a successful run can't report a false failure; route info-command
  output through safeWrite; harden console rerouting against circular args
- args: reject zero dimensions in --preview-size
- CliExportRunner: 30s timeout on the video metadata probe
- preload: type the CLI bridge methods against cliContracts
- args.test: platform-agnostic path assertions (win32-safe)
- docs: fence language, document mix-mode ducking

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J
Four additions that close the gap between CLI and GUI workflows:

- export --auto-zoom: runs the editor's cursor-dwell suggestion engine
  (timeline/zoomSuggestionUtils) over the recording's telemetry and adds
  focus-following zoom regions before rendering; never overlaps regions
  already in the project
- sources: lists capturable displays, windows and microphones (same
  enumeration as the GUI picker) so scripts can pick --display/--window/
  --mic-device values without trial and error
- pack <project> --out <dir>: copies the project plus referenced media
  and the cursor-telemetry sidecar into one portable folder; the media
  approval path (getApprovedProjectSession) and the export runner gain a
  same-basename sibling fallback so a packed folder keeps working after
  being moved to another location or machine
- captions <project>: transcribes the project's audio with the bundled
  on-device Whisper worker (mirrors VideoEditor.generateAutoCaptions:
  leading-silence trim, retry on empty, word grouping) and writes
  auto-caption annotations into the project; re-running replaces earlier
  auto-captions and preserves manual annotations

Also: console rerouting now serializes Error objects properly instead of
printing "{}".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J
- docs: real JSON payload example for `sources --json`; align the demo
  narration wording with --audio-mode replace
- args.test: edge-case coverage for sources/pack/captions parsers
  (missing paths, unknown options, extra positionals, non-integer and
  zero word counts)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J
…ndow demos

Real product demos move between windows (terminal → editor → browser), but a
recording can only capture one window or the whole screen. This records the
whole display plus a window-focus timeline, and turns that timeline into zoom
regions at export time — the renderer's zoom spring then pans/zooms the camera
to whichever window had focus, cinematically instead of hard-cutting.

- New Swift helper openscreen-macos-focus-helper: samples the frontmost
  app's focused window (NSWorkspace + CGWindowListCopyWindowInfo, reuses the
  Screen Recording permission) at 200 ms, emitting NDJSON on change + 1 s
  heartbeat. Wired into Package.swift and the build script.
- record --follow-windows (macOS): electron/cli/focusSampler.ts spawns the
  helper when capture starts (new cli-recording-started IPC) and writes a
  <video>.focus.json sidecar next to the recording, with display geometry
  for normalization and built-in diagnostics for silent-failure cases.
- export --follow-windows: pure converter src/lib/windowFocus/
  focusToZoomRegions.ts (unit-tested) builds zoom regions from the timeline —
  ≥1.5 s dwell filter, brief-detour merging, 6% framing margin, 5× scale cap,
  near-fullscreen windows pull the camera back to full frame, and generated
  regions never overlap manual zooms. Composes with --auto-zoom.

Windows/Linux: the sidecar format and converter are platform-neutral; only
the focus sampler is macOS-specific for now (mirrors the native capture
pipeline's mac-first history).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a headless Electron CLI for recording, source discovery, project packing, captioning, inspection, and export, with typed IPC contracts, NDJSON output, voiceover mixing, portable media resolution, multi-window capture, and macOS focus telemetry for automatic zoom regions.

Changes

Headless CLI

Layer / File(s) Summary
CLI contracts and argument parsing
src/lib/cliContracts.ts, electron/cli/args.ts, electron/cli/args.test.ts
Defines CLI request/result contracts, parses command options and validations, and adds parser coverage.
Electron CLI orchestration
electron/main.ts, electron/cli/cliMain.ts, electron/preload.ts, electron/electron-env.d.ts, src/App.tsx, electron/ipc/handlers.ts, package.json, electron/windows.ts
Boots CLI mode, manages hidden runner windows and IPC, emits text or NDJSON output, supports project packing and inspection, and resolves packed media beside project files.
Renderer recording, export, and caption runners
src/cli/*, src/hooks/useScreenRecorder.ts, src/lib/exporter/voiceoverMix.ts
Adds hidden-window runners for recording, source enumeration, exporting, and captioning, plus immediate recording startup and MP4 voiceover mixing.
Focus telemetry and multi-window export
src/lib/windowFocus/*, src/lib/windowSwitch/*, electron/cli/focusSampler.ts, electron/cli/multiWindowRecorder.ts, src/cli/multiWindowCompositor.ts, electron/native/screencapturekit/*, scripts/build-macos-screencapturekit-helper.mjs
Adds macOS focus sampling, multi-window recording, telemetry sidecars, switch timelines, video composition, automatic zoom-region generation, native helper packaging, and tests.
CLI documentation and repository support
README.md, docs/cli.md, .gitignore
Documents CLI commands, NDJSON output, platform behavior, and automated workflows, and ignores public/caption-assets.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant Electron
  participant RendererRunner
  participant FocusHelper
  participant NativeCapture
  participant ExportPipeline
  CLI->>Electron: invoke record or export command
  Electron->>RendererRunner: load hidden command runner
  RendererRunner->>NativeCapture: start recording or enumerate sources
  Electron->>FocusHelper: start optional focus sampling
  FocusHelper-->>Electron: emit focus samples
  NativeCapture-->>RendererRunner: return recording artifacts
  RendererRunner->>Electron: stream progress and completion
  Electron->>ExportPipeline: pass project and telemetry data
  ExportPipeline-->>CLI: write MP4/GIF and emit completion
Loading

Possibly related PRs

Suggested reviewers: etiennelescot

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description has useful detail, but it doesn't follow the required template and omits several required sections. Rewrite it using the repository template and add the missing sections: Summary, Related issue, Type of change, Release impact, Desktop impact, Screenshots / video, and Testing.
Docstring Coverage ⚠️ Warning Docstring coverage is 21.74% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the follow-windows multi-window demo automation and matches the main change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Nitpick comments (2)
src/lib/exporter/voiceoverMix.ts (2)

86-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

New pure helpers in this stack ship without unit tests. Only focusToZoomRegions gained coverage; several other newly added, dependency-free functions are directly testable with Vitest/jsdom and currently unverified.

  • src/lib/exporter/voiceoverMix.ts#L86-L147: add voiceoverMix.test.ts covering mix vs. replace mode, offset clamping, and the no-video-track / no-output error paths.
  • src/cli/CliExportRunner.tsx#L85-L123: cover fitPreviewBox (landscape/portrait fit) and replaceExtension (.openscreen, .json, and extension-less inputs); extracting them into a sibling module would make them importable without the component.
  • src/cli/CliCaptionsRunner.tsx#L23-L31: cover nextNumericIdFrom for empty input, non-numeric ids, and multi-digit trailing numbers.

As per coding guidelines: "Add a test for every new behavior in the same package as the code under test" and "Place unit tests next to their source files using *.test.ts or *.test.tsx; use Vitest with the jsdom environment."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/exporter/voiceoverMix.ts` around lines 86 - 147, Add Vitest/jsdom
unit tests next to each affected source: in
src/lib/exporter/voiceoverMix.ts:86-147, add voiceoverMix.test.ts covering mix
versus replace mode, offset clamping, missing video-track errors, and no-output
errors; in src/cli/CliExportRunner.tsx:85-123, test fitPreviewBox for landscape
and portrait fitting and replaceExtension for .openscreen, .json, and
extensionless inputs, extracting helpers into a sibling module if needed for
importability; in src/cli/CliCaptionsRunner.tsx:23-31, test nextNumericIdFrom
with empty input, non-numeric IDs, and multi-digit trailing numbers.

Source: Coding guidelines


63-77: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift

Stream the original audio track from mediabunny instead of decoding the whole MP4.

In "mix" mode this loads the full MP4 with videoBlob.arrayBuffer(), copies it again in decodeToBuffer(data.slice(0)), and materialises the entire original bed in an AudioBuffer. Use the primary InputAudioTrack with AudioSampleSink/AudioBufferSink (or encodedAudioPacketSource) and convert/sync just the decoded PCM samples needed for the Web Audio mix.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/exporter/voiceoverMix.ts` around lines 63 - 77, Replace the full-file
decode in the mix branch around decodeToBuffer with mediabunny’s primary
InputAudioTrack and AudioSampleSink or AudioBufferSink, streaming only the
decoded PCM samples needed for the Web Audio mix. Preserve originalGain,
destination routing, playback start, and the existing fallback when no decodable
audio track exists; avoid videoBlob.arrayBuffer(), data.slice(), and full-MP4
AudioBuffer materialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@electron/cli/args.test.ts`:
- Around line 140-152: Extend the CLI parser tests around the existing
export/record option cases to cover --follow-windows producing the expected
boolean value, and add a rejection case for combining --follow-windows with
--window. Use the existing parse helper and error assertion style so both new
behaviors in args.ts are exercised.

In `@electron/cli/args.ts`:
- Around line 270-284: Move the audio/GIF validation from the initial checks to
after the `request.outPath` extension inference and `request.format` assignment,
so both explicit `--format gif` and `.gif` output reject `--audio`. Preserve the
existing format-conflict and output-extension validation in the export argument
handling flow.

In `@electron/cli/cliMain.ts`:
- Around line 264-286: Update the surrounding flow before accessing media so its
presence is explicitly validated or narrowed first. Then keep using
media.webcamVideoPath and the ...media spread in the PackedProjectData
construction without additional null checks, preserving the existing webcam and
cursor-sidecar behavior.
- Around line 269-277: Update the pack flow near the cursorSidecar handling to
also detect and copy the window-focus sidecar written by the follow-windows
telemetry path, using the corresponding <screen source>.focus.json filename.
Preserve the existing missing-file behavior and ensure both cursor and focus
sidecars are included when present.
- Around line 545-562: Ensure the focus sampler is stopped during all recording
teardown paths, not only the successful branch guarded by result.success and
result.screenVideoPath. Update the surrounding cleanup flow and the
did-fail-load, render-process-gone, and window-all-closed handlers to invoke
focusSampler.stop() when present, while retaining sidecar writing only when
stopped data contains usable samples.

In `@electron/cli/focusSampler.ts`:
- Around line 122-129: Update the child-process error handling around the
sampler’s `this.child.on("error")` and `exit` listeners to notify the caller
when spawning fails or the helper exits unexpectedly, rather than only updating
`lastError` or `exitCode`. Add or reuse the sampler’s public callback/event
mechanism, invoke it for both failure paths, and preserve the existing
diagnostics state updates.

In
`@electron/native/screencapturekit/Sources/OpenScreenMacOSFocusHelper/main.swift`:
- Around line 89-96: The focus sample currently persists sensitive window titles
that focusTelemetryToZoomRegions does not consume. Remove windowTitle from the
emitted FocusSample payload and eliminate the related kCGWindowName/title
extraction in the focus sampling flow, while preserving appName, bounds, pid,
and windowNumber.

In `@src/cli/CliExportRunner.tsx`:
- Around line 206-208: Update the video probing flow around probeVideoDimensions
so a durationMs of 0 is not silently passed to the --follow-windows and
--auto-zoom region generation paths. Use the available telemetry span duration
as a fallback when possible; otherwise emit an actionable warning or error
before reporting zero regions.
- Around line 138-151: The session paths used in the export flow must be
verified as belonging to the project being exported before replacing values in
media. Update the logic around getCurrentRecordingSession and runPackCommand to
add a project/session binding check, or clear currentRecordingSession when
packing an arbitrary output folder, and only apply session paths when that
association is confirmed.

In `@src/cli/CliRecordRunner.tsx`:
- Around line 264-288: Update the completion flow around recordingStartedAtRef
and the phase transition to use the capture stop timestamp rather than
Date.now() after saving; set recordingStoppedAtRef.current when entering the
stopping phase, including duration-timer and external-stop paths, then calculate
durationMs from that timestamp. If the session manifest exposes a reliable
duration, prefer it.

In `@src/lib/exporter/voiceoverMix.ts`:
- Around line 54-61: The voiceover mixing flow around frameCount and
voiceoverNode.start currently truncates audio beyond the video duration without
reporting it. Update the relevant export function to calculate the portion
exceeding the OfflineAudioContext duration, return the clipped amount alongside
the existing result, and expose it so CliExportRunner can add a warning to
CliDoneResult.warnings; preserve current mixing behavior for voiceovers that
fit.

---

Nitpick comments:
In `@src/lib/exporter/voiceoverMix.ts`:
- Around line 86-147: Add Vitest/jsdom unit tests next to each affected source:
in src/lib/exporter/voiceoverMix.ts:86-147, add voiceoverMix.test.ts covering
mix versus replace mode, offset clamping, missing video-track errors, and
no-output errors; in src/cli/CliExportRunner.tsx:85-123, test fitPreviewBox for
landscape and portrait fitting and replaceExtension for .openscreen, .json, and
extensionless inputs, extracting helpers into a sibling module if needed for
importability; in src/cli/CliCaptionsRunner.tsx:23-31, test nextNumericIdFrom
with empty input, non-numeric IDs, and multi-digit trailing numbers.
- Around line 63-77: Replace the full-file decode in the mix branch around
decodeToBuffer with mediabunny’s primary InputAudioTrack and AudioSampleSink or
AudioBufferSink, streaming only the decoded PCM samples needed for the Web Audio
mix. Preserve originalGain, destination routing, playback start, and the
existing fallback when no decodable audio track exists; avoid
videoBlob.arrayBuffer(), data.slice(), and full-MP4 AudioBuffer materialization.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3560a1ba-7a60-4caf-a6f1-a631192f4edb

📥 Commits

Reviewing files that changed from the base of the PR and between a7a5272 and 31d7c41.

📒 Files selected for processing (27)
  • .gitignore
  • README.md
  • docs/cli.md
  • electron/cli/args.test.ts
  • electron/cli/args.ts
  • electron/cli/cliMain.ts
  • electron/cli/focusSampler.ts
  • electron/electron-env.d.ts
  • electron/ipc/handlers.ts
  • electron/main.ts
  • electron/native/screencapturekit/Package.swift
  • electron/native/screencapturekit/Sources/OpenScreenMacOSFocusHelper/main.swift
  • electron/preload.ts
  • electron/windows.ts
  • package.json
  • scripts/build-macos-screencapturekit-helper.mjs
  • src/App.tsx
  • src/cli/CliCaptionsRunner.tsx
  • src/cli/CliExportRunner.tsx
  • src/cli/CliRecordRunner.tsx
  • src/cli/CliSourcesRunner.tsx
  • src/hooks/useScreenRecorder.ts
  • src/lib/cliContracts.ts
  • src/lib/exporter/voiceoverMix.ts
  • src/lib/windowFocus/contracts.ts
  • src/lib/windowFocus/focusToZoomRegions.test.ts
  • src/lib/windowFocus/focusToZoomRegions.ts

Comment thread electron/cli/args.test.ts
Comment on lines +140 to +152
it("rejects invalid record values", () => {
expect(parse(["record", "--duration", "0"])).toMatchObject({ kind: "error" });
expect(parse(["record", "--cursor", "off"])).toMatchObject({ kind: "error" });
expect(parse(["record", "--project", "demo.json"])).toMatchObject({ kind: "error" });
});

it("parses --auto-zoom", () => {
expect(parse(["export", "a.openscreen", "--auto-zoom"])).toMatchObject({
kind: "export",
autoZoom: true,
});
expect(parse(["export", "a.openscreen"])).toMatchObject({ autoZoom: false });
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add coverage for --follow-windows.

This PR's headline flag has no parser test: neither the export/record boolean nor the new --follow-windows + --window rejection in args.ts (Lines 372-376) is exercised.

As per coding guidelines: "Add a test for every new behavior in the same package as the code under test."

💚 Proposed tests
 	it("rejects invalid record values", () => {
 		expect(parse(["record", "--duration", "0"])).toMatchObject({ kind: "error" });
 		expect(parse(["record", "--cursor", "off"])).toMatchObject({ kind: "error" });
 		expect(parse(["record", "--project", "demo.json"])).toMatchObject({ kind: "error" });
 	});
 
+	it("parses --follow-windows and rejects it with --window", () => {
+		expect(parse(["record", "--follow-windows"])).toMatchObject({
+			kind: "record",
+			followWindows: true,
+		});
+		expect(parse(["record", "--follow-windows", "--window", "Safari"])).toMatchObject({
+			kind: "error",
+		});
+		expect(parse(["export", "a.openscreen", "--follow-windows"])).toMatchObject({
+			kind: "export",
+			followWindows: true,
+		});
+		expect(parse(["export", "a.openscreen"])).toMatchObject({ followWindows: false });
+	});
+
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("rejects invalid record values", () => {
expect(parse(["record", "--duration", "0"])).toMatchObject({ kind: "error" });
expect(parse(["record", "--cursor", "off"])).toMatchObject({ kind: "error" });
expect(parse(["record", "--project", "demo.json"])).toMatchObject({ kind: "error" });
});
it("parses --auto-zoom", () => {
expect(parse(["export", "a.openscreen", "--auto-zoom"])).toMatchObject({
kind: "export",
autoZoom: true,
});
expect(parse(["export", "a.openscreen"])).toMatchObject({ autoZoom: false });
});
it("rejects invalid record values", () => {
expect(parse(["record", "--duration", "0"])).toMatchObject({ kind: "error" });
expect(parse(["record", "--cursor", "off"])).toMatchObject({ kind: "error" });
expect(parse(["record", "--project", "demo.json"])).toMatchObject({ kind: "error" });
});
it("parses --follow-windows and rejects it with --window", () => {
expect(parse(["record", "--follow-windows"])).toMatchObject({
kind: "record",
followWindows: true,
});
expect(parse(["record", "--follow-windows", "--window", "Safari"])).toMatchObject({
kind: "error",
});
expect(parse(["export", "a.openscreen", "--follow-windows"])).toMatchObject({
kind: "export",
followWindows: true,
});
expect(parse(["export", "a.openscreen"])).toMatchObject({ followWindows: false });
});
it("parses --auto-zoom", () => {
expect(parse(["export", "a.openscreen", "--auto-zoom"])).toMatchObject({
kind: "export",
autoZoom: true,
});
expect(parse(["export", "a.openscreen"])).toMatchObject({ autoZoom: false });
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/cli/args.test.ts` around lines 140 - 152, Extend the CLI parser
tests around the existing export/record option cases to cover --follow-windows
producing the expected boolean value, and add a rejection case for combining
--follow-windows with --window. Use the existing parse helper and error
assertion style so both new behaviors in args.ts are exercised.

Source: Coding guidelines

Comment thread electron/cli/args.ts
Comment on lines +270 to +284
if (!request.projectPath) throw new Error("export requires a <project.openscreen> path");
if (request.audioPath && request.format === "gif") {
throw new Error("--audio is only supported for MP4 exports");
}
if (request.outPath) {
const ext = path.extname(request.outPath).toLowerCase();
if (ext !== ".mp4" && ext !== ".gif") {
throw new Error(`--out must end in .mp4 or .gif, got "${request.outPath}"`);
}
const extFormat = ext === ".gif" ? "gif" : "mp4";
if (request.format && request.format !== extFormat) {
throw new Error(`--format ${request.format} conflicts with --out extension ${ext}`);
}
request.format = extFormat;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

--audio with a .gif --out bypasses the MP4-only guard.

The audio/GIF check on Line 271 runs before --out extension inference sets request.format on Line 283. So export p.openscreen --audio v.mp3 -o out.gif is accepted while --format gif is rejected — inconsistent, and the voiceover is silently dropped downstream.

Move the guard after format resolution.

🐛 Proposed fix
 	if (!request.projectPath) throw new Error("export requires a <project.openscreen> path");
-	if (request.audioPath && request.format === "gif") {
-		throw new Error("--audio is only supported for MP4 exports");
-	}
 	if (request.outPath) {
 		const ext = path.extname(request.outPath).toLowerCase();
 		if (ext !== ".mp4" && ext !== ".gif") {
 			throw new Error(`--out must end in .mp4 or .gif, got "${request.outPath}"`);
 		}
 		const extFormat = ext === ".gif" ? "gif" : "mp4";
 		if (request.format && request.format !== extFormat) {
 			throw new Error(`--format ${request.format} conflicts with --out extension ${ext}`);
 		}
 		request.format = extFormat;
 	}
+	if (request.audioPath && request.format === "gif") {
+		throw new Error("--audio is only supported for MP4 exports");
+	}
 	return request;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!request.projectPath) throw new Error("export requires a <project.openscreen> path");
if (request.audioPath && request.format === "gif") {
throw new Error("--audio is only supported for MP4 exports");
}
if (request.outPath) {
const ext = path.extname(request.outPath).toLowerCase();
if (ext !== ".mp4" && ext !== ".gif") {
throw new Error(`--out must end in .mp4 or .gif, got "${request.outPath}"`);
}
const extFormat = ext === ".gif" ? "gif" : "mp4";
if (request.format && request.format !== extFormat) {
throw new Error(`--format ${request.format} conflicts with --out extension ${ext}`);
}
request.format = extFormat;
}
if (!request.projectPath) throw new Error("export requires a <project.openscreen> path");
if (request.outPath) {
const ext = path.extname(request.outPath).toLowerCase();
if (ext !== ".mp4" && ext !== ".gif") {
throw new Error(`--out must end in .mp4 or .gif, got "${request.outPath}"`);
}
const extFormat = ext === ".gif" ? "gif" : "mp4";
if (request.format && request.format !== extFormat) {
throw new Error(`--format ${request.format} conflicts with --out extension ${ext}`);
}
request.format = extFormat;
}
if (request.audioPath && request.format === "gif") {
throw new Error("--audio is only supported for MP4 exports");
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/cli/args.ts` around lines 270 - 284, Move the audio/GIF validation
from the initial checks to after the `request.outPath` extension inference and
`request.format` assignment, so both explicit `--format gif` and `.gif` output
reject `--audio`. Preserve the existing format-conflict and output-extension
validation in the export argument handling flow.

Comment thread electron/cli/cliMain.ts
Comment on lines +264 to +286
let newWebcamPath: string | undefined;
if (media.webcamVideoPath) {
newWebcamPath = await copyIn(await resolveSource(media.webcamVideoPath));
}

// Cursor telemetry sidecar sits at "<video path>.cursor.json".
const cursorSidecar = `${screenSource}.cursor.json`;
const hasCursorData = await fs
.stat(cursorSidecar)
.then((stats) => stats.isFile())
.catch(() => false);
if (hasCursorData) {
await copyIn(cursorSidecar);
}

const packedProject: PackedProjectData = {
...data,
media: {
...media,
screenVideoPath: newScreenPath,
...(newWebcamPath ? { webcamVideoPath: newWebcamPath } : {}),
},
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd -H -t f 'tsconfig*.json' --exec sh -c 'echo "== $1"; cat "$1"' _ {}
rg -n 'media\?\.|media\.' electron/cli/cliMain.ts

Repository: getopenscreen/openscreen

Length of output: 1238


🏁 Script executed:

#!/bin/bash
set -eu
sed -n '180,295p' electron/cli/cliMain.ts
sed -n '1,80p' electron/cli/cliMain.ts
rg -n "export interface .*Data|interface .*Data|type .*Project|interface Project|type Project|media:|webcamVideoPath|screenVideoPath" .

Repository: getopenscreen/openscreen

Length of output: 24642


Narrow media before using it.

screenVideoPath’s check doesn’t narrow media; make media non-undefined first, then use media.webcamVideoPath and spread ...media without additional null checks.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/cli/cliMain.ts` around lines 264 - 286, Update the surrounding flow
before accessing media so its presence is explicitly validated or narrowed
first. Then keep using media.webcamVideoPath and the ...media spread in the
PackedProjectData construction without additional null checks, preserving the
existing webcam and cursor-sidecar behavior.

Source: Coding guidelines

Comment thread electron/cli/cliMain.ts
Comment on lines +269 to +277
// Cursor telemetry sidecar sits at "<video path>.cursor.json".
const cursorSidecar = `${screenSource}.cursor.json`;
const hasCursorData = await fs
.stat(cursorSidecar)
.then((stats) => stats.isFile())
.catch(() => false);
if (hasCursorData) {
await copyIn(cursorSidecar);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

pack doesn't copy the .focus.json sidecar.

The cursor sidecar is packed but the new window-focus sidecar written at Line 554 is not, so a packed project loses --follow-windows telemetry and export --follow-windows silently degrades.

🐛 Proposed fix
-	// Cursor telemetry sidecar sits at "<video path>.cursor.json".
-	const cursorSidecar = `${screenSource}.cursor.json`;
-	const hasCursorData = await fs
-		.stat(cursorSidecar)
-		.then((stats) => stats.isFile())
-		.catch(() => false);
-	if (hasCursorData) {
-		await copyIn(cursorSidecar);
-	}
+	// Telemetry sidecars sit next to the video: "<video path>.cursor.json" and
+	// "<video path>.focus.json" (record --follow-windows).
+	const copySidecar = async (suffix: string): Promise<boolean> => {
+		const sidecar = `${screenSource}${suffix}`;
+		const exists = await fs
+			.stat(sidecar)
+			.then((stats) => stats.isFile())
+			.catch(() => false);
+		if (exists) await copyIn(sidecar);
+		return exists;
+	};
+	const hasCursorData = await copySidecar(".cursor.json");
+	await copySidecar(".focus.json");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Cursor telemetry sidecar sits at "<video path>.cursor.json".
const cursorSidecar = `${screenSource}.cursor.json`;
const hasCursorData = await fs
.stat(cursorSidecar)
.then((stats) => stats.isFile())
.catch(() => false);
if (hasCursorData) {
await copyIn(cursorSidecar);
}
// Telemetry sidecars sit next to the video: "<video path>.cursor.json" and
// "<video path>.focus.json" (record --follow-windows).
const copySidecar = async (suffix: string): Promise<boolean> => {
const sidecar = `${screenSource}${suffix}`;
const exists = await fs
.stat(sidecar)
.then((stats) => stats.isFile())
.catch(() => false);
if (exists) await copyIn(sidecar);
return exists;
};
const hasCursorData = await copySidecar(".cursor.json");
await copySidecar(".focus.json");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/cli/cliMain.ts` around lines 269 - 277, Update the pack flow near
the cursorSidecar handling to also detect and copy the window-focus sidecar
written by the follow-windows telemetry path, using the corresponding <screen
source>.focus.json filename. Preserve the existing missing-file behavior and
ensure both cursor and focus sidecars are included when present.

Comment thread electron/cli/cliMain.ts
Comment on lines +545 to +562
if (
result.success &&
command.kind === "record" &&
command.followWindows &&
focusSampler &&
result.screenVideoPath
) {
const focusData = focusSampler.stop(resolveRecordedDisplayId(command.displayIndex));
if (focusData && focusData.samples.length > 0) {
const focusDataPath = `${result.screenVideoPath}.focus.json`;
await fs.writeFile(focusDataPath, JSON.stringify(focusData), "utf8");
result.focusDataPath = focusDataPath;
} else {
output.error(
`Focus sampling produced no samples (helper diagnostics: ${focusSampler.diagnostics()})`,
);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Focus helper is only stopped on the success path — it leaks otherwise.

focusSampler.stop() runs only when result.success && command.followWindows && result.screenVideoPath. On a failed recording, a renderer crash (Line 652), did-fail-load (Line 648), or a missing screenVideoPath, the spawned helper is never stopped before app.exit(), leaving an orphaned native process sampling window focus.

Stop the sampler unconditionally during teardown (e.g. in a finally and on the exit paths), and only write the sidecar when the data is usable.

🛡️ Sketch
 			ipcMain.handle("cli-done", async (_event, result: CliDoneResult) => {
 				if (finished) return;
 				finished = true;
+				const focusData =
+					focusSampler?.stop(
+						command.kind === "record" ? resolveRecordedDisplayId(command.displayIndex) : undefined,
+					) ?? null;
 
 				try {
@@
-					if (
-						result.success &&
-						command.kind === "record" &&
-						command.followWindows &&
-						focusSampler &&
-						result.screenVideoPath
-					) {
-						const focusData = focusSampler.stop(resolveRecordedDisplayId(command.displayIndex));
-						if (focusData && focusData.samples.length > 0) {
+					if (result.success && focusData && result.screenVideoPath) {
+						if (focusData.samples.length > 0) {
 							const focusDataPath = `${result.screenVideoPath}.focus.json`;

…and add a focusSampler?.stop() call to the did-fail-load / render-process-gone / window-all-closed handlers.

🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 554-554: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(focusDataPath, JSON.stringify(focusData), "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(detect-non-literal-fs-filename-typescript)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/cli/cliMain.ts` around lines 545 - 562, Ensure the focus sampler is
stopped during all recording teardown paths, not only the successful branch
guarded by result.success and result.screenVideoPath. Update the surrounding
cleanup flow and the did-fail-load, render-process-gone, and window-all-closed
handlers to invoke focusSampler.stop() when present, while retaining sidecar
writing only when stopped data contains usable samples.

Comment on lines +89 to +96
let windowNumber = info[kCGWindowNumber as String] as? Int ?? 0
let title = info[kCGWindowName as String] as? String ?? ""
return FocusSample(
pid: pid,
appName: app.localizedName ?? "",
windowNumber: windowNumber,
windowTitle: title,
bounds: bounds,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

windowTitle is captured/persisted but unused by the zoom converter.

title (from kCGWindowName) is read here, forwarded by focusSampler.ts, and written into the .focus.json sidecar on disk, yet focusTelemetryToZoomRegions (src/lib/windowFocus/focusToZoomRegions.ts) only consumes appName/bounds. Window titles frequently contain sensitive content (document names, email subjects, chat previews). Persisting this to disk for a feature that doesn't currently need it is a data-minimization/PII-retention risk. If it's intentionally kept for the "future GUI toggle" mentioned in focusToZoomRegions.ts's header comment, consider gating it behind an explicit opt-in and documenting the sidecar's retention/cleanup policy; otherwise drop it from the emitted payload.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@electron/native/screencapturekit/Sources/OpenScreenMacOSFocusHelper/main.swift`
around lines 89 - 96, The focus sample currently persists sensitive window
titles that focusTelemetryToZoomRegions does not consume. Remove windowTitle
from the emitted FocusSample payload and eliminate the related
kCGWindowName/title extraction in the focus sampling flow, while preserving
appName, bounds, pid, and windowNumber.

Comment on lines +138 to +151
// Prefer the main process's approved session paths: they carry the
// packed-project sibling fallback when the stored absolute paths are stale.
try {
const sessionResult = await window.electronAPI.getCurrentRecordingSession();
const session = sessionResult?.session;
if (session?.screenVideoPath) {
media.screenVideoPath = session.screenVideoPath;
if (media.webcamVideoPath && session.webcamVideoPath) {
media.webcamVideoPath = session.webcamVideoPath;
}
}
} catch {
// Fall back to the paths stored in the project file.
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Where is the CLI recording session set/cleared for export?
fd -t f 'cliMain.ts' -x rg -n -C5 'CurrentRecordingSession|setCurrentRecordingSession|session'
rg -n -C4 'set-current-recording-session|get-current-recording-session' --type=ts

Repository: getopenscreen/openscreen

Length of output: 1821


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate CliExportRunner and cliMain files =="
fd -t f '(^CliExportRunner\.tsx$|^cliMain\.ts$)'

echo
echo "== outline CliExportRunner =="
cli_file="$(fd -t f '^CliExportRunner\.tsx$' | head -n1 || true)"
if [ -n "${cli_file:-}" ]; then
  wc -l "$cli_file"
  ast-grep outline "$cli_file" --view expanded || true
  echo
  echo "== relevant CliExportRunner lines =="
  sed -n '1,240p' "$cli_file" | cat -n
fi

cliMain_file="$(fd -t f '^cliMain\.ts$' | head -n1 || true)"
if [ -n "${cliMain_file:-}" ]; then
  echo
  echo "== cliMain outline =="
  ast-grep outline "$cliMain_file" --view expanded || true
  wc -l "$cliMain_file"
fi

echo
echo "== search session APIs/exports/imports =="
rg -n -C3 'getCurrentRecordingSession|setCurrentRecordingSession|setCurrentRecordingSession|currentRecordingSession|recording-session|current-recording-session' --type=ts --type=tsx

Repository: getopenscreen/openscreen

Length of output: 13126


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate candidate render/project loading files =="
rr -t f 'Render.*|runExport|export*|media.*' | head -n 80 || true
fd -t f 'Render.*\.(ts|tsx)$' | sed -n '1,100p'
fd -t f '.*Export.*\.(ts|tsx)$' | sed -n '1,100p'

echo
echo "== project/session/projectPath usages in relevant files =="
rg -n -C4 'projectPath|screenVideoPath|webcamVideoPath|cliWindow|ipcRenderer|electronAPI|getCurrentRecordingSession|setCurrentRecordingSession|runExport|export' --glob '*.{ts,tsx}' src | sed -n '1,260p'

Repository: getopenscreen/openscreen

Length of output: 16520


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate candidate render/project loading files =="
rg -t f 'Render.*|runExport|export*|media.*' . | head -n 80 || true
fd -t f 'Render.*\.(ts|tsx)$' src | sed -n '1,120p'
fd -t f '.*Export.*\.(ts|tsx)$' src | sed -n '1,160p'

echo
echo "== project/session/projectPath usages in src =="
rg -n -C4 'projectPath|screenVideoPath|webcamVideoPath|cliWindow|ipcRenderer|electronAPI|getCurrentRecordingSession|setCurrentRecordingSession|runExport|export' --glob '*.{ts,tsx}' src | sed -n '1,260p'

Repository: getopenscreen/openscreen

Length of output: 16099


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== focus on cliMain session/currentRecordingSession and CLI request setup =="
rg -n -C8 'CurrentRecordingSession|setCurrentRecordingSession|currentRecordingSession|set-current-recording-session|get-current-recording-session|cliRequest|cli window|cli-sources|cli-export|cliDone|cliLog|projectPath' electron/cli/cliMain.ts

echo
echo "== exact lines around CliExportRunner media/session override fallback =="
sed -n '360,426p' src/cli/CliExportRunner.tsx | cat -n

echo
echo "== main/IPC implementation of getCurrentRecordingSession in all tracked files =="
rg -n -C8 'getCurrentRecordingSession|setCurrentRecordingSession|currentRecordingSession|set-current-recording-session|get-current-recording-session' .

Repository: getopenscreen/openscreen

Length of output: 38657


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== cliMain export setup with project session APIs =="
sed -n '440,495p' electron/cli/cliMain.ts | cat -n

echo
echo "== getApprovedProjectSession implementation =="
rg -n -C15 'function getApprovedProjectSession|async function getApprovedProjectSession' electron/ipc/handlers.ts

echo
echo "== loadProjectFileFromPath implementation and CLI call sites =="
rg -n -C8 'loadProjectFileFromPath|loadCurrentProjectFile|setCurrentRecordingSession|cliGetRequest|cli-done|cliLog' src electron -g '*.{ts,tsx}'

Repository: getopenscreen/openscreen

Length of output: 50380


Check that the current recording session belongs to the exported project before using these paths.

runPackCommand writes sibling-mapped paths back to the project, but it does not populate the main-process recording session. In that case, loadProjectFileFromPath falls back to an existing currentRecordingSession, which can carry unrelated recording paths from a previous operation. Add a project/session binding or clear currentRecordingSession when packing an arbitrary output folder.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/CliExportRunner.tsx` around lines 138 - 151, The session paths used
in the export flow must be verified as belonging to the project being exported
before replacing values in media. Update the logic around
getCurrentRecordingSession and runPackCommand to add a project/session binding
check, or clear currentRecordingSession when packing an arbitrary output folder,
and only apply session paths when that association is confirmed.

Comment on lines +206 to +208
const probed = await probeVideoDimensions(videoUrl);
const sourceWidth = probed.width || DEFAULT_SOURCE_DIMENSIONS.width;
const sourceHeight = probed.height || DEFAULT_SOURCE_DIMENSIONS.height;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Non-finite video duration silently disables zoom generation.

probeVideoDimensions maps a non-finite video.duration (common for WebM without a duration fix) to 0. Both --follow-windows and --auto-zoom then get totalMs: 0 and produce no regions, with the log line reporting "added 0 region(s)" rather than an actionable error. Consider falling back to the telemetry span or warning explicitly when durationMs === 0.

Also applies to: 219-222

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/CliExportRunner.tsx` around lines 206 - 208, Update the video probing
flow around probeVideoDimensions so a durationMs of 0 is not silently passed to
the --follow-windows and --auto-zoom region generation paths. Use the available
telemetry span duration as a fallback when possible; otherwise emit an
actionable warning or error before reporting zero regions.

Comment on lines +264 to +288
// Completion: recording flipped off and the session finished saving.
// biome-ignore lint/correctness/useExhaustiveDependencies: completion is keyed on recording/saving; fail is stable
useEffect(() => {
if (recordingStartedAtRef.current === null) return;
if (recording || saving) return;
if (phaseRef.current === "done") return;
phaseRef.current = "done";

void (async () => {
try {
const sessionResult = await window.electronAPI.getCurrentRecordingSession();
const session = sessionResult?.session;
if (!session?.screenVideoPath) {
throw new Error("Recording finished but no session manifest was stored");
}
const durationMs = Date.now() - (recordingStartedAtRef.current ?? Date.now());
const request = requestRef.current;
await window.electronAPI.cliDone({
success: true,
screenVideoPath: session.screenVideoPath,
webcamVideoPath: session.webcamVideoPath,
cursorDataPath: `${session.screenVideoPath}.cursor.json`,
durationMs,
...(request?.projectOut ? { projectData: buildDefaultProject(session) } : {}),
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

durationMs includes save time.

The timestamp is taken after saving completes, so the reported duration is start→save-finished rather than the actual capture length (finalizing native/webm recordings can take seconds). Capture the stop instant when the phase flips to stopping, or prefer the session manifest's duration if it carries one.

🐛 Proposed fix
+	const recordingStoppedAtRef = useRef<number | null>(null);
 			if (stopRequestedRef.current) {
 				phaseRef.current = "stopping";
+				recordingStoppedAtRef.current = Date.now();
 				setStatus("Stopping…");
 				toggleRecordingRef.current();
 				return;
 			}
-				const durationMs = Date.now() - (recordingStartedAtRef.current ?? Date.now());
+				const stoppedAt = recordingStoppedAtRef.current ?? Date.now();
+				const durationMs = stoppedAt - (recordingStartedAtRef.current ?? stoppedAt);

(also set recordingStoppedAtRef.current in the duration-timer and external-stop paths)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/CliRecordRunner.tsx` around lines 264 - 288, Update the completion
flow around recordingStartedAtRef and the phase transition to use the capture
stop timestamp rather than Date.now() after saving; set
recordingStoppedAtRef.current when entering the stopping phase, including
duration-timer and external-stop paths, then calculate durationMs from that
timestamp. If the session manifest exposes a reliable duration, prefer it.

Comment on lines +54 to +61
const frameCount = Math.max(1, Math.ceil(durationSec * OUTPUT_SAMPLE_RATE));
const context = new OfflineAudioContext(OUTPUT_CHANNELS, frameCount, OUTPUT_SAMPLE_RATE);

const voiceover = await decodeToBuffer(context, options.voiceoverData);
const voiceoverNode = context.createBufferSource();
voiceoverNode.buffer = voiceover;
voiceoverNode.connect(context.destination);
voiceoverNode.start(Math.max(0, options.offsetSec));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Voiceover longer than the video is silently truncated.

frameCount is derived solely from the video duration, so a voiceover (or a large offsetSec) that runs past the end is cut with no signal to the user. Consider surfacing this — e.g. return the clipped amount so CliExportRunner can emit a warning in CliDoneResult.warnings.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/exporter/voiceoverMix.ts` around lines 54 - 61, The voiceover mixing
flow around frameCount and voiceoverNode.start currently truncates audio beyond
the video duration without reporting it. Update the relevant export function to
calculate the portion exceeding the OfflineAudioContext duration, return the
clipped amount alongside the existing result, and expose it so CliExportRunner
can add a warning to CliDoneResult.warnings; preserve current mixing behavior
for voiceovers that fit.

Window mode captures one window cleanly, but a real demo moves between a
terminal, an editor, a browser. `record --windows "a,b,c"` captures every
matched window continuously in parallel — ScreenCaptureKit window capture,
so each feed stays clean even while occluded — plus the focus timeline.
`export --follow-windows` then composites one video that switches to
whichever window had focus, the incoming window sliding in from the side
it sat on (450 ms eased slide; direction follows screen geometry).

- Swift helper: new optional `multiWindows` request mode running N
  SCStreams + writers in one process. This is load-bearing: a second
  helper *process* interrupts the first stream (SCStreamErrorDomain
  -3805, empirically verified), while in-process streams coexist.
- Focus helper now reports the CGWindowID so focus samples bind exactly
  to captured windows.
- electron/cli/multiWindowRecorder.ts: match windows by title, drive the
  multi-capture helper + focus sampler, write a
  `<primary>.multiwindow.json` manifest (windows + focus timeline).
- src/lib/windowSwitch/: manifest contracts and a pure, unit-tested
  switch-timeline builder (1.2 s dwell, flicker collapse, unrecorded
  windows keep the current screen, geometry-based slide direction).
- src/cli/multiWindowCompositor.ts: offline lockstep decode of all
  window videos (frame channels with backpressure) → slide compositing →
  VideoEncoder/VideoMuxer intermediate, which then flows through the
  normal export pipeline (wallpaper, padding, annotations, --audio).

macOS only; audio via export --audio for now.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J
@PeterTakahashi

Copy link
Copy Markdown
Author

Pushed 2d1cd2f, which adds the mode this PR was really aiming for: clean multi-window capture with slide transitions.

openscreen record --windows "Terminal,Code,Chrome" --project demo.openscreen
openscreen export demo.openscreen -o demo.mp4 --follow-windows

Every matched window is captured continuously in parallel with ScreenCaptureKit window capture — each feed stays clean (no desktop, no overlap) even while occluded — alongside the focus timeline. Export composites one video that switches to whichever window had focus, the incoming window sliding in from the side it sat on (450 ms eased slide, direction from screen geometry). Because all windows are always recording, both sides of a transition are live footage.

Implementation notes:

  • One helper process, N streams — this is load-bearing: launching a second helper process interrupts the first stream with SCStreamErrorDomain -3805 (empirically verified: A starts → B starts → A dies). Multiple SCStreams inside one process coexist fine, so the Swift helper gained an optional multiWindows request mode (one SCStream + AVAssetWriter per window). The single-window path is untouched.
  • The focus helper now reports CGWindowID, binding focus samples exactly to captured windows.
  • The switch timeline is a pure, unit-tested module (1.2 s dwell, flicker collapse, focus on unlisted windows keeps the current screen); the offline compositor decodes all windows in lockstep (backpressured frame channels) and its output flows through the normal export pipeline, so wallpaper/padding/annotations/--audio all compose.
  • The earlier zoom-based --follow-windows display mode remains for full-screen recordings; the exporter picks the mode by the presence of the .multiwindow.json manifest.

Verified end-to-end on macOS: two real windows recorded in parallel via the CLI, three-segment focus timeline, exported 1080p with two slide transitions — frame inspection confirms both windows live mid-slide. Full suite 456 tests passing, lint/tsc clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (2)
src/lib/windowSwitch/switchTimeline.test.ts (1)

44-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a from-left case.

Only the from-right branch of the geometry rule is exercised; a switch from the right-hand window back to the left-hand one would catch a sign inversion in direction: toCenter >= fromCenter.

♻️ Suggested extra test
 	it("keeps the previous window when focus moves to an unrecorded window", () => {
it("slides in from the left when the target sits to the left", () => {
	const timeline = buildWindowSwitchTimeline(
		manifest([
			{ timeMs: 0, windowNumber: 202 },
			{ timeMs: 8000, windowNumber: 101 },
		]),
		20_000,
	);
	expect(timeline.transitions[0].direction).toBe("from-left");
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/windowSwitch/switchTimeline.test.ts` around lines 44 - 93, Add a
complementary test in the buildWindowSwitchTimeline suite where focus switches
from window 202 to window 101, using the existing manifest and timeline setup,
and assert the first transition direction is "from-left" to cover the opposite
geometry branch.
src/cli/multiWindowCompositor.ts (1)

92-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No colocated tests for the compositor.

FrameChannel handoff/finish semantics and the slide-offset math are pure enough to unit test under jsdom without WebCodecs; only composeMultiWindowVideo needs mocking. As per coding guidelines, "Add a test for every new behavior in the same package as the code under test."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/multiWindowCompositor.ts` around lines 92 - 99, Add colocated unit
tests for the compositor package covering FrameChannel handoff and finish
semantics, plus slide-offset calculations, using jsdom without WebCodecs; mock
dependencies needed to exercise composeMultiWindowVideo, including its
validation behavior. Keep the tests alongside the implementation and cover each
newly introduced behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@electron/cli/multiWindowRecorder.ts`:
- Around line 282-300: Update the window mapping in the multi-window recording
flow to use each window’s start-time bounds from the recording-started event and
its captureBounds data when no matching focus sample exists. Preserve
focus-sample bounds when available, and eliminate the { x: 0, y: 0, width: 0,
height: 0 } fallback so buildWindowSwitchTimeline receives meaningful geometry
for slide-direction calculation.
- Around line 166-172: Change the child-process termination handler from the
"exit" event to "close" so stdout/stderr are fully drained before rejecting
pending operations. Preserve the existing error construction and calls to
rejectAllStarted and rejectAllStopped, including successful code handling and
stderrTail details.
- Around line 116-121: Add a no-op rejection handler immediately after creating
the allStopped promise, while preserving its existing resolve/reject behavior
for stop().

In
`@electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift`:
- Around line 714-717: Update the recorder startup loop around recorder.start()
to catch partial-start failures, stop every recorder that was started (including
finalizing its AVAssetWriter), then rethrow the original error. Preserve the
existing await stopTask.value flow when all recorders start successfully.
- Around line 691-712: Prevent the stdin command loop around stopTask from
invoking recorder.stop() while recorder startup is still running. Gate command
processing until all recorder start() calls have completed, or update start() to
check isStopping before and during stream/writer setup and clean up any
partially started resources; ensure stop during startup cannot create or start
resources after shutdown begins.

In `@src/cli/multiWindowCompositor.ts`:
- Around line 140-169: Extend the try/finally cleanup scope in the compositor
function to include canvas, muxer, and encoder setup, including getContext,
muxer.initialize, and encoder.configure, so decoder cleanup always runs when
setup fails. Preserve the existing decoder kickoff and finally-based channel
draining behavior for normal processing.
- Around line 233-250: Update the cleanup in the finally block of the encoding
flow to defensively close the VideoEncoder on both success and error paths.
Ensure encoder.close() is invoked from finally, while preserving the existing
successful flush/finalize behavior and source-channel draining.

---

Nitpick comments:
In `@src/cli/multiWindowCompositor.ts`:
- Around line 92-99: Add colocated unit tests for the compositor package
covering FrameChannel handoff and finish semantics, plus slide-offset
calculations, using jsdom without WebCodecs; mock dependencies needed to
exercise composeMultiWindowVideo, including its validation behavior. Keep the
tests alongside the implementation and cover each newly introduced behavior.

In `@src/lib/windowSwitch/switchTimeline.test.ts`:
- Around line 44-93: Add a complementary test in the buildWindowSwitchTimeline
suite where focus switches from window 202 to window 101, using the existing
manifest and timeline setup, and assert the first transition direction is
"from-left" to cover the opposite geometry branch.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ad3fafc0-879e-4f5e-a1ff-924f7e2b5c57

📥 Commits

Reviewing files that changed from the base of the PR and between 31d7c41 and 2d1cd2f.

📒 Files selected for processing (14)
  • docs/cli.md
  • electron/cli/args.ts
  • electron/cli/cliMain.ts
  • electron/cli/focusSampler.ts
  • electron/cli/multiWindowRecorder.ts
  • electron/native/screencapturekit/Sources/OpenScreenMacOSFocusHelper/main.swift
  • electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift
  • src/cli/CliExportRunner.tsx
  • src/cli/multiWindowCompositor.ts
  • src/lib/cliContracts.ts
  • src/lib/windowFocus/contracts.ts
  • src/lib/windowSwitch/contracts.ts
  • src/lib/windowSwitch/switchTimeline.test.ts
  • src/lib/windowSwitch/switchTimeline.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • src/lib/windowFocus/contracts.ts
  • src/lib/cliContracts.ts
  • electron/cli/focusSampler.ts
  • docs/cli.md
  • src/cli/CliExportRunner.tsx
  • electron/cli/args.ts
  • electron/cli/cliMain.ts

Comment on lines +116 to +121
let resolveAllStopped: (paths: string[]) => void;
let rejectAllStopped: (error: Error) => void;
const allStopped = new Promise<string[]>((resolve, reject) => {
resolveAllStopped = resolve;
rejectAllStopped = reject;
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

allStopped can reject with no attached handler → unhandled rejection.

Nothing awaits allStopped until stop() is called. Any helper error event, spawn error, or early exit before that rejects a promise with no handler, which surfaces as an unhandled rejection in the main process. Attach a no-op catch at creation.

🛠️ Proposed fix
 	const allStopped = new Promise<string[]>((resolve, reject) => {
 		resolveAllStopped = resolve;
 		rejectAllStopped = reject;
 	});
+	// Keep the rejection handled until stop() awaits it.
+	allStopped.catch(() => {});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let resolveAllStopped: (paths: string[]) => void;
let rejectAllStopped: (error: Error) => void;
const allStopped = new Promise<string[]>((resolve, reject) => {
resolveAllStopped = resolve;
rejectAllStopped = reject;
});
let resolveAllStopped: (paths: string[]) => void;
let rejectAllStopped: (error: Error) => void;
const allStopped = new Promise<string[]>((resolve, reject) => {
resolveAllStopped = resolve;
rejectAllStopped = reject;
});
// Keep the rejection handled until stop() awaits it.
allStopped.catch(() => {});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/cli/multiWindowRecorder.ts` around lines 116 - 121, Add a no-op
rejection handler immediately after creating the allStopped promise, while
preserving its existing resolve/reject behavior for stop().

Comment on lines +166 to +172
child.on("exit", (code) => {
const error = new Error(
`Capture helper exited (${code})${stderrTail ? `: ${stderrTail}` : ""}`,
);
rejectAllStarted(error);
rejectAllStopped(error);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Reject on close, not exit, or the final recording-stopped lines can be lost.

The helper calls exit(0) immediately after the last recorder finishes. Node can emit exit before the stdout pipe is fully drained/parsed, so rejectAllStopped may fire while the last recording-stopped events are still buffered, turning a successful stop into Capture helper exited (0). close fires only after the stdio streams are closed.

🛠️ Proposed fix
-	child.on("exit", (code) => {
+	child.on("close", (code) => {
 		const error = new Error(
 			`Capture helper exited (${code})${stderrTail ? `: ${stderrTail}` : ""}`,
 		);
 		rejectAllStarted(error);
 		rejectAllStopped(error);
 	});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
child.on("exit", (code) => {
const error = new Error(
`Capture helper exited (${code})${stderrTail ? `: ${stderrTail}` : ""}`,
);
rejectAllStarted(error);
rejectAllStopped(error);
});
child.on("close", (code) => {
const error = new Error(
`Capture helper exited (${code})${stderrTail ? `: ${stderrTail}` : ""}`,
);
rejectAllStarted(error);
rejectAllStopped(error);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/cli/multiWindowRecorder.ts` around lines 166 - 172, Change the
child-process termination handler from the "exit" event to "close" so
stdout/stderr are fully drained before rejecting pending operations. Preserve
the existing error construction and calls to rejectAllStarted and
rejectAllStopped, including successful code handling and stderrTail details.

Comment on lines +282 to +300
const windows: CapturedWindow[] = matched.map((window, index) => {
const focusMatch = focusData?.samples.find(
(sample) => sample.windowNumber === window.windowId,
);
return {
windowId: window.windowId,
appName: focusMatch?.appName ?? "",
title: window.title,
videoPath: videoPaths[index],
bounds: focusMatch
? {
x: focusMatch.x,
y: focusMatch.y,
width: focusMatch.width,
height: focusMatch.height,
}
: { x: 0, y: 0, width: 0, height: 0 },
};
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Zero-bounds fallback silently degrades slide direction.

A window that never appears in the focus samples gets {0,0,0,0} bounds. buildWindowSwitchTimeline (src/lib/windowSwitch/switchTimeline.ts Line 100-106) derives the slide direction from bounds.x + width/2, so such windows always compare as center 0 and every transition into/out of them resolves to "from-right". Consider capturing bounds at start time (the helper already reports captureBounds in its recording-started event) instead of relying on focus samples.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/cli/multiWindowRecorder.ts` around lines 282 - 300, Update the
window mapping in the multi-window recording flow to use each window’s
start-time bounds from the recording-started event and its captureBounds data
when no matching focus sample exists. Preserve focus-sample bounds when
available, and eliminate the { x: 0, y: 0, width: 0, height: 0 } fallback so
buildWindowSwitchTimeline receives meaningful geometry for slide-direction
calculation.

Comment on lines +691 to +712
let stopTask = Task.detached {
while let line = readLine() {
let command = line.trimmingCharacters(in: .whitespacesAndNewlines)
switch command {
case "stop":
for recorder in recorders {
await recorder.stop()
}
exit(0)
case "pause":
for recorder in recorders {
recorder.pause()
}
case "resume":
for recorder in recorders {
recorder.resume()
}
default:
break
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect the Swift package manifest for language/tools version and strict concurrency settings.
fd -t f 'Package.swift' | xargs -r cat
fd -t f '.swift-version' | xargs -r cat

Repository: getopenscreen/openscreen

Length of output: 1024


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift"
echo "== main.swift line count =="
wc -l "$file"
echo "== outline around relevant symbols =="
ast-grep outline "$file" --match "ScreenCaptureRecorder" --view expanded || true
echo "== start/stop command and recorder usages =="
rg -n "ScreenCaptureRecorder|recorders|start\\(|stop\\(|pause\\(|resume\\(|isStopping|Task\\.detached|readLine" "$file"
echo "== relevant sections =="
cat -n "$file" | sed -n '620,735p'
cat -n "$file" | sed -n '1,180p'

Repository: getopenscreen/openscreen

Length of output: 13031


🏁 Script executed:

#!/bin/bash
set -euo pipefail
file="electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift"

echo "== ScreenCaptureRecorder start/stop implementations =="
cat -n "$file" | sed -n '133,290p'

echo "== multi-window recording path with stopTask/start/startup =="
cat -n "$file" | sed -n '691,720p'

echo "== single-window recording path with stopTask/start/startup =="
cat -n "$file" | sed -n '736,770p'

echo "== all isStopping references =="
rg -n "isStopping|start\\(|SCStream\\(|addOutput|addAudioInput|setupWriter|finishWriter" "$file"

Repository: getopenscreen/openscreen

Length of output: 8259


Prevent stop from racing against stream startup

The stdin command loop is launched before start() completes, so a stop during startup can call recorder.stop() before stream or writer have been set up. stop() marks isStopping, but the pending start() path still creates/starts SCStream and AVAssetWriter streams and returns, while the helper exits immediately afterward. Gate the command loop until all streams are ready, or make start() return early when isStopping and complete any partially created writer/stream only if it was started.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift`
around lines 691 - 712, Prevent the stdin command loop around stopTask from
invoking recorder.stop() while recorder startup is still running. Gate command
processing until all recorder start() calls have completed, or update start() to
check isStopping before and during stream/writer setup and clean up any
partially started resources; ensure stop during startup cannot create or start
resources after shutdown begins.

Comment on lines +714 to +717
for recorder in recorders {
try await recorder.start()
}
await stopTask.value

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Partial start failure leaves running streams and unfinalized MP4s.

start() is awaited sequentially; if the Nth window fails (window gone, permission, writer setup), the error propagates to main, which calls exit(1) while recorders 0..N-1 still hold live SCStreams whose AVAssetWriters were never finished — those output files are unplayable. Wrap the loop and stop everything already started before rethrowing.

🛠️ Proposed fix
-	for recorder in recorders {
-		try await recorder.start()
-	}
+	var started: [ScreenCaptureRecorder] = []
+	do {
+		for recorder in recorders {
+			try await recorder.start()
+			started.append(recorder)
+		}
+	} catch {
+		for recorder in started {
+			await recorder.stop()
+		}
+		throw error
+	}
 	await stopTask.value
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for recorder in recorders {
try await recorder.start()
}
await stopTask.value
var started: [ScreenCaptureRecorder] = []
do {
for recorder in recorders {
try await recorder.start()
started.append(recorder)
}
} catch {
for recorder in started {
await recorder.stop()
}
throw error
}
await stopTask.value
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift`
around lines 714 - 717, Update the recorder startup loop around recorder.start()
to catch partial-start failures, stop every recorder that was started (including
finalizing its AVAssetWriter), then rethrow the original error. Preserve the
existing await stopTask.value flow when all recorders start successfully.

Comment on lines +140 to +169
const canvas = document.createElement("canvas");
canvas.width = canvasWidth;
canvas.height = canvasHeight;
const ctx = canvas.getContext("2d");
if (!ctx) throw new Error("Failed to create compositor canvas");

const muxer = new VideoMuxer(
{ width: canvasWidth, height: canvasHeight, frameRate: FRAME_RATE, bitrate: BITRATE },
false,
);
await muxer.initialize();

let encodeError: Error | null = null;
const encoder = new VideoEncoder({
output: (chunk, meta) => {
void muxer.addVideoChunk(chunk, meta).catch((error: unknown) => {
encodeError = error instanceof Error ? error : new Error(String(error));
});
},
error: (error) => {
encodeError = error;
},
});
encoder.configure({
codec: CODEC,
width: canvasWidth,
height: canvasHeight,
bitrate: BITRATE,
framerate: FRAME_RATE,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Decoders are started before the try, so setup failures leak them.

decodeAll is kicked off at Line 127-138, but the finally that drains the channels only guards the loop starting at Line 172. If getContext("2d") returns null, muxer.initialize() rejects, or encoder.configure throws, every decoder is left running with a producer blocked on channel.push, and the pending VideoFrames are never closed. Either move the decoder kick-off inside the try, or extend the try/finally to cover canvas/muxer/encoder setup.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/multiWindowCompositor.ts` around lines 140 - 169, Extend the
try/finally cleanup scope in the compositor function to include canvas, muxer,
and encoder setup, including getContext, muxer.initialize, and
encoder.configure, so decoder cleanup always runs when setup fails. Preserve the
existing decoder kickoff and finally-based channel draining behavior for normal
processing.

Comment on lines +233 to +250
await encoder.flush();
encoder.close();
const blob = await muxer.finalize();
return { blob, durationMs: (totalFrames / FRAME_RATE) * 1000, timeline };
} finally {
for (const source of sources) {
source.lastFrame?.close();
source.lastFrame = null;
// Drain so pending decodeAll calls can finish and release resources.
void (async () => {
let frame = await source.channel.next();
while (frame) {
frame.close();
frame = await source.channel.next();
}
})();
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Encoder is never closed on the error path.

encoder.close() only runs on the success path; if the loop throws (decodeError, encodeError, encode failure), the VideoEncoder stays open and holds its GPU/codec resources for the lifetime of the renderer. Close it defensively in the finally.

🛠️ Proposed fix
 	} finally {
+		if (encoder.state !== "closed") encoder.close();
 		for (const source of sources) {
 			source.lastFrame?.close();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await encoder.flush();
encoder.close();
const blob = await muxer.finalize();
return { blob, durationMs: (totalFrames / FRAME_RATE) * 1000, timeline };
} finally {
for (const source of sources) {
source.lastFrame?.close();
source.lastFrame = null;
// Drain so pending decodeAll calls can finish and release resources.
void (async () => {
let frame = await source.channel.next();
while (frame) {
frame.close();
frame = await source.channel.next();
}
})();
}
}
await encoder.flush();
encoder.close();
const blob = await muxer.finalize();
return { blob, durationMs: (totalFrames / FRAME_RATE) * 1000, timeline };
} finally {
if (encoder.state !== "closed") encoder.close();
for (const source of sources) {
source.lastFrame?.close();
source.lastFrame = null;
// Drain so pending decodeAll calls can finish and release resources.
void (async () => {
let frame = await source.channel.next();
while (frame) {
frame.close();
frame = await source.channel.next();
}
})();
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/multiWindowCompositor.ts` around lines 233 - 250, Update the cleanup
in the finally block of the encoding flow to defensively close the VideoEncoder
on both success and error paths. Ensure encoder.close() is invoked from finally,
while preserving the existing successful flush/finalize behavior and
source-channel draining.

@EtienneLescot

Copy link
Copy Markdown
Collaborator

main has just been rebased onto the 1.8.0 line — a ~390-commit merge landing the v4 editor, the native Rust compositor and export path, and the Linux capture stack. Every open PR is out of date; sorry for the churn. Here's where this one stands:

Same situation as #176, which this is stacked on, plus two extras:

  • @/lib/exporter/muxer is gone along with the rest of the web export pipeline
  • the macOS helper Sources/OpenScreenScreenCaptureKitHelper/main.swift was split into ScreenCaptureRecorder.swift + AudioTrackMixer.swift, so the focus-helper changes need repointing

Suggestion: land #176 against the new main first, then rebase this on top. The window-focus → zoom-region idea is untouched by the merge — the zoom spring is still there, it's the CLI plumbing underneath that moved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants