Repository navigation
fix(studio): catch up the preview after missed file changes - #5126
Conversation
Edit accuracy: accurate 2059 (base branch 2059), smooth 1692 of thoseThe gate passes. Quarantined, measured but not gated (0) |
5480cb9 to
d98f309
Compare
d98f309 to
6841120
Compare
There was a problem hiding this comment.
Reviewed exact head 68411203a4e2967bbc6e814b5986ea40a7fe2f2f. The three Before/After images visibly show the initial preview, post-reconnect catch-up, and edits in the replaced folder. Focused existing suites passed (43 Studio coordinator/ownership tests, 18 CLI watcher/server tests), and all 11 required checks passed. Isolated adverse coordinator probes fail at this head; a real-filesystem watcher/server reproduction also exposes a nested-folder gap.
Blocking — reconnect can leave an unsaved source draft with no recovery. Selecting script.js in the Code panel changes editingFile.path without changing the preview’s activeCompPath=index.html (useFileManager.ts:125-153, useEditorSave.ts:141-170). A reconnect/root event is rewritten to index.html (useExternalFileChangeCoordinator.ts:402-407,485-486). If the pending script.js save fails, the failure branch at :315-349 records path=index.html, studioContent=null because it accepts the draft only when its path matches that invented event path; it also skips the failure snapshot. The banner offers only Discard (ExternalFileConflictBanner.tsx:190-206), which discards the actual script.js pending draft while re-reading index.html (useExternalFileChangeCoordinator.ts:497-514). The targeted hook probe expected the recoverable script.js/unsaved content and got index.html/null. Preserve the pending candidate’s real path/content for failed-drain recovery separately from the root-wide reload scope.
Important — root catch-up can leave mounted SDK sessions stale. With no active composition, the root rewrite targets literal index.html, although the master SDK session can own masterCompPath=film.html (useStudioSdkSessions.ts:26-35); its reload bus matches exact paths (useSdkSession.ts:409-414). A focused probe observed zero film.html reloads. Separately, DesignPanelPromoteProvider.tsx:54-55 mounts an SDK session for the selected nested composition’s sourceFile, beside the host session (StudioRightPanels.tsx:190-200); root catch-up emits only the active host path (useExternalFileChangeCoordinator.ts:251-267,402-407), and a second probe observed host.html reloaded but not the nested session. Ensure project-root catch-up reaches every affected mounted SDK session rather than collapsing scope to a fabricated file path.
Nested-folder replacement completeness. Swapping an already-populated .hyperframes/ directory changes the project signature, but the watcher emitted no listener event for that swap; the directory path does not pass its relevant-path classifiers (packages/cli/src/server/fileWatcher.ts:101-105,133-144), and re-arming produces no child manifest events. In a real createStudioServer repro, a preview GET with the old ETag returned stale 304 after the on-disk signature changed (studioServer.ts:426-435). Baseline code also excludes the directory, but I did not run this repro on baseline, so I am not labeling it a newly introduced regression. It remains a gap in the stated nested-folder replacement behavior. Invalidate the cached signature on relevant directory replacement, independently of whether an SSE file-content event is appropriate.
Verdict: REQUEST CHANGES for the reproduced draft-recovery and SDK catch-up failures. The passing CI and screenshots do not exercise those states.
— Review by tai (pr-review)
…ter a reconnect A reconnect or replaced folder is a project-wide change: every open SDK session reloads, a failed save is recorded against the file the draft belongs to, and two queued changes to different files merge into one project-wide reload.
Replacing a populated folder such as .hyperframes/ now reaches the signature cache through its files, so the shared signature rule stays as on main and Studio's own writes inside .hyperframes/ still invalidate nothing.
|
Thanks, all three addressed at
One related gap, also present on main, is listed under Limits and gets its own PR: a failed draft's recovery copy is not retired when you keep typing in that file and it saves on its own. |
There was a problem hiding this comment.
Re-reviewed exact head a709428ba8989fe1dad5bba4eeb97fdaa9f5820e. The original script.js draft-loss path and straightforward project-wide SDK reload are fixed: I exercised the Code-panel failed save through recovery/Discard and mounted both the master and nested SDK sessions. The focused Studio and CLI suites pass, and all 11 required checks are green. The held-change settlement still has two failing branches, alongside one pre-existing replacement limit:
Blocking — a failed reconnect can still lose its project-wide SDK catch-up. In useExternalFileChangeCoordinator.ts:313-320, the clean-after-blocked branch correctly merges the held payload with the next event into owed, but sends the next event's path and affectsPreview to the accepted-change/reload callbacks instead of the merged scope. A reconnect with an unsaved script.js draft first fails to drain and holds a path: "." reload. When a later scenes/nested.html event with affectsPreview: false retries successfully, the banner clears, but preview does not reload and the mounted film.html master SDK stays stale; only the nested SDK reopens. A two-session integration probe observed FILM,NESTED,NESTED opens instead of FILM,FILM,NESTED,NESTED, and zero preview reloads instead of one. Deliver the merged owed path and preview scope when clearing the block so the originally owed root reload reaches every session.
Also check Keep Studio after a held conflict. A conflict on film.html followed by an external scenes/nested.html event merges the blocked payload to path: "." with both affected compositions. Clicking Keep Studio at useExternalFileChangeCoordinator.ts:572-577 calls reloadAcceptedGeneration(conflict.filePath) and never passes the merged payload to onAcceptedPersistedFileChange: the master SDK reopens, but the nested SDK stays stale and the owed thumbnail update is lost. The same two-session integration probe fails this path. Fixing only the clean-after-blocked branch would leave this second held-scope failure.
Folder-replacement completeness — removal of a manifest still leaves a stale ETag. The new moved-in traversal reports present files, so replacing .hyperframes/ with another populated directory fixes the case where the replacement still contains studio-motion.json. It cannot report a manifest that existed only in the old directory. On a real Linux createStudioServer, I replaced .hyperframes/ containing a valid motion manifest with a populated directory containing only arbitrary.txt. After 600 ms the watcher emitted no paths, the fresh project signature changed, and a conditional preview GET with the old ETag returned stale 304; an unconditional GET rendered the new no-motion HTML but still advertised the old ETag. The watcher remains live for later writes. This is the pre-existing replacement gap noted in my first review, not a newly introduced regression and not an additional reason for REQUEST CHANGES. The new traversal does not fully resolve it: either invalidate the cached signature when a manifest directory is replaced without recognized child files (fileWatcher.ts:80,99-107; studioServer.ts:427-435), or disclose this remaining limit and track it separately.
Verdict: REQUEST CHANGES for the reproduced held-reconnect SDK failure. I have not approved this head or merged it.
— Review by tai (pr-review)
A clean save, Use external and Keep Studio now all deliver the merged scope a blocked change owes, and a moved-in .hyperframes folder reports its manifests so a dropped one still clears the cached signature.
|
Thanks, both held-scope failures fixed at
A restored draft's banner no longer adds a project-wide reload to an unrelated clean save. Two pre-existing banner gaps (a recovery copy outliving a later save of the same file; a restored banner replacing a live one) are listed under Limits and get their own PR. |
There was a problem hiding this comment.
Review at 80640b0ad438f8149f9934286f8ceae70f7fe9ca — requesting changes for one remaining held-reconnect ordering case.
The prior single-event settlement paths are fixed: a later clean save and Keep Studio after a conflict now deliver the held project-wide scope when the failed/conflict state has committed. The Linux folder-replacement path also clears the old cached signature when a moved-in .hyperframes/ no longer contains its manifest: the real server returned 200 with a new ETag instead of stale 304. Focused existing coordinator tests passed 77/77 and real-filesystem watcher/server tests 10/10; all 11 required checks pass.
A queued second event can drain before React commits the first failed reconnect's held state. packages/studio/src/hooks/useExternalFileChangeCoordinator.ts:355-363 schedules setBlocked({status:'failed', payload: rootReconnect}), but blockedRef.current is only refreshed on render (:175-179). The loop at :401-410 immediately starts the already queued next event. If that nested-file event saves cleanly before the render, :293-323 reads blockedRef.current === null, clears the block and delivers only the nested path with affectsPreview:false, not the root reload it owes. An SSE EventTarget probe with injected drain outcomes reproduces the ordering: reconnect open queues {path:'.'}, a scenes/nested.html event queues behind its failed save, and an immediately clean next drain runs without an intervening React commit. Production has that fast path when a tracked Studio edit rejects, its pending entry is removed, and the next drain finds no edits left to await. In the probe the mounted nested SDK reopens but the master SDK does not; preview reload count is 0 rather than 1, and thumbnail refresh receives only the nested composition rather than the project-wide scope. The focused adversarial test fails while the existing suite is green. Please preserve the held payload synchronously across the drain loop (or ensure the first failure commits before the next drain) and pin this queued-before-commit sequence in a test. This is the same project-wide catch-up invariant as my previous review, not an unrelated issue.
The non-required dense-short viewport gate is red on this head. It measures timeline scrolling rather than any file-change path; a previously passing head also failed its first attempt at a similar p95 and passed on retry. I cannot attribute that performance signal to this diff and did not use it as the finding. Permanent removal of .hyperframes/ without replacement still has the pre-existing stale-ETag limit; the replacement case claimed here is fixed.
— tai
|
Fixed the queued-before-commit ordering in 68ff783. The shared blocked-state transition now merges and commits its held payload synchronously, then publishes that exact value to React. Rendering no longer overwrites the coordinator state. The new SSE regression queues the failed reconnect and clean nested event in one batch, proves the second drain precedes the banner commit, and asserts one project-wide acceptance and reload. It fails at 80640b0 with zero Preview reloads; all 48 coordinator tests pass three consecutive runs with the fix. Before/After evidence is refreshed from the rebuilt head. |
terencecho
left a comment
There was a problem hiding this comment.
Review at 68ff7835849f16457683f2c0e0cad6bdb788c1d7 — approved.
The queued-before-commit reconnect race from my prior review is fixed. setBlocked now merges and writes the held state to blockedRef.current synchronously before scheduling React state, so an immediately queued clean drain reads and delivers the owed project-wide scope even when the UI has not rendered the block. The new regression test pins that ordering (it sees rendered blocked === null at the second drain) and verifies the root preview, SDK, thumbnail, and tree refresh. I also reran the actual SSE + tracked pending-edit rejection reproduction that failed on the previous head; it now passes. Earlier failed-then-clean and Keep Studio settlement paths remain green (87/87 focused Studio tests including scratch probes and source-save tests).
The moved-in .hyperframes/ watcher and real-server ETag replacement cases still pass (7/7 watcher and 3/3 server tests). No other production code changed from the prior reviewed head. Required checks currently pass. The non-required viewport gate remains red; its scrolling benchmark does not exercise this coordinator change, so I do not attribute that signal to this diff.
— tai
What
Studio's preview catches up on file changes it missed while its connection to the preview server was down, and keeps following the project after its folder is replaced on Linux.
Why
Studio learns about outside edits (an agent, an editor,
git checkout) from a server event stream. When that stream dropped and reconnected, every change made in between was lost: the preview, thumbnails and file tree kept showing the old files until the next unrelated change. On Linux, replacing the project folder (a checkout that swaps the directory, a sync tool,mv new film) left the watcher on the old inode, so later edits never reached Studio at all.How
script.jsopen in the Code panel), stores the recovery copy under that file, and Discard re-reads that file. A conflict whose recovery copy cannot be stored names the conflicting file the same way. The recovery copy is deleted only when that same file later saves cleanly: a clean save of another file (the single-file save queue moves on when you switch files) or a draft restored after a page reload keeps its copy..hyperframes/swapped in) reports the files it brings, which have no event of their own, so the cached signature clears through the manifest files. A moved-in.hyperframes/also reports both manifest paths, so a manifest the new folder no longer has clears it too. The shared rule for which paths affect the signature is unchanged, so Studio's own writes inside.hyperframes/still invalidate nothing. An unwatchable parent does not disable normal watching, and closing the watcher closes the parent watch too.Limits
Permanently removing
.hyperframes/without replacing it retains the pre-existing cached-signature limit; this PR proves replacement, including a replacement that drops a manifest.A change that is mid-drain (not queued) when you switch compositions still skips its file-tree and thumbnail refresh, as on main.
A catch-up queued just before a composition switch can reload the newly selected preview one extra time.
Two outside changes to different files that queue behind one drain reload every open session, not just the two files' sessions.
A failed draft's recovery copy is not retired when you keep typing in that file and it then saves on its own; after a page reload the older draft is offered again. This also happens on main (it deleted another file's copy instead) and gets its own follow-up PR: retiring the copy at the editor's own successful save needs to know the editor buffer was not replaced since the failure.
A recovery banner restored from storage can replace a live failed-save banner when you open that file, and then the live one's held reload is not delivered. Banner replacement is unchanged from main and goes into the same follow-up PR.
In a project with no composition and no open file, a failed catch-up has no file to name, so Discard cannot re-read one.
The stream sends nothing while idle, so a proxy that drops idle connections causes a catch-up reload on each reconnect even when nothing changed.
Validation
Studio changes at
6d26816aon miga, each affected test file three times in a row, all green: Studio 105 across change ownership, coordinator, SDK session lifecycle, reload bus and manual-edit parsing; CLI watcher units, watcher on the real filesystem and folder replacement through the real server 19. Studio, studio-server and CLI typecheck, changed-file lint and formatting clean. The moved-in-folder watcher change and the rename that follow are gated by CI on this head (the shared test box was overloaded).Each new test fails when its source file is put back to
68411203:68411203: 11 tests fail, including thescript.jsdraft kept on a failed reconnect, every session reloaded with no composition selected, a reconnect kept project-wide behind a later file change, the restored draft's snapshot kept, and the conflicting file named.68411203: the queued-reconnect test and the two-file snapshot test fail.68411203: a session onfilm.htmldoes not reopen on a project-wide change.68411203: the real-server test gets the stale ETag after a populated.hyperframes/is swapped in.Earlier validation of the reconnect and folder-replacement behaviour at
5480cb98andd98f3090(fourteen mutations, full Studio hooks suite 1535, Fallow, main-deletions guard) still covers the unchanged parts.Queued-event validation
At
68ff7835, the SSE regression queues a failed reconnect and a clean nested-file event in one React batch. It proves the second drain runs before the failure banner commits, then checks exactly one project-wide acceptance, Preview reload, SDK reload and tree refresh. With the coordinator restored to80640b0a, the test fails because Preview reloads zero times. With the fix restored, all 48 coordinator tests pass three consecutive runs with none skipped. Ownership (4) and SDK lifecycle (26) tests pass too. Studio typecheck, changed-file lint and formatting, comment checks and delta Fallow audit pass on the test host.Review findings
Independent delta review at
68ff7835: no blockers or major findings. The exact-head devbox capture also passes reconnect, folder replacement and a later write without reloading Studio’s document.Tai’s queued-before-commit finding at
80640b0ais fixed in68ff7835: the serial drain reads the coordinator’s synchronously committed held state instead of an older React snapshot.Independent adversarial review at
6d26816a: 1 blocker, 0 major, 5 minor.Blocker, fixed in
0783f645: a clean save of another file deleted the failed draft's recovery copy. Test: a failedscript.jsdraft keeps its copy whenstyle.cssthen saves cleanly (removing the guard fails exactly this test).Minor: two unrelated outside changes queued behind one drain reload every open session (listed under Limits).
Minor: the no-composition, no-open-file failure has no file for Discard (listed under Limits).
Minor: a draft restored after reload still loses its banner, not its copy, on an unrelated clean save; unchanged from before this PR, and the banner returns when that file is opened.
Delta review at
0783f645: 0 blocker, 1 major, 2 minor. Major: the recovery copy outlives a later successful save of the same file; reproduced on68411203too, so it is listed under Limits with a follow-up PR. Minor: CI at0783f645was red only on formatting, fixed ina709428b, where every check including the Windows lanes passes. Minor: a moved-in folder's files are each read once per open tab, as when the same files are copied in one at a time.Moved-in folder announce: excluded folders and temp files are filtered before any listener, subfolders are walked, and the per-file server read matches copying the same files in one by one.
Tai's re-review at
a709428b(fixed ina1b37024): a held reconnect's project-wide reload was dropped when a later change saved cleanly, and Keep Studio after a held conflict reloaded only the conflicting file; both now deliver the merged held scope through one helper. Tests fail ona709428band pass three runs in a row. The replaced.hyperframes/without a manifest now clears the cached signature too (test fails ona709428bwith a stale 304).Delta review at
a1b37024: CI red on lint (two stale hook dependencies), Fallow (watchDirectorycomplexity, split into a walk helper) and the comment ratchet, all fixed in80640b0a; minor: a draft restored after reload no longer adds a project-wide reload to an unrelated clean save (test fails ona1b37024).Before
The captures show the reconnect and folder-replacement flows, which this revision does not change visually; the new behaviour is in failure states (a failed save during catch-up) and in which sessions reload.
Main at
5fad52f2, real Chrome against a builthyperframes preview. The scene was edited to "Caught up after reconnect" while Studio's event stream was down. After the stream reconnects, the preview still shows the old text, and the check times out after 90 seconds.After
The same run rebuilt from
68ff7835. After reconnecting, the preview shows the edit made while the stream was down, without a page reload.The same run then replaces the project folder and edits the new one; the preview follows the new folder.