fix(player): gate play on real composition readiness, not just duration - #4054

Reviewed — core design is sound, but one real (nonblocking, should-fix) bug in the queue-play lifecycle, and it's untested. Holding my stamp on it; CI is all-pending so it can't merge yet regardless. Manual pass (Codex disabled per workspace cap).

Bug: a play queued while assets buffer is not cancelled by pause()/seek(), so it fires against the user's pause when assets settle.
Sequence (heavy comp — the exact scenario this PR targets):

1. Assets still buffering. User presses play → if (this._ready && !this._assetsReady) { this._pendingPlay = true; return; } (hyperframes-player.ts:364). Nothing plays; overlay shows.
2. User presses pause (or seek). Both set _paused = true but neither clears _pendingPlay nor calls _invalidateAssetsWait (that's wired only to _ready = false sites).
3. Assets settle → _settleAssetsReady → if (this._pendingPlay) this.play() (:1062), and play() has no _paused guard — it sets _paused = false and starts playback + dispatches play.

Result: the composition plays even though the user paused/seeked while it buffered. The buffering window is precisely the heavy-comp case this PR exists for, so it's reachable, not theoretical.

The same shape applies to the pre-existing runtime-readiness queue (_onProbeReady/_onRuntimeTimelineReady also do if (… || _pendingPlay) this.play() with no _paused guard), so the cleanest fix is at the cancel sites: clear _pendingPlay = false in pause() and seek() — one place, covers both queue paths. (Guarding each retry with !this._paused also works but is the enumerate-every-site version.)

Untested: the asset-ready gate tests (:2765+) cover doc === null, defer-then-play, and the 8s timeout — none exercises pause/seek during the pending window. A test that queues a play, pauses, then settles the asset and asserts isPlaying() === false (and no play event) would fail today.

Verified sound:

• Core compositionReadiness module is well-built — realm-aware asset scan (cross-realm iframe instanceof handled), settles on canplay-or-error (a failed media element can't block), 8s timeout fail-opens so play never hangs, onSettled fires exactly once, synchronous callback when nothing's pending (no regression for ready comps).
• Parity holds — player and Studio both consume the one @hyperframes/core/composition-readiness module. Studio's readinessSlice guards the async settle with a module-scope generation counter so a superseded wait can't set timelineReady for stale content; the player's _waitForAssetsReady uses the same generation pattern (_assetsGeneration) and _invalidateAssetsWait on teardown.

Will stamp once the pending checks are green and the pause/seek-cancels-pending gap is closed (or we agree it's a fast-follow).

Requesting one change at 41704f94e89de3d531ebb43d5e7c1c38665c4298. The core module, the Studio slice and the export wiring are all sound — the single problem is in the player's queue-play lifecycle, and it fires in exactly the window this PR creates.

The change

pause() and seek() don't clear _pendingPlay, so a queued play survives an explicit user pause and then starts playback anyway.

_pendingPlay is set in two places (hyperframes-player.ts:365, :388) and cleared in only two: the top of play() (:368) and _invalidateAssetsWait() (:1069). _invalidateAssetsWait() is called from navigation, disconnect, shader-option change and iframe reload — never from user transport control. pause() (:405) and seek() (:421) both set this._paused = true and neither touches the flag.

So, on a heavy composition inside the up-to-8s asset wait:

1. _ready is true, _assetsReady is false. play() early-returns at :364 and parks _pendingPlay = true. Nothing visibly starts — _paused stays true and controlsApi.updatePlaying is never called.
2. The user pauses, or scrubs. pause() dispatches pause; seek() honours its own documented contract that a seek lands paused. Either way the host now believes playback is stopped, and _pendingPlay is still true.
3. Assets settle. _settleAssetsReady (:1062) calls this.play() with no _paused guard, which falls straight through the :364 early-return (_assetsReady is now true), sets _paused = false, sends play to the runtime and dispatches the public play event.

The composition starts playing against the user's explicit pause, seconds after they stopped it. It's small and it's recoverable, but it makes pause() non-authoritative during the feature's own target scenario, and it is new behaviour — before this PR there was no queue for a pause to lose a race against.

The fix is one place, two lines — clear the flag wherever the user takes control:

  pause() {
    this._pendingPlay = false;
    if (!this._tryDirectTimelinePause()) this._sendControl("pause");

and the same first line at the top of seek(). seek() needs it as much as pause() does, since seek's contract is that it lands paused.

What I verified at source

• The ready-ordering the whole gate depends on is correct. Both _onRuntimeTimelineReady (:983-987) and _onProbeReady (:1001-1005) set this._ready = true before calling _waitForAssetsReady, so by the time the trailing if (autoplay || _pendingPlay) this.play() runs, play()'s :364 guard can actually see the pending state. If those were ordered the other way this gate would be a no-op on the autoplay path.
• The generation guards hold. In the player, every _ready = false site calls _invalidateAssetsWait() first, and _settleAssetsReady is idempotent (:1060). In Studio, timelineReadyGeneration is bumped on requestTimelineReady, on setTimelineReady, and in resetPlaybackReadinessState() — which createTimelineResetState() spreads, so beginTimelineSession invalidates too. I could not construct a stale settle that lands in a superseded project.
• The synchronous-settle claim is real, and it matters: settleCompositionReadiness calls back before returning when nothing is pending, which is why _waitForAssetsReady can check if (!this._assetsReady) afterwards and skip the loading overlay for the common case rather than flashing it.
• The export wiring is complete and the repo's own gates agree. ./composition-readiness is added in all three required places (dev exports, publishConfig.exports, package-subpaths.json), consistent with the neighbouring ./mediaTiming entry, and tsc emits dist/compositionReadiness.js with no build-entry registration needed. I ran the gates rather than eyeballing it: check:package-subpaths → "Package subpaths verified: 12 public workspaces"; check:package-cycles → no new cycle; check:workspace-contracts → clean; scripts/package-subpaths.test.mjs → 4/4.
• packages/core compositionReadiness.test.ts → 7/7, matching the test plan.

What I could not run, and the false finding I nearly filed

I could not run packages/player's suite, so the "376/376" and "1,413/1,413" claims are unverified by me.

This one is worth spelling out because it looked like a finding. packages/player/vitest.config.ts carries a hand-maintained alias list, and it covers exactly three specifiers — @hyperframes/core/composition-contract, /slideshow, /runtime/protocol. Those are precisely the three @hyperframes/core subpaths the player imported before this PR. This PR adds a fourth import, @hyperframes/core/composition-readiness, and does not add a fourth alias — and the suite fails for me with Missing "./composition-readiness" specifier in "@hyperframes/core" package, 143 of 154 red. That reads like an exhaustive list with one entry missing.

It isn't. I ran the control before writing it up: in my environment all four of those specifiers fail to resolve, including the three that are fine on main. My borrowed node_modules has no workspace link for @hyperframes/core at all, so nothing resolves through its export map; the alias list was simply masking that for the other three and left the new one exposed. So this is my rig, not your branch, and I am not reporting it as a defect.

The residual, stated as a question rather than a finding: I have no evidence either way about whether that alias list is load-bearing in CI. If your local run of the player suite was against a full bun install, export-map resolution answered and there is nothing to do. Worth one glance, no more.

Non-blocking

1. collectPendingCompositionAssets leaks its media listeners on the timeout path (compositionReadiness.ts:49-60). onSettled is the only thing that removes the canplay/error pair, so when settleCompositionReadiness resolves through the 8s timeout instead, those listeners stay attached and the per-element promise stays pending. Bounded and low-impact — but the elements it leaks on are exactly the stuck ones you're already warning about.
2. _onProbeReady has no if (this._ready) return guard, while _onRuntimeTimelineReady (:984) does. That asymmetry predates this PR, but it now has a wider blast radius: a late probe callback arriving after the runtime path already won would re-enter _waitForAssetsReady, bump the generation and re-show the loading overlay on a composition that had already settled.
3. Everything else in the pre-review table I audited independently and agree with; the readiness slice's module-scope generation counter is the right call for state no component renders from.

Happy to re-read as soon as _pendingPlay is cleared on the user-control paths — that alone clears my block.

— Rames

Approving — this clears my 5244505964.

Delta against the head I blocked (41704f94..39e58ba8): ahead 1, behind 0, two files, +90/−0, of which the product change is two lines.

The fix is the one I asked for, and it is in the better of the two possible places. this._pendingPlay = false at the top of pause() (:409) and seek() (:429) — at the flag rather than at a fire site. That matters more than it looks, because the queued play has three fire sites and my review named one. _settleAssetsReady:1070 was the one I traced; _onRuntimeTimelineReady:995 and _onProbeReady:1013 also do if (this.hasAttribute("autoplay") || this._pendingPlay) this.play(). Clearing at the source covers all three, including the two I never mentioned.

I checked those two for the same bug arriving through the other half of that condition — user pauses during the wait, a settle later plays anyway because autoplay is still on the element — and it is not reachable. Both are load-time readiness handlers (_onRuntimeTimelineReady returns early on this._ready, and both set it), so they run before there is anything for a user to pause, and a composition swap goes through _invalidateAssetsWait(), which clears the flag and then legitimately re-autoplays the new composition.

The new tests discriminate, and I proved it by mutation rather than by reading them. With the fix in place the asset-ready gate block is 9 passed. With only the two assignments removed and the test file untouched: exactly 2 failed, 7 passed — the two failures being the pause case and the seek case. The third new test ("settles a play() called twice while buffering into exactly one playback start (pre-existing idempotency, not the pause/seek cancel)") stays green under the mutation, so its name is honest: it documents behaviour that predates this commit instead of quietly claiming the new one.

One environment note for whoever runs this next: the block fails 9-for-9 with Failed to resolve import "@hyperframes/core/composition-readiness" until packages/core is built, because that export's node condition resolves to dist/compositionReadiness.js and a fresh bun install does not produce it. Every arm failing — including the pre-existing tests — is the only reason I read that as my own rig rather than as the branch.

Review by Rames