Skip to content

feat(studio): GSAP keyframe + motion-path editing#1567

Open
miguel-heygen wants to merge 2 commits into
feat/studio-keyframes-flagfrom
feat/studio-gsap-keyframe-editing
Open

feat(studio): GSAP keyframe + motion-path editing#1567
miguel-heygen wants to merge 2 commits into
feat/studio-keyframes-flagfrom
feat/studio-gsap-keyframe-editing

Conversation

@miguel-heygen

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

Copy link
Copy Markdown
Collaborator

Summary

This PR makes the GSAP timeline the single source of truth for element position and rotation in Studio's direct-manipulation editor. Previously a manual drag or rotate could land in two places — the GSAP timeline and the --hf-studio-offset / --hf-studio-rotation CSS variables — which let the two channels compose into doubled/phantom positions and let stale CSS vars fling an element off-screen on the next drag. Now every position/rotation edit (live preview, optimistic hold, committed mutation) routes through the GSAP channel:

  • Tweened/keyframed elements → keyframe mutations (unchanged path).
  • Static elements → an idempotent tl.set("#el", { x, y }) / tl.set("#el", { rotation }) instead of a CSS-var write.

It also fixes a cluster of multi-composition bugs surfaced once subcompositions are inlined into one preview (soft reload wiping siblings, keyframe reads picking the wrong timeline, source-file resolution losing the subcomp linkage, overlays hiding behind backgroundless scenes), and removes the now-dead DOM/CSS fallback wiring and the gesture-record badge.

What's changed

Single-source position/rotation math

  • hooks/draggedGsapPosition.ts (new) — computeDraggedGsapPosition(element, studioOffset, fallbackBase) translates a studio drag offset into absolute GSAP x/y, accounting for rotation and the drag-start base stamped on data-hf-drag-*. Leaf module (no store/runtime/core imports) so the live-preview file reuses it without pulling in the commit graph — preview and commit agree by construction.
  • hooks/gsapDragCommit.tscommitGsapPositionFromDrag delegates to computeDraggedGsapPosition (inlined trig removed). Adds commitStaticGsapPosition (idempotent tl.set({x,y}); re-nudge updates the existing set via two coalesced update-property, new element gets one add with method:"set" at 0) and commitStaticGsapRotation (mirror, single update-property). Adds findPositionSetAnimation/findRotationSetAnimation; re-exports computeDraggedGsapPosition.
  • hooks/gsapRuntimeBridge.tstryGsapDragIntercept: no live keyframed motion → treat element as static and commit a position-hold set (also correctly handles the stale-cache phantom). tryGsapRotationIntercept: resolves selector up front, no-tween → static commitStaticGsapRotation, uses the absolute angle directly (Math.round(angle)).
  • components/editor/manualOffsetDrag.ts — GSAP-channel live preview: applyOffsetDragDraftViaGsap (neutralize CSS translate, then gsap.set({x,y})) and applyRotationDraftViaGsap (neutralize CSS rotate, then gsap.set({rotation})), plus readGsapRotation. Draft/commit preview through GSAP, falling back to CSS only when gsap is unavailable. Base read via readAppliedStudioPathOffset (not the raw dormant var) so commit stays relative.
  • components/editor/manualEditsDom.ts / manualEdits.ts — add/re-export readAppliedStudioPathOffset: returns the offset only when the inline translate actually references the studio var; a lingering dormant var reads as zero (using it as a drag base re-commits a phantom offset and flings the element).
  • components/editor/useDomEditOverlayGestures.ts — rotate preview/restore/final-hold go through applyRotationDraftViaGsap (CSS fallback only when gsap absent).
  • components/editor/domEditOverlayStartGesture.ts — rotation base is readGsapRotation(el) + readStudioRotation(el).angle so a rotate starts from the actual visual angle and commits an absolute angle.
  • hooks/useGsapAwareEditing.ts / useDomEditSession.ts — drag/rotation commits no longer branch on handled or fall back to handleDomPathOffsetCommit/handleDomRotationCommit; the GSAP intercept is authoritative for top-level and subcomposition elements. DOM-fallback params/deps removed.

Multi-composition correctness

  • utils/gsapSoftReload.ts — a soft reload no longer tears down all __timelines + global children before re-running one script (which wiped every other inlined composition). It scopes teardown to the keys the re-run script re-registers (parsed from the script text), removes only matching stale scripts, kills only those timelines, clears clearProps only on the re-run composition's targets; bails to a full reload if no key can be parsed.
  • hooks/gsapRuntimeKeyframes.tsreadRuntimeKeyframes no longer assumes the first timeline key (multiple inlined subcompositions, unstable order); scans every timeline with getChildren for matching tweens, with compositionId still pinning the search.
  • components/editor/domEditingDom.ts + components/nle/NLELayout.tsx — module-scoped compositionId → source file map (setCompositionSourceMap, populated by NLELayout from index.html clips); getSourceFileForElement falls back to it to recover which source file a subcomp element edit writes to (honoring data-hf-original-composition-id first).

Overlay visibility

  • components/editor/useDomEditOverlayRects.ts + MotionPathOverlay.tsx — selection box and motion path use isElementVisibleForOverlay (basic display/visibility/opacity) instead of isElementVisibleInPreview (occlusion heuristic). An explicitly-selected element's overlay must track it even under a backgroundless full-bleed scene; occlusion stays for hover where a false hide is cheap.
  • MotionPathOverlay.tsx — park-on-click debounced (250ms) so a double-click cancels the seek; node drag ignores non-primary buttons so right-click reaches the context menu; park timer cleared on unmount.

Gesture recording cleanup

  • hooks/useGestureRecording.ts — removes pointerElementOffset / element-center-snapping (it discarded the manual-drag start position and produced a wrong 0% keyframe); the delta is anchored at the grab point.
  • hooks/useGestureCommit.ts — when the existing position animation is a set (a static hold), the recording replaces it via replace-with-keyframes rather than a time-range merge; the overlap-merge path is preserved for real tweens.

Other

  • hooks/useGsapAnimationOps.ts — "Delete all animations for element" passes softReload: true.
  • components/editor/DomEditOverlay.tsx + test — removes the GestureRecordBadge and its props (dead UI under single-source).

Why

The dual-channel design (timeline + CSS vars) was the root of a recurring bug class: doubled positions when both applied, phantom offsets when a dormant var was reused as a drag base, rotations drifting because the base was re-added each commit. Routing live preview, optimistic hold, and the committed mutation through one GSAP channel makes "what you see while dragging" equal "what gets written" by construction. The multi-composition fixes are required because the runtime now inlines subcompositions into one preview, breaking the single-composition assumptions in soft reload, keyframe reads, and source resolution.

Testing

  • manualOffsetDrag.test.ts — offset marked "applied" via the inline translate longhand referencing the studio vars (matching readAppliedStudioPathOffset); single-offset + GSAP-rebake-per-cycle; a dormant raw var reads as zero.
  • gsapRuntimeBridge.test.ts — stale-parse guard now asserts static-set behavior: no live motion → commits a single add/method:"set" holding the dragged position and must not resurrect the stale tween. "Runtime still has the tween" unchanged.
  • DomEditOverlay.test.ts — drops the record-button assertions alongside the removed badge.

Stack

Part of the GSAP keyframe/motion-path stack: #1553 → #1554 → #1555 → #1607 → #1608 → #1609 → #1610 → #1611 → #1567 → #1612 → #1613 → #1605. On #1611. (Was the consolidated #1567; now scoped to just the single-source slice after the split.) Builds independently; combined diff byte-identical to the originally-reviewed work.

@miguel-heygen miguel-heygen force-pushed the feat/studio-gsap-keyframe-editing branch 5 times, most recently from e347d3a to 07a6321 Compare June 18, 2026 23:36
@miguel-heygen miguel-heygen force-pushed the feat/core-motion-path-route branch from e450a92 to eb1c51f Compare June 19, 2026 00:38
@miguel-heygen miguel-heygen force-pushed the feat/studio-gsap-keyframe-editing branch 3 times, most recently from cd33fda to 12707a3 Compare June 19, 2026 17:58
@miguel-heygen miguel-heygen force-pushed the feat/core-motion-path-route branch from a01743d to 1dbedb6 Compare June 19, 2026 19:36
@miguel-heygen miguel-heygen force-pushed the feat/studio-gsap-keyframe-editing branch from 12707a3 to 8bf425e Compare June 19, 2026 19:36
@miguel-heygen miguel-heygen force-pushed the feat/core-motion-path-route branch from 1dbedb6 to 6aafc8a Compare June 19, 2026 19:52
@miguel-heygen miguel-heygen force-pushed the feat/studio-gsap-keyframe-editing branch from 8bf425e to 231bf67 Compare June 19, 2026 19:52
@miguel-heygen miguel-heygen force-pushed the feat/core-motion-path-route branch from 6aafc8a to d133735 Compare June 19, 2026 20:23
@miguel-heygen miguel-heygen force-pushed the feat/studio-gsap-keyframe-editing branch from 231bf67 to 6b3aa33 Compare June 19, 2026 20:23
@miguel-heygen miguel-heygen force-pushed the feat/core-motion-path-route branch from d133735 to b79f553 Compare June 19, 2026 22:33
@miguel-heygen miguel-heygen force-pushed the feat/studio-gsap-keyframe-editing branch from 6b3aa33 to b2b2e98 Compare June 19, 2026 22:33
@miguel-heygen miguel-heygen changed the base branch from feat/core-motion-path-route to graphite-base/1567 June 20, 2026 18:22
@miguel-heygen miguel-heygen force-pushed the feat/studio-gsap-keyframe-editing branch from b2b2e98 to c3ec03a Compare June 20, 2026 18:22

@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 c3ec03a3. The single-source GSAP routing is the right move and the implementation is careful: every drag/rotate path commits through tryGsap*Intercept, the live preview uses the same computeDraggedGsapPosition math, and the multi-composition fixes (scoped soft-reload teardown, all-timelines keyframe scan, source-file map) are the necessary follow-ons. commitStaticGsapPosition / commitStaticGsapRotation correctly handle the idempotent re-nudge path. MotionPathOverlay's park-on-click debounce, primary-button gate, and unmount cleanup are clean.

I cross-checked the deletions against #1611's gesture-recording connect logic and they align — the --hf-studio-offset strip + tl.set static path replace the CSS-var channel cleanly.

Looks good to me overall. A few things worth a second look:

Concerns
useGsapAwareEditing.ts:101-122 (drag) and :164-181 (rotation) — both branches now drop the handled check and the CSS fallback. That's correct under single-source, BUT when STUDIO_GSAP_DRAG_INTERCEPT_ENABLED=false (default true, line 99 in manualEditingAvailability.ts — but it IS overridable), the function returns nothing and the drag is silently no-op'd. The DOM CSS path is now unreachable. If anyone flips that env to false (the flag's purpose: "opt-in until its recording path is hardened"), drag/rotate stop working entirely. Either remove the flag (it's now load-bearing always-on) or restore the DOM fallback inside the !STUDIO_GSAP_DRAG_INTERCEPT_ENABLED branch. Right now the flag is half-removed — toggle works but it kills editing.
gsapSoftReload.ts:163-170pluginScript.onerror = () => executeScript() runs the timeline script even when the MotionPathPlugin CDN fetch fails. The script will then throw at gsap.registerPlugin(MotionPathPlugin) or at the first motionPath: usage, the timeline doesn't re-register, but deferredToAsync=true already made applySoftReload return true optimistically. Caller (commitMutation) thinks the soft-reload succeeded while the iframe is broken until the next full reload. Suggest: on plugin error, trigger a full reload (caller's fallback path) instead of executing a doomed script.
gsapDragCommit.ts:211-233 (commitFlatViaKeyframes runtime-read block) — iframeWin as any discards type safety on the most-edited surface. Three reads (gsapLib, el, timelines, mainTl) and a seek/getProperty/set ladder, all under a single try/catch that falls back to identity values silently. If any one throws (e.g. seek cross-realm error), the 0% keyframe ends up at identity — the very phantom this PR is trying to kill. Suggest: tighten the cast to the IframeWindow shape from gsapSoftReload.ts (export it) and split the try/catch by step so the failure mode is loggable.
MotionPathOverlay.tsx:265-290dblclick listener on window, not on the iframe or pan-surface. The bounds check at L274 catches it, but a stray dblclick on the studio header would still fire onDbl (it returns at the r.left/right check, fine). My concern: when createMode flips false mid-listener-lifetime (e.g. another commit re-enters create mode then exits), the deps [createMode, selection, compositionSize, iframeRef, commitMutation] will re-attach. Each remount calls commitCreatePath exactly once per dbl, so no leak — but consider listening on the iframe-overlay surface directly so out-of-bounds dblclicks don't even reach the handler. (Borderline nit.)

Nits
gsapSoftReload.ts:131for (const child of tl.getChildren(true)) + child.targets() — typed as Array<{ targets?: () => Element[] }> but accessed without targets?.() optional. Eslint no-non-null-assertion would catch this; the inner if (typeof child.targets === "function") saves it at runtime. Use the optional call syntax for clarity. (nit)
MotionPathOverlay.tsx:259useEffect(() => () => clearTimeout(parkTimerRef.current), []) cleans the timer on unmount but not when animId changes mid-drag. The selected animation can change underneath a pending park (250ms is enough to swap selections); the parkPlayheadOnKeyframe(anim, …) capture at L429 closes over the prior selectedGsapAnimations. Probably fine because anim is captured via find, but consider clearing the park timer when animId changes. (nit)
MotionPathOverlay.tsx:497-507kfMenu state holds percentage AND tweenPercentage set to the same ref.pct. With #1611's clipToTweenPercentage, are these two values supposed to differ here, or is ref.pct already tween-relative? If the latter, drop the duplicate field. (nit/question)
gsapRuntimeBridge.ts:73-87 — the scoring heuristic in findGsapPositionAnimation weights targetSelector === selector at +8 and ,-multi-selectors at -5, but a single-element selector that matches one of several comma-separated selectors won't fire the +8. Edge case, but worth a comment. (nit)

Questions
gsapSoftReload.ts:83-86 regex extracts __timelines["key"] literal-key forms only. Is there any path in the codegen that produces a computed key (__timelines[someVar])? If so the regex misses it and the function bails to full reload — documented behavior, just verifying that's the deliberate trade.
commitStaticGsapPosition/commitStaticGsapRotation — when existingSet exists but is on a DIFFERENT selector (e.g. a comma-list that includes ours), update-property updates the whole list. Is the bridge's findPositionSetAnimation/findRotationSetAnimation guaranteed to pin to an exact-selector single-target? I see the equality a.targetSelector === selector at L317/378 — confirms exact match. Good. (Confirmed in code, no action needed.)
useDomEditOverlayGestures.ts rotation path now goes through applyRotationDraftViaGsap with a CSS fallback only when gsap is absent. If iframe.contentWindow.gsap is loaded but the element has transform: … set inline by other studio code, the gsap.set({rotation}) should compose correctly — but does tryGsapRotationIntercept need to clear residual --hf-studio-rotation CSS vars on commit (the way the position path clears --hf-studio-offset via restoreOffset)? I didn't see a rotation-side analog.

What I didn't verify
• Behavior at a sub-composition selection edit when the source-file map (setCompositionSourceMap) hasn't yet been populated by NLELayout (first-frame race).
• The interplay between MotionPathOverlay's 250ms park debounce and parkTimerRef cleanup if selection changes within the debounce window.
applySoftReload behavior when two compositions own the same __timelines["key"] (regex match → both kept/torn-down). Not realistic in current pipeline, but worth a sibling assertion.

— 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 c3ec03a3. Concur with @james-russo-rames-d-jusso — the single-source GSAP routing is the right architectural move; the implementation is careful on the canonical paths but ships with a load-bearing flag-half-removal and carries forward the #1608 facade regression in the most user-visible place.

Verified at HEAD:

  • useDomEditSession.ts:414-418 correctly aliases the return shape so external consumers' handleDomPathOffsetCommit/handleDomRotationCommit field calls now resolve to the GSAP-aware variants (handleGsapAwarePathOffsetCommit / handleGsapAwareRotationCommit). Naming reads slightly drift-y (external callers think DOM, get GSAP) — refactor-grade follow-up only.
  • readAppliedStudioPathOffset dormant-var path verified clean (manualEditsDom.ts:79-83): returns {x:0, y:0} when inline translate doesn't reference STUDIO_OFFSET_X_PROP; returns readStudioPathOffset(...) only when the var is actually referenced. Tests at manualOffsetDrag.test.ts cover both applied-translate AND dormant-var paths. No phantom-offset re-introduction.

Concur with @james-russo-rames-d-jusso's BLOCKER (flag-half-removal):

useGsapAwareEditing.ts:101-122 (drag) and :164-181 (rotation) at HEAD: both branches drop the handled check + the CSS fallback (handleDomPathOffsetCommit / handleDomRotationCommit removed from params + dispatch). When STUDIO_GSAP_DRAG_INTERCEPT_ENABLED=false AND hasGsapAnims=false, the function returns void with no commit AND no CSS fallback. Drag/rotate stop working.

The flag's docstring says "opt-in until its recording path is hardened" — implies it's flippable. Right now it's load-bearing always-on but still presented as opt-in. Either delete the flag outright OR restore the CSS fallback inside the !STUDIO_GSAP_DRAG_INTERCEPT_ENABLED branch. Maps to band-aid bar pattern #2 (contradictory rules: flag says opt-in, code treats it as always-on) + #3 (silent scope gap: a documented escape hatch is broken) + the flag-companion pattern. Rames + I converge on hard request-changes.

Net-new — facade-await regression in the most user-visible place:

#1608's useGsapAwareEditing.ts:240-246 commitMutation facade lost its await (see my #1608 review). #1567 doesn't introduce that regression, but it DOES make the consumer in useGestureCommit.ts the user-visible failure mode. Verified at this PR's HEAD c3ec03a3:

  • useGestureCommit.ts:127, 172, 187, 200await liveSession.commitMutation(...)
  • useGestureCommit.ts:93showToast(\Recorded ${sortedPcts.length} keyframes`, "info")`
  • :98store.requestSeek(recordingStartTimeRef.current)
  • :100setGestureState("idle")

liveSession = domEditSessionRef.current resolves to useDomEditSession's return object, whose commitMutation is the broken facade.

This is the canonical HF #1558 shape — success toast + seek + state-flip all fire BEFORE the save lands. The fix lands at #1608; this PR is the consumer that propagates the lie to the most user-visible code path. Worth pinning the dependency: this PR shouldn't merge before #1608's facade is fixed, because the gesture-recording flow is materially in the "Recorded N keyframes" UX line you ship.

Concur with @james-russo-rames-d-jusso on the rest:

  • gsapSoftReload.ts:163-170pluginScript.onerror = () => executeScript() runs the timeline script even when MotionPathPlugin CDN fetch fails. The inner script throws at registerPlugin(MotionPathPlugin) or at first motionPath: usage; deferredToAsync=true already short-circuited verifyTimelinesPopulated, so caller thinks soft-reload succeeded while iframe is broken until next full reload. Borderline blocker depending on real-world plugin-fetch failure rate; fall back to full reload on plugin error is the cleaner shape.
  • gsapDragCommit.ts:211-233 iframeWin as any typecast + monolithic try/catch swallows step-level failures silently — the 0% keyframe lands at identity values, which is the very phantom this PR is supposed to kill. Tighten the cast + split the try/catch.
  • MotionPathOverlay.tsx:265-290 global dblclick on window — concur, attach to pan-surface or scope to iframe overlay.
  • Rest of nits also concur.

Net-new — vestigial interface fields:

DomEditOverlay.tsx at HEAD still carries recordingState?: GestureRecordingState; and onToggleRecording?: () => void; as optional props (lines 67-68), never consumed in the body after the gesture-record badge removal. NIT — vestigial-interface pattern; clean up alongside the badge deletion.

Net-new — no unit test for new delta-at-grab-point:

useGestureRecording.ts body says "removes pointerElementOffset / element-center-snapping; fixed wrong 0% keyframe bug." No useGestureRecording.test.ts exists at HEAD asserting the new grab-point anchoring vs the removed element-center-snapping. Behavior change is small, risk is bounded, but no regression net. NIT.

Verdict: BLOCK. Two hard request-changes (flag-half-removal + facade carryover in gesture recording) + one borderline-blocker (plugin CDN false-success). Once those land, rest is comment-level. Strongly suggest the #1608 facade fix lands first (or alongside), since gesture recording's "Recorded N keyframes" toast is the canonical HF #1558 shape and lives in this PR's diff-adjacent surface.

Review by Via

…eline

Dragging or rotating an element writes into the GSAP timeline (the single source of
truth) instead of a parallel --hf-studio-offset / --hf-studio-rotation CSS var: static
elements commit a tl.set (idempotent on re-edit), tweened elements edit keyframes, and
the live preview moves via gsap.set so what you see equals what is written and renders.
Removes the dual-channel CSS-var/transform reconciliation behind the
fling / disappear / runaway / double-stack / wrong-start bug class — for BOTH position
and rotation (gesture base read from the gsap transform, gsap.set live preview, tl.set/
keyframe commit, dropped the handleDom*Commit CSS fallbacks).

Subcompositions edit the same single-source way, which surfaced and fixes:
- resolve a subcomp element's source file via the composition-id map (the runtime drops
  the source linkage when inlining the subcomposition);
- a selected element's selection box AND motion path use basic visibility, not the
  occlusion heuristic (a backgroundless opacity-1 scene above it is not an opaque cover);
- soft reload rebuilds ONLY the committed composition's timeline, leaving other
  compositions' timelines intact (no cross-composition revert);
- read keyframes from the element's OWN composition timeline (scan all timelines, not
  the first unstable key);
- delete-all uses a soft reload too, so editing no longer hard-reloads the iframe.
…softReload onerror, tighten runtime ladder, per-group gestures

- DROP STUDIO_GSAP_DRAG_INTERCEPT_ENABLED: single-source GSAP intercept is the only
  position/rotation channel; the false branch silently killed drag+rotate (and let GSAP
  elements into the keyframe-corrupting CSS path). Removed flag + dead branch + env def + tests.
- gsapSoftReload: plugin onerror no longer fakes success — signals onAsyncFailure so the caller
  full-reloads; honors __hfMotionPathPluginLoading so a concurrent reload can't queue a dup script.
- gsapDragCommit: resolveDragRuntime narrows the as-any ladder; a mid-seek throw logs + drops
  partial reads (no phantom identity) and re-applies the drag override in finally.
- MotionPathOverlay: park-timer cleanup keyed on animId change.
- useGestureCommit: partitionKeyframesByGroup wraps the add-with-keyframes sites (per #1611 review).
@miguel-heygen miguel-heygen force-pushed the feat/studio-keyframes-flag branch from 7268f63 to 151f36b Compare June 20, 2026 21:20
@miguel-heygen miguel-heygen force-pushed the feat/studio-gsap-keyframe-editing branch from c3ec03a to 5ef68d0 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.

3 participants