jerryshao commented on code in PR #12293:
URL: https://github.com/apache/gravitino/pull/12293#discussion_r4408557832
##########
web-v2/web/src/app/rootLayout/UserSetting.js:
##########
@@ -51,12 +51,17 @@ export default function UserSetting() {
const currentMetalake = searchParams.get('metalake')
const dispatch = useAppDispatch()
const store = useAppSelector(state => state.metalakes)
+ const isAuthReady = !!auth.authType && (auth.authType !== 'oauth' ||
!!auth.authToken)
useEffect(() => {
+ if (!isAuthReady) {
+ return
+ }
+
Review Comment:
[Nit] `UserSetting` is rendered only by `SiteHeader` (`SiteHeader.js:127`),
and `SiteHeader` is mounted once in `rootLayout/Layout.js:38`, so this effect
and the identical one in `SiteHeader.js:44-52` both run for the same state: two
concurrent `GET /api/metalakes` per trigger. The duplication predates this PR,
but the PR adds a second trigger (the auth-ready transition), so each such
transition now costs two more requests, and the two responses arrive as two
separate `fetchMetalakes.fulfilled` actions that each replace `store.metalakes`
and so re-run the URL-defaulting effect at `SiteHeader.js:59-97`.
Since both copies are being touched here, consider keeping the fetch in one
owner (`SiteHeader`) and dropping it from this component.
Verified by: `grep -rn 'UserSetting|SiteHeader' web-v2/web/src` at head
0db3739 shows `Layout.js:38` as the only mount of `SiteHeader` and no user of
`UserSetting` other than `SiteHeader.js:127`; read both files in full.
##########
web-v2/web/src/app/rootLayout/SiteHeader.js:
##########
@@ -38,12 +38,18 @@ export function SiteHeader() {
const dispatch = useAppDispatch()
const store = useAppSelector(state => state.metalakes)
+ const auth = useAppSelector(state => state.auth)
+ const isAuthReady = !!auth.authType && (auth.authType !== 'oauth' ||
!!auth.authToken)
Review Comment:
[Important] This is the third inline copy of the readiness expression:
`UserSetting.js:54` and `src/app/metalakes/page.js:74` carry the same one,
while the authoritative copy is the module-private helper at
`src/lib/store/metalakes/index.js:111-115`, which is used as the `condition` of
`fetchMetalakes` (line 133). That helper is not exported, so the copies can
drift from it, and drift fails silently in exactly the way this PR is fixing:
if the condition is ever tightened (say, to require a credential for `basic`),
a component whose own copy still returns `true` will dispatch, the `condition`
will drop the action, and nothing retries because the component's dependency
never changes again.
Suggest exporting the predicate from the store as a selector (e.g.
`selectIsAuthReady`) and using it at all three call sites. That also gives the
auth-ready transition a surface that can be unit-tested without
`@testing-library/react`, which is not currently a dependency.
Verified by: `grep -rn isAuthReady web-v2/web/src` at head 0db3739 returns
four independent definitions (this line, `UserSetting.js:54`,
`metalakes/page.js:74`, and the store helper); read
`src/lib/store/metalakes/index.js:111-135` and confirmed the helper is declared
`const isAuthReady = state => {...}` with no `export`.
##########
web-v2/web/src/app/rootLayout/SiteHeader.js:
##########
@@ -38,12 +38,18 @@ export function SiteHeader() {
const dispatch = useAppDispatch()
const store = useAppSelector(state => state.metalakes)
+ const auth = useAppSelector(state => state.auth)
+ const isAuthReady = !!auth.authType && (auth.authType !== 'oauth' ||
!!auth.authToken)
useEffect(() => {
+ if (!isAuthReady) {
+ return
+ }
Review Comment:
[Question] The gate requires a token only for `oauth`, so when `authType ===
'basic'` it becomes true as soon as `/configs` resolves, even with no
credential in hand. The request interceptor reads the basic token from
`sessionStorage.accessToken` (`src/lib/utils/axios/index.js:204-212`) and sends
no `Authorization` header when it is missing - which is the normal state of a
freshly opened tab, since `sessionStorage` is per-tab - and the 401 path then
removes the stored tokens and performs a full-page `window.location.href =
'/login'` (`src/lib/utils/axios/index.js:288-308`).
`AuthProvider` sends that same user to `/login` anyway
(`src/lib/provider/session.js:103-110`), so the destination does not change;
what I am unsure about is whether the extra unauthenticated `GET
/api/metalakes` and a hard navigation racing `router.push` are acceptable here,
or whether the gate should also require `!!authToken` for `basic` (and a
`simpleAuthUser` for `simple`, per `index.js:198-203`). Since this PR turns the
predicate from a passive drop into an active trigger, it seems worth settling.
Could you confirm which behaviour is intended?
Verified by: read the request and response interceptors in
`src/lib/utils/axios/index.js` and `initAuth` in `src/lib/provider/session.js`
in full at head 0db3739; no request was executed, the reachability above is
from reading those code paths.
--
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]
