From f46055dcfaf2a0e3fbfc44b722145d294e05099f Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 17:19:46 -0700 Subject: [PATCH] fix(desktop): release Escape from a cover surface that is animating out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** fix Replacing one cover drawer with another swallowed the next Escape, so leaving the drawer that just arrived took two presses. `AnimatePresence` holds the replaced surface mounted through its exit animation (~210ms measured), and for that whole window two surfaces are listening for the key — with the outgoing one, registered first, winning it. Both layers had to stand down, which is why fixing one was not enough: - `CoverDrawer`'s capture-phase claim consumes the press with `stopImmediatePropagation`, invisibly to the successor. - `useEscapeKey` ignores an event that is already `defaultPrevented`, so an exiting *panel*'s `preventDefault` swallows the press from its successor's panel just as completely. This is the path agent activity actually takes: it sets `ownsEscape={false}` and routes the key through its panel, so no `CoverDrawer` Escape code runs at all. Both now gate on `useIsPresent`. Presence rather than the focus slot is deliberate: the slot is claimed only by a surface that captures focus, and a successor whose content takes focus instead never claims it — leaving the outgoing claim current, so a slot check passes for exactly the surface that must stand down. That version was tried and falsified in the browser. Outside `AnimatePresence` there is no presence context and `useIsPresent` is `true`, so the many panels that never animate out are unaffected; `useEscapeKey` also keeps its `escapeSurfaces` registration for the exit's duration, so background shortcuts still yield. Tests, each falsified by removing the guard it covers rather than assumed: two jsdom cases drive the real primitives under a real `AnimatePresence` with a keyed child swap, one per layer, asserting the overlap is live before pressing; a third pins the no-presence-context case. An e2e case presses once inside the window, reaching activity by navigation so no dismissable layer of the test's own can absorb it. Removing either guard fails exactly the new coverage. The visual half of the overlap — both panels cross-fading in the same window — is unchanged here and stays open in #6734 as a design question. Signed-off-by: ss-dev-00 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- .../src/features/channels/ui/CoverDrawer.tsx | 37 ++- .../channels/ui/CoverDrawerEscape.test.mjs | 267 ++++++++++++++++++ desktop/src/shared/hooks/useEscapeKey.ts | 17 ++ .../tests/e2e/agent-activity-cover.spec.ts | 89 ++++++ 4 files changed, 408 insertions(+), 2 deletions(-) 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, }) => {