feat(catalog): add the seven 3D-motion pieces as installable registry blocks - #4056
miguel-heygen wants to merge 60 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 7d089fd8. Note the head moved while I was reading (d09af651 → 7d089fd8), so I re-verified against the new head rather than posting findings pinned to the old one.
First, the head move is clean — and worth recording, because it looks alarming by every cheap measure.
style(catalog): run oxfmt on the seven ported composition files is +5905/-1894 across 8 files, and the whitespace-stripped content of frost-sequence-camera-orbit.html shrinks by 30,064 bytes. A formatter adds whitespace; it does not delete a quarter of a file's real content, so I chased it. The cause is benign: the data-composition-variables blob on <html> was a double-quoted attribute holding "-escaped JSON, and the formatter re-delimited the attribute with single quotes and decoded the entities (5,258 " → 0). Same JSON, ~5 bytes saved per quote.
That rewrite has exactly one failure mode — an apostrophe anywhere in the JSON would now terminate the attribute early — so I executed the check rather than reasoning about it: for all seven pieces at this head, every single-quote-delimited attribute whose value starts [/{ parses as JSON, 0 failures. Structural token counts (<script>, <style>, function, const, gsap., addEventListener, data-composition-id) are identical across the move. So the reformat is semantically safe on all seven.
blocker — this PR's own CREDITS.md states a precondition that merging would bypass
The added "Third-party licenses" entry says the seven pieces "were ported from a reference implementation. License terms for these pieces are not confirmed upstream — do not assume this repository's Apache 2.0 license applies to this directory until confirmed."
I read that as an open question, not a disclosure that closes by being written down. This is a public Apache-2.0 repository; the pieces land as installable registry blocks (/registry/install) and the docs surface publishes hover previews for them. Merging is the step that redistributes ~251k lines whose terms the PR itself declines to assert, to people who will reasonably assume the repo's license covers what they install. Worth resolving upstream — or landing the blocks behind whatever gate the answer implies — before this goes in. Flagging it because it is the one finding here that a later commit cannot quietly fix.
important — vendored MIT copies ship without their notices, and unevenly within this same PR
frost-sequence-camera-orbit/assets/frost.js:1declares its own contents:three@0.185.1 (MIT)+three-mesh-bvh@0.9.14 (MIT). That block's only licence file isassets/fonts/Geist-OFL.txt— a font licence. Neither MIT notice ships with the copy.cuboid-carouselandorbit-carddo shipassets/Three-LICENSE.txt. So the convention is established inside this PR and applied to two of the three blocks that need it.gsap-3.14.2.min.jsis vendored into four blocks (orbit-card,frost-sequence-camera-orbit,cuboid-carousel,code-slice-hero) with no GSAP licence file in any of them. GSAP's terms are not MIT, so it deserves naming explicitly rather than ridingCREDITS.md's "Thanks also to … GSAP".glass-shard-title/assets/glass-main.js(24,624 lines) ships with no header and no licence file, so its provenance can't be established from the tree at all.
important — frost.js is a build artifact whose source doesn't ship
Its first line reads "Generated by build.mjs; edit src/ and rebuild." There is no src/ and no build.mjs anywhere in this PR — I checked the full file list, not just that block. So the instruction in the file cannot be followed in this repository, and 78,870 lines of bundled three.js + BVH have no patch path here: a three.js security bump would have nowhere to land. motion.js (43,785 lines) is the same shape with no header at all. Either ship the source and build script, or record in the block's SKILL.md where upstream lives and how to regenerate.
note — glass-main.mjs (1,018 lines, authored, oxfmt-owned) sits beside glass-main.js (24,624 lines, built) with nothing in the tree relating them. Whatever produces the .js isn't in the repo, so the pair can drift silently and a reader can't tell which one is authoritative.
On the body's own split: "the hand-written part is small" holds as a line-count claim. I separated generated from authored by reading file headers rather than taking the split on faith, and the authored surface is the seven <piece>.html, glass-main.mjs, the per-block registry-item.json / presets/ / SKILL.md, and the seven docs/catalog/blocks/*.mdx. The thing that split surfaced is the .mjs/.js pair above.
Audited: the head-move delta (all 8 files, semantically), provenance and licence inventory per block, the generated-vs-authored split, CREDITS.md, .gitattributes LFS routing for the preview clips.
Trusting / not reviewed: the seven pieces' motion content and visual correctness, the check-docs-catalog parity checks, and the Mintlify preview — your own note says merge waits on your look at that, so I have deliberately not posted the approval that would release it.
Verdict: REQUEST CHANGES
Reasoning: the licensing precondition is stated by the PR itself, and three of the blocks that vendor MIT libraries ship without their notices. The reformat and the block wiring are fine; this is about what gets redistributed.
One mechanic so nobody is left guessing why it stays red: this repo sets dismiss_stale_reviews_on_push: false, so this review stays live across pushes and needs an explicit approval from this account to clear once it's addressed. Ping me and I'll turn it around quickly.
— Rames Jusso
|
Thanks for the thorough pass. Both findings addressed at the latest commit: Licensing precondition. The maintainer has confirmed the author's permission for these seven pieces under this repository's license. Vendored notices and frost's build artifacts. Every third-party library vendored across the seven blocks now ships its own license notice, consistently: GSAP (added to all four blocks that vendor it), three.js and the Geist font's OFL notice (added wherever they were missing), and three-mesh-bvh, opentype.js, and ClipperLib for For One thing this surfaced that's outside this pass: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
5cb1fc8 to
2cebedc
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at the squashed head. My previous CHANGES_REQUESTED is cleared by this review — both findings are addressed.
A note on how I checked: "history squashed to one commit" would normally mean my earlier read carries over, so I compared the tree hashes rather than the commit list. They differ (bbc5bddf → d32ee52f), so the squash carried content changes too and I re-read the delta. Worth knowing for next time — the squash also dropped glass-main.mjs and rebuilt frost.js/motion.js.
Strengths
- The licence audit went past what I flagged. I named three@0.185.1 + three-mesh-bvh@0.9.14 and GSAP; you also found OpentypeJS and Clipper and shipped notices for both (
registry/blocks/frost-sequence-camera-orbit/assets/). That's the enumeration done properly rather than to the letter of the review. - Every notice is wired into
registry-item.jsonwith atarget, so it actually installs alongside the library it covers. A notice that sits in the repo but not in the manifest would look right in the diff and ship nothing — checking the manifest was the part I expected to be missed, and it wasn't. source/build.mjsruns a TS preflight plus a headless WGSL shard-compile probe before bundling (source/tools/run-shard-compile.mjs). Catching shader failures without a GPU is a genuinely useful gate to have brought along.
Findings — none blocking
important — frost-sequence-camera-orbit.html:3173-3174 publishes window.__hf.buildReady.frost = frost.ready, and nothing in the repo reads buildReady (those two lines are its only occurrences, repo-wide). The rig has real async startup (source/src/frost.ts:336,586), and the producer's actual gate is window.__renderReady — read at packages/producer/src/services/fileServer.ts:653 and packages/producer/src/parity-harness.ts:79-80, and recognised by the compiler at packages/core/src/compiler/htmlDocument.ts:22. No block in this PR sets it. So if the intent was to hold capture until the ice rig is built, this hook doesn't do that; if it's a debug handle, it's dead code. Worth naming which — and if it is meant to gate capture, the wired form is window.__renderReady = false up front, flipped after await frost.ready.
important — nothing ties the shipped bundle to the source that now accompanies it. source/build.mjs writes assets/frost.js, the README documents "edit src/ and rebuild", but no workflow in .github/workflows/ references frost, source/package.json is private: true and outside the workspace build, and .oxlintrc.json now excludes source/** from lint entirely. I verified the absence of a gate; I did not test whether the current bundle matches the current source. The result is two copies of the same logic with nothing checking they correspond — and the build is exactly where that silently diverges.
important — registry/blocks/glass-shard-title/SKILL.md no longer matches its own directory after the squash. Line 82 still says to "rebuild with esbuild and re-inline after editing the .mjs source", but assets/glass-main.mjs was deleted here; the Files list still carries build-bundle.mjs (1 KB), which is absent at this head. The deletion itself is clean — the composition loads the built assets/glass-main.js and the manifest entry went with the file — but these are agent-facing instructions, so what's left points at two files that no longer exist. Same class as the frost finding I opened, just inverted: there the instruction outran the source, here the source was removed and the instruction stayed.
nit — the CodeQL "prototype-polluting function" alert on source/src/dials/defaults.ts:485 is still open at this head (the defaults.ts change in the squash is a comment rename, not a fix). I don't think it's exploitable: values only ever comes from the committed baked.json, withBaked has no callers outside its own file, and __proto__ already hits the k.startsWith("_") guard, while constructor can't recurse because functions don't satisfy typeof c === "object". But that mitigation is incidental — the guard was written to skip internal keys, not to stop pollution — so an explicit key check would both close the alert and make the protection intentional.
nit — the CREDITS.md line now reads "contributed with their author's permission under this repository's license", and the SKILL.md files drop their Source experiment: pointers. Dropping internal experiment names from a public repo is the house convention, so no issue there; the effect is just that CREDITS.md is the only remaining record and it doesn't name what was ported. If the upstream is nameable publicly, naming it would make the attestation auditable by someone who wasn't in this thread. To be explicit about my own position: I'm accepting the permission statement, not verifying it — an offline licensing confirmation isn't something a code review can audit.
Note, not a finding — GSAP's minified bundle was already vendored in this repo before this PR (skills/music-to-video/references/motion-primitives/assets/gsap.min.js, skills/talking-head-recut/assets/vendor/gsap.min.js both exist at the merge base), so how an Apache-2.0 repo redistributes an all-rights-reserved bundle isn't this PR's question to answer. I'd raised the missing notice; your GSAP-NOTICE.txt files now make these four blocks the only place in the repo that carries one. The two older copies still have none — worth a separate pass, not something to load onto this PR.
Verdict: APPROVE
Reasoning: The licensing precondition that blocked this — the PR's own CREDITS.md disclaiming its licence terms — is gone, and every vendored library now ships a notice that actually installs. The three important findings are drift and wiring issues that don't affect what renders, so they're better as follow-ups than as another round.
Gate mechanics, for whoever is tracking the merge: dismiss_stale_reviews_on_push is false on this repo, so my earlier CHANGES_REQUESTED survived the force-push and only an explicit approval from this account could clear it. This is that approval. Merge timing is yours — you'd mentioned the Mintlify preview.
— Rames Jusso
|
Refresh at
|
|
Correction to the refresh comment above: I said frost-sequence-camera-orbit's Inter font got an OFL notice added. That's only true of an intermediate commit in this range — a later commit in the same range found the font had zero runtime references and deleted it entirely (font + notice both), so at the current head there is no Inter font at all. The Cormorant Garamond notice for glass-shard-title is correct and still stands. |
9ed226b to
80c3a5d
Compare
The base branch was changed.
80c3a5d to
a68fdb6
Compare
…valuates only exports
…flash while probing WebGPU
…what the block installs CREDITS.md names every vendored dependency with its upstream and licence, and flags the two leaf textures whose origin is still to be confirmed. Each SKILL.md Files table now comes from the block manifest and its install and render paths point at compositions/<name>/. A test checks that every path a SKILL.md names is a manifest file that exists.
…g, not by console wording verify-catalog-payloads forgave a missing adapter only by matching the markup and the message, which library-reported errors and widgets never satisfy. The webgpu tag in the item manifest is now the owner: a tagged item ignores its page and console errors, an untagged one gets no forgiveness, and a marker payload with no html is listed instead of crashing the run.
…render paths CREDITS.md states the three.js version and vendored-or-CDN status per block, adds d3-delaunay, Clipper on the CDN and the Google Fonts loads, and lists every texture with no known origin. The SKILL.md test now also checks the mount and render paths against the manifest composition.
The two checks shared a copied manifest read, which fallow flagged as a clone with a high score.
… and check git not the disk The compositions/ and *-ad/ ignore rules matched folder names under docs/public/catalog/items, so 33 hosted fonts, textures and marks were generated but never committed and would 404 on the docs host. The rules now have a negation for that tree and the files are added. The drift check read the checkout, where git-ignored files still exist, so it passed locally and failed only in CI. It now unpacks HEAD with git archive and compares that.
…rror, and test the wiring The manifest webgpu tag now decides whether an item may lack an adapter, and the error wording decides which lines are that absence. Any other page or console error on the item, and every failed request, still fails. The per-item decision is one exported function the tests drive, marker payloads are parsed by a tested helper, and the SKILL.md variable counts are checked against their tables (cuboid-carousel said forty-nine for forty).
… adapter request failed to request .*adapter also matched errors about other adapters. It now matches only a request for the WebGPU adapter, so a tagged item cannot hide an unrelated failure behind it.
3 participants
Summary
Adds seven 3D-motion pieces to the catalog as installable registry blocks: a leaf-parting title card, a glass-shard title, a code-slicing hero, a frosted camera-orbit sequence, a bevelled-cuboid carousel, an orbiting feature card and a wireframe portal title. Each installs like any catalog block (
npx hyperframes add <id>), has a live detail page with source and install, and sits in its own "3D motion" section.This PR is the whole catalog change. The engine script-ordering fix that the
glass-shard-titlerender needed landed first, in #4133.The pieces are contributed with their author's permission under this repository's license (
CREDITS.md).What changed
registry/blocks/<id>/, tagged3d-motionso the existing tag-driven grouping gives them their own section.registry-item.json. Nothing binary was added for them.public/, never.js,.mjsor.hdr(measured against a deployed preview). Shared libraries (gsap, three, RoomEnvironment, BufferGeometryUtils) go once intodocs/public/catalog/vendor/*.jsonand are fetched and blob-loaded at runtime; item scripts are inlined as text; the one HDR is a data URI. Implemented inscripts/catalog-script-inlining.ts, one inliner per item that needs it.processAssetsembeds.jsas a data URI for every item without an inliner, as before, so other catalog items regenerate unchanged.check:docs-catalogin the Lint job and a new "Catalog payloads" job that loads every payload a PR changes in headless Chrome (verify-catalog-payloads.ts --changed origin/main). A WebGPU piece cannot draw on a CI browser without an adapter, so that one error is ignored for payloads that usenavigator.gpu; every other failure still fails the job.scripts/verify-catalog-payloads.tsmodels the docs host's real extension allowlist, so a payload that loads locally but 404s on the host fails the check.frost-sequence-camera-orbit/assets/motion.jsis removed: no reference anywhere, and zero requests across a full play-through.glass-shard-titleloads its vendored engine through<script src>instead of an inlined copy that failed two lint rules.source/directories are excluded from hosted item directories.shards-atlas.png,ice-inclusions-generated.png,matcap-1.png) are hosted through the registry'sfiles[].urlfield on the HeyGen CDN under content-addressed names, exactly like the carousel images:hyperframes addstill writes them to the same local paths, and the catalog payload's inlined scripts point at the URLs. No PNG over the non-LFS limit is committed.ice-fracture-detail-v1.pngwas loaded by nothing and is deleted.frost-sequence-camera-orbitships only what it renders with: the iteration presets (approved-material-v1tov7,current-test, the saved presets and their catalog) are removed, since nothing reads them at render.source/presets/keeps the two files the source build imports..oxlintrc.jsonignores the vendored bundles;CREDITS.mdhas one attribution line.Verification
bun scripts/verify-catalog-payloads.ts --only <id>exits 0 for all seven pieces, plusliquid-glass-widgets,liquid-glass-notificationandhw-write-titleto show the shared generator change did not break other items.git statusclean.glass-shard-titlevideo was found and replaced).crossOrigin=anonymousfrom the deployed preview's origin and read back from a canvas without tainting, which is what WebGL needs; the CDN answersaccess-control-allow-origin: *.Diff size
Far over the 1k-line guideline. Almost all of it is vendored bundles, fonts and textures across seven independent items. The hand-written surface is the manifests, the
scripts/catalog-*changes and their tests.Heavy pieces on a real GPU
The four WebGL/WebGPU pieces now draw across their timeline and flush the GPU inside
window.__hf.buildReady, so shader links and texture uploads happen before the player reports ready instead of on the first visible frames. Measured with the tile path (srcdoc player, muted, play from 0, Apple GPU, headless Chrome for Testing, single run), max rAF gap / longest long task, before then after:Frost: the remaining time is the build phase (geometry, distance fields, particle setup), not first-draw pipeline creation. The player reporting ready before the build gate resolves is a player readiness-gate fix in a separate PR.
Detail page: install command before the preview (deployed docs preview, headless Chromium)
The install command, its copy button and the "That writes ..." line now sit under the description and above the preview, as on the live catalog. Tabs are Preview, Code, Snippet, Docs with Preview selected. The Snippet tab holds the mount element with the values tuned in the panel; the duplicate Terminal command is gone. Frost Sequence Camera Orbit is a WebGPU composition: headless Chromium has no adapter, so its stage shows the recorded clip with the reason on the caption line and no Tune panel. Live WebGPU playback needs a real GPU Chrome.
Before
After
Catalog poster timing (deployed preview, head d7c75e4)
The pinned 3D motion group's six posters load eagerly at high priority. Measured from navigation start:
Copy button layout shift, and a pale copied label
Every copy button (agent request, wiring, link, JSON, render cmd, install) changed size when its label swapped to "Copied", so the row's siblings slid left. Root fix is in the one shared copy control: label and "Copied" now sit in the same grid cell, the inactive one hidden, so the button never resizes. Measured before/after on the deployed preview at 1440/1280/1100/900/390: no box moves now at any width (four of five widths moved before).
The primary button's hover state was then found unreadable in the copied state (white text on a pale background), caused by a hover rule with lower CSS specificity than the generic one. Fixed on the same control; verified computed background stays dark through hover, click, and the copied state.
After, copied state, primary button (agent request):