Skip to content

Conversation

@chirag-madlani
Copy link
Collaborator

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 useShallow selector from Zustand to minimize unnecessary re-renders, improving error handling in authentication callbacks, and adding memoization for analytics initialization.

State management optimizations:

  • Replaced direct usage of useApplicationStore() with useShallow selectors in components like App.tsx, AppRouter.tsx, UnAuthenticatedAppRouter.tsx, and GlobalSearchBar.tsx to reduce unnecessary re-renders and improve performance. [1] [2] [3] [4] [5] [6] [7] [8] [9]

Authentication flow improvements:

  • Enhanced the Okta token renewal logic in OktaAuthenticator.tsx by adding a fallback to redirect and re-authenticate if tokens are missing, increasing reliability in edge cases.
  • Refactored Auth0Callback.tsx to use useEffect and local state for error handling, ensuring errors during token retrieval are displayed correctly and improving user feedback. [1] [2] [3]

Component and routing updates:

  • Added a new route for SAML callback in AppRouter.tsx to improve authentication support for SAML providers.
  • Wrapped AppRouter export in memo to prevent unnecessary re-renders when props/state haven't changed.

Analytics improvements:

  • Updated WebAnalyticsProvider.tsx to memoize the analytics instance using useMemo, 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:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.31% (52727/80739) 43.35% (26268/60594) 46.71% (8198/17550)

@chirag-madlani
Copy link
Collaborator Author

Gitar fix CI

@gitar-bot
Copy link

gitar-bot bot commented Jan 8, 2026

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:

  1. SSO Configuration (shard 3/6) - Missing "Authority" field in SAML config UI
  2. Data Contracts (shard 4/6) - Execution timeout
  3. Bulk Operations (shard 2/6) - Multiple timeout failures

This PR only modifies:

  • State management optimizations (useShallow selectors)
  • Authentication callback improvements
  • Memoization patterns

However, the code review identified 2 critical bugs in the Okta token renewal logic that should be fixed:

  1. Missing await on tokenManager.get() calls - These return Promises but aren't awaited, so the fallback logic will never execute
  2. Unreachable code after signInWithRedirect() - The redirect causes a page navigation, making subsequent code unreachable

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:

  • Fix the Okta authenticator bugs?
  • Retry the failed CI jobs to see if they're flaky?
  • Investigate the test failures further?

Copy link
Contributor

Copilot AI left a 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 with useShallow selectors in multiple components to prevent unnecessary re-renders
  • Added fallback logic to Okta token renewal for handling missing tokens
  • Refactored Auth0Callback to use useEffect with 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;
    };

Copy link
Contributor

Copilot AI commented Jan 8, 2026

@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>
@chirag-madlani chirag-madlani added the To release Will cherry-pick this PR into the release branch label Jan 9, 2026
@gitar-bot
Copy link

gitar-bot bot commented Jan 9, 2026

🔍 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.

Issue

Three 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)

  1. should complete export flow and trigger download
  2. should include filters and search in export request
  3. should validate export response structure

All three tests are in Pages/AuditLogs.spec.ts under "Audit Logs - Export Functionality".

Pattern Analysis

These same 3 tests have failed consistently across multiple CI runs on this PR:

  • Failed in run 59899946766 (shard 3/6, January 9 11:48 UTC)
  • Failed again in run 59923566537 (shard 3/6, January 9 16:23 UTC)

This indicates a persistent issue with audit log export functionality, not a flaky test.

Additional Context

  • 14 flaky tests passed on retry
  • 453+ tests passed successfully
  • Test duration: ~43-45 minutes per shard

Root Cause

The failures are in backend audit log export functionality, which involves:

  • Exporting audit log data via API
  • Applying filters and search parameters
  • Validating export response structures
  • Triggering file downloads

Why This Is Unrelated to the PR

This PR only modifies:

  • Frontend state management (useShallow selectors in App.tsx, AppRouter.tsx, UnAuthenticatedAppRouter.tsx, GlobalSearchBar.tsx)
  • React memoization (WebAnalyticsProvider.tsx)
  • Authentication flows (OktaAuthenticator.tsx with improved error handling and fallback logic)
  • Auth0 callback (Auth0Callback.tsx refactored to useEffect)
  • Test coverage (OktaAuthenticator.test.tsx with 272 lines of tests)

None of these changes touch:

  • Audit log functionality
  • Export features
  • Download handling
  • Backend API endpoints for audit logs

Assessment

These 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:

  1. Backend API issue: The audit log export endpoint may have a regression
  2. Test environment issue: Export download handling may be broken in the CI environment
  3. Main branch issue: The problem may have been introduced in the main branch during one of the merges

The solution would be to investigate the audit log export functionality separately from this PR.

Code Review 👍 Approved with suggestions

Good 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.

⚠️ Bug: Okta fallback logic returns token before signInWithRedirect completes

📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx

The fallback logic calls oktaAuth.signInWithRedirect() which initiates a redirect, but then immediately tries to get the ID token and return it. Since signInWithRedirect() redirects the user to Okta's login page (a full page redirect), the code after it (const newToken = oktaAuth.getIdToken() ?? '') will likely never execute, or if it does momentarily, it will return an empty string because the user hasn't authenticated yet.

The redirect flow means the user leaves the page entirely, and when they return after authenticating, the component will re-mount and this code path won't be hit (because tokens will now exist).

Suggested fix:
Simply call signInWithRedirect() without returning a token, as the function won't continue after the redirect:

if (!existingIdToken && !existingAccessToken) {
  await oktaAuth.signInWithRedirect();
  return ''; // This line will likely never be reached due to redirect
}

Or consider if this is the intended behavior - the return value after redirect initiation is essentially meaningless.

More details 💡 1 suggestion ✅ 2 resolved
💡 Edge Case: renewToken caller may not handle empty string return value

📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx:111-114

The renewToken function can return an empty string in two scenarios:

  1. When no tokens exist and signInWithRedirect is called
  2. When an error occurs during token renewal

While signInWithRedirect will navigate away from the page, the caller of renewToken receives an empty string which may not be explicitly handled. If the redirect fails or is slow, code that depends on the token may proceed with an invalid/empty token.

Consider either:

  1. Throwing an error instead of returning empty string
  2. Documenting that callers should handle empty string returns
  3. Using a more explicit return type (e.g., {token: string, redirected: boolean} or throwing a custom error)
Edge Case: useEffect dependency includes handleSuccessfulLogin which may cause loops

📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AppCallbacks/Auth0Callback/Auth0Callback.tsx
The useEffect hook includes handleSuccessfulLogin in its dependency array. If handleSuccessfulLogin is not memoized with useCallback in AuthProvider, it could cause the effect to run multiple times or create infinite loops if the function reference changes on each render.

Additionally, after handleSuccessfulLogin is called, if it triggers a navigation or state change that causes isAuthenticated or user to change, the effect may run again.

Suggested fix:
Consider adding a ref or state flag to track whether login has already been processed:

const hasProcessedLogin = useRef(false);

useEffect(() => {
  if (isAuthenticated && user && !hasProcessedLogin.current) {
    hasProcessedLogin.current = true;
    // ... rest of the logic
  }
}, [isAuthenticated, user, getIdTokenClaims, handleSuccessfulLogin, t]);

Verify that handleSuccessfulLogin is properly memoized in the AuthProvider.

Bug: Missing await on tokenManager.get() calls which return Promises

📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx
The Okta Auth SDK's tokenManager.get() method returns a Promise<Token | undefined>, but the code is not awaiting these calls. This means existingIdToken and existingAccessToken will be Promise objects (which are truthy), not the actual token values.

The condition !existingIdToken && !existingAccessToken will always be false because Promise objects are truthy, meaning the fallback redirect logic will never execute.

Suggested fix:

const existingIdToken = await oktaAuth.tokenManager.get('idToken');
const existingAccessToken = await oktaAuth.tokenManager.get('accessToken');

if (!existingIdToken && !existingAccessToken) {
  await oktaAuth.signInWithRedirect();
  return '';
}

What Works Well

Clean adoption of useShallow selectors across components to minimize re-renders. Comprehensive test coverage added for OktaAuthenticator covering multiple edge cases. Proper memoization of analytics instance with correct dependency on currentUserId.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off Gitar will not commit updates to this branch.
Display: compact Hiding non-applicable rules.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2026

@chirag-madlani chirag-madlani enabled auto-merge (squash) January 9, 2026 15:42
@chirag-madlani chirag-madlani merged commit b626e08 into main Jan 9, 2026
22 of 23 checks passed
@chirag-madlani chirag-madlani deleted the feat-okta-renew-fallback branch January 9, 2026 17:47
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2026

Changes have been cherry-picked to the 1.11.5 branch.

github-actions bot pushed a commit that referenced this pull request Jan 9, 2026
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants