From 0cfaf6670c5601a085dd66cb1f7c06fe3ae3155e Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Mon, 10 Aug 2026 09:44:29 +0100 Subject: [PATCH] fix(web): address review of the dialog, toast and async primitives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of these were real defects in the previous two commits. friendlyMessage discarded exactly the messages it claimed to keep: the "is this a bare reason phrase" test was a shape regex, and "Default steps cannot be edited" has the same shape as "Not Found". It is an exact-match set of reason phrases now. Modal depended on onKeyDown, which is rebuilt whenever onClose changes identity — and onClose is an inline arrow at every call site, so any parent re-render (a 30s poll, a mutation flipping to pending) tore the effect down and rebuilt it: cleanup restored focus to the trigger, setup then moved it to the top of the dialog, mid-typing. onClose is held in a ref and the effect is keyed on `open` alone. Also in Modal: initial focus takes the first control in the body rather than the panel, since the header comes first in DOM order and every dialog was opening on its own dismiss button; the Tab trap pulls focus back when it has escaped the panel entirely rather than only handling the two ends; the scroll lock is refcounted, because per-instance save/restore released the page when an outer dialog unmounted under an open inner one; and the whole thing is portalled to the body so a nested confirm is not clipped by its parent's overflow box. The fleet's filtered-empty state keyed on the search alone, so a tag filter matching nothing told a customer with a full fleet to add their first server. Remaining: pending mutation errors are reset when a confirm dialog closes, so one member's failure no longer greets the next; ConfirmDialog clears typed confirmation when the target changes, not only when it reopens; deleting a workflow closes its dialogs before navigating rather than carrying a scroll lock onto the next page; toasts split into a polite and an assertive region, since one polite wrapper demotes the role="alert" children inside it; and a custom skeleton gets a live "Loading" beside it, having been aria-hidden with nothing else to announce. --- web/app/(app)/secrets/[group]/page.tsx | 6 +- web/app/(app)/servers/page.tsx | 22 ++- web/components/settings/MembersCard.tsx | 8 +- web/components/ui/Async.tsx | 41 +++- web/components/ui/ConfirmDialog.tsx | 8 +- web/components/ui/Modal.tsx | 183 +++++++++++------- web/components/ui/Toast.tsx | 45 +++-- .../workflows/EditWorkflowModal.tsx | 6 + 8 files changed, 228 insertions(+), 91 deletions(-) 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));