fix(ui): wait for token update before calling callback#27209
fix(ui): wait for token update before calling callback#27209chirag-madlani wants to merge 4 commits intomainfrom
Conversation
github-actions
bot
added
safe to test
There was a problem hiding this comment.
Pull request overview
Improves frontend auth token refresh reliability by replacing a fixed post-refresh delay with a storage-polling wait, aiming to ensure the refreshed token is persisted before notifying listeners and retrying requests.
Changes:
- Replaced the hardcoded 100ms timeout with
waitForTokenPersistence(oldToken)before invoking the refresh success callback and broadcasting thetokenRefreshedstorage event.
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts:103
- waitForTokenPersistence returns after maxAttempts even if the token never changes, but refreshToken will still invoke refreshSuccessCallback and set tokenRefreshed=true. If the goal is to only notify after persistence, consider having waitForTokenPersistence return a boolean (or throw) and gate the callback/storage event on successful persistence; otherwise the PR description should be softened to indicate it's a best-effort wait.
await this.waitForTokenPersistence(oldToken);
this.refreshSuccessCallback?.();
// To update all the tabs on updating channel token
// Notify all tabs that the token has been refreshed
localStorage.setItem(REFRESHED_KEY, 'true');
| const newToken = await this.fetchNewToken(); | ||
| if (newToken) { | ||
| // Wait briefly for token to be persisted in SW+IndexedDB before notifying | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| await this.waitForTokenPersistence(oldToken); | ||
| this.refreshSuccessCallback?.(); |
There was a problem hiding this comment.
Existing unit tests for TokenService.refreshToken rely on advancing fake timers by 100ms; with waitForTokenPersistence polling up to 20x50ms (and requiring getOidcToken to change from oldToken), those tests will hang/fail unless they mock getOidcToken to return the updated token and advance timers accordingly. Please update/add tests to cover the new persistence-wait behavior (e.g., sequence old->new token and assert callback is invoked after persistence).
| @@ -96,7 +96,7 @@ class TokenService { | |||
| const newToken = await this.fetchNewToken(); | |||
| if (newToken) { | |||
| // Wait briefly for token to be persisted in SW+IndexedDB before notifying | |||
There was a problem hiding this comment.
The inline comment still says "Wait briefly" even though the logic now polls storage for up to ~1s. Update the comment to reflect that we're waiting for token persistence (with a bounded timeout) rather than a fixed delay.
| // Wait briefly for token to be persisted in SW+IndexedDB before notifying | |
| // Wait for token persistence in SW+IndexedDB before notifying, with a bounded timeout |
🟡 Playwright Results — all passed (29 flaky)✅ 3589 passed · ❌ 0 failed · 🟡 29 flaky · ⏭️ 207 skipped
🟡 29 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 |
|
@copilot apply changes based on the comments in this thread |
…avior Agent-Logs-Url: https://github.com/open-metadata/OpenMetadata/sessions/c06d325e-33b2-4115-b553-31a93730920a Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
Applied both changes from the review thread in commit ef7ba5e:
|
Code Review ✅ ApprovedFixes a race condition in the UI token handling by ensuring the token update completes before invoking the callback. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts:104
fetchNewToken()clears therefreshInProgressflag (in itsfinally) before this newwaitForTokenPersistence(...)completes. This expands the window where other tabs can see no refresh in progress and start their own refresh while the token is still being persisted. To avoid duplicate refreshes/races, consider only clearingrefreshInProgressafter persistence is confirmed (or after the bounded wait) and notifications are sent.
const newToken = await this.fetchNewToken();
if (newToken) {
// Wait for token persistence in SW+IndexedDB before notifying, with a bounded timeout
await this.waitForTokenPersistence(oldToken);
this.refreshSuccessCallback?.();
// To update all the tabs on updating channel token
// Notify all tabs that the token has been refreshed
localStorage.setItem(REFRESHED_KEY, 'true');
}
Jest test CoverageUI tests summary
|
|
|
Covered with #27214 |
This pull request updates the token refresh logic to ensure that the new token is fully persisted before proceeding. Instead of waiting a fixed amount of time, it now explicitly waits for token persistence, which should make the refresh process more reliable.
Token refresh reliability improvement:
waitForTokenPersistence(oldToken)inTokenServiceUtil.ts, ensuring the new token is actually persisted before continuing.Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>