[US-17.1 / OBT-254] fix(auth): commit session state atomically to stop Access Denied flash - #38
Open
levigtri wants to merge 2 commits into
Conversation
login() published the user into state before fetching myRoles and myManagedOrgs, leaving a 1-2s window where user was set, isLoading was already false, and the permission arrays were still empty. LoginPage redirects as soon as user exists, so the shell mounted and evaluated its guards against empty permissions, rendering AccessDeniedPage until the requests landed. Empty permission arrays are indistinguishable from "not loaded yet", which is exactly what the guards read. User and permissions now live in a single Session object, so a half-built session cannot be represented: login only commits once every permission call has resolved, and clears the tokens if any of them fails instead of leaving orphan tokens in localStorage with the user stuck on a denied screen. refreshUser reloads permissions alongside the user, which it previously left stale. AdminRoute now renders the spinner while the session loads rather than denying access, and logout resets the languages and phases caches so a second account no longer sees the first account's cached data.
📝 WalkthroughWalkthroughAuthentication state is consolidated into a session object, with centralized loading and token handling. Logout now clears cached language and phase data. Admin routes show a spinner while authentication restores, and both stores expose reset methods. ChangesAuth session lifecycle
Sequence Diagram(s)sequenceDiagram
participant AdminRoute
participant AuthProvider
participant AuthAPI
participant LanguagesStore
participant PhasesStore
AdminRoute->>AuthProvider: read isLoading
AuthProvider->>AuthAPI: load /me, roles, and managed organizations
AuthAPI-->>AuthProvider: session data
AuthProvider-->>AdminRoute: render spinner or authorization result
AuthProvider->>LanguagesStore: reset on logout
AuthProvider->>PhasesStore: reset on logout
AuthProvider-->>AdminRoute: navigate to /login
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/languagesStore.ts (1)
11-57: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStore
reset()doesn't invalidate in-flightfetch()calls in either store.Both
languagesStore.tsandphasesStore.tsadd areset()(called fromAuthContext.tsx'slogout()) that synchronously clears state, but theirfetch()implementations unconditionally commit fetched data on resolution regardless of whether areset()happened in between. A fetch started before logout that resolves after logout/reset silently repopulates the store with data fetched under the previous session, and — becauselastFetchedis set fresh — that stale data is then served as "current" for up toCACHE_TTL. This reintroduces exactly the class of stale-state-during-account-switch bug this PR is otherwise fixing.
src/stores/languagesStore.ts#L11-L57: add a generation/version counter incremented inreset(), and havefetch()skip committing (set(...)) if the counter changed since the call started; also guard the "duplicate fetch" subscriber resolution against a reset happening mid-wait.src/stores/phasesStore.ts#L12-L53: apply the same generation-counter guard around theset({ phases, dependencies, ... })commit infetch().🤖 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/stores/languagesStore.ts` around lines 11 - 57, Prevent pre-reset fetches from repopulating either store by adding a generation counter to src/stores/languagesStore.ts#L11-L57 and src/stores/phasesStore.ts#L12-L53: capture the generation at fetch start, increment it in reset(), and skip the fetch result commit when the generation changes. In languagesStore.ts, also make the duplicate-fetch subscriber stop waiting and resolve with the reset state when reset occurs during the wait.
🤖 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/stores/languagesStore.ts`:
- Line 11: Protect the languages store from stale in-flight requests by adding a
generation counter shared by reset() and fetch(). Increment the generation in
reset(), and have each fetch capture its generation before awaiting; only the
matching request may commit state or resolve duplicate-fetch subscribers.
Invalidate or retain those subscribers until a current-generation fetch
completes so they do not receive reset’s empty languages result.
In `@src/stores/phasesStore.ts`:
- Line 12: Update the phasesStore fetch/reset flow so an in-flight fetch cannot
commit results after reset() invalidates the current session state. Add the same
request-generation or cancellation guard used by languagesStore.ts, and have the
fetch completion update phases, dependencies, lastFetched, and loading only when
it still belongs to the active generation; preserve reset()’s cleared state
otherwise.
---
Outside diff comments:
In `@src/stores/languagesStore.ts`:
- Around line 11-57: Prevent pre-reset fetches from repopulating either store by
adding a generation counter to src/stores/languagesStore.ts#L11-L57 and
src/stores/phasesStore.ts#L12-L53: capture the generation at fetch start,
increment it in reset(), and skip the fetch result commit when the generation
changes. In languagesStore.ts, also make the duplicate-fetch subscriber stop
waiting and resolve with the reset state when reset occurs during the wait.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 85b78624-5b61-46ff-9f78-db51b5131dd8
📒 Files selected for processing (4)
src/App.tsxsrc/contexts/AuthContext.tsxsrc/stores/languagesStore.tssrc/stores/phasesStore.ts
| lastFetched: number | null | ||
| fetch: () => Promise<LanguageResponse[]> | ||
| invalidate: () => void | ||
| reset: () => void |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
reset() doesn't guard against an in-flight fetch() clobbering the reset state.
If reset() (called from logout()) runs while a fetch() is mid-flight, the pending call's .then still unconditionally commits { languages: data, lastFetched: Date.now(), loading: false } once it resolves — overwriting the just-reset store with data fetched under the previous session/account. Any caller queued on the "duplicate fetch" subscriber (Lines 30-37) also resolves early with the emptied languages array instead of the real result.
♻️ Proposed fix using a generation counter
+let generation = 0
+
export const useLanguagesStore = create<LanguagesStore>((set, get) => ({
languages: [],
loading: false,
lastFetched: null,
fetch: async () => {
const state = get()
if (state.lastFetched && Date.now() - state.lastFetched < CACHE_TTL && state.languages.length > 0) {
return state.languages
}
if (state.loading) {
return new Promise<LanguageResponse[]>((resolve) => {
const unsub = useLanguagesStore.subscribe((s) => {
if (!s.loading) {
unsub()
resolve(s.languages)
}
})
})
}
+ const myGeneration = generation
set({ loading: true })
try {
const { data } = await languagesAPI.list()
- set({ languages: data, lastFetched: Date.now(), loading: false })
+ if (myGeneration !== generation) return get().languages
+ set({ languages: data, lastFetched: Date.now(), loading: false })
return data
} catch {
- set({ loading: false })
+ if (myGeneration === generation) set({ loading: false })
return state.languages
}
},
invalidate: () => {
set({ lastFetched: null })
},
reset: () => {
+ generation++
set({ languages: [], loading: false, lastFetched: null })
},Also applies to: 54-57
🤖 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/stores/languagesStore.ts` at line 11, Protect the languages store from
stale in-flight requests by adding a generation counter shared by reset() and
fetch(). Increment the generation in reset(), and have each fetch capture its
generation before awaiting; only the matching request may commit state or
resolve duplicate-fetch subscribers. Invalidate or retain those subscribers
until a current-generation fetch completes so they do not receive reset’s empty
languages result.
| lastFetched: number | null | ||
| fetch: () => Promise<void> | ||
| invalidate: () => void | ||
| reset: () => void |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
reset() doesn't guard against an in-flight fetch() clobbering the reset state.
Same issue as languagesStore.ts: if a fetch() is in flight when reset() (called from logout()) runs, the pending call still unconditionally commits { phases: data, dependencies: depsMap, lastFetched: Date.now(), loading: false } once it resolves, silently undoing the reset with data fetched under the previous session.
Also applies to: 50-53
🤖 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/stores/phasesStore.ts` at line 12, Update the phasesStore fetch/reset
flow so an in-flight fetch cannot commit results after reset() invalidates the
current session state. Add the same request-generation or cancellation guard
used by languagesStore.ts, and have the fetch completion update phases,
dependencies, lastFetched, and loading only when it still belongs to the active
generation; preserve reset()’s cleared state otherwise.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Signing out of one account and into another briefly rendered the Access Denied screen for 1-2 seconds before the app settled into its normal state. The tokens and the refresh interceptor were never at fault — the session state was.
AuthContext.login()published the user into React state and only then fetched the user's permissions (myRoles,myManagedOrgs). Between those two points the app sat in a half-built session:userwas set,isLoadingwas alreadyfalse(it is switched off at boot when there is no token), but the permission arrays were still empty.LoginPageredirects the momentuserexists, soAppShellmounted and evaluated its guards against empty permissions and renderedAccessDeniedPage. When the requests landed, the arrays filled and the screen silently corrected itself.The root cause is representational: an empty permission array is indistinguishable from "not loaded yet", and that is precisely what the guards read. This PR makes the session atomic —
userand permissions now live in oneSessionobject that is committed in a single state update — so the half-built session that produced the flash can no longer be represented at all.A platform admin never saw the bug (
is_platform_adminarrives inside/auth/me), and a page refresh never triggered it (restoreSession()loads everything underisLoading), which is why it only surfaced when switching into a manager account.Changes
Atomic session state —
src/contexts/AuthContext.tsx—user,appRolesandmanagedOrgIdsare consolidated into a singleSessionobject behind oneuseState. A sharedloadSession()helper fetches the user and both permission endpoints in parallel and returns the fully-formed session, sologin()andrestoreSession()commit through the same path and neither can publish a partial one.Login no longer leaves orphan tokens on failure —
src/contexts/AuthContext.tsx— if any permission call rejects,login()clears both tokens from localStorage and rethrows instead of leaving the user authenticated-but-permissionless, stranded on a denied screen with only a "Sign out" button.refreshUser()reloads permissions too —src/contexts/AuthContext.tsx— it previously refreshed only the user object, leaving roles and managed orgs stale after a role change.AdminRoutewaits for the session —src/App.tsx— it denied access without checkingisLoading. It now renders the spinner while the session loads. It was protected transitively byAppShelltoday; this closes the gap directly.Caches cleared on logout —
src/stores/languagesStore.ts,src/stores/phasesStore.ts,src/contexts/AuthContext.tsx— both Zustand caches (5min and 2min TTL) survived a logout, so a second account saw the first account's cached languages and phases until the TTL expired.logout()now resets both.Type of Change
Testing
/api/auth/my-roles). The user stays on the login page with an error toast, andtc_access_token/tc_refresh_tokenare absent from localStorage — previously the tokens persisted and the user was stuck on Access Denied.npm run typecheck,npm run lint,npm run build.Summary by CodeRabbit