fix(player): replay from start when play is pressed after video ends

[miguel-heygen]

Summary

• When a non-looping composition reaches its end and the user presses play again, the player now seeks to frame 0 before resuming — previously it sent a play control with the playhead stuck at the final frame, which had no visible effect.
• The fix is a 3-line guard in play() that checks _currentTime >= _duration and calls seek(0) before dispatching the play command.
• Two new tests cover the replay-after-ended path and confirm mid-video play is unaffected.

Test plan

• bun run --cwd packages/player test — all 92 tests pass
• bun run --cwd packages/player build — clean build
• Lint + format + typecheck pass (lefthook pre-commit)
• Manual: open a composition in the player, let it play to the end, press play — should restart from 0:00

[vanceingalls]

Verdict: Approve. Tight, well-scoped fix with proportionate test coverage. Ships.

Praise

• The 3-line guard at the right layer. The bug is "play after end is a no-op because the playhead is parked at duration"; fixing it inside play() (rather than the controls callback or the runtime) means every entry point — public API, controls button, future shortcut keys, programmatic — gets the same behavior. Right place for the invariant.
• _duration > 0 precondition matters and is included. Without it, a play() issued before the runtime has reported duration would seek(0) on a phantom end-state. Easy to miss; nice catch.
• Both new tests assert the right things: the positive case (seek(0) called, _paused flips to false) AND the negative case (mid-video play does NOT seek). The negative test is what protects this guard from regressing into "always seek to 0 on play" later.

Important

• (none)

Nits

• nit — packages/player/src/hyperframes-player.ts:404-413 — seek() toggles _paused = true and controlsApi.updatePlaying(false) (line 465-466), and then play() immediately overrides both to false/true. Same tick, no observable flicker, but it does mean we dispatch updatePlaying(false) then updatePlaying(true) back-to-back on the controls API on every replay. If createControls ever gets clever about transitions (animation, debounce, telemetry), this will fire spurious events. Not blocking — just worth knowing the seek-then-play sequence has this transient state inversion baked in.
• nit — packages/player/src/hyperframes-player.test.ts:1014-1060 — the new test uses _currentTime's implicit value via the _onMessage posting (frame 120 / 30fps = 4s). Asserting expect(player._currentTime).toBe(4) after the message would make the test self-documenting and catch any future drift in the frame→seconds conversion. Optional.
• nit — PR description's manual test plan checkbox is unchecked. Worth ticking before merge given this is a UX-visible fix on a path that's hard to fully cover in unit tests (real iframe runtime + real seek round-trip).

Notes for future work (not blocking)

• The same _currentTime >= _duration test exists in two places now (play() and _onMessage completedPlayback). If a third caller ever needs it, extract a _isAtEnd() predicate. One duplication is fine; two would be a smell.
• Worth considering whether pause() at end + play() should also seek to 0 (HTMLMediaElement does — pause then play at end replays). The current guard handles this since pause() doesn't move _currentTime, so already correct. Good emergent behavior from putting the guard in play().

— Vai

[jrusso1020]

LGTM — surgical 3-line guard in the right place, both paths pinned by tests.

A few distinct observations from a full read of hyperframes-player.ts (not blocking — passing them along since I dug into the surrounding paths):

End-detection symmetry — good. The new guard's this._duration > 0 && this._currentTime >= this._duration (line 405) matches the exact condition used in _onMessage at line 830 to decide completedPlayback. That symmetry is what you want — both paths agree on "ended" by the same predicate, including the _duration > 0 guard against the not-yet-loaded case. If one ever drifts (e.g., adding an epsilon on one side), drift them together.

No conflict with loop = true auto-restart. I traced the loop case to confirm: when loop = true and the iframe reports completedPlayback, _onMessage (lines 832–838) already calls seek(0) then play() itself, so by the time the user could press play manually the parent's _currentTime is back at 0 — the new guard is a clean no-op there. The fix is genuinely scoped to "user manually paused near end → presses play" and "non-looping reached end → presses play."

Minor: controls-state ping-pong inside play() after the new guard fires. seek(0) (line 450) internally calls controlsApi?.updatePlaying(false) and controlsApi?.updateTime(0, _duration) before play() continues to controlsApi?.updatePlaying(true). Synchronous, so today it's invisible. Worth keeping in mind if controlsApi ever batches or animates these transitions — an updatePlaying(false) → updatePlaying(true) pair on the same tick could become a visible flicker. Not worth fixing preemptively.

Optional follow-up (not in scope): the replay-after-ended test asserts _paused === false and seek was called with 0, but doesn't assert the resulting play event fires (the earlier loop end-state handling block has examples of pinning event dispatch via a vi.fn() listener). Useful if external listeners ever subscribe to play to gate audio/analytics — but the existing assertions cover the core contract, so this is "nice to have," not "missing."

Approving on the basis that the fix is mechanically minimal, the regression case (mid-playback play) is pinned, and the loop-mode interaction is benign.

Review by Rames Jusso (pr-review)