diff --git a/desktop/src/features/channels/ui/CoverDrawer.tsx b/desktop/src/features/channels/ui/CoverDrawer.tsx index 4f775f89bc..3d4f085e26 100644 --- a/desktop/src/features/channels/ui/CoverDrawer.tsx +++ b/desktop/src/features/channels/ui/CoverDrawer.tsx @@ -1,4 +1,4 @@ -import { motion, useReducedMotion } from "motion/react"; +import { motion, useIsPresent, useReducedMotion } from "motion/react"; import * as React from "react"; import { @@ -181,6 +181,16 @@ export function CoverDrawer({ testId, }: CoverDrawerProps) { const prefersReducedMotion = useReducedMotion(); + /** + * False from the moment `AnimatePresence` starts this drawer's exit. + * + * The covered slot belongs to the drawer that is arriving or settled, not to + * one that is animating away, and this is the only signal that distinguishes + * them — the focus slot cannot, because a drawer that never captures focus + * (its content may take it instead) leaves the outgoing drawer's claim + * current. See the Escape handler. + */ + const isPresent = useIsPresent(); const travelPx = prefersReducedMotion ? 0 : COVER_DRAWER_TRAVEL_PX; const drawerRef = React.useRef(null); const previousFocusRef = React.useRef(null); @@ -195,6 +205,29 @@ export function CoverDrawer({ React.useEffect(() => { if (!ownsEscape) return; + // Stand down for the whole exit: a drawer on its way out does not own the + // covered slot, so the key belongs to whatever replaced it. + // + // `AnimatePresence` keeps a replaced drawer mounted through its exit + // animation, so during a replacement two drawers have this listener + // installed at once, and capture-phase listeners on the same target fire in + // registration order — the outgoing one registered first, so it would + // otherwise always win. It then consumes the press via + // `stopImmediatePropagation`, which is invisible to the successor, and the + // user has to press Escape twice to leave the drawer that just arrived. + // + // `useEscapeKey` carries the same guard for the same reason. This one is not + // sufficient on its own: the agent activity drawer sets `ownsEscape={false}` + // and routes the key through its panel, so on that path no code here runs + // and it is the exiting *panel*'s `preventDefault` that swallows the press. + // + // Gating on presence rather than the focus slot is deliberate. The focus + // slot is claimed only by a drawer that captures focus, and a successor + // whose content takes focus instead never claims it — which leaves the + // outgoing drawer's claim current and makes a slot check pass for exactly + // the drawer that should stand down. Presence is the state that actually + // distinguishes arriving from leaving. + if (!isPresent) return; function handleEscape(event: KeyboardEvent) { if (event.key !== "Escape") return; @@ -219,7 +252,7 @@ export function CoverDrawer({ return () => { window.removeEventListener("keydown", handleEscape, { capture: true }); }; - }, [escapeYieldsToContent, onClose, ownsEscape]); + }, [escapeYieldsToContent, isPresent, onClose, ownsEscape]); React.useLayoutEffect(() => { // Capture the opener exactly once per drawer instance. diff --git a/desktop/src/features/channels/ui/CoverDrawerEscape.test.mjs b/desktop/src/features/channels/ui/CoverDrawerEscape.test.mjs index 5f8132f2cb..e164317ca9 100644 --- a/desktop/src/features/channels/ui/CoverDrawerEscape.test.mjs +++ b/desktop/src/features/channels/ui/CoverDrawerEscape.test.mjs @@ -67,6 +67,83 @@ async function renderDrawer({ escapeYieldsToContent }) { return { closes, view }; } +/** + * Renders the replacement window the way `ChannelPane` produces it: one + * `AnimatePresence` whose keyed child is swapped, so the outgoing drawer stays + * mounted in its exit phase while the successor mounts alongside it. + * + * Both are the real primitive, and the presence wrapper is real too, because the + * bug lives in the interaction between two instances under `AnimatePresence` — + * a harness that renders them as two independent trees reports both as present + * and cannot see it. + */ +async function renderReplacement({ successorOwnsEscape }) { + const React = await import("react"); + const { act, render } = await import("@testing-library/react"); + const { AnimatePresence } = await import("motion/react"); + const { CoverDrawer } = await import("./CoverDrawer.tsx"); + + const events = []; + + // Stands in for the agent session panel's own `useEscapeKey`: the successor + // drawer does not claim the key, its content handles it. + function SuccessorContent() { + React.useEffect(() => { + function onKeyDown(event) { + if (event.key === "Escape") events.push("successor-content-escape"); + } + window.addEventListener("keydown", onKeyDown); + return () => window.removeEventListener("keydown", onKeyDown); + }, []); + return React.createElement("div", { "data-testid": "successor-content" }); + } + + const outgoing = React.createElement( + CoverDrawer, + { + ariaLabel: "Thread", + key: "outgoing", + onClose: () => events.push("outgoing-close"), + scrimLabel: "Back to #general", + testId: "outgoing-drawer", + }, + React.createElement("div", null, "thread"), + ); + const successor = React.createElement( + CoverDrawer, + { + ariaLabel: "Agent activity", + key: "successor", + onClose: () => events.push("successor-close"), + ownsEscape: successorOwnsEscape, + scrimLabel: "Back to #general", + testId: "successor-drawer", + }, + React.createElement(SuccessorContent), + ); + + const view = render(React.createElement(AnimatePresence, null, outgoing)); + await act(async () => { + view.rerender(React.createElement(AnimatePresence, null, successor)); + }); + + // Both are mounted: the outgoing drawer is held through its exit animation. + // This is the ~210ms window an rAF probe measures in the browser, and it is + // the precondition for the assertions below — without it they prove nothing. + assert.equal( + dom.window.document.querySelectorAll('[data-testid="outgoing-drawer"]') + .length, + 1, + ); + assert.equal( + dom.window.document.querySelectorAll('[data-testid="successor-drawer"]') + .length, + 1, + ); + + return { events, view }; +} + function pressEscapeOn(element) { element.dispatchEvent( new dom.window.KeyboardEvent("keydown", { @@ -77,6 +154,85 @@ function pressEscapeOn(element) { ); } +/** + * Renders the replacement window in the shape production actually has it: each + * drawer holds a panel using the real `useEscapeKey`, which is how both the + * thread and the agent session panels take the key. + * + * The synthetic harness above cannot see the second half of this bug. Its + * successor listener acts on every press, but `useEscapeKey` deliberately + * ignores an event that is already `defaultPrevented` — so an exiting *panel* + * that still calls `preventDefault` swallows the press from a real successor + * just as thoroughly as the exiting drawer's `stopImmediatePropagation` does, + * one layer further down and with no cover-drawer code in the path. + */ +async function renderPanelReplacement() { + const React = await import("react"); + const { act, render } = await import("@testing-library/react"); + const { AnimatePresence } = await import("motion/react"); + const { CoverDrawer } = await import("./CoverDrawer.tsx"); + const { useEscapeKey } = await import("@/shared/hooks/useEscapeKey.ts"); + + const events = []; + + function Panel({ label, testId }) { + useEscapeKey( + React.useCallback(() => events.push(label), [label]), + true, + ); + return React.createElement("div", { "data-testid": testId }); + } + + // The thread drawer claims Escape itself; activity leaves it to its panel. + const outgoing = React.createElement( + CoverDrawer, + { + ariaLabel: "Thread", + key: "outgoing", + onClose: () => events.push("outgoing-drawer-close"), + scrimLabel: "Back to #general", + testId: "outgoing-drawer", + }, + React.createElement(Panel, { + label: "outgoing-panel-escape", + testId: "outgoing-panel", + }), + ); + const successor = React.createElement( + CoverDrawer, + { + ariaLabel: "Agent activity", + key: "successor", + onClose: () => events.push("successor-drawer-close"), + ownsEscape: false, + scrimLabel: "Back to #general", + testId: "successor-drawer", + }, + React.createElement(Panel, { + label: "successor-panel-escape", + testId: "successor-panel", + }), + ); + + const view = render(React.createElement(AnimatePresence, null, outgoing)); + await act(async () => { + view.rerender(React.createElement(AnimatePresence, null, successor)); + }); + + assert.equal( + dom.window.document.querySelectorAll('[data-testid="outgoing-drawer"]') + .length, + 1, + ); + assert.equal( + dom.window.document.querySelectorAll('[data-testid="successor-panel"]') + .length, + 1, + ); + + return { events, view }; +} + function composer() { return dom.window.document.querySelector('[data-testid="thread-composer"]'); } @@ -112,3 +268,114 @@ test("Escape from outside the drawer closes it even while content owns the key", assert.deepEqual(closes, ["close"]); }); + +test("Escape during a replacement reaches the successor, not the exiting drawer", async () => { + // The bug this guards: `AnimatePresence` holds the outgoing drawer mounted + // through its exit animation (~210ms), and its capture-phase listener calls + // `stopImmediatePropagation()`. A successor that does not claim Escape — the + // agent activity drawer, which routes the key through its panel's own + // `useEscapeKey` — therefore never sees the press, so the user has to press + // Escape twice to leave a drawer that just replaced another. + const { events, view } = await renderReplacement({ + successorOwnsEscape: false, + }); + + pressEscapeOn( + dom.window.document.querySelector('[data-testid="successor-content"]'), + ); + + // The press belongs to the drawer holding the covered slot. The exiting + // drawer is on its way out and must not act on it, let alone consume it. + assert.deepEqual(events, ["successor-content-escape"]); + + view.unmount(); +}); + +test("a claiming successor closes on a single Escape during a replacement", async () => { + // The same window, with a successor that does claim the key (thread over + // activity): exactly one drawer may act, and it must be the new one. + const { events, view } = await renderReplacement({ + successorOwnsEscape: true, + }); + + pressEscapeOn( + dom.window.document.querySelector('[data-testid="successor-content"]'), + ); + + assert.deepEqual(events, ["successor-close"]); + + view.unmount(); +}); + +test("Escape during a panel replacement reaches the successor's panel", async () => { + // The other half of the same bug, one layer down and with no cover-drawer + // code in the path: `useEscapeKey` ignores an event that is already + // `defaultPrevented`, so an exiting panel that still calls `preventDefault` + // silently consumes the press from its successor's panel. This is the path the + // agent activity drawer actually takes — it sets `ownsEscape={false}` and lets + // its panel handle the key — so fixing only the drawer's claim leaves the + // two-press bug in place. + const { events, view } = await renderPanelReplacement(); + + pressEscapeOn( + dom.window.document.querySelector('[data-testid="successor-panel"]'), + ); + + // Exactly one handler acts, and it belongs to the arriving surface. + assert.deepEqual(events, ["successor-panel-escape"]); + + view.unmount(); +}); + +test("a lone panel still closes on Escape outside AnimatePresence", async () => { + // `useEscapeKey` is used by panels that never animate out (split pane, + // single-panel thread, profile). With no `AnimatePresence` above them there is + // no presence context at all, and the guard must read as present rather than + // as "not exiting yet" — otherwise it would disable Escape for every one of + // those surfaces. + const React = await import("react"); + const { render } = await import("@testing-library/react"); + const { useEscapeKey } = await import("@/shared/hooks/useEscapeKey.ts"); + + const closes = []; + function Panel() { + useEscapeKey(() => closes.push("close"), true); + return React.createElement("input", { "data-testid": "thread-composer" }); + } + render(React.createElement(Panel)); + + pressEscapeOn(composer()); + + assert.deepEqual(closes, ["close"]); +}); + +test("a lone drawer still closes on Escape under StrictMode", async () => { + // The slot claim is taken in a layout effect, which `React.StrictMode` replays + // as setup → cleanup → setup. Each setup takes a *new* generation, so the + // guard above must be reading whatever the last setup stored rather than a + // stale claim from the discarded first pass — otherwise every drawer in + // development would ignore Escape entirely. The focus tests cannot see this: + // they assert on restore, which the coordinator handles separately. + const React = await import("react"); + const { render } = await import("@testing-library/react"); + const { CoverDrawer } = await import("./CoverDrawer.tsx"); + + const closes = []; + render( + React.createElement( + CoverDrawer, + { + ariaLabel: "Thread", + onClose: () => closes.push("close"), + scrimLabel: "Back to #general", + testId: "strict-drawer", + }, + React.createElement("input", { "data-testid": "thread-composer" }), + ), + { reactStrictMode: true }, + ); + + pressEscapeOn(composer()); + + assert.deepEqual(closes, ["close"]); +}); diff --git a/desktop/src/shared/hooks/useEscapeKey.ts b/desktop/src/shared/hooks/useEscapeKey.ts index 0422002746..fe51389fdb 100644 --- a/desktop/src/shared/hooks/useEscapeKey.ts +++ b/desktop/src/shared/hooks/useEscapeKey.ts @@ -1,3 +1,4 @@ +import { useIsPresent } from "motion/react"; import * as React from "react"; import { acquireEscapeSurface } from "@/shared/hooks/escapeSurfaces"; @@ -11,13 +12,29 @@ import { acquireEscapeSurface } from "@/shared/hooks/escapeSurfaces"; * app-level Escape shortcuts (mark channel read) know to yield instead * of racing this listener on registration order. * + * A surface animating out under `AnimatePresence` stands down: it stays + * registered (so background shortcuts keep yielding for the duration) but no + * longer acts on the key. `AnimatePresence` keeps a replaced surface mounted + * through its exit, so during a replacement two surfaces listen at once, and + * the outgoing one registered first — it would mark the press + * `defaultPrevented` and its successor, respecting exactly that flag, would + * ignore it. The user sees a swallowed keypress and has to press Escape twice. + * Outside `AnimatePresence` there is no presence context and this is always + * true, so surfaces that never animate out are unaffected. + * * Pass `enabled: false` to skip registering the listener entirely. */ export function useEscapeKey(onEscape: () => void, enabled: boolean = true) { + // Read through a ref rather than an effect dependency so entering the exit + // phase does not release and re-acquire the surface registration. + const isPresentRef = React.useRef(true); + isPresentRef.current = useIsPresent(); + React.useEffect(() => { if (!enabled) return; const releaseSurface = acquireEscapeSurface(); function handleKeyDown(event: KeyboardEvent) { + if (!isPresentRef.current) return; if (event.key === "Escape" && !event.defaultPrevented) { event.preventDefault(); onEscape(); diff --git a/desktop/tests/e2e/agent-activity-cover.spec.ts b/desktop/tests/e2e/agent-activity-cover.spec.ts index 214ff9799c..bdab356528 100644 --- a/desktop/tests/e2e/agent-activity-cover.spec.ts +++ b/desktop/tests/e2e/agent-activity-cover.spec.ts @@ -192,6 +192,36 @@ async function openActivityFromThreadComposer( await item.click({ force: true }); } +/** + * Opens activity over a covering thread by URL param. + * + * Reaches the same open handler as the thread-composer ingress, but through a + * navigation rather than a Radix popover — so the page holds no dismissable + * layer of its own and a press during the overlap can be attributed to the + * cover surfaces alone. + */ +async function openActivityByParam(page: Page, channelId: string) { + await page.evaluate( + ({ currentChannelId, pubkey }) => { + const hash = window.location.hash.replace(/^#/, "") || "/"; + const [path, query = ""] = hash.split("?"); + const params = new URLSearchParams(query); + params.delete("messageId"); + params.delete("thread"); + params.set("agentSession", pubkey); + params.set("agentSessionChannel", currentChannelId); + window.history.pushState( + {}, + "", + `${window.location.pathname}#${path}?${params.toString()}`, + ); + window.dispatchEvent(new HashChangeEvent("hashchange")); + window.dispatchEvent(new PopStateEvent("popstate")); + }, + { currentChannelId: channelId, pubkey: AGENT_PUBKEY }, + ); +} + /** * The default mock bridge already seeds alice as an agent in `#agents`, which * is what makes her eligible for the composer activity bar once she types. @@ -316,6 +346,65 @@ test("cover drawers replace each other in both directions without stacking", asy await expectFocusInside(page, "agent-activity-drawer"); }); +test("a single Escape closes the drawer that replaced another", async ({ + page, +}) => { + // `AnimatePresence` holds the replaced drawer mounted through its exit + // animation, so for a short window (~210ms measured) two surfaces are + // listening for Escape at once. The exiting one must stand down at both + // layers: the drawer's own capture-phase claim would consume the press with + // `stopImmediatePropagation`, and — on activity's path, where the drawer sets + // `ownsEscape={false}` and the panel handles the key — the exiting panel's + // `preventDefault` would swallow it just as completely, since `useEscapeKey` + // ignores an already-`defaultPrevented` event. Either one alone forces a + // second press. Deliberately does not wait for the overlap to settle; that + // wait is what makes the other tests here blind to this. + await page.setViewportSize(WIDE_VIEWPORT); + await page.addInitScript(() => { + localStorage.setItem("buzz.channels.threadViewMode", "focus"); + }); + await page.goto("/"); + const rootId = await seedThreadRoot(page); + // Both ingresses here are navigations, so the page never holds a Radix layer + // of its own — the composer ingress opens a popover that would legitimately + // own the next Escape, and it clears at the same time as the outgoing drawer + // (both ~233ms measured), leaving no moment where the overlap is live and the + // popover is gone. Keeping the page free of dismissable layers is what lets + // the press be attributed to the cover surfaces alone. + await page.getByTestId("channel-agents").click(); + await expect(page.getByTestId("chat-title")).toHaveText("agents"); + const channelId = await page.evaluate(() => { + const hash = window.location.hash.replace(/^#/, ""); + return hash.split("?")[0].split("/").pop() ?? ""; + }); + await openThreadByMessageLink(page, rootId); + await expect(page.getByTestId("focus-thread-drawer")).toBeVisible(); + + // Activity replaces the thread. The successor is up, and the outgoing thread + // drawer is still mounted mid-exit. + await openActivityByParam(page, channelId); + await expect(page.getByTestId("agent-activity-drawer")).toBeVisible(); + expect( + await page.getByTestId("focus-thread-drawer-overlay").count(), + ).toBeGreaterThan(0); + // Nothing but a cover drawer can absorb the press below. + await expect(page.locator("[data-radix-popper-content-wrapper]")).toHaveCount( + 0, + ); + + // One press, inside that window, and activity is gone. + await page.keyboard.press("Escape"); + await expect(page.getByTestId("agent-activity-drawer-overlay")).toHaveCount( + 0, + ); + await expect(page.getByTestId("agent-session-thread-panel")).toHaveCount(0); + await expect(page.getByTestId("channel-drop-zone")).not.toHaveAttribute( + "inert", + "", + ); + await expect(page).not.toHaveURL(/agentSession=/); +}); + test("narrow viewports keep the existing activity presentation", async ({ page, }) => {