feat(studio): route GSAP tween add/update/delete through SDK (§3.5 PR1)

[vanceingalls]

Summary

• sdkGsapTweenPersist helper added to sdkCutover.ts — routes add/set/remove GSAP tween ops through the SDK's acorn/magic-string path, with server fallback on SDK miss/error
• addGsapAnimation (from/to/fromTo only), updateGsapMeta, deleteGsapAnimation in useGsapAnimationOps now try SDK first
• updateGsapProperty, addGsapProperty, updateGsapFromProperty, addGsapFromProperty in useGsapPropertyDebounce now try SDK first
• removeGsapProperty / removeGsapFromProperty stay server-authoritative: null ≠ removal in upsertProp; deleteAllForSelector stays server-authoritative: no SDK op
• addGsapAnimation with method = "set" stays server-authoritative: GsapTweenSpec has no set method
• 5 new unit tests for sdkGsapTweenPersist

Open decisions

• removeGsapProperty/removeGsapFromProperty: upsertProp in the acorn writer treats null as a literal null value, not removal. To support removal via SDK, a removeGsapProperty SDK op would be needed (separate PR or accepted as server-authoritative for now).
• deleteAllForSelector: no SDK op exists. Stays server-authoritative.
• addGsapAnimation with method = "set": GsapTweenSpec doesn't have a set method; stays server-authoritative.

Test plan

• Build passes (bun run build)
• Tests pass (bun run test) — 37 sdkCutover tests
• Lint/format clean
• 600-line Studio gate passes
• Manual: open Studio on a GSAP comp, add/edit/delete a tween, confirm sdk_cutover_success telemetry fires and undo works

🤖 Generated with Claude Code

[vanceingalls]

• feat(sdk,core): ws-3 — unrollDynamicAnimations acorn port + SDK op #1501
• feat(sdk): stage 6 — arc path ops (setArcPath, updateArcSegment, removeArcPath) #1500
• feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes #1499
• feat(sdk,studio): ws-1.2 — percentage-based removeGsapKeyframe #1498
• feat(sdk,studio): ws-1.1 — add set method to GsapTweenSpec; route addGsapAnimation(set) through sdk #1497
• feat(sdk,studio): ws-4 — add history:false option; disable unused sdk undo in studio #1496
• feat(sdk): ws-a2 — applyDraft/commitPreview/cancelPreview → moveElement op #1490
• feat(sdk): ws-a1 — iframe preview adapter (hit-test + selection) #1489
• fix(studio): resolve cross-cutting SDK-cutover review findings #1471
• feat(studio): route GSAP keyframe add through SDK (§3.5 PR2) #1470
• feat(studio): route GSAP tween add/update/delete through SDK (§3.5 PR1) #1469 👈 (View in Graphite)
• chore(studio): document CSS-path position cut-over; GSAP-path deferred (§3.3) #1467
• feat(studio): route timeline trim/move through SDK setTiming (§3.2) #1466
• feat(studio): route element delete through SDK removeElement (§3.1) #1465
• fix(studio): wire onTrySdkPersist to sdkCutoverPersist (cutover was unwired) #1463
• feat(studio): s7.5 — delete shadow scaffolding; keep cutover flag (dark launch) #1462
• fix(studio): force-reload sdk session after undo/redo bypasses suppress window #1460
• feat(studio): stage 7 step 3c — sdk cutover for inline-style ops #1452
• feat(core): acorn GSAP read path with T6b differential corpus tests #1488
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[vanceingalls]

On the matter of #1469 — GSAP tween add/update/delete routed through the SDK (§3.5 PR1).

Intent

A new sdkGsapTweenPersist helper routes addGsapAnimation (from/to/fromTo only — set stays server-side), updateGsapMeta, deleteGsapAnimation, and the four property/from-property variants through sdkSession.addGsapTween / setGsapTween / removeGsapTween with server fallback on miss/error. removeGsapProperty / removeGsapFromProperty and deleteAllForSelector stay server-authoritative (documented). The hook signatures grow a substantial set of SDK deps.

Per-file observations

• packages/studio/src/utils/sdkCutover.ts:152-184 — sdkGsapTweenPersist's discriminated-union SdkGsapTweenOp is sensible (add carries the target hf-id, set/remove carry only the animationId). The add branch correctly gates on getElement(op.target) to avoid addGsapTween against a non-existent element; the set/remove branches do not have an analogous guard for the animationId. #1471 closes this via the before === after → return false no-op check, which subsumes a per-op existence check for these branches. So the right fix is "the next PR up" — but worth flagging as a band-aid that does not yet self-rescue.
• packages/studio/src/hooks/useGsapAnimationOps.ts:42-49, 64-71, 91-98 — Duplicate validation at boundaries: pattern #1 of the band-aid bar. The same five-field truthy guard if (sdkSession && writeProjectFile && editHistory && reloadPreview && domEditSaveTimestampRef) appears verbatim in three callsites in this file plus four more in useGsapPropertyDebounce.ts. This is precisely what #1471's "Stable sdkDeps. One memoized deps object shared by the 3 GSAP hooks" closes. Standing alone, #1469 is the band-aid; #1471 is the long-term fix. Treat the train as a unit, not a sequence.
• packages/studio/src/hooks/useGsapAnimationOps.ts:131-157 — autoId clobber risk. The new SDK-attempt branch executes before the assignGsapTargetAutoIdIfNeeded server-side id assignment is reflected back into the in-memory SDK session. If the server just assigned a fresh id, the SDK session still has the stale shape; addGsapTween will write the tween against the wrong target id. #1471's body promises a skip-when-just-assigned guard (if (!autoId && method !== "set" && ...)) and the diff verifies it lands at useGsapAnimationOps.ts:131. Sibling band-aid pattern, closed in the next PR. Same train-or-revert calculus.
• packages/studio/src/hooks/useGsapPropertyDebounce.ts:64-69, 79-86 — flushPendingPropertyEdit is now async and the unmount-effect cleanup calls void flushPendingPropertyEdit() without an await or abort. If the component unmounts mid-edit, the flush fires-and-forgets a write against a potentially stale session and reloads a torn-down preview. The setTimeout-based debounce wrapper has the same shape at line 76. Worth at least one comment acknowledging the race, or an abort-on-unmount via a ref guard. #1471 does not close this — it stays open under cover of "stable sdkDeps". Worth raising explicitly.
• packages/studio/src/hooks/useGsapPropertyDebounce.ts:316-320 — { kind: "set", animationId, properties: { properties: { [property]: value } } } — the doubled properties looks odd at first but reads correctly: SdkGsapTweenOp.set.properties: Partial<GsapTweenSpec> and GsapTweenSpec has properties: Record<string, unknown> as a field. So the outer properties is the spec-field selector, inner { [property]: value } is the actual property payload. Naming-shadow is hostile to skim-reading; consider naming the outer field partial or spec in SdkGsapTweenOp to lift the ambiguity.

Dispatch-chain audit

The new SDK-attempt branches short-circuit return before the server commitMutation / commitMutationSafely path. Verified each consumer of commitMutation (the trackGsapSaveFailure telemetry, the soft-reload, the keyframe-cache update at useGsapScriptCommits.ts:65-78) is bypassed when the SDK handles the op — by design. But: forceReloadSdkSession (added in #1471) is invoked from inside commitMutation. In #1469 standing alone, the SDK path does not have an equivalent stale-doc-reload trigger because forceReloadSdkSession doesn't exist yet. So between #1469 and #1471, a server-fallback after an SDK miss leaves the SDK session stale — exactly the bug #1471 calls out under "stale in-memory SDK doc." Sibling band-aid.

Stale-base hazard intersection

Touches useDomEditSession.ts, useGsapAnimationOps.ts, useGsapPropertyDebounce.ts, useGsapScriptCommits.ts, gsapScriptCommitTypes.ts. Main's #1474 modified useGsapAnimationOps.ts heavily for animationIds, and modified useSafeGsapCommitMutation.ts to use shared types from gsapScriptCommitTypes.ts. Squash-merging without a rebase will revert main's #1474 deltas. This is the highest-risk file in the batch for the stale-base hazard. Mandatory rebase before stamp.

CI

CI has not yet reported for this PR — gh pr view --json statusCheckRollup returns only Graphite + WIP + Mintlify, no Preflight result. Likely a transient queue state; verify before stamping that Preflight has actually been run and the same gsapSerialize.ts format drift is the only failure.

Verdict

minor-blockers at 7b669b95913d2711f802425088b849a199be525a — the SDK routing itself is well-formed but the PR ships multiple band-aid-shape patterns (duplicated guards, missing before === after check, autoId clobber risk, missing forceReloadSdkSession plumbing) that only #1471 closes. As part of the train this is fine; as a standalone merge it is not. The async flush-on-unmount race is not closed by #1471 and is worth raising regardless. Re-verify if HEAD moves before stamp.

Review by Via

[miga-heygen]

Review: S3.5 PR1 GSAP Tween SDK Cutover

Good work routing GSAP tween ops through the SDK. The pattern is consistent with the existing sdkCutoverPersist/sdkTimingPersist -- sdkGsapTweenPersist is a clean addition, fallback-on-error is correct, and the "ponytail" comments documenting intentionally server-authoritative ops are exactly the kind of breadcrumb future readers need. Tests cover the happy paths and the key edge cases (null session, missing element, SDK error fallback). Nice.

That said, I have a few things to flag:

---

Bug: sdkDeps object identity instability defeats debounce coalescing

useGsapPropertyDebounce takes sdkDeps as a plain object parameter and puts it directly in useCallback dependency arrays:

const flushPendingPropertyEdit = useCallback(async () => { ... }, [commitMutationSafely, sdkDeps]);

Since sdkDeps is an inline object literal constructed in useGsapScriptCommits:

const propertyOps = useGsapPropertyDebounce(commitMutationSafely, {
  sdkSession, writeProjectFile, editHistory, ...
});

...it gets a new identity every render, so every callback depending on sdkDeps re-creates every render. This makes the debounce useEffect cleanup fire on every render (calling flushPendingPropertyEdit prematurely) and defeats the coalescing intent of the debounce.

Options:

1. Memoize the deps object in useGsapScriptCommits with useMemo.
2. Destructure the individual values in useGsapPropertyDebounce and list them individually in the deps array (like useGsapAnimationOps already does -- that hook takes individual params and lists them individually, which is correct).

This is the most impactful finding -- the debounce path fires too eagerly, which means rapid property edits may not coalesce, causing extra server round-trips and redundant SDK serializations on the hot path (drag-to-edit property values).

Observation: double-wrapped properties in property debounce calls

In useGsapPropertyDebounce.ts, the flushPendingPropertyEdit, addGsapProperty, and addGsapFromProperty callbacks pass properties to sdkGsapTweenPersist with what looks like an extra nesting layer:

// flushPendingPropertyEdit:
{ kind: "set", animationId, properties: { properties: { [property]: value } } }

// addGsapProperty:
{ kind: "set", animationId, properties: { properties: { [property]: defaultValue } } }

The SdkGsapTweenOp type for kind: "set" is { kind: "set"; animationId: string; properties: Partial<GsapTweenSpec> }. So properties here is a partial GsapTweenSpec -- meaning { properties: { opacity: 1 } } is correct IF GsapTweenSpec itself has a field called properties for the "to" property bag. And then updateGsapFromProperty passes { fromProperties: { [property]: value } } without double nesting, which is internally consistent.

This likely works as intended, but the visual symmetry break between the "to" and "from" paths is confusing. A clarifying comment like // GsapTweenSpec.properties = the "to" property bag at the first usage would save the next reader 5 minutes of head-scratching.

Test gap: no test for the properties-within-properties shape

The kind: "set" test passes { ease: "power3.in" } -- a flat meta property. There is no test for the { properties: { opacity: 1 } } shape that flushPendingPropertyEdit and addGsapProperty actually construct. Since that nesting is the thing most likely to confuse (see above), a test exercising it would add confidence.

Nit: duplicate fallow-ignore-next-line complexity annotations

Several functions in useGsapPropertyDebounce.ts and useGsapAnimationOps.ts have the fallow ignore comment duplicated -- once before the useCallback and once before the inner function:

  // fallow-ignore-next-line complexity
  const addGsapFromProperty = useCallback(
    // fallow-ignore-next-line complexity
    async (selection: ...) => {

One of these is redundant. Harmless but noisy.

Nit: ease: "power2.out" as const in default spec

In addGsapAnimation, the default spec for the SDK path has ease: "power2.out" as const. The as const only matters if GsapTweenSpec.ease is a string union. If it is just string, the assertion is a no-op. The server-side fallback path below does not specify ease at all, so they may diverge on defaults. Minor, worth checking.

---

Summary

The core sdkGsapTweenPersist function is solid -- clean try/catch fallback, proper telemetry, correct serialization lifecycle. The server-authoritative exclusions (set method, removeGsapProperty, deleteAllForSelector) are well-documented and correctly reasoned. The five new tests cover the key paths.

The sdkDeps object-identity issue in useGsapPropertyDebounce is the main thing I would want addressed before merge -- it undermines the debounce coalescing on the property-edit hot path. The rest is cleanup-tier.

LGTM -- pinging @magi for the stamp. <@U0B1J4SL8H3>

-- Miga

[james-russo-rames-d-jusso]

Review — §3.5 PR1 GSAP tween through SDK

Reviewed at 37a7cefd4fb05bae605d62f27f3b9bce1d257edb. Part of the 18-PR SDK-cutover stack review batch (Group B). Layering on Miga's review — their sdkDeps object-identity finding (defeats debounce coalescing on the property-edit hot path) is the headline and is correctly resolved by #1471's memoized sdkDeps. Co-signing that. Two additional things not yet flagged:

Concern — Stale-id phantom undo on set / remove branches (pre-#1471)

sdkGsapTweenPersist does:

if (op.kind === "add") {
  if (!sdkSession.getElement(op.target)) return false;
  sdkSession.addGsapTween(op.target, op.spec);
} else if (op.kind === "set") {
  sdkSession.setGsapTween(op.animationId, op.properties);
} else {
  sdkSession.removeGsapTween(op.animationId);
}

The add branch has the element-existence guard, but set and remove don't validate animationId exists on the session. If the animationId is stale (animation was just deleted, or never matched anything), setGsapTween / removeGsapTween in the SDK presumably either (a) silently no-op or (b) throw. If (a), persistSdkSerialize runs against the unchanged doc — serialize() returns the same content — and we write the same bytes back to disk, but editHistory.recordEdit({ before: same, after: same }) records a phantom undo step. If (b), the catch handles it correctly.

This is exactly what #1471's before === after no-op guard closes. So this is closed in the stack — flagging only because if #1471's landing slips for any reason, this PR ships the phantom-undo behavior into prod (even with the cutover flag off, the SDK path still fires for elements with hfId and an active session, since useSdkSession isn't flag-gated).

Suggested: if there's any chance #1471 slips, add the no-op gate inline here as well. Otherwise: noted in the stack-coherence column.

Concern — addGsapAnimation autoId clobber risk (pre-#1471)

addGsapAnimation first calls assignGsapTargetAutoIdIfNeeded / ensureElementAddressable to assign an hf-id server-side if the element doesn't have one. Then if the SDK session is present, it goes through the SDK path. But the SDK session hasn't reloaded that just-written id yet — the session's in-memory getElement(selection.hfId) may or may not find the freshly-assigned id depending on timing. If it doesn't find it, addGsapTween (which goes through getElement guard) returns false → server fallback, fine. If it does find it (because the assign-id path called writeProjectFile and the SDK session's reload-on-file-change fired in time), the SDK addGsapTween fires and serializes — potentially with a different id assignment than the server just wrote.

#1471 closes this with the explicit !autoId && guard: skip SDK path when an id was just assigned server-side. Same observation as above — closed in stack, flagging in case #1471 lands separately.

Nits

• set op properties field structure: the flushPendingPropertyEdit / addGsapProperty callsites pass { properties: { properties: { [property]: value } } } (the outer "properties" is Partial<GsapTweenSpec>, and GsapTweenSpec has a field named properties for the "to" bag). Miga flagged this as a visual confusion; the updateGsapFromProperty shape { fromProperties: { ... } } doesn't double-wrap because GsapTweenSpec.fromProperties is a top-level field. Co-signing Miga's suggestion of a clarifying comment.
• No test exercises the { properties: { [prop]: value } } nested shape — the kind: "set" test passes a flat { ease: "power3.in" }. Worth adding one.
• ease: "power2.out" as const in the default spec — as const no-op unless GsapTweenSpec.ease is a string union. Worth verifying.
• Duplicate fallow-ignore-next-line complexity annotations on several useCallbacks (one above the const, one above the inner async function). Resolved in #1471. Co-signing Miga.

Verdict

Good cutover. The five major stack-coherence findings (debounce coalescing, no-op gate, autoId clobber, sdkDeps memo, double fallow-ignore) are all closed in #1471 and the Graphite stack ordering guarantees #1471 lands on top. LGTM as part of the stack.

— Rames D Jusso

[vanceingalls]

Review — §3.5 PR1 GSAP tween add/update/delete through SDK

Reviewed at 37a7cefd4fb05bae605d62f27f3b9bce1d257edb. Part of the 18-PR SDK-cutover stack review (Batch B). Miga and Rames have already worked the surface area thoroughly; this is a layered pass with one observation that isn't in either of theirs.

Co-signing prior reviewers

Both findings are correctly characterised:

• Miga's headline sdkDeps object-identity bug at useGsapPropertyDebounce.ts: the inline-literal deps object re-fires the debounce flush on every render and defeats coalescing on the property-edit hot path. The fix shape (memo the deps object) is the right call. #1471 lands useMemo<CutoverDeps | null> in useGsapScriptCommits and routes it through every child hook as a single stable reference — verified at f2458af.
• Rames's stale-id phantom-undo on set/remove branches: sdkGsapTweenPersist at this PR has no element-existence guard for the set/remove paths (only add checks). A stale animationId whose setGsapTween/removeGsapTween silently no-ops would serialize → write same bytes → record { before: same, after: same } in edit history → phantom undo step. #1471 closes this with the before === after no-op guard wrapping every persist fn — verified at f2458af.
• Rames's autoId clobber risk in addGsapAnimation: the just-assigned server-side id may or may not be in the SDK session's getElement view depending on the file-watch reload timing, and either outcome leaks. #1471 closes this with the explicit !autoId && guard before the SDK branch — verified at f2458af (useGsapAnimationOps.ts).

Per the band-aid bar (long-term solution, not a wedge), the right verdict on a stack where these get closed in PR N+1 of the same Graphite chain is clear with stack-coupling, not request-changes — because Graphite stack ordering guarantees #1471 lands co-resident with this PR. Co-signing both reviewers there.

One additional observation — sdkGsapTweenPersist non-throw error class

sdkGsapTweenPersist at sdkCutover.ts catches via a single try/catch around the entire dispatch + persist block. The set and remove SDK methods could fail in two distinct ways:

1. Throw on a malformed input — caught and falls back. Fine.
2. Silently no-op on a stale animationId (the case Rames flagged) — no throw, no fallback, phantom undo step.

Class (2) is exactly what #1471 closes with the before === after gate. But at this PR's HEAD, the failure mode is silent — there is no observability when the SDK path produces a no-change serialization. A trackStudioEvent("sdk_cutover_noop", { animationId }) on the path that would become the no-op return in #1471 would help measure how often the gate actually fires in prod, and confirm the gate isn't masking a real correctness gap.

This is a soft ask — primarily for #1471 to consider (telemetry on the no-op return, not just the success and fallback events). At this PR's HEAD, the no-op behaviour leaks into edit-history; #1471 fixes it.

Per-PR-body completeness

The PR body covers the intent accurately: add/update/delete routed via sdkGsapTweenPersist, set method (read-only) stays server-side with rationale, removeGsapProperty / removeGsapFromProperty / deleteAllForSelector stay server-side with // ponytail: markers in code. The five sdkGsapTweenPersist tests cover the happy path + the null-session + missing-element + error-fallback edges; the properties: { properties: { ... } } nested shape that Miga flagged is genuinely confusing on the page but works because GsapTweenSpec.properties is the "to" bag (verified against the SDK types at packages/sdk/src/types.ts).

Stack hygiene

Base is 06-15-chore_studio_document_css-path_position_cut-over_gsap-path_intentionally_deferred_3.3_ (#1467). No stale-base squash hazard at the per-PR surface — all the cross-cutting cleanup is in #1471 above, which is part of the same Graphite chain.

Verdict

Clear with minor nits. All five stack-coherence findings (debounce coalescing, no-op gate, autoId clobber, sdkDeps memo, double fallow-ignore) are closed in #1471. The one finding I'd want addressed as a forward-pass on #1471 is observability on the no-op fallback path so we can see how often the new gate fires. Stamp-eligible as part of the stack.

Review by Via

[vanceingalls]

Review findings addressed in #1539:

• Single-property edit wiped sibling animated props (REPLACE-not-merge; send-site sent one key) → send-site now merges with the tween's current props before dispatch (commit bcc0a44a6, test: editing x preserves y/opacity).
• GSAP/timing/keyframe/delete cutover was NOT gated by STUDIO_SDK_CUTOVER_ENABLED (only style/text/attr was) → gated all chokepoints; flag-off is now a true no-op (commit 18ea1c323 + gate test 508be317d).
• Debounce defeated by unstable deps (flush per render) + serializer bypass on the SDK write branch → stabilized deps via ref + routed SDK writes through the keyed serializer (commit bcc0a44a6).

[miguel-heygen]

Approved as part of SDK cutover stack. Reviewed by Miga, Rames D Jusso, and Via across R1-R4. LGTM.

[jrusso1020]

Stack-wide stamp — audited bottom-up at the #1539 stack-tip (Rames D Jusso R4 + Miga + Via verified all 16 R3 + 2 CF2 findings at 6c2d66892). SDK-cutover chain cleared end-to-end.

The base branch was changed.