Skip to content

fix(studio): make static-seek adapter honor keepPlaying option#1089

Merged
miguel-heygen merged 1 commit into
heygen-com:mainfrom
calcarazgre646:fix/static-seek-keep-playing
May 27, 2026
Merged

fix(studio): make static-seek adapter honor keepPlaying option#1089
miguel-heygen merged 1 commit into
heygen-com:mainfrom
calcarazgre646:fix/static-seek-keep-playing

Conversation

@calcarazgre646
Copy link
Copy Markdown
Contributor

Summary

createStaticSeekPlaybackAdapter.seek now accepts the { keepPlaying } option declared by the PlaybackAdapter contract (playbackTypes.ts:10) and aligns its default-pause semantics with wrapTimeline.seek (recently hardened in 3e7b464b, "ensure GSAP timeline stays paused after seek"). Same call sites, same wrapTimeline default — now the static-seek path matches.

Background

This is the follow-up I called out as "Out of scope" in #863 and that @jrusso1020 invited in review:

"createStaticSeekPlaybackAdapter type drift (out of scope, called out in PR). Worth tightening the type now — it's a one-line change and the body already preserves playback by accident. Leaving the type signature mismatched is a footgun for the next person who reads the adapter and thinks keepPlaying is unhandled. Happy to follow up in a separate PR if you'd rather keep this one tight."#863 review

What was wrong

The adapter's seek signature was (time) => void, narrower than the PlaybackAdapter.seek contract. The body also had an asymmetric default vs wrapTimeline.seek:

  • wrapTimeline.seek (line 137, post-3e7b464b): default seek tl.pause()tl.seek()tl.pause(). keepPlaying: true skips both pauses.
  • createStaticSeekPlaybackAdapter.seek (before this PR): default seek kept playing = true, so the RAF ticker continued calling renderSeek in the iframe even when the public useTimelinePlayer.seek wrapper had already called stopRAFLoop() + setIsPlaying(false).

User-observable on non-GSAP compositions (the path where this adapter is used — useTimelinePlayer.ts:160 fallback when win.__timelines is absent): scrubbing the timeline during playback would leave the iframe content silently advancing while the UI reported "paused".

Fix

packages/studio/src/player/lib/playbackAdapter.ts — 7 lines:

seek: (time, options) => {
  renderSeek(time);
  if (options?.keepPlaying) {
    if (playing) {
      playStartTime = currentTime;
      playStartNow = clock.now();
    }
    return;
  }
  // Default seek aligns with wrapTimeline: stop the RAF ticker so the
  // adapter's `playing` flag matches the public seek contract instead of
  // silently driving renderSeek in the background.
  playing = false;
  stopTicker();
},

keepPlaying: true keeps the existing "rebase playStartTime/playStartNow" behavior so the next tick renders from the new position. Default and explicit keepPlaying: false now clear playing + cancel the RAF.

Internal callers checked

All adapter.seek() call sites in packages/studio/src/player/hooks/useTimelinePlayer.ts are safe under the new default:

  • useTimelinePlayer.ts:199 (forward loop wrap-around): immediately calls adapter.play() after the seek, so a pause-on-default seek is replayed in the same tick.
  • useTimelinePlayer.ts:259 (replay from end inside play()): same — followed by adapter.play().
  • useTimelinePlayer.ts:286 (playBackward init): preceded by an explicit adapter.pause(), so default-pause is a no-op.
  • useTimelinePlayer.ts:307 (backward loop end, no-loop terminal): immediately followed by setIsPlaying(false); default-pause is the desired terminal state.
  • useTimelinePlayer.ts:316 (backward tick mid-traversal): the reverse path drives playback through its own RAF (reverseRafRef), not the static-seek internal tick, so cancelling the static-seek tick on every reverse step is harmless.
  • useTimelinePlayer.ts:362 (public seek(time, options)): forwards options straight through. keepPlaying: true from A/E shortcuts now reaches the static-seek adapter end-to-end.
  • useTimelineSyncCallbacks.ts:161 (initial adapter setup): adapter is paused at construction; default-pause is consistent.

Tests

packages/studio/src/player/lib/playbackAdapter.test.ts only covered wrapTimeline (no coverage for createStaticSeekPlaybackAdapter). Adds 7 tests under a new describe("createStaticSeekPlaybackAdapter seek keepPlaying option") block, driving the adapter with a deterministic fake StaticSeekPlaybackClock:

  • default seek stops the RAF ticker (isPlaying() === false, cancelAnimationFrame observed)
  • default seek prevents queued frames from advancing renderSeek beyond the seek target
  • keepPlaying: true preserves playback and rebases the ticker so the next frame renders from the new position
  • explicit keepPlaying: false matches the default
  • keepPlaying: true does not force playback when the adapter is already paused (intent is preserve, not force play)
  • back-compat: calling seek(t) without options matches the contract (still pauses)
  • default seek clamps overshoot to duration and still pauses

Validation

Check Result
bun run --cwd packages/studio test 612/612 ✅ (+7)
bun run --cwd packages/studio typecheck clean
bunx oxlint (touched files) 0/0
bunx oxfmt --check (touched files) clean
Lefthook (filesize + lint + format + typecheck + fallow + commitlint)
Fallow audit ✓ No issues in 2 changed files

Cross-PR check

Branched from upstream/main at 3cd6cd6a. Of the 30 open PRs, only #1085 (@miguel-heygen) touches the seek path — it refactors useTimelinePlayer.ts to extract shouldResumeForwardPlaybackAfterSeek / shouldStopAfterSeek helpers but does not touch playbackAdapter.ts. Zero file overlap. #1085's new public-seek logic still forwards options to adapter.seek(nextTime, options), so the contract this PR establishes is what that refactor relies on.

Out of scope (intentionally)

The remaining nits from @jrusso1020's #863 review (sibling play→pause→play churn in seekMasterAndSiblingTimelinesDeterministically, missing onDeterministicPause emit on seek(), setIsPlaying(true) asymmetry in the keepPlaying branch of runtime/player.ts) are independent of this adapter and not addressed here.

createStaticSeekPlaybackAdapter.seek now accepts the same options as the
PlaybackAdapter contract and aligns the default-pause semantics with
wrapTimeline (hardened in 3e7b464). Without keepPlaying the adapter
clears its `playing` flag and cancels the RAF ticker, so on non-GSAP
compositions a scrub during playback no longer leaves the iframe
silently advancing while the public seek wrapper marks isPlaying=false.

Follow-up to heygen-com#863 review: jrusso called out the type drift and invited
a separate PR; this also closes the asymmetry with wrapTimeline.
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I reviewed the live head fd2a97e, ran the focused Studio playback tests/typecheck/lint locally, and smoke-tested the keep-playing seek path in Studio with agent-browser. No blockers from my side.\n\nNote before merge: the commit on this PR is currently unverified by GitHub (verification reason: unknown_key). The PR author needs to verify/sign the commit(s) so the branch protection checks can allow the PR to merge.

@calcarazgre646
Copy link
Copy Markdown
Contributor Author

Thanks @miguel-heygen — signing key added; fd2a97ea now verified by GitHub. Should be unblocked for merge.

@miguel-heygen miguel-heygen merged commit 8ecef4b into heygen-com:main May 27, 2026
34 checks passed
miguel-heygen pushed a commit that referenced this pull request May 28, 2026
…1103)

When forward playback reaches loopEnd and the loop wraps back to
loopStart, the RAF tick was calling `adapter.seek(loopStart)` without
keepPlaying, then immediately `adapter.play()` to resume. With the
post-3e7b464b wrapTimeline contract (default seek pauses), this means
every loop boundary executes pause→seek→pause→play for GSAP and a
stop/start RAF ticker cycle for the static-seek adapter — purely
unnecessary churn.

Pass { keepPlaying: true } so seek skips the implicit pause; the
follow-up adapter.play() is then a no-op because the underlying
adapter never paused. Adds two tests covering the wrap-around branch
(previously uncovered) and the no-loop terminal path as a regression
guard.

Completes the keepPlaying rollout: #842 introduced the option for A/E
shortcuts, #863 extended it to the runtime player, #1089 aligned the
static-seek adapter, and this applies it to the last internal caller
that explicitly resumes after seek.

Co-authored-by: Carlos Alcaraz <193642530+calcarazgre646@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants