Fixes #31574: stop the stale redirect cookie navigating users away mid-session - #31575
Fixes #31574: stop the stale redirect cookie navigating users away mid-session#31575harsh-vador wants to merge 1 commit into
Conversation
The `redirectUrlPath` cookie was written on every refreshable 401 with a one-hour TTL, using the pathname captured when the axios interceptors were registered (once, on mount) rather than the user's current location. It was then replayed by PermissionProvider on *every* permission fetch — an effect keyed on `currentUser.teams` / `.roles` array identities, so any setCurrentUser (persona save, team update, profile edit, boot) re-ran it. Consumption only re-wrote the cookie with a one-second expiry instead of deleting it. Net effect: a token expiry while browsing armed a stale path, and a later unrelated user-state update navigated there — users clicking around Explore were dropped on the landing page, Glossary or Connections, and a reload after the "please refresh" screen discarded the URL they reloaded. - PermissionProvider: consume the cookie once per session (ref latch re-armed by resetPermissions on logout), delete it on read, and skip the navigate when the stored path is already the current location. - AuthProvider: read the location at call time, store `pathname + search`, and only store when the session is actually dropped to /signin — not on a 401 the silent refresh heals. - Shorten the cookie TTL from 1 hour to 5 minutes and drop setUrlPathnameExpiryAfterRoute; SignUpPage now deletes the cookie outright. - ErrorBoundary: retry re-renders the failing URL instead of navigating to HOME, and resetKeys clears a stuck boundary on route change. - Remove a leftover [VisibilityHandler] console.debug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
❌ 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 |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
harsh-vador
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const ErrorBoundary: React.FC<Props> = ({ children }) => { | ||
| const navigate = useNavigate(); | ||
|
|
||
| const onErrorReset = () => { | ||
| navigate(ROUTES.HOME); | ||
| }; | ||
| const location = useLocation(); | ||
|
|
||
| /* | ||
| * Retry renders the URL the user is actually on — sending them to the landing | ||
| * page instead silently discarded whatever they were looking at. `resetKeys` | ||
| * additionally clears a stuck boundary on any route change, so a failure on | ||
| * one page does not swallow the rest of the app. | ||
| */ | ||
| return ( | ||
| <ErrorBoundaryWrapper | ||
| FallbackComponent={ErrorFallback} | ||
| onReset={onErrorReset}> | ||
| resetKeys={[location.pathname, location.search]}> | ||
| {children} | ||
| </ErrorBoundaryWrapper> |
There was a problem hiding this comment.
💡 Quality: ErrorFallback "Home" button no longer navigates home
The diff removes onReset (which called navigate(ROUTES.HOME)) from ErrorBoundary, so for non-chunk errors the fallback button now only calls resetErrorBoundary() and re-renders the same children in place. But ErrorFallback.tsx still labels that button t('label.home'). The label is now misleading, and for a deterministic render error the retry immediately re-throws — leaving the user stuck on the broken page with no working escape from the offered button (previously it took them home). Consider relabeling the button to label.retry/label.try-again, or having ErrorFallback navigate home itself when the error is not a chunk-load error.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsRefactors redirect cookie handling to prevent users from being navigated away mid-session due to stale path storage. Consider restoring home navigation behavior for the ErrorBoundary 'Home' button. 💡 Quality: ErrorFallback "Home" button no longer navigates home📄 openmetadata-ui/src/main/resources/ui/src/components/common/ErrorBoundary/ErrorBoundary.tsx:23-37 The diff removes 🤖 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 |
|
| Count | Rule |
|---|---|
| 11 | i18next/no-literal-string |
| 11 | react-hooks/exhaustive-deps |
| 3 | sonarjs/no-duplicate-string |
| 3 | sonarjs/no-nested-functions |
| 2 | openmetadata-imports/no-internal-barrel-imports |
| 1 | openmetadata-imports/no-api-calls-in-iteration |
| 1 | sonarjs/cyclomatic-complexity |
| 1 | jsx-a11y/no-autofocus |
| 1 | sonarjs/no-duplicated-branches |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.test.tsx:18: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.test.tsx:144:71 |
i18next/no-literal-string |
disallow literal string: Logout |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.test.tsx:164:75 |
i18next/no-literal-string |
disallow literal string: Logout |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.test.tsx:196:14 |
i18next/no-literal-string |
disallow literal string: <button data-testid="login-button" onClick={() => { expect(typeof onLoginHandler).toBe('function'); onLoginHandler(); }}> Login </butto |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.test.tsx:216:17 |
i18next/no-literal-string |
disallow literal string: ConsumerComponent |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.test.tsx:263:26 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 10 times. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.test.tsx:531:14 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 3 times. |
| 🟡 | 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 919) 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:428:9 |
react-hooks/exhaustive-deps |
The 'resetUserDetails' function makes the dependencies of useMemo Hook (at line 919) change on every render. To fix this, wrap the definition of 'resetUserDetai |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:516:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'startTokenExpiryTimer'. Either include it or remove the dependency array. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:547:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'startTokenExpiryTimer'. Either include it or remove the dependency array. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:557:9 |
react-hooks/exhaustive-deps |
The 'handleFailedLogin' function makes the dependencies of useMemo Hook (at line 919) change on every render. Move it inside the useMemo callback. Alternatively |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:615: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:651:9 |
react-hooks/exhaustive-deps |
The 'initializeAxiosInterceptors' function makes the dependencies of useMemo Hook (at line 919) change on every render. To fix this, wrap the definition of 'ini |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:722:67 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:729: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:741:37 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:750:27 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:819:30 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 17 which is greater than 10 authorized.","cost":7,"secondaryLocations":[{"line":819,"column":29,"endLine":819,"endColum |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:908:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'cleanup', 'fetchAuthConfig', 'initializeAxiosInterceptors', and 'startTokenExpiryTimer'. Either include them or |
| 🟡 | src/context/PermissionProvider/PermissionProvider.test.tsx:71:48 |
i18next/no-literal-string |
disallow literal string: Loader |
| 🟡 | src/context/PermissionProvider/PermissionProvider.test.tsx:84:37 |
i18next/no-literal-string |
disallow literal string: Children |
| 🟡 | src/context/PermissionProvider/PermissionProvider.test.tsx:97:37 |
i18next/no-literal-string |
disallow literal string: Children |
| 🟡 | src/context/PermissionProvider/PermissionProvider.test.tsx:114:37 |
i18next/no-literal-string |
disallow literal string: Children |
| 🟡 | src/context/PermissionProvider/PermissionProvider.test.tsx:133:37 |
i18next/no-literal-string |
disallow literal string: Children |
| 🟡 | src/context/PermissionProvider/PermissionProvider.test.tsx:151:37 |
i18next/no-literal-string |
disallow literal string: Children |
| 🟡 | src/context/PermissionProvider/PermissionProvider.test.tsx:168:37 |
i18next/no-literal-string |
disallow literal string: Children |
| 🟡 | src/context/PermissionProvider/PermissionProvider.tsx:129:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'cookieStorage'. Either include it or remove the dependency array. |
| 🟡 | src/context/PermissionProvider/PermissionProvider.tsx:273:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'currentUser', 'fetchLoggedInUserPermissions', and 'resetPermissions'. Either include them or remove the dependen |
| 🟡 | src/pages/SignUp/SignUpPage.tsx:151:15 |
jsx-a11y/no-autofocus |
The autoFocus prop should not be used, as it can reduce usability and accessibility for users. |
| 🟡 | src/utils/AuthProvider.util.ts:115:21 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/utils/AuthProvider.util.ts:193:5 |
sonarjs/no-duplicated-branches |
This case's code block is the same as the block for the case on line 169. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
Jest test CoverageUI tests summary
|
|
✅ Playwright Results — workflow succeededValidated commit ✅ 796 passed · ❌ 0 failed · 🟡 0 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 51s ⏱️ Max setup 3m 2s · max shard execution 19m 58s · max shard-job elapsed before upload 23m 10s · reporting 4s 🌐 223.00 requests/attempt · 2.67 app boots/UI scenario · 23.62% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
1 participant
Describe your changes:
Fixes #31574
Users browsing the app — most visibly clicking service aggregations / filters in Explore — were intermittently thrown onto the landing page, Glossary or Connections. The click was never the cause.
Root cause. The
redirectUrlPathcookie is meant to be a one-shot "resume where you were after login" hint. Today:AuthProviderwrites it on every refreshable 401 with a 1-hour TTL, including 401s the silent refresh heals and the user never notices.useEffectwith an empty dep array, sohandleStoreProtectedRedirectPathcloses overlocation.pathnamefrom app boot — the stored path is not where the user actually is.PermissionProvider.redirectToStoredPath()runs on every permission fetch and navigates unconditionally. That effect is keyed oncurrentUser?.teams/?.roles— array identities — so everysetCurrentUser(persona save, team update, profile edit, boot) replays it.So: a token expiry while browsing armed a stale path, and an unrelated user-state update minutes later navigated there. The same cookie also explains "I hit refresh on the upgrade screen and landed somewhere else" — on boot the permission fetch replays an hour-old path and discards the URL that was reloaded.
Changes
PermissionProvider.tsx— consume the hint once per session (ref latch, re-armed byresetPermissions()across a logout/login boundary), delete the cookie on read, skip the navigate when the stored path already equalspathname + search, and fix the[history]dep to[navigate].AuthProvider.tsx— read the location at call time (globalThis.location) so the boot-time closure cannot write a stale path, storepathname + search, and store only fromresetUserDetails(the paths that really bounce to/signin) instead of from every 401.router.constants.ts/AuthProvider.util.ts— cookie TTL 1 hour → 5 minutes viaREDIRECT_PATHNAME_EXPIRY_MS; removedsetUrlPathnameExpiryAfterRoute(the 1-second-expiry pseudo-delete).SignUpPage.tsx— a brand-new user has nothing to resume, so the cookie is deleted outright.ErrorBoundary.tsx— retry now re-renders the URL that failed instead ofnavigate(ROUTES.HOME), andresetKeyson the location clears a stuck boundary on route change.[VisibilityHandler]console.debug.Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Tests
New unit tests (all green,
yarn test, 4 suites / 40 tests):PermissionProvider.test.tsxteams/rolesidentity change;AuthProvider.test.tsxredirectUrlPathcookie;/signinstores the current path (/explore/tables?quickFilter=abc), not the boot-time one.Also verified:
eslint0 errors on the changed files,prettier --checkclean,tsc --noEmitreports nothing new for them.Manual test steps
document.cookie = "redirectUrlPath=/glossary; path=/"while sitting on/explore.currentUserupdate (save a persona preference on My Data) or reload./glossary. After: stays put./explore, let the token expire and keep clicking aggregations — no navigation away.🤖 Generated with Claude Code