fix(project): make project saves atomic#741
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughProject persistence now uses a serialized atomic-write helper that maintains backups, syncs filesystem changes, cleans temporary files, and is integrated into all project save paths. Tests cover replacement, failures, concurrency, cleanup, and permission preservation. ChangesAtomic project persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectSaveIPC
participant writeProjectFileAtomically
participant NodeFilesystem
ProjectSaveIPC->>writeProjectFileAtomically: submit project path and JSON
writeProjectFileAtomically->>NodeFilesystem: write and sync temporary file
writeProjectFileAtomically->>NodeFilesystem: preserve existing project as backup
writeProjectFileAtomically->>NodeFilesystem: rename temporary file into place
writeProjectFileAtomically->>NodeFilesystem: sync parent directory
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/register/project.ts (1)
428-445: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winOrphaned
.bakleft behind on rename.The atomic writer now creates a
<project>.bakalongside each saved project. In the"rename"branch the old project file (Line 429) and its thumbnail (Line 434) are removed, but the old project's backup atgetProjectBackupPath(activeProjectPath)is not, so it will accumulate as an orphan in the projects directory. Suggest cleaning it up alongside the thumbnail.🧹 Proposed cleanup
await fs.rm(getProjectThumbnailPath(activeProjectPath), { force: true }).catch(() => undefined) + await fs.rm(getProjectBackupPath(activeProjectPath), { force: true }).catch(() => undefined)Note:
getProjectBackupPathis exported from../project/atomicSaveand would need importing here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/register/project.ts` around lines 428 - 445, Clean up the old project's atomic-save backup during the rename flow in the named-save branch. Import the exported getProjectBackupPath helper from ../project/atomicSave, then remove getProjectBackupPath(activeProjectPath) alongside the thumbnail cleanup after unlinking the old project, tolerating a missing backup file.
🧹 Nitpick comments (3)
electron/ipc/project/atomicSave.test.ts (3)
36-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting no temporary artifacts in the stale-backup test.
The other tests call
expectNoTemporaryArtifacts()to verify cleanup, but this one omits the check. Adding it keeps the assertion contract consistent across all tests and guards against leftover.tmpfiles in the stale-backup cleanup path.🧪 Suggested addition
await expect(fs.access(getProjectBackupPath(projectPath))).rejects.toMatchObject({ code: "ENOENT", }); + await expectNoTemporaryArtifacts(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/project/atomicSave.test.ts` around lines 36 - 44, Add an expectNoTemporaryArtifacts() assertion to the stale-backup test after writeProjectFileAtomically completes and the stale backup assertion, matching the cleanup checks used by the other tests.
78-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConcurrent serialization test assumes deterministic order.
The test asserts the final content is
{"revision":3}and backup is{"revision":2}, which requires revision 2 to commit before revision 3. While the queue serialization inwriteProjectFileAtomicallypreserves call order,Promise.alldoes not guarantee the order in which the twowriteProjectFileAtomicallycalls begin executing. The first call to reachpendingWrites.setwins the queue position. In practice, since both calls are synchronous up to theawait currentWrite, the order is deterministic, but this is subtle. Consider adding a brief comment explaining why the order is guaranteed to prevent future confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/project/atomicSave.test.ts` around lines 78 - 91, Add a brief comment in the “serializes overlapping writes to the same project” test explaining that each write reaches the queue synchronously before its first await, so Promise.all preserves invocation order and revision 2 commits before revision 3. Keep the existing assertions unchanged.
57-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert no temporary artifacts remain after the failed write.
The
finallyblock incommitProjectFileis supposed to clean up both temp files even when the commit fails, but this test only checksexpectNoTemporaryArtifacts()after the subsequent successful retry (line 75). Adding the assertion right after the rejected write (after line 65) would verify the failure-path cleanup independently.🧪 Suggested addition
await expect(fs.readFile(projectPath, "utf-8")).resolves.toBe('{"version":1,"name":"old"}'); + await expectNoTemporaryArtifacts(); await fs.rm(getProjectBackupPath(projectPath), { recursive: true });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/project/atomicSave.test.ts` around lines 57 - 76, Update the test case “keeps the active generation unchanged when backup commit fails” to call expectNoTemporaryArtifacts() immediately after the rejected write assertion, before removing the backup directory or retrying. Keep the existing final cleanup assertion as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/project/atomicSave.ts`:
- Around line 42-54: Update writeSyncedTemporaryFile to preserve exact POSIX
permissions: after opening the file and when mode is defined, apply handle-based
chmod/fchmod with that mode before syncing, while retaining the existing write
and guaranteed close flow.
---
Outside diff comments:
In `@electron/ipc/register/project.ts`:
- Around line 428-445: Clean up the old project's atomic-save backup during the
rename flow in the named-save branch. Import the exported getProjectBackupPath
helper from ../project/atomicSave, then remove
getProjectBackupPath(activeProjectPath) alongside the thumbnail cleanup after
unlinking the old project, tolerating a missing backup file.
---
Nitpick comments:
In `@electron/ipc/project/atomicSave.test.ts`:
- Around line 36-44: Add an expectNoTemporaryArtifacts() assertion to the
stale-backup test after writeProjectFileAtomically completes and the stale
backup assertion, matching the cleanup checks used by the other tests.
- Around line 78-91: Add a brief comment in the “serializes overlapping writes
to the same project” test explaining that each write reaches the queue
synchronously before its first await, so Promise.all preserves invocation order
and revision 2 commits before revision 3. Keep the existing assertions
unchanged.
- Around line 57-76: Update the test case “keeps the active generation unchanged
when backup commit fails” to call expectNoTemporaryArtifacts() immediately after
the rejected write assertion, before removing the backup directory or retrying.
Keep the existing final cleanup assertion as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e5574d80-879e-4bc5-9a66-b81da45a0b29
📒 Files selected for processing (3)
electron/ipc/project/atomicSave.test.tselectron/ipc/project/atomicSave.tselectron/ipc/register/project.ts
1 participant
Description
Makes all project JSON save routes commit complete files instead of overwriting the active file in
place.
<project>.bakbefore replacementMotivation
The three save handlers used
fs.writeFiledirectly on the active project. An interrupted writecould therefore truncate the user's only
.recordlyfile. Same-directory replacement ensures theactive path contains either the previous or the next complete JSON generation; pre-commit failures
leave it unchanged.
Type of Change
Related Issue(s)
No linked issue. Independent of draft PRs #737-#740.
Testing Guide
npx biome linton the three changed filesnpx tsc --noEmit --noUnusedLocals falsenpm test -- --reporter=dot: 97/97 files, 846 passed, 1 skippedDefault
tsc --noEmitretains only the current-main unused import failure addressed by #737.Risk / limits
Verified on Windows with fault, cleanup, concurrency, full-suite, and production-build gates.
Linux/macOS and physical interruption were not runtime-tested. The POSIX mode test runs only off
Windows. Automatic
.bakrecovery UX is intentionally separate; the existing All Files picker canopen a backup manually. No project schema or renderer contract changes.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests