[kixelated]

Problem

Autosave wrote over the scene file whenever the studio had a change. An edit made outside the studio (by hand, or by another studio on the same file) was silently lost unless the user picked File > Reload first. README.md documented this as last writer wins.

Approach

Autosave keeps the exact bytes the scene file and its history held when the studio last read or wrote them, and treats any other content there as an outside edit. It compares content, not timestamps, so the studio's own atomic rename never reads as one.

• It looks every second (POLL), before each write, and again right before each file's rename (after the temporary file is synced). A file the studio last found missing is created with persist_noclobber, so one that appears is never replaced.
• With nothing unsaved (the collection and history serialize to what the files hold), it takes the outside edit through the reload path, history included. The history counts on its own, since another studio writes it after the scene file. The UI reports reloaded <path> and drops a selected item and any drag, as File > Reload does.
• With unsaved edits, or an outside edit that does not load (bad TOML, bad history, or a canvas change the studio refuses), it holds a Conflict and writes nothing. A banner under the menu offers Keep the studio's (writes both files over whatever is there) or Take the file's (File > Reload). A held edit is re-evaluated every poll, so fixing a broken file, or undoing back to the saved state, resolves it.
• A poll that cannot read the files is retried as a save, so the error shows in the status bar until a save succeeds.
• A scene file or history deleted outside the studio holds nothing to keep, so it is written again within a poll, or by the save on quit, without waiting for an edit.
• A rename that succeeded before its directory sync failed counts as what the file holds, so it never reads as an outside edit, and the file is written again until the sync succeeds. Files to write again are tracked apart from what they hold, so a deletion or failed sync never makes the studio look unsaved.
• The UI lets go of a selected item and a preview drag under every state lock that uses them (copy, paste, preview, Items, Filters) when the autosave reloaded since, so neither lands on another item at the same index.
• The autosave reads and replaces the engine state through a small autosave::Document trait, so the "nothing unsaved" check and the replace happen under one state lock and no edit can slip in between.
• Guided setup's create-only-if-missing (from main) now feeds the same path: a scene file another program creates while the studio has none is an outside edit, not a "restart the studio" error.

Impact

• An outside edit to the scene file or its history is reloaded automatically when the studio has nothing unsaved.
• When both changed, or the change cannot be loaded, autosave pauses and a banner asks which version to keep. Quitting before picking saves nothing and exits with an error saying why.
• File > Reload still replaces unsaved edits. Its status message now names the file.
• README.md drops the last-writer-wins note and names the remaining window: a change saved between the studio's last look and its rename.

Alternatives

• A file watcher (notify) instead of a one-second poll would react instantly, but it adds a dependency and per-platform quirks around atomic-rename saves. Reading two small TOML files once a second costs nothing measurable.
• Checking on window focus, as editors do, misses edits while the studio runs in the background. The live program should still follow a hand edit then.
• An advisory lock between studios (std::fs::File::try_lock on a .show.toml.lock beside the file) would close the studio-to-studio half of the window. But that window is the microseconds between a read and a rename. The lock costs a permanent lock file beside every scene file, plus a new failure mode on filesystems without flock (some SMB/NFS mounts). The plan calls the lock worth it only if it works everywhere and closes the window. Those conditions are necessary, not sufficient, and I judged it not worth its weight.
• A modal dialog for the conflict would block switching scenes mid-stream, so the prompt is a non-blocking banner.

Choices made without asking

• The prompt is a banner under the menu, not a modal, so a live show stays controllable while the user decides.
• No advisory lock (see Alternatives).
• A scene file deleted outside the studio holds nothing to keep: the studio writes it again, rather than asking or running guided setup on the next launch.
• "Unsaved" includes a history-only change (an edit undone before it was written). Such a studio prompts rather than dropping its redo entry.
• "Keep the studio's" overwrites whatever the file holds when clicked, even if it changed again after the banner appeared.

Follow-ups

None.

Review

• Reviewer: Codex (quest review), gpt-6.1-sol, medium effort.
• Round 1 on 12b4868, 2 findings, both fixed in 367903a:
  • Observe history changes that arrive after the scene file: fixed. Both files' bytes are tracked, and a history-only change counts.
  • Surface polling failures that do not create a conflict: fixed. A failed poll is retried as a save and shows in the status bar.
• Round 2 on 5aa1baa (including the merge of guided setup's no-clobber create): no findings.
• GitHub CI then failed holds_an_outside_edit_against_unsaved_edits on Linux. The root cause was not Linux-specific (it reproduced in 10 of 40 runs on macOS). Each failed poll called Edits::record, which delayed the pending save by another quiet second, until the five-second limit. Whether the test hit this depended on which tokio::select! branch won when both fired at once. Fixed in 3d68ef7 with Edits::retry, which never delays a pending save. No failures in 60 runs after the fix.
• Round 3 on a545afa (the fix plus a conflict-free merge of main: snapping feat(studio): drag items in the preview, snapping to the canvas, items, spacing, and a grid #15, effects feat(scene): video effects: color filters, blur, drop shadow, keys, LUTs, masks, crop, and blend modes #17): no findings.
• Online findings on c611313/a545afa, fixed in 0276f02 (after a conflict-free merge of main: failover feat(studio): publish to a failover relay under the same epoch #19, guests feat(studio): guests join a moq-room from a web page and appear as sources #10):
  • OpenAI: a rename followed by a failed directory sync left a permanent false conflict. Fixed: the rename is recorded before the sync, and the file is written again until a sync succeeds. remembers_a_rename_it_could_not_sync reproduces it with a directory that cannot be opened (fails before the fix).
  • OpenAI: a reload between follow_reloads and the preview's lock could apply a stale drag to another item. Fixed (generalized in a9bc485).
  • pullfrog: a scene file deleted outside the studio was never recreated. Fixed: a deleted scene file or history is written again within a poll, or by the save on quit. writes_a_deleted_file_again (fails before the fix).
  • pullfrog: pin that Edits::retry never delays a waiting save. Added a_retry_does_not_put_off_a_waiting_save.
• New session (the branch's review session restarted), round 1 on 0276f02, 2 findings, both fixed in a9bc485:
  • The selection, not only the drag, could outlive a reload between two locks (paste, Filters). Fixed: follow_reloads(&State) runs under every lock that resolves the selected item or the drag.
  • Clearing written.history to force a rewrite of a deleted history made the studio look unsaved, so an outside edit beside it was held, not taken. Fixed: a separate Rewrite marks files to write again. Covered in writes_a_deleted_file_again.
• Round 1 of a further new session on a9bc485 (with the responses above): no findings.
• 291e130 is a conflict-free merge of main (docs(agents): rely on quest context for shared rules #26, docs and flake only).
• Online OpenAI findings on 0276f02 (the two above, already fixed in a9bc485) and on a9bc485: taking an outside edit to the history alone reset a pending rewrite of a scene file whose sync failed, so the next save could report success unsynced. Fixed in 6219818: take() keeps a file's rewrite while that file still holds the same bytes. Covered in remembers_a_rename_it_could_not_sync (fails without the fix).
• Round 2 on 6219818 (with responses): no findings.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 79027549-5290-4aad-9b35-a5695af398d9

Commits

Reviewing files that changed from the base of the PR and between 0904006 and 6219818.

Files selected for processing: 6

• README.md
• quest/m0/README.md
• quest/m0/outside-edits.md
• rs/moq-studio/src/autosave.rs
• rs/moq-studio/src/engine.rs
• rs/moq-studio/src/ui.rs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: c611313.

• [P2] Remember a rename that succeeded before directory sync failed (rs/moq-studio/src/autosave.rs:284–290, 535–539). The helper can replace the scene file and then fail opening or syncing its directory. The error returns before written.disk is updated, so the next retry mistakes the studio's own new bytes for an outside edit; because the document differs from the old snapshot it becomes a persistent conflict even after the I/O problem is fixed. The history write has the same issue. Track whether replacement committed separately from the durability error, and test a post-rename sync failure followed by recovery.
• [P2] Cancel stale drags under the same lock as the reload check (rs/moq-studio/src/ui.rs:114–115, 324–335, 588–614). follow_reloads() checks the counter and releases the state lock before the preview later reacquires it. If an automatic reload lands between those operations while a drag is active, arrange can use the old drag's index and starting rectangle against the new collection. If that slot now names another item, the user's next drag step moves that different item before the next frame clears the drag. Compare the reload generation while holding the state lock used to apply the drag, and cover an interleaved reload and drag.

Static GitHub-only review; I did not run the tests or exercise the UI.

[pullfrog]

Important

A scene file deleted outside the studio is never treated as an outside edit. With nothing unsaved the poll clears the conflict and the error, no write is due, and the save on quit writes nothing, so the file stays gone and the next launch runs guided setup. That loses the show from disk in a way the PR body's "the next write recreates it" does not cover, so it should be fixed before merge.

Reviewed all 5 commits and 6 files at c611313. The design holds up: content comparison instead of timestamps, taking an outside edit only when nothing is unsaved, a non-blocking banner, and a second check right before each rename. One new finding below. The post-rename directory sync failure is already reported in the earlier review on this PR, so I do not repeat it.

Deletion is not an outside edit (autosave.rs:354). outside() requires now.scene.is_some(), so a missing scene file never reaches look(). With nothing unsaved, look() clears conflict and error, write() has nothing to do because written.collection still matches, and main.rs's save on quit also writes nothing. The file stays deleted and main.rs:85 sends the next launch to guided setup. The PR body says a deleted file is recreated by "the studio's next write", but no write is scheduled when nothing is unsaved. Treat a scene file that was present in written.disk and is now missing as an outside edit and either force a rewrite or hold a conflict, and add a test that deletes the file after a save and then quits.

Checked and not raised: the lock order (written then the document state, with no UI path holding state while calling autosave), conflict recovery through Keep, the refusal path on a canvas change, and the quest-file deletion (it matches the repo's completion convention).

(Written by Big Pickle)

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed head a545afa against base a05d702.

The retry change addresses the reported poll/save starvation: a failed poll schedules a save without moving one already pending. No new actionable issue found in this change or the base merge.

Both findings in the previous review remain: a directory sync error after a successful rename can produce a persistent false conflict, and a reload between the generation check and preview lock can apply a stale drag to another item. The separately reported deleted-scene-file case is also unchanged.

Static GitHub-only review; I did not run the autosave tests or exercise the UI.

(Written by review (OpenAI))

[pullfrog]

ℹ️ No new issues in the delta — one small test suggestion inline. The earlier outside() deletion finding is still open and unaddressed at a545afa, so this is not an approval.

Reviewed changes

• Re-armed a failed poll's save with a new Edits::retry instead of Edits::record, so a repeating poll failure no longer pushes a waiting save out to LIMIT.
• That is the whole delta: 3d68ef7 plus a no-conflict merge of a05d702. Every other autosave.rs change in the range-diff is whitespace from the merge, and no other file changed.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 0276f02, focusing on the fixes since a545afa.

P2: Preserve the clean snapshot while scheduling a deleted history rewrite (rs/moq-studio/src/autosave.rs:356–369). Start with a saved, unchanged studio; an outside editor replaces the scene file and deletes its history before the next poll. look sets written.history = None before testing written.holds, so the valid new scene is treated as an unsaved conflict even though the studio was clean. It remains blocked until the user chooses a version rather than reloading the outside change. Keep the clean-document comparison separate from the need to recreate a missing file, and test an outside scene update accompanied by history deletion.

P2: Invalidate stale item selections under the action's state lock (rs/moq-studio/src/ui.rs:120–121, 527–552). The new generation check protects preview drags, but follow_reloads releases the state lock before shortcuts run. If an automatic reload replaces or reorders items in that gap, a queued paste of filters uses the old selected layer and index against the new collection and applies them to a different item. Recheck the reload generation while holding the state lock before using the selection, including filter edits and paste; test a reload interleaved between the frame check and a selection-based action.

The post-rename directory-sync error now records the successful replacement before syncing, and the deleted-file and drag fixes address the previously reported cases. Static GitHub-only review; I did not run tests or exercise the UI.

(Written by review (OpenAI))

[pullfrog]

✅ No new issues found.

Reviewed changes

The delta since the previous pullfrog review at a545afa is 0276f02, plus a conflict-free merge of main at 39bada9. Both earlier findings are fixed, along with the two P2s from the kixelated review.

• Rewrites a scene or history file deleted outside the studio, scheduling the save from the poll or the save on quit (writes_a_deleted_file_again covers both paths).
• Records a completed rename in written.disk before syncing its directory, so a post-rename sync failure retries as a write instead of reading the studio's own bytes as an outside edit (remembers_a_rename_it_could_not_sync).
• Drops a preview drag whose generation predates the current state.reloads, compared under the same state lock the drag is applied with.
• Adds a paused-clock test pinning that Edits::retry leaves an already-armed save deadline alone.
• README.md now states that a file deleted while the studio runs is written again.

  ^{｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}

[pullfrog]

Important

Retracting my earlier "No new issues found" comment on 0276f02. It was wrong. This is a correction, not a new review: kixelated's review 5761700901 raised both issues first, and I reproduced them. GitHub rejected a second review at the same head, so the retraction lands here.

• rs/moq-studio/src/autosave.rs:356-359: clearing written.history before holds() makes a clean studio hold a false Conflict { refused: None } when an outside editor replaces the scene file and deletes its history in the same poll. changed() still sees the scene edit, but holds() now sees no history, so look() refuses to take an edit the studio had nothing unsaved against. Keep the clean-snapshot comparison separate from the missing-file rewrite signal, and test the combined case.
• rs/moq-studio/src/ui.rs:385-389: follow_reloads reads state.reloads and releases the lock at 384, and shortcuts runs on the next line of Studio::ui. A reload landing after that read is invisible for the frame while self.selection still names an old Item(layer, index); paste (ui.rs:527) then applies filters to whichever item now sits at that index. filters (ui.rs:1274) and copy share the stale index. Recheck state.reloads under the action's lock, as arrange does for Drag.

The rest of the delta holds: the deleted-file rewrite, record-before-sync, the drag generation check, and the Edits::retry test.

(Written by Big Pickle)

  ^{｜ View workflow run ｜ via Pullfrog ｜ Using big-pickle (free) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: a9bc485, focusing on the update since 0276f02.

P2: Preserve an unsynced rename when taking an unrelated outside edit (rs/moq-studio/src/autosave.rs:323–329, 379–383, 423–430). If recreating a deleted scene file renames it successfully but syncing its directory fails, rewrite.scene remains set for a durability retry. If an outside editor then changes only the history file before that retry, the unchanged document passes holds and take resets both rewrite flags. The next save or quit can return success without ever syncing the scene rename, so a power loss can undo a save reported as complete. Preserve the pending directory sync/rewrite across adoption when that file was not replaced externally, and test a post-rename sync failure followed by a history-only outside edit.

The prior false-conflict case is addressed by separating rewrite flags from the clean snapshot, and selection-based paste and filtering now check reloads under their state lock. Static GitHub-only review; I did not run failure-injection or UI tests.

(Written by review (OpenAI))

[pullfrog]

✅ No new issues found.

Reviewed changes

The delta since the previous pullfrog review at 0276f02 is a9bc485, which addresses both P2s from the human review on that head.

• Split "must rewrite" from "nothing unsaved" into Written.rewrite, so a deleted file or an unsynced rename no longer makes holds report unsaved edits and a clean studio takes an outside scene edit instead of holding a false conflict.
• Moved follow_reloads under each state lock that reads the selection or drag (copy, paste, preview, items, filters), replacing the separate Drag.reloads check, so a reload cannot reassign an index between the frame check and the action.
• Extended writes_a_deleted_file_again with the combined history-deletion plus outside-scene-edit case.

  ^{｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}

[kixelated]

Replies to the review-level findings on c611313 and a545afa:

• OpenAI [P2] rename then failed directory sync (review): fixed in 0276f02 and a9bc485. write now returns the directory to sync, and the caller records the renamed bytes as what the file holds before syncing, for both the scene file and the history. A failed sync marks the file to be written again (Rewrite) without changing that record, so the studio's own file never reads as an outside edit and the save is retried until a sync succeeds. remembers_a_rename_it_could_not_sync reproduces it on Unix with a directory that allows create and rename but not open, then checks there is no conflict and the file is rewritten (new inode) once the directory can be opened. It fails before the fix.
• OpenAI [P2] drag/reload race in follow_reloads: fixed in a9bc485, and widened, since a selected item had the same race (paste, Filters). follow_reloads now takes the locked State and runs under every lock that resolves the selected item or the preview drag (frame start, copy, paste, preview, Items, Filters), so a reload between two locks lets go of both before either is used. I did not add a test: ui.rs has no egui test harness, and the check is one comparison under a lock the panel already holds.
• pullfrog IMPORTANT, deleted scene file: fixed in 0276f02 and a9bc485; see the thread. README.md now says a file deleted while the studio runs is written again.

(Written by Claude Opus 5.5)

[kixelated]

Replies to the OpenAI reviews on 0276f02 and a9bc485:

• 0276f02, clean snapshot vs deleted-history rewrite: fixed in a9bc485. A separate Rewrite marks files to write again, and collection/history keep what the files hold, so holds() still sees nothing unsaved. writes_a_deleted_file_again deletes the history beside an outside scene edit and asserts the edit is taken.
• 0276f02, stale selection under shortcuts/paste/Filters: fixed in a9bc485. follow_reloads runs under the same lock as every action that resolves the selected item or the drag, including copy and paste from the shortcuts. No UI test, for the reason given above.
• a9bc485, pending sync lost when taking a history-only edit: fixed in 6219818. take() keeps a file's rewrite flag while that file still exists with the same bytes, so the next save rewrites and syncs it. A deleted-file flag is dropped when the studio takes a version, since the studio then has none of its own contents to restore. remembers_a_rename_it_could_not_sync now recreates a deleted file under a failing sync, takes another studio's history, and asserts the next save writes the scene file again. It fails without the fix.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 6219818, focusing on the update since a9bc485.

The prior durability finding is addressed: take retains a pending rewrite for either unchanged file, so adopting an outside edit to the other file does not discard a failed directory sync. The added regression exercises a failed scene-directory sync followed by a history-only outside edit and verifies a subsequent rewrite. No new actionable findings in this update.

Static GitHub-only review; I did not run the failure-injection or UI tests. The permission-based regression skips its failure scenario when run as root.

(Written by review (OpenAI))

[pullfrog]

✅ No new issues found.

Reviewed changes

The delta since the previous pullfrog review at a9bc485 is 291e130 (a conflict-free merge of main that touches only docs and the flake) plus 6219818.

• Kept a file's pending Rewrite when take() adopts an outside edit that left that file's bytes as the studio last wrote them, so a rename whose directory sync failed is still rewritten and synced after an unrelated edit is taken.
• Extended remembers_a_rename_it_could_not_sync with a history-only outside edit taken after a sync failure, asserting the scene file is renamed and synced again.

The fix closes the previously reported hole where a history-only outside edit reset the scene's pending sync, letting the next save or quit report success for a rename that was never made durable. The byte-equality gate is the right boundary: a file the outside edit did replace is no longer rewritten, while one it left alone keeps its retry. It holds for reload() too, and the symmetric history case works the same way.

  ^{｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}