Skip to content

Fix infinite loading with auto redirect#26856

Merged
chirag-madlani merged 3 commits intomainfrom
fix-infinite-loading-with-auto-redirect
Mar 30, 2026
Merged

Fix infinite loading with auto redirect#26856
chirag-madlani merged 3 commits intomainfrom
fix-infinite-loading-with-auto-redirect

Conversation

@chirag-madlani
Copy link
Copy Markdown
Collaborator

This pull request addresses a race condition in the authentication provider's login handler and improves the reliability of the authentication flow. The most significant change is the addition of a polling mechanism to ensure the authenticator reference is available before invoking the login method. Additionally, a minor bug in the logic for determining token expiry handling is fixed. A new test is added to cover the race condition scenario.

Authentication flow improvements:

  • Added a polling mechanism in onLoginHandler in AuthProvider.tsx to handle cases where the authenticator reference (authenticatorRef.current) may not be available immediately, preventing race conditions during login. The handler now retries every 50ms until the ref is set.

Bug fixes:

  • Fixed the logic for determining when to start the token expiry timer by replacing an incorrect use of indexOf with includes for checking the provider type in AuthProvider.tsx.

Testing improvements:

  • Added a test case in AuthProvider.test.tsx to verify that onLoginHandler correctly handles the race condition with the polling mechanism.

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.

@chirag-madlani chirag-madlani requested a review from a team as a code owner March 30, 2026 10:44
Copilot AI review requested due to automatic review settings March 30, 2026 10:44
@github-actions github-actions Bot added safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Mar 30, 2026
Copy link
Copy Markdown
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 PR aims to improve reliability of the UI authentication flow by avoiding a race condition during login (auto-redirect) and adjusting token-expiry timer logic, with an accompanying unit test.

Changes:

  • Added a polling-based retry in onLoginHandler to wait for authenticatorRef.current before invoking login.
  • Updated token-expiry gating logic by switching from indexOf to includes (but also changed boolean behavior).
  • Added a unit test intended to cover the login race condition.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx Adds polling login invocation and adjusts token-expiry timer start condition.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.test.tsx Adds a test case intended to validate the new polling/race-condition behavior.

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Mar 30, 2026

Code Review ✅ Approved 2 resolved / 2 findings

Fixes infinite loading during auto-redirect by addressing race conditions in the login handler and adding polling loop max-retry limits. Two issues resolved: corrected shouldStartExpiry logic and added max-retry safeguards to prevent indefinite polling.

✅ 2 resolved
Bug: indexOf-to-includes change inverts shouldStartExpiry logic

📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx:299-304
The old code was:

[Basic, LDAP].indexOf(provider) === -1

This evaluates to true when the provider is NOT Basic or LDAP, meaning: "start expiry for non-Basic/LDAP providers (which don't need a refresh token)".

The new code:

[Basic, LDAP].includes(provider)

This evaluates to true when the provider IS Basic or LDAP — the exact opposite.

The comment on line 299 says "Basic & LDAP renewToken depends on RefreshToken hence adding a check here for the same", confirming the original intent: for Basic/LDAP, only start the timer if refreshToken is truthy; for other providers, always start it. The new code breaks this — for non-Basic/LDAP providers without a refresh token, shouldStartExpiry will now be false, so the token expiry timer will never start, potentially causing silent auth renewal to stop working for SSO providers.

Edge Case: Polling loop has no max-retry limit, can poll forever

📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx:171-181
The invokeLogin function retries every 50ms with setTimeout if authenticatorRef.current is not set, but there is no upper bound on the number of retries. If the authenticator component never mounts (e.g., a rendering error, a conditional branch that excludes it, or a configuration issue), this will poll indefinitely, keeping applicationLoading set to true and leaking timers — effectively a soft hang for the user with no feedback.

Adding a max-retry count (e.g., 100 retries = 5 seconds) with an error/fallback path would make this robust.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@github-actions
Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.99% (58732/90363) 44.74% (30898/69049) 47.89% (9318/19456)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 30, 2026

🟡 Playwright Results — all passed (19 flaky)

✅ 3420 passed · ❌ 0 failed · 🟡 19 flaky · ⏭️ 216 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 453 0 2 2
🟡 Shard 2 607 0 1 32
🟡 Shard 3 608 0 6 27
🟡 Shard 4 612 0 5 47
🟡 Shard 5 585 0 2 67
🟡 Shard 6 555 0 3 41
🟡 19 flaky test(s) (passed on retry)
  • Features/DataAssetRulesDisabled.spec.ts › should allow multiple domain selection for glossary term when entity rules are disabled (shard 1, 1 retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Features/UserProfileOnlineStatus.spec.ts › Should not show online status for inactive users (shard 3, 1 retry)
  • Flow/AddRoleAndAssignToUser.spec.ts › Verify assigned role to new user (shard 3, 1 retry)
  • Flow/CustomizeWidgets.spec.ts › Total Data Assets Widget (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should display deleted assets when showDeleted is checked and deleted is not present in queryFilter (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should not display soft deleted assets in search suggestions (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Table (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with data products attached at domain and subdomain levels (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Verify Domain entity API calls do not include invalid domains field in glossary term assets (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Column detail panel data type display and nested column navigation (shard 4, 1 retry)
  • Pages/EntityDataSteward.spec.ts › Glossary Term Add, Update and Remove (shard 5, 1 retry)
  • Pages/ExplorePageRightPanel.spec.ts › Should allow Data Consumer to edit tier for dashboard (shard 5, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)
  • Pages/Users.spec.ts › Check permissions for Data Steward (shard 6, 1 retry)
  • VersionPages/EntityVersionPages.spec.ts › Directory (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@harshach harshach added the To release Will cherry-pick this PR into the release branch label Mar 30, 2026
@sonarqubecloud
Copy link
Copy Markdown

@chirag-madlani chirag-madlani merged commit 0945626 into main Mar 30, 2026
91 checks passed
@chirag-madlani chirag-madlani deleted the fix-infinite-loading-with-auto-redirect branch March 30, 2026 16:23
@github-actions
Copy link
Copy Markdown
Contributor

Failed to cherry-pick changes to the 1.12.4 branch.
Please cherry-pick the changes manually.
You can find more details here.

chirag-madlani added a commit that referenced this pull request Apr 3, 2026
* Fix login handler to handle race conditions with authenticator ref availability

* added tests

* address comment

(cherry picked from commit 0945626)
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.

4 participants