feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes#1499
feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes#1499vanceingalls wants to merge 4 commits into
Conversation
…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
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
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.tsserializer paths): unchanged on-disk regime — percentage-keyframes shape is preserved across every op. The single new behavioral change is the hoist ofresolveConversionPropsintogsapSerialize.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 theParsedGsapAcornForWriteshape the new writer ops consume;parseGsapScriptAcornForWriteis 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 byGsapAnimationshape. Five fixture rows forremoveAll, five forconvert, two formaterialize, two forsplitGroups, three forsplitAnims. Each assertsexpect(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).
- A handler in
- Two new Studio cutover helpers (
sdkGsapRemoveAllKeyframesPersist,sdkGsapConvertToKeyframesPersist) and two new hook routes inuseGsapKeyframeOps.ts. Both checksdkSession && sdkDepsthen fall back to servercommitMutation— graceful degradation preserved. applyGsapOpnow decomposes intoapplyGsapKeyframeOp(early-return) + the existing switch. Refactor preserves all dispatch arms; thedefault → undefinedpropagates correctly to the outerapplyOpdefault → 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 forfrom(), last keyframe otherwise." The comment correctly identifies "the destination = the visible resting state" forto()andfromTo(). Sanity-check edge:set()is not enumerated in the doc but falls into the "otherwise" branch and collapses to the last keyframe.set()with akeyframesblock 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): theextrasiteration filterstypeof v === "number" || typeof v === "string". Arepeat: trueoryoyo: trueboolean extra (legal GSAP) gets silently dropped through this conversion. IfconvertToKeyframesis ever called on a tween carryingyoyo: true, the boolean is lost. Cheap detection: grepObject.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 atgsapParser.tsshould 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): thekfPropoverwrite vs.prependLeftbranch — when thekeyframesproperty exists, it's overwritten; when absent, it's prepended before the first property. Edge: an empty vars object ({}) is handled by theappendLeft(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 askippedSelectorvalue as an actual CSS selector will fail on the synthetic one. Cheap detection: grepskippedSelectors; 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 nofromPropertiesandinheritedPropsis empty (no earlier same-property animations), the midpoint uses0as the from-value. That's identity-fallback for everything (correct for x/y; wrong foropacity/scalewhich should default to1). The recast version may handle this differently — your parity testtween spanning split — truncated first half + fromTo second halfusesx: 200, which doesn't expose the identity-fallback bug. Recommended: add a fixture row where the spanning tween animatesopacityfrom-identity, and confirm recast and acorn agree. If they don't, the fix isbuildIdentityMap(anim.properties)instead of0-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
1 participant
What
Brief description of the change.
Why
Why is this change needed?
How
How was this implemented? Any notable design decisions?
Test plan
How was this tested?