feat(cli): headless record / export / info commands - #176
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesHeadless CLI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLIProcess
participant ElectronMain
participant HiddenRunner
participant MediaPipeline
CLIProcess->>ElectronMain: parse and start command
ElectronMain->>HiddenRunner: load command-specific runner
HiddenRunner->>MediaPipeline: record, export, enumerate, or caption
MediaPipeline-->>HiddenRunner: result and progress
HiddenRunner-->>ElectronMain: completion payload
ElectronMain-->>CLIProcess: NDJSON or text output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
src/cli/CliExportRunner.tsx (1)
42-57: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on metadata probe — a stalled load hangs the CLI forever.
onerrorcovers decode failures, but a media element that never fires either event (unreachable/locked file, stalled reader) leaves the promise pending, and the CLI has no export-side deadline to fall back on. A bounded wait keepsopenscreen exportfrom hanging headlessly.♻️ Proposed fix
function probeVideoDimensions(url: string): Promise<{ width: number; height: number }> { return new Promise((resolve, reject) => { const video = document.createElement("video"); video.preload = "metadata"; video.muted = true; + const timer = setTimeout(() => { + video.removeAttribute("src"); + video.load(); + reject(new Error(`Timed out reading video metadata: ${url}`)); + }, 30_000); video.onloadedmetadata = () => { + clearTimeout(timer); const width = video.videoWidth; const height = video.videoHeight; video.removeAttribute("src"); video.load(); resolve({ width, height }); }; - video.onerror = () => reject(new Error(`Failed to load video metadata: ${url}`)); + video.onerror = () => { + clearTimeout(timer); + reject(new Error(`Failed to load video metadata: ${url}`)); + }; video.src = url; }); }🤖 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 42 - 57, Update probeVideoDimensions to enforce a bounded timeout for video metadata loading, rejecting the promise when neither onloadedmetadata nor onerror fires before the deadline. Ensure successful metadata loads and errors clear the timeout and perform the existing video cleanup so the CLI cannot remain pending indefinitely.electron/cli/args.test.ts (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPath assertions are POSIX-only.
resolvePathdelegates topath.resolve, so on a Windows runnerparse(["export","demo.openscreen"])yields\work\demo.openscreenand every hard-coded/work/...expectation fails. Build expectations withpath.resolve(CWD, …)(orpath.join) so the suite is platform-agnostic.♻️ Suggested approach
+import path from "node:path"; import { describe, expect, it } from "vitest"; import { parseCliArgs } from "./args"; const CWD = "/work"; +const at = (name: string) => path.resolve(CWD, name);Then use
projectPath: at("demo.openscreen")instead of the literal.Also applies to: 17-20
🤖 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 4 - 8, Update path expectations in the tests around parse and the projectPath assertions to use a platform-aware helper such as at, backed by path.resolve(CWD, ...) or path.join(CWD, ...), instead of hard-coded /work/... strings. Ensure all affected expected paths, including demo.openscreen, match resolvePath on Windows and POSIX systems.src/lib/exporter/voiceoverMix.ts (1)
105-135: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel the partially started Output if remux fails.
After
output.start()fails once adding packets oraudioSource.add(mixedAudio)throws, thefinallyonly disposes the input;outputremains started and unfinalized, andBufferTarget.bufferwill not be the remux output. Track whetheroutput.start()has resolved and awaitoutput.cancel()in the catch path before rethrowing.🤖 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 105 - 135, The remux cleanup must cancel a started but unfinalized Output when packet or audio processing fails. In the surrounding flow that creates output and calls output.start(), track successful startup, catch subsequent errors, await output.cancel() when startup completed, then rethrow; retain input.dispose() in finally and avoid canceling if output.start() never resolved.electron/preload.ts (1)
309-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType these against the CLI contracts instead of
unknown.
electron-env.d.tsalready declaresCliProgressEvent/CliDoneResult; typing the preload the same way keeps the two sides from drifting.🤖 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/preload.ts` around lines 309 - 317, Update the cliProgress and cliDone methods in the preload API to use the existing CliProgressEvent and CliDoneResult types from electron-env.d.ts instead of unknown, while preserving their current IPC behavior and cliLog signature.electron/cli/cliMain.ts (1)
245-252: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
JSON.stringifyon a circular arg throws outsidesafeWrite.The
args.map(...)runs beforesafeWrite, so any log call carrying a circular object (common with Electron/DOM-ish objects) throws fromconsole.log, escaping into theuncaughtExceptionhandler and killing the CLI run.♻️ Proposed fix
+ const stringify = (a: unknown) => { + if (typeof a === "string") return a; + try { + return JSON.stringify(a); + } catch { + return String(a); + } + }; for (const level of ["log", "info", "warn", "error", "debug"] as const) { console[level] = (...args: unknown[]) => { - safeWrite( - process.stderr, - `${args.map((a) => (typeof a === "string" ? a : JSON.stringify(a))).join(" ")}\n`, - ); + safeWrite(process.stderr, `${args.map(stringify).join(" ")}\n`); }; }🤖 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 245 - 252, Update the console method overrides in the level loop so argument serialization cannot throw before safeWrite, including for circular or otherwise non-serializable objects. Preserve string arguments and the existing space-separated output, while using a safe fallback representation for values that JSON.stringify cannot handle.src/cli/CliRecordRunner.tsx (2)
122-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRef writes during render.
toggleRecordingRef/recordingRefare mutated in the render body; React may discard or replay that work. Move both into an effect.♻️ Proposed fix
- const toggleRecordingRef = useRef(toggleRecording); - toggleRecordingRef.current = toggleRecording; - const recordingRef = useRef(recording); - recordingRef.current = recording; + const toggleRecordingRef = useRef(toggleRecording); + const recordingRef = useRef(recording); + useEffect(() => { + toggleRecordingRef.current = toggleRecording; + recordingRef.current = recording; + });🤖 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 122 - 126, Move the current-value assignments for toggleRecordingRef and recordingRef out of the render body and into a React effect in the surrounding component. Keep both refs initialized with their corresponding values and update their .current fields after committed renders so the stop/finish effects continue using the latest values.Source: Linters/SAST tools
98-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test accompanies this runner.
The cohort only adds
electron/cli/args.test.ts; the record runner's bootstrap/stop/completion state machine is untested. As per coding guidelines, "Add a test for every new behavior in the same package as the code under test." Happy to draft a test that stubswindow.electronAPIand asserts the start/stop/cliDonetransitions.🤖 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 98 - 108, Add tests in the same package for CliRecordRunner’s bootstrap and recording state machine, stubbing window.electronAPI as needed. Cover recorder start, stop, and cliDone completion transitions, including the requestReady bootstrap path.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 `@docs/cli.md`:
- Around line 7-9: Add a language identifier to the fenced code block containing
the record-to-export workflow in docs/cli.md, using text for descriptive content
or bash only if it is intended to be executable.
In `@electron/cli/args.ts`:
- Around line 183-191: Update the --preview-size parsing in the argument switch
to reject zero-valued width or height after matching the dimensions. Preserve
the existing format validation and assignment for positive dimensions, while
throwing the same style of descriptive error when either parsed value is zero.
In `@electron/cli/cliMain.ts`:
- Around line 212-225: Update the JSON and human-readable output branches in the
CLI summary writer to use the existing safeWrite helper instead of direct
process.stdout.write calls. Ensure both writes handle closed-pipe EPIPE errors
before runCli installs its stdout error guards, preserving the current output
content and formatting.
- Around line 283-287: Hoist the finished flag to the surrounding function scope
and remove the inner declaration near the deferred exit logic. In the
app.on("window-all-closed") handler, return immediately when finished is set
before logging the unexpected closure or calling app.exit(1), while preserving
the existing failure behavior for unfinished runs.
In `@src/cli/CliRecordRunner.tsx`:
- Around line 226-234: Update the stop-signal effect around onCliStopRecording
to retain a stop request received during initialization instead of ignoring it.
Use the existing stopRequestedRef, then check it immediately after
startRecordingImmediately() resolves and abort or stop recording so the CLI
always reaches completion; preserve the current stopping behavior when recording
is already active.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 57-58: Update startRecordingImmediately and the underlying
startRecording flow to report startup success or failure instead of always
resolving; return true only after recording is enabled and false from catch or
early-return paths. In CliRecordRunner, inspect that result and call fail(...)
when startup returns false so CLI startup failures terminate rather than waiting
for completion.
In `@src/lib/exporter/voiceoverMix.ts`:
- Around line 53-73: Update the mix path around voiceoverNode and gainNode so
the default mix mode attenuates the original audio below unity before combining
it with the voiceover, preventing both sources from playing at full gain.
Preserve explicitly supplied originalGain values and replace-mode behavior.
- Around line 100-103: Materialize the video data lazily in the voiceover mixing
flow: avoid calling videoBlob.arrayBuffer() before renderMixedAudio when replace
mode does not use it. Update renderMixedAudio or its caller so the ArrayBuffer
is obtained only inside the mix branch, while preserving existing replace and
mix behavior.
---
Nitpick comments:
In `@electron/cli/args.test.ts`:
- Around line 4-8: Update path expectations in the tests around parse and the
projectPath assertions to use a platform-aware helper such as at, backed by
path.resolve(CWD, ...) or path.join(CWD, ...), instead of hard-coded /work/...
strings. Ensure all affected expected paths, including demo.openscreen, match
resolvePath on Windows and POSIX systems.
In `@electron/cli/cliMain.ts`:
- Around line 245-252: Update the console method overrides in the level loop so
argument serialization cannot throw before safeWrite, including for circular or
otherwise non-serializable objects. Preserve string arguments and the existing
space-separated output, while using a safe fallback representation for values
that JSON.stringify cannot handle.
In `@electron/preload.ts`:
- Around line 309-317: Update the cliProgress and cliDone methods in the preload
API to use the existing CliProgressEvent and CliDoneResult types from
electron-env.d.ts instead of unknown, while preserving their current IPC
behavior and cliLog signature.
In `@src/cli/CliExportRunner.tsx`:
- Around line 42-57: Update probeVideoDimensions to enforce a bounded timeout
for video metadata loading, rejecting the promise when neither onloadedmetadata
nor onerror fires before the deadline. Ensure successful metadata loads and
errors clear the timeout and perform the existing video cleanup so the CLI
cannot remain pending indefinitely.
In `@src/cli/CliRecordRunner.tsx`:
- Around line 122-126: Move the current-value assignments for toggleRecordingRef
and recordingRef out of the render body and into a React effect in the
surrounding component. Keep both refs initialized with their corresponding
values and update their .current fields after committed renders so the
stop/finish effects continue using the latest values.
- Around line 98-108: Add tests in the same package for CliRecordRunner’s
bootstrap and recording state machine, stubbing window.electronAPI as needed.
Cover recorder start, stop, and cliDone completion transitions, including the
requestReady bootstrap path.
In `@src/lib/exporter/voiceoverMix.ts`:
- Around line 105-135: The remux cleanup must cancel a started but unfinalized
Output when packet or audio processing fails. In the surrounding flow that
creates output and calls output.start(), track successful startup, catch
subsequent errors, await output.cancel() when startup completed, then rethrow;
retain input.dispose() in finally and avoid canceling if output.start() never
resolved.
🪄 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: d3dba1c5-2e28-4090-8d2f-2e52c6aea3d2
📒 Files selected for processing (16)
README.mddocs/cli.mdelectron/cli/args.test.tselectron/cli/args.tselectron/cli/cliMain.tselectron/electron-env.d.tselectron/main.tselectron/preload.tselectron/windows.tspackage.jsonsrc/App.tsxsrc/cli/CliExportRunner.tsxsrc/cli/CliRecordRunner.tsxsrc/hooks/useScreenRecorder.tssrc/lib/cliContracts.tssrc/lib/exporter/voiceoverMix.ts
| /** Starts recording with no countdown overlay. Used by the headless CLI runner. */ | ||
| startRecordingImmediately: () => Promise<void>; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
startRecordingImmediately never rejects, so CLI start failures hang the run.
startRecording catches everything internally (toast + state reset, Lines 1449-1465). The CLI runner awaits this in a try/catch (src/cli/CliRecordRunner.tsx Lines 190-196) expecting a throw; instead it resolves, recording stays false, recordingStartedAtRef stays null, the completion effect early-returns, and cli-done is never sent — the CLI blocks until the user kills it.
Return a success signal (or rethrow) from this entrypoint and have the runner fail on it.
🔧 Sketch
- /** Starts recording with no countdown overlay. Used by the headless CLI runner. */
- startRecordingImmediately: () => Promise<void>;
+ /** Starts recording with no countdown overlay. Resolves false when start failed. Used by the headless CLI runner. */
+ startRecordingImmediately: () => Promise<boolean>;Have startRecording return false from its catch/early-return paths and true once setRecording(true) has run, then in CliRecordRunner treat false as a failure and call fail(...).
Also applies to: 1733-1733
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useScreenRecorder.ts` around lines 57 - 58, Update
startRecordingImmediately and the underlying startRecording flow to report
startup success or failure instead of always resolving; return true only after
recording is enabled and false from catch or early-return paths. In
CliRecordRunner, inspect that result and call fail(...) when startup returns
false so CLI startup failures terminate rather than waiting for completion.
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
|
Addressed the CodeRabbit review in cc64b11 — all 8 actionable findings plus 6 of the nitpicks: Actionable
Nitpicks taken: metadata-probe 30s timeout, Re-verified after the changes: full unit suite (439 tests), repo lint, 🤖 Generated with Claude Code |
|
Pushed 6dcd778 with four more commands that close the remaining gaps between the CLI and GUI workflows (docs updated in
All verified on macOS; 443 unit tests (7 new), lint and tsc clean. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
electron/cli/cliMain.ts (1)
541-543: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCaptions rewrites the user's project in place — consider a temp-file + rename.
A crash or partial write during
writeProjectFileleaves the only copy of the project truncated. Writing to<path>.tmpandfs.rename-ing over the original makes the update atomic on the same filesystem.🤖 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 541 - 543, Update the captions success path around writeProjectFile to write result.projectData to a temporary file beside command.projectPath, then atomically rename the temporary file over the original on the same filesystem. Ensure cleanup of the temporary file on write or rename failure, while preserving the existing success condition and destination path.src/cli/CliSourcesRunner.tsx (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstant status strings held in
useStatein both CLI runners. Neither runner ever updatesstatus, so the state hook (and its unused setter) adds nothing over a plain constant.
src/cli/CliSourcesRunner.tsx#L57-L59: replaceconst [status] = useState("Enumerating sources…")with a plainconstand dropuseStatefrom the import.src/cli/CliCaptionsRunner.tsx#L138-L140: replaceconst [status] = useState("Generating captions…")with a plainconstand dropuseStatefrom the import.🤖 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/CliSourcesRunner.tsx` around lines 57 - 59, Replace the unused useState status tuple with a plain constant in CliSourcesRunner.tsx lines 57-59 and remove useState from that file’s imports; make the same change in CliCaptionsRunner.tsx lines 138-140 for the “Generating captions…” status and remove its useState import.
🤖 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 `@docs/cli.md`:
- Around line 77-86: Update the `openscreen sources --json` documentation
example to show valid parseable JSON, replacing the unquoted `displays`,
`windows`, and `microphones` placeholders with representative quoted keys and
JSON-compatible values.
- Around line 208-209: Update the step-4 heading in the CLI documentation to
accurately describe the selected --audio-mode replace behavior as replacing the
recording audio, or change the command to --audio-mode mix if the intended
behavior is to mix voiceover with existing audio; keep the heading and command
consistent.
In `@electron/cli/args.ts`:
- Around line 381-437: Add coverage in electron/cli/args.test.ts for the missing
rejection edge cases in parsePack and parseCaptions, including invalid or
incomplete --out/word-count arguments and extra positional arguments, while
preserving existing happy-path and unknown-option tests. Also add equivalent
edge-case tests for the sources parser using its visible parser entry point and
expected CLI errors.
---
Nitpick comments:
In `@electron/cli/cliMain.ts`:
- Around line 541-543: Update the captions success path around writeProjectFile
to write result.projectData to a temporary file beside command.projectPath, then
atomically rename the temporary file over the original on the same filesystem.
Ensure cleanup of the temporary file on write or rename failure, while
preserving the existing success condition and destination path.
In `@src/cli/CliSourcesRunner.tsx`:
- Around line 57-59: Replace the unused useState status tuple with a plain
constant in CliSourcesRunner.tsx lines 57-59 and remove useState from that
file’s imports; make the same change in CliCaptionsRunner.tsx lines 138-140 for
the “Generating captions…” status and remove its useState import.
🪄 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: 534a9821-c46d-4ef7-b317-7cc47c06faef
📒 Files selected for processing (11)
.gitignoredocs/cli.mdelectron/cli/args.test.tselectron/cli/args.tselectron/cli/cliMain.tselectron/ipc/handlers.tssrc/App.tsxsrc/cli/CliCaptionsRunner.tsxsrc/cli/CliExportRunner.tsxsrc/cli/CliSourcesRunner.tsxsrc/lib/cliContracts.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli/CliExportRunner.tsx
- electron/cli/args.test.ts
|
Addressed the second review round — all three findings:
Lint and the full unit suite pass. 🤖 Generated with Claude Code |
|
This one needs a bit more than a rebase. The model layer you build on survived —
The design holds — it's the plumbing that moved. Happy to sketch the mapping if that helps. |
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
Resolving the preload conflict kept both sides but missed the opening marker, so the file didn't parse. Both the stt block and the CLI bridge members are intended -- they collided only because each side appended to the same object.
|
Pushed the rebase to Two things I dropped as leftovers from the old What's left is six symbols:
The last row is the real work: the native exporters take an |
…eline
Completes the six-symbol port on top of contrib/pr-176-rebased:
- CliExportRunner now migrates the .openscreen project to an AxcutDocument
(migrateProjectDataToAxcutDocument + applyProbedDuration — without the
probe the clip has no duration and the export is a single frame) and
drives exportMultiNative/exportGifNative, mirroring the v4 ExportDialog:
same clip list, same scene JSON, same crop-aware smallest-clip sizing,
same gif dimension capping. Progress is synthesized from the native
frame-count push (export:native-progress) so the CLI's NDJSON contract
(percentage/currentFrame/totalFrames/ETA + done{outputPath,...}) is
unchanged. --preview-size becomes an accepted no-op (annotation geometry
is percentage-based in the scene); --audio now reads the natively
written file back, mixes the voiceover, and overwrites outPath.
- Auto-zoom repointed to @/lib/ai-edition/timeline/zoom-suggestions and
zoom-scale; suggestions append straight to document.zoomRanges.
- Deleted helpers vendored under src/cli/vendor/ with provenance notes:
captionSegmentsToAnnotationRegions (the CLI still writes caption
annotations into projects — the v2 route the migration preserves),
trimLeadingSilenceMono16k, clampZoomFocus, hasNativeCursorRecordingData.
- Cursor plumbing dropped from the runner: the native compositor discovers
the <video>.cursor.json sidecar by filename convention.
Known limitation carried over: the native pipeline has no cancel and no
extra-audio-track concept; kill mid-export leaves the worker running until
process exit, and --audio costs one extra read/write of the output.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J
- captions: the whisper.cpp stt:transcribe handler is registered in the GUI boot path, so CLI mode registered nothing and the renderer adapter failed with "No handler registered" — register registerSttIpc in runCli like set-locale. - docs/cli.md: one-shot dev build block (renderer, capture helpers, ffmpeg + Rust compositor, whisper STT server), --audio read-back behavior, --preview-size no-op, no-cancel caveat, whisper.cpp model auto-download. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ceTJjoPJYa5BdmXZdDc5J
|
@EtienneLescot Thank you for the rebase branch and the symbol map — that table saved hours. The branch is now ported on top of The six symbols:
The Also registered Verified on macOS (Apple Silicon): export MP4/GIF through the Rust compositor, One setup note that may bite other contributors: on an Apple Silicon machine with a Rosetta-installed rustup (default host #190 gets the same treatment next (multi-window mode re-added to the split 🤖 Generated with Claude Code |
36a2fff to
a297cee
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
electron/cli/cliMain.ts (2)
73-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe progress label says
Exportingfor every command.
createOutputis shared by the record, captions, and export runners. A captions run printsExporting 42%. Derive the label from the command kind, or use a neutral verb such asProgress.🤖 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 73 - 100, Update the progress text construction in createOutput’s progress handler so the label is not always “Exporting” for record and captions commands. Derive the verb from the command kind available to createOutput, or use the neutral “Progress” label, while preserving the existing percentage, frame, ETA, phase, TTY, and JSON output behavior.
442-621: 🩺 Stability & Availability | 🔵 TrivialConsider a global run timeout for non-record commands.
cli-done,did-fail-load, andrender-process-gonecover the known exits. If the renderer stalls without any of these events, the CLI process waits forever, which blocks scripted and CI callers. An optional--timeoutthat callsapp.exit(1)would bound the run.🤖 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 442 - 621, add an optional timeout for non-record CLI commands in the app.whenReady flow, starting it after the runner window is launched and clearing it when cli-done completes or an early renderer failure exits. When the timeout expires, report the failure through output.error and call app.exit(1); preserve unlimited waiting for record commands and avoid exiting after completion.electron/ipc/handlers.ts (1)
381-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fallback keeps the security guards intact.
path.basenamestrips traversal segments, and the sibling candidate stays inside the project directory, which is already intrustedDirs.approveReadableVideoPathstill enforces the extension and trusted-directory checks. The behavior on a missing sibling preserves the original error path.One optional cleanup:
resolveSourceinelectron/cli/cliMain.ts(Lines 234-247) implements the same sibling-lookup rule with a throwing tail. Extract one shared helper so the pack command and the loader cannot drift apart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/handlers.ts` around lines 381 - 418, Extract the shared sibling-lookup behavior from resolveWithSiblingFallback and resolveSource into one reusable helper, then update both callers to use it. Preserve basename sanitization, project-directory resolution, missing-sibling fallback to the original path, and each caller’s existing error behavior.
🤖 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.ts`:
- Around line 144-278: In parseExport, move the audio-format conflict validation
to after the request.outPath extension logic resolves request.format. Validate
the final request.format so --audio combined with a .gif output is rejected even
when --format is omitted, while preserving the existing format-extension
conflict checks.
In `@electron/cli/cliMain.ts`:
- Around line 252-267: Update copyIn to detect when the basename-derived
destination is already present in copied and generate a de-duplicated
destination with a suffix before copying. Ensure screen and webcam files with
identical basenames receive distinct project paths while preserving the existing
same-source skip behavior.
- Around line 370-380: Update runCli so the help and error output paths use
safeWrite instead of direct process.stdout.write/process.stderr.write calls,
ensuring broken-pipe handling is applied before app.exit. Preserve the existing
CLI_USAGE, error message formatting, exit codes, and returns.
- Around line 227-286: Update the media validation near the start of the flow to
guard with media != null and verify media.screenVideoPath directly, then assign
screenVideoPath from the narrowed media object. Keep the later
media.webcamVideoPath access and packedProject construction unchanged, relying
on the guard to establish media is defined.
---
Nitpick comments:
In `@electron/cli/cliMain.ts`:
- Around line 73-100: Update the progress text construction in createOutput’s
progress handler so the label is not always “Exporting” for record and captions
commands. Derive the verb from the command kind available to createOutput, or
use the neutral “Progress” label, while preserving the existing percentage,
frame, ETA, phase, TTY, and JSON output behavior.
- Around line 442-621: add an optional timeout for non-record CLI commands in
the app.whenReady flow, starting it after the runner window is launched and
clearing it when cli-done completes or an early renderer failure exits. When the
timeout expires, report the failure through output.error and call app.exit(1);
preserve unlimited waiting for record commands and avoid exiting after
completion.
In `@electron/ipc/handlers.ts`:
- Around line 381-418: Extract the shared sibling-lookup behavior from
resolveWithSiblingFallback and resolveSource into one reusable helper, then
update both callers to use it. Preserve basename sanitization, project-directory
resolution, missing-sibling fallback to the original path, and each caller’s
existing error behavior.
🪄 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: a14b94f5-594d-4149-94c2-5b36fd0c1ebe
📒 Files selected for processing (8)
README.mddocs/cli.mdelectron/cli/args.test.tselectron/cli/args.tselectron/cli/cliMain.tselectron/electron-env.d.tselectron/ipc/handlers.tselectron/main.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- electron/cli/args.test.ts
- docs/cli.md
- README.md
- electron/electron-env.d.ts
- electron/main.ts
| function parseExport(args: string[], cwd: string): CliCommand { | ||
| const request: CliExportRequest & { json?: boolean } = { | ||
| kind: "export", | ||
| projectPath: "", | ||
| outPath: null, | ||
| format: null, | ||
| quality: null, | ||
| gifFrameRate: null, | ||
| gifSizePreset: null, | ||
| previewWidth: null, | ||
| previewHeight: null, | ||
| autoZoom: false, | ||
| audioPath: null, | ||
| audioMode: "mix", | ||
| audioOffsetSec: 0, | ||
| }; | ||
|
|
||
| for (let i = 0; i < args.length; i++) { | ||
| const arg = args[i]; | ||
| switch (arg) { | ||
| case "-o": | ||
| case "--out": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| request.outPath = resolvePath(value, cwd); | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--format": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| if (value !== "mp4" && value !== "gif") { | ||
| throw new Error(`--format must be mp4 or gif, got "${value}"`); | ||
| } | ||
| request.format = value; | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--quality": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| if (value !== "medium" && value !== "good" && value !== "source") { | ||
| throw new Error(`--quality must be medium, good or source, got "${value}"`); | ||
| } | ||
| request.quality = value; | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--gif-fps": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| const fps = Number(value); | ||
| if (fps !== 15 && fps !== 20 && fps !== 25 && fps !== 30) { | ||
| throw new Error(`--gif-fps must be 15, 20, 25 or 30, got "${value}"`); | ||
| } | ||
| request.gifFrameRate = fps; | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--gif-size": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| if (value !== "medium" && value !== "large" && value !== "original") { | ||
| throw new Error(`--gif-size must be medium, large or original, got "${value}"`); | ||
| } | ||
| request.gifSizePreset = value; | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--preview-size": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| const match = /^(\d+)x(\d+)$/.exec(value); | ||
| if (!match) throw new Error(`--preview-size must look like 1280x720, got "${value}"`); | ||
| const previewWidth = Number(match[1]); | ||
| const previewHeight = Number(match[2]); | ||
| if (previewWidth <= 0 || previewHeight <= 0) { | ||
| throw new Error(`--preview-size dimensions must be positive, got "${value}"`); | ||
| } | ||
| request.previewWidth = previewWidth; | ||
| request.previewHeight = previewHeight; | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--auto-zoom": | ||
| request.autoZoom = true; | ||
| break; | ||
| case "--audio": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| request.audioPath = resolvePath(value, cwd); | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--audio-mode": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| if (value !== "mix" && value !== "replace") { | ||
| throw new Error(`--audio-mode must be mix or replace, got "${value}"`); | ||
| } | ||
| request.audioMode = value; | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--audio-offset": { | ||
| const [value, next] = takeValue(args, i, arg); | ||
| const seconds = Number(value); | ||
| if (!Number.isFinite(seconds) || seconds < 0) { | ||
| throw new Error( | ||
| `--audio-offset must be a non-negative number of seconds, got "${value}"`, | ||
| ); | ||
| } | ||
| request.audioOffsetSec = seconds; | ||
| i = next; | ||
| break; | ||
| } | ||
| case "--json": | ||
| request.json = true; | ||
| break; | ||
| default: | ||
| if (arg.startsWith("-")) throw new Error(`Unknown export option: ${arg}`); | ||
| if (request.projectPath) throw new Error(`Unexpected extra argument: ${arg}`); | ||
| request.projectPath = resolvePath(arg, cwd); | ||
| } | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
| return request; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the --audio/GIF conflict check order in parseExport.
The audio-format conflict check at Line 263 runs before format is inferred from --out's extension at Line 271-275. When the user passes --audio with --out <file>.gif and no explicit --format, request.format is still null at the time of the check, so the conflict passes silently. request.format is set to "gif" afterward, so the returned request has both audioPath set and format === "gif", violating the stated rule that audio is MP4-only.
Move the audio/gif check after format resolution, using the final resolved request.format.
🐛 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;The --preview-size zero-dimension rejection from the earlier review is correctly in place at Line 214-216.
📝 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.
| function parseExport(args: string[], cwd: string): CliCommand { | |
| const request: CliExportRequest & { json?: boolean } = { | |
| kind: "export", | |
| projectPath: "", | |
| outPath: null, | |
| format: null, | |
| quality: null, | |
| gifFrameRate: null, | |
| gifSizePreset: null, | |
| previewWidth: null, | |
| previewHeight: null, | |
| autoZoom: false, | |
| audioPath: null, | |
| audioMode: "mix", | |
| audioOffsetSec: 0, | |
| }; | |
| for (let i = 0; i < args.length; i++) { | |
| const arg = args[i]; | |
| switch (arg) { | |
| case "-o": | |
| case "--out": { | |
| const [value, next] = takeValue(args, i, arg); | |
| request.outPath = resolvePath(value, cwd); | |
| i = next; | |
| break; | |
| } | |
| case "--format": { | |
| const [value, next] = takeValue(args, i, arg); | |
| if (value !== "mp4" && value !== "gif") { | |
| throw new Error(`--format must be mp4 or gif, got "${value}"`); | |
| } | |
| request.format = value; | |
| i = next; | |
| break; | |
| } | |
| case "--quality": { | |
| const [value, next] = takeValue(args, i, arg); | |
| if (value !== "medium" && value !== "good" && value !== "source") { | |
| throw new Error(`--quality must be medium, good or source, got "${value}"`); | |
| } | |
| request.quality = value; | |
| i = next; | |
| break; | |
| } | |
| case "--gif-fps": { | |
| const [value, next] = takeValue(args, i, arg); | |
| const fps = Number(value); | |
| if (fps !== 15 && fps !== 20 && fps !== 25 && fps !== 30) { | |
| throw new Error(`--gif-fps must be 15, 20, 25 or 30, got "${value}"`); | |
| } | |
| request.gifFrameRate = fps; | |
| i = next; | |
| break; | |
| } | |
| case "--gif-size": { | |
| const [value, next] = takeValue(args, i, arg); | |
| if (value !== "medium" && value !== "large" && value !== "original") { | |
| throw new Error(`--gif-size must be medium, large or original, got "${value}"`); | |
| } | |
| request.gifSizePreset = value; | |
| i = next; | |
| break; | |
| } | |
| case "--preview-size": { | |
| const [value, next] = takeValue(args, i, arg); | |
| const match = /^(\d+)x(\d+)$/.exec(value); | |
| if (!match) throw new Error(`--preview-size must look like 1280x720, got "${value}"`); | |
| const previewWidth = Number(match[1]); | |
| const previewHeight = Number(match[2]); | |
| if (previewWidth <= 0 || previewHeight <= 0) { | |
| throw new Error(`--preview-size dimensions must be positive, got "${value}"`); | |
| } | |
| request.previewWidth = previewWidth; | |
| request.previewHeight = previewHeight; | |
| i = next; | |
| break; | |
| } | |
| case "--auto-zoom": | |
| request.autoZoom = true; | |
| break; | |
| case "--audio": { | |
| const [value, next] = takeValue(args, i, arg); | |
| request.audioPath = resolvePath(value, cwd); | |
| i = next; | |
| break; | |
| } | |
| case "--audio-mode": { | |
| const [value, next] = takeValue(args, i, arg); | |
| if (value !== "mix" && value !== "replace") { | |
| throw new Error(`--audio-mode must be mix or replace, got "${value}"`); | |
| } | |
| request.audioMode = value; | |
| i = next; | |
| break; | |
| } | |
| case "--audio-offset": { | |
| const [value, next] = takeValue(args, i, arg); | |
| const seconds = Number(value); | |
| if (!Number.isFinite(seconds) || seconds < 0) { | |
| throw new Error( | |
| `--audio-offset must be a non-negative number of seconds, got "${value}"`, | |
| ); | |
| } | |
| request.audioOffsetSec = seconds; | |
| i = next; | |
| break; | |
| } | |
| case "--json": | |
| request.json = true; | |
| break; | |
| default: | |
| if (arg.startsWith("-")) throw new Error(`Unknown export option: ${arg}`); | |
| if (request.projectPath) throw new Error(`Unexpected extra argument: ${arg}`); | |
| request.projectPath = resolvePath(arg, cwd); | |
| } | |
| } | |
| 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; | |
| } | |
| return request; | |
| } | |
| function parseExport(args: string[], cwd: string): CliCommand { | |
| const request: CliExportRequest & { json?: boolean } = { | |
| kind: "export", | |
| projectPath: "", | |
| outPath: null, | |
| format: null, | |
| quality: null, | |
| gifFrameRate: null, | |
| gifSizePreset: null, | |
| previewWidth: null, | |
| previewHeight: null, | |
| autoZoom: false, | |
| audioPath: null, | |
| audioMode: "mix", | |
| audioOffsetSec: 0, | |
| }; | |
| for (let i = 0; i < args.length; i++) { | |
| const arg = args[i]; | |
| switch (arg) { | |
| case "-o": | |
| case "--out": { | |
| const [value, next] = takeValue(args, i, arg); | |
| request.outPath = resolvePath(value, cwd); | |
| i = next; | |
| break; | |
| } | |
| case "--format": { | |
| const [value, next] = takeValue(args, i, arg); | |
| if (value !== "mp4" && value !== "gif") { | |
| throw new Error(`--format must be mp4 or gif, got "${value}"`); | |
| } | |
| request.format = value; | |
| i = next; | |
| break; | |
| } | |
| case "--quality": { | |
| const [value, next] = takeValue(args, i, arg); | |
| if (value !== "medium" && value !== "good" && value !== "source") { | |
| throw new Error(`--quality must be medium, good or source, got "${value}"`); | |
| } | |
| request.quality = value; | |
| i = next; | |
| break; | |
| } | |
| case "--gif-fps": { | |
| const [value, next] = takeValue(args, i, arg); | |
| const fps = Number(value); | |
| if (fps !== 15 && fps !== 20 && fps !== 25 && fps !== 30) { | |
| throw new Error(`--gif-fps must be 15, 20, 25 or 30, got "${value}"`); | |
| } | |
| request.gifFrameRate = fps; | |
| i = next; | |
| break; | |
| } | |
| case "--gif-size": { | |
| const [value, next] = takeValue(args, i, arg); | |
| if (value !== "medium" && value !== "large" && value !== "original") { | |
| throw new Error(`--gif-size must be medium, large or original, got "${value}"`); | |
| } | |
| request.gifSizePreset = value; | |
| i = next; | |
| break; | |
| } | |
| case "--preview-size": { | |
| const [value, next] = takeValue(args, i, arg); | |
| const match = /^(\d+)x(\d+)$/.exec(value); | |
| if (!match) throw new Error(`--preview-size must look like 1280x720, got "${value}"`); | |
| const previewWidth = Number(match[1]); | |
| const previewHeight = Number(match[2]); | |
| if (previewWidth <= 0 || previewHeight <= 0) { | |
| throw new Error(`--preview-size dimensions must be positive, got "${value}"`); | |
| } | |
| request.previewWidth = previewWidth; | |
| request.previewHeight = previewHeight; | |
| i = next; | |
| break; | |
| } | |
| case "--auto-zoom": | |
| request.autoZoom = true; | |
| break; | |
| case "--audio": { | |
| const [value, next] = takeValue(args, i, arg); | |
| request.audioPath = resolvePath(value, cwd); | |
| i = next; | |
| break; | |
| } | |
| case "--audio-mode": { | |
| const [value, next] = takeValue(args, i, arg); | |
| if (value !== "mix" && value !== "replace") { | |
| throw new Error(`--audio-mode must be mix or replace, got "${value}"`); | |
| } | |
| request.audioMode = value; | |
| i = next; | |
| break; | |
| } | |
| case "--audio-offset": { | |
| const [value, next] = takeValue(args, i, arg); | |
| const seconds = Number(value); | |
| if (!Number.isFinite(seconds) || seconds < 0) { | |
| throw new Error( | |
| `--audio-offset must be a non-negative number of seconds, got "${value}"`, | |
| ); | |
| } | |
| request.audioOffsetSec = seconds; | |
| i = next; | |
| break; | |
| } | |
| case "--json": | |
| request.json = true; | |
| break; | |
| default: | |
| if (arg.startsWith("-")) throw new Error(`Unknown export option: ${arg}`); | |
| if (request.projectPath) throw new Error(`Unexpected extra argument: ${arg}`); | |
| request.projectPath = resolvePath(arg, cwd); | |
| } | |
| } | |
| 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"); | |
| } | |
| return request; | |
| } |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 210-210: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 144 - 278, In parseExport, move the
audio-format conflict validation to after the request.outPath extension logic
resolves request.format. Validate the final request.format so --audio combined
with a .gif output is rejected even when --format is omitted, while preserving
the existing format-extension conflict checks.
| const media = data.media ?? (data.videoPath ? { screenVideoPath: data.videoPath } : undefined); | ||
| const screenVideoPath = media?.screenVideoPath; | ||
| if (!screenVideoPath) { | ||
| throw new Error("Project file does not reference a screen video"); | ||
| } | ||
|
|
||
| const projectDir = path.dirname(path.resolve(projectPath)); | ||
| const resolveSource = async (mediaPath: string): Promise<string> => { | ||
| const exists = await fs | ||
| .stat(mediaPath) | ||
| .then((stats) => stats.isFile()) | ||
| .catch(() => false); | ||
| if (exists) return mediaPath; | ||
| const sibling = path.join(projectDir, path.basename(mediaPath)); | ||
| const siblingExists = await fs | ||
| .stat(sibling) | ||
| .then((stats) => stats.isFile()) | ||
| .catch(() => false); | ||
| if (siblingExists) return sibling; | ||
| throw new Error(`Referenced media not found: ${mediaPath}`); | ||
| }; | ||
|
|
||
| await fs.mkdir(outDir, { recursive: true }); | ||
|
|
||
| const copied: string[] = []; | ||
| const copyIn = async (sourcePath: string): Promise<string> => { | ||
| const destination = path.join(outDir, path.basename(sourcePath)); | ||
| if (path.resolve(sourcePath) !== path.resolve(destination)) { | ||
| await fs.copyFile(sourcePath, destination); | ||
| } | ||
| copied.push(destination); | ||
| return destination; | ||
| }; | ||
|
|
||
| const screenSource = await resolveSource(screenVideoPath); | ||
| const newScreenPath = await copyIn(screenSource); | ||
|
|
||
| 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 } : {}), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'tsconfig*.json' --exec sh -c 'echo "== {} =="; cat {}'
rg -n '"typecheck"|tsc ' package.jsonRepository: getopenscreen/openscreen
Length of output: 1878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '200,295p' electron/cli/cliMain.ts
echo
echo "== package manager and TypeScript availability =="
ls -la package.json tsconfig.json
node - <<'JS'
const { version } = process;
console.log("node", version);
JS
npx -y tsc@latest --versionRepository: getopenscreen/openscreen
Length of output: 4401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect nearby declarations and any inferred usages without modifying repo files.
python3 - <<'PY'
from pathlib import Path
p=Path("electron/cli/cliMain.ts")
text=p.read_text()
for name in ["function packProject", "const media", "type PackedProjectData", "interface ProjectData"]:
i=text.find(name)
print(f"\n== occurrences of {name!r} ==")
if i!=-1:
j=text.rfind("\n",0,max(0,i-8000))
j=max(0,j)
k=text.find("\n",i+3000)
print(text[j:k].replace("\t", " ")[:5000])
PYRepository: getopenscreen/openscreen
Length of output: 5337
🌐 Web query:
TypeScript 5.0 narrowing guard same variable condition optional chaining does not narrow enclosing variable
💡 Result:
TypeScript's type narrowing behavior with optional chaining is intentional and restricted by design [1][2]. The compiler does not automatically narrow an enclosing variable based on a truthiness check of an optional chain (e.g., if (obj?.prop) { ... }) because optional chaining is primarily designed for safe property access rather than as a mechanism for control flow analysis that propagates narrowing to the root variable [2][3]. Key reasons for this behavior include: 1. Control Flow Limitations: TypeScript's control flow analysis does not generally treat optional chaining as a statement that proves the existence or non-nullish state of the root variable [3]. While humans can infer that obj?.prop being truthy implies obj is not nullish, the type checker does not perform this propagation to avoid soundness issues with JavaScript's non-exact types [2][4][5]. 2. Indirect Reference Restrictions: Narrowing through indirect references (such as property access) is subject to strict rules [1][6]. For narrowing to occur, the expression being checked often needs to be a const variable (or a parameter/readonly property) that has not been explicitly annotated in a way that hides its inferred type [1][6]. 3. Soundness Concerns: Because TypeScript types are not "exact" (objects can have properties not defined in their interface), assuming that a truthy property access guarantees the entire object's state can lead to runtime crashes if other parts of the code modify the object [4]. Consequently, the TypeScript team has declined requests to make optional chaining a general-purpose narrowing tool, noting it would exacerbate existing risks regarding unsoundness [2][5]. If you need to narrow an enclosing variable, the recommended approach is to check the variable directly before accessing its properties: if (obj!= null && obj.prop) { // obj is narrowed here console.log(obj.prop); } Alternatively, assigning the result of the optional chain to a variable and performing the check on that variable (if possible) or performing a direct check on the root object is required for the compiler to safely narrow the type [1][7].
Citations:
- 1: Adding explicit type that is the same as the inferred type prevents narrowing later on microsoft/TypeScript#51757
- 2: Optional Chaining and Type Narrowing microsoft/TypeScript#33736
- 3: Optional chaining, proper infer in type guard microsoft/TypeScript#34974
- 4: Optional chaining should narrow union types too microsoft/TypeScript#56264
- 5: Implement type narrowing for optional chaining operator on unions microsoft/TypeScript#56440
- 6: Invalid type inference for optional chaining operator when nullish check is extracted to
constvaraiable microsoft/TypeScript#58632 - 7: Optional Chaining and Type Narrowing microsoft/TypeScript#48748
Check media directly so later uses narrow.
if (!media?.screenVideoPath) narrows the optional property, but not media. Lines 265 and 282 still consume media as … | undefined, which conflicts with TypeScript strict mode. Add a direct media != null guard and then declare screenVideoPath from media.screenVideoPath.
🤖 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 227 - 286, Update the media validation
near the start of the flow to guard with media != null and verify
media.screenVideoPath directly, then assign screenVideoPath from the narrowed
media object. Keep the later media.webcamVideoPath access and packedProject
construction unchanged, relying on the guard to establish media is defined.
Source: Coding guidelines
| const copyIn = async (sourcePath: string): Promise<string> => { | ||
| const destination = path.join(outDir, path.basename(sourcePath)); | ||
| if (path.resolve(sourcePath) !== path.resolve(destination)) { | ||
| await fs.copyFile(sourcePath, destination); | ||
| } | ||
| copied.push(destination); | ||
| return destination; | ||
| }; | ||
|
|
||
| const screenSource = await resolveSource(screenVideoPath); | ||
| const newScreenPath = await copyIn(screenSource); | ||
|
|
||
| let newWebcamPath: string | undefined; | ||
| if (media.webcamVideoPath) { | ||
| newWebcamPath = await copyIn(await resolveSource(media.webcamVideoPath)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Same basename means the webcam copy overwrites the screen copy.
copyIn derives the destination only from path.basename(sourcePath). If the screen video and the webcam video have the same basename in different directories, the second copyFile overwrites the first, and both project paths point to one file. Add a de-duplicating suffix when the destination already exists in copied.
🤖 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 252 - 267, Update copyIn to detect when
the basename-derived destination is already present in copied and generate a
de-duplicated destination with a suffix before copying. Ensure screen and webcam
files with identical basenames receive distinct project paths while preserving
the existing same-source skip behavior.
| export function runCli(command: CliCommand): void { | ||
| if (command.kind === "help") { | ||
| process.stdout.write(CLI_USAGE); | ||
| app.exit(0); | ||
| return; | ||
| } | ||
| if (command.kind === "error") { | ||
| process.stderr.write(`Error: ${command.message}\n\n${CLI_USAGE}`); | ||
| app.exit(2); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
These two writes still bypass safeWrite.
openscreen help | head closes the pipe before the process.stdout.on("error") guards are installed on Line 407. The raw write calls then throw. This was reported in an earlier review and remains in the current code.
🛡️ Proposed fix
if (command.kind === "help") {
- process.stdout.write(CLI_USAGE);
+ safeWrite(process.stdout, CLI_USAGE);
app.exit(0);
return;
}
if (command.kind === "error") {
- process.stderr.write(`Error: ${command.message}\n\n${CLI_USAGE}`);
+ safeWrite(process.stderr, `Error: ${command.message}\n\n${CLI_USAGE}`);
app.exit(2);
return;
}🤖 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 370 - 380, Update runCli so the help
and error output paths use safeWrite instead of direct
process.stdout.write/process.stderr.write calls, ensuring broken-pipe handling
is applied before app.exit. Preserve the existing CLI_USAGE, error message
formatting, exit codes, and returns.
2 participants
Summary
This PR adds a headless CLI to OpenScreen so scripts, CI pipelines, and AI coding agents can produce polished demos end-to-end — record, edit the project JSON programmatically, render — with no visible windows and machine-readable output:
Full docs in
docs/cli.md(commands, NDJSON protocol, exit codes, an end-to-end agent example).Why
.openscreenprojects are plain JSON and the export pipeline is already proven headless by theHEADLESS=truee2e test — the missing piece was a way to drive both without the GUI. With this CLI, an AI coding agent can demo the product it just built: record it, zoom into the interesting parts, narrate with TTS, and ship an MP4.Design — deliberately minimal footprint
The CLI reuses the existing pipelines unchanged via the hidden-window pattern your
tests/e2e/gif-export.spec.tsalready established:electron/cli/args.ts— pure argv parser (14 unit tests). Skips leading Chromium switches soOpenscreen.AppImage --no-sandbox export …works.electron/cli/cliMain.ts— headless boot: no HUD/tray/menu/dock, reusesregisterIpcHandlerswith inert window callbacks (the big handler module is untouched), NDJSON/--jsonprotocol on stdout, diagnostics on stderr, SIGINT/SIGTERM/stdinstop, EPIPE-safe writes, exit codes 0/1/2, SwiftShader fallback for GPU-less environments.src/cli/CliExportRunner.tsx— loads the project via the existingload-project-file-from-pathbridge action, rebuilds the same exporter config the editor's dialog builds, runsVideoExporter/GifExporter. DOM-derived preview dimensions are replaced by a deterministic 1280×720 reference box (--preview-sizeto override).src/cli/CliRecordRunner.tsx— drivesuseScreenRecorder(source by display index or window-title substring, mic by device label, system audio, cursor modes,--duration), then optionally writes a ready-to-export project file.--audiovoiceover mixing (src/lib/exporter/voiceoverMix.ts): copies the exported video packets without re-encoding (mediabunnyInput/EncodedPacketSink→EncodedVideoPacketSource) and renders the audio offline withOfflineAudioContext(mixorreplace,--audio-offset).Only three GUI-side touches: the
main.tsCLI branch, new preload IPC methods, and exposingstartRecordingImmediately()(countdown-less start) fromuseScreenRecorder.Tested (macOS 26.6, Apple Silicon)
record --duration 4/5→ MP4 +.cursor.json+ session manifest; stop via SIGINT and via--duration;--projectoutput loads in the GUI editorexportMP4 (zooms, annotations, padding, multi-track audio fixture) and GIF (magic bytes + dimensions verified); output verified with ffprobe--audiowith a realsay-generated TTS track (volumedetect confirms audible narration)openscreen export | head(EPIPE) no longer crashes or shows Electron's error dialog; renderer console errors are forwarded to stderrnpm run lintclean,tscclean, all 441 vitest unit tests pass (14 new), GUI mode boots unaffectedPlatform caveats (honest disclosure)
Windows and Linux paths were statically reviewed, not device-tested (I only have macOS hardware). The review-driven fixes are included: leading-switch subcommand detection, stdin guard moved inside try (Windows GUI-subsystem stdio edge case), and platform notes in
docs/cli.md(Windows: no SIGTERM — use stdinstop/--duration; Linux Wayland: PipeWire portal may prompt and headless sessions can't record). Happy to iterate if maintainers can test on those platforms.🤖 Generated with Claude Code
Summary by CodeRabbit