feat(studio): stage 7 step 3b — SDK shadow dispatch parity mode

[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 👈 (View in Graphite)
• feat(sdk,studio): stage 7 step 3a — persistPath + SDK session reload-on-change #1449
• 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 266403f.

Verdict: approve-with-concerns — shadow mode is well-shaped: flag-gated, default-off, op-mapping is unit-tested, mismatches are structured and emitted via existing telemetry. Two things worth tightening before flipping the flag in prod, plus one verification.

Concerns

• packages/studio/src/utils/sdkShadow.ts:148-163 — sdkShadowDispatch is not wrapped in try/catch. If session.dispatch(op) throws (unknown EditOp shape, SDK internal invariant violation, etc.), the throw propagates up through reportShadowDispatch → onDomEditPersisted → into useDomEditCommits's persistDomEditOperations. The server patch already succeeded by then (the onDomEditPersisted callback fires after editHistory.recordEdit), so the user sees a crashed Studio state on top of a successfully-persisted edit — bad UX for a feature whose entire purpose is "compare quietly, never affect the user." Wrap the dispatch loop in a try/catch, emit a sdk_shadow_error telemetry, and return { dispatched: false, mismatches: [{ kind: 'dispatch_error', ... }] }. The PR's stated guarantee ("no user-visible change") fails today if dispatch throws.
• packages/studio/src/utils/sdkShadow.ts:127-147 — shadow dispatch DOES mutate the SDK session's in-memory linkedom doc (that's how the parity readback works). But the server is authoritative for the actual file. If the flag is enabled but #1452's cutover flag is OFF, the SDK doc drifts further from disk on every shadow dispatch and only reconverges when #1449's reload-on-change fires from the server-side write. The reload-on-change in #1449 listens on hf:file-change — does the server's patch-element/... endpoint emit hf:file-change? If not, the SDK doc accumulates writes that never get echoed back, and parity readback will (correctly) match for the just-dispatched op but mismatch for any earlier server-side patches the SDK didn't see. Worth confirming the reload event fires for server-side patch-element writes, otherwise shadow telemetry will be noisy.

Questions

• packages/studio/src/utils/sdkShadow.ts:179-188 — trackStudioEvent("sdk_shadow_dispatch", { ..., mismatches: JSON.stringify(result.mismatches) }). If telemetry is PostHog, what's the destination property limit? Stringifying an array of mismatches into a single field may truncate or get sampled. For triage, a mismatchKinds short-array (e.g. ["element_not_found","value_mismatch"]) alongside the full JSON would let dashboards group without parsing.
• packages/studio/src/hooks/useDomEditCommits.ts:251 — onDomEditPersisted fires only on the success branch (after editHistory.recordEdit). Confirmed it doesn't fire on !patchData.changed early-return paths — good. But the GSAP-aware commits (handleGsapAwareBoxSizeCommit etc., #1452 territory) go through gsapCommitMutation, NOT persistDomEditOperations — so shadow dispatch never observes GSAP-script edits. Intentional? (Shadow only tracks the inline-style dispatch path, not the GSAP path.) If yes, worth a one-liner in the PR description so future readers know shadow ≠ full-coverage parity.

Nits

• packages/studio/src/utils/sdkShadow.ts:69-78 — OP_FIELD_RESOLVERS keyed by string with index-access fallthrough means a future new PatchOperation type silently won't be checked for parity. Either tighten the type to Record<PatchOperation["type"], OpFieldResolver> (which #1452 does anyway in the same file — fix-hoist-to-tip pattern), or add an exhaustive-check test. (nit)

What I didn't verify

• Whether /api/projects/.../file-mutations/patch-element/... fires hf:file-change on the server side; this controls whether the shadow SDK doc gets reconciled with server writes when the cutover flag is OFF.
• Behaviour of trackStudioEvent truncation/sampling for the stringified-mismatches field.

Review by Rames D Jusso

[miguel-heygen]

CI blocked by the root oxfmt failure in #1423. Shadow dispatch parity design is sound.

Strengths:

• sdkShadow.ts patchOpsToSdkEditOps — the data- prefix-idempotency fix (op.property.startsWith("data-") ? op.property : \data-${op.property}``) is correct and the test covers the double-prefix case specifically.
• OP_FIELD_RESOLVERS typed as Record<PatchOperation["type"], OpFieldResolver> — exhaustive discriminated union ensures a TypeScript error if a new PatchOperation type is added without adding a resolver. Good forward-safety.
• sdkShadowDispatch + reportShadowDispatch separation is clean — pure dispatch logic vs. telemetry wrapper.
• STUDIO_SDK_SHADOW_ENABLED flag defaults false — safe rollout path.

Important:

• packages/studio/src/utils/sdkShadow.ts sdkShadowDispatch — shadow dispatch calls session.dispatch(op) for each op but does not use session.batch(). If any single dispatch throws (e.g. E_NO_GSAP_SCRIPT), the session document may be partially mutated while the mismatch is reported. In shadow mode this doesn't affect the authoritative server path, but it can cause the session's in-memory state to diverge from the server content until the next reload. Since the session is reloaded on external change (see #1449), this is low-risk in practice — but a try/catch per-dispatch with mismatch recording on error would be cleaner than letting the error propagate to the sdkShadowDispatch catch.

• packages/studio/src/utils/sdkShadow.test.ts integration tests use import(\"./sdkShadow\") via dynamic import — this is likely for module isolation / resetting module state between tests. Confirm there is no global state in sdkShadow.ts that could leak between test cases.

Nit:

• SdkShadowMismatch.code is typed string — consider using the E_* constants from #1426 to narrow it for consumers who want to switch on error type.

Verdict: APPROVE (pending #1423 oxfmt fix)
Reasoning: Shadow mode is correctly gated, telemetry is in place, and the prefix-idempotency fix is the right shape. Partial session mutation on error is acceptable in shadow mode.

— magi

[vanceingalls]

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

Fixed:

• sdkShadowDispatch try/catch — sdkShadowDispatch now wraps the dispatch loop in try/catch, emits a dispatch_error mismatch entry, and returns { dispatched: false, mismatches: [{ kind: "dispatch_error", ... }] } on throw. The outer reportShadowDispatch still emits telemetry. No user-visible change.
• OP_FIELD_RESOLVERS exhaustive type — already typed as Record<PatchOperation["type"], OpFieldResolver> on HEAD, giving a compile-time error for any new PatchOperation type without a resolver.

Design / acknowledged:

• Shadow SDK doc drift — confirmed: the server's patch-element/... endpoint emits hf:file-change (same event as the code-editor save path), so the SSE listener in feat(sdk,studio): stage 7 step 3a — persistPath + SDK session reload-on-change #1449's reload-on-change hook fires for every server-side write. The SDK doc reconverges between shadow dispatches.
• GSAP-aware commits not in shadow — intentional. Shadow mode covers the inline-style dispatch path only. GSAP-script operations go through gsapCommitMutation and are out of scope for the parity comparison. Will add a one-liner to the PR description.
• telemetry mismatchKinds — acknowledged; will add a mismatchKinds: string[] short-array alongside the stringified mismatches field in a follow-up before the flag goes to prod.
• SdkShadowMismatch.code typed string — leaving as string for now; the E_* constants live in the SDK's engine layer and importing them into studio would create a cross-package dep. Deferring.
• No global state in sdkShadow.ts — confirmed: the dynamic import in tests is for ESM module re-resolution (Vitest module isolation), not for state reset. sdkShadow.ts has no module-level mutable state.

[vanceingalls]

Follow-up on the batch and naming points:

• session.batch() in sdkShadow.ts — sdkShadowDispatch now wraps the dispatch loop in session.batch(...). While the shadow path already has try/catch, the batch ensures a mid-loop throw (if the session has a rollback-capable batch boundary) can't leave the session partially mutated. Matches the pattern used in sdkCutoverPersist.
• Rename: reportShadowDispatch → runShadowDispatch — the report prefix implied read-only/observability-only, but the function mutates the SDK session's linkedom doc. Renamed to runShadowDispatch to make the mutation explicit. Updated the docstring to call this out directly: "this function does mutate the SDK session — it is not read-only." One callsite updated (useDomEditSession).