Copilot commented on code in PR #11703:
URL: https://github.com/apache/gravitino/pull/11703#discussion_r3425320820
##########
web-v2/web/src/lib/utils/axios/index.js:
##########
@@ -40,6 +40,7 @@ import { useAuth as Auth } from '../../provider/session'
import { githubApis } from '@/lib/api/github'
import { isProdEnv } from '@/lib/utils'
import { oauthProviderFactory } from '@/lib/auth/providers/factory'
+import { store } from '@/lib/store'
Review Comment:
Importing the Redux `store` here introduces a circular dependency:
`lib/store` → `lib/store/auth` → `lib/api/auth` → `lib/utils/axios` →
`lib/store`. Even if it often “works” due to deferred access, this cycle can
lead to partially-initialized modules or hard-to-debug runtime behavior
depending on bundler evaluation order. Consider decoupling axios auth header
selection from Redux (e.g., persist `authType` to storage when configs load, or
pass authType into axios via config/requestOptions).
##########
web-v2/web/src/lib/store/auth/index.js:
##########
@@ -150,11 +150,14 @@ export const logoutAction = createAsyncThunk(
dispatch(clearIntervalId())
dispatch(setAuthToken(''))
- dispatch(setAuthUser(null))
- } else {
- dispatch(setAuthUser(null))
}
+ // Always clear authUser in Redux and sessionStorage on logout
+ // This ensures consistent behavior for both OAuth and simple auth
+ dispatch(setAuthUser(null))
+ sessionStorage.removeItem('simpleAuthUser')
+ sessionStorage.removeItem('simpleAuthToken')
Review Comment:
`dispatch(setAuthUser(null))` already removes `simpleAuthUser` from
sessionStorage in the reducer, so removing it again here is redundant. Keeping
only the `simpleAuthToken` cleanup avoids duplicated logic while preserving the
intended behavior.
##########
web-v2/web/src/lib/provider/session.js:
##########
@@ -101,6 +101,10 @@ const AuthProvider = ({ children }) => {
goToMetalakeListPage()
}
} else if (authType === 'oauth') {
+ // Clear any residual simpleAuthUser when authType is oauth
+ sessionStorage.removeItem('simpleAuthUser')
+ sessionStorage.removeItem('simpleAuthToken')
+
Review Comment:
These removals are likely ineffective for OAuth mode because the OAuth path
later dispatches `setAuthUser(user)`, and the `setAuthUser` reducer persists
that value back into `sessionStorage` under `simpleAuthUser`. If the goal is to
stop using `simpleAuthUser` for OAuth entirely, the persistence logic needs to
be gated by `authType` (or use a different storage key/action for OAuth users).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
