Add centralized invalid session cleanup and wire Dapper to it #1660
Conversation
Oksamies
commented
Dec 9, 2025
Oksamies
commented
- expose a clearInvalidSession helper from ts-api-react that handles storage, cookies, and stale flags with proper error reporting
- update Dapper instantiations (root app, singleton, client loaders) to rely on the shared cleanup hook instead of duplicating logic
- expose a clearInvalidSession helper from ts-api-react that handles storage, cookies, and stale flags with proper error reporting - update Dapper instantiations (root app, singleton, client loaders) to rely on the shared cleanup hook instead of duplicating logic
WalkthroughThis PR adds a new Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code graph analysis (3)packages/ts-api-react/src/SessionContext.tsx (1)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (2)
apps/cyberstorm-remix/cyberstorm/utils/dapperClientLoaders.ts (1)
⏰ 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). (3)
🔇 Additional comments (13)
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 |
There was a problem hiding this comment.
Pull request overview
This pull request centralizes invalid session cleanup logic by introducing a clearInvalidSession helper in the ts-api-react package and wiring it to all Dapper instantiations throughout the application. The centralization improves consistency and maintainability by eliminating duplicated cleanup logic.
Key Changes
- Introduced
clearInvalidSessionhelper that handles storage cleanup, cookie deletion, and stale flag setting with proper error reporting - Updated Dapper's
getCurrentUserto conditionally clear sessions only for 401 errors with "invalid token" messages - Wired all Dapper instantiations (root app, singleton, client loaders) to use the centralized cleanup hook
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ts-api-react/src/index.ts | Exports the new clearInvalidSession helper |
| packages/ts-api-react/src/SessionContext.tsx | Implements clearInvalidSession and integrates it into the session context and updateCurrentUser |
| packages/dapper-ts/src/methods/currentUser.ts | Adds conditional session cleanup for 401 errors with "invalid token" messages using string matching |
| apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts | Wires singleton Dapper to use clearInvalidSession |
| apps/cyberstorm-remix/cyberstorm/utils/dapperClientLoaders.ts | Wires client loader Dapper instances to use clearInvalidSession |
| apps/cyberstorm-remix/app/root.tsx | Wires root app Dapper to use clearInvalidSession with cookie domain override |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isInvalidTokenError(error)) { | ||
| // If the token is invalid, clear any persisted session data | ||
| this.removeSessionHook?.(); | ||
| } |
There was a problem hiding this comment.
The conditional session cleanup logic creates an inconsistency where only 401 errors with the exact text "invalid token" trigger session cleanup, while other 401 responses (e.g., expired tokens, revoked tokens, or authentication failures with different error messages) return null without clearing the session. This could leave stale session data in storage.
Consider either:
- Clearing the session for all 401 errors (restoring the previous behavior), or
- If the distinction is intentional, add a comment explaining why some 401s should preserve session data while others should clear it.
| if (isInvalidTokenError(error)) { | |
| // If the token is invalid, clear any persisted session data | |
| this.removeSessionHook?.(); | |
| } | |
| // Clear any persisted session data for all 401 errors | |
| this.removeSessionHook?.(); |
| function isInvalidTokenError(error: ApiError): boolean { | ||
| const detail = extractErrorDetail(error.responseJson); | ||
| return ( | ||
| typeof detail === "string" && detail.toLowerCase().includes("invalid token") |
There was a problem hiding this comment.
The string matching logic using .includes("invalid token") is fragile and could fail to match:
- Variations in casing (though
.toLowerCase()helps) - Variations in wording (e.g., "token is invalid", "invalid session token", "bad token")
- Localized error messages in different languages
- Changes to the API error message format
Consider using a more robust approach, such as:
- Checking for specific error codes if the API provides them
- Using a regex pattern to match variations
- Documenting the expected API error message format and adding a test to ensure it matches
| typeof detail === "string" && detail.toLowerCase().includes("invalid token") | |
| typeof detail === "string" && | |
| /invalid\s+token|token\s+is\s+invalid|invalid\s+session\s+token|bad\s+token/i.test(detail) |
| return; | ||
| } | ||
| try { | ||
| clearSession(_storage, true); |
There was a problem hiding this comment.
The new clearInvalidSession function calls clearSession(_storage, true), which clears the API host, while the previous implementation at line 205 called clearSession(_storage, false), which preserved the API host. This is a subtle behavior change that could affect session recovery flows.
Verify this is intentional. If the API host should be preserved during invalid session cleanup (to allow the user to re-authenticate with the same API), consider passing false instead.
| clearSession(_storage, true); | |
| clearSession(_storage, false); |
| clearSession: (clearApiHost?: boolean) => void; | ||
| /** Remove session cookies. */ | ||
| clearCookies: (domain: string) => void; | ||
| /** Clear all persisted session data and flag as stale. */ |
There was a problem hiding this comment.
[nitpick] The JSDoc comment should be more descriptive about what "clear all persisted session data" means. Consider specifying:
- Clears current user data
- Clears API host
- Clears session cookies
- Sets session as stale
Example: /** Clear all persisted session data (current user, API host, cookies) and mark session as stale. */
| /** Clear all persisted session data and flag as stale. */ | |
| /** | |
| * Clear all persisted session data (current user, API host, cookies) and mark session as stale. | |
| */ |
| } | ||
| if ( | ||
| typeof payload === "object" && | ||
| payload !== null && |
There was a problem hiding this comment.
Variable 'payload' is of type date, object or regular expression, but it is compared to an expression of type null.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1660 +/- ##
==========================================
+ Coverage 11.57% 11.58% +0.01%
==========================================
Files 317 317
Lines 22867 22930 +63
Branches 505 508 +3
==========================================
+ Hits 2647 2657 +10
- Misses 20220 20273 +53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Roffenlund
left a comment
Roffenlund
left a comment
There was a problem hiding this comment.
Looks good overall. I would suggest looking over the AI generated comments as some of them seem relevant to the implementation.
