feat(studio): per-composition render button in compositions tab#874
Conversation
Thread composition path through the full render pipeline so individual compositions can be rendered independently from the studio UI. - Add download icon button on each comp card (visible on hover) - Accept `composition` field in POST /projects/:id/render - Pass composition as `entryFile` to the producer's createRenderJob - Make the Export button in the Renders panel composition-aware (renders the active composition instead of always index.html)
The hover-only opacity made them undiscoverable.
jrusso1020
left a comment
There was a problem hiding this comment.
LGTM — clean threading of an optional composition field end-to-end.
Audited — chain integrity
CompositionsTab.tsx— per-card download button,e.stopPropagation()on click so it doesn't double-fire the parentonSelect,disabled={isRendering}gate ✓LeftSidebar.tsx/StudioLeftSidebar.tsx— prop-passthrough plumbing,handleRenderCompositioncallswaitForPendingDomEditSaves()BEFOREstartRenderso in-flight studio edits flush before the producer reads HTML from disk ✓StudioRightPanel.tsxExport button — derivescomposition = activeCompPath && activeCompPath !== "index.html" ? activeCompPath : undefined. Clean handling of the "viewing index.html" case (no regression) and the "viewing sub-composition" case (overrides) ✓useRenderQueue.ts—StartRenderOptions.composition?added; conditionalif (composition) body.composition = composition;only includes the field when present ✓core/src/studio-api/routes/render.ts— validatestypeof === "string" && length > 0before forwarding; defaults to undefined otherwise ✓core/src/studio-api/types.ts— adapter contract widened withcomposition?: string✓studio/vite.adapter.ts—...(opts.composition ? { entryFile: opts.composition } : {})conditional spread mapscomposition → entryFileonly when present, so producer'sindex.htmldefault is preserved ✓- Pure-additive end-to-end: every layer is a no-op when
compositionis undefined, so existing call sites (full-project renders) are byte-identical to before ✓
Body claim verification
Two claims to spot-check:
- "composition-aware Export button" — verified,
StudioRightPanel.tsx:198-208✓ - "visible on hover" — doesn't quite match:
CompositionsTab.tsx:240-242renders the button unconditionally whenonRenderis provided, noopacity-0/group-hover/card:opacity-100gating. The parent getsgroup/cardTailwind class added (the named-group hover infra), but no child class actually uses it. Either addopacity-0 group-hover/card:opacity-100 transition-opacityon the button to match the body, or drop "visible on hover" from the description.
Non-blocking notes
-
Comp-card render uses defaults, not user's selected settings — clicking the download icon on a card calls
renderQueue.startRender({ composition })without passingfps/quality/format/resolution, so those fall through to theuseRenderQueuedefaults (fps=30,quality="standard",format="mp4"). If the user has dialed in 60fps WebM in the Renders panel, the card button silently ignores that. Probably intentional ("quick render with defaults") but worth either documenting in a tooltip or passing through the current panel settings. The Export button inStudioRightPaneldoes pass them — the asymmetry is real. -
Path-jailing for
composition—render.ts:80-83validates the string is non-empty but doesn't path-jail.composition: "../../../etc/passwd"would flow through to the producer'sentryFileresolution. The trust boundary is bounded (the studio user owns the project they're viewing, so they already have arbitrary read/write to their own project dir via the file editor), so practical impact is low — defense-in-depth would still be worth apath.relative(projectDir, resolve(projectDir, composition)).startsWith("..")rejection at the route layer. Same defensive shapevite.adapter.tsalready implicitly relies on. -
Test plan all-unchecked — the 7-row test plan is entirely unverified per the body. Worth at least exercising the no-regression case (Export on
index.htmlview → full project) before merge to confirm the new conditional doesn't accidentally rewire the default.
CI all green on required checks (Test, Typecheck, Lint, Build, CodeQL, Test: runtime contract, CLI smoke (required), Tests on windows-latest, Preview parity, Smoke: global install, Perf:*); Render on windows-latest + regression-shards still in-progress. mergeable_state: blocked is reviewer-gate.
— Review by Rames Jusso (pr-review)
vanceingalls
left a comment
There was a problem hiding this comment.
Per-comp render is a clean, useful surface and the wiring is tight — but the contract was widened across the studio API surface and only one of the two adapter implementations was updated, plus the new server-side input lacks the path-sanitization that the rest of the route applies to every other field.
Strengths
packages/studio/src/components/StudioRightPanel.tsx:198-208— the Export button gracefully degrades to "render the active composition vs. full project" without a separate code path. Nice reuse ofactiveCompPath.packages/producer/src/services/renderOrchestrator.ts:1917-1939already handles<template>-wrapped sub-compositions standalone-extraction — this PR threads to a producer pipeline that was already designed for it. Good leverage.useRenderQueue.ts:99consistent with the prior pattern: omit the field when falsy rather than sending an enum-failing value.
Blockers
- blocker — Rule 2 — second adapter not updated. The contract
composition?: stringwas added toStudioApiAdapter.startRenderinpackages/core/src/studio-api/types.ts:91, and the vite adapter atpackages/studio/vite.adapter.ts:204honors it. But the CLI studio server atpackages/cli/src/server/studioServer.ts:227-260— the embedded server used byhyperframes previewoutside the monorepo (seepackages/cli/src/commands/preview.ts:321) — silently dropsopts.compositionand always rendersindex.html. Users runninghyperframes previewwill see the per-comp button in the UI, click it, and get the full project rendered instead. Add...(opts.composition ? { entryFile: opts.composition } : {})to thecreateRenderJobcall there, parallel to the vite adapter change.
Important
- important — input validation gap on the route.
packages/core/src/studio-api/routes/render.ts:80-83acceptscompositionas any non-empty string and forwards it straight to the producer, which doesjoin(projectDir, entryFile)+readFileSync(...)atpackages/producer/src/services/renderOrchestrator.ts:1907-1916. The route validatesformat,quality,resolution,fpsagainst enums, butcompositiongets only atypeof === "string" && length > 0check. A value like"../../../../etc/hosts"resolves outsideprojectDirand getsreadFileSync'd. The studio API is normally localhost-only, buthyperframes previewexposes a port (seepackages/cli/src/commands/preview.ts), so this is a real arbitrary-file-read primitive when that command is used. Defense-in-depth: reject paths containing..segments, leading/, leading~, or backslashes; orpath.resolve()and assert the result isstartsWith(resolvedProjectDir + sep). Same pattern asresolveRenderPathsalready uses. - important —
isRenderingis a global busy-flag, not per-job.useRenderQueue.ts:227isjobs.some(...), and that single boolean is what disables every comp-card button (CompositionsTab.tsx:208,disabled={isRendering}). Click comp A → every other comp's render button is disabled until A finishes. That contradicts a "queue" semantic and is a UX trap for the obvious "render all my compositions" workflow. Either name itisAnyRenderingand document the intent, or track per-composition busy state so multiple renders can queue. - important — no tests for the new server-side composition forwarding.
packages/core/src/studio-api/routes/render.test.tshas end-to-end coverage forresolution,fps,format,qualityforwarding (lines 41-115) but adds nothing forcomposition. The forwarding/sanitization is exactly the kind of thing that silently regresses on the next route refactor. Two pinning tests: (a)composition: "intro.html"reaches the adapter ascomposition: "intro.html"; (b) missing or emptycompositionreaches the adapter asundefined. If you take the sanitization finding above, add (c)composition: "../etc/passwd"is rejected at the route boundary. - important — accessibility: icon-only button has no accessible name.
CompositionsTab.tsx:210-237— the new render<button>has onlytitle="..."; the inner<svg>has noaria-label, no visible text, and noaria-labelledby. Screen readers will announce "button" with no label. Addaria-label={isRendering ? "Rendering..." :+ "Render ${name}}" +to the button (thetitle` is for sighted hover, not for AT). For consistency with other studio buttons, check whether there's a shared icon-button pattern.
Nits
- nit — SSE event-source ref is single-slotted.
useRenderQueue.ts:36, 147—eventSourceRef.currentis overwritten on everystartRender. Today theisRenderingflag mostly prevents concurrent calls so the prior subscriber would already be torn down — but if you fix the per-comp queue gating above, you'll trip this immediately. Pre-existing issue, but the new UI is what makes it reachable. - nit —
jobIdis second-resolution.routes/render.ts:81-83builds the jobId fromYYYY-MM-DD_HH-MM-SS+ project ID. Two renders started in the same wall-second collide on jobId → second one'soutputPathand SSE channel overwrite the first's. Per-comp button makes this trivially reachable (click comp A and comp B inside one second). Appendcrypto.randomUUID().slice(0,8)or a nano counter. Also pre-existing; new surface raises the priority.
Verdict: REQUEST CHANGES
Reasoning: The CLI-adapter omission (blocker) makes the new UI silently misbehave for one of the two ways users launch studio, and the unsanitized composition body field is a clear defense-in-depth gap on the same route that already validates every other input.
— Vai
jrusso1020
left a comment
There was a problem hiding this comment.
Self-correction: deferring to Vai's REQUEST_CHANGES — my APPROVE was too soft on the CLI adapter blocker.
Vai caught the path-enum gap I missed: I traced the chain through studio/vite.adapter.ts (the dev path) and stopped, but there's a second consumer at packages/cli/src/server/studioServer.ts:227 that silently drops the composition field. So when users run via the bundled CLI's hyperframes preview (not vite), the per-comp render button silently renders the full project — UI says "rendering composition X," filesystem gets full-project output. Wrong-output failure mode is worse than no-feature.
Per my own path-enum-when-body-promises-global discipline: the body's "Threads composition path through the full render pipeline" is a global contract claim. Missing the CLI adapter is exactly a Rule-3 (body-vs-diff) blocker, not a non-blocking note. My APPROVE underweighted this.
I also undersold the path-traversal finding as non-blocking ("trust boundary bounded — user owns project"). Vai's escalation is right: hyperframes preview binds a network port, and an unsanitized composition field bypassing the enum-validation pattern that protects every other body field on this route is a real-arbitrary-read vector if that port is reachable. Validate against the project's known composition list before joining the path.
Vai's other findings (icon-only button a11y, isRendering queue semantics, zero tests for forwarding) also stand — the test-coverage gap is the kind of thing that catches CLI-vs-vite parity bugs like this one.
Treating my prior APPROVE as superseded by Vai's REQUEST_CHANGES. Happy to re-stamp once:
- CLI adapter (
studioServer.ts) is wired to honorcompositionthe same wayvite.adapter.tsdoes - Path-traversal guard added at the route layer
- Test coverage for the forwarding chain
— Self-correction by Rames Jusso (pr-review)
…, settings sync - Wire `composition` → `entryFile` in CLI studio adapter (studioServer.ts) so `hyperframes preview` renders the correct composition, not always index.html - Add path-traversal guard: reject composition paths that resolve outside projectDir - Add `aria-label` to the icon-only render button for screen readers - Add 4 tests: forwarding, empty/missing → undefined, path-traversal → 400 - Persist render settings (format/quality/fps) to localStorage so comp card buttons use the same settings as the Export panel
|
Addressed all review findings in 8b790a1: Blockers resolved:
Important items resolved:
Acknowledged but deferred (pre-existing):
|
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving on 8b790a18. All four items from the prior round are properly resolved.
Audited
- CLI adapter wired (
cli/src/server/studioServer.ts:266) —...(opts.composition ? { entryFile: opts.composition } : {})mirrors the exact conditional-spread shape invite.adapter.ts. Bundled-CLIhyperframes previewnow honors the new field the same way the vite dev adapter does. Vai's wrong-output blocker fixed. ✓ - Path-jailing at the route layer (
core/src/studio-api/routes/render.ts:80-87):Right shape —const resolved = resolve(project.dir, body.composition); if (!resolved.startsWith(resolve(project.dir) + sep)) { return c.json({ error: "composition path must be within the project directory" }, 400); }
path.resolvenormalizes../traversal, and the+ septrailing separator avoids the classic/foo/barfalsely matching/foo/barEvillong-sibling-prefix bug. 400 response is appropriate. ✓ - Four-case test matrix (
render.test.ts:117-194): forwards valid path ✓, omits when not specified ✓, omits when empty ✓, rejects../../../etc/passwdwith 400 ✓ — that last one pins the security boundary against future regressions. ✓ aria-labelon the icon button (CompositionsTab.tsx:213) — mirrors the existingtitle, so screen reader users get the same "Render {name}" / "Rendering..." labels. Cleans Vai's a11y nit. ✓- Persisted render settings synced across both surfaces (
RenderQueue.tsx+StudioLeftSidebar.tsx):- New
getPersistedRenderSettings()readsformat/quality/fpsfromlocalStorage["hf-studio-render-settings"]with enum-clamped fallbacks FormatExportButtoninitializes itsuseStatefrom persisted values, and each onChange writes back viapersistSettings(...)handleRenderCompositionnow readsgetPersistedRenderSettings()and passesformat/quality/fpsthrough torenderQueue.startRender({ composition, format, quality, fps })- Both surfaces read+write the same key, so settings stay in sync. This is the right architecture — the comp-card download now actually picks up what the user dialed in on the Renders panel, which is what Miguel originally claimed in Slack and is now true. ✓
- New
Body claim verification
- "Threads
compositionpath through the full render pipeline" — verified across BOTH adapters now (vite + CLI). ✓ - "composition-aware Export button" — unchanged from prior round, still verified. ✓
- "visible on hover" — body still says this, but the button is still rendered unconditionally when
onRenderis provided. Stale text; no opacity gating actually applies. Either delete "(visible on hover)" from the body or addopacity-0 group-hover/card:opacity-100 transition-opacityto the button. Cosmetic.
Non-blocking — the only theoretical edge case left
path.resolve doesn't follow symlinks, so if compositions/foo.html inside the project dir is a symbolic link to /etc/passwd, the path-startsWith check passes but the producer's readFileSync walks the symlink out of the project. Bypassable only by a user who already has filesystem write access to plant the symlink inside their own project dir — which is the same trust boundary as the file editor in the studio gives them anyway, so practical exposure is nil. Defense-in-depth would be fs.realpathSync before the startsWith comparison, but the cost-benefit isn't there. Mentioning for completeness.
CI
All required green on this commit: Test, Typecheck, Lint, Build, CodeQL, Test: runtime contract, CLI smoke (required), Tests on windows-latest, Render on windows-latest, Preview parity, preview-regression, Smoke: global install, Perf:drift/load/fps/parity/scrub, player-perf, Analyze (python), Analyze (actions). Regression-shards still in-progress; mergeable_state: blocked is reviewer-gate.
Withdrawing the self-correction COMMENT. Both consumer adapters updated, path-jail enforced + tested, a11y addressed, settings synced. Clean to merge once regression shards land green.
— Re-review by Rames Jusso (pr-review)
vanceingalls
left a comment
There was a problem hiding this comment.
Clean follow-up — blocker dead, security tight, tests cover the new contract. The remaining queue-disabling-everyone item is fine to land as a follow-up rather than block again.
Re-review status of prior findings
- blocker — CLI adapter not threading
composition— ADDRESSED.packages/cli/src/server/studioServer.ts:266now spreads...(opts.composition ? { entryFile: opts.composition } : {})intocreateRenderJob, matching the vite adapter pattern exactly.hyperframes previewusers will now get the correct comp. - important — path-traversal on
composition— ADDRESSED.packages/core/src/studio-api/routes/render.ts:80-87resolves againstproject.dirand assertsstartsWith(resolve(project.dir) + sep). Verified against the attack matrix:../../etc/passwd, leading/, embedded.., and the prefix-match case (../proj-evil/fooagainstprojectDir=/home/user/proj) all correctly reject. The trailingsepis exactly what closes the prefix-match hole. - important —
isRenderingis a global busy-flag — STILL OPEN.useRenderQueue.ts:isRenderingis unchanged (jobs.some(j => j.status === 'rendering')), andStudioLeftSidebar.tsx:121passes that single boolean to every card viaCompositionsTab. Clicking comp A still disables every other comp's render button until A finishes. Not a release-blocker — the queue still functions — but the per-button UI keeps implying parallelism it doesn't have. Reasonable follow-up: track aSet<string>of in-flight comp IDs inuseRenderQueueand drive each card'sdisabledfrom membership in that set. - important — no tests for
compositionforwarding — ADDRESSED.render.test.ts:120-196adds four pinning tests: valid path forwards through, missing → undefined, empty string → undefined,../../../etc/passwd→ 400 with the adapter spy unreached. Exactly the shape requested. - important — icon-only button has no accessible name — ADDRESSED.
CompositionsTab.tsx:212addsaria-label={isRendering ? "Rendering..." :+ "Render ${name}}" + `. Title stays for sighted hover. Good split.
Fresh findings on the new code
- nit —
getPersistedRenderSettings()exported fromRenderQueue.tsx. Module-level helper exported from a JSX file is slightly awkward — if you ever code-split this component, the helper drags the JSX module along. Inlining or moving torenders/settings.tswould be cleaner, but no need to block on it. - nit — comp-card render reads localStorage at click time, not from React state (
StudioLeftSidebar.tsx:50-57). If a user changes settings in another browser tab, the comp-card render picks up the cross-tab value while the visible Export panel still shows the in-state value. Unlikely to bite in practice and worse fixes than the current code, leave as-is. - nit — pre-existing
jobIdsecond-resolution collision still present atrender.ts:92. With per-comp buttons now in the UI, two clicks inside the same wall-second still collide. Same recommendation as last round (appendcrypto.randomUUID().slice(0,8)or a nano counter), still pre-existing, can ride a follow-up.
Verdict: APPROVE
Reasoning: Blocker resolved, security finding closed with a correct ancestor check (verified against attack vectors), tests pin the new contract end-to-end, a11y addressed. The remaining queue-disabling-everyone behavior is a UX nit that's safe to ship as a follow-up rather than gate this PR again.
Review by Vai (re-review)
Move getPersistedRenderSettings/persistRenderSettings out of RenderQueue.tsx into renderSettings.ts so code-splitting the component doesn't drag along the helper.
…g, duration heuristic Based on testing the skill with subagents on HyperFrames PR #874 and Pacific PR #27684: - Emphasize that manifest schema must match build.mjs exactly (agents were inventing their own slide structures) - Add duration estimation formula: ~2.5 words/second - Specify which design.md tokens to extract for branding - Add GitHub org avatar as fallback logo source - Adjust narration length guidance for small vs large PRs
Summary
compositionpath through the full render pipeline: POST body → API route → adapter → producerentryFileindex.htmlChanges
core/src/studio-api/types.tscomposition?: stringonstartRenderoptscore/src/studio-api/routes/render.tscompositionstudio/vite.adapter.tscomposition→entryFilefor producercli/src/server/studioServer.tscomposition→entryFilefor producerstudio/src/components/renders/renderSettings.tsstudio/src/components/renders/useRenderQueue.tscompositioninStartRenderOptionsstudio/src/components/sidebar/CompositionsTab.tsxLeftSidebar.tsx,StudioLeftSidebar.tsx,StudioRightPanel.tsxTest plan
../../../etc/passwd→ rejected with 400