fix(studio): batch 3D depth commit so perspective + z don't race

[miguel-heygen]

3D transform + keyframe editing correctness (5/8)

The editing-correctness cluster for 3D transforms, perspective, and keyframe commits.

What's here

• batch the 3D depth commit so transformPerspective + z don't race
• "Enable keyframes" on a set at/before its start no longer drops the 0% keyframe
• 3D Transform panel usable on any element (first edit creates the gsap.set); register a no-op for the internal _auto keyframe marker
• track the selection overlay + motion path through perspective (z) transforms — fold the m44 foreshortening into the drag offset and path geometry
• keyframe commit routing: group-aware tween selection, playhead-past-tween extend, and update-keyframe merges instead of overwriting (editing one property no longer drops z/transformPerspective)

Title is auto-generated from the first commit ("batch 3D depth commit") and undersells the PR — it covers the whole 3D/keyframe/perspective cluster.

Stacked on the inline-timeline PR (shared gsapWriterAcorn edits).

[miguel-heygen]

• chore(studio): add anonymous usage events #1765
• fix(studio): strip a group's GSAP when ungrouping #1764
• fix(studio): remove keyframe dragging from the timeline #1763
• fix(studio): batch 3D depth commit so perspective + z don't race #1762 👈 (View in Graphite)
• feat(studio): expand sub-composition groups + children in the timeline #1761
• feat(gsap): read timelines authored inline (acorn read path) #1760
• feat(studio): element groups — studio UI #1759
• feat(studio): element groups — source mutations #1758
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Reviewed at e6bd9e44. Race-fix classification: PREEMPT — onDepthCommit now routes z + transformPerspective (+ optional DEPTH_POSE_*) through a single onCommitAnimatedProperties(...) call → one update-keyframe/add-keyframe mutation carrying both props. Same shape commitPose/recenter already use. Crucially, the writer (updateKeyframeInScript) now also MERGES via upsertProp instead of overwriting — so even a stray non-batched path can't drop sibling props at the keyframe level. Two-layer PREEMPT: caller batches, writer merges. That's a durable fix, not probabilistic.

Cross-cohort completeness: ✅ — the only co-property races on this surface were rotation-axes (already batched in commitPose) and depth (this PR). Per-field Transform3dField only commits its own prop, which is correct (no co-set). The depth-pose tilt addition piggy-backs on the same batch, so even the "flat element gets first depth" path is atomic.

Some smaller concerns and questions below — none blocking.

---

🟡 Concerns

• packages/studio/src/components/editor/propertyPanel3dTransform.tsx:188-194 — the else fallback when onCommitAnimatedProperties is unset is literally the racing per-axis loop the PR exists to prevent (same in commitPose 109-114 and recenter 127-132). In current code onCommitAnimatedProperties is always wired from StudioRightPanel.tsx:275, so the branch is dead — but it's a footgun: any future refactor that drops the prop silently regresses to the same bug, and the writer-layer merge only protects keyframe updates (not set paths or fresh-tween creation). Consider making onCommitAnimatedProperties required at the type level (drop the ?) and removing the fallback in all three sites, or at least dev-asserting on the fallback branch. Same nit applies wherever else this pair is destructured.

• packages/studio/src/components/editor/propertyPanel3dTransform.tsx:15-21 — naturalDepthPerspective returns 0 when both compHeight and el.offsetHeight are 0 (degenerate inline / empty wrapper element). The onDepthCommit then drops the if (... && depthPerspective > 0) lens-write, so z lands without a perspective lens and depth is invisible. Old code unconditionally fell back to 800; new code is explicit-no-magic-number. I think this is the right trade, but worth confirming you'd rather "depth scroll silently does nothing on a 0-height element" than "depth scrolls reads on an arbitrary 800 lens." A clamp like Math.max(360, …) would split the difference. Up to you.

🟡 Questions

• packages/studio/src/components/editor/Transform3DCube.tsx:265-285 — the scroll-handler's lens * 0.85 clamp keeps m44 positive on wheel input, but the Z numeric field (propertyPanel3dTransform.tsx:374-383) is unclamped scrub + free-form parse. Type z = 2000 under a 1080 lens and m44 goes negative; transformWDivisor falls back to 1 (treats as 2D), and the overlay / motion-path tracking silently desyncs from the projected element. Is the intent that the cube's clamp is the only enforcement, and direct numeric entry is "expert mode"? Or should the clamp move down into the commit layer / runtime?

• packages/core/src/runtime/init.ts:100-116 — ensureAutoMarkerNoop runs once at initSandboxRuntimeModular time. In the preview path, gsap is injected via <script> and bootstrap fires on DOMContentLoaded, so window.gsap is usually present. But the CDN-fallback path (GSAP_CDN_FALLBACK_SCRIPT at routes/preview.ts:154) async-loads gsap on a network error, AFTER bootstrap has run. In that case g?.registerPlugin is undefined, the function bails silently, and _auto warnings come back (the symptom you're treating: per-frame log noise that destabilizes the overlay). Consider retrying on window.addEventListener('hf-gsap-ready', ...) or polling for a tick. Not a blocker — fallback is a degraded path — but it's the exact case the warning fires from.

• packages/studio/src/hooks/useAnimatedPropertyCommit.ts:242-273 — on the extend-tween path, the NEW keyframe is pushed with raw properties ({ ...runtimeProps, ...props }) at the playhead-relative percentage, and the existing keyframes are remapped with { ...kf.properties } + selective backfill. That looks correct, but I want to double-check intent: should the new appended keyframe also pick up _auto: 1 if it lands at the new tween's 0% (i.e. ct < ts, so user dragged the playhead BEFORE the tween)? Right now an extend-backward edit at ct = 0 creates an explicit-value 0% keyframe, which means the "track nearest keyframe" semantics for _auto won't apply if more keyframes get added later. Probably what you want when the user explicitly edited at that time — just flagging.

Nits

• packages/studio/src/hooks/useEnableKeyframes.test.ts:171-204 — the new test covers the t <= setStart regression only. The PR's primary race fix (batched depth commit), pickBestAnimation group-aware filter, commitKeyframeProps extend-tween path, updateKeyframeInScript merge, and gsapRuntimeBridge flat-keyframed-pos branch all have new code paths but no new pinning tests. For a 448-line bug-cluster fix the regression net is light — a follow-up test PR would be welcome. (nit)

• packages/studio/src/components/editor/MotionPathOverlay.tsx:158-163 — Math.round((... ) / sc - elHome.x) / ps) — the Math.round happens before the / ps divide, so the de-magnified offset can land off-pixel by up to 1/ps comp px. Probably below the noise floor for typical lens values, but if you ever see "create-path landed slightly off where I clicked" on a steep-depth element, this is why. (nit)

— Rames D Jusso

[vanceingalls]

PR #1762 — fix(studio): batch 3D depth commit so perspective + z don't race

Verdict: LGTM with comments. No blockers. Two-layer race fix (caller batches + writer merges) is durable; companion sub-fixes (enable-keyframes 0%, first-edit-creates-set, motion-path through perspective, keyframe MERGE) are all the right shape. Converges with Rames at e6bd9e44.

Reviewed at e6bd9e44. Read the diff end-to-end and pulled runtime/init.ts, useAnimatedPropertyCommit.ts, gsapWriterAcorn.ts, propertyPanel3dTransform.tsx, useEnableKeyframes.ts, and gsapConstants.ts at the PR HEAD.

---

Per-fix verdict

Fix #1 (batch 3D depth commit — perspective + z + first-depth pose). ✅ Sound. onDepthCommit builds one props record (z + optional transformPerspective + optional rotationX/Y when flat) and dispatches through onCommitAnimatedProperties as a single mutation. The keyframed-element path goes pickBestAnimation → commitKeyframeProps → add/update-keyframe(properties) — atomic. The static-element path goes commitStaticSet → add(method:"set", properties) — also atomic. The "first-time depth on a static element with no same-group tween" path (useAnimatedPropertyCommit.ts:390-419) does add-with-keyframes with keyframes carrying all four props at once — also atomic.

Worth noting (and to Rames's question on group-completeness): z, rotationX/Y/Z, transformPerspective all classify as PropertyGroupName === "other" (gsapConstants.ts:55-62), so the new group-aware pickBestAnimation keeps the entire 3D cluster on one tween. That's why the batch can't accidentally spread across two tweens.

Fix #2 (enable-keyframes preserves 0%). ✅ Clean. useEnableKeyframes.ts:264-289 replaces the silent t <= setStart → return with a replace-with-keyframes that drops a single 0% keyframe holding the set's value. New test at useEnableKeyframes.test.ts:170-204 pins exactly that regression. The position backfill from setAnim.properties when runtime read fails is a nice defensive touch.

Fix #3 (first-edit creates gsap.set). ✅. The static-element branch routes through commitStaticSet which adds method: "set" at position: 0. The instantPatch is kind: "global-set", matching the runtime patch semantics for a hold. The dispatch chain — commitAnimatedProperties → !elementHasKeyframes → commitStaticSet → existing set found → commitSetProps else add(method:"set", global:true) — is symmetric and matches the manual-drag UX you cited in the comment.

Fix #4 (overlay + motion-path through perspective). ✅. transformWDivisor extracts matrix3d m44 and pScale = 1/m44 is forwarded through useMotionPathData and applied at every coordinate hop: MotionPathOverlay.tsx:243-249 magnifies n.x/n.y for drawing, :155-163 de-magnifies pointer→offset for create, :280-281 and :300-301 de-magnify for in-progress drag and commit. The home math now also accumulates ancestor transformTranslate while excluding el's own transform — correct, since the element's own transform is the animated offset the path itself draws. Two minor things at the bottom.

Fix #5 (keyframe MERGE not OVERWRITE). ✅. updateKeyframeInScript now mirrors addKeyframeToScript's existing-merge branch: per-key upsertProp on the ObjectExpression instead of a whole-value overwrite. That closes the "edit rotationY at 0% strips z + transformPerspective → lens animates from 0 → element pops" hole at the WRITER layer. Combined with Fix #1's caller-side batching, this is a two-layer defense: the writer can no longer drop sibling props even if a future caller forgets to batch.

One subtle behavioral delta: the old code did if (ease) record.ease = ease (truthy check, so ease: "" was a drop). The new code does if (ease !== undefined) upsertProp(...) (presence check, so ease: "" would now be written). In practice no caller passes an empty-string ease, so this is theoretical — flagging for the diff log, not as a concern.

---

🟡 Concerns (non-blocking)

🟡 packages/studio/src/components/editor/manualOffsetDrag.ts:217-228 + packages/studio/src/components/editor/useMotionPathData.ts:8-21 — duplicate matrix3d m44 extraction. Two near-identical functions: readTransformWDivisor (private, manualOffsetDrag) and transformWDivisor (exported, useMotionPathData). Same parse, same parts[15], same w > 0 ? w : 1 fallback. Both live in packages/studio/src/components/editor/, and transformWDivisor is already exported. manualOffsetDrag.ts could just import it. Pure duplication, not a behavioral bug — but if a future fix tunes one (e.g. handle parts[15] whitespace, or matrix2d coverage), the other silently drifts. Worth folding into one.

🟡 packages/studio/src/hooks/useAnimatedPropertyCommit.ts:209-274 — commitKeyframeProps reads stale anim.keyframes after the convert-to-keyframes await. When the picked anim is a set (wasKeyframed = false), the convert mutation lands server-side, but the in-memory anim is the pre-convert snapshot. kfs = anim.keyframes?.keyframes is then undefined → willExtend is false → falls through to existingKf (also undefined) → add-keyframe. That's the intended path for the just-converted case, so functionally correct — but the comment at :232-235 says "only for an already-keyframed tween, a freshly-converted set has no prior range worth remapping" and the wasKeyframed flag enforces that. Good. Worth a one-line code comment that the cached anim is intentionally not refreshed across the convert, so a future reader doesn't try to "fix" it. (Concur with Rames's question on the extend-tween path semantics — same area.)

🟡 Rames's propertyPanel3dTransform.tsx:188-194 fallback footgun. Concur. The else loop is the racing per-axis pattern this PR exists to kill. onCommitAnimatedProperties? is already wired everywhere from StudioRightPanel.tsx; making it required at the type level (drop the ?) and deleting the fallback in all three sites (onDepthCommit, commitPose, recenter) removes the regression seed. The writer-merge layer protects the keyframe path but not set paths or fresh-tween creation, so the fallback is a real latent risk. Not blocking because all current call sites pass the prop.

🟡 Rames's naturalDepthPerspective 0-height degenerate. Concur — Math.max(360, …) (or Math.max(720, …)) gives a defensible floor without going back to a magic 800. The "depth scroll silently does nothing" failure mode is worse than a slightly-wrong lens, since the user's debugging path is unclear.

🟡 Rames's Transform3DCube clamp vs numeric Z field asymmetry. Concur — the clamp belongs in the commit layer or at the parsePxMetricValue boundary, not in the wheel handler. As-is, a typed-in z = 2000 under a 1080 lens drives m44 negative, transformWDivisor falls back to 1, and the motion-path overlay silently desyncs. The two-layer batch fix here doesn't address it; surface it separately or push the clamp down.

🟡 Rames's _auto plugin re-registration on CDN fallback. Concur. ensureAutoMarkerNoop runs once at initSandboxRuntimeModular time; a hf-gsap-ready listener (or a one-tick retry via requestAnimationFrame when g?.registerPlugin is undefined) closes the fallback gap. The current code's silent bail-out re-introduces the warning storm on exactly the path that triggers it. Best-effort + idempotent already, so a retry is a one-liner add.

💭 Nits

💭 MotionPathOverlay.tsx:158-163 rounding before de-magnify. Concur with Rames. Math.round((... ) / sc - elHome.x) / ps rounds before dividing by ps. For typical lens values the drift is sub-pixel; for steep-depth elements (small ps) it's user-visible at create time. Math.round((... ) / sc / ps - elHome.x / ps) or rearranging to do the round last would be cleaner.

💭 useEnableKeyframes.test.ts:170-204 test coverage. Concur with Rames — the PR adds five new code paths and pins one. A follow-up regression-net PR would be welcome; the writer's merge-into-existing branch (Fix #5) in particular is at high risk of silent regression because it shadows the old whole-value path.

---

CI

• Player perf (load/fps/scrub/drift/parity): SUCCESS.
• Preview parity: SUCCESS.
• Preflight (lint + format): SUCCESS.
• Regression shards 1-8: in progress at review time (treat as pending, not failing).
• Perf: ${{ matrix.shard }} in the rollup is a CANCELLED job name from an older workflow run (the un-interpolated job name surfaces on cancellation). Not a real failure.

Prior reviewer state

Rames posted COMMENTED at the same e6bd9e44 SHA (15 minutes before this review). His race-fix classification (PREEMPT, two-layer) is correct and matches my read. His five concerns/questions (fallback footgun, 0-height lens, cube vs numeric clamp asymmetry, CDN-fallback gsap, light test net) all reproduce above with concur. No diff in severity — both reviewers land at COMMENTED / non-blocking.

Review by Via

[miguel-heygen]

Addressed the fallback footgun: onCommitAnimatedProperties is now required on the cube and the three per-axis else fallbacks (commitPose/recenter/onDepthCommit) are deleted — the racing read-modify-write path this PR exists to kill is gone. The render guard now keys on the batched commit, and the now-unused onCommitAnimatedProperty + CommitAnimatedProperty alias were dropped from the cube. Typecheck + lint clean.

[james-russo-rames-d-jusso]

R2 verified at be4cfa43 — PREEMPT shape preserved, atomic 3D depth commit holds.

Caller layer (propertyPanel3dTransform.tsx). Fallback footgun from R1 is resolved cleanly. Cube3dControl's onCommitAnimatedProperties is now required at the props type (line 66, no ?), and the three per-axis else loops in commitPose / recenter / onDepthCommit are deleted. onDepthCommit now atomically batches up to FOUR props (z + optional transformPerspective + optional rotationX/rotationY for the new depth-pose tilt) through a single onCommitAnimatedProperties call — the batch is wider than R1, and still one mutation. The render guard at PropertyPanel3dTransform (line 330) still gates on the optional prop, which is the right shape for the public-component contract.

Writer layer (gsapWriterAcorn.ts:783-799). updateKeyframeInScript merge-via-upsertProp is unchanged. Two-layer defense holds: caller batches, writer merges. A future caller that forgets to batch still can't drop sibling props at the keyframe level.

New commits since R1 reviewed. Only the tip (be4cfa43) is new — earlier commits in the stack (eab687fb, c03171ec, d231f5ca, b75f7967) are the same set R1 covered. The tip's useAnimatedPropertyCommit.ts pickBestAnimation group-aware filter + commitKeyframeProps extend-tween branch + fresh-same-group-tween fallback all build single mutations carrying the full props record ({ ...runtimeProps, ...props }) — no per-axis write path slipped in. Verified line-by-line at :209-274 (extend-tween: one replace-with-keyframes) and :339-419 (fresh tween: one add-with-keyframes with all props at once).

My R1 's open items at HEAD:

• 🟡 Fallback footgun — ✅ resolved (the load-bearing concern).
• 🟡 naturalDepthPerspective 0-height — still as-is; non-blocking, your call on the floor.
• 🟡 cube clamp vs numeric Z asymmetry — still as-is; separable PR.
• 🟡 _auto plugin CDN-fallback — still as-is; degraded-path concern.
• 🟡 extend-tween _auto semantics — still as-is; intent question.
• 💭 nits — test net light, motion-path round-before-de-magnify; non-blocking.

Runtime-interop sign-off deferred to Via per HF lane split — she already converged at the same SHA as R1 (LGTM-with-comments, same shape concerns).

R2 verification by Rames D Jusso

[vanceingalls]

PR #1762 R2 — be4cfa43

Verdict: LGTM. The one load-bearing concern from R1 (the fallback footgun on onCommitAnimatedProperties?) is resolved cleanly. All other R1 yellows + nits remain as-is and are intentionally deferred (Miguel's :eyes: reply scope-limited R2 to the footgun). Converges with Rames at the same SHA.

Reviewed at be4cfa43. Old HEAD e6bd9e44, new HEAD be4cfa43 — the branch was rebased (diverged: ahead 14 / behind 14, merge-base 8f7fb49d, chore: release v0.7.14), so the raw compare diff is dominated by base-advance noise from an unrelated inline-timeline parser feature. Author-attributable delta (content diff at each PR-touched path, old HEAD vs new HEAD): exactly one file — propertyPanel3dTransform.tsx. Every other PR-touched file is byte-identical between the two HEADs.

---

Per-finding status at be4cfa43

Rames's findings:

✅ #3 — onCommitAnimatedProperties? fallback footgun resolved (load-bearing). Cube3dControl's prop type at propertyPanel3dTransform.tsx:66 is now required — the ? is dropped. The local CommitAnimatedProperty type alias and the onCommitAnimatedProperty prop are deleted from Cube3dControl entirely. The three per-axis else loops at commitPose (was :109-114), recenter (was :127-132), and the depth-commit handler (was :188-194) are gone — each call site now does an unconditional void onCommitAnimatedProperties(element, props). The render guard at PropertyPanel3dTransform:330 correctly switches from gating on onCommitAnimatedProperty to gating on onCommitAnimatedProperties, which is the correct shape for the optional public-component contract. The racing read-modify-write path this PR exists to kill is no longer reachable from this file. Two-layer defense (caller batches, writer merges in updateKeyframeInScript) holds — and the writer-merge layer is unchanged from R1.

⚠️ #4 — naturalDepthPerspective 0-height clamp not added. No Math.max(360, …) floor; naturalDepthPerspective at propertyPanel3dTransform.tsx:15-22 still returns 0 on a 0-height degenerate element, in which case onDepthCommit drops the lens-write entirely. Non-blocking by Rames's own classification and mine. Deferred-by-omission; author's call on the floor.

⚠️ #5 — Transform3DCube clamp vs numeric Z asymmetry not addressed. Transform3DCube.tsx:97 still clamps only in the wheel handler; the numeric Z field in propertyPanel3dTransform.tsx remains unclamped scrub+free-form parse. A typed-in z = 2000 under a 1080 lens still drives m44 negative → transformWDivisor falls back to 1 → motion-path overlay silently desyncs. Separable PR — deferred.

⚠️ #6 — _auto plugin CDN-fallback retry not added. runtime/init.ts:100-117's ensureAutoMarkerNoop still runs once at initSandboxRuntimeModular time with no hf-gsap-ready listener / one-tick retry. Degraded-path concern, deferred.

💭 #7 — MotionPathOverlay.tsx rounding-before-divide not rearranged. :165-166 still does Math.round(((e.clientX - r.left) / sc - elHome.x) / ps) — round happens before the de-magnify divide. Sub-pixel drift on steep-depth elements. Nit-level, deferred.

💭 #8 — Test coverage net. No new pinning tests added between R1 and R2. The +25 net-line growth in the PR summary (vs base) is base-advance noise from main, NOT new tests — useEnableKeyframes.test.ts is byte-identical between old and new HEAD (205 lines both sides). The PR's test net still pins only Fix #2 (enable-keyframes 0%). Rames + I both flagged this as nit; Miguel's R2 ack treats it as a follow-up. Deferred.

My distinct findings:

⚠️ #1 — Duplicate matrix3d m44 extraction not folded. Both readTransformWDivisor (private at manualOffsetDrag.ts:220-225) and transformWDivisor (exported at useMotionPathData.ts:32) still exist with identical bodies (parts[15], w > 0 ? w : 1 fallback). Deferred — yellow, not a blocker.

⚠️ #2 — Stale anim.keyframes comment not added. useAnimatedPropertyCommit.ts:209-274's commitKeyframeProps still doesn't carry the "cached anim intentionally not refreshed across the convert-to-keyframes await" reader-aid comment I flagged in R1. Functionally correct (the wasKeyframed flag already enforces the right path); just a reader-aid request. Deferred.

---

CI at be4cfa43

• Preflight (lint + format) × 4: SUCCESS.
• Player perf (load / fps / scrub / drift / parity): SUCCESS.
• Preview parity: SUCCESS.
• Preview regression: SUCCESS.
• Detect changes × 2: SUCCESS.
• Regression shards 1-8: IN_PROGRESS at review time (treat as pending, not failing — the prior R1 SHA's shards are completed and green; new HEAD is the same content for 11 of 12 files so re-runs should converge).

No required-check failures.

Prior reviewer state at this SHA

• Rames posted R2 (COMMENTED) at be4cfa43 at 04:29:36Z (≈12 minutes before this review). His R2 explicitly confirms the fallback footgun is resolved (✅), all other R1 yellows are intact (still-as-is), the two-layer PREEMPT shape is preserved, and the new pickBestAnimation extend-tween / fresh-tween branches at useAnimatedPropertyCommit.ts:209-274 + :339-419 build single mutations with the full props record. He explicitly defers runtime-interop sign-off to me ("per HF lane split").
• Miguel posted an ack comment at 04:24:31Z confirming onCommitAnimatedProperties is now required, the three per-axis fallbacks are deleted, the render guard re-keyed, and typecheck + lint clean. Matches what I see in the diff.

Reviewers converge: both Rames and I land at LGTM-with-deferred-comments. No severity disagreement.

R2 by Via

[james-russo-rames-d-jusso]

R3 verified at acc5df1d — PREEMPT shape preserved through a Graphite restack that grew the PR scope.

R2 → R3 delta. The whole stack was restacked: be4cfa43 → acc5df1d (new SHAs, same commit titles for the bottom 4). One NEW tip commit landed: acc5df1d fix(studio): keyframe commit routing for 3D and cross-group edits. PR base is eg-04-subcomp-timeline; cumulative diff is now 12 files / 449 add / 124 del — depth-commit is one of five fixes in the stack.

PREEMPT shape (R2 invariants) still holds.

• Cube3dControl.onCommitAnimatedProperties is still required at the inner consumer (propertyPanel3dTransform.tsx:67-70) — no ?. Outer PropertyPanel3dTransformProps keeps it optional (:44), but the routing through Cube3dControl can't fall back.
• All three per-axis else fallback loops in commitPose / recenter / onDepthCommit remain deleted. git grep "for (const" on the file returns one hit, and it's the rotation-axis CHANGE-DETECTION loop in commitPose:93, not a commit loop.
• onDepthCommit is still a single unconditional onCommitAnimatedProperties(element, props) call (:174). Atomic batch is up to 4 props (z + transformPerspective + rotationX + rotationY on a flat element's first depth set).
• Writer-layer merge at gsapWriterAcorn.ts:780-799 — the function was rewritten in acc5df1d (not the depth-commit). It now MERGES props into the existing kf instead of overwriting the prop bag. This is a THIRD defensive layer for the same bug class: caller batches → writer merges → kf-update merges. The motivation in acc5df1d's commit message names exactly the R2 failure mode: "editing rotationY at the 0% keyframe would strip z / transformPerspective, so the lens then animates from 0 and the element pops".

R3-NEW notes (not blockers, defer where flagged).

• updateKeyframeInScript API contract shifted from REPLACE to MERGE at the kf-object level. In-repo callers (files.ts:801, :1103, sdk/mutate.ts:1277) only pass incremental edits, so they get strictly safer behavior. External SDK clients that relied on REPLACE to narrow a kf's prop set (e.g. "this kf should only carry x going forward") would silently see the dropped props preserved instead. No in-repo evidence for such a caller; can't speak to out-of-repo. Existing tests in gsapParser.test.ts:1862-1888 still pass under either semantic (the "replaces properties" test only asserts the new values present, not the old ones absent).
• pickBestAnimation is now group-aware: a known target group hard-filters candidates instead of just adding a +20 score bias. length <= 1 short-circuit removed. Only caller is in the same file at :262, and the new fall-through ("no same-group tween exists → create a fresh same-group keyframed tween with _auto baseline") handles the undefined return path correctly. Scope contained.
• New "extend tween + add keyframe when playhead outside range" branch in commitKeyframeProps (:230-273): remaps all existing kfs into the wider [newStart, newEnd] range, pushes a new kf at the playhead, sorts before submitting replace-with-keyframes. Edge: if the playhead lands within 0.05 pct of an existing remapped kf, both get pushed and the downstream handler is responsible for dedupe. Worth a question for the author but unlikely to bite — the playhead-outside-tween check already excludes the playhead-inside case where collision would be most common.
• Ease handling asymmetry in updateKeyframeInScript: merge path uses if (ease !== undefined) (correct for ease=""), overwrite fallback retains if (ease) (drops empty strings). Inconsistency favors the merge path, no regression vs. prior behavior.

Runtime-interop (GSAP read-modify-write timing under load, depth-pose preview semantics, motion-path foreshortening at MotionPathOverlay.tsx) deferred to Via per HF lane split.

R3 verification by Rames D Jusso

[vanceingalls]

PR #1762 R3 — acc5df1d

Verdict: LGTM. R2's footgun fix (onCommitAnimatedProperties? made required at Cube3dControl, all three per-axis fallback branches deleted, writer-layer merge in updateKeyframeInScript) is intact. R3 is NOT a pure rebase — four new commits land on top of R2's dcc92a52, each fixing a sibling pathology in the same keyframe-commit / 3D-transform surface. None reintroduce the race; each new code path holds up under audit. Converges with Rames at the same surface (he reviewed at e6bd9e44, one rebase behind — the load-bearing claim Rames re-flagged about the else fallback no longer matches the file at acc5df1d; the branches are gone).

R3 head acc5df1d. Old R2 head be4cfa43. Author-attributable delta (the 4 new commits on top of R2's content at dcc92a52): 12 files, +904 / —221 of compare-diff lines.

R2→R3 commit map

Commit	Theme	Files touched
7641a4e3	Enable-keyframes on a set at/before its start drops a 0% keyframe	useEnableKeyframes.ts + .test.ts
f05e67c2	3D Transform panel reachable on ANY element (not gated on gsapRuntimeValues)	PropertyPanel.tsx, propertyPanel3dTransform.tsx, Transform3DCube.tsx, runtime/init.ts
2335528e	Overlay + motion path track through perspective transforms (m44 magnify)	MotionPathOverlay.tsx, useMotionPathData.ts, manualOffsetDrag.ts
acc5df1d	Keyframe commit routing for 3D + cross-group edits	useAnimatedPropertyCommit.ts, gsapRuntimeBridge.ts, gsapWriterAcorn.ts

The R2 deferred yellows (naturalDepthPerspective 0-height, cube clamp vs numeric Z asymmetry, _auto plugin on CDN fallback, MotionPathOverlay rounding-before-divide, duplicate matrix3d m44 extraction, stale anim.keyframes comment, light test net) ALL remain — none introduced or worsened by the new commits, except for new specific notes called out below.

---

Per-commit audit at acc5df1d

7641a4e3 — keyframe drop at t ≤ setStart (Fix #2 from R1)

useEnableKeyframes.ts:255-282 adds a "playhead at or before set" branch: replace the set with a single keyframe at percentage 0 holding the read position, mirroring the no-animation branch. Pinning test at useEnableKeyframes.test.ts:171-204 covers the regression (single 0% keyframe committed, type replace-with-keyframes). This is exactly Fix #2 from R1, now landed with a test. Clean.

f05e67c2 — 3D panel on any element + _auto no-op plugin

• PropertyPanel.tsx:148-160 synthesizes an identity gsap3dValues when gsapRuntimeValues is null, so the 3D Transform panel renders on a fresh static element instead of being gated out (chicken-and-egg fix). Cube3dControl renders the cube ✱when onCommitAnimatedProperties is wired (always wired by the parent route — checked at StudioRightPanel.tsx).
• Transform3DCube.tsx:91-98 adds a depth-scroll clamp Math.max(min(next, 0.85*L), -4*L) so wheel-driven Z can't cross the lens — addresses the runaway "Z = 3195px past a 1080 lens" pathology. Matches Rames's m44 → 0 concern. Clamp is wheel-only; numeric-field path still unclamped (R2-deferred yellow, see below).
• runtime/init.ts:97-115 registers _auto as a no-op GSAP plugin to silence the per-frame "Invalid property _auto" warning. Idempotent + best-effort; sets window.__hfAutoNoopRegistered. Fine for the primary path.
• propertyPanel3dTransform.tsx:83-87, 142-180 adds a fixed depth-pose tilt (DEPTH_POSE_X=10, DEPTH_POSE_Y=-15) dropped on a flat element the first time depth lands, both in preview and commit. Preview + commit shape match (no snap on release). Atomic — folded into the same single onCommitAnimatedProperties(element, props) call.

2335528e — overlay + motion path through perspective

• useMotionPathData.ts:7-30 adds transformTranslate (computed-transform e/f extraction for ancestor offset) and transformWDivisor (matrix3d m44 reader for foreshortening). elementHome walks ancestors and folds in their transformTranslate (excluding the element's own transform — that's the animated offset). pScale = 1/m44 is plumbed through the hook return.
• MotionPathOverlay.tsx magnifies offset points by pScale at draw (so the path tracks the projected element), and de-magnifies pointer→offset (/ ps) on create-path and on node drag. The Math.round(...) / ps ordering nit Rames flagged at :163-166 is real — rounding happens before the divide — but as he notes, it's below the noise floor for typical lens values. Sub-pixel drift only on steep-depth elements; nit, deferred.
• manualOffsetDrag.ts:212-228, 240-242, 384-387 folds m44 into the screen→offset matrix ({ a: w/sx, … }) so depth elements' offset drags don't outrun the pointer. New private readTransformWDivisor duplicates the body of useMotionPathData.ts's exported transformWDivisor (R2 yellow #1 — duplicate m44 extraction — survives this commit and could be consolidated by importing the exported helper; deferred).

acc5df1d — keyframe routing for 3D + cross-group

• pickBestAnimation is now group-aware (useAnimatedPropertyCommit.ts:45-78): when a target property's group is known, only same-group tweens are candidates. The old animations.length <= 1 early return is gone — the new code falls through to length === 0 → undefined and length === 1 → that one. When targetGroup is undefined (no property hint), it preserves prior behavior across all animations. Correct.
• useAnimatedPropertyCommit.ts:333-345 adds an animated-element-with-keyframes early branch BEFORE the set-update path, so a 3D set no longer short-circuits to in-place update when the element is already keyframed — the depth scroll now drops a keyframe at the playhead instead of mutating the constant. The comment is explicit about ordering. Good.
• useAnimatedPropertyCommit.ts:230-273 adds an extend-tween branch when the playhead is outside an existing keyframed tween's range. Remaps existing keyframes' percentages onto the new wider range, then appends a new keyframe at the playhead's percentage. Uses selectedGsapAnimations.find((a) => !!a.keyframes) as a tween-range template. Backfills missing props from backfillDefaults. Looks correct. (Rames's question about whether the appended keyframe at the new 0% should pick up _auto is a UX choice — current explicit-value is intentional for user-driven extension; not a defect.)
• useAnimatedPropertyCommit.ts:367-393 adds a fresh same-group tween branch when pickBestAnimation returns undefined (target group has no candidate). Mirrors an existing keyframed tween's tStart + tDur so the new group animates over the same span. 0% baseline marked _auto: 1 when the playhead is past 0%. This is the cross-group fix — rotation/3D no longer contaminates an existing position tween.
• gsapRuntimeBridge.ts:244-251 adds hasKeyframedPosTween = !!posAnim?.keyframes, so a drag on an element whose position tween is currently a flat ("hold") constant goes through the keyframe path, not the static set fallback. Fixes the "drag didn't create a keyframe / didn't persist" bug for keyframed-but-currently-constant tweens.
• gsapWriterAcorn.ts:783-796 switches updateKeyframeInScript from whole-value overwrite to MERGE via upsertProp, preserving sibling props already keyframed at the same percentage. Mirrors addKeyframeToScript's merge-into-existing branch. This is the writer-layer half of Rames's "two-layer PREEMPT" — caller batches, writer merges. Solid backstop even if a future caller forgot to batch.

No new races introduced. The keyframe-routing reshape is internally consistent: every new branch terminates in exactly one mutation call.

---

Pre-existing R2 yellows — still deferred

⚠️ R2#4 — naturalDepthPerspective 0-height still returns 0 → onDepthCommit drops the lens-write entirely. Math.max(360, …) floor not added. Rames re-flagged at R3. Same trade as R2; author's call.

⚠️ R2#5 — cube clamp vs numeric Z asymmetry — the new wheel clamp at Transform3DCube.tsx:91-98 makes the asymmetry sharper, not better: wheel-driven Z is clamped against the lens, but the numeric Z field (propertyPanel3dTransform.tsx's Transform3dField) is still unclamped. A typed z = 2000 under a 1080 lens drives m44 → 0/negative → transformWDivisor returns 1 (treats as 2D) → overlay + motion path silently desync. Rames flagged the same. Separable PR.

⚠️ R2#6 — _auto plugin CDN-fallback retry — ensureAutoMarkerNoop now exists (runtime/init.ts:97-115), but it runs once at initSandboxRuntimeModular time with no hf-gsap-ready listener. The CDN fallback path (GSAP_CDN_FALLBACK_SCRIPT) async-loads gsap after bootstrap, so g?.registerPlugin is undefined and the function bails silently — _auto warnings come back on the exact path the warning fires from. Rames re-flagged. Non-blocking (degraded path), but the comment claiming the warning is suppressed is misleading for the CDN-fallback case. Worth a follow-up or a one-liner re-attempt on a hf-gsap-ready event.

⚠️ R2#7 — MotionPathOverlay rounding-before-divide — still Math.round(...) / ps at create-path. Sub-pixel only on steep-depth; nit, deferred.

⚠️ R2#1 — Duplicate matrix3d m44 extraction — readTransformWDivisor (private at manualOffsetDrag.ts:220-225) and transformWDivisor (exported at useMotionPathData.ts:32) still both exist with identical bodies. Could be folded by importing the exported helper. Deferred.

💭 R2#8 — Test coverage — One new pinning test (useEnableKeyframes.test.ts:171-204) pins Fix #2 only. The new pickBestAnimation group filter, extend-tween branch, fresh same-group tween branch, updateKeyframeInScript merge, and gsapRuntimeBridge flat-keyframed branch all ship without pinning tests. The PR's regression net is light vs the surface area touched. Rames re-flagged at R3. Worth a follow-up test PR.

Footgun status at acc5df1d — converges with R2, NOT with Rames's R3 re-flag

Rames's R3 review at e6bd9e44 re-flagged the else fallback at propertyPanel3dTransform.tsx:188-194 ("the racing per-axis loop the PR exists to prevent"). At acc5df1d that branch IS GONE — I read the file at :90-180 and :315-345: Cube3dControl accepts onCommitAnimatedProperties as required (no ? on the inner signature, :67-70), commitPose / recenter / onDepthCommit all call void onCommitAnimatedProperties(...) unconditionally, and the outer PropertyPanel3dTransform interface still has it optional (:44-47) but with the render guard at :329 gating Cube3dControl on its presence. Any future refactor that dropped the prop at the parent site would fail TypeScript on the <Cube3dControl onCommitAnimatedProperties={undefined}> shape — the contract is typed, no longer a silent runtime fallback. R2's footgun verdict holds.

Rames is one rebase behind on this specific finding; the surface differs at e6bd9e44 vs acc5df1d. Worth a quick note to him.

CI at acc5df1d

• Preflight (lint + format) × 4: SUCCESS.
• Player perf (load / fps / scrub / drift / parity): SUCCESS.
• Preview parity: SUCCESS.
• Preview regression: SUCCESS.
• Detect changes × 2: SUCCESS.
• Regression shards 1-8: IN_PROGRESS at review time.

14 success, 16 in-progress, 0 failures.

Prior reviewer state at this SHA

• Rames posted R3 (COMMENTED) at e6bd9e44 at 03:23:30Z — one rebase behind acc5df1d. He classifies the race fix as PREEMPT with the two-layer caller-batches + writer-merges shape and explicitly says "none blocking." His 🟡s match my own (naturalDepthPerspective 0-height, cube clamp vs numeric Z asymmetry, _auto CDN-fallback). His one mismatch — the else fallback re-flag at :188-194 — is outdated by the rebase from e6bd9e44 to acc5df1d; the branches are deleted. No severity disagreement.

Reviewers converge: both Rames (at e6bd9e44) and I (at acc5df1d) land at LGTM-with-deferred-comments.

R3 by Via