prevent app refetch on token silent renew#1102
Open
ghazwarhili wants to merge 24 commits into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR migrates user type references from ChangesUser Profile Type Migration and WebSocket Token Authentication
🚥 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.
🧹 Nitpick comments (1)
src/components/notifications/NotificationsProvider.tsx (1)
21-27:appendTokendoesn't account for URL fragments.If
urlever contains a#fragment, appending?access_token=...or&access_token=...after the fragment will make the token part of the fragment rather than the query string. Not an issue for current WebSocket URLs (which don't use fragments), but worth noting for robustness. A safer approach is to useURL/URLSearchParams, though that adds overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/NotificationsProvider.tsx` around lines 21 - 27, appendToken currently appends the access_token after the entire URL, which places it inside a fragment if the URL contains a '#' — fix appendToken to preserve any fragment by splitting the URL at the '#' (or use the URL/URLSearchParams API) so the query param is inserted into the main URL before the fragment, then reattach the fragment; update the function appendToken to detect and separate fragment, compute separator for the base part, append encoded access_token to the base, and finally append '#' + fragment if present.
🤖 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/components/notifications/NotificationsProvider.tsx`:
- Around line 21-27: appendToken currently appends the access_token after the
entire URL, which places it inside a fragment if the URL contains a '#' — fix
appendToken to preserve any fragment by splitting the URL at the '#' (or use the
URL/URLSearchParams API) so the query param is inserted into the main URL before
the fragment, then reattach the fragment; update the function appendToken to
detect and separate fragment, compute separator for the base part, append
encoded access_token to the base, and finally append '#' + fragment if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8cf6099-21f6-4a92-839d-1981cb4bb51e
📒 Files selected for processing (1)
src/components/notifications/NotificationsProvider.tsx
flomillot
reviewed
Apr 20, 2026
Contributor
flomillot
left a comment
flomillot
left a comment
There was a problem hiding this comment.
As discussed, I dont think it was necessary, but I let you check with the TL
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/announcement/useGlobalAnnouncement.ts (1)
26-47:⚠️ Potential issue | 🟠 MajorEffect dependency is still too broad and can refetch on silent token renewal.
The
userProfileobject reference can change during OIDC token renewal even when user identity remains the same. The library recreates the Profile instance when merging updated claims. Depend on the stablesubclaim instead to prevent unnecessaryfetchCurrentAnnouncement()calls.Suggested fix
- useEffect(() => { - if (userProfile) { + useEffect(() => { + if (userProfile?.sub) { fetchCurrentAnnouncement() .then((announcementDto) => { @@ } else { setAnnouncementInfos(undefined); // user disconnected } - }, [userProfile]); + }, [userProfile?.sub]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/announcement/useGlobalAnnouncement.ts` around lines 26 - 47, The effect currently depends on the whole userProfile object so it refires on silent OIDC token renewals; change the dependency to the stable user identifier instead (use userProfile?.sub) so fetchCurrentAnnouncement() is only called when the actual user changes—update the useEffect dependency array to depend on userProfile?.sub and keep the same logic inside (fetchCurrentAnnouncement, setAnnouncementInfos, error handling) while removing userProfile from the deps.
🧹 Nitpick comments (1)
src/components/announcement/AnnouncementBanner.tsx (1)
35-35: Tighten auth gating to avoid permissive “logged-in” checks.
userProfile?: Profile | {}combined withuserProfile !== undefinedcan incorrectly allow truthy placeholder objects. Prefer a strictProfileprop and boolean truthiness check.Suggested fix
- userProfile?: Profile | {}; + userProfile?: Profile; @@ - <Collapse in={userProfile !== undefined && visible} unmountOnExit sx={sx} style={{ margin: theme.spacing(1) }}> + <Collapse in={!!userProfile && visible} unmountOnExit sx={sx} style={{ margin: theme.spacing(1) }}>Also applies to: 97-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/announcement/AnnouncementBanner.tsx` at line 35, The prop type for AnnouncementBanner currently allows a permissive placeholder (userProfile?: Profile | {}), which combined with checks like userProfile !== undefined can treat empty objects as "logged-in"; change the prop to a strict optional Profile (userProfile?: Profile) and update any truthiness checks in AnnouncementBanner (and the other occurrence that checks userProfile !== undefined) to use explicit existence checks such as if (userProfile) or if (userProfile != null) or validate a specific Profile field (e.g., userProfile.id or userProfile.email) before treating the user as authenticated so empty objects no longer pass the gate.
🤖 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/components/notifications/NotificationsProvider.tsx`:
- Line 13: NotificationsProvider currently imports getUserToken and doesn't
expose the documented optional token-supplier API; update
NotificationsProviderProps to add an optional tokenSupplier (e.g. () =>
Promise<string> | string) and remove the hard dependency on getUserToken by
using the provided tokenSupplier where getUserToken is currently called
(references: NotificationsProviderProps, NotificationsProvider
function/component, and the places around the existing getUserToken usage at
lines noted in the review). For backward compatibility you may fallback to
calling getUserToken only when tokenSupplier is not provided, but ensure all
uses at the previously-importing sites (around the message fetch/auth code
paths) read the token via the new prop and types are updated accordingly.
- Around line 22-28: The code appends an access_token query param but
getUserToken() currently returns user.id_token; change the token source so
WebSocket auth uses the OAuth2 access token: update getUserToken (or its callers
in NotificationsProvider) to return user.access_token instead of user.id_token,
and update any places that build the socket URL (the initial connect and
reconnect logic that call appendToken) to pass that access token; ensure
appendToken continues to encode the token and that reconnect logic (the
reconnection call sites) uses the same getUserToken/access_token value.
In `@src/components/topBar/UserInformationDialog.tsx`:
- Around line 44-55: The dialog can display stale userDetails when switching
users or closing and reopening; update the component to clear userDetails
whenever the dialog is closed or the selected user changes by calling the state
setter (setUserDetails) when openDialog becomes false or when userProfile.sub
changes. Add a useEffect that watches [openDialog, userProfile?.sub] and calls
setUserDetails(null) if !openDialog or when userProfile?.sub is different, or
alternatively reset userDetails in the existing effect before calling
getUserDetails(userProfile.sub) to ensure old details are not shown. Ensure you
reference the existing userDetails and its setter (setUserDetails) and keep
getUserDetails usage intact.
---
Outside diff comments:
In `@src/components/announcement/useGlobalAnnouncement.ts`:
- Around line 26-47: The effect currently depends on the whole userProfile
object so it refires on silent OIDC token renewals; change the dependency to the
stable user identifier instead (use userProfile?.sub) so
fetchCurrentAnnouncement() is only called when the actual user changes—update
the useEffect dependency array to depend on userProfile?.sub and keep the same
logic inside (fetchCurrentAnnouncement, setAnnouncementInfos, error handling)
while removing userProfile from the deps.
---
Nitpick comments:
In `@src/components/announcement/AnnouncementBanner.tsx`:
- Line 35: The prop type for AnnouncementBanner currently allows a permissive
placeholder (userProfile?: Profile | {}), which combined with checks like
userProfile !== undefined can treat empty objects as "logged-in"; change the
prop to a strict optional Profile (userProfile?: Profile) and update any
truthiness checks in AnnouncementBanner (and the other occurrence that checks
userProfile !== undefined) to use explicit existence checks such as if
(userProfile) or if (userProfile != null) or validate a specific Profile field
(e.g., userProfile.id or userProfile.email) before treating the user as
authenticated so empty objects no longer pass the gate.
🪄 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: 95aa6798-915d-4f68-ae61-cfb53e49d553
📒 Files selected for processing (7)
src/components/announcement/AnnouncementBanner.tsxsrc/components/announcement/AnnouncementNotification.tsxsrc/components/announcement/useGlobalAnnouncement.tssrc/components/notifications/NotificationsProvider.tsxsrc/components/topBar/TopBar.tsxsrc/components/topBar/UserInformationDialog.tsxsrc/components/topBar/tests/TopBar.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/notifications/NotificationsProvider.tsx (1)
88-88: Consider removingconnectionsReffrom the dependency array.Refs are stable across renders and don't need to be listed as effect dependencies. ESLint's
react-hooks/exhaustive-depsrule typically exempts refs. Removing it clarifies intent: this effect reacts only toaccessTokenchanges.- }, [accessToken, connectionsRef]); + }, [accessToken]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/NotificationsProvider.tsx` at line 88, The useEffect in NotificationsProvider currently lists connectionsRef in its dependency array but refs are stable and shouldn't be included; update the effect that uses accessToken and connectionsRef so the dependency array contains only accessToken (remove connectionsRef) and ensure any values from the ref are accessed inside the effect body (no other unstable variables are omitted); this clarifies intent in the NotificationsProvider component and satisfies react-hooks expectations for useEffect.
🤖 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/components/notifications/NotificationsProvider.tsx`:
- Line 88: The useEffect in NotificationsProvider currently lists connectionsRef
in its dependency array but refs are stable and shouldn't be included; update
the effect that uses accessToken and connectionsRef so the dependency array
contains only accessToken (remove connectionsRef) and ensure any values from the
ref are accessed inside the effect body (no other unstable variables are
omitted); this clarifies intent in the NotificationsProvider component and
satisfies react-hooks expectations for useEffect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a58541de-9029-458c-bec7-d151a9d67c2a
📒 Files selected for processing (1)
src/components/notifications/NotificationsProvider.tsx
flomillot
requested changes
May 5, 2026
ghazwarhili
commented
May 6, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/notifications/utils.ts`:
- Line 7: Update the getUserToken function in src/redux/commonStore.ts so it
returns the user's access token (user?.access_token) instead of the ID token
(user?.id_token); this ensures appendToken (used by appendToken in
src/components/notifications/utils.ts) supplies a proper access_token query
parameter for WebSocket auth—locate getUserToken and change its return value to
user?.access_token (and run any related unit/manual checks to confirm consumers
like appendToken still expect an access_token).
🪄 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: 36ed8928-e300-47ed-a728-2e8ef1e41564
📒 Files selected for processing (4)
src/components/announcement/AnnouncementBanner.tsxsrc/components/notifications/NotificationsProvider.tsxsrc/components/notifications/utils.tssrc/components/topBar/UserInformationDialog.tsx
sBouzols
reviewed
May 6, 2026
sBouzols
reviewed
May 6, 2026
sBouzols
reviewed
May 6, 2026
flomillot
approved these changes
May 6, 2026
jonenst
reviewed
May 13, 2026
| const rws = new ReconnectingWebSocket(() => url, [], { | ||
| // this option set the minimum duration being connected before reset the retry count to 0 | ||
| .flatMap(([urlKey, url]) => { | ||
| const urlWithToken = appendToken(url); |
Contributor
There was a problem hiding this comment.
get token before and test only once if we have it ?
…new' into prevent-app-refetch-on-silent-renew
…new' into prevent-app-refetch-on-silent-renew
|
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
4 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
Emit token-free WebSocket URLs in useNotificationsUrlGenerator, stable across silent renews (depend on isAuthenticated, not on the token value).
Pass getUserToken as getToken prop to NotificationsProvider so the fresh token is appended at each connection/reconnection.