diff --git a/admin/src/features/activity/ActivityPage.test.tsx b/admin/src/features/activity/ActivityPage.test.tsx index aa121ac..b03879c 100644 --- a/admin/src/features/activity/ActivityPage.test.tsx +++ b/admin/src/features/activity/ActivityPage.test.tsx @@ -12,6 +12,7 @@ import { createAppRouter } from "@/routes"; import { health } from "@/lib/healthFixture"; import type { Client, Coverage, QueriesPage, QueryRow } from "@/lib/types"; import { queryRow } from "@/features/provenance/provenanceFixture"; +import { FakeEventSource } from "./fakeEventSource"; function client(id: number, ip: string, name: string, learnedName: string): Client { return { @@ -544,6 +545,40 @@ test("the policy simulation is reachable from the header, with no rows to click expect(history.location.pathname).toBe("/activity/test"); }); +test("the mode switch is a tab list whose selection is the url, and it keeps the filters", async () => { + // Live opens a stream as soon as its panel mounts, so the switch cannot be + // exercised without one; the fake stands in for the browser's EventSource. + vi.stubGlobal( + "EventSource", + class { + constructor(url: string) { + return new FakeEventSource(url) as unknown as EventSource; + } + }, + ); + const { history } = renderPage("/activity?mode=history&domain=ads&blocked=true"); + await screen.findByText("ads.example"); + + const tabs = within(screen.getByRole("tablist", { name: "Activity mode" })); + expect(tabs.getAllByRole("tab").map((tab) => tab.textContent)).toEqual(["History", "Live"]); + expect(tabs.getByRole("tab", { name: "History", selected: true })).toBeTruthy(); + expect(tabs.getByRole("tab", { name: "Live", selected: false })).toBeTruthy(); + + fireEvent.click(tabs.getByRole("tab", { name: "Live" })); + + await waitFor(() => expect(history.location.search).toContain("mode=live")); + // The investigation survives the switch: both filters are still in the URL. + expect(history.location.search).toContain("domain=ads"); + expect(history.location.search).toContain("blocked=true"); + expect( + within(screen.getByRole("tablist", { name: "Activity mode" })).getByRole("tab", { + name: "Live", + selected: true, + }), + ).toBeTruthy(); + expect(await screen.findByText(/these filters apply to history only/)).toBeTruthy(); +}); + test("Clear empties the url as well as the form", async () => { const { history } = renderPage("/activity?mode=history&domain=ads&blocked=true"); await screen.findByText("ads.example"); diff --git a/admin/src/features/activity/ActivityPage.tsx b/admin/src/features/activity/ActivityPage.tsx index f4dbd07..5080acc 100644 --- a/admin/src/features/activity/ActivityPage.tsx +++ b/admin/src/features/activity/ActivityPage.tsx @@ -10,6 +10,7 @@ import { Link, useNavigate, useSearch } from "@tanstack/react-router"; import * as stylex from "@stylexjs/stylex"; +import { Tab, TabList, TabPanel, Tabs } from "react-aria-components"; import { styles as shared } from "@/ui/styles"; import { colors } from "@/ui/tokens.stylex"; import ActivityFilters, { NO_FILTERS, type AppliedFilters } from "./ActivityFilters"; @@ -53,6 +54,13 @@ const styles = stylex.create({ fontWeight: 500, cursor: "pointer", }, + /** A Tab is a `div` with a roving tabindex, so RAC drives the ring, not `:focus-visible`. */ + modeFocusVisible: { + outlineWidth: 2, + outlineStyle: "solid", + outlineColor: colors.focus, + outlineOffset: 2, + }, modeIdle: { backgroundColor: { default: "transparent", ":hover": colors.surfaceHover }, color: { default: colors.textSecondary, ":hover": colors.text }, @@ -76,8 +84,30 @@ const styles = stylex.create({ lineHeight: "1.25rem", color: colors.textMuted, }, + panel: { + outlineStyle: "none", + }, + /** + * A panel with nothing tabbable in it — an empty or loading history — is given + * a tabindex by RAC so the reader can still reach its content, so it has to be + * able to show that it holds the focus. + */ + panelFocusVisible: { + outlineWidth: 2, + outlineStyle: "solid", + outlineColor: colors.focus, + outlineOffset: 2, + }, + /** Explicit, so RAC's default `react-aria-Tabs` class does not land instead. */ + tabsRoot: { + display: "block", + }, }); +function panelClass({ isFocusVisible }: { isFocusVisible: boolean }): string { + return stylex.props(styles.panel, isFocusVisible && styles.panelFocusVisible).className ?? ""; +} + export default function ActivityPage() { const search = useSearch({ from: "/shell/activity" }); const navigate = useNavigate({ from: "/activity" }); @@ -97,56 +127,66 @@ export default function ActivityPage() { return (
-
-

Activity

-
- {MODES.map((option) => { - const selected = option.mode === search.mode; - return ( - - ); - })} + + ))} + + + Current policy simulation +
- - Current policy simulation - -
- {/* - * Remounted whenever the applied search changes, which is what makes - * the back button work: the draft is derived state, and the browser - * moving the URL under it has to move the form with it. - */} - apply(NO_FILTERS)} - /> + {/* + * Remounted whenever the applied search changes, which is what makes + * the back button work: the draft is derived state, and the browser + * moving the URL under it has to move the form with it. + */} + apply(NO_FILTERS)} + /> - {live ? ( - <> + + + +

The stream carries every query the server answers; these filters apply to history only.

- - ) : ( - - )} + +
); } diff --git a/admin/src/features/activity/LiveActivity.test.tsx b/admin/src/features/activity/LiveActivity.test.tsx index a72fd7b..e9b8ae1 100644 --- a/admin/src/features/activity/LiveActivity.test.tsx +++ b/admin/src/features/activity/LiveActivity.test.tsx @@ -57,7 +57,17 @@ function json(payload: unknown): Response { return new Response(JSON.stringify(payload), { status: 200, headers: { "content-type": "application/json" } }); } -function stubFetch(handler: (url: string) => Response | Promise = () => json({})) { +/** + * The empty history page. Live mode asks for no query pages, but switching back + * to History does, and a page-shaped response is the only honest answer there. + */ +const EMPTY_PAGE = { queries: [], next_before: null, coverage: { complete: true, available_since: 0 } }; + +function defaultHandler(url: string): Response { + return json(url === "/api/queries" || url.startsWith("/api/queries?") ? EMPTY_PAGE : {}); +} + +function stubFetch(handler: (url: string) => Response | Promise = defaultHandler) { fetchMock = vi.fn((input: RequestInfo | URL) => { const url = String(input); if (url === "/api/version") return Promise.resolve(json(VERSION)); @@ -256,7 +266,40 @@ test("a recovered row links to its stored detail; a streamed one opens in place expect(screen.getByRole("button", { name: "streamed.example" })).toBeTruthy(); }); -test("a streamed row opens its own provenance, from the keyboard as well as the pointer", async () => { +/** The open detail. Named by its heading, so the query proves the name is visible. */ +function detailDialog(): HTMLElement { + return screen.getByRole("dialog", { name: "Streamed query" }); +} + +/** React Aria's ModalOverlay, two levels out from the dialog it wraps. */ +function backdrop(): HTMLElement { + return detailDialog().parentElement!.parentElement!; +} + +/** + * Activate a control the way a keyboard does. jsdom runs no default action for + * Enter on a button, so the click a browser would then dispatch is issued here; + * `detail: 0` is what marks it as keyboard-driven rather than pointer-driven, + * and is the flag React Aria itself reads. + */ +function pressWithKeyboard(control: HTMLElement) { + act(() => control.focus()); + fireEvent.keyDown(control, { key: "Enter" }); + fireEvent.click(control, { detail: 0 }); + fireEvent.keyUp(control, { key: "Enter" }); +} + +/** Activate a control the way a mouse does, through the full pointer sequence. */ +function pressWithMouse(control: HTMLElement) { + fireEvent.pointerDown(control, { pointerType: "mouse", button: 0 }); + fireEvent.pointerUp(control, { pointerType: "mouse", button: 0 }); + fireEvent.click(control, { detail: 1 }); +} + +test.each([ + ["the pointer", pressWithMouse], + ["the keyboard", pressWithKeyboard], +])("a streamed row opens its provenance in a named dialog, from %s", async (_label, press) => { await openLive(); act(() => sources[0]!.emit( @@ -273,69 +316,108 @@ test("a streamed row opens its own provenance, from the keyboard as well as the // only way to lose that is to opt out of it, which nothing here may do. expect(trigger.tagName).toBe("BUTTON"); expect(trigger.getAttribute("tabindex")).toBeNull(); - act(() => trigger.focus()); - expect(document.activeElement).toBe(trigger); + // The row opens a dialog, so it says so; what it no longer claims is to + // expand a region that stays in the page. + expect(trigger.getAttribute("aria-haspopup")).toBe("dialog"); + expect(trigger.getAttribute("aria-expanded")).toBeNull(); + expect(trigger.getAttribute("aria-controls")).toBeNull(); - fireEvent.click(trigger); - const heading = screen.getByRole("heading", { level: 1, name: "streamed.example" }); - expect(heading).toBeTruthy(); - const panel = heading.closest("div")!.parentElement!; - expect(within(panel).getByText("Blocked locally")).toBeTruthy(); - expect(panel.textContent).toContain("the query log may not have written it yet"); + press(trigger); - fireEvent.click(screen.getByRole("button", { name: "Close" })); - expect(screen.queryByRole("heading", { level: 1, name: "streamed.example" })).toBeNull(); + const dialog = detailDialog(); + expect(within(dialog).getByRole("heading", { level: 1, name: "streamed.example" })).toBeTruthy(); + expect(within(dialog).getByText("Blocked locally")).toBeTruthy(); + expect(dialog.textContent).toContain("the query log may not have written it yet"); + // React Aria may defer the move by a frame, depending on the modality it read + // from the activation, so the wait is the assertion rather than a workaround. + await waitFor(() => expect(dialog.contains(document.activeElement)).toBe(true)); + expect(within(dialog).getByRole("button", { name: "Close" })).toBeTruthy(); }); -test("the detail takes focus when a row opens it and hands it back when it closes", async () => { +test("tabbing forward and backward stays inside the open dialog", async () => { await openLive(); act(() => sources[0]!.emit("query", frame(1000, "streamed.example"))); + fireEvent.click(screen.getByRole("button", { name: "streamed.example" })); - const trigger = screen.getByRole("button", { name: "streamed.example" }); - expect(trigger.getAttribute("aria-expanded")).toBe("false"); - expect(trigger.getAttribute("aria-controls")).toBeNull(); + const dialog = detailDialog(); + await waitFor(() => expect(dialog.contains(document.activeElement)).toBe(true)); + // The related links land asynchronously; tabbing before them would walk a + // shorter dialog than the reader ever sees. + await waitFor(() => expect(within(dialog).getAllByRole("link").length).toBeGreaterThan(1)); - // A native button activates on Enter and Space; jsdom does not synthesize - // the click those keys fire, so the click is the activation. - act(() => trigger.focus()); - fireEvent.click(trigger); - - const panel = screen.getByRole("group", { name: "Streamed query" }); - expect(trigger.getAttribute("aria-expanded")).toBe("true"); - expect(trigger.getAttribute("aria-controls")).toBe(panel.id); - // The panel is inserted above the table, behind the trigger in tab order, so - // the only thing that keeps a forward tab inside it is focus moving in. - expect(panel.compareDocumentPosition(trigger) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); - expect(document.activeElement).toBe(panel); - expect(panel.contains(screen.getByRole("button", { name: "Close" }))).toBe(true); - - fireEvent.click(screen.getByRole("button", { name: "Close" })); - expect(screen.queryByRole("group", { name: "Streamed query" })).toBeNull(); - expect(document.activeElement).toBe(trigger); - expect(trigger.getAttribute("aria-expanded")).toBe("false"); - expect(trigger.getAttribute("aria-controls")).toBeNull(); + const visited = new Set(); + for (const shiftKey of [false, false, false, false, false, false, true, true, true, true]) { + fireEvent.keyDown(document.activeElement!, { key: "Tab", shiftKey }); + fireEvent.keyUp(document.activeElement!, { key: "Tab", shiftKey }); + expect(dialog.contains(document.activeElement)).toBe(true); + visited.add(document.activeElement!); + } + // Containment that never moved focus would satisfy the check above without + // trapping anything, so the walk has to have actually walked. + expect(visited.size).toBeGreaterThan(1); }); -test("opening a second row moves the expanded state and the focus with it", async () => { +test.each([ + ["the Close button", () => fireEvent.click(within(detailDialog()).getByRole("button", { name: "Close" }))], + ["Escape", () => fireEvent.keyDown(detailDialog(), { key: "Escape" })], + [ + "a click on the backdrop", + () => { + const overlay = backdrop(); + fireEvent.pointerDown(overlay, { pointerType: "mouse", button: 0 }); + fireEvent.pointerUp(overlay, { pointerType: "mouse", button: 0 }); + fireEvent.click(overlay, { detail: 1 }); + }, + ], +])("%s closes the dialog and returns focus to the row that opened it", async (_label, dismiss) => { await openLive(); - act(() => { - sources[0]!.emit("query", frame(1000, "first.example")); - sources[0]!.emit("query", frame(1001, "second.example")); - }); + act(() => sources[0]!.emit("query", frame(1000, "streamed.example"))); + const trigger = screen.getByRole("button", { name: "streamed.example" }); + act(() => trigger.focus()); + fireEvent.click(trigger); + expect(detailDialog()).toBeTruthy(); - const first = screen.getByRole("button", { name: "first.example" }); - const second = screen.getByRole("button", { name: "second.example" }); - fireEvent.click(first); - fireEvent.click(second); + dismiss(); - const panel = screen.getByRole("group", { name: "Streamed query" }); - expect(within(panel).getByRole("heading", { level: 1, name: "second.example" })).toBeTruthy(); - expect(document.activeElement).toBe(panel); - expect(first.getAttribute("aria-expanded")).toBe("false"); - expect(second.getAttribute("aria-expanded")).toBe("true"); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + expect(document.activeElement).toBe(screen.getByRole("button", { name: "streamed.example" })); +}); + +test("the stream runs on behind the open dialog, and its rows land in the table on close", async () => { + await openLive(); + act(() => sources[0]!.emit("query", frame(1000, "streamed.example"))); + fireEvent.click(screen.getByRole("button", { name: "streamed.example" })); + + const snapshot = detailDialog().textContent; + act(() => sources[0]!.emit("query", frame(1001, "arrived-while-open.example"))); + + // The connection is untouched: no close, no second EventSource. + expect(sources).toHaveLength(1); + expect(sources[0]!.closed).toBe(false); + // And the snapshot is a snapshot: nothing that arrives rewrites it. + expect(detailDialog().textContent).toBe(snapshot); + + fireEvent.click(within(detailDialog()).getByRole("button", { name: "Close" })); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + expect(screen.getByRole("button", { name: "arrived-while-open.example" })).toBeTruthy(); + expect(screen.getByRole("button", { name: "streamed.example" })).toBeTruthy(); +}); + +test("the rows and toolbar behind the dialog are out of reach while it is open", async () => { + await openLive(); + act(() => sources[0]!.emit("query", frame(1000, "streamed.example"))); + fireEvent.click(screen.getByRole("button", { name: "streamed.example" })); + + // React Aria hides everything outside the modal from assistive technology + // and from the pointer alike, so the row and the toolbar are unreachable by + // role: nothing behind the dialog can be operated while it is open. + expect(screen.queryByRole("button", { name: "streamed.example" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Freeze" })).toBeNull(); + expect(screen.getByRole("button", { name: "Close" })).toBeTruthy(); fireEvent.click(screen.getByRole("button", { name: "Close" })); - expect(document.activeElement).toBe(second); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + expect(screen.getByRole("button", { name: "Freeze" })).toBeTruthy(); }); /** @@ -359,23 +441,57 @@ function renderLiveWithCapacity(capacity: number) { ); } -test("an open streamed detail survives the row being evicted from the ring buffer", () => { - renderLiveWithCapacity(5); - act(() => sources[0]!.emit("open")); - act(() => sources[0]!.emit("query", frame(1000, "evicted.example"))); - fireEvent.click(screen.getByRole("button", { name: "evicted.example" })); - expect(screen.getByRole("heading", { level: 1, name: "evicted.example" })).toBeTruthy(); - - // One ringful more: the ring keeps the newest 5, so the selected row is gone - // from the table. The detail is a snapshot, not a lookup into the ring. +/** Push one ringful of filler through a 5-row ring, evicting whatever was there. */ +function evictWithFiller() { act(() => { for (let index = 0; index < 5; index += 1) { sources[0]!.emit("query", frame(2000 + index, `filler${index}.example`)); } }); +} + +test("an open dialog's snapshot survives its row being evicted from the ring buffer", async () => { + renderLiveWithCapacity(5); + act(() => sources[0]!.emit("open")); + act(() => sources[0]!.emit("query", frame(1000, "evicted.example"))); + fireEvent.click(screen.getByRole("button", { name: "evicted.example" })); + const snapshot = detailDialog().textContent; + expect(within(detailDialog()).getByRole("heading", { level: 1, name: "evicted.example" })).toBeTruthy(); + + // One ringful more: the ring keeps the newest 5, so the selected row is gone. + // The dialog holds the frame itself, not a lookup into the ring, so it neither + // blanks out nor closes. + evictWithFiller(); + expect(detailDialog().textContent).toBe(snapshot); + + fireEvent.click(within(detailDialog()).getByRole("button", { name: "Close" })); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); expect(screen.getAllByRole("row")).toHaveLength(6); expect(screen.queryByRole("button", { name: "evicted.example" })).toBeNull(); - expect(screen.getByRole("heading", { level: 1, name: "evicted.example" })).toBeTruthy(); +}); + +test("closing after the source row is evicted anchors focus in the results region", async () => { + renderLiveWithCapacity(5); + act(() => sources[0]!.emit("open")); + act(() => sources[0]!.emit("query", frame(1000, "evicted.example"))); + const trigger = screen.getByRole("button", { name: "evicted.example" }); + act(() => trigger.focus()); + fireEvent.click(trigger); + + evictWithFiller(); + expect(trigger.isConnected).toBe(false); + + fireEvent.click(within(detailDialog()).getByRole("button", { name: "Close" })); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + + // Never the body, never whichever row happens to sit where the old one did, + // and never the Freeze button: a stable anchor in the region the reader was + // reading. React Aria's own deferred restore runs after this and, finding + // focus already placed, leaves it alone. + const region = screen.getByRole("region", { name: "Live queries" }); + expect(document.activeElement).toBe(region); + await act(() => new Promise((resolve) => requestAnimationFrame(() => resolve(undefined)))); + expect(document.activeElement).toBe(region); }); test("the route renders the live ring at its production capacity", async () => { @@ -391,11 +507,15 @@ test("an open streamed detail survives Freeze and Resume", async () => { await openLive(); act(() => sources[0]!.emit("query", frame(1000, "held.example"))); fireEvent.click(screen.getByRole("button", { name: "held.example" })); + const snapshot = detailDialog().textContent; - fireEvent.click(screen.getByRole("button", { name: "Freeze" })); - expect(screen.getByRole("heading", { level: 1, name: "held.example" })).toBeTruthy(); - fireEvent.click(screen.getByRole("button", { name: "Resume" })); - expect(screen.getByRole("heading", { level: 1, name: "held.example" })).toBeTruthy(); + // Freeze is behind the dialog, so it is reached the way the code reaches it + // rather than by role, which the modal deliberately hides. + const freeze = () => screen.getByText("Freeze") as HTMLButtonElement; + act(() => freeze().click()); + expect(detailDialog().textContent).toBe(snapshot); + act(() => (screen.getByText("Resume") as HTMLButtonElement).click()); + expect(detailDialog().textContent).toBe(snapshot); }); test("the filter row stays visible, keeps its values, and is out of the tab order", async () => { @@ -431,7 +551,7 @@ test("live mode asks for no query pages, whatever filters the url retained", asy test("leaving live closes the stream, and coming back opens exactly one fresh one", async () => { await openLive("/activity?mode=live&domain=ads"); - fireEvent.click(screen.getByRole("button", { name: "History" })); + fireEvent.click(screen.getByRole("tab", { name: "History" })); await screen.findByRole("button", { name: "Apply filters" }); expect(sources).toHaveLength(1); expect(sources[0]!.closed).toBe(true); @@ -440,7 +560,7 @@ test("leaving live closes the stream, and coming back opens exactly one fresh on expect((screen.getByLabelText("Domain contains") as HTMLInputElement).value).toBe("ads"); expect((screen.getByLabelText("Domain contains") as HTMLInputElement).disabled).toBe(false); - fireEvent.click(screen.getByRole("button", { name: "Live" })); + fireEvent.click(screen.getByRole("tab", { name: "Live" })); await screen.findByRole("button", { name: "Freeze" }); expect(sources).toHaveLength(2); expect(sources[1]!.closed).toBe(false); diff --git a/admin/src/features/activity/LiveActivity.tsx b/admin/src/features/activity/LiveActivity.tsx index 7e505fc..b808955 100644 --- a/admin/src/features/activity/LiveActivity.tsx +++ b/admin/src/features/activity/LiveActivity.tsx @@ -1,6 +1,6 @@ /** - * Activity in live mode: the SSE stream, its bounded ring buffer, and the - * in-place detail a streamed row opens. + * Activity in live mode: the SSE stream, its bounded ring buffer, and the modal + * detail a streamed row opens. * * This subtree is mounted only while the URL says `mode=live`, which is what * closes the EventSource on the way back to history: the connection is a @@ -17,6 +17,7 @@ import { Link } from "@tanstack/react-router"; import * as stylex from "@stylexjs/stylex"; import { useClientNames } from "@/features/clients/clientNames"; import { summarizeEvent } from "@/features/provenance/querySummary"; +import Dialog from "@/ui/Dialog"; import { styles as shared } from "@/ui/styles"; import { colors } from "@/ui/tokens.stylex"; import { ActivityCells, ActivityTableHead, activityDomainLink } from "./cells"; @@ -162,29 +163,6 @@ const styles = stylex.create({ textDecorationLine: "underline", textDecorationStyle: "dotted", }, - detailPanel: { - marginTop: "1rem", - borderRadius: "0.25rem", - borderWidth: 1, - borderStyle: "solid", - borderColor: colors.borderStrong, - backgroundColor: colors.surface, - padding: "1rem", - }, - detailBar: { - display: "flex", - alignItems: "baseline", - justifyContent: "space-between", - gap: "0.75rem", - }, - detailLabel: { - fontSize: "0.75rem", - lineHeight: "1rem", - fontWeight: 600, - letterSpacing: "0.05em", - textTransform: "uppercase", - color: colors.textMuted, - }, footnote: { marginTop: "0.75rem", fontSize: "0.875rem", @@ -193,10 +171,6 @@ const styles = stylex.create({ }, }); -/** One panel at a time, so the trigger that opened it can name it in `aria-controls`. */ -const DETAIL_PANEL_ID = "live-query-detail"; -const DETAIL_LABEL_ID = "live-query-detail-label"; - const PILL_LABELS: Record = { connecting: "Connecting…", open: "Live", @@ -226,40 +200,15 @@ function StatusPill({ status }: { status: StreamStatus }) { * * The buffer is a 500-row ring that a gap merge also rewrites: a reference by * key would go stale under the reader while they were still reading it, and the - * panel would blank out for no reason they could see. The snapshot is the whole - * fact — a streamed frame carries its own provenance — so it survives eviction, - * a merge and a Freeze/Resume, and closes only when the reader closes it or - * leaves live mode. + * dialog would blank out or swap under their eyes for no reason they could see. + * The snapshot is the whole fact — a streamed frame carries its own provenance + * — so it survives eviction, a merge and a Freeze/Resume, and closes only when + * the reader closes it or leaves live mode. The stream behind it never stops. */ function LiveDetail({ row, origin, onClose }: { row: StreamedRow; origin: ActivitySearch; onClose: () => void }) { const summary = summarizeEvent(row.event); - const panel = useRef(null); - - // The panel opens above the table, behind the trigger in tab order, so a - // forward tab from the row would walk past it. Focus moves in on open — - // keyed on the row, so choosing a second row moves it again — and the - // closer puts it back on the trigger. - useEffect(() => { - panel.current?.focus(); - }, [row.key]); - return ( -
-
- - Streamed query - - -
+ + origin={origin} + /> } /> -
+ ); } @@ -291,21 +241,36 @@ export default function LiveActivity({ const clientNames = useClientNames(); const [selected, setSelected] = useState(null); const trigger = useRef(null); + const results = useRef(null); + const restoring = useRef(false); + + /** + * Where focus lands when the dialog closes. + * + * React Aria restores focus itself, but in a `requestAnimationFrame` and + * only while focus is still on the body — and its target is the row button, + * which the ring may have evicted while the reader was reading. So this runs + * in the effect that follows the focus scope's teardown and puts focus on a + * connected element first: the row if it is still there, the results region + * if it is not. React Aria's deferred pass then finds focus already placed + * and does nothing, so the two never fight over it. If a navigation unmounts + * this component the effect never runs, which is the right answer — there is + * no longer a table to return to. + */ + useEffect(() => { + if (selected !== null || !restoring.current) return; + restoring.current = false; + const from = trigger.current; + trigger.current = null; + (from?.isConnected === true ? from : results.current)?.focus(); + }, [selected]); function open(row: StreamedRow, from: HTMLButtonElement) { trigger.current = from; + restoring.current = true; setSelected(row); } - // The row that opened the panel takes focus back, unless the ring has - // already evicted it: a detached button cannot be focused, and the browser - // falls back to the document, which is the best available answer. - function close() { - setSelected(null); - trigger.current?.focus(); - trigger.current = null; - } - return ( <>
@@ -361,71 +326,81 @@ export default function LiveActivity({
)} - {selected !== null && } + {/* + * The region is the anchor focus falls back to when the row that + * opened the dialog is gone, so it is rendered unconditionally: an + * anchor that disappears with the last row is no anchor at all. + */} +
+ {live.rows.length === 0 ? ( + live.status !== "capped" && ( +

+ {live.status === "open" ? "Waiting for queries…" : "No queries received yet."} +

+ ) + ) : ( + <> +
+ + + + {live.rows.map((row) => { + const summary = summaryOf(row); + return ( + + + row.kind === "streamed" ? ( + + ) : ( + + {children} + + ) + } + /> + + ); + })} + +
+
+

+ Showing {live.rows.length} {live.rows.length === 1 ? "query" : "queries"} (newest first, + last {capacity} kept). +

+ + )} +
- {live.rows.length === 0 ? ( - live.status !== "capped" && ( -

- {live.status === "open" ? "Waiting for queries…" : "No queries received yet."} -

- ) - ) : ( - <> -
- - - - {live.rows.map((row) => { - const summary = summaryOf(row); - return ( - - - row.kind === "streamed" ? ( - - ) : ( - - {children} - - ) - } - /> - - ); - })} - -
-
-

- Showing {live.rows.length} {live.rows.length === 1 ? "query" : "queries"} (newest first, last{" "} - {capacity} kept). -

- - )} + {selected !== null && setSelected(null)} />} ); } diff --git a/admin/src/features/clients/ClientEditDialog.tsx b/admin/src/features/clients/ClientEditDialog.tsx index f60987d..5f31e78 100644 --- a/admin/src/features/clients/ClientEditDialog.tsx +++ b/admin/src/features/clients/ClientEditDialog.tsx @@ -18,16 +18,10 @@ interface Props { } const styles = stylex.create({ - heading: { - fontSize: "1.125rem", - lineHeight: "1.75rem", - fontWeight: 600, - }, form: { display: "flex", flexDirection: "column", gap: "1rem", - marginTop: "1rem", }, fieldLabel: { display: "block", @@ -67,8 +61,7 @@ export default function ClientEditDialog({ client, groups, onClose }: Props) { const readOnly = useReadOnlyConfig(); return ( - -

Edit {client.ip}

+
{ diff --git a/admin/src/features/clients/ClientsPage.test.tsx b/admin/src/features/clients/ClientsPage.test.tsx index 2f40897..56accd1 100644 --- a/admin/src/features/clients/ClientsPage.test.tsx +++ b/admin/src/features/clients/ClientsPage.test.tsx @@ -73,6 +73,39 @@ test("the address links to the client's detail page", async () => { expect(link.getAttribute("href")).toBe("/clients/1"); }); +test("delete asks first, naming the row, and the confirmation carries out the delete", async () => { + const { fetchMock } = await renderClientsPage({ ...BASE, "DELETE /api/clients/2": {} }); + + fireEvent.click(within(clientRow("192.168.1.11")).getByRole("button", { name: "Delete" })); + + const dialog = await screen.findByRole("alertdialog", { name: "Delete client" }); + // The operator has to be able to tell from the dialog alone which row this is. + expect(within(dialog).getByText(/kids-tablet\.lan \(192\.168\.1\.11\)/)).toBeTruthy(); + expect(within(dialog).getByText(/re-materialize on their next DNS query/)).toBeTruthy(); + // Asking is not deleting. + expect(fetchMock.mock.calls.filter(([, init]) => init?.method === "DELETE")).toEqual([]); + + fireEvent.click(within(dialog).getByRole("button", { name: "Delete" })); + await waitFor(() => + expect( + fetchMock.mock.calls.filter(([input, init]) => init?.method === "DELETE" && String(input).endsWith("/2")), + ).toHaveLength(1), + ); + await waitFor(() => expect(screen.queryByRole("alertdialog")).toBeNull()); +}); + +test("cancelling the confirmation keeps the client", async () => { + const { fetchMock } = await renderClientsPage({ ...BASE, "DELETE /api/clients/2": {} }); + + fireEvent.click(within(clientRow("192.168.1.11")).getByRole("button", { name: "Delete" })); + const dialog = await screen.findByRole("alertdialog", { name: "Delete client" }); + fireEvent.click(within(dialog).getByRole("button", { name: "Cancel" })); + + await waitFor(() => expect(screen.queryByRole("alertdialog")).toBeNull()); + expect(screen.getByText("192.168.1.11")).toBeTruthy(); + expect(fetchMock.mock.calls.filter(([, init]) => init?.method === "DELETE")).toEqual([]); +}); + test("shows the DNS-activity empty state when there are no clients", async () => { await renderClientsPage({ ...BASE, "GET /api/clients": { clients: [] } }); @@ -218,7 +251,7 @@ describe.each([ await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); }); - test("locks an open declared-client delete confirmation and leaves cancel working", async () => { + test("locks an open declared-client delete confirmation in place and leaves cancel working", async () => { const map = { ...BASE }; const { queryClient, fetchMock } = await renderClientsPage(map); // The Edit affordance appearing is the proof authority resolved to @@ -227,23 +260,66 @@ describe.each([ const declared = clientRow("192.168.1.10"); fireEvent.click(within(declared).getByRole("button", { name: "Delete" })); - expect(within(clientRow("192.168.1.10")).getByRole("button", { name: "Confirm delete" })).toBeTruthy(); + const dialog = await screen.findByRole("alertdialog", { name: "Delete client" }); await setConfigStatus(map, queryClient, status); - const confirming = clientRow("192.168.1.10"); - expect(within(confirming).queryByRole("button", { name: "Confirm delete" })).toBeNull(); - expect(within(confirming).getByText(lockNote)).toBeTruthy(); - expect(within(confirming).queryByText(otherNote)).toBeNull(); - expect(within(confirming).getByLabelText(/^Locked\./)).toBeTruthy(); + // The dialog stays put. Closing it would throw focus at the Delete button + // the same turn disabled, and the reason would survive only as a title + // attribute; here the reason is the dialog's own message. + 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(); - fireEvent.click(within(confirming).getByRole("button", { name: "Cancel" })); - await waitFor(() => - expect(within(clientRow("192.168.1.10")).getByRole("button", { name: "Delete" })).toBeTruthy(), - ); + fireEvent.click(within(dialog).getByRole("button", { name: "Cancel" })); + await waitFor(() => expect(screen.queryByRole("alertdialog")).toBeNull()); expect(fetchMock.mock.calls.filter(([, init]) => init?.method === "DELETE")).toEqual([]); }); + test("a cancelled confirmation stays closed when authority comes back", async () => { + const map = { ...BASE }; + const { queryClient } = await renderClientsPage(map); + await unlockedEdit(0); + + fireEvent.click(within(clientRow("192.168.1.10")).getByRole("button", { name: "Delete" })); + const dialog = await screen.findByRole("alertdialog", { name: "Delete client" }); + fireEvent.click(within(dialog).getByRole("button", { name: "Cancel" })); + await waitFor(() => expect(screen.queryByRole("alertdialog")).toBeNull()); + + // A poll that fails and then recovers must not raise a destructive + // question the operator already answered. + await setConfigStatus(map, queryClient, status); + await setConfigStatus(map, queryClient, BASE["GET /api/config/status"]); + + await waitFor(() => expect(screen.getAllByRole("button", { name: "Edit" }).length).toBeGreaterThan(0)); + expect(screen.queryByRole("alertdialog")).toBeNull(); + }); + + test("the confirm action comes back when authority does, without a second prompt", async () => { + const map = { ...BASE, "DELETE /api/clients/1": {} }; + const { queryClient, fetchMock } = await renderClientsPage(map); + await unlockedEdit(0); + + fireEvent.click(within(clientRow("192.168.1.10")).getByRole("button", { name: "Delete" })); + const dialog = await screen.findByRole("alertdialog", { name: "Delete client" }); + + await setConfigStatus(map, queryClient, status); + await waitFor(() => expect(within(dialog).queryByRole("button", { name: "Delete" })).toBeNull()); + + await setConfigStatus(map, queryClient, BASE["GET /api/config/status"]); + + // The question was never withdrawn, so the answer returns to the same + // dialog rather than asking the operator to start again. + const confirm = await within(dialog).findByRole("button", { name: "Delete" }); + fireEvent.click(confirm); + await waitFor(() => + expect( + fetchMock.mock.calls.filter(([input, init]) => init?.method === "DELETE" && String(input).endsWith("/1")), + ).toHaveLength(1), + ); + }); + test("keeps an open observed-client delete confirmation live (R3-4)", async () => { const map = { ...BASE, "DELETE /api/clients/2": {} }; const { queryClient, fetchMock } = await renderClientsPage(map); @@ -253,10 +329,11 @@ describe.each([ const observed = clientRow("192.168.1.11"); fireEvent.click(within(observed).getByRole("button", { name: "Delete" })); + const dialog = await screen.findByRole("alertdialog", { name: "Delete client" }); await setConfigStatus(map, queryClient, status); - fireEvent.click(within(clientRow("192.168.1.11")).getByRole("button", { name: "Confirm delete" })); + fireEvent.click(within(dialog).getByRole("button", { name: "Delete" })); await waitFor(() => expect( fetchMock.mock.calls.filter( diff --git a/admin/src/features/clients/ClientsPage.tsx b/admin/src/features/clients/ClientsPage.tsx index 674712e..997d263 100644 --- a/admin/src/features/clients/ClientsPage.tsx +++ b/admin/src/features/clients/ClientsPage.tsx @@ -9,6 +9,7 @@ import ClientEditDialog from "./ClientEditDialog"; 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 { useAuthority, useReadOnlyConfig, type Authority } from "@/features/configuration/authority"; import { styles as shared } from "@/ui/styles"; @@ -31,6 +32,16 @@ function declaredDeleteNote(authority: Authority): string { return "nxdns cannot say whether this client is declared in the configuration file until it reports its configuration status, so deleting it stays locked."; } +/** + * How the confirmation names the row. The address is always there and always + * unique, so it carries the sentence; a name the operator recognizes leads when + * the row has one. + */ +function clientLabel(client: Client): string { + const name = client.name !== "" ? client.name : client.learned_name; + return name === "" ? client.ip : `${name} (${client.ip})`; +} + const styles = stylex.create({ heading: { fontSize: "1.5rem", @@ -79,23 +90,11 @@ const styles = stylex.create({ color: colors.primaryOnSurface, textDecorationLine: "none", }, - confirmGroup: { - display: "inline-flex", - flexWrap: "wrap", - alignItems: "center", - justifyContent: "flex-end", - gap: "0.5rem", - }, actionGroup: { display: "inline-flex", alignItems: "center", gap: "0.5rem", }, - note: { - fontSize: "0.75rem", - lineHeight: "1rem", - color: colors.textMuted, - }, dangerText: { color: colors.danger, }, @@ -112,7 +111,7 @@ export default function ClientsPage() { const queryClient = useQueryClient(); const deleteMutation = useMutation(clientDeleteMutation(queryClient)); const [editing, setEditing] = useState(null); - const [confirmingId, setConfirmingId] = useState(null); + const [pendingDelete, setPendingDelete] = useState(null); const readOnly = useReadOnlyConfig(); const authority = useAuthority(); @@ -122,6 +121,12 @@ export default function ClientsPage() { const filterGroup: Group | undefined = group === undefined ? undefined : groups.find((row) => row.id === group); const rows = group === undefined ? clients : clients.filter((client) => client.group_id === group); + // Authority is polled, so it can turn while the confirmation is open. The + // dialog reads it on every render rather than trusting the state that opened + // it, and withdraws the answer that would now fail instead of withdrawing the + // question: the operator is told why, and only they close the dialog. + const deleteLocked = pendingDelete !== null && readOnly && pendingDelete.hand_edited; + return (

Clients

@@ -178,77 +183,39 @@ export default function ClientsPage() { {formatTime(client.first_seen)} {formatTime(client.last_seen)} - {confirmingId === client.id ? ( - - {/* Authority is polled, so it can turn while a confirmation - sits open. The confirm path reads it on every render - rather than trusting the state that opened it. */} - - {readOnly && client.hand_edited - ? declaredDeleteNote(authority) - : "Deleted clients re-materialize on their next DNS query."} - - {readOnly && client.hand_edited ? ( - - ) : ( - - )} + + {/* Naming a client writes configuration, so the affordance is + absent — not disabled — wherever the write cannot land. */} + {readOnly ? ( + + ) : ( - - ) : ( - - {/* Naming a client writes configuration, so the affordance is - absent — not disabled — wherever the write cannot land. */} - {readOnly ? ( - - ) : ( - + )} + - - )} + > + Delete + + ))} @@ -256,6 +223,24 @@ export default function ClientsPage() { )} + : undefined} + onConfirm={() => { + if (pendingDelete !== null) deleteMutation.mutate(pendingDelete.id); + setPendingDelete(null); + }} + onCancel={() => setPendingDelete(null)} + /> {editing !== null && setEditing(null)} />} diff --git a/admin/src/features/overview/OverviewFrame.tsx b/admin/src/features/overview/OverviewFrame.tsx index ea8d227..fffe552 100644 --- a/admin/src/features/overview/OverviewFrame.tsx +++ b/admin/src/features/overview/OverviewFrame.tsx @@ -10,6 +10,7 @@ import * as stylex from "@stylexjs/stylex"; import { useNavigate, useSearch } from "@tanstack/react-router"; +import { Radio, RadioGroup } from "react-aria-components"; import type { Period } from "@/lib/types"; import { styles as shared } from "@/ui/styles"; import { colors } from "@/ui/tokens.stylex"; @@ -38,7 +39,7 @@ const styles = stylex.create({ gap: "0.25rem", }, period: { - cursor: { default: "pointer", ":disabled": "not-allowed" }, + cursor: "pointer", borderStyle: "none", borderRadius: "0.25rem", paddingInline: "0.625rem", @@ -46,6 +47,13 @@ const styles = stylex.create({ fontSize: "0.875rem", lineHeight: "1.25rem", }, + /** A Radio is a `label`, so RAC drives the ring rather than `:focus-visible`. */ + periodFocusVisible: { + outlineWidth: 2, + outlineStyle: "solid", + outlineColor: colors.focus, + outlineOffset: 2, + }, /** The pressed fill is heavier than `surfaceHover`, so a hover cannot mimic it. */ periodSelected: { backgroundColor: { @@ -68,23 +76,29 @@ const styles = stylex.create({ export function PeriodPicker({ period, onChange }: { period: Period; onChange: (period: Period) => void }) { return ( -
+ onChange(next as Period)} + className={() => stylex.props(styles.periodGroup).className ?? ""} + > {PERIODS.map((option) => ( - + ))} -
+ ); } diff --git a/admin/src/features/overview/OverviewPage.test.tsx b/admin/src/features/overview/OverviewPage.test.tsx index 7ac2d65..516bc9e 100644 --- a/admin/src/features/overview/OverviewPage.test.tsx +++ b/admin/src/features/overview/OverviewPage.test.tsx @@ -189,7 +189,7 @@ test("a request in flight leaves the heading and the picker usable behind one lo renderApp(); await screen.findByRole("heading", { name: "Overview", level: 1 }); - expect(screen.getByRole("button", { name: "1h" })).toBeTruthy(); + expect(screen.getByRole("radio", { name: "1h" })).toBeTruthy(); // One loading state for the whole page, not one per panel. const loading = await screen.findByText("Loading…"); expect(loading.getAttribute("role")).toBe("status"); @@ -335,14 +335,23 @@ test("an empty window says so in every panel instead of drawing nothing", async test("a deep link opens on the period it names", async () => { renderApp("/overview?period=1h"); await screen.findByText("12"); - expect(screen.getByRole("button", { name: "1h" }).getAttribute("aria-pressed")).toBe("true"); - expect(screen.getByRole("button", { name: "24h" }).getAttribute("aria-pressed")).toBe("false"); + // One radio group named Period, holding the four periods and exactly one + // selection: the segmented picker is a single choice, not four toggles. + const picker = within(screen.getByRole("radiogroup", { name: "Period" })); + expect(picker.getAllByRole("radio").map((radio) => radio.getAttribute("value"))).toEqual([ + "1h", + "24h", + "7d", + "30d", + ]); + expect(picker.getByRole("radio", { name: "1h", checked: true })).toBeTruthy(); + expect(picker.getByRole("radio", { name: "24h", checked: false })).toBeTruthy(); }); test("a period the API does not have falls back to the default without carrying it in the url", async () => { const router = renderApp("/overview?period=90d"); await screen.findByText("1,000"); - expect(screen.getByRole("button", { name: "24h" }).getAttribute("aria-pressed")).toBe("true"); + expect(screen.getByRole("radio", { name: "24h", checked: true })).toBeTruthy(); expect(router.state.location.search).toEqual({}); }); @@ -350,7 +359,7 @@ test("the picker rescopes every panel and writes the period into the url", async const router = renderApp(); await screen.findByText("1,000"); - fireEvent.click(screen.getByRole("button", { name: "1h" })); + fireEvent.click(screen.getByRole("radio", { name: "1h" })); await screen.findByText("12"); await waitFor(() => expect(router.state.location.search).toEqual({ period: "1h" })); @@ -369,7 +378,7 @@ test("a failed request is one error for the whole page, stated once and retryabl expect(screen.getAllByRole("button", { name: "Retry" })).toHaveLength(1); // The heading and the picker survive it, so the reader can rescope or retry. expect(screen.getByRole("heading", { name: "Overview", level: 1 })).toBeTruthy(); - expect(screen.getByRole("button", { name: "1h" })).toBeTruthy(); + expect(screen.getByRole("radio", { name: "1h" })).toBeTruthy(); expect(screen.queryByText("Something went wrong")).toBeNull(); failing = false; diff --git a/admin/src/ui/ConfirmDialog.tsx b/admin/src/ui/ConfirmDialog.tsx index dec2374..742d048 100644 --- a/admin/src/ui/ConfirmDialog.tsx +++ b/admin/src/ui/ConfirmDialog.tsx @@ -8,6 +8,7 @@ * and a stray outside click is not one. Escape still cancels. */ +import type { ReactNode } from "react"; import * as stylex from "@stylexjs/stylex"; import { Dialog as AriaDialog, Heading, Modal, ModalOverlay } from "react-aria-components"; import { colors } from "./tokens.stylex"; @@ -19,6 +20,14 @@ interface Props { /** The full sentence the operator reads before confirming; names the entity. */ message: string; confirmLabel: string; + /** + * Stands in for the confirm action when the write can no longer land, the way + * an open edit dialog drops its Save. Closing the dialog instead would throw + * focus at a control that is now disabled and say nothing about why, so the + * question stays on screen and only the answer that would fail is withdrawn. + * Cancel is always live. + */ + lock?: ReactNode; onConfirm: () => void; onCancel: () => void; } @@ -80,7 +89,7 @@ const styles = stylex.create({ }, }); -export default function ConfirmDialog({ isOpen, title, message, confirmLabel, onConfirm, onCancel }: Props) { +export default function ConfirmDialog({ isOpen, title, message, confirmLabel, lock, onConfirm, onCancel }: Props) { return ( Cancel - + {lock === undefined ? ( + + ) : ( + lock + )} diff --git a/admin/src/ui/Dialog.tsx b/admin/src/ui/Dialog.tsx index a531729..8893c96 100644 --- a/admin/src/ui/Dialog.tsx +++ b/admin/src/ui/Dialog.tsx @@ -4,18 +4,25 @@ * React Aria owns the focus trap, the Escape handler and the `aria-modal` * wiring that the hand-rolled overlay only approximated. State is controlled by * the caller because the trigger is a table row button, not a `DialogTrigger`. + * + * The title is both the visible heading and the accessible name: `Heading + * slot="title"` is what React Aria points `aria-labelledby` at, so a caller + * cannot name the dialog one thing and show another. */ import type { ReactNode } from "react"; import * as stylex from "@stylexjs/stylex"; -import { Dialog as AriaDialog, Modal, ModalOverlay } from "react-aria-components"; +import { Dialog as AriaDialog, Heading, Modal, ModalOverlay } from "react-aria-components"; import { colors } from "./tokens.stylex"; +import { styles as shared } from "./styles"; interface Props { - /** The dialog's accessible name. */ - label: string; + /** The dialog's heading, and with it the dialog's accessible name. */ + title: string; isOpen: boolean; onClose: () => void; + /** `detail` is the wider panel a record needs; `form` fits a column of fields. */ + size?: "form" | "detail"; children: ReactNode; } @@ -30,25 +37,74 @@ const styles = stylex.create({ padding: "1rem", backgroundColor: "rgba(0, 0, 0, 0.4)", }, + /** + * A column so the header stays put and the body scrolls under it. The height + * cap is what keeps a long record inside the viewport instead of running off + * the bottom of a short one. + */ panel: { width: "100%", - maxWidth: "28rem", + maxHeight: "85vh", + display: "flex", + flexDirection: "column", borderRadius: "0.5rem", borderWidth: 1, borderStyle: "solid", borderColor: colors.border, backgroundColor: colors.surfaceRaised, color: colors.text, - padding: "1.5rem", boxShadow: "0 10px 15px -3px rgb(0 0 0 / 0.1), 0 4px 6px -4px rgb(0 0 0 / 0.1)", }, - /** The panel already draws the boundary; the dialog's own ring would double it. */ + panelForm: { + maxWidth: "28rem", + }, + panelDetail: { + maxWidth: "52rem", + }, + /** + * The panel already draws the boundary; the dialog's own ring would double + * it. `minHeight: 0` is what lets the body shrink far enough to scroll. + */ body: { outlineStyle: "none", + display: "flex", + flexDirection: "column", + minHeight: 0, + }, + header: { + flexShrink: 0, + display: "flex", + alignItems: "center", + justifyContent: "space-between", + gap: "1rem", + borderBottomWidth: 1, + borderBottomStyle: "solid", + borderBottomColor: colors.border, + paddingInline: "1.5rem", + paddingBlock: "0.75rem", + }, + title: { + fontSize: "1.125rem", + lineHeight: "1.75rem", + fontWeight: 600, + }, + /** 44px on both axes: the pointer-target floor, which the word alone misses. */ + close: { + minWidth: 44, + minHeight: 44, + display: "inline-flex", + alignItems: "center", + justifyContent: "center", + }, + content: { + minHeight: 0, + overflowY: "auto", + paddingInline: "1.5rem", + paddingBlock: "1.5rem", }, }); -export default function Dialog({ label, isOpen, onClose, children }: Props) { +export default function Dialog({ title, isOpen, onClose, size = "form", children }: Props) { return ( stylex.props(styles.overlay).className ?? ""} > - stylex.props(styles.panel).className ?? ""}> - - {children} + + stylex.props(styles.panel, size === "detail" ? styles.panelDetail : styles.panelForm).className ?? + "" + } + > + +
+ + {title} + + +
+
{children}