-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(ui): add fallback to okta renew stratagy #25143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...etadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx
Outdated
Show resolved
Hide resolved
...etadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx
Outdated
Show resolved
Hide resolved
...ta-ui/src/main/resources/ui/src/components/Auth/AppCallbacks/Auth0Callback/Auth0Callback.tsx
Outdated
Show resolved
Hide resolved
|
Gitar fix CI |
The CI failures appear to be unrelated to this PR's changes. The failing tests are in areas not modified by this PR: Failing Tests:
This PR only modifies:
However, the code review identified 2 critical bugs in the Okta token renewal logic that should be fixed:
The CI test failures suggest pre-existing issues with the test suite or infrastructure, not problems introduced by this PR. Would you like me to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request aims to improve UI performance and reliability through Zustand state management optimizations, enhanced authentication flows, and memoization improvements. The changes focus on reducing unnecessary re-renders using useShallow selectors and improving error handling in authentication callbacks.
Key Changes:
- Replaced direct
useApplicationStore()calls withuseShallowselectors in multiple components to prevent unnecessary re-renders - Added fallback logic to Okta token renewal for handling missing tokens
- Refactored Auth0Callback to use
useEffectwith improved error handling
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| WebAnalyticsProvider.tsx | Optimized analytics initialization with useMemo to memoize based on currentUserId |
| GlobalSearchBar.tsx | Applied useShallow selector to extract only needed store values |
| Auth0Callback.tsx | Refactored to use useEffect for token retrieval with separate error state handling |
| OktaAuthenticator.tsx | Added fallback to redirect when tokens are missing during renewal |
| UnAuthenticatedAppRouter.tsx | Applied useShallow selector for authConfig and isSigningUp |
| AppRouter.tsx | Applied useShallow selector, added SAML callback route, wrapped export in memo |
| App.tsx | Applied useShallow selector for application config values |
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx:63
- The renewToken method has been significantly modified to include fallback logic (lines 44-63), but there are no test cases for OktaAuthenticator.tsx. Other authenticators in the same directory (Auth0Authenticator, BasicAuthAuthenticator, GenericAuthenticator, MsalAuthenticator) all have corresponding test files. Tests should be added to verify the new fallback behavior when tokens don't exist, token renewal, and error handling.
const renewToken = async () => {
const existingIdToken = oktaAuth.tokenManager.get('idToken');
const existingAccessToken = oktaAuth.tokenManager.get('accessToken');
// Add fallback if renewToken fails
if (!existingIdToken && !existingAccessToken) {
await oktaAuth.signInWithRedirect();
const newToken = oktaAuth.getIdToken() ?? '';
return newToken;
}
const tokens = await oktaAuth.token.renewTokens();
oktaAuth.tokenManager.setTokens(tokens);
const newToken = tokens?.idToken?.idToken ?? oktaAuth.getIdToken() ?? '';
await setOidcToken(newToken);
return newToken;
};
openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AppRouter.tsx
Outdated
Show resolved
Hide resolved
...etadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx
Outdated
Show resolved
Hide resolved
...ta-ui/src/main/resources/ui/src/components/Auth/AppCallbacks/Auth0Callback/Auth0Callback.tsx
Outdated
Show resolved
Hide resolved
openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AppRouter.tsx
Outdated
Show resolved
Hide resolved
...etadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx
Outdated
Show resolved
Hide resolved
...etadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx
Outdated
Show resolved
Hide resolved
|
@chirag-madlani I've opened a new pull request, #25152, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Remove unnecessary React.memo wrapper from AppRouter - Remove memo import from React imports - Export AppRouter directly without memo wrapper - AppRouter has no props, so memo provides no performance benefit Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
🔍 CI failure analysis for c365e4f: Three Audit Logs export tests consistently failing across multiple runs - unrelated to PR's auth optimizations, indicates pre-existing issue with export feature.IssueThree Playwright E2E tests in the Audit Logs Export Functionality suite failed in shard 3/6, completely unrelated to this PR's authentication optimization changes. Failed Tests (Consistent Across Multiple Runs)
All three tests are in Pattern AnalysisThese same 3 tests have failed consistently across multiple CI runs on this PR:
This indicates a persistent issue with audit log export functionality, not a flaky test. Additional Context
Root CauseThe failures are in backend audit log export functionality, which involves:
Why This Is Unrelated to the PRThis PR only modifies:
None of these changes touch:
AssessmentThese test failures represent a pre-existing issue with the audit log export feature that is unrelated to this PR's authentication and performance optimizations. The consistent failure pattern across multiple runs suggests:
The solution would be to investigate the audit log export functionality separately from this PR. Code Review 👍 Approved with suggestionsGood performance optimizations with useShallow and useMemo. The Okta token renewal fallback improvements add error handling but the previous findings about async behavior and empty string returns remain unaddressed.
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)
|
|
Changes have been cherry-picked to the 1.11.5 branch. |
* feat(ui): add fallback to okta renew stratagy * Remove unnecessary React.memo wrapper from AppRouter (#25152) * Initial plan * Remove unnecessary React.memo wrapper from AppRouter - Remove memo import from React imports - Export AppRouter directly without memo wrapper - AppRouter has no props, so memo provides no performance benefit Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com> * address comments * fix errors * add tests --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> (cherry picked from commit b626e08)



This pull request focuses on improving performance and reliability in the UI by optimizing how state is selected from the application store, enhancing authentication flows, and making some component-level improvements. The most significant changes involve adopting the
useShallowselector from Zustand to minimize unnecessary re-renders, improving error handling in authentication callbacks, and adding memoization for analytics initialization.State management optimizations:
useApplicationStore()withuseShallowselectors in components likeApp.tsx,AppRouter.tsx,UnAuthenticatedAppRouter.tsx, andGlobalSearchBar.tsxto reduce unnecessary re-renders and improve performance. [1] [2] [3] [4] [5] [6] [7] [8] [9]Authentication flow improvements:
OktaAuthenticator.tsxby adding a fallback to redirect and re-authenticate if tokens are missing, increasing reliability in edge cases.Auth0Callback.tsxto useuseEffectand local state for error handling, ensuring errors during token retrieval are displayed correctly and improving user feedback. [1] [2] [3]Component and routing updates:
AppRouter.tsxto improve authentication support for SAML providers.AppRouterexport inmemoto prevent unnecessary re-renders when props/state haven't changed.Analytics improvements:
WebAnalyticsProvider.tsxto memoize the analytics instance usinguseMemo, ensuring the analytics provider is only re-initialized when the user ID changes. [1] [2]Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>