prevent app refetch on token silent renew#3898
Open
ghazwarhili wants to merge 18 commits into
Open
Conversation
|
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)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughRemoves the ad-hoc user-store singleton, shifts token usage to explicit parameters, updates websocket URL generation to stop depending on a global token, and changes components to consume ChangesSingle cohesive refactor
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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
🧹 Nitpick comments (1)
src/redux/user-store.ts (1)
28-35: Reuse the exportedUserTokentype instead of redeclaring it inline.The
userTokenshape here duplicates theUserTokentype now exported fromsrc/redux/reducer.type.ts. Duplicated structural types drift over time (e.g., ifrefresh_tokenortoken_typeis added later). Importing the canonical type keeps both in lockstep.♻️ Proposed refactor
-import { User } from 'oidc-client'; +import { User } from 'oidc-client'; +import type { UserToken } from './reducer.type'; type UserStoreState = { user: User | null; - userToken: { - access_token: string; - id_token: string; - expires_at: number; - } | null; + userToken: UserToken | null; };No circular dependency risk:
reducer.type.tshas no imports from redux files (store,reducer,user-store), so using it here is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/redux/user-store.ts` around lines 28 - 35, Replace the inline userToken shape in the UserStoreState type with the canonical exported UserToken type: import UserToken from the reducer.type file and change the userToken property type from the inline object to UserToken | null (keeping user: User | null unchanged); update any references to ensure they use UserStoreState and the imported UserToken so the single source of truth is used instead of the duplicated inline declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/use-notifications-url-generator.ts`:
- Around line 27-58: The current useMemo in use-notifications-url-generator
embeds a token via getUrlWithToken() but does not depend on the token, leading
to stale tokens on silent refresh; either stop baking the token into the
memorized URLs or include the live token in the memo deps. Fix by changing the
memo to generate token-less WS URLs (use wsBase + PREFIX_* paths and query
params but omit access_token) so the external
NotificationsProvider/useNotificationsListener can call getUserToken() at
connect time, or alternatively add the current access token (or getUserToken()
result / accessToken selector) to the dependency array of the useMemo so
getUrlWithToken(...) is re-run on token refresh; update references in
useNotificationsUrlGenerator (useMemo, getUrlWithToken, NotificationsUrlKeys,
studyUuid) and confirm provider behavior in useNotificationsListener before
choosing the token-less approach.
In `@src/redux/user-store.ts`:
- Around line 48-49: getUserToken currently returns userToken?.id_token but
callers (e.g., getUrlWithToken) expect the OAuth access token; update
getUserToken() to return userStore?.getState().userToken?.access_token ??
undefined so it returns the access token from the UserToken shape (preserving
the undefined fallback) and ensure any references to getUserToken() continue to
treat the value as an access token.
---
Nitpick comments:
In `@src/redux/user-store.ts`:
- Around line 28-35: Replace the inline userToken shape in the UserStoreState
type with the canonical exported UserToken type: import UserToken from the
reducer.type file and change the userToken property type from the inline object
to UserToken | null (keeping user: User | null unchanged); update any references
to ensure they use UserStoreState and the imported UserToken so the single
source of truth is used instead of the duplicated inline declaration.
🪄 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: 596b1866-e2d9-4593-9f97-a9ae4ca82d6e
📒 Files selected for processing (4)
src/hooks/use-notifications-url-generator.tssrc/redux/reducer.tssrc/redux/reducer.type.tssrc/redux/user-store.ts
flomillot
reviewed
Apr 20, 2026
Contributor
flomillot
left a comment
flomillot
left a comment
There was a problem hiding this comment.
I would rather keep the previous code of useNotificationsUrlGenerator and improving it a bit, by injecting tokenId as prop to getUrlWithToken instead of fetching it again as it's alredy present in the component
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/redux/reducer.ts (1)
1022-1028: Logic is correct; consider reordering the guard for readability.All paths check out:
- Initial login (
state.user === null): falls through →state.userassigned.- Silent renew (same
profile.sub): early return, butstate.tokenIdwas already refreshed on line 1023 so the WebSocket layer still picks up the new token viagetUserToken/selector.- Logout (
action.user == null,state.userset): sub comparison is'...' === undefined→ false → falls through →state.user = null,tokenId = undefined.- User switch (different sub): falls through →
state.userreplaced.Minor readability nit — putting the null check first reads more naturally and short-circuits the optional chaining:
♻️ Optional readability tweak
builder.addCase(USER, (state, action: UserAction) => { state.tokenId = action.user?.id_token ?? undefined; - if (state.user?.profile?.sub === action.user?.profile?.sub && state.user !== null) { + if (state.user !== null && state.user.profile?.sub === action.user?.profile?.sub) { return; } state.user = action.user; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/redux/reducer.ts` around lines 1022 - 1028, Reorder the early-return guard in the USER reducer for readability: first check that state.user is not null, then compare profile.sub to the incoming action.user profile.sub before returning; keep the token update (state.tokenId = action.user?.id_token ?? undefined) as-is and then set state.user = action.user when the guard doesn't short-circuit. Reference: the USER case that takes (state, action: UserAction), state.user, state.tokenId, and action.user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/redux/reducer.ts`:
- Around line 1022-1028: Reorder the early-return guard in the USER reducer for
readability: first check that state.user is not null, then compare profile.sub
to the incoming action.user profile.sub before returning; keep the token update
(state.tokenId = action.user?.id_token ?? undefined) as-is and then set
state.user = action.user when the guard doesn't short-circuit. Reference: the
USER case that takes (state, action: UserAction), state.user, state.tokenId, and
action.user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07fc4c29-26eb-4bd8-b14d-acaff775b89f
📒 Files selected for processing (7)
src/components/study-container.jsxsrc/hooks/use-notifications-url-generator.tssrc/redux/reducer.tssrc/redux/reducer.type.tssrc/redux/store.tssrc/redux/user-store.tssrc/services/utils.ts
💤 Files with no reviewable changes (2)
- src/redux/store.ts
- src/redux/user-store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/redux/reducer.type.ts
- src/hooks/use-notifications-url-generator.ts
flomillot
requested changes
May 5, 2026
flomillot
approved these changes
May 6, 2026
flomillot
reviewed
May 7, 2026
| }, []); | ||
|
|
||
| const user = useSelector((state) => state.user); | ||
| const userProfile = useSelector((state) => state.user?.profile ?? null, shallowEqual); |
Contributor
There was a problem hiding this comment.
@coderabbitai is shallowEqual the appropriate equal method here ?
|
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
2 participants
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.
PR Summary
Each oidc silent renew dispatched a user action carrying a fresh User instance (new access_token, id_token, expires_at) while the identity (profile.sub) was unchanged.
Two issues cascaded from this:
The reducer replaced state.user unconditionally, changing its reference on every renew. All components selecting state.user re-rendered, and effects depending on user re-ran, triggering redundant fetches (network visualization parameters, spreadsheet config, optional services, etc.).
The WebSocket URLs generated by useNotificationsUrlGenerator embedded the token as a query parameter, so token rotation changed the URL objects. NotificationsProvider then tore down and recreated all WebSockets, and each new onopen broadcast caused listeners (useNodeAliases, network elements, etc.) to resync their data.
Changes:
Split state.user: state.user keeps a stable reference across silent renews (only changes when profile.sub changes), and state.userToken holds the rotating tokens used by getUserToken and the WS layer.
useNotificationsUrlGenerator now emits token-free URLs based on an isAuthenticated boolean, so the urls object stays stable when the token rotates.
The fresh token is appended at WebSocket connection/reconnection time via getUserToken(), preserving reconnection behavior for real disconnects.