[saphid]

Problem

The desktop app saves desktop-settings.json and client-settings.json atomically. It writes a <file>.<pid>.<uuid>.tmp file, then renames it over the real file. If the write or rename fails, the temp file stays in the state directory. The rename can fail on Windows when another process briefly holds the settings file (EPERM/EBUSY), and the write can fail on a full disk.

desktop-settings.json is rewritten after every window move or resize. So while the failure persists, each save leaves one more .tmp file beside the settings.

Change

Both writers now wrap the write and the rename in Effect.ensuring(remove(tempPath, { force: true })). The connection catalog writer in the same app (DesktopConnectionCatalogStore.writeDocument) and the server's ServerSecretStore already clean up this way. After a successful rename the temp path is gone, so the remove does nothing.

Scope and approval

Very small, focused fix for an obvious bug, so no prior issue: two sibling writers were missing cleanup that the third writer in the same app already does. #20736 fixed the same problem for mobile's atomic writes. Overlap search: #28682 (open) changes how DesktopAppSettings reads settings, not how it writes them. I found no PR that touches temp-file cleanup in these writers.

Verification

• Red on main: a new test in each module turns the settings path into a non-empty directory so the rename fails. Both failed with AssertionError: expected [ Array(1) ] to deeply equal [], leaving desktop-settings.json.<pid>.<uuid>.tmp and client-settings.json.<pid>.<uuid>.tmp behind.
• Green: vp test run src/settings/DesktopAppSettings.test.ts src/settings/DesktopClientSettings.test.ts passes 21/21.
• vp run check:changed passed (format, lint, knip, typecheck of 2 packages, tests).
• Not checked: a real Windows rename lock. The test forces the rename to fail through a directory collision instead.

Reviewed with sol-loop: 1 round with GPT-6.1 Sol on high. Sol verdict: approved

Created with Claude Opus 5.5 in Claude Code (T3 Code).

🤖 Generated with Claude Code

[macroscopeapp]

Approvability

Verdict: Approved at c607c51

Macroscope's review found this PR approvable — Both desktop settings writers now clean up their temporary files after save attempts, while preserving existing save behavior and errors. Cleanup targets only each attempt’s uniquely named temporary file, keeping the change’s impact limited.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: main
• File: .coderabbit.config.ts
• Commit: 065a433
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Advanced
• Run ID: ece8c8df-318f-468c-af8a-1aa86070da2c

Commits

Reviewing files that changed from the base of the PR and between 065a433 and c607c51.

Files selected for processing: 4

• apps/desktop/src/settings/DesktopAppSettings.test.ts
• apps/desktop/src/settings/DesktopAppSettings.ts
• apps/desktop/src/settings/DesktopClientSettings.test.ts
• apps/desktop/src/settings/DesktopClientSettings.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Walkthrough

Both desktop settings writers now attempt to remove temporary files after write-and-replace operations, including failures. Tests cover replacement failures and verify that no .tmp files remain.

Changes: Desktop settings cleanup

Layer / File(s)	Summary
Temporary-file cleanup and failure tests apps/desktop/src/settings/DesktopAppSettings.ts, apps/desktop/src/settings/DesktopAppSettings.test.ts, apps/desktop/src/settings/DesktopClientSettings.ts, apps/desktop/src/settings/DesktopClientSettings.test.ts	Both settings writers attempt to remove temporary files after success or failure. Tests check that replacement failures retain the replace-settings-file error operation and leave no .tmp files.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to c607c

Failed settings replacements now attempt to remove temporary files. No merge-blocking risk was identified.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: failed desktop settings saves no longer leave temporary files behind.
Description check		The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the failure scenario, implementation, scope rationale, tests, validation results, and the …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the failure scenario, implementation, scope rationale, tests, validation results, and the untested Windows lock condition.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

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

[maria-rcks]

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

Verified exact head c607c51363b61c374cdff3381588eb6d5de6e6c0 on Linux with the real Electron client, settings IPC, filesystem, and backend. No source changes were needed.

• Reproduced on github/main (5b9731cedc): with settings destinations replaced by non-empty directories, toggling Settings > General > In-app notifications and resizing the desktop window left temporary files for both writers.
• On the PR, repeated each interaction twice under the same filesystem collision. Both writers preserved their replace-settings-file / EISDIR errors and left zero .tmp files.
• Restored destinations and repeated the interactions: both JSON files saved normally. Notification preference and 1100x780 window bounds survived restart. The completed thread and messages also survived.
• Created a project pointing at the real checkout through the app. A real Codex gpt-6.1-sol turn ran pwd and git rev-parse --is-inside-work-tree, streamed, and completed in 10 seconds. Chrome driven with the repo's Playwright also paired and rendered the built web client against this server.
• Blacksmith: both settings suites passed 21/21. vp run check:changed FETCH_HEAD^ passed format, lint, knip, affected typechecks, and tests. The explicit base was necessary for the worker's shallow snapshot.

Runtime transcript:

main: client-settings.json.<pid>.<uuid>.tmp remains
main: desktop-settings.json.<pid>.<uuid>.tmp remains
PR failure attempt 1, both writers: temporary files []
PR failure attempt 2, both writers: temporary files []
recovery: preference saved; bounds 1100x780; temporary files []
restart: preference and bounds restored

Not verified

Windows-specific file locks, macOS, disk-full write failures, and native mobile clients were not exercised. The rename failure was a real Linux directory collision. Vite's dev renderer stalled/blanked, so runtime verification used the unmodified built desktop/web clients from the exact checkout. Desktop ran with --password-store=basic to avoid this disposable VM's keyring setup prompt.