fix(parsers): splitting a clip keeps keyframes the parser cannot read

[miguel-heygen]

What changes for users

Splitting a clip no longer deletes the keyframes of a tween that crosses the split point when Studio cannot read those keyframes. Since 0.8.127, a keyframe list with a flag on a step (for example runBackwards: true), a spread step or a named step counts as "keyframes the parser cannot read". The split then treated such a tween as a plain tween: it cut it in half and wrote both halves without their keyframes, and it reported no skip.

Repro on 0.8.127, split at 1s:

tl.to("#a", { duration: 2, keyframes: [{ x: 0 }, { x: 100, runBackwards: true }] }, 0);
// became
tl.to("#a", { duration: 1 }, 0);
tl.fromTo("#a-2", {}, { duration: 1 }, 1);

Now:

• A tween like this that crosses the split stays as authored, and the split reports #a (keyframes spanning split), the same as for keyframes the parser can read.
• A tween wholly after the split moves to the new clip whole, keyframes included.
• When such a tween plays before the split, the new clip's starting state is unknown, so the split reports #a (unreadable keyframes before split) instead of silently starting from the wrong value.

The rule

A writer never rewrites keyframes it cannot read. authorsKeyframes(anim) (keyframes present, readable or not) is the one check for "this tween is keyframed". Writers that need to read the keyframes refuse when hasUnresolvedKeyframes is set and return the script unchanged.

• Split (both writers): keyframed means authorsKeyframes, both for what moves and for the starting state carried to the new clip. An unreadable tween before the split is reported, and no starting value is invented from it. Before this, acorn wrote a midpoint like tl.set("#a-2", { opacity: 0.25 }, 1) that no keyframe authored.
• Reading keyframes: one rule decides whether keyframes are readable. Every key must be a plain key: value with a static string name: no spread, no getter or method, no computed key other than a string literal. This applies to the outer object, to each % step and to each array step. A % step must be an inline object or a plain literal. A % object may hold only % steps, ease and easeEach; an extra array channel like y: [0, 5] makes it unreadable. A simple-array channel (x: [0, 10]) must have every value resolved, with no spread and no hole. Before this, the parser read what it could and dropped the rest, so a diamond drag or delete in Studio could delete a step's values, a channel or the whole animation, or write a syntax error. A plain literal step ("50%": 0.7, which GSAP ignores) still reads as empty and is repaired by the next edit, as before.
• Convert to keyframes (both writers): a tween that already authors keyframes is left alone. Before this, it was rewritten to keyframes: { "0%": {}, "100%": {} }.
• Split into property groups (both writers): refuses. Before this, it rewrote the tween into plain per-group tweens without its keyframes.
• Acorn keyframe edits (add, update, remove, move, resize) refuse. Add, move and resize go through ensureKeyframesNode; update and remove read the keyframes node directly and refuse there. Before this, they rewrote keyframes: steps() into empty steps, or rewrote an unreadable list.
• Property edits that replace a tween's properties refuse (both writers). Before this, they dropped the keyframes key.
• The unroller (unrollComputedTimeline) leaves a loop or helper whose tween has unreadable keyframes as authored, instead of writing it out as literal tweens without keyframes.

Tests

• Split cases for both writers, one per shape (a flagged step, a spread step, a named step, keyframes from a call): a split across the tween keeps it byte for byte and reports the skip, and a split before it moves the tween whole.
• For a flagged step and for keyframes from a call, each guarded writer above leaves the script byte for byte unchanged. A split after such a tween reports the unknown start. The recast parser reads flagged steps, so its split case covers the call shape only.
• One unroll case: a loop with unreadable keyframes stays as authored.
• A split across keyframes: steps() writes no start value on the new clip (both writers). Eleven unreadable shapes (a named step; a spread, getter, method, template or variable key in a step; a number key; an array channel beside % steps; an unresolved value, spread or hole in an array channel), plus [k] at the top level. For each, the flag is set, and move-keyframe and remove-keyframe leave the script unchanged. Each fails on the head before its fix, and removing any one guard fails its tests.
• Red first: every guard has cases that fail without it. The new cases that pass without the guards are shapes that writer already left alone (recast reads flagged steps, and removing a keyframe from steps() was already a no-op), plus the four "moves whole" pins.
• The parsers suite passed 3 runs in a row (1,164 tests). tsc, oxfmt, oxlint, Fallow and the comment checks are clean.

Follow-ups (not in this PR)

• Studio's split toast wording for these skips (Studio change, needs captures).
• Materialize and replace-with-keyframes still rebuild keyframes for direct callers. Studio uses them on purpose, with values read at runtime.
• The opt-in recast writer still has losses outside split, convert, split-into-groups and property edits, and its parser does not apply the stricter reading rule.

Not changed here

Recast keyframe edits (add, move and resize keyframe) go through recast's convert-to-keyframes, which now refuses. They were not probed separately. Materialize stays allowed: it rebuilds keyframes from values Studio read at runtime, on purpose.

Visible change

Parser-only code, but the new skip reaches Studio's split toast, which lists skipped selectors under its existing wording ("Some animations use non-ID selectors (...) and were not retargeted"). That wording already fits the spanning skip badly. Rewording the toast is Studio work with captures and belongs in its own PR.

Refused keyframe and property writes return the script unchanged. In Studio, a keyframe button on such a tween now does nothing instead of deleting the keyframes; a message for that can come with the toast change.

[github-actions]

Edit accuracy: accurate 2040 (base branch 2040), smooth 1560 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

[jrusso1020]

Reviewed at 3fd21a60. One remaining site needs the change: the Acorn writer's split still treats an unreadable keyframed tween as a plain tween when it works out the new clip's starting state. Otherwise this is right, and the guards themselves are minimal.

What holds up:

• Red first holds. With the four source files reset to main, 26 of the new cases fail, covering every guard named in the description.
• Tests at this head:
  • Parsers: 1164 passed.
  • Merged locally with current main, which now includes #5024 and touches the same files: parsers 1169 passed, Studio's full suite 7510 passed.
• The Acorn write paths production uses (studio-server and the SDK import gsap-writer-acorn) keep the keyframes for all four shapes (flag, spread, named step, steps()), or leave the script unchanged. I probed the eleven guarded writes plus:
  • remove property;
  • set arc path;
  • copy;
  • remove all keyframes.
• Reuse: no existing parser predicate does this. hasUnresolvedKeyframes is set only when a keyframes node exists and can't be parsed (gsapParser.ts:1043, gsapParserAcorn.ts:1285). So authorsKeyframes means "this tween has a keyframes key", and one helper is the right size.

Blocking: computeForwardBaselines still uses anim.keyframes (gsapWriterAcorn.ts:2531)

The description says "Split (both writers): keyframed means authorsKeyframes". In the Acorn writer that holds for applyTweenSplit, but not for the baseline pass of the same split. An unreadable tween falls through to the plain-tween code, which interpolates its flat vars into the new clip's set. For example, tl.to("#a", { opacity: 0.5, duration: 2, keyframes: steps() }, 0) split at 1:

Writer	Writes for #a-2	Skip reported
Acorn (default)	tl.set("#a-2", { opacity: 0.25 }, 1)	keyframes spanning split
Recast	nothing	keyframes spanning split

• The Acorn value is made up. 0.25 is a linear midpoint, and the real value depends on keyframes the writer can't read.
• The fix doesn't cover this case. This is the "silently starting from the wrong value" problem the description sets out to fix, and the spanning path doesn't report an unknown start to cover it.
• The same happens with an earlier set. With tl.set("#a", { opacity: 0 }, 0) before it, Acorn writes opacity: 0.25 and recast writes opacity: 0.
• It also happens before the split. A tween wholly before the split gets both the "unreadable" skip and a set from its flat vars.

The fix: mirror the recast branch.

if (authorsKeyframes(anim)) {
  const kfs = anim.keyframes?.keyframes ?? [];

I applied it locally:

• Acorn's output matches recast for all three scripts above.
• Parsers stays at 1164 passed, so no test covers this today.

A case asserting that no #a-2 set is written for a spanning steps() tween with a flat var would pin it.

Not blocking:

• Recast keyframe edits. The description notes these weren't probed. Recast reads flag, spread and named steps as readable, and some of its edits lose a step:

  • addKeyframeToScript on [{ x: 0 }, last] writes only 0% and 50%, dropping last;
  • resizeKeyframedTweenInScript keeps only 0%.

  This is the same on main, and Acorn is the default writer, so it can be a follow-up.

• Reuse in Studio: webmcp/tools/inspectTools.ts:97 reports hasKeyframes: animation.keyframes !== undefined, which says false for unreadable keyframes. authorsKeyframes fits there.

Verdict: REQUEST CHANGES
Reasoning: The guards are minimal, red first, and green against current main. The Acorn split's baseline pass still uses the old check, so a split across an unreadable keyframed tween writes a made-up starting value into the new clip, where recast writes none. The fix is two lines, verified locally.

— Rames Jusso

[terencecho]

Requesting changes on 3fd21a60 for one site the guard misses; Rames Jusso's changes-requested review at this head names the same line and I reproduced it myself. The rest holds: the keyframes text itself survives every writer path I ran.

Blocking: the Acorn split still invents a start value for an unreadable keyframed tween (gsapWriterAcorn.ts:2531, computeForwardBaselines)

• applyKeyframedTweenSplit now leaves the tween alone, but computeForwardBaselines still tests if (anim.keyframes). For an unreadable tween anim.keyframes is undefined, so it falls through to the flat-tween midpoint code at the same function and treats opacity: 0.5, duration: 2 as a plain tween.
• Reproduced at head with the PR's own splitAnimationsInScript from both writers on tl.to("#a", { opacity: 0.5, duration: 2, keyframes: steps() }, 0), split at 1 (originalId a, newId a-2, newElementStart 1). Acorn, the default writer, returns that script plus tl.set("#a-2", { opacity: 0.25 }, 1), a value no keyframe authored, and reports #a (keyframes spanning split). Recast returns the script unchanged, with the same skip report. So the two writers disagree and the new clip starts at a made-up state.
• The original tween's keyframes: steps() text is preserved, so this is not keyframe loss; it is a wrong written value for the exact input class this PR is about, and no test covers it (a split whose baseline is computed over an unreadable tween).
• Fix, as Rames suggested: if (authorsKeyframes(anim)) and anim.keyframes?.keyframes ?? [], mirroring recast, plus one test with this fixture asserting no set is written.

What I verified (head tarball, deps built, NODE_ENV=test; two lanes plus my own repro)

• About 50 writer ops over 10 unreadable-keyframe shapes on head and merge-base, through both writers: update-properties, add/move/resize keyframe, convert-to-keyframes, split-into-groups and unroll leave an unreadable tween byte-identical on head; each of them deleted or rewrote the keyframes on base. A split wholly after keeps the keyframes and moves the selector; a readable keyframed sibling is untouched.
• The flag: hasUnresolvedKeyframes is set in both parsers for identifier, spread, call, steps() and object forms, and not set on readable forms (22 ops over 3 call shapes, zero byte differences against base).
• Tests: 26 of the new cases fail with the four non-test sources swapped to base; 16 single-guard mutants are all caught; parsers 1150 passed on three runs here (two files, htmlParser.roundtrip and stableIds, need core's generators subpath built and fail the same way on base).
• Refusal is not reported as a save: the route returns changed:false and the studio blocks before sending an edit for an unreadable tween (gsapEditOutcome.ts:130).

Non-blocking (same on base unless noted)

1. A refused split shows the skippedSelectors toast with the wording "non-ID selectors ... not retargeted", which is wrong for the unreadable-keyframes case.
2. A percentage object with a computed key ([k]) leaves the flag unset, so Acorn update-properties and remove-keyframe(100) can still drop that frame.
3. materializeKeyframesFromScript and replaceTweenWithKeyframesInScript are unguarded; the studio gates them, but direct HTTP and SDK callers are not.
4. The opt-in recast writer (HYPERFRAMES_GSAP_WRITER=recast) still loses keyframes on four edits (update-props, update-keyframe, remove-keyframe, remove-all) for flag-step, spread and named arrays.
5. Not exercised: a browser run of a split on a real project, unroll with real element lists, the replace-with-keyframes route end to end.

CI. All checks at this head finished: 65 passed, 14 skipped, 0 failing, 0 pending. CI is a reference; the verdict rests on the reproduction above.

Reviewed on the PR head 3fd21a60; Rames Jusso (jrusso1020) has a changes-requested review at this head. This is a review verdict, not authorization to merge or deploy beyond what the gate already does.

— Review by tai (pr-review)

[jrusso1020]

Re-review at 69905712. My blocker from 3fd21a60 is fixed. Approving.

The blocker: a split invented a start value

• computeForwardBaselines (gsapWriterAcorn.ts:2531) now checks authorsKeyframes(anim) and reads anim.keyframes?.keyframes ?? []. A split across an unreadable keyframed tween now writes no tl.set("#a-2", …), the same as recast.
• "a split across unreadable keyframes writes no start value" in gsapWriter.parity.test.ts runs both writers. Putting the old if (anim.keyframes) back fails the Acorn case.

The new "unreadable" rule (gsapParserAcorn.ts)

• staticKeyName accepts only a plain property with a static string key, which rules out getters, methods, spreads, numeric keys and computed keys other than a string literal. A keyframes object or array step that fails it is unreadable. A % step must be a literal or a plain object. A key beside % steps other than ease or easeEach makes the whole object unreadable. An array channel with a value it can't resolve, a spread or a hole is unreadable too. Every one of these sets hasUnresolvedKeyframes, so the writers' existing guards keep the text as written. A key the old parser skipped ([k], [50]) can no longer silently drop a frame.
• The older recast parser in gsapParser.ts doesn't have the new rule. Production goes through Acorn (gsapParserExports.ts:47), and the recast copy only feeds the parity tests, so this isn't a gap.
• Mutants, each reverted on its own against the parity file:
  • the baseline fix: 1 test fails;
  • the isPlainStep check on % steps: 3 fail;
  • the stray-key else: 1 fails;
  • unresolved array values: 3 fail;
  • the top-level key check: 1 fails;
  • isPlainObject on array steps: 3 fail.

Tests at this head: parsers 1178 passed, 4 skipped, 3 todo, after building parsers, lint, studio-server and core.

Reuse / simpler (not blocking):

• Now that parseKeyframesNode rejects any non-static key up front, the continue lines for a non-property or non-string key in parsePercentageKeyframes and parseSimpleArrayKeyframes can't be reached. Those loops could read the key through staticKeyName too, so there is one way to read a key instead of three.
• From my earlier review: studio/src/webmcp/tools/inspectTools.ts:97 still reports hasKeyframes: animation.keyframes !== undefined, so an unreadable keyframed tween shows as having none. authorsKeyframes(animation) would fix that.

This approval may not open the gate by itself: a changes-requested review from the previous head is still recorded. Its blocking finding is the same baseline line, and I verified it fixed above, so what's left is that reviewer's re-review, not another commit.

Verdict: APPROVE
Reasoning: A split across an unreadable tween no longer invents a start value. Every newly unreadable shape is pinned by a test that fails without its guard, and the parsers suite is green at this head.

— Rames Jusso

[terencecho]

Approving 699057122bd2. My blocker is fixed, so this lifts my changes-requested review (5410387038, at 3fd21a60). It is a review verdict, not authorization to merge or deploy.

Fixed (verified at this head)

• computeForwardBaselines (gsapWriterAcorn.ts:2531-2532) now tests authorsKeyframes(anim) and reads anim.keyframes?.keyframes ?? []. I reran my witness through splitAnimationsInScript (split at 1): the unreadable steps() tween that spans the split writes no #a-2 and reports keyframes spanning split. With an earlier tl.set("#a", { opacity: 0 }, 0) it writes opacity: 0, not the invented 0.25, which matches recast. A wholly-before unreadable tween is skipped and its flat vars are not interpolated.
• Mutation: putting if (anim.keyframes) back fails the new acorn: a split across unreadable keyframes writes no start value test. The recast case passes either way.
• Every other read of anim.keyframes in gsapWriterAcorn.ts, which I should have done last time: :848 is reached only after the hasUnresolvedKeyframes check at :1249. :1596 returns the script when there are no readable keyframes. :1907 and :1986 sit behind the hasUnresolvedKeyframes return at :2030. The rest of the Acorn writer's .keyframes reads (:1787-:1888, :2822, :2845) are on local arrays. :1694 and :2650 already used authorsKeyframes.
• New parser rule (gsapParserAcorn.ts): a key that is not a static string, a method or getter, a spread, a stray channel beside % steps, and an array element that is unresolved, spread or a hole all mark the keyframes unreadable. Tests cover all eleven shapes.

Tests: the parsers suite passes at this head (1178 passed, 4 skipped, 3 todo), including the 208 cases in gsapWriter.parity.test.ts. CI shows no failures with nothing pending. I did not run the Studio suite or the e2e.

Not verified: the recast parser for the new key shapes. Its tests exercise the Acorn parser only. Rames notes production parses through Acorn, so I did not chase it.

Reviewed on 699057122bd2. Rames approved this head (5411231627).

— Review by tai (pr-review)