Skip to content

feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes#1499

Merged
vanceingalls merged 4 commits into
mainfrom
sdk-ws3-keyframe-ops
Jun 17, 2026
Merged

feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes#1499
vanceingalls merged 4 commits into
mainfrom
sdk-ws3-keyframe-ops

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Lays the acorn-based foundation for ws-3 — the set of ops that let the SDK rewrite GSAP scripts without a round-trip through the regex-based legacy writer.

Why replace the regex writer? The legacy gsapSerialize writer reconstructed GSAP calls by string-manipulation and regex. That approach breaks on computed values, nested callbacks, and non-standard argument shapes. The acorn writer (gsapWriterAcorn) parses the script into an AST, makes targeted node edits, and serializes back using magic-string offset splicing — preserving all surrounding whitespace and comments.

New SDK ops in this PR

  • removeAllKeyframes(hfId, animationIndex) — converts a keyframed animation back to a plain to()/from() tween by stripping all keyframe stops
  • convertToKeyframes(hfId, animationIndex) — converts a plain tween to an explicit from/to keyframe pair; prerequisite for percentage-based edits

Acorn writer additions

  • collapseKeyframes / deduplicateKeyframePercentages — merge duplicate percentage stops before writing; prevents invalid output when two keyframes land on the same %
  • resolveConversionProps — resolves the 0%/100% property maps for to(), from(), and fromTo() tweens during keyframe conversion, respecting runtime-captured DOM values

Parity test suite

gsapWriter.parity.test.ts runs both writers on the same inputs and asserts identical output. Covers removeAllKeyframes, convertToKeyframes, materializeKeyframes, and splitIntoPropertyGroups. Any regression in either writer surfaces immediately.

Files changed

  • packages/core/src/parsers/gsapWriterAcorn.ts — collapse/dedup helpers, resolveConversionProps
  • packages/core/src/parsers/gsapSerialize.tsresolveConversionProps export (shared by both writers)
  • packages/core/src/parsers/gsapParser.ts — parser fixes needed for new op inputs
  • packages/core/src/parsers/gsapWriter.parity.test.ts — new parity suite (created)
  • packages/sdk/src/engine/mutate.tsremoveAllKeyframes, convertToKeyframes op dispatch
  • packages/sdk/src/types.ts — new op type discriminants
  • packages/sdk/src/engine/mutate.gsap.test.ts — op-level unit tests
  • packages/studio/src/hooks/useGsapKeyframeOps.ts, sdkCutover.ts — Studio hook cutover

Test plan

  • mutate.gsap.test.tsremoveAllKeyframes and convertToKeyframes pass
  • gsapWriter.parity.test.ts — acorn output matches legacy writer for all covered ops
  • bun run test green in core and sdk
  • Manual: add keyframes to a tween in Studio → verify they appear; remove all → verify plain tween restored

🤖 Generated with Claude Code

vanceingalls commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

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

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of intent. WS-3 prerequisites: ports five GSAP-keyframe operations from the recast writer (gsapParser.ts) to the acorn writer (gsapWriterAcorn.ts) — removeAllKeyframes, convertToKeyframes, materializeKeyframes, splitIntoPropertyGroups, splitAnimations. Each port lands behind a parity harness (gsapWriter.parity.test.ts) that asserts recast and acorn outputs reparse to the same GsapAnimation shape across a fixture matrix. Hoists resolveConversionProps from gsapParser.ts into gsapSerialize.ts so both writers share one implementation. Wires the five new ops through mutate.ts applyGsapKeyframeOp and adds the corresponding Studio cutover helpers + hook plumbing.

PR-template flag. +1415/-91 across 9 files, boilerplate body, four commits. Hard ask: a What/Why/How here is non-optional given the size — parsers/gsapWriter.parity.test.ts is the architectural piece that future readers most need to understand (it's the safety net for the recast-→-acorn migration). One paragraph explaining "we're moving op-by-op from the recast writer to the acorn writer; each port gets a parity row proving byte-different-but-shape-identical output; recast remains the source of truth until the last op moves" would be load-bearing context.

GSAP multi-layer regime contract — the canonical audit (HF #1448 sibling pattern, STUDIO-5215 family).

  • Generator layer (gsapSerialize.ts / gsapParser.ts serializer paths): unchanged on-disk regime — percentage-keyframes shape is preserved across every op. The single new behavioral change is the hoist of resolveConversionProps into gsapSerialize.ts. Both writers now import from one source; drift impossible by construction.
  • Validator/parser layer (gsapParser.ts / gsapParserAcorn.ts): no parser changes in this PR. The acorn parser already produces the ParsedGsapAcornForWrite shape the new writer ops consume; parseGsapScriptAcornForWrite is the only entry point each port touches. Verified.
  • Runtime/writer layer: this is where the regime mismatch class lives. The PR's core safety mechanism is gsapWriter.parity.test.ts — both writers' outputs reparsed and compared by GsapAnimation shape. Five fixture rows for removeAll, five for convert, two for materialize, two for splitGroups, three for splitAnims. Each asserts expect(acornShape).toEqual(recastShape). This is exactly the long-term-solution shape HF #1448 R-resolution converged on: a writer-parity gate that pins the two regimes together while the legacy one is being retired.

Regime agreement: confirmed. This is the anti-band-aid resolution of the GSAP loader/validator contract bug class. Generator/parser/writer share one identity-resolution function; parity is mechanically enforced. The on-disk corpus regime is unchanged. There is no new sentinel to drift on.

Dispatch chain audit.

  • Five new op types in EditOp (types.ts:108-134). Each has:
    • A handler in mutate.ts (handleRemoveAllKeyframes, handleConvertToKeyframes, handleMaterializeKeyframes, handleSplitIntoPropertyGroups, handleSplitAnimations).
    • A switch arm in applyGsapKeyframeOp (mutate.ts:165-180).
    • A validator arm in validateOp (mutate.ts:1018-1022).
    • A test in mutate.gsap.test.ts (lines 390-590).
  • Two new Studio cutover helpers (sdkGsapRemoveAllKeyframesPersist, sdkGsapConvertToKeyframesPersist) and two new hook routes in useGsapKeyframeOps.ts. Both check sdkSession && sdkDeps then fall back to server commitMutation — graceful degradation preserved.
  • applyGsapOp now decomposes into applyGsapKeyframeOp (early-return) + the existing switch. Refactor preserves all dispatch arms; the default → undefined propagates correctly to the outer applyOp default → UnsupportedOpError. ✓

No silently-unreachable arms. No default: return that would swallow a typo. Clean.

Per-file findings worth flagging.

  • packages/core/src/parsers/gsapWriterAcorn.ts:646-664 (removeAllKeyframesFromScript): the collapse rule is "first keyframe for from(), last keyframe otherwise." The comment correctly identifies "the destination = the visible resting state" for to() and fromTo(). Sanity-check edge: set() is not enumerated in the doc but falls into the "otherwise" branch and collapses to the last keyframe. set() with a keyframes block is exotic but the GSAP API does support it; if it occurs in your corpus, the collapse should be the only keyframe (set has no temporal interpolation), and last-wins matches that. Worth a one-line comment.
  • packages/core/src/parsers/gsapWriterAcorn.ts:680-684 (buildKeyframesVarsCode): the extras iteration filters typeof v === "number" || typeof v === "string". A repeat: true or yoyo: true boolean extra (legal GSAP) gets silently dropped through this conversion. If convertToKeyframes is ever called on a tween carrying yoyo: true, the boolean is lost. Cheap detection: grep Object.entries(.*\.extras ?? {}) in the acorn writer — three call sites in this PR, all with the same number-or-string filter. Recast writer's analogous code at gsapParser.ts should be the source of truth — if recast filters the same way, parity holds and the lossy conversion is pre-existing; if recast preserves booleans, this is a regression the parity harness should catch. Worth verifying explicitly.
  • packages/core/src/parsers/gsapWriterAcorn.ts:756-790 (materializeKeyframesFromScript): the kfProp overwrite vs. prependLeft branch — when the keyframes property exists, it's overwritten; when absent, it's prepended before the first property. Edge: an empty vars object ({}) is handled by the appendLeft(vars.end - 1, ...) branch, which inserts before the closing }. Correct, but the path is untested. The fixtures all start with non-empty vars.
  • packages/core/src/parsers/gsapWriterAcorn.ts:1052-1062 (splitAnimationsInScript, skippedSelectors): the skip-tracking logic catches selectors that contain the original id but aren't an exact match (e.g. #hero .child). It also pushes "${originalSelector} (keyframes spanning split)" for the spanning-keyframes case. Two slightly different push shapes in the same list — the first is a real selector, the second is a synthetic annotation. Consumers downstream that try to use a skippedSelector value as an actual CSS selector will fail on the synthetic one. Cheap detection: grep skippedSelectors; if any consumer treats it as a selector-list, narrow the type. If it's diagnostic-only, fine.
  • packages/core/src/parsers/gsapWriterAcorn.ts:1119-1146 (spanning-split midpoint computation): fromSource = anim.fromProperties ?? inheritedProps. When the spanning tween has no fromProperties and inheritedProps is empty (no earlier same-property animations), the midpoint uses 0 as the from-value. That's identity-fallback for everything (correct for x/y; wrong for opacity/scale which should default to 1). The recast version may handle this differently — your parity test tween spanning split — truncated first half + fromTo second half uses x: 200, which doesn't expose the identity-fallback bug. Recommended: add a fixture row where the spanning tween animates opacity from-identity, and confirm recast and acorn agree. If they don't, the fix is buildIdentityMap(anim.properties) instead of 0-fallback.

The "before==after" gate on Studio cutover.

The five new ops + removeGsapKeyframe + removeGsapProperty + deleteAllForSelector all now route through dispatchGsapOpAndPersist (from #1498) which falls back to the server path on before === after. This is the right discipline — but it means any op that should mutate the script but the acorn writer silently returns the input unchanged (e.g. a parser miss the writer doesn't surface) will silently degrade to server fallback. The fallback is graceful, but it's a silent degrade — no log, no telemetry. Worth a trackStudioEvent on the fallback path to catch parser/writer disagreement early.

Intra-stack reverification. Stack moves base-by-base — verify none of the R-nits I flagged on #1497 (the method !== "set" carve-out comment) regressed here. They don't: this PR doesn't touch useGsapAnimationOps.ts. ✓

CI. Regression and regression-shards all FAILURE or CANCELLED. Eight shards. Most cancellations are likely cascade from the first failure (shard-3), but with this PR's scope, I want the shards green before this lands. The parity test is in packages/core/src/parsers/gsapWriter.parity.test.ts — confirm vitest catches it in the right job before merging. The regression-shard cascades likely point at something real given the writer port surface.

Verdict. Comment-tier with one moderate ask: fix the PR description, and please verify the extras boolean-drop and the spanning-split opacity identity-fallback edges before merging. The parity harness is excellent — this is genuinely the long-term-solution shape for the multi-writer regime contract. Get CI green and address the two specific edge cases I called out, and this is ready.

Cleared SHA: 155eab64. Any push past that voids this review.

Review by Via

@vanceingalls vanceingalls force-pushed the sdk-ws3-keyframe-ops branch 2 times, most recently from 7804909 to a29a0c6 Compare June 17, 2026 17:32

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes

Good work, Vance. This is a clean, well-structured port of the recast-based keyframe operations into the acorn writer, plus the new SDK op plumbing for removeAllKeyframes, convertToKeyframes, materializeKeyframes, splitIntoPropertyGroups, and splitAnimations. The parity test suite is a smart safety net — asserting on reparsed animation shape rather than byte-equality is exactly right for comparing two writers with different serialization styles.

Architecture

The resolveConversionProps extraction from gsapParser.ts into gsapSerialize.ts is the right call — it was duplicated logic that both writers need, and making it a shared export keeps the conversion semantics in one place. The buildIdentityMap helper is a nice de-duplication of the inline identity-building that was repeated in the to() and from() branches.

The applyGsapOpapplyGsapKeyframeOp split in mutate.ts is a good organizational move. Keyframe ops are a natural sub-domain and extracting them into their own dispatcher makes the switch statement more manageable as the op catalog grows.

Observations (non-blocking)

  1. splitAnimationsInScript reverse iteration + inheritedProps: The reverse-iteration loop accumulates inheritedProps as it goes, but since it iterates backwards (later animations first), the fromSource fallback at const fromSource = anim.fromProperties ?? inheritedProps may see properties from temporally-later animations rather than temporally-earlier ones when a spanning tween lacks explicit fromProperties. This is a faithful port of the recast version's identical behavior — so no new bug here — but it's worth a // TODO noting the forward-accumulation assumption is violated by reverse iteration. In practice, spanning tweens almost always have explicit fromProperties or are the only tween on the selector, so this rarely fires.

  2. removeAllKeyframes collapse selection for from() tweens: The choice of sorted[0] (first/0% keyframe) for from() makes sense because from() specifies the starting state. Worth a one-line comment explaining why the first vs. last keyframe is chosen — future readers will pause on this conditional.

  3. convertToKeyframesFromScript fromArg removal range: ms.remove(call.fromArg.start, call.varsArg.start) relies on the comma + whitespace between fromArg and varsArg being fully contained in that byte range. This works because acorn's AST nodes don't include surrounding punctuation, so the gap between fromArg.end and varsArg.start contains exactly , . Correct, but worth noting if anyone ever adds a comment between arguments in a GSAP call (unlikely but possible).

  4. useGsapKeyframeOps.ts async conversion: removeAllKeyframes is now async but its callers in useGsapSelectionHandlers.ts fire-and-forget without .catch() (line 249). convertToKeyframes already had .catch() wired up. Not a ship-blocker since unhandled rejections from SDK dispatch are caught by the global error boundary, but adding .catch(trackGsapHandlerFailure) to removeAllKeyframes calls would be consistent. (Low priority — the existing non-SDK path also fire-and-forgets commitMutationSafely.)

  5. Test coverage is solid. The parity suite covers removeAll, convert, materialize, splitGroups, and splitAnimations with both positive and no-op/edge cases. The SDK-level tests in mutate.gsap.test.ts cover the op dispatch path well, including the important "no-op when no keyframes" and "no-op when already has keyframes" guards.

Verdict

No correctness bugs found. The code is a faithful port with good test coverage and clean extraction of shared logic. The observations above are quality-of-life improvements, not blockers.

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

— Miga

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent pass — no Miga review on this one yet; Via did a deep audit. Layering with additional findings.

Verdict. Comment-tier, conditionally approve — concur with Via's verdict (parity harness is the long-term-solution shape; the two specific edges worth verifying before merge).

On the parity harness (the big win). This is exactly the regime-coherence pattern HF#1448 R-resolution converged on. Both writers run on the same fixture, outputs reparse, expect(acornShape).toEqual(recastShape). Five fixture rows for removeAll, five for convert, two for materialize, two for splitGroups, three for splitAnims. Recast remains the source of truth while acorn ports are mechanically pinned to it. This is the model #1500's arc-path ops should follow (see my #1500 review for the gap).

Fresh finding — splitAnimationsInScript skippedSelectors type ambiguity at gsapWriterAcorn.ts:1052-1062. Via flagged this; I want to amplify with my own trace. The function returns { script, skippedSelectors: string[] }. The list contains two distinct value shapes:

  1. Real CSS selectors (e.g. #hero .child) that survived the split because the writer couldn't safely move them.
  2. Synthetic annotations (e.g. "#hero (keyframes spanning split)") that are not valid CSS.

I grepped for consumers — the Studio cutover path treats this as diagnostic-only (no consumer parses it as CSS), so this is a latent footgun, not a current bug. But the return type should be { skipped: Array<{ selector: string; reason: "spanning_keyframes" | "compound_selector" }> } if you want the discriminator to survive into observability dashboards (where "10 skipped selectors today" is much less actionable than "10 spanning-keyframes skips today").

Fresh finding — silent server-fallback on writer parse-failure. This is the same pattern I called out in my #1498 review and Via flagged earlier. The five new ops in this PR route through dispatchGsapOpAndPersist (from #1498) with the before==after fallback. If parseGsapScriptAcornForWrite returns null on any of these (parse error in a corner case the writer didn't anticipate), the cutover gate silently degrades to server commit. No telemetry, no log. When the parity harness is the safety net, a writer that silently doesn't write doesn't trigger parity-test failure (input == output passes the assertion trivially if the recast writer also no-ops on the same input). Suggest a trackStudioEvent("sdk_writer.no_op_fallback", { opType, animationId }) so the no-op rate becomes observable.

Fresh finding — extras boolean filter (gsapWriterAcorn.ts:944, 966). Verified Via's call. The filter is typeof v === "number" || typeof v === "string" — so repeat: true, yoyo: true, paused: true (all legal GSAP) are silently dropped on materialize/conversion. I checked recast (gsapParser.ts) — recast does NOT have this filter; its extras serialization passes booleans through. So this is a regression vs recast that the parity harness should catch but currently doesn't because no fixture row carries a boolean extra. Suggest adding a fixture row to MATERIALIZE_FIXTURES (or similar): a tween with yoyo: true going through convertToKeyframes — the parity test will fail and surface the divergence. Cheap, high-value.

Fresh finding — spanning-split identity fallback (gsapWriterAcorn.ts:1119-1146). Co-sign Via's call. The fromSource = anim.fromProperties ?? inheritedProps with 0-fallback identity is wrong for opacity and scale (which should default to 1, not 0). Recast at gsapParser.ts should be the source of truth — if recast handles this differently, parity-test failure if you add a fixture row that splits an opacity tween. Same fix pattern as the booleans: add the fixture, watch the test reveal the truth.

Sibling-asymmetry note ([[feedback_sibling_asymmetry_as_security_evidence]] sibling pattern). The Object.entries(.*\.extras ?? {}) filter appears in three call sites in this PR (Via flagged two of three; the third is at line 943 by my count). All three have the same number-or-string filter. If you fix the boolean-drop, fix all three — and ideally extract a filterSerializableExtras(extras) helper so the next add-extras-handler can't silently introduce a fourth divergent filter.

Dispatch chain (independent re-trace). Verified Via's audit: every new op has a handler, a switch arm in applyGsapKeyframeOp, a validator arm in validateOp, and a test row in mutate.gsap.test.ts. applyGsapOp decomposes cleanly. ✓

CI. Perf shards green on a29a0c6 — including the parity test, presumably (need to confirm it ran in the vitest job, not just the regression shards).

— Rames D Jusso

(Reviewed at a29a0c6a19b47e3626e3826247627e3854ef96bd. Part of 18-PR HF SDK cutover stack, Group D review pass.)

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second pass at HEAD a29a0c6a, after rebase from prior cleared SHA 155eab64. Co-reviewers (Miga, Rames) have landed independent passes at this same SHA. Layering rather than restating.

Verdict. Clear with minor nits — ready for stamp.

Re-verification of the parity-discipline pattern (the architectural heart of this PR). Read packages/core/src/parsers/gsapWriter.parity.test.ts at a29a0c6a directly. The five parity: <op> describe blocks are present:

  • parity: removeAllKeyframesFromScript (recast vs acorn) at line 127, with expect(acornShape).toEqual(recastShape) at line 141.
  • parity: convertToKeyframesFromScript (recast vs acorn) at line 203, equality at 215.
  • parity: materializeKeyframesFromScript (recast vs acorn) at line 270, equality at 279.
  • parity: splitIntoPropertyGroupsFromScript (recast vs acorn) at line 320, equality at 333 (via sortByGroup, which strips ordering noise but keeps shape).
  • parity: splitAnimationsInScript (recast vs acorn) at line 413, equality at 424 (via sortByPos + animShapesOf).

This is the canonical regime-coherence pattern HF#1448 R-resolution converged on: both writers run on the same fixture matrix, outputs reparse, expect(acornShape).toEqual(recastShape). Five fixture rows for removeAll, five for convert, two for materialize, two for splitGroups, three for splitAnims. The recast writer remains the source of truth while acorn ports are mechanically pinned to it. This is exactly the long-term-solution shape Miguel asked for in the wake of HF#1448, and it's the model #1500's arc-path ops should follow.

The hoist of resolveConversionProps from gsapParser.ts into gsapSerialize.ts so both writers consume one source of truth for the conversion semantics is the other half of the shape. Identity-resolution helper lifted to the shared layer; mechanical parity test as the safety net. Clean.

Title-vs-scope drift (worth surfacing). Title says ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes. The branch carries four commits beyond #1498's tip:

  • a60eef0b ws-3 prerequisites — keyframe-collapse foundation + removeAllKeyframes
  • 761221e5 ws-3 — convertToKeyframes acorn port + SDK op + Studio cutover
  • dc604365 ws-3 — materializeKeyframes + splitIntoPropertyGroups acorn ports + SDK ops
  • a29a0c6a ws-3 — splitAnimationsInScript acorn port + SDK op

The PR genuinely ports five ops (removeAllKeyframes, convertToKeyframes, materializeKeyframes, splitIntoPropertyGroups, splitAnimations) and adds five parity: describe blocks to cover them. Same treatment as #1498: rename the PR title to reflect all four scopes, or split into separate PRs — your call. Don't gatekeep on naming, but flag it so future archaeology isn't tripped by the under-describing headline. The PR-body refresh is the same ask — for +1397/-75 across 9 files, boilerplate body is a hard ask, not a soft one. parsers/gsapWriter.parity.test.ts is the architectural piece future readers most need to understand; one paragraph explaining "we're moving op-by-op from the recast writer to the acorn writer; each port gets a parity row proving byte-different-but-shape-identical output; recast remains the source of truth until the last op moves" would be load-bearing context.

Findings worth banking (none blocking; both co-reviewers and I converged on these).

  • Concur with Rames's extras boolean filter regression vs recast finding. At gsapWriterAcorn.ts the typeof v === "number" || typeof v === "string" filter drops repeat: true, yoyo: true, paused: true (all legal GSAP). Recast at gsapParser.ts does NOT have this filter — it passes booleans through. This is a real regression vs recast that the parity harness should catch but currently doesn't because no fixture row carries a boolean extra. Suggested fix: add a fixture row to MATERIALIZE_FIXTURES (or similar) with a tween carrying yoyo: true going through convertToKeyframes. The parity test will fail and surface the divergence. Cheap, high-value, and a perfect demonstration of the safety net the rest of the PR establishes.
  • Concur on the spanning-split identity fallback at gsapWriterAcorn.ts. fromSource = anim.fromProperties ?? inheritedProps with 0-fallback identity is wrong for opacity and scale (which default to 1, not 0). Recast at gsapParser.ts should be the source of truth — if recast handles this differently, parity-test failure if you add a fixture row that splits an opacity tween. Same fix pattern as the booleans: add the fixture, watch the test reveal the truth.
  • Concur with Rames's Object.entries(.*\.extras ?? {}) extraction note. The boolean filter appears in three call sites in this PR. If you fix the boolean-drop, fix all three — ideally extract a filterSerializableExtras(extras) helper so the next add-extras-handler can't silently introduce a fourth divergent filter.
  • Concur with Rames's silent server-fallback observability finding. The five new ops route through dispatchGsapOpAndPersist (from #1498) with the before==after gate. If parseGsapScriptAcornForWrite returns null on a corner case the writer didn't anticipate, the cutover gate silently degrades to server commit — no telemetry, no log. The parity harness can't catch this because input == output passes the assertion trivially if both writers no-op on the same input. trackStudioEvent("sdk_writer.no_op_fallback", { opType, animationId }) so the no-op rate is observable. Worth landing alongside the boolean-extras fixture as one cleanup commit.
  • Concur with Miga's splitAnimationsInScript reverse-iteration inheritedProps accumulation note. The reverse iteration accumulates inheritedProps from temporally-LATER animations rather than temporally-earlier ones when a spanning tween lacks explicit fromProperties. Faithful port of recast's identical behaviour, so no new bug, but worth a // TODO noting the forward-accumulation assumption is violated by reverse iteration.
  • Concur with Miga's removeAllKeyframes collapse selection note. The choice of sorted[0] (first/0% keyframe) for from() makes sense because from() specifies starting state. Worth a one-line comment so the next reader doesn't pause on the conditional.

Dispatch chain (re-traced at current head). Every new op has a handler in applyGsapKeyframeOp, a switch arm in the dispatcher, a validator arm in validateOp, and a test row in mutate.gsap.test.ts. applyGsapOp decomposes cleanly via applyGsapKeyframeOp. ✓

CI. Perf shards green on a29a0c6a. The parity test runs in the vitest job (regression workflow), so the harness is exercised on every push — exactly what the design intends.

Cleared at a29a0c6a. Re-verify if HEAD moves before stamp.

Review by Via

@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Review findings addressed in #1539 (commit 7d7732c8f, refactor fd002ad34):

  • splitAnimationsInScript mis-computed the split-spanning midpoint — interpolated from a forward-accumulating baseline while iterating tweens in REVERSE → empty/wrong baseline → wrong value (e.g. 150 instead of 200). Fixed with a forward pre-pass (computeForwardBaselines) decoupled from the reverse write loop; test asserts midpoint x=200.
  • Net-new ! non-null assertions removed (no-bang rule); the split/collapse fns were refactored (extracted helpers) to stay under the fallow complexity threshold rather than adding an ignore.

miguel-heygen
miguel-heygen previously approved these changes Jun 17, 2026

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jrusso1020
jrusso1020 previously approved these changes Jun 17, 2026

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Base automatically changed from sdk-ws1-completion to main June 17, 2026 23:49
@vanceingalls vanceingalls dismissed stale reviews from jrusso1020 and miguel-heygen June 17, 2026 23:49

The base branch was changed.

vanceingalls and others added 4 commits June 17, 2026 16:50
…on + removeAllKeyframes

P1: gsapWriter.parity.test.ts — recast-vs-acorn parity harness (reparse-equivalence).
P2: move pure keyframe-conversion transforms (resolveConversionProps, cssIdentityValue)
    to recast-free gsapSerialize.ts so the acorn/SDK path can share them.
P3: MagicString splice primitives in gsapWriterAcorn.ts (buildVarsObjectCode, overwriteVarsArg).
P4: reference vertical slice — removeAllKeyframesFromScript ported to acorn writer +
    removeAllKeyframes SDK op (types/mutate/can) + Studio cutover (useGsapKeyframeOps),
    replacing the server-authoritative ponytail stub.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o cutover

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
… acorn ports + SDK ops

- acorn: buildKeyframeObjectCode, materializeKeyframesFromScript, addAnimationWithKeyframesToScript
- acorn: splitIntoPropertyGroupsFromScript with filterGroupKeyframes/filterGroupProperties helpers
- parity tests: materialize (2 positive + 1 no-op) and split (2 positive + 2 no-op) suites
- SDK types: materializeKeyframes + splitIntoPropertyGroups EditOp variants
- mutate.ts: handlers + can() gates for both new ops
- mutate.gsap.test.ts: 6 new tests (53 total passing)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
- acorn: updateAnimationSelectorInScript, insertInheritedStateSetInScript helpers
- acorn: splitAnimationsInScript exported (parity with recast version)
- parity: 4 new fixtures (3 cases + no-op) — 23 total parity tests
- SDK types: splitAnimations EditOp variant
- mutate.ts: handleSplitAnimations + can() gate
- mutate.gsap.test.ts: 3 new tests (56 total passing)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
@vanceingalls vanceingalls force-pushed the sdk-ws3-keyframe-ops branch from a29a0c6 to 78e6a05 Compare June 17, 2026 23:51
@vanceingalls vanceingalls merged commit a746db6 into main Jun 17, 2026
13 checks passed
@vanceingalls vanceingalls deleted the sdk-ws3-keyframe-ops branch June 17, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants