Repository navigation
docs(skills): document mounting one composition file several times - #6208
OldSlowDog wants to merge 1 commit into
Conversation
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for picking up #6139. The failing case in that issue is real and worth documenting, but I think this section describes a different cause than the one the engine has.
- Mounting one file several times with the same
data-composition-idon every host is supported. The runtime renames repeated hosts tostep__hf1,step__hf2, and so on (packages/core/src/runtime/compositionLoader.ts, around lines 385-404), and the scene script'sdocument.querySelector, GSAP targets andwindow.__timelines["step"]are all redirected to that mount (packages/core/src/compiler/compositionScoping.ts). That's also why theduplicate_composition_idlint rule skipsdata-composition-srchosts on purpose (packages/lint/src/rules/composition.ts, the comment above thedata-composition-srccheck). It matches the issue's own observation that shared ids work with a plain.scselector. - What breaks in the issue is the selector
[data-composition-id="step"] .sc. After the rename, no host carriesstepany more, so it matches nothing and the script throws once per mount. Your second bullet ("don't prefix in-file selectors with the composition id") is the right advice. It's the reason that's off: the prefixed selector matches none of the mounts, not all of them. - The example gives hosts
step-1..step-7while the scene root staysstep. That contradicts the rule a few lines above it on the same page ("Give the host, the scene root and the timeline key the same id ..."), so a reader would get two opposite instructions.
Suggested direction: keep the section short. Say that one file can be mounted several times with the same id, that the runtime gives each mount its own id behind the scenes, and that scene scripts should target elements by class or local id, never by [data-composition-id="..."], because that id is rewritten per mount.
Separately, the branch is based on a much older main and adds format.md as a new file, so it conflicts with the current one. Taking this branch's copy would also drop a fonts line that was added on main since. A rebase onto current main, so that the diff is only the new section, will fix both.
Verdict: REQUEST_CHANGES
Reasoning: The new guidance contradicts how the runtime and lint handle repeated mounts and conflicts with an existing rule on the same page, and the branch has merge conflicts that would drop current content.
bdabf65 to
139e012
Compare
OldSlowDog
changed the title
|
Thanks for the detailed correction — both points are fixed in You're right about the cause. I rewrote the section around it: repeated mounts sharing an id are supported (the runtime rewrites them to The branch is rebased onto current |
bfc3786 to
83026b1
Compare
|
Two refinements after that — head is now
Still six added lines on |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for the rework. Everything from my last review is addressed: the section now says repeated mounts can share a data-composition-id, explains that the prefixed selector matches nothing after the rename, drops the step-1..step-7 example, and the branch is rebased so the diff is just the new section. I checked the new wording against the rename in compositionLoader.ts, the per-mount query, GSAP and timeline scoping in compositionScoping.ts, and the lint rule, and it matches.
One thing before this can merge: changing format.md changes the hyperframes hash in skills-manifest.json, so the required "Skills: manifest in sync" check will fail once CI runs on this branch. Running bun run --cwd packages/cli gen:skills-manifest and committing the updated skills-manifest.json fixes it.
Two optional wording nits:
- The section uses
stepwithout saying where it comes from (the example just above usesintro). Something like "an id such asstep" on first use would help. - "An element
idworks too, but it still has to keep the page-wide uniqueness rule above": an id inside a file mounted several times appears once per mount, so it can never meet that rule. "Use a class for anything inside a file you mount more than once" would say it more directly.
Verdict: APPROVE
Reasoning: The guidance now matches how the runtime and lint handle repeated mounts and all earlier points are fixed; the manifest regeneration is mechanical and enforced by a required check.
Two wording fixes from review: - `step` now arrives with "(an id such as `step`)" on first use: the example above uses `intro`, so the bare id read as coming from nowhere. - Dropped "an element id works too, but it still has to keep the page-wide uniqueness rule above": an id inside a file mounted N times appears once per mount, so it can never satisfy that rule. It now says to use a class for anything inside a file mounted more than once, which is the only thing that actually holds. `format.md` is part of the `hyperframes` bundle, so its content hash in `skills-manifest.json` changed (77 files, 76c914f4... -> 5ddcce65...); regenerated so the "Skills: manifest in sync" check passes.
83026b1 to
cf82bac
Compare
|
Done in Both nits taken as well:
One note on the manifest, so you know what you're reviewing: this checkout is a partial clone, so I couldn't run |
|
Correction to the note above: I did end up running the real generator, and it agrees.
Against a full tree with this branch's So the committed manifest is not a hand-computed approximation — it is what the generator writes, diff |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks, this looks good. Both wording changes read well, and the "use a class" line is clearer than the old id sentence.
I checked the manifest update by running bun packages/cli/scripts/gen-skills-manifest.ts --check at this commit, and it reports the manifest in sync, so your reimplementation of the hash matches the real script.
The repository's CI workflows haven't run on this branch yet, so I'm leaving this as a comment for now. Once a maintainer approves the workflow run and the checks are green, this is ready from my side.
Verdict: COMMENT
Reasoning: All earlier points are fixed and the manifest check passes when run locally, but CI has not run on the fork head yet.
What changed
Added a short "Mounting one composition file several times" section to
skills/hyperframes/references/format.md, just below the existing "host id = scene id = timeline key" rule.Why
Issue #6139's failing case is real:
[data-composition-id="step"] .scthrows once per mount. The shared id is not the cause — mounting one file several times with the samedata-composition-idon every host is supported:compositionLoader.tsdetects the duplicate and rewrites the repeated hosts to<id>__hf1,<id>__hf2(uniqueCompositionId), keeping the authored id ondata-hf-original-composition-idand scoping that mount'sdocument.querySelector, GSAP targets andwindow.__timelinesentry to it.duplicate_composition_idskipsdata-composition-srchosts for the same reason: they are mounts, not composition roots.What breaks is targeting from inside the scene by that id. After the rename no host carries
stepany more, so the prefixed selector matches nothing.The section therefore says that mounts share
data-composition-idand differ by their own elementidanddata-variable-values, and that in-file selectors should use a class — an elementidstill has to satisfy the page-wide uniqueness rule on that page — never[data-composition-id="..."].Related work
checkerror. That is out of scope here and left as a follow-up, so the docs fix can land independently.How
Documentation only: six added lines to one existing file. Rewritten after review — the earlier revision described the shared id itself as the failure and gave the hosts distinct ids in an example, which contradicted the "same id" rule on the same page.
Test plan
format.md, so the surrounding rules — including the Google Fonts caching line — are untouched.packages/studio/packages/playerchanges, so the PR-captures (scripts/check-pr-captures.mjs) requirement does not apply.