diff --git a/admin/src/features/activity/ActivityPage.test.tsx b/admin/src/features/activity/ActivityPage.test.tsx index b03879c..7f75117 100644 --- a/admin/src/features/activity/ActivityPage.test.tsx +++ b/admin/src/features/activity/ActivityPage.test.tsx @@ -163,7 +163,7 @@ test("renders the first page with the seven columns filled in", async () => { expect(screen.getByText(/Showing 2 queries/)).toBeTruthy(); }); -test("resolves each row's client to its display name, keeping the IP as the tooltip", async () => { +test("resolves each row's client to its display name, reading the IP out with it", async () => { stubFetch((url) => { if (url === "/api/clients") return json({ clients: CLIENTS }); if (url !== "/api/queries") return new Response(JSON.stringify({ error: "not stubbed" }), { status: 404 }); @@ -183,19 +183,22 @@ test("resolves each row's client to its display name, keeping the IP as the tool // A hand-typed name wins outright; the learned name never surfaces for it. const named = await screen.findByText("Kitchen Pi"); - expect(named.getAttribute("title")).toBe("192.0.2.10"); + // The address reads out with the name it replaced, rather than sitting in a + // title only a mouse can reach. + expect(named.textContent).toBe("Kitchen Pi (192.0.2.10)"); + expect(named.getAttribute("title")).toBeNull(); expect(screen.queryByText("pi.lan")).toBeNull(); // A learned name reads muted and nothing more here: the "learned" tag would // repeat on every row of the table, so the Clients page carries it instead. const learned = screen.getByText("laptop.lan"); - expect(learned.getAttribute("title")).toBe("192.0.2.11"); + expect(learned.textContent).toBe("laptop.lan (192.0.2.11)"); expect(within(learned.closest("tr")!).queryByText("learned")).toBeNull(); // A known client with neither name, and a client the loaded list has never - // seen, both fall back to the bare address with no tooltip standing in. - expect(screen.getByText("192.0.2.12").getAttribute("title")).toBeNull(); - expect(screen.getByText("192.0.2.99").getAttribute("title")).toBeNull(); + // seen, both fall back to the bare address with nothing standing in for it. + expect(screen.getByText("192.0.2.12").textContent).toBe("192.0.2.12"); + expect(screen.getByText("192.0.2.99").textContent).toBe("192.0.2.99"); }); test("load more appends the next page and stops at the end of the log", async () => { diff --git a/admin/src/features/activity/LiveActivity.test.tsx b/admin/src/features/activity/LiveActivity.test.tsx index e9b8ae1..0ed26eb 100644 --- a/admin/src/features/activity/LiveActivity.test.tsx +++ b/admin/src/features/activity/LiveActivity.test.tsx @@ -178,7 +178,7 @@ test("streams rows, flags blocked ones, and freezes the display", async () => { expect(screen.getByText("later.example")).toBeTruthy(); }); -test("resolves each row's client to its display name, keeping the IP as the tooltip", async () => { +test("resolves each row's client to its display name, reading the IP out with it", async () => { await openLive(); act(() => { sources[0]!.emit("query", frame(1000, "named.example", { request: { client: "192.0.2.10" } })); @@ -188,11 +188,14 @@ test("resolves each row's client to its display name, keeping the IP as the tool }); const named = await screen.findByText("Kitchen Pi"); - expect(named.getAttribute("title")).toBe("192.0.2.10"); + // The address reads out with the name it replaced, rather than sitting in a + // title only a mouse can reach. + expect(named.textContent).toBe("Kitchen Pi (192.0.2.10)"); + expect(named.getAttribute("title")).toBeNull(); expect(screen.queryByText("pi.lan")).toBeNull(); const learned = screen.getByText("laptop.lan"); - expect(learned.getAttribute("title")).toBe("192.0.2.11"); + expect(learned.textContent).toBe("laptop.lan (192.0.2.11)"); expect(within(learned.closest("tr")!).queryByText("learned")).toBeNull(); expect(screen.getByText("192.0.2.12").getAttribute("title")).toBeNull(); diff --git a/admin/src/features/clients/ClientsPage.test.tsx b/admin/src/features/clients/ClientsPage.test.tsx index 7adb288..91d9be5 100644 --- a/admin/src/features/clients/ClientsPage.test.tsx +++ b/admin/src/features/clients/ClientsPage.test.tsx @@ -1,6 +1,6 @@ import { fireEvent, render, screen, waitFor, within } from "@testing-library/react"; import { ClientName, type ClientNames } from "./clientNames"; -import { BASE, MANAGED_FILE, NEVER, renderClientsPage, setConfigStatus } from "./testFixtures"; +import { BASE, CLIENTS, MANAGED_FILE, NEVER, renderClientsPage, setConfigStatus } from "./testFixtures"; /** The text of the elements an input points at with `aria-describedby`. */ function describedText(input: HTMLElement): string { @@ -187,9 +187,15 @@ test("an unknown group id filters to nothing and offers a way out", async () => expect(router.state.location.search).toEqual({}); }); +// Both statuses lock the declared delete, but only file authority proves the +// file declares the row; the anchors keep the two sentences apart. +const DECLARED_NOTE = /^This client is declared in the configuration file/; +const UNKNOWN_NOTE = /^nxdns cannot say whether this client is declared/; test("file mode drops every edit affordance and keeps the observed delete live (R2-4)", async () => { await renderClientsPage({ ...BASE, "GET /api/config/status": MANAGED_FILE }); - await screen.findAllByLabelText(/Managed by \/etc\/nxdns\/config\.zon/); + // The settled sentence, not the tag: "Locked" is already on screen while + // authority is pending, so waiting on it would not wait for this status. + await screen.findAllByText(/^Managed by \/etc\/nxdns\/config\.zon/); expect(screen.queryAllByRole("button", { name: "Edit" })).toEqual([]); @@ -197,6 +203,34 @@ test("file mode drops every edit affordance and keeps the observed delete live ( const observed = clientRow("192.168.1.11"); expect((within(declared).getByRole("button", { name: "Delete" }) as HTMLButtonElement).disabled).toBe(true); expect((within(observed).getByRole("button", { name: "Delete" }) as HTMLButtonElement).disabled).toBe(false); + + // Why the locked Delete will not answer, in visible text and exactly once: + // per row it would repeat down the whole page, and on the button it was a + // title that a keyboard and a touch screen never reached. + expect(screen.getAllByText(DECLARED_NOTE, { selector: "p" })).toHaveLength(1); + expect(within(declared).queryByText(DECLARED_NOTE, { selector: "p" })).toBeNull(); + // The description stays on the button itself too, for a reader on that control. + const locked = within(declared).getByRole("button", { name: "Delete" }); + expect(document.getElementById(locked.getAttribute("aria-describedby") ?? "")?.textContent).toMatch(DECLARED_NOTE); +}); + +test("an all-observed page still says why Edit is gone, with no delete note to carry it", async () => { + // Every row observed, so no Delete is locked. The edit lock is still real, and + // "Locked" appearing with nothing to explain it is the failure this guards. + const observedOnly = { clients: [CLIENTS.clients[1]] }; + await renderClientsPage({ + ...BASE, + "GET /api/clients": observedOnly, + "GET /api/config/status": MANAGED_FILE, + }); + // The settled sentence is both the anchor and the assertion: it is the whole + // explanation for the missing Edit action. + await screen.findAllByText(/^Managed by \/etc\/nxdns\/config\.zon/); + expect(await screen.findAllByText("Locked")).not.toHaveLength(0); + + // The delete sentence belongs only to a row that has one. + expect(screen.queryByText(DECLARED_NOTE)).toBeNull(); + expect((screen.getByRole("button", { name: "Delete" }) as HTMLButtonElement).disabled).toBe(false); }); test("file mode renders network assignments with no mutation control at all (R2-4)", async () => { @@ -217,7 +251,7 @@ test("file mode renders network assignments with no mutation control at all (R2- test("a failed config status exposes no configuration mutation, and still deletes an observed client (R3-4)", async () => { await renderClientsPage({ ...BASE, "GET /api/config/status": undefined }); - await screen.findAllByLabelText(/Configuration status unavailable/); + await screen.findAllByText(/^Configuration status unavailable/); expect(screen.queryAllByRole("button", { name: "Edit" })).toEqual([]); expect(screen.queryByRole("button", { name: "Save assignments" })).toBeNull(); @@ -233,10 +267,6 @@ test("a failed config status exposes no configuration mutation, and still delete // confirmation is already open. `undefined` is the failed status: the fetch stub // answers 404 for a key it does not hold. // -// Both statuses lock the declared delete, but only file authority proves the -// file declares the row; the anchors keep the two sentences apart. -const DECLARED_NOTE = /^This client is declared in the configuration file/; -const UNKNOWN_NOTE = /^nxdns cannot say whether this client is declared/; describe.each([ ["file authority", MANAGED_FILE, DECLARED_NOTE, UNKNOWN_NOTE], ["a failed status", undefined, UNKNOWN_NOTE, DECLARED_NOTE], @@ -259,7 +289,7 @@ describe.each([ expect(within(dialog).queryByRole("button", { name: "Save" })).toBeNull(); expect((within(dialog).getByLabelText("Name") as HTMLInputElement).value).toBe("laptop"); expect(within(dialog).getByText(/can no longer be saved/)).toBeTruthy(); - expect(within(dialog).getByLabelText(/^Locked\./)).toBeTruthy(); + expect(within(dialog).getByText("Locked")).toBeTruthy(); fireEvent.click(within(dialog).getByRole("button", { name: "Cancel" })); await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); @@ -284,7 +314,7 @@ describe.each([ await waitFor(() => expect(within(dialog).queryByRole("button", { name: "Delete" })).toBeNull()); expect(within(dialog).getByText(lockNote)).toBeTruthy(); expect(within(dialog).queryByText(otherNote)).toBeNull(); - expect(within(dialog).getByLabelText(/^Locked\./)).toBeTruthy(); + expect(within(dialog).getByText("Locked")).toBeTruthy(); fireEvent.click(within(dialog).getByRole("button", { name: "Cancel" })); await waitFor(() => expect(screen.queryByRole("alertdialog")).toBeNull()); @@ -364,7 +394,7 @@ test("a pending config status holds the same line as a failed one (R3-4)", async // The status request never settles, so authority stays pending for the whole // test: nothing configuration owns may be offered on that guess. await renderClientsPage({ ...BASE, "GET /api/config/status": NEVER }); - await screen.findAllByLabelText(/Checking which configuration source/); + await screen.findAllByText(/^Checking which configuration source/); expect(screen.queryAllByRole("button", { name: "Edit" })).toEqual([]); expect(screen.queryByRole("button", { name: "Save assignments" })).toBeNull(); diff --git a/admin/src/features/clients/ClientsPage.tsx b/admin/src/features/clients/ClientsPage.tsx index 997d263..7cf39d8 100644 --- a/admin/src/features/clients/ClientsPage.tsx +++ b/admin/src/features/clients/ClientsPage.tsx @@ -10,7 +10,7 @@ import NetworkAssignments from "./NetworkAssignments"; import { ClientDisplayName } from "./clientIdentity"; import InlineError from "@/lib/InlineError"; import ConfirmDialog from "@/ui/ConfirmDialog"; -import ConfigLockIndicator from "@/features/configuration/ConfigLockIndicator"; +import ConfigLockIndicator, { lockReason } from "@/features/configuration/ConfigLockIndicator"; import { useAuthority, useReadOnlyConfig, type Authority } from "@/features/configuration/authority"; import { styles as shared } from "@/ui/styles"; import { colors } from "@/ui/tokens.stylex"; @@ -25,6 +25,9 @@ import { colors } from "@/ui/tokens.stylex"; * alone cannot say which operator surface set the row — so the sentence names * the doubt rather than asserting a declaration, the way `provenanceOf` does. */ +/** The one note every locked Delete on this page describes itself with. */ +const DELETE_LOCK_NOTE_ID = "clients-delete-locked-note"; + function declaredDeleteNote(authority: Authority): string { if (authority.state === "resolved") { return "This client is declared in the configuration file; remove it there and restart."; @@ -52,6 +55,15 @@ const styles = stylex.create({ marginTop: "1rem", color: colors.textMuted, }, + lockNote: { + marginTop: "1rem", + display: "flex", + flexDirection: "column", + gap: "0.25rem", + fontSize: "0.875rem", + lineHeight: "1.25rem", + color: colors.textMuted, + }, filterBar: { marginTop: "1rem", display: "flex", @@ -127,6 +139,11 @@ export default function ClientsPage() { // question: the operator is told why, and only they close the dialog. const deleteLocked = pendingDelete !== null && readOnly && pendingDelete.hand_edited; + // The reason a locked Delete will not answer, printed once above the table. + // Per row it would repeat down the whole page; on the button it was a `title` + // that a keyboard and a touch screen never reached. + const deletesLocked = readOnly && rows.some((client) => client.hand_edited); + return (

Clients

@@ -142,6 +159,15 @@ export default function ClientsPage() { )} + {/* The whole explanation for this page's locks, printed once. Per row it + would repeat down the table; on the controls it was a `title` that a + keyboard and a touch screen never reached. */} + {readOnly && ( +
+

{lockReason(authority)}.

+ {deletesLocked &&

{declaredDeleteNote(authority)}

} +
+ )} {clients.length === 0 ? (

No clients yet. Rows appear automatically as devices on the network make DNS queries — there is @@ -187,7 +213,7 @@ export default function ClientsPage() { {/* Naming a client writes configuration, so the affordance is absent — not disabled — wherever the write cannot land. */} {readOnly ? ( - + ) : (