fix(auth): stop transient failures from logging players out; retry + session-expiry UX#4440
fix(auth): stop transient failures from logging players out; retry + session-expiry UX#4440evanpelle wants to merge 5 commits into
Conversation
…identity Players reported being "logged out every few days." The root cause is that the client's logOut() is unconditionally destructive — it revokes the server session (POST /auth/logout) AND removes the localStorage persistent_id plus cosmetics — and it was invoked on transient, recoverable failures. A non-200 from POST /auth/refresh is a *resolved* fetch (Cloudflare 5xx/ 520-524, a 429, or a spurious edge 401), so it bypasses the network-error catch path hardened in #2636 and hit logOut(). For a guest this wiped the persistent_id and cleared the refresh cookie, so the next refresh was sent cookie-less and the backend silently minted a brand-new guest (identity changes, no error). For linked accounts it forced an unnecessary re-login. getUserMe() did the same on a 401 from /users/@me on every app open. Changes: - doRefreshJwt(): on a non-200, clear __jwt only (mirror the network-error path); do not revoke the session or wipe identity. - getUserMe(): treat a 401 like any other non-200 (return false); no logOut(). - logOut(): gate the persistent_id / cosmetics wipe behind a new userInitiated flag so only an explicit user logout destroys local identity. This also protects the remaining error-path callers (commerce 401s, iss/aud mismatch) for free. - AccountModal.handleLogout(): pass userInitiated=true (the real logout button). - Add tests/client/Auth.test.ts covering both behaviors. Note: the CrazyGames third-party-iframe SameSite=Lax cookie loss is a separate account-persistence issue and is intentionally not addressed here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAuth refresh now records outcome state instead of logging out on failure, ChangesAuth session flow and session-expired UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/client/Auth.test.ts (1)
50-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert cosmetic settings behavior.
logOut()now preserves or clearsUserSettingsflag/pattern too, but this test only checksplayer_persistent_id. Add small assertions so the full logout contract is locked down.Proposed test addition
import { logOut, userAuth } from "../../src/client/Auth"; +import { UserSettings } from "../../src/core/game/UserSettings"; @@ // Error-path / programmatic logout must preserve the persistent identity. + const settings = new UserSettings(); localStorage.setItem(PERSISTENT_ID_KEY, "keep-me-456"); + settings.setFlag("country:us"); + settings.setSelectedPatternName("skin:test-skin"); await logOut(); expect(localStorage.getItem(PERSISTENT_ID_KEY)).toBe("keep-me-456"); + expect(settings.getFlag()).toBe("country:us"); + expect(settings.getSelectedSkinName()).toBe("test-skin"); // The real "Log out" button passes userInitiated=true and clears identity. await logOut(false, true); expect(localStorage.getItem(PERSISTENT_ID_KEY)).toBeNull(); + expect(settings.getFlag()).toBeNull(); + expect(settings.getSelectedSkinName()).toBeNull();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/client/Auth.test.ts` around lines 50 - 65, The logOut() test currently only verifies player_persistent_id, but it should also lock down the UserSettings logout behavior. In Auth.test.ts, extend the existing logOut() / logOut(false, true) assertions to check that the cosmetic settings flag/pattern stored in UserSettings is preserved for the programmatic logout path and cleared for the explicit user-initiated path, using the existing logOut and localStorage checks alongside PERSISTENT_ID_KEY.src/client/Auth.ts (1)
80-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse named options for logout intent
logOut(false, true)is easy to misread; an options object would make the user-initiated path explicit and safer for future call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/Auth.ts` around lines 80 - 83, The logOut function currently uses positional booleans, which makes user intent hard to read and easy to misuse. Update logOut to accept a named options object instead of allSessions/userInitiated boolean parameters, and adjust the user-initiated branch to read from that object. Use the logOut symbol and any call sites that pass logOut(false, true) to switch them to explicit named arguments so the logout intent is clear and safer to extend.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/Api.ts`:
- Around line 73-78: The transient non-200 path in getUserMe() is still caching
a promise that resolves to false in __userMe, which can cause later calls to
keep returning the stale false result instead of retrying. Update the
getUserMe/userAuth flow so only successful user lookups are cached, and ensure
false/unauthorized results are not stored in __userMe (or are cleared
immediately) while preserving the existing invalidateUserMe() behavior.
---
Nitpick comments:
In `@src/client/Auth.ts`:
- Around line 80-83: The logOut function currently uses positional booleans,
which makes user intent hard to read and easy to misuse. Update logOut to accept
a named options object instead of allSessions/userInitiated boolean parameters,
and adjust the user-initiated branch to read from that object. Use the logOut
symbol and any call sites that pass logOut(false, true) to switch them to
explicit named arguments so the logout intent is clear and safer to extend.
In `@tests/client/Auth.test.ts`:
- Around line 50-65: The logOut() test currently only verifies
player_persistent_id, but it should also lock down the UserSettings logout
behavior. In Auth.test.ts, extend the existing logOut() / logOut(false, true)
assertions to check that the cosmetic settings flag/pattern stored in
UserSettings is preserved for the programmatic logout path and cleared for the
explicit user-initiated path, using the existing logOut and localStorage checks
alongside PERSISTENT_ID_KEY.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 675014eb-b2ac-4b12-b0df-4ae03e38eae5
📒 Files selected for processing (4)
src/client/AccountModal.tssrc/client/Api.tssrc/client/Auth.tstests/client/Auth.test.ts
| // A 401 here is treated like any other non-200 (return false). We | ||
| // deliberately do NOT logOut(): the JWT was just refreshed by userAuth(), | ||
| // so a 401 from /users/@me is transient/ambiguous and must not revoke the | ||
| // session or wipe the persistent identity. The backend already made | ||
| // /users/@me 401 non-authoritative. | ||
| if (response.status !== 200) return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not cache transient false user lookups.
This path now treats 401/non-200 as transient, but __userMe still stores the promise that resolves to false. Later getUserMe() calls can keep returning the cached false and never retry until something calls invalidateUserMe().
Proposed fix
- __userMe = (async () => {
+ const request = (async () => {
try {
const userAuthResult = await userAuth();
if (!userAuthResult) return false;
@@
return false;
}
})();
- return __userMe;
+ __userMe = request;
+ const result = await request;
+ if (result === false && __userMe === request) {
+ __userMe = null;
+ }
+ return result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // A 401 here is treated like any other non-200 (return false). We | |
| // deliberately do NOT logOut(): the JWT was just refreshed by userAuth(), | |
| // so a 401 from /users/@me is transient/ambiguous and must not revoke the | |
| // session or wipe the persistent identity. The backend already made | |
| // /users/@me 401 non-authoritative. | |
| if (response.status !== 200) return false; | |
| const request = (async () => { | |
| try { | |
| const userAuthResult = await userAuth(); | |
| if (!userAuthResult) return false; | |
| // ... | |
| } catch { | |
| return false; | |
| } | |
| })(); | |
| __userMe = request; | |
| const result = await request; | |
| if (result === false && __userMe === request) { | |
| __userMe = null; | |
| } | |
| return result; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/Api.ts` around lines 73 - 78, The transient non-200 path in
getUserMe() is still caching a promise that resolves to false in __userMe, which
can cause later calls to keep returning the stale false result instead of
retrying. Update the getUserMe/userAuth flow so only successful user lookups are
cached, and ensure false/unauthorized results are not stored in __userMe (or are
cleared immediately) while preserving the existing invalidateUserMe() behavior.
… session expiry Builds on the transient-failure fix: instead of silently dropping to a guest identity when a JWT can't be obtained, the client now distinguishes WHY the refresh failed and reacts appropriately. doRefreshJwt() now: - Retries transient failures (network error / 5xx / 429) up to 3x with backoff (0.5s, 1s), behind the existing single-flight guard, so a brief edge blip no longer leaves a logged-in user unauthenticated at join time. - Treats 401/403 as definitive (refresh token genuinely dead): a "soft" logout — clears the in-memory JWT but preserves the session cookie + persistent identity — and, if a session was active, dispatches `auth-session-expired`. - Exposes getLastRefreshOutcome() so callers can tell "expired" from "transient". UX: - New <session-expired-modal>: on `auth-session-expired`, prompts re-login — but ONLY for users who were actually signed in to an account (tracked via Api.wasLoggedIn(), set from the last /users/@me). Guests never see it. - Ranked matchmaking shows "couldn't verify your login, try again" on a transient failure instead of the misleading "must login" + account bounce. Adds en.json strings (session_expired.*, matchmaking_button.connection_issue) and extends tests/client/Auth.test.ts: retry count, transient vs definitive outcome, identity preservation, and the active-session expiry dispatch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/Auth.ts`:
- Around line 254-259: The session-expired branch in Auth.ts only clears __jwt,
so the cached /users/@me result in getUserMe() can still be reused after
auth-session-expired. Update the expiration handling in the refresh/rejected
path to also invalidate the cached user promise/state maintained in Api.ts (the
getUserMe cache), so downstream callers stop seeing a stale logged-in user after
NotifySessionExpired() runs.
In `@tests/client/Auth.test.ts`:
- Around line 69-71: The Auth test setup leaves the auth-session-expired window
listener registered if an assertion fails, which can leak into later tests.
Update the affected test blocks in Auth.test.ts to use try/finally around the
existing assertions so the listener is always removed in finally. Make sure the
cleanup targets the same onExpired handler added via window.addEventListener and
apply this pattern to each affected test case.
- Around line 62-64: The logout test only checks for requests to /auth/logout,
so it can miss regressions where Auth.logOut() switches to /auth/revoke. Update
the assertion in Auth.test.ts to verify that fetchMock.mock.calls contains
neither logout endpoint, using the same fetchMock.mock.calls.some pattern
against both auth paths so logOut() and logOut(true) are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce5e19db-4d61-4b4b-9024-0ec6e2052af4
📒 Files selected for processing (8)
index.htmlresources/lang/en.jsonsrc/client/Api.tssrc/client/Auth.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/SessionExpiredModal.tstests/client/Auth.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/client/Main.ts
- resources/lang/en.json
| if (response.status === 401 || response.status === 403) { | ||
| console.error("Refresh rejected — session expired", response.status); | ||
| __jwt = null; | ||
| __lastRefreshOutcome = "expired"; | ||
| if (hadSession) notifySessionExpired(); | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear cached user data when the session expires.
This branch clears __jwt, but getUserMe() can still return the cached successful /users/@me promise from src/client/Api.ts lines 65-68. After auth-session-expired, downstream code like matchmaking can still see the stale user as logged in.
Proposed fix
export function invalidateUserMe() {
__userMe = null;
}
+
+if (typeof window !== "undefined") {
+ window.addEventListener("auth-session-expired", invalidateUserMe);
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/Auth.ts` around lines 254 - 259, The session-expired branch in
Auth.ts only clears __jwt, so the cached /users/@me result in getUserMe() can
still be reused after auth-session-expired. Update the expiration handling in
the refresh/rejected path to also invalidate the cached user promise/state
maintained in Api.ts (the getUserMe cache), so downstream callers stop seeing a
stale logged-in user after NotifySessionExpired() runs.
| expect( | ||
| fetchMock.mock.calls.some(([u]) => String(u).includes("/auth/logout")), | ||
| ).toBe(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert against both logout endpoints.
logOut() can hit either /auth/logout or /auth/revoke. This check only blocks the first path, so a regression to logOut(true) would still pass while revoking the session. Assert that no fetch call targets either auth logout endpoint.
Suggested fix
- expect(
- fetchMock.mock.calls.some(([u]) => String(u).includes("/auth/logout")),
- ).toBe(false);
+ expect(
+ fetchMock.mock.calls.some(([u]) =>
+ /\/auth\/(logout|revoke)\b/.test(String(u)),
+ ),
+ ).toBe(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect( | |
| fetchMock.mock.calls.some(([u]) => String(u).includes("/auth/logout")), | |
| ).toBe(false); | |
| expect( | |
| fetchMock.mock.calls.some(([u]) => | |
| /\/auth\/(logout|revoke)\b/.test(String(u)), | |
| ), | |
| ).toBe(false); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/client/Auth.test.ts` around lines 62 - 64, The logout test only checks
for requests to /auth/logout, so it can miss regressions where Auth.logOut()
switches to /auth/revoke. Update the assertion in Auth.test.ts to verify that
fetchMock.mock.calls contains neither logout endpoint, using the same
fetchMock.mock.calls.some pattern against both auth paths so logOut() and
logOut(true) are covered.
| const onExpired = vi.fn(); | ||
| window.addEventListener("auth-session-expired", onExpired); | ||
| const fetchMock = vi.fn(async (url: unknown) => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Always clean up the window listener in finally.
If an assertion fails before the last line, this listener stays registered and can make later auth-session-expired tests fail nondeterministically. Wrap the body in try/finally so cleanup always runs.
Suggested fix
const onExpired = vi.fn();
window.addEventListener("auth-session-expired", onExpired);
- const fetchMock = vi.fn(async (url: unknown) => {
- if (String(url).includes("/auth/refresh")) {
- return { status: 401, ok: false, json: async () => ({}) };
- }
- return okJson({});
- });
- vi.stubGlobal("fetch", fetchMock);
-
- const result = await userAuth();
-
- expect(result).toBe(false);
- expect(onExpired).not.toHaveBeenCalled();
- window.removeEventListener("auth-session-expired", onExpired);
+ try {
+ const fetchMock = vi.fn(async (url: unknown) => {
+ if (String(url).includes("/auth/refresh")) {
+ return { status: 401, ok: false, json: async () => ({}) };
+ }
+ return okJson({});
+ });
+ vi.stubGlobal("fetch", fetchMock);
+
+ const result = await userAuth();
+
+ expect(result).toBe(false);
+ expect(onExpired).not.toHaveBeenCalled();
+ } finally {
+ window.removeEventListener("auth-session-expired", onExpired);
+ }Also applies to: 89-91, 95-96, 119-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/client/Auth.test.ts` around lines 69 - 71, The Auth test setup leaves
the auth-session-expired window listener registered if an assertion fails, which
can leak into later tests. Update the affected test blocks in Auth.test.ts to
use try/finally around the existing assertions so the listener is always removed
in finally. Make sure the cleanup targets the same onExpired handler added via
window.addEventListener and apply this pattern to each affected test case.
…booleans
logOut(false, true) was opaque at the call site. Replace the two positional
booleans with a LogOutOptions object so intent is explicit:
logOut({ userInitiated: true }) // the real "Log out" button
logOut() // error-path / programmatic (no identity wipe)
No behavior change; all existing no-arg callers are unaffected (options default
to {}).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Everyone holds a session (guests included), so "logged in" was a misleading name for the modal gate. The actual question is whether the account was a linked (non-guest) one — Discord/Google/email — which is exactly what this returns. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/Auth.ts (1)
266-271: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlso clear the cached
/users/@meresult when the session expires.This branch clears
__jwt, butgetUserMe()insrc/client/Api.tscan still hand back its cached successful promise. Afterauth-session-expiredfires, downstream code (for example matchmaking) keeps seeing the old user as logged in. Please invalidate that cache here too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/Auth.ts` around lines 266 - 271, The session-expired branch in Auth.ts clears __jwt but leaves the cached /users/@me promise alive, so getUserMe() in Api.ts can still return stale authenticated data after auth-session-expired. Update the 401/403 handling in the refresh flow to also invalidate the getUserMe cache (the cached successful promise/state used by Api.ts) at the same time you set __jwt to null and __lastRefreshOutcome to "expired", so downstream callers fetch fresh unauthenticated state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/client/Auth.ts`:
- Around line 266-271: The session-expired branch in Auth.ts clears __jwt but
leaves the cached /users/@me promise alive, so getUserMe() in Api.ts can still
return stale authenticated data after auth-session-expired. Update the 401/403
handling in the refresh flow to also invalidate the getUserMe cache (the cached
successful promise/state used by Api.ts) at the same time you set __jwt to null
and __lastRefreshOutcome to "expired", so downstream callers fetch fresh
unauthenticated state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f37db4a0-3d09-445f-8eb8-9f8585779413
📒 Files selected for processing (3)
src/client/AccountModal.tssrc/client/Auth.tstests/client/Auth.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/AccountModal.ts
- tests/client/Auth.test.ts
It's an app-level auth modal (relevant on the menu and in-game), not a game overlay — grouping it with multi-tab-modal etc. was misleading. Place it in its own "App-level overlays" section, still body-level (outside the in-[.in-game]: hidden container) so it renders in both contexts. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Problem
Players report being "logged out every few days" — intermittent, not the 30‑day session limit. The cause is client‑side and self‑inflicted: a transient refresh failure was treated as a definitive auth failure, and
logOut()is unconditionally destructive (it revokes the server session viaPOST /auth/logoutand wipes the localStorageplayer_persistent_id+ cosmetics).A non‑200 from
POST /auth/refreshis a resolvedfetch(Cloudflare 5xx/520‑524, a 429, or a spurious edge 401), so it skipped the network‑error catch path that #2636 hardened and fell intologOut(). One bad refresh → guests get a brand‑new identity (newpublicId, lost stats/cosmetics, no error); linked users get forced re‑login.getUserMe()did the same on a 401 from/users/@meon every app open.Fix
The guiding principle: transient/ambiguous failures must not revoke the session or destroy local identity. Only an explicit user logout does. And we now react to why a refresh failed rather than treating every failure the same.
Commit 1 — stop the bleeding
doRefreshJwt()— on a non‑200, clear__jwtonly (mirrors the existing network‑error path); nologOut().getUserMe()— treat a 401 like any other non‑200 (return false); nologOut().logOut()— newuserInitiatedflag gates thepersistent_id/cosmetics wipe; protects all error‑path callers for free.AccountModal.handleLogout()passesuserInitiated: true.Commit 2 — resilience + clear UX (instead of silently dropping a logged‑in user to a guest identity)
doRefreshJwt()now distinguishes failure causes:5xx/429) → retry up to 3× with backoff (0.5s, 1s), behind the existing single‑flight guard. Exhausted → clear__jwtonly, recover later.401/403) → soft logout: clear__jwt, preserve identity, and (if a session was active) dispatchauth-session-expired.getLastRefreshOutcome().<session-expired-modal>prompts re‑login — but only for users who were actually signed in (Api.wasLoggedIn(), from the last/users/@me). Guests never see it.Decision tree
persistent_id). Linked user → warning modal. Guest → silent.userInitiated=true).Tests
tests/client/Auth.test.ts: retry count on transient (5xx), no‑retry on definitive 401,expiredvstransientoutcome, identity preserved in all involuntary cases, user‑initiated wipe, and the active‑session‑expiry dispatch. Fulltests/clientsuite (518) +EnJsonSorted/TranslationSystempass;tsc/eslint/prettierclean.How to confirm in prod
Correlate a
POST /auth/logoutimmediately followed by a cookie‑less/auth/refreshthat mints a new guest; watch the backendunknown refresh token401 log + guest‑creation rate.Out of scope (separate issue)
CrazyGames third‑party‑iframe
SameSite=Laxcookie loss — a different failure mode (constant, not "every few days"); handled separately.🤖 Generated with Claude Code