fix(studio): per-property-group intercept routing + drag/resize fixes#1356
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.
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. R2 hotspot of the stack — most R2 carry-overs from monolithic #1344 verified resolved here. Top-priority flag regression did NOT recur.
R2 carry-overs
- ✅
STUDIO_GSAP_DRAG_INTERCEPT_ENABLEDflag default — RESOLVED.manualEditingAvailability.ts:90-94shipsfalse; test L30 expectsfalse. The accidental flip from monolith #1344 R2 did not survive the restructure. (Matches #1341's disable-by-default intent.) - Soft-reload tests — not in this PR's diff. Carry to #1357.
tryGsapResizeInterceptsize — unchanged.gsapRuntimeBridge.ts:255-425≈ 170 LOC, same shape as monolith R2. Restructure did not refactor down. (Concern, not blocker — single big resize fn is the price of routing all 3 sub-paths through one entry.)- Duplicate IIFE outside-range — partial.
synthesizeIdentityPropshelper extracted at L242-251 ✅. Outside-range still has an IIFE wrapper at L354-368 (now?? (() => {...})()lazy-fallback shape, but still inline). (Nit.) - Cache triple-write — not in this PR. Carry to #1357.
- RC5 throw flip ✅ —
useGsapScriptCommits.ts:136-144throws on!result, silent return onresult.changed === false. File untouched by this PR (carried fine from #1355). activeKeyframePctconsumption ✅ —gsapRuntimeBridge.ts:307-309+gsapDragCommit.ts:151-153. Last-clicked-diamond override works.
New finding
- Hand-rolled
fetch()+window.location.hashparsing in drag commit —gsapDragCommit.ts:280-292. Outside-range drag path scrapes the project ID fromwindow.location.hashandfetches/api/projects/.../gsap-animations/...directly instead of going through the existingprojectIdRef/mutateGsapScriptchannel. User-gesture path so it's not a render-time determinism violation, but it's an ad-hoc backend call inside drag commit with no auth/error/retry plumbing and a fragile hash regex. Concern — wire it through the established mutation channel instead.
Stack coherence
PROPERTY_GROUPS taxonomy from #1354 consumed correctly — propertyGroup === "position" | "scale" | "rotation" predicates (not hardcoded property names). resolveGroupTween is the consistent entry point across drag/resize/rotate.
What I didn't verify
Studio vitest suite — CI shows no test job ran on this PR (preflight + previews only). Could not confirm manualEditingAvailability.test.ts:30 actually passes in CI vs. just looking right in source.
— 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 — blocker. Two R2 findings did not land + a new path-prefix bug strictly worsens one.
🚨 gsapDragCommit.ts:284 — window.location.hash regex still there, AND now mismatched. R3 shape:
`/api/projects/${encodeURIComponent(window.location.hash.match(/project\/([^?/]+)/)?.[1] ?? "")}/gsap-animations/${encodeURIComponent(pid)}`
Regex matches /project/ (singular) while the API path is /api/projects/ (plural). If HF routes use either shape, mid-drag the captured group is "" → fetch hits /api/projects//gsap-animations/... → silent 404 → empty allAnims[] → "no existing position tween, create one" path → duplicate position tween on every from()/fromTo() drag that's out-of-range. Fix unchanged from R2: thread projectId through GsapDragCommitCallbacks or use the already-wired fetchFallbackAnimations.
🚨 gsapDragCommit.ts:151-153 — commitKeyframedPosition clears activeKeyframePct BEFORE await mutation. R3 shape:
const { activeKeyframePct, setActiveKeyframePct } = usePlayerStore.getState();
const pct = activeKeyframePct ?? computeCurrentPercentage(selection, anim);
if (activeKeyframePct != null) setActiveKeyframePct(null);
await callbacks.commitMutation(...)setActiveKeyframePct(null) fires synchronously before await callbacks.commitMutation(...). If the mutation throws (RC5 flip from #1355), the targeted keyframe is lost and retry recomputes from playhead — silently shifting the keyframe. Fix: move into .finally() after success, or guard with try/catch that restores on throw.
tryGsapResizeIntercept grew ~+60 LOC at gsapRuntimeBridge.ts:255-425 (~170 → ~230 with the new outside-range branch synthesizing identity props + replaying keyframes at lines 343-407). Not blocking, but the structural smell grew. Worth a follow-up split into applyInRangeResize / applyOutOfRangeResize.
New concern (surfacing, not deep-diving): resolveGroupTween (lines 121-175) calls commitMutation with split-into-property-groups then immediately re-fetches via fetchFallbackAnimations. No skipReload: false guarantee or ordering proof that the re-fetched list sees the post-split state. If the server mutation is async-coalesced (coalesceKey pattern used elsewhere in this file), the re-fetch may see pre-split tween → infinite-recurse on next intercept. Worth a closer look.
Test gap (still): zero new tests for the new from()/fromTo() drag-extension path (~120 LOC), zero for the new outside-range resize path (~60 LOC), zero for resolveGroupTween.
Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verification pass — R3 item checked against current HEAD (8ae1921).
commitKeyframedPosition — activeKeyframePct cleared before await
The fix is in #1360, not this PR. In this PR's HEAD (gsapDragCommit.ts:153):
if (activeKeyframePct != null) setActiveKeyframePct(null);
await callbacks.commitMutation(...); // clear happens before this
The same pre-clear pattern exists in gsapRuntimeBridge.ts:309-310.
#1360's PR body claims the fix ("clear moved to after mutation succeeds") and #1360's gsapDragCommit.ts:164 confirms it — clear is after the await there. Since #1360 is above this PR in the stack, the fix will land when the stack merges in order. As a standalone PR this blocker is unresolved; the fix is deferred to #1360.
If the stack merges in order (bottom-up), this is fine architecturally. Flagging for awareness — stamp decision is Vance's call given the stacked context.
— Vai (verification pass)
Fallow audit reportFound 20 findings. Duplication (6)
Health (14)
Generated by fallow. |
3 participants
Summary
{x,y}to position groupdata-hf-studio-original-widthfor correct scale computationresolveGroupTweenhelper for group-aware tween resolutionautoKeyframeEnabledguard on all 3 GSAP-aware commit pathsTest plan
Stack: 3/7 — depends on #1355