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>
113 lines
4.0 KiB
TypeScript
113 lines
4.0 KiB
TypeScript
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);
|
|
});
|
|
});
|