Skip to content

feat(core): acorn GSAP write path — magic-string offset-splice (T6c)#1369

Open
vanceingalls wants to merge 1 commit into
06-12-feat_core_acorn_gsap_read_path_with_t6b_differential_corpus_testsfrom
06-12-feat_core_acorn_gsap_write_path_magic-string_offset-splice_t6c_
Open

feat(core): acorn GSAP write path — magic-string offset-splice (T6c)#1369
vanceingalls wants to merge 1 commit into
06-12-feat_core_acorn_gsap_read_path_with_t6b_differential_corpus_testsfrom
06-12-feat_core_acorn_gsap_write_path_magic-string_offset-splice_t6c_

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds the acorn-based GSAP write path — offset-safe AST splicing via `magic-string`. This is the second parser PR (T6b → T6c → T6d) and is what enables SDK mutation ops to add, update, and remove tweens without corrupting the surrounding script.

Why

The previous write path (in `packages/core/src/parsers/gsapParser.ts:1135-1730`) worked with regex-extracted position offsets. Those offsets are fragile: after any splice the byte positions of every subsequent node shift. `magic-string` solves this by tracking a mutable string with pending edits that are all applied once at the end — no cascading offset bugs regardless of how many edits are batched.

What changed

`packages/core/src/parsers/gsapWriterAcorn.ts` (new, ~370 lines)

  • `addAnimationToScript(script, animation: Omit<GsapAnimation, "id">)` — injects a new `tl.to/from/fromTo/set(...)` call after the last existing tween or after the `gsap.timeline()` declaration
  • `updateAnimationInScript(script, id, newProps)` — updates duration / ease / properties / fromProperties / position in-place in an existing tween call
  • `removeAnimationFromScript(script, id)` — removes a tween by `animationId`
  • `addKeyframeToScript / updateKeyframeInScript / removeKeyframeFromScript` — upsert/delete individual percentage keyframes inside a `keyframes: {...}` object
  • `valueToCode(value: number | string)` — serializes numeric and string GSAP prop values; object-valued extras (stagger, motionPath, etc.) are serialized separately via `gsapSerialize.ts`'s `serializeExtras` in the SDK layer (feat(sdk,core): phase 3b — 8 gsap/label ops + setClassStyle #1379)
  • Note: `addLabelToScript` / `removeLabelFromScript` are not in this PR — they land in the Phase 3b commit (feat(sdk,core): phase 3b — 8 gsap/label ops + setClassStyle #1379) which extends this file with +57 lines

`packages/core/src/parsers/gsapParserAcorn.ts` (+41 lines in this PR)

  • `ParsedGsapAcornForWrite` type + `parseGsapScriptAcornForWrite(script)` — write-path slice: strips non-essential data, adds `located[]` with AST node refs for splice ops
  • `TweenCallInfo` export (minimal AST handle for the writer)

`packages/core/src/parsers/gsapWriter.acorn.test.ts` (new, ~284 lines)

  • Round-trip tests for all writer ops
  • Edge cases: chained label removal, keyframe position moves that preserve existing properties

`packages/core/package.json`

  • Added `magic-string` dependency

Deferred / known gaps

  • Round-trip test (`parseGsapScriptAcornForWrite(addAnimationToScript(s, anim)).located` assertion): implicit in prod via the re-parse at `addAnimationToScript:208` but not an explicit test. Deferred to write-path test phase.
  • 2× `acorn.parse` in `addAnimationToScript`: re-parses output to derive the new ID. Pre-computed ID via `assignStableIds` is cleaner; deferred since the hot path batches through `apply-patches.ts`.
  • Label ops (`addLabelToScript`, `removeLabelFromScript`) and `updateAnimationInScript extras` handling land in feat(sdk,core): phase 3b — 8 gsap/label ops + setClassStyle #1379 (fix-hoist pattern).

Test plan

  • `bun run test packages/core` — all write-path tests pass; T6b differential tests still green
  • Stacked on: T6b (read path)
  • Stack above: T6d (parity suite)

vanceingalls commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

@vanceingalls vanceingalls force-pushed the 06-12-feat_core_acorn_gsap_read_path_with_t6b_differential_corpus_tests branch from 16ec510 to eb2633b Compare June 12, 2026 17:13
@vanceingalls vanceingalls force-pushed the 06-12-feat_core_acorn_gsap_write_path_magic-string_offset-splice_t6c_ branch from 51571d0 to 8681a02 Compare June 12, 2026 17:13

@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.

Approve-with-concerns. Write-path mechanics look right (single-MS, overwrite/appendLeft, no left-to-right offset drift). But the PR body lists capabilities that live in #1379, not here; and the test suite is missing the most important correctness signal for a "round-trip safe" writer.

What this PR actually ships (vs body):

  • Animation ops: addAnimationToScript, updateAnimationInScript (duration / ease / properties / fromProperties / position), removeAnimationFromScript.
  • Keyframe ops: add / update / removeKeyframeFromScript.
  • parseGsapScriptAcornForWrite exported from gsapParserAcorn.ts (+41 lines) — internal AST handle for the writer.
  • New magic-string dep + ./gsap-writer-acorn package export.

Blockers:

  • PR body claims that aren't in this diff. Three deliverables advertised in "What changed" are actually in #1379, not here:
    • addLabelToScript / removeLabelFromScript — not in the new file (verified: file ends at removeKeyframeFromScript line 369). They're added by #1379's gsapWriterAcorn.ts hoist (+57 lines).
    • updateAnimationInScript handling updates.extras — also hoisted by #1379 (the if (updates.extras) { ... } block at line 179).
    • valueToCode "serializes stagger objects and arrays (JSON.stringify fallback)" — actual signature is valueToCode(value: number | string): string and uses String(value) for non-string. Passing an object would emit [object Object]. GsapAnimation.extras is typed Record<string, unknown> in gsapSerialize.ts:23, so an extras value that's an object/array WILL hit this path once #1379 wires it up.
    • Net: a reviewer reading the body thinks labels + extras + object-valued props are landed and tested here. They're not. Either retitle the body to match the slice ("labels + extras come in T6d/3b"), or hoist the label ops and the extras block back down to this PR — your call but the contract should match the diff.

Concerns (non-blocking but real):

  • No write(read(s)) === s round-trip test. gsapWriter.acorn.test.ts has 5 toBe(SCRIPT_X) assertions but all are no-op paths (empty updates, unknown ID). The strongest correctness property for an "edit-in-place AST writer" is "a real edit produces output that re-parses to the expected animation set" — i.e. parseGsapScriptAcornForWrite(write(s, edit)).located matches the edit. None of the tests exercise the reader on the writer's output. Given the new writer literally calls parseGsapScriptAcornForWrite(result) inside addAnimationToScript to derive the new ID (line 667), the round trip is already implicit in the prod code path — but not covered by a test that would catch it breaking.
  • addAnimationToScript re-parses output to get the new ID (line 667). That's 2× acorn.parse per add. #1379 batches 8 ops via apply-patches.ts; if multiple are adds, this becomes the dominant cost. Cleaner: return the predicted stable ID via the same id-assignment fn the reader uses, computed on the in-memory AST you just built (you have selector + method + position + propertyGroup — that's the whole assignStableIds input).
  • removeAnimationFromScript ID-fuzzing fallback (line 677): animationId.replace(/-from-|-fromTo-/, "-to-") silently retries with a converted ID. If a caller has a stale from-method ID and the script happens to contain a same-target+position to tween, you'll delete the wrong one. Either document this fallback loudly or remove it — it's a sharp edge nobody will see at the call site.
  • Trivia / comment preservation untested. magic-string preserves bytes verbatim, so comments inside an unedited region are safe — but the read path (T6b, #1368) parses without onComment, and the writer's buildTweenStatementCode emits no comments. If updateAnimationInScript ever rebuilds part of the vars object (it doesn't today — it overwrites individual values), comments inside that object would be wiped. Worth at least one fixture with // inline comment inside the vars to lock the invariant.
  • Multi-edit ordering in one updateAnimationInScript call. You apply duration → ease → properties → fromProperties → position to the same MagicString instance, all overwrite/appendLeft against original-source offsets. magic-string handles this correctly because all edits resolve against the original byte map, not the running result — but the test suite never exercises >1 update field at once, so the invariant is implicit. One test that updates { duration, ease, position } simultaneously and checks all three landed would lock it.

Nits:

  • findInsertionPoint is mentioned in the PR body as an "extracted helper" but the diff inlines the logic in addAnimationToScript (and again in addLabelToScript over in #1379). Either extract it for real or drop the mention.
  • valueToCode type signature number | string while GsapAnimation.properties is typed wider — as number | string casts inside upsertProp callers are the giveaway. Tighten or widen.
  • Four // fallow-ignore-next-line complexity escapes in the new file. Most have a clear extract-helper exit; the ones in updateAnimationInScript and addKeyframeToScript are the most worthwhile to refactor later.
  • removeAnimationFromScript newline-after-end check uses script[exprStmt.end] === "\n" — fine for LF; CRLF (\r\n) would leave a dangling \r. Probably not a real concern given upstream normalization, but worth a comment if intentional.

Things NOT in the diff worth checking:

  • Old gsapWriter deprecation timeline. The body says "the previous write path (in gsapWriter.ts)" — there's no such file; the old write ops live in packages/core/src/parsers/gsapParser.ts:1135-1730. No deprecation marker / migration plan in this PR. Worth a roadmap note ("T6d removes the parser-resident write ops") so it doesn't ossify into permanent two-path-ism.
  • Determinism contract — clean here. No Date.now() / Math.random() / fetch(); only Math.abs for percentage tolerance. ✅
  • CI coverageregression shards still in flight at review time. Worth confirming gsap-letters-render-compat (shard-8) is green before merging since it's the closest thing to a real-world GSAP renderer touchpoint.

Cross-stack coherence:

  • #1379's tip touches gsapWriterAcorn.ts with +61 lines (label ops + extras). That's the load-bearing fix-hoist signal: the body of this PR describes deliverables that actually land at the stack tip. Calling it out because of the Graphite-stack pattern where R3 reviewers cross-read the tip's cumulative diff and the per-PR body falsely signals "shipped".
  • parseGsapScriptAcornForWrite is consumed exclusively by gsapWriterAcorn.ts in this PR and #1379 — no other entry points yet. The ./gsap-parser-acorn and ./gsap-writer-acorn package exports added here aren't reachable from apply-patches.ts until #1379 wires them. That's expected for a stack, just noting that this PR ships fully dead code until #1379 lands.

Review by Rames D Jusso

@vanceingalls vanceingalls force-pushed the 06-12-feat_core_acorn_gsap_read_path_with_t6b_differential_corpus_tests branch from eb2633b to c68d97d Compare June 12, 2026 19:36
@vanceingalls vanceingalls force-pushed the 06-12-feat_core_acorn_gsap_write_path_magic-string_offset-splice_t6c_ branch from 8681a02 to 8bc4979 Compare June 12, 2026 19:36
@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Thanks @james-russo-rames-d-jusso. Addressing the blockers:

PR body corrected — removed the three false claims:

Concerns:

  • No round-trip test — agreed; adding one is on the list for the write-path test phase. Current tests cover the no-op paths; the round-trip is implicit in addAnimationToScript (it re-parses its own output to derive the new ID) but not asserted.
  • Re-parse for new ID (2× acorn.parse) — the pre-computed ID approach is better long-term; deferred to a follow-on since the hot path goes through apply-patches.ts which batches ops before any serialization round-trip.
  • ID-fuzzing fallback in removeAnimationFromScript — will add a comment at the call site making the retry explicit and warning about the stale-ID edge case.
  • findInsertionPoint — extracted helper mention removed from body; the logic is inline.

@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.

R2 Verification — #1369

feat(core): acorn GSAP write path — magic-string offset-splice (T6c) — Vance Ingalls.

HEAD: 8bc4979 (committed 2026-06-12 18:05 UTC).
R1 commit: 8681a02 (committed 2026-06-12 17:02 UTC).
R1 review: 2026-06-12 17:19 UTC.
Author comment: 2026-06-12 20:45 UTC ("Thanks @james-russo-rames-d-jusso. Addressing the blockers…").

CI Status

All required checks green at HEAD:

  • regression-shards 1–8 ✅ (including shard-8 gsap-letters-render-compat — the closest real-world GSAP touchpoint, flagged in R1 as worth confirming).
  • Preflight (lint + format) ✅, player-perf ✅, preview-regression ✅, regression ✅.
  • Perf: drift / fps / load / parity / scrub ✅.
  • One Perf: ${{ matrix.shard }} failure — uninstantiated matrix template, not a real check.
  • Graphite mergeability_check pending (downstack blocker, expected for stack-mid PR).

Critical meta-finding: zero code delta + body unchanged since R1

  • diff packages/core/src/parsers/gsapWriterAcorn.ts @ 8681a02 vs 8bc4979byte-identical.
  • diff packages/core/src/parsers/gsapWriter.acorn.test.ts @ 8681a02 vs 8bc4979byte-identical.
  • PR body via gh pr view 1369 --json body STILL contains all five false-claim phrases R1 flagged:
    • addLabelToScript(script, name, position) listed in deliverables.
    • removeLabelFromScript(script, name) listed in deliverables.
    • valueToCode(value: unknown) "serializes GSAP prop values including stagger objects and arrays (JSON.stringify fallback prevents [object Object] in emitted code)".
    • findInsertionPoint "extracted helper" claim.
    • addAnimationToScript(script, id, target, method, props, extras) signature with extras — actual signature at writer:185 is (script: string, animation: Omit<GsapAnimation, "id">).
  • Two force-pushes recorded (17:13, 19:36 UTC) — both rebases over downstack changes, not R1-addressing edits. Commit message unchanged from the original.
  • Vance's 20:45 comment claims "PR body corrected" but the body field shows no such edit. Either the edit didn't save, he meant to do it and forgot, or he posted the comment intending to make the edit afterward. Either way: the state of the PR as observed at R2 has none of the R1 fixes landed.

R1 Blocker verification

R1-B1: PR body advertises 3 deliverables that live in #1379 — ❌ open

Verbatim re-check at HEAD 8bc4979:

  • addLabelToScript / removeLabelFromScript: grep -nE "addLabelToScript|removeLabelFromScript" gsapWriterAcorn.tsno matches. File still ends at removeKeyframeFromScript (writer:347). PR body still claims both exist here.
  • updateAnimationInScript handling updates.extras: writer:138–183 still has only duration / ease / properties / fromProperties / position branches. No if (updates.extras) block. The body's signature addAnimationToScript(script, id, target, method, props, extras) is also wrong — actual sig is addAnimationToScript(script, animation: Omit<GsapAnimation, "id">).
  • valueToCode: signature at writer:16 is still function valueToCode(value: number | string): string; line 18–19 still if (typeof value === "string") return JSON.stringify(value); return String(value);. No unknown, no object/array branch, no JSON.stringify(value) fallback for objects. Body still claims the opposite.
  • findInsertionPoint: grep -n "findInsertionPoint" gsapWriterAcorn.tsno matches. Inlined at addAnimationToScript:195–203. Body still claims the helper exists.

Vance's comment lays out the correct intent ("removed the three false claims") and the analysis is right (the object-safe path actually lives in gsapSerialize.ts via serializeExtras consumed in #1379's handleAddGsapTween/handleSetGsapTween, NOT in this PR's valueToCode). But the body itself wasn't actually edited. Until the body is updated to match the slice, this finding is open.

R1 Concern verification

R1-C1: No write(read(s)) === s round-trip test — ⚠️ partial

The test file at HEAD has more real-edit coverage than my R1 framing implied (I undercounted at R1; the file already had it("updates ease value in-place") at line 66, it("updates duration value in-place") at 77, it("updates position arg in-place") at 86, and a byte-identity outside edited ease span test at 124). My R1 "5 toBe(SCRIPT_X) assertions all no-op paths" was wrong — three of those toBes are real-edit byte-identity assertions on unchanged spans, which is a legitimate (though indirect) correctness signal.

What's still missing — and what Vance explicitly acknowledges ("No round-trip test — agreed; adding one is on the list for the write-path test phase") — is a test of the form:

const { script: written, id } = addAnimationToScript(SCRIPT_X, anim);
const reParsed = parseGsapScriptAcornForWrite(written);
expect(reParsed?.located.find((l) => l.id === id)).toMatchObject({...});

grep -nE "parseGsapScriptAcornForWrite\(.*\b(updateAnimation|addAnimation|removeAnimation|addKeyframe|updateKeyframe|removeKeyframe)" gsapWriter.acorn.test.tsno matches. The writer DOES already do this internally at addAnimationToScript:208 to derive the new ID, so the round trip is implicit in prod — but if parseGsapScriptAcornForWrite ever drifts from gsapWriterAcorn's output shape, this test would catch it and the suite as written would not.

Vance has accepted this; it's deferred to the write-path test phase. Reasonable deferral for this slice; not a merge-blocker on its own. Partial.

R1-C2: addAnimationToScript re-parses output to derive new ID (2× acorn.parse) — ❌ open, deferred

Writer:208 still calls parseGsapScriptAcornForWrite(result) after writing. Vance acknowledged in comment: "pre-computed ID approach is better long-term; deferred to a follow-on since the hot path goes through apply-patches.ts which batches ops before any serialization round-trip." Reasonable defer for a non-blocking concern. Open but accepted.

R1-C3: ID-fuzzing fallback in removeAnimationFromScript — ❌ open

removeAnimationFromScript:218 still has the uncommented fallback:

const convertedId = animationId.replace(/-from-|-fromTo-/, "-to-");
target = parsed.located.find((l) => l.id === convertedId);

No docstring above, no inline // comment explaining intent. Vance's comment says "will add a comment at the call site making the retry explicit and warning about the stale-ID edge case" — but the file wasn't touched between R1 and HEAD. The comment is forward-looking, not landed. Open.

Per the open-item alternative-resolution rubric: documenting intent via a comment WOULD legitimately close this as ✅ (intent-unmisreadable = resolved). The comment just hasn't been added yet.

R1-C4: Trivia / comment preservation untested — ❌ open

grep -n "// inline comment\|preserves comment\|comment-preserve" gsapWriter.acorn.test.ts → no matches. No fixture with // inline comment inside vars. Test file unchanged. Open.

Low-severity defer, but worth a one-line fixture once the round-trip phase happens.

R1 Nits verification

Not verified individually — nits are dismissable by author per repo convention. All nits (multi-edit ordering test, valueToCode type tightening, fallow-ignore escapes, CRLF newline check) remain unaddressed in code; consistent with no-code-delta finding.

Summary

ID Finding Verdict
R1-B1 PR body advertises 3 deliverables that live in #1379 ❌ open
R1-C1 No round-trip test ⚠️ partial (deferred + my R1 framing overstated)
R1-C2 addAnimationToScriptacorn.parse ❌ open (accepted defer)
R1-C3 ID-fuzzing fallback in removeAnimationFromScript ❌ open (comment forward-looking, not landed)
R1-C4 Trivia / comment preservation untested ❌ open

Resolved: 0/5.
Partial: 1/5 (R1-C1, with author acknowledging + deferring).
Open: 4/5.

New findings (R2-NEW)

  • R2-NEW1: PR body still mismatches the diff. This is the same finding as R1-B1, restated because the gap between Vance's comment ("PR body corrected") and the actual body field state is now itself a meta-issue. The author either lost the body edit before saving, or posted the comment in advance of the edit. Either way: before merge, the body must match the diff. A reviewer cross-reading the stack tip (#1379) for fix-hoist patterns will see gsapWriterAcorn.ts +57 there with label ops + extras and assume this PR's body is honest, then merge mid-stack with wrong expectations.

  • R2-NEW2: addAnimationToScript signature drift in body. Body lists addAnimationToScript(script, id, target, method, props, extras) (6 params). Actual signature at writer:185 is addAnimationToScript(script, animation: Omit<GsapAnimation, "id">) (2 params). This is a separate body-vs-code mismatch from the 4 R1-B1 ones; surface it to Vance explicitly so the body rewrite catches it.

Verdict

Approve-with-concerns, blocker on PR body unchanged.

The write-path mechanics remain sound (unchanged code, same correctness reasoning as R1, CI green including the GSAP renderer-compat shard). The only true merge-blocker is the body-vs-diff mismatch, which is a 2-minute fix Vance has already drafted in his comment — it just needs to actually land in the body field. Once the body is corrected to match the slice (deferring label ops + extras + object-valued valueToCode to #1379), this is clean to merge as a stack-mid PR.

R1-C3 (ID-fuzzing fallback) would also be cheaply closeable by adding the comment Vance promised. R1-C1 round-trip test is a legitimate defer per author's own acknowledgment.

CI

✅ All required checks green at HEAD 8bc4979. Graphite stack-mergeability pending on downstack PRs (expected). No flake. The gsap-letters-render-compat real-world GSAP renderer touchpoint (shard-8) is green at 22m26s.

Review by Rames D Jusso

@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.

Pre-review checklist

Existing reviews: 2 × COMMENT from james-russo-rames-d-jusso (no blockers).

CI: All 8 regression shards ✅, all named Perf: shards ✅, Preflight ✅, Preview parity ✅. Perf: ${{ matrix.shard }} failure is a known infra artifact (log expired; same job skipped on main). Graphite mergeability_check pending is not a required gate. Green.


Review

The T6c write path is well-conceived: parse once via the existing acorn machinery, then splice byte ranges with magic-string so every unedited character is preserved verbatim. The code is clean and the test suite covers all six public functions meaningfully.

Findings

gsapWriterAcorn.ts:63 — dead "ObjectProperty" branch (nit)

function isObjectProperty(prop: any): boolean {
  return prop?.type === "ObjectProperty" || prop?.type === "Property";
}

Acorn always emits "Property" nodes; "ObjectProperty" is Babel AST. The first arm will never fire. Harmless, but should be removed to avoid confusion if someone greps for Babel types in this file.


gsapWriterAcorn.ts:196-199 — silent ID-normalization fallback in removeAnimationFromScript (important)

const convertedId = animationId.replace(/-from-|-fromTo-/, "-to-");
target = parsed.located.find((l) => l.id === convertedId);

The stable IDs emitted by assignStableIds include the method token (-from-, -to-, -fromTo-). If a caller passes an ID with the wrong method token, this normalization silently retargets the removal to a different node that happens to be at the same selector+position. That could delete the wrong tween without any error signal. If this aliasing is intentional (e.g. to paper over an ID-generation inconsistency between the read path and the write path), it deserves a comment explaining which callers trigger it. If it is not intentional, it should be removed.


gsapWriterAcorn.ts:162-163 — silent empty-string ID on add failure (important)

const newId = reParsed?.located[reParsed.located.length - 1]?.id ?? "";
return { script: result, id: newId };

The re-parse on line 160 can fail (returns null) if the generated tween statement is somehow un-parseable, and it can also simply miss the newly added animation (e.g. if the position argument produced unexpected syntax). In both cases the caller gets back id: "". Since the id is the only handle callers will use to update/remove the new animation later, a silent "" is dangerous — it will be stored in whatever state layer the caller owns and silently become a no-op on all future write operations. At minimum a console.warn or a documented contract ("check id !== '' before storing") would help; ideally throw.


gsapWriterAcorn.ts:113removeProp comma arithmetic on last-only case (nit)

if (editableProps.length === 1) {
  ms.remove(propNode.start, propNode.end);
}

When there is exactly one property this correctly removes only the prop content, leaving keyframes: {}. That may be surprising UX (an animation with an empty keyframes object is a no-op in GSAP), but T6c's stated scope is "no collapse" so this is acceptable as-is — worth a comment saying single-keyframe collapse is deferred to T6d.


gsapWriterAcorn.ts:157-168 — double-parse in addAnimationToScript (nit)

buildTweenStatementCodems.appendLeftms.toString()parseGsapScriptAcornForWrite(result) — two full acorn parses per add call. Fine at current scale; if add becomes hot (batch imports), the ID could be derived deterministically from the input animation fields rather than from re-parsing. Not a T6c blocker.


Test coverage gaps (nit)

  • gsapWriter.acorn.test.ts — no test for removeKeyframeFromScript when only one keyframe remains; expected output is keyframes: {} and callers should handle that.
  • No test for removeAnimationFromScript where the ID has the wrong method token (the -from--to- fallback path).
  • No test for updateAnimationInScript with updates.fromProperties on a fromTo call.

The existing suite is solid for the happy path; the above are edge cases worth adding in a follow-up.


gsapParserAcorn.ts additions

ParsedGsapAcornForWrite and parseGsapScriptAcornForWrite are clean. The located array zipping calls[i]animations[i] is safe because both are built from the same calls slice in source order. The outer try/catch → null is the right default. TweenCallInfo export is minimally invasive. ✅


Offset arithmetic correctness

Spot-checked the two critical pieces:

  • upsertProp: appendLeft(objNode.end - 1, ...) — in acorn end is exclusive, so end - 1 is the index of }. MagicString.appendLeft(i, s) inserts s before the character at i, so the new prop lands just before }. ✅
  • removeAnimationFromScript chain vs. standalone detection: the guard N.callee?.object?.type !== "CallExpression" && exprStmt?.expression === N correctly routes inner chain links (where exprStmt.expression is the outermost call, not N) to the else splice path. Tested manually against SCRIPT_C structure. ✅

Verdict: APPROVE
Reasoning: The core offset arithmetic is correct, the magic-string splicing is byte-precise, all public functions have happy-path coverage, and CI is green. The two "important" findings (ID-normalization fallback, silent empty-id-on-add) are worth hardening but do not introduce data corruption — at worst they silently no-op. Neither is a blocker for this T6c scope. The nits are cosmetic.

— magi

@vanceingalls vanceingalls force-pushed the 06-12-feat_core_acorn_gsap_write_path_magic-string_offset-splice_t6c_ branch from 8bc4979 to 01039fa Compare June 12, 2026 22:20
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.

3 participants