Repository navigation
fix(core): stop script loading time from shifting WAAPI frames - #5017
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
I'm commenting rather than approving because the code is right but this sets a timing contract, and a maintainer should agree to that contract first.
The code (looks correct)
packages/core/src/runtime/adapters/waapi.ts:57-73: the zero anchor applies only on first discovery, at composition time 0, to running or finished animations withplaybackRate === 1ondocument.timeline. CSS animations and transitions are excluded. Paused, idle, other-timeline, late-created and rediscovered animations keep their old baselines.waapi.ts:155-159movesdidDiscover = trueafter the initialtrackAnimations. I checked whether this changes the oldnormalizeInitialAnimationTimeMspath. The firstrunAdapters("discover")ininit.tsruns at time 0, and rediscovery runs withdidDiscoveralready true. Only one case changes: an animation created after a pre-discoverseek(t>0)but before discover now keeps its raw time instead of the normalized one. I don't think the runtime reaches that case today. Mentioning it only so it's a known edge.
The decision for a maintainer
A running animation that had currentTime assigned before the runtime loads no longer keeps that value as an offset. The skill doc (skills/hyperframes-animation/adapters/waapi.md) now directs authors to delay or to pause() + currentTime. I agree it can't be told apart from load time. But it changes behavior for existing compositions, and the issue explicitly left "where script-created animations start" as an open decision. A maintainer should sign off on that contract before this merges.
What I ran
vitest run src/runtime/adapters/waapi: 43/43 pass.- Mutation check: with the base
waapi.tsrestored, 9 of the new adapter cases fail (wall-clock progress at 16/90/300/1500 ms, seek/pause before discover, finished rewind, repeated discovery, revert). - Not run: the 9 real-Chrome cases in
coreRuntimeBrowser.test.ts. CI hasn't run them either, because the fork workflows still need approval.
— Rames
jrusso1020
left a comment
There was a problem hiding this comment.
This needs a rebase, but only for a docs file. The code still merges cleanly.
Reviewed at c1ac9c5d, the same head as my earlier comment. Since then #5651 retired skills/hyperframes-animation/, so adapters/waapi.md no longer exists on main, and that modify/delete is the whole conflict. The WAAPI guidance now lives in one row of skills/hyperframes/references/runtimes.md, which already tells authors to call pause() and use delay for offsets. That matches the contract this PR proposes, so dropping the doc hunk is probably enough. If you want to keep the "pause() then currentTime" note, a short addition to that row is the place for it.
I checked the code against current main. waapi.ts merges without conflict next to #5503's holdAnimationAt change, which fixed a different bug (pause ordering at the first seek, not the initial baseline). On a local merge with the doc file dropped, all 326 adapter tests in packages/core/src/runtime/adapters/ pass, including your 21 new ones.
Two things unchanged from my earlier comment:
- The timing contract (a running animation's pre-runtime
currentTimeis no longer kept as an offset) still needs a maintainer to sign off before this merges. - The commit is unsigned, and
mainrequires signed commits, so please re-sign it when you rebase.
Need: yes, load time leaks into WAAPI frame positions (#4559) · Size: ok · Reuse: extends the existing baseline logic in the WAAPI adapter · Simpler: already minimal
— Rames
miguel-heygen
left a comment
There was a problem hiding this comment.
Adding one thing to the earlier review, which already covers the conflict, the timing contract question and commit signing.
CI did run on this commit, and two checks are red besides the conflict:
Skills: manifest in syncfails because theadapters/waapi.mdchange makes thehyperframes-animationentry inskills-manifest.jsonstale. Dropping that doc hunk when you rebase, as suggested above, fixes it. If you move the note intoskills/hyperframes/references/runtimes.mdinstead, runbun run --cwd packages/cli gen:skills-manifestand commit the result.Fallow auditreports new issues, but on a fork the job cannot post its findings as a PR comment, so they are not visible here. Runningbunx fallow audit --base origin/mainlocally after the rebase shows the same list.
Verdict: COMMENT
Reasoning: The code still looks right and the open items are the rebase, the timing contract sign-off and signing, plus two red checks the author should clear on the rebased commit.
|
On the timing contract: yes. A |
c1ac9c5 to
31095ca
Compare
|
Thanks for confirming the timing contract. Updated in 31095ca:
The PR remains ready for review and is mergeable. All nine upstream workflow runs for this head currently show |
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 31095cadf4a4c49b075ead56b3b1b75bb74cb6f1.
The initial-origin predicate excludes paused, non-unit-rate, custom-timeline and CSS animations. Miguel explicitly accepted the timing contract, the rebase drops the retired guide and the independent runtime pass found no source blocker.
The exact-head new WAAPI unit suite passed 21 cases. The producer browser suite did not collect because the donor core lacks compiler/html-document. No real-Chrome render in this sweep.
Need: yes · Size: ok · Reuse: existing implementation and tests · Simpler: already bounded · Structure: fine
Risk: medium - initial WAAPI clock anchoring and seek semantics.
Verdict: APPROVE
Reasoning: The initial-origin contract is explicit and the adapter controls pass; no remaining blocker found.
What
Keep initial WAAPI frames on composition time, rather than on how long the page took to load.
Why
The first baseline currently retains a running animation's
currentTime. If the animation starts before the runtime and more scripts load afterward, that elapsed wall-clock time becomes a permanent seek offset. Repeated exports can therefore put the same object at different positions in the same frame.In the original October 4 reproduction, I checked the producer's HTML compilation, capture, MP4 encoding, and decoded-frame path, not just the adapter. A 4-second, 100 px translation with an initial left edge of 40 px should land at 90 px at 2 seconds:
The scripts are tiny local files, with no artificial sleep. Paused-offset controls retain their authored position. These are macOS/Chrome measurements, not a cross-platform performance claim.
Related work
Refs #4559. Proposed contract and reproduction: #4559 (comment)
How
At initial discovery only, anchor running or already-finished JavaScript animations on
document.timelineat normal playback rate to composition zero. Including finished animations lets short, filled effects rewind after a slow load. Keep existing baselines for paused animations, CSS animations/transitions, other timelines, non-default playback rates, late creation, and rediscovery.Compatibility: a pre-runtime
currentTimeassignment left running is indistinguishable from elapsed loading time. This change does not retain it as a baseline offset. Use positive/negativedelay, orpause()followed bycurrentTime, for an explicit offset. There is a browser test for this behavior; it is not an accidental side effect. No new API or timing heuristic is introduced.Review follow-up
dd7290aeand dropped the edit to the retired WAAPI adapter guide. The currentskills/hyperframes/references/runtimes.mdalready describes document time and explicit offsets; no generated manifest change is needed.isBrowserCreatedCss, keeping the same timing contract without pushingensureBaselineover the Fallow complexity threshold.Test plan
test:hyperframe-runtime-cipasses: 107 files / 2,635 tests, runtime type checking, contract/behavior/seek/duration/parity/security checks, coverage gates, and linter.The browser suite needs Chrome. Initial broad local runs hit default test timeouts; reruns with two workers passed without changing deadlines or assertions. The last upstream commit only changes Studio copy; the tested runtime, producer, dependencies and scripts are unchanged. I have not validated Linux BeginFrame/GPU capture, distributed workers, or the Studio UI. This is not a general fix for non-default playback rates or custom timelines.