Skip to content

refactor(core): one pure owner for composition length and root size - #4990

Merged
miguel-heygen merged 4 commits into
mainfrom
fix/core-composition-length-resolvers
Oct 4, 2026
Merged

miguel-heygen merged 4 commits into
mainfrom
fix/core-composition-length-resolvers

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

What

Moves the rules for a composition's root, size and length out of private closures in packages/core/src/runtime/init.ts into one pure module, packages/core/src/runtime/compositionLength.ts, exported as @hyperframes/core/runtime/composition-length. The runtime now calls the module. A caller with no running player, such as a host page that publishes or previews compositions, can call the same functions on a parsed document instead of copying the rules.

Why

These decisions lived inside initSandboxRuntimeModular and none of them was exported:

  • the media window;
  • the authored floor;
  • the length taken from the clips;
  • the declared-length cut-off;
  • the one-frame minimum.

findRootCompositionElement read the global document. Anything outside the runtime that needed these answers had to copy the rules, and copies drift: the declared root length changed from a minimum to the length itself between 0.8.78 and main.

Related work

None.

How

  • compositionLength.ts exports:
    • MIN_VALID_TIMELINE_DURATION_SECONDS;
    • findRootCompositionElement(doc), now taking the document it reads (default document);
    • readCompositionSize(root);
    • resolveMediaWindowDurationSeconds(doc, { mediaStart, mediaDuration });
    • resolveAuthoredCompositionFloorSeconds(root, startResolver);
    • resolveContentDerivedDuration(root, startResolver, { unregisteredLottie });
    • resolveCompositionLengthSeconds({ declared, timeline, floors, fallback, derived }), the decision that used to be inline in getSafeTimelineDurationSeconds;
    • readStaticCompositionMeta(doc) for a composition nothing is running: no timelines, no adapters, and media without a known source length is pending.
  • init.ts keeps its caches, its timing-resolver scopes, the __hfResolveMediaStartSeconds hook and the durationSource publishing. It passes its resolvers into the module. The timeline, the floors and the adapter floor are still read only when no length is declared. The stage size posted to the host comes from readCompositionSize.
  • New subpath ./runtime/composition-length, declared in package-subpaths.json (the export maps are regenerated from it) and listed in tsconfig.json files, so dist/runtime/compositionLength.js is built and published.
  • The absolute media start stays where it was (the runtime's start resolver); the module takes it as a callback.

Test plan

  • Unit tests added/updated.
  • Pinning tests first. The first commit pins today's runtime results through initSandboxRuntimeModular (init.compositionLength.test.ts, 9 cases). They passed on main 3 runs in a row before any change, and still pass after it:
    • a declared root length cuts a longer timeline;
    • the timeline's length when nothing is declared;
    • a nested video's end at its absolute start;
    • a sub-composition's declared end;
    • the length taken from the clips;
    • 0 while a clip is pending;
    • a sub-frame timeline is ignored;
    • data-root over the first composition;
    • the root size reported to the host.
  • Module tests. compositionLength.test.ts covers the pure decision (including that a declared length reads nothing else) and readStaticCompositionMeta on both a linkedom and a DOMParser document. 24 tests across both files, 3 green runs in a row.
  • Neighbours. init.test.ts, init.timingResolver.test.ts and init.mediaClipIndex.test.ts: 220 passed.
  • One deliberate break per rule, each turning a test red:
    • declared length ignored (3 red);
    • media start dropped (3);
    • sub-composition floor dropped (1);
    • pending not holding at 0 (3);
    • one-frame minimum dropped (1);
    • data-root ignored (5).
  • Static checks.
    • core tsc --noEmit exit 0;
    • oxlint: 0 warnings and 0 errors on src/runtime;
    • oxfmt --check clean on the changed files;
    • check:package-subpaths clean;
    • after a full bun run build, verify:packed-manifests reports "Verified packages/core: packed manifest is publish-safe";
    • fallow audit --base origin/main with built dist removed: no issues.
  • Manual testing performed
  • Documentation updated (if applicable)
  • Comments follow CONTRIBUTING.md "Comments"

Not exercised: a real browser or a render. The runtime paths are covered through the jsdom init harness only. resolveMediaWindowDurationSeconds now queries timed media a second time per tick, after init.ts's cheap "any timed media?" check. That is one extra querySelectorAll, not measured.

@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 1556 (base branch 1556), smooth 1458 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 4, 2026 07:39

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 1df3c3a6. I checked this as a refactor, comparing each rule against init.ts on the merge base 3a0299e8.

Does it behave the same as main?

  • Media window. It uses the same video[data-start], audio[data-start] query, the same skip for non-finite starts and for durations of one frame or less, and the same one-frame cut-off on the result. init.ts still checks for no media before it opens the timing-resolver scope, so a page with no media still never pays for a resolver.
  • Authored floor. It applies the same "direct child of the root" filter, the same duration <= 0 skip, and takes the latest of the declared length and the sub-composition ends. The declared length is now read after the loop instead of before it. Both reads are pure, so the order doesn't matter.
  • Length from the clips. The pending rule is unchanged: media and composition hosts with no length count as pending. Splitting hasUnregisteredLottie into hasLottieLibrary() plus the module's own [data-lottie-src] query gives the same result. The pending result still has seconds: null, source unresolved and the same reason.
  • getSafeTimelineDurationSeconds. A declared length still returns before the timeline, the floors or any adapter runs, and it still clears durationSource. With nothing declared, the timeline, the floors and the adapter floor are all read every time and in the same order. The branch order is the same: timeline, then floor, then fallback, then derived. durationSource is still published only when the length was derived.
    • Main tested the timeline and the floor with isUsableTimelineDuration, which also needs Number.isFinite. The module's aboveOneFrame doesn't. That only matters for an infinite input. I checked all four inputs, and each is already finite at its source: getTimelineDurationSeconds returns null for non-finite values, the adapter floor keeps only finite values, and both floors are built from finite starts and lengths. NaN takes the same path on both sides and ends at 0. So this is the same behaviour.
  • Stage size. readCompositionSize is the same two parseCompositionDimension calls.

What I ran

  • core vitest, 5 files (compositionLength, init.compositionLength, init, init.timingResolver, init.mediaClipIndex): 245/245.

  • bun run build emits dist/runtime/compositionLength.{js,d.ts}. The ./runtime/composition-length entry is in both exports and publishConfig.exports.

  • I imported the built dist module in plain Node, with no window and no document global, and ran readStaticCompositionMeta on linkedom documents:

    Case Length
    Declared 4s over a 9s clip 4
    Clips only 7
    <video> with no length 0 (pending)
    Video with a length 4
    Sub-composition 8
    data-lottie-src 0

    data-root wins over the first composition, and a document with no composition returns null. Nothing in the import chain touches browser globals when the module loads.

  • Mutations, each run against the five files:

    • The timeline branch ignoring the floors fails 15 tests.
    • Dropping the Lottie pending clip fails 2 tests.
  • CI: the full run at this head (37185851510) passed with 47 success and 0 failed. Edit accuracy is 1556, equal to base. regression and preview-regression pass. A second CI run on the same SHA (37185940520) still has its 20 edit-accuracy shards queued.

Nits (non-blocking)

  • Two of the moved rules have no test, and none existed on main either. Each mutation below still passes all 245 tests:

    • deleting the "direct child of the root" filter in resolveAuthoredCompositionFloorSeconds (:66), so nested sub-compositions count toward the root's floor;
    • dropping the one-frame skip on a media duration in resolveMediaWindowDurationSeconds (:50).

    Both rules are now public API that other callers will rely on, so one test each would pin them.

  • readStaticCompositionMeta always returns fps: 30. That is createRuntimeState().canonicalFps, and the runtime only changes it from the export config, never from the composition. The value is right, but the doc comment ("Size, frame rate and length…") reads as if the frame rate comes from the composition. A sentence in the comment, or a named constant instead of building a whole runtime state to read one field, would make that clear.

— Rames

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified 1df3c3a6 independently. I found no behavior difference and no blocker. This is a comment, not a second approval: Rames already approved this head, which clears the gate, so I am not stacking a duplicate stamp.

What I checked (merge-base 3a0299e8 vs head, source side by side)

  • getSafeTimelineDurationSeconds: a declared length still returns first, before the timeline, floors or adapters run. With nothing declared the read order is unchanged (timeline, then media floor, authored floor, adapter floor, then fallback, then derived only in the last branch), and the branches are the same: timeline wins with max(timeline, floors, fallback), else floors, else fallback, else derived. durationSource is still published only for a derived length and cleared otherwise.
  • The only semantic difference is that the old isUsableTimelineDuration also required Number.isFinite; aboveOneFrame does not. It matters only for an infinite value, and none of the four inputs can be infinite: getTimelineDurationSeconds returns null for non-finite, the adapter floor requires finite, and the media and authored floors go through resolveCompositionDuration, which keeps only finite positive numbers. NaN fallbacks and the final > 0 ? x : 0 clamp are identical.
  • resolveMediaWindowDurationSeconds re-queries video[data-start], audio[data-start] on the document, the same selector the old inline loop used, and init.ts keeps its cheap mediaNodes.length === 0 early return. The cost is a second selector query inside the timing scope when media exists.
  • The authored-floor and content-derived loops are byte-for-byte equivalent apart from dropping isElementNode on querySelectorAll results (always elements). The Lottie rule is preserved: init.ts passes the library check and the module checks [data-lottie-src], which together equal the old hasUnregisteredLottie.
  • findRootCompositionElement(doc = document) keeps its behavior for the four existing no-argument callers (init.ts, clipTree.ts, timeline.ts). readCompositionSize is the same two parseCompositionDimension calls.
  • Exports: the new subpath is declared in package-subpaths.json, package.json and tsconfig.json files, so dist/runtime/compositionLength.js is built. The module only imports things the runtime already bundles.

Tests I ran (head copy in /tmp, nothing in the protected checkout)

  • compositionLength.test.ts and init.compositionLength.test.ts: 25/25. The 9 runtime pinning tests also pass 9/9 against the merge-base init.ts and compositionDimension.ts, so they pin pre-refactor behavior. The init-timingResolver and mediaClipIndex neighbours pass too (23 tests across those and compositionDimension/clipTree).
  • init.test.ts did not load in my environment (No such built-in module: node:), so I did not run it. CI's runtime test jobs passed at this head.
  • Mutations of the new module: dropping the floors from the timeline branch fails 3 tests. Three others survive in the four files I ran: removing the direct-child filter in the authored floor, removing the media one-frame skip, and removing the [data-lottie-src] check. All three rules were moved unchanged from init.ts, so this is a pre-existing coverage gap, not a regression. Worth pinning in a follow-up.

CI: 68 checks pass and none is red. The studio edit-accuracy shards were still pending when I looked; the sticky comment reports the gate passing at 1556 (equal to base). Nothing here is authorization to merge beyond what the gate already does.

— Review by tai (pr-review)

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 1df3c3a6 (full PR). This is a comment, not an approval. I agree with tai's and Rames's finding that behaviour is preserved; here is what I add.

Differential test, run through the real initSandboxRuntimeModular on base 3a0299e8 and on head. I tried 3,250 inputs:

  • 18 declared data-duration values: absent, NaN, Infinity, -Infinity, -3, 0, "", "abc", 0x5, 5s, 1e1, 0.01, 0.016, and others;
  • 11 timeline values: none, NaN, Inf, -2, 1/120, 1/60, throws, and others;
  • 16 child shapes: clips, negative starts, sub-compositions with bad or negative lengths, nested video, pending, infinite or tiny media, and Lottie;
  • 10 root sizes, plus late timeline registration.

I compared getDuration(), __hf.durationSource and the posted stage size. There were 0 diffs. No real composition changes length. The new Number.isFinite(declared) check cannot be reached from init, because parseStrictFiniteTimingNumber already turns "NaN" and "Infinity" into null on both refs.

NaN compared with Infinity, on the exported pure API. I ran resolveCompositionLengthSeconds against main's inline decision on 11,664 inputs, with each slot in {null, NaN, ±Inf, -1, 0, 0.01, 1/60, 3}.

  • NaN on its own: 0 diffs in any slot. NaN was never a hole, because NaN > 0 is false on both sides.
  • declared = Infinity: 893 diffs. This is the intended fix in 1df3c3a.
  • Nit (non-blocking), 1,040 diffs. A timeline() or floors() value of Infinity now returns Infinity, where main returned the fallback or the derived length. Main's isUsableTimelineDuration checked Number.isFinite, but aboveOneFrame (compositionLength.ts:21-22, used at :125 and :133) does not.
    • init cannot reach this, so I agree with Rames there: getTimelineDurationSeconds and the floor producers filter out non-finite values.
    • The subpath is public, though, so a host caller could reach it. Adding Number.isFinite to aboveOneFrame would give the module the same finite guarantee the last commit gave declared.

Subpath consumers: nothing imports @hyperframes/core/runtime/composition-length yet. The only importer is init.ts, and Studio, player and site are unchanged. That matches the body's "can call" wording, so treat it as future use, not a consolidation already done.

CI: the 11 required checks pass. Overall there are 70 passing, 18 pending (Studio edit-accuracy shards) and 6 skipped.

What I ran: the two jsdom harnesses above on base and head, and the PR's 2 test files at head (25/25). I did no mutation pass, because tai and Rames already covered it.

— Somu

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 4, 2026
Merged via the queue into main with commit 34ff5ca Oct 4, 2026
169 checks passed
@miguel-heygen
miguel-heygen deleted the fix/core-composition-length-resolvers branch October 4, 2026 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants