fix(settings): actually persist theme/wallpaper/dock across sessions - #1617
Conversation
…1601, #1603) The prior fix (#1611) wired up debounced auto-save and a mount-time restore for desktop settings, but the restore fetches (theme, wallpaper, dock, windows, widgets) live in SystemShortcuts, which mounts as a sibling of LoginGate before its /auth/status check resolves. On a fresh login (right after a logout) that mount-time restore fires while still unauthenticated, gets 401'd, and — being a one-shot effect — is never retried once the user actually logs in. The in-memory stores are left at their defaults for the rest of that session, which is exactly the reported symptom. Add an auth-ready-store that LoginGate publishes to once it reaches its "ready" phase, and gate the restore effects on it (resetting when it drops back to false so a later re-login re-fetches). Also split the dock/wallpaper auto-save guards off the outer "restore started" flag onto per-field "restore settled" flags, since the old shared flag flipped true the instant the restore effect began rather than once its fetch actually landed — a slow GET could otherwise lose a race to the debounced auto-save PUT and get overwritten with the pre-restore default value.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThis PR adds an auth-readiness store and a ChangesAuth-ready gated persistence restore
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LoginGate
participant AuthReadyStore
participant App
participant SessionPersistence
User->>LoginGate: Authenticates
LoginGate->>LoginGate: status.phase becomes "ready"
LoginGate->>AuthReadyStore: setReady(true)
App->>AuthReadyStore: subscribe via useOnAuthReady
AuthReadyStore-->>App: authReady = true
App->>App: restoreActiveTheme()
SessionPersistence->>AuthReadyStore: read authReady
AuthReadyStore-->>SessionPersistence: authReady = true
SessionPersistence->>SessionPersistence: fetch dock/wallpaper/settings
SessionPersistence->>SessionPersistence: set dockRestored/wallpaperRestored (finally)
SessionPersistence->>SessionPersistence: enable auto-save effects
User->>LoginGate: Logs out
LoginGate->>AuthReadyStore: setReady(false)
AuthReadyStore-->>SessionPersistence: authReady = false
SessionPersistence->>SessionPersistence: reset restored flags, skip fetch
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/hooks/use-session-persistence.ts (1)
189-214: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWindows/widgets/browser still need per-domain restore gates.
restored.currentflips true before those restore requests finish, so their debounced auto-saves can still fire against default state and overwrite persisted session data. Mirror the dock/wallpaper pattern with separate post-restore flags (or otherwise wait for each fetch to complete).🤖 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 `@desktop/src/hooks/use-session-persistence.ts` around lines 189 - 214, The browser/widgets session auto-save logic still relies on the shared restored flag, which can become true before the per-domain restore fetches complete and cause default-state overwrites. Update the relevant effects in use-session-persistence to gate auto-saves on separate post-restore refs/flags for each domain, following the dockRestored and wallpaperRestored pattern. Ensure the browser/widgets restore flows only enable their debounced save callbacks after their own fetch promises resolve, so the initial mount cannot persist stale defaults.
🧹 Nitpick comments (1)
desktop/src/hooks/use-on-auth-ready.ts (1)
15-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
useEffectEventinstead of the exhaustive-deps suppression.Since the project is on React 19.2.7,
useEffectEventis available and is the purpose-built replacement for this exact "exclude callback from deps + eslint-disable" pattern — it keepscallbackreading the latest closure without needing the ref juggling or lint suppression, and the effect can safely omit it from deps by design rather than via override.React's own docs on this pattern: "most users just disable the lint rule and exclude the dependency. But that can lead to bugs since the linter can no longer help you keep the dependencies up to date."
This is optional since the current single call site (
restoreActiveTheme(), no closured state) has no actual staleness risk, but the file's own comment signals intent to reuse this hook for other session-scoped restores, where closured values are more likely.♻️ Optional refactor using `useEffectEvent`
-import { useEffect, useRef } from "react"; +import { useEffect, useEffectEvent, useRef } from "react"; import { useAuthReadyStore } from "`@/stores/auth-ready-store`"; export function useOnAuthReady(callback: () => void) { const ranForThisSession = useRef(false); const authReady = useAuthReadyStore((s) => s.ready); + const onReady = useEffectEvent(callback); useEffect(() => { if (!authReady) { ranForThisSession.current = false; return; } if (ranForThisSession.current) return; ranForThisSession.current = true; - callback(); - // eslint-disable-next-line react-hooks/exhaustive-deps + onReady(); }, [authReady]); }Please confirm the repo's
eslint-plugin-react-hooksversion supportsuseEffectEventsemantics (v6.1.1+ per React docs) before adopting, since an outdated plugin may still flag it incorrectly.🤖 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 `@desktop/src/hooks/use-on-auth-ready.ts` around lines 15 - 29, The hook useOnAuthReady currently suppresses react-hooks/exhaustive-deps to exclude callback from the effect deps, which is brittle and can hide stale closure bugs. Refactor useOnAuthReady to use React 19’s useEffectEvent for the callback so the effect can depend only on authReady while still reading the latest callback, and remove the eslint-disable suppression and ref juggling; confirm the project’s eslint-plugin-react-hooks version supports useEffectEvent semantics before making the change.
🤖 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.
Outside diff comments:
In `@desktop/src/hooks/use-session-persistence.ts`:
- Around line 189-214: The browser/widgets session auto-save logic still relies
on the shared restored flag, which can become true before the per-domain restore
fetches complete and cause default-state overwrites. Update the relevant effects
in use-session-persistence to gate auto-saves on separate post-restore
refs/flags for each domain, following the dockRestored and wallpaperRestored
pattern. Ensure the browser/widgets restore flows only enable their debounced
save callbacks after their own fetch promises resolve, so the initial mount
cannot persist stale defaults.
---
Nitpick comments:
In `@desktop/src/hooks/use-on-auth-ready.ts`:
- Around line 15-29: The hook useOnAuthReady currently suppresses
react-hooks/exhaustive-deps to exclude callback from the effect deps, which is
brittle and can hide stale closure bugs. Refactor useOnAuthReady to use React
19’s useEffectEvent for the callback so the effect can depend only on authReady
while still reading the latest callback, and remove the eslint-disable
suppression and ref juggling; confirm the project’s eslint-plugin-react-hooks
version supports useEffectEvent semantics before making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d853ebd-3a6f-44d6-815c-f976097d58e5
📒 Files selected for processing (8)
desktop/src/App.tsxdesktop/src/components/LoginGate.tsxdesktop/src/hooks/__tests__/use-on-auth-ready.test.tsdesktop/src/hooks/__tests__/use-session-persistence.test.tsdesktop/src/hooks/use-on-auth-ready.tsdesktop/src/hooks/use-session-persistence.tsdesktop/src/stores/auth-ready-store.tstests/test_desktop_settings.py
1 participant
Summary
Fixes #1601 and #1603. Both were reported after #1611 landed (which wired up the apply path + debounced auto-save/restore machinery) — theme, wallpaper, dock position and dock icon size all still apply correctly in-session but revert to defaults after logout + login.
Root cause: the restore effects for desktop settings and the active theme live in
SystemShortcuts(App.tsx), which mounts as a sibling ofLoginGate— beforeLoginGate's own/auth/statuscheck resolves. On a fresh login right after a logout, that mount-time restore fires while the session is still unauthenticated, gets a 401, and — since it's a one-shot effect gated by a ref — is never retried once the user actually logs in. The in-memory stores stay at their defaults for the rest of that session, which is exactly the reported symptom. (The backend side,tinyagentos/desktop_settings.py, was already correctly disk-backed via SQLite, keyed by user — that part was fine.)There was also a secondary race: the dock/wallpaper auto-save effects were gated on the same flag the restore effect flipped the instant it started running, not once its fetch actually resolved. A slow restore GET could lose to the debounced auto-save PUT (500ms wallpaper / 1s dock) and get overwritten with the pre-restore default value.
Changes
desktop/src/stores/auth-ready-store.ts(new) — tiny storeLoginGatepublishes to once it reaches its "ready" phase.desktop/src/components/LoginGate.tsx— publish auth-readiness on phase change.desktop/src/hooks/use-on-auth-ready.ts(new) — small hook that runs a callback once per authenticated session and re-arms after a logout/login cycle; used for the active-theme restore.desktop/src/App.tsx— gate the active-theme restore on auth-ready instead of firing on bare mount.desktop/src/hooks/use-session-persistence.ts— gate the whole restore effect on auth-ready (resetting on logout so a later login re-fetches), and split the dock/wallpaper auto-save guards onto their own "restore settled" flags instead of the shared one.use-session-persistence.test.tsand newuse-on-auth-ready.test.tscover the full set-then-reload cycle (pre-auth: no fetch; login: restore fires; logout+login again: re-restores a different saved value, not stuck on the first session's);tests/test_desktop_settings.pyadds asave_preference/get_preferenceround-trip test and a fresh-store-instance-same-db test proving the backend is durable across restarts, not just in-memory.Test plan
cd desktop && npm run buildnpx vitest run(298 files, 2434 tests passed)python3 -m pytest tests/test_desktop_settings.py tests/test_routes_desktop.py -q(20 passed)Summary by CodeRabbit