feat: add CLI and agent interface for OpenScreen#350
Open
lupuletic wants to merge 6 commits intosiddharthvaddem:mainfrom
Open
feat: add CLI and agent interface for OpenScreen#350lupuletic wants to merge 6 commits intosiddharthvaddem:mainfrom
lupuletic wants to merge 6 commits intosiddharthvaddem:mainfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a full Node.js CLI, MCP server, shared TypeScript modules, Electron headless export bridge, new CLI commands for project/region/export management, renderer export runner, and extensive agent-facing documentation and rules. Many UI modules were converted to re-export shared logic. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f370510f11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lupuletic
pushed a commit
to lupuletic/openscreen
that referenced
this pull request
Apr 6, 2026
- Suppress human-readable text in --json render mode (P1) - Materialize media from legacy videoPath in loadProject (P1) - Validate GIF frame rate and size preset at CLI layer (P2) - Use Electron's actual productName for userData path lookup (P2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 31
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/skills/rules/project-setup.md`:
- Around line 7-28: The fenced code block containing the example "openscreen
--json project create ..." needs blank lines immediately before and after the
triple-backtick block to satisfy markdown lint rules (same fix as in
troubleshooting.md); edit the markdown around that fenced code block so there is
an empty line above the opening ``` and an empty line below the closing ```,
leaving the code unchanged.
In `@cli/skills/rules/troubleshooting.md`:
- Around line 27-28: Update the troubleshooting note about render commands to
not reference unimplemented commands directly: either remove the "still" and
"frames" command names from the sentence that lists Phase 3 features or mark
them explicitly as "planned / coming soon" so callers don't attempt to use them;
specifically edit the line mentioning "render, gif, still, and frames" (and the
Phase 3 note) to either list only implemented commands ("render" and "gif") or
append "(planned)" after "still" and "frames" so the doc accurately reflects
which CLI commands are available.
- Around line 5-9: The markdown headings in troubleshooting.md (e.g., the
"Project file not found" and "Invalid JSON in project file" sections) have
fenced code blocks immediately adjacent to headings; add a blank line between
each heading and its following ``` fenced code block and ensure there's a blank
line after the closing ``` as well, and apply the same blank-line pattern to the
other sections (lines 11-28) that have code blocks immediately following
headings so all headings and code blocks are separated by blank lines for proper
linting and rendering.
In `@cli/src/commands/annotate.ts`:
- Around line 17-23: The option parsing currently uses parseInt/parseFloat for
flags like "--start", "--end", "--position-x", "--position-y", "--width",
"--height", and "--font-size" which yields NaN for invalid input and does not
enforce ranges; replace or augment these with validation: implement custom
option parsers or a post-parse validation step that checks each parsed value is
a finite number (not NaN) and that position-x, position-y, width and height are
within 0–100, and font-size and start/end are positive integers (or start <
end); when validation fails, throw a clear error (or call
program.error/console.error and exit) so invalid inputs are rejected rather than
silently becoming NaN.
In `@cli/src/commands/project.ts`:
- Around line 86-92: The current boolean parsing silently coerces invalid values
to false via parseBool (used for flags like "--show-blur", "--gif-loop" and
others), so update the CLI to validate boolean literals strictly: implement or
use a strict parser (e.g., parseBoolStrict) that only accepts "true"/"false"
(and optionally "1"/"0", "yes"/"no") in a case-insensitive way and throws an
error on any other input; replace parseBool with this strict parser for the
options shown (including the other occurrences around lines 116-118) so invalid
values cause the command to fail fast and report the bad argument.
In `@cli/src/commands/render.ts`:
- Line 9: The CLI currently accepts invalid values for options like "--quality
<q>", "--frame-rate <n>" and "--size-preset <p>" and only fails later in the
renderer; update the option parsing in render.ts to validate inputs immediately
by restricting "--quality" to allowed choices ("medium","good","source"),
validating and converting "--frame-rate" via Number.parseInt and rejecting NaN
with a clear error, and validating "--size-preset" against the known preset
names; implement this using commander option choice/arg parsers or an explicit
validator right after parsing (referencing the ".option(\"--quality <q>\")",
".option(\"--frame-rate\"", and ".option(\"--size-preset\"" invocations) and
exit or throw a user-facing error when validation fails so invalid values never
reach the renderer.
- Line 31: The CLI option currently forces opts.loop to true by passing a
default (".option(\"--loop\", \"Loop GIF\", true)"); remove that third argument
so the option is declared as ".option(\"--loop\", \"Loop GIF\")" (so opts.loop
is undefined unless the flag is provided) and, if you want explicit disabling,
add the complementary "--no-loop" inverse option via Commander; this ensures the
gifLoop passed from render.ts (gifLoop: opts.loop) can be undefined and lets the
fallback logic in CliExportRenderer.tsx (const loop = gifLoop ?? editor.gifLoop
?? true) respect saved settings.
In `@cli/src/commands/shortcuts.ts`:
- Around line 38-48: loadShortcuts currently swallows all errors and returns
DEFAULT_SHORTCUTS even when shortcuts.json exists but is unreadable or
malformed; change loadShortcuts (and use
getShortcutsPath/mergeWithDefaults/DEFAULT_SHORTCUTS) so it first checks
fs.existsSync(filePath) and: if the file does not exist return
{...DEFAULT_SHORTCUTS}; if it does exist attempt to read and JSON.parse it and
on any read/parse error surface the error (rethrow or return a wrapped Error
with context) instead of falling back to defaults; keep mergeWithDefaults(raw)
for successful parses so only a truly missing file is treated as defaults.
In `@cli/src/commands/speed.ts`:
- Around line 11-16: The --start/--end/--speed options use parseInt/parseFloat
which can return NaN; update the parsing to validate values (either via
commander’s option validation callback or immediately after parsing) and produce
a clear error message and exit if NaN is encountered for start/end/speed;
additionally enforce the allowed speed set (0.25,0.5,0.75,1.25,1.5,1.75,2)
before calling addSpeedRegion so the CLI fails fast with a helpful message
rather than letting invalid numbers propagate into addSpeedRegion or
region-manager.
In `@cli/src/commands/trim.ts`:
- Around line 11-12: The requiredOption parsers for "--start" and "--end" in
trim.ts use raw parseInt which converts invalid inputs to NaN silently; replace
parseInt with a validating parser (same pattern used in speed.ts) that converts
the value to a number, checks Number.isNaN, and throws a descriptive Error
(e.g., "Invalid --start value: must be a number") so the CLI fails fast; update
the .requiredOption calls for "--start <ms>" and "--end <ms>" to use this
validator and ensure returned values are numeric milliseconds.
In `@cli/src/commands/zoom.ts`:
- Around line 11-16: The options parsing currently uses parseInt/parseFloat
directly for "--start", "--end", "--depth", "--focus-x", and "--focus-y" which
can silently produce NaN and "--depth <1-6>" doesn't enforce range; update the
CLI to validate parsed values immediately after parsing (or provide custom
parser functions) so that start/end are valid integers (use parseInt(value, 10)
and check Number.isInteger and !Number.isNaN), depth is an integer within 1–6,
and focus-x/focus-y are numbers between 0 and 1 (use Number.isFinite and range
checks); on invalid input throw a clear error or call program.error/exit with a
descriptive message mentioning the offending option (e.g., "--start", "--end",
"--depth", "--focus-x", "--focus-y", "--focus-mode") so users get immediate
feedback instead of NaN values.
In `@cli/src/core/electron-bridge.ts`:
- Around line 179-188: The catch in runExport currently logs errors via
outputError but swallows them so callers think the export succeeded; modify
runExport's catch to re-throw the original error (or throw a new Error with the
original as cause) after calling outputError so the Promise rejects for callers,
while keeping the finally block that cleans up configPath (fs.unlinkSync)
intact; reference the runExport function, the catch that calls outputError, and
the cleanup in the finally block when making the change.
- Around line 159-165: The stderr handler in the child process callback (the
child.stderr?.on("data", ...) block) currently checks for "ERROR" or "Error" and
misses other casing; change the detection to a case-insensitive test (e.g.,
normalize the chunk string with toLowerCase() and check includes("error") or use
a case-insensitive regex like /error/i) before setting lastError, keeping the
existing trimming and assignment behavior in the same callback.
- Around line 59-68: When the check for an existing file finds absOutput and
overwrite is false, calling outputError(...) and returning silently doesn't
signal failure to the caller; change that branch in the function containing
absOutput to throw a descriptive Error (e.g. new Error(`Output exists:
${absOutput}`)) immediately after calling outputError so the caller/outer catch
can detect and handle the failure instead of continuing normally; keep the
outputError call for user feedback and ensure the thrown Error propagates.
In `@cli/src/core/project-manager.ts`:
- Around line 1-19: The review notes the long relative imports (e.g.,
"../../../src/shared/project-schema", "../../../src/shared/aspect-ratios",
"../../../src/shared/export-types") in cli/src/core/project-manager.ts; to
address this, add and configure path aliases in the CLI tsconfig (or root
monorepo tsconfig) and update the import statements in ProjectManager to use
those aliases (e.g., "@shared/..." or similar) for symbols like
createProjectData, normalizeProjectEditor, PROJECT_VERSION, AspectRatio,
ExportFormat, ExportQuality, GifFrameRate, GifSizePreset, ProjectMedia and
validateProjectData to shorten and stabilize module resolution.
- Around line 79-87: The current saveProject function uses an atomic write by
creating tmpPath and then calling fs.renameSync(tmpPath, absPath) but never
cleans up tmpPath if renameSync throws; wrap the write/rename in a try/finally
around the renameSync to ensure the temporary file is removed on error.
Specifically, keep the write to tmpPath (fs.writeFileSync) and then perform
fs.renameSync inside a try block, and in the finally block check for existence
of tmpPath and remove it (fs.unlinkSync) if it still exists and rename did not
succeed; reference saveProject, tmpPath, absPath, writeFileSync, renameSync when
making the change.
In `@cli/src/core/region-manager.ts`:
- Around line 175-184: The style object construction in region-manager.ts is
verbose due to repeated conditional spreads; extract and use a small helper
(e.g., pickDefined) to filter out undefined properties and then merge with
DEFAULT_ANNOTATION_STYLE when building style (reference: the const style,
DEFAULT_ANNOTATION_STYLE, and opts properties like
fontSize/color/bgColor/fontFamily/fontWeight/fontStyle/textAlign); implement
pickDefined in a shared utils file and replace the repeated ...(opts.xxx !==
undefined ? { xxx: opts.xxx } : {}) patterns with a single ...pickDefined({...})
spread to keep intent clear and reduce boilerplate.
- Line 212: The current zIndex assignment uses
project.editor.annotationRegions.length + 1 which can produce duplicates after
removals; change the logic in the function that creates/pushes new regions (look
for the object with zIndex: project.editor.annotationRegions.length + 1) to
compute a unique zIndex instead—either maintain a persistent counter on the
manager (e.g., this.nextZIndex) that you increment when adding, or compute
Math.max(...annotationRegions.map(r => r.zIndex), 0) + 1 to derive the next
value; ensure the counter is initialized and updated on add (and not decremented
on remove) so newly added regions always get a higher, non-duplicating zIndex.
In `@cli/src/index.ts`:
- Around line 39-42: The stillCommand and framesCommand are being registered in
cli/src/index.ts even though their handlers in render.ts (stillCommand,
framesCommand) currently throw "not yet implemented"; remove or gate their
registration so they don't appear in --help until implemented by either (a)
deleting or commenting out the program.addCommand(stillCommand) and
program.addCommand(framesCommand) calls, or (b) wrapping those calls in a
feature-flag/condition (e.g., if (enableStillAndFrames)) and ensure the flag is
false by default; keep renderCommand and gifCommand registration unchanged.
- Around line 19-24: Parser-layer errors are emitted before the preAction hook
runs so setOutputMode({ json, quiet }) is too late; update the wiring so
JSON/quiet mode is set or respected at the Commander output layer. Specifically,
either (A) detect --json/--quiet from process.argv before parsing and call
setOutputMode early, or (B) use commander.configureOutput or supply a custom
writeErr handler that routes parser errors into outputError (the same path that
honors jsonMode) so unknown flags/bad args are formatted as JSON; make changes
around the Command setup where .option(...) and .hook("preAction", ...) are
defined so parser errors no longer bypass json output.
In `@cli/src/mcp-server.ts`:
- Around line 47-57: The readCached function currently swallows all errors;
modify the catch to capture the thrown error (e.g., catch (err)) and emit a
debug/log message including filePath and the error (use existing logger if
available, otherwise console.debug) before returning null so callers still get
null but you can troubleshoot missing/permission/encoding issues; keep fileCache
and fs.readFileSync usage intact and only change the catch to log the error
object.
- Around line 63-67: The current substring check using q.includes(keyword) on
RULE_MAPPING is too greedy and matches inside larger words; replace it with a
word-boundary-aware check (e.g., build a case-insensitive RegExp using \b around
an escaped keyword or tokenize q into words and compare exact tokens) when
iterating RULE_MAPPING in the loop that updates matchedFiles; ensure you escape
regex-special characters in keyword and normalize q (trim/lowercase) so matches
are exact whole-word matches rather than substrings.
In `@cli/src/output.ts`:
- Around line 43-47: In outputTable, rows shorter than headers produce undefined
values that get omitted when JSON-stringified; update the mapping in outputTable
(where rows.map and Object.fromEntries are used) to replace missing entries with
null (or a chosen sentinel) instead of leaving them undefined—e.g., when
building each [h, row[i]] pair, use row[i] ?? null—so the resulting objects
include all header keys when passed to outputJson.
In `@electron/main.ts`:
- Around line 387-393: The handleCliExit function can run before safetyTimer is
initialized and cause a TDZ ReferenceError; move the declaration of safetyTimer
above handleCliExit and initialize it (e.g., let safetyTimer = null) or guard
its use by checking that safetyTimer is defined before calling
clearTimeout(safetyTimer); update references to the safetyTimer variable
accordingly so handleCliExit, which uses safetyTimer, never triggers a
ReferenceError when invoked early.
- Around line 419-427: The call to runCliExport (which awaits
win.loadURL/win.loadFile and can reject on load failures) must be wrapped in a
try/catch so any rejection emits the structured CLI error envelope before
exiting; update the app.whenReady() handler around runCliExport() to catch
errors from win.loadURL/win.loadFile, send the same "__cli" error payload used
by the safetyTimer handler (or the existing CLI error emission helper), and then
exit with a non-zero code—preserve existing cleanup logic and mirror the
safetyTimer error-handling pattern.
In `@src/shared/aspect-ratios.ts`:
- Around line 47-55: The getNativeAspectRatioValue function can produce Infinity
when videoHeight or cropH (from cropRegion.height) is 0; guard the denominator
by computing const denom = videoHeight * (cropRegion?.height ?? 1) and if denom
=== 0 return a safe fallback (e.g., 0) or throw a descriptive error; update the
function (references: getNativeAspectRatioValue, videoHeight, cropRegion, cropH)
to check denom before returning (videoWidth * cropW) / denom so you never divide
by zero.
In `@src/shared/project-schema.ts`:
- Around line 178-400: The function normalizeProjectEditor is doing too many
responsibilities (region normalization, enum validation, clamping, legacy
handling) and should be decomposed: extract the repeated region mapping logic
into dedicated helpers like normalizeZoomRegions, normalizeTrimRegions,
normalizeSpeedRegions, and normalizeAnnotationRegions called from
normalizeProjectEditor; keep existing behavior (use isFiniteNumber, clamp,
DEFAULT_* constants, and preserve field validations such as depth, focusMode,
speed, zIndex, figureData) and ensure the new helpers accept the raw editor
array (e.g., editor.zoomRegions) and return the normalized arrays used in the
final return object; this keeps normalizeProjectEditor caller-visible API
unchanged while improving readability and testability.
- Around line 144-152: The deriveNextId function builds a RegExp from the prefix
which can lead to ReDoS or incorrect matches if prefix ever contains regex
metacharacters; escape the prefix before constructing the RegExp (or add a small
helper like escapeRegex) and use the escapedPrefix in new
RegExp(`^${escapedPrefix}-(\\d+)$`) inside deriveNextId so only literal prefix
strings are matched.
In `@src/shared/recording-session.ts`:
- Around line 33-52: normalizeProjectMedia currently accepts normalized strings
without validating for path traversal or absolute paths; add a validation step
(e.g., an isValidMediaPath function) and use it after normalizePath to reject
unsafe values. Specifically, after calling normalizePath for screenVideoPath and
webcamVideoPath inside normalizeProjectMedia, validate each result with
isValidMediaPath (which should reject empty, absolute paths, parent-traversal
segments like "..", and any other project-specific disallowed patterns); if
screenVideoPath is invalid return null, and only include webcamVideoPath in the
returned ProjectMedia when it also passes validation. Ensure references to
normalizeProjectMedia, normalizePath and the new isValidMediaPath are used so
reviewers can locate the changes.
In `@src/shared/shortcuts.ts`:
- Around line 150-157: mergeWithDefaults currently does a shallow copy of
DEFAULT_SHORTCUTS so the merged object reuses the same ShortcutBinding objects
and mutating merged[action] will mutate DEFAULT_SHORTCUTS; update
mergeWithDefaults to deep-clone each binding when initializing merged and when
assigning from partial (e.g., create new ShortcutBinding objects/copies for
entries in DEFAULT_SHORTCUTS and for partial[action] before assigning) so
DEFAULT_SHORTCUTS remains immutable; touch functions/variables:
mergeWithDefaults, DEFAULT_SHORTCUTS, SHORTCUT_ACTIONS, and ensure the returned
ShortcutsConfig contains independent ShortcutBinding instances.
In `@src/shared/types.ts`:
- Around line 176-185: clampFocusToDepth currently ignores the depth parameter;
change it to use ZOOM_DEPTH_SCALES[depth] to compute a half-viewport margin and
clamp focus.cx / focus.cy to [halfMargin, 1 - halfMargin] so the visible window
cannot spill past the source edges. Specifically, inside clampFocusToDepth (and
using the existing clamp helper or a small variant), compute const half =
(ZOOM_DEPTH_SCALES[depth] ?? 1) / 2 and return cx: clamp(focus.cx, half, 1 -
half), cy: clamp(focus.cy, half, 1 - half); keep NaN handling as-is or ensure
clamp still centers when value is NaN; alternatively, if depth-awareness is not
desired, rename clampFocusToDepth to reflect that 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c97aff4-11cf-484f-9d21-3cad8f4ba8ac
📒 Files selected for processing (41)
cli/skills/SKILL.mdcli/skills/rules/annotations.mdcli/skills/rules/export-render.mdcli/skills/rules/frames-stills.mdcli/skills/rules/project-setup.mdcli/skills/rules/trim-speed.mdcli/skills/rules/troubleshooting.mdcli/skills/rules/zoom-regions.mdcli/src/commands/annotate.tscli/src/commands/project.tscli/src/commands/render.tscli/src/commands/shortcuts.tscli/src/commands/speed.tscli/src/commands/trim.tscli/src/commands/zoom.tscli/src/core/electron-bridge.tscli/src/core/project-manager.tscli/src/core/region-manager.tscli/src/index.tscli/src/mcp-server.tscli/src/output.tscli/tsconfig.jsonelectron/main.tselectron/preload.tspackage.jsonsrc/App.tsxsrc/components/cli-export/CliExportRenderer.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/lib/exporter/types.tssrc/lib/recordingSession.tssrc/lib/shortcuts.tssrc/shared/aspect-ratios.tssrc/shared/export-types.tssrc/shared/index.tssrc/shared/project-schema.tssrc/shared/recording-session.tssrc/shared/shortcuts.tssrc/shared/types.tssrc/utils/aspectRatioUtils.tsvite.config.ts
Comment on lines
+178
to
+400
| export function normalizeProjectEditor(editor: Partial<ProjectEditorState>): ProjectEditorState { | ||
| const validAspectRatios = new Set<AspectRatio>(ASPECT_RATIOS); | ||
|
|
||
| const normalizedZoomRegions: ZoomRegion[] = Array.isArray(editor.zoomRegions) | ||
| ? editor.zoomRegions | ||
| .filter((region): region is ZoomRegion => Boolean(region && typeof region.id === "string")) | ||
| .map((region) => { | ||
| const rawStart = isFiniteNumber(region.startMs) ? Math.round(region.startMs) : 0; | ||
| const rawEnd = isFiniteNumber(region.endMs) ? Math.round(region.endMs) : rawStart + 1000; | ||
| const startMs = Math.max(0, Math.min(rawStart, rawEnd)); | ||
| const endMs = Math.max(startMs + 1, rawEnd); | ||
|
|
||
| return { | ||
| id: region.id, | ||
| startMs, | ||
| endMs, | ||
| depth: [1, 2, 3, 4, 5, 6].includes(region.depth) ? region.depth : DEFAULT_ZOOM_DEPTH, | ||
| focus: { | ||
| cx: clamp(isFiniteNumber(region.focus?.cx) ? region.focus.cx : 0.5, 0, 1), | ||
| cy: clamp(isFiniteNumber(region.focus?.cy) ? region.focus.cy : 0.5, 0, 1), | ||
| }, | ||
| focusMode: region.focusMode === "auto" ? "auto" : "manual", | ||
| }; | ||
| }) | ||
| : []; | ||
|
|
||
| const normalizedTrimRegions: TrimRegion[] = Array.isArray(editor.trimRegions) | ||
| ? editor.trimRegions | ||
| .filter((region): region is TrimRegion => Boolean(region && typeof region.id === "string")) | ||
| .map((region) => { | ||
| const rawStart = isFiniteNumber(region.startMs) ? Math.round(region.startMs) : 0; | ||
| const rawEnd = isFiniteNumber(region.endMs) ? Math.round(region.endMs) : rawStart + 1000; | ||
| const startMs = Math.max(0, Math.min(rawStart, rawEnd)); | ||
| const endMs = Math.max(startMs + 1, rawEnd); | ||
| return { | ||
| id: region.id, | ||
| startMs, | ||
| endMs, | ||
| }; | ||
| }) | ||
| : []; | ||
|
|
||
| const normalizedSpeedRegions: SpeedRegion[] = Array.isArray(editor.speedRegions) | ||
| ? editor.speedRegions | ||
| .filter((region): region is SpeedRegion => Boolean(region && typeof region.id === "string")) | ||
| .map((region) => { | ||
| const rawStart = isFiniteNumber(region.startMs) ? Math.round(region.startMs) : 0; | ||
| const rawEnd = isFiniteNumber(region.endMs) ? Math.round(region.endMs) : rawStart + 1000; | ||
| const startMs = Math.max(0, Math.min(rawStart, rawEnd)); | ||
| const endMs = Math.max(startMs + 1, rawEnd); | ||
|
|
||
| const speed = | ||
| region.speed === 0.25 || | ||
| region.speed === 0.5 || | ||
| region.speed === 0.75 || | ||
| region.speed === 1.25 || | ||
| region.speed === 1.5 || | ||
| region.speed === 1.75 || | ||
| region.speed === 2 | ||
| ? region.speed | ||
| : DEFAULT_PLAYBACK_SPEED; | ||
|
|
||
| return { | ||
| id: region.id, | ||
| startMs, | ||
| endMs, | ||
| speed, | ||
| }; | ||
| }) | ||
| : []; | ||
|
|
||
| const normalizedAnnotationRegions: AnnotationRegion[] = Array.isArray(editor.annotationRegions) | ||
| ? editor.annotationRegions | ||
| .filter((region): region is AnnotationRegion => | ||
| Boolean(region && typeof region.id === "string"), | ||
| ) | ||
| .map((region, index) => { | ||
| const rawStart = isFiniteNumber(region.startMs) ? Math.round(region.startMs) : 0; | ||
| const rawEnd = isFiniteNumber(region.endMs) ? Math.round(region.endMs) : rawStart + 1000; | ||
| const startMs = Math.max(0, Math.min(rawStart, rawEnd)); | ||
| const endMs = Math.max(startMs + 1, rawEnd); | ||
|
|
||
| return { | ||
| id: region.id, | ||
| startMs, | ||
| endMs, | ||
| type: region.type === "image" || region.type === "figure" ? region.type : "text", | ||
| content: typeof region.content === "string" ? region.content : "", | ||
| textContent: typeof region.textContent === "string" ? region.textContent : undefined, | ||
| imageContent: typeof region.imageContent === "string" ? region.imageContent : undefined, | ||
| position: { | ||
| x: clamp( | ||
| isFiniteNumber(region.position?.x) | ||
| ? region.position.x | ||
| : DEFAULT_ANNOTATION_POSITION.x, | ||
| 0, | ||
| 100, | ||
| ), | ||
| y: clamp( | ||
| isFiniteNumber(region.position?.y) | ||
| ? region.position.y | ||
| : DEFAULT_ANNOTATION_POSITION.y, | ||
| 0, | ||
| 100, | ||
| ), | ||
| }, | ||
| size: { | ||
| width: clamp( | ||
| isFiniteNumber(region.size?.width) | ||
| ? region.size.width | ||
| : DEFAULT_ANNOTATION_SIZE.width, | ||
| 1, | ||
| 200, | ||
| ), | ||
| height: clamp( | ||
| isFiniteNumber(region.size?.height) | ||
| ? region.size.height | ||
| : DEFAULT_ANNOTATION_SIZE.height, | ||
| 1, | ||
| 200, | ||
| ), | ||
| }, | ||
| style: { | ||
| ...DEFAULT_ANNOTATION_STYLE, | ||
| ...(region.style && typeof region.style === "object" ? region.style : {}), | ||
| }, | ||
| zIndex: isFiniteNumber(region.zIndex) ? region.zIndex : index + 1, | ||
| figureData: region.figureData | ||
| ? { | ||
| ...DEFAULT_FIGURE_DATA, | ||
| ...region.figureData, | ||
| } | ||
| : undefined, | ||
| }; | ||
| }) | ||
| : []; | ||
|
|
||
| const rawCropX = isFiniteNumber(editor.cropRegion?.x) | ||
| ? editor.cropRegion.x | ||
| : DEFAULT_CROP_REGION.x; | ||
| const rawCropY = isFiniteNumber(editor.cropRegion?.y) | ||
| ? editor.cropRegion.y | ||
| : DEFAULT_CROP_REGION.y; | ||
| const rawCropWidth = isFiniteNumber(editor.cropRegion?.width) | ||
| ? editor.cropRegion.width | ||
| : DEFAULT_CROP_REGION.width; | ||
| const rawCropHeight = isFiniteNumber(editor.cropRegion?.height) | ||
| ? editor.cropRegion.height | ||
| : DEFAULT_CROP_REGION.height; | ||
|
|
||
| const cropX = clamp(rawCropX, 0, 1); | ||
| const cropY = clamp(rawCropY, 0, 1); | ||
| const cropWidth = clamp(rawCropWidth, 0.01, 1 - cropX); | ||
| const cropHeight = clamp(rawCropHeight, 0.01, 1 - cropY); | ||
|
|
||
| return { | ||
| wallpaper: typeof editor.wallpaper === "string" ? editor.wallpaper : WALLPAPER_PATHS[0], | ||
| shadowIntensity: typeof editor.shadowIntensity === "number" ? editor.shadowIntensity : 0, | ||
| showBlur: typeof editor.showBlur === "boolean" ? editor.showBlur : false, | ||
| motionBlurAmount: isFiniteNumber(editor.motionBlurAmount) | ||
| ? clamp(editor.motionBlurAmount, 0, 1) | ||
| : typeof (editor as { motionBlurEnabled?: unknown }).motionBlurEnabled === "boolean" | ||
| ? (editor as { motionBlurEnabled?: boolean }).motionBlurEnabled | ||
| ? 0.35 | ||
| : 0 | ||
| : 0, | ||
| borderRadius: typeof editor.borderRadius === "number" ? editor.borderRadius : 0, | ||
| padding: isFiniteNumber(editor.padding) ? clamp(editor.padding, 0, 100) : 50, | ||
| cropRegion: { | ||
| x: cropX, | ||
| y: cropY, | ||
| width: cropWidth, | ||
| height: cropHeight, | ||
| }, | ||
| zoomRegions: normalizedZoomRegions, | ||
| trimRegions: normalizedTrimRegions, | ||
| speedRegions: normalizedSpeedRegions, | ||
| annotationRegions: normalizedAnnotationRegions, | ||
| aspectRatio: | ||
| editor.aspectRatio && validAspectRatios.has(editor.aspectRatio) ? editor.aspectRatio : "16:9", | ||
| webcamLayoutPreset: | ||
| editor.webcamLayoutPreset === "vertical-stack" || | ||
| editor.webcamLayoutPreset === "picture-in-picture" | ||
| ? editor.webcamLayoutPreset | ||
| : DEFAULT_WEBCAM_LAYOUT_PRESET, | ||
| webcamMaskShape: | ||
| editor.webcamMaskShape === "rectangle" || | ||
| editor.webcamMaskShape === "circle" || | ||
| editor.webcamMaskShape === "square" || | ||
| editor.webcamMaskShape === "rounded" | ||
| ? editor.webcamMaskShape | ||
| : DEFAULT_WEBCAM_MASK_SHAPE, | ||
| webcamPosition: | ||
| editor.webcamPosition && | ||
| typeof editor.webcamPosition === "object" && | ||
| isFiniteNumber((editor.webcamPosition as WebcamPosition).cx) && | ||
| isFiniteNumber((editor.webcamPosition as WebcamPosition).cy) | ||
| ? { | ||
| cx: clamp((editor.webcamPosition as WebcamPosition).cx, 0, 1), | ||
| cy: clamp((editor.webcamPosition as WebcamPosition).cy, 0, 1), | ||
| } | ||
| : DEFAULT_WEBCAM_POSITION, | ||
| exportQuality: | ||
| editor.exportQuality === "medium" || editor.exportQuality === "source" | ||
| ? editor.exportQuality | ||
| : "good", | ||
| exportFormat: editor.exportFormat === "gif" ? "gif" : "mp4", | ||
| gifFrameRate: | ||
| editor.gifFrameRate === 15 || | ||
| editor.gifFrameRate === 20 || | ||
| editor.gifFrameRate === 25 || | ||
| editor.gifFrameRate === 30 | ||
| ? editor.gifFrameRate | ||
| : 15, | ||
| gifLoop: typeof editor.gifLoop === "boolean" ? editor.gifLoop : true, | ||
| gifSizePreset: | ||
| editor.gifSizePreset === "medium" || | ||
| editor.gifSizePreset === "large" || | ||
| editor.gifSizePreset === "original" | ||
| ? editor.gifSizePreset | ||
| : "medium", | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
normalizeProjectEditor is comprehensive but... thicc
this 220-line function does a lot of heavy lifting — region normalization, enum validation, clamping, legacy field handling. it's all correct and well-structured, but at some point you might want to break out the region normalizers into separate functions for readability.
not blocking, just 2am thoughts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/project-schema.ts` around lines 178 - 400, The function
normalizeProjectEditor is doing too many responsibilities (region normalization,
enum validation, clamping, legacy handling) and should be decomposed: extract
the repeated region mapping logic into dedicated helpers like
normalizeZoomRegions, normalizeTrimRegions, normalizeSpeedRegions, and
normalizeAnnotationRegions called from normalizeProjectEditor; keep existing
behavior (use isFiniteNumber, clamp, DEFAULT_* constants, and preserve field
validations such as depth, focusMode, speed, zIndex, figureData) and ensure the
new helpers accept the raw editor array (e.g., editor.zoomRegions) and return
the normalized arrays used in the final return object; this keeps
normalizeProjectEditor caller-visible API unchanged while improving readability
and testability.
Comment on lines
+33
to
+52
| export function normalizeProjectMedia(candidate: unknown): ProjectMedia | null { | ||
| if (!candidate || typeof candidate !== "object") { | ||
| return null; | ||
| } | ||
|
|
||
| const raw = candidate as Partial<ProjectMedia>; | ||
| const screenVideoPath = normalizePath(raw.screenVideoPath); | ||
|
|
||
| if (!screenVideoPath) { | ||
| return null; | ||
| } | ||
|
|
||
| const webcamVideoPath = normalizePath(raw.webcamVideoPath); | ||
|
|
||
| return webcamVideoPath | ||
| ? { screenVideoPath, webcamVideoPath } | ||
| : { | ||
| screenVideoPath, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
nit: might want path validation for security
if these paths ever come from untrusted input (like a project file loaded from disk), there's no validation against path traversal (../../../etc/passwd vibes). the normalizer just trims the string and calls it a day.
probably fine for now if paths are only set internally, but worth considering a isValidMediaPath() check if this ever handles user-supplied project files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/recording-session.ts` around lines 33 - 52, normalizeProjectMedia
currently accepts normalized strings without validating for path traversal or
absolute paths; add a validation step (e.g., an isValidMediaPath function) and
use it after normalizePath to reject unsafe values. Specifically, after calling
normalizePath for screenVideoPath and webcamVideoPath inside
normalizeProjectMedia, validate each result with isValidMediaPath (which should
reject empty, absolute paths, parent-traversal segments like "..", and any other
project-specific disallowed patterns); if screenVideoPath is invalid return
null, and only include webcamVideoPath in the returned ProjectMedia when it also
passes validation. Ensure references to normalizeProjectMedia, normalizePath and
the new isValidMediaPath are used so reviewers can locate the changes.
Comment on lines
+176
to
+185
| export function clampFocusToDepth(focus: ZoomFocus, _depth: ZoomDepth): ZoomFocus { | ||
| return { | ||
| cx: clamp(focus.cx, 0, 1), | ||
| cy: clamp(focus.cy, 0, 1), | ||
| }; | ||
| } | ||
|
|
||
| function clamp(value: number, min: number, max: number) { | ||
| if (Number.isNaN(value)) return (min + max) / 2; | ||
| return Math.min(max, Math.max(min, value)); |
Contributor
There was a problem hiding this comment.
clampFocusToDepth() isn't actually using depth.
At higher zoom levels this still allows cx/cy at 0 or 1, so the visible window can spill past the source edges and show empty space. Clamp against the half-viewport implied by ZOOM_DEPTH_SCALES[depth], or rename the helper if depth-aware clamping is not intended.
🐛 Proposed fix
-export function clampFocusToDepth(focus: ZoomFocus, _depth: ZoomDepth): ZoomFocus {
+export function clampFocusToDepth(focus: ZoomFocus, depth: ZoomDepth): ZoomFocus {
+ const scale = ZOOM_DEPTH_SCALES[depth];
+ const halfViewport = 0.5 / scale;
return {
- cx: clamp(focus.cx, 0, 1),
- cy: clamp(focus.cy, 0, 1),
+ cx: clamp(focus.cx, halfViewport, 1 - halfViewport),
+ cy: clamp(focus.cy, halfViewport, 1 - halfViewport),
};
}📝 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
| export function clampFocusToDepth(focus: ZoomFocus, _depth: ZoomDepth): ZoomFocus { | |
| return { | |
| cx: clamp(focus.cx, 0, 1), | |
| cy: clamp(focus.cy, 0, 1), | |
| }; | |
| } | |
| function clamp(value: number, min: number, max: number) { | |
| if (Number.isNaN(value)) return (min + max) / 2; | |
| return Math.min(max, Math.max(min, value)); | |
| export function clampFocusToDepth(focus: ZoomFocus, depth: ZoomDepth): ZoomFocus { | |
| const scale = ZOOM_DEPTH_SCALES[depth]; | |
| const halfViewport = 0.5 / scale; | |
| return { | |
| cx: clamp(focus.cx, halfViewport, 1 - halfViewport), | |
| cy: clamp(focus.cy, halfViewport, 1 - halfViewport), | |
| }; | |
| } | |
| function clamp(value: number, min: number, max: number) { | |
| if (Number.isNaN(value)) return (min + max) / 2; | |
| return Math.min(max, Math.max(min, value)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/types.ts` around lines 176 - 185, clampFocusToDepth currently
ignores the depth parameter; change it to use ZOOM_DEPTH_SCALES[depth] to
compute a half-viewport margin and clamp focus.cx / focus.cy to [halfMargin, 1 -
halfMargin] so the visible window cannot spill past the source edges.
Specifically, inside clampFocusToDepth (and using the existing clamp helper or a
small variant), compute const half = (ZOOM_DEPTH_SCALES[depth] ?? 1) / 2 and
return cx: clamp(focus.cx, half, 1 - half), cy: clamp(focus.cy, half, 1 - half);
keep NaN handling as-is or ensure clamp still centers when value is NaN;
alternatively, if depth-awareness is not desired, rename clampFocusToDepth to
reflect that behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97dfa7a0cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
cli/src/commands/shortcuts.ts (1)
42-52:⚠️ Potential issue | 🟠 MajorDon’t silently treat a broken
shortcuts.jsonas defaults.Line 49 swallows all read/parse failures, so corrupted JSON looks like a valid default config and can get overwritten on the next
set. OnlyENOENTshould fall back to defaults; everything else should surface.nit: safer load path
function loadShortcuts(): ShortcutsConfig { const filePath = getShortcutsPath(); try { - if (fs.existsSync(filePath)) { - const raw = JSON.parse(fs.readFileSync(filePath, "utf-8")); - return mergeWithDefaults(raw); - } - } catch { - // Fall through to defaults + if (!fs.existsSync(filePath)) { + return { ...DEFAULT_SHORTCUTS }; + } + const raw = JSON.parse(fs.readFileSync(filePath, "utf-8")); + return mergeWithDefaults(raw); + } catch (error) { + throw new Error( + `Failed to load shortcuts from ${filePath}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); } - return { ...DEFAULT_SHORTCUTS }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/shortcuts.ts` around lines 42 - 52, The loadShortcuts function currently swallows all errors when reading/parsing shortcuts.json; change it so only a missing-file error (ENOENT) falls back to DEFAULT_SHORTCUTS and all other errors are propagated (or rethrown) so corrupted JSON is not silently treated as defaults. In practice, wrap fs.readFileSync/JSON.parse in a try/catch that checks the caught error.code === 'ENOENT' (or uses fs.existsSync beforehand) and returns mergeWithDefaults(raw) only on success; for any other exception, rethrow or surface the error instead of returning DEFAULT_SHORTCUTS. Ensure you reference loadShortcuts, getShortcutsPath, mergeWithDefaults, and DEFAULT_SHORTCUTS when modifying the error handling.cli/src/commands/render.ts (2)
5-23:⚠️ Potential issue | 🟠 Major
--qualitystill accepts any stringpast review flagged that
--qualityaccepts anything and only fails downstream. user can pass--quality bananaand get a cryptic error from the renderer. should validate against["medium", "good", "source"]at parse time with commander'schoices()or a custom validator.🐛 suggested fix using commander choices
- .option("--quality <q>", "Export quality (medium, good, source)", "good") + .option("--quality <q>", "Export quality (medium, good, source)") + .addOption( + new (await import("commander")).Option("--quality <q>", "Export quality") + .choices(["medium", "good", "source"]) + .default("good") + )or validate in the action handler like gifCommand does for frame-rate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/render.ts` around lines 5 - 23, The renderCommand currently allows any string for --quality and fails later; update validation to restrict values to ["medium","good","source"] by adding a choices() to the .option for quality on renderCommand or by checking opts.quality in the .action before calling runExport (similar to gifCommand's frame-rate validation) and returning a clear error via outputError if the value is invalid so runExport receives only valid qualities.
34-34:⚠️ Potential issue | 🟠 Major
--loopalways ends uptrue, overriding saved settingswith Commander,
.option("--loop", "desc", true)meansopts.loopis alwaystrue- even when the flag isn't provided. this nukes any savedgifLoop = falsesetting. user can't create one-shot GIFs via CLI.🐛 use --no-loop pattern
- .option("--loop", "Loop GIF", true) + .option("--loop", "Loop GIF (default: true)") + .option("--no-loop", "Disable GIF looping")this lets
opts.loopbeundefinedwhen neither flag is passed, allowing the fallback logic in the renderer to respect saved settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/render.ts` at line 34, The CLI option definition currently forces opts.loop to true by using a default value in the Commander call; remove the default so the flag is undefined when not provided and add the inverse flag pattern (use the --no-loop variant) so users can explicitly disable looping. Update the option declaration in the render command (the .option(...) call that currently reads "--loop") to omit the true default and register the corresponding --no-loop form so downstream code can consult opts.loop (or undefined) and let the renderer's gifLoop fallback logic decide when saved settings should apply.cli/src/core/electron-bridge.ts (3)
59-68:⚠️ Potential issue | 🟠 Majorearly return doesn't propagate failure to caller
this was flagged before and is still present. when the file exists and
--overwriteisn't set,outputErroris called but the function returns normally.runExportresolves successfully, so callers can't distinguish this from a success. kinda cursed for programmatic use.🐛 suggested fix
if (!overwrite) { try { fs.accessSync(absOutput); - outputError(`Output file already exists: ${absOutput}. Use --overwrite to replace.`); - return; + const msg = `Output file already exists: ${absOutput}. Use --overwrite to replace.`; + outputError(msg); + throw new Error(msg); } catch (e) { + if (e instanceof Error && e.message.includes("already exists")) throw e; // File doesn't exist, proceed } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/electron-bridge.ts` around lines 59 - 68, When the file exists and overwrite is false the current early return after calling outputError does not signal failure to callers of runExport; change this to propagate an error by throwing or rejecting so runExport does not resolve successfully. Specifically, in the block that checks fs.accessSync(absOutput) (using absOutput, overwrite and outputError), replace the plain return with a thrown Error (or Promise rejection) containing the same descriptive message (e.g., "Output file already exists: ...") so callers of runExport receive a failure instead of a silent success.
184-193:⚠️ Potential issue | 🟠 Majorerror is swallowed - function returns successfully even on failure
the catch block calls
outputErrorbut doesn't re-throw, sorunExportcompletes successfully even when the export fails. callers have no way to know something went wrong programmatically.🐛 suggested fix
} catch (e) { outputError(e instanceof Error ? e.message : String(e)); + throw e; } finally { // Clean up temp config file try { fs.unlinkSync(configPath); } catch { /* ignore */ } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/electron-bridge.ts` around lines 184 - 193, The catch in runExport calls outputError but swallows the error so callers think the export succeeded; update runExport's catch to re-throw the original error (or throw a new Error) after calling outputError so failures propagate to callers — specifically modify the catch block around outputError(e instanceof Error ? e.message : String(e)) to follow that call with `throw e` (or `throw new Error(...)`) while keeping the existing finally cleanup (fs.unlinkSync(configPath)) intact.
164-170: 🧹 Nitpick | 🔵 Trivialstderr error detection is case-sensitive
still checking for "ERROR" or "Error" specifically. electron/chromium can be inconsistent with error formatting, might miss lowercase "error".
💡 more robust check
child.stderr?.on("data", (chunk: Buffer) => { const text = chunk.toString(); - if (text.includes("ERROR") || text.includes("Error")) { + if (/error/i.test(text)) { lastError = text.trim(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/electron-bridge.ts` around lines 164 - 170, The stderr handler on child.stderr in electron-bridge.ts only checks for "ERROR" or "Error" and can miss lowercase "error"; change the detection to be case-insensitive (e.g., convert chunk.toString() to lowercase and check includes("error") or use a case-insensitive regex) so lastError is set for any casing of "error" while preserving the existing trimming behavior; update the anonymous callback attached to child.stderr?.on("data", ...) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/render.ts`:
- Around line 65-89: stillCommand and framesCommand are currently stubs that
just call outputError; replace these stubs with real handlers that parse and
validate their respective options and invoke the actual rendering logic (e.g.,
implement functions like renderStill(projectPath, outputPath, {frame, format,
jpegQuality, scale, overwrite}) and renderFrames(projectPath, outputDir, {start,
end, everyNth, format, overwrite}) or hook into existing render/export APIs),
ensuring options from stillCommand (frame, format, jpeg-quality, scale,
overwrite) and framesCommand (start, end, every-nth, format, overwrite) are
converted to proper types and validated before calling the renderer and that
filesystem checks (create/overwrite output file or directory) are handled; keep
the same Command definitions but replace the .action((_opts) =>
outputError(...)) with async handlers that call the new render functions and
surface errors via outputError.
- Around line 25-26: Replace the duplicated local frame-rate list by importing
the canonical VALID_GIF_FRAME_RATES (or the helper isValidGifFrameRate) from the
shared export-types module and use isValidGifFrameRate(frameRate) wherever the
file currently validates against VALID_GIF_FRAME_RATES; remove the local const
VALID_GIF_FRAME_RATES to avoid drift and ensure all validation in render.ts uses
the shared symbol isValidGifFrameRate for consistency.
In `@cli/src/commands/shortcuts.ts`:
- Around line 91-108: Normalize the "space" alias before persisting by
converting opts.key === "space" (case-sensitive or consider lowercasing) into
the actual space character " " prior to assigning into the ShortcutsConfig;
update the .action((opts) => { ... }) handler where you call loadShortcuts() and
set config[opts.action as keyof ShortcutsConfig] so that the value saved uses "
" instead of the literal "space" (e.g., compute a normalizedKey variable from
opts.key and use that in the object with key: normalizedKey).
In `@cli/src/core/electron-bridge.ts`:
- Around line 98-116: The args array used when spawning Electron (see the args
constant and the spawn(electronBin, args, ...) call in electron-bridge.ts)
includes "--no-sandbox" for headless/CI/container runs; add a concise inline
comment above the args declaration explaining that "--no-sandbox" is
intentionally required for headless Electron in CI/containers (where sandboxing
can fail) and should not be removed, and mention that HEADLESS and
ELECTRON_DISABLE_SECURITY_WARNINGS env vars are set for the same
headless/testing context to help future maintainers understand the rationale.
In `@cli/src/core/project-manager.ts`:
- Around line 70-72: The current check throws a generic Error when
validateProjectData(parsed) returns false, losing the specific failure reason;
change the flow to obtain and surface a descriptive validation message (e.g.,
have validateProjectData return an error string or call a helper like
getProjectValidationError(parsed)) and include that message when throwing (e.g.,
throw new Error(`Invalid project data in ${absPath}: ${validationMessage}`)),
referencing validateProjectData, parsed, and absPath so callers see exactly what
failed.
- Around line 140-164: The editProject function silently relies on
normalizeProjectEditor to clamp out-of-range edits; add explicit CLI-layer
validation inside editProject (before calling normalizeProjectEditor and
saveProject) that checks each editable field in the edits object (e.g., padding
∈ [0,100], aspectRatio allowed set, borderRadius, shadowIntensity,
exportQuality, gifFrameRate, etc.) and either returns/throws a descriptive error
or emits a clear warning when a value is invalid, referencing function names
editProject, normalizeProjectEditor, loadProject and saveProject and the
EditorProjectData type so reviewers can locate the change; ensure validation
paths abort before saveProject on errors and preserve existing behavior only for
valid inputs.
- Around line 74-81: The code redundantly calls resolveProjectMedia twice;
remove the second call and reuse the media produced by validateProjectData
instead. Replace the local call at the return site so you no longer invoke
resolveProjectMedia(parsed) and instead conditionally include parsed.media (or
the media property returned by validateProjectData) in the returned object,
keeping editor: normalizeProjectEditor(parsed.editor) unchanged; ensure
resolveProjectMedia remains called inside validateProjectData and that
validateProjectData returns the media on parsed.
---
Duplicate comments:
In `@cli/src/commands/render.ts`:
- Around line 5-23: The renderCommand currently allows any string for --quality
and fails later; update validation to restrict values to
["medium","good","source"] by adding a choices() to the .option for quality on
renderCommand or by checking opts.quality in the .action before calling
runExport (similar to gifCommand's frame-rate validation) and returning a clear
error via outputError if the value is invalid so runExport receives only valid
qualities.
- Line 34: The CLI option definition currently forces opts.loop to true by using
a default value in the Commander call; remove the default so the flag is
undefined when not provided and add the inverse flag pattern (use the --no-loop
variant) so users can explicitly disable looping. Update the option declaration
in the render command (the .option(...) call that currently reads "--loop") to
omit the true default and register the corresponding --no-loop form so
downstream code can consult opts.loop (or undefined) and let the renderer's
gifLoop fallback logic decide when saved settings should apply.
In `@cli/src/commands/shortcuts.ts`:
- Around line 42-52: The loadShortcuts function currently swallows all errors
when reading/parsing shortcuts.json; change it so only a missing-file error
(ENOENT) falls back to DEFAULT_SHORTCUTS and all other errors are propagated (or
rethrown) so corrupted JSON is not silently treated as defaults. In practice,
wrap fs.readFileSync/JSON.parse in a try/catch that checks the caught error.code
=== 'ENOENT' (or uses fs.existsSync beforehand) and returns
mergeWithDefaults(raw) only on success; for any other exception, rethrow or
surface the error instead of returning DEFAULT_SHORTCUTS. Ensure you reference
loadShortcuts, getShortcutsPath, mergeWithDefaults, and DEFAULT_SHORTCUTS when
modifying the error handling.
In `@cli/src/core/electron-bridge.ts`:
- Around line 59-68: When the file exists and overwrite is false the current
early return after calling outputError does not signal failure to callers of
runExport; change this to propagate an error by throwing or rejecting so
runExport does not resolve successfully. Specifically, in the block that checks
fs.accessSync(absOutput) (using absOutput, overwrite and outputError), replace
the plain return with a thrown Error (or Promise rejection) containing the same
descriptive message (e.g., "Output file already exists: ...") so callers of
runExport receive a failure instead of a silent success.
- Around line 184-193: The catch in runExport calls outputError but swallows the
error so callers think the export succeeded; update runExport's catch to
re-throw the original error (or throw a new Error) after calling outputError so
failures propagate to callers — specifically modify the catch block around
outputError(e instanceof Error ? e.message : String(e)) to follow that call with
`throw e` (or `throw new Error(...)`) while keeping the existing finally cleanup
(fs.unlinkSync(configPath)) intact.
- Around line 164-170: The stderr handler on child.stderr in electron-bridge.ts
only checks for "ERROR" or "Error" and can miss lowercase "error"; change the
detection to be case-insensitive (e.g., convert chunk.toString() to lowercase
and check includes("error") or use a case-insensitive regex) so lastError is set
for any casing of "error" while preserving the existing trimming behavior;
update the anonymous callback attached to child.stderr?.on("data", ...)
accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72f2515d-c2b1-4d73-9b8b-4ec83a7be884
📒 Files selected for processing (4)
cli/src/commands/render.tscli/src/commands/shortcuts.tscli/src/core/electron-bridge.tscli/src/core/project-manager.ts
11 tasks
lupuletic
force-pushed
the
feat/cli-agent-interface
branch
from
97dfa7a to
0c8d2ce
Compare
lupuletic
pushed a commit
to lupuletic/openscreen
that referenced
this pull request
Apr 12, 2026
- Suppress human-readable text in --json render mode (P1) - Materialize media from legacy videoPath in loadProject (P1) - Validate GIF frame rate and size preset at CLI layer (P2) - Use Electron's actual productName for userData path lookup (P2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lupuletic
pushed a commit
to lupuletic/openscreen
that referenced
this pull request
Apr 12, 2026
- render.ts: use `--loop`/`--no-loop` negatable pair so omitting the flag falls through to the project's `editor.gifLoop` (was: default=true clobbered the project setting); drop duplicated VALID_GIF_FRAME_RATES in favor of isValidGifFrameRate from src/shared/export-types - electron-bridge.ts: carryover buffer for stdout so JSON messages split across chunk boundaries aren't dropped; flush trailing partial line on close; document why `--no-sandbox` + HEADLESS env are required - shortcuts.ts: normalize `--key space` to the literal " " character so the binding actually matches at runtime (DEFAULT_SHORTCUTS uses " ") - project-manager.ts: replace generic "Invalid project data" with field-specific errors (missing version / media / editor); call resolveProjectMedia once instead of twice; add validateEditOptions so the CLI rejects out-of-range padding/motion-blur/aspect-ratio/quality/ format/gif options up-front instead of silently clamping Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a comprehensive CLI (`openscreen`) that makes OpenScreen accessible to AI agents and automation. Inspired by Remotion's architecture: CLI as the primary interface, skills for agent guidance, minimal MCP for discovery. **CLI commands (20+ subcommands):** - `project create/info/validate/edit` — project file CRUD - `zoom/trim/speed/annotate add/list/remove` — region management - `render` — headless MP4 export via Electron bridge - `gif` — headless GIF export via Electron bridge - `shortcuts get/set` — keyboard shortcut management - `--json` flag for machine-readable output on all commands **Shared code extraction (`src/shared/`):** - Pure TypeScript types and logic extracted from video-editor - Used by both CLI and Electron app, no code duplication - Original files converted to thin re-exports (zero breaking changes) **Electron headless bridge:** - `--cli-export` mode in electron/main.ts - `CliExportRenderer` React component for headless rendering - Full WebCodecs + PixiJS pipeline, real-time progress reporting - Produces valid MP4 and GIF files with all effects applied **Agent skills (SKILL.md + 7 rule files):** - Follows Remotion/agentskills.io pattern - Covers project setup, zoom, trim, speed, annotations, export, troubleshooting **Minimal MCP server:** - Single `openscreen_help` tool for doc search - Keyword-based rule file lookup with caching Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Suppress human-readable text in --json render mode (P1) - Materialize media from legacy videoPath in loadProject (P1) - Validate GIF frame rate and size preset at CLI layer (P2) - Use Electron's actual productName for userData path lookup (P2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- render.ts: use `--loop`/`--no-loop` negatable pair so omitting the flag falls through to the project's `editor.gifLoop` (was: default=true clobbered the project setting); drop duplicated VALID_GIF_FRAME_RATES in favor of isValidGifFrameRate from src/shared/export-types - electron-bridge.ts: carryover buffer for stdout so JSON messages split across chunk boundaries aren't dropped; flush trailing partial line on close; document why `--no-sandbox` + HEADLESS env are required - shortcuts.ts: normalize `--key space` to the literal " " character so the binding actually matches at runtime (DEFAULT_SHORTCUTS uses " ") - project-manager.ts: replace generic "Invalid project data" with field-specific errors (missing version / media / editor); call resolveProjectMedia once instead of twice; add validateEditOptions so the CLI rejects out-of-range padding/motion-blur/aspect-ratio/quality/ format/gif options up-front instead of silently clamping Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes drifts discovered by walking the skill rules end-to-end against
the CLI on a real 47s recording.
- output.ts: add outputList<T> so `--json` list commands emit the raw
object array instead of the display-formatted `outputTable` keys
(was: `{"Start (ms)": "2000"}`, now: `{"startMs": 2000}`). JSON list
shape now matches JSON add shape, so agents can use a single parser
- zoom/trim/speed/annotate list: migrated to outputList
- project-manager.ts: `--wallpaper wallpaperN` now translates to
`/wallpapers/wallpaperN.jpg` (the path the renderer expects) in both
createProject and editProject. Hex colors and absolute paths pass
through unchanged. Out-of-range N (e.g. wallpaper99) now errors with
a specific message instead of silently storing garbage
- render.ts + index.ts + mcp-server.ts: drop the `still` and `frames`
stub commands that errored with "not yet implemented" at runtime.
The command surface now matches what actually works so agents aren't
led toward dead ends
- SKILL.md: remove the stills/frames rule reference and description
line. Delete cli/skills/rules/frames-stills.md entirely
- troubleshooting.md: add the "Video decode ended early" gotcha with
the ffprobe recipe (regions must fall within video duration — the
CLI doesn't probe at add time). Correct the Electron install note
- export-render.md: correct the progress JSON example (phase is only
present during `finalizing`, not `extracting`); document the new
`--loop`/`--no-loop` tri-state contract
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lupuletic
force-pushed
the
feat/cli-agent-interface
branch
from
0c8d2ce to
960a609
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (9)
cli/src/commands/project.ts (1)
116-118:⚠️ Potential issue | 🟠 MajorMake boolean parsing strict instead of silently turning junk into
false.Right now any unrecognized literal becomes
false, so--gif-loop treuor--show-blur TRUEquietly flips the setting off. That’s kinda cursed for automation; bad literals should error, not coerce.nit: cleaner strict parser
function parseBool(value: string): boolean { - return value === "true" || value === "1" || value === "yes"; + const normalized = value.trim().toLowerCase(); + if (normalized === "true" || normalized === "1" || normalized === "yes") return true; + if (normalized === "false" || normalized === "0" || normalized === "no") return false; + throw new Error(`Invalid boolean value: ${value}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/project.ts` around lines 116 - 118, The parseBool function currently coerces any unrecognized string to false; change it to perform strict validation in parseBool(value: string): boolean by accepting only explicit true literals ("true","1","yes") and explicit false literals ("false","0","no") (case-insensitive), returning the corresponding boolean for those, and throwing a clear Error (or CLI-specific error) when the input does not match any allowed token so invalid flags like "treu" or "TRUE" are rejected instead of silently flipping the value.cli/src/commands/annotate.ts (1)
17-23:⚠️ Potential issue | 🟠 MajorFail fast on invalid annotation numbers.
Same gap as before:
parseInt/parseFloathere can yieldNaN, and the documented ranges (0-100, positive ms, etc.) are not enforced. SinceaddAnnotationRegion()stores these values basically as-is, a bad flag can leave the project in a weird state instead of erroring immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/annotate.ts` around lines 17 - 23, The CLI option parsers in annotate.ts use parseInt/parseFloat which can return NaN and do not enforce ranges before calling addAnnotationRegion; add explicit validation for each parsed value (start, end, positionX, positionY, width, height, fontSize) right after parsing: check for NaN and for required ranges (start/end >= 0, start < end, positionX/positionY in 0-100, width/height > 0 and <=100 if applicable, fontSize > 0), and if any check fails throw or call program.error with a clear message so the process fails fast instead of storing invalid values in addAnnotationRegion().cli/src/core/electron-bridge.ts (2)
60-65:⚠️ Potential issue | 🟠 MajorDon’t return success after reporting “output already exists”.
This branch still calls
outputError()and then returns normally, sorunExport()resolves even though the export never started. For a programmatic CLI surface, that’s lowkey risky — the caller should get a rejected promise here.🐛 simple fix
if (!overwrite) { try { fs.accessSync(absOutput); - outputError(`Output file already exists: ${absOutput}. Use --overwrite to replace.`); - return; + throw new Error(`Output file already exists: ${absOutput}. Use --overwrite to replace.`); } catch { // File doesn't exist, proceed } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/electron-bridge.ts` around lines 60 - 65, The code calls outputError(...) when absOutput exists but then returns normally, letting runExport resolve successfully; change this to reject so callers see failure: after fs.accessSync detects the existing file (in the branch where overwrite is false) replace the plain return with throwing an Error (e.g., throw new Error(`Output file already exists: ${absOutput}`)) or return Promise.reject(...) so runExport (and callers) get a rejected promise; the change should be made in the same block that references overwrite, fs.accessSync, absOutput, and outputError.
206-208:⚠️ Potential issue | 🟠 MajorRe-throw after
outputError()so export failures propagate.This catch still swallows the failure after logging it, which means callers can treat a broken export as success. That’s especially awkward for automation.
nit: cleaner propagation
} catch (e) { outputError(e instanceof Error ? e.message : String(e)); + throw e; } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/electron-bridge.ts` around lines 206 - 208, The catch block currently logs export failures using outputError(...) but swallows the error; update the catch in the export flow (the try/catch surrounding outputError) to re-throw the original error after logging (e.g., outputError(...); throw e;) so callers of the function (the export routine that invokes outputError) receive the failure and automation can detect it; ensure you preserve the original Error object when re-throwing rather than throwing a new string.electron/main.ts (2)
477-480:⚠️ Potential issue | 🟠 MajorWrap
runCliExport()so load failures still emit the CLI error envelope.
runCliExport()can reject onloadURL()/loadFile()failures, and right now that bubbles out without writing a structured__clierror line. For an agent-facing command, that’s pretty rough UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/main.ts` around lines 477 - 480, The app.whenReady() branch should guard the runCliExport() call so loadURL()/loadFile() rejections produce the structured CLI error envelope; wrap the await runCliExport() call in a try/catch inside the app.whenReady() handler, catch any error thrown by runCliExport(), serialize it into the same "__cli" error line used by other CLI paths (emit the structured CLI error envelope), then ensure the process exits or returns the same way as a normal CLI error path. Target the runCliExport() invocation in the app.whenReady() block and mirror the existing CLI error emission/exit behavior.
419-424:⚠️ Potential issue | 🔴 Critical
handleCliExit()can still hit the TDZ on fast renderer failures.If the renderer posts
"error"beforeloadURL()/loadFile()finishes,clearTimeout(safetyTimer)runs beforesafetyTimeris initialized and throws aReferenceError, which masks the real export failure.🐛 minimal fix
let exiting = false; + let safetyTimer: ReturnType<typeof setTimeout> | undefined; function handleCliExit(type: string) { if (exiting) return; exiting = true; - clearTimeout(safetyTimer); + if (safetyTimer) clearTimeout(safetyTimer); setTimeout(() => app.exit(type === "done" ? 0 : 1), 500); } @@ - const safetyTimer = setTimeout( + safetyTimer = setTimeout(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/main.ts` around lines 419 - 424, handleCliExit can run before safetyTimer is initialized and throw a TDZ ReferenceError; change it so it safely clears the timer (and/or avoids TDZ) by ensuring safetyTimer is declared/initialized before possible calls or by checking its existence before calling clearTimeout inside handleCliExit (referencing the symbols safetyTimer and handleCliExit and the exiting flag) — e.g., declare safetyTimer with an explicit undefined initial value or guard clearTimeout(safetyTimer) with a truthy check so clearTimeout is only invoked when safetyTimer is set.cli/src/commands/zoom.ts (1)
11-16:⚠️ Potential issue | 🟠 MajorReject
NaNand out-of-range zoom args here.This still accepts bad input like
--depth nopeor--focus-x 2and passes it straight intoaddZoomRegion(), which lowkey means invalid values get written to the project instead of failing fast. The help text advertises ranges, but nothing here enforces them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/zoom.ts` around lines 11 - 16, The CLI options currently use parseInt/parseFloat but don't validate values before calling addZoomRegion; add explicit checks right after parsing the options in zoom.ts to reject NaN and out-of-range values: ensure start and end are finite numbers (Number.isFinite) and start < end, ensure depth is an integer between 1 and 6, ensure focus-x and focus-y are numbers between 0 and 1 inclusive, and ensure focus-mode is one of "manual" or "auto"; if any check fails, print a clear error and exit non-zero (or throw) before invoking addZoomRegion so invalid values never get written.src/shared/project-schema.ts (1)
177-180:⚠️ Potential issue | 🟡 MinorEscape
prefixbefore building the regex.lowkey risky:
deriveNextId("zoom(1)", ids)changes the regex semantics instead of matching the literal prefix. If this input ever broadens beyond hardcoded strings, it also becomes a security footgun.🛡️ safer version
+function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + export function deriveNextId(prefix: string, ids: string[]): number { + const escapedPrefix = escapeRegExp(prefix); const max = ids.reduce((acc, id) => { - const match = id.match(new RegExp(`^${prefix}-(\\d+)$`)); + const match = id.match(new RegExp(`^${escapedPrefix}-(\\d+)$`)); if (!match) return acc; const value = Number(match[1]); return Number.isFinite(value) ? Math.max(acc, value) : acc; }, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/project-schema.ts` around lines 177 - 180, The regex in deriveNextId currently interpolates raw prefix into new RegExp, which misbehaves for prefixes containing regex metacharacters; to fix, escape prefix before building the RegExp (e.g., replace metacharacters via prefix.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') or call a shared escape helper) and then use the escapedPrefix when constructing new RegExp in deriveNextId so ids.match() only treats prefix as a literal.src/shared/types.ts (1)
227-237:⚠️ Potential issue | 🟠 Major
clampFocusToDepth()still ignoresdepth.kinda cursed: depth 5/6 regions can still normalize to
0or1, which lets the zoom window spill past the source edges and show empty space. This helper should clamp against the half-viewport implied byZOOM_DEPTH_SCALES[depth], not plain[0, 1].🐛 minimal fix
-export function clampFocusToDepth(focus: ZoomFocus, _depth: ZoomDepth): ZoomFocus { +export function clampFocusToDepth(focus: ZoomFocus, depth: ZoomDepth): ZoomFocus { + const scale = ZOOM_DEPTH_SCALES[depth]; + const halfViewport = 0.5 / scale; return { - cx: clampFocusValue(focus.cx), - cy: clampFocusValue(focus.cy), + cx: clampFocusValue(focus.cx, halfViewport, 1 - halfViewport), + cy: clampFocusValue(focus.cy, halfViewport, 1 - halfViewport), }; } -function clampFocusValue(value: number): number { +function clampFocusValue(value: number, min = 0, max = 1): number { if (Number.isNaN(value)) return 0.5; - return Math.min(1, Math.max(0, value)); + return Math.min(max, Math.max(min, value)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types.ts` around lines 227 - 237, clampFocusToDepth currently ignores the depth parameter and always clamps to [0,1]; change it to use the half-viewport implied by ZOOM_DEPTH_SCALES[depth] so the focus cannot move past the image edges: compute halfViewport = ZOOM_DEPTH_SCALES[depth] / 2 (or the appropriate half-size of the visible viewport for your scale) and clamp focus.cx and focus.cy to [halfViewport, 1 - halfViewport]; update clampFocusToDepth to call a clamping helper that accepts min/max (or inline the clamp logic) instead of the existing clampFocusValue so the depth is respected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/output.ts`:
- Around line 81-86: outputProgress currently emits NDJSON to stdout when
jsonMode is true, breaking the intended single-JSON output contract; update
outputProgress (and its callers) so that when jsonMode is true it does not write
progress to stdout — either skip emitting progress entirely or emit progress to
stderr instead, or gate it behind a new jsonStream flag (e.g., jsonStream !==
true). Concretely, change the jsonMode branch in outputProgress to avoid
process.stdout.write for progress (use process.stderr.write or return early),
and add/check a new jsonStream boolean if you need streaming progress behavior
so the normal jsonMode path continues to produce one valid JSON payload.
In `@src/components/cli-export/CliExportRenderer.tsx`:
- Around line 66-68: The parsed config from JSON.parse in CliExportRenderer (the
config variable used before calling runExport) must be schema-validated at
runtime to fail fast on malformed values (e.g., invalid format, gifFrameRate,
gifSizePreset) — add a Zod (or equivalent) schema for CliExportConfig, run
schema.parse(config) immediately after JSON.parse(text) in the code path that
builds config (and similarly where parsing occurs at lines ~178-206), and if
validation fails throw or show a clear error message before calling runExport so
only validated, well-typed config reaches runExport.
- Around line 47-53: The code silently does nothing when
URLSearchParams(...).get("configPath") returns null; add an explicit error
emission in that branch so the CLI doesn't hang: after retrieving configPath
from URLSearchParams, if configPath is falsy call sendToMain("error", { message:
"Missing configPath query parameter" }) (or similar descriptive message) and
return; keep the existing loadAndRunExport(configPath).catch(...) unchanged for
the truthy branch so errors from loadAndRunExport are still reported.
- Around line 194-202: The GifExporterConfig's videoPadding field is dead and
duplicated since GifExporter and FrameRenderer use the padding from base; remove
videoPadding from the GifExporterConfig type and from all construction sites
(e.g., the new GifExporter call in CliExportRenderer where videoPadding:
editor.padding is passed), and also remove any references/assignments to
videoPadding inside the GifExporter class and its constructor so the exporter
relies solely on base.padding (or the existing padding prop passed to
FrameRenderer).
- Line 221: The hardcoded codec "avc1.640033" in the VideoEncoder config can
fail on some platforms; update CliExportRenderer to avoid bailing by (1)
introducing a fallback codec list (e.g., array of codec strings) and iterating
through it before giving up, calling VideoEncoder.isConfigSupported() for each
candidate and selecting the first supported codec, and (2) optionally expose a
preferred codec option on CliExportRenderer's config so callers can override the
default list; reference VideoEncoder.isConfigSupported(), the existing codec:
"avc1.640033" usage, and the CliExportRenderer constructor/props to locate where
to add the fallback logic and the config passthrough.
In `@src/shared/project-schema.ts`:
- Around line 416-419: The current clamps allow cropWidth/cropHeight to become
zero when rawCropX/rawCropY is 1; fix by enforcing a minimum crop invariant:
define a MIN_CROP (e.g. 0.01), clamp cropX = clamp(rawCropX, 0, 1 - MIN_CROP)
and cropY = clamp(rawCropY, 0, 1 - MIN_CROP), then compute cropWidth =
clamp(rawCropWidth, MIN_CROP, 1 - cropX) and cropHeight = clamp(rawCropHeight,
MIN_CROP, 1 - cropY) (or use Math.max(1 - cropX, MIN_CROP) as the upper bound)
so cropRegion (cropX, cropY, cropWidth, cropHeight) can never normalize to a
zero-sized box.
- Around line 243-253: The normalization currently drops region.zoomInDurationMs
and region.zoomOutDurationMs; update the object returned in the
region-normalization block in src/shared/project-schema.ts (the code used by
normalizeProjectEditor() / createProjectSnapshot()) to include zoomInDurationMs
and zoomOutDurationMs so a load/save round-trip preserves them—set each to the
source value when it's a finite number (e.g., via
isFiniteNumber(region.zoomInDurationMs) ? region.zoomInDurationMs : undefined)
or to your established default if you have one, mirroring the pattern used for
focus and depth.
---
Duplicate comments:
In `@cli/src/commands/annotate.ts`:
- Around line 17-23: The CLI option parsers in annotate.ts use
parseInt/parseFloat which can return NaN and do not enforce ranges before
calling addAnnotationRegion; add explicit validation for each parsed value
(start, end, positionX, positionY, width, height, fontSize) right after parsing:
check for NaN and for required ranges (start/end >= 0, start < end,
positionX/positionY in 0-100, width/height > 0 and <=100 if applicable, fontSize
> 0), and if any check fails throw or call program.error with a clear message so
the process fails fast instead of storing invalid values in
addAnnotationRegion().
In `@cli/src/commands/project.ts`:
- Around line 116-118: The parseBool function currently coerces any unrecognized
string to false; change it to perform strict validation in parseBool(value:
string): boolean by accepting only explicit true literals ("true","1","yes") and
explicit false literals ("false","0","no") (case-insensitive), returning the
corresponding boolean for those, and throwing a clear Error (or CLI-specific
error) when the input does not match any allowed token so invalid flags like
"treu" or "TRUE" are rejected instead of silently flipping the value.
In `@cli/src/commands/zoom.ts`:
- Around line 11-16: The CLI options currently use parseInt/parseFloat but don't
validate values before calling addZoomRegion; add explicit checks right after
parsing the options in zoom.ts to reject NaN and out-of-range values: ensure
start and end are finite numbers (Number.isFinite) and start < end, ensure depth
is an integer between 1 and 6, ensure focus-x and focus-y are numbers between 0
and 1 inclusive, and ensure focus-mode is one of "manual" or "auto"; if any
check fails, print a clear error and exit non-zero (or throw) before invoking
addZoomRegion so invalid values never get written.
In `@cli/src/core/electron-bridge.ts`:
- Around line 60-65: The code calls outputError(...) when absOutput exists but
then returns normally, letting runExport resolve successfully; change this to
reject so callers see failure: after fs.accessSync detects the existing file (in
the branch where overwrite is false) replace the plain return with throwing an
Error (e.g., throw new Error(`Output file already exists: ${absOutput}`)) or
return Promise.reject(...) so runExport (and callers) get a rejected promise;
the change should be made in the same block that references overwrite,
fs.accessSync, absOutput, and outputError.
- Around line 206-208: The catch block currently logs export failures using
outputError(...) but swallows the error; update the catch in the export flow
(the try/catch surrounding outputError) to re-throw the original error after
logging (e.g., outputError(...); throw e;) so callers of the function (the
export routine that invokes outputError) receive the failure and automation can
detect it; ensure you preserve the original Error object when re-throwing rather
than throwing a new string.
In `@electron/main.ts`:
- Around line 477-480: The app.whenReady() branch should guard the
runCliExport() call so loadURL()/loadFile() rejections produce the structured
CLI error envelope; wrap the await runCliExport() call in a try/catch inside the
app.whenReady() handler, catch any error thrown by runCliExport(), serialize it
into the same "__cli" error line used by other CLI paths (emit the structured
CLI error envelope), then ensure the process exits or returns the same way as a
normal CLI error path. Target the runCliExport() invocation in the
app.whenReady() block and mirror the existing CLI error emission/exit behavior.
- Around line 419-424: handleCliExit can run before safetyTimer is initialized
and throw a TDZ ReferenceError; change it so it safely clears the timer (and/or
avoids TDZ) by ensuring safetyTimer is declared/initialized before possible
calls or by checking its existence before calling clearTimeout inside
handleCliExit (referencing the symbols safetyTimer and handleCliExit and the
exiting flag) — e.g., declare safetyTimer with an explicit undefined initial
value or guard clearTimeout(safetyTimer) with a truthy check so clearTimeout is
only invoked when safetyTimer is set.
In `@src/shared/project-schema.ts`:
- Around line 177-180: The regex in deriveNextId currently interpolates raw
prefix into new RegExp, which misbehaves for prefixes containing regex
metacharacters; to fix, escape prefix before building the RegExp (e.g., replace
metacharacters via prefix.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') or call a
shared escape helper) and then use the escapedPrefix when constructing new
RegExp in deriveNextId so ids.match() only treats prefix as a literal.
In `@src/shared/types.ts`:
- Around line 227-237: clampFocusToDepth currently ignores the depth parameter
and always clamps to [0,1]; change it to use the half-viewport implied by
ZOOM_DEPTH_SCALES[depth] so the focus cannot move past the image edges: compute
halfViewport = ZOOM_DEPTH_SCALES[depth] / 2 (or the appropriate half-size of the
visible viewport for your scale) and clamp focus.cx and focus.cy to
[halfViewport, 1 - halfViewport]; update clampFocusToDepth to call a clamping
helper that accepts min/max (or inline the clamp logic) instead of the existing
clampFocusValue so the depth is respected.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84e9226b-22d5-4a88-ac40-113599d3d6a2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
cli/skills/SKILL.mdcli/skills/rules/annotations.mdcli/skills/rules/export-render.mdcli/skills/rules/project-setup.mdcli/skills/rules/trim-speed.mdcli/skills/rules/troubleshooting.mdcli/skills/rules/zoom-regions.mdcli/src/commands/annotate.tscli/src/commands/project.tscli/src/commands/render.tscli/src/commands/shortcuts.tscli/src/commands/speed.tscli/src/commands/trim.tscli/src/commands/zoom.tscli/src/core/electron-bridge.tscli/src/core/project-manager.tscli/src/core/region-manager.tscli/src/index.tscli/src/mcp-server.tscli/src/output.tscli/tsconfig.jsonelectron/ipc/handlers.tselectron/main.tselectron/preload.tspackage.jsonsrc/App.tsxsrc/components/cli-export/CliExportRenderer.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/lib/exporter/types.tssrc/lib/recordingSession.tssrc/lib/shortcuts.tssrc/shared/aspect-ratios.tssrc/shared/export-types.tssrc/shared/index.tssrc/shared/project-schema.tssrc/shared/recording-session.tssrc/shared/shortcuts.tssrc/shared/types.tssrc/utils/aspectRatioUtils.tsvite.config.ts
✅ Files skipped from review due to trivial changes (15)
- vite.config.ts
- src/shared/index.ts
- src/App.tsx
- cli/tsconfig.json
- cli/skills/SKILL.md
- cli/src/mcp-server.ts
- cli/skills/rules/annotations.md
- cli/skills/rules/zoom-regions.md
- cli/skills/rules/troubleshooting.md
- cli/skills/rules/trim-speed.md
- cli/skills/rules/export-render.md
- src/shared/recording-session.ts
- src/components/video-editor/projectPersistence.ts
- cli/skills/rules/project-setup.md
- cli/src/core/region-manager.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- package.json
- cli/src/index.ts
- src/lib/recordingSession.ts
- cli/src/commands/trim.ts
- cli/src/commands/speed.ts
- src/components/video-editor/types.ts
- src/utils/aspectRatioUtils.ts
- cli/src/commands/shortcuts.ts
- src/shared/shortcuts.ts
- src/lib/exporter/types.ts
- src/lib/shortcuts.ts
- cli/src/commands/render.ts
- src/shared/export-types.ts
- cli/src/core/project-manager.ts
- src/shared/aspect-ratios.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
cli/src/commands/shortcuts.ts (1)
42-52:⚠️ Potential issue | 🟠 MajorOnly treat missing file as defaults; don’t swallow parse/read failures.
Line 49 currently catches everything and Line 52 silently returns defaults. lowkey risky: malformed/unreadable
shortcuts.jsongets masked, andshortcuts setcan overwrite user config with defaults + one change.Suggested fix (ENOENT-only fallback)
function loadShortcuts(): ShortcutsConfig { const filePath = getShortcutsPath(); try { - if (fs.existsSync(filePath)) { - const raw = JSON.parse(fs.readFileSync(filePath, "utf-8")); - return mergeWithDefaults(raw); - } - } catch { - // Fall through to defaults + const raw = JSON.parse(fs.readFileSync(filePath, "utf-8")); + return mergeWithDefaults(raw); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return { ...DEFAULT_SHORTCUTS }; + } + throw new Error( + `Failed to load shortcuts from ${filePath}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); } - return { ...DEFAULT_SHORTCUTS }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/shortcuts.ts` around lines 42 - 52, The loadShortcuts function currently swallows all errors when reading/parsing the shortcuts file; change it so only a missing-file (ENOENT) falls back to DEFAULT_SHORTCUTS. Specifically, in loadShortcuts keep the exists check and attempt JSON.parse(fs.readFileSync(...)) and if you still surround with try/catch, rethrow any caught error unless error.code === 'ENOENT' so parse/read failures propagate; otherwise return mergeWithDefaults(raw) or { ...DEFAULT_SHORTCUTS } for ENOENT. Ensure references to loadShortcuts, mergeWithDefaults, and DEFAULT_SHORTCUTS are preserved.cli/src/mcp-server.ts (2)
50-51:⚠️ Potential issue | 🟡 Minor
readCachedswallows failures, which makes debugging docs issues painfullowkey risky for ops: this hides whether reads failed due to missing files vs perms vs encoding. Keep returning
null, but log the failure context first.nit: cleaner error handling while preserving behavior
- } catch { + } catch (err) { + console.debug(`[openscreen-mcp] failed reading ${filePath}:`, err); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/mcp-server.ts` around lines 50 - 51, The readCached function currently swallows all errors in its catch block and returns null; change the catch to capture the error (e.g., catch (err)) and log meaningful context (filename/path being read, operation name, and err.message/err.stack) using the project logger if available (e.g., processLogger.error) or console.error, then continue returning null to preserve existing behavior; reference the readCached function and its existing return null so you only add logging before the return.
59-61:⚠️ Potential issue | 🟡 MinorKeyword matching is too greedy (
includes) and can route to wrong docs
includeswill match inside larger words (e.g.contexthitstext), so help results can drift. Word-boundary matching is safer here.use escaped, boundary-aware matching
+function escapeRegExp(input: string): string { + return input.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + function loadSkillContent(query: string): string { - const q = query.toLowerCase(); + const q = query.trim().toLowerCase(); const matchedFiles = new Set<string>(); for (const [keyword, file] of Object.entries(RULE_MAPPING)) { - if (q.includes(keyword)) { + const keywordRegex = new RegExp(`\\b${escapeRegExp(keyword)}\\b`, "i"); + if (keywordRegex.test(q)) { matchedFiles.add(file); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/mcp-server.ts` around lines 59 - 61, The current keyword check using q.includes(keyword) in the loop over RULE_MAPPING is too permissive and matches substrings (e.g., "text" in "context"); replace it with a boundary-aware, escaped regex check: for each keyword from RULE_MAPPING build an escaped RegExp with word boundaries (e.g., new RegExp(`\\b${escapeRegExp(keyword)}\\b`, 'i')) and test that against q before adding to matchedFiles; add an escapeRegExp helper if one doesn't exist to safely escape regex metacharacters and keep case-insensitive matching if desired.cli/src/commands/render.ts (1)
6-24:⚠️ Potential issue | 🟡 MinorrenderCommand doesn't validate
--qualityunlike gifCommand
--quality badgets passed through silently.normalizeProjectEditorwould default it to"good", but that's kinda cursed for CLI UX — user thinks they set something that got ignored.gifCommand does validate its options (frame-rate, size-preset). renderCommand should probably do the same for consistency.
🛡️ quick fix for quality validation
+const VALID_QUALITIES = ["medium", "good", "source"] as const; + export const renderCommand = new Command("render") .description("Render project as MP4 video") .requiredOption("--project <path>", "Path to .openscreen project file") .requiredOption("--output <path>", "Output MP4 file path") .option("--quality <q>", "Export quality (medium, good, source)", "good") .option("--overwrite", "Overwrite existing output file") .action(async (opts) => { try { + if (!VALID_QUALITIES.includes(opts.quality)) { + outputError(`Invalid quality: ${opts.quality}. Valid values: ${VALID_QUALITIES.join(", ")}`); + return; + } await runExport({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/render.ts` around lines 6 - 24, renderCommand currently accepts any value for --quality and silently passes invalid values to runExport; add explicit validation for opts.quality before calling runExport (similar to gifCommand's option validation). Check that opts.quality is one of "medium", "good", or "source" and if not, call outputError with a clear message (or throw) and return early; update the validation logic where renderCommand.action is defined so invalid input is rejected before invoking runExport.
🧹 Nitpick comments (4)
cli/src/commands/render.ts (1)
26-26: could import GIF_SIZE_PRESETS from shared instead of local copy
VALID_GIF_PRESETSduplicates what's already exported asGIF_SIZE_PRESETSfromsrc/shared/export-types.ts. not blocking, just a consistency nit similar to the resolved frame-rate import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/render.ts` at line 26, Replace the local duplicate VALID_GIF_PRESETS with the shared export GIF_SIZE_PRESETS: remove the const VALID_GIF_PRESETS declaration and add an import for GIF_SIZE_PRESETS (the same exported constant used elsewhere) then use GIF_SIZE_PRESETS wherever VALID_GIF_PRESETS was referenced to keep a single source of truth and consistent types.src/shared/project-schema.ts (1)
357-360: style object spread is permissive — could pass invalid types throughthe spread here doesn't validate individual style properties. if
region.style.fontSizeis a string like"big", it passes through unchecked. downstream code expectsnumberfor fontSize perAnnotationTextStyleinterface.lowkey fine for backward compat, but you might want property-level validation similar to how you handle
blurData.intensitybelow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/project-schema.ts` around lines 357 - 360, The style spread merges region.style into DEFAULT_ANNOTATION_STYLE permissively and can let invalid types (e.g., region.style.fontSize as "big") slip through; update the code around the style merge to validate and coerce each AnnotationTextStyle property (at least fontSize, color, fontWeight, etc.) similar to how blurData.intensity is handled: check typeof each property on region.style, convert or fall back to DEFAULT_ANNOTATION_STYLE values, and only then merge into the final style object; reference DEFAULT_ANNOTATION_STYLE, region.style, and the AnnotationTextStyle properties when implementing the per-property validation/coercion.electron/main.ts (1)
436-442:webSecurity: falseis a bit spicy for headless exportneeded for file:// protocol access, but disables same-origin policy entirely. since this is CLI-only with a hidden window and only processes local files, the attack surface is limited. just noting it here.
if you want to tighten this later, you could use a custom protocol handler instead of disabling webSecurity entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/main.ts` around lines 436 - 442, Change the insecure webPreferences entry (webSecurity: false) to true and implement a secure custom protocol for local file access instead of disabling webSecurity: use Electron's protocol.registerFileProtocol in app.whenReady (or registerSchemesAsPrivileged at startup) to map a safe scheme (e.g., app-file://) to local files, update window creation to load URLs via that scheme (where createWindow / loadURL is used), and keep preload (preload.mjs) and contextIsolation/nodeIntegration settings unchanged; this removes webSecurity: false while preserving file:// access via protocol.registerFileProtocol and related registration code.src/shared/types.ts (1)
201-212: kinda cursed: SPEED_OPTIONS lets users pick 3×/4×/5× but CLI rejects themSPEED_OPTIONS here includes speeds up to 5×, but
cli/src/core/region-manager.tsvalidates against VALID_SPEEDS which is[0.25, 0.5, 0.75, 1.25, 1.5, 1.75, 2]— no 3×, 4×, or 5×.Users clicking those options in the GUI settings panel will hit a validation error if the value reaches the backend. worth syncing these: either remove 3/4/5 from SPEED_OPTIONS or add them to VALID_SPEEDS (and update the CLI command help text at
cli/src/commands/speed.ts:15too).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types.ts` around lines 201 - 212, SPEED_OPTIONS exposes 3×/4×/5× but the backend validator VALID_SPEEDS doesn't include those, causing GUI selections to be rejected; either remove the 3, 4, 5 entries from SPEED_OPTIONS or add 3, 4, 5 to VALID_SPEEDS and update the CLI help text for the speed command accordingly (update the help/usage string referenced in the speed command) so the UI, backend validator (VALID_SPEEDS), and CLI help are consistent; locate SPEED_OPTIONS and VALID_SPEEDS by name and ensure the speed command's help text is updated to reflect the final allowed speeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/core/region-manager.ts`:
- Around line 26-30: validateTimeRange allows NaN inputs to pass validation;
update validateTimeRange(startMs: number, endMs: number) to explicitly reject
NaN by checking Number.isNaN(startMs) and Number.isNaN(endMs) and throw a clear
Error (e.g., "startMs must be a valid number" / "endMs must be a valid number")
before the existing negative/ordering checks so parseInt failures don't produce
corrupted region data.
In `@cli/src/mcp-server.ts`:
- Line 109: The current top-level invocation main().catch(console.error) logs
startup errors but leaves the process exit code at zero; update the catch
handler where main() is invoked so it logs the error (using console.error or
existing logger) and then calls process.exit(1) to ensure failures return a
non-zero exit status; locate the main() invocation in cli/src/mcp-server.ts and
change the promise rejection handler to both log the error and call
process.exit(1).
- Around line 72-79: If all matched files fail to load (sections remains empty),
return a clear fallback string instead of an empty body: after the loop that
builds sections (using matchedFiles and readCached) check if sections.length ===
0 and return a deterministic fallback such as a descriptive message like "No
matching rules could be loaded" (or include the list of matchedFiles names)
before the final return; modify the code around the sections join and return to
implement this fallback so callers never receive an empty string.
---
Duplicate comments:
In `@cli/src/commands/render.ts`:
- Around line 6-24: renderCommand currently accepts any value for --quality and
silently passes invalid values to runExport; add explicit validation for
opts.quality before calling runExport (similar to gifCommand's option
validation). Check that opts.quality is one of "medium", "good", or "source" and
if not, call outputError with a clear message (or throw) and return early;
update the validation logic where renderCommand.action is defined so invalid
input is rejected before invoking runExport.
In `@cli/src/commands/shortcuts.ts`:
- Around line 42-52: The loadShortcuts function currently swallows all errors
when reading/parsing the shortcuts file; change it so only a missing-file
(ENOENT) falls back to DEFAULT_SHORTCUTS. Specifically, in loadShortcuts keep
the exists check and attempt JSON.parse(fs.readFileSync(...)) and if you still
surround with try/catch, rethrow any caught error unless error.code === 'ENOENT'
so parse/read failures propagate; otherwise return mergeWithDefaults(raw) or {
...DEFAULT_SHORTCUTS } for ENOENT. Ensure references to loadShortcuts,
mergeWithDefaults, and DEFAULT_SHORTCUTS are preserved.
In `@cli/src/mcp-server.ts`:
- Around line 50-51: The readCached function currently swallows all errors in
its catch block and returns null; change the catch to capture the error (e.g.,
catch (err)) and log meaningful context (filename/path being read, operation
name, and err.message/err.stack) using the project logger if available (e.g.,
processLogger.error) or console.error, then continue returning null to preserve
existing behavior; reference the readCached function and its existing return
null so you only add logging before the return.
- Around line 59-61: The current keyword check using q.includes(keyword) in the
loop over RULE_MAPPING is too permissive and matches substrings (e.g., "text" in
"context"); replace it with a boundary-aware, escaped regex check: for each
keyword from RULE_MAPPING build an escaped RegExp with word boundaries (e.g.,
new RegExp(`\\b${escapeRegExp(keyword)}\\b`, 'i')) and test that against q
before adding to matchedFiles; add an escapeRegExp helper if one doesn't exist
to safely escape regex metacharacters and keep case-insensitive matching if
desired.
---
Nitpick comments:
In `@cli/src/commands/render.ts`:
- Line 26: Replace the local duplicate VALID_GIF_PRESETS with the shared export
GIF_SIZE_PRESETS: remove the const VALID_GIF_PRESETS declaration and add an
import for GIF_SIZE_PRESETS (the same exported constant used elsewhere) then use
GIF_SIZE_PRESETS wherever VALID_GIF_PRESETS was referenced to keep a single
source of truth and consistent types.
In `@electron/main.ts`:
- Around line 436-442: Change the insecure webPreferences entry (webSecurity:
false) to true and implement a secure custom protocol for local file access
instead of disabling webSecurity: use Electron's protocol.registerFileProtocol
in app.whenReady (or registerSchemesAsPrivileged at startup) to map a safe
scheme (e.g., app-file://) to local files, update window creation to load URLs
via that scheme (where createWindow / loadURL is used), and keep preload
(preload.mjs) and contextIsolation/nodeIntegration settings unchanged; this
removes webSecurity: false while preserving file:// access via
protocol.registerFileProtocol and related registration code.
In `@src/shared/project-schema.ts`:
- Around line 357-360: The style spread merges region.style into
DEFAULT_ANNOTATION_STYLE permissively and can let invalid types (e.g.,
region.style.fontSize as "big") slip through; update the code around the style
merge to validate and coerce each AnnotationTextStyle property (at least
fontSize, color, fontWeight, etc.) similar to how blurData.intensity is handled:
check typeof each property on region.style, convert or fall back to
DEFAULT_ANNOTATION_STYLE values, and only then merge into the final style
object; reference DEFAULT_ANNOTATION_STYLE, region.style, and the
AnnotationTextStyle properties when implementing the per-property
validation/coercion.
In `@src/shared/types.ts`:
- Around line 201-212: SPEED_OPTIONS exposes 3×/4×/5× but the backend validator
VALID_SPEEDS doesn't include those, causing GUI selections to be rejected;
either remove the 3, 4, 5 entries from SPEED_OPTIONS or add 3, 4, 5 to
VALID_SPEEDS and update the CLI help text for the speed command accordingly
(update the help/usage string referenced in the speed command) so the UI,
backend validator (VALID_SPEEDS), and CLI help are consistent; locate
SPEED_OPTIONS and VALID_SPEEDS by name and ensure the speed command's help text
is updated to reflect the final allowed speeds.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c20f83f-34b8-42d3-a689-b047e0ed5dfa
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
cli/skills/SKILL.mdcli/skills/rules/annotations.mdcli/skills/rules/export-render.mdcli/skills/rules/project-setup.mdcli/skills/rules/trim-speed.mdcli/skills/rules/troubleshooting.mdcli/skills/rules/zoom-regions.mdcli/src/commands/annotate.tscli/src/commands/project.tscli/src/commands/render.tscli/src/commands/shortcuts.tscli/src/commands/speed.tscli/src/commands/trim.tscli/src/commands/zoom.tscli/src/core/electron-bridge.tscli/src/core/project-manager.tscli/src/core/region-manager.tscli/src/index.tscli/src/mcp-server.tscli/src/output.tscli/tsconfig.jsonelectron/ipc/handlers.tselectron/main.tselectron/preload.tspackage.jsonsrc/App.tsxsrc/components/cli-export/CliExportRenderer.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/lib/exporter/types.tssrc/lib/recordingSession.tssrc/lib/shortcuts.tssrc/shared/aspect-ratios.tssrc/shared/export-types.tssrc/shared/index.tssrc/shared/project-schema.tssrc/shared/recording-session.tssrc/shared/shortcuts.tssrc/shared/types.tssrc/utils/aspectRatioUtils.tsvite.config.ts
✅ Files skipped from review due to trivial changes (14)
- vite.config.ts
- src/App.tsx
- cli/tsconfig.json
- src/shared/index.ts
- cli/skills/rules/project-setup.md
- cli/skills/SKILL.md
- cli/skills/rules/annotations.md
- cli/skills/rules/zoom-regions.md
- cli/skills/rules/trim-speed.md
- cli/skills/rules/troubleshooting.md
- cli/skills/rules/export-render.md
- src/shared/shortcuts.ts
- src/shared/recording-session.ts
- src/shared/aspect-ratios.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- electron/preload.ts
- electron/ipc/handlers.ts
- cli/src/index.ts
- package.json
- src/lib/recordingSession.ts
- cli/src/commands/speed.ts
- src/lib/shortcuts.ts
- cli/src/commands/zoom.ts
- src/components/video-editor/projectPersistence.ts
- cli/src/output.ts
- src/components/video-editor/types.ts
Comment on lines
+26
to
+30
| function validateTimeRange(startMs: number, endMs: number): void { | ||
| if (startMs < 0) throw new Error(`startMs must be >= 0, got ${startMs}`); | ||
| if (endMs <= startMs) | ||
| throw new Error(`endMs (${endMs}) must be greater than startMs (${startMs})`); | ||
| } |
Contributor
There was a problem hiding this comment.
NaN inputs slip through validation silently.
validateTimeRange doesn't check for NaN. Since the CLI uses parseInt which returns NaN on bad input, and NaN < 0 evaluates to false, invalid time inputs will pass validation and produce corrupted region data. The subsequent endMs <= startMs check also returns false when either operand is NaN.
🛡️ Proposed fix: reject NaN values
function validateTimeRange(startMs: number, endMs: number): void {
+ if (!Number.isFinite(startMs) || !Number.isFinite(endMs)) {
+ throw new Error(`startMs and endMs must be finite numbers, got startMs=${startMs}, endMs=${endMs}`);
+ }
if (startMs < 0) throw new Error(`startMs must be >= 0, got ${startMs}`);
if (endMs <= startMs)
throw new Error(`endMs (${endMs}) must be greater than startMs (${startMs})`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/core/region-manager.ts` around lines 26 - 30, validateTimeRange
allows NaN inputs to pass validation; update validateTimeRange(startMs: number,
endMs: number) to explicitly reject NaN by checking Number.isNaN(startMs) and
Number.isNaN(endMs) and throw a clear Error (e.g., "startMs must be a valid
number" / "endMs must be a valid number") before the existing negative/ordering
checks so parseInt failures don't produce corrupted region data.
Comment on lines
+72
to
+79
| const sections: string[] = []; | ||
| for (const file of matchedFiles) { | ||
| const content = readCached(path.join(rulesDir, file)); | ||
| if (content) sections.push(content); | ||
| } | ||
|
|
||
| return sections.join("\n\n---\n\n"); | ||
| } |
Contributor
There was a problem hiding this comment.
If matched files fail to load, tool can return an empty string
kinda cursed failure mode: keyword match succeeds, file reads fail, response body becomes blank. Add a fallback when sections.length === 0.
fallback when no section content is available
const sections: string[] = [];
for (const file of matchedFiles) {
const content = readCached(path.join(rulesDir, file));
if (content) sections.push(content);
}
- return sections.join("\n\n---\n\n");
+ if (sections.length === 0) {
+ return (
+ readCached(path.join(skillsDir, "SKILL.md")) ??
+ "OpenScreen CLI help. Use `openscreen --help` to see all commands."
+ );
+ }
+
+ return sections.join("\n\n---\n\n");
}📝 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
| const sections: string[] = []; | |
| for (const file of matchedFiles) { | |
| const content = readCached(path.join(rulesDir, file)); | |
| if (content) sections.push(content); | |
| } | |
| return sections.join("\n\n---\n\n"); | |
| } | |
| const sections: string[] = []; | |
| for (const file of matchedFiles) { | |
| const content = readCached(path.join(rulesDir, file)); | |
| if (content) sections.push(content); | |
| } | |
| if (sections.length === 0) { | |
| return ( | |
| readCached(path.join(skillsDir, "SKILL.md")) ?? | |
| "OpenScreen CLI help. Use `openscreen --help` to see all commands." | |
| ); | |
| } | |
| return sections.join("\n\n---\n\n"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/mcp-server.ts` around lines 72 - 79, If all matched files fail to
load (sections remains empty), return a clear fallback string instead of an
empty body: after the loop that builds sections (using matchedFiles and
readCached) check if sections.length === 0 and return a deterministic fallback
such as a descriptive message like "No matching rules could be loaded" (or
include the list of matchedFiles names) before the final return; modify the code
around the sections join and return to implement this fallback so callers never
receive an empty string.
| await server.connect(transport); | ||
| } | ||
|
|
||
| main().catch(console.error); |
Contributor
There was a problem hiding this comment.
Startup failures should exit non-zero for automation
Right now failures are logged, but process can still exit successfully. For agent/CI usage, set a non-zero exit code in the catch path.
make failure status explicit
-main().catch(console.error);
+main().catch((err) => {
+ console.error(err);
+ process.exitCode = 1;
+});📝 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
| main().catch(console.error); | |
| main().catch((err) => { | |
| console.error(err); | |
| process.exitCode = 1; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/mcp-server.ts` at line 109, The current top-level invocation
main().catch(console.error) logs startup errors but leaves the process exit code
at zero; update the catch handler where main() is invoked so it logs the error
(using console.error or existing logger) and then calls process.exit(1) to
ensure failures return a non-zero exit status; locate the main() invocation in
cli/src/mcp-server.ts and change the promise rejection handler to both log the
error and call process.exit(1).
New review round on the force-pushed branch plus four parallel review
sub-agents (simplify, security, codex, code-review) surfaced a mix of
real bugs, hardening opportunities, and doc drift. All in scope here.
Argument parsing (cli/src/cli-parsers.ts — new file)
- New validating parsers (parseIntArg, parseFloatArg, parseIntInRange,
parseBoolStrict) that reject NaN/junk input up-front instead of
silently coercing. parseFloatArg now uses a strict regex so `"10%"`
and `"0.5foo"` are rejected — Number.parseFloat alone would have
happily returned 10 / 0.5 and defeated the validation this round is
adding. parseIntArg accepts `"+5"` / `"-0"`.
- Wired into zoom/trim/speed/annotate/project commands — replaces the
raw parseInt / parseFloat / ad-hoc parseBool that previously let NaN
and unknown boolean literals propagate into the schema layer.
- zoom --depth now enforced to [1,6] at parse time.
Output streams (cli/src/output.ts + cli/skills/rules/export-render.md)
- outputProgress in --json mode now routes to stderr, not stdout, so
stdout is a single parseable JSON envelope (the terminal done/error
line). Agents can `openscreen --json render ... | jq .` directly.
Documented the new contract in export-render.md including the
failure case (stdout empty, error on stderr, non-zero exit).
- outputTable normalizes ragged rows (short rows padded with empty
strings so the JSON branch doesn't produce undefined fields).
CliExportRenderer (src/components/cli-export/CliExportRenderer.tsx)
- Explicit __cli error envelope when URL query lacks configPath — the
CLI side previously sat on its 10-minute safetyTimer in that case.
- New validateCliExportConfig runs after JSON.parse and before casts:
checks shape of project/media/editor, format ∈ mp4|gif, quality enum,
and gif-specific enum ranges (gifFrameRate ∈ {15,20,25,30},
gifSizePreset ∈ medium|large|original).
- New pickSupportedH264Codec probes a fallback list (avc1.640033 →
avc1.64001F → avc1.42E01E) via VideoEncoder.isConfigSupported with
a runtime guard for `typeof VideoEncoder === "undefined"` so old
Electron builds fail with a clear error.
- Dropped dead `videoPadding` field from the GifExporter call — it
was never read.
Shortcuts handling (cli/src/commands/shortcuts.ts + shared/shortcuts.ts)
- loadShortcuts now surfaces malformed JSON / read failures so
`shortcuts get` can't silently lie about active bindings.
- New loadShortcutsOrDefaults used by `shortcuts set` falls back to
defaults (with a stderr warning) if the file is corrupt, keeping
the self-healing path alive so users can overwrite a bad file.
- mergeWithDefaults in src/shared/shortcuts.ts now deep-clones bindings
so mutating a merged value can't poison DEFAULT_SHORTCUTS.
Shared schema / utilities (src/shared/*)
- project-schema.ts:
* normalizeProjectEditor preserves zoomInDurationMs / zoomOutDurationMs
on round-trip (previously dropped, so load→save silently erased them).
* cropRegion min-size invariant (MIN_CROP = 0.01) — x/y clamped to
[0, 1 - MIN_CROP] so cropX=1 can no longer produce a zero-sized box.
* deriveNextId escapes regex metacharacters in the prefix (ReDoS nit
from static analysis; also hardens for future dynamic callers).
- aspect-ratios.ts: getNativeAspectRatioValue now falls back to 16:9
when the math is degenerate (videoHeight/cropH = 0) instead of
returning Infinity and poisoning downstream layout math.
Other bug fixes
- cli/src/core/electron-bridge.ts: stderr error match is now
case-insensitive (/\b(error|fail|fatal)\b/i) so "ERROR:" vs "Error:"
vs "error:" from Chromium all surface consistently.
- cli/src/core/project-manager.ts: saveProject cleans up the orphaned
.tmp file if renameSync fails (antivirus, file lock, ENOSPC, etc.).
- cli/src/core/region-manager.ts: annotation zIndex now uses
max(existing) + 1 instead of length + 1 so removing a middle region
and adding a new one can't produce two annotations at the same
z-level.
Tests (src/components/video-editor/projectPersistence.test.ts, +10)
- normalizeProjectEditor: preserves zoom durations; omits them when
absent; enforces MIN_CROP at x=y=1; passes valid crop through
unchanged.
- deriveNextId: empty list; consecutive ids; ignores other prefixes;
escapes regex metacharacters in the prefix.
- mergeWithDefaults: returned bindings are fresh objects (mutation
doesn't poison DEFAULT_SHORTCUTS); partial overrides still resolve
other actions from defaults as fresh copies.
Verification
- tsc --noEmit (root + cli): 0 errors
- biome check: 0 errors (2 pre-existing warnings unchanged)
- vitest --run --exclude '**/*.browser.test.*': 50/50 pass
(was 40 — 10 new tests, all new pure-function invariants)
- End-to-end: full headless MP4 render on the 47s AV1 test-yc.openscreen
project — stdout is exactly one JSON line, stderr streams progress,
resulting file is a valid ftypisom MP4
- Manual: shortcuts self-heal flow (corrupt file → `shortcuts get`
errors, `shortcuts set` warns + fixes); strict parser rejects "10%"
and "0.5foo" on --focus-x; parseIntArg accepts "+100"
Review sources: CodeRabbit round 3 (6 Major + 1 Minor on the 7 files
the user highlighted), Codex /review (2 P2s: parseFloatArg strictness
+ shortcuts self-heal), code-review sub-agent (15 numbered findings,
all Medium/Low), security sub-agent (no findings on the diff),
code-simplifier sub-agent (5 applied in-place).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The CLI previously had no human-facing docs — just the SKILL.md +
rule files for agents. A user cloning the repo had no clear path to
"how do I install this and run my first command". Fixes that.
- cli/bin/openscreen.mjs: thin shim that resolves the package root,
finds `node_modules/.bin/tsx` and `cli/src/index.ts`, and forwards
argv. Clear error messages if either is missing. Works for both
direct invocation and `npm link`.
- package.json: add "bin": { "openscreen": "cli/bin/openscreen.mjs" }.
After `npm link` from the repo root, `openscreen` is available
globally on PATH — end-to-end tested including a full headless
MP4 render from outside the repo cwd.
- cli/README.md: full setup + usage guide. Covers the two-tier
architecture (pure Node.js vs headless Electron), three invocation
styles (`npm run cli --`, direct shim, `npm link`), quick start
walk-through from a real recording, command reference, JSON output
contract (including the stdout/stderr split for render progress),
how to wire it into Claude Code / Cursor / MCP, troubleshooting
pointers, and a contributor guide for adding new commands.
- README.md: short "CLI & Agent Interface" section pointing at
cli/README.md and cli/skills/SKILL.md. Keeps the root README
GUI-focused but discoverable for users who want the programmatic
surface.
Note: `npm i -g openscreen` is not yet wired — it would require
moving tsx from devDependencies into runtime dependencies (or
compiling the CLI to JS at publish time). Documented as a follow-up
in cli/README.md. `npm link` from a local clone is the current
supported install path.
Verification
- tsc --noEmit (root + cli): 0 errors
- biome check: 0 errors (2 pre-existing warnings unchanged)
- vitest: 50/50
- `npm link` → `which openscreen` → `/Users/.../bin/openscreen`
- `openscreen --version` → 1.3.0
- `openscreen --json project info` from /tmp: returns valid JSON
- `openscreen --json render` from /tmp: 30.9 MB MP4, stdout = single
success envelope, stderr = progress ticks
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/cli-export/CliExportRenderer.tsx (1)
76-85:⚠️ Potential issue | 🟡 MinorValidate
project.media.screenVideoPathexplicitly.Right now any object passes
project.media, so a truncated temp config can slip through and then blow up later intoFileUrl(media.screenVideoPath)with a generic.replaceerror. Since this path is agent-facing, it’d be nicer to fail here with a proper config envelope.nit: tighten the config guard a bit more
const project = c.project as Record<string, unknown>; if (!project.media || typeof project.media !== "object") { throw new Error("CLI export config: missing 'project.media'"); } + const media = project.media as Record<string, unknown>; + if ( + typeof media.screenVideoPath !== "string" || + media.screenVideoPath.trim().length === 0 + ) { + throw new Error( + "CLI export config: 'project.media.screenVideoPath' must be a non-empty string", + ); + } + if ( + media.webcamVideoPath !== undefined && + (typeof media.webcamVideoPath !== "string" || + media.webcamVideoPath.trim().length === 0) + ) { + throw new Error( + "CLI export config: 'project.media.webcamVideoPath' must be a non-empty string when provided", + ); + } if (!project.editor || typeof project.editor !== "object") { throw new Error("CLI export config: missing 'project.editor'"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/cli-export/CliExportRenderer.tsx` around lines 76 - 85, The config validation currently only checks that c.project, project.media and project.editor are objects, allowing a missing screenVideoPath to surface later as a cryptic error; update the validation in CliExportRenderer to explicitly check that project.media.screenVideoPath exists and is a non-empty string (e.g., typeof project.media.screenVideoPath === "string" && project.media.screenVideoPath.trim() !== "") and throw a clear Error like "CLI export config: missing 'project.media.screenVideoPath'" so callers fail fast before toFileUrl(media.screenVideoPath) is invoked.cli/src/core/electron-bridge.ts (1)
59-68:⚠️ Potential issue | 🟠 Major
runExport()still resolves after fatal errors.Both fatal paths here only emit
outputError()and then fall through, so the exportedPromise<void>can report success even when the export failed. that’s kinda cursed for anything callingrunExport()as a core API.Also applies to: 209-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/electron-bridge.ts` around lines 59 - 68, runExport currently calls outputError on fatal conditions (e.g., existing absOutput when !overwrite and the other fatal branch later) but then falls through allowing the returned Promise to resolve; modify runExport so that after calling outputError it immediately aborts the operation by throwing an Error (or returning a rejected Promise) with a clear message (include absOutput in the message), and apply the same change to the other fatal path that currently only calls outputError so callers receive a rejected Promise instead of a false success.
🧹 Nitpick comments (4)
cli/src/core/project-manager.ts (3)
160-175: createProject doesn't validate option ranges like editProject does
editProjecthasvalidateEditOptionsthat rejects--padding 200with an error, butcreateProjectjust passes options straight tonormalizeProjectEditorwhich silently clamps. if a user runsopenscreen project create --padding 200, they'll get 100 with no warning.for consistency, might want to add similar validation here, or extract a shared validator for the overlapping options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/project-manager.ts` around lines 160 - 175, createProject currently builds editorOverrides and calls normalizeProjectEditor without range validation, so out-of-range values (e.g., --padding 200) get silently clamped; call the same validation used by editProject (validateEditOptions) or extract a shared validator and run it on options before constructing editorOverrides and calling normalizeProjectEditor (validate the overlapping fields like padding, borderRadius, shadowIntensity, motionBlurAmount, exportQuality and exportFormat), and if validation fails throw/report the error so createProject rejects invalid inputs instead of silently clamping.
265-275:mediareturn type saysProjectMedia | undefinedbut it can't be undefined
loadProjectthrows ifresolveProjectMediareturns falsy (line 81-85), soproject.mediawill always be defined here. the return type could just beProjectMediafor accuracy.lowkey doesn't matter since consumers will handle both, but the types don't reflect reality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/project-manager.ts` around lines 265 - 275, The returned type for media in inspectProject is incorrect: because loadProject throws when resolveProjectMedia returns falsy, project.media is guaranteed to be present; update the inspectProject signature to return media: ProjectMedia (not ProjectMedia | undefined) and adjust any dependent code or types that assumed undefined. Locate inspectProject in project-manager.ts and update its return type, ensuring references to loadProject and resolveProjectMedia remain consistent with the non-optional media type.
190-235: some editable fields aren't validated
borderRadiusandshadowIntensitypass through unvalidated whilepaddingandmotionBlurAmountget checked. if there are valid ranges for these in the normalizer, might be worth adding them here for consistency. not blocking tho - the normalizer will clamp them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/project-manager.ts` around lines 190 - 235, validateEditOptions currently validates padding, motionBlurAmount, aspectRatio, exportQuality/Format, gifFrameRate and gifSizePreset but ignores borderRadius and shadowIntensity; add validation checks in validateEditOptions for edits.borderRadius and edits.shadowIntensity using the same allowed ranges or constants used by the normalizer (or the exact numeric ranges the normalizer clamps to), and throw descriptive Errors on invalid values (e.g., `Invalid borderRadius: ...` and `Invalid shadowIntensity: ...`) so inputs are rejected consistently before normalization; reference the same constants or range logic from the normalizer to keep behavior aligned.cli/src/core/region-manager.ts (1)
199-202:contentis stored twice for text/image typesyou've got
content: opts.contentplus eithertextContentorimageContentset to the same value. probably intentional for backwards compat or different consumers, but it's a bit redundant. just noting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/core/region-manager.ts` around lines 199 - 202, The object currently stores opts.content twice: once as content and again as textContent/imageContent; update the construction so you don't duplicate values — set content: opts.content only for non-"text"/"image" types (or remove content and keep only textContent/imageContent for explicit types) and keep textContent: opts.content when opts.type === "text" and imageContent: opts.content when opts.type === "image" (adjust callers if you remove the generic content). Ensure you modify the code that builds this object (references: opts.type, opts.content, textContent, imageContent) so consumers get a single authoritative field per type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shared/project-schema.ts`:
- Around line 193-199: validateProjectData currently accepts any numeric
version; tighten the version gate to reject unknown future versions by
explicitly allowing only supported versions (the legacy v1 shape and the current
PROJECT_VERSION). Update the check in validateProjectData (the project.version
test) to only return true when project.version === 1 or project.version ===
PROJECT_VERSION, ensuring unknown numeric versions are rejected up front.
---
Duplicate comments:
In `@cli/src/core/electron-bridge.ts`:
- Around line 59-68: runExport currently calls outputError on fatal conditions
(e.g., existing absOutput when !overwrite and the other fatal branch later) but
then falls through allowing the returned Promise to resolve; modify runExport so
that after calling outputError it immediately aborts the operation by throwing
an Error (or returning a rejected Promise) with a clear message (include
absOutput in the message), and apply the same change to the other fatal path
that currently only calls outputError so callers receive a rejected Promise
instead of a false success.
In `@src/components/cli-export/CliExportRenderer.tsx`:
- Around line 76-85: The config validation currently only checks that c.project,
project.media and project.editor are objects, allowing a missing screenVideoPath
to surface later as a cryptic error; update the validation in CliExportRenderer
to explicitly check that project.media.screenVideoPath exists and is a non-empty
string (e.g., typeof project.media.screenVideoPath === "string" &&
project.media.screenVideoPath.trim() !== "") and throw a clear Error like "CLI
export config: missing 'project.media.screenVideoPath'" so callers fail fast
before toFileUrl(media.screenVideoPath) is invoked.
---
Nitpick comments:
In `@cli/src/core/project-manager.ts`:
- Around line 160-175: createProject currently builds editorOverrides and calls
normalizeProjectEditor without range validation, so out-of-range values (e.g.,
--padding 200) get silently clamped; call the same validation used by
editProject (validateEditOptions) or extract a shared validator and run it on
options before constructing editorOverrides and calling normalizeProjectEditor
(validate the overlapping fields like padding, borderRadius, shadowIntensity,
motionBlurAmount, exportQuality and exportFormat), and if validation fails
throw/report the error so createProject rejects invalid inputs instead of
silently clamping.
- Around line 265-275: The returned type for media in inspectProject is
incorrect: because loadProject throws when resolveProjectMedia returns falsy,
project.media is guaranteed to be present; update the inspectProject signature
to return media: ProjectMedia (not ProjectMedia | undefined) and adjust any
dependent code or types that assumed undefined. Locate inspectProject in
project-manager.ts and update its return type, ensuring references to
loadProject and resolveProjectMedia remain consistent with the non-optional
media type.
- Around line 190-235: validateEditOptions currently validates padding,
motionBlurAmount, aspectRatio, exportQuality/Format, gifFrameRate and
gifSizePreset but ignores borderRadius and shadowIntensity; add validation
checks in validateEditOptions for edits.borderRadius and edits.shadowIntensity
using the same allowed ranges or constants used by the normalizer (or the exact
numeric ranges the normalizer clamps to), and throw descriptive Errors on
invalid values (e.g., `Invalid borderRadius: ...` and `Invalid shadowIntensity:
...`) so inputs are rejected consistently before normalization; reference the
same constants or range logic from the normalizer to keep behavior aligned.
In `@cli/src/core/region-manager.ts`:
- Around line 199-202: The object currently stores opts.content twice: once as
content and again as textContent/imageContent; update the construction so you
don't duplicate values — set content: opts.content only for non-"text"/"image"
types (or remove content and keep only textContent/imageContent for explicit
types) and keep textContent: opts.content when opts.type === "text" and
imageContent: opts.content when opts.type === "image" (adjust callers if you
remove the generic content). Ensure you modify the code that builds this object
(references: opts.type, opts.content, textContent, imageContent) so consumers
get a single authoritative field per type.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 975fe197-3ab5-45a0-8a85-459b4f3812a8
📒 Files selected for processing (17)
cli/skills/rules/export-render.mdcli/src/cli-parsers.tscli/src/commands/annotate.tscli/src/commands/project.tscli/src/commands/shortcuts.tscli/src/commands/speed.tscli/src/commands/trim.tscli/src/commands/zoom.tscli/src/core/electron-bridge.tscli/src/core/project-manager.tscli/src/core/region-manager.tscli/src/output.tssrc/components/cli-export/CliExportRenderer.tsxsrc/components/video-editor/projectPersistence.test.tssrc/shared/aspect-ratios.tssrc/shared/project-schema.tssrc/shared/shortcuts.ts
✅ Files skipped from review due to trivial changes (1)
- cli/skills/rules/export-render.md
🚧 Files skipped from review as they are similar to previous changes (6)
- cli/src/commands/trim.ts
- cli/src/commands/project.ts
- cli/src/commands/zoom.ts
- cli/src/commands/annotate.ts
- src/shared/shortcuts.ts
- src/shared/aspect-ratios.ts
Comment on lines
+193
to
+199
| export function validateProjectData(candidate: unknown): candidate is EditorProjectData { | ||
| if (!candidate || typeof candidate !== "object") return false; | ||
| const project = candidate as Partial<EditorProjectData>; | ||
| if (typeof project.version !== "number") return false; | ||
| if (!resolveProjectMedia(project)) return false; | ||
| if (!project.editor || typeof project.editor !== "object") return false; | ||
| return true; |
Contributor
There was a problem hiding this comment.
Reject unsupported project versions up front.
validateProjectData() accepts any numeric version, but this module only knows the legacy v1 shape and the current PROJECT_VERSION. Letting v3+ through as “valid” is lowkey risky because edit/save flows can normalize away unknown fields and then write the project back as v2.
nit: safer version gate
export function validateProjectData(candidate: unknown): candidate is EditorProjectData {
if (!candidate || typeof candidate !== "object") return false;
const project = candidate as Partial<EditorProjectData>;
- if (typeof project.version !== "number") return false;
+ if (project.version !== 1 && project.version !== PROJECT_VERSION) return false;
if (!resolveProjectMedia(project)) return false;
if (!project.editor || typeof project.editor !== "object") return false;
return true;
}📝 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
| export function validateProjectData(candidate: unknown): candidate is EditorProjectData { | |
| if (!candidate || typeof candidate !== "object") return false; | |
| const project = candidate as Partial<EditorProjectData>; | |
| if (typeof project.version !== "number") return false; | |
| if (!resolveProjectMedia(project)) return false; | |
| if (!project.editor || typeof project.editor !== "object") return false; | |
| return true; | |
| export function validateProjectData(candidate: unknown): candidate is EditorProjectData { | |
| if (!candidate || typeof candidate !== "object") return false; | |
| const project = candidate as Partial<EditorProjectData>; | |
| if (project.version !== 1 && project.version !== PROJECT_VERSION) return false; | |
| if (!resolveProjectMedia(project)) return false; | |
| if (!project.editor || typeof project.editor !== "object") return false; | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/project-schema.ts` around lines 193 - 199, validateProjectData
currently accepts any numeric version; tighten the version gate to reject
unknown future versions by explicitly allowing only supported versions (the
legacy v1 shape and the current PROJECT_VERSION). Update the check in
validateProjectData (the project.version test) to only return true when
project.version === 1 or project.version === PROJECT_VERSION, ensuring unknown
numeric versions are rejected up front.
3 tasks
Enriquefft
added a commit
to Enriquefft/openscreen
that referenced
this pull request
Apr 27, 2026
…ed concerns The index now distinguishes "designed" (state coordination, with companion doc) from "noticed, not yet planned" (two concerns surfaced via filtered review of PR siddharthvaddem#350). Audit doc dropped — PR-review observations belong on PR siddharthvaddem#350 itself, not in the RFC.
Contributor
|
Reviewed @lupuletic — overall architecture is good, but a few things worth addressing before more verbs land on top, GUI/CLI state coordination being the biggest. I've opened a draft RFC at #497 to plan these properly; would suggest aligning on direction before this PR's successors land. Also, the diff is very large as you pointed out (17,885 / -11,661 across 129 files, four loosely-related streams of work). I'd propose restructuring into 5 PRs that can land in parallel with the planning:
A and E are independent of everything else; B unblocks C and D in parallel. Some things worth covering in the RFC (#497):
The RFC explicitly flags that more concerns probably exist that nobody has surfaced yet — the planning window is to find them, not just the ones above. A few days of planning before split-PR-A lands is the proposal; the split work itself is parallel and unblocked. Happy to contribute on either side: more RFC content (companion docs for the noticed items, or new ones if other concerns surface) and/or implementation on any of the 5 proposed PRs. If you have prior design notes, scratch docs, or past discussions on this direction, would love to see them — happy to fold into #497 (or wherever planning lands). The RFC is a draft starting point, not a finished claim. cc @siddharthvaddem for visibility. |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #349
Summary
openscreen) making OpenScreen accessible to AI agents and automationsrc/shared/for code sharing between CLI and Electron appArchitecture
Inspired by Remotion's approach: CLI is the primary agent interface (agents call it via bash), skills teach agents how to use it, MCP is lightweight discovery only.
Two-tier design:
CLI Commands
Usage
New dependencies
commander— CLI framework@modelcontextprotocol/sdk— MCP serverzod— Schema validation for MCPtsx(dev) — TypeScript runnerTest plan
npm test— 33/33 unit tests pass (no regressions)tsc --noEmit— zero type errors--jsonmode outputs valid JSON for all commands🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests