feat(studio): coordinate external file changes

[miguel-heygen]

External-change stack 3/5. Adds the single generation-safe owner for external file events, exact echo suppression, drain ordering, retry, and both-version preservation. 650 changed lines. Split from #2984.

[miguel-heygen]

• fix(studio): reconcile external edits before reload #2993
• feat(studio): add external conflict recovery UI #2992
• feat(studio): coordinate external file changes #2991 👈 (View in Graphite)
• 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]

Stack 3/5. Adds the single generation-safe owner for hf:file-change events (useExternalFileChangeCoordinator.ts + semantic Vitest suite). Not yet wired at any call site at HEAD 670270d1 — this lands the module, the wire-up is later in the stack.

Correctness at the fix boundary — solid.

• processChange (useExternalFileChangeCoordinator.ts:445-550) correctly gates: write-token consume → self-write echo → duplicate identity → generation++ → drain → mounted+generation re-check before every setBlocked / reload. Every async continuation checks mountedRef.current && generation === generationRef.current before mutating state, and the mount effect's cleanup bumps generation (:370-376), so an unmount during in-flight drain cannot land a stale reload.
• Stale-drain supersede is exercised end-to-end by the test at :135-149 (two rapid v2 / v3 events; drain[0] resolves → no reload; drain[1] resolves → reload once). This is a semantic assertion, not a call-count trick.
• consumeStudioWriteToken from PR #2990 is invoked exactly once per event (:457), which is the whole point of the token registry — no risk of duplicate consumption because the identity dedupe check runs after the token gate. F1 conflict-first semantics from #2989 are preserved: the coordinator only interprets status:"conflict" as a conflict, never fabricates one from a payload alone.
• snapshotWriteTailRef (:367, :436-443) serializes conflict-snapshot writes so a fast-follow event can't race an earlier set and land out-of-order in IndexedDB. .catch(() => undefined) on the chain isolates one failed write from stalling the tail.
• The unconditional pendingTimelinePaths.delete(path) at :454 before the echo checks is documented as intentionally clearing the legacy marker; since the coordinator isn't wired yet, there's no live interaction with usePreviewPersistence's reader of the same ref. When it is wired, the caller will need to drop usePreviewPersistence's handler in the same commit — flag for the wire-up PR, not this one.

Test discipline — passes feedback_test_asserts_guard_semantics_not_presence.

• Order-based assertion on drain → preview → sdk (:69-82).
• Blocked-state shape matched with toMatchObject on the failure record — asserts semantic content (studioContent, recovered), not just that a mock was called.
• Restore-from-durable-snapshot tests (:151-168, :201-226) exercise the real loadConflictSnapshot → setBlocked path via vi.waitFor, and follow through by triggering keepStudioFile() to confirm the reconstructed StudioFileConflictError carries the right attemptedContent/currentVersion.
• Fixtures are injected options (not hand-typed replicas of internal shapes), so tests exercise the real state machine on the real API surface.

Nits (non-blocking):

• useExternalFileChangeCoordinator.test.tsx has no explicit exercise of useExternalFile(). The path is small but distinct from keepStudioFile — a follow-up assertion that it calls discardPendingChanges + reloadPreview + deleteConflictSnapshot would close the last coverage gap.
• The retry test at :170-199 asserts that deleteConflictSnapshot fires but doesn't assert that the successful second drain triggers reloadPreview. Adding a reloadPreview.toHaveBeenCalledOnce() after retry() would make the "retry actually resumes the pipeline" behavior explicit.

CI green across the required set (typecheck, lint, producer unit + integration, studio smoke, timeline viewport gate, Windows render, preview parity). mergeStateStatus: BLOCKED is stack-ordering, not a check failure.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at 670270d1. External-change stack 3/5. Two new files, 650 additions: the coordinator hook (useExternalFileChangeCoordinator.ts, 429 lines) and its focused test file (222 lines, 9 cases).

The hook is the actual owner of external file events — brings together the write-token primitive from #2989, the externalConflictStorage persistence from #2990, the sdkSelfWriteRegistry content-hash fallback, and the serializeStudioFileMutations drain from earlier PRs into a single generation-safe coordinator. Overall shape is right: generationRef is bumped on every state transition and checked after every await, persistSnapshotInOrder serializes IDB writes so concurrent conflict events don't race, and lastEventIdentityRef de-dups exact re-arrivals.

Three inline concerns, all 🟡 non-blocking. Two are UX/wire-up contracts that need to be named before #2992/#2993 land; one is production silent-drop risk on the SSE fallback.

Verified end-to-end:

• ✅ Generation-safety: generationRef.current += 1 bumps on unmount (:141), projectId/activeCompPath change (:146), recovery load start (:153), and processChange entry (:230). Every subsequent state write checks !mountedRef.current || generation !== generationRef.current — after drainPendingChanges (:232), after each snapshot persist (:276, :291, :303), and in every user-action callback (:347, :353, :376, :392, :399, :412). Test :129-143 verifies stale drain from an older generation is dropped.

• ✅ Echo suppression precedence: write-token check first (:224 — exact 1:1 identity via consumeStudioWriteToken), then content-hash fallback (:225 via sdkSelfWriteRegistry.isSelfWriteEcho, 32-bit FNV-1a with 2s TTL). Test :78-90 covers write-token; content-hash path is exercised indirectly through #2989's sdkSelfWriteRegistry.ts:36-72 primitive.

• ✅ Legacy path-only marker cleared but not trusted (:220-221): the comment names the invariant — the ref-set is cleared to prevent stale suppression but ownership is decided ONLY from the exact token / content below. Correct fix for the R1 concern on #2989 about path-only false-positive suppression.

• ✅ Drain-then-decide sequencing (:231-304): await drainPendingChanges(), then branch on clean (previous-failed→delete snapshot then clear blocked, else clear blocked; reload preview+sdk), failed (persist failure snapshot in-order, block failed), or conflict (persist conflict snapshot in-order, block conflict). Snapshot-persist failure escalates to failed-state (:290-302), so IDB unavailability doesn't hide the drain result.

• ✅ Serialized snapshot writes (persistSnapshotInOrder at :203-210): tail-promise chain with per-node error swallow so one failure doesn't break the chain. The write's actual result still propagates to its awaiter. Non-obvious pattern; the .catch(() => undefined) in both directions is load-bearing.

• ✅ Recovery restore (:151-193): on mount OR projectId/recoveryFilePath change, loadConflictSnapshot runs, is guarded on generation + mounted + cancelled, and constructs either a conflict or failed blocked state. The .catch() swallows storage failures so an unavailable IDB (restricted browser) doesn't create an unhandled rejection.

• ✅ useExternalFile freshness (:349-352): always re-reads via readProjectFile for failed states, uses conflict.currentContent only when available. Contrast this with keepStudioFile (see inline finding on :384).

• ✅ CI at HEAD: all 30 checks green (Fallow audit, Format, Lint, Build, CodeQL, CLI smoke, Preview parity, Producer integration tests, etc.). Graphite mergeability green. Focused-drain suite Magi cited (22/22) covers this hook + its siblings.

Body-only nits:

• Test coverage gaps. 9 test cases land the key paths but a few important ones aren't exercised:

  • isSelfWriteEcho content-hash fallback: covered indirectly via sdkSelfWriteRegistry.ts's own suite but not for THIS hook's write-token-absent-but-content-echoes path.
  • Event identity de-duplication (:227-229): no test asserts that two identical events collapse to one drain call.
  • persistConflictSnapshot failure → falls back to failed state (:290-302): no test rejects persistConflictSnapshot.
  • overwriteConflict failure in keepStudioFile (:396-411): sets blocked to failed, untested.
  • deleteConflictSnapshot failure in the clean branch (:239-244): untested.
  • useExternalFile on a live conflict (:349): only tested indirectly; no case where currentContent is null and readProjectFile is the source.
  • activeCompPath change clears blocked (:145-149): untested.
  • resetSaveQueues invocation from all three user callbacks (:338, :355, :414): not asserted.
    Not blocking — foundation surface — but a few of these guard user-facing branches that only surface once #2992/#2993 wire up the UI.

• lastEventIdentityRef retains identity across user resolution (:227-229). After useExternalFile (accept external) or keepStudioFile (overwrite), setBlocked(null) fires but lastEventIdentityRef.current still holds the resolved event's identity string. If a collaborator later reverts the file to that same content (undo their edit), the resulting external event is identity === lastEventIdentityRef.current and gets deduped — the user isn't notified. Content-based dedup with resolution should probably also clear lastEventIdentityRef.current = null in the success branches of useExternalFile (:358) and keepStudioFile (:416).

• retry semantics ponytail (:335-341). The path is: resetSaveQueues?.() → lastEventIdentityRef.current = null → processChange(payload, true) → drainPendingChanges(). Since save queues were just reset, drain returns clean, blocked clears, preview reloads. So retry() effectively means "reset save queues and re-observe the external event." It does NOT re-attempt the original save (the save was already lost when the queue was reset). Load-bearing — a ponytail: comment naming this would prevent a future maintainer from reordering "drain then reset" and inadvertently changing the semantics.

• recoveryFilePath default (:113): defaults to activeCompPath. Undocumented and untested with a distinct value — worth a short doc-comment naming why they'd ever differ.

Ready from where I sit once the three inline concerns are considered — the keepStudioFile recovered-vs-live asymmetry (:381, :384) is the load-bearing one for the wire-up PR. Leaving as COMMENTED.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟡 keepStudioFile on a recovered failed state uses potentially-stale snapshotted external content, asymmetric with useExternalFile's always-fresh read.

For recovered failed states, payload.content was reconstructed from the persisted snapshot at line 160-164 — it's the AT-FAILURE-TIME external content, not the current on-disk state. The ?? here prefers that snapshotted content over readProjectFile. If a collaborator has edited the file since the snapshot was persisted (could be minutes or hours ago after a restart), the reconstructed StudioFileConflictError.currentContent is stale.

Compare with useExternalFile at :349-352:

const external =
  current.status === "conflict" && current.error.currentContent != null
    ? current.error.currentContent
    : await readProjectFile(path);

For failed states (whether live or recovered), useExternalFile ALWAYS re-reads via readProjectFile. keepStudioFile here doesn't.

Why non-blocking: overwriteConflict's server-side path is If-Match/If-None-Match guarded per #2990's R2 verified behavior — the stale currentContent doesn't drive the actual overwrite mechanism; only currentVersion does. A stale-content-with-fresh-version scenario is edge-case (the collaborator would have to write the same version string but different content, which the version-first server contract rules out).

Why worth naming: any UI that displays blocked.error.currentContent in the conflict prompt will show the recovered snapshot's stale bytes rather than the current on-disk external content, which is confusing when the user is deciding "is my local version worth keeping?". Options:

1. Align with useExternalFile: always await readProjectFile(current.path) on recovered failed, ignore payload.content.
2. Document as intentional: comment naming that currentContent is informational-only for keepStudioFile (server drives correctness via ETag), so stale content is fine.

Recommend (1) — the extra fetch is one round-trip and the UI displays truthful bytes.

— Rames D Jusso

[james-russo-rames-d-jusso]

🟡 keepStudioFile is a silent no-op on a LIVE failed state; only proceeds on recovered failed states.

The guard if (!current.recovered || current.studioContent == null) return; early-returns for:

• Live failed (!current.recovered) — the failure just happened in this session; user hasn't restarted
• Recovered failed with no studioContent — snapshot was persisted without local candidate

So a user who sees the "Studio couldn't save your edit" banner from a LIVE failure and clicks "Keep local" would see... nothing happen. No error, no state change, no telemetry event.

Looking at the state model: for a LIVE failed state, studioContent IS set (line 283 — candidate?.path === path ? candidate.content : null), so the second half of the guard doesn't trip. It's the !current.recovered that gates.

Is this intentional? The retry path exists for live failed states (retry() — reset save queues, re-process). So the design may be: "live failed → retry is the answer; keep-local is only offered after the failure survives a restart (i.e., recovered)."

But that's a load-bearing UI contract baked into the hook that isn't documented anywhere. The wire-up PR (#2992?) needs to gate the "Keep local" button on blocked.status === "failed" && blocked.recovered === true, else the button silently no-ops.

Recommend:

1. Document the contract — a ponytail: comment above the guard naming the invariant: keepStudioFile is only meaningful for recovered-failed states; live-failed uses retry(). UI must gate accordingly.
2. OR loosen the guard — allow live-failed with studioContent to construct-and-overwrite too. Then keep-local becomes a universal escape hatch instead of a retry-first-then-keep-local flow.

(1) is cheaper. Either works.

— Rames D Jusso

[james-russo-rames-d-jusso]

🟡 The production EventSource fallback has no onerror handler — a dropped SSE connection silently stops observing external file changes.

const eventSource = new EventSource("/api/events");
eventSource.addEventListener("file-change", handler);
return () => eventSource.close();

EventSource defaults to auto-reconnect on transient network drops (per WHATWG spec) — but only for CONNECT-time and TRANSPORT-level failures. Terminal errors (401/403 auth failure, permanent 5xx from the SSE endpoint, network partition where the socket half-closes without a reconnect signal) leave the coordinator with a dead source and no observation of external writes, silently. The user's Studio session appears functional but any subsequent agent write / manual edit / undo won't fire the coordinator's handler.

Options:

1. Add eventSource.onerror — log/telemetry the failure, expose a connected: boolean state in the return handle, wire-up PR surfaces it (e.g., a small "file sync paused" indicator with a Retry button).
2. Register a readyState-watching effect — poll eventSource.readyState === CLOSED on a low-frequency tick, re-mount the effect if closed.
3. Ignore for now — this is a rare-edge scenario; the wire-up PR can add observability later.

Recommend (1) — an onerror handler adds 3 lines and gives the wire-up PR something to render. The connected state doesn't have to be surfaced in the UI immediately, but the plumbing is there.

Also worth naming: the two other event-source paths (__HF_STUDIO_HOT_TEST_ADAPTER__ for tests, import.meta.hot for Vite HMR) also have no error handling, but those are dev-time-only where connection failures are visible to the developer. The production path is the one that goes silent.

— Rames D Jusso