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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change adds a session-invalidaton API ( Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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 @@
## testing_setup #1660 +/- ##
=================================================
+ Coverage 11.57% 13.68% +2.11%
=================================================
Files 320 320
Lines 22915 22954 +39
Branches 509 597 +88
=================================================
+ Hits 2652 3141 +489
+ Misses 20263 19813 -450 ☔ 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.
Oksamies
force-pushed
the
12-09-add_centralized_invalid_session_cleanup_and_wire_dapper_to_it
branch
from
010dc62 to
157de3f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
apps/cyberstorm-remix/cyberstorm/session/__tests__/SessionContext.test.ts
Dismissed
Show dismissed
Hide dismissed
apps/cyberstorm-remix/cyberstorm/session/__tests__/SessionContext.test.ts
Dismissed
Show dismissed
Hide dismissed
apps/cyberstorm-remix/cyberstorm/session/__tests__/SessionContext.test.ts
Dismissed
Show dismissed
Hide dismissed
| getCurrentUserMock = vi.fn().mockResolvedValue(null); | ||
| const { DapperTs } = await import("@thunderstore/dapper-ts"); | ||
| vi.mocked(DapperTs).mockImplementation( | ||
| () => ({ getCurrentUser: getCurrentUserMock }) as any |
Check failure
Code scanning / ESLint
Disallow the `any` type Error test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
14-21: Mock is missingclearInvalidSession.The mock for
getSessionToolsdoesn't includeclearInvalidSession, but the actual implementation (line 39-42 in dapperSingleton.ts) callstools.clearInvalidSession(). This will cause runtime errors when the tests run.🔎 Apply this diff to fix the mock:
vi.mock("../../security/publicEnvVariables", () => ({ getSessionTools: vi.fn().mockReturnValue({ getConfig: vi.fn().mockReturnValue({ apiHost: "http://localhost", sessionId: "test-session", }), + clearInvalidSession: vi.fn(), }), }));
🧹 Nitpick comments (1)
packages/dapper-ts/vitest.config.ts (1)
7-10: Verify that disabling browser tests won't miss critical coverage.The PR adds session cleanup logic that interacts with browser storage and cookies. Switching to a node environment and disabling browser tests means these browser-specific behaviors won't run in their actual runtime environment.
Please confirm:
- Is there test coverage for the browser-specific session cleanup elsewhere?
- Are the tests in this package purely for node-compatible code paths?
If browser APIs are involved in the tests, you'll need mocking in node or risk missing environment-specific bugs.
Optional: Clean up unused browser config block.
Since browser tests are disabled, consider removing the entire
browserconfiguration block for clarity.🔎 View suggested cleanup
test: { include: ["src/**/__tests__/**/*.test.ts"], exclude: ["dist/**/*"], environment: "node", - browser: { - enabled: false, - }, },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/cyberstorm-remix/app/root.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/session/__tests__/SessionContext.test.ts(3 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperClientLoaders.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperClientLoaders.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)apps/cyberstorm-remix/vitest.config.ts(1 hunks)packages/dapper-ts/src/methods/__tests__/currentUser.test.ts(1 hunks)packages/dapper-ts/src/methods/currentUser.ts(1 hunks)packages/dapper-ts/vitest.config.ts(1 hunks)packages/ts-api-react/src/SessionContext.tsx(5 hunks)packages/ts-api-react/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cyberstorm-remix/cyberstorm/utils/dapperClientLoaders.ts
- apps/cyberstorm-remix/app/root.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (3)
initializeClientDapper(28-44)getDapperForRequest(65-105)resetDapperSingletonForTest(107-110)
apps/cyberstorm-remix/cyberstorm/session/__tests__/SessionContext.test.ts (1)
packages/ts-api-react/src/SessionContext.tsx (13)
getCookie(61-66)SESSION_STORAGE_KEY(52-52)getConfig(129-139)API_HOST_KEY(53-53)clearCookies(102-104)COOKIE_DOMAIN_KEY(55-55)storeCurrentUser(189-194)clearInvalidSession(106-127)CURRENT_USER_KEY(58-58)STALE_KEY(54-54)setSessionStale(84-86)getSessionCurrentUser(232-246)updateCurrentUser(196-219)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperClientLoaders.test.ts (4)
apps/cyberstorm-remix/cyberstorm/utils/dapperClientLoaders.ts (1)
makeTeamSettingsTabLoader(23-44)packages/dapper-ts/src/index.ts (1)
DapperTs(41-102)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-67)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)
packages/dapper-ts/src/methods/__tests__/currentUser.test.ts (4)
packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)packages/dapper-ts/src/index.ts (1)
DapperTsInterface(36-39)packages/thunderstore-api/src/get/currentUser.ts (2)
fetchCurrentUser(14-27)fetchCurrentUserTeamPermissions(29-46)packages/dapper-ts/src/methods/currentUser.ts (2)
getCurrentUser(9-27)getCurrentUserTeamPermissions(29-39)
packages/ts-api-react/src/SessionContext.tsx (2)
packages/ts-api-react/src/index.ts (3)
clearInvalidSession(6-6)StorageManager(14-14)clearSession(5-5)packages/ts-api-react/src/storage.ts (2)
_storage(37-43)StorageManager(1-69)
🪛 GitHub Check: CodeQL
apps/cyberstorm-remix/cyberstorm/session/__tests__/SessionContext.test.ts
[warning] 63-63: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.
[warning] 75-75: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.
[warning] 94-94: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.
🪛 GitHub Check: ESLint
apps/cyberstorm-remix/cyberstorm/session/__tests__/SessionContext.test.ts
[failure] 53-53: Disallow the any type
Unexpected any. Specify a different type.
⏰ 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: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (16)
apps/cyberstorm-remix/vitest.config.ts (1)
3-10: Module alias configuration is correctly configured and actively used.The
cyberstormdirectory exists at the expected location with the required modules (security, session, utils), and the alias is actively used across test files and application code throughout the codebase. The path resolution usingimport.meta.urlis appropriate for ESM environments.apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
199-218: LGTM!The test properly validates that
resetDapperSingletonForTestclears the request-scoped proxy cache and forces config factory re-resolution. The test structure is clear and assertions are appropriate.packages/dapper-ts/src/methods/currentUser.ts (1)
19-26: LGTM! Simplified and more robust error handling.Treating all 401 responses as invalid sessions is the right approach. This removes the fragile string-matching logic and ensures consistent session cleanup behavior.
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
39-42: LGTM!Clean integration of the session cleanup callback into DapperTs initialization. The code properly obtains session tools and wires the cleanup hook.
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperClientLoaders.test.ts (1)
1-109: LGTM!Comprehensive test coverage for the loader lifecycle:
- DapperTs construction with config factory and cleanup callback
- ApiError translation to Response with proper status/detail handling
- Cleanup callback invocation verification
Well-structured and thorough.
packages/dapper-ts/src/methods/__tests__/currentUser.test.ts (1)
1-136: LGTM!Excellent test coverage for currentUser methods:
- Success path
- 401 handling with session cleanup
- Non-401 error rethrow
- Team permissions forwarding
All critical paths validated.
packages/ts-api-react/src/SessionContext.tsx (5)
20-23: LGTM!The JSDoc clearly describes the function's purpose. The addition to the interface is appropriate.
106-127: LGTM! Well-implemented session cleanup.The implementation correctly:
- Preserves API host for re-auth flows (
clearSession(_storage, false))- Handles cookie domain resolution with override support
- Guards against errors with try-catch and logging
- Marks session as stale
Clean and robust error handling.
274-279: LGTM!The internal wrapper properly forwards the cookie domain override with fallback to the configured cookie domain.
207-209: LGTM!Good integration of the new centralized cleanup helper in the default cleanup path.
323-323: LGTM!Properly exposed in the public API.
packages/ts-api-react/src/index.ts (1)
6-6: LGTM!Correctly exposes the new
clearInvalidSessionhelper in the public API.apps/cyberstorm-remix/cyberstorm/session/__tests__/SessionContext.test.ts (4)
43-59: LGTM!Proper DapperTs mock setup in beforeEach with cleanup in afterEach. The mock implementation is appropriate for testing.
61-98: LGTM!Good coverage of utility functions:
getCookie,getConfig, andclearCookies. Tests verify both success and fallback cases.
100-139: LGTM! Excellent error handling test.Tests both happy path (clearing session data) and error resilience (storage failures don't throw). The error spy verification ensures failures are logged properly.
417-457: LGTM!Good coverage of edge cases:
- Invalid stored user data (throws appropriate error)
- updateCurrentUser with valid user (stores and clears stale)
- updateCurrentUser with null (removes stale user and clears stale)
Oksamies
force-pushed
the
12-09-add_centralized_invalid_session_cleanup_and_wire_dapper_to_it
branch
from
157de3f to
9a2c418
Compare
Oksamies
force-pushed
the
testing_setup
branch
from
74e350c to
272a60d
Compare
Oksamies
force-pushed
the
12-09-add_centralized_invalid_session_cleanup_and_wire_dapper_to_it
branch
2 times, most recently
from
4d5b057 to
cc6d3e0
Compare
Oksamies
force-pushed
the
testing_setup
branch
from
272a60d to
a7f22bc
Compare
Oksamies
force-pushed
the
12-09-add_centralized_invalid_session_cleanup_and_wire_dapper_to_it
branch
from
cc6d3e0 to
ca8fcd5
Compare
Oksamies
force-pushed
the
testing_setup
branch
from
a7f22bc to
e360607
Compare
Oksamies
force-pushed
the
12-09-add_centralized_invalid_session_cleanup_and_wire_dapper_to_it
branch
from
ca8fcd5 to
96fc6d9
Compare
- 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
Oksamies
force-pushed
the
graphite-base/1660
branch
from
e360607 to
de89cb7
Compare
Oksamies
force-pushed
the
12-09-add_centralized_invalid_session_cleanup_and_wire_dapper_to_it
branch
from
96fc6d9 to
aad57cf
Compare
