fix(producer): suppress GSAP call side effects during render seeks#2037
Conversation
terencecho
left a comment
There was a problem hiding this comment.
Review
LGTM at head f731ecdf. The fix cleanly addresses the reported root cause: suppressEvents is threaded end-to-end through admin/verify seek paths, the GSAP adapter no longer duplicates the root seek when a captured timeline exists, and verifyStaticFramesSafe now restores the playhead to frame 0 in a finally block so real capture begins from a known state.
What I cross-checked
1. Suppresses only during admin seeks, not normal playback/render.
- User-facing
player.seek(time)is untouched — no options threading; regular playback semantics unchanged. renderSeek(t)called without options →suppressEventsdefaults toundefined→ falsy →.call()callbacks fire normally.- Real linear capture at
frameCapture.ts:1776still callswindow.__hf.seek(t)with no options → callbacks fire during the actual capture pass. That is what the linear pass depends on. - Only three explicit
suppressEvents: truecall sites in the diff, all administrative:verifyStaticFramesSafe(frameCapture.ts:2078)captureDeVerificationFrames(frameCapture.ts:3000)- Adapter
discoverbootstrap (init.ts:1909)
2. Doesn't drop legitimate callbacks.
- The linear capture loop is the only path that must fire callbacks, and it does not opt into suppression.
- The GSAP adapter's pre-existing "nudge to
safeTime + 0.001(always silent) → seek back tosafeTime" trick means callbacks were never fired via the adapter path even before this PR, since the second seek is always backward. ThreadingsuppressEventsinto the second call atadapters/gsap.ts:21is effectively a no-op today, but it is the right defensive choice — worth a one-line comment noting that the nudge is what actually silences forward crossings, so a future refactor that removes the nudge doesn't accidentally regress this.
3. Correctly scoped/restored.
suppressEventsis a per-totalTime()/seek()GSAP flag, not sticky state. No leaked global to worry about.verifyStaticFramesSafe's newtry { … } finally { await seekToFrame(0).catch(() => {}); }restores playhead to 0 withsuppressEvents: true, so admin verification leaves neither state-mutation nor a poisoned playhead behind.- The
if (adapter.name === "gsap" && state.capturedTimeline) continue;guard in bothonDeterministicSeek(init.ts:2074) andseekTimelineAndAdapters(init.ts:2481) is correct:createGsapAdapter({ getTimeline: () => state.capturedTimeline })atinit.ts:2231confirms the adapter's target is the captured root timeline, so skipping avoids a duplicate seek without dropping any seek coverage. Standalone GSAP timelines (registered inwindow.__timelinesbut not nested under the root) run throughseekStandaloneRegisteredTimelines, which is unaffected by the skip and correctly threadsoptsthrough.
Non-blocking notes
a. bindRootTimelineIfAvailable still uses totalTime(seekTime, false) (init.ts:1202). Under a fresh render this path is safe because state.currentTime === 0 at bind time and the preceding progress(0.0001, true) nudge is silent, so the forward crossing 0.0001 → 0 fires nothing. But transportTick re-invokes bindRootTimelineIfAvailable every 60 ticks (init.ts:2516), and if the runtime ever rebinds mid-render at a non-zero playhead, this call would fire .call() callbacks again. Not observed by the new fixture, but defense-in-depth would be suppressEvents: true on that specific bind-time seek too, since it is administrative.
b. RuntimeSeekOptions type is generic (types.ts:216) but only carries suppressEvents today. Fine as an extensible shape; just wanted to flag it in case future call-site additions layer in unrelated flags (stepMedia, flushAdapters, …).
c. Test naming nit. The new engine test lives in frameCapture-staticDedupVerifyDensity.test.ts but exercises the silent-verification-seek + restore-to-frame-0 behavior of verifyStaticFramesSafe, which is broader than "verify density." Optional: move to a new frameCapture-staticDedupVerifySilence.test.ts or rename the file. Not worth a rev if the shard groupings depend on the current filename.
d. CI: regression-shards (all 8) and Windows render verification are still IN_PROGRESS at review time. Since the fix touches core render capture semantics and a new golden fixture ships as LFS, worth waiting for those to land green before merge — the risk they surface something is low but non-zero.
Nothing above is blocking.
— Review by tai (pr-review)
terencecho
left a comment
There was a problem hiding this comment.
Stamping per Terence's ok. Full review posted above; no blockers. — Review by tai (pr-review)
Problem
Issue #2036 was a producer-only GSAP side-effect bug: render-internal seeks were firing authored
.call()callbacks before real capture, so a counter that should render as0 → 1 → 2 → 3 → 4 → 4came out of publishedhyperframes@0.7.41as0 → 3 → 4 → 5 → 6 → 4.The preview looked correct because the corruption came from producer probes, not the authored timeline: calibration, static-dedup, drawElement verification, and one duplicate root GSAP seek all drove timeline state with callback events enabled.
CI also caught the subtle half of the fix: simply removing the duplicate root seek prevents
.call()inflation, but it also removes GSAP's forced-render refresh for ordinary root timelines. That drifted an existingstyle-4-prodcaption checkpoint in regression shard 5.Fix
suppressEventsthroughwindow.__hf.seek,renderSeek, deterministic adapters, and the GSAP adapter so render/admin probes can seek silently.tl.calland callback-only tweens), so callback side effects are not re-armed while normal tween timelines keep their previous visual refresh behavior.gsap-call-render-seek, including LFS-backed compiled/output baselines, to lock the bug down end-to-end.Closes #2036
Tests
bun run buildbunx oxlint <changed source/test files>bunx oxfmt --check <changed source/test files>init.test.ts44/44,player.test.ts40/40hyperframes lint/hyperframes validateongsap-call-render-seek: 0 errors, 0 warnings, no console errorsgsap-call-render-seekwith 100/100 visual checkpointsstyle-4-prod+gsap-call-render-seekboth passed; the shard-5 checkpoint at 11.876s is clean again