feat(video): add Remotion engine + html-in-canvas + 6 templates#613
feat(video): add Remotion engine + html-in-canvas + 6 templates#613
Conversation
…-canvas) Open Design's interactive-video surface previously supported a single local engine — HyperFrames (HTML + GSAP). This wires Remotion in as a sibling so the user can pick whichever authoring shape (HTML timeline vs. React components) fits the project, including the new experimental HTML-in-Canvas capture path for compositions that need CSS the default DOM composer can't handle. What's new - Two video models: `remotion-html` (standard) and `remotion-html-in-canvas` (flips Chromium's experimental drawElementImage capture path on). New provider `remotion`, integrated, no credentials required. - Daemon dispatcher branch: `renderRemotionViaCli` runs `npx remotion render <entry> <compId> <out>` from the daemon process, mirroring HyperFrames' sandbox-bypass pattern (puppeteer/Chrome can't capture frames under the agent shell's macOS sandbox). - New `--composition-id` CLI flag plus `compositionId` plumbed through `od media generate` → `/api/.../media/generate` → `generateMedia` → `ctx.compositionId`. The dispatcher rejects mismatched ids with a clear error so a typo can't yield a 0-byte mp4. - System-prompt + media-contract carve-outs teaching the agent when to pick Remotion vs. HF vs. photoreal t2v, the canonical 4-file scaffold under `.remotion-cache/<id>/`, and a concrete 5-question `<question-form>` to emit on turn 1 (pattern, accent, headline, duration, optional subject data). - New `skills/remotion/SKILL.md` with full authoring playbook: timing primitives, common patterns (product reveal, dashboard, code reveal, lifecycle saga), HTML-in-Canvas guidance, and OD-specific restrictions (don't run remotion render from the agent shell). - Six prompt-templates under `prompt-templates/video/remotion-*.json`: HTML-in-Canvas showcase, product lifecycle saga, marketing growth funnel, dev-tool code reveal, dashboard/KPI reveal, brand sizzle with dual-aspect (9:16 + 1:1) registration. Verification - `node scripts/verify-media-models.mjs` (web + daemon registries match) - `pnpm --filter @open-design/contracts typecheck` - `pnpm --filter @open-design/web typecheck` - `pnpm --filter @open-design/daemon test` (428/428) - `pnpm check:residual-js` - Smoke: daemon's `listPromptTemplates` loads all 6 new entries; `findMediaModel` resolves both new model ids and the `remotion` provider; the NewProjectPanel video allowlist surfaces them. Live `npx remotion render` end-to-end is not exercised here — the renderer is wired identically to the proven HyperFrames path; live-fire smoke belongs in a follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e5d1317ee
ℹ️ 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".
| 'verbose', | ||
| ]; | ||
| if (allowHtmlInCanvas) { | ||
| args.push('--allow-html-in-canvas'); |
There was a problem hiding this comment.
Remove unsupported html-in-canvas flag
When the user selects remotion-html-in-canvas, every render appends --allow-html-in-canvas to npx remotion render. I checked the current Remotion render CLI docs and the upstream CLI source: the documented/parsed render flags include options such as --concurrency and --log, but there is no --allow-html-in-canvas flag. This means the experimental model path will be rejected by the CLI before rendering; HTML-in-canvas should be enabled through the supported Remotion API/component/config path rather than an unknown render flag.
Useful? React with 👍 / 👎.
lefarcen
left a comment
There was a problem hiding this comment.
Hey @pftom! Strong architectural work — the Remotion carve-out mirrors HyperFrames cleanly and the skill + 6 templates are comprehensive. But there are must-fix security + correctness issues before merge:
Blockers
Merge conflict — mergeStateStatus: DIRTY. Rebase against main first.
The inline findings below are prioritized P1/P2 security + correctness. Several come from spawn/child-process + path handling in the new renderRemotionViaCli flow:
- P1 — Missing
npm installbefore render (new.remotion-cache/<id>projects won't have dependencies) - P1 — Timeout only kills direct
npxprocess, not Chromium tree (orphans remain) - P2 —
compositionDirsymlink escape (not usingrealpathcheck) - P2 —
compositionIdargument injection (IDs starting with-parsed as flags) - P2 — Conflicting discovery instructions ("one question" vs "five-field form")
- P2 — Duration metadata mismatch (templates want 18s/20s/45s, picker only allows
[3,5,8,10,15,30])
See inline comments for concrete fixes. The two-model split (remotion-html vs -html-in-canvas) makes sense for exposing the experimental path without metadata complexity.
Non-blocking polish
After P1/P2 are fixed:
- Consider adding a live end-to-end smoke test (currently deferred in test plan)
- Skill.md: explicit "when NOT to use html-in-canvas" guardrails would help agents avoid overuse
| const message = stderr || (err && err.message ? err.message : String(err)); | ||
| throw new Error(`remotion render failed: ${truncate(message, 480)}`); | ||
| } finally { | ||
| await rm(tmpRoot, { recursive: true, force: true }); |
There was a problem hiding this comment.
P1 Fresh .remotion-cache/<id> projects are not made runnable before render. The skill/templates scaffold package.json + source but don't run npm install, yet npx -y remotion render expects react, react-dom, and remotion to be in node_modules. npx only installs the CLI package itself. First real render will fail with module-resolution errors. Fix: Before invoking runRemotionRender, check if compAbs/node_modules exists; if not, run npm install or pnpm install (with same timeout/child-process discipline), or document in the skill that agents must run install before dispatch.
| emit(chunk); | ||
| }); | ||
|
|
||
| const timer = setTimeout(() => { |
There was a problem hiding this comment.
P1 Timeout only kills the direct npx process via child.kill('SIGKILL'), not the Remotion/Chromium process tree it spawns. On timeout, orphaned Chrome workers keep running after task failure and after tmpRoot is removed, so the 8-minute timeout does not actually bound CPU/memory. Fix: Spawn in a process group and kill the group (process.kill(-child.pid, 'SIGKILL') after setting detached: true in spawn options), or use a tree-kill helper library to reliably terminate descendants.
| 'remotion render knows which composition to capture.', | ||
| ); | ||
| } | ||
| const projectRootResolved = path.resolve(projectDir); |
There was a problem hiding this comment.
P2 compositionDir is checked lexically but not with realpath, so a symlink inside the project can point outside while passing the startsWith(projectRoot + sep) guard. Daemon then runs unsandboxed npx in that directory. Fix: Resolve realpath(projectDir) and realpath(compAbs) after existence checks, reject symlink escapes. Consider requiring the real path to remain under .remotion-cache/.
| onProgress, | ||
| }) { | ||
| return new Promise((resolve, reject) => { | ||
| const args = [ |
There was a problem hiding this comment.
P2 compositionId is passed as a positional CLI argument without validation. IDs beginning with - (e.g. --malicious-flag) can be interpreted by Remotion's option parser as flags, not as the composition id (argument injection, even though spawn(..., args) avoids shell injection). Fix: Validate composition IDs against a conservative Remotion-id pattern such as /^[A-Za-z0-9_-]{1,128}$/ before constructing args, or prefix with -- separator if Remotion supports it.
| metadata.videoModel === 'remotion-html-in-canvas' | ||
| ) { | ||
| lines.push( | ||
| 'Special case: `remotion-html` is a local React-component → MP4 renderer, not a photoreal text-to-video model. Read `skills/remotion/SKILL.md`, scaffold a tiny Remotion project under `.remotion-cache/<id>/` (src/index.ts calling `registerRoot`, src/Root.tsx registering one `<Composition id="…">`, plus the component files), then dispatch with `--composition-dir` AND `--composition-id`. Ask at most one clarifying question first.', |
There was a problem hiding this comment.
P2 The Remotion discovery instructions conflict: line 277 says "ask at most one clarifying question," while line 280 requires a full five-field <question-form> unless a template covers every value. That delays straightforward "make this Remotion video" requests and conflicts with the fast-dispatch media contract. Fix: Make the form conditional on genuinely missing information — treat metadata fields like videoLength, videoAspect, selected template, and user prompt as sufficient defaults. Only show the form when truly ambiguous.
Resolve conflict in apps/daemon/src/prompts/media-contract.ts: keep the remotion-html / remotion-html-in-canvas carve-out from this branch while adopting main's "$OD_NODE_BIN" "$OD_BIN" invocation. Update the remotion code block to match the new invocation form.
mrcfps
left a comment
There was a problem hiding this comment.
@pftom Thanks for putting this Remotion path together — the provider/model wiring and templates are substantial. I found one additional blocking trust-boundary issue in the daemon render path that should be addressed before merge.
Generated by Looper 0.6.1 · runner=reviewer · agent=opencode| } | ||
| const child = spawn('npx', args, { | ||
| cwd: compAbs, | ||
| env: process.env, |
There was a problem hiding this comment.
Blocking — sandbox the Remotion render process before passing daemon environment.
Problem: this new render path starts npx remotion render from the agent-authored compositionDir and passes the daemon's full process.env into that process. Unlike a static media upload, the Remotion project under .remotion-cache/<id>/ is generated from the user's prompt and can include Remotion config/package code that the CLI loads during render.
Why it matters: the PR intentionally moves rendering out of the agent sandbox and into the unsandboxed daemon process. With the current env: process.env and unrestricted cwd, prompt-authored project code can execute with the daemon's filesystem/network access and read credentials or runtime paths that should not be exposed to generated media code.
Evidence: the changed lines resolve compAbs from a project-relative, agent-written directory and then spawn npx with cwd: compAbs plus the daemon environment here.
Suggested change: render Remotion compositions inside an actual sandbox/container or a constrained helper process with a minimal allowlisted environment, and reject/disable project remotion.config.*, package lifecycle hooks, or other Node-side extension points unless they are explicitly safe. At minimum, combine this with a realpath check that keeps the composition under .remotion-cache/ and avoid forwarding daemon secrets via process.env.
The localized-content e2e test asserts that LOCALIZED_CONTENT_IDS for de/fr/ru covers every skill, design system, and prompt template found on disk. This branch added skills/remotion/ and 6 remotion video prompt templates (Demo + Developer categories, plus several new tags), breaking the test. Add 'remotion' to the per-locale skill fallback arrays, the 6 remotion prompt template ids to the prompt-template fallback arrays, the Demo and Developer categories, and the 16 missing prompt-template tags (changelog, chromium, code, compositing, dashboard, developer, dev-tool, experimental, funnel, growth, html-in-canvas, kpi, lifecycle, narrative, remotion, sizzle-reel) to all three locales.
Resolve conflicts where main added the 'nanobanana' image provider alongside this branch's 'remotion' video provider: - apps/daemon/src/media-models.ts: keep both new providers - apps/web/src/media/models.ts: keep both ids in MediaProviderId union and both entries in MEDIA_PROVIDERS - apps/web/src/components/NewProjectPanel.tsx: extend the surface whitelist with both — image gets 'nanobanana', video keeps 'remotion'
|
@pftom I'm holding off on generating review comments for #613 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. Generated by Looper 0.6.1 · runner=reviewer · agent=opencode |
Summary
remotion-htmland the experimentalremotion-html-in-canvas) wired into the picker, the daemon dispatcher, and the system prompt's discovery flow.npx remotion render <entry> <compositionId> <out>from its own (unsandboxed) process — same pattern HyperFrames uses to dodge the agent-shell macOS sandbox that hangs puppeteer/Chrome partway through frame capture.skills/remotion/SKILL.md) and six high-quality prompt-templates: HTML-in-Canvas showcase, product lifecycle saga, marketing growth funnel, dev-tool code reveal, dashboard/KPI reveal, and a dual-aspect brand sizzle.Why two models?
The picker exposes the engine choice directly, so
remotion-html-in-canvaslives next toremotion-html— picking it flips--allow-html-in-canvason the render call. That keeps the experimental path one click away for compositions that legitimately need it (mix-blend + filter stacks, position:sticky inside clipped scrollers, etc.) without burying it inside metadata.Public surface
MediaProviderIdgains'remotion'.VIDEO_MODELSgainsremotion-html+remotion-html-in-canvas.od media generategains--composition-id <id>(required for Remotion, ignored otherwise)./api/projects/:id/media/generateaccepts a newcompositionIdbody field..remotion-cache/<id>/(mirrors.hyperframes-cache/).Test plan
node scripts/verify-media-models.mjs(web + daemon registries match)pnpm --filter @open-design/contracts typecheckpnpm --filter @open-design/web typecheckpnpm --filter @open-design/daemon test(428/428)pnpm check:residual-jslistPromptTemplatesloads all 6 new templatesfindMediaModel('remotion-html')andfindMediaModel('remotion-html-in-canvas')resolvenpx remotion renderend-to-end smoke (deferred — wired identically to HyperFrames; should be exercised manually before release)remotion-htmlandremotion-html-in-canvascards appear in the New-Project video picker