From c65d92d8f89ad6b0d2ea4786f9b39de74d7bda15 Mon Sep 17 00:00:00 2001 From: m5r Date: Sat, 29 Aug 2026 13:03:30 +0200 Subject: [PATCH] admin: title-only information becomes visible text the client address follows its name as visible muted text in the query tables, the config lock indicator prints its reason beside the tag except in table rows where a page-level note explains the lock instead, and the locked delete buttons describe themselves through that one visible note. the chart legend tooltip is deleted because a named client is deliberately not addressed in the chart, and the dead series address field went with it. titles that merely repeat visible copyable text stay. --- .../features/activity/ActivityPage.test.tsx | 15 +++--- .../features/activity/LiveActivity.test.tsx | 9 ++-- .../src/features/clients/ClientsPage.test.tsx | 50 +++++++++++++++---- admin/src/features/clients/ClientsPage.tsx | 36 ++++++++++--- admin/src/features/clients/clientNames.tsx | 16 ++++-- .../ConfigLockIndicator.test.tsx | 13 +++-- .../configuration/ConfigLockIndicator.tsx | 46 +++++++++++++---- admin/src/features/overview/ClientChart.tsx | 10 +--- .../features/overview/OverviewPage.test.tsx | 10 ++++ 9 files changed, 154 insertions(+), 51 deletions(-) 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 ? ( - + ) : (