yiekheng 429ae0827f 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>
2026-05-10 21:08:40 +08:00

120 lines
4.6 KiB
TypeScript

import { describe, it, expect } from "vitest";
import { NAV_ITEMS, navItemsForRole, pickActiveNavKey } from "./nav-config";
describe("navItemsForRole", () => {
it("includes every NAV_ITEM for an admin", () => {
const items = navItemsForRole("admin");
expect(items).toHaveLength(NAV_ITEMS.length);
for (const original of NAV_ITEMS) {
expect(items.find((i) => i.key === original.key)).toBeDefined();
}
});
it("hides admin-only entries for the 'user' role", () => {
const items = navItemsForRole("user");
const keys = items.map((i) => i.key);
expect(keys).not.toContain("admin");
});
it("returns the un-restricted entries (no visibleTo) for the 'user' role", () => {
const items = navItemsForRole("user");
const keys = items.map((i) => i.key);
expect(keys).toEqual(
expect.arrayContaining(["dashboard", "accounts", "reminders", "activity", "settings"]),
);
});
it("admin nav entry routes to /settings/users", () => {
const admin = NAV_ITEMS.find((i) => i.key === "admin");
expect(admin).toBeDefined();
expect(admin!.href).toBe("/settings/users");
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();
}
}
});
});