diff --git a/docs/config/notifications.mdx b/docs/config/notifications.mdx index 32bdb95ce0a..dc6a1406f6b 100644 --- a/docs/config/notifications.mdx +++ b/docs/config/notifications.mdx @@ -9,7 +9,7 @@ Xum can send system notifications to alert you about important events. Notificat There are two ways to receive notifications: -1. **Automatic notifications** — Toggle the bell icon in the workspace header to get notified when the agent completes a response. Use Ctrl+Shift+, (⌘+Shift+, on macOS) to toggle quickly. +1. **Automatic notifications** — Click the bell icon in the workspace header and check **Notify on all responses** to get notified when the agent completes a response. Use Ctrl+Shift+, (⌘+Shift+, on macOS) to toggle quickly. 2. **Agent-triggered notifications** — The `notify` tool lets agents send notifications for specific events. You control when agents use this through prompts or scoped instructions. diff --git a/src/browser/components/AgentModePicker/AgentModePicker.test.tsx b/src/browser/components/AgentModePicker/AgentModePicker.test.tsx index c7ac3395b1f..32aa55886a8 100644 --- a/src/browser/components/AgentModePicker/AgentModePicker.test.tsx +++ b/src/browser/components/AgentModePicker/AgentModePicker.test.tsx @@ -1,6 +1,7 @@ import React from "react"; import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { cleanup, fireEvent, render, waitFor } from "@testing-library/react"; +import { act, cleanup, fireEvent, render, waitFor } from "@testing-library/react"; +import { createCustomEvent, CUSTOM_EVENTS } from "@/common/constants/events"; import { installDom } from "../../../../tests/ui/dom"; import { AgentProvider, type AgentContextValue } from "@/browser/contexts/AgentContext"; @@ -145,6 +146,20 @@ describe("AgentModePicker", () => { expect(view.getByLabelText("Select agent").getAttribute("aria-expanded")).toBe("false"); }); + test("Escape closes the picker right after the open shortcut", () => { + const view = renderPicker(); + // The hotkey and the palette open the picker through this event. Escape can arrive before + // the next animation frame, so the list must already own focus when the open commits. + act(() => { + window.dispatchEvent(createCustomEvent(CUSTOM_EVENTS.OPEN_AGENT_PICKER)); + }); + expect(view.getAllByTestId("agent-option").length).toBe(3); + + fireEvent.keyDown(document.activeElement ?? document.body, { key: "Escape" }); + + expect(view.queryAllByTestId("agent-option").length).toBe(0); + }); + test("uiSelectable false without lock flag does not disable the picker", async () => { const { getByLabelText, queryAllByTestId } = renderPicker({ initialAgentId: "explore", diff --git a/src/browser/components/AgentModePicker/AgentModePicker.tsx b/src/browser/components/AgentModePicker/AgentModePicker.tsx index 4b83a0236c9..a452c3d4030 100644 --- a/src/browser/components/AgentModePicker/AgentModePicker.tsx +++ b/src/browser/components/AgentModePicker/AgentModePicker.tsx @@ -1,4 +1,5 @@ import React, { useCallback, useEffect, useId, useMemo, useRef, useState } from "react"; +import { flushSync } from "react-dom"; import { Bot, ChevronDown, Monitor, Route, SquareCode } from "lucide-react"; import type { LucideIcon } from "lucide-react"; @@ -179,16 +180,23 @@ export const AgentModePicker: React.FC = (props) => { return; } - setIsPickerOpen(true); - // macOS permissions change outside the app, so re-read them whenever the picker opens. - computerUse?.refresh(); - // Pre-select the current agent (or specified) in the list. const targetId = opts?.highlightAgentId ?? normalizedAgentId; const currentIndex = options.findIndex((opt) => opt.id === targetId); - setHighlightedIndex(currentIndex >= 0 ? currentIndex : 0); + // Commit the list now so it can take focus before the next key arrives. Escape and the + // arrow keys are handled only on the list; when the shortcut opened the picker, the old + // frame-deferred focus could run before the list mounted, so focus stayed in the + // composer and Escape did nothing (#5676). + flushSync(() => { + setIsPickerOpen(true); + setHighlightedIndex(currentIndex >= 0 ? currentIndex : 0); + }); + dropdownRef.current?.focus(); + // macOS permissions change outside the app, so re-read them whenever the picker opens. + computerUse?.refresh(); - // Focus the dropdown container for keyboard navigation. + // Focus again after the frame: the command palette opens the picker while it closes, + // and its focus restore moves focus back to the composer after this handler returns. requestAnimationFrame(() => { dropdownRef.current?.focus(); }); diff --git a/src/browser/components/LeftSidebar/LeftSidebar.tsx b/src/browser/components/LeftSidebar/LeftSidebar.tsx index 55e41a26d13..f19547340f3 100644 --- a/src/browser/components/LeftSidebar/LeftSidebar.tsx +++ b/src/browser/components/LeftSidebar/LeftSidebar.tsx @@ -1,10 +1,11 @@ -import React from "react"; +import React, { useSyncExternalStore } from "react"; import { cn } from "@/common/lib/utils"; import type { FrontendWorkspaceMetadata } from "@/common/types/workspace"; import { LEFT_SIDEBAR_COLLAPSED_WIDTH_PX, LEFT_SIDEBAR_DEFAULT_WIDTH_PX } from "@/constants/layout"; import ProjectSidebar from "../ProjectSidebar/ProjectSidebar"; import { TitleBar } from "../TitleBar/TitleBar"; import { isDesktopMode } from "@/browser/hooks/useDesktopTitlebar"; +import { useEscapeToDismiss } from "@/browser/hooks/useEscapeToDismiss"; interface LeftSidebarProps { collapsed: boolean; @@ -16,6 +17,18 @@ interface LeftSidebarProps { workspaceRecency: Record; } +const MOBILE_OVERLAY_QUERY = "(max-width: 768px)"; + +function readMobileOverlayQuery(): boolean { + return typeof window !== "undefined" && window.matchMedia(MOBILE_OVERLAY_QUERY).matches; +} + +function subscribeToMobileOverlayQuery(onChange: () => void): () => void { + const query = window.matchMedia(MOBILE_OVERLAY_QUERY); + query.addEventListener("change", onChange); + return () => query.removeEventListener("change", onChange); +} + export function LeftSidebar(props: LeftSidebarProps) { const { collapsed, @@ -27,9 +40,17 @@ export function LeftSidebar(props: LeftSidebarProps) { } = props; const isDesktop = isDesktopMode(); // Match the CSS gate for the mobile "overlay" sidebar (width-only, any pointer - // type); we don't show a drag handle in that mode since CSS pins the width. - const isMobileOverlay = - typeof window !== "undefined" && window.matchMedia("(max-width: 768px)").matches; + // type); we don't show a drag handle in that mode since CSS pins the width. Subscribed, so a + // window resized into the overlay width also gets the drawer's Escape handling. + const isMobileOverlay = useSyncExternalStore( + subscribeToMobileOverlayQuery, + readMobileOverlayQuery, + () => false + ); + + // The drawer covers the page like a modal, so Escape closes it the way a backdrop tap does + // (#5685), unless a popover, menu or dialog inside it owns Escape. + useEscapeToDismiss(!collapsed && isMobileOverlay, onToggleCollapsed); const handleBeforeOpenSettings = () => { // Keep settings navigation escapable on narrow viewports by dismissing the diff --git a/src/browser/components/WorkspaceMenuBar/WorkspaceMenuBar.tsx b/src/browser/components/WorkspaceMenuBar/WorkspaceMenuBar.tsx index 759e2d37452..151e1130b50 100644 --- a/src/browser/components/WorkspaceMenuBar/WorkspaceMenuBar.tsx +++ b/src/browser/components/WorkspaceMenuBar/WorkspaceMenuBar.tsx @@ -684,9 +684,11 @@ export const WorkspaceMenuBar: React.FC = ({ + {/* A click only opens the settings: it used to also flip "Notify on all + responses", so nobody could look at the settings without changing them + (#5691). The checkbox and the shortcut toggle it. */} + {/* A plain label: the settings live only in the popover. When the tooltip repeated + them, Radix reopened it on the refocused bell after Escape closed the popover, + and a second copy of the settings appeared (#5691). */} -
- - -

- Agents can also notify on specific events.{" "} - - Learn more - -

-
+ Notifications +
+ Notify on all responses: {notifyOnResponse ? "on" : "off"} + + {" "} + ({formatKeybind(KEYBINDS.TOGGLE_NOTIFICATIONS)}) + diff --git a/src/browser/hooks/useAIViewKeybinds.ts b/src/browser/hooks/useAIViewKeybinds.ts index d61d386d7cc..984c1fb94e6 100644 --- a/src/browser/hooks/useAIViewKeybinds.ts +++ b/src/browser/hooks/useAIViewKeybinds.ts @@ -14,6 +14,7 @@ import type { StreamingMessageAggregator } from "@/browser/utils/messages/Stream import { isCompactingStream, cancelCompaction } from "@/browser/utils/compaction/handler"; import { stopStream } from "@/browser/utils/stopStream"; import { useAPI } from "@/browser/contexts/API"; +import { isEscapeDismissOverlayOpen } from "@/browser/hooks/useEscapeToDismiss"; import type { EditingMessageState } from "@/browser/utils/chatEditing"; interface UseAIViewKeybindsParams { @@ -91,6 +92,11 @@ export function useAIViewKeybinds({ return; } + // An open overlay (tutorial, narrow-screen drawer) takes this Escape to close itself. + if (interruptKeybind === KEYBINDS.INTERRUPT_STREAM_NORMAL && isEscapeDismissOverlayOpen()) { + return; + } + // Normal mode uses Escape; skip when typing in inputs unless explicitly opted in. if ( interruptKeybind === KEYBINDS.INTERRUPT_STREAM_NORMAL && diff --git a/src/browser/hooks/useEscapeToDismiss.test.tsx b/src/browser/hooks/useEscapeToDismiss.test.tsx new file mode 100644 index 00000000000..d7b96aee83c --- /dev/null +++ b/src/browser/hooks/useEscapeToDismiss.test.tsx @@ -0,0 +1,151 @@ +import type { ReactNode, RefObject } from "react"; +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { cleanup, renderHook, waitFor } from "@testing-library/react"; +import { GlobalWindow } from "happy-dom"; +import type { ChatInputAPI } from "@/browser/features/ChatInput"; +import { APIProvider, type APIClient } from "@/browser/contexts/API"; +import { createTestApiClient, type TestApiOverrides } from "@/browser/testUtils"; +import { useAIViewKeybinds } from "./useAIViewKeybinds"; +import { useEscapeToDismiss } from "./useEscapeToDismiss"; + +let originalWindow: typeof globalThis.window; +let originalDocument: typeof globalThis.document; +let originalHTMLElement: unknown; + +// The app mounts the stream-interrupt listener (ChatPane) before an overlay opens, so this +// harness registers it first too: the order decides which window listener sees Escape first. +function renderWithStreamInterrupt(overlayOpen: boolean) { + const interruptStream = mock(() => Promise.resolve({ success: true as const, data: undefined })); + const client: TestApiOverrides = { workspace: { interruptStream } }; + const onDismiss = mock(() => undefined); + const chatInputAPI: RefObject = { current: null }; + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + renderHook( + () => { + useAIViewKeybinds({ + workspaceId: "ws", + canInterrupt: true, + showRetryBarrier: false, + chatInputAPI, + jumpToBottom: () => undefined, + loadOlderHistory: null, + handleOpenTerminal: () => undefined, + handleOpenInEditor: () => undefined, + aggregator: undefined, + setEditingMessage: () => undefined, + vimEnabled: false, + }); + useEscapeToDismiss(overlayOpen, onDismiss); + }, + { wrapper } + ); + return { interruptStream, onDismiss }; +} + +function pressEscape(target: EventTarget, init: KeyboardEventInit = {}): KeyboardEvent { + const event = new window.KeyboardEvent("keydown", { + key: "Escape", + bubbles: true, + cancelable: true, + ...init, + }); + target.dispatchEvent(event); + return event; +} + +function addComposer(): HTMLTextAreaElement { + const composer = document.createElement("textarea"); + document.body.appendChild(composer); + composer.focus(); + return composer; +} + +describe("useEscapeToDismiss", () => { + beforeEach(() => { + originalWindow = globalThis.window; + originalDocument = globalThis.document; + originalHTMLElement = (globalThis as unknown as { HTMLElement: unknown }).HTMLElement; + const domWindow = new GlobalWindow() as unknown as Window & typeof globalThis; + globalThis.window = domWindow; + globalThis.document = domWindow.document; + // The keybind helpers check `target instanceof HTMLElement`. + (globalThis as unknown as { HTMLElement: unknown }).HTMLElement = domWindow.HTMLElement; + }); + + afterEach(() => { + cleanup(); + globalThis.window = originalWindow; + globalThis.document = originalDocument; + (globalThis as unknown as { HTMLElement: unknown }).HTMLElement = originalHTMLElement; + }); + + test("with the overlay closed, Escape interrupts the stream exactly as before", async () => { + const { interruptStream, onDismiss } = renderWithStreamInterrupt(false); + + // The composer ignores Escape for the interrupt unless it opts in, and nothing claims it. + const fromComposer = pressEscape(addComposer()); + expect(fromComposer.defaultPrevented).toBe(false); + expect(interruptStream).not.toHaveBeenCalled(); + + // Outside an editable element, Escape still interrupts. + pressEscape(document.body); + await waitFor(() => expect(interruptStream).toHaveBeenCalledTimes(1)); + expect(onDismiss).not.toHaveBeenCalled(); + }); + + test("with the overlay open, Escape from the composer closes it and interrupts nothing", () => { + const { interruptStream, onDismiss } = renderWithStreamInterrupt(true); + + const event = pressEscape(addComposer()); + + expect(onDismiss).toHaveBeenCalledTimes(1); + expect(event.defaultPrevented).toBe(true); + expect(interruptStream).not.toHaveBeenCalled(); + }); + + test("with the overlay open, Escape from targets the interrupt accepts closes it and interrupts nothing", async () => { + const { interruptStream, onDismiss } = renderWithStreamInterrupt(true); + const optedIn = addComposer(); + optedIn.setAttribute("data-escape-interrupts-stream", ""); + + pressEscape(document.body); + pressEscape(optedIn); + + expect(onDismiss).toHaveBeenCalledTimes(2); + // stopStream runs asynchronously, so give a wrongly started interrupt time to arrive. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(interruptStream).not.toHaveBeenCalled(); + }); + + test("leaves Escape to whatever already handled it", () => { + const { onDismiss } = renderWithStreamInterrupt(true); + const composer = addComposer(); + + // An open popover, menu or edit mode: they call preventDefault or stop propagation. + const claim = (e: Event) => e.preventDefault(); + document.addEventListener("keydown", claim); + pressEscape(composer); + document.removeEventListener("keydown", claim); + const stop = (e: Event) => e.stopPropagation(); + document.addEventListener("keydown", stop); + pressEscape(composer); + document.removeEventListener("keydown", stop); + + // IME composition, modified Escape, a terminal, and an open modal dialog. + pressEscape(composer, { isComposing: true }); + pressEscape(composer, { ctrlKey: true, shiftKey: true }); + const terminal = document.createElement("div"); + terminal.setAttribute("data-terminal-container", ""); + document.body.appendChild(terminal); + pressEscape(terminal); + const modal = document.createElement("div"); + modal.setAttribute("role", "dialog"); + modal.setAttribute("aria-modal", "true"); + document.body.appendChild(modal); + pressEscape(composer); + + expect(onDismiss).not.toHaveBeenCalled(); + }); +}); diff --git a/src/browser/hooks/useEscapeToDismiss.ts b/src/browser/hooks/useEscapeToDismiss.ts new file mode 100644 index 00000000000..de67cf3c4e1 --- /dev/null +++ b/src/browser/hooks/useEscapeToDismiss.ts @@ -0,0 +1,68 @@ +import { useEffect, useLayoutEffect, useRef } from "react"; +import { + isDesktopViewportFocused, + isDialogOpen, + isTerminalFocused, + KEYBINDS, + matchesKeybind, +} from "@/browser/utils/ui/keybinds"; + +/** + * Close an overlay (the narrow-screen sidebar drawer) on Escape, but only when nothing else + * claimed the key. + * + * The overlay does not own focus, so Escape usually arrives from somewhere else, often the + * composer. A capture-phase listener took Escape from every popover, menu, dialog and edit mode + * (#5670 tried one for the tutorial and reverted it). This one runs in the window's bubble phase, + * after the open layers had their turn: + * - Radix popovers, menus and dialogs stop Escape in the document capture phase. + * - React handlers that call stopKeyboardPropagation stop it at the React root. + * - Document listeners (composer suggestions, the send-mode menu) call preventDefault. + * The stream interrupt (useAIViewKeybinds) listens in the same phase and mounts first, so it + * would see Escape before this listener. It asks isEscapeDismissOverlayOpen() instead and yields + * while an overlay is open: one Escape never does both. When no overlay is open, no listener + * exists and Escape behaves exactly as it does without this hook. + * + * Only one overlay uses this hook. Before a second one does, decide which of them answers + * Escape: listeners run in registration order, not in visual order. + */ +let openOverlayCount = 0; + +/** True while an overlay that closes on Escape is open (see useEscapeToDismiss). */ +export function isEscapeDismissOverlayOpen(): boolean { + return openOverlayCount > 0; +} + +export function useEscapeToDismiss(enabled: boolean, onDismiss: () => void): void { + // A ref keeps one subscription per open overlay. Re-subscribing on every render would move this + // listener behind later window listeners and change which one sees Escape first. + const onDismissRef = useRef(onDismiss); + useLayoutEffect(() => { + onDismissRef.current = onDismiss; + }, [onDismiss]); + + useEffect(() => { + if (!enabled) return; + + const handleKeyDown = (e: KeyboardEvent) => { + // KEYBINDS.CANCEL is bare Escape: modified Escape belongs to other shortcuts. + if (!matchesKeybind(e, KEYBINDS.CANCEL)) return; + // Something closer to the focus already handled Escape, or an IME is composing text. + if (e.defaultPrevented || e.isComposing) return; + // Terminals and remote desktops own their keyboard, Escape included. + if (isTerminalFocused(e.target) || isDesktopViewportFocused(e.target)) return; + // A modal above the overlay closes first. + if (isDialogOpen()) return; + + e.preventDefault(); + onDismissRef.current(); + }; + + openOverlayCount += 1; + window.addEventListener("keydown", handleKeyDown); + return () => { + openOverlayCount -= 1; + window.removeEventListener("keydown", handleKeyDown); + }; + }, [enabled]); +} diff --git a/src/node/services/agentSkills/builtInSkillContent.generated.ts b/src/node/services/agentSkills/builtInSkillContent.generated.ts index a945ee47b13..34a40158644 100644 --- a/src/node/services/agentSkills/builtInSkillContent.generated.ts +++ b/src/node/services/agentSkills/builtInSkillContent.generated.ts @@ -4768,7 +4768,7 @@ export const BUILTIN_SKILL_FILES: Record> = { "", "There are two ways to receive notifications:", "", - "1. **Automatic notifications** — Toggle the bell icon in the workspace header to get notified when the agent completes a response. Use Ctrl+Shift+, (⌘+Shift+, on macOS) to toggle quickly.", + "1. **Automatic notifications** — Click the bell icon in the workspace header and check **Notify on all responses** to get notified when the agent completes a response. Use Ctrl+Shift+, (⌘+Shift+, on macOS) to toggle quickly.", "", "2. **Agent-triggered notifications** — The `notify` tool lets agents send notifications for specific events. You control when agents use this through prompts or scoped instructions.", "", diff --git a/tests/bugbash/repros/creationFormForgetsName.e2e.ts b/tests/bugbash/repros/creationFormForgetsName.e2e.ts index 5cdad6ac428..6fb1c415c79 100644 --- a/tests/bugbash/repros/creationFormForgetsName.e2e.ts +++ b/tests/bugbash/repros/creationFormForgetsName.e2e.ts @@ -18,8 +18,8 @@ test( await name.fill("n3-repro"); await screen.getByRole("textbox", "Message").fill("create from the default form"); await screen.getByRole("button", "Send message").tap(); - // The new workspace opens; its menu bar shows the notifications toggle. - await expect(screen.getByRole("button", "Notify on all responses")).toBeVisible({ + // The new workspace opens; its menu bar shows the notifications button. + await expect(screen.getByRole("button", "Notifications")).toBeVisible({ timeout: 30_000, }); diff --git a/tests/bugbash/repros/escapeClosesOverlays.e2e.ts b/tests/bugbash/repros/escapeClosesOverlays.e2e.ts new file mode 100644 index 00000000000..56e6d624b49 --- /dev/null +++ b/tests/bugbash/repros/escapeClosesOverlays.e2e.ts @@ -0,0 +1,85 @@ +// #5676, #5685, #5691: Escape closes the agent picker, the narrow-screen sidebar drawer and the +// notifications popover (once). Each test fails on the old code. +import { test } from "@e2e-dev/web"; +import { expect } from "e2e"; +import { + armLayerProbe, + expectNotifyOnAllResponses, + NOTIFICATIONS_POPOVER, + openPlayground, + waitForLayerEscapeReady, + WORKSPACE_TITLE, +} from "./helpers"; + +test( + "Escape closes the agent picker opened with Ctrl+Shift+A", + { tags: ["bugbash", "5676"] }, + async ({ app, screen, browser }) => { + await browser.setViewport({ width: 1440, height: 900 }); + await openPlayground(app, screen, browser); + const picker = screen.getByRole("button", "Select agent"); + // The old frame-deferred focus missed on every other open, so try twice. + for (let attempt = 0; attempt < 2; attempt++) { + await screen.getByRole("textbox", "Message").tap(); + await browser.keyboard.press("Control+Shift+A"); + await expect(picker).toBeExpanded(); + await browser.keyboard.press("Escape"); + await expect(picker).not.toBeExpanded(); + } + } +); + +test( + "Escape closes the narrow-screen sidebar drawer", + { tags: ["bugbash", "5685"] }, + async ({ app, screen, browser }) => { + await browser.setViewport({ width: 390, height: 844 }); + await openPlayground(app, screen, browser); + const drawerBackdrop = browser.locator(".mobile-overlay"); + const openMenu = screen.getByRole("button", "Open sidebar menu"); + if (await openMenu.isVisible()) await openMenu.tap(); + await expect(drawerBackdrop).toHaveCount(1); + await expect( + screen.getByRole("navigation", "Projects").getByText(WORKSPACE_TITLE) + ).toBeVisible(); + + await browser.keyboard.press("Escape"); + await expect(drawerBackdrop).toHaveCount(0); + } +); + +test( + "A click on the notifications bell does not change the setting", + { tags: ["bugbash", "5691"] }, + async ({ app, screen, browser }) => { + await browser.setViewport({ width: 1440, height: 900 }); + await openPlayground(app, screen, browser); + // The helper clicks the bell, reads the checkbox and closes the popover. The click used to + // turn the setting on as well. + await expectNotifyOnAllResponses(screen, browser, false); + await expectNotifyOnAllResponses(screen, browser, false); + } +); + +test( + "One Escape closes the notifications popover without a second copy of the settings", + { tags: ["bugbash", "5691"] }, + async ({ app, screen, browser }) => { + await browser.setViewport({ width: 1440, height: 900 }); + await openPlayground(app, screen, browser); + const bell = screen.getByRole("button", "Notifications"); + await armLayerProbe(browser, NOTIFICATIONS_POPOVER); + await bell.tap(); + await expect(screen.getByRole("checkbox", /^Notify on all responses/)).toBeVisible(); + await waitForLayerEscapeReady(browser); + await browser.keyboard.press("Escape"); + // Radix returns focus to the bell, and its tooltip opens on that focus. The tooltip used to + // repeat the settings, checkbox included. + await expect(bell).toBeFocused(); + const layer = browser.locator("[data-radix-popper-content-wrapper]"); + await expect(layer).toBeVisible(); + await expect( + browser.locator("[data-radix-popper-content-wrapper] [role=checkbox]") + ).toHaveCount(0); + } +); diff --git a/tests/bugbash/repros/helpers.ts b/tests/bugbash/repros/helpers.ts index ae0d3e8e5fd..5968212fd63 100644 --- a/tests/bugbash/repros/helpers.ts +++ b/tests/bugbash/repros/helpers.ts @@ -53,6 +53,159 @@ export async function sendMessageForEdit( return edit; } +/** + * Selector of the notifications popover's content: the element that Radix registers as a + * DismissableLayer. + */ +export const NOTIFICATIONS_POPOVER = "[data-radix-popper-content-wrapper] [role=dialog]"; + +interface LayerProbe { + arm(selector: string): void; + state(): string; +} + +/** + * Runs in the page before the app loads (`addInitScript`). It tells the tests when a Radix + * DismissableLayer (popover, menu, dialog) is ready to take Escape. + * + * Why the tests need it: a layer ignores Escape for a short time after it opens. Radix 1.1 + * registers the layer in an effect (`context.layers.add(node)`), then sends + * `dismissableLayer.update`. Every layer re-renders on that event (`force({})`) and only then + * computes its true `index`. Its Escape listener reads the handler that the last committed render + * created, and a passive effect (`useCallbackRef`) installs that handler. Until that re-render has + * committed and its passive effects have run, `index` is -1 and Escape does nothing. CI's runner + * sends Escape fast enough to land in that window. A person cannot. + * + * The probe proves each step instead of guessing a delay: + * 1. `arm(selector)` is called before the open. + * 2. On `dismissableLayer.update`, the probe checks that `context.layers` now holds the element + * and records the layer's force state (its second `useState`). + * 3. React calls `onPostCommitFiberRoot` of the DevTools hook after a commit's passive effects + * have run. When the force state differs from the recorded one there, a render after the + * registration has committed and its handler is installed: the state is "ready". + * The probe reads React internals (fiber, hook list) and checks their shape. If Radix or React + * changes them, `state()` reports an error, and the test fails loudly instead of racing. + */ +function layerProbeInit(): void { + interface Hook { + memoizedState: unknown; + next: Hook | null; + } + interface Fiber { + type: { displayName?: string } | null; + return: Fiber | null; + alternate: Fiber | null; + memoizedState: Hook | null; + dependencies: { firstContext: { memoizedValue: unknown } | null } | null; + } + interface Armed { + selector: string; + // Force state at registration. `undefined` until the layer has registered. + registeredForceState?: unknown; + ready: boolean; + error?: string; + } + let armed: Armed | null = null; + + const fiberOf = (el: Element): Fiber | null => { + const key = Object.keys(el).find((k) => k.startsWith("__reactFiber$")); + return key ? ((el as unknown as Record)[key] ?? null) : null; + }; + const layerFiberOf = (el: Element): Fiber | null => { + let fiber = fiberOf(el); + while (fiber && fiber.type?.displayName !== "DismissableLayer") fiber = fiber.return; + return fiber; + }; + const forceState = (fiber: Fiber | null): unknown => fiber?.memoizedState?.next?.memoizedState; + + document.addEventListener("dismissableLayer.update", () => { + if (!armed || armed.ready || armed.error || armed.registeredForceState !== undefined) return; + const el = document.querySelector(armed.selector); + if (!el) return; + const found = layerFiberOf(el); + if (!found) { + armed.error = "probe: no DismissableLayer fiber above the element (Radix changed?)"; + return; + } + // React keeps two fibers per component. Hook 1 is `node`: the fiber that has rendered with + // `node` set to this element is the newer one. Before that render the layer cannot have + // registered, so an update event then comes from another layer. + const fiber = [found, found.alternate].find((f) => f?.memoizedState?.memoizedState === el); + if (!fiber) return; + const context = fiber.dependencies?.firstContext?.memoizedValue as + | { layers?: unknown } + | undefined; + // Hook 2 is the force state, an object. + const force = forceState(fiber); + if (!(context?.layers instanceof Set) || typeof force !== "object" || force === null) { + armed.error = "probe: DismissableLayer context or force state changed shape (Radix changed?)"; + return; + } + // This event can come from another layer; only this element's registration counts. + if (!context.layers.has(el)) return; + armed.registeredForceState = force; + }); + + ( + window as unknown as { __REACT_DEVTOOLS_GLOBAL_HOOK__: unknown } + ).__REACT_DEVTOOLS_GLOBAL_HOOK__ = { + supportsFiber: true, + renderers: new Map(), + inject: () => 1, + checkDCE: () => undefined, + onScheduleFiberRoot: () => undefined, + onCommitFiberRoot: () => undefined, + onCommitFiberUnmount: () => undefined, + onPostCommitFiberRoot: () => { + if (!armed || armed.ready || armed.registeredForceState === undefined) return; + const el = document.querySelector(armed.selector); + const fiber = el ? layerFiberOf(el) : null; + if (!fiber) return; + // React keeps two fibers per component, and only one is committed. The other still holds + // the older state, so a change on either one means a newer render committed. + const before = armed.registeredForceState; + if (forceState(fiber) !== before || forceState(fiber.alternate) !== before) { + armed.ready = true; + } + }, + }; + + const probe: LayerProbe = { + arm: (selector) => { + armed = { selector, ready: false }; + }, + state: () => { + if (!armed) return "not armed: call armLayerProbe before the open"; + if (armed.error) return armed.error; + if (armed.ready) return "ready"; + return armed.registeredForceState === undefined ? "not registered" : "registered"; + }, + }; + (window as unknown as { __layerProbe: LayerProbe }).__layerProbe = probe; +} + +/** Call before the next open of the layer that `selector` matches. */ +export async function armLayerProbe(browser: Browser, selector: string): Promise { + await browser.evaluate((s: string) => { + (window as unknown as { __layerProbe: LayerProbe }).__layerProbe.arm(s); + return null; + }, selector); +} + +/** + * Waits until the armed layer takes Escape (see layerProbeInit). The caller then presses Escape + * once, as a person does. + */ +export async function waitForLayerEscapeReady(browser: Browser): Promise { + await expect + .poll(() => + browser.evaluate(() => + (window as unknown as { __layerProbe: LayerProbe }).__layerProbe.state() + ) + ) + .toBe("ready"); +} + /** Opens the app with tutorials off and selects the seeded workspace. */ export async function openPlayground( app: { open(path?: string): Promise }, @@ -60,6 +213,7 @@ export async function openPlayground( browser: Browser ): Promise { await disableTutorials(browser); + await browser.addInitScript(layerProbeInit); await app.open(); // A fresh context starts with the project collapsed in the sidebar. const expand = screen.getByRole("button", "Expand project demo-app"); @@ -72,7 +226,30 @@ export async function openPlayground( } if (await expand.isVisible()) await expand.tap(); await screen.getByText(WORKSPACE_TITLE).first().tap(); - await expect(screen.getByRole("button", "Notify on all responses")).toBeVisible({ + await expect(screen.getByRole("button", "Notifications")).toBeVisible({ timeout: 15_000, }); } + +/** + * Asserts the "Notify on all responses" setting: opens the bell's settings popover, reads the + * checkbox, and closes it with Escape. A click on the bell only opens the popover (#5691). + */ +export async function expectNotifyOnAllResponses( + screen: Screen, + browser: Browser, + checked: boolean +): Promise { + await armLayerProbe(browser, NOTIFICATIONS_POPOVER); + await screen.getByRole("button", "Notifications").tap(); + const setting = screen.getByRole("checkbox", /^Notify on all responses/); + await expect(setting).toBeVisible(); + if (checked) { + await expect(setting).toBeChecked(); + } else { + await expect(setting).not.toBeChecked(); + } + await waitForLayerEscapeReady(browser); + await browser.keyboard.press("Escape"); + await expect(setting).toBeHidden(); +} diff --git a/tests/bugbash/repros/knownFailureDetailsShortcuts.e2e.ts b/tests/bugbash/repros/knownFailureDetailsShortcuts.e2e.ts index b0d7ae5c165..b91b496da3d 100644 --- a/tests/bugbash/repros/knownFailureDetailsShortcuts.e2e.ts +++ b/tests/bugbash/repros/knownFailureDetailsShortcuts.e2e.ts @@ -2,7 +2,7 @@ // details button, which stops every key, so global shortcuts do nothing. Fails until #5671 is fixed. import { test } from "@e2e-dev/web"; import { expect } from "e2e"; -import { openPlayground } from "./helpers"; +import { expectNotifyOnAllResponses, openPlayground } from "./helpers"; test( "global shortcuts still work after the Workspace details popover closes", @@ -12,9 +12,8 @@ test( // desktop keyboard flow. await browser.setViewport({ width: 1440, height: 900 }); await openPlayground(app, screen, browser); - const bell = screen.getByRole("button", "Notify on all responses"); - await expect(bell).toHaveAttribute("aria-pressed", "false"); - + // A fresh workspace starts with "Notify on all responses" off. The check opens the bell's + // popover, so it runs only at the end: it would move focus away from the details button. await screen.getByRole("textbox", "Message").tap(); await browser.keyboard.press("Control+Shift+D"); const details = screen.getByRole("dialog"); @@ -26,6 +25,6 @@ test( // Any global shortcut shows it; this one toggles notifications (see notificationsShortcut). await browser.keyboard.press("Control+Shift+Comma"); - await expect(bell).toHaveAttribute("aria-pressed", "true"); + await expectNotifyOnAllResponses(screen, browser, true); } ); diff --git a/tests/bugbash/repros/notificationsShortcut.e2e.ts b/tests/bugbash/repros/notificationsShortcut.e2e.ts index 246d2c7dedb..7735bdbea85 100644 --- a/tests/bugbash/repros/notificationsShortcut.e2e.ts +++ b/tests/bugbash/repros/notificationsShortcut.e2e.ts @@ -1,22 +1,22 @@ // N5 (#5670): Ctrl+Shift+N was bound to both "New scratch chat" and "Toggle notifications". // Notifications moved to Ctrl/Cmd+Shift+Comma; this repro fails if the shortcut stops toggling it. import { test } from "@e2e-dev/web"; -import { expect } from "e2e"; -import { openPlayground } from "./helpers"; +import { expectNotifyOnAllResponses, openPlayground } from "./helpers"; test( "Ctrl+Shift+Comma toggles notifications in a workspace", { tags: ["bugbash", "N5"] }, async ({ app, screen, browser }) => { await openPlayground(app, screen, browser); - const bell = screen.getByRole("button", "Notify on all responses"); - await expect(bell).toHaveAttribute("aria-pressed", "false"); + await expectNotifyOnAllResponses(screen, browser, false); - await screen.getByRole("textbox", "Message").tap(); + const composer = screen.getByRole("textbox", "Message"); + await composer.tap(); await browser.keyboard.press("Control+Shift+Comma"); - await expect(bell).toHaveAttribute("aria-pressed", "true"); + await expectNotifyOnAllResponses(screen, browser, true); + await composer.tap(); await browser.keyboard.press("Control+Shift+Comma"); - await expect(bell).toHaveAttribute("aria-pressed", "false"); + await expectNotifyOnAllResponses(screen, browser, false); } ); diff --git a/tests/ui/compaction/compaction.test.ts b/tests/ui/compaction/compaction.test.ts index ee82e66b844..248501971b2 100644 --- a/tests/ui/compaction/compaction.test.ts +++ b/tests/ui/compaction/compaction.test.ts @@ -447,12 +447,12 @@ describe("Auto-follow-up and compaction notification behavior (mock AI router)", globalThis as { Notification: unknown } ).Notification; - // Enable notifications via UI (click bell button in workspace header) - const notifyButton = app.view.container.querySelector( - '[data-testid="notify-on-response-button"]' - ); - if (!notifyButton) throw new Error("Notify button not found"); - fireEvent.click(notifyButton); + // Enable notifications with the toggle shortcut. The bell only opens the settings popover, + // and Radix popover content does not render in happy-dom. + if (!app.view.container.querySelector('[data-testid="notify-on-response-button"]')) { + throw new Error("Notify button not found"); + } + fireEvent.keyDown(window, { key: ",", code: "Comma", ctrlKey: true, shiftKey: true }); // Send seed message and wait for notification await app.chat.send(seedMessage);