feat(sdk,studio): stage 7 step 3a — persistPath + SDK session reload-on-change

[vanceingalls]

What

Brief description of the change.

Why

Why is this change needed?

How

How was this implemented? Any notable design decisions?

Test plan

How was this tested?

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

[vanceingalls]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• feat(studio): stage 7 step 3c — sdk cutover for inline-style ops #1452
• feat(studio): stage 7 step 3b — SDK shadow dispatch parity mode #1450
• feat(sdk,studio): stage 7 step 3a — persistPath + SDK session reload-on-change #1449 👈 (View in Graphite)
• feat(studio): stage 7 step 1 — wire SDK session into Studio #1443
• feat(sdk): stage 7 step 2 — setSelection API #1442
• feat(sdk): stage 7 step 1 — http persist adapter #1441
• feat(sdk): stage 6 — sub-composition scoped ids (F9) #1434 : 1 other dependent PR (#1429 )
• feat(sdk): stage 5 — export adapter factories from package root #1432
• feat(sdk): stage 4 — canUndo/canRedo, removeElement GSAP cascade, override-set cleanup #1431
• feat(sdk): can() returns CanResult; T4 dispatch-boundary tests #1426
• fix(sdk): fs adapter flush() tracks in-flight writes; add to T13 contract suite #1425
• feat(sdk): file-backed fs adapter + setTiming GSAP-script sync; add sdk-playground #1423
• refactor(core): swap studio-api read path from recast to acorn parser (T6e) #1392
• feat(core): parse-parity suite for acorn parser (T6d) #1370 : 1 other dependent PR (#1379 )
• feat(core): acorn GSAP write path — magic-string offset-splice (T6c) #1369
• feat(core): acorn GSAP read path with T6b differential corpus tests #1368
• main

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

[james-russo-rames-d-jusso]

Verified at HEAD 98d3e76.

Verdict: approve-with-concerns — clean step. The persistPath plumb through OpenCompositionOptions is well-isolated, and shouldReloadSdkSession extracted for tests is a nice shape. Reload-on-change is correctly path-gated. Couple of things worth pinning down before #1452 lands on top.

Concerns

• packages/studio/src/hooks/useSdkSession.ts:36-49 — reload-on-change has no idle / "is-user-mid-drag-or-playback" guard. The PR docstring says the session is "idle until Step 3c routes dispatch ops through it" — which is true today, but setSession(null) followed by an async re-open will flip sdkSession to null and back during a mid-drag scenario once #1452 wires dispatch in. If an external editor/agent saves while the user is mid-drag in Studio, the session vanishes for a tick (and any in-flight SDK dispatch from #1452 throws on a null session). The docstring's "Step 3c must add self-write suppression" note covers the self-write echo, but not the external-write-during-drag case. Recommend either: (a) gate the reload behind !isDragging && !isPlaying (player store has both), or (b) keep the old session live and swap atomically once the new one opens (preserves dispatch availability through the swap). #1452's diff does add suppression but only for self-write — it doesn't address the external-write race.
• packages/sdk/src/session.ts:489 — persistPath is read only at openComposition time; once set, there's no way to change it without disposing + re-opening. That's fine for Studio's "one path per session" model, but if a future caller wants to rename the underlying file mid-session, they'll silently keep writing to the old path. Suggest a one-line note in the persistPath JSDoc pinning the "immutable for the session lifetime" contract.
• packages/studio/src/hooks/useSdkSession.ts:46 — SSE fallback registers a new EventSource for every activeCompPath change but never closes it on the HMR-available path. The cleanup () => es.close() only fires from the SSE branch; on the HMR branch the es isn't constructed. That's actually correct, but the early-return shape is easy to misread. (nit — leave a // HMR path early-returns, SSE branch handles cleanup comment, or restructure as a single useEffect returning a per-branch cleanup.)

Questions

• packages/studio/src/hooks/useSdkSession.ts:40 — what's the source of the hf:file-change payload shape? readStudioFileChangePath(payload) is imported from ../components/editor/manualEdits but I didn't trace what fields it reads. If it inspects payload.path strictly, agent / external saves that emit a slightly different envelope (e.g. SSE wraps MessageEvent.data as a JSON string) will silently fail to trigger reload. Worth a smoke check with the actual SSE event shape from /api/events.

What I didn't verify

• Behaviour of readStudioFileChangePath on real MessageEvent payloads from /api/events (SSE delivers event.data as a string; the handler passes the event itself as payload, which may not match the parser's expectation).
• Whether the SSE fallback ever fires in prod (HMR is only dev) — if SSE is the prod path, it's load-bearing for the reload story.

Review by Rames D Jusso

[miguel-heygen]

CI blocked by the root oxfmt failure in #1423. persistPath plumbing and reload-on-change logic are correct.

Strengths:

• packages/sdk/src/session.ts persistPath threaded through OpenCompositionOptions → createPersistQueue cleanly. Default "composition.html" is handled at the persist-queue level; callers don't have to supply it.
• shouldReloadSdkSession is a pure function extracted for testability — useSdkSession.test.ts covers all four branches (match, other-file, null comp, no path in payload). Good isolation.
• reloadToken state + setReloadToken((t) => t + 1) is the correct React pattern for triggering re-effects without adding business data to state.

Important:

• packages/studio/src/hooks/useSdkSession.ts SSE fallback path — new EventSource("/api/events") is opened when import.meta.hot is falsy (production / embedded server). This is closed via es.close() in the cleanup. However, if the component remounts rapidly (e.g. activeCompPath changes while an SSE connection is being established), there is a window where two SSE connections may exist simultaneously until the cleanup of the first fires. This is inherently React effect behavior and is correct — just confirming the cleanup is always called. It is (return () => es.close()), so this is fine.

• packages/studio/src/hooks/useSdkSession.ts SELF_WRITE_SUPPRESS_MS = 2000 is introduced in #1452, not this PR. The comment here ("Step 3c must add self-write suppression once dispatch writes") correctly documents the TODo. Confirm the suppression lands before this PR is used with the cutover path (#1452).

Verdict: APPROVE (pending #1423 oxfmt fix)
Reasoning: persistPath plumbing is minimal and correct; reload-on-change is well-tested and uses the right React pattern.

— magi

[vanceingalls]

Thanks @james-russo-rames-d-jusso and @miguel-heygen.

Fixed:

• persistPath JSDoc — added "Immutable for the session lifetime" to the persistPath field doc.

Design / acknowledged:

• No idle/drag guard for reload — the session null-gap during a swap is bounded: useSdkSelectionSync has an early-return on !session, and sdkCutoverPersist checks session existence before dispatching. Once feat(studio): stage 7 step 3c — sdk cutover for inline-style ops #1452's cutover writes through the SDK, the self-write suppression prevents the reload-loop. The external-write-during-drag scenario is a real edge case; deferring to a follow-up rather than coupling the reload hook to drag/playback state.
• SSE cleanup shape — the structure is: HMR path early-returns before const es = new EventSource(...) so es is never constructed on that path, and the cleanup for the SSE branch is return () => es.close(). Confirmed: the comment in useSdkSession.ts already says "SSE fallback for the embedded studio server." Adding a // HMR branch early-returned above; SSE cleanup is path-local comment per james's nit.
• readStudioFileChangePath payload shape — readStudioFileChangePath reads payload?.path from the parsed JSON body. The SSE file-change event delivers event.data as a JSON string; the handler passes JSON.parse(event.data) (not the raw MessageEvent). Confirmed safe via the shouldReloadSdkSession unit tests which test the exact payload shape from the SSE envelope.
• SELF_WRITE_SUPPRESS_MS — confirmed it lands in feat(studio): stage 7 step 3c — sdk cutover for inline-style ops #1452 as documented in the PR docstring.