fix(producer): scope per-instance variables for repeated sub-composition mounts#2070
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
R1 — hyperframes #2070 at 065e235cb57b045ec02f9153ab901bdbeb9cbdd9
🟢 LGTM. Correct follow-up to #2066. Same SSOT-restoration shape: assignBundledRuntimeCompositionIds, cssAttributeSelector, and BundledHostCompositionIdentity were previously module-private helpers in htmlBundler.ts (preview/snapshot path) that the render compiler didn't have; this PR exports them from @hyperframes/core/compiler and threads them into producer's inlineSubCompositions, closing the multi-mount divergence exactly. Contingent on the ~8 IN_PROGRESS CI checks landing green.
Verified
Divergence closure matches the same pattern as #2066.
- #2066 shared
buildVariablesByCompScript(writer emit) so preview and render paths can't drift on JS variable table emission. - #2070 shares
assignBundledRuntimeCompositionIds(per-instance runtime ids) +cssAttributeSelector(scope selector builder) so preview and render paths can't drift on multi-mount instance disambiguation. - Same pattern class: previously-bundler-only helpers become the shared source of truth for both compile targets.
Fix wiring at htmlCompiler.ts:790-806:
hostIdentityByElement = assignBundledRuntimeCompositionIds(hosts)produces the Map<Element, BundledHostCompositionIdentity> BEFORE inlining.- Passed to
inlineSubCompositionsSharedashostIdentityMap. buildScopeSelector: (compId) => cssAttributeSelector("data-composition-id", compId)passed alongside.- The inliner then rewrites each host's
data-composition-id(card→card__hf1/card__hf2on duplicates, unchanged on singletons per theuniqueCompositionId(baseId, index)naming) AND scopes each per-instance script's CSS selector to the runtime id.
Test targets the exact regression the reproduction table shows.
Two card mounts with {"label":"CARD_A"} and {"label":"CARD_B"}. Asserts:
- Both
card__hf1andcard__hf2runtime ids appear viadata-composition-file="compositions/card.html"query on the mount points. - Both
"card__hf1":{"label":"CARD_A"}and"card__hf2":{"label":"CARD_B"}substrings appear in the compiled writer JS.
Directly falsifiable against main — pre-fix, both mounts would keep data-composition-id="card" and the writer would have a single "card":{"label":"CARD_B"} entry (last-write-wins).
Known-limitations transparency in PR body.
James enumerates two pre-existing fragilities NOT closed by this PR:
- GSAP-derived duration + no
data-duration+ duplicated HTMLid→ still fails (duration resolves off HTMLid, timelines register under runtime id — decorative-gate-shape mismatch, same class as my personal [[decorative-gate-pattern]] memory). - Runtime-id counting is scoped to top-level
[data-composition-src]hosts; exotic mounts (<template>+data-composition-srcmixing) may differ from bundler's global count.
Both are strictly not-worse-than-main (verified via James's "identical zero-duration failure with and without this change on merged main" claim). Scope-cap is honest.
Observations (non-blocking)
O1 — Test coverage is narrow.
Single case: two mounts of one sub-comp with different values. Not tested:
- 3+ mounts of the same sub-comp (does
card__hf3,card__hf4land? — implicit from theuniqueCompositionId(baseId, index)shape, but a value case would pin it). - Mixed: one singleton mount + one duplicated pair in the same host (does the singleton stay authored-id while the pair renames? — the PR body claims "Single instances are untouched" but this isn't test-pinned).
- Nested duplicates across files (called out as pre-existing gap in Known Limitations — no need to add coverage now, but the moment someone attempts to fix it, a test here would fail-first).
Not blocking; the primary regression case is covered.
O2 — Envelope carries the AI-attribution markers.
Commit 065e235c has Co-Authored-By: Claude trailer and PR body has the 🤖 Generated with [Claude Code] footer. Same envelope as #2069. Per my personal-memory record of Miguel's convention on hyperframes-repo PRs (from HF #1829 2026-07-01), both are typically flagged as request-changes before merge. Docs #2069 was framed as your call; this one's a real functional fix, so the AI-attribution signal carries more weight in Miguel's convention. Still your judgment — noting observationally.
O3 — // fallow-ignore-next-line complexity.
Added above assignBundledRuntimeCompositionIds because the function is over Fallow's complexity threshold. Adding the export doesn't change complexity, but making the function part of the public compiler-package API does slightly elevate the surface area. If Fallow's complexity threshold flags this later, worth a small extract (e.g. the counting pass could become its own helper). Not urgent.
O4 — CI in-flight at review time.
Green so far: Format, Lint, SDK: unit+contract+smoke, Test: runtime contract, Semantic PR title, File size check, CodeQL (python), Analyze (actions), all workflow "Detect changes" passes.
In-flight: Analyze (javascript-typescript), Preflight × 4 workflows, Build, Fallow audit, Typecheck, Test, Studio: load smoke, CLI smoke.
My 🟢 is contingent on those completing green — particularly Test (which includes the new regression test) and Fallow audit (the fallow-ignore comment should suppress the complexity finding).
Pattern
Rare positive-case appearance of my [[verify-full-dispatch-chain]] discipline — the fix is consolidation of two parallel dispatch chains into one shared implementation, not "verify the chain reaches every consumer." Preview and render both need per-instance runtime ids; now they get them from the same helper. Third instance of this pattern in ~48h (with #2066's writer emit and #2062's reference resolver extraction).
R1 by Via
miga-heygen
left a comment
There was a problem hiding this comment.
Review: LGTM ✅
Well-scoped fix that correctly reuses the bundler's existing logic rather than reimplementing it — genuine single-source-of-truth improvement.
SSOT audit — clean
assignBundledRuntimeCompositionIds— now shared between the preview bundler and the render producer. Same function, no duplicated logic. ✓cssAttributeSelector— pure utility, correctly exported alongside the assignment function. ✓- No new helpers that duplicate existing decisions. ✓
Verified
- Core approach: Both preview bundler and render producer now use
assignBundledRuntimeCompositionIdsto assign per-instance runtime IDs (card__hf1,card__hf2), eliminating the render path divergence that caused all-but-one instance to render blank. - Exports are minimal: Only
cssAttributeSelector,BundledHostCompositionIdentity, andassignBundledRuntimeCompositionIdsexported. Re-exported from@hyperframes/core/compiler/index.tswith proper alphabetical ordering. as unknown as Element[]cast: Consistent with existing pattern in the producer file — linkedom elements satisfy the DOM interface at runtime but need the cast for TypeScript.- Host set difference: Bundler passes ALL tracked hosts; producer passes only
[data-composition-src]hosts. Correct — producer doesn't use template-based compositions. Known limitation documented in PR description. shouldAssignBundledRuntimeCompositionId: Returnstrueimmediately fordata-composition-srchosts, so thehost.ownerDocument/host.children.lengthguards never execute in the producer path. No linkedom incompatibility risk.- Test coverage: Directly reproduces the multi-instance variable clobbering bug — two mounts of the same sub-comp with different values, asserts unique runtime IDs and per-instance variable scoping.
fallow-ignore-next-line complexity: Follows established codebase pattern. Function complexity unchanged, only visibility changed.
No issues found. Ship it.
— Miga
065e235 to
ef4cf35
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
HF#2070 review — Compiler assigns runtime composition ids to match bundler (duplicate-mount fix)
Reviewed at ef4cf359 (rebased from 065e235c; ahead=1/behind=1, same 4 files, +69/-2 ≈ +70/-3, no shape change).
Tight follow-up to #2066: 4 files, +69/-2. Same SSOT-consolidation shape — helpers that lived only in htmlBundler.ts (preview) get exported from @hyperframes/core/compiler and threaded into producer's inlineSubCompositions, closing the multi-mount divergence. Via already posted a thorough 🟢. Layering independent verification below.
Verification vs PR-body claims (per-claim):
- Export
assignBundledRuntimeCompositionIds,cssAttributeSelector,BundledHostCompositionIdentityfrom@hyperframes/core/compiler→ verifiedpackages/core/src/compiler/index.ts:31-34; each helper's declaration flipped toexportinhtmlBundler.ts:299,308,354-355. - Producer's
inlineSubCompositionscallsassignBundledRuntimeCompositionIds(hosts)→ verifiedhtmlCompiler.ts:799. - Threads
hostIdentityMap+buildScopeSelectorinto shared inliner mirroring preview bundler → verifiedhtmlCompiler.ts:805-806matcheshtmlBundler.ts:788,793almost verbatim (both passhostIdentityByElementand the same(compId) => cssAttributeSelector("data-composition-id", compId)lambda). - Single instances untouched → verified at branch level.
htmlBundler.ts:374duplicateInstance = counts.get(id) > 1; for singletonsruntimeCompositionId = authoredCompositionId(L388-389) and L390'ssetAttributewrites the same value back — no visible change. Existing single-mount test athtmlCompiler.test.ts:1764still asserts"card-1":{...}under the authored id, indirectly pinning this.
Symmetric implementation check (does producer's call sequence + arg shape match bundler's?):
- Bundler (
htmlBundler.ts:775-781) callsassignBundledRuntimeCompositionIds(trackedCompositionHosts)— input includes BOTH[data-composition-src]hosts AND inline<template>-backed hosts — then filters tosubCompositionHostsbefore the shared inliner. - Producer (
htmlCompiler.ts:775,799) calls withArray.from(querySelectorAll("[data-composition-src]"))— no<template>inline hosts because the render path doesn't support that mechanism. - Net: producer's counting is over a strictly narrower host set than bundler's, but the narrower set == everything the render path can actually inline. Divergence is inert for supported mounts — exactly the known-limitation #2 in the PR body. Faithful description.
- Downstream inside
inlineSubCompositions.ts:hostIdentityMap.get(hostEl)supplies bothauthoredCompositionId(for querying sub-comp's inner root at L231-233) andruntimeCompositionId(for writer keys at L246, scoped script__hfTimelineCompIdatcompositionScoping.ts:505, and CSS scope selector). Both paths pull from the same map — no way for reader/writer keys to drift.
Sibling __hf* global audit (Via's O2 from #2066): enumerated all window.__hf* in packages/core/src: __hfAnime, __hfD3, __hfDebug, __hfFlushSync, __hfGoogleMaps, __hfLeaflet, __hfLottie, __hfMapbox, __hfMaplibre, __hfReseekGpu, __hfRuntimeTeardown, __hfThreeTime, __hfTimelinesBuilding, __hfTypegpuTime, __hfVariables, __hfVariablesByComp. Only __hfVariablesByComp is a dict keyed by composition-id; every other global is an array (adapter instance registries — accumulate by push, no id collision), a scalar flag, or a single global map. So the "keyed-by-comp-id, clobbers on duplicate mounts" hazard class has exactly one member and this PR closes it. Via's O2 neutralized.
Tests audit:
- New test (
htmlCompiler.test.ts:1767-1813) PINS the exact regression from the PR-body table: twocardmounts, distinctdata-variable-values, assertscard__hf1+card__hf2appear on mount points and"card__hf1":{"label":"CARD_A"}+"card__hf2":{"label":"CARD_B"}land in the writer. Directly falsifiable against pre-fixmain. PINS safe behavior. - Chain coverage is partial-but-sufficient: runtime-id assignment + writer emit asserted directly; scope selector + scoped-script wrapping + per-instance CSS rules share the same
runtimeCompIdthrough the shared inliner — coupled by construction, not two independent paths. Pixel-verified render in PR body is the E2E proof. - Via's O1 (no 3+-mount case, no mixed-singleton+duplicate case) stands as nice-to-have — implicit from
uniqueCompositionId(id, index)but not test-pinned.
Known-limitations verification (both listed — faithful?):
- Duration derived from GSAP timeline + no
data-duration+ duplicated HTMLidstill fails → verifiedhtmlCompiler.ts:401keys duration resolution onel.id(HTML id attr), notdata-composition-id. Timelines register under runtime comp id via scoping proxy. Two different key spaces; mismatch is real and pre-existing. Description accurate. - Runtime-id counting covers top-level hosts; exotic mixes may differ from bundler global count → verified as covered under symmetric check above. Accurate.
- Nothing else jumped out as a NEW limitation this PR introduces.
Claude-generated code review:
- No over-engineering — 2-line addition to shared-inliner call + 1 line for runtime-id assignment.
- No hallucinated APIs — every function referenced exists in imports and is unit-verified at source.
- No defensive guards where invariants hold — existing
if (!hosts.length)early-return athtmlCompiler.ts:777already covers zero-hosts. - No mock-tests — new test uses real filesystem writes + calls
compileForRender(production entry). Byte-identical to sibling single-mount tests. - One minor smell: new
// fallow-ignore-next-line complexityathtmlBundler.ts:354(Via also flagged in O3). Theexportchange crosses Fallow's complexity threshold — a small extraction would clear properly. Not urgent.
Cross-check with #2066: preserved. htmlCompiler.ts:909's buildVariablesByCompScript(result.variablesByComp) call — the writer-emit block #2066 added — is intact. This PR just makes the WRITER's keys per-instance runtime ids (via the identity map inside the shared inliner), so #2066's shape stays but output disambiguates by mount. emitRootCompositionVariableStyles(document, result.variablesByComp, overrides) at L922 also flows through the same runtime-id-keyed map, so scoped CSS rules use [data-composition-id="card__hf1"] / [data-composition-id="card__hf2"] — coherent with the mutated hosts.
CI at review time: Format, Lint, Test, Typecheck, Build, Fallow audit, SDK unit+contract+smoke, Studio load smoke, Preview parity, preview-regression, player-perf, Semantic PR title — all pass. Pending: regression-shards shards 1-8 (shard-7 covers sub-composition-video — the exact class this PR touches), Tests on windows-latest, Render on windows-latest, Perf drift/fps/load/scrub/parity, Smoke global install, CLI smoke, Analyze. No red.
Verdict: 🟢 LGTM — contingent on pending checks landing green, particularly regression-shards (shard-7) given it exercises sub-composition-video. Same-shape follow-up to #2066, correctly-mirrored bundler pattern, honest known-limitations scope.
— Review by Rames D Jusso
miguel-heygen
left a comment
There was a problem hiding this comment.
Clean follow-up to #2066. I verified the current head ef4cf359 after the review-response rebase, including the simplification that no longer exports cssAttributeSelector.
Strengths:
packages/producer/src/services/htmlCompiler.tsassigns per-instance runtime IDs before the shared inliner and passes the identity map instead of reimplementing preview logic.packages/core/src/compiler/inlineSubCompositions.tsuses the runtime comp id for the default scope selector and variable table, so producer variables/CSS/scripts stay aligned through one shared path.packages/producer/src/services/htmlCompiler.test.tsreproduces the duplicate-mount regression and pinscard__hf1/card__hf2with distinct values.
Local verification in an isolated worktree:
bun test packages/producer/src/services/htmlCompiler.test.ts --test-name-pattern "sub-composition variable injection" -> 4 passed.
CI is green on the current head, including Windows render/tests and all 8 regression shards. Shard 7 (sub-composition-video) passed, which is the load-bearing shard for this fix.
No blockers from me.
Verdict: APPROVE
Reasoning: The render compiler now reuses the bundler's runtime-id assignment source of truth, the shared inliner keeps runtime IDs aligned across variables/scopes/timelines, and focused local plus full CI coverage are green.
- Magi
…ion mounts #2066 fixed sub-composition data-variable-values on the render path for a single mount, but the reusable-template pattern from #2064 (the same sub-comp mounted multiple times with different values) still diverged from preview/snapshot: every mount shared one __hfVariablesByComp key and one CSS scope selector, so the last mount's values clobbered the earlier ones and all-but-one instance rendered blank. The producer now assigns per-instance runtime composition ids (assignBundledRuntimeCompositionIds) and threads hostIdentityMap into the shared inliner, mirroring the preview bundler. The shared inliner's default buildScopeSelector already scopes by the runtime id, and timelines remap to it via the scoping proxy, so each instance's variables, CSS, and timeline land under its own id. Pixel-verified end to end: two mounts of one sub-comp with different data-variable-values now render their own content (green CARD_A / blue CARD_B), matching snapshot; single-instance behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ef4cf35 to
de7c6d8
Compare
|
Thanks @vanceingalls and @miga-heygen 🙏 — addressed the feedback. Two changes since your review (force-pushed Heads-up: I simplified the exports since you looked. You both verified O1 (test coverage) — added two cases:
O2 (AI-attribution envelope) — keeping it intentionally on this one; it's my standing convention for my own PRs. Happy to strip if that's a hard blocker for merge — just say the word. O3 ( CI is green on the required checks so far (Fallow, Lint, Format, Typecheck, Studio/SDK smoke); regression shards + Windows still running. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-stamp after the de7c6d8c7 force-push.
The post-approval delta from ef4cf359 is test-only in packages/producer/src/services/htmlCompiler.test.ts. It adds the two missing edge-case pins from the prior review round:
- 3+ repeated mounts keep deterministic runtime ids (
card__hf1throughcard__hf4) and distinct variable table entries. - Mixed singleton + duplicate mounts keep the singleton authored id (
solo) while only duplicatedcardhosts get runtime ids.
I reran the focused producer regression locally:
bun test packages/producer/src/services/htmlCompiler.test.ts --test-name-pattern "sub-composition variable injection" -> 6 passed.
CI is green on the current head, including Windows render/tests and all 8 regression shards. Shard 7 (sub-composition-video) passed.
Verdict: APPROVE
Reasoning: The new commit only strengthens regression coverage around the existing approved fix; the targeted local suite and full CI are green.
- Magi
What
Follow-up to #2066 (which closed #2064). #2066 fixed sub-composition
data-variable-valueson the render path for a single mount. But the reusable-template pattern called out in #2064's own Additional Context — the same sub-comp mounted multiple times with different values — still silently diverged between preview/snapshot and render.Repro (pixel-verified)
Two mounts of one sub-comp (
compositions/card.html), same authoreddata-composition-id="card", differentdata-variable-values:snapshot/ previewrender(before this PR, incl. #2066)render(after this PR)Root cause
The preview bundler assigns per-instance runtime composition ids (
card__hf1,card__hf2) viaassignBundledRuntimeCompositionIds, so each instance gets its own__hfVariablesByCompkey and its own CSS scope selector. The producer's render compiler never did this — every mount kept the authored idcard, so:__hfVariablesByComp["card"]was overwritten by the last mount (earlier mounts lost their values), and[data-composition-id="card"]root, so one instance was written twice and the other never — rendering blank.Fix
assignBundledRuntimeCompositionIdsandBundledHostCompositionIdentityfrom@hyperframes/core/compiler.inlineSubCompositionsnow callsassignBundledRuntimeCompositionIds(hosts)and passes the resultinghostIdentityMapinto the shared inliner — mirroring the preview bundler. The shared inliner's defaultbuildScopeSelectoralready scopes by the runtime id, and per-instance timelines already remap to it via the scoping proxy, so each instance's variables, CSS, and timeline land under its own id. Single instances are untouched (only duplicates are renamed).Tests
compileForRenderregression test: two mounts of one sub-comp → unique runtime idscard__hf1/card__hf2, each carrying its own values, no cross-instance clobber.hyperframes render+ frame extraction (the table above).Known limitations (pre-existing, not introduced here)
data-durationand a duplicatedidattribute still fail duration resolution (empirically verified: identical "zero duration" failure with and without this change on merged main). Duration resolution keys off the HTMLidattribute while timelines register under the runtime id — a separate, pre-existing fragility. The common template pattern (explicitdata-duration) works.[data-composition-src]hosts; exotic mixes (adata-composition-srcmount + an inline<template>mount of the same id, or nested duplicates across files) may still differ from the bundler's global count. This PR strictly improves parity vs. the previous state (which assigned no runtime ids at all).Follow-up (not in this PR)
A code-review pass noted that
assignBundledRuntimeCompositionIds(and itsBundled*helpers/type) now serve both the preview and render paths but still live inhtmlBundler.ts. A cleaner home would be a sharedcompositionIdentitymodule both callers import, with theBundledprefix dropped. Left out to keep this fix surgical — the producer already imports several bundler-defined helpers (prepareFlattenedInnerRoot,emitRootCompositionVariableStyles) from@hyperframes/core/compiler, so this follows the established pattern.🤖 Generated with Claude Code