feat(settings): add secondary sidebar and standalone sections - #520
Conversation
![mogplex[bot]](https://avatars.githubusercontent.com/in/3107927?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Solid, well-tested restructure of Settings into a secondary sidebar plus per-section pages. I found no security or data-correctness problems: the team Audit section is still gated on canManageMembers in TeamSettingsClient (with server enforcement behind /api/teams/[teamId]/audit-events), the new /[scope]/settings/billing return path still passes sanitizeReturnPath in lib/billing/checkout.ts, and getLegacySettingsDestination cannot produce a self-redirect loop (every destination differs from /[scope]/settings).
What I would like addressed before merge is one real state bug and a few smaller issues:
- The "Back to main navigation" state is keyed to the exact pathname, so it becomes stale and suppresses the Settings menu when the user returns to that same Settings URL later in the session (
components/app-sidebar.tsx,components/top-bar.tsx). - While the Settings menu is open, the rendered width is
Math.max(width, 240)whilewidth,aria-valuenow, andlocalStoragekeep tracking the raw value, so resizing inside Settings persists an invisible width that snaps on Back. - Pressing Back unmounts the focused button, dropping keyboard focus to
<body>. - Every team Settings section page still loads members + keys + models + model catalog through
useTeamSettingsActions, even on sections that need none of them.
Nothing here is a blocker for correctness of the shipped feature; they are UX/a11y/perf polish plus two readability notes. Verdict: approve-ready once the Back-state staleness and focus handling are resolved or consciously accepted.
Warnings
- Back-to-main-navigation state goes stale and hides the Settings menu on re-entry (components/app-sidebar.tsx)
const showSettings = inSettings && mainNavigationPath !== pathname;keeps the Back choice forever, keyed to one exact URL, and it is only cleared when the admin "Settings" link is clicked.
Repro: on /alex/settings/billing press Back (mainNavigationPath = /alex/settings/billing), navigate to /alex/control, then click "Manage billing" in the sidebar footer. You land back on /alex/settings/billing and the main navigation renders instead of the Settings menu, with no visible way to open the Settings menu for that page other than clicking Settings (which redirects to the default section).
Suggestion: treat Back as a transient toggle rather than a path memo — keep a boolean and reset it whenever the route leaves the Settings tree, e.g. useEffect(() => { if (!inSettings) setMainNavigationDismissed(false); }, [inSettings]);. The same logic is duplicated in components/top-bar.tsx (MobileSheetNav), so extracting a shared useSettingsNavState(scope, pathname) hook would keep desktop and mobile from diverging.
- Sidebar width state, aria-valuenow and localStorage diverge from the rendered width in Settings mode (components/app-sidebar.tsx)
style={{ width: showSettings ? Math.max(width, 240) : width }}overrides the rendered width, but the resizer remains active and keeps writing towidth/SIDEBAR_WIDTH_KEYand reportingaria-valuenow={Math.round(width)}.
Consequences while the Settings menu is open: dragging or arrow-keying the resizer below 240px produces no visual movement (looks broken), aria-valuenow reports a width the sidebar does not have (misleading for AT), and the persisted value silently changes so the sidebar jumps when the user presses Back.
Suggestion: either hide/disable the resizer while showSettings is true, or clamp the state itself (setWidth(Math.max(next, SETTINGS_MIN_WIDTH))) so state, ARIA, and rendering agree. Also promote the bare 240 to a named constant next to MIN_WIDTH / COMPACT_THRESHOLD / MAX_WIDTH.
- Focus is dropped when the Back button unmounts (components/settings/settings-navigation.tsx)
The Back button is the focused element when activated;onBackflipsshowSettingsto false, which unmountsSettingsNavigation(and the button) and leaves focus on<body>. Keyboard and screen-reader users lose their place and have to tab from the top of the document.
Suggestion: after restoring the main navigation, move focus to the Settings nav link (a ref on the settings item in SidebarNavLink, or the first primary link) inside a useEffect keyed on showSettings. The mobile sheet has the same pattern in components/top-bar.tsx.
- Each team Settings section page fetches all team settings data (components/settings/team-settings-client.tsx)
TeamSettingsClientcallsuseTeamSettingsActions(teamId)for every section, and that hook unconditionally subscribes to/api/teams/{id}/members,/keys,/modelsanduseModels()(plus/audit-eventsfor admins). Splitting the old tabbed page into four routes means the Audit page loads provider-key metadata and the full model catalog, the Keys page loads the audit log, and so on — revalidated on each client navigation between sections.
Suggestion: pass section into useTeamSettingsActions and pass null as the SWR key for datasets the active section does not render (the existing canManageMembers ? auditKey : null pattern already shows the shape). Members data is the only cross-section dependency (it drives canManageMembers and the header team name).
Suggestions
- Every Settings click round-trips through the client-side redirect shim (app/(dashboard)/[scope]/settings/settings-page-client.tsx)
/[scope]/settingsnow always rendersSettingsPageClient, which paints "Opening Settings…" and thenrouter.replaces in an effect. That is correct for legacy links (and necessary for#hashlinks, which the server cannot see), but it also makes the normal sidebar "Settings" click a two-step client navigation with a visible placeholder.
Suggestion: when searchParams is empty, do a server redirect() in page.tsx for the common case and keep the client shim only for query-bearing legacy links; or point the sidebar nav item at the resolved default section (/settings/account vs /settings/members) while keeping /settings in subpaths so isAppNavItemActive still highlights it. If the current behaviour is deliberate to keep hash links working, a short comment in settings-page-client.tsx explaining why the redirect must stay client-side would help the next reader.
- Team
billing404 guard only works because a static route shadows the dynamic segment (app/(dashboard)/[scope]/settings/[section]/page.tsx)
if (section !== "members" && section !== "keys" && section !== "models" && section !== "audit") notFound();would 404/[scope]/settings/billingfor teams — it is only unreachable becausesettings/billing/page.tsxtakes precedence oversettings/[section]/page.tsx. Same implicit dependency formcpon the personal side.
This is fragile if either static page is ever moved or renamed. Suggestion: add a brief comment noting that billing and mcp are handled by their own route segments, so this narrowing list intentionally excludes them.
- Mobile admin links skip the sheet-close callback wholesale (components/top-bar.tsx)
InMobileSheetNav, primary links getonClick={onNavigate}(closes the sheet) while every admin link getsonClick={() => setMainNavigationPath(null)}(keeps it open). Keeping the sheet open for Settings is the right drill-in behaviour, but it is keyed on theadminsection rather than on the item, so any future admin entry silently stops closing the sheet.
Suggestion: branch on item.id === "settings" and call onNavigate() for the rest.
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Reviewed the Settings secondary-sidebar refactor (routing, sidebar/mobile drill-in, Billing tabs, legacy redirects). No security or data-loss problems found: the team [section] page still resolves scope server-side and notFounds personal-only sections, and the audit section remains gated behind canManageMembers in TeamSettingsClient (the audit SWR fetch is also conditional), so making /team/settings/audit directly addressable does not widen access. Redirect mapping in lib/settings-redirect.ts looks correct for tab/hash/oauth/slack/billing combinations and preserves remaining query params. Four non-blocking issues: the /scope/settings entry point is now a JS-only client redirect for provider/checkout returns, Billing tab switching uses router.push (history pollution, regression from the previous replace-based tab handling), /team/settings/mcp still renders the personal MCP page although team nav and the [section] guard exclude it, and the sidebar Back control has a visible label ("Settings") that is not contained in its accessible name ("Back to main navigation"). Everything else — the render-phase state reset in useSettingsNavigation, focus restore, compact-width restore, and BillingUsage extraction — reads correctly.
4 findings were added inline.
| </div> | ||
| ) | ||
| // Keep the root redirect client-side: legacy hash links are not sent to the server. | ||
| router.replace(getLegacySettingsDestination(scope, query, window.location.hash), { scroll: false }); |
There was a problem hiding this comment.
Warning: Settings entry point is now a JS-only client redirect
/{scope}/settings no longer renders content; it renders "Opening Settings…" and redirects from a useEffect. Only the hash case genuinely needs the client (hashes are not sent to the server) — query-only entries, which include the OAuth/Slack/Stripe return URLs called out in the PR description, could be redirected server-side.
Impact: every legacy bookmark and provider return pays an extra client round trip and shows an intermediate flash, and any JS failure leaves the user parked on the placeholder instead of at Billing/Connections.
Suggestion: in settings/page.tsx, read searchParams and call redirect(getLegacySettingsDestination(scope, query)) when a query is present, keeping SettingsPageClient as the fallback for bare/hash-only URLs. If the client-only path was a deliberate choice (e.g. to avoid a dynamic render), a short comment explaining why would help.
| const params = new URLSearchParams(searchParams.toString()); | ||
| if (value === "usage") params.set("tab", "usage"); | ||
| else params.delete("tab"); | ||
| const query = params.toString(); |
There was a problem hiding this comment.
Warning: Billing tab switching pushes history entries
changeTab uses router.push, so each toggle between Billing Settings and Usage adds a history entry. The tab handlers this PR removes (handleTabChange in settings-page-client and team-settings-client) both used router.replace, so this is a behavior change: browser Back now cycles through tab states instead of returning to the previous page, which is especially confusing next to the new sidebar Back affordance.
Suggestion: use router.replace(..., { scroll: false }) to match the prior convention, unless deep-linkable back-navigation between the two tabs is an intentional requirement.
|
|
||
| if (scope.kind === "team") { | ||
| // Billing and MCP have static route segments; this page owns the other sections. | ||
| if (section !== "members" && section !== "keys" && section !== "models" && section !== "audit") notFound(); |
There was a problem hiding this comment.
Warning: Team scope can still reach the personal MCP page
TEAM_SETTINGS has no mcp entry and this guard notFounds anything outside members/keys/models/audit, but the static settings/mcp/page.tsx segment wins over [section] and performs no scope-kind check — /{team}/settings/mcp renders the personal MCP servers page with no nav entry pointing at it. The route predates this PR, but the PR is what establishes the personal-only section list, so the inconsistency is now part of this contract.
Suggestion: in settings/mcp/page.tsx, const scope = await getScopeContext(); if (scope.kind === "team") notFound(); (mirroring the guard here), or add mcp to TEAM_SETTINGS if team-scoped MCP is intended.
Related nit on the same page: the personal branch renders a title-only page for any PERSONAL_SETTINGS id without a matching component. It is unreachable today (mcp/billing have static routes), but a future section added to the list would silently render an empty page — consider a notFound() in the else branch.
|
|
||
| return ( | ||
| <nav aria-label="Settings" className="flex min-h-0 flex-1 flex-col gap-1 overflow-y-auto px-4"> | ||
| <button type="button" onClick={onBack} aria-label="Back to main navigation" |
There was a problem hiding this comment.
Suggestion: Back button's visible label is not part of its accessible name
The button shows "Settings" but aria-label="Back to main navigation" replaces that text in the accessible name, so speech-input users saying "click Settings" get no match (WCAG 2.5.3 Label in Name), and screen-reader users hear a label that never matches what sighted users describe.
Suggestion: drop the aria-label and add visually hidden text instead, e.g. <NavArrowLeft aria-hidden="true" /><span className="sr-only">Back to main navigation:</span>Settings, or keep the aria-label but make it contain the visible string ("Settings — back to main navigation").
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Solid, well-scoped navigation refactor. I checked the redirect helper, the new dynamic section route, the sidebar/mobile drill-in hook, the Billing tabs, and the audit authorization path — no security or correctness blockers found. Specifically verified: (1) getLegacySettingsDestination builds paths only from the PERSONAL_SETTINGS/TEAM_SETTINGS allowlist, so a crafted ?tab=/hash cannot inject a path segment, and remaining params are re-encoded by URLSearchParams; (2) the team Audit section is still gated by canManageMembers in team-settings-client.tsx with distinct loading/denied messages, and the nav gate (role === "owner" || "admin") matches the * capability grant in lib/team-capabilities.ts, so the new direct URL is not an authz regression; (3) returnPath is now /{scope}/settings/billing, which still passes SAFE_RETURN_PATH in lib/billing/checkout.ts; (4) useSettingsNavigation does a render-phase state adjustment, which is the sanctioned React pattern here, is conditional, is SSR-safe (initial state matches the incoming pathname, so no update on first render), and is called before the early return in MobileSheetNav, so hook order is stable. Remaining notes are warnings/suggestions only.
Warnings
- Billing tab switching carries the stale
billing=checkout param forward (components/settings/billing-section.tsx)
changeTabcopies the whole current query string and only adds/removestab:
const params = new URLSearchParams(searchParams.toString());
if (value === "usage") params.set("tab", "usage"); else params.delete("tab");
router.push(query ? `${pathname}?${query}` : pathname, { scroll: false });After a checkout return (/{scope}/settings/billing?billing=topup), switching to Usage pushes ?billing=topup&tab=usage, so the "Payment submitted…" role="status" banner stays visible on the Usage tab and is baked into every history entry the user creates afterwards. Since tab changes intentionally push history (per the PR description), the stale confirmation can also reappear on Back long after it is relevant.
Suggestion: params.delete("billing") inside changeTab (the banner has already been consumed and mutate() already fired in the checkoutResult effect). Low risk, one line.
- Settings menu resolves personal vs. team on the client even though the server route already knows the scope (components/settings/settings-navigation.tsx)
SettingsNavigationreconstructs the scope kind fromuseMemberships():
const team = memberships.teams.find((item) => item.slug === scope);
const personal = memberships.personal.slug === scope;
const items = team || personal ? buildSettingsNavItems(...) : [];Two consequences worth considering:
- On a cold load of a Settings URL the secondary menu renders "Loading settings…" until
/api/membershipsresolves, even thoughgetScopeContext()already gave the serverscope.kindfor the same request. The sidebar is the primary navigation surface here, so the empty-then-populate flash is the most visible part of the new UX. - If
/api/membershipserrors, the user lands on a Settings page whose only control is the Back button ("Settings navigation is unavailable."). That is a reasonable degradation, but the SWRerroris ignored —isLoadingfalse plus error produces the generic "unavailable" copy with no retry.
The role lookup genuinely needs memberships (for the Audit entry), so this is a design trade-off rather than a bug. Suggestion: pass the scope kind down from the [scope] layout (or derive it from the route) so the section list renders immediately, and keep memberships only for the Audit gate.
Suggestions
- Personal branch of the dynamic section route silently renders an empty page for
mcp/billing(app/(dashboard)/[scope]/settings/[section]/page.tsx)
The team branch has an explicit guard (if (section !== "members" && ... ) notFound();), but the personal branch renders<h1>{item.label}</h1>with no body whensectionismcporbilling. Today that is unreachable because Next.js prefers the staticsettings/mcpandsettings/billingsegments over the dynamic one — but the invariant is implicit, so renaming or removing a static route would produce a silent blank page with a correct-looking heading instead of a 404.
Suggestion: mirror the team-side guard, e.g. an explicit if (section === "mcp" || section === "billing") notFound(); with a comment, or a switch whose default calls notFound().
await getScopeContext()result is discarded without explanation (app/(dashboard)/[scope]/settings/billing/page.tsx)
export default async function BillingSettingsPage() {
await getScopeContext();
The return value is unused, so the call reads as dead code a future contributor would delete. Its real effects are non-obvious but load-bearing: it throws loudly if the scope middleware did not run, and reading `headers()` forces dynamic rendering, which is what keeps `BillingSection`'s `useSearchParams()` from tripping the missing-Suspense static-generation error. A one-line comment (as done elsewhere in this PR, e.g. the client-side redirect note in `settings-page-client.tsx`) would prevent an accidental removal.
- `/api/settings` is fetched only to derive an error banner (app/(dashboard)/[scope]/settings/_components/personal-account-page.tsx)
```ts
const { error: settingsError } = useSWR<SettingsView>("/api/settings", ...);
const settingsLoadError = settingsError ? "Unable to load settings preferences" : null;
The payload is discarded; the request exists purely so a failure can be reported. If the Account page no longer consumes SettingsView after the tab split, dropping the fetch removes a request per page load. If it is intentional (e.g. warming the SWR cache for another section, or surfacing an auth failure early), a short comment would make that clear.
- Question: connection-return params override an explicit section tab (lib/settings-redirect.ts)
IngetLegacySettingsDestination,isConnectionReturn = params.has("oauth") || params.has("slack")wins over atabthat names a real section, so a legacy link like/{scope}/settings?tab=keys&oauth=successlands on/connectionsrather than Provider Keys. That looks deliberate given the doc comment about provider return messages, and OAuth returns historically targeted the Connections tab — flagging only to confirm it is intended, since the precedence is easy to misread. If intended, extending the existing comment to say "provider returns win over an explicit tab" would settle it for the next reader.
1 participant
Summary
Settings now opens a secondary sidebar with a back arrow, following the supplied Vercel interaction. Personal and team sections have their own scoped pages, replacing Settings tabs. Billing has Billing Settings and Usage tabs; Usage has balance details and recent costs.
Why
Settings sections need direct URLs and room to grow without a horizontal tab bar. Bookmarks and old query/hash links redirect to the right page, including OAuth, Slack, and checkout return messages. Mobile uses the same drill-in menu; saved compact sidebar widths are restored on Back. Back restores keyboard focus to Settings; returning through another route opens the Settings menu again. Settings temporarily hides the resize handle while enforcing its readable minimum width.
Verification
pnpm lint(0 errors)pnpm typecheckMigration / Rollout Notes
Checklist