fix(core): restore nested seek scrubbing#404
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Root-cause analysis in the PR body is correct, and the fix shape is right. Walking through the mechanics for the record since this is the kind of thing that's easy to regress again:
The regression chain is:
- Pre-
e72bcfa: master pause didn't propagate to siblings, so scene animations kept running after the user clicked pause. That was a real bug #359 fixed. e72bcfafixed it by calling.pause()on every sibling timeline inwindow.__timelineswhenever the master paused.- The gap: seek didn't get the same treatment. In GSAP, once a child timeline has been individually paused via
child.pause(), master's.seek()doesn't advance that child's local playhead on its own — the child's_pausedflag gates the propagation. So scrub → master moves, scene visuals stay frozen at their pre-pause state. That's the "scrubbing doesn't work" James saw.
Why this fix works and doesn't introduce a new regression:
The seek path now re-arms siblings via .play() before the master seek, lets the deterministic seek propagate through the master→children graph, then re-pauses in a finally. Semantically that matches what a user expects from "seek": a single paused snapshot of the composition at time T. The setIsPlaying(false) post-seek confirms the player ends in paused state regardless of prior state.
try/finally around the master seek + try/catch around each sibling pause is the right shape — one broken sibling timeline shouldn't poison the whole seek. Nice defensive shape.
To answer James's earlier question ("is e72bcfa still needed?"): yes, keep it. This fix is the counterpart, not a replacement. Reverting #359 would reintroduce the "pause doesn't stop scene animations" bug. Miguel's fix is additive — pause propagation + seek propagation, both required. The two together are what the old (pre-e72bcfa) code got right by accident because master-pause was the only pause happening.
Non-blocking observations worth noting:
-
Re-arm fires on every sibling regardless of prior state. If a sibling was already playing when seek was called, it gets
.play()(probably no-op), then.pause()in the finally. Net effect: any sibling that was playing before the seek is now paused. Consistent withplayer.seek()being a pause-and-seek operation persetIsPlaying(false), but technically a behavior change from the pre-e72bcfastate where a seek during playback kept siblings running. A test that exercises the "sibling was playing" branch would pin the current contract explicitly — the existing tests both pause first, so the "was-playing" case is implicit. -
play()/pause()vspaused(bool)setter. In GSAP,.play()and.pause()walk the timeline's event path and mark it as active..paused(true)/.paused(false)flip the internal flag without the event overhead. For this use case (round-trip re-arm → seek → re-pause inside a single synchronous block) either works, but the setter form is marginally cheaper and avoids any edge-case where GSAP's play event fires a listener that observes the transient unpaused state. Worth considering if you ever profile scrub latency. -
Scope of the sweep.
forEachSiblingTimeline(registry, master, ...)walks every entry inwindow.__timelinesexcept the master. If a composition registers unrelated background timelines there (e.g. an ambient audio timeline not part of the scene graph), they'll also get re-armed and re-paused on every scrub. Probably not an issue today because the convention is that anything in__timelinesis scene-related, but worth a comment documenting that invariant so a future author doesn't add an unrelated entry and get surprised. -
Master's paused state.
seekTimelineDeterministicallyruns.seek()on the master regardless of its paused state. If the master is paused and.seek()respects the paused flag, child propagation might also be gated. Worth confirming empirically (or via GSAP docs) — but the fact that Miguel verified againstproduct-promoin agent-browser and it works tells me the master-seek-while-paused path is fine in practice.
Test coverage is focused on exactly the scenario that regressed — pause + seek on a nested composition — with both seek() and renderSeek() covered. Scene5 being at 0 when master seeks to 3 (because scene5 starts at 12) is the right edge case to pin.
Approved. Nice turnaround on a subtle regression.
— Rames Jusso
What changed
seek()andrenderSeek()inpackages/core/src/runtime/player.test.tsWhy this changed
product-promoscrubbed correctly in0.3.1but regressed in0.4.12e72bcfaed303ad977f655f7a3e11dd6641ba95d5, which fixed nested play/pause propagation but left seek propagation incompleteRoot cause
e72bcfae, pausing the master timeline also paused the nested scene timelines (scene1-logo-intro,scene2-4-canvas,scene5-logo-outro), which was correct for playback controlseek()on the master timeline0.4.12was: the scrubber moved,window.__player.seek()ran,mainmoved to the requested time, but the nested scene timelines stayed pinned at their old local times, so the preview frame was wrongWhy this fix works
e72bcfaewhile restoring correct scrub behaviorVerification
bun run --filter @hyperframes/core test -- src/runtime/player.test.tscd packages/core && bun run test:hyperframe-runtime-seekagent-browseragainstproduct-promopreview0.3.1: scrubbing advanced nested scenes correctly0.4.12: scrubbing moved the master timeline but left nested scene timelines stuckReviewer notes