diff --git a/web/app/(app)/secrets/[group]/page.tsx b/web/app/(app)/secrets/[group]/page.tsx index 37b497f..7b26192 100644 --- a/web/app/(app)/secrets/[group]/page.tsx +++ b/web/app/(app)/secrets/[group]/page.tsx @@ -142,6 +142,7 @@ function SecretRow({ group, secret }: { group: string; secret: Secret }) { mutate: remove, isPending: removing, error: removeError, + reset: resetRemove, } = useMutation({ mutationFn: () => api.deleteSecret(group, secret.key), onSuccess: () => { @@ -199,7 +200,10 @@ function SecretRow({ group, secret }: { group: string; secret: Secret }) { confirmLabel="Delete key" loading={removing} error={removeError ? friendlyMessage(removeError) : null} - onClose={() => setConfirming(false)} + onClose={() => { + resetRemove(); + setConfirming(false); + }} onConfirm={() => remove()} body={ <> diff --git a/web/app/(app)/servers/page.tsx b/web/app/(app)/servers/page.tsx index edf7348..21c408c 100644 --- a/web/app/(app)/servers/page.tsx +++ b/web/app/(app)/servers/page.tsx @@ -228,11 +228,25 @@ function ServersPageBody() { skeleton={} isEmpty={visible.length === 0} empty={ - search.trim() ? ( + /* Narrowed to nothing is not the same as owning nothing. Telling a + customer with a full fleet to "add your first server" because a + tag filter matched none of it is the version of this that gets + screenshotted. */ + search.trim() || Object.keys(selected).length > 0 ? ( setSearch("") }} + title="No servers match those filters." + description={ + Object.keys(selected).length > 0 && search.trim() + ? "Nothing matches both the tag filter and the search." + : Object.keys(selected).length > 0 + ? "No server carries every tag selected above." + : "Clear the search to see the rest of the fleet." + } + action={ + search.trim() + ? { label: "Clear search", onClick: () => setSearch("") } + : { label: "Clear filters", onClick: () => setSelected({}) } + } /> ) : ( api.deleteInstanceUser(member.id), onSuccess: (_data, member) => { @@ -193,7 +194,12 @@ export function MembersCard() { confirmLabel="Remove member" loading={isRemoving} error={removeError ? friendlyMessage(removeError) : null} - onClose={() => setRemoving(null)} + onClose={() => { + // Without this the next member's dialog opens showing the + // previous member's failure. + resetRemove(); + setRemoving(null); + }} onConfirm={() => removing && removeUser(removing)} body={ <> diff --git a/web/components/ui/Async.tsx b/web/components/ui/Async.tsx index e5c0542..8850fd1 100644 --- a/web/components/ui/Async.tsx +++ b/web/components/ui/Async.tsx @@ -125,7 +125,22 @@ export function AsyncBoundary({ empty?: React.ReactNode; children: React.ReactNode; }) { - if (isLoading) return <>{skeleton ?? }; + if (isLoading) { + // A skeleton is aria-hidden decoration, so on its own it hands a screen + // reader an empty region and no indication anything is coming. The + // spinner carries its own role="status"; a custom skeleton needs one + // supplied beside it. + return skeleton ? ( + <> + + Loading + + {skeleton} + + ) : ( + + ); + } if (error) return ; if (isEmpty && empty) return <>{empty}; return <>{children}; @@ -138,6 +153,25 @@ export function AsyncBoundary({ * customer nothing and reads as a crash. Classify first, then show the detail * underneath rather than instead of an explanation. */ +/* The reason phrases request() falls back to when the response body was empty. + An exact-match set, not a shape test: a pattern loose enough to catch + "Not Found" also catches "Default steps cannot be edited", which is the + opposite of what this is for. */ +const STATUS_TEXT = new Set([ + "Bad Request", + "Unauthorized", + "Forbidden", + "Not Found", + "Method Not Allowed", + "Conflict", + "Unprocessable Entity", + "Too Many Requests", + "Internal Server Error", + "Bad Gateway", + "Service Unavailable", + "Gateway Timeout", +]); + export function friendlyMessage(error: unknown): string { const status = (error as { status?: number } | null)?.status; const raw = error instanceof Error ? error.message : typeof error === "string" ? error : ""; @@ -145,8 +179,9 @@ export function friendlyMessage(error: unknown): string { // The backend writes its 4xx messages for an operator and they are usually // the most specific thing available ("default steps cannot be edited", // "vulnerability scanning is not licensed"). Keep them; only replace the - // ones that are a status code wearing a coat. - const useful = raw && !/^HTTP \d{3}$/.test(raw) && !/^[A-Z][a-z]+( [A-Z]?[a-z]+)*$/.test(raw) ? raw : ""; + // ones that are a status code wearing a coat — "HTTP 409", or the bare + // reason phrase fetch() falls back to when the body was empty. + const useful = raw && !/^HTTP \d{3}$/.test(raw) && !STATUS_TEXT.has(raw) ? raw : ""; if (status === 401) return "Your session has expired. Sign in again to continue."; if (status === 403) return useful || "You do not have permission to do this."; diff --git a/web/components/ui/ConfirmDialog.tsx b/web/components/ui/ConfirmDialog.tsx index 2899112..226ce7e 100644 --- a/web/components/ui/ConfirmDialog.tsx +++ b/web/components/ui/ConfirmDialog.tsx @@ -43,10 +43,12 @@ export function ConfirmDialog({ const [typed, setTyped] = useState(""); const inputId = useId(); - // A reopened dialog must not carry the previous attempt's typing. + // A reopened dialog must not carry the previous attempt's typing — nor may + // a row reused for a different item stay armed with the name it matched + // before, which is why `requireTyped` is a dependency and not just `open`. useEffect(() => { - if (open) setTyped(""); - }, [open]); + setTyped(""); + }, [open, requireTyped]); const armed = !requireTyped || typed === requireTyped; diff --git a/web/components/ui/Modal.tsx b/web/components/ui/Modal.tsx index a11769a..a09e4b2 100644 --- a/web/components/ui/Modal.tsx +++ b/web/components/ui/Modal.tsx @@ -1,6 +1,7 @@ "use client"; -import { useCallback, useEffect, useId, useRef } from "react"; +import { useEffect, useId, useRef, useState } from "react"; +import { createPortal } from "react-dom"; /* * Dialogs nest: a confirm sits on top of the edit modal that raised it. Both @@ -10,6 +11,38 @@ import { useCallback, useEffect, useId, useRef } from "react"; */ const stack: symbol[] = []; +/* + * The scroll lock is refcounted rather than saved and restored per dialog. + * Per-instance save/restore breaks when the outer dialog unmounts first — which + * a dialog that navigates away on success does — since the outer's cleanup then + * releases the lock while the inner one is still on screen. + */ +let lockCount = 0; +let lockedOverflow = ""; +let lockedPadding = ""; + +function lockScroll() { + const { body } = document; + if (lockCount === 0) { + lockedOverflow = body.style.overflow; + lockedPadding = body.style.paddingRight; + // Padding replaces the scrollbar's width so the layout does not jump + // sideways as it disappears. + const gap = window.innerWidth - document.documentElement.clientWidth; + body.style.overflow = "hidden"; + if (gap > 0) body.style.paddingRight = `${gap}px`; + } + lockCount++; +} + +function unlockScroll() { + lockCount = Math.max(0, lockCount - 1); + if (lockCount === 0) { + document.body.style.overflow = lockedOverflow; + document.body.style.paddingRight = lockedPadding; + } +} + const FOCUSABLE = [ "a[href]", "button:not([disabled])", @@ -19,6 +52,12 @@ const FOCUSABLE = [ "[tabindex]:not([tabindex='-1'])", ].join(","); +function focusableIn(root: HTMLElement): HTMLElement[] { + return Array.from(root.querySelectorAll(FOCUSABLE)).filter( + (el) => el.offsetParent !== null || el === document.activeElement, + ); +} + export function Modal({ open, title, @@ -33,56 +72,25 @@ export function Modal({ wide?: boolean; }) { const panelRef = useRef(null); + const bodyRef = useRef(null); const restoreRef = useRef(null); const titleId = useId(); const idRef = useRef(Symbol("modal")); /* - * Everything below is what an accessible dialog owes the person using it, - * and none of it was here: focus stayed on the page behind, Tab walked out - * of the dialog into content the overlay had covered, the background - * scrolled under the panel, and closing left focus on so the next - * Tab restarted from the top of the document. + * onClose is an inline arrow at every call site, so its identity changes on + * each render of the parent — and a parent re-renders on every react-query + * poll and every mutation state flip. Holding it in a ref is what keeps the + * effect below keyed on `open` alone: depending on the handler tore the + * whole thing down and rebuilt it mid-interaction, which yanked focus out + * of whatever the user was typing in and back to the top of the dialog. */ + const closeRef = useRef(onClose); + closeRef.current = onClose; - // Escape closes. Tab is confined to the panel. - const onKeyDown = useCallback( - (e: KeyboardEvent) => { - // Only the topmost dialog reacts. - if (stack[stack.length - 1] !== idRef.current) return; - - if (e.key === "Escape") { - e.stopPropagation(); - onClose(); - return; - } - if (e.key !== "Tab" || !panelRef.current) return; - - const items = Array.from(panelRef.current.querySelectorAll(FOCUSABLE)).filter( - (el) => el.offsetParent !== null || el === document.activeElement, - ); - if (items.length === 0) { - // Nothing focusable inside; keep focus on the panel rather than - // letting Tab escape to the page underneath. - e.preventDefault(); - panelRef.current.focus(); - return; - } - - const first = items[0]; - const last = items[items.length - 1]; - const active = document.activeElement as HTMLElement | null; - - if (e.shiftKey && (active === first || active === panelRef.current)) { - e.preventDefault(); - last.focus(); - } else if (!e.shiftKey && active === last) { - e.preventDefault(); - first.focus(); - } - }, - [onClose], - ); + // Portals need a DOM that exists, which it does not during SSR. + const [mounted, setMounted] = useState(false); + useEffect(() => setMounted(true), []); useEffect(() => { if (!open) return; @@ -90,42 +98,80 @@ export function Modal({ const id = idRef.current; stack.push(id); restoreRef.current = document.activeElement as HTMLElement | null; + lockScroll(); - // The page behind must not scroll while a dialog is over it. Padding - // replaces the scrollbar's width so the layout does not jump sideways - // as it disappears. - const { body } = document; - const prevOverflow = body.style.overflow; - const prevPadding = body.style.paddingRight; - const gap = window.innerWidth - document.documentElement.clientWidth; - body.style.overflow = "hidden"; - if (gap > 0) body.style.paddingRight = `${gap}px`; + const onKeyDown = (e: KeyboardEvent) => { + // Only the topmost dialog reacts. + if (stack[stack.length - 1] !== id) return; + const panel = panelRef.current; + if (!panel) return; + + if (e.key === "Escape") { + e.stopPropagation(); + closeRef.current(); + return; + } + if (e.key !== "Tab") return; + + const items = focusableIn(panel); + if (items.length === 0) { + e.preventDefault(); + panel.focus(); + return; + } + + const first = items[0]; + const last = items[items.length - 1]; + const active = document.activeElement as HTMLElement | null; + + // Focus can be outside the panel entirely — on after a + // control unmounted, or on the page behind. Pull it back rather + // than letting Tab continue out into content the overlay covers. + if (!active || !panel.contains(active)) { + e.preventDefault(); + (e.shiftKey ? last : first).focus(); + return; + } + + if (e.shiftKey && (active === first || active === panel)) { + e.preventDefault(); + last.focus(); + } else if (!e.shiftKey && active === last) { + e.preventDefault(); + first.focus(); + } + }; document.addEventListener("keydown", onKeyDown, true); - // Focus the first real control, falling back to the panel itself. The - // close button is deliberately not preferred: opening a dialog on its - // dismiss control reads as "are you sure you want to be here". - const target = - panelRef.current?.querySelector(FOCUSABLE) ?? panelRef.current; + /* + * Focus the first control in the body, not in the panel: the header + * comes first in DOM order, so querying the whole panel opens every + * dialog on its own dismiss button, which reads as "are you sure you + * want to be here". + */ + const target = (bodyRef.current && focusableIn(bodyRef.current)[0]) ?? panelRef.current; target?.focus(); return () => { const at = stack.lastIndexOf(id); if (at !== -1) stack.splice(at, 1); document.removeEventListener("keydown", onKeyDown, true); - // Each dialog restores what it found, so an inner one closing over - // an outer one puts back "hidden" rather than releasing the page. - body.style.overflow = prevOverflow; - body.style.paddingRight = prevPadding; + unlockScroll(); // Return focus to whatever opened the dialog, if it is still there. if (restoreRef.current?.isConnected) restoreRef.current.focus(); + restoreRef.current = null; }; - }, [open, onKeyDown]); + }, [open]); - if (!open) return null; + if (!open || !mounted) return null; - return ( + /* + * Portalled to the body. A nested confirm would otherwise render inside its + * parent panel's overflow-auto box and be clipped by it, and a dialog is + * not part of the content it covers. + */ + return createPortal(
-
+ , + document.body, ); } diff --git a/web/components/ui/Toast.tsx b/web/components/ui/Toast.tsx index ad76639..285e3a4 100644 --- a/web/components/ui/Toast.tsx +++ b/web/components/ui/Toast.tsx @@ -94,15 +94,37 @@ export function ToastProvider({ children }: { children: React.ReactNode }) { return ( {children} -
- {toasts.map((t) => ( + {/* + * Two regions, not one polite container holding role="alert" + * children: live-region politeness is taken from the nearest + * ancestor that declares it, so a single polite wrapper demotes the + * errors inside it. Errors interrupt because they mean the thing + * the operator asked for did not happen; a confirmation can wait + * for a pause in speech. + * + * The wrapper is a plain flex column so both regions stack as one + * visual list. + */} +
+ t.kind !== "error")} politeness="polite" onDismiss={dismiss} /> + t.kind === "error")} politeness="assertive" onDismiss={dismiss} /> +
+ + ); +} + +function ToastRegion({ + toasts, + politeness, + onDismiss, +}: { + toasts: Toast[]; + politeness: "polite" | "assertive"; + onDismiss: (id: number) => void; +}) { + return ( +
+ {toasts.map((t) => (
))} -
- +
); } diff --git a/web/components/workflows/EditWorkflowModal.tsx b/web/components/workflows/EditWorkflowModal.tsx index 64827af..9c73079 100644 --- a/web/components/workflows/EditWorkflowModal.tsx +++ b/web/components/workflows/EditWorkflowModal.tsx @@ -59,6 +59,12 @@ export function EditWorkflowModal({ open, workflow, onSaved, onClose }: { open: try { await api.deleteWorkflow(workflow.workflow_id); toast.success(`Deleted ${workflow.name}.`); + // Close both dialogs before navigating. Leaving them mounted takes + // their scroll lock and focus trap onto the workflows list and + // holds it there until the route change happens to unmount them. + setConfirmingDelete(false); + setBusy(false); + onClose(); router.push("/workflows"); } catch (e) { setError(friendlyMessage(e));