From 429ae0827fec69b44a0e93d67be4e58d33a2cafd Mon Sep 17 00:00:00 2001 From: yiekheng Date: Sun, 10 May 2026 21:08:40 +0800 Subject: [PATCH] fix(web): only ONE nav item highlighted at a time + drop redundant Close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. 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 . 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) --- apps/web/src/app/login/login-form-client.tsx | 2 +- apps/web/src/app/page.tsx | 2 +- .../src/app/reminders/[id]/actions-bar.tsx | 2 +- .../app/settings/users/user-row-client.tsx | 2 +- apps/web/src/components/app-shell.tsx | 5 +- apps/web/src/components/nav-config.test.ts | 88 +++++++++++++- apps/web/src/components/nav-config.ts | 36 ++++++ ...no-dialog-footer-show-close-button.test.ts | 112 ++++++++++++++++++ 8 files changed, 243 insertions(+), 6 deletions(-) create mode 100644 apps/web/src/test/no-dialog-footer-show-close-button.test.ts diff --git a/apps/web/src/app/login/login-form-client.tsx b/apps/web/src/app/login/login-form-client.tsx index 15cd900..41a2efb 100644 --- a/apps/web/src/app/login/login-form-client.tsx +++ b/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. - + 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 `` so we don't accidentally cross into the + // children. Multi-line opening tags are handled by `[\s\S]`. + const matches = content.matchAll( + //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 ", () => { + // 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 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 — :\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 ", () => { + expect( + findHits("foo"), + ).toHaveLength(1); + }); + it("matches when other props are present alongside showCloseButton", () => { + expect( + findHits(''), + ).toHaveLength(1); + }); + it("matches across multiple lines", () => { + const src = ``; + expect(findHits(src)).toHaveLength(1); + }); + it("does NOT match a clean ", () => { + expect(findHits("x")).toHaveLength(0); + }); + it("does NOT match a similarly-named prop on an unrelated component", () => { + expect(findHits("")).toHaveLength(0); + }); +});