feat(studio): full Blocks panel — browse, search, add, drag-and-drop registry items#947
Conversation
miguel-heygen
force-pushed
the
feat/studio-blocks-panel
branch
from
a8ae933 to
6549f6c
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (dryRun) { | ||
| console.log(`[dry-run] ${entry.name}: would set preview →`, preview); | ||
| } else { | ||
| writeFileSync(manifestPath, JSON.stringify(manifest, null, 2) + "\n", "utf-8"); |
| if (dryRun) { | ||
| console.log(`[dry-run] ${entry.name}: normalize string → { video: "${videoUrl}" }`); | ||
| } else { | ||
| writeFileSync(manifestPath, JSON.stringify(manifest, null, 2) + "\n", "utf-8"); |
| const res = await fetch(`/api/projects/${projectId}/registry/install`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ blockName }), | ||
| }); |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE — env flag default-false confirmed. Two flag-coverage gaps worth fixing before flipping to default-true (non-blocking for internal-test-only as stated).
Default-false confirmation (per Home's scrutiny ask)
// packages/studio/src/components/editor/manualEditingAvailability.ts:60
export const STUDIO_BLOCKS_PANEL_ENABLED = resolveStudioBooleanEnvFlag(
env,
["VITE_STUDIO_ENABLE_BLOCKS_PANEL", "VITE_STUDIO_BLOCKS_PANEL_ENABLED"],
false, // ← default ✓
);resolveStudioBooleanEnvFlag walks the names, returns fallback if none resolve. In non-Vite hosts (Next.js, Node, jest), import.meta.env is undefined → env = {} → fallback returned. Default-false holds across all hosts. ✓
Flag-gate coverage audit (per Home's scrutiny ask — "checked in one place but bypassed in another")
Client-side gates that DO check the flag:
LeftSidebar.tsxgrid template:STUDIO_BLOCKS_PANEL_ENABLED ? "1fr 1fr 1fr 1fr" : "1fr 1fr 1fr"✓- Tab button render:
{STUDIO_BLOCKS_PANEL_ENABLED && (<button>Blocks</button>)}✓ - Tab content render:
{STUDIO_BLOCKS_PANEL_ENABLED && tab === "blocks" && onAddBlock && (<BlocksTab/>)}✓
Two gaps:
1. getPersistedTab() doesn't check the flag → blank-panel bug
function getPersistedTab(): SidebarTab {
const stored = localStorage.getItem(STORAGE_KEY);
if (stored === "assets") return "assets";
if (stored === "code") return "code";
if (stored === "blocks") return "blocks"; // ← flag not checked
return "compositions";
}Scenario: user enables the flag in dev, navigates to the Blocks tab → localStorage saves "blocks". Flag later turned off (e.g. deploying to a non-internal env, or rolling back). On next mount:
tabstate initializes to"blocks"- Tab button doesn't render (flag-gated)
- Tab content doesn't render (flag-gated)
- Result: empty sidebar with no UI affordance to recover (no Blocks button visible to "select another tab from")
Fix:
if (stored === "blocks" && STUDIO_BLOCKS_PANEL_ENABLED) return "blocks";Or auto-fall-through to "compositions" when flag-off + stored-blocks.
2. Server-side endpoints bypass the flag
Both adapters (vite.adapter.ts and studioServer.ts per the Rule-2 canonical pair) unconditionally expose listRegistryCatalog + installRegistryBlock. The shared API route (core/src/studio-api/routes/registry.ts) returns 501 only when the adapter methods are absent — they're always present, so:
GET /api/registry/blocks→ always lists all 78 catalog itemsPOST /api/projects/:id/registry/install→ always installs a block into the project (writes files to disk, mutates the composition HTML viainsertTimelineAssetIntoSource)
For internal-test-only intent, this is consistent: dev users WILL want to poke endpoints. The flag gates UI iteration, not security. But if the intent ever shifts to "users shouldn't install blocks until we ship," the gap means anyone with API knowledge can still hit curl POST /api/projects/X/registry/install -d '{"blockName": "us-map"}' and mutate the project.
Defense-in-depth fix: thread STUDIO_BLOCKS_PANEL_ENABLED (or a server-side env equivalent) into the adapter constructor and 404 the routes when off:
async listRegistryCatalog() {
if (!process.env.HYPERFRAMES_ENABLE_BLOCKS_PANEL) return []; // or throw
// ...
},Either way, worth flipping back on as part of "ship-ready" criteria when you're ready to default-true the UI.
Audited
Rule-2 contract widening (per the canonical vite.adapter.ts ↔ studioServer.ts pair):
- New
StudioApiAdaptermethods:listRegistryCatalog?,installRegistryBlock? - Both adapters implement both methods ✓
- Shared route handler (
core/src/studio-api/routes/registry.ts) treats them as optional via theif (!adapter.X) return 501check - Studio API types updated (
packages/core/src/studio-api/types.ts)
Path-traversal guard in vite.adapter.installRegistryBlock:
if (!isPathWithin(opts.project.dir, targetPath)) {
throw new Error(`Target path escapes project directory: ${file.target}`);
}Per-file check before write. ✓
blockInstaller.ts client-side:
- For components: regex-strips dark backgrounds → transparent. Hardcoded set (
#0a0a0a,#000000,#000,#0a0805,rgba(...)). Brittle: future components with a different background color or inlinestyle="background:..."won't be patched. The captioning team's recent component additions might already hit this. Worth a TODO or a more general regex. buildUniqueCompositionId(baseName, existingIds)increments suffix_2,_3, ... on collision. Reasonable.- For blocks: appends at
max(start + duration)of existing elements. For components: overlays atstart=0spanning full duration. Matches the body's described behavior. ✓
CDN-hosted preview in BlocksTab.tsx (+289): each card video URL comes from the manifest's new preview: { video, poster } field. Manifests backfilled across 68 registry-item.json files (+428/-70 total). Body says 78; actual file count is 68 — minor body drift but the substantive change is consistent.
CI
All required checks ✓: Format, Lint, Build, Test, Typecheck, Preflight, CLI smoke, Test: runtime contract, Smoke: global install, Render on windows-latest, Tests on windows-latest. mergeable_state: blocked is the reviewer gate.
— Rames
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Studio Blocks panel — solid scope and the env-flag-default-false rollout is the right call. The flag gating is clean on the UI surface (tab button, BlocksTab, BlockCard, drag-source all behind STUDIO_BLOCKS_PANEL_ENABLED). Calibrated strengths and findings below; verdict is COMMENT because the flag-off-by-default ships the new code without enabling it, so blockers can land + flipped-on-after-fix without gating the merge. The blockers must be fixed before the flag is enabled in any environment, including internal.
No prior reviewer findings on the PR (only CodeQL bot inline comments, addressed below).
Calibrated strengths
- Path-safety check on the install endpoint (
packages/studio/vite.adapter.ts:307callingisPathWithin(opts.project.dir, targetPath)) is the right belt-and-suspenders defense alongside the CLI'sassertSafeTargetPathininstaller.ts. Good instinct on a server endpoint that writes files into project dirs. resolveBlockCategory(packages/core/src/registry/types.ts:201) is pure, tag-driven, and centralized — no per-component category strings to keep in sync. Easy to test.- The 300ms
hoverTimeronBlockCard(packages/studio/src/components/sidebar/BlocksTab.tsx:1113) before swapping in the<video>is the right call — without it, mousing across the 78-card grid would slam the CDN and decoder.
Findings
Blockers
1. Component transparent-background patcher is over-broad and will mangle 7+ caption components on install. (packages/studio/src/utils/blockInstaller.ts:93-99)
const transparentContent = compContent.replace(
/background:\s*(?:#(?:0a0a0a|000000|000|0a0805)|rgba?\([^)]*\))\s*;/g,
"background: transparent;",
);The intent is clear (make the outermost composition root transparent so the component overlays its host). But rgba?\([^)]*\) is unanchored and matches any background: rgba(...) anywhere in the file, including intentional in-component decorations. Verified at the PR head:
caption-highlight.html:46—.hl-overlay { ... background: rgba(0, 0, 0, 0.4); ... }is the intentional dim-the-video-under-the-captions overlay. After install, that becomesbackground: transparent;— overlay disappears, captions lose their contrast scrim.- Same shape in
caption-clip-wipe(0.45),caption-kinetic-slam(0.45),caption-matrix-decode(0.85),caption-neon-glow(0.75),caption-particle-burst(0.45),caption-texture(0.55). - The solid-hex side (
#0a0a0a,#000000,#000,#0a0805) is whitelisted to specific values but also unanchored — any inner element using one of those for a decoration (e.g., a.pill { background: #000000 }) gets wiped too.
The failure mode is silent — no error, just visually-wrong output that the user can't easily diagnose. This is the kind of bug that wastes internal-testing cycles.
Fix shape (one approach): parse the HTML and only rewrite the background: declaration on the outermost <style> html, body { ... } and the root container rule, not a global regex. Or have the registry manifest declare an explicit rootSelector (e.g., #hl-container) and only rewrite that block. Or — simpler — do this transform inside the component's source so installed files are already transparent-rooted, no install-time patching at all.
This must be fixed before the flag is flipped on, even in internal testing. The whole point of the panel is "install caption components onto a host" — and 7 of ~14 caption components have a translucent overlay that the patcher destroys.
Important
2. Server-side install endpoints are not gated by the env flag. /api/registry/blocks and /api/projects/:id/registry/install are registered unconditionally in packages/core/src/studio-api/createStudioApi.ts:30 and packages/core/src/studio-api/routes/registry.ts. The env flag only suppresses the UI surface. Any caller hitting those endpoints (a stale tab, a script, an MCP agent) can install regardless of the front-end flag. Not a security blocker (the CLI's hyperframes add does the same thing, and the path-safety check holds), but the PR description framing is "internal testing only via flag" — the server doesn't honor that. Either gate the route registration behind a server-side env flag too, or update the PR description to clarify the gating is UI-only.
3. recentBlocks field is read but never written. packages/studio/src/utils/studioUiPreferences.ts:1705,1713-1717 adds a recentBlocks?: string[] field and a read-path filter, but no code path in this diff writes to it. Dead-on-arrival — either wire up the write (presumably from addBlockToProject success) or drop the field until you need it. Either way, ship it intentional.
4. getPersistedTab() returns "blocks" regardless of the flag. packages/studio/src/components/sidebar/LeftSidebar.tsx:24 — if a flag-on user picks the Blocks tab, then the flag is later flipped off (e.g., for incident rollback), localStorage still says "blocks" and tab initializes to "blocks". The rendered tabs row drops the Blocks button (gated by flag) and the body renders nothing (the STUDIO_BLOCKS_PANEL_ENABLED && tab === "blocks" && onAddBlock gate short-circuits). User sees an empty sidebar with no obvious way to recover. Fix: in getPersistedTab(), fall through to "compositions" when stored === "blocks" and the flag is off.
5. No tests added for the new mutating code. blockInstaller.ts (the file that writes to project HTML), useBlockCatalog, resolveBlockCategory, and the new server route all ship without tests. resolveBlockCategory is pure and trivial to test (just feed tag arrays through it — vfx vs scenes vs captions). The flag-off rendering path (LeftSidebar with flag off should not render the Blocks button or BlocksTab) is also untested, and that's the safety net we're relying on to ship this. Add at minimum: resolveBlockCategory snapshot test, addBlockToProject happy-path + the regex-patch unit test (which would have caught blocker 1), and a LeftSidebar render test with flag off.
6. A11y gaps on the new UI. BlocksTab.tsx:
- Search input has no
<label>oraria-label(line ~985). - Category pills are buttons-in-a-row but not a
role="tablist"/aria-pressedpattern — keyboard arrow-key navigation between categories doesn't work; you can only tab through them. BlockCardclose button (inBlockParamsPanel.tsx:771) has noaria-label="Close"— screen-readers announce a meaningless SVG button.- Loading and error states (
BlocksTab.tsx:951,958) have noaria-liveregion — the loading-to-loaded transition is silent.
These are catchable now with one pass; cheaper than retrofitting after the flag is on.
7. CodeQL findings on the PR head are not addressed. Three alerts (visible on the PR):
Client-side request forgeryonblockInstaller.ts:64(thefetch(\/api/projects/${projectId}/registry/install`)URL interpolation). Same-origin so SSRF isn't possible; the alert is technically a false positive in this architecture, but it should be dismissed in CodeQL with a reasoning note rather than left red on the PR.projectIdcomes from internal Studio routing state, not user input — document that in a// codeql-suppress-justification` comment or dismiss the alert.- Two
Potential file system race conditionalerts onscripts/backfill-block-previews.ts:65,92. The script is a one-shot backfill, but worth a glance to confirm.
CodeQL is currently the only failing check (1 FAILURE, no other reds). Not branch-protected so not blocking merge, but it shouldn't ship red.
Nits
8. console.log in BlockParamsPanel.handleChange (BlockParamsPanel.tsx:761) — debug log shouldn't ship to internal users. Remove or wire to the param-apply pipeline once Phase 3 lands.
9. Unused destructured blockName in BlockParamsPanel (line 749) — declared in the props interface but not consumed in the component body.
10. addBlockToProject "10s fallback duration" for components on an empty host (blockInstaller.ts:153) — the Math.max(..., 10) floor is arbitrary. Probably fine for the common case, but worth a comment explaining the intent (or pull the literal into a named constant).
11. .filesize-allowlist additions (App.tsx, Timeline.tsx) — both files are already big and this PR adds to them. Not a blocker, but worth noting that the right long-term move is to keep extracting from App.tsx (which is now appending more handler wiring on top of the recent context refactor) rather than continuing to grow + allowlist.
12. useBlockCatalog fetch has no AbortSignal (useBlockCatalog.ts:1339) — uses a cancelled ref to drop late results, which is fine, but doesn't actually abort the network request. Minor.
Verdict
Verdict: COMMENT
Reasoning: The env-flag-default-false rollout is the right call and makes this safe to land without enabling. The blocker (over-broad transparent-bg regex) and the two other important items (server-side gating, dead recentBlocks, persisted-tab fallback, missing tests, a11y, CodeQL alerts) all need to be addressed before the flag is enabled in any environment, including internal testing — fixing them in this PR or a fast follow-up is fine.
Review by Vai
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Acknowledging Vai's regex-over-match catch — that's the substantive form of my "brittle regex" note.
My review called the background-strip regex "brittle" because it wouldn't catch future components with non-listed colors. Vai found the more important direction: the rgba?\([^)]*\) pattern is unanchored and matches any rgba background, including legitimate translucent scrims:
const transparentContent = compContent.replace(
/background:\s*(?:#(?:0a0a0a|000000|000|0a0805)|rgba?\([^)]*\))\s*;/g,
"background: transparent;",
);Vai's grep of HEAD found 7 caption components with rgba(0,0,0,0.4)-style intentional scrims (caption-highlight, caption-clip-wipe, caption-kinetic-slam, caption-matrix-decode, caption-neon-glow, caption-particle-burst, caption-texture). When any of those are installed via Add, the scrim becomes fully transparent — silent visual breakage on flag-flip-on.
The miss shape on my end: I noted the forward direction (regex won't match future colors) but didn't audit the match set against existing components. For pattern-replacement regex, both narrow-miss and wide-over-match are common failure modes; a quick git grep "background:\s*rgba" in the registry would have surfaced the scrims immediately.
Fix Vai suggested: anchor to alpha=0 explicitly:
/rgba\(\s*\d+\s*,\s*\d+\s*,\s*\d+\s*,\s*0(?:\.0+)?\s*\)/…which matches rgba(0,0,0,0) and rgba(0,0,0,0.0) etc. but leaves translucent scrims intact.
Vai's other flagged items align with mine: server-side route gate, getPersistedTab() flag check, no tests for the new modules, a11y gaps. Both reviews converge: env-flag-default-false rollout is the right call to ship today, but the regex anchor + server-route gate are must-fix before flipping the flag on. My APPROVE was sized for "ship today with flag off" — Vai's COMMENT captures the additional must-fix-before-flip-on punch list, which is the more useful framing.
Defer to Vai as primary on the re-review when the regex anchor + server gate land.
— Rames
miguel-heygen
force-pushed
the
feat/studio-blocks-panel
branch
4 times, most recently
from
30033c2 to
ec1cf97
Compare
…registry items
Adds a Blocks tab to the Studio left sidebar with the full 78-item registry
catalog (58 blocks + 20 components). Users can browse by category, search by
title/description, preview CDN-hosted poster thumbnails with video-on-hover,
and install items on-demand with one click or drag-to-timeline.
Core changes:
- BlockCategory type + resolveBlockCategory() for 7 categories (Captions, VFX,
Transitions, Effects, Social, Data, Scenes)
- Registry API routes: GET /api/registry/blocks (catalog) + POST install
- StudioApiAdapter extended with listRegistryCatalog + installRegistryBlock
- Vite adapter reads from disk; CLI adapter fetches from GitHub (24h cache)
- BlockParam interface + params on 6 blocks for future parameter controls
Studio UI:
- 4th sidebar tab "Blocks" with responsive grid, category pills, search bar
- BlockCard: CDN poster thumbnail, video autoplay on hover, duration + WebGL badges
- On-demand install: blocks append as sub-compositions on timeline; components
overlay at start=0 spanning full duration with transparent background patching
- TIMELINE_BLOCK_MIME drag-and-drop to timeline
- BlockParamsPanel (Phase 3 scaffold) auto-opens for parameterized blocks
Registry manifests:
- All 58 blocks backfilled with preview: { video, poster } CDN URLs
- All 20 components normalized to object format + poster URLs added
- 6 blocks annotated with params (Liquid Glass/Background, Portal, Chart,
Logo Outro, Magnetic)
- flowchart-vertical preview generated and uploaded to CDN
miguel-heygen
force-pushed
the
feat/studio-blocks-panel
branch
from
ec1cf97 to
ffbc18a
Compare
4 participants
Summary
hyperframes preview(fetches from GitHub with 24h cache)preview: { video, poster }CDN URLsTest plan
hyperframes preview(outside monorepo) — Blocks tab loads from GitHub registrybun run buildpasses