fix(desktop): offline sign out behavior fixed#512
Conversation
📝 WalkthroughWalkthroughThe auth service now returns explicit result codes ("success", "invalid", "network_error") from token refresh operations; callers branch on these: "success" updates tokens and restores session, "invalid" clears the session, and "network_error" preserves the existing session for offline use. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant AuthSvc as Auth Service
participant Remote as Auth Server
participant Session
Note right of AuthSvc `#DDEBF7`: refreshTokens() → "success" | "invalid" | "network_error"
Caller->>AuthSvc: refreshTokens()
AuthSvc->>Remote: POST /auth/refresh (with refresh token)
alt 200 + valid JSON & shape
Remote-->>AuthSvc: 200 + tokens
AuthSvc->>Session: persist updated tokens/session
AuthSvc-->>Caller: "success"
Caller->>Session: resume restored session
else 401 or 403 OR malformed JSON
Remote-->>AuthSvc: 401/403 or invalid body
AuthSvc->>Session: clear session
AuthSvc-->>Caller: "invalid"
Caller->>Caller: trigger login flow
else other HTTP error or network failure
Remote--x AuthSvc: network/server error
AuthSvc-->>Caller: "network_error"
Caller->>Session: preserve session for offline use
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
AviPeltz
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/auth/auth-service.ts (1)
183-188: Add runtime validation for token refresh response.The type assertion assumes the API returns the expected structure without validation. If the refresh endpoint returns a 200 status but with missing or incorrectly typed fields, this will cause runtime errors when accessing token properties (e.g., line 191-195).
🔎 Proposed fix with runtime validation
const tokens = (await response.json()) as { accessToken: string; accessTokenExpiresAt: number; refreshToken: string; refreshTokenExpiresAt: number; }; + + // Validate response structure + if ( + typeof tokens.accessToken !== "string" || + typeof tokens.accessTokenExpiresAt !== "number" || + typeof tokens.refreshToken !== "string" || + typeof tokens.refreshTokenExpiresAt !== "number" + ) { + console.error("[auth] Invalid token refresh response structure"); + return "invalid"; + }
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/auth/auth-service.ts (1)
176-180: Consider the implications of treating 5xx errors as network errors.Server errors (500, 502, 503, etc.) are currently treated as "network_error" to keep the session for offline use. While this provides better UX, a 500 could also indicate that the tokens are malformed or the refresh endpoint is broken, not just a temporary connectivity issue. This is a design trade-off between offline UX and strict token validation.
Consider whether certain 5xx errors (like 500 Internal Server Error) should be treated as "invalid" if they consistently fail, while keeping 502/503/504 as "network_error" since those are typically temporary issues.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/main/lib/auth/auth-service.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/main/lib/auth/auth-service.ts (2)
66-82: Offline session handling looks good.The logic correctly preserves the session when a network error occurs during token refresh on startup, enabling offline usage. The fall-through behavior for invalid tokens (line 81) is clear and leads to proper session cleanup at lines 84-86.
123-134: Consistent offline handling in token access.The null return on network errors (line 131) allows callers to handle offline scenarios gracefully while maintaining the user's session. This is consistent with the startup behavior.
AviPeltz
changed the title
Expand this to see my work.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/main/lib/auth/auth-service.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (5)
apps/desktop/src/main/lib/auth/auth-service.ts (5)
66-82: LGTM! Startup refresh flow handles all three outcomes correctly.The three-way result handling appropriately restores the session on success, preserves it for offline use on network errors, and clears it when tokens are invalid.
123-134: LGTM! Offline behavior is correctly preserved.The token refresh handling returns null for network errors without clearing the session, allowing users to remain signed in while offline. Invalid tokens correctly fall through to clear the session.
152-181: LGTM! Three-way result model and HTTP status handling are well-designed.The explicit return type provides type safety, and the distinction between authentication failures (401/403 → "invalid") versus temporary issues (other errors → "network_error") is appropriate for offline support.
183-199: JSON parse error handling correctly addresses previous review feedback.The separate try-catch block now returns "invalid" for malformed server responses rather than treating them as network errors, which prevents corrupted state. However, see the next comment about response validation.
212-218: Past review comment fully resolved.The improved error handling correctly distinguishes network errors from JSON parsing failures. Parse errors are now caught separately (lines 190-199) and return "invalid", while this catch block handles only genuine network connectivity issues. The enhanced error logging provides better debugging information.
|
This change does fix the “offline sign-out” class of bugs nicely: A few things I’d tighten up before merge (mostly around correctness + debuggability):
Non-code / PR hygiene:
Build is still pending on the PR checks as of now; everything else is green. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/auth/auth-service.ts (1)
178-246: LGTM: Comprehensive token refresh with proper error categorization.The refactored
refreshTokens()method correctly implements the offline fix by:
- Returning explicit result codes for different failure scenarios
- Distinguishing authentication failures (401/403 →
"invalid") from network/server issues (other errors →"network_error")- Safely parsing and validating responses before updating session state
- Providing detailed error logging for debugging
The error handling properly addresses both previous review comments (JSON parse errors and response validation).
Consider adding unit tests for the error scenarios.
Given the complexity of the error handling logic (network errors, auth errors, parse errors, validation errors), unit tests would help ensure all branches behave correctly and prevent regressions.
Would you like me to generate a test suite covering these scenarios?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/main/lib/auth/auth-service.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (5)
apps/desktop/src/main/lib/auth/auth-service.ts (5)
6-6: LGTM: Import properly organized.The tokenStorage import is correctly placed and necessary for the functionality used throughout the file.
40-45: LGTM: Well-defined interface.The TokenResponse interface clearly defines the expected shape of the token refresh response, supporting the runtime validation logic.
47-65: LGTM: Robust runtime validation.The type guard comprehensively validates the token response shape, checking field presence, types, and basic constraints. This addresses the previous concern about runtime validation.
92-108: LGTM: Offline behavior correctly implemented.The initialization flow now properly handles offline scenarios by preserving the session when
refreshTokens()returns"network_error". This allows users to remain signed in and work offline, which is the core fix for the offline sign-out behavior described in the PR.
133-133: LGTM: Graceful offline handling in token retrieval.The access token retrieval correctly handles offline scenarios by returning
nullwithout clearing the session when refresh fails due to network issues. The updated comment accurately describes this behavior.Also applies to: 149-159
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Refactor
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.