Repository navigation
🐛 fix: Resolve Admin Capability Gate In Route Loader - #109
dustinhealy wants to merge 2 commits into
Conversation
The layout decided admin access from a component-level query, so while the effective-capabilities request was in flight it fell through and mounted the full panel before replacing it with the access-denied screen. Resolving the check in the route loader and reading it with a suspense query means the layout only ever renders a decided state, with the route pending component covering the wait. Also removes an unreferenced capabilities query factory that declared the same cache key as the hook with an incompatible payload and no error classification, so the two could not safely coexist.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfe15fcbd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The exported query factory's data contract belongs with the rest of the local type definitions rather than inside the hook, so future consumers reach for the shared type instead of redefining the result shape.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…n Errors (#185) * 🛂 fix: Decide Admin Access Before the Panel Renders and Fail Closed on Errors The /_app layout read the viewer's capabilities with a component query and showed AccessDenied only once that query had succeeded. Until then it rendered the whole panel: sidebar, header and the routed page. A signed-in user without access:admin, such as one whose grant was revoked within the 60-second session revalidation window, saw the panel before the denial. When the capabilities request failed with an error the hook does not map, such as a 500, the panel stayed up for them. The /_app loader now loads the capabilities through capabilitiesQueryOptions(userId), the query useCapabilities reads, so the layout first renders with the answer. An admin gets the panel, anyone else gets AccessDenied, and a failed load shows PermissionsUnavailable with its Reload button. The AppPending shell covers the wait. A failed background refetch keeps the last answer, so an admin stays in the panel; a refetch without access:admin still takes the panel away. Admins also get their full sidebar on first paint instead of Configuration and Access appearing after load. The unused effectiveCapabilitiesOptions, which cached a different shape under the same query key, is removed. Supersedes #109 and #101 by @dustinhealy: the loader approach comes from #109 and the fail-closed states from #101. * 🛂 fix: Fetch Stale Capabilities Again Before the Panel Renders * 🛂 fix: Fail Closed When the Loader Can't Re-Verify Stale Capabilities
|
Thanks @dustinhealy for this. The flash you found still reproduced on main: after a grant was revoked, the sidebar, header and dashboard mounted before Access denied appeared. #185, now merged, fixes it by combining this PR with #101 on current main. It keeps this PR's approach:
The root-context typing and a pending component were already on main, so those parts weren't carried over. This PR conflicts with main, so I'm closing it in favor of #185. If you see a case #185 misses, please comment or open an issue. |
Summary
AppLayoutdecided admin access from a component-level query, so while/api/admin/grants/effectivewas in flight the layout fell through and rendered the full panel (sidebar, header, dashboard) before replacing it with the access-denied screen. A non-admin briefly sees the entire admin UI, including its navigation.This resolves the capability check in the
_approute loader withqueryClient.ensureQueryDataand reads it in the component withuseSuspenseQuery, so the layout only ever renders a decided state: authorized or denied. The route'spendingComponentcovers the wait. This is the pattern TanStack documents for data-derived route gating, for exactly this reason ("No flash of loading states").Change Type
What changed
useCapabilities.tsexports acapabilitiesQueryOptions(userId)factory so the loader and the hook share one query definition and one cache entry. The hook's signature and all existing consumers are unchanged._app.tsxprefetches that query inloader, rendersAppPendingwhile it resolves, and derives the admin decision synchronously viauseSuspenseQuery. TheisLoading/isErrorfall-through is gone.__root.tsxusescreateRootRouteWithContext<{ queryClient }>so the loader can reach the query client through route context. The router already supplies it; only the type was missing.effectiveCapabilitiesOptionsfromserver/capabilities.ts. It had no callers anywhere in the repo, and it declared the same['effectiveCapabilities', userId]cache key as the hook with an incompatible payload (string[]versus{ available, capabilities }) and no 401/403/404 classification. Leaving both would mean whichever populated the key first won, and a consumer of the other would read the wrong shape.Testing
Verified in a container against a mock of the admin API, driving the real panel through a genuine login. Because the flash is a sub-frame DOM transition, a
MutationObserverinstalled before any app JS runs timestamps the first appearance of each marker; screenshots and polling are too coarse to catch it.Before, on this branch's base, at real timing (no artificial latency):
The panel is on screen for ~42ms for a user who is about to be denied.
Before: the full panel mounts while capabilities load, then is replaced by the access-denied screen.
clip-base-flash.webm
This clip was captured on a branch that also changes the access-denied screen itself, so its final frame differs from what
mainrenders. The flashing panel is the same code path this PR fixes.After, same harness:
No privileged chrome enters the DOM at any point.
After: neutral loading only, then the access-denied screen.
Local gates:
eslintclean on the changed files,prettier --checkclean,tsc --noEmitclean, and the fullvitestsuite passes.Checklist