fix(studio): restore timeline move/resize fallback parity (review #1466)

[vanceingalls]

Addresses the #1466 review findings (Via + Miguel). Lands as one fix PR at the stack tip — everything is behind STUDIO_SDK_CUTOVER_ENABLED (default false, dark launch), so no mid-stack restack needed.

#1466 — real regressions fixed (both fallback entry points)

• Resize dropped live data-playback-start/data-media-start patch → restored conditional liveAttrs push.
• Move/resize fallback dropped GSAP-position sync + reloadPreview → restored shift/scaleGsapPositions + reloadPreview() (SDK path folds both into setTiming; fallback must do them explicitly).
• Undo-coalesce drift → plumbed coalesceKey through persistTimelineEdit so SDK and fallback paths coalesce identically.
• Documented hasPbsAdjustment 2nd clause + sdkTimingPersist before-capture transition limitation.

#1500 — no code change needed

• Parity-harness gap (request-changes): already closed at the tip — arc + unroll have recast-vs-acorn parity (added with the WS-3.F shift/scale port).
• blockRemoveRange comma-residue (potential): verified correct — no residue on first/middle/last/multi-block removal.
• Remaining items (segments-drop warn, curviness default asymmetry, single-axis arc, xPercent) are comment-tier robustness nits, deferred.

tsc + timeline tests (37) + oxfmt/oxlint green.

🤖 Generated with Claude Code

[miga-heygen]

Review: fix(studio): restore timeline move/resize fallback parity (review #1466)

Three commits, six files changed, +89/-33 — a focused fix PR that addresses three distinct review findings from the SDK cutover stack. Reviewed each commit's intent against the diff. Here's the breakdown.

---

Commit 1: Restore timeline move/resize fallback parity

Resize live DOM patch — data-playback-start/data-media-start restoration (useTimelineEditing.ts L215-231)

Good fix. The pre-cutover resize handler patched all three timing attrs on the live DOM; the §3.2 rewrite dropped the conditional playback-start/media-start patch. This correctly restores it: the playbackStartAttr check disambiguates the two possible attr names, and it only fires when updates.playbackStart != null. Clean.

Move fallback — GSAP position sync + reloadPreview (useTimelineEditing.ts L162-178)

The moveFallback closure correctly:

1. Calls enqueueEdit with coalesceKey for undo parity
2. Computes delta from updates.start - element.start
3. Calls shiftGsapPositions only when delta !== 0 && element.domId && pid — correct guards
4. Always calls reloadPreview() (even when no GSAP shift is needed)
5. .catch on the GSAP call logs but doesn't reject — the preview still reloads

One observation: shiftGsapPositions(...).then(() => reloadPreview()) means if the shift succeeds, preview reloads. But if the shift fails, the .catch fires and reloadPreview() is never called in that branch. The return reloadPreview() on L177 only runs when delta === 0 or !element.domId. This is probably fine in practice (a failed server mutation shouldn't reload a stale state), but worth confirming this is intentional — if the user expects visual feedback even on a failed shift, they'd get a stale preview. Logging is the right call either way.

Resize fallback — GSAP scale + reloadPreview (useTimelineEditing.ts L265-287)

Same pattern as move, same .catch behavior. The scaleGsapPositions call passes all six positional args (pid, targetPath, domId, oldStart, oldDuration, newStart, newDuration). Matches the export signature scaleGsapPositions(projectId, targetPath, domId, oldStart, oldDuration, newStart, newDuration) from the helpers file. Correct.

coalesceKey plumbing (timelineEditingHelpers.ts)

Adding coalesceKey?: string to PersistTimelineEditInput and passing it through to recordEdit — clean, non-breaking addition. The optional field means existing callers without a coalesceKey are unaffected. The key format timeline-move:${element.hfId ?? element.id} / timeline-resize:${...} mirrors the SDK branch's key, which is exactly the point.

One design note: using element.hfId ?? element.id for the fallback coalesceKey while the SDK branch used element.hfId directly (since SDK always has hfId). This means the fallback key could differ from the SDK key when hfId is null — but that's also exactly the case when the SDK path can't run (no hfId → no SDK), so undo coalescing stays consistent within each path. Correct.

hasPbsAdjustment comment improvement (useTimelineEditing.ts L253-256)

The old comment was terse. The new comment explains why the second clause exists (trimming start of a clip with pbs implicitly shifts the in-point, which setTiming can't express). Good documentation of a non-obvious guard.

---

Commit 2: Retire duplicate removeGsapKeyframe keyframeIndex variant

types.ts — Removed the { type: "removeGsapKeyframe"; animationId: string; keyframeIndex: number } union member. The percentage-based variant remains. This eliminates the TS ambiguity where two members share the same type discriminant but different shapes — TS can't narrow on type alone when two members have the same literal. Clean fix.

mutate.ts — dispatchRemoveGsapKeyframe simplified from a conditional branch to a direct call to handleRemoveGsapKeyframeByPercentage. The dead handleRemoveGsapKeyframe function (18 lines) is removed. resolveKeyframe is correctly kept — it's still used by setGsapKeyframe (which takes a keyframeIndex for setting values at a known position).

mutate.gsap.test.ts — The one test that used keyframeIndex: 1 is converted to percentage: 50. Test description updated from "removes keyframe at index 1 (50%)" to "removes keyframe at 50%". The percentage value 50 corresponds to the 50% keyframe in the test fixture. Verified this maps correctly.

No concerns here. The retirement is clean and the PR description correctly notes there are no production callers of the index-based API.

---

Commit 3: Gate ALL cutover persist paths on STUDIO_SDK_CUTOVER_ENABLED

sdkTimingPersist (sdkCutover.ts L174) — if (!STUDIO_SDK_CUTOVER_ENABLED) return false; added before the session check. Without this, sdkTimingPersist would run whenever an SDK session exists (which it always does for shadow/selection). This is the critical fix: without the flag gate, flipping the cutover flag OFF couldn't actually disable the timing cutover path.

dispatchGsapOpAndPersist (sdkCutover.ts L236) — Same pattern. This is the shared chokepoint for every GSAP-op cutover (keyframes, properties, tweens, arcs). Same reasoning applies.

sdkDeletePersist (sdkCutover.ts L340) — Same pattern for the delete path.

All three return false when the flag is off, which causes the caller's .then((handled) => { if (!handled) return fallback(); }) chain to take the fallback. Correct.

The comments on each guard are helpful — they explain why a simple session check was insufficient (session always exists for shadow/selection).

---

Cross-cutting observations

1. No new React hooks issues. The useCallback dependency arrays for handleTimelineElementMove and handleTimelineElementResize look correct. projectIdRef is a ref (stable identity), shiftGsapPositions/scaleGsapPositions are module-level async functions (stable), and reloadPreview was already in the deps array. No missing deps.

2. CI status: One Detect changes job failed but on a different workflow run (81980564073 vs the passing 81980631123). The core checks (Preflight, Preview parity, WIP) all pass. The failure looks like a flaky base-branch detect job, not a PR issue.

3. No unnecessary complexity. Each fix is surgical — no new abstractions, no generalization for generalization's sake. The fallback closures (moveFallback, resizeFallback) are local, well-scoped, and mirror the existing control flow.

4. The sdkTimingPersist "before" comment (L180-185 in the diff) is an honest documentation of a known limitation during the dark-launch transition: the SDK's serialized state may drift from disk while the server still writes some paths. Acknowledging this rather than hand-waving it is good. The "revisit once cutover is always-on" note is appropriate — this is acceptable tech debt for a gated launch.

---

Verdict

All three review findings (#1466 regressions, #1498 duplicate union, cutover gating gap) are correctly resolved. The fallback paths now have full parity with pre-cutover behavior. The flag gating ensures the stack is genuinely dark-launchable. No bugs, no missing deps, no unnecessary complexity. The minor .catch → no-reload observation is a conscious tradeoff (logged, not silent), not a bug.

LGTM — ready for stamp.

— Miga

[james-russo-rames-d-jusso]

R2 verification — fix-PR for the 18-PR SDK cutover stack (reviewed at 18ea1c32)

Layering on Miga's R2 (LGTM at the same SHA) — I agree with their per-commit verdict and the .catch → no-reload observation. Adding what I tracked from my own R1 rollup on the parent stack + one fresh finding Miga's pass didn't flag.

R1 finding verification (cross-PR rollup)

A) Timeline fallback parity (originally flagged on #1500 / #1466) — ✅ resolved.

• Resize live DOM patch restored at useTimelineEditing.ts:218-232 — conditional data-playback-start/data-media-start push reinstated, attr-name disambiguation mirrors the timelineEditingHelpers.ts:72,83 convention.
• Move/resize fallback GSAP sync + reloadPreview restored at useTimelineEditing.ts:162-178 (moveFallback) and :262-287 (resizeFallback).
• coalesceKey plumbed through persistTimelineEdit at timelineEditingHelpers.ts:100,123 — undo granularity now matches between SDK and fallback paths.
• Parity-harness gap I flagged on #1500 — PR body claims it was already closed at the stack tip with the WS-3.F shift/scale port. I didn't re-verify that on the tip (this PR doesn't touch the parity harness); taking on faith from the body and the merged shift/scale port.

B) Union retire — duplicate removeGsapKeyframe discriminator (originally flagged on #1498) — ✅ resolved.

• types.ts:101 member removed (the keyframeIndex variant); mutate.ts:153 dispatcher simplified to direct percentage call; dead handleRemoveGsapKeyframe (18 lines) deleted.
• No remaining removeGsapKeyframe + keyframeIndex callsites in repo (grep -rn 'removeGsapKeyframe.*keyframeIndex' packages/ is empty). The one test that referenced the by-index API converted to percentage: 50.
• resolveKeyframe correctly retained — setGsapKeyframe still uses keyframeIndex, which is fine (single-variant op, no discriminator collision).
• Per the dispatch-map silent-drop lens — no risk here, because TS union narrowing now leaves only one shape on removeGsapKeyframe; any caller passing keyframeIndex would fail to type-check rather than silently no-op.

C) Cutover gating — the data-loss-on-Cmd-Z seam I flagged on #1522/#1524 — ✅ resolved, but worth being explicit about what resolved it. Two distinct mechanisms:

• The persist-queue race itself (SDK auto-write queue + Studio's explicit write double-writing the same path; oldSession.flush() on forceReload overwriting undo writes) was already closed at HEAD by useSdkSession.ts:78-121 opening the session with no persist queue (openComposition(content, { history: false }), no persist/persistPath args) and the cleanup doing compRef.current?.dispose() only — explicitly no flush(). The comment at L94-101 + L117-119 documents this design choice with the exact "would race the revert write on undo/redo reload" rationale I called out.
• This PR's commit 3 adds the complementary defense: flag-off → cutover persist paths return false early → caller takes the legacy server path. Without commit 3, flipping STUDIO_SDK_CUTOVER_ENABLED to false would not have disabled sdkTimingPersist / GSAP-op cutover / delete cutover (they only guarded on !sdkSession, and the session always exists for shadow/selection). So flag-off on merge is now a true no-op in prod — exactly what dark launch requires.

Together: the data-loss class is closed both at the design level (no queue, no flush) and at the rollout-control level (flag-off → legacy path). Both pieces are load-bearing.

D) Other R1 items I flagged across the stack — partially carried by this PR:

• #1471 fire-and-forget race on forceReloadSdkSession (Promise-return / epoch token) — ➕ not in scope; the dark-launch gate makes it a flip-prerequisite rather than a merge-blocker, which matches the PR body's framing.
• #1466 shiftGsapPositions / scaleGsapPositions GSAP-sync restoration — ✅ resolved by this PR (Commit 1).
• #1499 extras boolean filter, #1500 elements: [] validation, #1490 NaN / 60fps querySelector — ➕ out of scope, deferred to follow-up per PR body.

Fresh findings

(concern, not blocker) No regression test for the dark-launch flag-gate. Commit 3 is the load-bearing fix for "flag off = true no-op", but sdkCutover.test.ts:15-17 mocks STUDIO_SDK_CUTOVER_ENABLED: true at module level and there's no sibling test covering flag-off → false return → fallback taken. The whole thesis of the dark launch rests on this contract, and without a test, a future refactor of shouldUseSdkCutover / the gate guards could silently re-enable cutover on flag-off without anyone noticing until prod observes it. A 3-line test per chokepoint (sdkTimingPersist, dispatchGsapOpAndPersist, sdkDeletePersist) with the module mock re-set to false would pin this. Not blocking — but worth landing as a follow-up commit before flip, since this is the contract you'll point at when flipping.

(nit) sdkCutover.ts:211-212 — the sdkGsapTweenPersist "add" early-out checks sdkSession.getElement(op.target) before the flag-gate inside dispatchGsapOpAndPersist runs. Benign (returns false either way), but slightly defeats the "don't touch SDK paths on flag-off" intent if you want strict purity. (nit)

Verdict

Approve with the test-coverage follow-up as a soft-blocker before flag-flip, not before merge. Commits 1-3 are all surgical, well-commented, and correctly resolve the R1 findings they target. Miga's verdict stands. The dark-launch gate is the high-leverage piece of this PR — without it, the stack was shipping data-loss-class code live on merge regardless of flag state. Stack is genuinely dark-launchable now.

— Rames D Jusso

[vanceingalls]

R2 verification — stack-tip review-fixes PR (reviewed at 18ea1c32)

A late entrant to the parlour — Miga's verdict and Rames's verification are both posted above, and the gentleman is right to put them on the wire first. I find myself in pleasant agreement with the bulk of their conclusions, with two observations of my own to lay alongside: one that elevates the test-coverage soft-ask Rames already raised, and one fresh finding regarding a cross-queue stale-base squash hazard that neither pass appears to have inspected.

Verification of the three commits

Commit 1 — fb759b54 timeline fallback parity (closes the #1466 finding). The fallback closures moveFallback (useTimelineEditing.ts:166-178) and resizeFallback (:265-287) are hoisted to a shared local helper and called from BOTH the SDK-fallback branch (if (!handled) return moveFallback() at :194 / resize at :301) AND the no-session branch (return moveFallback() at :196 / resize at :303). Symmetric coverage on the parity contract — the exact shape the briefing required. No duplication-at-boundaries (pattern #1). The coalesceKey plumbing through persistTimelineEdit (timelineEditingHelpers.ts:100,123) mirrors the SDK branch's key, so undo granularity is identical on either path. The hasPbsAdjustment clause-2 comment at :253-256 correctly documents why a start-trim of a pbs-bearing clip cannot route through the SDK. Clean closure.

Commit 2 — 870872019 retire removeGsapKeyframe keyframeIndex variant (closes #1498). Verified at 18ea1c32:

• Union member removed at types.ts:101 — only the percentage variant remains.
• mutate.ts:153 dispatcher simplified to a direct handleRemoveGsapKeyframeByPercentage call; the dead handleRemoveGsapKeyframe (18 lines) excised.
• resolveKeyframe correctly retained — setGsapKeyframe is a different op that legitimately uses keyframeIndex (no discriminator collision, single-variant op).
• The sole by-index test at mutate.gsap.test.ts:402-418 converted to the percentage: 50 API.
• Dispatch-chain audit at headRefOid: useGsapKeyframeOps.ts does not call removeGsapKeyframe directly (routes through sdkGsapRemoveKeyframePersist which uses percentage); no remaining removeGsapKeyframe.*keyframeIndex co-occurrences anywhere on the branch.

But — and here is where the parlour falls silent — see the cross-queue hazard below before the union retire is considered absolutely closed.

Commit 3 — 18ea1c32 dark-launch gate at all three persist chokepoints (closes the #1469 un-gated-flag finding). Verified at headRefOid:

• sdkTimingPersist (sdkCutover.ts:174) — if (!STUDIO_SDK_CUTOVER_ENABLED) return false; at the head.
• dispatchGsapOpAndPersist (:236) — same. This is the shared chokepoint for every GSAP-op cutover (keyframes, properties, tweens, arcs), so gating here closes the entire family in one stroke.
• sdkDeletePersist (:340) — same.
• The existing sdkCutoverPersist (style/text/attr) gates via the canonical shouldUseSdkCutover(STUDIO_SDK_CUTOVER_ENABLED, …) helper at :138. So the regime is now: helper-based gate on the inline-style path (older), literal one-line guard on the three persist-function entry points (this PR).

The chosen closure shape is the right one. A more uniform refactor — broadening shouldUseSdkCutover to take ALL persist functions through it — would have been a larger diff in a stack-tip review-fixes PR, and the literal one-liner is honest about what it is. Without this commit, flag-off was an outright lie: sdkTimingPersist / GSAP-op / delete cutover ran whenever a session existed, which is always (shadow/selection opens one by default). The merge would have shipped the cutover bugs live regardless of flag state. With it, flag-off is a true no-op in prod and the remaining cutover correctness issues become flip-prerequisites rather than merge-blockers. Exactly the dark launch the PR body advertises.

Commit independence

The three commits could in principle land independently — commit 1 only touches the timeline editing surface, commit 2 only the SDK union + tests, commit 3 only the persist gates. No interdependence; the bundling is for ergonomic reasons (one fix PR off the stack tip rather than a restack of three mid-stack edits). Fine.

Findings

(blocker — cross-queue stale-base squash hazard on commit 2's union retire.) When this 18-PR stack cumulatively squash-merges onto current main, the keyframeIndex-variant retirement collides with packages/studio/src/utils/sdkShadowGsapKeyframe.ts — a file present on main (sha 38a63316) but ABSENT from this branch's base (sdk-ws3-unroll-dynamic). Two relevant lines on main:

• :128 constructs { type: "removeGsapKeyframe", animationId, keyframeIndex } — the very variant this PR retires from the union.
• :20 doc comment explicitly anticipates the collision: "SDK mapping (main, pre PR #1498 percentage-variant)… remove → removeGsapKeyframe{animationId, keyframeIndex} — landmine from PR #1498."

The author knew it was coming. The squash math will not delete sdkShadowGsapKeyframe.ts (the branch never had it, so the squash diff is computed against current main at merge time and leaves it in place). But types.ts post-merge will no longer contain the keyframeIndex member. Result: tsc red on main at line 128 — the constructed object matches no union member. This is the textbook cross-queue stale-base squash regression from the verify-r2-nits-across-stack discipline (Flavour 2).

Three possible resolutions, ranked by preference:

1. Update sdkShadowGsapKeyframe.ts:128 in THIS PR to translate percentage → percentage instead of percentage → keyframeIndex. Adds the file to the diff and resolves the post-merge type error. The cleanest path; the author cited #1498 as the landmine, so the migration is overdue regardless.
2. Rebase the stack onto current main before squash-merge. Brings sdkShadowGsapKeyframe.ts onto the branch and lets the migration land in the same PR; still requires a code change to migrate the call site.
3. Land this PR, then immediately follow with a one-commit fix-up PR migrating sdkShadowGsapKeyframe.ts:128 to the percentage variant before squash-merge. Acceptable only if the squash is gated on the follow-up landing first.

Option 1 is the cheapest fix and closes the door fully. I'd request changes on this one rather than waving it through as "the next reader's problem" — the "small footgun" pattern is exactly this shape.

(soft, escalating Rames's concern #1.) Rames already noted the missing flag-off regression test for the new dark-launch gates. I find his framing as "concern, not blocker" generous — under the local band-aid bar this is closer to pattern #1 (silent divergence in CI). The whole thesis of dark launch rests on the contract flag === false ⇒ persist returns false ⇒ caller takes fallback. The module mock at sdkCutover.test.ts:15-17 pins STUDIO_SDK_CUTOVER_ENABLED: true and there is no sibling test re-mocking it to false against sdkTimingPersist / dispatchGsapOpAndPersist / sdkDeletePersist. A future refactor of the gates — inverting a condition, hoisting into a helper, removing what looks like a "redundant" early return — passes CI green and re-enables cutover under flag-off. A three-line vi.doMock(..., { STUDIO_SDK_CUTOVER_ENABLED: false }) test per chokepoint would pin the contract you'll be pointing at when you flip the flag in prod. Worth landing before flip; not a merge blocker.

(nit, agreeing with Miga.) Miga observed that a failed shiftGsapPositions / scaleGsapPositions triggers the .catch and skips reloadPreview(). The arrangement is probably intentional (don't paint a stale preview on top of a failed server mutation), but the console.error is the only user-visible signal — a toast on persistent failure would be friendlier. The pre-cutover server path had the same shape, so this is parity-with-old-behavior rather than a new regression. Logging is the right primary call.

(nit, agreeing with Rames.) sdkGsapTweenPersist at sdkCutover.ts:211-212 checks sdkSession.getElement(op.target) before the inner flag-gate runs. Benign — the inner gate still returns false — but slightly defeats the "no SDK touch on flag-off" purity. A leading if (!STUDIO_SDK_CUTOVER_ENABLED) return Promise.resolve(false); at the top of sdkGsapTweenPersist would match the discipline of the other three. Soft.

Verdict

Minor blockers — hold for the sdkShadowGsapKeyframe.ts:128 migration before the stack squashes to main. Commits 1 and 3 close cleanly with zero new band-aids. Commit 2 closes cleanly within the stack but introduces a cross-queue stale-base squash hazard against main's sdkShadowGsapKeyframe.ts. The fix is one-line; I'd want it landed before the cumulative stack squash rather than after the inevitable red-CI ping.

The flag-test coverage gap (Rames) and the .catch-no-reload observation (Miga) are both correct and stand as soft asks before flag-flip but not before merge.

If the sdkShadowGsapKeyframe.ts migration lands in this PR (option 1) or the stack rebases to absorb the file (option 2), I expect to clear without further concerns.

Review by Via

[vanceingalls]

Thanks all — addressed:

Flag-gate regression tests (Rames + Via, soft-blocker before flip) → added packages/studio/src/utils/sdkCutover.gate.test.ts (commit 508be317d): mocks STUDIO_SDK_CUTOVER_ENABLED: false and pins all three chokepoints (sdkTimingPersist / sdkGsapTweenPersist / sdkDeletePersist) → false + no write, with a valid session present. Locks the flag===false ⇒ persist returns false ⇒ fallback contract the flip rests on.

Leading flag-gate on sdkGsapTweenPersist (Via nit) → 057e0d0e6: the add-op getElement check now runs after a leading if (!STUDIO_SDK_CUTOVER_ENABLED) return false, so flag-off does zero SDK touch at every entry, matching the other three.

.catch → no-reload on GSAP-shift failure (Miga + Via nit) → intentional, kept. Parity with the pre-cutover server path (don't paint a stale preview over a failed mutation); console.error is the primary signal. A toast on persistent failure is a reasonable follow-up but out of scope here.

Cross-queue squash hazard on the union retire (Via, blocker) → verified it does not apply to this reconciled stack. sdkShadowGsapKeyframe.ts is on main (uses the retired keyframeIndex at :128), but this stack deletes that file — git diff main...sdk-ws3-unroll-dynamic --diff-filter=D matches it. So the deletion is in the stack's contribution; post-merge the file is gone and nothing references the retired variant. (I can't apply option 1 — the file doesn't exist in this branch to edit.) The only residual is a modify/delete conflict at merge (main's 38a63316 modified it, the stack deletes it) → resolve as delete (the stack removes shadow by design). The keep-shadow reintroduction will use the percentage variant, so no future collision.

Note: this PR also now carries the full correctness pass for the 13-finding stack review (commits 7d7732c8f core/sdk, bcc0a44a6 studio, fd002ad34 fallow-clean refactor) — see the per-PR pointers I'm leaving on the stack.

[miga-heygen]

Re-review — PR #1539 at `f9bdd18fd5` (9 commits, 6 new since initial R2)

Picking up where the initial R2 left off at `18ea1c32` (3 commits). Six new commits landed since then — covering the 8 GSAP write-path findings, the merge-tween-props fix, debounce stabilization, GSAP write serialization, on-disk undo baseline, self-write identity, and the dark-launch flag-gate test + two post-review-note fixes.

---

New commits reviewed

`7d7732c8` — fix(core,sdk): correct 8 GSAP write-path review findings

Eight surgical fixes in the core writers, all correct:

1. `findKfPropByPct` closest-match (finding #2): Now tracks `bestDist` instead of returning the first hit within `PCT_TOLERANCE=2`. Keyframes at 49%/50%/51% no longer alias — the exact or nearest match wins, deterministic tie-break on index. Test `gsapWriter.reviewFixes.test.ts` pins this with a dense 0/49/50/100 fixture. Clean.

2. `enableArcPath` empty-vars collision (finding #4): The upsertProp → appendLeft overlap with removePropsByKey's end boundary is fixed by switching to `ms.appendRight(vars.start + 1, ...)` — inserts motionPath right after the opening brace, orthogonal to any removal range. The `survivesRemoval` check controls trailing comma insertion. Verified the test asserts no bare `{}` output.

3. `splitAnimationsInScript` forward baseline (finding #5): The entire reverse-accumulate-then-interpolate approach is replaced with a two-pass architecture: `computeForwardBaselines` runs forward to capture the inherited-props state available before each tween, then the existing reverse write loop uses those pre-computed baselines. This fixes the wrong-midpoint bug (x=150 instead of x=200) where a spanning tween read the reverse accumulator's state instead of earlier tweens' contributions. Well-decomposed — `buildSpanningSplit` and `applyTweenSplit` are focused helpers that each do one thing.

4. `unrollDynamicAnimations` sibling preservation (finding #9): The blanket loop-body overwrite is replaced with `buildLoopUnrollPreserving`, which iterates per-element and per-statement, substituting the loop index AST-based (`substituteLoopIndex`). Non-target statements (e.g. `tl.set(...)`) survive per iteration. The forEach/for-of fallback correctly detects "no numeric index variable" and degrades to the blanket overwrite (valid code, no dangling references). Test covers both for-loop preservation and forEach fallback.

5. AST-based index substitution (R2 #5): `substituteLoopIndex` walks the AST for real Identifier references, skipping string literals and non-computed member/key positions. The `isIndexBindingPosition` guard is correct — `obj.i` and `{ i: ... }` are left untouched. Test pins that `"row-i"` survives while `items[i]` is substituted per iteration.

6. Per-segment curviness (finding #10): Both the parser (`gsapParser.ts`) and serialize (`gsapSerialize.ts`) `buildMotionPathObjectCode` now emit the cubic form when `curvinessVaries` — segments with differing curviness no longer silently drop to the simple path (which serializes only `segments[0].curviness`). Test verifies the output is `type: "cubic"` and differs from the uniform-curviness case.

7. `readLastWaypointXY` negative coords (finding #11): New `readNumericLiteralNode` handles `UnaryExpression` with `operator: "-"` — negative waypoint coordinates (`x: -120, y: -40`) no longer vanish when disabling an arc path. Test round-trips enable→disable and asserts the negatives survive.

8. Non-null assertion cleanup throughout: All the `!.` assertions in `gsapWriterAcorn.ts` are replaced with proper nullish guards (`const first = waypoints[0]; if (!first) return []`, etc.). This is the right pattern — the assertions were correct at runtime but masked the TS narrowing. Each replacement preserves behavior while making the null path explicit.

`bcc0a44a` — fix(studio): SDK cutover review fixes — merge tween props, stabilize debounce, serialize gsap writes, on-disk undo baseline, self-write identity

This is the heavyweight studio commit. Five distinct fixes:

Finding #1 — merge tween properties: `mergeTweenProperties` in `useGsapPropertyDebounce.ts` reads the tween's current properties from the in-memory SDK doc and spreads the single edited key on top, so `setGsapTween`'s replace semantics don't drop siblings. The test documents the regression (single-key set → loses y and opacity) and verifies the merge. All six call sites (`flushPendingPropertyEdit`, `addGsapProperty`, `removeProperty`, `editGsapFromProperty`, `addGsapFromProperty`, and the inline remove-property) route through the merge. The `kind: "to" | "from"` discriminator correctly selects `anim.properties` vs `anim.fromProperties`. Fallback returns the single edit unchanged when the tween can't be found — safe default.

Finding #7 — debounce ref-stabilization: `sdkRef = useRef(sdk); sdkRef.current = sdk;` replaces the direct `sdk` capture in every callback's dependency array. Without this, the fresh object literal on every render re-fires the `useCallback` identity → re-fires the cleanup effect → premature flush. The dep arrays now contain only `[commitMutationSafely]` (stable). The React integration test (`useGsapPropertyDebounceFlush.test.ts`) renders with a fresh `sdk` wrapper every render, re-renders mid-debounce, and asserts zero premature commits. Good contract pin.

Finding #8 — per-file GSAP write serialization: `CutoverDeps.serialize` is an optional per-key serializer that `dispatchGsapOpAndPersist` uses to wrap its entire read→dispatch→serialize→write cycle. `useGsapScriptCommits.ts` wires it to the existing `serializerRef` (the same one legacy `commitMutation` uses for `gsap-file:${file}`). The interleave test constructs a blocking first write and verifies the second write's start comes strictly after the first's completion. Correct architecture — the serializer is the existing mechanism, the wiring just extends it to the SDK path.

Finding #12 — on-disk undo baseline: `captureOnDiskBefore` reads the pre-edit file content from the server (via the new `readProjectFile` on CutoverDeps), falling back to `serializedBefore` on error. Both `sdkTimingPersist` and `dispatchGsapOpAndPersist` use it. The tests verify the on-disk path and the fallback. `useTimelineEditing.ts` wires `readProjectFile: (path) => readFileContent(projectIdRef.current, path)` for both move and resize. Correct — undo now restores the exact file bytes instead of a normalized SDK re-emit.

Finding #14 — self-write identity registry: `sdkSelfWriteRegistry.ts` is a module-scoped FNV-1a hash registry keyed by file path. `markSelfWrite` records what we wrote; `isSelfWriteEcho` consumes a matching entry (single-use) within the TTL. `shouldReloadOnFileChange` in `useSdkSession.ts` uses identity when content is available, falling back to the time window when it isn't. The handler now attempts to read file content from the payload or via the adapter. This closes the undo-swallowed-by-time-window bug: an undo's reverted bytes aren't registered, so they never match → reload fires. The registry tests cover: echo match, undo miss, per-file isolation, single-use consumption, TTL expiry.

One observation on the self-write path: the handler does an async `readAdapter.read(compPath)` when payload content is unavailable. This introduces a small race — between the read and the decision, another write could land. But the identity check makes this safe: even if stale, the decision is "does this content match something I wrote?" — a stale read just means a missed suppression (reload fires, which is the safe default). No concern.

`fd002ad3` — refactor(core): extract split/collapse helpers

Pulls `buildCollapsedFlatVars` and several split helpers out of the monolithic `splitAnimationsInScript` to satisfy the no-fallow-ignore rule. Pure extraction — no behavioral change. The helpers are well-named and single-purpose.

`508be317` — test(studio): pin dark-launch flag-gate contract

This directly addresses Rames's concern and Via's soft-blocker from the first R2: three tests with `STUDIO_SDK_CUTOVER_ENABLED: false` verifying that `sdkTimingPersist`, `sdkGsapTweenPersist`, and `sdkDeletePersist` all return `false` without calling `writeProjectFile`. Exactly the contract pin both reviewers asked for.

`057e0d0e` — fix(studio): leading flag-gate on `sdkGsapTweenPersist`

Adds `if (!STUDIO_SDK_CUTOVER_ENABLED) return Promise.resolve(false);` at the top of `sdkGsapTweenPersist`, before the `getElement` check. Addresses Via's nit about the pre-gate `getElement` SDK touch on flag-off. Clean.

`f9bdd18f` — fix(core): unroll-preservation regressions — non-for loops + AST index substitution

The final commit tightens the loop-unroll preservation:

• `buildLoopUnrollPreserving` returns `null` (→ blanket overwrite fallback) when `loopIndexVarName` returns null (forEach/for-of/while), preventing preserved siblings from referencing an unbound loop variable.
• `substituteLoopIndex` uses AST-based substitution instead of regex, so string literals containing the index char are untouched.
  Both are covered by the tests added in the earlier commit.

---

Verification against initial R2 findings

Finding	Status
#1466 resize drops live DOM pbs patch	Fixed (commit 1, verified in initial R2)
#1466 missing reloadPreview in resize fallback	Fixed (commit 1, verified in initial R2)
#1498 duplicate removeGsapKeyframe union	Retired (commit 2, verified in initial R2)
#1469 dark-launch flag gate missing	Fixed (commit 3, verified in initial R2) + leading gate added (`057e0d0e`)
Rames: flag-gate regression test missing	Added (`508be317`)
Via: sdkShadowGsapKeyframe.ts:128 squash hazard	STILL OPEN — see below

Outstanding: Via's blocker — `sdkShadowGsapKeyframe.ts:128` cross-queue squash hazard

Via flagged that `packages/studio/src/utils/sdkShadowGsapKeyframe.ts` on `main` constructs `{ type: "removeGsapKeyframe", animationId, keyframeIndex }` at line 128-130. Commit 2 of this PR retires the `keyframeIndex` variant from the `EditOp` union in `types.ts`. When the stack squash-merges onto main, `sdkShadowGsapKeyframe.ts` survives (the branch never had it, so the squash diff doesn't touch it) but the union member it constructs is gone → `tsc` red on main.

This file is NOT in the PR #1539 diff. Via's option 1 (migrate line 128 to the percentage variant in THIS PR) is the cleanest one-line fix. This should be addressed before the stack squashes to main.

setTiming per-tween shift/scale (new in `mutate.ts`)

The new `handleSetTiming` logic in `mutate.ts:444-488` computes per-tween position remapping and duration scaling by the clip's start delta and duration ratio. This replaces the old absolute-write approach (which collapsed stagger and blew durations to the full clip). Two new tests verify: moving a clip shifts both tweens by the delta (preserving the 3s stagger), and resizing x2 scales both positions and durations proportionally. The math is correct: `shifted = remapStart + (animation.position - oldStart) * ratio`, rounded to 3 decimal places, clamped to 0. Duration scaling similarly. Good guards (`typeof animation.position !== "number" → skip`, `oldDuration > 0` for ratio). This is a correctness improvement over the pre-cutover behavior.

---

Verdict

The six new commits correctly address the remaining review findings from the initial R2. The core writer fixes are surgical and well-tested. The studio-side fixes (merge-tween-props, debounce stability, GSAP write serialization, on-disk undo baseline, self-write identity) are architecturally sound. The dark-launch flag-gate tests pin the contract both Rames and Via requested.

One open item remains: Via's sdkShadowGsapKeyframe.ts:128 migration. That file on main still constructs the retired keyframeIndex union member. Until it's migrated to the percentage variant (a one-line change), the stack will tsc-red on squash-merge to main.

With that migration landed (in this PR or a follow-up before squash), this is clear.

LGTM — ready for stamp, pending the sdkShadowGsapKeyframe.ts migration.

— Miga

[james-russo-rames-d-jusso]

R3 verification — fix-PR at SHA f9bdd18f (was 18ea1c32 at R2)

Six new commits since R2 — far more than just the R2 follow-ups. Vance hoisted the entire 13-finding stack-review correctness pass (8 core/sdk + 5 studio) into this fix-PR per the canonical fix-hoist-to-tip pattern, then patched one regression from the #9 unroll fix. R3 treats it as a fresh review on the new content plus verification of the two R2 follow-ups.

R2 finding verification

R2-NEW #5 — dark-launch flag-gate regression test (soft-blocker before flip): ✅ resolved. 508be317 adds sdkCutover.gate.test.ts with STUDIO_SDK_CUTOVER_ENABLED: false at the module level and three independent assertions — sdkTimingPersist, sdkGsapTweenPersist, sdkDeletePersist all return false with a valid session present, with writeProjectFile never called for the timing case. Locks the flag-off contract per chokepoint. The pre-flip blocker is closed.

R2-NEW #6 — early-out nit on sdkGsapTweenPersist: ✅ resolved. 057e0d0e lifts the if (!STUDIO_SDK_CUTOVER_ENABLED) return Promise.resolve(false) to the leading line at sdkCutover.ts:254, before the op.kind === "add" getElement touch. Now matches the other three chokepoints' discipline — flag-off does zero SDK reads at every entry.

R3-NEW — the 13-finding correctness pass (commits 7d7732c8 + bcc0a44a + fd002ad3)

Reviewed each finding's fix shape against the diff. Net read: each fix has a paired test that reproduces the bug shape and asserts the corrected result, several explicitly assert not.toContain for the wrong shape (good — guards against parity-collusion regressions, the failure mode that hid these in the first place).

Worth calling out by name:

• #3 setTiming per-tween shift/scale (mutate.ts:444-470): math is right — remapStart + (animation.position - oldStart) * ratio preserves intra-clip offset, animation.duration * ratio scales per-tween. Three guards added: oldStart !== null, typeof animation.position === "number", animation.duration > 0. The stagger-preservation test at mutate.gsap.test.ts:644-678 asserts both the corrected positions (2→3, 5→6) and rejects the absolute-collapse shape (not.toContain("duration: 5")). Solid.

• #14 self-write identity (sdkSelfWriteRegistry.ts + useSdkSession.ts:52-60): FNV-1a hash + consume-on-match + per-file keying + 2 s TTL. Collision risk is "suppress one real reload within TTL for same file" — negligible. Single-funnel markSelfWrite at persistSdkSerialize:159 covers all four cutover persist paths. Tests cover echo / undo / per-file isolation / consume-on-match / TTL expiry / content-null fallback. Clean design.

• #9 unroll regression fix (commit f9bdd18f): two real regressions caught — non-for loops returned a null indexVar so siblings kept undefined item/row references (ReferenceError at render), and the regex substitution corrupted string literals (".row-i" → ".row-0"). Both fixes are right: buildLoopUnrollPreserving now returns null for non-for loops → caller falls back to blanket overwrite (drops siblings, valid code); substituteLoopIndex switches to AST walk with isIndexBindingPosition guard. Two regression tests at gsapWriter.reviewFixes.test.ts:135-181 pin both bug shapes. Note this is the second regression iteration on the same #9 fix — the original commit's for(let i…)-only fixture missed both — but the new tests close the gap.

R3-NEW concerns

sdkTimingPersist is not wrapped in the per-file serializer (sdkCutover.ts:222-231). dispatchGsapOpAndPersist and the legacy commitMutation path both serialize same-file read-modify-writes through deps.serialize("gsap-file:${path}", run), but timing edits do serialize-before → dispatch → captureOnDiskBefore → write without the wrap. Two rapid clip-drag persists on the same file can interleave: T2's captureOnDiskBefore may read pre-T1 bytes before T1's write completes, capturing the wrong undo before (undo would revert past both edits). coalesceKey collapses the undo entries but doesn't prevent the race. Not new in this round (R2 had the same shape), and the practical blast radius is small (timeline drag is the only caller and is debounced), but flagging now since #8 explicitly motivated the serializer for GSAP writes and timing has the same shape. Worth either threading the serializer through or documenting the timing-specific exemption.

Duplicate 2000ms constant (sdkSelfWriteRegistry.ts:19 SELF_WRITE_TTL_MS vs useSdkSession.ts:33 SELF_WRITE_SUPPRESS_MS). The two values are load-bearing in concert: if the registry TTL outruns the suppress window the withinSuppressWindow fallback can return "reload" while a still-registered echo would say "suppress", and vice versa. Same numeric value today, but a future tune of one would silently de-sync. Suggest exporting one constant from the registry module and importing in the hook. (nit)

CI

player-perf and Perf: parity both failed at f9bdd18f, but inspecting Perf: parity logs (run 27717141578 job 81993095152) shows the failure was the Install ffmpeg apt step hitting "The operation was canceled" at 20:40:21 mid-Get:50 — an infra/runner timeout, not a code regression. The player-perf summary check failed as a consequence of perf-shards being cancelled (not failed). A rerun should clear both.

Stamp recommendation

Approve once CI is green on rerun. The R2 follow-ups closed cleanly, the 13-finding correctness pass is rigorous (test design pins bug shapes, not parity), and the regression on #9 was caught and patched in the same round. The sdkTimingPersist-serializer asymmetry is the only meaningful open thread and it's flagged as a follow-up, not a blocker for this PR (it's pre-existing).

Reviewed at f9bdd18f.

— Rames D Jusso

[miguel-heygen]

Reviewed — 6 new commits covering GSAP write-path correctness, studio debounce/serialization, dark-launch flag-gate tests. Miga's thorough re-review confirms all changes. The sdkShadowGsapKeyframe.ts:128 blocker is on main, not this PR. LGTM.

[vanceingalls]

R3 verification at HEAD f9bdd18f.

Six commits since 18ea1c32; the parlour is considerably tidier than I left it.

R2 closure

Cross-queue squash hazard (R2 blocker) — CLOSED, with a correction to my own reasoning. compare/main...sdk-cutover-review-fixes reports packages/studio/src/utils/sdkShadowGsapKeyframe.ts as status="removed", deletions=257 — the stack's contribution to main includes the deletion. Branch contents fetch returns 404; on main the file remains at sha 38a63316. My R2 inspected the branch BASE (sdk-ws3-unroll-dynamic) rather than tip-vs-main, and missed that the deletion was introduced higher in the stack. Post-merge: file gone, no caller to a retired union member, no tsc red. Modify/delete conflict resolves as delete. Fully dispatched.

Flag-off regression test (Rames (a), my soft-block) — CLOSED. 508be317 adds sdkCutover.gate.test.ts mocking STUDIO_SDK_CUTOVER_ENABLED: false and pinning sdkTimingPersist / sdkGsapTweenPersist / sdkDeletePersist each to false with a valid session, asserting writeProjectFile is never called. Dark-launch contract now load-bearing in CI.

Leading flag-gate on sdkGsapTweenPersist (Rames (b), my nit) — CLOSED. 057e0d0e inserts if (!STUDIO_SDK_CUTOVER_ENABLED) return Promise.resolve(false); at sdkCutover.ts:252, AHEAD of the op.kind === "add" && !sdkSession.getElement(...) check. Flag-off is now a clean no-op at every entry.

New substantive work

7d7732c8 — 8 GSAP write-path correctness fixes. #2 closest-keyframe, #3 per-tween delta+ratio, #4 motionPath appendRight, #5 forward-pass split baseline, #9 unroll preservation, #10 cubic motionPath, #11 negative-coord recovery, #15 no-bang. Each paired with a test in gsapWriter.reviewFixes.test.ts / mutate.gsap.test.ts asserting real-world-correct outputs — the commit correctly notes BOTH writers had been identically wrong so parity stayed green over the bug. Substantive.

bcc0a44a — 5 studio cutover fixes. #1 tween-prop merge, #7 debounce-flush via deps-ref, #8 per-file serialized GSAP write through the legacy keyed serializer, #12 on-disk bytes as undo baseline, #14 self-write-by-content-hash discrimination. Each paired with a red-before-fix test. #8 closes a real interleave window by feeding SDK gsap-writes through the same per-file lock as commitMutation.

fd002ad3 — fallow refactor. Extracts buildSpanningSplit / applyTweenSplit / buildCollapsedFlatVars; removes a previously-added fallow-ignore. Honours "never ignore — fix"; behaviour-only.

f9bdd18f — unroll-preservation regressions on 7d7732c8 #9. Two REAL regressions, self-caught: (i) loopIndexVarName returns null on forEach/for-of/for-in/while; original substitution silently no-op'd, leaving preserved siblings with an unbound item — now returns null at the caller, falling back to the blanket overwrite (valid code); (ii) the \bvar\b regex over raw source corrupted ".row-i" → ".row-0" — now AST-walked via acornWalk.ancestor with an isIndexBindingPosition guard for member-property / object-key positions. Two regression tests pin both.

Chokepoint audit

All four sdkCutover.ts chokepoints gate leading: sdkTimingPersist (~L174), sdkGsapTweenPersist (L252, new), dispatchGsapOpAndPersist (~L236), sdkDeletePersist (~L340). bcc0a44a paths feed THROUGH these, not around.

Pre-post freshness

No new reviewer reviews since R2 (19:28:04Z); only the author's reply at 20:09:42Z. No race.

Verdict

CLEAR. Every R2 blocker, soft-ask, and nit closed with file:line proof; new work introduces no fresh band-aids and one self-caught regression closed in the same series. Stack is genuinely dark-launchable; the union retire ships without a cross-queue post-mortem.

Review by Via

[miga-heygen]

R3 + CF2 Verification — All 16 Findings

Reviewed the tip at 4f1eb960 (16 commits on the sdk-cutover-review-fixes branch). Verified each finding against the actual diff.

R3 Findings

#1/#2/#9 — Unroll AST safety ✅
The unroll now refuses (no-ops, returning the script verbatim) on unsafe patterns rather than corrupting or dropping code:

• forEach/for-of/for-in/while loops with siblings → loopIndexVarName returns null → unrollSiblingStrategy returns "refuse".
• Object shorthand { i } → isShorthandIndexUse detects it → refuse.
• Shadowed index (nested for (let i …)) → rebindsIndex catches it → refuse.
• Safe for loops with items[i] siblings still unroll (regression guard test present).
• Index substitution is now AST-based (substituteLoopIndex via acornWalk.ancestor) — string literals like "row-i" are no longer corrupted. isIndexBindingPosition correctly skips non-computed member props and object keys.
• Empty element list → early return at the top of unrollDynamicAnimations + validateOp rejects with E_INVALID_ARGS.
• Comprehensive test coverage: 6 scenarios in gsapWriter.reviewFixes.test.ts + mutate.test.ts.

#3 — stripGsapForId re-parse ✅
stripGsapForId in htmlParser.ts now re-parses after each removal in a for(;;) loop, finding the first still-matching animation in each fresh parse. Includes an infinite-loop guard (updated === current → break). Test: "strips ALL gsap tweens for the removed element, not just the first" covers two tweens on one element.

#4 — history:false persist decouple ✅
In session.ts openComposition, the persist setup is now OUTSIDE the history !== false block. history:false only disables the undo stack; auto-save runs independently. Verified by the smoke test: "still persists when history:false (undo opt-out must not disable auto-save)".

#5/#11 — Single-dispatch undo invertibility ✅
_dispatch in session.ts now reverses the inverse patch list for single-op dispatches: [...inverse].reverse(). Comment correctly notes parity with batch(). Test in session.test.ts: nested removeElement([child, parent]) undo restores both elements — the child's inverse (add child) now fires AFTER the parent's (add parent), so the parent exists when the child attaches.

#6/#13a — validateOp target resolution ✅
New gsapAnimationMissing helper checks that the animationId actually resolves via the parser. Applied to: removeGsapTween, removeAllKeyframes, convertToKeyframes, splitIntoPropertyGroups, setArcPath, removeArcPath, updateArcSegment, unrollDynamicAnimations, materializeKeyframes. Returns E_TARGET_NOT_FOUND with a hint about positional IDs being stale. Test coverage for removeGsapTween unresolved id.

#7 — easeEach routing ✅
updateAnimationInScript now calls keyframesObjectNode(call.varsArg) — if the tween has a keyframes property whose value is an ObjectExpression, it writes easeEach on that node instead of a top-level ease. Test: "writes easeEach (per-keyframe), not a no-op top-level ease".

#8 — convertToKeyframes preserves delay ✅
buildKeyframesVarsCode now takes varsNode and source params, uses preservedVarsEntries(varsNode, source) to carry over every non-editable key (delay, callbacks, stagger, yoyo, etc.) verbatim from source. The old path rebuilt from animation.extras which had no delay field. Test: "preserves delay on the converted vars object" — verifies delay: 0.3 survives.

#10 — materializeKeyframes empty-array guard ✅
materializeKeyframesFromScript now has an early if (keyframes.length === 0) return script; guard. validateOp rejects with E_INVALID_ARGS. Two tests: writer no-op + validateOp rejection.

#12 — addLabel dedup ✅
addLabelToScript now searches for an existing label with findLabelStatements (extracted from removeLabelFromScript). If found, it overwrites the position argument instead of appending a duplicate. The corpus test was updated from "two addLabel calls (no dedup)" to "MOVES it instead of duplicating (dedup contract)". The removeLabel test now uses a hand-authored dup to verify it still cleans up all matches.

#13b — Quote-insensitive selector comparison ✅
handleDeleteAllForSelector normalizes quotes: selector.replace(/'/g, '"') on both sides of the comparison. Test: "removes a tween authored with double quotes when given a single-quoted selector".

#14 — Node = any type alias ✅
type Node = any declared at the top of gsapWriterAcorn.ts. Every bare : any annotation in the touched functions replaced with : Node. Comment: "acorn ESTree nodes are structurally untyped here; mirror gsapParserAcorn.ts / gsapInline.ts rather than re-deriving the full ESTree union." This is a type-only change (zero runtime effect), which is the correct scope.

CF2 Findings

#15 — handleSetTiming DOM id matching ✅
The tween-matching loop in handleSetTiming now checks BOTH matchHfId and matchDomId (el.getAttribute("id")). selectorMatchesId is called against both. Test: "#domId-targeted tween shifts when the clip moves" — verifies position remap from 2→5.

#16 — handleSetTiming data-duration support ✅

• oldDuration now prefers data-duration over data-end − data-start.
• Write-back respects whichever attribute the clip uses: data-duration clips update data-duration on resize, data-end clips update data-end.
• timingPath now accepts "duration" as a field, and keyToPath regex updated to include duration.
• Test: "a data-duration clip updates data-duration and scales its tween".

Additional Fixes Verified

• setTiming GSAP per-tween shift/scale: positions are remapped using the start DELTA and duration RATIO, not absolute values. Two tests (move +1 preserves stagger; resize x2 scales positions) confirm no stagger collapse.
• No-bang (!): every ! non-null assertion removed from touched files, replaced with guards/early-returns/narrowing.
• Duplicate removeGsapKeyframe variant retired: the keyframeIndex discriminant removed from EditOp union; dispatcher simplified to percentage-only path.
• applyDraft element caching: _draftEl typed as HTMLElement | null, cached via isConnected check, re-queried only on id change or detach. Test verifies single querySelector call across 3 applyDraft invocations.
• Fallow refactoring: buildSpanningSplit, applyTweenSplit, buildCollapsedFlatVars extracted to satisfy complexity threshold; fallow-ignore removed.
• Dark-launch flag-gate contract: all 4 persist chokepoints (sdkTimingPersist, sdkGsapTweenPersist/dispatchGsapOpAndPersist, sdkDeletePersist) now check STUDIO_SDK_CUTOVER_ENABLED at the top. sdkCutover.gate.test.ts pins this contract with the flag mocked false.
• Move/resize fallback parity: shiftGsapPositions + scaleGsapPositions + reloadPreview() restored on the no-session and timing-returned-unhandled fallback paths; coalesceKey plumbed through persistTimelineEdit for undo parity.
• Self-write identity registry: sdkSelfWriteRegistry.ts + shouldReloadOnFileChange discriminate cutover echo from undo write by content hash (FNV-1a), not just timestamp window. Consume-on-match semantics prevent stale suppression.
• Forward-baseline split: computeForwardBaselines pre-pass computes inherited props in timeline order before the reverse write loop, fixing wrong midpoint on spanning tweens.
• Debounce stability: sdkRef ref stabilizes the flush callback identity so parent re-renders don't trigger spurious unmount-flush commits.
• Serialized GSAP writes: deps.serialize routes through the legacy gsap-file:${file} per-key serializer.
• On-disk undo baseline: captureOnDiskBefore reads exact disk bytes via deps.readProjectFile, falling back to serialize().
• Resize live DOM patch: liveAttrs now includes conditional data-playback-start/data-media-start push.

Summary

LGTM — all R3/CF2 findings verified. Every claim in Vance's response matches the actual code. The fixes are well-structured, each with targeted tests that reproduce the original bug scenario (verified-red-before-green pattern). The dark-launch gate contract is now properly pinned. Ready for stamp and bottom-up merge.

— Miga

[vanceingalls]

R4 verification at HEAD 4f1eb9606 — 16 findings + 2 CF2 closure pass.

Addendum: Miga's R3+CF2 verification landed at 22:52Z, ~1h ahead of mine. My readings agree across every line; this is an independent re-walk with source citations.

Six commits, fourteen files, every claimed closure verified against the AST itself.

Per-finding closure

#	Proof at HEAD	Verdict
1/2/9 unroll refusal	gsapWriterAcorn.ts L2028-2095: REFUSE_UNROLL, unrollSiblingStrategy, hasUnsafeLoopIndexUse, rebindsIndex, isShorthandIndexUse; L2186 short-circuit. loopIndexVarName returns null on ForOf/ForIn/While/forEach → forced refuse. Tests L168-212: forEach-with-sibling, { i } shorthand, shadowed inner for, safe-for regression — each asserted unchanged AND new Function()-parseable	VERIFIED
3 stripGsapForId re-parse	htmlParser.ts L685-703: for(;;) re-parse, first-still-matching removal, infinite-loop guard	VERIFIED
4 history:false persist decouple	session.ts L511-526: persist block outside history !== false. Test smoke.test.ts L230	VERIFIED
5/11 single-dispatch undo reverse	session.ts L300-306: [...inverse].reverse() with parity comment. Test session.test.ts L302	VERIFIED
6/13a validateOp resolves id	mutate.ts L1097-1117 (gsapAnimationMissing) wired into every id-bearing GSAP case L1203-1226; validateArcSegment L1121-1138 checks enabled + range	VERIFIED
7 easeEach routing	gsapWriterAcorn.ts L327-333: keyframesObjectNode detect, upsert easeEach not top-level ease	VERIFIED
8 delay preservation	gsapWriterAcorn.ts L1080-1085 via preservedVarsEntries L546; NON_EDITABLE_VAR_KEYS L500 includes delay. Test L310	VERIFIED
10 materializeKeyframes []	gsapWriterAcorn.ts L1163 early-return; mutate.ts L1226 E_INVALID_ARGS	VERIFIED
12 addLabel move-not-dup	gsapWriterAcorn.ts L1416-1440: findLabelStatements-first, overwrites posArg. Corpus test flipped from "no dedup" to "MOVES it"	VERIFIED
13a partial setArcPath <2-wp	Deliberate writer-no-op + studio fallback. Coupling argument is sound; stale-id + segment-range ARE rejected	ACCEPTED
13b deleteAllForSelector quotes	mutate.ts L945-948: .replace(/'/g, '"') both sides. Test L964	VERIFIED
14 Node = any alias	gsapWriterAcorn.ts L33; every bare : any AST annotation now : Node. Type-only	VERIFIED
15 CF2 DOM id match	mutate.ts L471-490: matchHfId OR matchDomId via selectorMatchesId. Test "#domId tween shifts when clip moves"	VERIFIED
16 CF2 data-duration precedence	mutate.ts L398-413 prefers data-duration; L432-453 writes back to whichever attribute the clip uses; patches.ts L62 gains "duration" field. Test "data-duration clip updates data-duration and scales its tween"	VERIFIED

Branch-vs-main surprise check

compare/main...sdk-cutover-review-fixes — six removed entries, all packages/studio/src/utils/sdkShadow* (the R3 cross-queue squash hazard closure). sdkShadowGsapKeyframe.ts confirmed removed. No surprise deletions.

Pre-existing-state spot-check

At f9bdd18f, none of REFUSE_UNROLL / hasUnsafeLoopIndexUse / rebindsIndex / isShorthandIndexUse / unrollSiblingStrategy existed, and [...inverse].reverse() was absent from session.ts. Genuine closures — no phantoms.

Six-commit auxiliary R1 retakes

applyDraft element cache (5dcd168a, #1490a) and empty-element-list unroll guard (5b3db8de, #1501b) — correctly scoped, independently tested, verified in passing.

CI

15 required-pass / 0 fail / 8 regression-shards in flight (non-blocking); no failures at tip.

Recommendation: CLEAR

The cabinet is in order. Bottom-up merge has my concurrence.

Review by Via

[miguel-heygen]

Re-approved at 6c2d668 — cascadeRemoveAnimations fix + aliased-multi-target undo test added. All reviewers clear. LGTM.

[jrusso1020]

Re-stamping at 6c2d66892 — Rames D Jusso's R4 verification confirms the two new commits (cascadeRemoveAnimations re-parse loop + aliased-multi-target undo test) close the remaining caveats from the round-3 pass. All 16 round-3 + 2 CF2 findings verified by Miga/Via/Rames D Jusso. CI yellow (regression shards still running, non-blocking). Cleared for bottom-up merge from my side.

The base branch was changed.