feat(sdk): ws-3c — addWithKeyframes + replaceWithKeyframes SDK ops (acorn writer)

[vanceingalls]

WS-3.C — addWithKeyframes + replaceWithKeyframes SDK ops (acorn writer)

Part of the AI Studio (Pacific) SDK integration. Stacked on #1571 (WS-D). This is the last hotspot op before recast retirement, and gates #1573 (WS-3.F) — the GSAP differential suite must be green here before recast can be removed.

What this does

Adds two GSAP keyframe ops on the acorn writer path:

• addWithKeyframes — add an animation with keyframes.
• replaceWithKeyframes — replace an existing keyframed animation.

Both are wired through the SDK (mutate.ts handlers + validateOp) and the Studio dispatch layer (SDK-first with server fallback). The acorn writer now emits _auto: 1 to match the recast writer's output exactly (verified by parity tests).

Implementation notes

• buildKeyframeObjectCode / addAnimationWithKeyframesToScript gain an auto?: boolean flag.
• Studio cutover helpers (sdkAddWithKeyframesPersist, sdkReplaceWithKeyframesPersist) share a dispatchWithKeyframes helper to avoid fallow duplication; useGsapAnimationOps wires the callbacks.
• Position-derived ID landmine (documented in the commit): replaceWithKeyframes removes the old tween (renumbering survivors) then inserts the replacement. The MutationResult patch pair operates on the full GSAP script text, not per-ID patches, so undo/redo is safe — but callers holding stale animationIds must re-parse after a structural edit.

Files (6 changed)

• core: parsers/gsapWriterAcorn.ts, parsers/gsapWriter.parity.test.ts
• sdk: types.ts, engine/mutate.ts
• studio: utils/sdkCutover.ts, hooks/useGsapAnimationOps.ts

Gates

• bun run build ✅ · bun test (core + sdk + parity) 163/0 ✅
• GSAP differential suite (gsapWriterParity.acorn.test.ts) 14/0 GREEN ✅ — this is the gate for refactor(core): gate acorn GSAP writer behind cutover flag; keep recast default (WS-3F) #1573
• bunx oxlint 0/0 ✅ · bunx oxfmt --check ✅ · fallow --gate new-only ✅

Deferred

Remaining Studio callers (gsapDragCommit.ts, gsapRuntimeBridge.ts, useGestureCommit.ts, useEnableKeyframes.ts) still pass keyframe ops as untyped records on the server path — wired to the SDK in #1573 or a dedicated cutover pass.

🤖 Generated with Claude Code

[vanceingalls]

• refactor(core): gate acorn GSAP writer behind cutover flag; keep recast default (WS-3F) #1573
• feat(sdk): ws-3c — addWithKeyframes + replaceWithKeyframes SDK ops (acorn writer) #1572 👈 (View in Graphite)
• feat(sdk): addElement forward op — mint hf-id, inverse = removeElement (WS-D) #1571
• feat(sdk): ws-c elastic timing + word-alignment resolver (WS-C) #1570
• feat(sdk): ws-b variables/brand — object-valued font/image + B1 JSON model #1569
• main

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

[miga-heygen]

PR Review — feat(sdk): ws-3c — addWithKeyframes + replaceWithKeyframes SDK ops (acorn writer)

Solid piece of work. The two new ops land cleanly across all three layers (core writer, SDK engine, Studio dispatch), the parity test suite is thorough, and the dispatchWithKeyframes helper is a smart way to avoid the fallow duplication between add and replace. The position-derived ID landmine is documented clearly in both the PR body and the handler comment, and the coarse gsapScriptChange patch pair sidesteps the renumbering hazard elegantly.

A few observations — nothing blocking, but worth considering:

---

1. handleReplaceWithKeyframes — silent no-op remove is guarded, but fragile

The handler does removeAnimationFromScript(script, op.animationId) without checking if the remove actually changed anything. If the ID doesn't exist, removeAnimationFromScript returns the original script unchanged, and the handler proceeds to insert a NEW tween — turning a "replace" into an "add."

validateOp already guards against this via gsapAnimationMissing(parsed, op.animationId), so in the normal dispatch path this can't fire. But if someone bypasses validateOp (tests, internal tooling, future refactor that drops the check), this becomes a silent data-integrity bug.

An optional belt-and-suspenders guard:

const afterRemove = removeAnimationFromScript(script, op.animationId);
if (afterRemove === script) return EMPTY; // animation already gone

Not blocking — the validateOp gate is there and correct. But the pattern would match cascadeRemoveAnimations's own if (next === current) return current guard style.

2. _auto: 1 — numeric literal, not boolean

The acorn writer emits _auto: 1 (numeric) while the type definition allows auto?: boolean. This is intentional for recast parity (confirmed by the parity tests passing), but it's worth a brief inline comment in buildKeyframeObjectCode explaining why it's 1 not true — future contributors seeing if (kf.auto) props.push('_auto: 1') might "fix" it to _auto: true and break the GSAP runtime contract. A one-liner like // GSAP runtime expects _auto: 1 (numeric), not true prevents that.

3. Type duplication in KeyframeEntry / KeyframeSpec

There are now three near-identical keyframe type shapes:

• EditOp["addWithKeyframes"]["keyframes"][number] in types.ts
• KeyframeEntry in useGsapAnimationOps.ts
• KeyframeSpec in sdkCutover.ts

All have { percentage, properties, ease?, auto? }. The Studio-side types aren't exported from types.ts because they serve different layers (one is the SDK op shape, the others are UI/cutover plumbing), but they'll drift independently. Consider extracting a shared KeyframeSpec from types.ts and importing it in both Studio files — or at minimum, adding a // Mirrors EditOp.addWithKeyframes.keyframes[number] comment to each copy so future editors know where the source of truth lives.

4. Parity tests — replaceWithKf{Recast,Acorn} helpers could check the intermediate state

The replaceWithKfRecast / replaceWithKfAcorn test helpers do remove → add but don't assert that the remove actually removed something. If removeAnimRecast or removeAnimAcorn silently no-ops (bad ID), the test would still pass because it'd be testing "add" not "replace." Adding expect(removed).not.toBe(script) after the remove call would catch that regression path.

5. Minor: sdkAddWithKeyframesPersist doesn't pass resolverTarget

sdkReplaceWithKeyframesPersist passes { animationId, opLabel: "replaceWithKeyframes" } as the resolverTarget to dispatchGsapOpAndPersist for resolver-parity telemetry. sdkAddWithKeyframesPersist doesn't — which makes sense since there's no existing animation to resolve. Just confirming this is intentional and not an oversight (it is — adds have no prior animation ID to check).

---

Verdict

Clean, well-documented, follows existing patterns faithfully. The parity tests are comprehensive and the position-derived-ID hazard is handled correctly at every layer. The five observations above are all non-blocking — the first two are the most actionable (defensive guard + inline comment).

LGTM — ready for stamp.

— Miga

[james-russo-rames-d-jusso]

Review — feat(sdk): ws-3c — addWithKeyframes + replaceWithKeyframes SDK ops (acorn writer)

Reviewing at HEAD 4ef28dbc10e0fe60c214427c11ad60ebe59e376d. Stacked on #1571 (WS-D). Net +423/-0. Gates #1573 (the recast retirement).

_auto: 1 parity with the recast writer is the right fix and the parity tests at gsapWriter.parity.test.ts:923-942 cover it. replaceWithKeyframes as remove+add through the existing gsapScriptChange patch pair is structurally sound — undo/redo carry the full script before/after, so the position-derived-ID renumber problem documented in the PR body doesn't bite the patch layer (it just bites callers, who must re-parse — that's the right tradeoff). A few asymmetries vs sibling ops to call out.

Concerns

1. No typed methods on Composition for addWithKeyframes / replaceWithKeyframes — asymmetric with addGsapTween. (packages/sdk/src/session.ts, packages/sdk/src/types.ts)

addGsapTween and setGsapTween both have typed methods on Composition (sibling of setVariableValue, etc.) AND op-union entries; consumers can call comp.addGsapTween(target, tween) or comp.dispatch({type: "addGsapTween", ...}). PR #1572 only adds the op-union entries — Studio dispatches raw via s.dispatch({type: "addWithKeyframes", ...}) (sdkCutover.ts:460-462).

Internally consistent for this PR, but it leaves the SDK surface asymmetric: callers who learned comp.addGsapTween() will have to drop down to raw dispatch for addWithKeyframes. WS-3.F may add these; if so, please mark explicitly in the deferred section. If not, add the typed methods here — addWithKeyframes(targetSelector, position, duration, keyframes, ease?): string mirroring addGsapTween's shape.

2. replaceWithKeyframes doesn't surface "previous animationId no longer valid" via meta. (packages/sdk/src/engine/mutate.ts:984-1004)

The handler returns meta: { animationId } with the NEW id (line 1003). Callers holding the OLD op.animationId will mistakenly believe their stale handle is still valid until the next read. The PR body's "Position-derived ID landmine" note documents this for humans, but the meta channel could give callers a programmatic signal. Two cheap options:

• Return meta: { animationId, replacedAnimationId: op.animationId } so the caller can map old→new in their local state without a re-parse.
• Or document meta.animationId after replaceWithKeyframes as "id of the replacement (may equal op.animationId by coincidence, treat as new)" — at minimum a one-line code comment near the return.

Not blocking — but a stale-handle bug here will look like "my animation disappeared" in Studio, and the breadcrumb is at the call site.

3. Iterative-merge gate check. Per my standing rubric for "behind a gate, merge-and-iterate" workstream PRs: this one merges to ws-d-add-element (stacked) and the new ops are part of the SDK public type union the moment this lands. The new EditOp shapes are wire-schema-shaped — once published they are hard to walk back. Verify the addWithKeyframes / replaceWithKeyframes shapes are stable before publish. Open questions:

• Should auto?: boolean be on the keyframe-spec level or on the op level (e.g. autoEndpoints: 'both' | 'first' | 'last' | 'none')? The current per-keyframe-entry placement matches the recast emitter's shape, which is the safer choice — keeping it for symmetry.
• position: number (seconds) vs position: number | string for label-relative offsets ("intro+=0.5") — addGsapTween accepts number | string (GsapTweenSpec.position). New ops here are number-only. Deliberate, or should they accept label-relative too? If number-only, that's a constraint worth a one-liner in the type doc.

Nits

• The cross-handler comment at mutate.ts:990-993 ("Position-derived IDs renumber, so the inverse patch restores the full GSAP script rather than trying to re-insert by ID") is load-bearing and worth a 1-line repeat above handleAddWithKeyframes too — both ops share the script-level patch shape, but a reader landing on handleAddWithKeyframes first will miss the rationale.
• useGsapAnimationOps.ts:222-230 — the legacy commitMutation fallback for addWithKeyframes and replaceWithKeyframes (when sdkSession/sdkDeps are absent) still passes ops as untyped server records (type: "add-with-keyframes", hyphenated). The PR body's "Deferred" section names the un-cutover Studio callers but not the fallback contract itself. If the server schema for add-with-keyframes already exists, fine; if not, callers landing on this path silently fail.

Verified

• _auto: 1 parity: recast writer emits _auto: 1 when kf.auto is true (gsapParser.ts:1234); acorn writer pre-#1572 didn't (verified by reading gsapWriterAcorn.ts:1128-1142 on origin/main). PR adds it (gsapWriterAcorn.ts:1132). Parity tests at gsapWriter.parity.test.ts:923-942 cover both-endpoint and single-endpoint auto cases.
• replaceWithKeyframes correctness: removeAnimationFromScript then addAnimationWithKeyframesToScript, captured at the script-string level (mutate.ts:995-1002) — the patch pair is gsapScriptChange(oldScript, newScript) which carries before/after whole, so undo/redo are safe even though intermediate IDs renumber.
• gsapAnimationMissing guard in validateOp for replaceWithKeyframes (mutate.ts:1570) — uses the existing helper, consistent with setGsapTween / removeGsapTween validation shape.
• Empty keyframe-list rejection: both ops return E_INVALID_ARGS at validate (mutate.ts:1561-1567, :1572-1578). Good.
• dispatchWithKeyframes helper in sdkCutover.ts:455-465 correctly switches on animationId !== undefined for add vs replace — clean.
• GSAP differential suite green per PR body — gsapWriterParity.acorn.test.ts 14/0 is the canonical gate before #1573. Important — this PR is load-bearing for the recast retirement; the parity wins are real.

What I didn't verify

• That the server-side add-with-keyframes / replace-with-keyframes record handlers exist for the commitMutation fallback path in useGsapAnimationOps.ts:222-230 / :266-274. The PR is SDK+Studio; server is presumably an orthogonal landing.
• Whether the deferred Studio callers (gsapDragCommit.ts, gsapRuntimeBridge.ts, etc.) need any contract-locking from these new op shapes — assumed they'll cutover in #1573 / dedicated pass per the PR body.

Math + parity is right. (1) and (2) are the two surfaces I'd want tightened — both are cheap.

Review by Rames D Jusso

[miguel-heygen]

LGTM

The base branch was changed.