Skip to content

fix(studio): preserve playback across forward RAF loop wrap-around#1103

Merged
miguel-heygen merged 1 commit into
heygen-com:mainfrom
calcarazgre646:fix/loop-wrap-keep-playing
May 28, 2026
Merged

fix(studio): preserve playback across forward RAF loop wrap-around#1103
miguel-heygen merged 1 commit into
heygen-com:mainfrom
calcarazgre646:fix/loop-wrap-keep-playing

Conversation

@calcarazgre646
Copy link
Copy Markdown
Contributor

Summary

Pass { keepPlaying: true } to the forward RAF loop's wrap-around adapter.seek(loopStart) call so the seek doesn't trigger the adapter's implicit pause that the very next line (adapter.play()) has to undo.

Background

After the keepPlaying contract was established for the public seek API (#842, #863) and aligned across both adapters (wrapTimeline hardened in 3e7b464b, createStaticSeekPlaybackAdapter in #1089), the internal loop wrap-around in useTimelinePlayer.ts:232 still calls the bare 1-arg form:

if (time >= loopEnd) {
  if (usePlayerStore.getState().loopEnabled && dur > 0) {
    adapter.seek(loopStart);   // ← default seek, which now means "pause"
    liveTime.notify(loopStart);
    adapter.play();            // ← resume what was just paused
    setIsPlaying(true);
    rafRef.current = requestAnimationFrame(tick);
    return;
  }

The intent here is unambiguous: we are inside loopEnabled && dur > 0, the return short-circuits the no-loop terminal-pause branch below, and the very next statement resumes playback. The default-pause behavior is pure churn.

What happens per wrap, per adapter

Path Without keepPlaying With keepPlaying
wrapTimeline (GSAP) tl.pause()tl.seek()tl.pause()tl.play() (4 ops, two of which fight each other) tl.seek(); tl.play() is a no-op on an already-playing timeline (1 op)
createStaticSeekPlaybackAdapter renderSeek()playing = falsecancelAnimationFrame → then play() re-arms playing = true and re-schedules RAF renderSeek() → rebase playStartTime / playStartNow; play() early-returns because playing is still true

The follow-up adapter.play() and setIsPlaying(true) stay as a safety belt — they're cheap no-ops on the happy path but keep the branch correct if the adapter somehow already drifted to paused.

Tests

packages/studio/src/player/hooks/useTimelinePlayer.seek.test.ts had no coverage for the RAF wrap-around path (verified by grep: zero loop/wrap/loopStart references across the entire studio test suite for this file). Adds a new describe("useTimelinePlayer RAF loop wrap-around") block with:

  1. Wrap-around with loop enabled — sets inPoint=2, outPoint=5 (which auto-enables loop per fix(studio): auto-enable loop when work-area markers are set #859), calls api.play(), advances mock adapter time past outPoint, drives one captured RAF callback, asserts adapter.seek was invoked with (2, { keepPlaying: true }) and adapter.play ran.
  2. End of timeline without loop (regression guard) — sets loopEnabled=false, advances past duration, asserts no seek to loopStart happened and adapter.pause() was called instead (the other branch in the same if (time >= loopEnd) block).

Both tests install/restore a local requestAnimationFrame capture so RAF callbacks can be driven deterministically; no global state leaks between tests.

Validation

Check Result
bun run --cwd packages/studio test 620/620 ✅ (+2)
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 clean (no new findings)
useTimelinePlayer.ts line count 599 (under the 600-line studio gate)

Cross-PR check

Branched from upstream/main at 7be4f92f. Of the open PRs, none touch packages/studio/src/player/hooks/useTimelinePlayer.ts:

Out of scope (intentionally)

  • The backward RAF loop's terminal adapter.seek(loopStart) at line 336 correctly stays on the default (it is followed by setIsPlaying(false) and is the no-loop terminal pause).
  • The backward loop's wraparound case (if (loopEnabled) branch at line 327) doesn't call adapter.seek directly — it resets local startTime / nextTime and lets the reverse RAF drive the seek on the next tick with the adapter already paused (adapter.pause() at the start of playBackward).
  • The play() callback's recover-from-end seek at line 297 was considered but is an edge case (the adapter is usually paused there, so keepPlaying would be inert); leaving it out keeps this PR tight.

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: heygen-com#842 introduced the option for A/E
shortcuts, heygen-com#863 extended it to the runtime player, heygen-com#1089 aligned the
static-seek adapter, and this applies it to the last internal caller
that explicitly resumes after seek.
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.

Fix is correct. { keepPlaying: true } on the forward RAF loop wrap-around seek prevents both adapters from implicitly stopping playback mid-transition:

  • createStaticSeekPlaybackAdapter: skips playing = false + cancelAnimationFrame
  • wrapTimeline (GSAP): skips the double tl.pause() around tl.seek()

The subsequent adapter.play() stays as a safety belt. Backward loop seek sites correctly left untouched (terminal-pause and per-tick-on-paused-adapter paths).

Tests cover both the positive case (keepPlaying: true passed + play() called after RAF) and the regression guard (no wrap-around seek when loopEnabled=false).

Minor: the inline comment "play() below is then a no-op" is only true for the happy path — if the adapter drifted paused despite keepPlaying, play() would catch it. Could say "fallback" instead of "no-op." Non-blocking.

@miguel-heygen miguel-heygen merged commit 0c0ccce into heygen-com:main May 28, 2026
34 checks passed
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