Persist user login state in Electron app

[LeslieLeung]

Add persistent token storage for Electron desktop app to maintain user login state between application restarts. Previously, users had to log in every time they launched the app.

Changes:

• Add token storage IPC handlers in Electron main process
• Expose token storage APIs via preload script
• Create TokenStorage service to abstract storage (localStorage for web, electron-store for desktop)
• Update AuthService to use async token storage
• Update ApiClient to handle async token operations
• Initialize auth state on app startup to restore login session
• Update type definitions for new Electron APIs

Technical details:

• Tokens stored securely using electron-store in desktop app
• Backward compatible with web app (continues using localStorage)
• Async storage API ensures consistency across platforms

[claude]

Pull Request Review: Persist User Login State in Electron App

Overview

This PR adds persistent token storage for the Electron desktop app, allowing users to maintain their login state across application restarts. The implementation is well-structured and uses an abstraction layer to handle both Electron and web environments.

---

✅ Strengths

1. Clean Architecture: The TokenStorage abstraction elegantly handles platform differences between Electron (electron-store) and web (localStorage).

2. Backward Compatibility: The implementation maintains full compatibility with the web app while adding Electron-specific functionality.

3. Consistent API: Converting all token operations to async ensures consistency across platforms, which prevents subtle bugs.

4. Secure Storage: Using electron-store is appropriate for Electron apps - it stores data in OS-specific secure locations.

5. Proper Error Handling: The token refresh flow correctly handles failures and clears tokens when refresh fails.

---

🔍 Code Quality Issues

1. Race Condition in Token Refresh (Medium Priority)

Location: frontend/packages/api-client/src/client.ts:94-135

The token refresh logic has a potential race condition. When isRefreshing is true, queued requests are added to failedQueue, but if the refresh succeeds quickly, the queue might not be processed correctly before new requests arrive.

Suggested Fix: Ensure processQueue is called with proper synchronization:

// After successful refresh (around line 127)
this.isRefreshing = false
this.processQueue(null, access_token) // Process queue BEFORE setting isRefreshing to false

2. Missing Null Checks in TypeScript (Low Priority)

Location: frontend/packages/api-client/src/tokenStorage.ts:18-20

While the code handles null gracefully at runtime, the type definitions could be more defensive:

async getAccessToken(): Promise<string | null> {
  if (this.isElectron && window.electronAPI) {
    return await window.electronAPI.getAccessToken()
  }
  return localStorage.getItem('access_token')
}

The check window.electronAPI is redundant since this.isElectron already verifies this. Consider simplifying or adding a type guard.

3. Initialization Timing Issue (Medium Priority)

Location: frontend/apps/web/src/App.tsx:46-51

The app shows a loading spinner during auth initialization, which is good. However, if loadUser() throws an unhandled error, the spinner will remain indefinitely. Consider adding error handling:

useEffect(() => {
  loadUser()
    .catch((error) => {
      console.error('Failed to load user:', error)
      // Still set initialized to allow app to render
    })
    .finally(() => {
      setIsInitialized(true)
    })
}, [loadUser])

---

🔒 Security Considerations

1. Token Storage Security (Info)

electron-store stores data in plain text files on disk (not encrypted by default). While this is acceptable for most use cases and standard practice for desktop apps, sensitive applications might want to consider:

• Using the OS keychain/credential manager for token storage
• Implementing token encryption at rest

Current implementation is acceptable for this application's threat model, but worth noting for future consideration.

2. Token Exposure in IPC (Low Risk)

Location: frontend/apps/web/electron/main.ts:394-427

Tokens are passed through IPC channels. This is generally safe since IPC in Electron is isolated, but ensure:

• The preload script uses contextBridge (✅ already done)
• No nodeIntegration is enabled in renderer (should verify)

---

🐛 Potential Bugs

1. clearTokensAndRedirect is async but not always awaited (High Priority)

Location: Multiple locations in client.ts

While most calls to clearTokensAndRedirect() are awaited, the function doesn't need to be async anymore since tokenStorage.clearTokens() returns a Promise that can be handled independently. However, since it now performs async operations, all call sites should ensure proper error handling.

Lines 83, 90: Already awaited ✅
Lines 133: Already awaited ✅

No issues found - good job!

2. Missing Error Handling in IPC Handlers (Low Priority)

Location: frontend/apps/web/electron/main.ts:394-427

The IPC handlers don't have try-catch blocks. While electron-store operations rarely fail, consider adding error handling for production robustness:

ipcMain.handle('set-access-token', async (_event, token: string | null) => {
  try {
    if (token === null) {
      store.delete('accessToken')
    } else {
      store.set('accessToken', token)
    }
    return true
  } catch (error) {
    console.error('Failed to set access token:', error)
    return false
  }
})

---

🧪 Test Coverage

Major Gap: No tests added for the new functionality.

Recommended Tests:

1. Unit tests for TokenStorage:

   • Test Electron path (with mocked window.electronAPI)
   • Test web path (with mocked localStorage)
   • Test isAuthenticated() with/without tokens

2. Integration tests for AuthService:

   • Test login flow persists tokens
   • Test logout clears tokens
   • Test token refresh updates storage

3. E2E test for Electron:

   • Test app restart maintains login state
   • Test logout clears persistent state

---

📊 Performance Considerations

1. Async Operations in Hot Path: Token storage operations are now async, which adds minimal overhead. The implementation is efficient and appropriate.

2. App Startup: The initialization check in App.tsx adds a small delay to app startup, which is necessary and acceptable for ensuring proper auth state.

---

📝 Documentation

Missing:

• No comments explaining why async storage is necessary
• IPC handlers could benefit from JSDoc comments
• Consider adding migration notes if users have existing tokens in localStorage

---

🎯 Recommendations Summary

Must Fix Before Merge:

1. Add error handling to App.tsx initialization (line 46-51)
2. Add basic unit tests for TokenStorage class

Should Fix:

1. Add error handling to IPC handlers
2. Simplify redundant checks in TokenStorage

Nice to Have:
3. Add JSDoc comments to IPC handlers
4. Consider adding migration logic for existing localStorage tokens

---

✅ Verdict

Approve with Minor Changes Required

The PR is well-implemented with good architectural decisions. The main concerns are:

• Missing test coverage
• Error handling in initialization

Once tests are added and error handling is improved, this will be ready to merge. Great work on the clean abstraction and backward compatibility! 🎉

---

Note: This review follows the project's conventions as documented in CLAUDE.md. The monorepo structure and async patterns align well with the existing architecture.