Repository navigation
fix(core): each mounted sub-composition shows its own variable values - #5158
miguel-heygen wants to merge 22 commits into
Conversation
data-var-text, data-var-src and color grading resolved per-instance values through the nearest data-composition-id. In a compiled render that is the sub-composition's own root, which keeps its authored id, while the values are keyed by the host's id, so every instance fell back to the defaults. An authored root directly inside a host now defers to the host.
Edit accuracy: accurate 271 (base branch 271), smooth 118 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
A composition root with no per-instance values of its own now defers to its direct parent, the host, instead of recognising hosts by their source attribute. This also covers inline-template hosts and keeps a nested host's own values, and is tested for text and src bindings.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
The per-instance variable scoping looks right to me. variableScopeOf skips past the authored root to the nearest mounted host, the compiler, bundler and runtime all mark that host, and color grading and bindings share the one helper, so they can't drift. The nested-host and empty-instance tests cover the cases I'd worry about. The three changed test files pass locally at this head (244/244).
Blocker: nested data-composition-src is resolved against the parent file, but the rest of the system resolves it against the project root.
compositionLoader.ts:738 does new URL(src, ancestry.at(-1) ?? document.baseURI), so a child path is read relative to the file that contains it. Everywhere else that path is root-relative at every nesting level:
- The compiler resolves nested hosts with the same
resolveHtml(src)it uses for top-level ones (inlineSubCompositions.ts:360,htmlBundler.ts:1032,resolve(sourceDir, relativePath)). - The render pre-flight says so explicitly (
producer/src/services/htmlCompiler.ts:264): "data-composition-src is always resolved root-relative, even from within a nested sub-composition". collectSubCompositionSrcsinparsers/src/assetResolution.ts:71says the same: "Callers must resolve each value against the PROJECT ROOT, never the referencing file's directory".
Repro at this head: index.html mounts compositions/scene.html, which mounts data-composition-src="compositions/card.html" (the normal layout). The loader fetches:
http://localhost:3000/compositions/scene.html
http://localhost:3000/compositions/compositions/card.html
The second request 404s, the host is emptied and external_composition_load_failed fires. A render of the same project shows the card, so the preview and the render disagree. On main the child isn't fetched at all in this pass, so the nested discovery is the new part and it lands on the wrong path.
Fix: resolve against document.baseURI for every level, and keep ancestry only for the cycle and depth checks (nestedCompositionRefusal works the same on root-resolved hrefs). The first new loader test then needs its child as scenes/inner.html rather than inner.html. Please also add a nested case with a relative path in a subdirectory, because the current tests only use absolute URLs or files at the root.
Nits (optional)
authoredSource.ts:101hardcodes"data-hf-variable-host". ImportingVARIABLE_HOST_ATTRkeeps the strip in step with the stamp if the name ever changes.- The 4-at-a-time loop at
compositionLoader.ts:729waits for the slowest load in each batch before starting the next four. It's fine as a bound, but a small pool that starts the next fetch as soon as one finishes would avoid one slow scene holding up the rest.
Verdict: REQUEST CHANGES
Reasoning: The variable-scoping fix is sound, but the new nested-child loading resolves paths relative to the parent file, while the compiler and render resolve them from the project root, so normal nested projects 404 in preview.
— Rames
|
Fixed in fe51808. Nested composition URLs now use the existing payload resolver with the project document base at every level. Ancestry is used only for cycle and depth checks. The loader and bundler share a nested-directory fixture; the loader fails with the old parent-relative resolver while the bundler succeeds. Both changed suites passed three consecutive runs, and headless Chrome loaded the same card in preview and bundled output without missing requests or page errors. |
somanshreddy
left a comment
somanshreddy
left a comment
There was a problem hiding this comment.
Review at fe518089. Scope: the fix commit over a0e24481, which Rames reviewed (4 files), plus CI at this head. I didn't re-review the full PR beyond that. This is a comment, not an approval.
The root-path fix works. mountExternalCompositions now resolves each child's data-composition-src through resolvePayloadSourceUrl(src, null), so every level resolves against document.baseURI. The ancestry is kept only for the cycle and depth checks (compositionLoader.ts:739-742), which is the fix Rames asked for. The targeted suites pass locally: compositionLoader, htmlBundler and applyVariableBindings, 246/246. I reverted line 739 to the old new URL(src, ancestry.at(-1) ?? document.baseURI) and two of the new tests failed ("loads external children in one pass…" and "matches bundled root-relative child paths…"), so the fix is pinned.
Blocker: CI. The required Test is red because Producer: unit tests fails, and this PR causes it.
htmlCompiler.parity.test.ts:102throwsmount diagnostic external_composition_load_failedforcompositions/card.html, withENOENT … open '/tmp/hf-compiler-parity-*/http:/localhost:3000/compositions/card.html'. It's raised fromcompositionLoader.ts:750.- The same failure is on
a0e24481, and main is green, so it comes from this PR's loader change rather than from this fix commit. - The loader now hands the producer's file-backed fetch an absolute
http://localhost:3000/…URL. The producer joins that onto its temp dir as if it were a path. Either the producer's fetch needs to map same-origin absolute URLs back to project paths, or the loader needs to keep passing a root-relative path there. - Until this is fixed, external sub-compositions don't mount in the producer's compile path. That's exactly what the parity test exists to catch.
Studio: timeline viewport gate is also red at this head, but it was green at a0e24481. I haven't attributed it; it may be a flake.
Verdict: the root-path blocker is fixed. One blocker remains: the producer parity failure, which turns the required Test red.
— Somu
…ids and order The producer parity test now mounts through the real file server, so the loader's root-relative URLs reach real files. Late nested hosts get their runtime id from core's shared identity map (a repeated composition is suffixed on every instance, a single one keeps its authored id when free), replacing the producer's private copy. The runtime loader still fetches sub-compositions concurrently but mounts them in document order, so their stylesheets land in <head> in a stable cascade order.
…d styles by host Mounting one sub-composition at a time made a later host wait for an earlier host's fetch, which hangs when that fetch depends on the later mount. Mounts run concurrently again; each injected link and style is inserted before the head nodes of any host later in the document, so the cascade order no longer depends on which fetch finishes first. The parity test gains a scene that mounts the same card twice, which pins the suffix-every-copy rule.
# Conflicts: # packages/core/src/compiler/htmlBundler.test.ts
|
@jrusso1020 thanks for the review at a0e2448. Your points were in the review body (there are no inline threads), so here is each one with the commit that addresses it. Current head: d2db3a5, all checks green. Blocker: nested Nit: Nit: 4-at-a-time batches wait for the slowest load. Done in a335439. The loader now runs four workers, and each one takes the next host as soon as its current load finishes. New test "starts a queued load as soon as any earlier load finishes": it holds the first of five loads until the fifth has started. Against the old batch loop that test times out. Also changed since your review:
Could you re-review at d2db3a5? |
… and same-named templates
# Conflicts: # packages/core/src/runtime/compositionLoader.ts
…the page lists them
…ts styles in page order
# Conflicts: # packages/studio/src/utils/authoredSource.test.ts # packages/studio/src/utils/authoredSource.ts
…st on large pages
…ad assets name their host
Fallow audit reportQuality gate failed Found 3 findings: Dead code 1, Duplication 2. Dead code (1)
Duplication (2)
Generated by fallow.
|
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 3 important · 0 nits — head dd6fd8af
The nested-path change request is resolved: mountExternalCompositions resolves every level through resolvePayloadSourceUrl(src, null) against document.baseURI, and keeps the ancestry only for cycle and depth checks. The merge with main is what broke this head.
Blocker: the main merge breaks main's own tests, and required Test is red. Test (runtime) fails on Linux and Tests on windows-latest: core fails on Windows. I reproduced it locally:
inlineSubCompositions.reuse.test.ts:57still callsassignBundledRuntimeCompositionIdswith a host list, so it fails withTypeError: document.querySelectorAll is not a function. This PR changed the function to take a document.pageBuild.golden.test.tsfails both the preview and the render goldens (from #5954). Nestedbadge__hfNstyles now merge into the card's<style>, and a<script data-hyperframes-preview-runtime>has moved into the head between styles.
Update the call site. For the goldens, confirm which differences are the intended page-order change, then regenerate. Fallow audit is also failing.
Important:
- Render orders head styles differently from preview and mount when a host has no id or shares one. Compiled assets are tagged with the host's id (
htmlCompiler.ts:1059-1061).headOrdermaps id → host, and the last element with a given id wins (compositionLoader.ts:200-210). A no-id host's assets get no tag at all. Repro: atpltemplate host followed by a no-id external host. Preview and mount givestyle:tpl, link:ext, style:ext; render giveslink:ext, style:ext, style:tpl. On main, both preview and render put template styles last, so they agreed. - Cycle and depth limits from the compiler are lost at runtime. The compiler leaves a refused nested host with
data-composition-srcon it (inlineSubCompositions.ts:791-797). Its inlined parent only carriesdata-composition-file, somountExternalCompositionstreats the refused host as top-level with an empty ancestry (compositionLoader.ts:695-707) and mounts it again. Witha.html↔b.html, Studio mount showsAB, while bundled preview and render plus the runtime showABAB(main showedABA). Separately, a level where every host is refused gets no id in the bundler (htmlBundler.ts:570): a self-includingscene.htmlgivesscene, scenein preview butscene, scene__hf1in mount. - A template host nested inside an external composition gets different ids on each path. Repro: a page
tplhost with#tpl-template, andscene.htmlalso containing atplhost. Mount givestpl, tpl__hf1; render (producer, then runtime) givestpl__hf1, tpl__hf2, because the unassigned nested copy still counts as waiting. The PR lists this shape as unsupported in preview, but render and mount disagreeing breaks the PR's own one-policy claim.
Scope
I read the loader, compositionHostIds, and the producer and bundler head-emission diffs, and checked the pool (in-order results, no worker rejects), duplicate-link ownership, variableScopeOf, and Studio stripping of data-hf-variable-host: all fine. The important items were reproduced with scratch tests on the PR's preview/render/mount parity harness, not by running real renders. I didn't read the whole parity test file.
|
Closing: this conflicts with main, has had no update for over 5 days, and no open issue or report names who it hurts. Reopen if someone hits it; the branch is kept. |
|
We hit this. Mounting the same block several times inside a scene (index.html → scene → block) with per-instance
Same block mounted directly from index.html works. Reproduced with 0.8.226 and 0.8.228 in preview and snapshot. Our workaround: omit Use case: one avatar block (person + emotion schedule as variables) reused across all scenes of an explainer. Would appreciate a reopen. |
Repeated sub-composition mounts now bind text, image sources, CSS values, and variable-driven color grading to each instance's values. An empty nested instance falls back to the top-level defaults instead of inheriting its outer instance's values.
Ownership is assigned internally by compilation and runtime mounting, independently of whether an instance has a variable-table entry. Live clipboard serialization strips that served metadata. Authored HTML is unchanged; the only public addition is the
runtime/variable-scopesubpath below.The runtime also loads external children discovered inside fetched or template-mounted parents during the same load call. It resolves child composition URLs against the project root at every nesting level, attempts each host once, preserves mounted parent identities, shares the compiler's cycle/depth checks (both compare the same key: the path resolved from the project root, without query or hash, so
./compositions/a.htmlandcompositions/a.htmlare one file), and keeps at most four loads in flight: a pool, so one slow composition does not hold back the ones queued behind it. Stylesheets from mounted compositions are inserted in host document order (a nested composition's right after its parent's, before the parent's later siblings) regardless of which fetch finishes first, and preview and render now emit sub-composition links and styles in the same order instead of breadth-first, each composition's links right before its own styles. So a page-wide rule (a same-named@keyframes, a font face, a stylesheet link) resolves the same way on every path. One position function in core decides that order for the compiler and the runtime. Neighbouring head styles still merge into one sheet, but a merge no longer crosses a link, so a stylesheet link keeps its place in the cascade. Page-level<template>hosts follow the same order: preview sorts their styles with the rest, and render, which mounts those hosts at runtime, tags each compiled link and style with its host's runtime id (data-hf-head-host) so the runtime places a template's styles among them by page order. The runtime builds the position index once per mount pass, not per inserted node. Scripts keep their existing inlining order; a stylesheet two compositions both link sits once, in the first one's slot. An external host now always loads its file: a same-named<template id="X-template">in the page no longer replaces it, which matches preview and render (they always inlined the file and its declared variables). Studio strips the variable-host mark through the constant core stamps it with, exported as@hyperframes/core/runtime/variable-scope.Preview, render and runtime mounting now share one owner for runtime composition ids (
assignCompositionHostIdsinruntime/compositionHostIds.ts). A host keeps its authored id unless another element in the document already uses it, or another host mounts the same composition; in either case it takes the lowest free<id>__hfN. All three paths assign one discovery level at a time: the top-level hosts first, then the hosts that their content reveals, and so on. So a card reused at two depths keepscardat the shallower level, and the deeper copy becomescard__hf1. Before this, the compiler numbered both copies and the runtime did not. Each host is assigned once per load.assignBundledRuntimeCompositionIdsnow takes the document instead of a host list, so no caller can seed a different set of hosts.Known unsupported shape (unchanged from main): a page-level
<template id="X-template">used by an empty host that sits inside an external composition. Preview expands template hosts only at the top level, so that nested host stays empty in preview, and render and mount give it different ids. It needs nested template expansion in the bundler and the producer, which is a separate change.Validation on Linux:
./compositions/scene.html; with the old raw-string cycle check, preview and render inline a second copy and the row fails. A parity row has a late card whose id also names a page template; the old template-first loader mounts the template and fails it, and a loader test pins that the file is fetched. Core runtime and compiler 2478/2478, producer parity and runtime browser 76/76, three runs each; producer compiler 140/140; studio-server preview 114/114.@keyframes pop; preview, render and mount must all list them s1, card, s2. On the previous head preview and render gave s1, s2, card and the test fails. Core runtime and compiler 2505/2505, producer parity, scriptOrder and runtime browser 84/84, producer compiler 140/140, three runs each.Before
Same author-only fixture on main. Repeated instances resolve through authored root identities.
After
File instances show File A and File B; template instances show Template A and Template B; the empty nested instance shows Top.