fix(studio): match the write receipt in dev, so an edit stops reloading the preview - #3206
Conversation
…ng the preview Every edit in the canvas reloaded the preview iframe on the dev server. The write receipt exists to prevent exactly that — Studio marks its own writes so the file-watcher echo can be recognised as its own rather than as somebody editing the file underneath it. The receipt is matched on the file's current bytes as well as its path, so `consumeFileWriteReceipt` takes a version. The dev plugin called it with the path alone, so the version compared against `undefined` and no receipt ever matched: every Studio write looked external, and the preview reloaded. The CLI server, which is what ships, has always passed the version. The plugin now reads the file and passes its version, the same way the CLI server does. Measured on the dev server, driving a real drag, resize and text edit: iframe reloads went from one per edit to none, and the reload trace now reports 'suppressed: own write token' where it previously logged a full external path.
vanceingalls
left a comment
There was a problem hiding this comment.
Findings
Small, tightly-scoped, correct parity fix. The vite plugin now calls consumeFileWriteReceipt(path, version) exactly the same way the CLI server does at packages/cli/src/server/studioServer.ts:759,763 — compute fileContentVersion(readFileSync(...)) first, only consume when version is truthy. Verified against the CLI reference at head. Dev-only fix, prod scope = 0.
Non-blocking observations:
1. readFileSync on every file-change event. Adds a synchronous disk read to the hot path. Composition files are tiny in practice, but a large asset accidentally routed through this handler (e.g. a wired-in binary that trips the chokidar filter) would block the dev server's event loop. Not a real hazard today; noting for future scope creep.
2. Read/consume race window. Chokidar fires → we readFileSync (bytes at time T) → compute version → consumeFileWriteReceipt(path, versionAtT). If Studio writes AGAIN between the change fire and the read, we compute the newer version, and the receipt (queued against the older version) never matches → the older write looks external → spurious reload. Rare — requires a second Studio write to land inside the chokidar-notify latency window — but real. Not blocking; a follow-up could consume against a small ring of recent-version candidates if it ever becomes visible.
3. catch {} swallows more than deletion. The comment says "A deletion has no current bytes to match a write receipt against," but the catch also absorbs permission errors, encoding errors, filesystem races. Semantically fine — all of those correctly fall through to "receipt is null, treat as external, reload." Consider widening the comment to reflect the broader semantics, or narrow the catch to NodeJS.ErrnoException where code === "ENOENT" and re-throw otherwise so a real filesystem problem surfaces in dev.
4. No automated test. Body's test plan is manual observation of hf-reload-debug. A synthetic unit test — stub consumeFileWriteReceipt + fileContentVersion, fire a change with matching + non-matching versions, assert receipt truthy/null — would guard the parity invariant against future drift. Cheap follow-up, not blocking.
Verdict
APPROVE. Diagnosis is exact (the plugin's call signature drifted from the CLI's; expectedVersion was undefined so version never matched), fix is parity with the reference (studioServer.ts:759,763), scope is dev-server only. All 44 required checks green. Nothing blocks merge.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 3e6fed26.
The fix is right and it's exactly the right shape — the dev plugin now mirrors what packages/cli/src/server/studioServer.ts:759 has always done: version = fileContentVersion(readFileSync(absPath, "utf-8")); const receipt = version ? consumeFileWriteReceipt(absPath, version) : null;. Same code, same call order, same deletion-swallows-to-null path. This isn't a bandaid — it converges dev on prod's contract rather than papering over a gap.
consumeFileWriteReceipt(absPath, expectedVersion) at packages/studio-server/src/helpers/fileVersion.ts:36-51 requires entry.version === expectedVersion for a receipt to fire. expectedVersion being undefined on the dev plugin's old call would never equal a stored "sha256:...", so every Studio write looked external. Diagnosis matches the PR body exactly.
The deletion branch is subtle and correct: readFileSync throws on a deleted file → catch sets version = null → the version ? consume(...) : null gate returns null → the change is treated as external, which is exactly what a deletion IS.
Concur with Via's approve. She already flagged the two I would have called out inline — readFileSync on the hot path (her #1), and the read/consume race window if Studio double-writes inside the chokidar-notify latency (her #2). Her #3 (broaden the catch comment to reflect the actual semantics, or narrow to ENOENT and re-throw) is a clean follow-up. Her #4 (no automated test) is worth the ~10-line follow-up she describes.
Small addition: consider extracting the shared helper
Both studioServer.ts:759-763 and the new vite.config.ts:177-193 block are byte-identical logic (try { readFileSync } catch { null } → fileContentVersion → consumeFileWriteReceipt). Cheap DRY: publish something like consumeFileWriteReceiptForCurrentBytes(absPath): FileWriteReceipt | null from packages/studio-server/src/helpers/fileVersion.ts and swap both call sites. Not a blocker — the immediate goal is dev-matches-prod — but the third caller that wires this up will otherwise start the divergence clock again. Nit.
🔮 Forward-looking (D2 lens) — stack anticipation notes
Miguel confirmed this is 1/5 in the rich-text stack. Recording things I want to check when D2+ lands so future reviews can grep for them:
FL1 — Byte-identical writes duplicate receipts. recordFileWriteReceipt at fileVersion.ts:26-33 appends to an array; two writes producing byte-identical content (e.g., toggling bold on and off in rapid succession, both writing the same bytes back) leave two receipts with the same version string in the map. consume's findIndex splices out the first match, but a subsequent legitimate external write of those same bytes would match the still-lingering receipt and be silenced. The sha256 collision path is not the risk — the risk is Studio itself writing the same bytes twice within TTL, which rich-text UX makes routine (undo/redo, format-toggle, cursor moves that trigger a save with no actual change). Manifests as: an external editor makes a change that happens to match a byte-state Studio recently wrote, and the reload doesn't fire. Check on D2+: does the rich-text write path debounce byte-identical POSTs at the Studio client, or does every save-shaped event hit the server even when bytes are unchanged?
FL2 — 10-second receipt TTL vs rich-text write batching. RECEIPT_TTL_MS = 10_000 in fileVersion.ts:13. If any D2+ change introduces a debounced/batched write pattern (buffer edits, flush every N seconds; save-on-blur; long autosave), and the flush interval approaches or crosses the TTL boundary, receipts expire before the watcher fire lands → false-positive reload. Concrete audit: any new setTimeout/debounce around the write path with delay > ~5s. Check on D2+: does the rich-text write path introduce any delay > ~2s between edit-event and file-write?
Test plan claims verify
- "Before: a drag logged
file-change… thenreload, thenrefreshPlayer" and "After: …suppressed: own write token. Iframe reloads are zero across drag, resize and an inline text edit." — mechanism is small enough that manual verification is fine. See Via's #4 for the follow-up test. - "Full studio suite (3727), format and lint green." — verifiable from CI.
What I didn't verify
- Did not run the dev server locally to reproduce the before/after. Trusting Miguel's manual repro (and Via's independent read).
- Did not audit whether ANY dev-mode-only call site of
consumeFileWriteReceiptexists other than thisvite.config.tsblock.grepshows three call sites (cli/studioServer.ts:763,studio-server/index.ts,studio/vite.config.ts:196); if a fourth existed, it would be silently under the same bug.
LGTM. Stamp routing per standing rule.
What
Editing anything in the canvas on the dev server reloaded the preview iframe. It no longer does.
Why
The write receipt exists to prevent exactly this: Studio marks its own writes so the file-watcher echo can be told apart from somebody editing the file underneath it. The receipt is matched on the file's current bytes as well as its path, so
consumeFileWriteReceipt(absPath, expectedVersion)takes a version.The dev plugin called it with the path alone.
expectedVersionwasundefined, the version comparison never matched, and so every Studio write looked external and reloaded the preview. The CLI server — which is what ships — has always passed the version, so this is dev-server only.How
The plugin reads the file and passes its version, the same way
studioServer.tsdoes, and treats a deletion (no readable bytes) as unmatched.Test plan
Driven on the dev server against a real composition, with
hf-reload-debugon:file-changewith a full external path, thenreload, thenrefreshPlayer, and the iframe navigated — one reload per edit.file-changecarrying the write token, thensuppressed: own write token. Iframe reloads are zero across drag, resize and an inline text edit.Found while chasing a flash after every canvas edit. The other half of that flash was Vite's own HMR full-reloading the page, fixed separately in #3163; with both in, the canvas stops flashing.