fix: activate nested child timelines on renderSeek (sub-comp at t=0)

[miguel-heygen]

Summary

• Producer inlining path strips inner root element (innerHTML when compId matches), losing id attribute
• Bundler path preserves it via flattenInnerRoot + data-hf-authored-root-id
• Sub-comp CSS/GSAP using #ID selectors fail silently during render while working in preview
• Adds regression fixture + test documenting the divergence

Root cause

In inlineSubCompositions.ts line 307: hostEl.innerHTML = compId ? innerRoot.innerHTML : innerRoot.outerHTML
When compId is truthy, innerHTML strips the wrapper div with its id attribute.
The CSS scoper rewrites #ID to [data-hf-authored-root-id=ID] but no element has that attribute in the producer path.

Workaround

Use [data-composition-id="name"] as scope in sub-compositions instead of #name.

Proper fix (follow-up)

Make the producer path add data-hf-authored-root-id to the host element when the inner root has an id attribute.

[jrusso1020]

Reviewed full files on the changed paths. Solid investigation. A few additive points focused on areas Vance / Vai are less likely to flag — the preview-vs-render parity invariants and the runtime activation semantics. Posting as COMMENT, not stamping.

Strong points

• The activateNestedChildTimelines + renderSeek(t, activateChildren=true) pair is the right shape for the t=0 sub-comp bug. Mirrors the activateSiblingTimelines precedent in player.ts — consistent with existing render path.
• The frameCapture.ts change (always force rebind when ready) plugs a real timing hole: sub-comp scripts that finish executing before the poll starts wouldn't have grown the timeline count during the poll, so the previous gate timelinesAfterPoll > timelinesBeforePoll would skip rebinding and leave child timelines un-nested.
• Characterization tests for the #ID divergence (compositionScoping.test.ts + new inlineSubCompositions.test.ts) are well-named and pin the current behavior cleanly, including the "documents the divergence" test which makes the gap explicit.
• The fixture under __fixtures__/sub-comp-t0/ is exactly the right shape for a regression test — first sub-comp at data-start="0.001", second at data-start="2.5" so it can verify both paths.

Concerns worth addressing before merge

1. Title/summary describes a different fix than what landed (important)

The PR title says "fix: sub-composition #ID selector scoping divergence between preview and render" and the Summary leads with that, but the actual runtime fix in this diff (the only real behavior change in production code) is the activateNestedChildTimelines call + frameCapture.ts rebind change — which addresses a separate bug: sub-compositions at data-start near 0 not activating during render.

The #ID issue is only documented (with tests + a "use [data-composition-id=...] as scope" workaround note) — the PR body's Proper fix (follow-up) line confirms it's not actually fixed here.

This will bite future bisection: someone searching git log for "when did t=0 sub-comp activation get fixed?" won't find it via the title or summary. Suggest either:

• Renaming to fix: activate nested child timelines on renderSeek (sub-comp at t=0) + putting the #ID divergence work as a secondary section, OR
• Splitting into two PRs — one for the runtime fix (init.ts + frameCapture.ts + their tests + the t=0 fixture), one for the #ID characterization tests/fixtures/workaround docs.

I'd lean toward renaming since the work is interrelated. But the current framing makes the actual fix invisible.

2. activateNestedChildTimelines scope — confirms intent on inactive sub-comps

The function unpauses every non-master timeline, including timelines for sub-comps that aren't active at the current frame (e.g., one with data-start=10 while we're rendering t=1).

Worth confirming this is intentional and that GSAP's paused(false) on a child timeline that hasn't yet been reached by the parent's playhead doesn't have side effects — specifically:

• Does it trigger any onStart / onUpdate(0) callbacks?
• Could it cause initial-state property writes on elements inside an inactive sub-comp (overwriting elements visible in another sub-comp via the same CSS selector)?

If yes, scoping the activation to only-currently-active sub-comps (those whose [data-start, data-start+data-duration] window contains t) would be safer. If no (which I think is the case based on GSAP behavior), worth a one-line comment in activateNestedChildTimelines saying "unpause everything; per-frame visibility is gated by the engine — GSAP paused(false) on a child without play() is a no-op until the parent reaches it" — saves the next reader the same question.

3. frameCapture.ts — log line for the unconditional rebind

The new code path always calls __hfForceTimelineRebind when the poll returns ready. The cost is small but unconditional per render, vs. the previous "only when timelines grew" gate. If a future regression makes the rebind expensive, you'll want a counter — currently no observability into "did this rebind fire and find anything new?" Cheap follow-up: log the count of timelines rebind-touched (or a no-op fast-path if the rebind is genuinely free).

4. Test fixture network-load risk

__fixtures__/sub-comp-id-selector/intro.html and sub-comp-t0/index.html reference https://cdn.jsdelivr.net/npm/gsap@3.14.2/dist/gsap.min.js. Fine if the tests only inspect the inliner output (which they appear to — inlineSubCompositions.test.ts passes a mock resolveHtml). Worth confirming neither fixture is loaded into a real browser by any other test in this PR, since CI without network → silent skip is a familiar trap.

5. Future regression-shard coverage

Once the __fixtures__/sub-comp-t0/ project lands, consider adding it to the regression-shards matrix in .github/workflows/ci.yml alongside the existing sub-composition-video test — would catch a re-regression of the t=0 activation behavior in the actual render path, not just the unit-level mock. Not a blocker; the unit test is the bigger value.

Non-blocking

• The "Proper fix (follow-up)" for #ID selectors — making the producer path add data-hf-authored-id to the host element when the inner root has an id attribute — is the right shape. If you want a sibling PR, the change is at inlineSubCompositions.ts:307 (per the PR body's own root-cause analysis). Happy to take that on if it'd help.

Overall: real fix lands cleanly; just needs the title/summary aligned to what actually changed, and the activateNestedChildTimelines scope question answered before merge.

— Rames Jusso

[vanceingalls]

Solid regression-coverage PR with two distinct fixes glued together. The diagnosis in the PR body is verifiable end-to-end: inlineSubCompositions.ts:307 (hostEl.innerHTML = compId ? innerRoot.innerHTML : innerRoot.outerHTML) strips the wrapper carrying id, and compositionScoping.ts rewrites #id → [data-hf-authored-id="id"] but the producer path never adds that attribute to any element — confirmed in source. The renderSeek child-activation fix mirrors the existing activateSiblingTimelines pattern in player.ts:175.

Calibrated strengths

• inlineSubCompositions.test.ts is the kind of test I want to see on a divergence bug: separate cases for producer vs bundler path, with expect(host.querySelector("#intro")).toBeNull() and expect(scopedCss).toContain('[data-hf-authored-id="intro"]') pinning the exact failure shape, not the happy path. The "documents the divergence" test at the bottom names the contract violation explicitly.
• init.test.ts:444 pins the bug-shape correctly: pre-pauses the child timelines, calls renderSeek(0.5), asserts both became unpaused AND that visibility flipped to "visible". Without the fix the second assertion would fail — that's the right pinning.
• frameCapture.ts:368-382 — replacing the timelinesAfterPoll > timelinesBeforePoll guard with "always rebind once ready" is the right call. The count comparison was a fast-page-load race; the unconditional rebind is idempotent (__hfForceTimelineRebind just resets childrenBound=false and re-binds, see init.ts:967).

Important findings

• important — title vs. scope mismatch (commit 3 is a different bug): The PR title and summary are about #ID selector scoping. Commit 3 (fix: activate nested child timelines during renderSeek — init.ts + frameCapture.ts, plus the sub-comp-t0 fixture and init.test.ts:444 case) is a separate bug — sub-composition timelines at data-start≈0 not activating during render. Same symptom class ("renders silently wrong") but a different root cause and a different code path. Future readers grepping git log --oneline for the t=0 renderSeek bug won't find this PR. Either split into two PRs, or update the title to "fix: two sub-composition render-vs-preview divergences" and add a short paragraph in the body about commit 3's bug.

• important — this PR documents the divergence, it doesn't fix it: The PR body says it plainly ("Workaround: use [data-composition-id="name"] … Proper fix (follow-up): make the producer path add data-hf-authored-id to the host"). The regression test at inlineSubCompositions.test.ts:152-171 ASSERTS the bug-shape (expect(authoredIdElement).toBeNull()), so a future PR that lands the real fix will fail this test and will need to flip those assertions. That's a footgun: a less-aware author may delete the assertions to "make the tests pass" and lose the regression coverage. Two concrete asks:

  1. Mark the "documents the divergence" test with it.todo or a // FIXME(#ISSUE): remove once producer path adds data-hf-authored-id comment + a linked tracking issue/ticket.
  2. Add a one-paragraph entry in docs/guides/troubleshooting.mdx or wherever sub-comp authoring is documented, surfacing the workaround. Right now users with #id selectors in sub-comps hit a silent render-vs-preview mismatch and have no docs to find. A compile-time console.warn when inlineSubCompositions sees a sub-comp with an authored root id and no flattenInnerRoot (i.e. the producer path) would be even better — fast feedback at compile, not after a render.

• important — activateNestedChildTimelines naming + scope: The function name says "ChildTimelines" but it iterates window.__timelines siblings (registry entries other than master), not nested-under-master children. This is exactly the registry pattern used by forEachSiblingTimeline in player.ts:32. The behavior is correct (it's what player.ts:175 activateSiblingTimelines already does), but the name will mislead the next reader. Suggest renaming to activateRegisteredSiblingTimelines or just activateSiblingTimelines (matching player.ts). Also: prefer tl.play() over tl.paused(false) for consistency with player.ts:94 — both unpause, but play() also handles reverse/direction state.

Nits

• nit — broad-unpause side effect: activateNestedChildTimelines unpauses every entry in __timelines except master, with no restore. player.ts's seekMasterAndSiblingTimelinesDeterministically (line 64) re-pauses siblings after seeking; the new code does not. For render-seek (one frame at a time), the residual unpaused state is probably fine — the next call re-unpauses idempotently and no transport tick runs between frames. Worth a one-line comment explaining the deliberate divergence from the seek path's "restore after" pattern, otherwise it'll look like a missed finally.
• nit — seekTimelineAndAdapters(t, activateChildren = false): The optional positional activateChildren flag is footgun-shaped. If a future seek site is added and the author forgets it, render-seek-only sites silently regress to the old behavior. Consider an enum or { activateChildren?: boolean } opts arg, or split into two functions (renderSeekTimelineAndAdapters calling the activate path explicitly).
• nit — [id="intro"] attribute-selector edge: replaceAuthoredRootIdSelectors in compositionScoping.ts:71 correctly skips inside [...] brackets, so [id="intro"] does NOT get rewritten to use data-hf-authored-id. Probably fine (designers rarely author [id=...]), but worth a one-line test asserting the no-rewrite behavior so the boundary is pinned.

CI status (head 0d12a465a3):

• 0 failures.
• 8 regression-shards IN_PROGRESS at review time — shard-7 includes sub-composition-video which is the most relevant to this PR's surface. None FAILED so far, but pending != green; this verdict assumes those shards complete cleanly. Worth a re-check before merging.
• All other required checks green: Build, Test, Typecheck, CLI smoke (required), Smoke: global install, Preflight (lint+format), Preview parity, Render on windows-latest, Tests on windows-latest, Player perf (load/fps/scrub/drift/parity), CodeQL (actions / js-ts / python), Fallow audit.

Verdict: APPROVE
Reasoning: Diagnosis is verifiable in source, tests pin the right invariants, and the renderSeek fix is consistent with the existing player.ts:175 pattern. The "this PR documents the divergence rather than fixing it" point is intentional per the PR body, but please file the follow-up ticket so the proper producer-side fix doesn't fall on the floor.

Review by Vai

[jrusso1020]

Re-review on 6652fae2. All five concerns from my prior pass + all of Vai's concerns are addressed:

• ✅ Title now reads fix: activate nested child timelines on renderSeek (sub-comp at t=0) — matches the actual runtime fix.
• ✅ Function renamed activateNestedChildTimelines → activateSiblingTimelines, aligning with player.ts:32's forEachSiblingTimeline / :89's activateSiblingTimelines. The function-level comment now explicitly clarifies "siblings in the registry, not GSAP child tweens" — good.
• ✅ tl.paused(false) → tl.play() — clearer intent.
• ✅ Positional flag → options object ({ activateChildren: true }) — un-footgunned.
• ✅ [id="intro"] attribute-selector edge pinned by a new test (does not rewrite [id="intro"] attribute selectors).
• ✅ Producer-side root-cause fix landed in this PR — inlineSubCompositions.ts:307 now propagates authoredRootId to the host as data-hf-authored-id when innerHTML strips the wrapper. The rewritten [data-hf-authored-id=...] selectors now actually resolve in the producer path. This was the "Proper fix (follow-up)" — kudos for including it.
• ✅ New test producer path propagates data-hf-authored-id to host when inner root has id directly verifies the new behavior; flips the previous bug-shape assertion appropriately.
• ✅ Regression-shards updated — sub-comp-t0 and sub-comp-id-selector added to shard-7 in regression.yml, alongside the existing sub-composition-video. The new packages/producer/tests/sub-comp-t0/ + sub-comp-id-selector/ directories include LFS-tracked baseline output.mp4s, so the regression actually compares against ground-truth video output.
• ✅ No-restore divergence documented — the new comment block explicitly notes "render-seek is one-frame-at-a-time with no transport tick between frames, so the residual unpaused state is harmless — the next call re-activates idempotently." Good explanation.

Minor / non-blocking:

• PR body is slightly stale. The "Workaround" + "Proper fix (follow-up)" sections still describe the producer-side fix as deferred, but the diff actually ships it (inlineSubCompositions.ts:307 change + producer path propagates data-hf-authored-id to host test). Worth updating the body so a future bisector reading just the description doesn't mis-infer. Cosmetic; not a blocker.
• The activateSiblingTimelines still iterates and unpauses every non-master timeline (including those outside their [data-start, data-start+data-duration] window). The added comment explains why that's safe — agreed for the render path, where per-frame visibility gates output.

CI at 6652fae2: required checks all green (Build, Test, Typecheck, Lint, CLI smoke, Preview parity, Windows render, CodeQL, Fallow). Regression-shards mostly green; shard-7 (which now includes sub-comp-t0 + sub-comp-id-selector) worth checking before merge — those are the directly-relevant ones for this fix.

No further blockers from my side. Holding off on the stamp per merge policy; happy to defer to Vai's APPROVE and your final call.

— Rames Jusso

[vanceingalls]

Re-review on 6652fae after Miguel's push. Three importants and two nits from the prior review (4322670036) were either addressed or escalated. Notably commit 4 lands the producer-side root-cause fix that the prior review flagged as missing — a stronger PR than v1.

Status of prior findings

• important — title vs. scope mismatch: STILL OPEN. The title is fix: activate nested child timelines on renderSeek (sub-comp at t=0) and the body's "Proper fix (follow-up)" paragraph reads as if the producer fix is deferred. Both are now stale:

  • Commit 4 (fix(#969): producer path propagates data-hf-authored-id to host element, inlineSubCompositions.ts:308-313) IS the root-cause fix. Closes #969.
  • The PR now bundles three fixes: (1) renderSeek child activation, (2) regression coverage for the #ID divergence, (3) producer-side fix for that divergence.
  • Please update the title to something like fix: sub-composition render-vs-preview divergences (#ID scoping + renderSeek at t=0) and rewrite the "Proper fix (follow-up)" section to "Producer fix (this PR)". Otherwise git log --oneline searches for the #ID bug land on commit 4 only by luck of the #969 reference, and future readers reading the PR body will be misinformed.

• important — documents-vs-fixes the divergence: RESOLVED. Commit 4 propagates authoredRootId to the host (inlineSubCompositions.ts:311-313) sourced from innerRoot.getAttribute("id") at line 190 — same value that drives the CSS rewrite at line 214, so the rewritten [data-hf-authored-id="X"] selector is guaranteed to resolve. The divergence-asserting test at inlineSubCompositions.test.ts:41-65 was kept as a behavioral contract test (innerHTML still strips #intro, scoper still rewrites to attribute selector) — that's correct, those are still true. New test at :134-153 asserts the host now carries data-hf-authored-id="intro". Tracking issue #969 exists and is referenced in the commit. Good.

• important — activateNestedChildTimelines naming + tl.play(): RESOLVED. Renamed to activateSiblingTimelines (init.ts:1735), comment at :1729-1732 cross-references player.ts:32 and player.ts:89. tl.play() replaces tl.paused(false) at :1740. Positional activateChildren is now an opts arg { activateChildren?: boolean } at :1747 — addresses the footgun nit too.

Status of prior nits

• nit — broad-unpause no-restore: RESOLVED. Comment at init.ts:1731-1734 explains the deliberate divergence from the seek-path "restore after" pattern (render-seek has no transport tick between frames, so residual unpaused state is idempotent). That's exactly the rationale I wanted documented.
• nit — positional activateChildren flag: RESOLVED via opts arg (see above).
• nit — [id="intro"] boundary test: RESOLVED. compositionScoping.test.ts:516-525 asserts [id="intro"] attribute selectors are NOT rewritten. Boundary pinned.

New observations on the producer fix (commit 4)

• nit — inlineSubCompositions.ts:311-313 guards on compId && authoredRootId — this is correct: the propagation only happens on the path that strips the wrapper (hostEl.innerHTML = innerRoot.innerHTML). The flattenInnerRoot branch (bundler) and the no-innerRoot fallback (line 320) don't need it. Tight scope, no over-reach.
• nit — collision risk: If the host element already has data-hf-authored-id (unlikely but possible if upstream tooling sets it), commit 4 will overwrite it without a guard. Probably fine since the producer path is the only path that runs this branch, but a one-line check (if (!hostEl.hasAttribute("data-hf-authored-id"))) would be cheap defense.
• nit — regression fixtures (packages/producer/tests/sub-comp-t0 + sub-comp-id-selector) are properly added to regression.yml shard-7 alongside sub-composition-video. Golden MP4 baselines are checked in (3 lines each via git LFS pointer). That's the right setup — these will catch a future regression on either the renderSeek path or the #ID scoping path.

CI status (head 6652fae2):

• 0 failures so far.
• regression-shards shard-1 through shard-8 are all PENDING — including shard-7 which now exercises sub-comp-t0 and sub-comp-id-selector (the new fixtures for this PR) alongside sub-composition-video. These are the highest-signal checks for this PR and must be green before merge.
• All other required checks pass: Build, Test, Typecheck, CLI smoke (required, pending), Smoke: global install (pending), Render on windows-latest (pending), Tests on windows-latest (pending), Preflight (lint+format), Preview parity, Perf (load/fps/scrub/drift/parity), CodeQL (actions/js-ts/python), Fallow audit, File size check, Semantic PR title.

Verdict: APPROVE (conditional on shard-7 regression passing and on the title/body update)

The technical work is done — the producer-side fix at inlineSubCompositions.ts:311-313 resolves the root cause that the prior review flagged as documented-but-not-fixed, naming is now consistent with player.ts, opts arg removes the positional-flag footgun, and the new fixtures + boundary test pin all the right invariants. The only remaining ask is housekeeping: the title and "Proper fix (follow-up)" body section are now stale and misleading. Update before merge.

Review by Vai (re-review)