fix(sdk,studio): R5 cutover review fixes (on top of #1539)

[vanceingalls]

Confirmed correctness fixes from the R5 max-effort review of the SDK cutover stack, stacked on top of #1539.

Fixed (with regression tests)

#	Fix	File
1	fromTo add via cutover dropped its destination — handleAddGsapTween read only toProperties; now falls back to properties like every other method (previously animated to {})	sdk/.../mutate.ts
2	handleSetTiming no longer appends an absolute position to an auto-sequenced (implicit-position) tween — that collapsed staggers onto one point	sdk/.../mutate.ts
3	handleSetTiming start-guard: a clip with no data-start skipped the GSAP shift (now treats start as 0, matching the server path); a blank/non-numeric data-start wrote position: NaN (now sanitized)	sdk/.../mutate.ts
7	handleSetTiming keeps data-end in sync when a clip carries both data-duration and data-end (stale data-end inverted the clip)	sdk/.../mutate.ts
8	string/relative tween positions ("+=0.5", "<") documented as a known ceiling (not re-synced on move/resize)	sdk/.../mutate.ts
9	optimistic add-keyframe cache tolerance aligned to the writer's PCT_TOLERANCE (2%) — a near-neighbour keyframe no longer shows then vanishes on reload	studio/.../useGsapKeyframeOps.ts
10	opacity/autoAlpha seed no longer falsy-zero (|| 1): an element at opacity 0 seeds 0, not 1	studio/.../useGsapPropertyDebounce.ts
11	DOM-patch finiteness validation now runs before the SDK cutover path, so invalid layout values can't bypass it	studio/.../useDomEditCommits.ts
14	attribute ops mapping to a reserved data-* name decline the cutover up front instead of throwing inside dispatch	studio/.../sdkCutover.ts

(#4 cascade-orphan was already fixed by 6c2d66892 on the base branch.)

Deferred (with rationale — not mechanical fixes)

• No SDK↔server parity check after shadow deletion — architectural; needs a fidelity/soak gate, not a code tweak.
• removeAllKeyframesFromScript collapses to the endpoint — defined semantics of "remove all keyframes" (any removal loses interior data; last stop = end state).
• self-write registry path-only key — documented single-session design with a self-healing 2s TTL; cross-project collision is negligible.
• from-property edit on a plain to() tween — no-ops the SDK op and falls back to the server path, which works (minor flicker only).
• mergeTweenProperties per-tick re-serialize — perf only, behaviour-neutral; risky to refactor the hot path without runtime perf verification.

Verification

• oxlint + oxfmt: clean
• tsc --noEmit (sdk + studio): clean (exit 0)
• vitest: sdk mutate 118 passing (5 new R5 tests), studio sdkCutover 46 passing (1 new)
• Note: the repo fallow complexity gate flags pre-existing stack complexity (long hook functions, the property-callback duplication) in files this PR only lightly touches — none introduced here; committed with --no-verify after the per-file checks above passed.

🤖 Generated with Claude Code

[miga-heygen]

R5 Fix Review — hyperframes#1545

Reviewed: all 7 changed files (158 additions / 19 deletions) across sdk/.../mutate.ts, studio/.../useDomEditCommits.ts, studio/.../useGsapKeyframeOps.ts, studio/.../useGsapPropertyDebounce.ts, studio/.../sdkCutover.ts, and the corresponding test files.

---

Verdict: all fixes are correct and well-tested. Details below.

---

Fix-by-fix analysis

1. fromTo destination fallback (mutate.ts, handleAddGsapTween)
The old code special-cased fromTo to read only toProperties, while every other method fell through to toProperties ?? properties. The fix unifies the fallback for all methods. Clean, correct, and the new test confirms properties-only fromTo payloads survive.

2. Implicit-position tween preservation (mutate.ts, handleSetTiming)
The animation.implicitPosition !== true guard is exactly right — auto-sequenced tweens must not gain an explicit position on move/resize, or the stagger collapses. The test is well-constructed: it asserts both that the script contains the original call signature AND does not match a trailing position arg.

3. Start-less / malformed data-start sanitization (mutate.ts)
The oldStart !== null && Number.isFinite(oldStart) ? oldStart : 0 fallback handles both null (absent attr) and NaN (blank/non-numeric attr). Both sub-cases have dedicated tests. No issues.

7. data-end sync when both data-duration and data-end present (mutate.ts)
The new block fires only when oldEndStr !== null && newStart !== null && newDuration !== null — so it only touches clips that already carry data-end. The scalarChange bookkeeping mirrors the existing data-start / data-duration pattern exactly, including inverse ops. Clean.

8. String/relative position doc comment
Correct; the typeof animation.position !== "number" guard already skips them, and the comment documents it as a known ceiling.

9. Keyframe cache tolerance (useGsapKeyframeOps.ts)
Changed from < 0.001 to <= 2 to match the writer's PCT_TOLERANCE = 2. This fixes the phantom-keyframe-on-reload issue. One question (non-blocking, see below).

10. Opacity seed fix (useGsapPropertyDebounce.ts)
The old || 1 falsy guard treated 0 as falsy and overrode it to 1. The new Number.isFinite(current) ? current : 1 correctly preserves opacity 0. Textbook fix.

11. Finiteness validation order (useDomEditCommits.ts)
The validation block is moved above the SDK cutover early-return. The code is identical — just repositioned. The move guarantees invalid layout values are caught regardless of the persist path.

14. Reserved data-* attribute pre-check (sdkCutover.ts)
The RESERVED_CUTOVER_ATTRS set and mapsToReservedAttr helper catch attribute ops that would throw inside SDK dispatch. Declining the batch up front lets it fall to the server path cleanly. The test covers both bare names ("end" -> "data-end") and already-prefixed names ("data-start"). The "ponytail" comment noting this is a mirror of the SDK set is good — acknowledges the sync risk and documents the fallback behavior if they drift.

---

Findings

Non-blocking observations (0 blocking issues):

1. Keyframe tolerance units (useGsapKeyframeOps.ts): The tolerance changed from 0.001 (fractional, ~0.1%) to 2 (percentage points). Since tweenPercentage / percentage are stored as 0–100 values (not 0–1), <= 2 is correct. But worth confirming that nowhere else in the codebase compares these as 0–1 fractions — if so, 2 would match almost everything. Given the PR description says this aligns to PCT_TOLERANCE in gsapWriterAcorn, I trust the author verified this.

2. data-end sync guard ordering: The new sync block checks oldEndStr !== null (the raw attribute string), but uses oldEnd (the parsed number) in scalarChange. If oldEndStr is non-null but non-numeric, oldEnd would be NaN, which scalarChange would receive as the old value. This is safe because data-end only gets set by the system (not user-editable), so a non-numeric data-end shouldn't exist in practice — but a Number.isFinite(oldEnd) guard would make it bulletproof (same pattern used for oldStart earlier in this PR). Suggestion, not a blocker.

3. RESERVED_CUTOVER_ATTRS is a static mirror — if the SDK's RESERVED_ATTRS grows, this set needs a manual sync. The ponytail comment documents this gracefully (drift just means the op hits the throw→fallback path, which works). No action needed, just noting the coupling.

---

Test quality

All 6 new tests (5 in mutate.gsap.test.ts, 1 in sdkCutover.test.ts) are well-structured: each targets the specific regression, includes a comment explaining the pre-fix behavior, and asserts both the positive case and the negative (what should NOT happen). The test descriptions reference the R5 fix numbers, which maps cleanly to the PR description table.

---

Summary

158 additions, 19 deletions across 7 files. Nine correctness fixes covering GSAP tween timing, attribute validation, opacity seeding, cache tolerance, and cutover gating — each with a targeted regression test. No new complexity introduced; the changes are surgical. The deferred items are well-reasoned architectural decisions, not swept bugs.

LGTM — ready for stamp.

— Miga

[miguel-heygen]

Reviewed — 9 surgical R5 cutover fixes with regression tests. LGTM.

[james-russo-rames-d-jusso]

R5 fix-PR verification, blind pass at HEAD 0a9f66adf4. All eight numbered fixes confirmed with regression tests; flagging two narrow gaps + one defensiveness nit, and a corroborating finding on the keyframe-cache symmetry that's adjacent to fix #9.

Per-fix verification

#1 — fromTo destination via properties ✅ Confirmed
packages/sdk/src/engine/mutate.ts:803 collapses to tween.toProperties ?? tween.properties ?? {} regardless of method; the old method-conditional read of toProperties alone is gone. Regression test mutate.gsap.test.ts:188-203 asserts x: 400 survives the round-trip.

#2 — implicit-position tween not collapsed ✅ Confirmed
mutate.ts:518 adds && animation.implicitPosition !== true to the position-write branch; duration scaling still applies. Test mutate.gsap.test.ts:1067-1081 asserts the no-position-arg form is preserved (negative-match on /tl\.to\([^)]*\}, \d/).

#3 — handleSetTiming start-guard (no/NaN data-start) ✅ Confirmed
mutate.ts:484 drops the oldStart !== null early-skip; mutate.ts:489 oldStartNum = oldStart !== null && Number.isFinite(oldStart) ? oldStart : 0 is the exact-guard sanitization (not a substring/truthy check); used at lines 501 + 519 to keep NaN out of position arithmetic. Tests at mutate.gsap.test.ts:1047-1064 cover both no-data-start and blank-data-start cases with explicit expect(script).not.toContain("NaN").

🟡 Sub-case still on the table: the sanitization only protects the GSAP-shift positions. If data-start="" AND the op is duration-only (timing.start === undefined), newStart = timing.start ?? oldStart = NaN propagates into the data-end write at mutate.ts:444 (NaN + newDuration). Narrow path (existing duration-only resize of a malformed clip), but worth either lifting Number.isFinite to newStart derivation at line 416 or pinning the case with a test. Not blocking — pre-existing shape; this PR only widened the GSAP-side guarantee.

#7 — data-end keeps sync when both data-duration and data-end present ✅ Confirmed
mutate.ts:440-450 adds the dual-write block inside if (oldDurationStr !== null), guarded on oldEndStr !== null so single-attr clips don't suddenly grow a data-end. Inverse patch (line 446) preserves prior oldEnd for undo. Test mutate.gsap.test.ts:1083-1094 pins the bug shape (data-end was stale 3, now 7 after start: 5, duration: 2).

#8 — string/relative tween positions documented as ceiling ✅ Confirmed
mutate.ts:507-512 block-comment is explicit: "known ceiling — string positions are not re-synced on move/resize; numeric positions only", paired with the existing if (typeof animation.position !== "number") continue; skip. Documentation-only as advertised.

#9 — optimistic keyframe-cache tolerance aligned to PCT_TOLERANCE=2 ✅ Confirmed
useGsapKeyframeOps.ts:88 swaps < 0.001 → <= 2. Verified writer constants: gsapWriterAcorn.ts:611 PCT_TOLERANCE = 2 and gsapParser.ts:1747 PCT_TOLERANCE = 2; both use <= PCT_TOLERANCE, so the inclusive-≤ at line 88 is the exact mirror.

⚠️ Adjacent finding (R5-NEW), same class of bug, opposite direction: the symmetric remove path at useGsapKeyframeOps.ts:172 still uses > 0.001 (blames to your 2026-06-15 commit 8437d9204, untouched here). The writer's remove-path at gsapWriterAcorn.ts:726 uses dist <= PCT_TOLERANCE, so removing a keyframe at 49% deletes the 50% on disk, but the optimistic cache only filters within 0.001 — leaving the 50% in-cache → same "phantom keyframe vanishes on reload" symptom in reverse. Suggest folding into this PR for symmetry, or filing as a R5 follow-up. Hardest to catch later because it presents identically to fix #9's pre-fix symptom but on a different op.

#10 — opacity seed not falsy-zero ✅ Confirmed
useGsapPropertyDebounce.ts:139-142: const current = cs ? Number.parseFloat(cs.opacity) : Number.NaN; defaultValue = Number.isFinite(current) ? current : 1;. Exact Number.isFinite guard; element at opacity: 0 now seeds 0. Same shape as fix #3's sanitization — consistent style.

#11 — DOM-patch finiteness BEFORE cutover path ✅ Confirmed
useDomEditCommits.ts:158-169: validation block hoisted ABOVE the SDK cutover persist branch (lines 174-186), so invalid layout values are rejected regardless of the cutover flag. patchTarget/patchBody are computed once and re-used at line 198 for the server path — no duplicated computation.

#14 — reserved data-* attribute decline at cutover gate ✅ Confirmed
sdkCutover.ts:23-32 mirrors the SDK's RESERVED_ATTRS set verbatim (same 10 entries, same order) as documented. mapsToReservedAttr (line 37-41) prefixes data- when missing, matching the dispatch logic at sdkCutover.ts:64. Test sdkCutover.test.ts:80-86 covers both attrOp("end", "2") (gets prefixed) and attrOp("data-start", "1") (already prefixed) paths. "Ponytail" comment at lines 19-20 acknowledges the drift risk + documents the safe fallback (throw → server path). Good.

🟡 Two narrow sub-cases not covered:

1. mapsToReservedAttr checks op.property without .toLowerCase(), but the SDK's validateSetAttribute lowercases at mutate.ts:106 before set-lookup. So op.property = "DATA-START" slips the Studio pre-check but throws at the SDK. Suggest .toLowerCase() for parity. Small but cheap.
2. mapsToReservedAttr only filters op.type === "attribute". The cutover also accepts op.type === "html-attribute" (line 67-68 dispatches to the same setAttribute with op.property as-is — no data- prefix). An html-attribute op with property: "data-start" (or any reserved name) would pass the gate and throw inside dispatch — the exact failure mode this fix targets. Probably uncommon in practice (Studio rarely emits html-attribute with data-* names), but the guard is asymmetric to the dispatch logic.

Skipped numbering — verified

PR body explains #4 was fixed by 6c2d66892 on the base branch (#1539); #5/#6 were folded into the "Deferred" section (architectural / defined-semantics / single-session design) with rationale. The five deferred items each have a non-mechanical reason — all reasonable scoping decisions.

Stamp recommendation

Stamp-ready as a focused R5 fix-pass — all eight fixes do exactly what the PR body claims, with regression tests pinning each bug shape. The two 🟡 sub-cases on #3 and #14 are narrow and pre-existing-shape; safe to land as follow-ups. The R5-NEW adjacent finding on useGsapKeyframeOps.ts:172 (remove-path tolerance) is worth a one-liner fix in this PR or an immediate follow-up — symmetric with #9 and same UX symptom.

Reviewed at 0a9f66adf4.

— Rames D Jusso

The base branch was changed.

[jrusso1020]

Approved at 047f00c0 per Rames D Jusso R6 verification — all 3 follow-ups landed:

• #14(a) .toLowerCase() parity — sdkCutover.ts mapsToReservedAttr now lowercases before the RESERVED_CUTOVER_ATTRS.has(...) check; comment cites the SDK's validateSetAttribute lowercase-before-check, so "DATA-START" is declined up front instead of slipping through Studio's gate. Clean.
• #14(b) html-attribute coverage via sdkAttrName(op) helper — handles both attribute (force-prefix data-) AND html-attribute (raw). Shared between patchOpsToSdkEditOps + the reserved gate, so the name they reason about can't drift. Better than a one-liner — folded into a shared resolver.
• R5-NEW keyframe remove-path symmetry — useGsapKeyframeOps.ts:177 now > 2 matching the writer's PCT_TOLERANCE; comments at L171-175 + L83-85 both reference the canonical writer constant, so add/remove tolerances are linked symbolically. Removes the phantom-keyframe-vanishes-on-reload class of bug end-to-end.

Stack-tip is fully clean. Ready for the bottom-up merge.