fix(studio): property panel group-aware keyframe routing#1358
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
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verdict: 🚨 BLOCKER — onMoveKeyframe lookup uses wrong percentage space.
Blocker
StudioPreviewArea.tsx:188 — move keyframe silently no-ops on any tween where position ≠ elStart or duration ≠ elDuration.
anim.keyframes.keyframes.find((k) => k.percentage === oldPct)anim.keyframes.keyframes[i].percentage is tween-pct (the raw parser output), but oldPct from the diamond-drag callback is clip-pct. When tween-pct ≠ clip-pct, the find() returns undefined and the move silently no-ops.
This is the same bug shape #1357 just fixed for deleteKeyframe (which now uses tweenPercentage). One-line fix: line 187 already computes tweenOldPct; use that on line 188 instead of oldPct.
A single mixed-position-tween test would have caught it.
R2 carry-overs
- Architectural plumbing for "edits stay scoped to clicked diamond" ✅ —
PropertyPanel.tsx:230-234—animIdForProp(prop)maps property → group → animation, defaulting to legacygsapAnimId. Right surface for Vance's UX bug fix. - 3D-transform diamond handlers ✅ —
propertyPanel3dTransform.tsx:46,78-83,114-119— scale/z diamond handlers route throughresolveAnimIdForPropwith sensible fallback. - Delete handler reads cached
propertyGroup✅ —StudioPreviewArea.tsx:163-170— falls back to first keyframed anim; useskf?.tweenPercentage ?? pctfor the wire payload.
Other concerns
rdpSimplify.ts:97-124— framing nit.simplifyGestureSamplessignature widens to accept(key) => numberbut the only caller in this PR (useGestureCommit.ts:80) still passes literal5. The per-property variant is consumed in #1359. PR title overclaims "Fix per-property epsilon" — this PR is just the API foundation; the actual fix is in #1359. Not a bug, but framing.
Test coverage
Zero new tests across 4 files (StudioPreviewArea.tsx, PropertyPanel.tsx, propertyPanel3dTransform.tsx, rdpSimplify.ts). Most concerning gap: the animIdForProp group-routing logic and the cache-driven group lookup in delete/move handlers — those are the core UX-fix mechanic and have no coverage.
Cross-PR coherence
Reads cached?.keyframes.find(... propertyGroup) shape emitted by #1357 ✅. Reads anim.propertyGroup shape emitted by #1355's parser ✅.
— Review by Rames D Jusso
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 — concerns. The R2 blocker landed for remove but NOT for add.
✅ onMoveKeyframe remove-half fixed at StudioPreviewArea.tsx:178-194. Handler now reads cachedKf = cached?.keyframes.find((k) => Math.abs(k.percentage - oldPct) < 0.2), derives tweenOldPct = cachedKf?.tweenPercentage ?? oldPct, passes tweenOldPct to handleGsapRemoveKeyframe(anim.id, tweenOldPct) at line 190. Group routing also lands: picks anim via cachedKf?.propertyGroup first. Cache now carries tweenPercentage + propertyGroup (verified at playerStore.ts:9-12).
🚨 onMoveKeyframe add-half NOT fixed — symmetric bug. Line 192 still passes raw newPct (clip-relative) to handleGsapAddKeyframe(anim.id, newPct, prop, val). For any tween whose start ≠ element start or duration ≠ element duration, the dropped position lands at the wrong tween-pct — the move silently shifts the keyframe to a different absolute time. Fix: compute tweenNewPct from the same cached cache or via (elStart + newPct/100 * elDuration - tweenStart) / tweenDuration * 100, mirror the remove path.
✅ onChangeKeyframeEase scoping made explicit. At :173-177 iterates all selectedGsapAnimations and applies ease to each. R3 made the all-anims behavior explicit rather than accidental — same outcome (ease applies to position + scale + rotation together) but intent is now visible. Acceptable per Via's "confirm UX intent" out, worth a UX-product nod. Same broadening applied to onDeleteAllKeyframes at :161-163.
Tests: zero net tests added. Same as R2. A single mixed-position-tween test would catch both halves of the percentage-space bug.
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
animIdForProphelper routing keyframe diamonds to correct property-group animationresolveAnimIdForPropprop in 3D transform panel for scale/z diamond routingpropertyGroupfor routingrdpSimplify(0.01 for opacity/scale)Test plan
Stack: 5/7 — depends on #1357