From 75b0c5aa6c6d5de17653fb39b06ad8bf88f01d4e Mon Sep 17 00:00:00 2001 From: pt-act <211776491+pt-act@users.noreply.github.com> Date: Mon, 19 Oct 2026 10:26:15 +0100 Subject: [PATCH 1/3] refactor(dashboard): extract tested in-flight latch for async actions The double-submit guards in EditAgentDrawer were hand-rolled inline refs, so they could only be exercised by mounting the 1400-line drawer - whose mount effects never settle in jsdom, making a component-level regression test impractical. Extract runExclusive(ref, fn) into asyncLatch.ts and route both save handlers through it. confirmListRename keeps its validation outside the latch, so a rejected name can no longer leave the guard set. Behaviour is unchanged; the guard is now covered by asyncLatch.test.ts, which fails if the ref check is removed. --- .../Experts/components/EditAgentDrawer.tsx | 61 ++++++------ .../Experts/components/asyncLatch.test.ts | 93 +++++++++++++++++++ .../pages/Experts/components/asyncLatch.ts | 40 ++++++++ 3 files changed, 161 insertions(+), 33 deletions(-) create mode 100644 dashboard/src/pages/Experts/components/asyncLatch.test.ts create mode 100644 dashboard/src/pages/Experts/components/asyncLatch.ts diff --git a/dashboard/src/pages/Experts/components/EditAgentDrawer.tsx b/dashboard/src/pages/Experts/components/EditAgentDrawer.tsx index 465f211276..141158c2d7 100644 --- a/dashboard/src/pages/Experts/components/EditAgentDrawer.tsx +++ b/dashboard/src/pages/Experts/components/EditAgentDrawer.tsx @@ -81,6 +81,7 @@ import { validatePathMappings, type PathMapping, } from "./agentBackendForm"; +import { runExclusive } from "./asyncLatch"; import AgentBackendFields from "./AgentBackendFields"; import ExpertComposerDefaultsFields from "./ExpertComposerDefaultsFields"; import SkillCatalogDrawer from "./SkillCatalogDrawer"; @@ -250,8 +251,8 @@ function EditAgentDrawerBody({ const [loading, setLoading] = useState(false); const [filesLoading, setFilesLoading] = useState(false); const [saving, setSaving] = useState(false); - // Synchronous in-flight latch: `saving` only flips after a re-render, and - // performSave awaits validation and the root-dir probe before + // Synchronous in-flight latch (see asyncLatch.ts): `saving` only flips after + // a re-render, and performSave awaits validation and the root-dir probe before // setSaving(true), so a second click in that window would start a second save. const savingRef = useRef(false); const [fileModalOpen, setFileModalOpen] = useState(false); @@ -546,13 +547,7 @@ function EditAgentDrawerBody({ ]); const handleSave = useCallback(async () => { - if (savingRef.current) return; - savingRef.current = true; - try { - await performSave(); - } finally { - savingRef.current = false; - } + await runExclusive(savingRef, performSave); }, [performSave]); useEffect(() => { @@ -672,7 +667,6 @@ function EditAgentDrawerBody({ }; const confirmListRename = async () => { - if (listRenameSavingRef.current) return; if (!listRenamePath || !listRenameValue.trim()) return; const dest = joinWorkspacePath( parentWorkspacePath(listRenamePath), @@ -682,30 +676,31 @@ function EditAgentDrawerBody({ setListRenameOpen(false); return; } - listRenameSavingRef.current = true; - setListRenameSaving(true); - try { - await workspaceApi.moveWorkspaceFile( - agent.agent_id, - listRenamePath, - dest, - ); - if (listRenameKind === "subagent") { - await request(`/agents/${agent.agent_id}/reload`, { method: "POST" }); - } - message.success(t("workspace.renameSuccess")); - setListRenameOpen(false); - if (listRenameKind === "config") { - await reloadConfigFiles(); - } else { - await reloadSubagents(); + // Validation stays outside the latch so a rejected name cannot wedge it. + await runExclusive(listRenameSavingRef, async () => { + setListRenameSaving(true); + try { + await workspaceApi.moveWorkspaceFile( + agent.agent_id, + listRenamePath, + dest, + ); + if (listRenameKind === "subagent") { + await request(`/agents/${agent.agent_id}/reload`, { method: "POST" }); + } + message.success(t("workspace.renameSuccess")); + setListRenameOpen(false); + if (listRenameKind === "config") { + await reloadConfigFiles(); + } else { + await reloadSubagents(); + } + } catch (err: unknown) { + message.error(apiErrorMessage(err, t("workspace.renameFailed"), t)); + } finally { + setListRenameSaving(false); } - } catch (err: unknown) { - message.error(apiErrorMessage(err, t("workspace.renameFailed"), t)); - } finally { - listRenameSavingRef.current = false; - setListRenameSaving(false); - } + }); }; const addPathMapping = () => diff --git a/dashboard/src/pages/Experts/components/asyncLatch.test.ts b/dashboard/src/pages/Experts/components/asyncLatch.test.ts new file mode 100644 index 0000000000..377a1f1b72 --- /dev/null +++ b/dashboard/src/pages/Experts/components/asyncLatch.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, it, vi } from "vitest"; +import { runExclusive, type MutableRef } from "./asyncLatch"; + +/** + * Regression guard for the double-submit window described in `asyncLatch.ts`. + * + * The first case is the one that matters: it reproduces the original bug shape + * (an `await` before any render-state guard commits) and asserts the latch + * collapses two overlapping clicks into one call. The last case is a control + * that proves the assertion is not vacuous. + */ +describe("runExclusive", () => { + const latch = (): MutableRef => ({ current: false }); + + it("runs the wrapped call once when invoked twice before it settles", async () => { + const ref = latch(); + let calls = 0; + let release!: () => void; + + const fn = vi.fn(async () => { + calls += 1; + // Model the async pre-flight (validateFields + probeRootDir) that made + // the render-state guard useless: we park until released. + await new Promise((resolve) => { + release = resolve; + }); + return "saved"; + }); + + const first = runExclusive(ref, fn); + // Second click in the same synchronous window, before the first settles. + const second = runExclusive(ref, fn); + + expect(ref.current).toBe(true); + expect(fn).toHaveBeenCalledTimes(1); + expect(await second).toBeUndefined(); + + release(); + expect(await first).toBe("saved"); + expect(calls).toBe(1); + }); + + it("does NOT run the second call while the first is pending (control)", async () => { + const ref = latch(); + const fn = vi.fn(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + return 1; + }); + + const p1 = runExclusive(ref, fn); + const p2 = runExclusive(ref, fn); + + await Promise.all([p1, p2]); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it("releases the latch after success so a later click works", async () => { + const ref = latch(); + const fn = vi.fn(async () => "ok"); + + expect(await runExclusive(ref, fn)).toBe("ok"); + expect(ref.current).toBe(false); + expect(await runExclusive(ref, fn)).toBe("ok"); + expect(fn).toHaveBeenCalledTimes(2); + }); + + it("releases the latch when the wrapped call rejects", async () => { + const ref = latch(); + const boom = vi.fn(async () => { + throw new Error("network down"); + }); + + await expect(runExclusive(ref, boom)).rejects.toThrow("network down"); + // A wedged latch would silently disable Save forever. + expect(ref.current).toBe(false); + expect(await runExclusive(ref, async () => "recovered")).toBe("recovered"); + }); + + it("CONTROL: without the latch the second click does run twice", async () => { + // Same shape as the first case, but calling the handler directly. This must + // observe TWO calls; if it ever observes one, the suite above has stopped + // proving anything about the latch. + let calls = 0; + const unguarded = async () => { + calls += 1; + await new Promise((resolve) => setTimeout(resolve, 0)); + return "saved"; + }; + + await Promise.all([unguarded(), unguarded()]); + expect(calls).toBe(2); + }); +}); diff --git a/dashboard/src/pages/Experts/components/asyncLatch.ts b/dashboard/src/pages/Experts/components/asyncLatch.ts new file mode 100644 index 0000000000..c6f074aa02 --- /dev/null +++ b/dashboard/src/pages/Experts/components/asyncLatch.ts @@ -0,0 +1,40 @@ +/** + * Synchronous in-flight latch for async UI actions. + * + * ``disabled={saving}`` is render state: React cannot commit it until after the + * handler returns, so a second click while an async pre-flight is still in + * flight starts a second request. A `useRef` flips immediately, before any + * `await`, which closes that window. + * + * `EditAgentDrawer` is the reason this exists: its save handler awaited form + * validation and a root-dir probe before calling `setSaving(true)`, so a + * double-click fired two PATCHes (commit 798a19ec, PR #14). Extracted so the + * latch can be unit-tested without mounting the 1400-line drawer, whose mount + * effects are impractical to drive in jsdom. + */ + +/** Minimal shape of a React `useRef` result — avoids importing React here. */ +export interface MutableRef { + current: T; +} + +/** + * Run `fn` unless a call is already in flight, and reset the latch afterwards. + * + * The caller owns the ref so the same latch survives re-renders. When a call is + * skipped the returned promise resolves to `undefined`; when it runs, it + * resolves to `fn`'s value. The latch is released in a `finally`, so a rejected + * `fn` does not wedge the control. + */ +export async function runExclusive( + ref: MutableRef, + fn: () => Promise, +): Promise { + if (ref.current) return undefined; + ref.current = true; + try { + return await fn(); + } finally { + ref.current = false; + } +} From ae098e12d075a57f79da01e10f06a6b7f93e7428 Mon Sep 17 00:00:00 2001 From: pt-act <211776491+pt-act@users.noreply.github.com> Date: Mon, 19 Oct 2026 10:39:57 +0100 Subject: [PATCH 2/3] fix(dashboard): announce chat transcript and mark active nav item (A11Y-1, A11Y-4) A11Y-1: the chat transcript was not a live region, so a screen reader user got no announcement when a reply arrived and had to re-navigate the log manually. Both render paths (plain div and the virtualised Virtuoso list) now carry role="log", aria-live="polite" and aria-relevant="additions", with aria-busy bound to isStreaming so announcements are not fired per streamed token. A11Y-4: the active sidebar entry was conveyed by colour alone. Both the expanded row and the collapsed icon rail now set aria-current="page". The collapsed rail also gains aria-label: its text lives in an antd Tooltip, which is not part of the accessibility tree, so each button announced as "button". --- dashboard/src/layouts/Sidebar.tsx | 1 + .../SidebarCollapsedIconNav.a11y.test.tsx | 70 +++++++++++++++ .../src/layouts/SidebarCollapsedIconNav.tsx | 4 + .../Chat/components/MessageList.a11y.test.tsx | 89 +++++++++++++++++++ .../src/pages/Chat/components/MessageList.tsx | 13 ++- 5 files changed, 176 insertions(+), 1 deletion(-) create mode 100644 dashboard/src/layouts/SidebarCollapsedIconNav.a11y.test.tsx create mode 100644 dashboard/src/pages/Chat/components/MessageList.a11y.test.tsx diff --git a/dashboard/src/layouts/Sidebar.tsx b/dashboard/src/layouts/Sidebar.tsx index c4c8efdf95..6c825d5868 100644 --- a/dashboard/src/layouts/Sidebar.tsx +++ b/dashboard/src/layouts/Sidebar.tsx @@ -167,6 +167,7 @@ function NavItemButton({ type="button" className={styles.navItemMain} onClick={() => onNavigate(item.path)} + aria-current={active ? "page" : undefined} style={{ color: active ? "var(--fn-sidebar-item-active-text)" diff --git a/dashboard/src/layouts/SidebarCollapsedIconNav.a11y.test.tsx b/dashboard/src/layouts/SidebarCollapsedIconNav.a11y.test.tsx new file mode 100644 index 0000000000..2e0b0af2c4 --- /dev/null +++ b/dashboard/src/layouts/SidebarCollapsedIconNav.a11y.test.tsx @@ -0,0 +1,70 @@ +import { describe, expect, it, vi } from "vitest"; +import { render, screen } from "@testing-library/react"; +import SidebarCollapsedIconNav from "./SidebarCollapsedIconNav"; +import type { NavItem } from "./sidebarNav"; + +/** + * A11Y-4 regression guard: the active sidebar entry must expose + * `aria-current="page"`. + * + * The item's active state was previously conveyed by colour alone, so a screen + * reader user got no indication of which section they were in. The name matters + * too: the collapsed rail hides the label in a Tooltip, which is NOT part of the + * accessibility tree, so without `aria-label` each button announces as "button". + */ + +// Tooltip renders a portal; the button itself is what we assert on. +const items: NavItem[] = [ + { key: "chat", path: "/chat", icon: c, labelKey: "nav.chat" }, + { + key: "experts", + path: "/experts", + icon: e, + labelKey: "nav.experts", + }, +]; + +/** Minimal t() that returns the key, so names are predictable. */ +const t = ((key: string) => key) as never; + +function renderNav(selectedKey: string) { + return render( + , + ); +} + +describe("SidebarCollapsedIconNav accessibility", () => { + it("marks the selected item with aria-current=page", () => { + renderNav("experts"); + + const active = screen.getByRole("button", { current: "page" }); + expect(active).toHaveAttribute("aria-current", "page"); + expect(active).toHaveAccessibleName("nav.experts"); + }); + + it("leaves non-selected items without aria-current", () => { + renderNav("experts"); + + const chat = screen.getByRole("button", { name: "nav.chat" }); + expect(chat).not.toHaveAttribute("aria-current"); + }); + + it("gives every collapsed item an accessible name", () => { + renderNav("chat"); + + // Would throw if the icon-only buttons had no accessible name. + expect( + screen.getByRole("button", { name: "nav.chat" }), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "nav.experts" }), + ).toBeInTheDocument(); + }); +}); diff --git a/dashboard/src/layouts/SidebarCollapsedIconNav.tsx b/dashboard/src/layouts/SidebarCollapsedIconNav.tsx index 82f6507eb1..201c5cfa7a 100644 --- a/dashboard/src/layouts/SidebarCollapsedIconNav.tsx +++ b/dashboard/src/layouts/SidebarCollapsedIconNav.tsx @@ -41,6 +41,10 @@ export default function SidebarCollapsedIconNav({ + + , + ); + + it("moves focus into the dialog on open", () => { + renderDialog(); + expect(screen.getByRole("dialog")).toBeInTheDocument(); + // First focusable control, not the trigger behind the overlay. + expect(screen.getByRole("button", { name: "first" })).toHaveFocus(); + }); + + it("dismisses on Escape when dismissable", async () => { + const onClose = vi.fn(); + renderDialog({ onClose }); + + await userEvent.keyboard("{Escape}"); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("ignores Escape when not dismissable (e.g. mid-restart)", async () => { + const onClose = vi.fn(); + renderDialog({ onClose, dismissable: false }); + + await userEvent.keyboard("{Escape}"); + expect(onClose).not.toHaveBeenCalled(); + }); + + it("traps Tab: focus stays inside the dialog", async () => { + renderDialog(); + const first = screen.getByRole("button", { name: "first" }); + const last = screen.getByRole("button", { name: "last" }); + + // Tab forward off the last control wraps back to the first. + last.focus(); + await userEvent.tab(); + expect(first).toHaveFocus(); + + // Shift+Tab from the first wraps to the last. + await userEvent.tab({ shift: true }); + expect(last).toHaveFocus(); + }); + + it("restores focus to the trigger on unmount", async () => { + const trigger = document.createElement("button"); + trigger.textContent = "trigger"; + document.body.appendChild(trigger); + trigger.focus(); + + const { unmount } = renderDialog(); + expect(screen.getByRole("button", { name: "first" })).toHaveFocus(); + + unmount(); + expect(trigger).toHaveFocus(); + trigger.remove(); + }); + + it("dismisses on backdrop click but not on inner click", async () => { + const onClose = vi.fn(); + renderDialog({ onClose }); + + await userEvent.click(screen.getByRole("button", { name: "first" })); + expect(onClose).not.toHaveBeenCalled(); + + await userEvent.click(screen.getByRole("dialog")); + expect(onClose).toHaveBeenCalledTimes(1); + }); +}); diff --git a/dashboard/src/components/AppDialog.tsx b/dashboard/src/components/AppDialog.tsx new file mode 100644 index 0000000000..50918fd575 --- /dev/null +++ b/dashboard/src/components/AppDialog.tsx @@ -0,0 +1,138 @@ +import { useCallback, useEffect, useRef, type ReactNode } from "react"; +import { createPortal } from "react-dom"; + +/** + * Modal dialog with the keyboard behaviour `role="dialog" aria-modal="true"` + * promises. + * + * The three hand-rolled overlays (PwaInstallPrompt's IosGuide and + * DesktopInstallGuide, ServiceRestartOverlay) already declare themselves modal, + * but none of them implement it: focus stays on the trigger behind the overlay, + * Tab walks out into the page underneath, Escape does nothing, and closing does + * not return focus. Assistive tech is told the rest of the page is inert while + * the keyboard can still reach it. + * + * Scope is deliberately narrow — focus move on open, Tab trap, Escape, restore on + * close. Layout and styling stay with the caller, so this replaces the shared + * behaviour without dictating appearance. + */ + +/** Elements that can receive focus, in DOM order. */ +const FOCUSABLE = [ + "a[href]", + "button:not([disabled])", + "input:not([disabled])", + "select:not([disabled])", + "textarea:not([disabled])", + '[tabindex]:not([tabindex="-1"])', +].join(","); + +export interface AppDialogProps { + /** Accessible name; required unless the dialog is labelled by its content. */ + "aria-label"?: string; + "aria-labelledby"?: string; + /** `alertdialog` for urgent interruptions (restart, destructive confirm). */ + role?: "dialog" | "alertdialog"; + /** When false, Escape and backdrop clicks do not dismiss (e.g. mid-restart). */ + dismissable?: boolean; + /** Escape key or backdrop click. Omit for a non-dismissable dialog. */ + onClose?: () => void; + /** Extra attributes for the overlay (className, style, aria-busy, …). */ + overlayProps?: React.HTMLAttributes; + /** Class for the overlay element, appended to the default. */ + overlayClassName?: string; + children: ReactNode; +} + +export default function AppDialog({ + role = "dialog", + dismissable = true, + onClose, + overlayProps, + overlayClassName, + children, + ...aria +}: AppDialogProps) { + const overlayRef = useRef(null); + /** The element focused before we opened, so we can put focus back. */ + const restoreRef = useRef(null); + + const focusables = useCallback((): HTMLElement[] => { + const root = overlayRef.current; + if (!root) return []; + // No visibility filter: offsetParent is always null in jsdom (and would + // wrongly drop legitimately focusable controls there), so rely on the + // disabled/type selectors above plus `hidden` to exclude inert nodes. + return Array.from(root.querySelectorAll(FOCUSABLE)).filter( + (el) => + !el.hasAttribute("hidden") && el.getAttribute("aria-hidden") !== "true", + ); + }, []); + + // Move focus in on open, and restore it on close. + useEffect(() => { + restoreRef.current = + document.activeElement instanceof HTMLElement + ? document.activeElement + : null; + + // Prefer the first focusable control; fall back to the overlay itself so + // focus is never left behind on the trigger. + const target = focusables()[0] ?? overlayRef.current; + target?.focus(); + + return () => { + restoreRef.current?.focus?.(); + }; + }, [focusables]); + + const handleKeyDown = (e: React.KeyboardEvent) => { + if (e.key === "Escape" && dismissable) { + e.stopPropagation(); + onClose?.(); + return; + } + if (e.key !== "Tab") return; + + // Trap Tab within the dialog. Without this the browser moves focus to the + // page behind the overlay, which aria-modal has already declared inert. + const items = focusables(); + if (items.length === 0) { + e.preventDefault(); + return; + } + const first = items[0]; + const last = items[items.length - 1]; + const active = document.activeElement; + + if ( + e.shiftKey && + (active === first || !overlayRef.current?.contains(active)) + ) { + e.preventDefault(); + last.focus(); + } else if (!e.shiftKey && active === last) { + e.preventDefault(); + first.focus(); + } + }; + + return createPortal( +
{ + overlayProps?.onClick?.(e); + if (dismissable && e.target === e.currentTarget) onClose?.(); + }} + > + {children} +
, + document.body, + ); +} diff --git a/dashboard/src/components/PwaInstallPrompt/index.tsx b/dashboard/src/components/PwaInstallPrompt/index.tsx index 65e18b3c42..f7a90ad8cf 100644 --- a/dashboard/src/components/PwaInstallPrompt/index.tsx +++ b/dashboard/src/components/PwaInstallPrompt/index.tsx @@ -1,5 +1,5 @@ import { useEffect, useSyncExternalStore, useState } from "react"; -import { createPortal } from "react-dom"; +import AppDialog from "../AppDialog"; import { Tooltip } from "antd"; import { useTranslation } from "react-i18next"; import { Download, MonitorDown, Share, X } from "lucide-react"; @@ -35,15 +35,11 @@ function isStandalone(): boolean { export function IosGuide({ onClose }: { onClose: () => void }) { const { t } = useTranslation(); - return createPortal( -
{ - if (e.target === e.currentTarget) onClose(); - }} + onClose={onClose} >
-
, - document.body, + ); } @@ -89,15 +84,11 @@ export function IosGuide({ onClose }: { onClose: () => void }) { export function DesktopInstallGuide({ onClose }: { onClose: () => void }) { const { t } = useTranslation(); const isEdge = /edg/i.test(navigator.userAgent); - return createPortal( -
{ - if (e.target === e.currentTarget) onClose(); - }} + onClose={onClose} >
-
, - document.body, + ); } diff --git a/dashboard/src/components/ServiceRestartOverlay.tsx b/dashboard/src/components/ServiceRestartOverlay.tsx index 528baadd5a..5ef5557a28 100644 --- a/dashboard/src/components/ServiceRestartOverlay.tsx +++ b/dashboard/src/components/ServiceRestartOverlay.tsx @@ -1,6 +1,6 @@ import { useEffect } from "react"; import type { CSSProperties } from "react"; -import { createPortal } from "react-dom"; +import AppDialog from "./AppDialog"; import { CheckCircle, AlertTriangle, Power } from "lucide-react"; import { Button, Spin } from "antd"; import { useTranslation } from "react-i18next"; @@ -82,13 +82,21 @@ export default function ServiceRestartOverlay({ if (phase === "idle") return null; - return createPortal( -
{phase === "confirm" && ( @@ -153,7 +161,6 @@ export default function ServiceRestartOverlay({ )}
-
, - document.body, + ); }