[jayhemnani9910]

What

A relative data-start (data-start="first", "first + 0.5") now looks for the referenced clip inside the referencing clip's own composition first, and only falls back to the document-wide lookup when nothing there matches.

Why

The docs say references resolve inside the same composition, and hyperframes timeline and the audio mix already do that. The runtime start resolver used document.getElementById, so once two compositions in the rendered document contain the same id, every reference to it resolves against the first one in the document.

The common way to hit this is mounting one block twice. Each instance keeps the block's own ids, so the second instance's relative clips are timed off the first instance and drop out of the render.

Repro on 0.8.142 (also on current main):

compositions/card.html

<template id="card-template">
  <div data-composition-id="card" data-width="320" data-height="180" data-duration="2" data-no-timeline>
    <div id="first" class="clip" data-start="0" data-duration="1" style="position:absolute;inset:0;background:#00f"></div>
    <div id="second" class="clip" data-start="first" data-duration="1" style="position:absolute;inset:0;background:#0f0"></div>
    <audio id="beep" src="tone.wav" data-start="first" data-duration="1" data-media-start="3"></audio>
  </div>
</template>

index.html root:

<div id="root" data-composition-id="main" data-start="0" data-duration="4" data-width="320" data-height="180" data-no-timeline>
  <div id="card-a" data-composition-id="card" data-composition-src="compositions/card.html" data-no-timeline data-start="0" data-duration="2" data-track-index="1"></div>
  <div id="card-b" data-composition-id="card" data-composition-src="compositions/card.html" data-no-timeline data-start="2" data-duration="2" data-track-index="2"></div>
</div>

(tone.wav is any audio file longer than 4 s.)

hyperframes lint is clean, and hyperframes timeline shows second at 1-2 s in card-a and 3-4 s in card-b. In hyperframes render --fps 10, sampling the centre pixel per frame:

• Expected: blue 0-1 s, green 1-2 s, blue 2-3 s, green 3-4 s.
• Actual: blue 0-1 s, green 1-2 s, blue 2-3 s, black 3-4 s.

The beep plays at 1-2 s and 3-4 s as expected, so in card-b the sound plays over a frame where its clip is missing. card-b's second resolves to 1 s (the end of card-a's first), a time when card-b is not on screen yet.

Related work

No existing issue or PR found. #4481 fixed the same repeated-id problem for Studio's timeline rows. #4334 is a different startResolver problem (the walk-up from a wrapped mount) and does not overlap.

How

findReferenceTarget takes the referencing element. It finds that element's composition (for a composition host, its parent composition) and returns the first element with the referenced id whose own composition is the same one. If none matches, the existing getElementById and data-composition-id lookups run unchanged, so a reference that only exists elsewhere in the document resolves as before.

Test plan

• Unit tests added/updated: startResolver.test.ts mounts one composition twice and checks the second instance's referenced clip starts at 3 s. It fails without the change (resolves to 1) and passes with it.
• Manual testing performed: the repro above renders blue, green, blue, green with the beep under both green spans.
• Package suites match main; oxlint, oxfmt, tsc, fallow and the comment checks pass.
• Documentation updated (if applicable)
• Comments follow CONTRIBUTING.md "Comments"

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[jrusso1020]

The runtime fix looks correct, and the test fails without it. Before this merges, the render-side resolver that claims to mirror this one should be scoped the same way.

What I checked:

• The docs claim holds. docs/reference/html-schema.mdx says "References stay within one composition", and the runtime was resolving document-wide.
• compositionScopeOf (startResolver.ts:49) treats a composition host as belonging to its parent composition. So data-start="card-a" on a sibling host still resolves in the parent scope, and the document-wide fallback (:61) keeps cross-composition references working as before.
• Ran vitest run src/runtime/startResolver.test.ts: 33/33 pass. Mutation check: with startResolver.ts reverted and the new test kept, it fails with expected 1 to be 3.

Please address in this PR:

• packages/engine/src/services/referenceResolver.ts:33 (findReferenceTargetEl) still does getElementById document-wide. Its header says it "Mirrors the browser runtime's startResolver so snapshot/render agree". The producer uses it to place video, image and audio in parseVideoElements/parseImageElements/parseAudioElements, through renderMediaCollector.ts. Your repro passes because the engine works in composition-local time and both mounts are the same block, so the first #first happens to have the same local timing. With two different compositions that share an id at different local times, the runtime now picks the in-scope element and the engine still picks the first one in the document. A <video data-start="intro"> would then get its visibility window from one clip and its extracted frames from another. Scoping the engine lookup the same way (nearest [data-composition-id], host counts as its parent's) keeps the two in step.
• packages/sdk/src/session.ts:384 says "refId remains a bare global id, matching runtime resolution". That is no longer true after this change, so update the comment, or the SDK's lookup if it should follow.

No other open PR targets this. #4334 is a different startResolver bug, as the description says.

— Rames

[jayhemnani9910]

Both points are in 13eacd9, plus 9ac9d2b.

Engine. findReferenceTargetEl now follows the runtime: it looks inside the referencing clip's composition first (a host counts as its parent's), then falls back to the document-wide getElementById / data-composition-id lookup as before. New test in videoFrameExtractor.test.ts: scene-a and scene-b each have an #intro (2 s and 3 s), and a video in scene-b with data-start="intro" now starts at 3. Before the change it got 2. RefResolverEl is exported, so 9ac9d2b makes the new parentElement / closest / querySelectorAll members optional. An adapter with only getAttribute still works and gets the old document-wide lookup (test in referenceResolver.test.ts).

I also rendered it: scene-a at 0-4 s with a red 2 s #intro, scene-b at 4-8 s with a blue 3 s #intro and a green video at data-start="intro", which hyperframes timeline puts at 7-8 s. Centre pixel per second, before: red, red, black, black, blue, blue, green, black. After: red, red, black, black, blue, blue, blue, green.

SDK. I fixed the comment and left the lookup. getElementTimings resolves refId through resolveScoped, which matches data-hf-id, not id, and prefers the top-level match for a bare id, the same rule dispatch and undo use (pinned by "resolves a colliding bare id to the TOP-LEVEL match"). So it never matched the runtime: for <h1 id="intro" data-start="1" data-duration="3"> + <p data-start="intro">, getElementTimings puts the <p> at 0 and the runtime puts it at 4. Making the SDK follow the runtime would change how it addresses elements, so I'd rather do that separately if you want it. session.timings.test.ts had the same "the runtime is global too" comment; updated that too.

Left alone: a reference to a clip in a different composition (the fallback path) still comes out in the referencing composition's local time on the render side. With a at 10 s holding a 2 s #cue and a video in b (at 20 s) with data-start="cue", collectRenderMedia gives 22 and the runtime gives 12. That was the same before this PR, so I'd follow up on it separately.

Engine, SDK, producer and CLI suites give the same results as the PR head before these commits, plus the two new tests. tsc, oxlint, oxfmt, fallow and both comment checks pass.

[jrusso1020]

Approving at 9ac9d2b. Both things I asked for in my earlier review are addressed.

1. The render-side resolver now has the same scope as the runtime. Resolved. packages/engine/src/services/referenceResolver.ts:37-59 adds compositionScopeOf and a scoped first pass in findReferenceTargetEl, and :92 passes the referencing element in. The logic matches startResolver.ts:49-66 line for line. A composition host counts as part of its parent composition, a candidate must have the same scope (so ids inside nested compositions are skipped), and the old getElementById / data-composition-id lookups are the fallback. The new RefResolverEl members are optional, so an adapter that only has getAttribute still works and falls back to the document-wide lookup. The new referenceResolver.test.ts pins that.
2. The SDK comment is no longer wrong. Resolved. packages/sdk/src/session.ts:384 now says the SDK resolves a bare id top-level first, unlike the runtime. The comment in session.timings.test.ts was updated to match.

What I ran at this head:

• startResolver.test.ts: 33/33 pass. referenceResolver.test.ts + videoFrameExtractor.test.ts: 201/201 pass. session.timings.test.ts: 23/23 pass.
• Mutation check: with referenceResolver.ts restored from the base, the new parseVideoElements case (two compositions that both have #intro, at different local times) fails with expected 2 to be 3. So the test pins the engine fix, not just the runtime one.

Non-blocking follow-up: getElementTimings in the SDK still prefers the top-level match for a colliding bare id (session.timings.test.ts:225), while the runtime and the engine now prefer the clip's own composition. For that case the SDK can report a start that differs from what renders. The comment now says so honestly, and I'm fine with it landing separately. If the SDK is meant to report render timing, it should follow in a later PR.

— Rames

[jrusso1020]

Following up on my approval at 9ac9d2b8, now that fork CI has run. Required "Tests on windows-latest" is red, and it failed the same way on a rerun:

packages/cli/src/utils/checkDeadline.process.test.ts › "managed launches own distinct profiles while the first browser is alive" (line 155) expects a script did not finish error from compositions/hang.html. It gets check_runtime_failure / "Navigation timeout of 10000 ms exceeded" on index.html instead.

This test came in with #5537 on 10-13, after this branch's merge base of 10-11. The same job passes on main and on other PRs run at the same time, so this doesn't look like a flake. My guess is an interaction between the startResolver change here and #5537's bounded checks for compositions that never finish, but I haven't verified that. Please rebase onto main and check whether that test passes locally. I'll recheck at the new head.

— Rames

[jayhemnani9910]

Rebased onto main (dc0e80e). New head is c926d2a, and all three commits are signed.

checkDeadline.process.test.ts passes locally at the new head (3 of 3, Linux), and so do the tests this PR touches in core, engine and sdk. I can't run Windows here.

I don't see how this change reaches that test. The runtime and engine changes only run for a data-start that names another clip, and the test project only has data-start="0". If Windows is still red at the new head, I'll dig into it.

[miguel-heygen]

Thanks for sticking with this one. The change itself reads correct to me, and the earlier two requests are properly resolved: the render side resolver is scoped the same way as the runtime now, and the SDK comment matches what the SDK actually does.

I tried to break the scoping and could not. A reference to the clip's own composition host still falls through to the document wide lookup, a sibling host inside the same scope matches because a host counts as part of its parent, an id inside a nested composition is correctly skipped, and a clip with no composition ancestor takes the old path untouched. I also walked every caller of the shared resolver, the lint project walk, the timeline describe, the render media collector, the audio mixer and both video and image parsing, and they all pass real elements, so they all move to the scoped behaviour together. Nothing is left silently on the old path, which was the thing I most wanted to check about making those interface members optional.

What is blocking is evidence rather than code. This head has no test runs at all, only the mergeability and WIP markers, and the same is true of the rebase before it. The last head that did get a full run was red on the required Windows jobs at the deadline process test, and that question has never been retested. The branch is also a long way behind main now and diverged. Could you rebase onto current main and push, so a full run happens at a head we can both read? If the Windows job is green there, this is done.

Two smaller things while you are in here, neither blocking. The scoped lookup scans every element with an id and compares in JS, and since the reference grammar is restricted the id is safe inside an attribute selector, so a single scoped query would do the same work with less loop. And the lint, timeline and audio mixer consumers all change behaviour through this resolver with no duplicate id case of their own, so a later refactor could drift one of them back without anything failing.

[jrusso1020]

Re-approving at 4640c1fa. My approval was at c926d2a1, so I diffed the PR's own patch across the two heads. The only real change is that main moved the shared resolver from packages/engine/src/services/referenceResolver.ts into packages/parsers, and the branch moved the fix with it. compositionScopeOf and the scoped first pass in findReferenceTargetEl are the same logic I approved, the engine file on main is now just a re-export, and the engine test moved to videoFrameExtractor.test.ts unchanged.

The bug is still on main. startResolver.ts and the parsers resolver both still resolve a relative data-start with a document-wide getElementById, so a block mounted twice times its second instance off the first. A trial merge against current main is clean.

On Miguel's changes request at this head: I agree with him on the code, and the code is why I'm approving. His block is about getting a full test run at a fresh head, since the Windows checkDeadline.process.test.ts failure I flagged earlier was never retested. That's a fair ask and my approval doesn't clear his request. A rebase onto current main and a run of the Windows job settles it. Miguel's single scoped querySelectorAll('[id="..."]') suggestion would also be a nice simplification while you're rebasing, but it isn't required.

Need: yes, repeated block mounts drop clips from the render · Size: ok · Reuse: runtime and the shared parsers resolver now share the same scoping rule; the SDK still differs, which its comment now says honestly · Simpler: a single scoped attribute query instead of scanning every [id], optional

— Rames

[jayhemnani9910]

Rebased onto main (acb47de) and pushed 4989af9, so the full run happens at this head.

Also took the single scoped query, [id="${refId}"] inside the scope, in both the runtime and the parsers resolver.

Locally on Linux: parsers 1328, core 5147, sdk 671, lint 1039 pass. Engine has the 6 reds that fail on main here too (ffprobe x4, WSL browserManager x2). The Windows job I can only see on CI.

The workflows at this head are waiting on approval (fork PR), so the run starts once someone approves them.

[jrusso1020]

Re-approving at 4989af9d. Against my approval at 4640c1fa, the only change is the single scoped [id="${refId}"] query in the runtime and the parsers resolver, which Miguel suggested. It's safe without CSS.escape because the reference grammar only allows [A-Za-z0-9_.:-]. I've approved the fork CI runs at this head. Miguel's request for a green Windows run at this head still stands.

[jayhemnani9910]

@miguel-heygen the Windows jobs are green at 4989af9 now that the fork runs are approved: Render on windows-latest, and Tests on windows-latest for core, engine-cli, studio-1 and studio-2. Could you take another look?

[miguel-heygen]

Thanks for the rebase and for chasing the Windows run. That was the only thing I was waiting on, and it is green at this head: the Windows render job and all four Windows test lanes pass, including the engine and CLI lane where the deadline process test failed before. The full run has no failures, and the branch still merges cleanly with main.

I compared this head with the one I reviewed before. The three earlier commits are the same patches, rebased, and the new commit swaps the scan of every element with an id for one scoped [id="..."] query in both the runtime and the parsers resolver. It keeps the same document order and the same composition check, so the same element wins. It is safe without escaping because parseStartExpression only lets through ids made of letters, digits, _, ., : and -, and none of those can break out of a quoted attribute value.

One follow-up from my earlier note, not needed for this PR: the lint, timeline and audio mixer paths change behaviour through this resolver but have no duplicate-id test of their own, so a later refactor could move one of them back without anything failing.

Verdict: APPROVE
Reasoning: The code was already right, the requested full run including Windows is now green at this head, and the only new change is the simpler scoped query, which behaves the same.