refactor(core): one pure owner for composition length and root size - #4990
Conversation
Edit accuracy: accurate 1556 (base branch 1556), smooth 1458 of thoseThe gate passes. Quarantined, measured but not gated (0) |
jrusso1020
left a comment
There was a problem hiding this comment.
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.tsstill 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 <= 0skip, 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
hasUnregisteredLottieintohasLottieLibrary()plus the module's own[data-lottie-src]query gives the same result. The pending result still hasseconds: null, sourceunresolvedand the same reason. getSafeTimelineDurationSeconds. A declared length still returns before the timeline, the floors or any adapter runs, and it still clearsdurationSource. 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.durationSourceis still published only when the length was derived.- Main tested the timeline and the floor with
isUsableTimelineDuration, which also needsNumber.isFinite. The module'saboveOneFramedoesn't. That only matters for an infinite input. I checked all four inputs, and each is already finite at its source:getTimelineDurationSecondsreturns null for non-finite values, the adapter floor keeps only finite values, and both floors are built from finite starts and lengths.NaNtakes the same path on both sides and ends at 0. So this is the same behaviour.
- Main tested the timeline and the floor with
- Stage size.
readCompositionSizeis the same twoparseCompositionDimensioncalls.
What I ran
-
core vitest, 5 files (
compositionLength,init.compositionLength,init,init.timingResolver,init.mediaClipIndex): 245/245. -
bun run buildemitsdist/runtime/compositionLength.{js,d.ts}. The./runtime/composition-lengthentry is in bothexportsandpublishConfig.exports. -
I imported the built
distmodule in plain Node, with nowindowand nodocumentglobal, and ranreadStaticCompositionMetaon linkedom documents:Case Length Declared 4s over a 9s clip 4 Clips only 7 <video>with no length0 (pending) Video with a length 4 Sub-composition 8 data-lottie-src0 data-rootwins over the first composition, and a document with no composition returnsnull. 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.regressionandpreview-regressionpass. 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.
- deleting the "direct child of the root" filter in
-
readStaticCompositionMetaalways returnsfps: 30. That iscreateRuntimeState().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
left a comment
There was a problem hiding this comment.
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 withmax(timeline, floors, fallback), else floors, else fallback, else derived.durationSourceis still published only for a derived length and cleared otherwise.- The only semantic difference is that the old
isUsableTimelineDurationalso requiredNumber.isFinite;aboveOneFramedoes not. It matters only for an infinite value, and none of the four inputs can be infinite:getTimelineDurationSecondsreturns null for non-finite, the adapter floor requires finite, and the media and authored floors go throughresolveCompositionDuration, which keeps only finite positive numbers. NaN fallbacks and the final> 0 ? x : 0clamp are identical. resolveMediaWindowDurationSecondsre-queriesvideo[data-start], audio[data-start]on the document, the same selector the old inline loop used, andinit.tskeeps its cheapmediaNodes.length === 0early 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
isElementNodeonquerySelectorAllresults (always elements). The Lottie rule is preserved:init.tspasses the library check and the module checks[data-lottie-src], which together equal the oldhasUnregisteredLottie. findRootCompositionElement(doc = document)keeps its behavior for the four existing no-argument callers (init.ts,clipTree.ts,timeline.ts).readCompositionSizeis the same twoparseCompositionDimensioncalls.- Exports: the new subpath is declared in
package-subpaths.json,package.jsonandtsconfig.jsonfiles, sodist/runtime/compositionLength.jsis built. The module only imports things the runtime already bundles.
Tests I ran (head copy in /tmp, nothing in the protected checkout)
compositionLength.test.tsandinit.compositionLength.test.ts: 25/25. The 9 runtime pinning tests also pass 9/9 against the merge-baseinit.tsandcompositionDimension.ts, so they pin pre-refactor behavior. The init-timingResolver and mediaClipIndex neighbours pass too (23 tests across those and compositionDimension/clipTree).init.test.tsdid 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 frominit.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
left a comment
There was a problem hiding this comment.
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-durationvalues: 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 > 0is false on both sides. declared = Infinity: 893 diffs. This is the intended fix in 1df3c3a.- Nit (non-blocking), 1,040 diffs. A
timeline()orfloors()value ofInfinitynow returnsInfinity, where main returned the fallback or the derived length. Main'sisUsableTimelineDurationcheckedNumber.isFinite, butaboveOneFrame(compositionLength.ts:21-22, used at:125and:133) does not.initcannot reach this, so I agree with Rames there:getTimelineDurationSecondsand the floor producers filter out non-finite values.- The subpath is public, though, so a host caller could reach it. Adding
Number.isFinitetoaboveOneFramewould give the module the same finite guarantee the last commit gavedeclared.
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
What
Moves the rules for a composition's root, size and length out of private closures in
packages/core/src/runtime/init.tsinto 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
initSandboxRuntimeModularand none of them was exported:findRootCompositionElementread the globaldocument. 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.tsexports:MIN_VALID_TIMELINE_DURATION_SECONDS;findRootCompositionElement(doc), now taking the document it reads (defaultdocument);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 ingetSafeTimelineDurationSeconds;readStaticCompositionMeta(doc)for a composition nothing is running: no timelines, no adapters, and media without a known source length is pending.init.tskeeps its caches, its timing-resolver scopes, the__hfResolveMediaStartSecondshook and thedurationSourcepublishing. 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 fromreadCompositionSize../runtime/composition-length, declared inpackage-subpaths.json(the export maps are regenerated from it) and listed intsconfig.jsonfiles, sodist/runtime/compositionLength.jsis built and published.Test plan
initSandboxRuntimeModular(init.compositionLength.test.ts, 9 cases). They passed on main 3 runs in a row before any change, and still pass after it:data-rootover the first composition;compositionLength.test.tscovers the pure decision (including that a declared length reads nothing else) andreadStaticCompositionMetaon both a linkedom and a DOMParser document. 24 tests across both files, 3 green runs in a row.init.test.ts,init.timingResolver.test.tsandinit.mediaClipIndex.test.ts: 220 passed.data-rootignored (5).tsc --noEmitexit 0;src/runtime;oxfmt --checkclean on the changed files;check:package-subpathsclean;bun run build,verify:packed-manifestsreports "Verified packages/core: packed manifest is publish-safe";fallow audit --base origin/mainwith builtdistremoved: no issues.Not exercised: a real browser or a render. The runtime paths are covered through the jsdom init harness only.
resolveMediaWindowDurationSecondsnow queries timed media a second time per tick, afterinit.ts's cheap "any timed media?" check. That is one extraquerySelectorAll, not measured.