fix(player): drive src URL timelines without runtime#673
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: Comment (request a small follow-up before merge — see Important #1). Solid, well-isolated fix for #672. The design (separate DirectTimelineAdapter typed branch in _resolvePlaybackDurationAdapter, runtime-bridge takes precedence) is right, the tests cover the happy paths, and the runtime-bypass guard is exactly the right invariant. One real correctness gap on seek() mid-playback in direct-timeline mode, plus a few audit-all-sites consistency items.
Architecture & scope
Right call splitting this from #671 — different layer (packages/player vs packages/core/runtime), different repro, different surface. They share a bug-class theme (clock/transport correctness) but bundling would have made #671 harder to land cleanly.
The shape of the fix is good:
DirectTimelineAdapteris a properly narrowed type with its ownis*guard at the trust boundary (untrusted iframe globals).- Runtime bridge takes precedence:
_resolveDirectTimelineAdapterFromWindowreturns null if__hfor__playerexists. The "does not bypass" test pins this invariant. _resolvePlaybackDurationAdaptercleanly replaces the inlinegetAdapterin the probe — readable refactor with no behavior change for runtime-backed compositions.
Blockers
None.
Important
-
seek()while playing in direct-timeline mode silently pauses the parent but leaves GSAP running —packages/player/src/hyperframes-player.ts:505-527._tryDirectTimelineSeekcallstimeline.seek(t)and returns.seek()then runs_stopDirectTimelineClock()and_paused = true. GSAP'sseek()does not pause a playing timeline, so the iframe keeps animating while the parent reportspaused === true, freezes_currentTime, and stops emittingtimeupdate. The runtime/postMessage paths happen to converge to a paused state on the iframe side; the direct-timeline path does not. Failure mode: a scrub during playback leaves the iframe galloping past the scrub target while the controls bar is frozen at the scrub value. Why: the rest of the player's API contract is "seek lands paused" (see the comment at line 459-461 of base, preserved in PR). To match,_tryDirectTimelineSeekshould also calltimeline.pause()aftertimeline.seek(t). Add a unit test that assertstimeline.pauseis called when seeking while a direct-timeline play is in flight. -
End-of-playback in
_startDirectTimelineClockdoesn't pause parent media —hyperframes-player.ts:931-942. The non-loop branch doestimeline.pause(); _paused = true; dispatch("ended"). Compare to the postMessage end-of-playback path at lines 1071-1075, which also runsif (this._audioOwner === "parent") this._pauseParentMedia(). Why this matters per the audit-all-sites rule: today, direct-timeline mode is mutually exclusive with parent-audio promotion (promotion only fires onmedia-autoplay-blocked, which a standalone GSAP composition never posts). So this is latent today, not exploitable. But the next person who lets a direct-timeline composition acquire parent media (e.g. viaaudio-srcattribute, which works orthogonally to runtime presence) will hit a silent-mute-leak bug exactly like #671's end-of-playback miss. Mirror the existing pattern — one line, defends the invariant going forward. -
audio-src+ standalone GSAP composition will play audio uncoupled from the timeline. Not a regression introduced by this PR, but worth flagging as a known limitation now that this code path is real:_setupParentAudioFromUrlcreates a parent<audio>driven byplay()/pause()/seek()mirrors. In direct-timeline mode there's no rate sync, no per-frame mirror, and no end-of-playback pause for parent media (see Important #2). Either document "direct-timeline mode is timeline-only, audio-src is unsupported" in a code comment, or add an explicit warn-and-noop when both are present. Don't ship this silently. -
playbackRate/volume/mutedare no-ops in direct-timeline mode —hyperframes-player.ts:395-417. These send_sendControl(...)to a postMessage listener that doesn't exist in standalone GSAP compositions. The fix is out of scope for "make playback work" but the divergence from<video>semantics will surprise consumers. Minimum: add a TODO and a code comment so this isn't a year-long mystery. Better: routeplaybackRatethroughtimeline.timeScale()when a direct-timeline adapter is cached (GSAP timelines support it natively).
Nits
-
_stopDirectTimelineClock()is called unconditionally inseek()— line 510. Harmless because the postMessage/sync paths can't have a running rAF, but if you later allow mixed modes, this becomes confusing. Move the call inside the_tryDirectTimelineSeektruthy branch, or just leave a one-line comment explaining why it's safe to call always. -
Three near-identical
_tryDirectTimelineXmethods — lines 865-899. Each doesresolve → try { call → cache → return true } catch { return false }. Consider:private _withDirectTimeline<R>(fn: (t: DirectTimelineAdapter) => R): R | null { ... }
Optional; current shape is readable.
-
_resolveDirectTimelineAdapterFromWindowre-walks the DOM on every call — line 976-979.querySelector("[data-composition-id]")runs on every play/pause/seek when the adapter isn't cached yet. The probe loop already finds and caches the adapter; in practice this is hit at most once before the first probe completes. Not worth a fix, but worth a comment so a future reader doesn't pessimize the hot path. -
Try/catch swallows errors silently — lines 859-863, 871-874, 883-886, 894-898, 912-916. If
timeline.seekthrows (corrupted adapter, GSAP mid-teardown, etc.), we fall through to postMessage which also won't work. Worth aconsole.warnat minimum; debugging this in the field with no telemetry will be painful. (No telemetry layer to call out, but at least leave breadcrumbs.)
Praise
- Tight type narrowing at the iframe trust boundary.
isDirectTimelineAdapterchecks all five required methods before casting. This is exactly right for an untrusted-globals adapter — noasshortcuts. - The "does not bypass an installed runtime bridge" test pins down the most important invariant of this PR (runtime-owned compositions stay runtime-owned). Adding it deliberately, with the bridge stubbed alongside the timeline, is the kind of test that prevents the next person from re-introducing the bug.
- Probe-time adapter caching at line 1169-1170 avoids re-resolving on every play/pause/seek and keeps the adapter and
_durationin lockstep — clean. - PR description is exemplary — root cause, before/after JSON evidence from agent-browser, named QA artifacts. This is the bar.
- CI: 27/27 green, including the player perf suite (drift, scrub, parity, fps). Reassuring on a player-internals change.
— Vai
jrusso1020
left a comment
There was a problem hiding this comment.
Architecture is right. The direct-timeline adapter is the correct surface for src=URL standalone GSAP compositions, and the runtime-bridge precedence is preserved. Reproducer in #672 (player.currentTime = 2 from a clean page) is fixed. One state-machine contract violation worth fixing before stamp.
What I verified against the issue and source
Issue #672 repro is fixed. Tanuja's case is player.currentTime = 2 from a fresh page — no play() first. Path:
seek(2)→_trySyncSeekreturns false (no__player.seek) →_tryDirectTimelineSeekfinds the cached adapter from probe, callstimeline.seek(2)→ returns true. ✓_currentTime = 2, controls UI updates.- The GSAP timeline was never started (initial state from
paused: trueor simply un-started), soseek(2)jumps to t=2 and stays there. The#titleopacity transition lands at the t=2 keyframe state. ✓
Runtime-bridge precedence is correct. _resolveDirectTimelineAdapterFromWindow short-circuits via _hasRuntimeBridge(win) (checks __hf and __player) before resolving the timeline. Verified the third test (does not bypass an installed runtime bridge for direct __timelines playback) exercises this — when __player: { play, pause } is present, postMessage is used and the direct timeline stubs are untouched. ✓
Lifecycle cleanup of _directTimelineRaf is honored across the four transitions:
play()→_startDirectTimelineClock()✓pause()→_stopDirectTimelineClock()✓_onIframeLoad()→ resets_directTimelineAdapter = nulland stops clock ✓disconnectedCallback→ stops clock and resets adapter ✓- End-of-playback in
tick()→_directTimelineRaf = nullafterendedevent ✓
Refactor of _resolvePlaybackDurationAdapter out of inline getAdapter is clean — the duration probe and the playback path now share one resolver, so a future refactor can't split the precedence rules between them.
Blocker — _tryDirectTimelineSeek doesn't pause the underlying timeline
The player's existing seek() contract is "leave the player paused at the seek target" — this is established at hyperframes-player.ts:524 (this._paused = true at the end of seek) and was honored pre-PR by both _trySyncSeek (runtime owns the pause) and the postMessage path (runtime owns the pause). The new direct path breaks this:
private _tryDirectTimelineSeek(timeInSeconds: number): boolean {
const timeline = this._directTimelineAdapter || this._resolveDirectTimelineAdapter();
if (!timeline) return false;
try {
timeline.seek(timeInSeconds); // ← GSAP seek() does NOT pause; preserves play state
this._directTimelineAdapter = timeline;
return true;
} catch {
return false;
}
}Per GSAP docs for Timeline.seek(time, suppressEvents): "If the animation was paused, it remains paused. If it wasn't paused, it remains playing."
So in the direct path, this sequence — which is reachable from the controls UI (play button + scrubber drag) — produces a divergent state:
- User clicks play →
play()→_tryDirectTimelinePlay→timeline.play()→ GSAP playing,_paused = false, rAF running. - User drags scrubber to t=2 →
seek(2)→_tryDirectTimelineSeek→timeline.seek(2)→ GSAP keeps playing from t=2 onward. - seek() then sets
_paused = trueand_stopDirectTimelineClock()cancels the rAF. - Now: GSAP timeline is advancing (animations visible in iframe), parent state says paused, currentTime frozen at 2 in the controls UI. Eventually GSAP hits the end and stops, but parent never sees it.
User sees: clicked pause/scrubber, animations didn't stop. The exact same shape of failure as #672, just one interaction sequence away.
This is the cleanup-contract case — the new direct transport must honor the same player-API contract the existing transports do. The fix is one line:
private _tryDirectTimelineSeek(timeInSeconds: number): boolean {
const timeline = this._directTimelineAdapter || this._resolveDirectTimelineAdapter();
if (!timeline) return false;
try {
timeline.seek(timeInSeconds);
timeline.pause(); // Match player.seek() contract: leaves the timeline paused.
// GSAP timeline.seek() does not pause a playing timeline; we
// need an explicit pause() to keep parent state and underlying
// state coherent. (Or: switch to GSAP's `pause(time)` form,
// but the adapter type currently models pause as zero-arg.)
this._directTimelineAdapter = timeline;
return true;
} catch {
return false;
}
}And a test in the same shape as the existing direct-mode tests:
it("pauses the timeline on seek to keep parent and underlying state coherent", () => {
const timeline: TimelineStub = {
duration: vi.fn(() => 5),
time: vi.fn(() => 0),
seek: vi.fn(),
play: vi.fn(),
pause: vi.fn(),
};
stubContentWindow({ __timelines: { main: timeline } });
player.play();
player.seek(2);
// Pin: GSAP timeline.seek() doesn't pause a playing timeline, so the
// direct-mode seek must explicitly pause to match player.seek()'s
// `_paused = true` contract. Otherwise iframe animations advance while
// the parent thinks playback is stopped.
expect(timeline.seek).toHaveBeenCalledWith(2);
expect(timeline.pause).toHaveBeenCalled();
});Smaller observations (non-blocking)
playbackRate isn't propagated to direct mode. The player exposes playbackRate as an attribute (line 503-508). Runtime mode presumably maps it to gsap.timeline().timeScale(rate). Direct mode never reads this.playbackRate and doesn't call timeline.timeScale(rate), so player.playbackRate = 2 is a no-op in direct-timeline mode. Probably fine for v1 of this fix — not part of #672 — but would be worth a TODO comment near _tryDirectTimelinePlay so a future bug report has the trail.
Loop branch in tick() at the seek/play boundary works only because of seek's pause-and-restart sequence. if (this.loop) { this.seek(0); this.play(); } works correctly today because seek(0) sets _paused = true and stops the clock, then play() flips back to _paused = false and restarts the clock. If anyone simplifies seek's clock handling later (e.g. "seek shouldn't stop the rAF"), this loop branch breaks subtly. Not a current bug, just a fragile coupling worth a one-line comment.
_tryDirectTimeline* swallow errors silently with bare try { } catch { return false; }. A malformed timeline that throws on every method call will degrade gracefully but produce no diagnostic — the user sees "play does nothing" with no console signal. Worth at least one console.warn on first failure (debounced if necessary), since the path is opt-in for users embedding their own GSAP-shaped objects.
Praise
- Type narrowing via
isDirectTimelineAdapterandisRuntimeDurationAdapteris the right shape —Reflect.get+ structural check + tagged union (PlaybackDurationAdapter). Compiles down clean and gives the reader explicit intent at each branch. - Composition-id resolution at
_resolveDirectTimelineAdapterFromWindow:973-979(root[data-composition-id]lookup, fall back to last key) correctly handles the bundled-preview case where multiple compositions are registered andObject.keysorder would otherwise pick a sub-composition's duration. - Probe-loop adapter caching (
adapter.kind === "direct-timeline" ? adapter.timeline : null) means subsequent_tryDirectTimeline*calls don't re-probe — the lazy||resolution is purely a fallback for the iframe-reload race. - "Does not bypass installed runtime bridge" test is exactly the kind of contract-pin that prevents future refactors from breaking the precedence rule.
- PR description cleanly enumerates the three transports (
__player.seek/ direct timeline / postMessage) and the verification matrix.
Summary
Architecture: ✅. Issue #672 repro fixed: ✅. Runtime-bridge precedence and lifecycle cleanup: ✅. One-line state-machine contract fix needed before stamp: pause the timeline on seek so parent + GSAP stay coherent across the play→seek interaction. Will re-stamp once that lands + a test pin.
(Vai may flag adjacent things in parallel — only my own audit so far.)
— Review by Rames Jusso (pr-review)
vanceingalls
left a comment
There was a problem hiding this comment.
Approve. Both findings cleanly resolved in db2fe66c.
Headline (_tryDirectTimelineSeek pause-after-seek):
timeline.seek(timeInSeconds);
+ // GSAP seek() preserves play state; the player seek() contract lands paused.
+ timeline.pause();
this._directTimelineAdapter = timeline;Exact one-liner with a load-bearing inline comment explaining the contract. The new test at hyperframes-player.test.ts:924-944 is the right shape — play() → clear the pause mock → seek(2) → assert both timeline.seek(2) AND pause called, plus post not called (which pins that this stays in direct-timeline mode without leaking through the postMessage path).
Bonus that wasn't in my review (worth surfacing): the natural-end-of-playback branch at hyperframes-player.ts:940 now also calls _pauseParentMedia() when _audioOwner === "parent". That's the audit-all-sites discipline applied to a sibling state-machine path — same precondition (transport-pause), same cleanup contract, every site now agrees. Exactly the right instinct from the #671 lesson.
Three rounds of review on this PR, every finding addressed cleanly + a bonus correctness fix. Ship.
— Vai
jrusso1020
left a comment
There was a problem hiding this comment.
Round 2. Blocker is resolved cleanly. Approving.
Round-1 concerns: addressed
- ✅
_tryDirectTimelineSeeknow callstimeline.pause()aftertimeline.seek(timeInSeconds). Verified athyperframes-player.ts:866-872— the inline comment ("GSAP seek() preserves play state; the player seek() contract lands paused.") accurately captures the semantic mismatch this guards against. - ✅ New test
pauses same-origin __timelines after seek while playingpins the contract. Verified the call sequence:player.play()→pause.mockClear()→player.seek(2)should produce exactlytimeline.seek(2)+ onetimeline.pause(). Traced the production path:play()calls_tryDirectTimelinePlay(which callstimeline.play(), no pause);seek(2)calls_tryDirectTimelineSeek(which now callstimeline.seek(2)thentimeline.pause()). The test'sexpect(pause).toHaveBeenCalledTimes(1)correctly counts only the seek-induced pause. ✓ - ✅ The
_paused = truecontract fromseek()(line 524) is now honored by all three transports — runtime (_trySyncSeekdefers to__player.seekwhich the runtime pauses), postMessage (frame seek; receiver pauses), direct timeline (now pauses explicitly). Vai's "pick one rule and enforce at boundary" framing is honored: the rule is "seek lands the timeline paused," and every transport produces that post-condition.
Bonus catch in the same fix-up
End-of-playback in tick() now also calls _pauseParentMedia() when _audioOwner === "parent":
timeline.pause();
if (this._audioOwner === "parent") this._pauseParentMedia();
this._paused = true;This is a defensive consistency fix matching the pattern from pause(). Standalone __timelines compositions typically don't have parent-owned audio, but the parent-media path is wired up the same way regardless of the transition trigger now — explicit pause vs auto-pause-at-end vs end-of-direct-timeline-playback all converge on the same cleanup. Nice.
CI
Test, Typecheck, Build, Lint, Test: runtime contract, CLI smoke, all Perf:* jobs, Render on windows-latest, Tests on windows-latest, Preview parity, Smoke: global install, preview-regression, player-perf, CodeQL, render-compat — all green. regression-shards (visual regression on style-N-prod) still in progress, but those are unrelated to this code path and slow by design.
Carried-over follow-ups (still non-blocking)
Repeating the round-1 non-blockers for the trail — none of these need to land here:
playbackRateisn't propagated to direct mode (timeline.timeScale(rate)never called). Worth a TODO comment near_tryDirectTimelinePlayso a future user-report has the trail._tryDirectTimeline*swallow errors silently withtry { } catch { return false; }. Worth at least one debouncedconsole.warnon first failure since the path is opt-in for users embedding their own GSAP-shaped objects.
Summary
Issue #672 fixed; player-API contract honored across all three transports; one new test pins the contract; CI green. ✅
— Review by Rames Jusso (pr-review)
Problem
Fixes #672.
<hyperframes-player src="...">can load a same-origin standalone GSAP composition and parse its duration fromwindow.__timelines, but playback controls do not drive that timeline when the iframe never installs the HyperFrames runtime bridge.In the reported repro,
player.currentTime = 2updated the player element's cached time, whileiframe.contentWindow.__timelines.main.time()stayed at0and the visible title remained at its initial opacity.What this fixes
This PR adds a direct same-origin timeline adapter for standalone
window.__timelinescompositions when no runtime bridge is present.seek()now falls back fromwindow.__player.seek()to directtimeline.seek(seconds)before usingpostMessage.play()/pause()directly calltimeline.play()/timeline.pause()for standalone timeline-only embeds.currentTimewhile a direct timeline is playing by samplingtimeline.time()onrequestAnimationFrame.window.__hforwindow.__playeris installed, the player does not bypass the runtime to drive raw__timelines.Root cause
The iframe probe treated raw
window.__timelinesas a valid duration adapter and marked the player ready, but the control paths only talked to either:window.__player.seek()for same-origin runtime installs, orpostMessage({ source: "hf-parent", type: "control" })for runtime bridge installs.A standalone same-origin
src=URLcomposition with onlywindow.__timelineshas neither awindow.__playersync bridge nor a postMessage listener. The player could discover duration from the timeline but had no transport that actually drove it.Verification
Local checks
bun run --filter @hyperframes/player test— 107/107 passbun run --filter @hyperframes/player typecheckbun run --filter @hyperframes/player buildbunx oxlint packages/player/src/hyperframes-player.ts packages/player/src/hyperframes-player.test.tsbunx oxfmt --check packages/player/src/hyperframes-player.ts packages/player/src/hyperframes-player.test.tsgit diff --checkBrowser verification
Used
agent-browseragainst a local same-origin harness matching issue #672:<script type="module" src="/player.js">.<hyperframes-player src="/composition.html" shader-loading="none" controls>./composition.htmlregisters onlywindow.__timelines.mainand has no runtime bridge.Before the fix, reproduced the bug:
{ "afterSeek": { "playerTime": 2, "timelineTime": 0, "titleOpacity": "0", "hasRuntime": false } }After the fix, against the final rebuilt bundle:
{ "afterSeek": { "playerTime": 2, "timelineTime": 2, "titleOpacity": "1", "titleTransform": "matrix(1, 0, 0, 1, 0, 0)", "hasRuntime": false }, "afterPlay": { "playerTime": 2.484, "timelineTime": 2.5, "paused": false, "hasRuntime": false } }Agent-browser artifacts were captured locally:
qa-artifacts/issue-672/repro-before-fix.pngqa-artifacts/issue-672/fixed-after-seek-and-play.pngqa-artifacts/issue-672/fixed-flow.webm(14.2s)Notes
The QA artifacts are local-only and intentionally not committed. The initial recording pass exposed a broken local Homebrew ffmpeg/x265 linkage; I refreshed ffmpeg and reran the agent-browser recording successfully.