fix(studio): gesture recording replaces existing position keyframes#1359
Conversation
Add PropertyGroupName type system (position/scale/size/rotation/visual/other), PROPERTY_GROUPS constant, classifyPropertyGroup/classifyTweenPropertyGroup functions. Parser generates group-aware animation IDs, resolves position strings (+=, -=, <, >), uses numeric matching with 2% tolerance, and preserves IDs across all mutations.
…mutations Server-side mutations for atomic property-group splitting and keyframe replacement. Client commitMutation returns early on changed:false instead of throwing.
Rewire GSAP runtime bridge for property-group routing: drag sends only {x,y}
to position group, resize routes to scale group via data-hf-studio-original-width,
rotation routes to rotation group. Add resolveGroupTween helper, from-extend
with split-first-then-position-only pattern, autoKeyframeEnabled guards,
GSAP base + delta fix in drag draft, cancel-restores-GSAP-x/y from data attrs.
Tag cached keyframes with propertyGroup for group-aware operations. Add tweenPercentage for accurate keyframe matching, activeKeyframePct for diamond-click targeting, context menu offset, selected diamond z-index, clearProps after kill in soft reload.
Add animIdForProp helper routing keyframe diamonds to correct property-group animation. Wire StudioPreviewArea delete/move/toggle handlers to use propertyGroup for routing. Fix per-property epsilon in rdpSimplify.
miguel-heygen
force-pushed
the
fix/kf-property-panel
branch
from
c664e48 to
57ac327
Compare
miguel-heygen
force-pushed
the
fix/kf-gesture-recording
branch
from
9dbe446 to
da4a238
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verdict: approve-with-nits — one architectural Q for the author.
R2 carry-over for Vance's "should edit, not append" bug is partially addressed via a different mechanism than expected.
Findings
-
useGestureCommit.ts:120-122— usesselectedGsapAnimations.find(propertyGroup === "position" && targetSelector === selector)rather than theactiveKeyframePctplumbing ingsapRuntimeBridge.ts:307/gsapDragCommit.ts:151. Different shape: gesture-record fixes the duplicate-tween hazard (one position tween per element), not the replace-keyframe-at-active-pct hazard.Vance's monolith #1344 complaint was specifically about clicking a keyframe diamond then editing — this PR doesn't wire the diamond-click path into gesture recording. If this is intentional split (gesture = replace whole tween; drag = edit at active pct), call it out in the PR body. Otherwise there's a coverage gap on the specific UX symptom Vance reported.
-
useGestureCommit.ts:128-135—replace-with-keyframesmutation shape matchespackages/core/src/studio-api/routes/files.ts:413exactly (animationId,targetSelector,position,duration,keyframes). Wire-coherent. -
useGestureCommit.ts:67+:180— capturingselat start viacapturedSelectionRefis the right call (commit always targets the recorded element). ButliveSession.selectedGsapAnimationsat line 120 is still read live — if user reselects mid-record, thefind()returns nothing (different selector) and falls through toadd-with-keyframes. Correct fallback, just worth a comment. -
useGestureRecording.ts:130-134— sign fix onpointerElementOffset / scalelooks right. But the wheel-handlerpointerElementOffsetat:317-321is computed fromr.pointerwhich only updates on pointermove. If the first event is a wheel with no prior pointermove,r.pointeris its init value (0,0?) andelCenterViewportis element center → offset is huge-negative. Verifyr.pointeris seeded onpointerenter, not justpointermove.
Test coverage
Zero new tests for the replace-vs-append branch or the per-property epsilon. Behavior is timing-sensitive and the failure mode (duplicate tweens) is exactly what manual-test #1344 surfaced — a unit test for the branch logic is cheap.
Cross-stack
Consumes replace-with-keyframes from #1355 (verified wire-coherent), selectedGsapAnimations.propertyGroup from earlier stack work.
— Review by Rames D Jusso
Gesture recording uses replace-with-keyframes mutation to replace existing position-group tween. Fix N1 sign inversion and N9 wheel startPointer with pointerElementOffset subtraction.
miguel-heygen
force-pushed
the
fix/kf-gesture-recording
branch
from
da4a238 to
b4906c9
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R3 verification — approve-with-nits. Main R2 staleness concern resolved.
✅ useGestureCommit.ts:48-51 — capturedSelectionRef introduced. Selection snapshotted at recording start and used by commit. Via's staleness concern cleanly addressed.
❌ Architectural Q still open. Gesture commit at useGestureCommit.ts:74 still uses propertyGroup === "position" find, not activeKeyframePct. With #1357's activeKeyframePct state landing in the same stack, a reader will reasonably ask why gesture doesn't consult it. One sentence in PR body or a comment at line 72 ("gesture records the position-group tween for this element — activeKeyframePct is a UI-cursor concept and not the right axis here") would close the question.
❌ Per-property epsilons still inline at useGestureCommit.ts:46-49 — opacity → 0.01, scale* → 0.01, default → 5. Now isolated in one callback (better than scattered), but the next reviewer adding rotation/skew will have to hunt. const RDP_EPSILON_BY_KEY: Record<string, number> two lines above lands it.
Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
LGTM — Via reviewed, stamp applied.
— Vai
miguel-heygen
dismissed
vanceingalls’s stale review
The base branch was changed.
3 participants
Summary
replace-with-keyframesmutation to replace existing position-group tweenstartPointerwithpointerElementOffsetsubtractionTest plan
Stack: 6/7 — depends on #1358