diff --git a/desktop/src/features/channels/lib/channelAuxiliarySurface.ts b/desktop/src/features/channels/lib/channelAuxiliarySurface.ts index 7b419e5aa3..c461146ca6 100644 --- a/desktop/src/features/channels/lib/channelAuxiliarySurface.ts +++ b/desktop/src/features/channels/lib/channelAuxiliarySurface.ts @@ -3,10 +3,21 @@ import type { ThreadViewMode } from "@/features/channels/lib/threadViewModePrefe /** * The one auxiliary surface a channel shows beside (or over) its timeline. * - * Exactly one at a time, in this priority order. The open handlers in - * `useChannelAgentSessions` / `useChannelProfilePanel` already clear the other - * panels' state so last-opened wins; this resolution is what guarantees that a - * lingering search param or a race can still only ever produce one surface. + * Exactly one at a time, in the fixed priority order below. + * + * This priority is a **safety net, not the product rule.** Last-opened-wins is + * implemented by the open handlers, which clear the competing state as they open + * (`useChannelAgentSessions`: `openAgentSession` clears the thread head, and + * `openThreadAndCloseAgentSession` clears the agent session). By the time this + * resolver runs, at most one candidate should normally be live. + * + * The ordering only decides cases the handlers cannot: two candidates present at + * once with no ordering information between them — a restored/hand-edited URL + * carrying both `agentSession` and a thread param, or a stale param that has not + * been reconciled yet. Then it picks deterministically instead of rendering two + * surfaces. Do not read priority as "thread beats agent" in the UX; a thread + * opened while activity is up wins because the handler cleared the agent + * session, and activity opened over a thread wins for the same reason. */ export type ChannelAuxiliarySurface = | "agent-session" diff --git a/desktop/src/features/channels/lib/coverDrawerFocusSlot.test.mjs b/desktop/src/features/channels/lib/coverDrawerFocusSlot.test.mjs new file mode 100644 index 0000000000..33dee31be3 --- /dev/null +++ b/desktop/src/features/channels/lib/coverDrawerFocusSlot.test.mjs @@ -0,0 +1,65 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + claimCoverDrawerFocus, + hasCoverDrawerFocusClaim, + releaseCoverDrawerFocus, +} from "./coverDrawerFocusSlot.ts"; + +test("a fresh claim holds the slot", () => { + const claim = claimCoverDrawerFocus(); + + assert.equal(hasCoverDrawerFocusClaim(claim), true); +}); + +test("a successor's claim supersedes the outgoing drawer's", () => { + // The replacement case: the outgoing drawer's restore is deferred a frame, + // and by the time it runs the incoming drawer has claimed and taken focus. + const outgoing = claimCoverDrawerFocus(); + const incoming = claimCoverDrawerFocus(); + + assert.equal(hasCoverDrawerFocusClaim(outgoing), false); + assert.equal(hasCoverDrawerFocusClaim(incoming), true); +}); + +test("only the newest claim holds the slot across a chain of replacements", () => { + const claims = [ + claimCoverDrawerFocus(), + claimCoverDrawerFocus(), + claimCoverDrawerFocus(), + ]; + + const newest = claims.at(-1); + for (const claim of claims.slice(0, -1)) { + assert.equal(hasCoverDrawerFocusClaim(claim), false); + } + assert.equal(hasCoverDrawerFocusClaim(newest), true); +}); + +test("releasing invalidates the outstanding claim without granting a new one", () => { + // The view-mode switch case: nothing replaces the drawer, but the caller has + // already placed focus, so the drawer's own restore must not fire. + const claim = claimCoverDrawerFocus(); + releaseCoverDrawerFocus(); + + assert.equal(hasCoverDrawerFocusClaim(claim), false); +}); + +test("a claim taken after a release holds the slot again", () => { + releaseCoverDrawerFocus(); + const claim = claimCoverDrawerFocus(); + + assert.equal(hasCoverDrawerFocusClaim(claim), true); +}); + +test("claims are never reused, so a stale claim cannot alias a live one", () => { + const first = claimCoverDrawerFocus(); + releaseCoverDrawerFocus(); + claimCoverDrawerFocus(); + releaseCoverDrawerFocus(); + const later = claimCoverDrawerFocus(); + + assert.notEqual(first, later); + assert.equal(hasCoverDrawerFocusClaim(first), false); +}); diff --git a/desktop/src/features/channels/lib/coverDrawerFocusSlot.ts b/desktop/src/features/channels/lib/coverDrawerFocusSlot.ts new file mode 100644 index 0000000000..21a7c802af --- /dev/null +++ b/desktop/src/features/channels/lib/coverDrawerFocusSlot.ts @@ -0,0 +1,52 @@ +/** + * Single-slot coordinator for cover drawer focus restoration. + * + * There is one covered slot in a channel, so there is one focus claim. A drawer + * takes a claim when it captures focus and checks it back at teardown: it hands + * focus to whatever it stole it from only if its claim is still the current one. + * + * This exists because restoration is deferred a frame (the drawer has to let the + * exit animation start before moving focus), and a lot can happen in that frame. + * When one drawer replaces another the successor mounts and focuses itself while + * the outgoing one is still animating out, so an unconditional restore would + * yank focus out of the new drawer and into the channel that is now inert — + * unreachable by keyboard, with no visible focus ring anywhere. + * + * A monotonic generation answers "was I superseded?" without anyone having to + * name their successor or reason about mount ordering: any newer claim, from any + * source, invalidates every older one. That keeps the decision out of the drawer + * primitive, which cannot see the surrounding presentation and should not be + * interpreting it. + */ + +let generation = 0; + +/** + * Take the focus slot for a drawer that has just captured focus. + * + * The returned claim is opaque; pass it to {@link hasCoverDrawerFocusClaim} at + * teardown to find out whether this drawer is still the one that owes focus back. + */ +export function claimCoverDrawerFocus(): number { + generation += 1; + return generation; +} + +/** Whether `claim` is still the current claim, i.e. nothing has superseded it. */ +export function hasCoverDrawerFocusClaim(claim: number): boolean { + return claim === generation; +} + +/** + * Invalidate the outstanding claim because focus has been placed deliberately + * elsewhere. + * + * For transitions that retire a drawer without another drawer replacing it, and + * that have already decided where focus belongs — switching a thread from the + * focus drawer to the split pane, which moves focus to the view-mode toggle or + * the thread body itself. Without this the drawer's own restore would fire a + * frame later and pull focus back to whatever opened the thread. + */ +export function releaseCoverDrawerFocus(): void { + generation += 1; +} diff --git a/desktop/src/features/channels/ui/CoverDrawer.tsx b/desktop/src/features/channels/ui/CoverDrawer.tsx index f33305f99f..02c242d691 100644 --- a/desktop/src/features/channels/ui/CoverDrawer.tsx +++ b/desktop/src/features/channels/ui/CoverDrawer.tsx @@ -1,6 +1,10 @@ import { motion, useReducedMotion } from "motion/react"; import * as React from "react"; +import { + claimCoverDrawerFocus, + hasCoverDrawerFocusClaim, +} from "@/features/channels/lib/coverDrawerFocusSlot"; import { COVER_DRAWER_SLIVER_WIDTH_PX, COVER_DRAWER_TRAVEL_PX, @@ -35,13 +39,6 @@ type CoverDrawerProps = { escapeYieldsToContent?: boolean; /** Accessible name for the scrim, which is the click target back to the channel. */ scrimLabel: string; - /** - * Whether unmounting should hand focus back to wherever it was before the - * drawer opened. Evaluated at unmount, so a caller can distinguish a real - * dismissal from a presentation switch that has already claimed focus - * elsewhere. Defaults to restoring. - */ - shouldRestoreFocusOnClose?: () => boolean; /** * Test id of the drawer surface. The overlay and scrim derive theirs from it * (`-overlay`, `-scrim`) so one id names the whole presentation. @@ -181,17 +178,12 @@ export function CoverDrawer({ onClose, ownsEscape = true, scrimLabel, - shouldRestoreFocusOnClose, testId, }: CoverDrawerProps) { const prefersReducedMotion = useReducedMotion(); const travelPx = prefersReducedMotion ? 0 : COVER_DRAWER_TRAVEL_PX; const drawerRef = React.useRef(null); const previousFocusRef = React.useRef(null); - // Read at unmount, so the drawer never re-registers its focus effect (and - // never re-steals focus) just because the predicate identity changed. - const shouldRestoreFocusRef = React.useRef(shouldRestoreFocusOnClose); - shouldRestoreFocusRef.current = shouldRestoreFocusOnClose; React.useEffect(() => { if (!ownsEscape) return; @@ -226,13 +218,17 @@ export function CoverDrawer({ document.activeElement instanceof HTMLElement ? document.activeElement : null; + const focusClaim = claimCoverDrawerFocus(); drawerRef.current?.focus({ preventScroll: true }); return () => { const previousFocus = previousFocusRef.current; - const shouldRestoreFocus = shouldRestoreFocusRef.current; requestAnimationFrame(() => { - if (shouldRestoreFocus && !shouldRestoreFocus()) return; + // Deferred by a frame so the exit animation can start, which is exactly + // long enough for a replacing drawer to mount and take focus. Restore + // only while this drawer still holds the slot; otherwise the successor + // owns focus and restoring would drop it into the inert channel. + if (!hasCoverDrawerFocusClaim(focusClaim)) return; previousFocus?.focus({ preventScroll: true }); }); }; diff --git a/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs b/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs new file mode 100644 index 0000000000..74b94f8a9d --- /dev/null +++ b/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs @@ -0,0 +1,165 @@ +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +// `pretendToBeVisual` is what gives jsdom `requestAnimationFrame`. The drawer +// defers its focus restore to one, so without it the restore silently never +// runs and every assertion here would pass for the wrong reason. +const dom = new JSDOM("", { + pretendToBeVisual: true, + url: "http://localhost", +}); + +before(() => { + Object.assign(globalThis, { + Element: dom.window.Element, + HTMLElement: dom.window.HTMLElement, + IS_REACT_ACT_ENVIRONMENT: true, + MutationObserver: dom.window.MutationObserver, + Node: dom.window.Node, + // The drawer calls the bare global, not `window.requestAnimationFrame`. + cancelAnimationFrame: dom.window.cancelAnimationFrame, + document: dom.window.document, + requestAnimationFrame: dom.window.requestAnimationFrame, + window: dom.window, + }); + // `navigator` is getter-only on Node, so it needs defineProperty rather than + // assignment; motion/react reads it during render. + Object.defineProperty(globalThis, "navigator", { + configurable: true, + value: dom.window.navigator, + writable: true, + }); + // `useReducedMotion` subscribes to a media query jsdom does not implement. + dom.window.matchMedia ??= () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, + }); +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); +}); + +after(() => dom.window.close()); + +/** + * The drawer defers its focus restore to a `requestAnimationFrame`, so every + * assertion here has to run after that frame has actually fired. Two hops: + * React commits the unmount, then the rAF callback runs. + */ +async function flushDeferredFocusRestore(act) { + for (let hop = 0; hop < 2; hop += 1) { + await act(async () => { + await new Promise((resolve) => + dom.window.requestAnimationFrame(() => resolve()), + ); + }); + } +} + +async function loadHarness() { + const React = await import("react"); + const { act, render } = await import("@testing-library/react"); + const { CoverDrawer } = await import("./CoverDrawer.tsx"); + const { releaseCoverDrawerFocus } = await import( + "@/features/channels/lib/coverDrawerFocusSlot" + ); + + const opener = dom.window.document.createElement("button"); + opener.setAttribute("data-testid", "opener"); + dom.window.document.body.append(opener); + opener.focus(); + assert.equal(dom.window.document.activeElement, opener); + + const drawer = (testId) => + React.createElement( + CoverDrawer, + { ariaLabel: testId, onClose: () => {}, scrimLabel: testId, testId }, + React.createElement("div", null, testId), + ); + + return { act, drawer, opener, releaseCoverDrawerFocus, render }; +} + +function activeTestId() { + return ( + dom.window.document.activeElement?.getAttribute("data-testid") ?? + dom.window.document.activeElement?.tagName ?? + "none" + ); +} + +test("a drawer restores focus to whatever it covered when it simply closes", async () => { + const { act, drawer, opener, render } = await loadHarness(); + const view = render(drawer("first-drawer")); + assert.equal(activeTestId(), "first-drawer"); + + await act(async () => { + view.unmount(); + }); + await flushDeferredFocusRestore(act); + + assert.equal(dom.window.document.activeElement, opener); +}); + +test("a replaced drawer leaves focus with its successor, not the covered content", async () => { + // The bug this guards: restoration is deferred a frame, and in that frame the + // successor has already mounted and taken focus. An unconditional restore + // yanks focus back out of the new drawer and into content that is now inert — + // keyboard-dead, with no visible focus ring anywhere on screen. + const { act, drawer, render } = await loadHarness(); + const view = render(drawer("outgoing-drawer")); + + // Replacement, with no fully-closed intermediate state: the successor mounts + // and claims focus while the outgoing drawer is still animating out. + const successor = render(drawer("incoming-drawer")); + assert.equal(activeTestId(), "incoming-drawer"); + await act(async () => { + view.unmount(); + }); + await flushDeferredFocusRestore(act); + + assert.equal(activeTestId(), "incoming-drawer"); + successor.unmount(); +}); + +test("only the last-opened drawer keeps focus across a chain of replacements", async () => { + const { act, drawer, render } = await loadHarness(); + const first = render(drawer("first-drawer")); + const second = render(drawer("second-drawer")); + const third = render(drawer("third-drawer")); + + await act(async () => { + first.unmount(); + second.unmount(); + }); + await flushDeferredFocusRestore(act); + + assert.equal(activeTestId(), "third-drawer"); + third.unmount(); +}); + +test("a released slot leaves focus where the caller put it", async () => { + // The thread view-mode switch: nothing replaces the drawer, but the switch has + // already decided where focus belongs, so the drawer's own restore must not + // fire and drag focus back to whatever opened the thread. + const { act, drawer, releaseCoverDrawerFocus, render } = await loadHarness(); + const view = render(drawer("thread-drawer")); + + const splitPane = dom.window.document.createElement("button"); + splitPane.setAttribute("data-testid", "split-pane"); + dom.window.document.body.append(splitPane); + + await act(async () => { + releaseCoverDrawerFocus(); + splitPane.focus(); + view.unmount(); + }); + await flushDeferredFocusRestore(act); + + assert.equal(activeTestId(), "split-pane"); +}); diff --git a/desktop/src/features/channels/ui/FocusThreadDrawer.tsx b/desktop/src/features/channels/ui/FocusThreadDrawer.tsx index 8dcbee8a26..8fc98bb62e 100644 --- a/desktop/src/features/channels/ui/FocusThreadDrawer.tsx +++ b/desktop/src/features/channels/ui/FocusThreadDrawer.tsx @@ -1,6 +1,5 @@ import type * as React from "react"; -import { getThreadViewMode } from "@/features/channels/lib/threadViewModePreference"; import { CoverDrawer } from "@/features/channels/ui/CoverDrawer"; type FocusThreadDrawerProps = { @@ -14,20 +13,14 @@ type FocusThreadDrawerProps = { onClose: () => void; }; -/** - * A real dismissal keeps focus mode selected; a presentation switch has already - * selected split mode and owns focus inside the new panel. - */ -function shouldRestoreFocusOnClose() { - return getThreadViewMode() === "focus"; -} - /** * The focus-mode thread presentation: a {@link CoverDrawer} holding the thread. * - * Everything about the surface itself — motion, scrim, Escape, focus capture — - * lives in `CoverDrawer`. What is thread-specific is the focus-restore rule - * above, which has to yield to the view-mode toggle. + * Everything about the surface itself — motion, scrim, Escape, focus + * capture/restore — lives in `CoverDrawer`. Switching this thread to the split + * pane is not a dismissal and must not restore focus to whatever opened the + * thread; that case is handled where the switch happens, by releasing the + * drawer's focus slot before this unmounts. See `useThreadViewModeSwitch`. */ export function FocusThreadDrawer({ channelName, @@ -41,7 +34,6 @@ export function FocusThreadDrawer({ escapeYieldsToContent={hasActiveEdit} onClose={onClose} scrimLabel={`Back to #${channelName}`} - shouldRestoreFocusOnClose={shouldRestoreFocusOnClose} testId="focus-thread-drawer" > {children} diff --git a/desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs b/desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs new file mode 100644 index 0000000000..b624a34617 --- /dev/null +++ b/desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs @@ -0,0 +1,177 @@ +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +before(() => { + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + }); +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); +}); + +after(() => dom.window.close()); + +const AGENT_PUBKEY = "a".repeat(64); +const THREAD_HEAD_ID = "thread-head-1"; +const OTHER_THREAD_HEAD_ID = "thread-head-2"; + +/** + * Renders the real hook over a recording state harness that re-renders on every + * write, the way the panel state hooks do — the open handlers read the current + * thread/profile state as props, so a plain mutable object would hand them stale + * values and the breadcrumb assertions would silently pass for the wrong reason. + * + * Last-opened-wins is a property of these two handlers clearing each other's + * state, so the assertions are on the state writes, not on a rendered surface. + */ +async function renderAgentSessionHandlers({ openThreadHeadId = null } = {}) { + const React = await import("react"); + const { act, renderHook } = await import("@testing-library/react"); + const { useChannelAgentSessions } = await import( + "./useChannelAgentSessions.ts" + ); + + const state = { + channelManagementOpen: false, + openAgentSessionChannelId: null, + openAgentSessionPubkey: null, + openThreadHeadId, + profilePanelPubkey: null, + }; + const openedThreads = []; + let commit = () => {}; + const write = (key, value) => { + state[key] = value; + commit(); + }; + + const rendered = renderHook(() => { + const [, force] = React.useState(0); + commit = () => force((version) => version + 1); + + return useChannelAgentSessions({ + activeChannel: { id: "channel-1", name: "general" }, + activeChannelId: "channel-1", + agentsLoaded: true, + channelMembers: [{ pubkey: AGENT_PUBKEY, role: "bot" }], + handleOpenThread: (message) => { + openedThreads.push(message.id); + write("openThreadHeadId", message.id); + }, + managedAgents: [ + { + agentSource: "managed", + canInterruptTurn: true, + name: "ss-dev-00", + pubkey: AGENT_PUBKEY, + status: "deployed", + }, + ], + openAgentSessionPubkey: state.openAgentSessionPubkey, + openThreadHeadId: state.openThreadHeadId, + profilePanelPubkey: state.profilePanelPubkey, + setChannelManagementOpen: (open) => write("channelManagementOpen", open), + setExpandedThreadReplyIds: () => {}, + setOpenAgentSessionChannelId: (value) => + write("openAgentSessionChannelId", value), + setOpenAgentSessionPubkey: (value) => + write("openAgentSessionPubkey", value), + setOpenThreadHeadId: (value) => write("openThreadHeadId", value), + setProfilePanelPubkey: (value) => write("profilePanelPubkey", value), + setThreadReplyTargetId: () => {}, + setThreadScrollTargetId: () => {}, + }); + }); + + const run = (body) => act(async () => body(rendered.result.current)); + + return { openedThreads, run, state }; +} + +test("opening activity over a thread clears the thread", async () => { + const { run, state } = await renderAgentSessionHandlers({ + openThreadHeadId: THREAD_HEAD_ID, + }); + + await run((handlers) => handlers.openAgentSession(AGENT_PUBKEY, "channel-1")); + + assert.equal(state.openAgentSessionPubkey, AGENT_PUBKEY); + assert.equal(state.openThreadHeadId, null); +}); + +test("opening a thread over activity clears the agent session", async () => { + const { openedThreads, run, state } = await renderAgentSessionHandlers(); + await run((handlers) => handlers.openAgentSession(AGENT_PUBKEY, "channel-1")); + + await run((handlers) => + handlers.openThreadAndCloseAgentSession({ id: THREAD_HEAD_ID }), + ); + + assert.equal(state.openAgentSessionPubkey, null); + assert.deepEqual(openedThreads, [THREAD_HEAD_ID]); + assert.equal(state.openThreadHeadId, THREAD_HEAD_ID); +}); + +test("either ordering ends with exactly one surface's state live", async () => { + // Both directions back to back with no close in between: whichever handler ran + // last is the only one holding state, so the surface resolver never sees two + // candidates and its priority tie-break never has to decide. + const { run, state } = await renderAgentSessionHandlers(); + + await run((handlers) => handlers.openAgentSession(AGENT_PUBKEY, "channel-1")); + await run((handlers) => + handlers.openThreadAndCloseAgentSession({ id: THREAD_HEAD_ID }), + ); + + assert.equal(state.openAgentSessionPubkey, null); + assert.equal(state.openThreadHeadId, THREAD_HEAD_ID); + + await run((handlers) => handlers.openAgentSession(AGENT_PUBKEY, "channel-1")); + + assert.equal(state.openAgentSessionPubkey, AGENT_PUBKEY); + assert.equal(state.openThreadHeadId, null); +}); + +test("back from activity opened over a thread returns to that thread", async () => { + // The replacement clears the thread, so the breadcrumb is what keeps it + // recoverable; without it, last-opened-wins would be a one-way door. + const { run, state } = await renderAgentSessionHandlers({ + openThreadHeadId: THREAD_HEAD_ID, + }); + + await run((handlers) => handlers.openAgentSession(AGENT_PUBKEY, "channel-1")); + await run((handlers) => handlers.backFromAgentSession()); + + assert.equal(state.openAgentSessionPubkey, null); + assert.equal(state.openThreadHeadId, THREAD_HEAD_ID); +}); + +test("back never resurrects a thread from an earlier replacement", async () => { + // Alternating both directions leaves one breadcrumb, not a stack: + // `openThreadAndCloseAgentSession` clears the recorded target, so the next + // activity open records the thread actually on screen. + const { run, state } = await renderAgentSessionHandlers({ + openThreadHeadId: THREAD_HEAD_ID, + }); + + await run((handlers) => handlers.openAgentSession(AGENT_PUBKEY, "channel-1")); + await run((handlers) => + handlers.openThreadAndCloseAgentSession({ id: OTHER_THREAD_HEAD_ID }), + ); + await run((handlers) => handlers.openAgentSession(AGENT_PUBKEY, "channel-1")); + await run((handlers) => handlers.backFromAgentSession()); + + assert.equal(state.openThreadHeadId, OTHER_THREAD_HEAD_ID); +}); diff --git a/desktop/src/features/channels/ui/useThreadViewModeSwitch.ts b/desktop/src/features/channels/ui/useThreadViewModeSwitch.ts index 8dd41cfc51..5fdb9c62dd 100644 --- a/desktop/src/features/channels/ui/useThreadViewModeSwitch.ts +++ b/desktop/src/features/channels/ui/useThreadViewModeSwitch.ts @@ -1,5 +1,6 @@ import * as React from "react"; +import { releaseCoverDrawerFocus } from "@/features/channels/lib/coverDrawerFocusSlot"; import { setThreadViewMode, type ThreadViewMode, @@ -89,6 +90,10 @@ export function useThreadViewModeSwitch({ ); onModeChange?.(mode); setThreadViewMode(mode); + // Changing presentation is not a dismissal: this function decides where + // focus goes below, so release the cover drawer's focus slot to stop the + // outgoing drawer's own deferred restore from overriding that choice. + releaseCoverDrawerFocus(); requestAnimationFrame(() => { requestAnimationFrame(() => { document diff --git a/desktop/tests/e2e/agent-activity-cover.spec.ts b/desktop/tests/e2e/agent-activity-cover.spec.ts index ae4c5e189a..214ff9799c 100644 --- a/desktop/tests/e2e/agent-activity-cover.spec.ts +++ b/desktop/tests/e2e/agent-activity-cover.spec.ts @@ -87,6 +87,111 @@ async function openActivityFromComposer(page: Page) { await expect(page.getByTestId("agent-session-thread-panel")).toBeVisible(); } +/** + * There is one covered slot, so at most one drawer overlay may be in the DOM. + * Asserts on overlays rather than the drawer surfaces because the overlay is + * what makes the channel unreachable — two of them stacked is the failure the + * user would actually feel. + */ +async function expectExactlyOneCoverDrawer( + page: Page, + expected: "agent-activity-drawer" | "focus-thread-drawer", +) { + const others = ["agent-activity-drawer", "focus-thread-drawer"].filter( + (testId) => testId !== expected, + ); + await expect(page.getByTestId(`${expected}-overlay`)).toHaveCount(1); + for (const testId of others) { + await expect(page.getByTestId(`${testId}-overlay`)).toHaveCount(0); + } + await expect(page.getByTestId("channel-drop-zone")).toHaveAttribute( + "inert", + "", + ); +} + +/** + * Focus must end inside the drawer that won the slot. + * + * This is a positive check, not the regression guard: the covered channel is + * `inert`, so a wrongly-restored focus into it is silently refused by the + * browser and lands on `` instead of visibly stealing focus. The + * discriminating assertions for the handoff live in + * `CoverDrawerFocusHandoff.test.mjs`, which drives the primitive directly. + * Polls because both the successor's capture and the loser's deferred restore + * land asynchronously. + */ +async function expectFocusInside(page: Page, testId: string) { + await expect + .poll(() => + page.evaluate( + (currentTestId) => + document + .querySelector(`[data-testid="${currentTestId}"]`) + ?.contains(document.activeElement) ?? false, + testId, + ), + ) + .toBe(true); +} + +/** + * Opens a thread while activity covers the channel. + * + * The covered channel is inert, so its thread summaries cannot be clicked. A + * `messageId` deep link reaches the same place: `useChannelRouteTarget` closes + * the agent session and opens the thread in one navigation, which is exactly the + * open-over-open transition under test — no closed intermediate state. + * + * The router uses hash history, so the param has to be written into the hash + * fragment (see the same technique in `scroll-history.spec.ts`); rewriting + * `location.search` would leave the router none the wiser. + */ +async function openThreadByMessageLink(page: Page, threadHeadId: string) { + await page.evaluate((targetId) => { + const hash = window.location.hash.replace(/^#/, "") || "/"; + const [path, query = ""] = hash.split("?"); + const params = new URLSearchParams(query); + params.set("messageId", targetId); + window.history.pushState( + {}, + "", + `${window.location.pathname}#${path}?${params.toString()}`, + ); + window.dispatchEvent(new HashChangeEvent("hashchange")); + window.dispatchEvent(new PopStateEvent("popstate")); + }, threadHeadId); +} + +/** + * Opens activity while a thread covers the channel, from the thread composer's + * own activity bar — the one ingress that is reachable while the channel behind + * is inert, so the thread never closes first. + */ +async function openActivityFromThreadComposer( + page: Page, + threadHeadId: string, +) { + await page.evaluate( + ({ currentThreadHeadId, pubkey }) => { + window.__BUZZ_E2E_EMIT_MOCK_TYPING__?.({ + channelName: "agents", + pubkey, + threadHeadId: currentThreadHeadId, + }); + }, + { currentThreadHeadId: threadHeadId, pubkey: AGENT_PUBKEY }, + ); + + const drawer = page.getByTestId("focus-thread-drawer"); + const trigger = drawer.getByTestId("bot-activity-composer-trigger"); + await expect(trigger).toBeVisible(); + await trigger.click(); + const item = page.getByTestId(`bot-activity-composer-item-${AGENT_PUBKEY}`); + await expect(item).toBeVisible(); + await item.click({ force: true }); +} + /** * 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. @@ -170,7 +275,9 @@ test("agent activity covers the channel at wide viewports", async ({ await expect(channel).not.toHaveAttribute("inert", ""); }); -test("only one cover drawer is ever open", async ({ page }) => { +test("cover drawers replace each other in both directions without stacking", async ({ + page, +}) => { await page.setViewportSize(WIDE_VIEWPORT); await page.addInitScript(() => { localStorage.setItem("buzz.channels.threadViewMode", "focus"); @@ -182,35 +289,31 @@ test("only one cover drawer is ever open", async ({ page }) => { const agentDrawer = page.getByTestId("agent-activity-drawer"); const threadDrawer = page.getByTestId("focus-thread-drawer"); await expect(agentDrawer).toBeVisible(); + await expectExactlyOneCoverDrawer(page, "agent-activity-drawer"); - // Opening a thread from the covered channel is not possible while it is - // inert, so drive the same handler the timeline uses: leave activity, then - // open the thread. The thread drawer takes the covered slot alone. - await page.getByTestId("auxiliary-panel-close").click(); - await expect(agentDrawer).toHaveCount(0); - - const summary = page.locator( - `[data-testid="message-thread-summary"][data-thread-head-id="${rootId}"]`, - ); - await expect(summary).toBeVisible(); - await summary.click(); + // Direction 1: thread opens over activity, with no closed intermediate. The + // covered channel is inert so its thread summaries can't be clicked, but a + // message link resolves through the same route-target handler, which clears + // the agent session and opens the thread in one navigation. + await openThreadByMessageLink(page, rootId); await expect(threadDrawer).toBeVisible(); await expect(agentDrawer).toHaveCount(0); + await expectExactlyOneCoverDrawer(page, "focus-thread-drawer"); + // The replaced surface's param is gone, not merely outranked. + await expect(page).not.toHaveURL(/agentSession=/); + await expect(page).toHaveURL(new RegExp(`thread=${rootId}`)); + await expectFocusInside(page, "focus-thread-drawer"); - // Now open activity with the thread still in the URL: the agent surface wins - // nothing — the thread owns the slot — and crucially there is never more - // than one drawer overlay in the DOM. - await expect(page.getByTestId("focus-thread-drawer-overlay")).toHaveCount(1); - await expect(page.getByTestId("agent-activity-drawer-overlay")).toHaveCount( - 0, - ); - - // Escape returns the channel, then activity may cover again. - await page.keyboard.press("Escape"); - await expect(threadDrawer).toHaveCount(0); - await openActivityFromComposer(page); + // Direction 2: activity opens over the thread, again with no closed + // intermediate — the thread drawer's own composer activity bar is live while + // it covers, and its trigger calls the same open handler. + await openActivityFromThreadComposer(page, rootId); await expect(agentDrawer).toBeVisible(); - await expect(page.getByTestId("focus-thread-drawer-overlay")).toHaveCount(0); + await expect(threadDrawer).toHaveCount(0); + await expectExactlyOneCoverDrawer(page, "agent-activity-drawer"); + await expect(page).not.toHaveURL(new RegExp(`thread=${rootId}`)); + await expect(page).toHaveURL(/agentSession=/); + await expectFocusInside(page, "agent-activity-drawer"); }); test("narrow viewports keep the existing activity presentation", async ({