feat(skills): product-launch-video skill + consolidate motion knowledge into hyperframes-animation#1745
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Review — 188527f
Thorough read of the full diff (66 files, +3804/−2976). This is a substantial PR covering three areas: (1) the new product-launch-video shot-sequence architecture, (2) consolidation of motion knowledge into hyperframes-animation as the single source of truth, and (3) several lint/audit false-positive fixes. Overall the code is well-structured with good comments. Here are my findings:
CI: Skills: manifest in sync is failing
The skills manifest is out of date. 5 skills need regeneration:
bun run --cwd packages/cli gen:skills-manifest
Commit the updated skills-manifest.json. This is a required check blocker.
Code findings
1. contrast-audit.browser.js — ancestor visibility: hidden check is redundant (nitpick, not blocking)
The ancestor walk checks acs.visibility === "hidden", but visibility is an inherited CSS property — getComputedStyle(el).visibility on the child already returns "hidden" when any ancestor sets it (unless a child explicitly overrides to visible). The existing per-element check on L89 already catches this. The ancestor walk is still valuable for opacity (the real karaoke issue) and display: none (defense-in-depth for elements in a display:none subtree that still appear in querySelectorAll results), so this is a nitpick, not a bug. Could simplify by checking only opacity and display in the walk, but it is not worth changing.
2. layout-audit.browser.js — parsePx is referenced but not shown in the diff
The new code at the text-overflow vertical tolerance calls parsePx(elementStyle.fontSize). I trust this function exists in the file already (it is not new), but worth a sanity check that it handles the "36px" → 36 conversion correctly and returns a finite number, since it feeds into Math.max(tolerance, ... * 0.2). If parsePx returns NaN or undefined, Math.max would produce NaN and the overflow check would silently pass everything. Low risk since this function is presumably well-established.
3. captions.ts — isCaptionComposition gating looks correct but is heuristic-dependent
The three signals (file path contains "caption", rootCompositionId === "captions", styles matching .caption-* / .cg- patterns) are reasonable, but a composition that genuinely needs the caption_exit_missing_hard_kill check could be missed if it uses unconventional class names. The test covers the false-positive case well. Acceptable trade-off given the alternative (flooding content frames with false positives).
4. media.ts — media_in_subcomposition rule is clean
Good addition. Catches <video>/<audio> inside sub-compositions early at lint time rather than letting them render as blank/black. The test covers both the positive (sub-comp) and negative (host-root) cases. The error message and fix hint are helpful.
5. assemble-index.mjs — guardFrame comment/script stripping is sound but could mask edge cases
The scan variable strips <!-- comments --> and <script>/<style> bodies before checking for <video>/<audio> tags and timed elements. This prevents false positives from comments mentioning <video>. However, the regex /<script\b[\s\S]*?<\/script>/gi uses a non-greedy match, which is correct for typical HTML but could theoretically mismatch on </script> appearing inside a string literal in a script. In practice, HyperFrames compositions are machine-generated with predictable structure, so this is fine.
6. assemble-index.mjs — ensureBgmCovers uses spawnSync correctly
The ffprobe/ffmpeg fallback is well-designed — degrades gracefully when tools are absent, never hard-fails assembly. The loop-extend approach with fade-in/fade-out is sensible. One minor note: "-stream_loop", "-1" creates infinite loops — the -t flag correctly truncates, so this is safe.
7. build-frame.mjs — dark-mode brand polarity inversion is the most complex logic change
The polarity-aware color mapping (invert = presetGroundDark !== brandGroundDark) and the flipL lightness mirror are well thought out. The semantic status color preservation (positive/negative/success/error) via regex is a smart addition — prevents brand-repainting from turning a red "error" indicator into brand-blue.
The neutral preservation (chroma < 16 → keep neutral with whisper of brand hue at sat <= 0.06) fixes the reported issue where grey text ladders were getting fully saturated. Good.
The remapRgbaToAccent function for rgba() values fills a gap where non-hex colors were previously left as-is. The neutral-overlay detection (max - min < 16) is a reasonable threshold.
8. build-frame.mjs — isIconFont and isMonoFont are string-based heuristics
These regex checks cover a wide range of known icon/mono font names. If a brand uses a font not in these patterns, it would be misclassified. Acceptable risk — the alternative (a canonical database) is overkill.
9. build-frame.mjs — ink/canvas contrast check relaxed from polarity to separation
Changed from li >= lc (ink must be darker than canvas) to Math.abs(li - lc) < 40 (they must differ by at least 40 luminance). This correctly supports dark-mode brands where ink is lighter than canvas. The threshold of 40 is reasonable for readability.
10. tokens.mjs — pickAccent now uses role diversity + palette prominence
The accent selection is much improved — ranking by role diversity (appears in link + icon + button vs. just one CTA) and palette prominence (earlier in frequency-ordered palette = more prominent) addresses the case where a one-off bright CTA was beating the pervasive brand color. The brandRolesFromStats signature change (adding colorsInOrder) is backward-compatible since the function returns null early if stats are empty.
11. captions.mjs — font path fix from ../ to root-relative
Changed "../assets/fonts" to "assets/fonts" and "../capture/assets/fonts" to "capture/assets/fonts". This fixes a real bug where ../ would escape the root and 404 in Studio/preview. Good catch.
12. captions.mjs — dark-ground caption contrast auto-fix
The lum < 90 threshold for detecting a dark caption canvas is reasonable. The CSS overrides for .caption-word states (upcoming/active/spoken) using color-mix and CSS custom properties are clean. Uses var(--cap-accent) for the active word highlight, which ties nicely to the brand.
13. tts.mjs — hardcoded Marcia voice ID
The default voice is now pinned to 05f19352e8f74b0392a8f411eba40de1 (Marcia) for English. This is deterministic, which is the goal. Non-English still falls back to the first matching catalog voice. If Marcia is ever removed from the HeyGen catalog, this would fail — but that is the correct behavior (fail loudly rather than silently picking a random voice).
Skill/doc content (spot-checked)
- Blueprint and rule indexes are consistent (15 blueprints, 36 rules as stated).
- Cross-skill references (
../hyperframes-animation/{rules,blueprints}-index.md) follow the sibling-skill convention already used byfaceless-explainerandpr-to-video. - The
examples/<id>.htmlreferences have been cleaned up consistently across consumer skills. - The frame-worker sub-agent instructions are updated to match the new shot-sequence architecture.
- The new
cut-catalog.mdreference provides concrete GSAP patterns for within-scene transitions.
Test coverage
layout-audit.browser.test.ts: Two new tests for glyph-ink vertical tolerance (within band = no flag, beyond band = flag). Good.captions.test.ts: Test for false positive on content frame mentioning "karaoke" in a comment. Good.media.test.ts: Tests formedia_in_subcompositionrule — sub-comp flagged, host-root not flagged. Good.
Verdict
Approve with one required fix: regenerate the skills manifest (bun run --cwd packages/cli gen:skills-manifest and commit). The code changes are solid — the dark-mode brand handling, false-positive lint fixes, and motion knowledge consolidation are all well-reasoned with appropriate test coverage. No correctness bugs found.
— Miga
jrusso1020
left a comment
There was a problem hiding this comment.
Additive review on top of <https://github.com/heygen-com/hyperframes/pull/1745#pullrequestreview-4581586216|Miga's pass>. Miga covered the priority-1 code surface (captions.ts, media.ts, layout-audit, contrast-audit, assemble-index.mjs, build-frame.mjs, tokens.mjs, captions.mjs, tts.mjs) thoroughly — verified their findings + adding two structural verifications they didn't deep-dive.
CI blocker (Miga also flagged)
Skills: manifest in sync is failing. bun run --cwd packages/cli gen:skills-manifest + commit the updated skills-manifest.json. This is the only required-check blocker on the PR.
Additive — consolidation integrity verified (the load-bearing claim)
The PR body says "consolidate motion knowledge into hyperframes-animation as the single source of truth," and replaces 13 master-state blueprints with 15 new ones. Walked the consolidation to confirm motion knowledge from each removed blueprint genuinely survives in the new shape:
| Removed blueprint (master) | Where its motion knowledge consolidated |
|---|---|
brand-reveal-assemble-zoom |
absorbed into logo-assemble-lockup.md |
comparison-split-cards |
renamed to comparison-split.md |
concept-demo-decode-pan |
absorbed into spatial-pan-stations.md |
cta-orbit-collapse |
absorbed into constellation-hub.md (also referenced by cursor-click-ripple rule) |
demo-page-scroll-spotlight |
absorbed into device-surface-showcase.md |
hook-counter-burst |
renamed/restructured to dataviz-countup.md |
messaging-multi-phrase |
absorbed into kinetic-type-beats.md (also referenced by dynamic-content-sequencing rule) |
metric-video-text-pivot |
renamed/restructured to video-text-pivot.md |
problem-mockup-overwhelm |
renamed to overwhelm-surround.md |
proof-logo-chain |
absorbed into constellation-hub.md (also referenced by avatar-cloud-network + hacker-flip-3d rules) |
takeover-ticker-displace |
renamed to ticker-takeover.md |
workflow-approve-press |
absorbed into cursor-ui-demo.md |
Verification approach: git ls-tree _pr1745 skills/hyperframes-animation/blueprints/ vs origin/main for the delta, then git grep for each removed-blueprint name across the PR tree to confirm at least one new blueprint or rule references it (not silently dropped).
The 14 example HTML files in skills/hyperframes-animation/examples/ (keeping old blueprint names like brand-reveal-assemble-zoom.html, proof-logo-chain.html, etc.) are intentionally preserved as canonical motion references that the new blueprints + rules link to — not dangling.
Net: no motion knowledge is silently lost in the reshape. The consolidation claim is genuine.
Additive — product-launch-video integration depth verified
Confirmed skills/product-launch-video/ has no blueprints/ subdirectory — the new skill references shared hyperframes-animation rather than duplicating, matching the stated goal. Cross-references found in 8 files:
SKILL.md— 1 reference (top-level "see hyperframes-animation for blueprints/rules")references/cut-catalog.md— 1references/motion-language.md— 1references/story-design.md— 1references/visual-design.md— 1 (Step-4 method is explicitly delegated here)scripts/lib/transition-registry.mjs— 1scripts/lib/transitions.json— 1sub-agents/frame-worker.md— 1
Healthy integration depth — the skill isn't a thin wrapper, it's genuinely reaching back into the shared library at each layer (skill spec, references, scripts, sub-agent).
Verdict
COMMENT — external author, routing through <@U08E7PV788Z> for any stamp/merge call per protocol. The CI manifest fix Miga flagged is the only blocker. Nothing additional from me blocks merge; the consolidation integrity + product-launch-video integration both verify clean.
— Jerrai
…o hyperframes-animation
- Add the product-launch-video skill: shot-sequence architecture where each
visual frame is a time-coded shot sequence picked from a blueprint menu and
paced to the voiceover (anti-PowerPoint). Includes the frame-worker sub-agent,
story/visual/motion-design references, and audio/captions/transitions/
stage-assets/assemble-index scripts.
- Consolidate motion knowledge in hyperframes-animation as the single source of
truth: promote the updated atomic rules (31 -> 36) and rename product-launch-
video's archetypes into hyperframes-animation blueprints (13 -> 15, replacing
the old set). product-launch-video, faceless-explainer, and pr-to-video now
reference them via ../hyperframes-animation/{rules-index,blueprints-index}.md
and the rules/blueprints dirs. Fixes the discrete-text-sequence broken links;
blueprints no longer ship per-id runnable examples, so example references in
the consumers were dropped.
- Default HeyGen TTS voice to Marcia (deterministic; was the API's first English
voice, which drifts on catalog re-sort). Override with --voice.
- assemble-index pre-assembly frame guards: auto-repair a sub-comp root missing
canvas dims; hard-fail on <video>/<audio> inside a sub-comp; hard-fail on a
timed non-root element missing class="clip" or overlapping same-track clips.
- Lint/CLI: lint media inside sub-compositions as an error; stop false-positive
caption layout/lint findings; contrast/layout-audit skip elements hidden by an
invisible ancestor.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
188527f to
52ebe28
Compare
- script/style blanking regex now matches closing tags with trailing whitespace (</script >, </style >) — js/bad-tag-filter (high). - drop the existsSync precheck before reading/repairing a frame file; read directly and handle ENOENT, removing the check->write TOCTOU window — js/file-system-race (high). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
309b4dd to
6b94b66
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
R2 verification at 6b94b660 — both R1 blockers resolved cleanly:
- Skills manifest in sync — regenerated; CI now 52/52 green.
- CodeQL js/bad-tag-filter (high) —
guardFrame's tag-blanking regex updated from<\/script>/<\/style>to<\/script[^>]*>/<\/style[^>]*>so closing tags with trailing whitespace (e.g.</script >) get caught too. The old shape would leave content un-blanked past</script >, defeating the lint guard's purpose. Correct fix. - CodeQL js/file-system-race (high) — the
if (!existsSync(compAbs)) { ... } let html = readFileSync(compAbs, "utf8")pattern (check-then-read TOCTOU window) replaced withtry { html = readFileSync(compAbs, "utf8") } catch { ... }. Direct read + ENOENT handling closes the race. Correct fix. - CodeQL open alerts on PR: 0.
No remaining blockers from my side. Still COMMENT (external author, stamp/merge routing through <@U08E7PV788Z>).
— Jerrai
miga-heygen
left a comment
There was a problem hiding this comment.
Re-review at 6b94b660
Previous review (at 188527f) flagged one CI blocker: Skills: manifest in sync was failing because skills-manifest.json wasn't regenerated after the skill changes. Miao pushed one new commit (6b94b660 — fix(skills): clear CodeQL alerts in assemble-index.mjs) and rebased the branch on main.
What changed since last review
Commit 6b94b660 addresses two CodeQL high-severity alerts:
-
js/bad-tag-filter— TheguardFramefunction's script/style blanking regex used<\/script>/<\/style>which misses closing tags with trailing whitespace like</script >. Fixed to<\/script[^>]*>/<\/style[^>]*>. Correct — the[^>]*class swallows any non->bytes before the closing bracket, matching the tag regardless of extraneous whitespace or attributes. -
js/file-system-race— The original code didif (!existsSync(compAbs)) { ... } let html = readFileSync(compAbs, "utf8")— a classic TOCTOU (time-of-check-to-time-of-use) race where the file could vanish between check and read. Replaced withtry { html = readFileSync(compAbs, "utf8") } catch { ... }which reads directly and handles ENOENT in the catch. Clean fix, no behavior change beyond closing the race window.
Manifest regeneration — skills-manifest.json now included in the rebased first commit with updated hashes. The product-launch-video hash changed from d47b790f to dedfc10e in the fix commit (expected — the script file it checksums was modified).
CI status
All 52 checks passing, including:
Skills: manifest in sync— pass (was the blocker, now resolved)CodeQL— pass, 0 open alerts- All regression shards (8/8), build, typecheck, lint, format, tests — green across the board
Verdict
Both issues from the first review are resolved. The CodeQL fixes are straightforward and correct. No new concerns introduced. Ready for stamp + merge from the maintainer side.
— Miga
Summary
Adds the product-launch-video skill and consolidates HyperFrames' motion knowledge so that
hyperframes-animationis the single source of truth for atomic motion rules and multi-phase scene blueprints.product-launch-video,faceless-explainer, andpr-to-videonow reference that shared library instead of carrying duplicate copies.What changed
New skill —
product-launch-videoframe-workersub-agent, story / visual / motion-design references, and the audio · captions · transitions · stage-assets · assemble-index scripts.Consolidation into
hyperframes-animation(single source of truth)ambient-glow-bloom,depth-of-field-blur,depth-scatter-assemble,motion-blur-streak,spring-pop-entrance).product-launch-video's archetypes intohyperframes-animationblueprints (13 → 15, replacing the previous set) and regeneratedblueprints-index.md/rules-index.md.product-launch-video,faceless-explainer, andpr-to-videonow reference../hyperframes-animation/{rules,blueprints}-index.md(matching the existing sibling-skill convention) — no duplicated rule/blueprint content.discrete-text-sequencecross-links.Other
--voice.lintfailures surface before a wasted render): auto-repair a sub-comp root missing canvas dims; hard-fail on<video>/<audio>inside a sub-composition; hard-fail on a timed non-root element missingclass="clip"or on overlapping same-track clips.Notable / behavior changes
examples/<id>.html(the new blueprints are template-style). The now-invalid example references in the consumer skills were dropped accordingly.3d.md,references/typography.md) are pre-existing and intentionally left unchanged.Verification
rules-index.md↔rules/: 36 ↔ 36 consistent ·blueprints-index.md↔blueprints/: 15 ↔ 15 consistent.product-launch-videoresolve; no new dangling links.oxlint·oxfmt· typecheck ·commitlintall pass (pre-commit hooks green).🤖 Generated with Claude Code