Skip to content

fix(parsers): emit global gsap.set position holds before the timeline declaration#2099

Merged
vanceingalls merged 1 commit into
mainfrom
07-09-fix_parsers_emit_global_gsap.set_position_holds_before_the_timeline_declaration
Jul 9, 2026
Merged

fix(parsers): emit global gsap.set position holds before the timeline declaration#2099
vanceingalls merged 1 commit into
mainfrom
07-09-fix_parsers_emit_global_gsap.set_position_holds_before_the_timeline_declaration

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Problem

In the studio, moving one element resets every previously-moved element back to its authored position.

Repro:

  1. Drag #image-clip, then #video-clip — both persist as base gsap.set("#el", { x, y }) lines and render correctly.
  2. Drag #title-text (any element whose edit triggers a script rewrite + soft reload).
  3. #image-clip and #video-clip snap back to their authored positions — live and on every subsequent render — even though the gsap.set lines are still in the file.

Root cause

The writer emits base position holds after the tween calls:

const tl = gsap.timeline({ paused: true });
tl.from("#image-clip", { scale: 0.9, duration: 0.8 }, 0.2);
gsap.set("#image-clip", { x: 267, y: 20 });   // ← wiped
window.__timelines["main"] = tl;

On a soft reload the studio rebind kicks progress(0.0001, true) synchronously — before GSAP's lazy queue flushes — so the from() tween on the same target first-initializes during a backwards render. GSAP's _initTween then reverts its internal isFromStart set, which removes the element's whole inline transform — taking the base set's x/y with it. _parseTransform re-reads the computed transform as identity and bakes x/y = 0 into the GSAP cache, so every later render writes translate(0px, 0px).

Standalone 12-line repro confirms: a base set emitted after a conflicting from() tween loses its x/y under any seek once from-init runs; emitted before the timeline construction it survives every render order (sync seek, rewind kick, lazy flush, double from-tweens), because the from() records the set pose as its pre-tween state and every revert restores it.

Fix

packages/parsers/src/gsapWriterAcorn.ts:

  • addAnimationToScript — a global gsap.set now inserts on the line above the timeline declaration (findGlobalSetInsertionPoint). The new-id lookup diffs content-based ids against the pre-insert set instead of assuming the appended statement is last in source order.
  • updateAnimationInScript — a legacy trailing global set is relocated above the declaration whenever it's touched, healing files written before this change on the next nudge.

Testing

  • New unit tests: hoisted insert position + id round-trip, legacy relocation on update, already-hoisted no-move.
  • Full parsers suite (796), SDK mutate/session (231), studio-server files routes, studio drag-commit — all pass.
  • Browser E2E of the real demo script through two full soft-reload cycles (clearProps sweep → re-run → rebind kick): moved positions stable at translate(267px, 20px) / translate(60px, 200px); the old trailing-set shape reproduces the translate(0px, 0px) reset.

🤖 Generated with Claude Code

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

… declaration

A base `gsap.set(...)` written AFTER the tween calls is wiped on the next
soft reload: when a `from()` tween on the same target lazily initializes
during a backwards render (the studio rebind's progress(0.0001) kick), GSAP
reverts its internal isFromStart set, which removes the whole inline
`transform` — taking the base set's x/y with it. The from() tween then
re-parses the computed transform as identity and bakes x/y = 0 into the
GSAP cache, so every element previously moved in the studio snaps back to
its authored position whenever any other element is edited.

Emitting the global set BEFORE the timeline construction makes it part of
the pre-tween state the from() records, so every revert restores the moved
pose instead of stripping it.

- addAnimationToScript: global sets insert above the timeline declaration;
  the new-id lookup now diffs content-based ids instead of assuming the
  appended statement is last in source order.
- updateAnimationInScript: a legacy trailing global set is relocated above
  the declaration whenever it's touched, healing files written before this
  change on the next nudge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vanceingalls vanceingalls force-pushed the 07-09-fix_parsers_emit_global_gsap.set_position_holds_before_the_timeline_declaration branch from 1f04f4e to e0b5d01 Compare July 9, 2026 08:08

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits

Clean fix for a subtle GSAP ordering bug. The root cause analysis is spot-on — from() lazy-init during a backwards render (progress(0.0001, true) on rebind) reverts the inline transform, wiping any gsap.set() that was emitted after the tween calls. Hoisting the set above the timeline declaration makes it part of the pre-tween state that from() records and restores on revert.

What I liked:

  • The fix is minimal and surgical — two insertion-point changes, one legacy relocation heal, no structural rewrites.
  • The new-id detection in addAnimationToScript correctly diffs id-counts rather than assuming positional order. Handles duplicate ids gracefully.
  • The updateAnimationInScript relocation is a nice "heal on touch" pattern — legacy files self-correct on the next nudge without a migration.
  • Test coverage is solid: insert-before, legacy relocation, already-hoisted no-op, and round-trip id resolution.

Minor nit (non-blocking):

The whitespace-collapsing ternary in the relocation logic is a bit dense:

const moveStart = /^\s*$/.test(script.slice(lineStart, exprStmt.start))
  ? lineStart
  : exprStmt.start;

A brief inline comment like // include leading blank prefix on the line would help future readers parse the intent faster.

Ship it.

— Miga

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at e0b5d01.

Nice piece of GSAP archaeology — the root-cause writeup matches what _initTween / _parseTransform actually do on the backwards-revert path (isFromStart nukes the whole inline transform, and any trailing gsap.set(x, y) on the same target is collateral). Hoisting the base set above the timeline declaration puts x/y into the pre-tween state from() records at init, so every revert restores it. Clean, minimal, well-tested.

Blockers

None.

Concerns

None structural. Two thin edge-case notes below — neither should gate.

Nits

  • Legacy-heal moveEnd edge case in updateAnimationInScript (gsapWriterAcorn.ts:399-404): moveEnd extends past exprStmt.end only when script[exprStmt.end] === "\n". If a legacy file's trailing gsap.set(...) is the very last statement with no trailing newline, the move splices it into the same line as the timeline declaration (gsap.set(...);var tl = gsap.timeline(...);). Real HF studio scripts always land window.__timelines[...] = tl; after the set (which enforces a \n), so this doesn't fire in practice — but always appending a \n when the move doesn't consume one closes the hole cheaply.
  • Incremental healing is intentional and worth a body note: only the trailing set whose id matches animationId is relocated; other trailing sets in the same legacy file stay put until each is individually nudged. Worth a one-liner in the PR body so a future reader who inspects a partially-healed file doesn't file it as a bug.

Questions

  • Multi-timeline scriptsfindTimelineDeclarationStatement uses parsed.timelineVar (scalar). If a script ever declares two named timelines, does the parser track both or just the first? Not blocking (every HF studio script I've seen is single-timeline), but if the parser holds a single value, the hoist targets that decl and any set correctly lands above it — noting the assumption for the record.

What I didn't verify

  • The browser E2E claim (soft-reload cycles, translate(267px, 20px) persistence) — trusted the writeup + the standalone-12-line repro claim.
  • Non-parser call sites of the writer API — public signatures haven't shifted ({script, id} return, same updateAnimationInScript args), so no downstream should need changes.
  • Full CI at the new HEAD SHA — some jobs still IN_PROGRESS at review time (Test, Build, Typecheck, Studio: load smoke, CLI smoke, Render on windows-latest). Prior run at the earlier SHA on this PR was fully green; scope is small enough that these should follow.

Review by Rames D Jusso

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at e0b5d01c6d2e4e71e70c1cf46babe4a952a5e69c.

Additive to Miga and Rames: no new blocking findings from my pass.

What I verified:

  • packages/parsers/src/gsapWriterAcorn.ts:314 computes the insertion point from the actual timeline declaration line start, and addAnimationToScript only routes through it for method === "set" && global, so normal timeline tween insertion stays on the existing findInsertionPoint path.
  • packages/parsers/src/gsapWriterAcorn.ts:397 heals only touched legacy global sets by moving their expression statement above the declaration; that keeps the migration incremental and avoids a broader script rewrite.
  • packages/parsers/src/gsapWriterAcorn.ts:516 switches new-id detection to an id-count diff, which is the right fix once the inserted set is no longer source-order-last.
  • packages/parsers/src/gsapWriter.acorn.test.ts:228 and :256 pin the two important behaviors: new global sets hoist before the timeline and legacy trailing sets relocate on update while already-hoisted sets stay put.

Verification: bun run --filter @hyperframes/parsers test -- gsapWriter.acorn.test.ts passed locally, 29/29. GitHub had no failed check runs at review time; two Windows jobs were still pending, so this approval is code-level and does not override CI.

Review by Magi.

Verdict: APPROVE
Reasoning: The parser write-path change is tightly scoped to off-timeline global position holds, preserves the existing tween path, and has focused regression coverage for both new insertion and legacy healing.

@vanceingalls vanceingalls merged commit bdb9a34 into main Jul 9, 2026
60 checks passed
@vanceingalls vanceingalls deleted the 07-09-fix_parsers_emit_global_gsap.set_position_holds_before_the_timeline_declaration branch July 9, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants