fix(desktop): coordinate cover drawer focus with a shared slot claim

**Category:** fix
**User Impact:** Opening a thread over agent activity (or activity over a
thread) now leaves keyboard focus inside the drawer that just opened. Before,
the outgoing drawer's deferred focus restore fired after the new drawer had
already taken focus and pushed focus into the covered channel, which is
`inert` — so focus landed on `<body>` with no visible focus ring and no
keyboard path back into the visible drawer.

**Problem:** Two review findings, one root cause. `CoverDrawer` decided at
teardown whether to restore focus by calling a caller-supplied predicate that
read thread view-mode state, so a presentation-only primitive was
interpreting a specific caller's presentation, and the predicate could not
see the replacement race at all. Separately, last-opened-wins was only
demonstrated through the resolver's fixed thread-beats-agent priority; the
e2e "exclusivity" test closed one drawer before opening the other and its
final step opened nothing, so no test exercised a real open-over-open
transition or asserted the replaced surface's URL param was cleared.

**Solution:** Replace the predicate with a module-level focus slot: a drawer
claims the slot when it captures focus and restores only if its claim is
still current, so any successor's claim — from any source, with no mount
ordering assumptions — invalidates the loser's pending restore. The thread
view-mode switch releases the slot from the thread side, keeping that
decision out of the primitive while leaving plain thread close identical.
Document the resolver's priority as a stale-simultaneous-param safety net
and test the open handlers directly for both orderings, which is where the
real rule lives.

<details>
<summary>File changes</summary>

**desktop/src/features/channels/lib/coverDrawerFocusSlot.ts**
New single-slot coordinator: `claimCoverDrawerFocus`,
`hasCoverDrawerFocusClaim`, `releaseCoverDrawerFocus`. A monotonic
generation answers "was I superseded?" without any drawer naming its
successor.

**desktop/src/features/channels/ui/CoverDrawer.tsx**
Drops the `shouldRestoreFocusOnClose` prop. Claims the slot on focus
capture and gates the deferred restore on still holding it.

**desktop/src/features/channels/ui/FocusThreadDrawer.tsx**
No longer reads thread view-mode state to decide focus restore.

**desktop/src/features/channels/ui/useThreadViewModeSwitch.ts**
Releases the focus slot when the switch places focus itself.

**desktop/src/features/channels/lib/channelAuxiliarySurface.ts**
Documents the priority order as a safety net for stale/simultaneous params,
not the product rule.

**desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs**
Drives `CoverDrawer` in jsdom: plain close restores, a replaced drawer
leaves focus with its successor, a chain of replacements keeps only the
last, and a released slot leaves focus where the caller put it.

**desktop/src/features/channels/lib/coverDrawerFocusSlot.test.mjs**
Claim/supersede/release semantics, including that claims are never reused.

**desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs**
Tests the open handlers directly in both orderings, plus the breadcrumb
that keeps the replaced thread recoverable.

**desktop/tests/e2e/agent-activity-cover.spec.ts**
Rewrites the exclusivity test as genuine open-over-open transitions with no
closed intermediate: thread over activity via a `messageId` deep link,
activity over thread via the thread composer's activity bar (the one
ingress reachable while the channel is inert). Asserts the replaced param
is cleared and exactly one overlay exists.

</details>

Note on the e2e focus assertions: they are positive checks, not the
regression guard. Because the covered channel is `inert`, a wrongly-restored
focus is silently refused and lands on `<body>` rather than visibly stealing
focus — mutating the guard to a no-op still passes e2e. The discriminating
assertions therefore live in `CoverDrawerFocusHandoff.test.mjs`, where
disabling the guard fails 3 of 4 tests while the plain-close test keeps
passing.

Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
Co-authored-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
This commit is contained in:
ss-dev-00
2026-08-21 20:41:21 -07:00
committed by Rizz
parent 371933130e
commit 5a6dc8704f
9 changed files with 622 additions and 56 deletions

View File

@@ -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"

View File

@@ -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);
});

View File

@@ -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;
}

View File

@@ -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<HTMLDivElement>(null);
const previousFocusRef = React.useRef<HTMLElement | null>(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 });
});
};

View File

@@ -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("<!doctype html><html><body></body></html>", {
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");
});

View File

@@ -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}

View File

@@ -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("<!doctype html><html><body></body></html>", {
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);
});

View File

@@ -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

View File

@@ -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 `<body>` 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 ({