use user Profile instead of user#4005
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 the app entrypoint from ChangesSilent Renew Auth Flow and Entrypoint Migration
Sequence Diagram(s)sequenceDiagram
participant Browser
participant EntryPoint as index.tsx
participant SilentRenew
participant AppWrapper
Browser->>EntryPoint: load /src/index.tsx
EntryPoint->>EntryPoint: check pathname.endsWith(SILENT_RENEW_CALLBACK_PATH)
alt silent-renew callback path
EntryPoint->>SilentRenew: render SilentRenew
SilentRenew->>SilentRenew: initializeAuthenticationProd(cached IdP settings)
SilentRenew-->>SilentRenew: store UserManager, invoke handleSilentRenewCallback
else other path
EntryPoint->>EntryPoint: Promise.all(polyfill, font, CSS, config, app-wrapper)
EntryPoint->>AppWrapper: render AppWrapper
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
…into cleanup-apache-config-requests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/components/silent-renew-app.tsx (1)
30-34: 💤 Low valueSimplify the callback - the null check is redundant.
Since
handleSilentRenewCallbackClosureis recreated wheneveruserManagerchanges (it's in the dependency array), the null check on line 31 will never be true when the callback is invoked afteruserManageris set. You can either remove the check or passuserManagerdirectly to the handler.♻️ Proposed simplification
Option 1: Remove the null check
const handleSilentRenewCallbackClosure = useCallback(() => { - if (userManager) { - handleSilentRenewCallback(userManager); - } + handleSilentRenewCallback(userManager!); }, [userManager]);Option 2: Keep it defensive and clear
The current implementation is defensive and readable. If you prefer safety over minor optimization, the existing code is fine.
🤖 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/components/silent-renew-app.tsx` around lines 30 - 34, The null check on userManager inside the handleSilentRenewCallbackClosure callback is redundant because userManager is included in the dependency array, ensuring the callback is only recreated when userManager changes and will always be defined when the callback executes. Remove the if (userManager) condition and directly call handleSilentRenewCallback(userManager) to simplify the code while maintaining the same behavior.
🤖 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/parameters-tabs.tsx`:
- Around line 448-454: The NetworkVisualizationParametersInline component is
receiving a userProfile prop but expects a prop named user according to its type
definition. Locate the NetworkVisualizationParametersInline component invocation
and rename the userProfile prop to user, changing userProfile={userProfile} to
user={userProfile} to ensure the component receives the user prop correctly.
- Around line 99-103: The custom equality function in the useSelector hook for
userProfile contains a redundant check for the `profile` field which does not
exist on the UserProfile type, and only checks four specific fields, missing
other OIDC standard claims that could change. Fix this by removing the
`a?.profile === b?.profile` check from the equality comparison, and either
remove the custom equality function entirely to rely on default shallow
comparison, or replace it with logic that properly compares the complete
userProfile object including all OIDC claims that should trigger re-renders.
In `@src/index.tsx`:
- Line 15: The pathname check using endsWith is too permissive and will match
any path ending with the suffix, potentially matching unintended routes. Replace
the endsWith comparison in the window.location.pathname condition with an exact
path match using strict equality (===) to ensure only the exact route
'/silent-renew-callback' triggers the silent-renew-callback component, not other
routes that happen to end with that string.
In `@src/services/utils.ts`:
- Around line 54-61: The fetchIdpSettings() function does not handle errors when
calling localStorage.setItem(), which throws a DOMException in private browsing
mode or restrictive environments, causing the promise to reject. Wrap the
localStorage.setItem call in a try-catch block within the second .then() handler
to gracefully handle the DOMException. In the catch block, log the error but
continue execution and still return the settings, ensuring the function
completes successfully even when localStorage is unavailable.
- Around line 65-75: The getCachedIdpSettings function's try-catch block only
wraps the JSON.parse call, but localStorage.getItem can throw errors in
restrictive environments. Move the localStorage.getItem(IDP_SETTINGS_CACHE_KEY)
call inside the try-catch block so that any exceptions from getItem are also
caught and handled. This ensures the function gracefully falls back to
fetchIdpSettings() when localStorage is unavailable or throws an error, not just
when the JSON parsing fails.
---
Nitpick comments:
In `@src/components/silent-renew-app.tsx`:
- Around line 30-34: The null check on userManager inside the
handleSilentRenewCallbackClosure callback is redundant because userManager is
included in the dependency array, ensuring the callback is only recreated when
userManager changes and will always be defined when the callback executes.
Remove the if (userManager) condition and directly call
handleSilentRenewCallback(userManager) to simplify the code while maintaining
the same behavior.
🪄 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: 08c7c860-f8c1-4fdc-ae75-d69a47f8e052
📒 Files selected for processing (7)
index.htmlsrc/components/parameters-tabs.tsxsrc/components/silent-renew-app.tsxsrc/index.jsxsrc/index.tsxsrc/services/utils.tssrc/vite-env.d.ts
💤 Files with no reviewable changes (1)
- src/index.jsx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 860 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 812 | 1 | 47 | 0 |
Click to see the invalid file list
- silent-renew-callback.html
This reverts commit 0f75bc9.
…into cleanup-apache-config-requests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/index.tsx (1)
15-17: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse exact callback-path matching at Line 15.
endsWith(...)can incorrectly match unrelated routes (for example, nested paths ending in the same suffix). Match the callback route exactly.Suggested fix
-if (globalThis.location.pathname.endsWith(SILENT_RENEW_CALLBACK_PATH)) { +if (globalThis.location.pathname === SILENT_RENEW_CALLBACK_PATH) { root.render(<SilentRenew />); } else {🤖 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/index.tsx` around lines 15 - 17, The route check in the top-level render logic currently uses endsWith on globalThis.location.pathname, which can incorrectly treat unrelated nested paths as the callback route. Update the SilentRenew routing condition in src/index.tsx to perform an exact pathname match against SILENT_RENEW_CALLBACK_PATH, and keep the existing root.render(<SilentRenew />) branch unchanged while preserving the rest of the entrypoint logic.
🤖 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/index.tsx`:
- Around line 18-28: The startup import chain in index.tsx currently ends after
Promise.all(...).then(...) without handling failures, so add a terminal catch to
the Promise chain around the dynamic imports and AppWrapper render. Update the
bootstrap flow so any rejection from Promise.all or the subsequent
import('./components/app-wrapper') is handled in one place, with a visible
failure path such as logging the error and/or surfacing it clearly instead of
leaving an unhandled rejection.
---
Duplicate comments:
In `@src/index.tsx`:
- Around line 15-17: The route check in the top-level render logic currently
uses endsWith on globalThis.location.pathname, which can incorrectly treat
unrelated nested paths as the callback route. Update the SilentRenew routing
condition in src/index.tsx to perform an exact pathname match against
SILENT_RENEW_CALLBACK_PATH, and keep the existing root.render(<SilentRenew />)
branch unchanged while preserving the rest of the entrypoint logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
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 `@package.json`:
- Around line 14-15: The dependency entry is malformed and causes npm to reject
the manifest: in package.json, fix the corrupted `@grgit` idsuite/commons-ui key
to the valid `@gridsuite/commons-ui` name, and make sure the version for
`@gridsuite/commons-ui` matches the intended bump to 0.237.0 rather than 0.240.0.
Keep only the correctly named dependency entry so npm install/npm ci can parse
the package manifest successfully.
🪄 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: 81516c88-1740-4a3c-b1be-f68e352b87c7
📒 Files selected for processing (2)
package.jsonsrc/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.tsx
| "@grgit idsuite/commons-ui": "0.237.0", | ||
| "@gridsuite/commons-ui": "0.240.0", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Malformed dependency key breaks npm install.
Line 14 declares "@GRGit idsuite/commons-ui" — an invalid npm package name (contains a space, likely a corrupted rename of @gridsuite/commons-ui). npm package names must match a strict pattern (no spaces), so npm install/npm ci will fail. Additionally, line 15 already declares the real @gridsuite/commons-ui dependency but pinned to 0.240.0, which conflicts with the PR objective stating the version bump is to 0.237.0.
This looks like a bad merge/diff artifact rather than an intentional change.
🐛 Proposed fix
- "`@grgit` idsuite/commons-ui": "0.237.0",
- "`@gridsuite/commons-ui`": "0.240.0",
+ "`@gridsuite/commons-ui`": "0.237.0",📝 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.
| "@grgit idsuite/commons-ui": "0.237.0", | |
| "@gridsuite/commons-ui": "0.240.0", | |
| "`@gridsuite/commons-ui`": "0.237.0", |
🤖 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 `@package.json` around lines 14 - 15, The dependency entry is malformed and
causes npm to reject the manifest: in package.json, fix the corrupted `@grgit`
idsuite/commons-ui key to the valid `@gridsuite/commons-ui` name, and make sure
the version for `@gridsuite/commons-ui` matches the intended bump to 0.237.0
rather than 0.240.0. Keep only the correctly named dependency entry so npm
install/npm ci can parse the package manifest successfully.
2 participants
PR Summary
Context
On every OIDC silent renew, oidc-client-ts loads
/silent-renew-callbackinside ahidden iframe. Because
index.tsxalways rendered<AppWrapper />, the entireapplication was bootstrapped inside that iframe just to end up calling
signinSilentCallback(): Redux store,IntlProviderand all translations,AG Grid registration, MUI theme,
SnackbarProviderandNotificationsProvider.NotificationsProvideradditionally fetchedenv.jsonand opened a notificationWebSocket on each renewal — all of it pure overhead repeated at every token refresh.
Changes
index.tsx: branch on the pathname before rendering. On/silent-renew-callbackrender a new lightweight
SilentRenewApp; otherwise render<AppWrapper />as before.components/silent-renew-app.tsx: rebuilds theUserManagerviainitializeAuthenticationProd(..., isSilentRenew=true, ...)(same settings, samelocalStoragestate store, same Azure authority hack as the parent) and completesthe flow through commons-ui's
SilentRenewCallbackHandler.utils/rest-api.ts:fetchIdpSettingsnow caches the result inlocalStorage;added
getCachedIdpSettings, used only by the silent-renew path, which reads thatcache (no network) and falls back to a real fetch if it is missing/corrupted.
Result
Inside the silent-renew iframe, requests go from a full SPA boot
(
idpSettings.json+env.json+ WebSocket + app bundle execution) down to asingle
POST /token— the actual token exchange. No more spurious WebSocketreconnections triggered by the renewal iframe.
Testing
env.json, no longerre-fetches
idpSettings.json, and does not open a WebSocket — onlyPOST /tokenremains.notification WebSocket is not reconnected on each cycle.