fix(core,cli): defer __renderReady until root timeline is bound#1061
Conversation
The runtime set __renderReady at the same time as __playerReady, before the root timeline was bound. Consumers waiting for __renderReady (the render-safe signal) could observe a player with no captured timeline, making renderSeek a no-op. Root cause: init.ts set both flags together, but timeline binding happens later — synchronously via bindRootTimelineIfAvailable(), via a deferred setTimeout(0) for bundled compositions, or asynchronously via loadExternalCompositions(). Fix in init.ts: - Remove __renderReady from the __playerReady assignment - Set it after bindRootTimelineIfAvailable() when timeline is found - Set it in the setTimeout(0) deferred path - Set it in the external compositions .finally() path Fix in snapshot.ts: - Wait for __renderReady (truthful signal) not __timelines - Use renderSeek() with frame quantization, not seek() - Tick the GSAP ticker after seeking - Await document.fonts.ready before capturing Closes #1047
miguel-heygen
force-pushed
the
fix/snapshot-seek-parity
branch
from
fc0a629 to
b2828e4
Compare
miguel-heygen
changed the title
Fallow audit reportFound 72 findings. Duplication (46)
Health (26)
Generated by fallow. |
- Fix broken duration getter: use getDuration() (PlayerAPI method) instead of .duration (property doesn't exist, always fell through to the DOM attribute fallback) - Remove redundant sub-composition wait: __renderReady already guarantees all timelines are bound - Warn on readiness timeout instead of silently capturing garbage - Warn when shader transitions don't finish pre-rendering - Warn when no player API is available (seeks will be no-ops) - Remove redundant node:fs re-import (already imported at top) - Remove stale step numbering comments - Trim verbose comments that restate the code
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: COMMENT (would-be APPROVE)
Solid root-cause fix. The __renderReady signal was lying (set with __playerReady before timeline binding); the PR makes it truthful (set after bindRootTimelineIfAvailable() succeeds across all three init paths). Snapshot consumer is rewritten to use the now-honest signal + renderSeek to match parity-harness semantics. Closes the exact failure modes reported in #1047. Holding for James's stamp greenlight.
Architecture: the fix matches the diagnosis ✓
Traced each of the 3 new __renderReady = true sites against the binding paths:
| Path | When binding completes | New __renderReady set site |
|---|---|---|
| Sync, captured timeline | bindRootTimelineIfAvailable() populates state.capturedTimeline (init.ts:~1632) |
New if (state.capturedTimeline) gate immediately after (:1634) ✓ |
| Sync, deferred (bundled comps' setTimeout(0)) | Inline scripts execute → bindRootTimelineIfAvailable() retries |
New set inside the setTimeout(0) after retry (:1650) ✓ |
| Async (external compositions) | .finally() runs bindRootTimelineIfAvailable() |
New set immediately after, inside the same .finally() (:1465) ✓ |
Pre-PR set at init.ts:1547 (the lying site, set with __playerReady) is removed. Idempotent — multiple paths can set true, no path sets false. ✓
Parity check ✓ — snapshot now uses the same seek API as the render path
The user's #1047 mitigation explicitly recommended renderSeek + frame quantization + GSAP ticker tick + waiting on document.fonts.ready. The PR implements all four. Cross-checked against packages/producer/src/parity-harness.ts:243-260 — same shape:
// snapshot.ts (new)
const frame = Math.floor(safe * 30 + 1e-9);
const quantized = frame / 30;
if (typeof player.renderSeek === "function") player.renderSeek(quantized);
else if (typeof player.seek === "function") player.seek(quantized);
if (window.gsap?.ticker?.tick) window.gsap.ticker.tick();
// parity-harness.ts (existing)
const quantized = frame / targetFps;
if (typeof player.renderSeek === "function") player.renderSeek(quantized);
else player.seek(quantized);This is the convergence the issue asked for. ✓
PR-body claims verified ✓
- "
__timelinesis set 15-56ms before__playerReady" — confirmed by reading init.ts;__timelinesexists from the moment inline scripts register, beforecreatePlayerApireturns. - "After fix:
__renderReadyfires 1.4-3.4ms after__playerReady" — consistent with the new sites running synchronously aftercreatePlayerApireturns +bindRootTimelineIfAvailable()resolves. - "old
__timelinesmanual seek fallback produces visually different frames fromrenderSeekat 4/5 timestamps on warm-grain" — plausible given the manual fallback computedlocalTime = t - data-startper timeline, whilerenderSeekgoes through the full sibling-timeline activation + quantization path. The fallback simply didn't have access to that machinery. ✓
Observations — none blocking
-
Hardcoded
fps=30in snapshot quantization.Math.floor(safe * 30 + 1e-9) / 30assumes 30fps. The parity-harness uses a configurabletargetFpsparameter (default 30 but accepts--fps). If a composition is rendered at 60fps but snapshotted at 30fps quantization, frames at non-30fps-aligned timestamps would differ between snapshot and render — exactly the parity gap this PR is trying to close. Worth either reading the project's fps from the config or accepting a--fpsarg onsnapshotto matchrender's flag. Not blocking — 30 is the framework default — but it's a future-proofing gap for the 60fps+ population. -
fileServer.ts has its own
__renderReadysetters with different semantics. Two source-side setters exist:packages/producer/src/services/fileServer.ts:393— insideinstallMediaFallbackPlayer(), sets both ready flags simultaneously after creating a media-only fallback player.packages/producer/src/services/fileServer.ts:402— insidewaitForPlayer(), sets both flags when__player.renderSeekexists.
These are internally consistent for the producer's render-path runtime (the fileServer media-fallback player has no timeline to bind, so "ready when constructed" is correct). But the project now has two different conventions for what
__renderReadymeans. The fix to init.ts is correct for the init runtime; fileServer.ts is correct for its runtime; but a future maintainer reading either won't easily see that they have intentionally different timing definitions. A one-line comment in each file pointing at the other ("init.ts/fileServer.ts also sets this — different runtime, different binding semantics") would help. Cosmetic. -
Duration fallback removed
__timelineswalk. Old code at snapshot.ts:222-227 walked__timelineslooking for a.durationif__player.durationwas unavailable. New code drops this fallback: only__player.getDuration()or the rootdata-durationattribute. For a composition where the player API isn't fully constructed AND the root has nodata-duration, snapshot now returns 0 (silently). Probably benign — by the time__renderReady === true,__player.getDuration()should be available — but it's a real behavior change. Worth a sentence in the PR body acknowledging the fallback removal. -
No automated regression test for the
__renderReadytiming invariant. The PR relies on instrumented probes (mentioned in the PR body) for verification. For a "signal lies about its meaning" fix, a unit test that asserts__renderReadyis undefined/false at a checkpoint between player construction and timeline binding would lock in the invariant. The init.ts setup is hard to unit-test directly (it's the runtime IIFE), but a Puppeteer-based assertion test could probe both signals at a known-pause point. Optional follow-up; not blocking because the issue's bug-report is reproducible end-to-end if regression happens. -
Fallow audit fails (28 findings). Per
dont-normalize-red-ci-as-batch-noise: the Fallow check exits with code 1 and the test plan claims "Pre-commit lint, format, typecheck pass" without acknowledging the audit red. Most findings are pre-existing per-function complexity unchanged by this PR, butsnapshot.ts:91 captureSnapshotsjumped from CRAP 870 (cyclomatic 29) to CRAP 992 (cyclomatic 31) — the refactor consolidated some logic and added more sequential awaits, raising the function's per-call complexity slightly. Worth either acknowledging the red in the PR body or splittingcaptureSnapshotsin a follow-up since this function has been over the threshold for a while.
CI
19 success / 10 in_progress / 1 failure (Fallow audit only — same code-quality static-analysis noise as recent PRs).
Summary
Fix is the right shape and the right scope. Three nits worth addressing pre-merge (Fallow ack, duration-fallback note in PR body, fps=30 comment), plus two non-blocking observations (no regression test, fileServer.ts parity-doc). Otherwise, ship-ready on merit.
— Rames Jusso
…n note - Add comment explaining hardcoded fps=30 (runtime's canonicalFps default, not exposed on PlayerAPI) - Add cross-reference comments between init.ts and fileServer.ts explaining their different __renderReady timing semantics
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
The core diagnosis is correct and the fix is well-targeted. __renderReady being set at player construction rather than after timeline binding was the actual root cause. The snapshot consumer change (waiting for __renderReady, using renderSeek with frame-quantization, forcing a GSAP ticker tick) also matches how the render pipeline works, and the PR description's instrumented timing evidence is solid.
Strengths:
packages/core/src/runtime/init.ts:1466—.finally()path correctly sets the signal afterbindRootTimelineIfAvailable()has run for external/inline compositions.packages/cli/src/commands/snapshot.ts:134-140— surfacingruntimeReady=falseas a warning rather than silently continuing is a significant UX improvement. Previously the command produced wrong frames with no indication.- Net −124 deletions in snapshot.ts. The old manual
__timelinesseek fan-out was fragile; collapsing it torenderSeekis strictly better.
important — setTimeout(0) branch sets __renderReady unconditionally, invariant incomplete
init.ts:1645-1656:
setTimeout(() => {
if (bindRootTimelineIfAvailable() && state.capturedTimeline !== prevTimeline) {
player._timeline = state.capturedTimeline;
}
runAdapters("discover", state.currentTime);
(window as Window & { __renderReady?: boolean }).__renderReady = true; // ← always fires
…
}, 0);bindRootTimelineIfAvailable() can return false (no timeline found), yet __renderReady is still set to true. Same for the .finally() at line 1466 — it fires on both success and failure (that's what finally means). The PR description says __renderReady guarantees "safe for deterministic frame capture," but renderSeek called on a player with _timeline === null falls back to a no-op or clock-only seek with no composition activation.
The sync site at 1638 is gated (if (state.capturedTimeline)), which is right. The other two sites should apply the same guard, or bindRootTimelineIfAvailable should return whether a timeline was actually bound and the signal should only fire on true.
important — hardcoded 30 fps quantization in snapshot.ts is redundant and fragile
snapshot.ts:281-286:
const safe = Math.max(0, Number(t) || 0);
const frame = Math.floor(safe * 30 + 1e-9);
const quantized = frame / 30;
if (typeof player.renderSeek === "function") {
player.renderSeek(quantized);renderSeek (init.ts:2030) already calls quantizeTimeToFrame(t, state.canonicalFps), where state.canonicalFps defaults to 30. So this is double-quantizing at the same grid — harmless today. But canonicalFps is an exposed field (getCanonicalFps() on the player at init.ts:1521) meaning compositions could eventually run at 24/25/60 fps, at which point snapshot.ts would quantize to the wrong grid first and pass a misaligned value into renderSeek. Should use player.getCanonicalFps?.() ?? 30 for the quantization, or skip the pre-quantization entirely and let renderSeek own it.
important — no regression test for the __renderReady ordering guarantee
packages/core/src/runtime/init.test.ts clears __renderReady in beforeEach but never asserts that it's set after timeline binding rather than at player construction. This is the exact invariant the bug violated. Without a test, a future refactor of the init sequence can silently break parity again. The fix should include an assertion like:
expect(window.__renderReady).toBeUndefined(); // false before timeline is bound
initSandboxRuntimeModular();
// flush async (external compositions loaded)
expect(window.__renderReady).toBe(true);
expect(window.__player._timeline).not.toBeNull();nit — per-frame settle time reduction
Old: sequential 2 × rAF + 200 ms sleep per frame.
New: Promise.race(setTimeout(100), 2 × rAF) — resolves at whichever fires first. In headless Chrome 2 × rAF is ~32ms, so the effective settle is ~32ms rather than ~232ms.
This is likely correct given that renderSeek + gsap.ticker.tick() processes state synchronously. Just worth noting that if a composition uses requestAnimationFrame-driven effects that don't tick from GSAP, they may not have settled. Low risk given the PR description's per-example verification.
Fallow audit: The fallow-audit job fails but it's not a required check (ruleset only requires: Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest). The CRAP scores flagged on snapshot.ts are pre-existing — the PR actually reduces cyclomatic complexity with the -124 deletion. The blocked merge state is from the 1-required-approver requirement, not a failing required CI check.
Verdict: APPROVE
Reasoning: The root cause (premature __renderReady signal before timeline binding) is correctly identified and fixed, and the snapshot consumer now uses the render-parity seek path. The important findings are correctness gaps in edge cases (no-timeline composition, non-30-fps future) and missing test coverage, but neither breaks the bug fix for the reported scenario.
— Vai
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approved per James's go-ahead.
The three doc/test asks from my prior review stand as non-blocking follow-ups (hardcoded fps=30 in snapshot quantization, removed __timelines duration fallback, Fallow audit ack). None gate merge.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Acknowledging Vai's catch I missed
Verified Vai's #1 directly at the PR head. Only Site 2 is gated; Sites 1 and 3 are not:
Site 1 — init.ts:1466 (async .finally()):
externalCompositionsReady = true;
bindRootTimelineIfAvailable();
(window as Window & { __renderReady?: boolean }).__renderReady = true; // ← unconditionalSite 2 — init.ts:1637-1639 (sync, post-createPlayerApi):
if (state.capturedTimeline) {
(window as Window & { __renderReady?: boolean }).__renderReady = true;
} // ✓ gatedSite 3 — init.ts:1645-1655 (setTimeout(0) rebinding):
if (externalCompositionsReady) {
setTimeout(() => {
const prevTimeline = state.capturedTimeline;
if (bindRootTimelineIfAvailable() && state.capturedTimeline !== prevTimeline) {
player._timeline = state.capturedTimeline;
}
runAdapters("discover", state.currentTime);
(window as Window & { __renderReady?: boolean }).__renderReady = true; // ← unconditional
postTimeline();
postState(true);
}, 0);
}In Site 3, the inner if (bindRootTimelineIfAvailable() && ...) gates the player timeline assignment but the __renderReady = true is outside the gate. So if bindRootTimelineIfAvailable() returns false (no timeline registered), __renderReady = true still fires.
So the fix has a hole: in the async (.finally) and deferred (setTimeout) paths, __renderReady can still lie if binding fails — restoring the original "signal lies about its meaning" bug at different timing.
Suggested fix (same shape as Site 2 — gate on state.capturedTimeline):
// Site 1
bindRootTimelineIfAvailable();
if (state.capturedTimeline) {
(window as Window & { __renderReady?: boolean }).__renderReady = true;
}
// Site 3 — move into the existing if-block
if (bindRootTimelineIfAvailable() && state.capturedTimeline !== prevTimeline) {
player._timeline = state.capturedTimeline;
(window as Window & { __renderReady?: boolean }).__renderReady = true;
}Where I went wrong
In my review I wrote:
"Pre-PR set at
init.ts:1547(the lying site, set with__playerReady) is removed. Idempotent — multiple paths can set true, no path sets false. ✓"
I confirmed the 3 new sites were present and after bindRootTimelineIfAvailable(), but I didn't audit the gating at each site. I treated "after binding-call" as equivalent to "after binding-success," which they're not — the binding can fail and the unconditional sites still flip the flag.
This is a "audit gating at each site, not just presence" lesson — adjacent to the discriminator-expansion review lens (each consumer of a shared state change needs independent verification). Saving this to memory.
My APPROVE stands as posted, since James greenlit on merit, but I'd treat Vai's Site 1/3 catch as a real follow-up before this lands — a one-line gate at each unconditional site closes the hole without disrupting the architecture.
Vai's #2 (double quantization) and #3 (no test for ordering) align with what I had — agreed.
— Rames Jusso
…ation, add tests - Guard __renderReady with `if (state.capturedTimeline)` in all three paths (setTimeout(0) and .finally() were setting it unconditionally even when bindRootTimelineIfAvailable returned false) - Remove redundant fps=30 pre-quantization in snapshot — renderSeek already calls quantizeTimeToFrame internally with the runtime's canonicalFps, so pre-quantizing was double-quantizing at a potentially wrong grid - Add regression tests: __renderReady is set when timeline exists, stays undefined when no timeline is available
window.d.ts already declares __timelines, __player, __playerReady,
and __renderReady on the global Window interface. The casts in
init.ts and init.test.ts were re-asserting the same types.
- Add __hfRuntimeTeardown to window.d.ts (used 6x in init.ts)
- Remove runtimeWindow cast variable from init.ts — use window directly
- Remove all (window as Window & { __player?: ... }).__player casts
from init.test.ts — window.__player is already typed as PlayerAPI
- Remove all (window as Window & { __timelines?: ... }).__timelines
casts from init.test.ts — window.__timelines is already typed
- Remove (window as Window & { __playerReady/renderReady }}) casts
from init.ts — already declared globally
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Updated review — LGTM
All three prior findings are resolved.
Finding 1 (blocker) — __renderReady gating: FIXED
All three assignment sites in init.ts are now gated on state.capturedTimeline:
.finally()path (line 1463-1464) — gated- Sync post-construction site (line 1638-1639) — gated
setTimeout(0)branch (line 1653-1654) — gated
The original unconditional site that sat next to __playerReady = true is removed entirely. The fileServer.ts media-fallback sets it unconditionally, but the inline comment correctly justifies it as a different runtime (no timeline to bind). That's intentional.
Finding 2 — Double quantization in snapshot.ts: FIXED
The pre-quantization block is gone. The new seek path just clamps to Math.max(0, Number(t) || 0) and delegates to renderSeek — no pre-rounding at 30fps.
Finding 3 — Test coverage for __renderReady ordering: ADDRESSED
The negative test ("does not set __renderReady when no timeline is available") is the right regression guard — it would catch any unconditional assignment. The positive test name implies an ordering assertion but only checks final state; the negative test is what actually enforces the invariant. Minor nit, doesn't affect correctness.
CI is in-flight (Format ✅, required checks pending) — results should land shortly.
— Vai
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approved — all review items addressed at 3347486a
Verified each fix at the new head:
Vai's items
- Site 1 & 3 gating ✓ — all three sites now
if (state.capturedTimeline) { window.__renderReady = true }. Asymmetric-gating gap closed. - Double quantization ✓ — better than just removing the pre-quantization, Miguel went with the architecturally cleaner shape: drop snapshot-side quantization entirely and let
renderSeekown it viaquantizeTimeToFrame(t, state.canonicalFps). That's the right call — the snapshot consumer shouldn't duplicate the player's frame logic. - No test for ordering ✓ — two new tests:
"sets __renderReady only after timeline is bound, not at __playerReady time""does not set __renderReady when no timeline is available"
These pin both the positive ordering invariant and the negative (no-timeline-no-flag) case — exactly the regression guard the fix needs.
My items
- Hardcoded fps=30 ✓ — implicitly addressed by removing snapshot-side quantization entirely.
renderSeekusescanonicalFps, so future 60fps support flows through naturally. - fileServer.ts cross-ref ✓ — comment added at
fileServer.ts:391-392: "Media-fallback player has no timeline to bind, so render-ready is immediate. init.ts defers__renderReadyuntil the timeline is bound — different runtime." - Duration fallback removal note ✓ — PR body now has a dedicated "Duration fallback removal" section explaining the dead-code-removal rationale.
- Fallow audit acknowledgment ✓ — PR body now has a "Fallow audit" section explaining the non-zero exit is from pre-existing duplication in
init.ts(clone groups untouched by this PR) and inherited complexity findings.
Bonus cleanup
The refactor(core): remove redundant as casts using window.d.ts declarations commit moves the __renderReady/__playerReady window properties into an ambient declaration. Cleaner site-level code without the (window as Window & {...}) boilerplate. Adjacent improvement, well-scoped.
CI
10 success / 18 in_progress / 1 failure (Fallow audit only — now explicitly acknowledged in PR body).
Every review item from both reviewers is in. The asymmetric-gating issue Vai caught is closed cleanly. My APPROVE stands; James's stamp greenlight from earlier carries forward.
The capturedTimeline guard broke CSS/WAAPI/Lottie compositions that have no GSAP timeline — __renderReady was never set, causing the parity harness to timeout after 30s. renderSeek works with or without a GSAP timeline (adapter-only seeking), so the correct invariant is "timeline binding was attempted" not "a timeline was found." Set __renderReady unconditionally in all three paths, after bindRootTimelineIfAvailable has run.
Merge activity
|
3 participants
Summary
__renderReadywas set at the same time as__playerReady, before the root timeline was bound. Moved it to fire only afterbindRootTimelineIfAvailable()succeeds — either immediately, via the deferredsetTimeout(0)rebinding for bundled compositions, or via the async.finally()for externally loaded compositions.__renderReady(the now-truthful signal) instead of__timelines || __playerReady. UsesrenderSeek()with frame quantization and GSAP ticker tick, matching the parity harness. Awaitsdocument.fonts.readybefore capturing.Root cause
init.tsset__renderReady = trueon line 1547, at the same time as__playerReady. But timeline binding happens later:bindRootTimelineIfAvailable()(line 1632)setTimeout(0)for bundled compositions (line 1642)loadExternalCompositions(line 1461)This meant
__renderReadylied — it said "safe for deterministic frame capture" while the player had no timeline. The snapshot command (and any consumer using__renderReady) could callrenderSeek()on a player with no captured timeline.Duration fallback removal
The old snapshot code walked
__timelineslooking for.durationas a fallback. This was dead code — it used the nonexistent.durationproperty (not a method call) which always returnedundefined, falling through to the DOM attribute anyway. The new code usesgetDuration()(the actual PlayerAPI method) with the DOMdata-durationattribute as fallback. Since__renderReadyguarantees the player is fully initialized,getDuration()should always be available at seek time.Verification
Instrumented the runtime to confirm:
__timelinesis set 15-56ms before__playerReady(same synchronous block, but the readiness check pattern matters for future consumers)__renderReadyfires 1.4-3.4ms after__playerReady, confirming the timeline is bound before the signalrenderSeek(5)works correctly at the moment__renderReadyis set (verified across warm-grain, play-mode, product-promo)__timelinesmanual seek fallback produces visually different frames fromrenderSeekat 4/5 timestamps on warm-grain — the animation states diverge due to missing time quantization and sibling timeline activationFallow audit
Fallow exits non-zero due to pre-existing duplication in
init.ts(clone groups at lines 757-805 and 1612-1905, untouched by this PR) and inherited complexity findings across both files. ThecaptureSnapshotsfunction was already over the CRAP threshold pre-PR; the net -66 line reduction improves it slightly but doesn't cross it below. No new complexity or duplication introduced.Test plan
bun run buildpasseshyperframes snapshotworks on product-promo and warm-grain (built CLI)__renderReadytiming verified via instrumented probes (3 examples, 3 runs each)Closes #1047