feat(shell): tell the admin denials apart, and give members a way out - #67
Conversation
The admin frame treated every refusal the same. A de-roled admin was sent to the login screen as if they had never signed in, and a stale button click surfaced "Something broke - a fault on this end" for what is really an expected race: another admin clearing your admin bit while you had the page open. resolveAdmin now returns a discriminated union rather than a nullable context, and a single private denyAdmin() turns a reason into a destination. It is a switch with an explicit `never` return, so adding a third denial reason fails to compile instead of silently falling through to the login redirect. getAdminContext is no longer exported: an AdminResolution is always truthy, so the old `if (!ctx) redirect(...)` pattern still compiles against the new signature and simply never fires - an unguarded admin page with no type error. Keeping it private makes a resurrected copy a compile error. The layout guards too, without replacing the per-page calls. Neither is sufficient alone: layouts do not re-run on soft navigation, and pages do not cover the admin route someone adds next and forgets to gate. Sign-out is this-device-only - one session row, keyed by the digest of the caller's own cookie, leaving other devices alone. POST-only, because the session cookie is sameSite=lax and the browser still attaches it to a cross-site top-level GET, so a link on any external page would sign people out. The clearing attributes now come from one constant shared with the login callback: a Set-Cookie whose path, sameSite or secure differs names a *different* cookie, which would leave the real session live while the response looked like success. The admin bar names its own register and takes a distinct aria-label, so the two headers are not distinguishable only by which links happen to be in them. The roster is renamed Accounts -> Members: both navs must call one destination by one name (WCAG 3.2.4), but that shared name cannot contain "account" when the member nav shows it directly beside "Your account" - a pair separated only by a possessive. The route and the account table are unchanged. NavItem loses its `key` field. Two callers keyed items by different conventions - one to the route, one to an arbitrary label - and both typechecked, so a mismatch silently produced no active tab. Matching on href removes the second identity. The active label moves from --gold to --ink: gold against --ink-dim is 1.18:1, carried almost entirely by hue, which nearly vanishes for a red-green colorblind reader. --ink is 1.68:1, still short of 3:1, so the gold hairline is still doing necessary work.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
Comment |
#67 split "no session" from "not admin" but left the first case covering two things: a visitor with no cookie at all, and one whose cookie no longer resolves. Both redirected to /login?error=session_expired, so anyone who typed /admin without ever signing in was told a session they never had had ended. resolveAdmin now returns session-expired only for a cookie that fails to resolve; an absent cookie gets the plain login page. This is the same line account/page.tsx draws, and the first time denyAdmin's exhaustive switch has actually caught an added union member.
1 participant
Follow-up to a design critique of
src/app/admin/layout.tsxandsrc/app/_components/admin-nav.tsx— the frame all three admin pages sit in.The problems
Every refusal looked the same.
resolveAdminreturnedAdminContext | null, so a de-roled admin and a signed-out visitor were indistinguishable. The first got a login screen for a session they still had; clicking a stale admin button surfaced the error boundary's "Something broke — that's a fault on this end, not something you did" for what is an expected race.There was no way to sign out.
The two headers were distinguishable only by which links happened to be in them — no register name, no distinct accessible name, and the same destination called
Adminfrom one bar andAccountsfrom the other.What's here
Guard.
resolveAdminreturns a discriminated union; one privatedenyAdmin(reason): nevermaps a reason to a destination. It's aswitchwith an explicitneverreturn, so a third denial reason fails to compile rather than falling through to the login redirect — verified by adding one (TS2534), then restoring.getAdminContextis no longer exported. AnAdminResolutionis always truthy, so the oldif (!ctx) redirect(...)still compiles against the new signature and simply never fires — an unguarded admin page with no type error and no failing test. Private makes a resurrected copy a compile error.The layout guards too, without replacing the per-page calls. Neither is sufficient alone: layouts don't re-run on soft navigation; pages don't cover the admin route added next and forgotten.
Sign-out, this device only. One session row, keyed by the digest of the caller's own cookie. POST-only — the session cookie is
sameSite: "lax", which the browser does attach to cross-site top-level GET navigations, so a GET route would be triggerable from any external link. Clearing attributes now come from one constant shared with the login callback: aSet-Cookiewhosepath/sameSite/securediffers names a different cookie, leaving the real session live while the response looks successful.Naming. The admin bar names its register and takes its own
aria-label. The roster is renamed Accounts → Members: both navs must call one destination one name (WCAG 3.2.4), but that name can't contain "account" when the member nav shows it beside "Your account" — a pair separated only by a possessive. Route andaccounttable unchanged.NavItemloseskey. Two callers used different conventions (route vs. arbitrary label); both typechecked, so a mismatch silently produced no active tab. Matching onhrefremoves the second identity.Active label
--gold→--ink. Gold vs--ink-dimis 1.18:1, carried almost entirely by hue, which nearly vanishes for a red-green colorblind reader.--inkis 1.68:1 — still short of 3:1, so the gold hairline is still doing necessary work, not decorating.Verification
npm run typechecknpm run lint<img>warnings)npm run format:checknpm test432 passed (432)npx playwright test62 passedThree claims checked by mutation, not just assertion:
AdminDenialmember →TS2534, restoredthrowinrequireAdminAction→ new de-role test fails, restoredendSession→ sign-out test fails, restoredNot run:
npm run build. Contrast figures are computed from the OKLCH tokens, not measured in a browser.Review focus
admin-guard.ts— the layout/page/action three-way split.requireAdminPageandrequireAdminActionnow have identical bodies; both names kept because merging touches twelve unrelated files. There's aNOTEin the source. Reasonable, or merge now?requireAdminActionthrow → redirect: actions that previously surfaced errors now navigate away./admin/accountswhile the page says Members is acceptable.:hoverare now both--ink, so a hovered inactive tab matches the active tab's text exactly — separated by hover's--hull-hifill vs. the active hairline.🤖 Generated with Claude Code