Skip to content

feat(studio): motion-path geometry + commit helpers#1609

Open
miguel-heygen wants to merge 2 commits into
feat/studio-drag-infrafrom
feat/studio-motionpath-helpers
Open

feat(studio): motion-path geometry + commit helpers#1609
miguel-heygen wants to merge 2 commits into
feat/studio-drag-infrafrom
feat/studio-motionpath-helpers

Conversation

@miguel-heygen

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

Copy link
Copy Markdown
Collaborator

Summary

This PR adds the pure, testable core of the on-canvas motion-path editor: geometry construction, gesture-to-source-mutation commit helpers, selection resolution, and a preview-occlusion visibility test. Everything here is framework-free (no React, no DOM coupling beyond the one occlusion helper), so the logic that turns a live tween into a draggable polyline — and turns a drag into a GSAP source edit — is unit-tested in isolation. The overlay component itself lands later in the stack; this is the substrate.

Net change: 6 files, +537 lines (4 source modules, 2 test files). No existing code is modified except one additive export in domEditOverlayGeometry.ts.

What's changed

motionPathGeometry.ts (new)

Converts a live tween from readRuntimeKeyframes (ReadTween) into renderable overlay geometry, in composition space.

  • MotionNodeRef — discriminated union: { type: "keyframe"; pct } (an x/y position keyframe at a tween-relative percentage) or { type: "waypoint"; index } (a motionPath anchor at a source index).
  • MotionPathNode / MotionPathGeometry — a node is { x, y, ref }; the geometry is { kind: "linear" | "arc", points, nodes } where points is an SVG polyline string.
  • buildMotionPathGeometry(read) — returns geometry or null. kind is "arc" when the tween carries an arcPath, else "linear". Arc keyframes are treated as path waypoints (index-aligned with source); linear nodes carry their tween-relative percentage. Single-axis tweens are handled explicitly (detects tweenHasX/tweenHasY, defaults the missing axis to 0) so an x-only or y-only tween still draws a straight path. A keyframe that omits an axis the tween does animate is skipped. Returns null for no positional motion or fewer than two valid nodes. Arcs are drawn as polylines through waypoints (dense curve sampling is a deliberate later refinement).
  • nearestPointOnPath(px, py, nodes) — projects a point onto the closest polyline segment, returning { x, y, segIndex, t, dist }. Drives the ghost "add" node and where a new node lands. Returns null for fewer than two nodes.

motionPathCommit.ts (new)

Maps a finished gesture to a GSAP source mutation through the selection-bound commit facade (which owns soft reload, undo snapshot, save-failure feedback). All helpers pass softReload: true + a human-readable undo label.

  • commitNodeupdate-keyframe (keyframe) or update-motion-path-point (waypoint).
  • commitAddWaypointadd-motion-path-point; commitAddKeyframeadd-keyframe (tween-relative %); commitRemoveWaypointremove-motion-path-point; commitCreatePathadd-motion-path (default NEW_PATH_DURATION 1.5s).

motionPathSelection.ts (new)

Selection/animation resolution, split out to avoid a circular import between the overlay and its diagnostics.

  • selectorFor(sel)#${CSS.escape(id)} preferred, falls back to sel.selector.
  • editableAnimationId(animations, kind) — picks the one animation editable on-canvas: literal, statically resolved, matching the rendered geometry kind. ok requires !hasUnresolvedKeyframes && !hasUnresolvedSelector && !provenance. Returns null for dynamic / helper-provenance tweens (read-only).

domEditOverlayGeometry.ts (modified — additive)

  • isElementVisibleInPreview(el) (new export) — extends isElementVisibleForOverlay with an occlusion hit-test. The composition stacks scenes by z-index and fades them in, so an earlier element can stay opacity 1 yet be covered. Walks the painted stack via elementsFromPoint at five sample points; a point "sees" the element if reached before any unrelated effectively-opaque element (transparent covers skipped). Helper effectiveOpacity(el, win) computes cumulative ancestor opacity. If every in-viewport sample is blocked it's hidden; if nothing was testable it is not hidden on that basis.

Why

The overlay must reliably (1) draw the path a tween traces and (2) translate a drag into a precise source edit — both easy to get subtly wrong (single-axis tweens drawing no path, arc waypoint indices drifting, an "add node" on the wrong segment, a node committing the wrong mutation). Pure modules let us pin that down with unit tests before any pixels/React state, keep the overlay thin, and break a circular import. The occlusion test exists because the stacked-scene fade-in makes naive opacity checks report covered elements as visible.

Testing

Two new Vitest suites:

  • motionPathGeometry.test.tsbuildMotionPathGeometry: linear path with keyframe refs; order/percentages preserved; arc path with waypoint refs; null for opacity-only; single-axis x-only ("0,0 -260,0") and y-only ("0,0 0,500") regression cases; skips keyframes missing a coord; null for <2 nodes / null input. nearestPointOnPath: nearest-segment projection, t at endpoints, second-segment selection, endpoint clamp, null for <2 nodes.
  • motionPathCommit.test.tseditableAnimationId (arc, position-keyframe, null for dynamic + helper-provenance + no-match); commitNode routes keyframe→update-keyframe, waypoint→update-motion-path-point (both softReload: true); add/remove waypoint; commitAddKeyframe; commitCreatePath (1.5s default). All commit assertions verify both payload and softReload: true.

Stack

Part of the GSAP keyframe/motion-path stack: #1553 → #1554 → #1555 → #1607 → #1608 → #1609 → #1610 → #1611 → #1567 → #1612 → #1613 → #1605. This is #1609, on top of #1608. Builds independently; the combined diff across the stack is byte-identical to the originally-reviewed work.

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

Review @ ec4218d2 — pure-additions substrate for the motion-path overlay (geometry, commit helpers, selection resolution, occlusion test). Reviewed standalone + against #1610's consumer call sites.

Looks good

  • No tautology risk. Every new export has a non-test production consumer in #1610: buildMotionPathGeometry (MotionPathOverlay.tsx:205), nearestPointOnPath (:433/:445), editableAnimationId (:336), selectorFor (:241/:260), commitNode (:420), commitAddWaypoint (:455), commitAddKeyframe (:466), commitRemoveWaypoint (:474), commitCreatePath (:279). isElementVisibleInPreview is consumed by both MotionPathOverlay AND useDomEditOverlayRects.ts:151 — strong cross-overlay parity, exactly the kind of shared visibility rule that prevents drift.
  • Single-axis tween regression is explicitly tested (points: \"0,0 -260,0\" and \"0,0 0,500\"); good catch on the prior skip-if-missing-y foot-gun.
  • nearestPointOnPath handles coincident nodes (len2 === 0 → t = 0); endpoint clamps tested; segment indexing tested.
  • buildMotionPathGeometry returns null cleanly on <2 nodes, null input, opacity-only tweens. NaN-safe via Number.isFinite axis guard.
  • editableAnimationId's !a.provenance gate keeps helper tweens read-only — matches the overlay's drag-disabled path.

Concerns

  • isElementVisibleInPreview couples to a one-way fade assumption (domEditOverlayGeometry.ts:38-45). The docstring says "this composition stacks scenes by z-index and fades them IN (never out)." The effectiveOpacity > 0.01 cutoff treats anything mostly-opaque as an opaque cover and bails. If a future composition ever fades scenes OUT (or animates opacity through mid-values during a crossfade), an element under a 0.5-opacity cover will be classified as occluded → motion path + selection box both disappear during the crossfade. Worth either (a) a brief comment in the docstring pinning this to a runtime invariant, or (b) loosening the cutoff (e.g. > 0.85) so a mid-fade isn't read as a hard cover. Cite the assumption explicitly somewhere a future scene-author will see it.
  • Occlusion sample set is corners + center (OCCLUSION_SAMPLE_POINTS at 20%/50%/80%). Donut-shaped elements (large bounding box with transparent middle) would be wrongly classified as visible when all five samples hit transparent content. Today's text/image scenes don't hit this, but worth a code comment noting the assumption — it's the kind of thing a future scene type (a frame/ring decoration) silently regresses.

Nits

  • nearestPointOnPath returns t clamped past-end → for a point past the last node, you get segIndex = lastSegment, t = 1. Callers in #1610 use np.segIndex + 1 for insertion, which would insert AFTER the last node. Reasonable, but the JSDoc comment doesn't say whether "past the end" is the intended insertion semantics. One line clarifying "t = 1 on the last segment means insert at the end" would help. (nit)
  • The doc comment block header \"ponytail: the arc is drawn as a polyline...\" on line 35 of motionPathGeometry.ts is inside the buildMotionPathGeometry JSDoc but nearestPointOnPath is declared first below it. Cosmetic doc-comment ordering only. (nit)

What I didn't verify

  • I didn't run the test suite locally; I verified test shapes by reading.
  • I didn't trace every mutation type (update-keyframe, add-motion-path-point, etc.) into the GSAP parser to confirm the payload shape is what commitMutation expects — relied on PR body + the commit helpers' tests covering payload shape.

Stamp-routable from my side once the occlusion docstring caveat is acknowledged (either fix or accept-as-known-assumption). Geometry math + helpers are solid.

— 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 ec4218d2. Concur with @james-russo-rames-d-jusso — solid foundation slice. Pure helpers with strong unit-test coverage and the right cross-overlay parity hook.

Verified at HEAD:

motionPathGeometry.test.ts coverage matrix is comprehensive and asserts against expected payloads, not just verify-no-throw:

  • linear path + ordering/percentages preserved
  • arc path with waypoint refs (kind === "arc", indices 0/1/2)
  • opacity-only → null
  • single-axis x-only ("0,0 -260,0") and y-only ("0,0 0,500") — explicit regression cases
  • skip-keyframe-missing-axis (excludes keyframes missing a coordinate without throwing)
  • <2 valid nodes / null input
  • nearestPointOnPath: nearest-segment + t-fraction, t-clamp at endpoints, second-segment selection, past-segment endpoint clamp, <2 nodes → null

No stubs, no escape hatches.

Commit helper dispatch — verified handlers exist in routes/files.ts at HEAD:

  • commitNode (keyframe) → update-keyframe (files.ts:1033)
  • commitNode (waypoint) → update-motion-path-point (:1082)
  • commitAddWaypointadd-motion-path-point (:1088)
  • commitAddKeyframeadd-keyframe (:1020)
  • commitRemoveWaypointremove-motion-path-point (:1094)
  • commitCreatePathadd-motion-path (:1097)

All six route to real handlers in the recast switch. Note: routes/files.ts carries TWO switch blocks (executeGsapMutationRecast + executeGsapMutationAcorn), each with add-keyframe / update-keyframe cases — these are cutover branches gated by STUDIO_SDK_CUTOVER_ENABLED, not racing. Dispatch IS determinate. But the acorn switch is missing all four motion-path cases (see my #1555 review) — dark-launch breakage when the cutover flag flips.

Concur with @james-russo-rames-d-jusso:

  • isElementVisibleInPreview couples to a one-way-fade assumption at domEditOverlayGeometry.ts:38-45. Worth either a docstring caveat or loosening the effectiveOpacity > 0.01 cutoff so mid-crossfade isn't read as a hard cover. The shared rule with useDomEditOverlayRects is the right call (path + selection box share one truth) — that's the strongest cross-overlay parity win in the PR. Just pin the assumption.
  • Donut-shaped occlusion samples: corners + center won't catch transparent middles. Comment naming the assumption suffices for now.

Net-new (NIT, soft cross-PR drift to #1610):

readRuntimeKeyframes (already on main) explicitly handles the multi-tween case by returning the tween under the playhead — its source comment names this production case. editableAnimationId in this PR picks the FIRST animation matching the predicate, playhead-blind. For an element with two position-keyframed tweens at disjoint time ranges, the geometry is drawn from playhead-tween but commitNode/commitAdd* will write via the first-match animationId — potentially the wrong tween. Single-tween case (dominant) is correct.

Likely fails loud server-side (id mismatch → structured error) rather than silent wrong-write, but the runtime helper's own comment documents this as a real production case. NIT not BLOCK; future fix would key the picker on the same time window the geometry came from. Surfaces at the commit boundary in #1610.

LGTM. No band-aid patterns. Tests assert against expected output, helpers are real-consumer-grounded.

Review by Via

@miguel-heygen miguel-heygen force-pushed the feat/studio-drag-infra branch from 57f4cd8 to c66e82a Compare June 20, 2026 21:20
@miguel-heygen miguel-heygen force-pushed the feat/studio-motionpath-helpers branch from ec4218d to 395c6f7 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