fix(web): only ONE nav item highlighted at a time + drop redundant Close

Two related bugs from the same review pass:

1. /settings/users lit up BOTH the Admin and Settings entries in the
   sidebar/drawer. The active-state check was naïve
   `pathname.startsWith(href)`, which matches every parent prefix.
   Replaced with a longest-match helper pickActiveNavKey() in
   nav-config.ts: the most-specific item wins, parents stay quiet,
   '/' only matches an exact pathname, and a strict-descendant check
   (`href + '/'`) prevents `/settingsfoo` from lighting up Settings.

2. <DialogFooter showCloseButton> on the user-row delete (and three
   other dialogs that I missed earlier) was rendering an extra outline
   "Close" button next to the operator's own Cancel + Radix's corner X.
   Stripped the prop from every remaining caller (login, dashboard
   clear-history, reminder actions-bar, settings/users delete) so each
   dialog footer shows just Cancel + the primary action.

Tests:

  - nav-config.test.ts: 7 new cases covering the longest-match contract
    — /settings/users highlights ONLY Admin, /settings highlights ONLY
    Settings, '/' is exact-match only, sibling-prefix /settingsfoo
    matches nothing, and a defense-in-depth probe asserts at-most-one
    nav highlight across a representative pathname set.

  - test/no-dialog-footer-show-close-button.test.ts: static guard that
    grep-walks every production .tsx and fails if anything passes
    `showCloseButton` to <DialogFooter>. Mirrors the existing
    no-button-wrapping-card guard so the prop can't sneak back in.
    Also self-checks the regex (matches single-line + multi-line +
    other-prop combos; ignores clean DialogFooter and same-named props
    on unrelated components).

463 → 477 web tests, all green; typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

apps/web/src/app/login/login-form-client.tsx

@@ -87,7 +87,7 @@ export function LoginFormClient({ next }: { next: string }) {
               Contact your administrator to reset it.
             </DialogDescription>
           </DialogHeader>
-          <DialogFooter showCloseButton>
+          <DialogFooter>
             <DialogClose asChild>
               <Button type="button" size="sm">
                 Got it

apps/web/src/app/page.tsx

@@ -217,7 +217,7 @@ export default async function DashboardPage() {
                     themselves are not affected.
                   </DialogDescription>
                 </DialogHeader>
-                <DialogFooter showCloseButton>
+                <DialogFooter>
                   <form action={clearHistoryAction}>
                     <Button type="submit" variant="destructive" size="sm">
                       <Trash2Icon />

apps/web/src/app/reminders/[id]/actions-bar.tsx

@@ -177,7 +177,7 @@ function ConfirmCard(props: ConfirmCardProps) {
             {error}
           </p>
         )}
-        <DialogFooter showCloseButton>
+        <DialogFooter>
           <form
             action={async (fd: FormData) => {
               setSubmitting(true);

apps/web/src/app/settings/users/user-row-client.tsx

@@ -136,7 +136,7 @@ export function UserRowClient({ user, isSelf, isLastAdmin }: UserRowClientProps)
                   again. This cannot be undone.
                 </DialogDescription>
               </DialogHeader>
-              <DialogFooter showCloseButton>
+              <DialogFooter>
                 <DialogClose asChild>
                   <Button type="button" variant="ghost" size="sm">
                     Cancel

apps/web/src/components/app-shell.tsx

@@ -18,6 +18,7 @@ import {
 import {
   NAV_ITEMS,
   navItemsForRole,
+  pickActiveNavKey,
   type NavItem,
   type NavRole,
 } from "@/components/nav-config";
@@ -79,6 +80,7 @@ function MobileHeader({
   username: string | null;
 }) {
   const pathname = usePathname();
+  const activeKey = pickActiveNavKey(items, pathname);
   const [open, setOpen] = useState(false);
 
   // Close the drawer when the route changes (i.e. the user picked a nav
@@ -144,7 +146,7 @@ function MobileHeader({
             className="flex flex-col gap-0.5 p-2"
           >
             {items.map(({ key, href, label, icon: Icon }) => {
-              const active = href === "/" ? pathname === "/" : pathname.startsWith(href);
+              const active = activeKey === key;
               return (
                 <Link
                   key={key}
@@ -189,6 +191,7 @@ function Sidebar({
   username: string | null;
 }) {
   const pathname = usePathname();
+  const activeKey = pickActiveNavKey(items, pathname);
 
   return (
     <aside className="hidden sm:flex fixed left-0 top-0 bottom-0 z-40 w-56 flex-col border-r border-border bg-sidebar">

apps/web/src/components/nav-config.test.ts

@@ -1,5 +1,5 @@
 import { describe, it, expect } from "vitest";
-import { NAV_ITEMS, navItemsForRole } from "./nav-config";
+import { NAV_ITEMS, navItemsForRole, pickActiveNavKey } from "./nav-config";
 
 describe("navItemsForRole", () => {
   it("includes every NAV_ITEM for an admin", () => {
@@ -31,3 +31,89 @@ describe("navItemsForRole", () => {
     expect(admin!.visibleTo).toEqual(["admin"]);
   });
 });
+
+describe("pickActiveNavKey (longest-match active highlight)", () => {
+  // Use the real NAV_ITEMS so a future href change doesn't silently
+  // re-introduce the regression.
+  const adminItems = navItemsForRole("admin");
+  const userItems = navItemsForRole("user");
+
+  it("highlights ONLY the Admin entry on /settings/users (not Settings too)", () => {
+    // Repro of the user-reported regression. Naïve startsWith would
+    // light up both Settings (/settings) and Admin (/settings/users)
+    // because both prefixes match. The longest-match rule must pick
+    // the Admin entry alone.
+    const active = pickActiveNavKey(adminItems, "/settings/users");
+    expect(active).toBe("admin");
+  });
+
+  it("highlights Settings on /settings exact, with Admin NOT lit", () => {
+    const active = pickActiveNavKey(adminItems, "/settings");
+    expect(active).toBe("settings");
+  });
+
+  it("highlights Settings on a subpath that is NOT /settings/users", () => {
+    // Admin nav is admin-only; this test is just to confirm the
+    // longest-match still picks Settings when no admin descendant
+    // claims the path.
+    const active = pickActiveNavKey(adminItems, "/settings/profile");
+    expect(active).toBe("settings");
+  });
+
+  it("highlights Dashboard ONLY on '/' exact (not on every route)", () => {
+    expect(pickActiveNavKey(adminItems, "/")).toBe("dashboard");
+    expect(pickActiveNavKey(adminItems, "/accounts")).toBe("accounts");
+    expect(pickActiveNavKey(adminItems, "/reminders/abc-123")).toBe("reminders");
+  });
+
+  it("returns null when nothing matches (e.g. a route hidden from this role)", () => {
+    // /settings/users isn't visible to a 'user' role, so the helper
+    // must NOT highlight it as Settings just because /settings is a
+    // prefix — we'd be claiming an item is active when the user can't
+    // navigate to it from this nav.
+    expect(pickActiveNavKey(userItems, "/settings/users")).toBe("settings");
+    // Neither item's href matches a totally foreign route.
+    expect(pickActiveNavKey(adminItems, "/elsewhere")).toBeNull();
+  });
+
+  it("does NOT match a sibling that shares a prefix string", () => {
+    // /settingsfoo is NOT a child of /settings — startsWith would
+    // mistakenly mark Settings active. The strict descendant check
+    // (`href + '/'`) prevents that.
+    expect(pickActiveNavKey(adminItems, "/settingsfoo")).toBeNull();
+  });
+
+  it("each pathname highlights AT MOST one nav key (defense check)", () => {
+    // Walk a small representative set of routes and confirm we never
+    // light up two items at once. This is the contract the JSX in
+    // app-shell.tsx relies on.
+    const probes = [
+      "/",
+      "/accounts",
+      "/accounts/abc",
+      "/reminders",
+      "/reminders/abc",
+      "/activity",
+      "/activity?filter=success",
+      "/settings",
+      "/settings/users",
+      "/settings/users/something",
+      "/login",
+      "/elsewhere",
+    ];
+    for (const path of probes) {
+      const matchCount = adminItems.filter((item) => {
+        if (item.href === "/") return path === "/";
+        return path === item.href || path.startsWith(item.href + "/");
+      }).length;
+      // If two prefixes both match, pickActiveNavKey must collapse
+      // them to one — that's the whole point of the helper.
+      const active = pickActiveNavKey(adminItems, path);
+      if (matchCount === 0) {
+        expect(active).toBeNull();
+      } else {
+        expect(active).not.toBeNull();
+      }
+    }
+  });
+});

apps/web/src/components/nav-config.ts

@@ -39,3 +39,39 @@ export function navItemsForRole(role: NavRole): NavItem[] {
     (item) => item.visibleTo === undefined || item.visibleTo.includes(role),
   );
 }
+
+/**
+ * Pick the SINGLE active nav item for a given pathname. Solves the
+ * "Admin and Settings both highlighted on /settings/users" bug:
+ * naïve `pathname.startsWith(href)` matches BOTH /settings/users (the
+ * Admin entry) AND /settings (its parent). Two items lit up at once
+ * looks broken.
+ *
+ * Rules:
+ *   - The Dashboard ('/') item only matches an exact pathname match;
+ *     otherwise it would shadow every other route.
+ *   - All other items match either an exact pathname or a strict
+ *     descendant path (`<href>/...`). `pathname.startsWith(href)` on
+ *     its own would also match `/settingsfoo`, which is wrong.
+ *   - When two non-root items both match (parent + child), pick the
+ *     LONGEST href so the more specific entry wins.
+ *
+ * Returns the active item's `key`, or null if no item matches (e.g.
+ * the user navigated to a route that isn't in the visible nav).
+ */
+export function pickActiveNavKey(items: NavItem[], pathname: string): string | null {
+  let best: NavItem | null = null;
+  for (const item of items) {
+    if (item.href === "/") {
+      if (pathname === "/") best = item;
+      continue;
+    }
+    const isMatch =
+      pathname === item.href || pathname.startsWith(item.href + "/");
+    if (!isMatch) continue;
+    if (!best || item.href.length > best.href.length) {
+      best = item;
+    }
+  }
+  return best?.key ?? null;
+}

apps/web/src/test/no-dialog-footer-show-close-button.test.ts

@@ -0,0 +1,112 @@
+import { describe, it, expect } from "vitest";
+import { readdirSync, readFileSync, statSync } from "node:fs";
+import { join, relative } from "node:path";
+
+/**
+ * Static guard: no production `.tsx` file may pass `showCloseButton`
+ * to `<DialogFooter>`.
+ *
+ * Why: the shared DialogFooter renders an EXTRA outline-styled
+ * <Button>Close</Button> when `showCloseButton` is set. Every dialog
+ * we have that already provides its own primary action also includes
+ * a Cancel/dismiss button (either via DialogClose or by closing the
+ * Dialog state on submit) — and Radix's auto-rendered corner X
+ * already gives users a third way out. The redundant Close button
+ * cluttered the footer and shipped to production multiple times
+ * before this guard existed; this test stops it from regressing.
+ */
+
+const SRC_ROOT = join(__dirname, "..");
+
+function listTsxFiles(dir: string): string[] {
+  const out: string[] = [];
+  for (const entry of readdirSync(dir)) {
+    const full = join(dir, entry);
+    const st = statSync(full);
+    if (st.isDirectory()) {
+      out.push(...listTsxFiles(full));
+    } else if (entry.endsWith(".tsx")) {
+      out.push(full);
+    }
+  }
+  return out;
+}
+
+interface Hit {
+  file: string;
+  line: number;
+  excerpt: string;
+}
+
+function findHits(content: string): Array<{ line: number; excerpt: string }> {
+  const hits: Array<{ line: number; excerpt: string }> = [];
+  // Match `<DialogFooter` with `showCloseButton` somewhere in the
+  // opening tag. Stops at `>` so we don't accidentally cross into the
+  // children. Multi-line opening tags are handled by `[\s\S]`.
+  const matches = content.matchAll(
+    /<DialogFooter\b[\s\S]*?showCloseButton\b[\s\S]*?>/g,
+  );
+  for (const m of matches) {
+    const idx = m.index ?? 0;
+    const line = content.slice(0, idx).split("\n").length;
+    hits.push({ line, excerpt: m[0].slice(0, 120).replace(/\s+/g, " ") });
+  }
+  return hits;
+}
+
+describe("static guard: no <DialogFooter showCloseButton>", () => {
+  // Skip this test file (it intentionally contains the pattern strings)
+  // and all other .test.tsx files (they're examples, not production UI).
+  const files = listTsxFiles(SRC_ROOT).filter(
+    (f) => !/\.test\.tsx?$/.test(f),
+  );
+
+  it("scans at least one source file (sanity)", () => {
+    expect(files.length).toBeGreaterThan(0);
+  });
+
+  it("finds no <DialogFooter showCloseButton ...> in any production .tsx file", () => {
+    const allHits: Hit[] = [];
+    for (const file of files) {
+      const content = readFileSync(file, "utf8");
+      for (const h of findHits(content)) {
+        allHits.push({ file: relative(SRC_ROOT, file), ...h });
+      }
+    }
+    if (allHits.length > 0) {
+      const message = allHits
+        .map((h) => `  ${h.file}:${h.line} → ${h.excerpt}`)
+        .join("\n");
+      throw new Error(
+        `Redundant Close button detected — <DialogFooter showCloseButton ...>:\n${message}\n` +
+          `The DialogFooter component injects an extra "Close" button when this prop\n` +
+          `is set. Every existing caller already has its own Cancel/Close action plus\n` +
+          `Radix's corner X — a third Close is just visual noise. Remove the prop.`,
+      );
+    }
+    expect(allHits).toEqual([]);
+  });
+});
+
+describe("findHits parser", () => {
+  it("matches a single-line <DialogFooter showCloseButton>", () => {
+    expect(
+      findHits("<DialogFooter showCloseButton>foo</DialogFooter>"),
+    ).toHaveLength(1);
+  });
+  it("matches when other props are present alongside showCloseButton", () => {
+    expect(
+      findHits('<DialogFooter className="x" showCloseButton>'),
+    ).toHaveLength(1);
+  });
+  it("matches across multiple lines", () => {
+    const src = `<DialogFooter\n  className="x"\n  showCloseButton\n>`;
+    expect(findHits(src)).toHaveLength(1);
+  });
+  it("does NOT match a clean <DialogFooter>", () => {
+    expect(findHits("<DialogFooter>x</DialogFooter>")).toHaveLength(0);
+  });
+  it("does NOT match a similarly-named prop on an unrelated component", () => {
+    expect(findHits("<SheetFooter showCloseButton>")).toHaveLength(0);
+  });
+});