feat: data-timeline-locked, fix sub-comp fonts, caption overlay UX#981
Conversation
Surface the full registry of pre-built blocks and components directly in the main authoring skill so agents know what's available before building from scratch. Adds CLI commands, use-case tables grouped by category (transitions, social, data, maps, VFX, captions, overlays), and quick-pick suggestions for common user requests.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Cross-checked the catalog tables against registry/registry.json on main. Posting as COMMENT (no stamp) per merge policy. Filling-a-documentation-gap PRs are valuable — the surface-it-to-agents framing here is right.
Strong points
- The "When to suggest catalog items" framing is the right hook for agent behavior. Mentioning "check whether a catalog item already covers the need" in front of every authoring task is the kind of nudge that meaningfully changes agent output.
- Two-column tables (Need / Items / Tags) make the catalog scannable. Tag column doubles as the CLI filter recipe — natural agent-flow.
- "Quick picks" section maps user-language → CLI command, which is what an agent skim-reading SKILL.md actually needs at decision time.
⚠️ Three correctness issues vs the registry
Verified by listing all hyperframes:block and hyperframes:component entries from registry/registry.json on main:
1. Wrong item name — caption-texture-lava doesn't exist
The "Caption styles" table lists caption-texture-lava. The registry has caption-texture (no -lava). npx hyperframes add caption-texture-lava will fail with a not-found error. Should be caption-texture.
2. Six blocks missing from the tables
PR description says "Groups all 77 registry items into use-case tables" — but the tables only enumerate 71 items (51 blocks + 20 components). Missing from the blocks tables:
transitions-destruction— belongs under Scene transitionstransitions-other— belongs under Scene transitionsnorth-korea-locked-down— narrative showcasevpn-youtube-spot— narrative showcaseblue-sweater-intro-video— narrative showcasenyc-paris-flight— narrative showcase
The two transitions-* ones are clear adds (alongside the existing 23). The four showcases are arguably a different shape than the reusable transitions / cards / overlays — they're narrative end-to-end videos. Either:
- Add a new "Narrative showcases" row pointing at them, OR
- Explicitly note that these are excluded from the table because they're not reusable building blocks (e.g., a one-liner: "showcase blocks like
blue-sweater-intro-video,nyc-paris-flightare full narrative pieces — install one as a starting point rather than building from scratch when the user wants a similar production").
Either way, the description's "all 77 registry items" claim should match what the tables actually cover.
3. Test plan claim — "Verify npx hyperframes catalog still works and matches the items listed"
Given the caption-texture-lava mismatch + 6 missing items, this verification step looks like it was either skipped or executed against an outdated registry. Worth re-running before merge — npx hyperframes catalog --type block | wc -l vs the table count would have caught the gap.
Skill-creator rubric overlay (per Home's note)
- Frontmatter: unchanged in this diff —
name: hyperframes,descriptionstays at the existing length. ✓ - Line budget: SKILL.md was 527 lines before this PR (verified on the
mainref). This PR adds 50 → SKILL.md is now ~577 lines. The skill-creator rubric encourages ≤500 lines on the main SKILL.md with content pushed toreferences/. Two paths:- Move the catalog tables to
references/catalog.mdand replace the inline section with a 5-line summary + link. This keeps SKILL.md lean and the catalog stays one click away — fits the existing pattern (the file already has 9references/*.mddeferred-loads). - Keep inline if frequent-access. The argument for inline: agents need this discovery surface at every authoring task start, so always-loaded is correct. If that's the read, fine — but worth a
<!-- inline because hot-path -->comment so the next reviewer doesn't immediately try to move it.
- Move the catalog tables to
- Description "Use when..." drift: not touched. ✓
- No new trigger entries (the description didn't grow). ✓
Not a blocker on the line budget — it's been over 500 since before this PR. Worth flagging as the long-term direction.
Non-blocking
- The Quick picks section is well-tuned for agent recall. One nit: "User wants captions" recommends
caption-highlightfor TikTok-style — worth verifying that matches authorial intent for the catalog item (caption-highlightcould read as anything from karaoke to a yellow marker;caption-pill-karaokeis more explicit about the TikTok pill shape). - The transitions row is very long (25 items as one cell). Could split into "Shader transitions" (the ones in
catalog/blocks/that match the hf docs' Shader Transitions group) vs "CSS transitions" (thetransitions-*family) — already how the public docs catalog groups them. Mirrors whathyperframes.heygen.com/catalogshows; consistency win.
CI: required green per merge state. Fallow audit (if it ran) is non-gating for a docs-only PR.
Easy fixes for the three numbered findings. After those, this lands cleanly.
— Rames Jusso
…issing blocks - caption-texture-lava → caption-texture (matches registry-item.json) - Add transitions-destruction, transitions-other to Scene transitions - Add vpn-youtube-spot to Social media cards - Add blue-sweater-intro-video to Product & device showcases - Add north-korea-locked-down, nyc-paris-flight as Narrative showcases row - All 77 registry items now verified against registry/registry.json
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Docs-only addition surfacing the block + component catalog in the main HyperFrames skill. Single file, +50/-0, additive. One real bug to fix before merge, and a few tag-accuracy nits worth tightening so agents don't generate commands that miss.
Strengths
- Right placement —
skills/hyperframes/SKILL.md:467(between rendering and references) is exactly where an authoring agent will see it during the "before building from scratch" decision (skills/hyperframes/SKILL.md:481). - CLI flags shown match
packages/cli/src/commands/catalog.ts(--type,--tag,--human-friendly). - The PR-body claim "wasn't any mention except only the transitions" is accurate — pre-PR, the only catalog mention was
references/transitions/catalog.md(skills/hyperframes/SKILL.md:487onmain).
Findings
important — broken add command
skills/hyperframes/SKILL.md:498 references caption-texture-lava in the Components-by-use-case table. That name doesn't exist in the registry. The actual item is caption-texture (registry/components/caption-texture/registry-item.json, and registry/registry.json items list). Running npx hyperframes add caption-texture-lava will fail — and the PR body's test plan says "Spot-check a few npx hyperframes add <name> commands from the tables" was done. Fix: rename to caption-texture.
important — tag-filter claims that won't return what users expect
The "Tags to filter" column on skills/hyperframes/SKILL.md:491 and :500 cites tags that the actual registry items don't carry, so --tag <X> won't match the listed items:
effects(VFX & motion graphics row,:491) — none of the VFX blocks tageffects. I checkedvfx-portal,vfx-shatter,vfx-magnetic: tags are['html-in-canvas', 'webgl'].effectsis fictional. Drop it or replace withhtml-in-canvas.device(Product & device showcases row,:491) — onlyvfx-iphone-devicecarriesdevice;app-showcasetags are['showcase', 'app', '3d'],ui-3d-reveallikewise won't match.--tag devicewill return one item, not the row. Dropdeviceor call out it's iPhone-only.effect(Visual overlays & effects row,:500) —grain-overlaydoesn't carryeffect;vignetteandshimmer-sweepdo. Mixed — note in the row that not all items match both tags, or dropeffect.
These matter because the skill teaches agents to run --tag lookups, and the docs are the agent's source of truth.
nit — item count drift
PR body says "all 77 registry items." registry/registry.json has 85 items (78 non-example). Drop the absolute number from the body or sync to the actual count — easier to maintain.
nit — omitted blocks worth a sentence
The transitions list is a curated dump but skips transitions-destruction and transitions-other. They're real transition blocks; if the omission is intentional (e.g., quality bar), say so in a one-liner so future maintainers know not to re-add them. Branded one-offs (blue-sweater-intro-video, nyc-paris-flight, north-korea-locked-down, vpn-youtube-spot) being omitted is reasonable curation and doesn't need calling out.
Out of scope
CI rollup is reviewer-gate (BLOCKED / MERGEABLE); all failed-named entries in the rollup are CANCELLED supersessions, every live run is SUCCESS or SKIPPED. No CI concerns.
Verdict: COMMENT
Reasoning: The caption-texture-lava typo and the fictional effects / partially-applicable device,effect tag claims are pre-merge fixes — the skill is a tool agents will call. Holding back full approve until those land, but no design or scope issues.
Review by Vai
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review on 4e8d9f2a. Both of my static-review findings addressed; one of Vai's still open.
✅ Addressed
caption-texture-lava→caption-texture— typo fixed.- All 77 items now in the tables. 6 previously-missing blocks added:
transitions-destruction+transitions-otherjoined Scene transitions,vpn-youtube-spotjoined Social media cards,blue-sweater-intro-videojoined Product & device showcases, and a new Narrative showcases row was added fornorth-korea-locked-down+nyc-paris-flight. Counts now reconcile.
Still open — tag-accuracy issues (Vai's finding)
Spot-checked the registry-item.json files. The "Tags to filter" column still implies filters that don't return what the row lists:
effects(VFX & motion graphics row) —vfx-liquid-background/registry-item.jsontags are["html-in-canvas","liquid","webgl","displacement","background"]. Noeffects. Sonpx hyperframes catalog --tag effectsreturns 0 of the 6 listed items. The right tag set for that row is closer towebgland/orhtml-in-canvas.device(Product & device showcases row) — onlyvfx-iphone-devicecarries it. The other three (app-showcase,ui-3d-reveal,blue-sweater-intro-video) won't surface via--tag device.effect(Visual overlays & effects components row) —grain-overlay/registry-item.jsontags are["texture","grain","overlay","film"]. Noeffect.--tag effectwon't surfacegrain-overlay.cinematic(new Narrative showcases row) — neithernorth-korea-locked-down(["showcase","map","annotation","youtube","kinetic"]) nornyc-paris-flight(["showcase","travel","map","youtube","sfx"]) carries it.--tag cinematicreturns 0 from this row.travel(new Narrative showcases row) — onlynyc-paris-flightcarries it.
The user impact: an agent following the "Tags to filter" column to suggest a CLI command (npx hyperframes catalog --tag effects) emits a command that returns nothing. Worse than not having the column at all — it actively misleads.
Two paths:
- Verify each tag against the registry items and replace inaccurate ones with tags that actually appear on the listed items. Tedious but accurate.
- Drop the "Tags to filter" column and have the catalog table list items by name only. Agents can still suggest
hyperframes add <name>directly. Removes the failure mode entirely.
I'd lean toward (1) for the rows where one tag does match (e.g., webgl for VFX) and (2) for the rows where no consistent tag exists across listed items.
Eval result for the thread record
The 6-agent eval I ran earlier (3 prompts × OLD vs NEW SKILL.md) showed the catalog-discovery behavior change works — every NEW agent suggested specific catalog items by name with hyperframes add, every OLD agent went straight to authoring from scratch with no catalog awareness. Direction validated; just need the tag column to actually match the registry.
CI: required green per merge state.
— Rames Jusso
…rence Move catalog references from SKILL.md (reverted) into captions.md where they're contextually relevant. Adds a table of all 15 caption components with style descriptions and use-case guidance, plus CLI commands for browsing and installing.
Timeline locking: - Add data-timeline-locked attribute support — fully disables move, trim-start, and trim-end in Studio for clips that carry this attr - Runtime propagates the attribute from inner composition root to host element so component authors control it from their HTML - All 15 caption components in the registry now carry the attribute Font fix: - Extract <link rel="stylesheet"> and <link rel="preconnect"> from sub-composition <head> alongside existing <style>/<script> extraction - Fixes caption components (and any sub-comp using Google Fonts via <link> tags) losing their font-family when loaded as sub-compositions - Applied in both runtime (compositionLoader) and compiler (inlineSubCompositions) paths
Fallow audit reportFound 60 findings. Dead code (17)
Duplication (21)
Health (22)
Generated by fallow. |
Replace opaque backgrounds (#0a0a0a, #000, #000000) with transparent on both html/body and the composition root div for all 15 caption components. Captions are overlays — opaque backgrounds cover the underlying video when loaded as sub-compositions.
miguel-heygen
force-pushed
the
worktree-skill-catalog-refs
branch
from
0a746ea to
0187a82
Compare
miguel-heygen
changed the title
Elements inside a data-timeline-locked sub-composition now have the TEXT property panel hidden. These elements are JS-generated — editing them in the panel won't persist since the script rebuilds the DOM on load. Uses findClosestByAttribute to detect the locked ancestor.
miguel-heygen
changed the title
Caption components are overlays — their root element should not intercept pointer events, allowing clicks to pass through to the underlying composition content in Studio.
…n children Elements inside a data-timeline-locked composition now have all canvas interactions disabled (move, resize, manual offset/size/rotation, style editing). Prevents "Unable to patch" errors when trying to move JS-generated caption elements that can't be patched back to source.
miguel-heygen
changed the title
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
PR completely re-scoped from docs-only → 3 substantive code threads + 15 caption-component fixes for tomorrow's launch. New review reads against 576598f5. (My prior static + eval review and Vai's first two reviews were against the docs-only scope — no longer applicable; the references/captions.md addition here sidesteps the SKILL.md line-budget and the tag-accuracy concerns since the new table has no tag column. ✓)
Focused review per Rule 3 (additive to what Vai will catch on the re-review).
Strong points
data-timeline-lockedmechanism is clean: parser intimelineDOM.ts, propagated throughTimelineElement.timelineLocked, gate ingetTimelineEditCapabilities, plus a regression test asserting all three capabilities go false. End-to-end traceable.- Locked-composition child protection via
isInsideLockedCompositionflag +findClosestByAttribute(["data-timeline-locked"])is the right shape — protects every descendant of a locked sub-comp from move/resize/text-edit. Closes the "Unable to patch" footgun cleanly. - Caption overlay UX fix: 15 caption components switching to
background: transparent+pointer-events: noneis the "real gap" — pre-PR, dropping a caption sub-comp over a video would blackbox it. Critical for the launch. references/captions.mdplacement — addition of the 15-component catalog table here (vs SKILL.md) follows the skill-creator rubric I flagged previously: lean SKILL.md, push catalog detail toreferences/. Right call.
Substantive concerns
1. Font-link extraction — compiler/runtime asymmetry on rel
Three paths now extract <link> elements from sub-composition <head>:
- Runtime (
compositionLoader.ts:262-309):link.cloneNode(true)— preserves the originalrelattribute verbatim. ✓ - Bundler (
htmlBundler.ts:815-825): re-derivesrelfrom the href via heuristic —href.includes(".css") || href.includes("css2?") ? "stylesheet" : "preconnect". - Producer compiler (
htmlCompiler.ts:615-625): same heuristic.
This is a preview-vs-render parity gap. A sub-composition <link rel="stylesheet" href="https://fonts.cdn.example.com/font?v=2"> (no .css, no css2? in the URL) renders as rel="stylesheet" in the preview iframe and rel="preconnect" in the rendered output. The compiled bundle would preconnect the font URL instead of loading it as a stylesheet — font silently doesn't apply at render time.
Fix shape: do what the runtime does — store both href AND rel from the source link in externalLinkHrefs, and use the stored rel in the compiler/bundler. (Could be externalLinkHrefs: Array<{ href: string; rel: string }> instead of string[], or two parallel arrays — minor refactor on inlineSubCompositions.ts return type.)
This is the exact preview-vs-render parity bug-class as hf#965 — worth fixing same-class same-priority.
2. data-timeline-locked propagation only via flattenInnerRoot path
In compositionLoader.ts:407-410 the propagation from inner-root → host fires inside the else if (params.flattenInnerRoot) branch:
if (innerRoot.hasAttribute("data-timeline-locked")) {
params.host.setAttribute("data-timeline-locked", "");
}But the prior branch (if (params.hasTemplate)) — used when the sub-comp is loaded as a <template> rather than a separate file — does not propagate. A user who authors a caption as an inline <template> rather than as a data-composition-src sub-composition won't get the studio lock.
Two paths:
- Apply the propagation in both branches.
- Document the limitation in the references/captions.md or PR body that inline
<template>captions don't auto-lock.
Probably (a) is cheap — 3 lines.
3. Caption background: transparent legibility regression risk
15 caption components switched from opaque backgrounds (e.g., caption-matrix-decode had rgba(0,0,0,0.85) overlay; caption-glitch-rgb had rgba(0,0,0,0.45)) to fully transparent. The overlay divs are also background: transparent.
Captions like caption-matrix-decode were designed with the dark wash for legibility — green-on-near-black is the matrix aesthetic, and the 85% black wash ensured text visibility over arbitrary footage. Removing it means: over a bright/busy video, the captions may be unreadable.
Two possible reads:
- Intentional: the caption sub-comp is now expected to overlay a user's already-edited video, and the user controls underlying contrast. Captions are now "pure overlays," and the wash was wrong because it darkened the user's video.
- Regression: the captions were authored to ship with their own wash, and removing it breaks their visual design over light footage.
Worth confirming intent in the PR body. If (a) — note in references/captions.md that the user is responsible for contrast (e.g., "darken your underlying video by ~20% before applying these captions, or pair with grain-overlay for tonal control"). If (b) — restore the wash as a child div with pointer-events: none that doesn't block timeline editing.
4. link dedup is exact-match only
inlineSubCompositions.ts:230 dedupes via !externalLinkHrefs.includes(href). Two sub-comps loading the same Google font URL → fine, dedupes. Two sub-comps loading the same family at different weight subsets (e.g., family=Inter:wght@400 vs family=Inter:wght@700) → both included; small over-fetch but functional. Worth noting in the PR body if intentional; if not, normalize the query params before dedup.
5. rel heuristic also misses common patterns
The href.includes(".css") || href.includes("css2?") substring check would miss:
fonts.googleapis.com/icon?family=Material+Icons(no.css, nocss2?)- Custom font endpoints like
cdn.example.com/fonts?family=X(no.css) - Adobe Fonts endpoints (
use.typekit.net/abc123.cssdoes match.cssbut only barely)
Tied to concern #1 above — fix shape solves it.
Non-blocking
- Cosmetic:
references/captions.md:140Lava texture row maps tocaption-texture— the style label says "Lava texture" but the component is the genericcaption-texture. If the texture genuinely is lava-themed, fine; if the texture is configurable, the row name might be misleading. pointer-events: noneon caption roots — correct for video overlays, but worth verifying it doesn't break Studio click-to-select on the caption sub-comp's own clip in the iframe. The Studio uses its own pointer routing, so likely fine, but worth a manual confirm..filesize-allowlistaddsdomEditingLayers.ts— clean bookkeeping for the new dispatch logic.
CI
Required green per merge state. Fallow audit per usual non-gating.
Verdict from me
The headline launch fix (caption overlay UX) is solid and correct. The data-timeline-locked + locked-composition-child protection is a clean studio safety addition. The font-link compiler/runtime asymmetry (concern #1) is a real preview-vs-render parity hazard — same class as hf#965, worth fixing before tomorrow's launch since captions will exercise this path heavily. The legibility regression (concern #3) is the second priority — quick PR-body clarification on intent + a doc note will close it.
Other concerns are non-blocking. Time-critical given tomorrow's launch — concerns #1 and #3 are 30-min fixes; the rest can land as follow-ups.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
PR was completely changed since my prior review — this is a fresh pass on 576598f5. Different scope entirely now: data-timeline-locked attribute support in Studio + sub-composition font-link extraction + 15 caption components going transparent + locked-comp child-edit protection.
Strengths
getTimelineEditCapabilitiesearly-return ontimelineLocked(timelineEditing.ts:215) reuses the existingtimingSource === "implicit"lock shape — same return contract, single code path. Clean.- Test added at
timelineEditing.test.ts:266pins the new branch with a representative caption selector. - Runtime-side link injection at
compositionLoader.ts:302usesCSS.escape(href)on the dedupe selector — correct. - Layered defense for locked-comp children: edit-capabilities (
canSelect: false, structural lock atdomEditingLayers.ts:182) plus text-panel hide atdomEditingLayers.ts:493. Two enforcement points, both with a clear reason string. Good UX shape — JS-generated DOM that "Unable to patch" was failing on now visibly degrades the panel rather than crashing.
Findings
important — rel-inference is fragile; preserve the source rel instead
inlineSubCompositions.ts:226 collects link[rel="stylesheet"], link[rel="preconnect"] hrefs but throws away the original rel. Both downstream injectors then guess the rel back from the URL:
rel: href.includes(".css") || href.includes("css2?") ? "stylesheet" : "preconnect"at htmlBundler.ts:818 and htmlCompiler.ts:618.
Failure modes:
- A
<link rel="preconnect" href="https://fonts.googleapis.com">(no.css, nocss2?) → correctly inferred aspreconnect. ✓ - A future Google Fonts URL that drops
css2?(e.g. a/v3/variant) → silently downgrades topreconnect, font load drops, exactly the regression this PR is fixing. - A non-Google-Fonts stylesheet with a clean URL like
https://cdn.example.com/font/montserrat→ silently downgrades topreconnect.
Fix: store { href, rel } tuples in externalLinkHrefs (consider renaming to externalLinks) and pass through verbatim. The source HTML already has the correct rel; re-deriving it is the bug surface.
important — CSS-selector injection on the compiler-side dedupe
Runtime path correctly escapes: document.head.querySelector(\link[href="${CSS.escape(href)}"]`) at [compositionLoader.ts:302`](https://github.com/heygen-com/hyperframes/blob/576598f5/packages/core/src/runtime/compositionLoader.ts#L302).
Compiler paths do NOT escape:
htmlBundler.ts:814—document.querySelector(\link[href="${href}"]`)`htmlCompiler.ts:614—document.querySelector(\link[href="${href}"]`)`
Today's registry hrefs are all hard-coded Google Fonts URLs without quotes or backslashes, so the failure isn't latent in shipped components — but a future component author who copies in a CDN href with a " (rare, but possible after URL-encoded query strings get hand-edited) will throw a SyntaxError from querySelector and break the entire bundle pass. Use CSS.escape(href) here to match the runtime path; cheap consistency fix.
important — no unit test on the font-link extraction (this PR's headline launch fix)
The PR body lists the font fix as the launch-day driver, but the only added test is for timelineLocked capability. There's no test that:
- An
<link rel="stylesheet">in a sub-composition<head>survivesinlineSubCompositionsintoexternalLinkHrefs. - The pair
preconnect+stylesheetfrom a Google-Fonts-shaped head ends up both injected. - Dedup works (same href across two sub-comps).
These are 5-line snapshot tests against the new InlineSubCompositionsResult.externalLinkHrefs field. Without them, the launch-tomorrow regression that the body says "fixes Google Fonts loaded via <link> tags being silently dropped" has no CI safety net — a future refactor of inlineSubCompositions.ts will silently break it again, same shape as the bug being fixed today.
nit — data-timeline-locked propagation lives in runtime only, not in either compiler inliner
compositionLoader.ts:407-409 propagates data-timeline-locked from innerRoot to params.host. Neither inlineSubCompositions.ts (the bundler inliner) nor htmlCompiler.ts's inlineSubCompositions does this propagation.
If a bundled output is ever re-opened in Studio (Studio's parseTimelineFromDOM reads the attribute off the host element at timelineDOM.ts:283), the lock wouldn't apply. Probably not on the launch-day critical path (Studio normally loads from source, not from bundle), but the contract "data-timeline-locked propagates from inner root to host" should hold in all three inliners or the divergence will bite later. Worth a follow-up issue, not a launch blocker.
nit — fallow audit shows 57 findings (most pre-existing) but the rule's CRAP-score bump on bundleToSingleHtml and mountCompositionContent is this PR's contribution
Fallow audit is RED on the rollup but the comment lists mostly pre-existing dead-code and duplication entries. New contribution is small CRAP-score bumps on functions you added branches to. Non-gating, but worth a note in a future cleanup pass — mountCompositionContent is at 864.7 (threshold 30).
CI
Fresh statusCheckRollup at review time on 576598f5:
- Failed:
Fallow audit(CI workflow) — audit-comment workflow; not in the required-for-merge gate per the BLOCKED + MERGEABLE state being reviewer-driven, not CI-driven. - In-progress (still running):
Render catalog previews,Render on windows-latest, all 8regression-shards,Tests on windows-latest,CLI smoke,Typecheck,Test,Build(the last few are required CI). Cancellations earlier in the rollup are from a superseded earlier push, not real failures. - Passed so far: all 6 Player perf jobs, Format, Lint, Test: runtime contract, Preview parity, Semantic PR title, CodeQL, File size check.
No required-check failure yet that would block — but Test / Typecheck / Build / CLI smoke (required) on the new SHA are still in progress. Re-check before merge.
Verdict
Verdict: COMMENT
Reasoning: The rel-inference, missing href escape on the compiler paths, and zero-test coverage on the headline font-link fix are real correctness gaps that should land before the launch — they're the exact regression class this PR exists to prevent. Quick fixes; happy to re-stamp once they're in. Studio-side timeline locking + child protection looks correct and is well-tested for its own surface.
Review by Vai (re-review — PR completely changed since prior review)
Store {href, rel, crossorigin} from source <link> elements instead of
re-deriving rel from a URL substring heuristic. Fixes preview-vs-render
parity: a stylesheet link whose href lacks ".css" or "css2?" was
emitted as preconnect in the compiled output, silently dropping the font.
Also documents that caption components ship with transparent backgrounds
intentionally — users add contrast layers in the host composition.
Escape href values in querySelector calls for link dedup in both htmlBundler.ts and htmlCompiler.ts to match the runtime path (which uses CSS.escape). Prevents SyntaxError on hrefs containing quotes. Add two tests for inlineSubCompositions font-link extraction: - Verifies <link> elements are extracted with original rel + crossorigin - Verifies dedup across multiple sub-compositions sharing the same font
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review on 9c159ad9. Three of the four concerns from my last pass + Vai's are cleanly addressed; one still open.
✅ Addressed
relpreservation across compiler + runtime —externalLinkHrefs: string[]→externalLinks: { href; rel; crossorigin? }[]. Both compiler paths (htmlBundler.ts+htmlCompiler.ts) now writelink.relfrom the source instead of inferring from the URL substring. Bonus:crossoriginis also preserved (which the heuristic version dropped entirely). The preview-vs-render parity hazard I flagged is closed.- Unit tests for
externalLinksextraction — new 66-line test block ininlineSubCompositions.test.tscovers (a)relandcrossoriginpreservation, (b) dedup across multiple sub-comps sharing the same font URL. Vai's "zero coverage on the headline launch-day fix" concern closed. hrefescaping in compiler dedupe query selectors — added manualreplace(/\\/g, "\\\\").replace(/"/g, '\\"')on both compiler paths before interpolating into thequerySelector. Handles backslash + double-quote which are the realistic break cases for URL hrefs. Acceptable; see one note below.- Caption legibility intent documented —
references/captions.mdnow explicitly says "Caption components ship with transparent backgrounds — they're pure overlays." That clarifies the intent on my concern #2 (the wash was correctly stripped because captions are meant as pure overlays, with the user controlling underlying contrast). Good — minimum-viable doc.
Still open
data-timeline-locked propagation only in runtime — compiler paths missing
The compositionLoader.ts runtime path propagates data-timeline-locked from inner-root → host (line ~409 of the original diff). The two compiler paths (htmlBundler.ts inlineSubCompositions and htmlCompiler.ts inlineSubCompositions) don't have the equivalent. Net effect: a composition bundled via the compiler that uses a data-timeline-locked sub-comp src may lose the lock attribute on the host in the rendered output, meaning the studio's timeline-edit guard wouldn't engage after a round-trip through the bundler.
3-line fix in inlineSubCompositions.ts parallel to the existing data-hf-authored-id propagation already there: when compId && innerRoot.hasAttribute("data-timeline-locked"), copy onto hostEl.
Not blocking the launch (the runtime path covers the studio-loading case, which is the dominant flow), but worth landing same-window since the bug shape is identical to the font-link preview-vs-render parity I just commented out.
Minor — CSS.escape vs manual replace
The new manual replace(/\\/g, "\\\\").replace(/"/g, '\\"') handles backslash + double quote correctly for href URLs (which rarely contain other CSS-special chars). Equivalent to CSS.escape(href) for this domain. Not a correctness gap — just slightly more code than necessary. CSS.escape would be more idiomatic and matches what the runtime path already uses. Cosmetic; skip if pressed for time.
Eval result still applies
The earlier 6-agent eval (3 prompts × old/new SKILL.md) showed the catalog-discovery direction works. The current PR moved the docs into references/captions.md rather than SKILL.md, which means the agent-trigger surface is now narrower (only loaded when the agent reaches captions work) but cleaner per skill-creator rubric. Same direction; different deferred-loading shape.
Verdict
The launch-blocking concerns from both my and Vai's reviews are addressed. The data-timeline-locked compiler-side propagation gap and the CSS.escape style nit are both real but neither blocks tomorrow's launch — the runtime covers the dominant flow. Ship-ready for the launch from my read; defer to Miguel's call on whether to fold in the 3-line propagation fix now or as a same-day follow-up.
— Rames Jusso
The runtime path (compositionLoader.ts) already propagated data-timeline-locked from inner root to host, but both compiler inliners (bundler + producer) did not. Bundled output re-opened in Studio would lose the lock. Now propagated in inlineSubCompositions alongside the existing data-hf-authored-id propagation.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Round-3 re-review on 9c159ad9 (diff vs 576598f5: +93/-21 across 4 src + 1 doc). Per Rule 3, additive — James (via Rames) already cataloged the round-2→3 fixes; this confirms independently and flags one regression check.
Strengths
inlineSubCompositions.ts:100,153,227-237— schema rename fromexternalLinkHrefs: string[]toexternalLinks: {href, rel, crossorigin}[]is the right fix shape: readrelfrom the source<link>rather than re-derive from URL substrings.crossoriginpassthrough is a bonus the heuristic dropped entirely.inlineSubCompositions.test.ts:134-198— new tests positively pin both the extraction shape (rel + crossorigin preservation acrosspreconnect/stylesheet) AND the dedup across multiple sub-comps. Closes the zero-coverage gap on the launch-day fix.- Both compiler consumers updated symmetrically —
htmlBundler.ts:815-823andhtmlCompiler.ts:615-625both now writelink.rel+ optionalcrossorigin. Symmetry is what was broken at round-2.
Addressed (from my prior review)
- rel-inference asymmetry — closed.
htmlBundler.ts:819andhtmlCompiler.ts:620both setlinkEl.setAttribute("rel", link.rel)from the preserved sourcerel. No more.includes(".css") || .includes("css2?")substring guess. - CSS-selector dedupe escape — closed. Both compiler paths apply
replace(/\\/g, "\\\\").replace(/"/g, '\\"')before interpolating intoquerySelector(htmlBundler.ts:816,htmlCompiler.ts:617). Equivalent toCSS.escapefor the realistic href chars (backslash + quote). See nit. - Test coverage on
externalLinks— closed. The new tests exercise the exactrel="stylesheet"+ non-.csshref case (Google Fontscss2?family=…) that motivated the fix — i.e. the wording-level break the substring heuristic would have produced is now positive-pinned. - timeline-locked propagation in compiler paths — still nit, not addressed. See below.
Regression check (round-2 → round-3)
No new regressions. The schema flip is consumed in exactly two sites (bundler + compiler), both updated. Runtime path was already correct (cloneNode(true)) and is untouched. Sub-comp dedup key remains href-only, matching pre-PR behavior.
Important
inlineSubCompositions.ts:227-238— link extraction selector is'link[rel="stylesheet"], link[rel="preconnect"]'. That now missesrel="preload"(<link rel="preload" as="font" href="...">) which is the modern fonts.googleapis.com pattern for FOIT/FOUT mitigation. If captions ever ship preload hints, they'll be silently dropped on the compiler path the same way.css2?was. Not launch-blocking (caption fixtures don't use preload today per the test fixtures), but the same family of bug James and I both flagged. Either expand the selector to includepreload/prefetch/dns-prefetchor document the supportedrelset.
Nits
- timeline-locked compiler propagation — covered in James's review above; +1, same call. 3-line addition parallel to the
data-hf-authored-idhost-propagation already ininlineSubCompositions.ts. Runtime covers the studio-loading flow, so this is a same-window follow-up, not launch-blocking. CSS.escapewould be more idiomatic than the manual replace and matches the runtime path; cosmetic.
CI
statusCheckRollup at review time: Build, Test, Test: runtime contract, Lint, Preflight, File size check SUCCESS. Typecheck, regression-shards (1-8), Perf (load/fps/scrub/drift/parity), Preview parity, Render on windows-latest, Tests on windows-latest, Render catalog previews, Smoke: global install, Analyze (javascript-typescript) IN_PROGRESS. Two failures: Fallow audit (60 findings; sampled — mostly pre-existing dead code/unresolved imports unrelated to this PR's diff) and CLI smoke (required) (npm ETIMEDOUT 150.171.109.147:443 installing onnxruntime-node postinstall script — infra/network flake on the runner, not a PR-introduced break; safe to re-run).
Both failures are environmental/pre-existing and not caused by the round-3 diff. Recommend re-running CLI smoke before merge.
Verdict: APPROVE
Reasoning: All three launch-blocking importants from the prior review are addressed cleanly with positive-pin test coverage; remaining items (timeline-locked compiler propagation, preload selector gap, CSS.escape cosmetic) are nit/important-follow-up that don't gate tomorrow's launch. CI failures are infra and pre-existing, not regressions.
Review by Vai (round 3)
Merge activity
|
3 participants
Summary
data-timeline-lockedattributetimelineDOM.ts, checked ingetTimelineEditCapabilitiesLocked composition child protection
data-timeline-lockedsub-composition cannot be moved, resized, or style-edited on the canvas — prevents "Unable to patch" errors for JS-generated contentisInsideLockedCompositionflag onDomEditSelection, checked in bothresolveDomEditCapabilitiesandisTextEditableSelectionFix font loss in sub-compositions
compositionLoader.ts) and compiler (inlineSubCompositions.ts,htmlBundler.ts,htmlCompiler.ts) now extract<link rel="stylesheet">and<link rel="preconnect">from sub-composition<head>alongside existing<style>/<script>extraction<link>tags being silently dropped when a component is used as a sub-compositionTransparent caption overlays
transparentpointer-events: noneadded to composition roots so captions don't intercept clicksCaption catalog reference
skills/hyperframes/references/captions.mdTest plan
bunx vitest run packages/studio/src/player/components/timelineEditing.test.ts— 37 tests passdata-timeline-lockedclip cannot be moved or trimmed