fix(player): treat runtime timeline as cross-origin ready#1690
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Read all 5 files end-to-end + cross-referenced _onProbeReady for the same-origin parity check. The fix is well-designed and the body claims verify. Posting as COMMENT and routing the stamp decision back through Jerrai per the delegation note (Home offered to handle the APPROVE click after my review).
Audited against Home's 5 concerns
1. Replay-state correctness ✓ — _onRuntimeTimelineReady explicitly calls this._replayBridgeState() AFTER setting _ready = true. Bridge state isn't dropped; it's replayed via the existing mechanism that mute/volume/playback-rate already use. The _ready early-return at the top makes the call idempotent — multiple timeline messages don't double-fire.
2. Autoplay honoring ✓ — this._setIframeMediaMuted(this.muted) runs BEFORE this.play(), which satisfies the browser's muted-autoplay allowance. The new honors autoplay after cross-origin runtime timeline readiness test pins this. Sequence ordering matters here (mute-then-play, not play-then-mute) and the diff has it right.
3. Backwards-compat — one design-intent question worth surfacing: in same-origin mode where the runtime timeline message arrives BEFORE _onProbeReady completes, the new path sets this._directTimelineAdapter = null unconditionally. Compare with _onProbeReady which sets it via adapter.kind === "direct-timeline" ? adapter.timeline : null. Net effect: a same-origin composition where the runtime timeline beats the probe forfeits the direct-adapter seek path and falls back to message-passing seek instead.
This is likely intentional (cross-origin is the new canonical path; direct adapter only matters when same-origin probe wins the race), but the PR body doesn't name it. A one-line code comment — "direct-adapter is forfeited when the runtime path wins the race; same-origin compositions where the probe completes first still set it via _onProbeReady" — would lock in the design intent and prevent a future reader from "fixing" what they perceive as a same-origin regression.
4. Test coverage ✓ on the success + autoplay paths. One missing path worth adding: the timeout-never-arrives fallback. If the iframe is cross-origin AND the runtime never posts a timeline message, the player stays not-ready until the same-origin probe times out — and the probe will time out because cross-origin reflection is exactly what the runtime-message path is meant to replace. Worth a test pinning that the probe's timeout error path still surfaces correctly so a hostile or buggy iframe doesn't silently leave the player in a permanent loading state. Per feedback_audit_try_catch_scope_and_cross_site_applicability_on_new_helper_prs, that's exactly the kind of escape-the-protection question that asymmetric paths need a test for.
5. CSP claim — the OSS player PR enables Pacific to drop the CSP weakening; doesn't perform it. The cleanup is a Pacific-side follow-up. Body wording ("Lets Pacific use the signed streamed_url directly instead of weakening app CSP or blob-wrapping user HTML") reads correctly — it's an enabling change. Verification that no Pacific caller still requires the old workaround would be done at the Pacific PR side; out of scope for this review.
Two parity differences I noticed vs _onProbeReady
_onRuntimeTimelineReadydoesn't computecompositionSize/_rescale()— the runtime timeline message doesn't carry size info (its shape is{ durationInFrames, scenes }). Probably fine for cross-origin CDN-served compositions that are fixed-size; worth confirming this isn't a missing capability for the cross-origin user._onRuntimeTimelineReadycalls_replayBridgeState(),_onProbeReadydoesn't. TheonRuntimeReadycallback already triggers_replayBridgeState()separately, so calling it here means it can run twice for the same iframe (once ononRuntimeReadygeneral signal, again on the timeline message). Looking at the bridge-state replay shape (re-sending mute/volume/playback-rate control messages), this should be idempotent — but worth a quick sanity check that the duplicate call doesn't cause visible UI flicker.
Body claims verified
- "+89/-0" ✓
- "5 files" ✓
- "Failed-first regression" — the new test would fail before the player change because
_onRuntimeTimelineReadydidn't exist, so the timeline message just updated playback state without triggering ready. ✓ - "Add a runtime timeline-ready callback in the player message handler" — verified at
runtime-message-handler.ts:115-119(callbacks.onRuntimeTimelineReady(duration)invocation inside thetimelinemessage handler, only whendurationInFramesis finite and positive). - "Mark the player ready from the first finite runtime timeline duration" — verified via
if (this._ready) returnearly-return at the top of_onRuntimeTimelineReady. Only the first arrival sticks. - "Stop the same-origin composition probe once runtime readiness is confirmed" —
this.probe.stop()is called in_onRuntimeTimelineReady. ✓ - "Replay bridge state and honor
autoplayafter cross-origin runtime readiness" — both verified in the function body. ✓
CI
Re-pulled at post-time: 20 SUCCESS / 13 IN_PROGRESS / 0 FAILURE / 4 SKIPPED / 1 NEUTRAL. No failures; remaining checks normal-in-progress.
Stamp posture
Miguel is trusted-stamper; per the delegation Home explicitly clarified "no bot-author hold." Code reads clean on merit; the two notes above are non-blocking (design-intent comment + a timeout-fallback test). Routing the stamp click to Jerrai per the delegation's offer — LGTM, please stamp once you've seen this review.
Review by Jerrai
Summary
Fixes cross-origin HyperFrames playback in
<hyperframes-player>by treating the runtimetimelinepostMessage as the readiness signal.This removes the need for embedders to fetch composition HTML into an app-origin
blob:just so the player can inspect same-origin iframe state. That blob workaround makes generated HyperFrames HTML inherit the host app CSP, which blocks the inline scripts that build and register GSAP timelines.What changed
autoplayafter cross-origin runtime readiness.Validation
readystayedfalsebefore the player readiness patch.bun run --filter @hyperframes/player test -- src/runtime-message-handler.test.ts src/hyperframes-player.test.ts src/slideshow/hyperframes-slideshow.test.tsbun run --filter @hyperframes/player testbun run --filter @hyperframes/player typecheckbun run --filter @hyperframes/player buildbunx oxfmt --check packages/player/src/runtime-message-handler.ts packages/player/src/runtime-message-handler.test.ts packages/player/src/hyperframes-player.ts packages/player/src/hyperframes-player.test.ts packages/player/src/slideshow/hyperframes-slideshow.test.tsNote: slideshow tests emit existing localhost retry stderr, but the test process exits successfully.