Skip to content

fix(studio): clamp playhead to composition duration in RAF loop#1076

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/studio-playhead-overflow-past-duration
May 25, 2026
Merged

fix(studio): clamp playhead to composition duration in RAF loop#1076
miguel-heygen merged 2 commits into
mainfrom
fix/studio-playhead-overflow-past-duration

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

Summary

  • Fix playhead overshooting past data-duration in the studio preview (e.g., showing 0:19 on a 0:10 composition)
  • Clamp adapter.getTime() to adapter.getDuration() before notifying liveTime in the forward RAF loop

Root Cause

The studio player's RAF loop in useTimelinePlayer.ts called liveTime.notify(time) (line 231) BEFORE the duration check on line 237. When adapter.getTime() returned a value past the composition's data-duration — due to timing drift, delayed duration calculation, or compositions loaded from external HTML — the playhead would visually overshoot.

The web player component (packages/player/) already had this clamping:

  • playback-state.ts line 42: Math.min(rawTime, current.duration)
  • direct-timeline-clock.ts line 56: Math.min(currentTime, duration)

But the studio player's forward loop was missing it.

Fix

-        const time = adapter.getTime();
+        const rawTime = adapter.getTime();
         const dur = adapter.getDuration();
+        const time = dur > 0 ? Math.min(rawTime, dur) : rawTime;
         liveTime.notify(time);

Matches the clamping pattern already used in the web player. The dur > 0 guard ensures we don't clamp when duration isn't yet resolved.

Test plan

  • Open a 10s composition in the studio, play it — playhead should stop at 0:10, not overshoot
  • Verify loop mode still works (seeks back to start at duration boundary)
  • Verify compositions with data-duration from external HTML (e.g., website-to-html captures) don't overflow

The studio player's RAF loop in useTimelinePlayer notified the playhead
position via liveTime.notify(time) before checking the duration limit.
When adapter.getTime() returned a value past the composition's
data-duration (due to timing drift or delayed duration calculation),
the playhead would visually overshoot — showing e.g. 0:19 on a 0:10
composition.

The web player component already had this clamping (playback-state.ts
line 42, direct-timeline-clock.ts line 56), but the studio player's
forward loop was missing it.

Fix: clamp time to dur before notifying, matching the pattern already
used in the web player: Math.min(rawTime, dur) when dur > 0.
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

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

Verdict: COMMENT (would-be APPROVE pending CI fix)

Fix is correct. Holding pending the file-size CI failure.

Correctness verified ✓

  • Off-by-one at duration: Math.min(rawTime, dur) returns dur when rawTime === dur. An animation at exactly t=duration still receives the t=dur notification (the liveTime.notify(time) fires before the loop-end check at line 239). ✓
  • Loop semantics preserved: loopEnd = rawLoopStart < rawLoopEnd ? rawLoopEnd : dur. When time is clamped to dur and no outPoint exists, loopEnd = dur, so time >= dur >= loopEnd triggers the loop-back. ✓
  • Pause-at-end semantics preserved: if (time >= loopEnd) with loopEnabled = false calls adapter.pause() and setIsPlaying(false). ✓
  • No duplicate adapter.getDuration() call: the dur variable was already on line 230 pre-PR; just moved one line up to be available for the clamp. Single call per tick. ✓
  • Web player clamp pattern matches: verified packages/player/src/playback-state.ts:42 and direct-timeline-clock.ts:~56 use the identical dur > 0 ? Math.min(...) : raw shape. Cross-package parity. ✓
  • dur > 0 guard correct: when duration isn't yet resolved (initial mount, before composition loaded), pass rawTime through unclamped. ✓

🚩 CI red — file-size check

packages/studio/src/player/hooks/useTimelinePlayer.ts is now 611 lines (max 600 per the file-size CI rule from hf#1012). The +1 net line from this PR tipped it over.

Options:

  1. Refactor a small section out (e.g., extract startRAFLoop or stopRAFLoop into a sibling helper). Largest scope.
  2. Inline the clamp without a rawTime variable — replace the 3-line block with 2 lines:
    const dur = adapter.getDuration();
    const time = dur > 0 ? Math.min(adapter.getTime(), dur) : adapter.getTime();
    Net 0 lines added vs. pre-PR. Would put the file at 610 (still over 600). Doesn't get under the limit alone — need 11 more lines removed. So this isn't enough by itself.
  3. Raise the file-size limit — but this was JUST raised from 500 → 600 in hf#1012. Doing it again means the limit is non-load-bearing.

I'd recommend option 1 — extract startRAFLoop and stopRAFLoop (the RAF forward-tick logic) into a separate useRAFPlayback.ts hook. They're naturally cohesive (~40 lines together). That brings the file well under 600 without the perpetual "raise the limit" cycle.

CI: 20 success / 4 in_progress / 1 failure (file-size). All other checks green.

Per dont-normalize-red-ci-as-batch-noise: this failure can't be dismissed — the file-size rule is required and the +1 line directly causes the fail. The fix itself is 3 lines; the refactor to enable it is the larger change.

— Rames Jusso

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Strengths: the clamp mirrors the pattern already present in seek (line 402, Math.min(duration, time)) and is symmetric with the Math.max(0, nextTime) lower-bound guard in the reverse RAF tick (lines 357–358). The dur > 0 guard correctly threads unresolved-duration states through as a no-op rather than clamping to zero.


important — outPoint > dur leaves the loop termination dead

setOutPoint in playerStore.ts:137 accepts any finite number without clamping to duration. When outPoint > dur, rawLoopEnd = outPoint (line 234), so loopEnd > dur, and with the new clamp time can never reach loopEnd. The time >= loopEnd branch (line 238) will never fire — the player silently ticks forever instead of pausing or looping at the end.

Before this PR the bug was masked: adapter.getTime() drifted past dur, so time >= loopEnd eventually fired anyway. Post-PR the drift is gone and the dead-end is exposed.

Fix options (pick one):

  • Clamp rawLoopEnd to dur when computing loopEnd: const rawLoopEnd = outPoint !== null ? Math.min(outPoint, dur) : dur; (parallel with how seek already clamps at line 402).
  • Add a dur-clamp inside setOutPoint in playerStore.ts.

Neither is blocked on this PR if the team judges outPoint > dur as an invariant that's never triggered in practice today — but the lack of a clamp in the store means it's a latent issue worth a follow-up ticket.


important — no test for the RAF tick boundary

The test file (useTimelinePlayer.test.ts) already has createManualAnimationClock infrastructure (line 62) and a sibling "clamps explicit seeks" test (line 137) that shows the right shape. The forward RAF tick clamping at line 231 is not covered. A test driving the clock past dur and asserting liveTime doesn't exceed it would pin this behavior and prevent future regressions.


nit — File size check is now failing (611/600 lines)

Not a required gate (not in the branch ruleset's required_status_checks), so it won't block merge. But this file is now 11 lines over the project's 600-line soft cap, and every future PR touching it will trip the same check. Worth a follow-up to extract the reverse-shuttle logic or the keyboard/sync callbacks into their own hooks — the re-export block at the top of the file (lines 7–26) signals the decomposition has already started.


CI note: Render on windows-latest and Tests on windows-latest are still pending. Nothing required is failing, but worth confirming they go green before merge.

Verdict: APPROVE
Reasoning: The clamp is correct, matches the existing pattern in seek and the web player, and the dur > 0 guard is the right shape. The outPoint > dur dead-loop is a latent issue worth a follow-up ticket but doesn't break any behavior that worked before this PR.

— Vai

When outPoint exceeds composition duration, rawLoopEnd > dur makes the
time >= loopEnd branch unreachable after the playhead clamp — the player
ticks forever. Clamp rawLoopEnd to dur in both forward and backward RAF
loops, matching the seek() clamping. Add test for the boundary behavior.
Trim blank lines to satisfy 600-line filesize gate.
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

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

Verified at 0897d3ef:

  • outPoint > dur loop-termination fix ✓: both forward (useTimelinePlayer.ts:234) and backward (:334) RAF loops now use outPoint !== null ? Math.min(outPoint, dur) : durtime >= loopEnd branch reachable again.
  • RAF tick boundary test ✓: new test in useTimelinePlayer.test.ts uses createManualAnimationClock, steps 3000ms on a 2s adapter, asserts getTime() === 2 + isPlaying() === false + renderedTimes === [0, 2].
  • File-size CI ✓: trimmed blank separator lines, file now at 599 / 600.
  • CI: 9 success / 15 in_progress / 0 failures.

Re-approved.

— Rames Jusso

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Re-review after author's fixes. All three prior blockers resolved.

1. outPoint > dur loop termination — FIXED in both loops

Forward RAF tick (startRAFLoop):

  • rawTimetime clamp: dur > 0 ? Math.min(rawTime, dur) : rawTime
  • rawLoopEnd: outPoint !== null ? Math.min(outPoint, dur) : dur

Backward RAF tick (playBackward):

  • rawLoopEnd: outPoint !== null ? Math.min(outPoint, duration) : duration

Both loops fixed, not just one.

2. Test coverage — ADDED

New test "clamps time at the duration boundary during RAF tick" using createManualAnimationClock. Steps 3s past a 2s duration, asserts getTime() === 2, isPlaying() === false, and renderedTimes === [0, 2]. Covers the overshoot clamp and auto-pause after clamp.

The outPoint > dur path isn't tested directly, but the fix is a one-liner in two symmetric spots — acceptable test scope.

3. File size gate — PASSED

useTimelinePlayer.ts is 599 lines at this HEAD. CI File size check passes.

CI: All required checks green. CLI smoke (required) still pending but not failing.

LGTM. Ship it.

— Vai

@miguel-heygen miguel-heygen merged commit 07d1455 into main May 25, 2026
34 of 35 checks passed
@miguel-heygen miguel-heygen deleted the fix/studio-playhead-overflow-past-duration branch May 25, 2026 23:18
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.

3 participants