Skip to content

fix(studio): stop a Studio edit from reloading the preview - #3137

Merged
miguel-heygen merged 6 commits into
mainfrom
stack/preview-write-receipts
Aug 9, 2026
Merged

fix(studio): stop a Studio edit from reloading the preview#3137
miguel-heygen merged 6 commits into
mainfrom
stack/preview-write-receipts

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

A write made by Studio itself no longer reloads the preview iframe. Writes from outside Studio still do.

Why

Every Studio edit went to disk, the file watcher saw the change, and the preview reloaded as if an external editor had touched the file. The result was a visible flash after every edit, and any in-flight gesture lost its state.

How

The client mints a write token, marks it before issuing the request, and sends it as a header. The server records a receipt against the file version and attaches it to the watcher event, so the client recognises its own write and suppresses the reload. Marked before the request because the watcher event can beat the response.

Every write path claims its writes, not just the DOM ones: batch, patch-element, group, timeline and caption. A rollback write deliberately does not claim, so the preview does reload.

Test plan

  • 68 server route tests, 1189 studio hook and util tests
  • Editing an element in Studio does not flash the preview; touching the file in an external editor still reloads it

Base of the stack: every later change commits through these write paths.


Part of a stack re-cutting #3077, which stays open as the reference for the whole tree. Preview fixes land first, then the rich-text feature.

…ere external

Every mutation route wrote the file without leaving a write receipt, so the
watcher's broadcast of Studio's own edit arrived with no identity on it. The
external-change coordinator could not tell that echo from an agent or an editor
writing the file behind Studio's back, so it took the safe branch and did a full
iframe reload. That reload hides the stage for the length of the reload, which is
what the flash after a text edit was.

Every mutation write now goes through one helper that records the receipt, and
the client claims the write before the request goes out rather than after it: the
server writes and the watcher fires while the request is still in flight, so a
token marked from the response can arrive after the echo it was meant to match.

Reproduced in the browser before and after, with the reload path traced end to
end. Before, a patch-element write logged `token: null` then a reload from the
coordinator; after, the same write logs the token and `suppressed: own write
token`, with no reload.

Adds `hf-reload-debug` (localStorage, off by default) alongside the existing
`hf-resize-debug`: it records each file-change decision and its reason, plus the
stack of whoever asked for a full reload.
…DOM ones

The receipt only helps when the client marked the token it sent, and the GSAP
mutation writers never sent one. A drag commits through gsap-mutations, so the
server minted a token the client had never seen, the change came back looking
like someone else's, and the preview did the full reload the receipt was meant
to prevent.

Same one-line claim on both GSAP mutation writers, the timing sync's mutation
call, and the caption auto-save PUT.

The rollback call stays deliberately unclaimed and says why: it runs because a
mutation did not converge, so the preview is on bytes nobody can vouch for and
the reload is the point.

Verified live: a drag-shaped update-properties on the timeline now logs
`suppressed: own write token` with no reload, where it logged a coordinator
reload before.
Claiming the timeline writes pushed this file one line past the 600-line
gate. Same change as the branch made later, landed with the commit that
caused it.
@miguel-heygen
miguel-heygen marked this pull request as ready for review August 9, 2026 15:51

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 6060bf1a7.

Right shape for the class of bug. Dual-layer suppression (token receipt on the file-mutation route + isSelfWriteEcho content-hash fallback in sdkSelfWriteRegistry) is what makes this robust — token catches the fast path, content matches when chokidar coalesces or fires multiple events per write. Marking the token BEFORE the request is the load-bearing sequencing decision and the docstring on studioWriteHeaders() earns its keep by naming exactly why. Every mutation route on the server routes through the new writeMutationResult helper (with three intentional exceptions — rollback, upload, insert-composition), so a future mutation route can't silently forget to leave a receipt. Test at files.test.ts:429-438 is a direct contract lock: sends the header, expects the receipt back with the sent token and the resulting version.

One concern about a sibling write path that carries a token but skips the client-side mark; a few smaller notes.

Concern

razorSplitTransaction.ts sends X-Hyperframes-Write-Token without calling markStudioWriteToken() — every razor cut still round-trips a reload. packages/studio/src/utils/razorSplitTransaction.ts:113-125 mints its own cut:${crypto.randomUUID()} token, ships it in the header, and the server records a receipt against it at packages/studio-server/src/routes/files.ts:2553-2582 (split-batch route). Fine as far as the server is concerned. But the client never calls markStudioWriteToken(transactionToken), so when the watcher echo comes back:

  • consumeStudioWriteToken("cut:xxx")false (never marked)
  • isSelfWriteEcho(path, content) → also false for razor writes (content-echo hashes are registered via markSelfWrite in the SDK cutover path — see sdkSelfWriteRegistry.ts:31's "flow through persistSdkSerialize" contract — and razor doesn't flow through that path; it writes server-side directly)
  • reload fires

That's on top of the deliberate synchronize() call at useRazorSplit.ts:47 that already runs reloadPreview() after the cut completes. Net result: every razor cut is a preview reload from the explicit synchronize plus a second one from the unrecognised echo. The second is exactly what this PR's whole thesis is closing everywhere else, and razor is a Studio-owned mutation authored by the same team, so it fits the enumeration in the PR body ("batch, patch-element, group, timeline and caption") thematically even if it's not literally listed.

The cheap fix is one line at razorSplitTransaction.ts:113:

const transactionToken = `cut:${crypto.randomUUID()}`;
markStudioWriteToken(transactionToken);  // ← add

If you want it uniform with the rest of this PR, replace with the helper — but the helper mints a fresh token and razor-split needs its own transaction-shape prefix (cut:...) for server-side coordination (grepped transactionToken on the server — it's passed in the request body separately from the header for split-batch atomic semantics), so the split path probably wants to keep minting its own token and just also mark it. Grep confirms these are the only two X-Hyperframes-Write-Token setters on the client (useFileManager.ts:137 correctly does both mark and send, razorSplitTransaction.ts:124 sends without mark, studioWriteHeaders() does both). One-of-two doing the wrong thing is what [[feedback_sibling_primitive_pattern_divergence_check]] picks out.

Not a blocker on this PR — it's pre-existing and out of the stated scope. Just naming it because the PR is the natural sibling sweep for this pattern.

Nits

useFileManager.ts:125-138 duplicates the new studioWriteHeaders() helper. Same shape (mint token → mark → put in headers), inlined as three separate statements. Pre-existing (from PR #2990/#2991 per the module comment), works correctly. Not a bug — but this PR introduces the exact helper the inline site should call, so leaving the two shapes side-by-side hands the next contributor an ambiguity about which is the blessed pattern. If the inline site stays, add // intentionally inline: needs per-path preflight or similar; if not, one-line switch to ...studioWriteHeaders() and drop the createStudioWriteToken / markStudioWriteToken imports.

Server-side write→receipt ordering safe today, worth naming. writeMutationResult does writeFileSync(absPath, html) THEN recordFileWriteReceipt(absPath, ...) (files.ts:414-427). This works because writeFileSync is synchronous, chokidar's callback queues to the next event-loop tick, and the receipt-record completes in the same synchronous block — so the receipt is always available by the time consumeFileWriteReceipt runs. Fine. But the ordering invariant ("receipt AFTER write is safe because the watcher tick can't preempt the synchronous handler") isn't obvious from the code, and a future refactor introducing an await between the two lines (say, an async backup or an async validation) breaks the invariant silently — receipt goes in AFTER the watcher already broadcast a nulled event. One-line ponytail comment ([[project_hf_ponytail_comment_idiom]]) at the write site names the invariant and warns off the async-insertion class of regression.

registerFileRoutes at files.ts:2422 still writes inline instead of routing through writeMutationResult. The insert-composition route (api.post("/projects/:id/file-mutations/insert-composition/*", ...)) does its own writeFileSync(ctx.absPath, insertion.html) → createWriteToken → recordFileWriteReceipt sequence, same shape as the helper but hand-rolled. This is one of the routes the PR body's "every mutation route writes through here so no route can forget" was meant to close. Would be a one-line change to call writeMutationResult and drop the inline construction — matches the other three routes converted in this diff (writeIfChanged, applyGsapMutations, patch-element).

refreshPlayer unconditionally allocates an Error for the stack trace. packages/studio/src/player/hooks/useTimelinePlayer.ts:449logReload("refreshPlayer", { stack: new Error("refreshPlayer").stack }). The new Error(...).stack is eagerly evaluated as a call argument, so the Error object + stack capture happen every time refreshPlayer runs, whether or not hf-reload-debug is enabled. logReload bails immediately when disabled, but the allocation has already happened. Cheap on any single call (refreshPlayer runs rarely), but the pattern is worth avoiding in a debug helper that reads as "off by default = no cost":

if (isEnabled()) logReload("refreshPlayer", { stack: new Error("refreshPlayer").stack });

Or make logReload accept a lazy-data thunk (() => data) and only invoke on the enabled path. Small — flag it because "opt-in debug that costs nothing when off" is the useful invariant the current shape doesn't quite hold.

What lands cleanly

  • Mark-before-request sequencing. The docstring on studioWriteHeaders() at studioFileVersion.ts:56-63 is exactly the kind of "why this ordering matters" note that saves a future contributor from a race-hunting session ([[feedback_fire_and_forget_telemetry_read_race]] is the general shape). Server writes the file and the watcher broadcasts while the response is still in flight — marking the token from the response would arrive after the echo it was meant to match. Named precisely.
  • consumeFileWriteReceipt as a FIFO queue, not scalar. helpers/fileVersion.ts:26-46 — receipts are an array per file, recordFileWriteReceipt pushes with a 10s TTL filter, consumeFileWriteReceipt shifts. Rapid successive writes to the same file don't overwrite one another's receipts; each gets its own echo. Right shape.
  • Rollback deliberately unclaimed. timelineTimingSync.ts:60-64// Deliberately unclaimed: a rollback runs because a mutation did not converge, so let the restored file reload the preview. This is the correct semantic — rollback is a recovery-from-divergence path, and reloading the preview is exactly the "start from disk" the user needs at that point. Naming it in a comment (rather than an omission) is the right shape for a load-bearing negative.
  • Dual-layer suppression is complementary. Token catches the fast path (Studio-marked writes with fresh receipts). Content-echo (isSelfWriteEcho) catches the slow path (rapid writes where receipts got shifted, or chokidar-multi-event scenarios). Neither replaces the other — either alone would have a class of case it drops. [[feedback_dual_path_observability_contract]] shape, applied to write-suppression.
  • Debug helper is opt-in and content-scoped. reloadDebug.ts:1-3localStorage.setItem("hf-reload-debug", "1") then reload; grep [hf-reload]. Right shape for a targeted debug channel that doesn't pollute production console. Cache the enabled flag once per session, not on every log call.
  • TTL discipline on both sides. Client tokens (WRITE_TOKEN_TTL_MS = 5 * 60_000) and server receipts (RECEIPT_TTL_MS = 10_000) both bounded; unbounded-map-growth backstop is present. The mismatch (client 5min vs server 10s) is deliberate — client covers "slow request, retry, delayed echo"; server only needs the window from write to next-tick watcher fire.
  • X-Hyperframes-Write-Token length cap. createWriteToken() at helpers/fileVersion.ts:21-24 rejects client-supplied tokens longer than 200 chars, falls back to a fresh UUID. Sensible against a client sending a runaway header. Would flag if it accepted arbitrary length.

Series note: this is the base of Miga's stack/preview-write-receipts stack (re-cutting #3077), and the framing "preview fixes land first, then the rich-text feature" reads well — the token/receipt infrastructure is the sort of foundational contract that wants to sit at the bottom of the stack so every later Studio write inherits it automatically. CI is fully green through this SHA except for one Test job still IN_PROGRESS on the latest run, so nothing regressed. LGTM from my side; would take the razor-split mark as a follow-up if not this PR.

Review by Rames D Jusso

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta-reviewed 6060bf1a7..0ef44138f (commit fix(studio): cover remaining write receipt paths).

All five R1 items addressed cleanly.

  • Concern (razor-split mark)packages/studio/src/utils/razorSplitTransaction.ts:121markStudioWriteToken(transactionToken) inserted right after the mint. Test at razorSplitTransaction.test.ts:120-124 locks the contract: sends the header, asserts the token starts with cut:, asserts consumeStudioWriteToken(writeToken) returns true. Direct.
  • Nit (useFileManager inline duplication)useFileManager.ts:127-131 switched to ...studioWriteHeaders(), dropped the createStudioWriteToken / markStudioWriteToken imports. createStudioWriteToken is now module-private (grep confirms zero external callers).
  • Nit (write→receipt ordering ponytail comment) → new writeFileWithReceipt helper at files.ts:413-423 with the inline warning // The synchronous write cannot yield before its receipt is recorded; keep this block await-free. Names the invariant and warns off the async-insertion regression class.
  • Nit (insert-composition inline)files.ts:2430-2435 now routes through the new writeFileWithReceipt. Both the mutation-route writeMutationResult and the insert-composition route share the same shape. New assertion at files.test.ts:137-141 locks it: insert-composition records a receipt with the client-supplied token.
  • Nit (refreshPlayer eager Error allocation)logReload now accepts Record<string, unknown> | (() => Record<string, unknown>) and evaluates lazily only on the enabled path (reloadDebug.ts:24-26). Caller at useTimelinePlayer.ts:449 passes a thunk: logReload("refreshPlayer", () => ({ stack: new Error("refreshPlayer").stack })). "Opt-in debug that costs nothing when off" now holds.

Clean pass, no new findings. LGTM from my side.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1 review at 0ef4413 (post follow-up commit)

Verdict: REQUEST_CHANGES (high confidence). The receipt mechanism is sound and the follow-up commit 0ef44138 "fix(studio): cover remaining write receipt paths" closed the insert-composition route and one P3 nit — but the two patch-element-batches routes flagged in the prior review remain unwired at the current head. The commit title over-promises: those two named batch routes were not touched, so the layer-reorder / DOM-edit-batch commit path still triggers the flash the PR is meant to eliminate.

P1 — batch-route receipt gap still live at HEAD

packages/studio-server/src/routes/files.ts:2740-2763 (POST .../patch-element-batches) and packages/studio-server/src/routes/files.ts:2765-2800 (POST .../patch-elements-batch/*) still call commitElementPatchBatches(...) (lines 2758 and 2786) and never call recordFileWriteReceipt or route through the new writeFileWithReceipt helper. commitElementPatchBatches (files.ts:302-401) writes via writeFile(...) at line 373 and returns nothing token-related.

The client at packages/studio/src/hooks/useDomEditCommitsHelpers.ts:103 still sends X-Hyperframes-Write-Token on both routes via studioWriteHeaders(); the server discards it.

Scenario: user drags a layer in the Layers panel → useElementLifecycleOps.ts:305 calls commitDomEditPatchBatches(batches, { label: "Reorder layers", skipReload: true, … })patchElementBatches POSTs to /file-mutations/patch-element-batches with the header → server writes with no receipt → useDomEditCommits.ts skips the explicit reloadPreview() because skipSafe = true → CLI watcher at packages/cli/src/server/studioServer.ts:755 calls consumeFileWriteReceipt(...), gets null, SSE payload has no writeTokenuseExternalFileChangeCoordinator.ts:228 fails consumeStudioWriteToken(null) → falls into the external-change branch → reloadAcceptedGeneration → reloadPreview → refreshPlayer → the visibility-hidden reload the PR body calls "the flash".

Fix: route both handlers through writeMutationResult (or fold the receipt into commitElementPatchBatches by threading c / a writeToken through). Small delta, same shape as the fixes applied elsewhere in this PR.

P2 — receipt/debounce race (widens materially with this PR)

packages/studio-server/src/helpers/fileVersion.ts:26-41 is byte-identical to the earlier review head; the FIFO consume keyed only by absPath remains. Scenario: a timeline scrub or drag fires N writeMutationResult calls to index.html within the 300 ms watcher debounce window (packages/cli/src/server/fileWatcher.ts:41-52) → server queue is [R1..RN], one broadcast pops R1, R2..RN sit in the queue for up to 10 s → an external editor writes index.html within that window → watcher fires, consumeFileWriteReceipt returns R2 (stale bytes, stale token) → coordinator sees writeToken: T2, consumeStudioWriteToken(T2) returns true because the client's 5-minute map still has it → external write is silently suppressed and Studio's in-memory state stays on stale disk. Cheapest closure: index the receipt by (path, version) on consume so a mismatched external version can't match a stale entry; the version is already carried on the event.

P3 — nits

  • useTimelinePlayer.ts:449 deferred-alloc — FIXED in 0ef44138 via the callback thunk. Thanks.
  • reloadDebug.ts:8 enabled cached at module-init — unchanged; still a minor DX papercut. Not a blocker.
  • Test coverage widened by one route (insert-composition at files.test.ts:137) in the follow-up. Good. The two batch routes flagged in P1 still have zero receipt-behavior assertions — the batch-route tests at lines 468, 504, 532, 562, 627, 674 check response payloads only, never consumeFileWriteReceipt. If the P1 wiring lands, add a consumeFileWriteReceipt(...).toBeDefined() assertion alongside the existing batch tests.

Per-lens findings (delta vs prior head)

  1. Reload trigger — unchanged; suppression order (token → content-hash echo → event-identity dedup) still correct.
  2. Effect-dep drift — none.
  3. State-syncing via useEffect — none.
  4. Element-identity / keys — untouched.
  5. Mid-drag / mid-typing lifecycle — unchanged; token still marked before request; receipt lands synchronously.
  6. Committed-vs-ephemeral rollback — unchanged; rollback writes still don't claim.
  7. Fast Refresh / HMR — unchanged; new helpers export functions only.
  8. Test coverage — one route added; batch routes still gap (P3).
  9. CI — all checks GREEN at HEAD 0ef44138.
  10. Adjacent regression riskwriteFileWithReceipt (files.ts:413-425) is the new shared helper; correct in scope. Backups still in .hyperframes/backup (excluded from watcher). Peer symmetry note: every other HTTP write path in the studio client uses studioWriteHeaders() (token-only) — the sdkEditTransaction.ts writer that calls markSelfWrite is the outlier. So the batch-route path is symmetric with peer routes on the client side; the fault is purely the server-side receipt gap flagged in P1.

Once P1 is wired, happy to re-verify at the new head.

— Via

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 re-verify @ 4d2a48ae6

Verdict: APPROVE. All three R1 findings closed at exact sites; new commit 4d2a48ae6 "fix(studio): preserve batch write receipts" lands the coverage plus the FIFO race fix in one clean pass.

Per-finding delta:

  • P1 (batch-route receipt gap): FIXED. New wrapper commitElementPatchBatchesWithReceipts (packages/studio-server/src/routes/files.ts:403-418) now called from both routes — patch-element-batches at files.ts:2784 and patch-elements-batch/* at files.ts:2812. It delegates to commitElementPatchBatches and then loops each changed file through recordMutationReceipt (files.ts:411-416), which computes version + writeToken from X-Hyperframes-Write-Token and calls recordFileWriteReceipt (files.ts:430-440). Factored out cleanly from the former writeFileWithReceipt (files.ts:442-451), which now delegates to the same helper. Every changed file in a batch leaves exactly one per-file receipt.
  • P2a (receipt/debounce FIFO race): FIXED. consumeFileWriteReceipt(absPath, expectedVersion) (packages/studio-server/src/helpers/fileVersion.ts:36-52) is now version-indexed via current.findIndex(entry => entry.version === expectedVersion) + splice(idx, 1) (lines 46-47), replacing the prior current.shift(). Watcher side (packages/cli/src/server/studioServer.ts:754-762) readFileSyncs the current bytes, hashes via fileContentVersion, and threads that as the second arg. Two staggered writes each match their own content-version echo regardless of insertion order.
  • P3 (test coverage): FIXED. New tests: files.test.ts:586-628 (two-file batch commit leaves exact receipts for both files); files.test.ts:516-521 (existing batch test now asserts consumeFileWriteReceipt returns the expected {path, version, writeToken}); fileVersion.test.ts:27-46 (two receipts same path, different versions, consume returns each by version in either order).

Two minor observations (non-blocking, informational):

  1. studioServer.ts:756-761 re-reads the whole file synchronously per watcher echo to compute the version. Doubles disk work on large HTML files under rapid mutation streams; not a regression from prior semantics.
  2. Watcher readFileSync inside a try treats delete-echoes as "no matching receipt", so a delete always looks external. Likely intentional (reloading on delete is safe), just noting the semantics.

CI: 26 SUCCESS, 10 SKIPPED, 1 NEUTRAL (CodeQL summary), 9 IN_PROGRESS, 0 FAILURE. Waiting on Analyze (js-ts), Windows Render + Tests, Preview parity, Build, Typecheck, Producer suites, CLI npx (windows), Studio load smoke, CLI smoke. Every completed check green.

Stamp holds pending in-progress checks completing green.

— Via

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta-reviewed 0ef44138f..4d2a48ae6 (commits fix(studio): match watcher echoes by exact content version + fix(studio): preserve batch write receipts).

The two-part change reads correctly at the seam it targets. Version-match findIndex in consumeFileWriteReceipt (packages/studio-server/src/helpers/fileVersion.ts:41-45) is strictly stronger than the old FIFO — under concurrent single-file writes it credits the writer whose bytes are ACTUALLY on disk, where FIFO would have handed the echo to whoever queued first regardless of who wrote last. The new test at packages/studio-server/src/helpers/fileVersion.test.ts:37-46 pins the out-of-order lookup shape directly (both receipts remain claimable by their own version). Splitting writeFileWithReceipt into a pure recordMutationReceipt + a write+record composite (routes/files.ts:420-451) is the right shape for the batch path — commitElementPatchBatches already does the writes inline, so a commitElementPatchBatchesWithReceipts wrapper (:403-418) that iterates result.files and records each is the clean split.

One BLOCKER-tier concern and a couple of smaller ones below.

Blocker

Multi-file batch echoes collapse to ONE watcher event — N-1 per-file receipts orphan. packages/cli/src/server/fileWatcher.ts:37,46-51 — the project watcher uses a SINGLE module-scoped debounceTimer shared across every filename in the tree:

watcher = watch(projectDir, { recursive: true }, (_event, filename) => {
  if (!filename) return;
  const relativePath = filename.toString();
  if (!shouldWatchProjectFile(relativePath)) return;
  if (debounceTimer) clearTimeout(debounceTimer);
  debounceTimer = setTimeout(() => {
    for (const fn of listeners) {
      fn(relativePath);
    }
  }, DEBOUNCE_MS);
});

A group-drag batch writing index.html + scene.html inside commitElementPatchBatchesWithReceipts fires two fs events back-to-back (both writes are synchronous in the same JS tick). Both events hit the shared timer; the second cancels the first; after 300 ms silence the listener runs ONCE with only the LAST filename. The SSE listener at studioServer.ts:753-773 then reads that file's bytes, calls consumeFileWriteReceipt for that version only, and emits ONE file-change event. The receipt for the other file sits in the map until the 10 s RECEIPT_TTL_MS sweep.

Consequences:

  • The Studio tab that initiated the batch receives ONE echo (for the last file) with the batch's writeToken. consumeStudioWriteToken(T) returns true → reload suppressed for the whole batch. Reload suppression itself still works, because the client only needs one echo per user action. ✅
  • But any per-file consumer of the SSE file-change event (client-side cache invalidation, thumbnail refresh, parse cache eviction) only fires for the LAST file. Anything the client keys off per-path stays stale for the other N-1 files until an unrelated event triggers a refresh.
  • A concurrent OTHER tab observing the batch receives the same one-file echo, sees the token isn't in its local map, and reloads the WHOLE preview — recovering, so no correctness bug in that tab. But the initiating tab's per-path invalidation is the exposure.
  • Orphan receipts also poison version-match: if index.html is later modified externally to bytes that happen to hash the same as the group-drag write (identical content re-emit), findIndex matches the orphan and credits it to the group-drag token, silently swallowing the external write echo. The window is TTL-bounded and content-identity narrow, but the shape is there.

The new test at packages/studio-server/src/routes/files.test.ts:586-630 ("leaves one exact write receipt for every file in a durable element patch batch") only verifies receipts are RECORDED for both files server-side — it never drives the watcher, so this coalescing gap isn't visible in the test suite.

Two ways to close: (a) per-filename debounceTimer (Map<string, Timeout>) so per-file bursts each fire their own listener call and pass their own filename, or (b) accumulate a Set<string> of filenames per debounce burst and iterate on flush. Either restores the invariant the PR body implies — one echo per file, one receipt consumed per file. If per-file echo is intentionally NOT the guarantee (and the reload-suppression-only view is enough), a comment on commitElementPatchBatchesWithReceipts saying "batch writes emit one echo for the last file only; per-file consumers must tolerate missing events" would let the next contributor know.

Concerns

readFileSync on every debounced event runs on the main event loop. studioServer.ts:759fileContentVersion(readFileSync(absPath, "utf-8")) blocks synchronously per SSE listener, per debounced burst. Studio HTML files can carry inline data URIs and large embedded assets — bounded only by what the user's project contains, not by the server. With N Studio tabs open (N SSE listeners), each event fires N synchronous full-file reads back-to-back on the same tick. A large-HTML project can visibly stall the loop under a rapid edit sequence. Consider computing the version ONCE per debounced burst (outside the listener fan-out) and passing it in, or reading + hashing asynchronously with a small LRU keyed on (absPath, mtimeMs, size).

SSE catch swallows every read error as deletion. studioServer.ts:758-762 — the comment names deletion (ENOENT), but the bare catch {} also silently absorbs EACCES, EIO, EISDIR, and any transient EMFILE. All of them fall through to "no receipt → external change" and cost a full preview reload with no server-side signal. Gate on err.code === "ENOENT" and re-log the rest (or at least emit them at debug), so a real failure mode doesn't hide as "the client kept reloading for a bit."

Nits

Same-version receipts: findIndex first-match is caller-visible. fileVersion.ts:44 — if two clients happen to write identical bytes within TTL, both record receipts with the same version; the debounced watcher fires once, and findIndex returns the earlier receipt. That client's writeToken is echoed; the later writer sees a non-matching token and reloads. Behavior is deterministic and probably harmless (identical-content writes are rare and the visible effect is one reload), but naming it in a docstring on consumeFileWriteReceipt would let a future contributor decide whether to switch to .pop() / last-match / all-match semantics.

Rollback silently doesn't record receipts either. files.ts:383-398 — if commitElementPatchBatches throws after some writes succeed, it rolls back by writing file.before for each attempted write. Those rollback writes don't route through recordMutationReceipt, so the watcher sees them as external and the client reloads. Not a regression (prior R2 behavior was the same for the single-file mutation route on the pre-existing throw path), but the batch route has a wider blast radius — a throw mid-batch now causes up to N spurious reloads. Worth naming in a code comment on the rollback loop so the next reader knows this is by design (or worth a follow-up if it isn't).

What lands cleanly

  • Version-matched receipt lookup (fileVersion.ts:41-45) — direct fix for the concurrent-write correctness gap FIFO had.
  • Contract-lock test for out-of-order matching (fileVersion.test.ts:37-46) — pins the exact behavior the new API guarantees.
  • recordMutationReceipt / writeFileWithReceipt split (files.ts:420-451) — clean separation for the batch path where commitElementPatchBatches owns the write.
  • Batch-route wiring at files.ts:2784, 2812 — both element-batch endpoints now route through the new receipts wrapper, no route left uncovered.
  • Batch-multi-file server-side receipt-recording test at files.test.ts:586-630 — proves both files' receipts land with the batch's shared writeToken.

The blocker isn't in what changed at this layer — it's in the watcher below, which the receipts path now depends on for per-file echo. If per-file client-side consumers of file-change don't exist (i.e. the client only uses the event as a coarse "did anything change" signal), the blocker downgrades to a concern. Worth confirming before landing.

Review by Rames D Jusso

Copy link
Copy Markdown
Collaborator Author

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

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta-reviewed 4d2a48ae6..f1de808e7 (commit fix(cli): emit each changed path from a debounce burst).

BLOCKER from R3 is FIXED cleanly, exactly at the seam I flagged. packages/cli/src/server/fileWatcher.ts:37 adds const pendingPaths = new Set<string>(); in closure scope. Every fs event now pendingPaths.add(relativePath) before scheduling the debounce (:47), and the flush at :49-58 snapshots [...pendingPaths], clears the set, nulls the timer, and iterates each collected path calling every listener once:

debounceTimer = setTimeout(() => {
  const changedPaths = [...pendingPaths];
  pendingPaths.clear();
  debounceTimer = null;
  for (const changedPath of changedPaths) {
    for (const fn of listeners) {
      fn(changedPath);
    }
  }
}, DEBOUNCE_MS);

The two-file batch write (index.html + scene.html inside the same JS tick) now yields two SSE file-change events, one per receipt — the per-file-consumer invariant the receipts path always assumed is restored. The Set dedups within a burst (three back-to-back events on the same file collapse to one call), which is what a debouncer is for. close() at :85 also clears the pending set — no leaked reference.

The test at packages/cli/src/server/fileWatcher.test.ts:47-62 locks the exact behavior: three emits (scene-a, scene-b, scene-a) → after 300 ms → listener.mock.calls === [["scene-a.html"], ["scene-b.html"]]. Direct contract lock on both the per-file emission and the dedup.

Small note on ordering: [...pendingPaths] iterates the Set in insertion order (JS spec), so the first-touched filename in the burst gets its listener call first. Deterministic and matches the test expectation.

What's still deferred from R3

Fine to defer; naming here so it doesn't get lost:

  • readFileSync on the SSE listener runs on the main event loop per debounced event (studioServer.ts:759). With the new per-file emission, the same K-file burst now fires K reads instead of 1 — same-per-file cost, higher aggregate. Studio HTML files are effectively unbounded (embedded inline assets). Worth a follow-up: hash once per debounced burst upstream and pass version into the listener, or add a small LRU keyed on (absPath, mtimeMs, size).
  • SSE catch swallows every read error as deletion (studioServer.ts:758-762). Gate on err.code === "ENOENT" and surface the rest.
  • Rollback writes don't record receipts (files.ts:383-398). A throw mid-batch rolls back N writes as external-looking events. Not a regression, but a wider blast radius on the batch route.
  • findIndex first-match on same-version receipts (fileVersion.ts:44). Deterministic and probably harmless (identical-content concurrent writes are rare), but worth naming in a docstring.

What lands cleanly

  • Pending-paths Set + snapshot-on-flush (fileWatcher.ts:37, 47, 49-58, 85) — the right seam, the right shape.
  • Direct contract-lock test at fileWatcher.test.ts:47-62.
  • Existing behavior preserved: same-file burst still dedups to one call; the Set also serves that purpose without a separate check.

R3 BLOCKER closed. LGTM from my side on this PR now.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R3 re-stamp @ f1de808 — Rames R2 BLOCKER (single project-wide debounce timer emits last-only filename on multi-file bursts) is closed. Fix at packages/cli/src/server/fileWatcher.ts:37 (pendingPaths = new Set<string>() accumulator), :47 (pendingPaths.add(relativePath) on every burst event), :49-58 (single debounce timer flushes by iterating [...pendingPaths] and calling fn(changedPath) once per distinct accumulated path), :82 (clears on close to prevent leaks). Regression test at packages/cli/src/server/fileWatcher.test.ts:52-60 asserts a 3-event burst (scene-a.html, scene-b.html, scene-a.html) produces [["scene-a.html"], ["scene-b.html"]] — one call per distinct path, duplicate collapsed, no last-only truncation. Delta vs R2 head 4d2a48a: 1-commit-ahead, only fileWatcher.ts (+10/-2) and fileWatcher.test.ts (+34/-4). — Via

@miguel-heygen
miguel-heygen merged commit bea32b8 into main Aug 9, 2026
48 checks passed
@miguel-heygen
miguel-heygen deleted the stack/preview-write-receipts branch August 9, 2026 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants