feat: add CLI and agent interface for OpenScreen#350
Open
lupuletic wants to merge 2 commits intosiddharthvaddem:mainfrom
Open
feat: add CLI and agent interface for OpenScreen#350lupuletic wants to merge 2 commits intosiddharthvaddem:mainfrom
lupuletic wants to merge 2 commits intosiddharthvaddem:mainfrom
Conversation
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. ## What's included **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>
📝 WalkthroughWalkthroughAdds a full CLI (openscreen) with 20+ commands, shared TypeScript modules for validation/types, an Electron headless export bridge, AgentSkills-format documentation and MCP server, plus wiring to reuse GUI export code via a new cli-export renderer. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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".
- 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}
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
+7
to
+28
| ```bash | ||
| openscreen --json project create \ | ||
| --video /path/to/recording.webm \ | ||
| --output /path/to/project.openscreen \ | ||
| --wallpaper wallpaper3 \ | ||
| --padding 32 \ | ||
| --border-radius 12 \ | ||
| --aspect-ratio 16:9 | ||
| ``` | ||
|
|
||
| **Required:** `--video` (path to screen recording) and `--output` (path for project file). | ||
|
|
||
| **Optional settings:** | ||
| - `--wallpaper` — wallpaper1 through wallpaper18, or a hex color | ||
| - `--padding` — 0 to 100 (default: 50) | ||
| - `--border-radius` — corner radius in pixels (default: 0) | ||
| - `--aspect-ratio` — 16:9, 9:16, 1:1, 4:3, 4:5, 16:10, 10:16, native (default: 16:9) | ||
| - `--webcam` — path to webcam video for picture-in-picture | ||
| - `--shadow-intensity` — shadow amount (default: 0) | ||
| - `--export-quality` — medium, good, source (default: good) | ||
| - `--export-format` — mp4, gif (default: mp4) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
markdown lint: blank lines around code blocks
same issue as troubleshooting.md—add blank lines before/after fenced code blocks for better cross-renderer compatibility.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 27-27: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/skills/rules/project-setup.md` around lines 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.
Comment on lines
+5
to
+9
| ### "Project file not found" | ||
| ```bash | ||
| # Use absolute paths for reliability | ||
| openscreen --json project info --project /absolute/path/to/demo.openscreen | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
markdown lint: missing blank lines around headings/code blocks
static analysis caught this—headings and fenced code blocks should have blank lines around them. kinda nitpicky but keeps the markdown portable across renderers.
📝 add blank lines
### "Project file not found"
+
```bash
# Use absolute paths for reliability
openscreen --json project info --project /absolute/path/to/demo.openscreen"Invalid JSON in project file"
</details>
apply the same pattern to the other sections (lines 11-28) where code blocks immediately follow headings.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
[warning] 6-6: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @cli/skills/rules/troubleshooting.md around lines 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.
</details>
<!-- fingerprinting:phantom:poseidon:ocelot:e75c183d-ac47-4338-affe-1951fb2ef496 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+27
to
+28
| ### Render commands fail with "requires Electron headless bridge" | ||
| The `render`, `gif`, `still`, and `frames` commands need the Electron app installed. These commands are Phase 3 features. |
There was a problem hiding this comment.
heads up: "still" and "frames" commands mentioned but not implemented yet
the PR summary says these are still planned. might want to either note they're coming soon or remove the reference until they ship, otherwise agents might try to use them and hit confusing errors.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/skills/rules/troubleshooting.md` around lines 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.
Comment on lines
+17
to
+23
| .requiredOption("--start <ms>", "Start time in milliseconds", parseInt) | ||
| .requiredOption("--end <ms>", "End time in milliseconds", parseInt) | ||
| .option("--position-x <0-100>", "X position (0-100)", parseFloat) | ||
| .option("--position-y <0-100>", "Y position (0-100)", parseFloat) | ||
| .option("--width <n>", "Width (percent of viewport)", parseFloat) | ||
| .option("--height <n>", "Height (percent of viewport)", parseFloat) | ||
| .option("--font-size <px>", "Font size in pixels", parseInt) |
There was a problem hiding this comment.
same parseInt/parseFloat validation gap
applies here too—invalid numeric inputs become NaN silently. the position/dimension options (0-100, 0-100) don't actually enforce ranges.
🤖 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 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.
Comment on lines
+86
to
+92
| .option("--show-blur <bool>", "Show blur", parseBool) | ||
| .option("--motion-blur <n>", "Motion blur amount (0-1)", parseFloat) | ||
| .option("--aspect-ratio <ratio>", "Aspect ratio") | ||
| .option("--export-quality <q>", "Export quality (medium, good, source)") | ||
| .option("--export-format <f>", "Export format (mp4, gif)") | ||
| .option("--gif-frame-rate <fps>", "GIF frame rate (15, 20, 25, 30)", parseInt) | ||
| .option("--gif-loop <bool>", "GIF loop", parseBool) |
There was a problem hiding this comment.
Reject unknown boolean literals instead of quietly coercing them to false.
parseBool() treats anything outside true/1/yes as false, so typos like --gif-loop treu or values like TRUE silently flip settings off. That’s kinda cursed for automation; bad input should fail fast here.
nit: cleaner strict parser
function parseBool(value: string): boolean {
- return value === "true" || value === "1" || value === "yes";
+ const normalized = value.trim().toLowerCase();
+ if (["true", "1", "yes"].includes(normalized)) {
+ return true;
+ }
+ if (["false", "0", "no"].includes(normalized)) {
+ return false;
+ }
+ throw new Error(`Invalid boolean value: ${value}. Use true/false, 1/0, or yes/no.`);
}Also applies to: 116-118
🤖 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 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.
Comment on lines
+144
to
+152
| export function deriveNextId(prefix: string, ids: string[]): number { | ||
| const max = ids.reduce((acc, id) => { | ||
| const match = id.match(new RegExp(`^${prefix}-(\\d+)$`)); | ||
| if (!match) return acc; | ||
| const value = Number(match[1]); | ||
| return Number.isFinite(value) ? Math.max(acc, value) : acc; | ||
| }, 0); | ||
| return max + 1; | ||
| } |
There was a problem hiding this comment.
static analysis flagged potential ReDoS — low risk but worth escaping
the prefix parameter is used to build a regex. currently it's only called with internal strings like "zoom", "trim", etc., so it's safe in practice. but if this ever receives user input with regex metacharacters, it could cause issues.
🛡️ safer version with regex escaping
+function escapeRegExp(str: string): string {
+ return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
+
export function deriveNextId(prefix: string, ids: string[]): number {
const max = ids.reduce((acc, id) => {
- const match = id.match(new RegExp(`^${prefix}-(\\d+)$`));
+ const match = id.match(new RegExp(`^${escapeRegExp(prefix)}-(\\d+)$`));
if (!match) return acc;
const value = Number(match[1]);
return Number.isFinite(value) ? Math.max(acc, value) : acc;
}, 0);
return max + 1;
}🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 145-145: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${prefix}-(\\d+)$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 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 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.
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", | ||
| }; | ||
| } |
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, | ||
| }; | ||
| } |
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
+150
to
+157
| export function mergeWithDefaults(partial: Partial<ShortcutsConfig>): ShortcutsConfig { | ||
| const merged = { ...DEFAULT_SHORTCUTS }; | ||
| for (const action of SHORTCUT_ACTIONS) { | ||
| if (partial[action]) { | ||
| merged[action] = partial[action] as ShortcutBinding; | ||
| } | ||
| } | ||
| return merged; |
There was a problem hiding this comment.
mergeWithDefaults() still shares the default binding objects.
This is only a shallow copy, so mutating a binding on the merged result also mutates DEFAULT_SHORTCUTS. That's lowkey nasty for any settings flow that edits bindings in place.
♻️ Proposed fix
export function mergeWithDefaults(partial: Partial<ShortcutsConfig>): ShortcutsConfig {
- const merged = { ...DEFAULT_SHORTCUTS };
+ const merged = Object.fromEntries(
+ SHORTCUT_ACTIONS.map((action) => [action, { ...DEFAULT_SHORTCUTS[action] }]),
+ ) as ShortcutsConfig;
for (const action of SHORTCUT_ACTIONS) {
if (partial[action]) {
- merged[action] = partial[action] as ShortcutBinding;
+ merged[action] = { ...(partial[action] as ShortcutBinding) };
}
}
return merged;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/shortcuts.ts` around lines 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.
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)); |
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".
| .requiredOption("--output <path>", "Output GIF file path") | ||
| .option("--frame-rate <fps>", "GIF frame rate (15, 20, 25, 30)", "15") | ||
| .option("--size-preset <s>", "GIF size (medium, large, original)", "medium") | ||
| .option("--loop", "Loop GIF", true) |
There was a problem hiding this comment.
Defining --loop with a default of true means opts.loop is always truthy, even when the user does not pass the flag. Since this value is forwarded unconditionally to runExport, GIF exports cannot respect a project configured with gifLoop: false, and there is no CLI path to produce a non-looping GIF from gif.
Useful? React with 👍 / 👎.
Comment on lines
+120
to
+122
| child.stdout?.on("data", (chunk: Buffer) => { | ||
| const lines = chunk.toString().split("\n").filter(Boolean); | ||
| for (const line of lines) { |
There was a problem hiding this comment.
The parser treats each data chunk as a complete set of newline-delimited JSON messages, but stream chunk boundaries are arbitrary. If a single JSON message is split across chunks, both fragments are dropped by JSON.parse, which can lose done/error metadata and make machine-readable output unreliable for automation. Keep a carry-over buffer between events and only parse complete newline-terminated records.
Useful? React with 👍 / 👎.
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
Comment on lines
+25
to
+26
| const VALID_GIF_PRESETS = ["medium", "large", "original"] as const; | ||
| const VALID_GIF_FRAME_RATES = [15, 20, 25, 30] as const; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
validation lists duplicated from shared module
VALID_GIF_FRAME_RATES is already exported from src/shared/export-types.ts. local copy could drift out of sync if the canonical list changes. recommend importing from shared instead.
♻️ import from shared
+import { VALID_GIF_FRAME_RATES, isValidGifFrameRate } from "../../../src/shared/export-types";
+
-const VALID_GIF_PRESETS = ["medium", "large", "original"] as const;
-const VALID_GIF_FRAME_RATES = [15, 20, 25, 30] as const;
+const VALID_GIF_PRESETS = ["medium", "large", "original"] as const; // could also move to sharedthen use isValidGifFrameRate(frameRate) for validation.
🤖 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 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.
Comment on lines
+65
to
+89
| export const stillCommand = new Command("still") | ||
| .description("Capture a single frame as PNG or JPEG (requires built Electron app)") | ||
| .requiredOption("--project <path>", "Path to .openscreen project file") | ||
| .requiredOption("--output <path>", "Output image file path") | ||
| .option("--frame <ms>", "Timestamp in milliseconds", "0") | ||
| .option("--format <f>", "Image format (png, jpeg)", "png") | ||
| .option("--jpeg-quality <q>", "JPEG quality (1-100)", "90") | ||
| .option("--scale <n>", "Device scale factor", "1") | ||
| .option("--overwrite", "Overwrite existing output file") | ||
| .action((_opts) => { | ||
| outputError("The still command is not yet implemented. Use render for full video export."); | ||
| }); | ||
|
|
||
| export const framesCommand = new Command("frames") | ||
| .description("Export a sequence of frames as images (requires built Electron app)") | ||
| .requiredOption("--project <path>", "Path to .openscreen project file") | ||
| .requiredOption("--output-dir <dir>", "Output directory for frames") | ||
| .option("--start <ms>", "Start timestamp in milliseconds", "0") | ||
| .option("--end <ms>", "End timestamp (default: video end)") | ||
| .option("--every-nth <n>", "Export every Nth frame", "1") | ||
| .option("--format <f>", "Image format (png, jpeg)", "png") | ||
| .option("--overwrite", "Overwrite existing output files") | ||
| .action((_opts) => { | ||
| outputError("The frames command is not yet implemented. Use render for full video export."); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
stub commands are reasonable placeholders
options are defined for future implementation, and the error message is clear. good that you're reserving the command surface area now.
want me to help sketch out the implementation for still and frames commands when you're ready to tackle them?
🤖 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 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.
Comment on lines
+91
to
+108
| .requiredOption("--key <key>", "Key (e.g., z, t, space)") | ||
| .option("--ctrl", "Require Ctrl/Cmd modifier") | ||
| .option("--shift", "Require Shift modifier") | ||
| .option("--alt", "Require Alt/Option modifier") | ||
| .action((opts) => { | ||
| try { | ||
| if (!SHORTCUT_ACTIONS.includes(opts.action)) { | ||
| outputError(`Invalid action: ${opts.action}. Valid: ${SHORTCUT_ACTIONS.join(", ")}`); | ||
| return; | ||
| } | ||
|
|
||
| const config = loadShortcuts(); | ||
| config[opts.action as keyof ShortcutsConfig] = { | ||
| key: opts.key, | ||
| ...(opts.ctrl ? { ctrl: true } : {}), | ||
| ...(opts.shift ? { shift: true } : {}), | ||
| ...(opts.alt ? { alt: true } : {}), | ||
| }; |
There was a problem hiding this comment.
Normalize the space key alias before saving.
Line 91 says --key accepts space, but Line 104 stores raw input. lowkey risky: defaults use " " as the space key, so persisting "space" can produce a binding that looks fine in CLI but doesn’t trigger at runtime.
kinda cursed edge-case, easy fix
.action((opts) => {
try {
+ const normalizedKey =
+ typeof opts.key === "string" && opts.key.toLowerCase() === "space"
+ ? " "
+ : opts.key;
+
if (!SHORTCUT_ACTIONS.includes(opts.action)) {
outputError(`Invalid action: ${opts.action}. Valid: ${SHORTCUT_ACTIONS.join(", ")}`);
return;
}
const config = loadShortcuts();
config[opts.action as keyof ShortcutsConfig] = {
- key: opts.key,
+ key: normalizedKey,
...(opts.ctrl ? { ctrl: true } : {}),
...(opts.shift ? { shift: true } : {}),
...(opts.alt ? { alt: true } : {}),
};🤖 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 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).
Comment on lines
+98
to
+116
| const args = [ | ||
| mainJs, | ||
| "--cli-export", | ||
| "--config", | ||
| configPath, | ||
| "--output", | ||
| absOutput, | ||
| "--no-sandbox", | ||
| ]; | ||
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| const child = spawn(electronBin, args, { | ||
| env: { | ||
| ...process.env, | ||
| HEADLESS: "true", | ||
| ELECTRON_DISABLE_SECURITY_WARNINGS: "true", | ||
| }, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
--no-sandbox present - probably needed but worth documenting
--no-sandbox is often required for headless Electron in CI/containers. might be worth a brief comment explaining why it's there, so future maintainers don't accidentally remove it.
🤖 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 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.
Comment on lines
+70
to
+72
| if (!validateProjectData(parsed)) { | ||
| throw new Error(`Invalid project data in: ${absPath}`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
generic validation error loses useful context
when validateProjectData returns false, you throw "Invalid project data" but the user has no idea what failed - missing version? bad media? no editor? for CLI tooling, a more specific error would be helpful for debugging. lowkey annoying to troubleshoot.
💡 could surface more detail
one option: have validateProjectData return an error string (or null on success) instead of a boolean, so you can relay the specific failure reason. but that'd require changes in the shared module, so maybe a future improvement.
🤖 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 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.
Comment on lines
+74
to
+81
| // Materialize media from legacy videoPath if needed | ||
| const media = resolveProjectMedia(parsed); | ||
|
|
||
| return { | ||
| ...parsed, | ||
| ...(media ? { media } : {}), | ||
| editor: normalizeProjectEditor(parsed.editor), | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
resolveProjectMedia is called twice
it's already called inside validateProjectData (line 70, see context snippet 2), then again here at line 75. the function walks the media structure each time. not expensive, but technically redundant.
🤖 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 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.
Comment on lines
+140
to
+164
| export function editProject(projectPath: string, edits: EditProjectOptions): EditorProjectData { | ||
| const project = loadProject(projectPath); | ||
|
|
||
| const updatedEditor: ProjectEditorState = { ...project.editor }; | ||
|
|
||
| if (edits.wallpaper !== undefined) updatedEditor.wallpaper = edits.wallpaper; | ||
| if (edits.padding !== undefined) updatedEditor.padding = edits.padding; | ||
| if (edits.borderRadius !== undefined) updatedEditor.borderRadius = edits.borderRadius; | ||
| if (edits.shadowIntensity !== undefined) updatedEditor.shadowIntensity = edits.shadowIntensity; | ||
| if (edits.showBlur !== undefined) updatedEditor.showBlur = edits.showBlur; | ||
| if (edits.motionBlurAmount !== undefined) updatedEditor.motionBlurAmount = edits.motionBlurAmount; | ||
| if (edits.aspectRatio !== undefined) updatedEditor.aspectRatio = edits.aspectRatio; | ||
| if (edits.exportQuality !== undefined) updatedEditor.exportQuality = edits.exportQuality; | ||
| if (edits.exportFormat !== undefined) updatedEditor.exportFormat = edits.exportFormat; | ||
| if (edits.gifFrameRate !== undefined) updatedEditor.gifFrameRate = edits.gifFrameRate; | ||
| if (edits.gifLoop !== undefined) updatedEditor.gifLoop = edits.gifLoop; | ||
| if (edits.gifSizePreset !== undefined) updatedEditor.gifSizePreset = edits.gifSizePreset; | ||
|
|
||
| // Re-normalize to ensure constraints are met | ||
| const normalizedEditor = normalizeProjectEditor(updatedEditor); | ||
| const updated: EditorProjectData = { ...project, editor: normalizedEditor }; | ||
|
|
||
| saveProject(projectPath, updated); | ||
| return updated; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
silent clamping may surprise CLI users
normalizeProjectEditor at line 159 will silently clamp values (e.g., padding to [0,100], invalid aspectRatio to "16:9"). if a user runs openscreen project edit --padding 200, they'll get 100 with no warning. for GUI this is fine, but CLI users might expect an error or at least a heads-up.
consider adding CLI-layer validation that warns/errors on out-of-range values before calling editProject, or at minimum document this behavior.
🤖 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 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.
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
2 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