fix(producer,core): honor relative data-start id-refs in render media scheduling

[valeriangalliat]

What

data-start="intro" (and intro + 0.5) already resolves in the live player. Render compile and render media scheduling treated it as a number, so relative starts failed in actual renders — chained sub-composition slots (data-start="hook") stacked at 0–2s and every scene after the first rendered black.

Closes #3361.

Why

compileTimingAttrs / injectDurations wrote data-end from parseFloat("intro"). Extract prefers data-end over duration, so the clip's end was unusable.

collectRenderMedia's resolveHostWindow did the same parseFloat on each composition host, so nested media inherited a 0 offset instead of its resolved window.

How

• Use parseNumeric in the timing compiler. If start isn't numeric, leave data-end off so extract can resolve the id-ref later.
• Resolve host data-start through the existing engine resolveReferencedStart in renderMediaCollector (the same helper video/audio/image extract already use), reading the inlined render document.

No new resolver. Regex compiler still has no DOM.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

---

• bun test src/compiler/timingCompiler.test.ts in packages/core
• bun test src/services/videoFrameExtractor.test.ts -t "still resolves relative data-start" in packages/engine
• bun test src/services/renderMediaCollector.test.ts in packages/producer
• bun test src/services/htmlCompiler.test.ts -t "offsets nested media by a host data-start id-ref" in packages/producer

[vanceingalls]

Adversarial R1 review — silent-corruption class (relative data-start id-refs zeroing chained sub-composition slots). CI green at 7e97ad9 (43 checks pass, 7 skipped, no reds); reviewDecision: REVIEW_REQUIRED; base MERGEABLE.

Fix boundary
The two parseFloat("intro") = NaN sites are:

• compileTimingAttrs / injectDurations / clampDurations writing data-end from a non-numeric start (would produce data-end="NaN").
• renderMediaCollector.resolveHostWindow folding host offsets from data-start.

Both now go through parseNumeric (skip-write on non-numeric) or resolveReferencedStart (same helper the video/audio/image parsers already use, so the four surfaces agree). Not a new resolver, not a new grammar — good.

Adversarial lenses

• Circular id-ref (A → B → A): resolveReferencedStart has a visiting: Set guard that returns 0 without caching the sentinel, and cleans up via try/finally. The visiting set is shared across top-level resolutions in one collectHostWindows pass but the finally-block guarantees emptiness between top-level calls — no cross-element leakage. Not covered by a test in this PR but the guard is pre-existing in referenceResolver.ts, not new here.
• Missing id-ref: silently 0, matching runtime startResolver semantics (snapshot and render agree — the whole point of the file's docstring).
• Deep chain (intro → hook → body): recursion + startCache handles it; cache holds resolved values so repeat lookups amortize.
• Cross-composition-host id-ref (body refs a hook that lives inside a different data-composition-src): the id-ref only reads the target's own data-start/duration, not its host ancestors' offsets. This mirrors runtime startResolver, which is document-scoped via getElementById. Same-scope sibling refs are the intended case and land correctly; cross-scope refs would ignore ancestor offsets, but that limitation already existed in the runtime — not a regression introduced here.
• Widen/narrow corruption window: fix is symmetric — previously parseFloat on "hook" returned NaN in the compiler (writing data-end="NaN") and 0 in the collector (silent zero). Now both no-op on non-numeric, deferring to the id-ref resolver. Same-parent numeric hosts land identical timings pre/post (resolveReferencedStart on data-start="0" = 0; unchanged).
• data-end null fallback: parseNumeric in renderMediaCollector.ts is a local re-implementation using Number.parseFloat (accepts "10s" as 10) vs the shared parseNumeric in @hyperframes/parsers using Number() (rejects "10s" as NaN). Pre-existing, only affects hostEnd (data-end), and both filter Infinity/NaN. Not a regression but worth converging on one someday.

Regression coverage
Four tests hit the invariant from four angles:

• timingCompiler.test.ts: compiler leaves data-end off for data-start="intro" and for injectDurations on id-ref start.
• videoFrameExtractor.test.ts: extractor still resolves relative data-start AFTER compileTimingAttrs — proves compiler doesn't stomp the id-ref.
• renderMediaCollector.test.ts (new): the exact chained-scene fixture (data-start="hook") — red clip stays [0,2], blue moves to [2,4].
• htmlCompiler.test.ts: end-to-end with real project dir, body-video schedules at [2,4] (would have been [0,2] on main).

Fails-on-main / passes-on-fix invariant is directly asserted in both the collector and the E2E compiler test.

Nits (non-blocking)

• Local parseNumeric in renderMediaCollector.ts diverges from the shared one on trailing-garbage tolerance. Consider replacing with the shared export in a follow-up.
• Consider a unit test for a cycle (A → B → A) to lock in the guard.

Approving.

— Via

[terencecho]

Concur with @via-bot APPROVE at 7e97ad9 on the host-composition id-ref fix — mechanism is sound, reused resolveReferencedStart is the right approach (all four surfaces agree with the browser runtime), CI fully green with 4-angle regression coverage (compiler unit, extractor E2E, collector unit, producer E2E) that directly asserts the fails-on-main invariant.

Six code-cited follow-ups — none blockers, worth naming so future reviewers don't hunt them again:

1. data-composition-id querySelector first-match ambiguity on reused sub-comps. packages/engine/src/services/referenceResolver.ts:35 — findReferenceTargetEl resolves via querySelector (first match). If a project inlines the same sub-composition twice (both nodes carry data-composition-id="hook" post-inlining), data-start="hook" references silently bind to the FIRST inlined instance regardless of proximity. Scenario: body uses data-start="hook" in scene B, but scene A already inlined a hook earlier; body's window anchors to A's hook, not B's — chained scenes with reused sub-comps could still mis-schedule. Not exercised by the new tests.

2. parseFloat→parseNumeric semantics change drops lax-numeric authoring. packages/core/src/compiler/timingCompiler.ts:155 — switches from parseFloat to parseNumeric (which is Number()-based, not parseFloat-based). parseFloat("5s") returned 5; parseNumeric("5s") returns null. A composition with lax numeric data-start ("5s", trailing whitespace/junk) now silently switches to the id-ref path — data-end is dropped, element becomes "unresolved" pending durations. Very low likelihood in production HTML, but not tested and worth flagging as a behavior change.

3. discoverMediaFromBrowser still uses parseFloat at leaf-media level. packages/producer/src/services/htmlCompiler.ts:2156 — for <video data-start="hook"> at the leaf level (rather than host-div id-ref), this yields start: NaN in the browser-discovered entry. Existing reconciliation is fortuitously safe via if (projectedEnd > 0) (NaN > 0 is false, so existing.end is preserved), BUT a new browser-inserted video with id-ref start (dynamic JS insertion) is pushed with start: NaN and never gets rescued. Edge case — reported bug was host-div-level, but the leaf-media path has a mirror gap.

4. Two parseNumeric variants in the same PR's surface. packages/producer/src/services/renderMediaCollector.ts:47 has a local parseNumeric using Number.parseFloat + Number.isFinite (accepts "10s"), while the core-side parseNumeric imported into timingCompiler.ts uses Number() (rejects). Same name, different acceptance. Consider unifying to a single parseNumeric exported from core, or renaming the local one.

5. Studio timeline UI diverges from render placement for id-ref clips. packages/studio/src/player/lib/timelineElementHelpers.ts:110 — Studio's timeline still parses data-start via Number.parseFloat without routing through resolveReferencedStart. Not a scheduling bug in the render pipeline (out of scope for this PR), but the UX now shows a clip at 0s in the timeline while it renders at 2s. Worth a follow-up.

6. Cycle-guard returns 0 silently, no diagnostic. resolveReferencedStart cycle-guard (referenceResolver.ts:53) returns 0 on visiting.has(el) — but doesn't emit a diagnostic. A ring cycle A→B→A silently produces A=0 and B=0+B.duration — bounded but soft. Missing test + missing log warn.

Confirmed no cross-language mirror concern (no Python producer parses data-start).

Missing tests worth adding:

• No test for data-start="hook + 0.5" / "hook - 0.5" offset forms, though the PR description explicitly calls these out.
• No test for CHAINED id-refs (body → hook → intro) proving transitivity end-to-end.
• No test for target with unknown duration (resolveReferencedDuration → null) — reference collapses to targetStart (not target's end), a silent semantic that authors may not expect.
• No test for reused sub-composition first-match ambiguity.

— Review by tai (pr-review)

[james-russo-rames-d-jusso]

PR state. HEAD 7e97ad9ff663e7841a1bce069859d93fada15422, reviewDecision: APPROVED (Via at head), tai posted COMMENT with 6 code-cited follow-ups. All CI green. Independent verify at head; layering on peer coverage, not repeating.

Blockers. None.

Concerns (orthogonal to peers)

• Name-collision hazard on parseNumeric. packages/producer/src/services/renderMediaCollector.ts:47-49 defines a LOCAL parseNumeric(value: string | null): number | null that uses Number.parseFloat(value) (accepts "10s", "5px", etc.). The SHARED parseNumeric imported from @hyperframes/parsers/composition-contract uses Number() (rejects those). Two functions with the same name and different semantics in the same PR — a future maintainer refactoring the local into import { parseNumeric } from "@hyperframes/parsers/composition-contract" will silently change data-end parse acceptance without realising it. tai flagged the semantics divergence but framed it as "two variants" — the deeper problem is the collision on the name. Rename the local to parseLenientNumeric (or similar) and add a // deliberately lax: pre-fix behavior for hostEnd only, see #3252 discussion comment, or fold the local into the shared module with an explicit strict: boolean flag.

• Leaf-media discoverMediaFromBrowser still uses parseFloat at packages/engine/src/services/videoFrameExtractor.ts:562-590. tai's finding verified — playbackRateAttr ? parseFloat(playbackRateAttr) : Number.NaN (line 590). The PR's stated invariant is "all four surfaces agree with the browser runtime"; this leaf surface still uses the pre-fix parser. Producer-side is fixed; engine leaf is not. Not this PR's regression (leaf was already parseFloat), but the "four surfaces agree" claim in the PR body is one surface short.

Nits
• Cycle-guard visiting: Set returns 0 silently on A→B→A — no diagnostic (Via nit). Fair. Adding a console.warn(\[hyperframes] chained data-start cycle detected: ${chain.join(" -> ")}`)` on first hit would surface author errors without changing behavior.

Questions
• data-composition-id querySelector first-match ambiguity across reused sub-comps (tai finding) — is this a real hazard in production compositions today, or theoretical? If reused sub-comps are rare, it's a follow-up; if common, it's a scoped concern.

Adversarial ledger
• Regression coverage covers the invariant from four angles (Via) — verified. Compiler unit skips NaN write; extractor E2E; new collector unit exercises chained red[0,2] / blue[2,4]; producer E2E confirms body-video at [2,4], would-be [0,2] on main. Deterministic invariant that fails on main.
• Cycle safety across top-level calls — visiting: Set cleaned via try/finally, no cross-call state leak. Verified.
• data-end="NaN" write elimination — compileTimingAttrs correctly skips the write on parseNumeric(startStr) === null. Verified in timingCompiler.ts diff.
• Cross-composition-host id-refs (Via note) — still ignore ancestor offsets, matching runtime startResolver's document-scoped getElementById. Design limit, not regression.

Peer-coverage layering.
Concur with Via APPROVE at 7e97ad9: mechanism sound; the two parseFloat("intro") = NaN corruption sites both correctly defer to shared parseNumeric / resolveReferencedStart; the fix matches the browser runtime the video/audio/image parsers use. Via's cycle-safety catch is fair; her parseFloat vs Number() nit is the seed of my name-collision concern above.

Concur with tai COMMENT 6 follow-ups: data-composition-id first-match ambiguity, parseFloat→parseNumeric semantics narrowing, leaf-media parseFloat still in place, two-variant parseNumeric, Studio-timeline UI divergence from render placement, cycle-guard silent-return. All 6 verify at head.

Orthogonal add: the parseNumeric NAME-collision framing above (vs tai's "two variants exist" framing). Same code sites, sharper risk statement.

Deploy-skew. None. Compiler + collector + resolver land together; leaf-media parseFloat is a pre-existing surface (not this PR's regression).

Tests. 4 new files across compiler/collector/extractor/producer — hits the invariant from all four rendering surfaces. New collector test asserts the fail-on-main scenario deterministically. Zero regression on 43 pass / 7 skip run.

Stamp stance. 🟢 layered concur. Fix is correct; the name-collision and leaf-parseFloat items are follow-up territory (worth a filed issue). Not clicking OG stamp unbidden — Via peer bot has APPROVED, human sign-off is a separate authorization.

— Review by Rames D Jusso