Persist desktop preferences across builds and restarts#4408
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces significant new persistence functionality with complex state management and migration logic. Multiple unresolved review comments identify potential bugs including race conditions during shutdown, data loss scenarios, and error handling gaps that warrant careful human review. You can customize Macroscope's approvability policy. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caf75c3371
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| > { | ||
| const shutdown = yield* DesktopShutdown.DesktopShutdown; | ||
| const desktopWindow = yield* DesktopWindow.DesktopWindow; | ||
| yield* desktopWindow.flushRendererState; |
There was a problem hiding this comment.
When the user exits by closing the last window on Linux/Windows, window-all-closed calls app.quit only after the BrowserWindow has emitted closed; DesktopWindow then clears the main-window ref, so this new before-quit flush finds no live webContents and returns without asking the renderer to drain its debounced ui-state/composer preference writes. A preference changed shortly before closing the window can therefore be dropped. Please flush or prevent the window close before the renderer is destroyed, rather than only during before-quit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
One finding on the Effect service error-modeling conventions. See inline comment.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09e4513bf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runFork(flushBoundsPersist); | ||
| let closeAllowed = false; | ||
| let closeFlushPending = false; | ||
| window.on("close", (event) => { |
There was a problem hiding this comment.
🟡 Medium window/DesktopWindow.ts:580
The close handler skips the renderer-state flush when desktopState.quitting is true, so any quit path that sets quitting without first calling flushRendererState destroys the window while durable preference writes are still pending. The update-install path (DesktopUpdates.installDownloadedUpdate) sets quitting to true and then destroys windows without flushing, so recent UI/composer state is silently lost. The quitting fast-path bypass should only apply after a flush has already completed, or the update-install path must call flushRendererState before destroying windows.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/window/DesktopWindow.ts around line 580:
The `close` handler skips the renderer-state flush when `desktopState.quitting` is true, so any quit path that sets `quitting` without first calling `flushRendererState` destroys the window while durable preference writes are still pending. The update-install path (`DesktopUpdates.installDownloadedUpdate`) sets `quitting` to `true` and then destroys windows without flushing, so recent UI/composer state is silently lost. The `quitting` fast-path bypass should only apply after a flush has already completed, or the update-install path must call `flushRendererState` before destroying windows.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6b194d2. Configure here.
| return yield* Effect.gen(function* () { | ||
| yield* desktopWindow.flushRendererState; | ||
| yield* desktopWindow.flushMainWindowBounds; | ||
| yield* Ref.set(desktopState.quitting, true); |
There was a problem hiding this comment.
Install flush races non-macOS quit
Medium Severity
installDownloadedUpdate keeps desktopState.quitting false until after flushRendererState, which can take up to 10s. In that window a user close can finish the close interceptor, destroy the main window, and on Windows/Linux fire window-all-closed → app.quit while install is still waiting on a dead webContents before destroyAll / quitAndInstall. That races lifecycle shutdown with update install and can stall or fail the update path.
Reviewed by Cursor Bugbot for commit 6b194d2. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b194d22bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| closeFlushPending = true; | ||
| void runPromise( | ||
| Effect.all([flushRendererStateForWindow(window), flushBoundsPersist], { |
There was a problem hiding this comment.
When the main window is closed before the web app has installed the onRendererStateFlush listener (for example after a failed/slow initial load or a crashed renderer), this path still prevents the native close and sends a flush request that no renderer can acknowledge. The only way out is the 10s timeout in flushRendererStateForWindow, so users trying to close a broken window now see the app hang instead of closing; track renderer flush readiness or skip this flush for unloaded/crashed contents.
Useful? React with 👍 / 👎.
| if (browserState === null) { | ||
| return null; | ||
| } | ||
| await window.desktopBridge.setRendererState(key, browserState.raw); |
There was a problem hiding this comment.
When the desktop renderer-state file is missing but an existing browser-local t3code:ui-state:v1 or t3code:composer-preferences:v1 value is present, this await makes hydration depend on the first durable migration write succeeding. If that write rejects due to a transient IPC/filesystem error, the already-validated browser state is not returned, so the UI/composer hydrators render defaults and keep writes guarded even though usable state is still in localStorage; apply the browser state for the current session and retry/preserve the failed durable copy separately.
Useful? React with 👍 / 👎.
| yield* desktopWindow.flushRendererState; | ||
| yield* desktopWindow.flushMainWindowBounds; | ||
| yield* Ref.set(desktopState.quitting, true); |
There was a problem hiding this comment.
After an update install is accepted, flushRendererState can wait up to 10 seconds while desktopState.quitting is still false. If the user closes the last Linux/Windows window during that window, the normal window-all-closed quit path can run before this flow reaches quitAndInstall, so the requested update install may be skipped; set the quitting latch before the slow flushes or make the close/quit path honor updateInstallInFlightRef.
Useful? React with 👍 / 👎.
| if (!legacySettings) { | ||
| return null; | ||
| } | ||
| await window.desktopBridge.setClientSettings(legacySettings); |
There was a problem hiding this comment.
When a desktop client-settings file is absent but valid browser-local settings still exist, this migration write is now on the read path. A transient desktop write failure rejects getClientSettings, so startup hydrates with defaults and leaves the user's valid local settings unused even though they are still readable; return the legacy settings for the current session and retry the durable copy separately instead of making the first seed write mandatory.
Useful? React with 👍 / 👎.
1 participant
What Changed
t3codeElectron profile while keeping unpackaged development builds isolated int3code-dev.localStorage.Why
Desktop preferences were split across Electron profile directories, renderer-origin storage, and server-side settings. Build identity and origin changes could therefore make labels, manual project ordering, explicit unread state, and default model selections appear to reset even though other durable state still existed.
This gives all packaged channels one stable source of truth, preserves the desired Alpha/Nightly sharing behavior, and prevents unpackaged development sessions from modifying that packaged profile.
Data Safety
Verification
git diff --checkpassed.Checklist