fix(ui): visibility handler no longer refreshes when there's nothing to refresh - #31819
Merged
Merged
Conversation
…to refresh
Three cases were quietly firing tokenService.refreshToken() on every
tab focus:
1. **No token in storage** (user on /signin, or just logged out).
The old check hit extractDetailsFromToken('') → isExpired:true →
refresh call. On OIDC that meant a hidden signinSilent iframe to
the IdP on every focus of an unauthenticated tab.
2. **Token has no `exp` claim** (opaque/non-JWT/spec-violating
id_token). extractDetailsFromToken returns timeoutExpiry:0 in the
isNil(exp) branch — same value it uses for near-expiry — and the
old shorthand `timeoutExpiry <= 0` treated both the same.
3. Legit near-expiry (< 60s to exp) — unchanged; still refreshes.
Now:
- No token → early return.
- Expired → refresh (unchanged).
- Missing/non-positive exp → early return; next real 401 drives it.
- timeoutExpiry <= 0 (near-expiry, exp confirmed valid) → refresh.
- Otherwise → reschedule the proactive timer.
Also drops the noisy `[VisibilityHandler]` console.debug that fired
on every focus regardless of outcome.
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Comment on lines
+564
to
567
| } catch { | ||
| // Storage read errors fall through: the next real 401 will drive | ||
| // the refresh via the axios interceptor. | ||
| } |
There was a problem hiding this comment.
💡 Quality: Empty catch silently swallows refreshToken errors
The refactor replaced the previous catch (error) { console.error(...) } with a fully empty catch {}. Beyond the intended removal of the noisy debug log, this now also silently discards any error thrown by tokenService.current?.refreshToken() (e.g. a failing silent-renew), leaving no trace for debugging genuine refresh failures. Consider retaining a low-noise console.debug/console.error of the caught error so real failures remain diagnosable while still fixing the per-focus IdP noise.
Was this helpful? React with 👍 / 👎
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
| Count | Rule |
|---|---|
| 8 | react-hooks/exhaustive-deps |
| 3 | sonarjs/no-nested-functions |
| 2 | openmetadata-imports/no-internal-barrel-imports |
| 2 | sonarjs/cyclomatic-complexity |
| 1 | openmetadata-imports/no-api-calls-in-iteration |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.test.tsx:17:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:80:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:280:9 |
react-hooks/exhaustive-deps |
The 'onLoginHandler' function makes the dependencies of useMemo Hook (at line 959) change on every render. Move it inside the useMemo callback. Alternatively, w |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:357:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'navigate', 'setApplicationLoading', 'setCurrentUser', and 'setIsAuthenticated'. Either include them or remove |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:415:9 |
react-hooks/exhaustive-deps |
The 'resetUserDetails' function makes the dependencies of useMemo Hook (at line 959) change on every render. To fix this, wrap the definition of 'resetUserDetai |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:515:45 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 12 which is greater than 10 authorized.","cost":2,"secondaryLocations":[{"line":515,"column":44,"endLine":515,"endColum |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:577:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'getLoggedInUserDetails' and 'startTokenExpiryTimer'. Either include them or remove the dependency array. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:587:9 |
react-hooks/exhaustive-deps |
The 'handleFailedLogin' function makes the dependencies of useMemo Hook (at line 959) change on every render. Move it inside the useMemo callback. Alternatively |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:645:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'authConfig?.provider', 'handledVerifiedUser', 'navigate', 'resetUserDetails', and 'startTokenExpiryTimer'. Eit |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:690:9 |
react-hooks/exhaustive-deps |
The 'initializeAxiosInterceptors' function makes the dependencies of useMemo Hook (at line 959) change on every render. To fix this, wrap the definition of 'ini |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:758:67 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:765:23 |
openmetadata-imports/no-api-calls-in-iteration |
Avoid issuing one API request per item. Fetch at the data owner, use a bulk endpoint, or use useQueries with an intentional concurrency policy. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:777:37 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:786:27 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:855:30 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 17 which is greater than 10 authorized.","cost":7,"secondaryLocations":[{"line":855,"column":29,"endLine":855,"endColum |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:948:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'cleanup', 'fetchAuthConfig', 'initializeAxiosInterceptors', and 'startTokenExpiryTimer'. Either include them or |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
Contributor
Jest test CoverageUI tests summary
|
karanh37
previously approved these changes
Aug 20, 2026
Contributor
✅ Playwright Results — workflow succeededValidated commit ✅ 320 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 51m 18s ⏱️ Max setup 3m 29s · max shard execution 15m 21s · max shard-job elapsed before upload 18m 44s · reporting 5s 🌐 216.03 requests/attempt · 2.22 app boots/UI scenario · 4.99% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
- AuthProvider.tsx: prettier reflow on the destructure of extractDetailsFromToken (checkstyle fix flagged by CI on this PR). - AuthProvider.test.tsx: five new tests under "AuthProvider visibility handler" pinning the guards the hotfix adds so they can't regress: * no token in storage → NO refreshToken() call * token with no exp claim → NO refreshToken() call * fresh token (>60s of headroom) → NO refreshToken() call * expired token → exactly one refreshToken() call * near-expiry (<60s of headroom) → exactly one refreshToken() call Adds shared mocks for getOidcToken and extractDetailsFromToken (with requireActual spreads so the pre-existing 14 tests still run untouched).
Greptile P1 on #31819: when jwt-decode throws (opaque / non-JWT / malformed token), extractDetailsFromToken falls through to {exp: 0, isExpired: true, timeoutExpiry: 0}. The previous ordering hit the `isExpired` branch first and fired refreshToken() before the invalid-exp guard could stop it — so opaque tokens still triggered an IdP request on every tab focus. Move the invalid-exp guard above the isExpired branch so both jwt- decode-throws AND isNil(exp) paths short-circuit the same way. The next real 401 still drives the refresh via the interceptor. Adds a regression test covering the jwt-decode-throws shape (exp: 0, isExpired: true).
|
karanh37
approved these changes
Aug 20, 2026
chirag-madlani
pushed a commit
that referenced
this pull request
Aug 20, 2026
Same ordering fix Greptile P1 flagged on the sibling hotfix PR (#31819). extractDetailsFromToken returns {exp: 0, isExpired: true, timeoutExpiry: 0} when jwt-decode throws (opaque / non-JWT / malformed token). Previously the isExpired branch fired ensureFreshToken() before the invalid-exp guard could stop it — so opaque tokens triggered an IdP request on every tab focus. Guard now runs BEFORE isExpired so both jwt-decode-throws AND isNil(exp) paths short-circuit the same way. The next real 401 still drives a refresh via the axios interceptor. Adds a Jest test with the {exp:0, isExpired:true} shape so this ordering can't get reversed, and updates the SSO flow matrix to spell out both branches.
chirag-madlani
added a commit
that referenced
this pull request
Aug 20, 2026
…to refresh (#31819) * fix(ui): visibility handler no longer refreshes when there's nothing to refresh Three cases were quietly firing tokenService.refreshToken() on every tab focus: 1. **No token in storage** (user on /signin, or just logged out). The old check hit extractDetailsFromToken('') → isExpired:true → refresh call. On OIDC that meant a hidden signinSilent iframe to the IdP on every focus of an unauthenticated tab. 2. **Token has no `exp` claim** (opaque/non-JWT/spec-violating id_token). extractDetailsFromToken returns timeoutExpiry:0 in the isNil(exp) branch — same value it uses for near-expiry — and the old shorthand `timeoutExpiry <= 0` treated both the same. 3. Legit near-expiry (< 60s to exp) — unchanged; still refreshes. Now: - No token → early return. - Expired → refresh (unchanged). - Missing/non-positive exp → early return; next real 401 drives it. - timeoutExpiry <= 0 (near-expiry, exp confirmed valid) → refresh. - Otherwise → reschedule the proactive timer. Also drops the noisy `[VisibilityHandler]` console.debug that fired on every focus regardless of outcome. * test(ui): pin visibility handler guards + prettier reflow - AuthProvider.tsx: prettier reflow on the destructure of extractDetailsFromToken (checkstyle fix flagged by CI on this PR). - AuthProvider.test.tsx: five new tests under "AuthProvider visibility handler" pinning the guards the hotfix adds so they can't regress: * no token in storage → NO refreshToken() call * token with no exp claim → NO refreshToken() call * fresh token (>60s of headroom) → NO refreshToken() call * expired token → exactly one refreshToken() call * near-expiry (<60s of headroom) → exactly one refreshToken() call Adds shared mocks for getOidcToken and extractDetailsFromToken (with requireActual spreads so the pre-existing 14 tests still run untouched). * fix(ui): visibility handler skips refresh for opaque tokens too Greptile P1 on #31819: when jwt-decode throws (opaque / non-JWT / malformed token), extractDetailsFromToken falls through to {exp: 0, isExpired: true, timeoutExpiry: 0}. The previous ordering hit the `isExpired` branch first and fired refreshToken() before the invalid-exp guard could stop it — so opaque tokens still triggered an IdP request on every tab focus. Move the invalid-exp guard above the isExpired branch so both jwt- decode-throws AND isNil(exp) paths short-circuit the same way. The next real 401 still drives the refresh via the interceptor. Adds a regression test covering the jwt-decode-throws shape (exp: 0, isExpired: true). (cherry picked from commit 935dead)
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsAdds visibility-refresh gating to prevent unnecessary IdP calls when signed out or using opaque tokens. Consider handling the empty catch block around refreshToken errors to prevent silent failures. 💡 Quality: Empty catch silently swallows refreshToken errors📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx:564-567 The refactor replaced the previous 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Merged
2 of 5 tasks
This branch was previously deployed
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
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.
Summary
Stops `tokenService.refreshToken()` from firing on every tab focus when there's nothing to refresh — the current behavior on main hits the IdP on every visibility change while the user is signed out (or holds an opaque token), which surfaces as `/auth/refresh` (or a hidden `signinSilent` iframe) calls in the Network tab and unnecessary IdP-side load.
Root cause
`handleVisibilityChange` used the shorthand:
```ts
if (isExpired || timeoutExpiry <= 0) {
await tokenService.current?.refreshToken();
}
```
`extractDetailsFromToken` returns `{isExpired:true, timeoutExpiry:0}` for empty tokens (signed-out) and `{isExpired:false, timeoutExpiry:0}` for tokens without an `exp` claim. Both fell into the refresh branch.
The fix
Also drops the noisy `[VisibilityHandler]` `console.debug` that fired on every focus and made real refresh calls hard to spot in the console.
Relationship to the AuthCoordinator refactor PR
The AuthCoordinator refactor PR (#31675) already carries this fix as part of the larger cleanup + full Jest coverage (`AuthCoordinator.test.ts › tab visibility gating`). Shipping this as a targeted hotfix on `main` unblocks users on the current release while the refactor is under review.
Test plan
🤖 Generated with Claude Code
Greptile Summary
The PR prevents the tab-visibility handler from initiating token renewal when storage has no usable expiring token.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(ui): visibility handler skips refres..." | Re-trigger Greptile
Context used: