Skip to content

feat(core): route motion-path mutations through studio-api + fix clip stamping#1555

Open
miguel-heygen wants to merge 3 commits into
feat/core-gsap-motion-path-mutationsfrom
feat/core-motion-path-route
Open

feat(core): route motion-path mutations through studio-api + fix clip stamping#1555
miguel-heygen wants to merge 3 commits into
feat/core-gsap-motion-path-mutationsfrom
feat/core-motion-path-route

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Stack: GSAP keyframe + motion-path editing — studio-api wiring + runtime clip fixes (#1553#1561).

What

Two things:

  1. Route the new keyframe / motion-path mutations through the studio-api source-mutation layer and file route, so Studio edits persist through the single mutation entry point.
  2. Fix runtime clip stamping and in-flow clip hiding in runtime/init.ts.

Why

  • Studio must persist GSAP edits via studio-api (consistent validation + undo), not ad-hoc writers.
  • Clip stamping: an auto-stamped animated scene container was suppressing its own animated children from becoming timeline clips, so that scene couldn't inline-expand.
  • In-flow timed clips hidden with visibility:hidden still reserve their layout box, so a split clip's two halves stacked instead of overlapping.

How

  • sourceMutation.ts + routes/files.ts: dispatch motion-path/keyframe ops through studio-api.
  • init.ts: ancestor-suppression now counts authored data-start only (hasAuthoredTimedAncestor); in-flow timed clips hide with display:none while positioned clips keep visibility:hidden.

Test plan

  • Unit tests added (sourceMutation)
  • bun run test (core) green
  • ⚠️ Known issue under investigation: hdr-hlg-regression shard fails on the final ~4 frames (PSNR ~9.7 dB). Tracking whether the in-flow-clip hiding change interacts with HDR layered capture.

Single-source manual offset + rotation (added)

add/set mutations that write a position or rotation now strip the matching legacy CSS var (--hf-studio-offset / --hf-studio-rotation) from the target — matching add-with-keyframes / replace-with-keyframes. Once the GSAP timeline owns that channel (single source of truth), the legacy var must be cleared so it can't double-apply.

miguel-heygen commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

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.
Learn more

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

@jrusso1020 jrusso1020 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.

Holding per two independent verifications (Rames D Jusso + Via) of the same blocker. Calling out one specifically because it's a regression of work that landed earlier today.

🛑 Silent removal of the 5 readiness adapters from runtime/init.ts

packages/core/src/runtime/init.ts:9-13 + :1918-1924 — the five adapter registrations from https://github.com/heygen-com/hyperframes/pull/1548|#1548 (createMapboxAdapter / createLeafletAdapter / createGoogleMapsAdapter / createMaplibreAdapter / createD3Adapter) are removed in this diff. The factories themselves (and their .test.ts siblings) survive in runtime/adapters/, but the wiring at the registry — the thing that actually makes them participate in getReadyPromise() polling — is gone.

#1548 merged at 2026-06-18T05:21:28Z (~10 hours ago) specifically to gate __renderReady on map/viz async asset loading. If #1555 lands as-is, any composition that uses mapbox / leaflet / google-maps / maplibre / d3 will silently never reach render-readiness — the exact bug class #1548 was built to prevent. This is a real production regression, not just a code-style concern.

Most likely cause is a Graphite restack mishap when #1548 merged mid-stack and the conflict resolution dropped the wiring rather than preserved it. The benign path is one rebase + re-add the import + the 5 factory calls; the malignant path would require intentional rationale I can't find anywhere in the PR description.

Ask: confirm intentional (with rationale + regression-window acknowledgement) OR re-restack to preserve #1548's wiring.

🛑 HOLD_SYNC_MUTATION_TYPES set is incomplete

Echoing Rames D Jusso (packages/core/src/studio-api/routes/files.ts:534-548) — the set covers keyframe / motion-path-point mutations + delete but misses 5 mutation types that also change tween start or create a tween at position > 0:

  • add-motion-pathaddMotionPathToScript authors a 2-anchor motionPath at caller's position; if position > 0, fresh tween snaps from CSS-base → (0,0) on start.
  • add (generic) — can create position tweens via properties: {x, y}.
  • update-metabody.updates.position changes start; existing hold becomes stale.
  • shift-positions / scale-positions — bulk-shift; pre-existing holds end up at wrong t.
  • split-animations — produces new tweens at new positions.

syncPositionHoldsBeforeKeyframes is idempotent + script-scoped, so an over-broad set just costs one extra parse. Cheap expansion.

Non-blocking notes

Echoing Rames D Jusso:

  • init.ts:1440-1448timedClipInFlow WeakMap caches getComputedStyle(el).position lazily, never invalidates. Studio-edit-then-replay round-trip → stale cache picks wrong display branch. Either invalidate on style-position mutation or document the invariant.
  • init.ts:1555-1559 — visible branch uses raw timedClipInFlow.get(rawNode), hidden branch uses isTimedClipInFlow(rawNode) (force-populates). Works by happy accident; align both or comment why.

Plus: PR body is the unfilled template across this whole stack. Worth filling in even a one-paragraph description before merge.

Once the adapter regression is addressed (re-add the wiring OR explicit rationale + downstream owner ack), happy to re-review.

@miguel-heygen miguel-heygen force-pushed the feat/core-motion-path-route branch from 070799e to 227aebe Compare June 18, 2026 15:48

@jrusso1020 jrusso1020 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.

Re-reviewed at 227aebe4. Top blocker closed — adapter wiring restored at init.ts:9-13, :1939-1943, so the 5 readiness-adapter registrations from #1548 are back. Thanks for the fast restack.

Reduced-scope REQUEST_CHANGES on the remaining items, per Via's R2 verification:

1. HOLD_SYNC_MUTATION_TYPES set still incomplete at routes/files.ts:534-548.
7 keyframe-adjacent types added in the restack (good) — remove-all-keyframes, add-with-keyframes, replace-with-keyframes, convert-to-keyframes, materialize-keyframes, delete, delete-all-for-selector. The 5 position-changing-non-keyframe types still need to be added:

  • add-motion-pathaddMotionPathToScript authors a 2-anchor motionPath at caller's position; if position > 0, fresh tween snaps from CSS-base → (0,0) on start.
  • add (generic) — can create position tweens via properties: {x, y}.
  • update-metabody.updates.position changes start; existing hold becomes stale.
  • shift-positions / scale-positions — bulk-shift; pre-existing holds end up at wrong t.
  • split-animations — produces new tweens at new positions.

Cheap: syncPositionHoldsBeforeKeyframes is idempotent + script-scoped, so an over-broad set just costs one parse.

2. timedClipInFlow WeakMap stale-cache at init.ts:1445-1452.
Still no invalidation hook. Studio-edit-then-replay round-trip → stale cache picks wrong display branch. Either timedClipInFlow.delete(el) on style-position mutation, OR explicit invariant comment naming the round-trip case as out-of-scope.

3. init.ts:1560-1563 — visible/hidden branch asymmetry.
Visible branch uses raw timedClipInFlow.get(rawNode) (returns undefined for new nodes); hidden branch uses isTimedClipInFlow(rawNode) (force-populates). Works by happy accident — align both or comment why.

Once these three land, happy to re-approve. Also: PR body is still the unfilled template; please fill in before merge.

@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 b79f5537 (R3 — verifying R2 follow-through on the three open items + new commit b79f5537 "strip legacy path-offset/rotation + drop obsolete studio lint rule"). Stack-aware.

⚠️ Two of the three R2 items are still open at HEAD; layered findings on the new commit + the lint-rule deletion.

R2 items, verified at HEAD

1. ✅ Adapter wiring — confirmed restored at init.ts:9-13 (imports) + :1954-1958 (registrations). The 5 readiness adapters from #1548 are back. Top R2 blocker resolved.

2. 🛑 HOLD_SYNC_MUTATION_TYPES still incompleteroutes/files.ts:594-611 at HEAD has the 13-entry set, but add-motion-path, add (generic), update-meta, shift-positions, scale-positions, split-animations are STILL missing. The mechanism is identical to what R2 described — any mutation that creates or shifts a position tween at position > 0 leaves the pre-keyframe hold stale, producing the one-frame snap-to-CSS-base symptom the hold-sync was built to prevent. Particularly load-bearing for add-motion-path because #1554's addMotionPathToScript is the entry point for the new motion-path UX. Cheap fix: syncPositionHoldsBeforeKeyframes is idempotent + script-scoped, so an over-broad set just costs one extra parse.

3. 🛑 timedClipInFlow + timedClipIsLeaf WeakMap stale-cache (init.ts:1442-1471) — both caches still lack an invalidation hook. Studio-edit-then-replay round-trip (e.g. user toggles position:relative→absolute, or wraps a leaf in a new timed container) → stale cached value picks wrong display branch. The leaf check (el.querySelector('[data-start]') === null) is especially fragile — adding a timed child to a previously-leaf element is the canonical hot-edit operation. Either invalidate on style-position / DOM-mutation, OR document the round-trip case as out-of-scope with an explicit invariant comment.

4. 🛑 Visible/hidden branch asymmetry (init.ts:1575-1579) — still:

if (isVisibleNow) {
  if (timedClipInFlow.get(rawNode)) rawNode.style.removeProperty("display");
} else if (isTimedClipInFlow(rawNode) && isTimedClipLeaf(rawNode)) {
  rawNode.style.display = "none";
}

Visible branch uses raw .get() (undefined for nodes that never went through the hidden branch); hidden branch uses isTimedClipInFlow() (force-populates). The hidden→visible transition works because the hidden branch always runs first for an in-flow leaf; but the first-ever-visible call for an in-flow leaf returns undefined → style.removeProperty("display") is a no-op, which is benign IF nothing else set display:none. Today nothing else does. But this works by happy accident; align both branches via isTimedClipInFlow(rawNode) OR add a comment naming the invariant.

New findings on commit b79f5537

5. ⚠️ Security-delete reintroduction self-check on the deleted lint rule (packages/core/src/lint/rules/gsap.ts:855-890). The gsap_studio_edit_blocked rule is REMOVED with rationale "obsolete studio lint rule." The rule warned when a user-authored timeline targets #id / .class selectors that Studio's drag/resize then can't write back. The PR's premise is that #1554's mutation surface now CAN write back to those targets, so the warning is no longer informative. Verify: does #1554's writer actually round-trip tl.to("#id", { x, y }, 0) style author code, or only the studio-generated tl.to("[data-hf-id=\"…\"]", …) form? If the writer only handles data-hf-id selectors, deleting the lint rule restores a silent-skip footgun that the rule was specifically calling out. (Looked at gsapParser.ts write paths quickly — they look selector-agnostic via findTargetElement, but a regression test pinning round-trip on a #id-selector tween would convert this from "concern" to "verified.")

6. ⚠️ Orphan export: TIMELINE_REGISTRY_ASSIGN_PATTERN (packages/core/src/lint/utils.ts:24). The deleted rule was the only consumer (verified via repo-wide code search). Exported regex with no consumer is dead code; safe to remove in this PR or follow-up.

7. ⚠️ stripStudioEditsFromTarget rotation branch — sibling-asymmetry feedback (routes/files.ts:316-372). The expanded path-offset + rotation strip is well-shaped (touched flag per-attribute, both branches mirror each other on attribute lifecycle). The new branch correctly handles data-hf-studio-rotation / data-hf-studio-rotation-draft / data-hf-studio-original-rotate / data-hf-studio-original-inline-rotate / data-hf-studio-original-rotation-transform-origin. Two questions:

  • The keyframesWritePosition helper (routes/files.ts:370-381) covers position but there's no keyframesWriteRotation equivalent. add-with-keyframes / replace-with-keyframes call stripStudioEditsFromTarget unconditionally when keyframes write position — so a keyframe set that writes ROTATION but not position won't strip the rotation channel, leaving the legacy CSS var to double with the new tween. Suggest a keyframesWritesGroup(kfs, 'position' | 'rotation') helper.
  • The generic add case at :909-918 correctly checks both position and rotation via classifyPropertyGroup. Good consistency on that path.

8. ⚠️ splitElementInHtml clone descendant data-hf-id strip (sourceMutation.ts:330-333). Correct fix — descendants would have duplicated data-hf-ids otherwise. But the test added at :511-526 only covers a single-element split with no descendants; the asymmetry was discovered by inspection. Suggest a fixture with <div id="title"><span data-hf-id="…">Hi</span></div> to pin the descendant-strip behavior.

Concerns (carryover from R1, non-blocking)

  • PR body has been updated nicely with the single-source manual offset + rotation rationale. Thank you.

Questions

  • The hdr-hlg-regression known-issue flag in the PR body (final ~4 frames, PSNR ~9.7 dB) — is this still open? If yes, what's the rollback story if it doesn't reproduce post-merge? Worth either landing a fix or filing a tracking issue with clear ownership before merge.

Stamp routability

⚠️ NOT stamp-ready as-is. Items 2-4 are R2 carryover blockers; items 5-8 are R3-NEW concerns. Once items 2-4 land + item 5 is verified (or a regression test added), happy to re-approve.

— Rames D Jusso

@vanceingalls vanceingalls 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 b79f5537. Concur with @james-russo-rames-d-jusso's R3 — two of three R2 carryover items remain open, with the third structurally defensible but documentation-light. Plus one net-new minor blocker and the lint-rule deletion warrants verification.

R2 carryover — verified at HEAD:

🛑 HOLD_SYNC_MUTATION_TYPES (R3 item 2): STILL INCOMPLETE. routes/files.ts:594-611 at b79f5537 carries 13 entries; the 5 jrusso/Rames-named position-changing-non-keyframe types remain absent: add-motion-path (recast handler at L1097), generic add (L676/L957), update-meta (L673/L954), shift-positions / scale-positions (L853/L858, L1177/L1183), split-animations (L816/L1140). Each can create or shift a position tween's first keyframe value — exactly the condition the set is supposed to gate.

This is structurally the decorative-gate pattern: the read site fires (HOLD_SYNC_MUTATION_TYPES.has(body.type) at L1677) but the write site (syncPositionHoldsBeforeKeyframes) never runs for those mutation types because they're never set members. Read path real, gating value 0% for those types. Same shape as decorative billing gates, just in a non-billing domain — the user-visible failure mode is a one-frame snap-to-CSS-base instead of a security/billing exposure, but the structural lie is identical. Cheap fix: idempotent + script-scoped, over-broad set just costs one extra parse. Particularly load-bearing for add-motion-path because #1554's addMotionPathToScript 2-anchor default authors (0,0) → (x,y) and is the canonical entry point for the new motion-path UX (Rames flagged this from the #1554 side).

🛑 timedClipInFlow + timedClipIsLeaf WeakMap stale-cache (R3 item 3): STILL OPEN. init.ts:1442-1471 at HEAD has both caches with no .delete(el) invalidation hook anywhere in the file (grep confirmed). Studio-edit-then-replay round-trip leaves stale cache; the leaf check (el.querySelector('[data-start]') === null) is especially fragile under hot-edit. Either invalidate on style-position / DOM-mutation OR document the round-trip case as out-of-scope with an explicit invariant comment.

🟡 Visible/hidden branch asymmetry (R3 item 4): STRUCTURALLY DEFENSIBLE, DOCUMENTATION-LIGHT. init.ts:1576-1578 at HEAD: visible branch uses raw .get(), hidden branch uses isTimedClipInFlow() wrapper. The asymmetry works because the hidden branch always runs first for an in-flow leaf, so the cache is populated by the time visible-branch reads it; first-ever-visible call returning undefined is a no-op no-op (nothing else sets display:none). I soft-pedal Rames's R2/R3 from blocker to NIT — the invariant holds today, but a one-line comment naming the assumption ("visible-branch raw .get() relies on hidden-branch having populated the cache first; if a future caller sets display:none outside this path, this needs isTimedClipInFlow() here too") would prevent the next reader re-deriving the invariant from scratch.

Net-new (minor blocker on cutover dark-launch surface):

executeGsapMutationAcorn at routes/files.ts:628-884 is MISSING cases for update-motion-path-point, add-motion-path-point, remove-motion-path-point, add-motion-path. The recast path (executeGsapMutationRecast at L885+) handles all four. The acorn default: branch at L883 returns { error: "unknown mutation type: ..." } with HTTP 400, so flipping STUDIO_SDK_CUTOVER_ENABLED=true hard-fails every motion-path mutation in Studio. Dark-launch breakage (flag off in prod), not active regression — but PR body says "Route the new keyframe / motion-path mutations through the studio-api source-mutation layer and file route" without qualifying that one side is unrouted. Either: (a) add the cases to the acorn switch (even as 501 Not Implemented), (b) explicitly fall through to recast for these types, or (c) disclose in the PR body. Maps to band-aid pattern #3 (silent scope gap on stated coverage). [verify-full-dispatch-chain] applies: producer side in #1554 ships 4 new motion-path mutation types; consumer side in this PR routes them in only ONE of TWO dispatch branches.

Concur with @james-russo-rames-d-jusso on R3 items 5-8:

  • Item 5 (deleted gsap_studio_edit_blocked lint rule): verified isElementGsapTargeted survives at packages/studio/src/hooks/gsapTargetCache.ts:49 with 4 active consumers in useDomGeometryCommits.ts that now show an explicit toast (GSAP_CSS_FALLBACK_BLOCKED_MESSAGE) instead of silently dropping the save. The rule's premise ("silently skips saving") is obsolete. BUT Rames's deeper question — "does the writer round-trip #id-selector tweens, or only data-hf-id?" — is the real verification ask. If the writer only handles data-hf-id selectors, deleting the lint rule restores a silent-skip footgun. Worth a regression test pinning round-trip on a #id-selector tween. NIT-level until that's verified.
  • Item 6 (orphan TIMELINE_REGISTRY_ASSIGN_PATTERN): concur, safe to remove.
  • Item 7 (keyframesWritePosition no rotation analog): concur, add-with-keyframes / replace-with-keyframes writing rotation-only keyframes won't strip the legacy CSS var.
  • Item 8 (splitElementInHtml descendant-strip fixture missing): concur, the test at :511-526 covers single-element only; descendant case discovered by inspection deserves a regression fixture.

Verdict: BLOCK. Items 2-4 are R2 carryover, item 5 needs round-trip verification, item 7 is a sibling-symmetry gap, plus the acorn dispatch dark-launch breakage. Once items 2 + 3 land (item 4 acceptable with a comment) + acorn dispatch covers motion-path types (or disclosure), happy to re-evaluate.

Review by Via

… stamping

Wire the new mutations into the file save route. Only authored clips suppress
descendant stamping, so auto-stamped animated scenes can inline-expand.

Hide in-flow timed clips with `display:none` only when they are LEAF clips (no
nested timed clips). `display:none` on a container removes its whole subtree,
hiding descendants that are still inside their own visibility window — e.g. an
in-flow composition root whose effective window clamps to the timeline end would
black out a child video that should still show (the hdr-hlg regression).
Containers keep `visibility:hidden`, which a visible descendant can override; only
leaves leave the flow, which is all the split-overlap case needs.
…lint rule

A position or rotation add/set mutation makes the GSAP timeline the single source of
truth for that channel, so any lingering --hf-studio-offset / --hf-studio-rotation CSS
var must be cleared to avoid double-applying. stripStudioEditsFromTarget now clears both
channels, and the add-strip fires for the position AND rotation property groups.

Also removes the obsolete `gsap_studio_edit_blocked` lint rule: it warned that Studio
cannot save drag/resize edits to elements in a registered timeline — the exact premise
the single-source work inverts (the timeline is now the edit target). Removed the rule,
its now-unused TIMELINE_REGISTRY_ASSIGN_PATTERN import, and its 5 tests.
… cache, strip rotation channel

- HOLD_SYNC_MUTATION_TYPES: add add-motion-path (load-bearing — addMotionPathToScript
  authors past t=0 → first-frame snap-to-(0,0) without the hold), update-meta,
  shift-positions, scale-positions, split-animations. (add stays out: flat tweens
  only, syncPositionHoldsBeforeKeyframes is a no-op for non-keyframed tweens.)
- init.ts: timedClip in-flow/leaf WeakMaps now invalidate on clipTreeSignature change;
  visible/hidden branches both go through isTimedClipInFlow (was .get() by accident).
- keyframesWriteRotation mirrors keyframesWritePosition so a rotation-only keyframe set
  strips the stale --hf-studio-rotation channel.
@miguel-heygen miguel-heygen force-pushed the feat/core-gsap-motion-path-mutations branch from 4ee9b9f to fd0a505 Compare June 20, 2026 21:20
@miguel-heygen miguel-heygen force-pushed the feat/core-motion-path-route branch from b79f553 to 1612463 Compare June 20, 2026 21:20
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