claude/settings-menu-urls-kbp6x8 (#116)

Co-authored-by: Claude <noreply@anthropic.com>
Reviewed-on: #116

app/components/league/settings/SettingsNavigation.tsx

@@ -1,6 +1,7 @@
 import type { ComponentType } from "react";
 import type { LucideProps } from "lucide-react";
 import { ArrowLeft } from "lucide-react";
+import { Link } from "react-router";
 import { cn } from "~/lib/utils";
 
 type SettingsNavSection = {
@@ -17,10 +18,37 @@ export type SettingsGridSection = SettingsNavSection & {
 export function SettingsMobileGridNav({
   sections,
   onSectionChange,
+  buildHref,
 }: {
   sections: readonly SettingsGridSection[];
-  onSectionChange: (sectionId: string) => void;
+  onSectionChange?: (sectionId: string) => void;
+  buildHref?: (sectionId: string) => string;
 }) {
+  const cardClassName = (section: SettingsGridSection) =>
+    cn(
+      "flex cursor-pointer flex-col gap-3 rounded-xl border bg-card p-4 text-left transition-colors hover:border-primary/40 active:scale-[0.98]",
+      section.isDanger && "border-destructive/40"
+    );
+
+  const cardInner = (section: SettingsGridSection) => (
+    <>
+      <div
+        className={cn(
+          "flex h-10 w-10 items-center justify-center rounded-lg",
+          section.isDanger
+            ? "bg-destructive/15 text-destructive"
+            : "bg-primary/20 text-primary"
+        )}
+      >
+        <section.icon className="h-5 w-5" />
+      </div>
+      <div>
+        <p className="text-sm font-semibold leading-tight">{section.label}</p>
+        <p className="mt-0.5 text-xs text-muted-foreground">{section.subtitle}</p>
+      </div>
+    </>
+  );
+
   return (
     <div className="mb-5 lg:hidden">
       <div className="mb-3">
@@ -29,32 +57,22 @@ export function SettingsMobileGridNav({
         </p>
       </div>
       <div className="grid grid-cols-2 gap-3">
-        {sections.map((section) => (
-          <button
-            key={section.id}
-            type="button"
-            onClick={() => onSectionChange(section.id)}
-            className={cn(
-              "flex cursor-pointer flex-col gap-3 rounded-xl border bg-card p-4 text-left transition-colors hover:border-primary/40 active:scale-[0.98]",
-              section.isDanger && "border-destructive/40"
-            )}
-          >
-            <div
-              className={cn(
-                "flex h-10 w-10 items-center justify-center rounded-lg",
-                section.isDanger
-                  ? "bg-destructive/15 text-destructive"
-                  : "bg-primary/20 text-primary"
-              )}
+        {sections.map((section) =>
+          buildHref ? (
+            <Link key={section.id} to={buildHref(section.id)} className={cardClassName(section)}>
+              {cardInner(section)}
+            </Link>
+          ) : (
+            <button
+              key={section.id}
+              type="button"
+              onClick={() => onSectionChange?.(section.id)}
+              className={cardClassName(section)}
             >
-              <section.icon className="h-5 w-5" />
-            </div>
-            <div>
-              <p className="text-sm font-semibold leading-tight">{section.label}</p>
-              <p className="mt-0.5 text-xs text-muted-foreground">{section.subtitle}</p>
-            </div>
-          </button>
-        ))}
+              {cardInner(section)}
+            </button>
+          )
+        )}
       </div>
     </div>
   );
@@ -62,19 +80,30 @@ export function SettingsMobileGridNav({
 
 export function SettingsMobileSectionPill({
   onShowGrid,
+  backHref,
 }: {
-  onShowGrid: () => void;
+  onShowGrid?: () => void;
+  backHref?: string;
 }) {
+  const className =
+    "flex cursor-pointer items-center gap-1.5 rounded-full border bg-card px-3 py-1.5 text-sm font-medium hover:bg-muted";
+  const content = (
+    <>
+      <ArrowLeft className="h-3.5 w-3.5" />
+      Back to all settings
+    </>
+  );
   return (
     <div className="mb-5 lg:hidden">
-      <button
-        type="button"
-        onClick={onShowGrid}
-        className="flex cursor-pointer items-center gap-1.5 rounded-full border bg-card px-3 py-1.5 text-sm font-medium hover:bg-muted"
-      >
-        <ArrowLeft className="h-3.5 w-3.5" />
-        Back to all settings
-      </button>
+      {backHref ? (
+        <Link to={backHref} className={className}>
+          {content}
+        </Link>
+      ) : (
+        <button type="button" onClick={onShowGrid} className={className}>
+          {content}
+        </button>
+      )}
     </div>
   );
 }
@@ -83,33 +112,57 @@ export function SettingsDesktopNav({
   sections,
   activeSection,
   onSectionChange,
+  buildHref,
+  navLabel = "League settings",
 }: {
   sections: readonly SettingsGridSection[];
   activeSection: string;
-  onSectionChange: (sectionId: string) => void;
+  onSectionChange?: (sectionId: string) => void;
+  buildHref?: (sectionId: string) => string;
+  navLabel?: string;
 }) {
+  const itemClassName = (section: SettingsGridSection) =>
+    cn(
+      "flex w-full cursor-pointer items-center gap-2.5 rounded-md px-3 py-2 text-left text-sm font-medium text-muted-foreground hover:bg-muted hover:text-foreground",
+      activeSection === section.id && "bg-muted text-foreground"
+    );
+
+  const itemInner = (section: SettingsGridSection) => (
+    <>
+      <section.icon className={cn("h-4 w-4 shrink-0", section.isDanger && "text-destructive")} aria-hidden="true" />
+      {section.label}
+    </>
+  );
+
   return (
     <aside className="hidden lg:block">
       <div className="sticky top-6 rounded-xl border bg-card p-3">
         <p className="px-3 pb-2 text-xs font-semibold uppercase tracking-wide text-muted-foreground">
           Manage
         </p>
-        <nav aria-label="League settings" className="space-y-1">
-          {sections.map((section) => (
-            <button
-              key={section.id}
-              type="button"
-              onClick={() => onSectionChange(section.id)}
-              aria-current={activeSection === section.id ? "page" : undefined}
-              className={cn(
-                "flex w-full cursor-pointer items-center gap-2.5 rounded-md px-3 py-2 text-left text-sm font-medium text-muted-foreground hover:bg-muted hover:text-foreground",
-                activeSection === section.id && "bg-muted text-foreground"
-              )}
-            >
-              <section.icon className={cn("h-4 w-4 shrink-0", section.isDanger && "text-destructive")} aria-hidden="true" />
-              {section.label}
-            </button>
-          ))}
+        <nav aria-label={navLabel} className="space-y-1">
+          {sections.map((section) =>
+            buildHref ? (
+              <Link
+                key={section.id}
+                to={buildHref(section.id)}
+                aria-current={activeSection === section.id ? "page" : undefined}
+                className={itemClassName(section)}
+              >
+                {itemInner(section)}
+              </Link>
+            ) : (
+              <button
+                key={section.id}
+                type="button"
+                onClick={() => onSectionChange?.(section.id)}
+                aria-current={activeSection === section.id ? "page" : undefined}
+                className={itemClassName(section)}
+              >
+                {itemInner(section)}
+              </button>
+            )
+          )}
         </nav>
       </div>
     </aside>

app/components/league/settings/__tests__/SettingsNavigation.test.tsx

@@ -0,0 +1,67 @@
+import { describe, it, expect, vi } from "vitest";
+import { render, screen, fireEvent } from "@testing-library/react";
+import { MemoryRouter } from "react-router";
+import { Bell, User } from "lucide-react";
+import {
+  SettingsDesktopNav,
+  SettingsMobileGridNav,
+  SettingsMobileSectionPill,
+  type SettingsGridSection,
+} from "../SettingsNavigation";
+
+const sections: readonly SettingsGridSection[] = [
+  { id: "profile", label: "Profile", icon: User, subtitle: "Name, avatar" },
+  { id: "notifications", label: "Notifications", icon: Bell, subtitle: "Email & Discord" },
+];
+
+describe("SettingsNavigation link mode", () => {
+  it("renders desktop nav entries as anchors built from buildHref", () => {
+    render(
+      <MemoryRouter>
+        <SettingsDesktopNav
+          sections={sections}
+          activeSection="profile"
+          buildHref={(id) => `/settings/${id}`}
+        />
+      </MemoryRouter>
+    );
+
+    expect(screen.getByRole("link", { name: "Profile" })).toHaveAttribute("href", "/settings/profile");
+    expect(screen.getByRole("link", { name: "Notifications" })).toHaveAttribute(
+      "href",
+      "/settings/notifications"
+    );
+    expect(screen.getByRole("link", { name: "Profile" })).toHaveAttribute("aria-current", "page");
+  });
+
+  it("renders the mobile grid as anchors and the back pill as an anchor", () => {
+    render(
+      <MemoryRouter>
+        <SettingsMobileGridNav sections={sections} buildHref={(id) => `/settings/${id}`} />
+        <SettingsMobileSectionPill backHref="/settings" />
+      </MemoryRouter>
+    );
+
+    expect(screen.getByRole("link", { name: /Profile/i })).toHaveAttribute("href", "/settings/profile");
+    expect(screen.getByRole("link", { name: /back to all settings/i })).toHaveAttribute(
+      "href",
+      "/settings"
+    );
+  });
+});
+
+describe("SettingsNavigation button mode (league settings)", () => {
+  it("still fires onSectionChange when no buildHref is provided", () => {
+    const onSectionChange = vi.fn();
+    render(
+      <SettingsDesktopNav
+        sections={sections}
+        activeSection="profile"
+        onSectionChange={onSectionChange}
+      />
+    );
+
+    fireEvent.click(screen.getByRole("button", { name: "Notifications" }));
+    expect(onSectionChange).toHaveBeenCalledWith("notifications");
+  });
+});

app/components/user/settings/AccountSection.tsx

@@ -31,7 +31,7 @@ export function AccountSection({ email, linkedAccounts }: Props) {
     setLinkingDiscord(true);
     setLinkError(null);
     try {
-      await authClient.linkSocial({ provider: "discord", callbackURL: "/settings?section=account" });
+      await authClient.linkSocial({ provider: "discord", callbackURL: "/settings/account" });
     } catch {
       setLinkError("Failed to connect Discord. Please try again.");
       setLinkingDiscord(false);

app/components/user/settings/NotificationsSection.tsx

@@ -1,4 +1,4 @@
-import { useFetcher } from "react-router";
+import { Link, useFetcher } from "react-router";
 import { Switch } from "~/components/ui/switch";
 import { Label } from "~/components/ui/label";
 
@@ -14,10 +14,9 @@ type Props = {
   discordPingEnabled: boolean;
   hasDiscordLinked: boolean;
   draftEmailNotificationsEnabled: boolean;
-  onNavigateToAccount: () => void;
 };
 
-export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, draftEmailNotificationsEnabled, onNavigateToAccount }: Props) {
+export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, draftEmailNotificationsEnabled }: Props) {
   const discordFetcher = useFetcher();
   const emailFetcher = useFetcher();
 
@@ -87,13 +86,12 @@ export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, dra
         {!hasDiscordLinked ? (
           <p className="text-sm text-muted-foreground">
             Link your Discord account in{" "}
-            <button
-              type="button"
-              onClick={onNavigateToAccount}
+            <Link
+              to="/settings/account"
               className="text-primary underline-offset-4 hover:underline"
             >
               Account settings
-            </button>{" "}
+            </Link>{" "}
             to enable Discord pings for league notifications.
           </p>
         ) : (

app/components/user/settings/__tests__/AccountSection.test.tsx

@@ -55,7 +55,7 @@ describe("AccountSection", () => {
     await waitFor(() => {
       expect(mockLinkSocial).toHaveBeenCalledWith({
         provider: "discord",
-        callbackURL: "/settings?section=account",
+        callbackURL: "/settings/account",
       });
     });
   });

app/routes.ts

@@ -69,7 +69,7 @@ export default [
   route("forgot-password", "routes/forgot-password.tsx"),
   route("reset-password", "routes/reset-password.tsx"),
   route("onboarding", "routes/onboarding.tsx"),
-  route("settings", "routes/settings.tsx"),
+  route("settings/:section?", "routes/settings.tsx"),
   route("user-profile", "routes/user-profile-redirect.tsx"),
   route("how-to-play", "routes/how-to-play.tsx"),
   route("rules", "routes/rules.tsx"),

app/routes/__tests__/settings.test.ts

@@ -0,0 +1,102 @@
+import { describe, it, expect, vi } from "vitest";
+
+vi.mock("~/lib/auth.server", () => ({
+  auth: { api: { getSession: vi.fn() } },
+}));
+vi.mock("~/lib/cloudinary.server", () => ({ deleteCloudinaryImageByUrl: vi.fn() }));
+vi.mock("~/lib/email.server", () => ({
+  sendEmail: vi.fn(),
+  wrapInEmailTemplate: vi.fn(),
+  emailParagraph: vi.fn(),
+  escapeHtml: vi.fn(),
+}));
+vi.mock("~/models/user", () => ({
+  findUserById: vi.fn(),
+  findUserByUsername: vi.fn(),
+  isUserInActiveDraft: vi.fn(),
+  updateUser: vi.fn(),
+  anonymizeUserAccount: vi.fn(),
+  USERNAME_RE: /^[a-zA-Z0-9_-]{3,30}$/,
+}));
+vi.mock("~/models/account", () => ({ findLinkedAccountsByUserId: vi.fn() }));
+vi.mock("~/models/team", () => ({ findTeamsByOwnerId: vi.fn(), removeTeamOwner: vi.fn() }));
+vi.mock("~/models/commissioner", () => ({ removeAllCommissionersByUserId: vi.fn() }));
+
+import { loader, shouldRevalidate } from "../settings";
+import { auth } from "~/lib/auth.server";
+
+type RevalidateArgs = Parameters<typeof shouldRevalidate>[0];
+
+function revalidateArgs(overrides: Partial<RevalidateArgs>): RevalidateArgs {
+  return {
+    currentParams: {},
+    nextParams: {},
+    formMethod: undefined,
+    defaultShouldRevalidate: true,
+    ...overrides,
+  } as unknown as RevalidateArgs;
+}
+
+type LoaderArgs = Parameters<typeof loader>[0];
+
+function loaderArgs(section?: string): LoaderArgs {
+  const path = section ? `/settings/${section}` : "/settings";
+  return {
+    params: { section },
+    request: new Request(`http://localhost${path}`),
+    context: {},
+  } as unknown as LoaderArgs;
+}
+
+describe("/settings loader section handling", () => {
+  it("redirects an unknown section back to /settings", async () => {
+    const result = (await loader(loaderArgs("bogus"))) as Response;
+    expect(result).toBeInstanceOf(Response);
+    expect(result.status).toBe(302);
+    expect(result.headers.get("Location")).toBe("/settings");
+    // The bad section short-circuits before we ever touch the session.
+    expect(auth.api.getSession).not.toHaveBeenCalled();
+  });
+
+  it("does not redirect a valid section before auth runs", async () => {
+    vi.mocked(auth.api.getSession).mockResolvedValue(null as never);
+    const result = (await loader(loaderArgs("account"))) as Response;
+    // Falls through to the auth check, which redirects to login (not /settings).
+    expect(result.headers.get("Location")).toBe("/login?redirectTo=/settings");
+    expect(auth.api.getSession).toHaveBeenCalled();
+  });
+});
+
+describe("/settings shouldRevalidate", () => {
+  it("skips revalidation when only the section param changes", () => {
+    expect(
+      shouldRevalidate(
+        revalidateArgs({ currentParams: { section: "profile" }, nextParams: { section: "account" } })
+      )
+    ).toBe(false);
+  });
+
+  it("revalidates after a mutation regardless of section", () => {
+    expect(
+      shouldRevalidate(
+        revalidateArgs({
+          currentParams: { section: "profile" },
+          nextParams: { section: "account" },
+          formMethod: "POST",
+        })
+      )
+    ).toBe(true);
+  });
+
+  it("defers to the default when the section is unchanged", () => {
+    expect(
+      shouldRevalidate(
+        revalidateArgs({
+          currentParams: { section: "account" },
+          nextParams: { section: "account" },
+          defaultShouldRevalidate: true,
+        })
+      )
+    ).toBe(true);
+  });
+});

app/routes/settings.tsx

@@ -1,5 +1,4 @@
-import { redirect, useSearchParams } from "react-router";
-import { useState } from "react";
+import { redirect, type ShouldRevalidateFunctionArgs } from "react-router";
 import { Bell, Key, Lock, Shield, User } from "lucide-react";
 import { auth } from "~/lib/auth.server";
 import { findUserById, findUserByUsername, isUserInActiveDraft, updateUser, anonymizeUserAccount, USERNAME_RE } from "~/models/user";
@@ -57,7 +56,26 @@ export function meta(): Route.MetaDescriptors {
   return [{ title: "Settings - Brackt" }];
 }
 
+export function shouldRevalidate({
+  currentParams,
+  nextParams,
+  formMethod,
+  defaultShouldRevalidate,
+}: ShouldRevalidateFunctionArgs) {
+  // Mutations (profile/avatar/notification updates) must refresh loader data.
+  if (formMethod && formMethod !== "GET") return true;
+  // Switching sections only changes the path param, and the loader payload is the
+  // same for every section, so don't refetch user/draft status/linked accounts.
+  if (currentParams.section !== nextParams.section) return false;
+  return defaultShouldRevalidate;
+}
+
 export async function loader(args: Route.LoaderArgs) {
+  const section = args.params.section;
+  if (section && !(VALID_SECTION_IDS as Set<string>).has(section)) {
+    return redirect("/settings");
+  }
+
   const session = await auth.api.getSession({ headers: args.request.headers });
   if (!session) {
     return redirect("/login?redirectTo=/settings");
@@ -213,18 +231,12 @@ export async function action(args: Route.ActionArgs): Promise<ActionData | Respo
   return { intent: intent as string, error: "Unknown action." };
 }
 
-export default function SettingsPage({ loaderData, actionData }: Route.ComponentProps) {
+export default function SettingsPage({ loaderData, actionData, params }: Route.ComponentProps) {
   const { user, isInActiveDraft, linkedAccounts, dataRequestCooldownUntil } = loaderData;
-  const [searchParams] = useSearchParams();
-  const raw = searchParams.get("section");
-  const initialSection: SectionId = raw && (VALID_SECTION_IDS as Set<string>).has(raw) ? (raw as SectionId) : "profile";
-  const [activeSection, setActiveSection] = useState<SectionId>(initialSection);
-  const [mobileView, setMobileView] = useState<"grid" | "section">("grid");
-
-  const handleSectionChange = (id: string, mobile = false) => {
-    setActiveSection(id as SectionId);
-    if (mobile) setMobileView("section");
-  };
+  const section = params.section;
+  const activeSection: SectionId =
+    section && (VALID_SECTION_IDS as Set<string>).has(section) ? (section as SectionId) : "profile";
+  const mobileView: "grid" | "section" = section ? "section" : "grid";
 
   const ad = actionData as ActionData | undefined;
   const profileSuccess = ad?.intent === "update-profile" && "success" in ad;
@@ -246,20 +258,21 @@ export default function SettingsPage({ loaderData, actionData }: Route.Component
         <div className="lg:hidden">
           <SettingsMobileGridNav
             sections={SECTIONS}
-            onSectionChange={(id) => handleSectionChange(id, true)}
+            buildHref={(id) => `/settings/${id}`}
           />
         </div>
       )}
 
       {mobileView === "section" && (
-        <SettingsMobileSectionPill onShowGrid={() => setMobileView("grid")} />
+        <SettingsMobileSectionPill backHref="/settings" />
       )}
 
       <div className={`grid gap-6 lg:grid-cols-[220px_minmax(0,1fr)] ${mobileView === "grid" ? "hidden lg:grid" : "grid"}`}>
         <SettingsDesktopNav
           sections={SECTIONS}
           activeSection={activeSection}
-          onSectionChange={(id) => handleSectionChange(id)}
+          buildHref={(id) => `/settings/${id}`}
+          navLabel="Account settings"
         />
 
         <main className="min-w-0">
@@ -279,7 +292,6 @@ export default function SettingsPage({ loaderData, actionData }: Route.Component
               discordPingEnabled={user.discordPingEnabled}
               hasDiscordLinked={linkedAccounts.some((a) => a.providerId === "discord")}
               draftEmailNotificationsEnabled={user.draftEmailNotificationsEnabled}
-              onNavigateToAccount={() => handleSectionChange("account")}
             />
           )}
           {activeSection === "api" && <ApiSection />}