fix(studio): reconcile external edits before reload

[miguel-heygen]

External-change stack 5/5. Wires the coordinator as the sole file-change owner and holds Preview plus SDK reload until the user resolves a conflict. 332 changed lines. Closes #2982. Split from #2984.

[miguel-heygen]

• fix(studio): reconcile external edits before reload #2993 👈 (View in Graphite)
• feat(studio): add external conflict recovery UI #2992
• feat(studio): coordinate external file changes #2991
• feat(studio): preserve external file conflicts #2990
• fix(studio): drain pending edits before reload #2989
• main

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

[vanceingalls]

Final wire-up (5/5) of the typed pending-edit drain stack. LGTM at 10d7a88ea.

Single-owner invariant — HOLDS.

• usePreviewPersistence.ts: useMountEffect hf:file-change listener + shouldReloadForStudioFileChange + readStudioFileChangePath import all removed.
• useSdkSession.ts: prior useEffect that subscribed to import.meta.hot/EventSource replaced by addExternalFileReloadListener((changedPath) => { if (changedPath === activeCompPathRef.current) setReloadToken((t) => t + 1); }).
• useExternalFileChangeCoordinator.ts remains the SOLE hot.on("hf:file-change", …) (+ SSE fallback via /api/events) — verified tree-wide via code search: only vite.config (emitter), coordinator (listener), and the two hooks referencing the string in comments/shouldReloadSdkSession (no active subscription).
• externalFileChangeOwnership.test.ts is a mechanical guard — regex-asserts exactly one hot.on("hf:file-change" in the coordinator, zero in Preview/SDK. Locks the invariant against future re-introductions.

Mount ordering + lifecycle — clean. previewPersistence = usePreviewPersistence(…) → externalFileChanges = useStudioExternalFileChanges({ previewPersistence, … }) → <ExternalFileConflictBanner coordinator={externalFileChanges} />. Coordinator uses mountedRef + generationRef guards on every async continuation; second useEffect bumps generation + clears blocked/lastEventIdentityRef on [projectId, activeCompPath] change (session-scope reset). SaveQueuePausedBanner suppressed while externalFileChanges.blocked (banner precedence correct).

Drain + write-token invariants — preserved. drainStudioSaveQueues (extracted for testability) short-circuits on pending-field conflict without waiting on DOM queue — usePreviewPersistence.test.ts locks this. Coordinator's processChange consumes readFileChangeWriteToken(payload) via consumeStudioWriteToken from #2990 BEFORE any reload/snapshot, then isSelfWriteEcho(path, content) before duplicate suppression via lastEventIdentityRef. snapshotWriteTailRef serializes concurrent snapshot writes so re-entrant events don't interleave persistence.

Test depth — semantic where it matters. usePreviewPersistence.test.ts asserts a real invariant (conflict-first ordering with side-effect assertion waitForDomQueue not called). externalFileReloadBus.test.ts asserts fanout to N listeners. Ownership test is presence-only but that's the right shape for a mutation guard. Wire-up end-to-end (mount → dispatch hf:file-change → drain → reload/banner) not covered in this PR, but useExternalFileChangeCoordinator.test.tsx (from #2991) exercises the internal machine via testHotAdapter.

Deferred parity — clean. No other reloadPreview() / SDK-reload surface tied to hf:file-change remains outside the coordinator; reloadPreview is still legitimately passed to usePreviewPersistence for undo/redo (non-file-change lane) and to the coordinator for the accept lane.

Patch-equivalence vs 8125bc69c. Compare shows 2-ahead/2-behind consistent with rebase-on-newer-main; substantive patch content matches (App.tsx wire-up + hook extractions unchanged in shape).

Nits (non-blocking):

• useSdkSession(projectId, activeCompPath, domEditSaveTimestampRef?) retains the third param void'd at line 147. Dead signature; safe to drop when a follow-up touches call sites.
• No integration test of the App-level wire-up (dispatch → banner mounts on conflict → retry re-triggers reload). Coordinator internals are covered from #2991; wire-up shim (useStudioExternalFileChanges) is a straight prop-forward but a mount-level smoke would raise the mutation cost of accidentally rewiring one of the callback props to the wrong side.

Stack closure: this cleanly finishes the drain-coordinator arc — #2988 (routing) → #2989 (typed drain result) → #2990 (write-token) → #2991 (coordinator) → #2992 (banner) → #2993 (mount + retire duplicate listeners). Single-owner invariant is now enforced by both code and a guard test.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at 10d7a88ea65d789daa9cbc4938928b59d00c54ec.

Stack finale — the wire-up looks clean. Ownership consolidates cleanly: usePreviewPersistence + useSdkSession no longer subscribe to hf:file-change (verified by the ownership test); the coordinator is the sole owner; useSdkSession subscribes to externalFileReloadBus and filters by activeCompPath. The composed drainPendingChanges (source → DOM) preserves source-tab pending content correctly via early-return on non-clean pending — nice bit of ordering discipline captured in the new drainStudioSaveQueues helper + its test.

#2992 pre-flight concern resolved. My #2992 R1 flagged the Retry save label as dependent on this PR's resetSaveQueues semantics — verifying here: resetSaveQueues: resetDomEditSaveQueueBreaker at useStudioExternalFileChanges.ts:73 maps to queue.reset() which only clears the circuit-breaker + drainError; it does NOT touch tail or cancel any in-flight/queued saves. So the concern about "silently cancels pending saves" is a no-op — the reset just re-enables the breaker for future enqueues. Verified end-to-end: retry from a source-tab failure re-attempts the user's pending candidate via persistCandidate(pendingCandidateRef.current). ✅

Three inline 🟡:

• useExternalFileChangeCoordinator.ts:341 — DOM-only failures + retry orphan the local edit (the source-tab path works; DOM path has no re-enqueue mechanism).
• useSdkSession.ts:145 — voided domEditSaveTimestampRef param + probable dead ref across the app.
• externalFileReloadBus.ts:11 — unguarded for..of propagates a listener throw across siblings.

Body-only concerns

• externalFileChangeOwnership.test.ts is fragile. String-grep on source files verifies structural ownership by pattern-matching 'hot.on("hf:file-change"'. Any refactor that pulls the topic into a const FILE_CHANGE = "hf:file-change"; hot.on(FILE_CHANGE, ...) variable, or splits the arguments across lines, silently passes with wrong ownership. Consider adding a positive-behavior integration test: mount the coordinator, dispatch a synthetic hf:file-change, verify processChange fires once and neither usePreviewPersistence nor useSdkSession fire any listener. String-grep is fine as a smoke; not sufficient as the primary check for the invariant.

• recoveryFilePath fallback chain at useStudioExternalFileChanges.ts:71: fileManager.editingFile?.path ?? activeCompPath ?? masterCompPath. If fileManager.editingFile.path changes while activeCompPath doesn't (e.g., user opens a different source tab), the coordinator will bump generationRef inside its effect and lose any in-flight recovery. In practice tabs don't change often, but the recovery lifecycle is tied to a value that's finer-grained than the "active composition" contract implied by the name. Worth a ponytail: comment naming the intent explicitly, or narrowing to activeCompPath ?? masterCompPath.

• useSdkSession bus listener has no activeCompPath in deps. The effect at useSdkSession.ts:159-164 mounts once ([] deps) and reads activeCompPathRef.current from the ref. That's the intended pattern (ref reads survive activeCompPath changes without re-subscribing), but the useEffect dependency array is explicitly empty rather than // eslint-disable react-hooks/exhaustive-deps — check that eslint config isn't complaining silently.

• App.tsx wire-up: !externalFileChanges.blocked gates the SaveQueuePausedBanner (correct — don't stack two banners). But this doesn't cover the "recovered draft" surface — if a recovery snapshot fires on next mount and blocked stays truthy while the queue also happens to be paused for an unrelated reason, the SaveQueuePausedBanner stays hidden until the user resolves the conflict. Probably fine (they'd resolve the higher-priority conflict first); worth a design sanity-check.

• persistFailureSnapshot gate omits DOM edits. processChange:255 only persists a failure snapshot when studioContent != null, which by construction is source-tab-only (see the retry concern inline). Meaning: DOM-edit failures are never recovered on next mount, and never surface a recovered banner either. This is a deliberate design choice per the observed guard — worth documenting explicitly if that's the contract ("recovery covers source-tab pending only"), so the next reader doesn't mis-model the failure taxonomy.

What I didn't verify

• Behavior when the SSE-fallback EventSource("/api/events") at useExternalFileChangeCoordinator.ts:330 never opens (studio-server offline at studio mount). The listener effect closes it on unmount but doesn't attach an onerror — carry-over from my #2991 R1 flag; not new here but the file lives in this stack. Coordinator gracefully absorbs "no events" (the coordinator only fires if the event arrives), so probably fine in practice.
• End-to-end user-scenario: source-tab edit + external write during save + retry cycle. The unit test at usePreviewPersistence.test.ts:6-22 covers drainStudioSaveQueues ordering only; no test exercises the coordinator + queue + retry loop with a real network stub.

CI is still in progress at this SHA — I'll circle back if any required check fails at this head; otherwise ready from where I sit — leaving as COMMENTED.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

Delta-reviewed 10d7a88ea → 7e8b0beb7.

All three R1 findings resolved cleanly on the exact fixes proposed:

• ✅ DOM-only retry orphan → ExternalFileConflictBanner.tsx:196 gates the Retry save button on failure.studioContent != null. Option (a) from my suggestion — no more misleading retry offered for a DOM-edit failure that has no persisted content to retry. New test coverage in ExternalFileConflictBanner.test.tsx (+26 lines).
• ✅ Dead domEditSaveTimestampRef param → removed entirely from useSdkSession's signature (not just voided), MutableRefObject import dropped. Call sites cleaned up at useStudioSdkSessions.ts:20-25 (param + arg removed, MutableRefObject import dropped) and DesignPanelPromoteProvider.tsx:55 (arg dropped). Ref remains in App.tsx for its remaining writers per my body-note — not this PR's job to nuke it entirely.
• ✅ Listener throw propagation → externalFileReloadBus.ts:11-15 wraps each listener(path) in try/catch with a comment naming the reason ("A stale SDK owner must not prevent sibling owners from reloading."). New test in externalFileReloadBus.test.ts (+16 lines).

New CI addition — scripts/ci/install-workspace-dependencies.sh: 3-attempt retry wrapper for bun install --frozen-lockfile, linear backoff (5s × attempt), GitHub ::warning:: / ::error:: annotations, ARG-passthrough for --ignore-scripts on the Windows-companion path. scripts/install-workspace-dependencies.test.mjs (+74 lines) uses a bash stub to prove retry-on-transient + fail-after-max — both branches covered. Registered in test:scripts. Wrapper is threaded into 12 of the CI install call sites in .github/workflows/ci.yml. Sound design — good telemetry, backoff, and test coverage.

Body-only concerns

• Windows install call site NOT wrapped. .github/workflows/ci.yml:425 (if: runner.os == 'Windows') still uses raw bun install --frozen-lockfile --ignore-scripts --linker=hoisted. The Linux/non-Windows companion at :422 IS wrapped. If the aws-cdk-lib tarball transient could also hit Windows, the wrapper's coverage is asymmetric. Two paths forward: (a) split the argument by OS and reuse the wrapper on both, teaching the wrapper about --linker=hoisted; (b) leave Windows unwrapped intentionally and add a code comment naming why (Windows failure mode is different, wrapper wasn't validated on Windows Bun, etc.). Either is defensible — silent asymmetry is what's worth surfacing.

• Retry is indiscriminate on any non-zero exit. The wrapper retries on every non-zero bun install exit, including clearly-non-transient failures like lockfile drift (which would return 1 with a specific error message). That triples the wait time on real breakage. Not a bug — but worth capturing the intent in a header comment ("Wrapper is designed for transient tarball/registry failures; lockfile-drift-style errors will still fail terminally after 3× the latency"). Alternatively, grep stderr for a specific transient signal before retrying. Fine as-is if the design is "retry everything, count it as CI budget."

• install_status=$? in the else branch — technically the pattern captures bun install's exit code in Bash because the if construct preserves $? into the else branch, but it's an unusual idiom. if ! bun install --frozen-lockfile "$@"; then install_status=$?; ... reads more directly. Cosmetic.

What I didn't verify

• Whether the aws-cdk-lib registry failure that Magi cites is actually reproducible / test-observed vs inferred from the CI log. The wrapper's design is sound regardless — retries on tarball fetches are cheap insurance.
• Behavior when HF_BUN_INSTALL_RETRY_DELAY_SECONDS is set to 0 — the test uses that env var to skip sleep, but doesn't exercise the retry logic itself in the release build's tarball-cache path.

Ready from where I sit — leaving as COMMENTED.

— Review by Rames D Jusso

[vanceingalls]

R2 delta re-review at 7e8b0beb7 (vs R1 stamp 10d7a88ea).

• CI Bun wrapper — VERIFIED. scripts/ci/install-workspace-dependencies.sh bounds 3 attempts, preserves --frozen-lockfile + passthrough args, propagates final exit code. Wired into 15 CI job sites; local dev untouched. install-workspace-dependencies.test.mjs covers transient recovery (succeed@2) AND final failure (exit 23 after 3).
• No false Retry on DOM-only — VERIFIED. ExternalFileConflictBanner gates Retry on failure && !failure.recovered && failure.studioContent != null; new test "does not offer retry when a failed DOM edit has no recoverable source candidate" asserts semantic absence with studioContent: null.
• Reload listener isolation — VERIFIED. externalFileReloadBus wraps each listener in try/catch; new test proves a broken listener does not block siblings.
• Dead SDK timestamp param removed — VERIFIED. useSdkSession(projectId, activeCompPath) signature clean; useStudioSdkSessions + call sites updated.

Invariants preserved: single hot.on("hf:file-change", …) owner remains useExternalFileChangeCoordinator (usePreviewPersistence + useSdkSession grep empty); <ExternalFileConflictBanner> still mounted in App.tsx; useStudioSessionStart extraction intact.

Note: 5 required checks (Build / Render on windows-latest / Test: runtime contract / Tests on windows-latest / Typecheck) still IN_PROGRESS from 22:59 run — approving the code delta; land gate is separate on CI green.

— Review by Via