Repository navigation
fix(desktop): prevent idle logout and memory leak in auth system - #202
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughUpdated the Electron preload IPC API so Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Init
participant AuthFetch as AuthFetch
participant AuthStore as AuthStore
participant Desktop as Electron (ipcRenderer)
participant API as Auth API
App->>AuthFetch: new AuthFetch()
AuthFetch->>AuthFetch: initializeListeners()
AuthFetch->>Desktop: ipcRenderer.on('auth:cleared', handler)
Desktop-->>AuthFetch: returns cleanup fn
App->>AuthStore: initializeEventListeners()
AuthStore->>Desktop: ipcRenderer.on('auth:cleared', handler)
Desktop-->>AuthStore: returns cleanup fn stored in _authClearedCleanup
Note over AuthStore,AuthFetch: Session load -> receives 401
AuthStore->>AuthFetch: refreshAuthSession()
AuthFetch->>AuthFetch: check deviceToken
alt deviceToken available
AuthFetch->>API: refresh with deviceToken
API-->>AuthFetch: success / error
else no deviceToken
AuthFetch->>AuthStore: indicate re-auth required
end
AuthFetch-->>AuthStore: {success, shouldLogout}
alt refresh success
AuthStore->>AuthStore: clearJWTCache() and retry loadSession()
else non-fatal failure
AuthStore->>AuthStore: increment failedAuthAttempts, update lastFailedAuthCheck
else fatal failure
AuthStore->>AuthStore: logout()
end
Desktop->>AuthFetch: auth:cleared event
AuthFetch->>AuthFetch: invoke cleanup as needed
Desktop->>AuthStore: auth:cleared event
AuthStore->>AuthStore: invoke _authClearedCleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (3)
apps/web/src/lib/auth/auth-fetch.ts (2)
44-61: Redundant cleanup check in constructor-only method.Since
initializeListeners()is only called from the constructor and guarded byinitialized, the pre-existing listener cleanup on lines 47-50 will never execute (there cannot be an existing cleanup function on first initialization). This cleanup logic would only be useful ifinitializeListenerswere called multiple times, but theinitializedflag prevents that.Consider whether this defensive cleanup is intentional for future-proofing, or if it can be simplified.
462-485: Consider logging the refresh token fallback result.When the refresh token fallback succeeds (line 464-485), there's no success log message like there is for device token refresh (line 452). Adding a debug log would improve observability.
💡 Suggested improvement
response = await fetch('/api/auth/mobile/refresh', { method: 'POST', headers: { 'Content-Type': 'application/json' }, credentials: 'include', body: JSON.stringify({ refreshToken, deviceToken, deviceId: deviceInfo.deviceId, platform: 'desktop', }), }); + + if (response.ok) { + this.logger.debug('Desktop: Refresh token fallback succeeded'); + } } catch (networkError) {apps/web/src/stores/useAuthStore.ts (1)
539-545: Web event listeners not cleaned up.While IPC listeners now have cleanup, the
window.addEventListenercalls forauth:refreshedandauth:expireddon't have corresponding cleanup stored. IfinitializeEventListenersis called multiple times (e.g., HMR), listeners could accumulate.The
removeEventListenercalls on lines 540-541 mitigate this, but consider adding a similar cleanup pattern for consistency and to enable explicit teardown if needed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/preload/index.tsapps/web/src/lib/auth/auth-fetch.tsapps/web/src/stores/useAuthStore.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase withuseprefix), Zustand stores (camelCase withuseprefix), and React components (PascalCase)
Lint with Next/ESLint as configured inapps/web/eslint.config.mjs
Message content should always use the message parts structure with{ parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from@pagespace/lib/permissions(e.g.,getUserAccessLevel,canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from@pagespace/dbpackage for database access
Use ESM modules throughout the codebase
**/*.{ts,tsx}: Never useanytypes - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting
Files:
apps/web/src/stores/useAuthStore.tsapps/web/src/lib/auth/auth-fetch.tsapps/desktop/src/preload/index.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: React hook files should use camelCase matching the exported hook name (e.g.,useAuth.ts)
Zustand store files should use camelCase withuseprefix (e.g.,useAuthStore.ts)
Files:
apps/web/src/stores/useAuthStore.tsapps/web/src/lib/auth/auth-fetch.tsapps/desktop/src/preload/index.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier
Files:
apps/web/src/stores/useAuthStore.tsapps/web/src/lib/auth/auth-fetch.tsapps/desktop/src/preload/index.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.{ts,tsx}: Use message parts structure for message content:{ parts: [{ type: 'text', text: '...' }] }
For database access, always use Drizzle client from@pagespace/db:import { db, pages } from '@pagespace/db';
Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs
Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Use Vercel AI SDK with async/await for all AI operations and streaming
Use Next.js 15 App Router and TypeScript for all routes and components
Files:
apps/web/src/stores/useAuthStore.tsapps/web/src/lib/auth/auth-fetch.ts
**/*auth*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*auth*.{ts,tsx}: Use custom JWT authentication with jose library for user management
Use bcryptjs for password hashing
Files:
apps/web/src/lib/auth/auth-fetch.ts
🧬 Code graph analysis (1)
apps/web/src/stores/useAuthStore.ts (1)
apps/web/src/lib/auth/auth-fetch.ts (4)
refreshAuthSession(545-547)refreshAuthSession(787-788)clearJWTCache(640-642)clearJWTCache(784-785)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit Tests
- GitHub Check: Lint & TypeScript Check
🔇 Additional comments (11)
apps/web/src/lib/auth/auth-fetch.ts (6)
32-42: LGTM! Good singleton initialization guard.The
initializedflag properly prevents duplicate listener registration during HMR or multiple imports. This addresses the memory leak root cause mentioned in the PR objectives.
428-433: Good guard for missing device token on desktop.This correctly identifies when desktop needs re-authentication (no device token) and returns an appropriate
shouldLogout: trueresult, aligning with the PR goal of distinguishing genuine auth failures from transient issues.
437-460: LGTM! Network error handling prevents false logouts.The try/catch around the device token refresh correctly catches network errors and returns
shouldLogout: false, preventing the desktop app from logging out due to transient network issues. This addresses a key issue in the PR objectives.
487-519: Well-structured response status handling.The cascading checks properly distinguish between:
- 401: Genuine auth failure → logout
- 429: Rate limited → don't logout, retry later
- 5xx: Server error → don't logout, retry later
- Other failures → don't logout
This matches the PR goal of only logging out on genuine authentication failures.
280-300: Potential unhandled rejection in queued request processing.The
queue.forEach(async ...)pattern creates fire-and-forget promises. If any queued request fails, the rejection is caught and logged, but the overallrefreshToken()method won't await these. This is likely intentional for parallelism, but ensure callers don't depend on queued requests completing beforerefreshToken()returns.
52-60: The code safely handles the optional chaining;window.electron.onalways returns a cleanup function.The preload implementation confirms that the
onmethod (lines 32-37 ofapps/desktop/src/preload/index.ts) always returns a cleanup function with type() => void. Theif (cleanup)check in your code properly guards against the case where the optional chaining short-circuits (ifwindow.electronor theonmethod doesn't exist), making this implementation correct.apps/desktop/src/preload/index.ts (2)
32-42: LGTM! Proper IPC listener lifecycle management.The
onmethod now correctly:
- Captures the handler reference
- Registers it with
ipcRenderer.on- Returns a cleanup function that removes only that specific listener
The new
removeAllListenersmethod provides a nuclear option for complete channel cleanup. This directly addresses the memory leak issue from the PR.
99-100: Type definitions correctly reflect the API changes.The
onreturn type is now() => void(cleanup function) andremoveAllListenersis properly declared.apps/web/src/stores/useAuthStore.ts (3)
283-309: Improved desktop token refresh flow with proper failure distinction.The unified refresh flow correctly:
- Uses
refreshAuthSession()which handles device tokens properly- Clears JWT cache before retry
- Distinguishes fatal vs non-fatal failures via
shouldLogout- Records failed attempts without clearing user state for transient failures
This aligns with the auth-fetch.ts changes and addresses the idle logout issue.
456-458: Non-private property exposed on helpers object.
_authClearedCleanupis prefixed with underscore suggesting it's internal, but it's directly on the exportedauthStoreHelpersobject. This is consistent with_authPromiseand_serverSessionInitializedin the store, so the pattern is acceptable, but the naming convention matches.
548-560: LGTM! Proper IPC listener cleanup prevents memory leaks.The cleanup pattern correctly:
- Cleans up existing listener before adding new one
- Nullifies the cleanup reference after calling it
- Stores the new cleanup function for future use
- Safely handles the optional chaining on
window.electron.on?.This directly addresses the memory leak root cause from the PR objectives.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
c7b8b10 to
8eb39a0
Compare
Root cause: Desktop was getting logged out overnight due to: 1. Memory leak: IPC listeners for auth:cleared were never cleaned up, causing multiple callbacks to fire and corrupt auth state 2. Missing refresh token: Desktop login never received a refresh token, so when device token refresh fell back to refresh token, it failed 3. Premature logout: Network errors and rate limits triggered logout instead of being handled by retry logic Changes: - preload/index.ts: Add cleanup function return from on() listener, add removeAllListeners() method for complete cleanup - auth-fetch.ts: Add initialized flag to prevent duplicate listeners, store and cleanup auth:cleared listener properly, make desktop refresh more robust - only logout on genuine 401 auth failure, handle network errors and rate limits gracefully - useAuthStore.ts: Store and cleanup IPC listener reference properly, use unified refresh flow for desktop, distinguish transient failures from genuine auth invalidation The desktop app should now stay logged in for the full 90-day device token lifetime without being logged out due to overnight idle.
8eb39a0 to
35b8381
Compare
Root cause: Desktop was getting logged out overnight due to:
Changes:
The desktop app should now stay logged in for the full 90-day device token lifetime without being logged out due to overnight idle.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.