fix(studio): resolve cross-cutting SDK-cutover review findings

[vanceingalls]

Addresses the cross-cutting findings from the §3.0–§3.5 SDK-cutover stack review. The isolated keyframe-merge fix (#11 core) was folded into #1470 where it belongs; everything here is the shared/architectural set that can't be split without breaking intermediate builds.

Root causes fixed

• Two writers → one. Disabled the SDK persist queue (useSdkSession); Studio's writeProjectFile is the sole writer. Kills double-write, duplicate-on-write-throw, and the undo flush race.
• Stale in-memory SDK doc. forceReloadSdkSession() after every server-authoritative write (timeline delete/asset-drop/move-fallback, GSAP commitMutation, DOM server path, element delete) so a later SDK edit can't serialize the pre-write doc and revert it. Replaces the dead onDomEditPersisted hook.
• No-op detection. sdkCutover persist fns compare serialize() before/after; an unchanged dispatch (stale id, unsupported op) returns false → server fallback, no phantom undo entry. Subsumes the inconsistent getElement guards.
• batch() everywhere. All GSAP/timing/delete dispatches wrapped for atomic rollback on mid-op throw.
• Sub-composition guard. Central wrongCompositionFile check refuses edits whose targetPath ≠ the session's composition.
• @font-face preserved. Skip the SDK path when prepareContent is set (custom-font injection).
• skipRefresh / soft-reload honored. CutoverOptions.skipRefresh + a refresh hook that soft-reloads (preserves playhead via new extractGsapScriptText) and invalidates the panel cache — matching the server path.
• Stable sdkDeps. One memoized deps object shared by the 3 GSAP hooks (fixes the per-render debounce-flush regression; also collapses 3 duplicate interfaces).
• autoId clobber. addGsapAnimation skips the SDK path when an id was just server-assigned.

Known limitation (documented)

Server-fallback element delete does not GSAP-cascade like the SDK path (ponytail note + upgrade path in useElementLifecycleOps); the fallback only fires for elements absent from the SDK doc.

Verification

bun run build clean; sdkCutover 40/0, gsapWriter.acorn 22/0 (+ new sibling-preservation regression test); zero new failures vs baseline in the studio suite.

🤖 Generated with Claude Code

[vanceingalls]

• feat(sdk,core): ws-3 — unrollDynamicAnimations acorn port + SDK op #1501
• feat(sdk): stage 6 — arc path ops (setArcPath, updateArcSegment, removeArcPath) #1500
• feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes #1499
• feat(sdk,studio): ws-1.2 — percentage-based removeGsapKeyframe #1498
• feat(sdk,studio): ws-1.1 — add set method to GsapTweenSpec; route addGsapAnimation(set) through sdk #1497
• feat(sdk,studio): ws-4 — add history:false option; disable unused sdk undo in studio #1496
• feat(sdk): ws-a2 — applyDraft/commitPreview/cancelPreview → moveElement op #1490
• feat(sdk): ws-a1 — iframe preview adapter (hit-test + selection) #1489
• fix(studio): resolve cross-cutting SDK-cutover review findings #1471 👈 (View in Graphite)
• feat(studio): route GSAP keyframe add through SDK (§3.5 PR2) #1470
• feat(studio): route GSAP tween add/update/delete through SDK (§3.5 PR1) #1469
• chore(studio): document CSS-path position cut-over; GSAP-path deferred (§3.3) #1467
• feat(studio): route timeline trim/move through SDK setTiming (§3.2) #1466
• feat(studio): route element delete through SDK removeElement (§3.1) #1465
• fix(studio): wire onTrySdkPersist to sdkCutoverPersist (cutover was unwired) #1463
• feat(studio): s7.5 — delete shadow scaffolding; keep cutover flag (dark launch) #1462
• fix(studio): force-reload sdk session after undo/redo bypasses suppress window #1460
• feat(studio): stage 7 step 3c — sdk cutover for inline-style ops #1452
• feat(core): acorn GSAP read path with T6b differential corpus tests #1488
• main

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

[vanceingalls]

On the matter of #1471 — cross-cutting SDK-cutover review findings, resolved.

Intent

The load-bearing cleanup PR for the §3.0–§3.5 stack. Nine root-cause categories in the body, each addressing a class of correctness or architectural defect that the earlier PRs in the train introduced or left open. This is the PR that turns the train from a sequence of band-aid-shape commits into a long-term solution.

Per-root-cause verification

1. Two writers → one. useSdkSession.ts:91-95 drops persist: adapter, persistPath, and the persist:error handler from openComposition; the cleanup at line 109-114 no longer awaits flush(). Studio's writeProjectFile is now the sole writer. Verified. Closes the double-write race the persist queue would otherwise produce.
2. Stale in-memory SDK doc. forceReloadSdkSession() is invoked after every server-authoritative write: useDomEditCommits.ts:248 (post-server-patch tail), useElementLifecycleOps.ts:127 (post-server-fallback delete), useTimelineEditing.ts:107/313/398 (post-timeline server-fallback writes), useGsapScriptCommits.ts:73 (post-commitMutation server tail). Replaces the dead onDomEditPersisted callback that #1462 left orphaned. Verified.
3. No-op detection. All five persist fns (sdkCutoverPersist, sdkTimingPersist, sdkGsapTweenPersist, sdkGsapKeyframePersist, sdkDeletePersist) now capture const before = sdkSession.serialize(); pre-dispatch and const after = sdkSession.serialize(); post-dispatch, returning false (server fallback) when after === before. Verified. This subsumes the per-op getElement guards on set/remove branches for #1469's tween persist — a nice consolidation.
4. batch() everywhere. sdkSession.batch(() => sdkSession.setTiming(...)), batch(() => sdkSession.removeElement(...)), batch(() => sdkSession.dispatch({ type: "addGsapKeyframe", ... })), and the tween-persist's batch wrapping the add/set/remove branches. Verified. All five persist fns now have atomic-rollback semantics on mid-op throw.
5. Sub-composition guard. wrongCompositionFile(deps, targetPath) early-return in all five persist fns, gated on deps.compositionPath != null && targetPath !== deps.compositionPath. Verified. Threaded through from useDomEditSession.ts:248, 262 and useTimelineEditing.ts:163, 235. This is a correctness-critical fix — without it, an edit targeting a sub-composition file would serialize the active composition into the wrong file.
6. @font-face preserved. useDomEditCommits.ts:160-167 adds !options?.prepareContent to the SDK-attempt guard. Verified. Necessary: sdkCutoverPersist serializes only the SDK doc; prepareContent exists to inject @font-face rules into the serialized output, and bypassing it on the SDK path would drop the injected font.
7. skipRefresh / soft-reload honored. CutoverDeps.refresh?: (after: string) => void and CutoverOptions.skipRefresh?: boolean. persistSdkSerialize at sdkCutover.ts:88-110 now selects deps.refresh(after) when provided, else falls back to reloadPreview() gated on !options?.skipRefresh. The GSAP path passes sdkRefresh constructed at useGsapScriptCommits.ts:122-134 from extractGsapScriptText(after) → applySoftReload → onCacheInvalidate with a full-reload fallback. Verified. The new extractGsapScriptText export at utils/gsapSoftReload.ts:34-47 is correctly null-safe (returns null when zero or multiple GSAP scripts are present — the same ambiguity-fallback semantics applySoftReload already had internally).
8. Stable sdkDeps. useGsapScriptCommits.ts:138-153 builds the sdkDeps object via useMemo over its actual dependency list, then passes the single object to all three child hooks (useGsapPropertyDebounce, useGsapAnimationOps, useGsapKeyframeOps). The three duplicate SdkAnimationDeps / SdkPropertyDeps / SdkKeyframeDeps interfaces collapse to { sdkSession?, sdkDeps? }. Verified. The per-render debounce-flush regression that #1469 carried via inline-literal object identity is closed by the memoization.
9. autoId clobber. useGsapAnimationOps.ts:131 adds !autoId to the SDK-attempt guard: if (!autoId && method !== "set" && selection.hfId && sdkSession && sdkDeps). Verified. The comment at lines 126-130 correctly identifies the race (server just wrote the id, SDK session hasn't seen the reload yet, persisting its serialization would clobber).

Per-file observations on residual nits

• packages/studio/src/App.tsx:264-272 — selectSidebarTabStable and getSidebarTabStable (two useCallbacks with [] deps) are replaced with a useRef({ select, get }). Functionally equivalent under React semantics (both stable across renders, both closing over leftSidebarRef). The rationale isn't explained in the PR body; worth one-liner clarification that this is a cosmetic consolidation, not a hook-rule fix.
• packages/studio/src/hooks/useGsapPropertyDebounce.ts:25-58 — flushPendingPropertyEdit is still async and still fired via void flushPendingPropertyEdit() from both the debounce-timeout (line 67-69 of pre-PR) and the unmount-effect cleanup (line 73-75). The fire-and-forget race I flagged on #1469 is not closed here — an unmount mid-flush still drops the write or reloads a torn-down preview. Not a blocker because the practical hit-rate is small, but it stays open under cover of "stable sdkDeps," which addresses identity but not the race. Worth at least a comment.
• packages/studio/src/utils/sdkCutover.ts:85-91 — the doc comment on CutoverDeps.refresh correctly notes "REPLACES the default reloadPreview()" — important detail. The interplay with skipRefresh is well-described.
• packages/studio/src/hooks/useGsapScriptCommits.ts:122-134 — sdkRefresh correctly applies extractGsapScriptText → applySoftReload and falls back to reloadPreview when soft-reload either has no script or fails to apply. Verified applySoftReload returns the right success boolean for the gate. Verified onCacheInvalidate is called unconditionally (right behavior — even when soft-reload succeeded, the cache is stale).

Dispatch-chain audit

The most important consumer-chain audit on this PR is the forceReloadSdkSession plumbing. Threaded through:

• App.tsx:189-193 → passed to useTimelineEditing.
• App.tsx:303-309 → passed to useDomEditSession.
• useDomEditSession.ts:101, 197 → passed to useGsapScriptCommits and into useDomEditCommits.
• useDomEditCommits.ts:316 → passed to useElementLifecycleOps.

Every server-authoritative write path now calls it; every SDK-authoritative path correctly does not (the SDK doc is already current, calling forceReload would echo-reload). Dispatch chain audited, no short-circuit hazard, no consumer left on the old onDomEditPersisted shape. The retired callback's type declaration is also removed from UseDomEditCommitsParams — clean.

Stale-base hazard intersection

Highest-risk PR in the batch for the stale-base squash hazard. Touches all seven hotspot files: App.tsx, useDomEditCommits.ts, useDomEditSession.ts, useElementLifecycleOps.ts, useGsapAnimationOps.ts, useSdkSession.ts, useTimelineEditing.ts. Main's #1473 modified six of these (everything except useSdkSession.ts); main's #1474 modified useGsapAnimationOps.ts heavily for animationIds. Squash-merging this PR onto current main without rebasing the stack root will:

• silently revert #1474's animationIds wiring in useGsapAnimationOps.ts,
• silently revert #1473's useTimelineEditing.ts / useElementLifecycleOps.ts shadow-dispatch additions,
• silently revert #1474's CommitMutation type-extraction from useSafeGsapCommitMutation.ts.

The diff size (+348/-336, 14 files) is exactly the magnitude where the stale-base squash hazard becomes catastrophic rather than annoying. Mandatory rebase before stamp on this PR specifically.

CI

Same inherited gsapSerialize.ts format drift as the rest of the batch.

Verdict

clear-with-process-blocker at dcc46d51002879576886c687c139b801d473f2ac — the architectural cleanup itself is the right shape: every one of Miguel's nine root causes is verified resolved, the dispatch chain is audited and consistent, the consolidation of sdkDeps collapses the band-aid pattern that #1469 / #1470 carried. Standing alone the PR is the long-term solution, not a band-aid. The single residual band-aid (useGsapPropertyDebounce fire-and-forget unmount race) is small enough to fold into a follow-up PR or document inline. The blocker is process, not code: this PR's seven-hotspot footprint plus the 58-commit stack-base lag against main makes the squash-merge math dangerous. Rebase the stack root onto current main and re-verify before stamp. Re-verify if HEAD moves before stamp.

Review by Via

[miga-heygen]

Review: fix(studio): resolve cross-cutting SDK-cutover review findings

Solid remediation PR. The two-writer kill, stale-doc resync, batch() everywhere pattern, no-op detection, and sub-composition guard are all well-motivated and tightly scoped. This reads like someone who understood every race condition in the original and addressed them surgically. A few findings worth discussing:

---

1. sidebarTabRef captures stale closures at init time

packages/studio/src/App.tsx

const sidebarTabRef = useRef({
  select: (t: SidebarTab) => leftSidebarRef.current?.selectTab(t),
  get: () => leftSidebarRef.current?.getTab() ?? "compositions",
});

The old code used useCallback(..., []) — stable identity, reads leftSidebarRef.current at call time. The new useRef({...}) also captures leftSidebarRef (a ref), so the indirection works only because both closures read through .current at call time. This is correct but fragile — if anyone later replaces leftSidebarRef.current?.selectTab(t) with a direct state-based call, the ref initializer will silently freeze the first-render value. A short inline comment ("reads through ref.current — stable because the ref never changes") would save future readers from wondering. Not blocking, just a clarity nit.

2. sdk vs sdkDeps naming collision in useGsapPropertyDebounce

packages/studio/src/hooks/useGsapPropertyDebounce.ts

The outer parameter is renamed from sdkDeps to sdk, but inside every callback you destructure const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {};. So sdk is the container, sdkDeps is one of its fields, and the type exported from sdkCutover is also called CutoverDeps. Three levels of "sdk deps" at different scopes. This hasn't caused a bug, but the next person editing this file WILL shadow something. Consider renaming the outer param to sdkConfig or sdkContext — anything that doesn't re-enter the namespace. Same pattern appears in the addGsapFromProperty and updateGsapFromProperty callbacks.

3. No-op detection correctness — serialize() string equality

packages/studio/src/utils/sdkCutover.ts — all persist fns

The no-op guard does:

const before = sdkSession.serialize();
sdkSession.batch(() => { /* dispatch */ });
const after = sdkSession.serialize();
if (after === before) return false;

This is smart — it collapses stale-id, unsupported-op, and redundant-dispatch into one guard. Two edge thoughts:

(a) Serialization is not free. For compositions with many elements, serialize() is called twice per SDK-path attempt (even on the happy path). If the SDK session ever grows to non-trivial document sizes, this could become a perf concern on rapid-fire operations like property debounce at 150ms intervals. Worth a // perf: serialize is O(doc) comment so future optimizers know this is the place to look if debounce-path latency spikes. Not blocking now — the debounce already coalesces, so the window is fine for realistic doc sizes.

(b) String equality of serialized HTML is sensitive to whitespace/attribute-order normalization. If serialize() ever becomes non-deterministic (e.g., Set iteration order for attributes), this guard would false-negative (treat a real change as no-op). This is a property of the SDK's serialize contract, not this PR's problem, but the assumption is worth documenting near the guard.

4. forceReloadSdkSession after timeline delete — double reload?

packages/studio/src/hooks/useTimelineEditing.ts — deleteTimelineClip

forceReloadSdkSession?.();
reloadPreview();

The force-reload resyncs the SDK doc from disk. The reloadPreview() right after reloads the iframe. But forceReloadSdkSession triggers useSdkSession's file-watch reload (bumps reloadToken), which in the open effect does its own composition parse. Meanwhile, reloadPreview() also forces the iframe to re-fetch the file. Is there a window where the iframe reload reads the file before forceReload finishes its async open? The ECHO_SUPPRESS_WINDOW_MS (2s) in useSdkSession should suppress the echo from reloadPreview's subsequent file-change event, but I want to confirm: does forceReload guarantee the SDK doc is current before the preview reload paints, or is it fire-and-forget? If fire-and-forget, there's a narrow window where a user's next edit (between preview paint and SDK open completing) would hit a still-stale session. This pattern repeats in dropAssetToTimeline and the enqueue chain.

5. batch() added to test mocks — good, but sdkDeletePersist mock was missing it

packages/studio/src/utils/sdkCutover.test.ts

The batch mock is added to sdkDeletePersist, sdkTimingPersist, sdkGsapTweenPersist, and sdkGsapKeyframePersist test factories. Good — the tests now match the implementation. The mockSession in the sdkDeletePersist block also correctly adds batch: vi.fn((fn) => fn()), which synchronously executes the callback. This accurately models the real batch() behavior (execute inline, commit atomically).

One thing: the sdkCutoverPersist mock factory already had batch, but its serialize mock was updated from a static return to mockReturnValueOnce("<html>before</html>").mockReturnValue("<html></html>") to satisfy the new no-op guard. The "before" vs "after" distinction is correct for the happy path. But the test for "falls back when getElement returns null" (line ~140 area) never reaches serialize(), so it's fine. Just noting the mock is now order-dependent — serialize call count matters. If a future test adds an extra serialize call before the assertion, it'll get the wrong value. A factory reset between tests (which vitest does via beforeEach patterns) mitigates this, but it's worth being aware of.

6. extractGsapScriptText — single-script assumption

packages/studio/src/utils/gsapSoftReload.ts

export function extractGsapScriptText(html: string): string | null {
  const doc = new DOMParser().parseFromString(html, "text/html");
  const scripts = findGsapScriptElements(doc);
  if (scripts.length !== 1) return null;
  return scripts[0].textContent || null;
}

The JSDoc says "Returns null when zero or multiple GSAP scripts are present (ambiguous — caller should fall back to a full reload)." This is the right behavior. But new DOMParser().parseFromString(html, "text/html") will run in Node (linkedom) or the browser depending on context. In the browser it's fine; in a test/SSR context, DOMParser may not exist. Since this is Studio code (browser-only), it's fine — just flagging in case this util ever gets imported into a shared package.

7. Ponytail sweep — fallow-ignore-next-line complexity annotations

Three fallow-ignore-next-line complexity annotations were removed from useGsapPropertyDebounce.ts (the functions got simpler), and one was added to gsapSoftReload.ts doReload. The removal is great — the code genuinely got simpler. The addition to doReload is fine if fallow was flagging it. No objection.

8. compositionPath plumbing — two inline expansions

packages/studio/src/hooks/useTimelineEditing.ts — move/resize handlers

The sdkTimingPersist calls now pass compositionPath: activeCompPath inside an inline deps object:

{
  editHistory: { recordEdit },
  writeProjectFile,
  reloadPreview,
  domEditSaveTimestampRef,
  compositionPath: activeCompPath,
}

This is duplicated between moveTimelineClip and resizeTimelineClip. The GSAP hooks solved this with a single memoized sdkDeps object in useGsapScriptCommits. The timeline editing hook constructs these deps objects inline inside useCallback — every call creates a fresh object. It's not a perf problem (these aren't in a hot loop), but it's inconsistent with the pattern established 50 lines up in the same stack. Consider extracting a shared timelineSdkDeps ref or memo in this hook for consistency. Minor.

---

Summary

The architectural changes are correct and well-reasoned. The single-writer decision, no-op detection via serialize comparison, batch() wrapping for atomicity, and the wrongCompositionFile guard are all solid patterns that close real race conditions. The PR description is excellent — root causes, verification, and the known limitation (server-fallback delete doesn't GSAP-cascade) are all documented honestly.

Main thing I'd want confirmed: finding #4 (forceReload timing relative to preview reload). The rest are naming/consistency nits.

LGTM — pinging @magi for the stamp. <@U0B1J4SL8H3>

— Miga

[james-russo-rames-d-jusso]

Review — cross-cutting SDK-cutover fixes (PR audit)

Reviewed at f2458af4ac8d0541c883ef0e9e0e2f89f6d0aa79. Part of the 18-PR SDK-cutover stack review batch (Group B). The load-bearing PR — auditing each claim in the body against the diff.

Claim audit

Claim	Verified	Notes
Two writers → one (disable SDK persist queue)	✅	useSdkSession.ts opens openComposition(content) with no persist/persistPath; cleanup dispose() only, no flush(). Docstring updated.
forceReloadSdkSession() after every server-authoritative write	✅	Plumbed through useDomEditCommits (server-fallback DOM), useElementLifecycleOps (server-fallback delete), useTimelineEditing (enqueueEdit + delete + assetDrop), useGsapScriptCommits (server commit). Replaces dead onDomEditPersisted.
No-op detection (before === after)	✅	Added to all four persist fns. Closes the phantom-undo class for stale-id set/remove.
batch() everywhere	✅	All four persist fns now wrap dispatch in sdkSession.batch(...).
Sub-composition guard (wrongCompositionFile)	✅	Central function, applied in all four persist fns.
@font-face preserved (skip SDK on prepareContent)	✅	!options?.prepareContent guard in useDomEditCommits SDK-path call.
skipRefresh / soft-reload	✅	CutoverOptions.skipRefresh + deps.refresh hook; sdkRefresh uses new extractGsapScriptText + applySoftReload with full-reload fallback.
Stable sdkDeps (memoized)	✅	useGsapScriptCommits useMemo with editHistory.recordEdit, writeProjectFile, reloadPreview, domEditSaveTimestampRef, sdkRefresh, activeCompPath deps.
autoId clobber guard	✅	!autoId && check in addGsapAnimation.
Cache-merge fix (keyframe append → findIndex-then-merge)	✅	useGsapKeyframeOps.addKeyframe optimistic apply now merges into matching percentage.

All ten claims verified against the diff. No claim/diff mismatch.

Concerns

1. forceReloadSdkSession is fire-and-forget — race window with user's next edit (co-signing Miga's #4).

forceReload calls setReloadToken(t => t + 1) which triggers the open effect; the effect does adapter.read(activeCompPath).then(async (content) => { ... setSession(comp) }). Between forceReloadSdkSession() returning and the new comp reaching setSession, the OLD session is still in state. If the user's next edit fires during that gap (~100-300ms HTTP read + parse), sdkCutoverPersist reads selection.hfId from the OLD session, serialize() returns the OLD doc, and we write a doc that doesn't include the just-committed server write. The SELF_WRITE_SUPPRESS_MS=2000 time-window doesn't help here because forceReload is the explicit-bypass path, and the suppress window is the WRONG direction (it suppresses external file-change events, not stale-session reads).

This pattern repeats in useTimelineEditing.enqueueEdit (.then(() => forceReloadSdkSession?.())), handleTimelineElementDelete, handleTimelineAssetDrop, the GSAP server-commit path, and the element-delete server-fallback. All fire-and-forget.

Suggested: either (a) make forceReload return a Promise that callers can await, so the subsequent reloadPreview() happens after the session is current, or (b) add a per-write epoch token threaded through sdkSession.id or similar so the SDK persist functions can detect "session is older than the last server write" and decline. (b) is more work but eliminates the race deterministically.

This is the one finding I'd want concretely addressed before merge — the others below are nits/observability.

2. No tests for the new correctness gates.

wrongCompositionFile and before === after are critical correctness gates added by this PR, but the test file changes only adjust existing happy-path mocks (serialize returns distinct before/after so the gate doesn't trip). I don't see explicit tests that assert:

• sdkCutoverPersist (and siblings) returns false when deps.compositionPath !== targetPath.
• sdkCutoverPersist returns false when serialize() returns identical before/after.
• batch() is actually invoked (not just mocked away).

A regression in either gate (e.g. a future refactor that drops the compositionPath check, or a serialize() determinism change that breaks string equality) would slip through. Add at least one test per gate.

3. Divergent edit-history before semantics across the persist family (carryover from #1466).

sdkCutoverPersist and sdkDeletePersist use the server-fetched originalContent as before. sdkTimingPersist / sdkGsapTweenPersist / sdkGsapKeyframePersist use the SDK's pre-dispatch serialize(). After this PR, undo on a DOM/delete edit restores disk state; undo on a timing/gsap edit restores the SDK's in-memory snapshot. If the SDK doc has drifted from disk (between the now-required forceReload and the actual session reload — see concern #1), undo doesn't restore disk state. A // note: timing/gsap use serialized before; delete/dom use disk before comment near persistSdkSerialize would surface the divergence.

4. sdkRefresh swallows applySoftReload returning false silently.

const sdkRefresh = useCallback((after: string) => {
  const script = extractGsapScriptText(after);
  if (!(script && applySoftReload(previewIframeRef.current, script))) reloadPreview();
  onCacheInvalidate();
}, [...]);

Correct fallback shape, but no telemetry on the fallback. When soft-reload fails (scripts.length !== 1, etc.) we hard-reload silently. Worth a trackStudioEvent("sdk_soft_reload_fallback", { reason: ... }) so we can see how often the soft-reload path actually fires in prod.

Nits

• Co-signing Miga's #2: sdk / sdkDeps / CutoverDeps naming collision in useGsapPropertyDebounce. Rename the outer param to sdkConfig or sdkContext.
• Co-signing Miga's #1: sidebarTabRef swap from useCallback to useRef({...}) is functionally equivalent but the docstring "reads through ref.current — stable because the ref initializer never re-runs" would save future readers.
• Co-signing Miga's #6: extractGsapScriptText uses DOMParser — Studio-only, fine; if it ever moves to a shared package, gate on environment.
• compositionPath: activeCompPath is plumbed inline in 5 different callsites (timeline, dom-edit-session, dom-edit-delete, animation, keyframe), one as a memo dep. Functionally identical, but a useTimelineEditing helper that returns the deps object would deduplicate; left as-is is fine for this PR's scope.

Verdict

Solid remediation PR. Body claims all verified against diff. Architectural direction (single writer, no-op gate, batch, sub-comp guard, soft-reload refresh hook) is the right shape. The forceReload race window is the one finding I'd want addressed; the test gaps are the close second; the rest are visibility.

LGTM with the forceReload race acknowledged or addressed.

— Rames D Jusso

[vanceingalls]

Review — cross-cutting SDK-cutover review findings, resolved

Reviewed at f2458af4ac8d0541c883ef0e9e0e2f89f6d0aa79. Part of the 18-PR SDK-cutover stack review (Batch B). The load-bearing PR of the §3.0–§3.5 stack — the cleanup that closes the band-aid debt the prior cutover PRs accreted.

Celebration — this is the anti-band-aid resolution we kept asking for

Under the band-aid bar (long-term solution, not a wedge), the right verdict on a stack-trailing cleanup PR depends entirely on whether the cleanup actually closes the doors that earlier PRs left ajar. I audited each of the nine root causes the PR body names against the diff at this head, and the result is rare: all nine landed, intact, and at the layer where the underlying drift hazard lives. Concretely:

#	Root cause	Verified
1	Two writers → one (disable SDK persist queue; Studio's writeProjectFile is sole writer)	✅ useSdkSession.ts:94-100 opens openComposition(content) with no persist/persistPath; cleanup is dispose() only — the prior flush().finally(dispose) is gone with an explicit comment on the undo/redo race the flush was creating.
2	forceReloadSdkSession() after every server-authoritative write to kill the stale in-memory doc	✅ Plumbed through useDomEditCommits (server-fallback DOM at :121), useElementLifecycleOps (server-fallback delete at :127-130), useTimelineEditing (enqueueEdit chain, delete, asset-drop), useGsapScriptCommits (post-commit at :875). Replaces the dead onDomEditPersisted hook.
3	No-op detection (before === after)	✅ Added to sdkCutoverPersist, sdkTimingPersist, sdkGsapTweenPersist, sdkGsapKeyframePersist, sdkDeletePersist — five distinct call sites at sdkCutover.ts:148-153, 175-179, 207-215, 244-247, 268-272. Closes the phantom-undo class for stale-id set/remove.
4	batch() everywhere	✅ All five persist fns wrap dispatch in sdkSession.batch(...). Verified against each function body.
5	Sub-composition guard (wrongCompositionFile)	✅ Central helper at sdkCutover.ts:107-109, applied as the first guard in all five persist fns. The docstring on compositionPath names exactly the failure mode it prevents: writing the full active-comp serialization into a sub-composition file.
6	@font-face preserved (skip SDK on prepareContent)	✅ !options?.prepareContent guard at useDomEditCommits.ts:95-99 with the rationale comment. Lets the server's prepareContent run for custom-font injection.
7	skipRefresh / soft-reload honored	✅ CutoverOptions.skipRefresh + deps.refresh hook + sdkRefresh callback that uses the new extractGsapScriptText + applySoftReload with full-reload fallback. Matches the server path's playhead-preserving soft-reload.
8	Stable sdkDeps (memoized)	✅ useGsapScriptCommits.ts:872 useMemo<CutoverDeps | null> with editHistory.recordEdit, writeProjectFile, reloadPreview, domEditSaveTimestampRef, sdkRefresh, activeCompPath — one stable reference, threaded into every GSAP child hook. Closes Miga's debounce-coalescing flag from #1469 R1.
9	autoId clobber guard	✅ !autoId && predicate in useGsapAnimationOps.ts:80 before the SDK branch; comment names the failure mode (SDK serialization would clobber the server's just-assigned id).

Bonus: the cache-append-vs-writer-merge divergence I flagged at #1470 R1 lands here too as a findIndex-then-merge apply in useGsapKeyframeOps.addKeyframe — verified.

This is the [[bandaid-bar-hyperframes-reviews]] memory's "Inline telemetry + sync-drain on exit" shape generalised across an entire stack: every prior band-aid is unwound at the source layer, not papered over. The PR body's claim audit is honest — I verified every line.

The "Known limitation" is correctly anti-band-aid

Server-fallback element delete does not GSAP-cascade like the SDK path.

Under the band-aid bar this would normally be band-aid pattern #3 (silent scope-gap), which is request-changes. It isn't, because the PR body explicitly scopes it out AND the code at useElementLifecycleOps.ts:111-117 ships a // ponytail: comment with the upgrade path ("Upgrade path: cascade in removeElementFromHtml by selector/hf-id to fully match"). The fallback only fires for elements absent from the SDK doc — a class where targeting GSAP tweens are unlikely by construction. This is exactly the discipline I want: comment, not request-changes, because the band-aid is named and scoped, not hidden.

Concerns — concur with Rames on the forceReload race

forceReloadSdkSession is fire-and-forget — race window with the user's next edit. Co-signing Rames's concern #1 in full. The mechanism:

• forceReload = () => setReloadToken(t => t + 1) (useSdkSession.ts:121).
• The reload effect's adapter.read(activeCompPath).then(async (content) => { ... setSession(comp) }) runs after the next render and the HTTP round trip.
• Between forceReloadSdkSession() returning and setSession(comp) reaching state, the OLD sdkSession prop is still threaded into every consumer.
• If the user fires the next edit during that gap (~50-300ms typical), sdkCutoverPersist reads selection.hfId from the OLD session view, serialize() returns the OLD doc, and we write a doc that doesn't include the just-committed server write — a silent revert.

SELF_WRITE_SUPPRESS_MS=2000 doesn't help: it suppresses external file-change reloads, not stale-session reads, and the direction of the suppression is wrong for this race. This pattern repeats in five callsites (timeline enqueue/delete/asset-drop, GSAP server-commit, element-delete server-fallback).

This is the one finding under the band-aid bar that lands close to "minor blocker": the cure (resync after server write) introduces a narrow stale-session window of its own. Two paths I'd consider, in order of cost:

1. Make forceReload return a Promise so callers can await before the subsequent reloadPreview() — and audit each callsite to do so. Cheap, deterministic, but threads async through five paths.
2. Per-write epoch token threaded through the session — persist fns decline when their session epoch is older than the last server-write epoch. More work, but eliminates the race without async plumbing.

Either is sound; the literal patch text is less important than acknowledging this in a follow-up issue so it doesn't fall through. Comment-grade for THIS PR (Graphite stack ordering ensures the cleanup ships with the rest of the cutover and the race window is small in practice), but it should not ship to prod without one of the two paths landed.

Concerns — co-sign Rames on test gaps for the new correctness gates

wrongCompositionFile and before === after are critical correctness gates added by this PR, but the test diff only adjusts existing happy-path mocks (e.g. serialize returns distinct before/after so the gate doesn't trip). I don't see explicit tests that assert:

• sdkCutoverPersist returns false when deps.compositionPath !== targetPath.
• sdkCutoverPersist returns false when serialize() returns identical before/after.
• batch() is actually invoked (mock-spy assertion, not just smoke-mocked away).

A regression in either gate — a future refactor dropping the compositionPath check, or a serialize() determinism change that breaks string equality — would slip through CI. Add at least one explicit test per gate. This is the one ask I would not let ship to prod without resolving.

Concerns — concur with Miga on sidebarTabRef semantics

packages/studio/src/App.tsx swaps two useCallback(..., []) closures for a single useRef({ select, get }). Functionally equivalent because both versions read through leftSidebarRef.current at call time, BUT the new shape silently freezes the initializer if anyone ever refactors the inner body to capture state directly. Rather than re-iterate the nit, please add Miga's suggested comment: "reads through ref.current — stable because the ref initializer never re-runs". That single line saves the next reader.

Concerns — concur with Miga on the sdk / sdkDeps / CutoverDeps naming collision

useGsapPropertyDebounce.ts has three different identifiers with "sdk deps" in their meaning at three different scopes (outer param sdk, destructured field sdkDeps, exported type CutoverDeps). This is the kind of naming collision that ships green and bites in six months. Co-signing Miga's rename to sdkConfig or sdkContext for the outer parameter — strictly a clarity ask, not a behaviour gate.

Soft observations

• No telemetry on sdkRefresh soft-reload fallback (useGsapScriptCommits.ts:875 area). When the soft-reload predicate fails (scripts.length !== 1, or applySoftReload returns false), we silently hard-reload. A trackStudioEvent("sdk_soft_reload_fallback", { reason }) would surface how often the soft-reload path actually fires in prod and confirm the gate isn't masking a real correctness gap. Co-signing Rames's #4.
• extractGsapScriptText uses DOMParser — Studio-only, so fine, but if this ever moves to a shared package it needs an environment guard. Documenting against the future drift.
• compositionPath: activeCompPath is plumbed inline at five different callsites (timeline move/resize/delete/asset-drop + GSAP). Functionally identical, but a single helper would deduplicate. Left as-is is acceptable for this PR's scope.

Stack hygiene

Base is 06-15-feat_studio_route_gsap_keyframe_add_through_sdk_3.5_pr2_ (#1470). No stale-base squash hazard at the per-PR surface — this PR sits at the top of the cross-cutting cleanup arc, with #1489/#1490 (iframe adapter) layered above. The 14 files touched here are the union of the prior PRs' surface plus the new gsapSoftReload util — verified each is intentional.

Verdict

Minor blockers — hold for fix. The architectural direction is exactly right and the 9-root-cause audit verified at HEAD is rare in our review history (this is the kind of resolution PR the band-aid bar exists to reward — celebrating accordingly). But two findings keep this just shy of stamp-eligible:

1. Test gaps for wrongCompositionFile and before === after — both are critical gates this PR introduces, neither has an explicit regression test. Add one per gate.
2. forceReloadSdkSession race window — the cure for staleness introduces a narrow stale-session window of its own. Either acknowledge in a follow-up issue with the path chosen, OR (preferred) land the Promise-returning variant in this PR.

Everything else (sidebarTabRef comment, sdk/sdkDeps/CutoverDeps rename, soft-reload fallback telemetry) is nit-grade.

Hand to Vai for stamp once the two above are resolved or explicitly deferred with linked follow-up issues.

Review by Via

[vanceingalls]

Two of the cross-cutting findings tracked here are fixed in #1539 (commit bcc0a44a6):

• Undo before baseline inconsistency (style records on-disk bytes; timing/GSAP recorded serialize() → undo reformatted the file) → timing/GSAP now capture on-disk originalContent.
• forceReloadSdkSession write-identity-blind suppress window → seq/hash discriminator so an undo write always reloads (new sdkSelfWriteRegistry).

[miguel-heygen]

Approved as part of SDK cutover stack. Reviewed by Miga, Rames D Jusso, and Via across R1-R4. LGTM.

[jrusso1020]

Stack-wide stamp — audited bottom-up at the #1539 stack-tip (Rames D Jusso R4 + Miga + Via verified all 16 R3 + 2 CF2 findings at 6c2d66892). SDK-cutover chain cleared end-to-end.

The base branch was changed.