fix(studio): drain pending edits before reload - #2989
Conversation
44bee4c to
f52398c
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 1d66603e.
Foundation PR (1/5 per the body) for typed drain + recovery across Code, DOM queue, focused-field blur, and write-token identity. Design shape is right: discriminated {status:"clean"|"conflict"|"failed"} return across three drain APIs, focused-field blur added to flushStudioPendingEdits to commit pending debounced work before the drain, and a module-level write-token map so downstream can distinguish own writes from external ones.
Three concerns inline (all 🟠/🟡, no blocker):
- Zero app consumers at merge time — write-token API +
EditorSaveHandle's new drain methods are exposed but no runtime callsite invokes them. Expected for a 1/5 stack, but recommend doc-comments naming the downstream PR that will wire each surface so 1/5-alone-merged doesn't leave orphaned scaffolding. - 10s write-token TTL may be tight for slow-network / large-payload Studio writes — a 12s POST would let its own echo classify as external. Consider tying TTL to the save-timeout window or refreshing on request completion.
- Blur → single-microtask wait may miss
useEffect-driven pending-edit registrations under React 18 batching. Probably fine for current call sites but worth confirming.
Verified — no fix needed: the concurrent-Code-save race path Miguel flagged in the delegation message. Re-typing during an in-flight persist cancels the rAF, launches a fresh persistCandidate on the new content, and saveProjectFilesWithHistory's shared coordinator serializes the writes. The inFlightRef === task guard in persistCandidate's .finally correctly protects against overwriting a newer in-flight task's ref cleanup by a stale older task.
Nits (body-only):
- The
React blur handlers enqueue their save synchronously, while state-driven registrations can land in the next microtask.comment atstudioPendingEdits.ts:36-37is textbookponytail:territory — non-obvious timing invariant that future contributors will otherwise remove as "unnecessary microtask await." Consider prefixing with// ponytail:per the HF idiom. - Three structurally identical drain-result types (
EditorSaveDrainResult/DomEditSaveDrainResult/StudioPendingEditsDrainResult) could consolidate totype StudioSaveDrainResult<E = unknown> = ...for uniform downstream handling. Not required — foundation-shape is fine — but a compose-site alias would help stack PRs 2-5 avoid triple-narrowing. - Test coverage adds happy + failure paths for each new drain, plus the join-in-flight scenario for
useEditorSave. Missing:flushPendingSaveconflict branch (asserts{status:"conflict"}), write-token TTL expiry, anddiscardPendingSave(unused in this PR — expected to be exercised in stack 2/5).
CI note: the Tests on windows-latest > useStudioTestHooks.test.tsx > timeline performance fixture failure is a 5s timeout on the 50k keyframe fixture generation, with a preceding happy-dom network abort trying to fetch Google Fonts CSS. Classic flake pattern, unrelated to useEditorSave / drain logic. Magi noted the shard is rerunning.
Ready from my side, leaving as COMMENTED.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Read the diff at 1d66603e and cross-referenced the drain machinery against the existing serializeStudioFileMutations coordinator. Foundation PR (1/5 in the external-change stack) reads clean.
Concurrent Code-save race — VERIFIED
packages/studio/src/utils/studioFileMutationCoordinator.ts maintains a WeakMap<writer, Map<path, Promise>>. saveProjectFilesWithHistory (packages/studio/src/utils/studioFileHistory.ts:61) wraps the entire readFile → writeFile → recordEdit sequence inside serializeStudioFileMutations, so concurrent Code saves to the same file serialize FIFO with the read-modify-write inside the critical section. No stale-clone problem — Magi's claim holds.
Drain internals I traced through:
useEditorSave.flushPendingSave— cancels the rAF, joins the in-flight task via identity (candidate === inFlightCandidateRef.current), or fires a freshpersistCandidate..finallyusesinFlightRef.current === taskclosure-capture to avoid stomping a newer in-flight save. Correct.pendingCandidateRefclear-on-settle guards against clearing a newer candidate (if (pendingCandidateRef.current === candidate)). Correct.- Write-token echo (
markStudioWriteToken/consumeStudioWriteToken) is string-keyed, TTL-pruned on every mark/consume — bounded memory.
Observations for the downstream stacks (non-blocking):
- P2 —
flushStudioPendingEditsusesresults.find(status === "rejected"), so first-encountered rejection wins. If a batch contains both aStudioFileConflictErrorand a generic failure, the reported severity depends on Set/promise ordering, not severity. Consider two passes (find(reason instanceof StudioFileConflictError) ?? find(...)) so conflict beats failed for the reload-decision path. - P2 —
DomEditSaveQueue.drainErroris set on every rejection but only cleared onreset(). A rejected save followed by successful saves leaveswaitForIdle()reporting{status: "failed", error: staleError}. Defensible ("since last reset, at least one save failed") but worth documenting or tying to breaker state. - P2 —
active.blur() + await Promise.resolve()catches synchronous blur handlers (as tested). React state-driven blur→setState→useEffect→trackStudioPendingEditmay not fit in a single microtask tick. The while-loop mitigates because iterations continue draining, but only for promises that made it intopendingEditPromisesbefore dispatch. Follow-up: considerawait flushSyncor a second tick. - nit —
useEditorSave.flushPendingSave/getPendingCandidate/discardPendingSaveandmarkStudioWriteToken/consumeStudioWriteTokenhave no production caller yet (grep acrosspackages/). Expected for foundation 1/5 — wiring lives in later stacks. Just flagging so reviewers of stacks 2-5 know these entry points arrived here. - nit — 10s write-token TTL: if a real API write takes >10s (rare, but slow-server case), the token is evicted and the receipt gets treated as external. Would tighten if the generator is deterministic; if UUID-v4, low collision risk.
Tests (16 focused):
- Cover individual queues + typed drain shapes (clean/conflict/failed).
- Do NOT cover mixed conflict+failed in one drain.
- Do NOT cover the race between Code editor rAF-buffered save and DOM edit queue draining concurrently — but
serializeStudioFileMutationshas its own coverage.
CI (1d66603e, run 30880080197): Typecheck / Lint / Format / Fallow audit / Producer unit + integration / Studio load smoke / Studio timeline viewport gate / Windows Tests / 8 of 9 regression shards — all green. Shard-4 (style-16-prod style-4-prod plan-v2-partial-color audio-mux-parity subtitles ...) still in_progress ~2h in — visual-regression shard, orthogonal to the Studio drain code paths.
Approving on the drain-correctness axis. Follow-ups above can land in stacks 2-5 or as separate polish PRs.
— Review by Via
- prioritize conflicts and clear recovered DOM queue errors - cover delayed blur effects and missing drain branches - document stacked consumers and extend write-token retention
Addressed in Validation: oxfmt/oxlint and |
vanceingalls
left a comment
There was a problem hiding this comment.
R2 delta re-review at ac89e2d1 (vs R1 head 1d66603e)
All three P2s and both nits from R1 land cleanly, with driving tests for each branch.
F1 — flush conflict-priority — FIXED. flushStudioPendingEdits now iterates all rejected results per batch and returns conflict before falling back to firstFailure (studioPendingEdits.ts:36-58). Test prioritizes a conflict when pending edits fail with mixed errors drives the mixed-error path against two listeners — one failing, one conflicting — and asserts conflict wins.
F2 — DomEditSaveQueue drainError recovery — FIXED. run() now clears drainError = null alongside consecutiveFailures = 0 when a save succeeds and the breaker is closed (domEditSaveQueue.ts:66-71). Test clears a stale drain failure after a successful save exercises the failure→success sequence and asserts waitForIdle() returns { status: "clean" }.
F3 — post-blur React-effect capture — FIXED. flushStudioPendingEdits now awaits both a microtask and one macrotask boundary (waitForPostBlurEffects = setTimeout(0)) after active.blur(), and its comment names the two commit paths (studioPendingEdits.ts:34-39). Test waits for a post-blur effect to register its pending edit listener uses a blur handler whose setTimeout(0) registers addStudioPendingEditFlushListener — the flush must observe that listener, and it does.
Nit-1 — no-caller foundation piece — SKIPPED (documented). JSDoc on EditorSaveHandle.{getPendingCandidate,flushPendingSave,discardPendingSave} and on the token helpers now cites the downstream consumers (#2990, #2991, #2993). Fine as a stack-context anchor.
Nit-2 — write-token TTL — FIXED. WRITE_TOKEN_TTL_MS is now 5 * 60_000 with a header comment on why (studioFileVersion.ts:1-3). Test keeps a token through a slow write and expires abandoned identity state proves consume-at-61s succeeds and consume-at-301s prunes.
Shared type — DONE. New StudioSaveDrainResult<Failure = unknown> in studioSaveDiagnostics.ts:59-62. EditorSaveDrainResult / DomEditSaveDrainResult / StudioPendingEditsDrainResult collapse to aliases of it. No ad-hoc union left.
New (non-blocking) — Fallow audit is red at ac89e2d. Eight minor findings: seven fallow/code-duplication in useEditorSave.test.tsx (the new Probe/captured.handle/root scaffolding is copy-pasted across the two new tests + the pre-existing one — a small test helper would eliminate it), and one fallow/high-cognitive-complexity at flushStudioPendingEdits:30 (16 vs threshold 15) — a direct consequence of the F1 conflict-priority loop. Both are cosmetic against the correctness win. Flagging for follow-up, not blocking merge.
Signed-off by Via.
— Review by Via
- share the editor-save hook harness across drain regressions - extract settled failure inspection from the drain loop
|
Follow-up at d066cbe: the fresh Fallow audit exposed duplicated hook mounts and a one-point cognitive-complexity overage. I extracted the shared test harness and settled-failure inspection without changing drain behavior or assertions. Validation:
Hosted CI is running on this exact head. |
vanceingalls
left a comment
There was a problem hiding this comment.
R3 exact-head verify at d066cbe7 (delta from R2 ac89e2d1).
Delta scope (2 files)
packages/studio/src/hooks/useEditorSave.test.tsx— purely extractive:mountEditorSave(writeProjectFile)helper folds 4 duplicatedProbe/createRoot/render/unmountblocks into one. Every test's hook args, ordering, and assertions are byte-preserved.packages/studio/src/utils/studioPendingEdits.ts—inspectDrainFailures(results)extracted from the drain loop. Complexity delta only.
Drain-semantics equivalence (F1 / F2 / F3)
- F1 conflict-first classification: preserved. Helper linearly scans each batch's settled results — returns
{conflict}on the firstStudioFileConflictError, else{firstFailure}at first non-conflict rejection. Same "conflict wins in batch" order as before. Cross-batch first-failure preservation retained viafirstFailure ??= batchFailures.firstFailureat the loop level. Final result payload is identical (firstFailure.reasonafter extract ==firstFailurebefore extract — the wrapper isPromiseRejectedResultwhose.reasonis what the pre-refactorhasFailurebranch already returned). - F2
DomEditSaveQueue.rundrainError clearing:packages/studio/src/utils/domEditSaveQueue.tsis NOT in the R2→R3 delta. Byte-identical at both HEADs. - F3 blur post-effect capture:
waitForPostBlurEffects+await Promise.resolve(); await waitForPostBlurEffects();unchanged at HEAD; verified via file read atref=d066cbe7.
CI
All completed checks SUCCESS at d066cbe7. Analyze (js/ts), Preflight (lint+format), Preview parity still IN_PROGRESS — approval predicated on those finishing green (they exercise the refactored paths).
Approving.
— Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Delta-reviewed 1d66603e..d066cbe7.
Verified each of my R1 concerns against the current head. All resolved. Three orthogonal improvements went in beyond what I flagged.
| # | R1 concern | R2 verdict | Anchor |
|---|---|---|---|
| I1 | Write-token API has zero app consumers — add docstrings naming the downstream PR | 🟢 RESOLVED | studioFileVersion.ts:12,18,26 name PR #2990/#2991; useEditorSave.ts:38,40,42 name PR #2993 |
| I2 | 10-second write-token TTL is tight for slow-network Studio writes | 🟢 RESOLVED | studioFileVersion.ts:3 — WRITE_TOKEN_TTL_MS = 5 * 60_000, comment names slow-writes + retries + subsequent echo. Test at studioFileVersion.test.ts:53-59 asserts 61s → still valid, 301s → expired |
| I3 | Blur → single-microtask may miss useEffect-driven pending-edit registrations under React 18 | 🟢 RESOLVED | studioPendingEdits.ts:52-53 — Promise.resolve() + waitForPostBlurEffects() (setTimeout-0 task boundary). Test at studioPendingEdits.test.ts:39-63 explicitly registers listener in setTimeout(0) from a blur handler and asserts flush waits for it |
| B1 | ponytail: marker on the timing comment |
🟢 RESOLVED | studioPendingEdits.ts:50 — // ponytail: prefix landed, comment now names the react-effects invariant |
| B2 | Three structurally identical drain-result types — consolidate | 🟢 RESOLVED | studioSaveDiagnostics.ts:59-62 — shared StudioSaveDrainResult<Failure = unknown>; all three sites (studioPendingEdits.ts:9, useEditorSave.ts:34, domEditSaveQueue.ts:27) alias to it |
| B3 | flushPendingSave conflict-branch test |
🟢 RESOLVED | useEditorSave.test.tsx:105-121 preserves conflict details; plus studioPendingEdits.test.ts:103-127 prioritizes conflict over mixed errors |
| B4 | Write-token TTL expiry test | 🟢 RESOLVED | studioFileVersion.test.ts:53-59 covers both hold-through-slow-write and abandoned-token-expiry |
| B5 | discardPendingSave test |
🟢 RESOLVED | useEditorSave.test.tsx:126-140 asserts null candidate, no writeProjectFile, cancelAnimationFrame called |
Orthogonal improvements beyond my findings (all correct):
- Conflict-priority drain semantics (
studioPendingEdits.ts:17-28,64-68) —inspectDrainFailuresscans all rejections; anyStudioFileConflictErrorshort-circuits to{status:"conflict"}. Previously "first rejection wins" would return{status:"failed"}even when a subsequent listener rejected with a conflict. User impact of a conflict is higher (needs external-content reconciliation before commit), so it dominates. ThefirstFailure ??= …accumulator across loop iterations correctly preserves the earliest non-conflict failure across drain rounds. Good. - Recovered-queue error clearing (
domEditSaveQueue.ts:66-70) —drainError = nullon successful save when the breaker is closed. Previously one transient failure would leavewaitForIdle()reporting{status:"failed", error:<stale>}indefinitely. Test atdomEditSaveQueue.test.ts:118-131proves recovery. - Extracted helpers —
waitForPostBlurEffects()andinspectDrainFailures()make the drain sequence testable and self-documenting.
Nothing else surfaced in the delta pass. Foundation ships clean. Ready from my side, leaving as COMMENTED.
* fix(studio): drain pending edits before reload * fix(studio): address drain review feedback (#2989) - prioritize conflicts and clear recovered DOM queue errors - cover delayed blur effects and missing drain branches - document stacked consumers and extend write-token retention * test(studio): satisfy drain audit gate (#2989) - share the editor-save hook harness across drain regressions - extract settled failure inspection from the drain loop * feat(studio): preserve external file conflicts * fix(studio): isolate retry write receipts * test(studio): cover external conflict recovery safety

External-change stack 1/5. Adds typed draining and recovery for Code, DOM, and queued Studio saves before reload. 422 changed lines. Split from #2984.