Repository navigation
fix(player): load compositions from document duration - #5176
miguel-heygen wants to merge 5 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
terencecho
left a comment
There was a problem hiding this comment.
Review at da357bf7163b3455550225b47248bcfa9543b3f3 — requesting changes for two readiness regressions.
-
packages/player/src/composition-probe.ts:154-170: The positive document duration can stop the probe before a standalone GSAP timeline has a positive duration, even when the registered timeline is the intended playback source. For example, a same-origin composition declaresdata-duration="6";__timelines.mainexists but reportsduration() === 0at the first 200 ms poll, then author initialization adds 9 s of tweens to that same object at ~700 ms. This head settles at 6 s and stops probing.hyperframes-player.ts:1149-1159only refreshes duration when the timeline object's identity changes, so an ordinary later Play still uses the stale 6 s player duration;direct-timeline-clock.ts:72-104clamps and stops the 9 s animation at 6 s. The base waited for the positive native duration and would resolve 9 s at the next poll, before the five-tick runtime-injection fallback. A timeline that stays at zero is also accepted as the direct playback adapter instead of reaching that fallback, so Play can report playing with no timeline progress. Please preserve the existing adapter/injection grace for an unresolved registered timeline, or refresh/reconcile the playback transport and duration when it becomes usable. The new test checks native precedence only when the native duration is already positive at the first tick, and its zero-duration case asserts readiness but not playback. -
packages/player/src/composition-probe.ts:122-125: The new timeout also fires while an injected runtime script is pending. If a nested composition injects the runtime at 200 ms but a slow CDN load produces itstimelinemessage at 9 s, this path emits anerrorat 8 s;runtime-message-handler.ts:151-176still calls_onRuntimeTimelineReady, whose only guard is_ready, so the same player later emitsready. The base kept waiting in the injected-but-not-yet-present branch instead of emitting this contradictory terminal error. If 8 s is a hard rejection, latch the failure and ignore a late runtime handshake; otherwise keep pending until injection resolves. Please test a delayed runtimetimelinepostMessage, not only a delayed__playerproperty.
The source paths above were traced at this exact head; I did not reproduce them in a full browser. The focused readiness tests and passing checks do not cover either transition; the required Windows render check was still pending at review time.
— tai
|
Addressed at be3d5c8. Registered zero-duration GSAP timelines retain the existing initialization grace, then runtime injection handles a timeline that stays empty. The same-object growth case resolves the native nine-second duration and plays past eight seconds in Chrome. Timeout and author-error failures belong to the failed document; late runtime messages cannot promote it, including while its replacement is navigating. A distinct same-origin document can handshake before load. The accepted opaque/cross-origin recovery limit is stated in the PR body. Regression tests cover both review cases, old-document messages during navigation, replacement readiness before load, and opaque load-event recovery. Focused tests pass three runs with no skipped tests; build, types, lint, formatting, comment checks, and audit pass. The refreshed After capture and real-browser runs use this head. |
Edit accuracy: accurate 2059 (base branch 2059), smooth 1584 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencecho
left a comment
There was a problem hiding this comment.
Review at be3d5c830619061ee4b106f8e416c8d90606e3e7 — approved.
The two readiness regressions in my prior review at da357bf7 are resolved in this head. A registered zero-duration direct timeline remains pending during its initialization grace instead of settling at the document's six-second duration; the new same-object growth test observes the native nine-second duration. A terminal probe timeout or retained author error now latches the failed document, and the Player rejects its late timeline handshake; the new tests exercise timeout, navigation, and replacement-document recovery. I traced the effective diff and production message/probe paths. The checks on this head report no failing or pending jobs.
I ran focused Chromium navigation-order probes but not the local test suite for this re-review. The PR's stated scope remains probe readiness, not standalone-document playback controls or universal interception of runtime-present script errors.
— tai
The composition probe rejects valid documents without a usable adapter, and pending runtime injection can bypass its timeout. It now uses the existing document-duration resolver, surfaces the first retained author error, and keeps pending paths within eight seconds. A registered empty GSAP timeline retains its initialization grace and runtime-injection fallback; a positive native duration wins. Failed documents cannot accept a late runtime handshake. No authoring format changes.
Validation at
be3d5c83: 17 readiness tests and five terminal/recovery tests pass in three runs, with no skipped tests. Regression witnesses fail on the relevant earlier heads. The existing 248 player tests, player production build and typechecks, full lint, formatting, comment checks, and audit pass. Independent source review approved this head.Real Chrome on this head verifies a timeline growing from zero to nine seconds and playback past eight seconds, an empty timeline loading through the runtime with a six-second document duration and advancing playback, rejection of a nine-second handshake after the eight-second timeout, and same-origin recovery while the replacement document is still loading. The old failed document cannot announce ready during navigation.
Failure recovery is Document-scoped for same-origin previews; opaque/cross-origin failures remain latched until the next iframe load, so replacement pre-load handshakes may be ignored.
Before
A real Studio source edit with the edited shadow's runtime fetch fault-injected reproduces the generic eight-second reload timeout. The previous preview stays visible.
After
The same edit on this head reports
missingScene is not definedpromptly. The previous preview remains visible.Scope: these changes govern probe readiness. They do not add playback controls for a standalone document without a runtime or timeline. Existing runtime handshakes and Studio adapter selection can establish readiness before probing, so runtime-present script-error reloads are not universally covered by this fix.