fix(runtime): immediateRender for set tweens + array timeline normalization#1692
fix(runtime): immediateRender for set tweens + array timeline normalization#1692miguel-heygen wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
f7a2176 to
43b8a3e
Compare
0054138 to
1deda5d
Compare
43b8a3e to
7ab746b
Compare
1deda5d to
93b9b8d
Compare
…zation - Set tweens now emit immediateRender:true so they render on page load without requiring the runtime to seek past position 0 - Runtime IIFE normalizes array timelines (window.__timelines = [tl]) to keyed objects, and auto-adds data-start on root elements - Drag teardown clears translate:none to prevent #1673 fly-off - Position-only set tweens hidden from timeline diamonds (3 cache paths) - Parser: ease-only keyframe update preserves existing properties
7ab746b to
f53eaa4
Compare
93b9b8d to
7498be8
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: NIT (one band-aid risk worth flagging — see #1)
Head: 7498be8. Base: chore/studio-remove-console-logs (Graphite stack — verify final-state at merge time). The intent of every chunk is sound and the comments explaining the GSAP no-op-at-creation-position quirk are exemplary. Three observations, one of them a Pattern #2 band-aid the author may want to address before this lands.
1. bindRootTimelineIfAvailable no longer restores state.currentTime on rebind (potential regression)
packages/core/src/runtime/init.ts ~L1029–1052. Pre-PR the boundDuration > 0 branch did:
state.capturedTimeline.pause();
const seekTime = Math.max(0, state.currentTime || 0);
state.capturedTimeline.totalTime(seekTime, false);
Post-PR it does:
state.capturedTimeline.progress(0.0001, true);
state.capturedTimeline.progress(0, false);
state.capturedTimeline.pause();
The progress-cycle is the right kick — it correctly forces GSAP to render tl.set() at position 0 instead of no-op'ing because the playhead never moved off the creation point. But it unconditionally lands at progress(0), whereas the previous code restored state.currentTime. bindRootTimelineIfAvailable is called from the rAF transport loop every 60 frames (~1s) at L2069/L2272, plus other late-bind sites — if a child composition timeline registers after the user has scrubbed (or after a soft reload restores a non-zero currentTime), the rebind will now snap the playhead back to 0. The sibling rebindTimelineFromResolution at L1336 still does seek(previousTime, false) correctly; only this path regressed.
Suggested shape: cycle progress past 0 to force the set render, then totalTime(Math.max(0, state.currentTime || 0), false) to honor the prior scrub. Same effect for the t=0 case (still kicks GSAP off the creation point) but doesn't drop user scrub state.
2. Array __timelines normalization is one-shot at init — most agent scripts will miss it (Pattern #2 decorative gate)
init.ts L79–91. The normalization runs once inside initSandboxRuntimeModular(). But the comment three blocks down (L2122–2125) says explicitly: "inline scripts registering child timelines in __timelines haven't executed yet (they run in the browser's next microtask). Defer a rebinding attempt to catch them." In other words, the populate-path runs after init, by design. An agent script that does window.__timelines = [tl] post-init lands in the populate phase the gate doesn't cover — the array form persists, and downstream timelines[compositionId] lookups (L452, L559, L2152) silently miss because the keys are "0", "1" instead of composition IDs.
Two ways out:
- Wrap
window.__timelinesin a setter (Object.defineProperty) that re-normalizes on every assignment. - Or do the normalization opportunistically inside
resolveRootTimelineFromDocument/ the standalone-seek helpers — anywhere we readwindow.__timelines, normalize-on-read.
The init-time gate as written only helps the (presumably rare) case of window.__timelines = [tl] evaluated synchronously before initSandboxRuntimeModular() returns.
3. Nice-to-haves and confirmations
data-start="0"default on root composition — interacts fine with #1662's ancestor walk. The root has no[data-start]ancestors (the walk breaks atrootComp), and the root will always have descendants soisTimedClipLeafreturns false (nodisplay:none). Safe.immediateRender: truecross-PR with #1686 overlay validator — validator only inspects intent / overlap, not GSAP vars-bag keys, so no rejection. Generator and runtime are now in agreement onset()semantics — regime drift closed.- Position-only
settween filtering / drag teardowntranslate: noneclear — both look correctly scoped; the teardown's "do NOT clearProps:transform" comment is exactly the kind of band-aid-avoidance note the file needed. - Ease-only keyframe update (gsapParser.ts L2227+) — preserves existing properties via
isObjectProperty/propKeyNamediscovery. Reasonable; the only nit is the early-return short-circuit assumeseaseis non-empty truthy, so an explicitease: ""(clear-ease intent) would fall through to the existingbuildKeyframeValueNodepath and nuke the property bag. Probably not a real path, but worth a comment if it could be. - No tests added for any of the new behaviors. The test plan is all manual. The array-normalization in particular would be a single-shot unit test (
window.__timelines = [tl]; init(); expect(window.__timelines).toEqual({...})).
CI
At review time: Preflight + Detect changes + WIP passing; regression-shards, Perf:*, and Preview parity still in progress. Worth a glance before merging in case any GSAP-set-tween regression test trips on the new behavior.
Prior reviews
None yet (Via is first to land). Rames was tagged in parallel — recommend reconvening on findings #1 and #2 before merge.
Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R1 @ 7498be8 — Rames D Jusso review
Reviewing runtime resilience fixes. The set-tween / array-normalization / drag-teardown core is sound; two finds are load-bearing — a silent revert of a benchmarked perf PR, and a busted runtime debug surface.
🛑 Blockers
-
packages/core/src/runtime/media.ts:289-308— silent revert of #1651 (b651a9d). James's perf commit two days ago added askipForInjectedVideogate that skipped per-tickel.currentTime = relTime+el.load()drift recovery on<video>elements with sibling<img id="__render_frame_<id>__">(frame-injection pipeline, render mode only). #1651 was validated N=3 baseline vs N=3 with-fix onsynth-30-heavy: wall -1.8% (-2.2s), avgScreenshot -2.0%, avgBeforeCapture -7.0%, identical output MD5. This PR drops the gate entirely —git diff b651a9d2..7498be80 -- packages/core/src/runtime/media.tsshows the entireskipForInjectedVideoblock removed without mention in the PR title, body, or commit message. Either restore the gate (it's orthogonal to the immediateRender fix) or document the rollback rationale + accept the per-render wall regression (~1.8% on video-heavy renders, ~2400 wasted seeks per 30×3s render). My read is unintentional rebase loss — please confirm. -
packages/core/src/runtime/diagnostics.ts:47-50—swallow()now hasif (w.__hfDebug || w.__HYPERFRAMES_DEBUG) { /* eslint-disable-next-line no-console -- intentional debug surface */ }with no body. The whole point of the function per the docstring (lines 14-28) is the opt-in__hfDebugconsole surface. This silently kills the documented runtime debug pathway in core (not studio — this ispackages/core, out of #1691's scope but the change snuck into this PR). The orphaned doc-comment lines 17-19 are also now nonsense (" whenwindow.__hfDebug === true…" with no preceding bullet about console.debug). Restore theconsole.debugcall AND fix the docstring, or document the new contract (presumably "only the__hf.onSwallowedhandler path is supported").
-
packages/core/src/runtime/init.ts:79-91(array timeline normalization) — usesdocument.querySelector("[data-composition-id]")?.getAttribute("data-composition-id")to pick the root composition ID. The runtime's authoritative resolverresolveRootCompositionElement()at line 247-263 of the same file checksdata-root="true"first, falls back to topmost-non-nested, then first. For a single-comp page (Miguel's stated agent-mistake target), the two agree. For multi-comp pages with sub-compositions or an explicitdata-rootmarker that isn't first in DOM order, the array-normalization picks the wrong key. The bridge to use isresolveRootCompositionElementitself — but it's declared later in the same function and depends on closures. Two options: (a) hoist the resolver function earlier, (b) replicate its priority logic inline (checkdata-root="true"first). As written, the array → object normalization works for the common case but lies about which composition owns the single timeline on multi-comp pages. -
packages/core/src/runtime/init.ts:96-99(data-start defaulting) — samequerySelector("[data-composition-id]")shape. Same multi-comp caveat. Lower-stakes because authoring without data-start on a non-root composition is already broken-shaped, but worth aligning with the resolver for consistency. -
packages/core/src/runtime/init.ts:1032-1038— newif (boundDuration <= 0)branch callsprogress(1, true); progress(0, false); pause(). For a forever-looping ambient timeline (tl.repeat(-1)with finiteboundDuration~ 0 from the safe-duration accessor),progress(1)forces it to end state then pause — which may visually snap mid-loop in unexpected ways at init. Hard to reason about without seeing what shape returnsboundDuration <= 0fromgetSafeTimelineDurationSeconds(_, 0). Worth a comment + a test for "infinite-loop timeline rebind doesn't visually snap to end-frame." -
packages/core/src/runtime/init.ts:1046-1053— replacement oftotalTime(seekTime, false)with theprogress(0.0001, true); progress(0, false); pause()cycle. Hard to evaluate: does this respectstate.currentTime? PreviouslytotalTime(seekTime, false)honored a non-zero soft-reload restore time. The new shape always seeks to ~0 (progress(0)). Was the soft-reloadstate.currentTime > 0restore intentionally dropped, or did the lateral on the fix lose it? Test for "soft-reload at t=2s shows frame@2s, not frame@0". -
packages/core/src/parsers/gsapParser.ts:1295—tl.setalways emitsimmediateRender: true. Inverse op (memory: feedback_inverse_operation_tolerance_asymmetry): the PARSE path (extras: { immediateRender }) at line 527 / 880 of the stress test already tolerates this. ✅ round-trip clean. But: position-onlytl.setper the PR description is "hidden from timeline diamonds" — verify the diamond-filter check matchesimmediateRender: truerather than just method=="set" + position-only. Otherwise pre-PR saved compositions with set tweens lackingimmediateRenderstill show diamonds while new ones don't — UI inconsistency. -
packages/core/src/parsers/gsapParser.ts:2227-2243— ease-only keyframe update preserves existing properties for ObjectExpression values. ✅. But for non-ObjectExpression values (primitive — e.g.keyframes: { "50%": "0.5" }— uncommon but legal in GSAP's keyframe shorthand), it falls through tobuildKeyframeValueNode(properties={}, ease)at line 2244 which wipes properties. Either short-circuit-return on non-ObjectExpression or accept the corner. Likely fine; flag as nit. -
Backward-compat (memory: status-machine in-flight rows gap): array-shaped timelines saved to disk before this PR worked because something else normalized them downstream? Or did they fail-silent and users blamed GSAP? Either way, post-PR they normalize at init. Migration concern is one-way: a comp authored against post-PR array-shaped runtime that gets opened in pre-PR runtime breaks. Minor — agents don't load Studio in old runtimes — but worth a one-liner in PR body.
-
Test coverage — diff shows no test additions for: array normalization (
window.__timelines = [tl]→ keyed), data-start default, drag-translate-none clear, immediateRender emission, ease-only keyframe preservation. Each of these is a behavior addition that should have a unit test, especially the parser changes (gsapParser.stress.test.tsalready has a similar shape at line 872-880). Filing as a test-debt concern rather than blocker because the runtime ones are hard-to-unit-test, but parser-level should pin.
🟡 Questions
packages/studio/src/hooks/gsapRuntimeBridge.ts:235-251refactor extractshasNonHoldto a variable + removes the explanatory comment. Pure refactor, no behavior change? Wanted to confirm the deleted comment lines 246-251 of the prior file ("STATIC case (single source of truth = GSAP timeline) …") were preserved elsewhere or intentionally dropped. The shape is non-obvious to readers; that comment was load-bearing for onboarding. Suggest re-adding it above theif (!hasNonHold)branch.
🟢 Looks good
packages/studio/src/components/editor/manualOffsetDrag.ts:446-453(#1673 drag teardown) — narrow + correctly defensive. Only clearstranslate: noneliterally, preserves committed transform. ✅packages/core/src/parsers/gsapWriterAcorn.ts:324-331—updates.easeEach ?? updates.easeconsolidation for keyframe-aware writers is clean. Comment loss is fine; the code reads.addAnimationWithKeyframesToScript(_, easeEach?)plumbing through both parsers — symmetric ✅.
— Rames D Jusso
Stamp routing: @rames Jusso once findings addressed (or punted with rationale).

Summary
Runtime resilience fixes for GSAP set tweens and common agent authoring mistakes.
immediateRender: trueon set tweens: GSAPtl.set()at position 0 on a paused timeline doesn't render until seeked past 0. AddingimmediateRender: trueforces GSAP to apply the set when added to the timeline, fixing position persistence after page refresh.window.__timelines = [tl](common agent mistake) auto-normalized to keyed object{ "composition-id": tl }.data-startdefault: root[data-composition-id]withoutdata-startgetsdata-start="0"automatically.translate: noneinendManualOffsetDragMembersso button-less pointermoves after soft reload don't compute stale deltas.tl.setdoesn't show diamonds.isObjectProperty/propKeyName.Test plan
window.__timelines = [tl]loads correctly