fix(render): consolidate duration and timing correctness#2405
Conversation
`compositions --json` computed each timed child's start with a bare
parseFloat(data-start ?? "0") in parseCompositions (host duration) and
parseSubComposition (sub-comp duration). A relative reference like
data-start="s1" ("start when clip s1 ends") is not numeric, so parseFloat
returned NaN and that clip's contribution to the max-end was silently
dropped — a host with two 3s clips (2nd data-start="s1") reported duration 3
instead of 6, breaking compositions/inspect/snapshot for composition-clip
relative timing.
Resolve relative references the same way the extractor does (parseStartExpression
from @hyperframes/core + a findReferenceTargetEl/resolveReferencedStart port,
since the engine's referenceResolver isn't a public export across the package
boundary). Verified: host duration now 6; 3 tests pass.
(Implemented via Codex; verified independently.)
vanceingalls
left a comment
There was a problem hiding this comment.
Runtime-interop + cross-layer contract lens (Rames covers test + observability — do not duplicate). Adversarial-user pass completed on head ac02bd38324406142c40e0b36f15bfe6d435a2c0 — one candidate finding refuted by the actual DOM library behavior; see "Cleared" below.
Strengths
- Line counts add up exactly across the three absorbed source PRs (2077 →
compositions.ts+72/-3 + test +41/-1; 2239 →probeStage.ts+11/-1 + test +24; 2338 →probeStage.ts+1 + test +18/-1). Consolidation is a clean superset. probeStage.ts:212threadsvariables: job.config.variablesintocreateCaptureSessionat the same shape used by the frame-capture path inrenderOrchestrator.ts:2016and the distributedrenderChunk.tspath. Engine consumes it atframeCapture.ts:920viaevaluateOnNewDocumentbefore any page script runs — probe and capture now see the same initial__hfVariablesstate.FRAME_BOUNDARY_EPSILON = 1e-3atprobeStage.ts:86is safely above six-decimal serialization error (~5e-7 s at 30fps → ~1.5e-5 frame) and well below a visible frame interval at any realistic fps.- CLI's
resolveStart(compositions.ts:53-94) reuses the sharedparseStartExpressiongrammar from@hyperframes/core, so the CLI and runtime resolver agree on what a relative ref means at the parse level — divergence is limited to composition-relative (CLI) vs absolute-timeline (runtime) resolution, which is intentional for the two use cases.
Cross-slice interactions checked (all clear)
- Variable-aware probe vs rounded frame-boundary duration. Both operate on
composition.durationafter the browser probe returns it. Exact-boundary durations produced by variables (e.g.short: true→ 2.0s) pass throughdurationToFrameCount(2.0, 24) = 48cleanly (|48.0-48| ≤ 1e-3). - Relative
data-startvs variable-aware probing.compositions.tsruns in the CLI on static markup and never sees the variable-resolved DOM; the producer's probe uses the engine's runtime resolver, not the CLI helper. Independent code paths, no coupling introduced.
Nits
compositions.ts:113—parseCompositionsnewly exported so the new test can call it. Consider a/** @internal */JSDoc so external CLI consumers don't rely on it accidentally.compositions.ts:215— insideparseSubComposition,resolveStart(doc, el, ...)is called with the outerdoc, notsearchDocument. When the sub-comp uses a<template>wrapper, ref targets live insidetemplate.content(aDocumentFragment, inert per HTML spec). This currently works only becauselinkedom.DOMParser(utils/dom.ts) traverses into template content ongetElementById— non-standard behavior verified againstlinkedom@0.18.12. At runtime the point is moot becausepackages/core/src/compiler/inlineSubCompositions.tsflattens templates before render. Consider threadingsearchDocument(or the ref-lookup root) through tofindReferenceTargetElso the CLI is robust if the DOMParser is ever swapped for a spec-compliant one.compositions.ts:50—findReferenceTargetElomitsCSS.escape(refId)while the runtime resolver (startResolver.ts:42) uses it. Currently safe becauseparseStartExpression's regex restricts refIds to[A-Za-z0-9_.:-]+(all CSS-safe), but the two layers should probably match.
Notes (already acknowledged in the PR body)
- The mirror-not-import duplication between
resolveStart/resolveReferencedDurationincompositions.tsand the runtime resolver incore/runtime/startResolver.tsremains a Fallow warn-level and an architectural smell — parity between the two now depends on manual sync. Not blocking this PR; worth a follow-up ticket to export a sharedcreate<Layer>StartTimeResolver-style helper from@hyperframes/core.
Verdict: APPROVE
Reasoning: All three absorbed contracts are preserved shape-for-shape, cross-slice interactions are semantically consistent, engine actually consumes variables at the point the probe now supplies them, and no substantive runtime-interop drift under the actual (linkedom + inline-runtime) execution environment. CI green, mergeable_state = blocked is reviewer-gate only.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at ac02bd38324406142c40e0b36f15bfe6d435a2c0 — consolidates #2338 / #2239 / #2077. Small (4 files, +167/-6).
Three orthogonal duration-correctness wins
- Composition variables threaded into duration discovery —
runProbeStageatprobeStage.ts:212now includesvariables: job.config.variablesin the capture-session config. Compositions whosedata-durationdepends on a variable (e.g.short: trueshortens scenes) probe under the same variable set the final render will use. - Decimal-frame-boundary tolerance —
durationToFrameCount(duration, fps)atprobeStage.ts:88-93rounds to the nearest integer when the raw frame count is within1e-3of it (32.866667 × 30 = 986.00001 → 986); otherwise ceilings normally (32.867 × 30 = 986.01 → 987). Fixes the "6-decimal duration written back from an exact frame boundary adds a phantom frame" bug from #2239. - Relative
data-startresolution in duration reporting —parseCompositionsatcompositions.ts:58-116now callsresolveStart(doc, el, ...), walkingdata-start="s1"-style references through@hyperframes/core'sparseStartExpression. Uses sharedstartCache(memoization) +visitingset (cycle guard). Mirrors #2041's runtime resolver atstartResolver.ts:120+.
Verification
- CLI test covers the sub-composition reference chain end-to-end —
compositions.test.ts:12-40renders a host withs1.data-start="0" data-duration="3"ands2.data-start="s1" data-duration="3", assertshost.duration === 6. Load-bearing for the "relative starts affect reported duration" invariant. durationToFrameCountcovers both directions —probeStage.test.ts:294-318asserts 32.866667 → 986 (nearest, within ε) AND 32.867 → 987 (ceiling, outside ε). Boundary case covered.variablesthreading test —probeStage.test.ts:284-292captures theoptspassed tocreateCaptureSessionvia the newcapturedOptions[]array and asserts the variable map arrives intact.- Cache/cycle discipline in
resolveStart—visiting.addbefore recursion,visiting.deletein afinally,startCache.seton every terminating branch. Return-0 short-circuit on cycle detection is a safe silent-fail (composition would then behave as if the ref weren't there). resolveReferencedDurationfallback ordering — explicitdata-durationwins; thendata-endminus resolved start; thennull. If a referenced element only hasdata-composition-srcwith no explicitdata-duration,resolved = targetStart + offset(position without duration accumulation) — matches how the runtime resolver handles the same shape.
Adversarial pass
findReferenceTargetEluses raw interpolation ([data-composition-id="${refId}"]) vs.CSS.escape(refId)in the runtime resolver atstartResolver.ts:80.parseStartExpression's regex constrains refId to[A-Za-z0-9_.:-]+— none of those need escaping inside a double-quoted attribute selector. Functionally equivalent; consistency nit not worth surfacing.FRAME_BOUNDARY_EPSILON = 1e-3— 0.001 frames × (1/30) = 33 µs at 30fps, 17 µs at 60fps. Well below any authored precision. Absolute rather than relative epsilon is right here — the target case is round-tripped decimal rendering (X/30 → 6-digit string → parseFloat → drift bounded by IEEE 754 at ~1e-14, then amplified by ×fps).resolveStartrecursion has no depth cap — max depth = chain length ofdata-startrefs. For pathological content-authored chains, could stack-overflow. Not a real concern for CLI parsing local files.- Cache scope in
parseSubComposition— separatestartCache/visitingper call vs. shared withparseCompositions. Correct — different DOM instances, no reuse benefit.
Verdict framing
Small, well-scoped, all three named regressions covered by executable tests. Already stamped and green. LGTM from my side.
Summary
Verification