From 88a96d7682b3c8c9f7c813e3ed349f9daaeaa124 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Fri, 21 Aug 2026 19:25:24 -0700 Subject: [PATCH 1/8] feat(desktop): cover the channel with the agent activity drawer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** feat **User Impact:** At wide viewports, opening an agent's activity inside a channel now covers the channel content area in a right-anchored drawer instead of squeezing into a ~380px split pane, so tool calls, diffs, and command output are readable without resizing. **Problem:** The agent session panel shared the thread's split `RightAuxiliaryPane`, which is too narrow for a transcript. The focus drawer that solves this for threads was welded to thread-specific breadcrumb and view-mode-toggle concerns, so activity could not reuse it. **Solution:** Extract the drawer surface into a presentation-only `CoverDrawer` and give activity its own thin wrapper. Activity always covers at wide viewports and offers no focus/split toggle; narrow, overlay, and single-panel presentations are unchanged for both surfaces. A single resolved auxiliary surface makes the two drawers mutually exclusive by construction, so last-opened wins and two drawers can never stack. Thread drawer behavior is unchanged.
File changes **desktop/src/features/channels/ui/CoverDrawer.tsx** New presentation-only drawer extracted from `FocusThreadDrawer`: motion, scrim, focus capture/restore, and an `ownsEscape` opt-out for content that already handles Escape itself. **desktop/src/features/channels/ui/AgentActivityDrawer.tsx** Activity's wrapper. Delegates Escape to the panel so the settings menu dismisses first. **desktop/src/features/channels/ui/FocusThreadDrawer.tsx** Reduced to a `CoverDrawer` wrapper plus its thread-specific focus-restore rule. Behavior unchanged. **desktop/src/features/channels/lib/channelAuxiliarySurface.ts** Resolves the one auxiliary surface and which surface, if any, covers — the structural guarantee that only one drawer exists. **desktop/src/features/channels/lib/agentSessionPanelPresentation.ts** Maps presentation to the panel's layout props. Suppresses the panel's own enter motion inside the drawer to avoid a double slide. **desktop/src/features/channels/lib/coverDrawerLayout.ts** Sliver width and travel distance, moved off the thread-specific module. **desktop/src/features/channels/ui/AgentSessionThreadPanel.tsx** Accepts and forwards `enterMotion` (additive), which `AuxiliaryPanel` already supported but the panel never threaded through. **desktop/src/features/channels/ui/ChannelPane.tsx** Renders branches off the resolved surface instead of an implicit fall-through chain, and wraps the agent panel in the cover drawer. **desktop/tests/e2e/agent-activity-cover.spec.ts** Covers covering vs. splitting, drawer exclusivity, suppressed panel motion, Escape/scrim dismissal, and unchanged narrow presentation. **desktop/tests/e2e/activity-scope-label-screenshots.spec.ts** Lengthens the long-name fixture so the header truncation assertion still clamps at the wider cover-drawer width.
Signed-off-by: ss-dev-00 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- desktop/playwright.config.ts | 1 + desktop/src/app/AppShell.tsx | 4 +- .../channels/coverDrawerCloseRequest.test.mjs | 21 ++ .../channels/coverDrawerCloseRequest.ts | 22 ++ .../focusedThreadCloseRequest.test.mjs | 21 -- .../channels/focusedThreadCloseRequest.ts | 16 - .../agentSessionPanelPresentation.test.mjs | 54 ++++ .../lib/agentSessionPanelPresentation.ts | 57 ++++ .../lib/channelAuxiliarySurface.test.mjs | 182 +++++++++++ .../channels/lib/channelAuxiliarySurface.ts | 77 +++++ .../channels/lib/coverDrawerLayout.ts | 34 ++ .../channels/lib/threadFocusLayout.ts | 26 -- .../channels/ui/AgentActivityDrawer.tsx | 43 +++ .../channels/ui/AgentSessionThreadPanel.tsx | 8 + .../src/features/channels/ui/ChannelPane.tsx | 117 ++++--- .../src/features/channels/ui/CoverDrawer.tsx | 301 ++++++++++++++++++ .../channels/ui/FocusThreadDrawer.tsx | 253 ++------------- ...rPresence.ts => useCoverDrawerPresence.ts} | 6 +- .../activity-scope-label-screenshots.spec.ts | 5 +- .../tests/e2e/agent-activity-cover.spec.ts | 234 ++++++++++++++ 20 files changed, 1143 insertions(+), 339 deletions(-) create mode 100644 desktop/src/features/channels/coverDrawerCloseRequest.test.mjs create mode 100644 desktop/src/features/channels/coverDrawerCloseRequest.ts delete mode 100644 desktop/src/features/channels/focusedThreadCloseRequest.test.mjs delete mode 100644 desktop/src/features/channels/focusedThreadCloseRequest.ts create mode 100644 desktop/src/features/channels/lib/agentSessionPanelPresentation.test.mjs create mode 100644 desktop/src/features/channels/lib/agentSessionPanelPresentation.ts create mode 100644 desktop/src/features/channels/lib/channelAuxiliarySurface.test.mjs create mode 100644 desktop/src/features/channels/lib/channelAuxiliarySurface.ts create mode 100644 desktop/src/features/channels/lib/coverDrawerLayout.ts create mode 100644 desktop/src/features/channels/ui/AgentActivityDrawer.tsx create mode 100644 desktop/src/features/channels/ui/CoverDrawer.tsx rename desktop/src/features/channels/ui/{useFocusDrawerPresence.ts => useCoverDrawerPresence.ts} (68%) create mode 100644 desktop/tests/e2e/agent-activity-cover.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 69250c4b53..7c6465cdff 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -95,6 +95,7 @@ export default defineConfig({ "**/thread-reply-anchor-roleplay.spec.ts", "**/threadpane-ultrawide.spec.ts", "**/thread-focus-mode.spec.ts", + "**/agent-activity-cover.spec.ts", "**/animated-avatar.spec.ts", "**/reminders.spec.ts", "**/reminder-click-repro.spec.ts", diff --git a/desktop/src/app/AppShell.tsx b/desktop/src/app/AppShell.tsx index e111f93ca0..0cf787647d 100644 --- a/desktop/src/app/AppShell.tsx +++ b/desktop/src/app/AppShell.tsx @@ -75,7 +75,7 @@ import { import { useDueReminderBadgeCount } from "@/features/reminders/hooks"; import { useReminderNotifications } from "@/features/reminders/useReminderNotifications"; import { AppSidebar } from "@/features/sidebar/ui/AppSidebar"; -import { requestFocusedThreadClose } from "@/features/channels/focusedThreadCloseRequest"; +import { requestCoverDrawerClose } from "@/features/channels/coverDrawerCloseRequest"; import { CommunityRail } from "@/features/sidebar/ui/CommunityRail"; import { useChannelMutes } from "@/features/sidebar/lib/useChannelMutes"; import { useChannelStars } from "@/features/sidebar/lib/useChannelStars"; @@ -846,7 +846,7 @@ export function AppShell() { addCommunityDialog.onOpenChange } onNewMessage={goNewMessage} - onBackgroundClick={requestFocusedThreadClose} + onBackgroundClick={requestCoverDrawerClose} onCreateChannelOpenChange={setIsCreateChannelOpen} onOpenAddCommunity={addCommunityDialog.openDialog} onSendFeedback={() => setIsSendFeedbackOpen(true)} diff --git a/desktop/src/features/channels/coverDrawerCloseRequest.test.mjs b/desktop/src/features/channels/coverDrawerCloseRequest.test.mjs new file mode 100644 index 0000000000..75a3bf70db --- /dev/null +++ b/desktop/src/features/channels/coverDrawerCloseRequest.test.mjs @@ -0,0 +1,21 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + requestCoverDrawerClose, + subscribeToCoverDrawerCloseRequest, +} from "./coverDrawerCloseRequest.ts"; + +test("cover drawer close requests reach active subscribers only", () => { + let calls = 0; + const unsubscribe = subscribeToCoverDrawerCloseRequest(() => { + calls += 1; + }); + + requestCoverDrawerClose(); + assert.equal(calls, 1); + + unsubscribe(); + requestCoverDrawerClose(); + assert.equal(calls, 1); +}); diff --git a/desktop/src/features/channels/coverDrawerCloseRequest.ts b/desktop/src/features/channels/coverDrawerCloseRequest.ts new file mode 100644 index 0000000000..6e8f2b705e --- /dev/null +++ b/desktop/src/features/channels/coverDrawerCloseRequest.ts @@ -0,0 +1,22 @@ +const listeners = new Set<() => void>(); + +/** + * Request dismissal of the channel's open cover drawer. + * + * One channel of a channel pane is covered at a time (focus-mode thread or + * agent activity), so this needs no discriminator — whichever drawer is open + * subscribes and closes. + */ +export function requestCoverDrawerClose(): void { + for (const listener of listeners) { + listener(); + } +} + +/** Subscribe the active cover drawer to external dismissal requests. */ +export function subscribeToCoverDrawerCloseRequest( + listener: () => void, +): () => void { + listeners.add(listener); + return () => listeners.delete(listener); +} diff --git a/desktop/src/features/channels/focusedThreadCloseRequest.test.mjs b/desktop/src/features/channels/focusedThreadCloseRequest.test.mjs deleted file mode 100644 index 6f30d7ec6d..0000000000 --- a/desktop/src/features/channels/focusedThreadCloseRequest.test.mjs +++ /dev/null @@ -1,21 +0,0 @@ -import assert from "node:assert/strict"; -import test from "node:test"; - -import { - requestFocusedThreadClose, - subscribeToFocusedThreadCloseRequest, -} from "./focusedThreadCloseRequest.ts"; - -test("focus thread close requests reach active subscribers only", () => { - let calls = 0; - const unsubscribe = subscribeToFocusedThreadCloseRequest(() => { - calls += 1; - }); - - requestFocusedThreadClose(); - assert.equal(calls, 1); - - unsubscribe(); - requestFocusedThreadClose(); - assert.equal(calls, 1); -}); diff --git a/desktop/src/features/channels/focusedThreadCloseRequest.ts b/desktop/src/features/channels/focusedThreadCloseRequest.ts deleted file mode 100644 index 3628d70767..0000000000 --- a/desktop/src/features/channels/focusedThreadCloseRequest.ts +++ /dev/null @@ -1,16 +0,0 @@ -const listeners = new Set<() => void>(); - -/** Request dismissal of an open focus-mode thread drawer. */ -export function requestFocusedThreadClose(): void { - for (const listener of listeners) { - listener(); - } -} - -/** Subscribe the active channel surface to focus-mode dismissal requests. */ -export function subscribeToFocusedThreadCloseRequest( - listener: () => void, -): () => void { - listeners.add(listener); - return () => listeners.delete(listener); -} diff --git a/desktop/src/features/channels/lib/agentSessionPanelPresentation.test.mjs b/desktop/src/features/channels/lib/agentSessionPanelPresentation.test.mjs new file mode 100644 index 0000000000..9294d134a4 --- /dev/null +++ b/desktop/src/features/channels/lib/agentSessionPanelPresentation.test.mjs @@ -0,0 +1,54 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { getAgentSessionPanelPresentation } from "./agentSessionPanelPresentation.ts"; + +test("the cover drawer owns motion and gets standalone, opaque chrome", () => { + assert.deepEqual( + getAgentSessionPanelPresentation({ + isCoverDrawer: true, + isSinglePanelView: false, + useSplitAuxiliaryPane: true, + }), + { + enterMotion: false, + isSinglePanelView: true, + layout: "standalone", + transparentChrome: false, + }, + ); +}); + +test("the split pane keeps docked chrome and its own enter motion", () => { + assert.deepEqual( + getAgentSessionPanelPresentation({ + isCoverDrawer: false, + isSinglePanelView: false, + useSplitAuxiliaryPane: true, + }), + { + enterMotion: true, + isSinglePanelView: false, + layout: "split", + transparentChrome: true, + }, + ); +}); + +test("narrow viewports keep today's overlay and single-panel presentations", () => { + for (const isSinglePanelView of [false, true]) { + assert.deepEqual( + getAgentSessionPanelPresentation({ + isCoverDrawer: false, + isSinglePanelView, + useSplitAuxiliaryPane: false, + }), + { + enterMotion: true, + isSinglePanelView, + layout: "standalone", + transparentChrome: false, + }, + ); + } +}); diff --git a/desktop/src/features/channels/lib/agentSessionPanelPresentation.ts b/desktop/src/features/channels/lib/agentSessionPanelPresentation.ts new file mode 100644 index 0000000000..e7380da511 --- /dev/null +++ b/desktop/src/features/channels/lib/agentSessionPanelPresentation.ts @@ -0,0 +1,57 @@ +/** + * `AnimatePresence` key shared by every agent activity presentation. + * + * The split pane and the cover drawer are two containers for one session, so + * presence is a property of the session, not of either container — crossing the + * viewport breakpoint changes how it is shown, not whether it is open. + */ +export const AGENT_SESSION_SURFACE_KEY = "agent-session-surface"; + +export type AgentSessionPanelPresentation = { + enterMotion: boolean; + isSinglePanelView: boolean; + layout: "standalone" | "split"; + transparentChrome: boolean; +}; + +type AgentSessionPanelPresentationOptions = { + /** The panel is rendered inside the agent activity cover drawer. */ + isCoverDrawer: boolean; + isSinglePanelView: boolean; + useSplitAuxiliaryPane: boolean; +}; + +/** + * Maps channel presentation into the agent session panel's layout props. + * + * TODO(#6538): once the `conversation` transcript variant lands on main, this + * should also return `transcriptVariant: "conversation"` for the cover drawer + * and `undefined` otherwise, so the reading view is pinned by presentation + * rather than inferred from panel width. The variant does not exist on main + * yet, so the prop is deliberately not set here. + */ +export function getAgentSessionPanelPresentation({ + isCoverDrawer, + isSinglePanelView, + useSplitAuxiliaryPane, +}: AgentSessionPanelPresentationOptions): AgentSessionPanelPresentation { + if (isCoverDrawer) { + return { + // The drawer animates itself; a second slide inside it would compound. + enterMotion: false, + // Fills the drawer, and selects the standalone header chrome that owns + // its own backdrop — the drawer is not sharing the channel's header, and + // it has no resizable neighbour to draw a resize border against. + isSinglePanelView: true, + layout: "standalone", + transparentChrome: false, + }; + } + + return { + enterMotion: true, + isSinglePanelView: useSplitAuxiliaryPane ? false : isSinglePanelView, + layout: useSplitAuxiliaryPane ? "split" : "standalone", + transparentChrome: useSplitAuxiliaryPane, + }; +} diff --git a/desktop/src/features/channels/lib/channelAuxiliarySurface.test.mjs b/desktop/src/features/channels/lib/channelAuxiliarySurface.test.mjs new file mode 100644 index 0000000000..bbad854850 --- /dev/null +++ b/desktop/src/features/channels/lib/channelAuxiliarySurface.test.mjs @@ -0,0 +1,182 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + resolveChannelAuxiliarySurface, + resolveChannelCoverDrawer, +} from "./channelAuxiliarySurface.ts"; + +const NO_SURFACE = { + channelManagementOpen: false, + hasActiveChannel: true, + hasProfilePanel: false, + hasSelectedAgent: false, + hasThreadHead: false, + shouldShowThreadSkeleton: false, +}; + +test("no candidate surface resolves to nothing", () => { + assert.equal(resolveChannelAuxiliarySurface(NO_SURFACE), null); +}); + +test("every candidate open at once still resolves to exactly one surface", () => { + assert.equal( + resolveChannelAuxiliarySurface({ + channelManagementOpen: true, + hasActiveChannel: true, + hasProfilePanel: true, + hasSelectedAgent: true, + hasThreadHead: true, + shouldShowThreadSkeleton: true, + }), + "channel-management", + ); +}); + +test("surfaces resolve in priority order as higher ones drop away", () => { + const all = { + channelManagementOpen: true, + hasActiveChannel: true, + hasProfilePanel: true, + hasSelectedAgent: true, + hasThreadHead: true, + shouldShowThreadSkeleton: true, + }; + + assert.equal( + resolveChannelAuxiliarySurface({ ...all, channelManagementOpen: false }), + "thread", + ); + assert.equal( + resolveChannelAuxiliarySurface({ + ...all, + channelManagementOpen: false, + hasThreadHead: false, + }), + "thread-skeleton", + ); + assert.equal( + resolveChannelAuxiliarySurface({ + ...all, + channelManagementOpen: false, + hasThreadHead: false, + shouldShowThreadSkeleton: false, + }), + "agent-session", + ); + assert.equal( + resolveChannelAuxiliarySurface({ + ...all, + channelManagementOpen: false, + hasSelectedAgent: false, + hasThreadHead: false, + shouldShowThreadSkeleton: false, + }), + "profile", + ); +}); + +test("channel-scoped surfaces need an active channel", () => { + const withoutChannel = { ...NO_SURFACE, hasActiveChannel: false }; + + assert.equal( + resolveChannelAuxiliarySurface({ + ...withoutChannel, + channelManagementOpen: true, + }), + null, + ); + assert.equal( + resolveChannelAuxiliarySurface({ + ...withoutChannel, + hasSelectedAgent: true, + }), + null, + ); + // The profile panel is identity-scoped, so it survives without a channel. + assert.equal( + resolveChannelAuxiliarySurface({ + ...withoutChannel, + hasProfilePanel: true, + }), + "profile", + ); +}); + +test("agent activity always covers at wide viewports, whatever the thread preference", () => { + for (const threadViewMode of ["focus", "split"]) { + assert.equal( + resolveChannelCoverDrawer({ + surface: "agent-session", + threadViewMode, + useSplitAuxiliaryPane: true, + }), + "agent-session", + ); + } +}); + +test("threads cover only in focus mode", () => { + for (const surface of ["thread", "thread-skeleton"]) { + assert.equal( + resolveChannelCoverDrawer({ + surface, + threadViewMode: "focus", + useSplitAuxiliaryPane: true, + }), + "thread", + ); + assert.equal( + resolveChannelCoverDrawer({ + surface, + threadViewMode: "split", + useSplitAuxiliaryPane: true, + }), + null, + ); + } +}); + +test("narrow and single-panel viewports never cover", () => { + for (const surface of ["agent-session", "thread", "thread-skeleton"]) { + assert.equal( + resolveChannelCoverDrawer({ + surface, + threadViewMode: "focus", + useSplitAuxiliaryPane: false, + }), + null, + ); + } +}); + +test("split-only surfaces never cover", () => { + for (const surface of ["channel-management", "profile", null]) { + assert.equal( + resolveChannelCoverDrawer({ + surface, + threadViewMode: "focus", + useSplitAuxiliaryPane: true, + }), + null, + ); + } +}); + +test("only one drawer can cover, because only one surface resolves", () => { + // Thread and agent activity both requested: the surface resolution picks the + // thread, so the agent drawer cannot also be covering. + const surface = resolveChannelAuxiliarySurface({ + ...NO_SURFACE, + hasSelectedAgent: true, + hasThreadHead: true, + }); + const drawer = resolveChannelCoverDrawer({ + surface, + threadViewMode: "focus", + useSplitAuxiliaryPane: true, + }); + + assert.equal(surface, "thread"); + assert.equal(drawer, "thread"); +}); diff --git a/desktop/src/features/channels/lib/channelAuxiliarySurface.ts b/desktop/src/features/channels/lib/channelAuxiliarySurface.ts new file mode 100644 index 0000000000..7b419e5aa3 --- /dev/null +++ b/desktop/src/features/channels/lib/channelAuxiliarySurface.ts @@ -0,0 +1,77 @@ +import type { ThreadViewMode } from "@/features/channels/lib/threadViewModePreference"; + +/** + * 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. + */ +export type ChannelAuxiliarySurface = + | "agent-session" + | "channel-management" + | "profile" + | "thread" + | "thread-skeleton"; + +type ChannelAuxiliarySurfaceOptions = { + channelManagementOpen: boolean; + hasActiveChannel: boolean; + hasProfilePanel: boolean; + hasSelectedAgent: boolean; + hasThreadHead: boolean; + shouldShowThreadSkeleton: boolean; +}; + +/** Which auxiliary surface the channel pane should render, if any. */ +export function resolveChannelAuxiliarySurface({ + channelManagementOpen, + hasActiveChannel, + hasProfilePanel, + hasSelectedAgent, + hasThreadHead, + shouldShowThreadSkeleton, +}: ChannelAuxiliarySurfaceOptions): ChannelAuxiliarySurface | null { + if (channelManagementOpen && hasActiveChannel) return "channel-management"; + if (hasThreadHead) return "thread"; + if (shouldShowThreadSkeleton) return "thread-skeleton"; + if (hasActiveChannel && hasSelectedAgent) return "agent-session"; + if (hasProfilePanel) return "profile"; + return null; +} + +/** A cover drawer overlays the channel content area instead of splitting it. */ +export type ChannelCoverDrawer = "agent-session" | "thread"; + +type ChannelCoverDrawerOptions = { + surface: ChannelAuxiliarySurface | null; + threadViewMode: ThreadViewMode; + useSplitAuxiliaryPane: boolean; +}; + +/** + * Which surface, if any, presents as a cover drawer. + * + * Threads honour the user's view-mode preference. Agent activity does not and + * deliberately offers no toggle: its transcript is tool calls, diffs, and + * command output, which a 380px side pane cannot show usefully — so at any + * viewport wide enough for two panes it always covers. Narrow/overlay and + * single-panel viewports keep their existing presentations for both. + * + * Returning a single value is what makes the two drawers mutually exclusive: + * there is one covered slot, and the resolved surface owns it. + */ +export function resolveChannelCoverDrawer({ + surface, + threadViewMode, + useSplitAuxiliaryPane, +}: ChannelCoverDrawerOptions): ChannelCoverDrawer | null { + if (!useSplitAuxiliaryPane) return null; + + if (surface === "thread" || surface === "thread-skeleton") { + return threadViewMode === "focus" ? "thread" : null; + } + + return surface === "agent-session" ? "agent-session" : null; +} diff --git a/desktop/src/features/channels/lib/coverDrawerLayout.ts b/desktop/src/features/channels/lib/coverDrawerLayout.ts new file mode 100644 index 0000000000..00aa710f0f --- /dev/null +++ b/desktop/src/features/channels/lib/coverDrawerLayout.ts @@ -0,0 +1,34 @@ +/** + * Layout constants shared by the channel's cover drawers. + * + * A cover drawer overlays the channel content area with a right-anchored + * surface rather than splitting the row into two resizable panes. Both the + * focus-mode thread drawer and the agent activity drawer are the same + * geometry — only their contents and their open condition differ. + */ + +/** + * Width of the channel sliver left visible to the left of a cover drawer. + * + * Wide enough to read a truncated `‹ #channel` label and to be a comfortable, + * full-height click target back to the channel, but narrow enough that the + * drawer still reads as the primary surface. The sliver keeps showing the real, + * still-mounted channel timeline (dimmed by the scrim) so the user never loses + * their place. + */ +export const COVER_DRAWER_SLIVER_WIDTH_PX = 72; + +/** + * Horizontal distance a cover drawer travels on enter/exit. + * + * Deliberately a fraction of the drawer's own width rather than a true slide + * from off-screen: opening a thread is a high-frequency act — threads are chat + * sessions and get flipped between constantly — and full-width travel turns a + * routine move into ceremony. Short travel keeps it light and repeatable. + * + * The floor matters as much as the ceiling: the shared 24px side-panel nudge is + * only ~3% of this drawer's width, which reads as no movement at all, leaving + * the opacity fade as the only perceptible change. This is large enough for the + * eye to track a direction and for the ease to have somewhere to decelerate. + */ +export const COVER_DRAWER_TRAVEL_PX = 120; diff --git a/desktop/src/features/channels/lib/threadFocusLayout.ts b/desktop/src/features/channels/lib/threadFocusLayout.ts index f14f3399bd..edfbb3a0cd 100644 --- a/desktop/src/features/channels/lib/threadFocusLayout.ts +++ b/desktop/src/features/channels/lib/threadFocusLayout.ts @@ -5,17 +5,6 @@ * rather than splitting the row into two resizable panes. */ -/** - * Width of the channel sliver left visible to the left of the focus drawer. - * - * Wide enough to read a truncated `‹ #channel` label and to be a comfortable, - * full-height click target back to the channel, but narrow enough that the - * drawer still reads as the primary surface. The sliver keeps showing the real, - * still-mounted channel timeline (dimmed by the scrim) so the user never loses - * their place. - */ -export const THREAD_FOCUS_SLIVER_WIDTH_PX = 72; - /** * Max width of the centered message column inside the focus drawer. * @@ -26,21 +15,6 @@ export const THREAD_FOCUS_SLIVER_WIDTH_PX = 72; */ export const THREAD_FOCUS_COLUMN_MAX_WIDTH_PX = 880; -/** - * Horizontal distance the focus drawer travels on enter/exit. - * - * Deliberately a fraction of the drawer's own width rather than a true slide - * from off-screen: opening a thread is a high-frequency act — threads are chat - * sessions and get flipped between constantly — and full-width travel turns a - * routine move into ceremony. Short travel keeps it light and repeatable. - * - * The floor matters as much as the ceiling: the shared 24px side-panel nudge is - * only ~3% of this drawer's width, which reads as no movement at all, leaving - * the opacity fade as the only perceptible change. This is large enough for the - * eye to track a direction and for the ease to have somewhere to decelerate. - */ -export const THREAD_FOCUS_DRAWER_TRAVEL_PX = 120; - /** * `AnimatePresence` key shared by both thread layouts. * diff --git a/desktop/src/features/channels/ui/AgentActivityDrawer.tsx b/desktop/src/features/channels/ui/AgentActivityDrawer.tsx new file mode 100644 index 0000000000..e0121baa44 --- /dev/null +++ b/desktop/src/features/channels/ui/AgentActivityDrawer.tsx @@ -0,0 +1,43 @@ +import type * as React from "react"; + +import { CoverDrawer } from "@/features/channels/ui/CoverDrawer"; + +type AgentActivityDrawerProps = { + channelName: string; + children: React.ReactNode; + onClose: () => void; +}; + +/** + * The agent activity presentation at wide viewports: a {@link CoverDrawer} + * holding the channel-scoped agent session panel. + * + * Unlike the thread, activity has no split/focus choice — a transcript of tool + * calls, diffs, and command output is only legible at this width, so it always + * covers and never offers a presentation toggle. That also means it needs no + * conditional focus-restore rule: closing it is always a real dismissal, so + * focus returns to whatever opened it. + * + * Escape stays with the panel rather than being claimed by the drawer. The + * panel already closes on Escape in this presentation, and routing the key + * through its `useEscapeKey` keeps the settings menu's own dismissal first — + * the thread drawer claims the key instead because its composer's mention + * autocomplete would otherwise swallow a press meant for the thread. + */ +export function AgentActivityDrawer({ + channelName, + children, + onClose, +}: AgentActivityDrawerProps) { + return ( + + {children} + + ); +} diff --git a/desktop/src/features/channels/ui/AgentSessionThreadPanel.tsx b/desktop/src/features/channels/ui/AgentSessionThreadPanel.tsx index c1933f14bb..ce4ca8d204 100644 --- a/desktop/src/features/channels/ui/AgentSessionThreadPanel.tsx +++ b/desktop/src/features/channels/ui/AgentSessionThreadPanel.tsx @@ -67,6 +67,12 @@ type AgentSessionThreadPanelProps = { channel: Channel | null; channelId?: string | null; canInterruptTurn: boolean; + /** + * When false, the panel skips its own slide-in. Set by the cover drawer, + * which already animates itself, so the two don't compound into a double + * slide. Defaults to animating. + */ + enterMotion?: boolean; layout?: "standalone" | "split"; isSinglePanelView?: boolean; profiles?: UserProfileLookup; @@ -88,6 +94,7 @@ export function AgentSessionThreadPanel({ canInterruptTurn, channel, channelId = null, + enterMotion = true, layout = "standalone", isSinglePanelView = false, profiles, @@ -458,6 +465,7 @@ export function AgentSessionThreadPanel({ return ( + agentSessionSelection.resolveSelectedAgentSession({ + agentSessionAgents, + openAgentSessionPubkey, + profilePanelPubkey, + profiles, + }), + [agentSessionAgents, openAgentSessionPubkey, profilePanelPubkey, profiles], + ); + // One resolution for both "which panel" and "which presentation", so the two + // cover drawers can never stack: there is a single covered slot and the + // resolved surface owns it. + const auxiliarySurface = resolveChannelAuxiliarySurface({ + channelManagementOpen, + hasActiveChannel: Boolean(activeChannel), + hasProfilePanel: Boolean(profilePanelPubkey), + hasSelectedAgent: Boolean(selectedAgent), + hasThreadHead: Boolean(threadHeadMessage), + shouldShowThreadSkeleton, + }); + const coverDrawer = resolveChannelCoverDrawer({ + surface: auxiliarySurface, + threadViewMode, + useSplitAuxiliaryPane, + }); + const useFocusThreadDrawer = coverDrawer === "thread"; + const useAgentActivityDrawer = coverDrawer === "agent-session"; + const { channelIsCovered, markExitComplete } = useCoverDrawerPresence( + coverDrawer !== null, + useAgentActivityDrawer ? onCloseAgentSession : onCloseThread, ); const pendingMainEditRef = React.useRef(null); const editTargetRef = React.useRef(editTarget); @@ -530,23 +563,8 @@ export const ChannelPane = React.memo(function ChannelPane({ onExternalTargetResolved: onThreadScrollTargetResolved, onModeChange: markExitComplete, }); - const selectedAgent = React.useMemo( - () => - agentSessionSelection.resolveSelectedAgentSession({ - agentSessionAgents, - openAgentSessionPubkey, - profilePanelPubkey, - profiles, - }), - [agentSessionAgents, openAgentSessionPubkey, profilePanelPubkey, profiles], - ); const hasSplitAuxiliaryPane = - useSplitAuxiliaryPane && - (channelManagementOpen || - Boolean(threadHeadMessage) || - shouldShowThreadSkeleton || - Boolean(activeChannel && selectedAgent) || - Boolean(profilePanelPubkey)); + useSplitAuxiliaryPane && auxiliarySurface !== null; const wrapAux = ( panel: React.ReactNode, testId: string, @@ -579,6 +597,20 @@ export const ChannelPane = React.memo(function ChannelPane({ ) : ( wrapAux(panel, "message-thread-panel", { key: THREAD_SURFACE_KEY }) ); + const wrapAgentSessionPanel = (panel: React.ReactNode) => + useAgentActivityDrawer ? ( + + {panel} + + ) : ( + wrapAux(panel, "agent-session-thread-panel", { + key: AGENT_SESSION_SURFACE_KEY, + }) + ); const threadHeaderLeading = useSplitAuxiliaryPane ? ( ) : undefined; @@ -588,6 +620,11 @@ export const ChannelPane = React.memo(function ChannelPane({ isSinglePanelView, useSplitAuxiliaryPane, }); + const agentSessionLayoutProps = getAgentSessionPanelPresentation({ + isCoverDrawer: useAgentActivityDrawer, + isSinglePanelView, + useSplitAuxiliaryPane, + }); const timelineReplyHandler = activeChannel?.archivedAt || isHuddleTranscript ? undefined : onOpenThread; return ( @@ -826,15 +863,15 @@ export const ChannelPane = React.memo(function ChannelPane({ ) : null} {/* - * `AnimatePresence` keeps the focus thread drawer mounted through its exit - * animation — without it the drawer's own existence condition - * (`useFocusThreadDrawer`, which is derived from `threadHeadMessage`) goes - * false on the same frame as the close, and there is nothing left to - * animate. It can hold the real thread through the exit rather than a - * frozen snapshot because the panel is fully prop-driven. + * `AnimatePresence` keeps a cover drawer mounted through its exit + * animation — without it the drawer's own existence condition (derived + * from `threadHeadMessage` / the selected agent) goes false on the same + * frame as the close, and there is nothing left to animate. It can hold + * the real content through the exit rather than a frozen snapshot because + * both panels are fully prop-driven. */} - {channelManagementOpen && activeChannel ? ( + {auxiliarySurface === "channel-management" && activeChannel ? ( - ) : threadHeadMessage ? ( + ) : auxiliarySurface === "thread" && threadHeadMessage ? ( (() => { const panel = ( { if (isHuddleTranscript) { return wrapThreadPanel(); @@ -938,7 +975,9 @@ export const ChannelPane = React.memo(function ChannelPane({ ); return wrapThreadPanel(panel); })() - ) : activeChannel && selectedAgent ? ( + ) : auxiliarySurface === "agent-session" && + activeChannel && + selectedAgent ? ( (() => { // When the panel was opened from a different channel than the // currently active one, re-scope it to the active channel so @@ -966,20 +1005,16 @@ export const ChannelPane = React.memo(function ChannelPane({ : null } channelId={effectiveAgentSessionChannelId} - isSinglePanelView={ - useSplitAuxiliaryPane ? false : isSinglePanelView - } - layout={useSplitAuxiliaryPane ? "split" : "standalone"} - transparentChrome={useSplitAuxiliaryPane} + {...agentSessionLayoutProps} profiles={profiles} onBack={onBackFromAgentSession} onClose={onCloseAgentSession} widthPx={threadPanelWidthPx} /> ); - return wrapAux(panel, "agent-session-thread-panel"); + return wrapAgentSessionPanel(panel); })() - ) : profilePanelPubkey ? ( + ) : auxiliarySurface === "profile" && profilePanelPubkey ? ( (() => { const panel = ( void; + /** + * Whether the drawer claims Escape for itself, ahead of anything inside it. + * + * Claiming it means a single press always leaves, even from a nested control + * that would otherwise handle the key. Leave this off when the drawer's own + * content already closes on Escape through `useEscapeKey`, which yields to + * nested controls that mark the event handled. Defaults to claiming. + */ + ownsEscape?: boolean; + /** + * Whether content inside the drawer currently owns Escape ahead of the + * drawer's own claim. + * + * Only meaningful while `ownsEscape` is set. A capture-phase claim runs before + * anything inside the drawer, so a drawer that unconditionally closes on + * Escape would dismiss the whole surface out from under an in-progress edit + * instead of letting that edit cancel first — losing the draft. Setting this + * yields the press to the drawer's own subtree for exactly that case; presses + * from outside the drawer still close it, so it cannot be wedged open. + */ + 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. + */ + testId: string; +}; + +/** + * Scrim over the channel content area behind a cover drawer. + * + * Veil, not shadow, and no blur: the channel fades toward the surface colour + * rather than being darkened. A black wash is a multiply — it scales text and + * background down together, so dark-on-light text keeps its contrast ratio and + * stays readable at any opacity short of a solid bar. Fading toward + * `background` instead compresses text against the surface in both themes, + * which is what pushes the sliver back to colour and shape. Matches the shared + * header backdrop's `bg-background/80` vocabulary, a touch heavier because this + * one has to defeat body text rather than sit over a gap. + */ +const COVER_SCRIM_CLASS = "bg-background/75 dark:bg-background/80"; + +/** + * Hover eases the veil one step in both themes. + * + * Feedback that the sliver is a target — deliberately not a peek: one step is + * enough to register as interactive without making the channel readable. + */ +const COVER_SCRIM_HOVER_CLASS = + "hover:bg-background/65 dark:hover:bg-background/70"; + +/** Arrive and settle. The iOS sheet curve, shared with `buzz-side-panel-enter`. */ +const ENTER_EASE = [0.32, 0.72, 0, 1] as const; + +/** + * Leave immediately. Shares the enter's fast-start shape rather than the + * conventional accelerating ease-in for exits. + * + * The "exits accelerate away" rule assumes the whole travel is visible; an + * ease-in spends its opening frames barely moving and pays that back at the end. + * Here the tail is hidden under the opacity fade, so acceleration buys nothing + * and those opening frames are the entire perception of responsiveness — a + * dismissal that hasn't visibly moved 40ms in reads as hesitation regardless of + * its total duration. Decisiveness comes from the duration below instead. + */ +const EXIT_EASE = ENTER_EASE; + +const SCRIM_ENTER_SECONDS = 0.2; + +/** + * Slightly ahead of the drawer's exit, and deliberately so. + * + * A scrim that outlasts the drawer leaves the channel dimmed with nothing on top + * of it, which reads as lag at the exact moment the user has committed to + * leaving. Undimming first hands the channel back the instant it is asked for. + */ +const SCRIM_EXIT_SECONDS = 0.12; + +/** + * Enter: opacity front-loaded, transform long. + * + * The two channels animate over deliberately different windows, and that + * asymmetry is the whole point. Short travel *requires* an opacity fade — an + * opaque surface this large appearing 120px off its mark with no fade is a hard + * cut, not a slide. But pairing both properties on one timing function (as a + * single CSS keyframe must) welds them together for the full duration, and since + * opacity covers 100% of its range while transform covers ~3% of the drawer's + * width, the fade is what the eye reads. Resolving opacity in the first ~90ms + * leaves the remaining ~190ms as pure travel: the fade is over before it + * registers, and what's perceived is sliding. + * + * It also keeps the drawer's own entrance from exposing its contents' load + * order. Anything arriving late (replies resolving, media decoding) lands on an + * already-opaque surface and reads as "the panel is loading" rather than the UI + * assembling itself. + */ +const ENTER_TRANSITION = { + opacity: { duration: 0.09, ease: "linear" }, + x: { duration: 0.28, ease: ENTER_EASE }, +} as const; + +/** + * Exit: half the enter's duration, opacity barely back-loaded. + * + * Opening and closing are not symmetric tasks. The enter has something to say — + * it establishes where the panel came from and that the channel is still behind + * it. The exit has nothing to say: attention has already left for the channel, + * so its only job is to get out of the way without popping. That makes duration + * the thing to spend, and 140ms is about the floor before the drawer reads as + * vanishing rather than leaving. + * + * The opacity hold shrinks with it. Its purpose is to let the drawer commit to + * moving before it dissolves, so it reads as sliding out — but at this duration a + * hold proportional to the old one would eat half the animation. 20ms is enough + * to register solidity in the first frame or two. + */ +const EXIT_TRANSITION = { + opacity: { delay: 0.02, duration: 0.12, ease: "linear" }, + x: { duration: 0.14, ease: EXIT_EASE }, +} as const; + +/** + * Reduced motion keeps a crossfade and drops the travel. + * + * Travel is the part that's motion; the fade is what makes appearing and + * disappearing legible. With `x` pinned to 0 the front/back-loaded opacity + * timings would read as dead air on a stationary surface, so both collapse to + * one short symmetric fade. + */ +const REDUCED_MOTION_TRANSITION = { duration: 0.12, ease: "linear" } as const; + +/** + * Right-anchored drawer that overlays the channel content area. + * + * Presentation only — it knows nothing about what it covers the channel with. + * The thread focus drawer and the agent activity drawer are both this surface + * with different contents and different open conditions. + * + * Must be rendered inside `ChannelPane`'s relative layout root, and beneath an + * `AnimatePresence` so the exit animation can run: everything here is absolutely + * positioned against the channel content area, so the app sidebar is never + * covered. The channel stays mounted underneath — a narrow scrim-dimmed sliver + * of it remains visible for depth, and the whole scrim (sliver included) is one + * tall click target back to the channel. Orientation lives in the drawer's own + * header, where the eye already is — the sliver carries no label of its own. + * + * `z-41` places the drawer above the channel section (whose inner `isolate` + * wrapper traps the timeline's z-50 pill, z-40 composer overlay, and z-50 drop + * overlay) and the `z-30` shared header backdrop, while staying below the + * global `z-45` top chrome. Setting z-index on the positioned container also + * gives the drawer its own stacking context, so the panel chrome inside is + * isolated. + */ +export function CoverDrawer({ + ariaLabel, + children, + escapeYieldsToContent = false, + 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; + + function handleEscape(event: KeyboardEvent) { + if (event.key !== "Escape") return; + // Yield to an in-progress edit inside the drawer: the capture-phase claim + // runs first, so without this the press would dismiss the whole surface + // and lose the draft instead of cancelling the edit. Scoped to the + // drawer's own subtree, so a press from outside still closes it. + const target = event.target; + if ( + escapeYieldsToContent && + target instanceof Node && + drawerRef.current?.contains(target) + ) { + return; + } + event.preventDefault(); + event.stopImmediatePropagation(); + onClose(); + } + + window.addEventListener("keydown", handleEscape, { capture: true }); + return () => { + window.removeEventListener("keydown", handleEscape, { capture: true }); + }; + }, [escapeYieldsToContent, onClose, ownsEscape]); + + React.useLayoutEffect(() => { + previousFocusRef.current = + document.activeElement instanceof HTMLElement + ? document.activeElement + : null; + drawerRef.current?.focus({ preventScroll: true }); + + return () => { + const previousFocus = previousFocusRef.current; + const shouldRestoreFocus = shouldRestoreFocusRef.current; + requestAnimationFrame(() => { + if (shouldRestoreFocus && !shouldRestoreFocus()) return; + previousFocus?.focus({ preventScroll: true }); + }); + }; + }, []); + + return ( +
+ + + +
{children}
+ +
+ ); +} diff --git a/desktop/src/features/channels/ui/FocusThreadDrawer.tsx b/desktop/src/features/channels/ui/FocusThreadDrawer.tsx index 410f2d8672..8dcbee8a26 100644 --- a/desktop/src/features/channels/ui/FocusThreadDrawer.tsx +++ b/desktop/src/features/channels/ui/FocusThreadDrawer.tsx @@ -1,141 +1,33 @@ -import { motion, useReducedMotion } from "motion/react"; -import * as React from "react"; +import type * as React from "react"; -import { - THREAD_FOCUS_DRAWER_TRAVEL_PX, - THREAD_FOCUS_SLIVER_WIDTH_PX, -} from "@/features/channels/lib/threadFocusLayout"; import { getThreadViewMode } from "@/features/channels/lib/threadViewModePreference"; -import { cn } from "@/shared/lib/cn"; +import { CoverDrawer } from "@/features/channels/ui/CoverDrawer"; type FocusThreadDrawerProps = { channelName: string; children: React.ReactNode; + /** + * Whether the thread has an edit in progress, which Escape must cancel before + * it can dismiss the drawer. See `CoverDrawer`'s `escapeYieldsToContent`. + */ hasActiveEdit: boolean; onClose: () => void; }; /** - * Scrim over the channel content area behind the focus drawer. - * - * Veil, not shadow, and no blur: the channel fades toward the surface colour - * rather than being darkened. A black wash is a multiply — it scales text and - * background down together, so dark-on-light text keeps its contrast ratio and - * stays readable at any opacity short of a solid bar. Fading toward - * `background` instead compresses text against the surface in both themes, - * which is what pushes the sliver back to colour and shape. Matches the shared - * header backdrop's `bg-background/80` vocabulary, a touch heavier because this - * one has to defeat body text rather than sit over a gap. - */ -const FOCUS_SCRIM_CLASS = "bg-background/75 dark:bg-background/80"; - -/** - * Hover eases the veil one step in both themes. - * - * Feedback that the sliver is a target — deliberately not a peek: one step is - * enough to register as interactive without making the channel readable. - */ -const FOCUS_SCRIM_HOVER_CLASS = - "hover:bg-background/65 dark:hover:bg-background/70"; - -/** Arrive and settle. The iOS sheet curve, shared with `buzz-side-panel-enter`. */ -const ENTER_EASE = [0.32, 0.72, 0, 1] as const; - -/** - * Leave immediately. Shares the enter's fast-start shape rather than the - * conventional accelerating ease-in for exits. - * - * The "exits accelerate away" rule assumes the whole travel is visible; an - * ease-in spends its opening frames barely moving and pays that back at the end. - * Here the tail is hidden under the opacity fade, so acceleration buys nothing - * and those opening frames are the entire perception of responsiveness — a - * dismissal that hasn't visibly moved 40ms in reads as hesitation regardless of - * its total duration. Decisiveness comes from the duration below instead. + * A real dismissal keeps focus mode selected; a presentation switch has already + * selected split mode and owns focus inside the new panel. */ -const EXIT_EASE = ENTER_EASE; - -const SCRIM_ENTER_SECONDS = 0.2; - -/** - * Slightly ahead of the drawer's exit, and deliberately so. - * - * A scrim that outlasts the drawer leaves the channel dimmed with nothing on top - * of it, which reads as lag at the exact moment the user has committed to - * leaving. Undimming first hands the channel back the instant it is asked for. - */ -const SCRIM_EXIT_SECONDS = 0.12; - -/** - * Enter: opacity front-loaded, transform long. - * - * The two channels animate over deliberately different windows, and that - * asymmetry is the whole point. Short travel *requires* an opacity fade — an - * opaque surface this large appearing 120px off its mark with no fade is a hard - * cut, not a slide. But pairing both properties on one timing function (as a - * single CSS keyframe must) welds them together for the full duration, and since - * opacity covers 100% of its range while transform covers ~3% of the drawer's - * width, the fade is what the eye reads. Resolving opacity in the first ~90ms - * leaves the remaining ~190ms as pure travel: the fade is over before it - * registers, and what's perceived is sliding. - * - * It also keeps the drawer's own entrance from exposing its contents' load - * order. Anything arriving late (replies resolving, media decoding) lands on an - * already-opaque surface and reads as "the thread is loading" rather than the UI - * assembling itself. - */ -const ENTER_TRANSITION = { - opacity: { duration: 0.09, ease: "linear" }, - x: { duration: 0.28, ease: ENTER_EASE }, -} as const; - -/** - * Exit: half the enter's duration, opacity barely back-loaded. - * - * Opening and closing are not symmetric tasks. The enter has something to say — - * it establishes where the thread came from and that the channel is still behind - * it. The exit has nothing to say: attention has already left for the channel, - * so its only job is to get out of the way without popping. That makes duration - * the thing to spend, and 140ms is about the floor before the drawer reads as - * vanishing rather than leaving. - * - * The opacity hold shrinks with it. Its purpose is to let the drawer commit to - * moving before it dissolves, so it reads as sliding out — but at this duration a - * hold proportional to the old one would eat half the animation. 20ms is enough - * to register solidity in the first frame or two. - */ -const EXIT_TRANSITION = { - opacity: { delay: 0.02, duration: 0.12, ease: "linear" }, - x: { duration: 0.14, ease: EXIT_EASE }, -} as const; - -/** - * Reduced motion keeps a crossfade and drops the travel. - * - * Travel is the part that's motion; the fade is what makes appearing and - * disappearing legible. With `x` pinned to 0 the front/back-loaded opacity - * timings would read as dead air on a stationary surface, so both collapse to - * one short symmetric fade. - */ -const REDUCED_MOTION_TRANSITION = { duration: 0.12, ease: "linear" } as const; +function shouldRestoreFocusOnClose() { + return getThreadViewMode() === "focus"; +} /** - * Right-anchored thread drawer that overlays the channel content area. - * - * Must be rendered inside `ChannelPane`'s relative layout root, and beneath an - * `AnimatePresence` so the exit animation can run: everything here is absolutely - * positioned against the channel content area, so the app sidebar is never - * covered. The channel stays mounted underneath — a narrow scrim-dimmed sliver - * of it remains visible for depth, and the whole scrim (sliver included) is one - * tall click target back to the channel. Orientation lives in the drawer - * header's breadcrumb, where the eye already is — the sliver carries no label of - * its own. + * The focus-mode thread presentation: a {@link CoverDrawer} holding the thread. * - * `z-41` places the drawer above the channel section (whose inner `isolate` - * wrapper traps the timeline's z-50 pill, z-40 composer overlay, and z-50 drop - * overlay) and the `z-30` shared header backdrop, while staying below the - * global `z-45` top chrome. Setting z-index on the positioned container also - * gives the drawer its own stacking context, so the panel chrome inside is - * isolated. + * 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. */ export function FocusThreadDrawer({ channelName, @@ -143,113 +35,16 @@ export function FocusThreadDrawer({ hasActiveEdit, onClose, }: FocusThreadDrawerProps) { - const prefersReducedMotion = useReducedMotion(); - const travelPx = prefersReducedMotion ? 0 : THREAD_FOCUS_DRAWER_TRAVEL_PX; - const drawerRef = React.useRef(null); - const previousFocusRef = React.useRef(null); - - React.useEffect(() => { - function handleEscape(event: KeyboardEvent) { - if (event.key !== "Escape") return; - const target = event.target; - if ( - hasActiveEdit && - target instanceof Node && - drawerRef.current?.contains(target) - ) { - return; - } - event.preventDefault(); - event.stopImmediatePropagation(); - onClose(); - } - - window.addEventListener("keydown", handleEscape, { capture: true }); - return () => { - window.removeEventListener("keydown", handleEscape, { capture: true }); - }; - }, [hasActiveEdit, onClose]); - - React.useLayoutEffect(() => { - previousFocusRef.current = - document.activeElement instanceof HTMLElement - ? document.activeElement - : null; - drawerRef.current?.focus({ preventScroll: true }); - - return () => { - const previousFocus = previousFocusRef.current; - requestAnimationFrame(() => { - // A real dismissal keeps focus mode selected; a presentation switch - // has already selected split mode and owns focus inside the new panel. - if (getThreadViewMode() === "focus") { - previousFocus?.focus({ preventScroll: true }); - } - }); - }; - }, []); - return ( -
- - - -
{children}
- -
+ {children} + ); } diff --git a/desktop/src/features/channels/ui/useFocusDrawerPresence.ts b/desktop/src/features/channels/ui/useCoverDrawerPresence.ts similarity index 68% rename from desktop/src/features/channels/ui/useFocusDrawerPresence.ts rename to desktop/src/features/channels/ui/useCoverDrawerPresence.ts index 271c867ae3..3d725cc979 100644 --- a/desktop/src/features/channels/ui/useFocusDrawerPresence.ts +++ b/desktop/src/features/channels/ui/useCoverDrawerPresence.ts @@ -1,9 +1,9 @@ import * as React from "react"; -import { subscribeToFocusedThreadCloseRequest } from "@/features/channels/focusedThreadCloseRequest"; +import { subscribeToCoverDrawerCloseRequest } from "@/features/channels/coverDrawerCloseRequest"; /** Keeps the covered channel inert and owns external dismissal while open. */ -export function useFocusDrawerPresence(open: boolean, onClose: () => void) { +export function useCoverDrawerPresence(open: boolean, onClose: () => void) { const [present, setPresent] = React.useState(false); React.useEffect(() => { @@ -12,7 +12,7 @@ export function useFocusDrawerPresence(open: boolean, onClose: () => void) { React.useEffect(() => { if (!open) return; - return subscribeToFocusedThreadCloseRequest(onClose); + return subscribeToCoverDrawerCloseRequest(onClose); }, [onClose, open]); const markExitComplete = React.useCallback(() => setPresent(false), []); diff --git a/desktop/tests/e2e/activity-scope-label-screenshots.spec.ts b/desktop/tests/e2e/activity-scope-label-screenshots.spec.ts index 4ed0a211e5..242f58d152 100644 --- a/desktop/tests/e2e/activity-scope-label-screenshots.spec.ts +++ b/desktop/tests/e2e/activity-scope-label-screenshots.spec.ts @@ -6,8 +6,11 @@ import { installMockBridge, TEST_IDENTITIES } from "../helpers/bridge"; const SHOTS = "test-results/activity-scope-label"; const AGENT_PUBKEY = TEST_IDENTITIES.tyler.pubkey; +// Long enough to overflow the widest presentation the pane has (the cover +// drawer, which is far wider than the split pane), so the truncation assertion +// below measures real clamping rather than an accident of panel width. const LONG_AGENT_NAME = - "Observer Agent With An Exceptionally Long Display Name"; + "Observer Agent With An Exceptionally Long Display Name That Keeps Going Well Past Any Reasonable Header Width"; const AGENTS_CHANNEL_ID = "94a444a4-c0a3-5966-ab05-530c6ddc2301"; // #agents // Open the activity pane via profile → "View activity" (same ingress the diff --git a/desktop/tests/e2e/agent-activity-cover.spec.ts b/desktop/tests/e2e/agent-activity-cover.spec.ts new file mode 100644 index 0000000000..ae4c5e189a --- /dev/null +++ b/desktop/tests/e2e/agent-activity-cover.spec.ts @@ -0,0 +1,234 @@ +import { expect, test, type Page } from "@playwright/test"; + +import { KIND_TYPING_INDICATOR } from "../../src/shared/constants/kinds"; +import { TEST_IDENTITIES, installMockBridge } from "../helpers/bridge"; + +const AGENT_PUBKEY = TEST_IDENTITIES.alice.pubkey; + +/** Two panes fit, so the agent panel covers. */ +const WIDE_VIEWPORT = { width: 1280, height: 800 }; + +/** Below the two-pane breakpoint, so today's presentation is unchanged. */ +const NARROW_VIEWPORT = { width: 860, height: 800 }; + +async function waitForMockLiveSubscription( + page: Page, + channelName: string, + kind?: number, +) { + await expect + .poll(() => + page.evaluate( + ({ currentChannelName, currentKind }) => + ( + window as Window & { + __BUZZ_E2E_HAS_MOCK_LIVE_SUBSCRIPTION__?: (input: { + channelName: string; + kind?: number; + }) => boolean; + } + ).__BUZZ_E2E_HAS_MOCK_LIVE_SUBSCRIPTION__?.({ + channelName: currentChannelName, + kind: currentKind, + }) ?? false, + { currentChannelName: channelName, currentKind: kind }, + ), + ) + .toBe(true); +} + +async function seedThreadRoot(page: Page) { + await expect + .poll(() => + page.evaluate( + () => typeof window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__ === "function", + ), + ) + .toBe(true); + return page.evaluate(() => { + const root = window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "agents", + content: "Cover drawer exclusivity thread", + createdAt: 1_700_800_000, + }); + if (!root) throw new Error("Failed to seed thread root"); + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "agents", + content: "A reply so the thread summary renders.", + parentEventId: root.id, + createdAt: 1_700_800_001, + }); + return root.id; + }); +} + +/** + * Opens the agent activity panel from the composer activity bar — the ingress + * that has no prior pane, so the header shows close and no back arrow. + */ +async function openActivityFromComposer(page: Page) { + await page.getByTestId("channel-agents").click(); + await expect(page.getByTestId("chat-title")).toHaveText("agents"); + await waitForMockLiveSubscription(page, "agents", KIND_TYPING_INDICATOR); + + await page.evaluate((pubkey) => { + window.__BUZZ_E2E_EMIT_MOCK_TYPING__?.({ + channelName: "agents", + pubkey, + }); + }, AGENT_PUBKEY); + + const trigger = page.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 }); + await expect(page.getByTestId("agent-session-thread-panel")).toBeVisible(); +} + +/** + * 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. + * Re-seeding her through `managedAgents` instead *replaces* that relay-agent + * row with a managed one that is not in the channel's working-agent set, so the + * trigger never renders — use the default seed. + */ +test.beforeEach(async ({ page }) => { + await installMockBridge(page); +}); + +test("agent activity covers the channel at wide viewports", async ({ + page, +}) => { + await page.setViewportSize(WIDE_VIEWPORT); + await page.goto("/"); + await openActivityFromComposer(page); + + const channel = page.getByTestId("channel-drop-zone"); + const drawer = page.getByTestId("agent-activity-drawer"); + const panel = page.getByTestId("agent-session-thread-panel"); + + // Covering, not splitting: the panel lives inside the drawer, the channel is + // inert behind it, and there is no split pane to resize. + await expect(drawer).toBeVisible(); + await expect(drawer.getByTestId("agent-session-thread-panel")).toBeVisible(); + await expect(channel).toHaveAttribute("inert", ""); + await expect( + page.getByTestId("right-auxiliary-pane-resize-handle"), + ).toHaveCount(0); + + // Activity never offers the thread's focus/split switch. + await expect(page.getByTestId("thread-view-mode-toggle")).toHaveCount(0); + + // The drawer owns the entrance, so the panel must not slide too — a second + // animation inside a moving container compounds into a double slide. + await expect(panel).not.toHaveClass(/buzz-side-panel-enter/); + + // Wide enough to read a transcript: the drawer takes the channel content + // area less the sliver, so it is far wider than the split pane it replaces. + const drawerWidth = (await drawer.boundingBox())?.width ?? 0; + expect(drawerWidth).toBeGreaterThan(700); + + // The drawer captures focus, and the panel keeps its close affordance. + await expect + .poll(() => + page.evaluate(() => + Boolean( + document + .querySelector('[data-testid="agent-activity-drawer"]') + ?.contains(document.activeElement), + ), + ), + ) + .toBe(true); + await expect(page.getByTestId("agent-session-back")).toHaveCount(0); + await expect(page.getByTestId("auxiliary-panel-close")).toBeVisible(); + + // Escape leaves — and the settings menu still gets its own press first. + await page.getByTestId("agent-session-settings-menu-trigger").click(); + await expect(page.getByTestId("agent-session-stop-turn")).toBeVisible(); + await page.keyboard.press("Escape"); + await expect(page.getByTestId("agent-session-stop-turn")).toHaveCount(0); + await expect(drawer).toBeVisible(); + await page.keyboard.press("Escape"); + await expect(page.getByTestId("agent-activity-drawer-overlay")).toHaveCount( + 0, + ); + await expect(panel).toHaveCount(0); + await expect(channel).not.toHaveAttribute("inert", ""); + + // The scrim is the click target back to the channel. + await openActivityFromComposer(page); + await expect(drawer).toBeVisible(); + await page + .getByTestId("agent-activity-drawer-scrim") + .click({ position: { x: 24, y: 300 } }); + await expect(page.getByTestId("agent-activity-drawer-overlay")).toHaveCount( + 0, + ); + await expect(channel).not.toHaveAttribute("inert", ""); +}); + +test("only one cover drawer is ever open", async ({ page }) => { + await page.setViewportSize(WIDE_VIEWPORT); + await page.addInitScript(() => { + localStorage.setItem("buzz.channels.threadViewMode", "focus"); + }); + await page.goto("/"); + const rootId = await seedThreadRoot(page); + await openActivityFromComposer(page); + + const agentDrawer = page.getByTestId("agent-activity-drawer"); + const threadDrawer = page.getByTestId("focus-thread-drawer"); + await expect(agentDrawer).toBeVisible(); + + // 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(); + await expect(threadDrawer).toBeVisible(); + await expect(agentDrawer).toHaveCount(0); + + // 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); + await expect(agentDrawer).toBeVisible(); + await expect(page.getByTestId("focus-thread-drawer-overlay")).toHaveCount(0); +}); + +test("narrow viewports keep the existing activity presentation", async ({ + page, +}) => { + await page.setViewportSize(NARROW_VIEWPORT); + await page.goto("/"); + await openActivityFromComposer(page); + + await expect(page.getByTestId("agent-session-thread-panel")).toBeVisible(); + await expect(page.getByTestId("agent-activity-drawer")).toHaveCount(0); + await expect(page.getByTestId("agent-activity-drawer-overlay")).toHaveCount( + 0, + ); + await expect(page.getByTestId("agent-activity-drawer-scrim")).toHaveCount(0); + // Below the breakpoint the channel is replaced rather than covered, so the + // pane it would be made inert behind is not rendered at all — and nothing + // else on the page is inert either. + await expect(page.getByTestId("channel-drop-zone")).toHaveCount(0); + expect(await page.locator("[inert]").count()).toBe(0); +}); From 1afe30f6503222382bb15686aae09f9f54f7d0f7 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Fri, 21 Aug 2026 20:41:21 -0700 Subject: [PATCH 2/8] fix(desktop): coordinate cover drawer focus with a shared slot claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **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 `` 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.
File changes **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.
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 `` 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 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- .../channels/lib/channelAuxiliarySurface.ts | 19 +- .../lib/coverDrawerFocusSlot.test.mjs | 65 +++++++ .../channels/lib/coverDrawerFocusSlot.ts | 52 +++++ .../src/features/channels/ui/CoverDrawer.tsx | 24 +-- .../ui/CoverDrawerFocusHandoff.test.mjs | 165 ++++++++++++++++ .../channels/ui/FocusThreadDrawer.tsx | 18 +- ...useChannelAgentSessionExclusivity.test.mjs | 177 ++++++++++++++++++ .../channels/ui/useThreadViewModeSwitch.ts | 5 + .../tests/e2e/agent-activity-cover.spec.ts | 153 ++++++++++++--- 9 files changed, 622 insertions(+), 56 deletions(-) create mode 100644 desktop/src/features/channels/lib/coverDrawerFocusSlot.test.mjs create mode 100644 desktop/src/features/channels/lib/coverDrawerFocusSlot.ts create mode 100644 desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs create mode 100644 desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs 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 ({ From 1ff5968dae00ef2f8df367de4f03dccec769cb61 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 11:46:30 -0700 Subject: [PATCH 3/8] test(desktop): promote the cover drawer preview into a reference screenshot spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The throwaway spec used to compare presentation options has now paid for itself twice, so give it a permanent home next to the other activity screenshot specs and register it in the smoke project. One seeded turn with the shape a real investigation has: a `@mention` that starts it, thinking, three file reads, a shell command, a relay post, a step that failed, a plan, and an answer containing code. The prompt is framed the way the harness frames it — a `[Buzz event: ...]` section with `From:`/`Content:` lines — because `parsePromptText` reads the author pubkey and the user-visible text out of exactly that shape; a plain text prompt would render as an unattributed bubble and would not exercise the header the drawer exists to make readable. Three frames, all at the drawer's design width: the full window as it opens (the drawer against the sliver and the scrimmed channel, which a panel-only shot cannot show), then the expanded turn at its head and at its tail. Expanding matters — the transcript opens folded, so the default frame is a stack of one-line summaries that shows none of the turn's shape. Expanded it is taller than the drawer, hence two frames. The PNGs are the deliverable, not a pixel baseline, so the assertions are scoped to what this slice owns: the drawer covers, the panel is mounted inside it, there is no split resize handle, and the turn actually rendered. Transcript structure, grouping, and styling belong to the transcript variants being restyled in parallel and are asserted by their own specs; asserting them here would make the reference shots fail for reasons unrelated to the drawer. For the same reason the helpers reach for DOM contracts rather than transcript markup — the scroller is found by computed overflow, and folds are opened through the native `
` `open` property rather than by clicking a variant-owned toggle. Two things the helpers guard rather than assume. The expander asserts it found at least one collapsed row, so a moved fold cannot silently turn the expanded shots into duplicates of the collapsed one. And the scroll-to-head polls a value read two frames after the write: the panel is tail-anchored and expanding pins it back to the bottom, so an immediate read would report the write instead of the outcome and the shot would quietly become a second tail frame. Signed-off-by: ss-dev-00 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- desktop/playwright.config.ts | 1 + .../agent-activity-cover-screenshots.spec.ts | 490 ++++++++++++++++++ 2 files changed, 491 insertions(+) create mode 100644 desktop/tests/e2e/agent-activity-cover-screenshots.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 7c6465cdff..ba90195309 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -96,6 +96,7 @@ export default defineConfig({ "**/threadpane-ultrawide.spec.ts", "**/thread-focus-mode.spec.ts", "**/agent-activity-cover.spec.ts", + "**/agent-activity-cover-screenshots.spec.ts", "**/animated-avatar.spec.ts", "**/reminders.spec.ts", "**/reminder-click-repro.spec.ts", diff --git a/desktop/tests/e2e/agent-activity-cover-screenshots.spec.ts b/desktop/tests/e2e/agent-activity-cover-screenshots.spec.ts new file mode 100644 index 0000000000..9ae78b738d --- /dev/null +++ b/desktop/tests/e2e/agent-activity-cover-screenshots.spec.ts @@ -0,0 +1,490 @@ +import { expect, test, type Page } from "@playwright/test"; + +import { waitForAnimations } from "../helpers/animations"; +import { installMockBridge, TEST_IDENTITIES } from "../helpers/bridge"; + +const SHOTS = "test-results/agent-activity-cover"; + +const AGENT_PUBKEY = TEST_IDENTITIES.tyler.pubkey; +const HUMAN_PUBKEY = TEST_IDENTITIES.bob.pubkey; +const CHANNEL_ID = "94a444a4-c0a3-5966-ab05-530c6ddc2301"; // #agents +const SESSION_ID = "session-cover-001"; +const TURN_ID = "turn-cover-001"; + +/** + * The width the drawer is designed for: two panes fit, so activity covers, and + * the drawer gets the channel content area less the sliver. Anything narrower + * is a different presentation with its own spec + * (`agent-activity-cover.spec.ts`), so the reference shot is taken here. + */ +const DRAWER_VIEWPORT = { width: 1440, height: 900 }; + +const MANAGED_AGENTS = [ + { + pubkey: AGENT_PUBKEY, + name: "Observer Agent", + status: "running" as const, + channelNames: ["agents"], + }, +]; + +/** + * Anchor the turn just before "now" so the header's relative recency label + * ("Last updated 1m ago") reads like a live session rather than an archive. + * Absolute timestamps inside the transcript are not asserted, so a moving + * anchor costs nothing and keeps the reference shot honest. + */ +const TURN_START_MS = Date.now() - 90_000; +const at = (offsetSeconds: number) => + new Date(TURN_START_MS + offsetSeconds * 1_000).toISOString(); + +type ObserverEventSeed = { + seq: number; + timestamp: string; + kind: string; + agentIndex: number | null; + channelId: string | null; + sessionId: string | null; + turnId: string | null; + payload: unknown; +}; + +let seq = 0; + +function sessionUpdate( + offsetSeconds: number, + update: unknown, +): ObserverEventSeed { + seq += 1; + return { + seq, + timestamp: at(offsetSeconds), + kind: "acp_read", + agentIndex: 0, + channelId: CHANNEL_ID, + sessionId: SESSION_ID, + turnId: TURN_ID, + payload: { + jsonrpc: "2.0", + method: "session/update", + params: { sessionId: SESSION_ID, update }, + }, + }; +} + +/** + * A tool call as the harness actually reports it: an `in_progress` announcement + * followed by a terminal update carrying the output. Seeding only the terminal + * update would skip the correlation path the transcript uses to pair them. + */ +function toolCall( + offsetSeconds: number, + input: { + args: Record; + failed?: boolean; + id: string; + output: string; + title: string; + toolName: string; + }, +): ObserverEventSeed[] { + return [ + sessionUpdate(offsetSeconds, { + sessionUpdate: "tool_call", + rawInput: input.args, + status: "in_progress", + title: input.title, + toolCallId: input.id, + toolName: input.toolName, + }), + sessionUpdate(offsetSeconds + 1, { + content: [ + { type: "content", content: { type: "text", text: input.output } }, + ], + rawInput: input.args, + sessionUpdate: "tool_call_update", + status: input.failed ? "failed" : "completed", + title: input.title, + toolCallId: input.id, + toolName: input.toolName, + }), + ]; +} + +/** + * One finished turn with the shape a real investigation has: a mention that + * starts it, thinking, file reads, a shell command, a relay post, a step that + * failed, a plan, and an answer containing code. + * + * The prompt is framed the way the harness frames it — a `[Buzz event: ...]` + * section with `From:`/`Content:` lines — because `parsePromptText` reads the + * author pubkey and the user-visible text out of exactly that shape. A plain + * text prompt would render as an unattributed bubble and would not exercise the + * header the drawer is meant to make readable. + */ +function buildTurnEvents(): ObserverEventSeed[] { + seq = 0; + const events: ObserverEventSeed[] = []; + + seq += 1; + events.push({ + seq, + timestamp: at(0), + kind: "acp_write", + agentIndex: 0, + channelId: CHANNEL_ID, + sessionId: SESSION_ID, + turnId: TURN_ID, + payload: { + jsonrpc: "2.0", + id: 1, + method: "session/prompt", + params: { + prompt: [ + { + type: "text", + text: [ + "[Buzz event: @mention]", + "Event ID: 4f1c8e6d2b7a90c3e5148af6b0d29c73518ea4d6c09b7f2318ad45e6019cb372", + "Channel: agents (#94a444a4-c0a3-5966-ab05-530c6ddc2301)", + "Kind: 9", + `From: bob (npub: npub1hv32jnyjyr9dwmlagvvsejul4j4ushx2vph2ghde5ktxxu6hlxcqzt5qsn, hex: ${HUMAN_PUBKEY})`, + "Time: 2026-08-24T18:04:11+00:00", + "Content: @Observer Agent the mention badge lands on the wrong channel row after a reconnect. Trace where the feed category is set and confirm whether the singular/plural mismatch is the cause. Post what you find here.", + ].join("\n"), + }, + { + type: "text", + text: "[Thread context]\nThis is the thread history with 3 prior messages.", + }, + ], + }, + }, + }); + + events.push( + sessionUpdate(4, { + sessionUpdate: "agent_thought_chunk", + messageId: "thought-1", + content: { + type: "text", + text: "The badge is driven by the feed category on the alert event, so a mismatch would show up where that string is built. Start at the emit site, then follow the value into the sidebar row selector.", + }, + }), + ); + + events.push( + ...toolCall(12, { + args: { path: "desktop/src/features/feed/lib/feedCategory.ts" }, + id: "call-read-1", + output: 'export type FeedCategory = "mention" | "reply" | "reaction";', + title: "read_file", + toolName: "buzz_dev_mcp__read_file", + }), + ...toolCall(15, { + args: { path: "desktop/src/features/feed/lib/alertRouting.ts" }, + id: "call-read-2", + output: 'if (category === "mentions") { routeToChannel(channelId); }', + title: "read_file", + toolName: "buzz_dev_mcp__read_file", + }), + ...toolCall(18, { + args: { path: "desktop/src/features/channels/ui/ChannelRowBadge.tsx" }, + id: "call-read-3", + output: 'const hasMention = categories.includes("mention");', + title: "read_file", + toolName: "buzz_dev_mcp__read_file", + }), + ); + + events.push( + ...toolCall(24, { + args: { + command: "rg -n 'mentions\"' desktop/src --glob '*.ts' --glob '*.tsx'", + }, + id: "call-shell-1", + output: [ + 'desktop/src/features/feed/lib/alertRouting.ts:41: if (category === "mentions") {', + 'desktop/src/features/feed/emitFeedAlert.ts:88: category: "mentions",', + 'desktop/src/features/feed/emitFeedAlert.test.mjs:12: category: "mentions",', + "", + "3 matches across 3 files", + ].join("\n"), + title: "shell", + toolName: "buzz_dev_mcp__shell", + }), + ); + + events.push( + ...toolCall(31, { + args: { + command: + "buzz messages send --channel 94a444a4-c0a3-5966-ab05-530c6ddc2301 --content 'Confirmed the plural/singular mismatch at the emit site.'", + }, + id: "call-relay-1", + output: '{\n "accepted": true,\n "event_id": "a41c9e2f…"\n}', + title: "shell", + toolName: "buzz_dev_mcp__shell", + }), + ); + + events.push( + ...toolCall(37, { + args: { command: "pnpm vitest run desktop/src/features/feed" }, + failed: true, + id: "call-fail-1", + output: + "Error: ENOENT: no such file or directory, scandir 'desktop/src/features/feed/__tests__'\n at readdirSync (node:fs:1478:26)\nexit code 1", + title: "shell", + toolName: "buzz_dev_mcp__shell", + }), + ); + + events.push( + sessionUpdate(44, { + sessionUpdate: "plan", + entries: [ + { content: "Trace the feed category emit site", status: "completed" }, + { + content: "Confirm the sidebar row reads the singular form", + status: "completed", + }, + { + content: "Add a regression test for the routing branch", + status: "pending", + }, + { content: "Open the fix PR", status: "pending" }, + ], + }), + ); + + events.push( + sessionUpdate(52, { + sessionUpdate: "agent_message_chunk", + messageId: "reply-1", + content: { + type: "text", + text: [ + 'Confirmed — it is the singular/plural mismatch, and it is on the emit side. `emitFeedAlert` writes `category: "mentions"` while every reader compares against the singular `"mention"`, so the alert never matches the mention branch and falls through to the generic channel-activity path. That is why the badge lands on whichever row was last touched rather than the mentioning channel.', + "", + "```ts", + "// desktop/src/features/feed/emitFeedAlert.ts", + '- category: "mentions",', + '+ category: "mention",', + "```", + "", + "The reader side needs no change. No fix pushed yet — the feed test directory the suite expects does not exist, so the regression test needs a home first.", + ].join("\n"), + }, + }), + ); + + return events; +} + +async function seedTurn(page: Page) { + await expect + .poll(() => + page.evaluate( + () => typeof window.__BUZZ_E2E_SEED_OBSERVER_EVENTS__ === "function", + ), + ) + .toBe(true); + await page.evaluate( + ({ evts, pubkey }) => { + window.__BUZZ_E2E_SEED_OBSERVER_EVENTS__?.({ + agentPubkey: pubkey, + events: evts, + }); + }, + { evts: buildTurnEvents(), pubkey: AGENT_PUBKEY }, + ); +} + +/** + * Composer activity bar → the agent's row. + * + * This ingress has no prior pane, so the drawer opens with its own close + * affordance and no back arrow — the presentation this reference shot is of. + */ +async function openActivityFromComposer(page: Page) { + await page.getByTestId("channel-agents").click(); + await expect(page.getByTestId("chat-title")).toHaveText("agents"); + + await page.evaluate((pubkey) => { + window.__BUZZ_E2E_EMIT_MOCK_TYPING__?.({ + channelName: "agents", + pubkey, + }); + }, AGENT_PUBKEY); + + const trigger = page.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 }); +} + +/** + * Scroll the transcript to the head of the turn and confirm it stayed there. + * + * Finds the scrolling ancestor by computed overflow rather than by class name: + * the transcript's styling belongs to the transcript variants, which are being + * restyled in parallel, and this spec must not break when they change. + * + * The panel is tail-anchored, and growing its content (by expanding folds) pins + * it back to the bottom — so a scroll issued while that is still settling gets + * undone a frame later and the shot silently becomes a second tail frame. + * Re-issues the scroll and then re-reads it on a later task, so the assertion + * only passes once the position actually survives a frame. + */ +async function scrollTranscriptToTop(page: Page) { + const panel = page.getByTestId("agent-session-thread-panel"); + const scrollToTopAndSettle = () => + panel.evaluate((element) => { + let node = element.querySelector('[role="log"]')?.parentElement ?? null; + while (node) { + const overflowY = window.getComputedStyle(node).overflowY; + if ( + (overflowY === "auto" || overflowY === "scroll") && + node.scrollHeight > node.clientHeight + ) { + const scroller = node; + scroller.scrollTop = 0; + // Read back after two frames: a re-pin from the tail anchor lands in + // an effect or rAF, so an immediate read would report the write rather + // than the outcome. + return new Promise((resolve) => { + requestAnimationFrame(() => + requestAnimationFrame(() => resolve(scroller.scrollTop)), + ); + }); + } + node = node.parentElement; + } + return Promise.resolve(-1); + }); + + await expect.poll(scrollToTopAndSettle, { timeout: 10_000 }).toBe(0); +} + +/** + * Scroll the transcript to the tail of the turn. + * + * Same scroller discovery as {@link scrollTranscriptToTop}; no re-pin race to + * fight here because the tail is where the anchor wants to be anyway. + */ +async function scrollTranscriptToBottom(page: Page) { + await page.getByTestId("agent-session-thread-panel").evaluate((element) => { + let node = element.querySelector('[role="log"]')?.parentElement ?? null; + while (node) { + const overflowY = window.getComputedStyle(node).overflowY; + if ( + (overflowY === "auto" || overflowY === "scroll") && + node.scrollHeight > node.clientHeight + ) { + node.scrollTop = node.scrollHeight; + return; + } + node = node.parentElement; + } + }); +} + +/** + * Open every collapsed row in the transcript. + * + * The transcript opens with thinking and tool runs folded, so the default frame + * is a stack of one-line summaries — true to the product, but it shows none of + * the turn's actual shape. Expanding gives the second shot the content the + * drawer's width exists for: command output, a failed step, plan items. + * + * Driven through the native `
` `open` property rather than by clicking + * a toggle, because the fold's markup and any toggle test id belong to the + * transcript variants being restyled in parallel. `open` is the DOM contract + * underneath whatever they render, and it survives their changes. + */ +async function expandTranscriptRows(page: Page) { + const expanded = await page + .getByTestId("agent-session-thread-panel") + .evaluate((panel) => { + const rows = panel.querySelectorAll("details:not([open])"); + for (const row of rows) { + (row as HTMLDetailsElement).open = true; + } + return rows.length; + }); + // A zero here would mean the folds moved and the shot silently became a + // duplicate of the collapsed one, which is the failure worth catching. + expect(expanded).toBeGreaterThan(0); +} + +/** + * Reference screenshots of a realistic agent turn in the cover drawer. + * + * The PNGs are the deliverable — they are what design and review look at, and + * regenerating them is the point of keeping this spec. So the assertions are + * deliberately limited to what Slice A owns: the drawer covers, the panel is + * mounted inside it, and there is no split resize handle. Transcript structure, + * grouping, and styling belong to the transcript variants and are asserted by + * their own specs; asserting them here would make the reference shots fail for + * reasons that have nothing to do with the drawer. + */ +test.describe("agent activity cover drawer screenshots", () => { + test.use({ viewport: DRAWER_VIEWPORT }); + + test("realistic turn in the cover drawer", async ({ page }) => { + await installMockBridge(page, { managedAgents: MANAGED_AGENTS }); + await page.goto("/", { waitUntil: "domcontentloaded" }); + + // Seed before opening so the panel has content on its first paint, and + // again after: the panel subscribes on mount, and re-seeding is how the + // observer store notifies an already-mounted subscriber. + await seedTurn(page); + await openActivityFromComposer(page); + await seedTurn(page); + + const drawer = page.getByTestId("agent-activity-drawer"); + const panel = page.getByTestId("agent-session-thread-panel"); + await expect(drawer).toBeVisible(); + await expect( + drawer.getByTestId("agent-session-thread-panel"), + ).toBeVisible(); + await expect( + page.getByTestId("right-auxiliary-pane-resize-handle"), + ).toHaveCount(0); + await expect(page.getByTestId("channel-drop-zone")).toHaveAttribute( + "inert", + "", + ); + + // The turn actually rendered — without this the shots could be of an empty + // drawer and still pass every structural assertion above. + await expect( + page.getByTestId("transcript-user-message").first(), + ).toBeVisible({ timeout: 10_000 }); + + await waitForAnimations(page); + // Full window: the drawer against the sliver and the scrimmed channel, + // which is the part of this presentation a panel-only shot cannot show. + // Folds are left as the product leaves them — collapsed on open. + await page.screenshot({ path: `${SHOTS}/01-turn-in-drawer.png` }); + + // Second shot expanded, at the head: this is the frame that shows what the + // drawer's width buys — the prompt, thinking, and the reads and shell output + // that are invisible while the runs are folded. Scrolled back to the head + // because expanding overflows the panel and it is anchored to the tail. + await expandTranscriptRows(page); + await scrollTranscriptToTop(page); + await waitForAnimations(page); + await panel.screenshot({ path: `${SHOTS}/02-turn-expanded-head.png` }); + + // Third shot, the tail of the same expanded turn: the failed step with its + // error, the plan, and the answer with code. Expanded, the turn is taller + // than the drawer, so no single frame holds both ends of it. + await scrollTranscriptToBottom(page); + await waitForAnimations(page); + await panel.screenshot({ path: `${SHOTS}/03-turn-expanded-tail.png` }); + }); +}); From fbc0cbf39db11bc065d65f9895d6e950bdf5a1e1 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 13:52:11 -0700 Subject: [PATCH 4/8] fix(desktop): capture the cover drawer's opener once across a StrictMode replay The drawer captured `document.activeElement` in a layout effect with an empty dependency list. The app root is wrapped in `React.StrictMode`, which in development replays every effect as setup -> cleanup -> setup, so by the second setup the drawer had already focused itself and recorded *itself* as the element it had covered. On a real close the deferred restore then focused a node React had detached, which is a silent no-op that drops focus to `` -- keyboard-stranded, no visible focus ring, in the exact dev build people use the feature in. A one-shot ref guards the capture. Refs survive the replay, so the second setup keeps the opener the first one recorded. The flag is deliberately not reset in cleanup: the only cleanup it sees before a real close is the simulated one, which is precisely what it exists to ignore. The focus-slot claim stays unconditional. Re-claiming on the replayed setup is what makes the first cleanup's deferred restore stand down, so the coordinator was already behaving correctly here -- only the capture was wrong. The existing focus-handoff tests could not see this because they render without StrictMode, so the new case renders under it via `render(..., { reactStrictMode: true })`. Verified to fail at the previous head (focus lands on `HTMLBodyElement` instead of the opener) and to pass with the guard. Signed-off-by: ss-dev-00 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- .../src/features/channels/ui/CoverDrawer.tsx | 33 ++++++++++++++++--- .../ui/CoverDrawerFocusHandoff.test.mjs | 20 +++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/desktop/src/features/channels/ui/CoverDrawer.tsx b/desktop/src/features/channels/ui/CoverDrawer.tsx index 02c242d691..4f775f89bc 100644 --- a/desktop/src/features/channels/ui/CoverDrawer.tsx +++ b/desktop/src/features/channels/ui/CoverDrawer.tsx @@ -184,6 +184,14 @@ export function CoverDrawer({ const travelPx = prefersReducedMotion ? 0 : COVER_DRAWER_TRAVEL_PX; const drawerRef = React.useRef(null); const previousFocusRef = React.useRef(null); + /** + * Whether the opener has been captured for this drawer instance. + * + * Distinct from `previousFocusRef.current === null`, which is a legitimate + * capture (nothing was focused) and must not be retried. See the capture + * effect for why one attempt is all this gets. + */ + const hasCapturedPreviousFocusRef = React.useRef(false); React.useEffect(() => { if (!ownsEscape) return; @@ -214,10 +222,27 @@ export function CoverDrawer({ }, [escapeYieldsToContent, onClose, ownsEscape]); React.useLayoutEffect(() => { - previousFocusRef.current = - document.activeElement instanceof HTMLElement - ? document.activeElement - : null; + // Capture the opener exactly once per drawer instance. + // + // `React.StrictMode` replays effects in development as setup → cleanup → + // setup, and by that second setup this drawer has already focused itself. An + // unconditional read of `document.activeElement` would therefore record the + // drawer as its own opener, and a real close would focus a node React has + // since detached — leaving focus on ``, keyboard-stranded. Refs survive + // the replay, so a one-shot flag is enough. The flag is deliberately not + // reset in cleanup: the only cleanup it would see before a real close is the + // simulated one, which is exactly what it exists to ignore. + // + // Re-claiming the focus slot on the replayed setup is correct and stays as + // is — that new generation is what makes the first cleanup's deferred + // restore stand down. + if (!hasCapturedPreviousFocusRef.current) { + hasCapturedPreviousFocusRef.current = true; + previousFocusRef.current = + document.activeElement instanceof HTMLElement + ? document.activeElement + : null; + } const focusClaim = claimCoverDrawerFocus(); drawerRef.current?.focus({ preventScroll: true }); diff --git a/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs b/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs index 74b94f8a9d..ce07ddc7e0 100644 --- a/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs +++ b/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs @@ -143,6 +143,26 @@ test("only the last-opened drawer keeps focus across a chain of replacements", a third.unmount(); }); +test("a drawer restores focus to the opener even when StrictMode replays its capture", async () => { + // The app root is wrapped in `React.StrictMode`, which in development runs + // every effect setup → cleanup → setup. By that second setup the drawer has + // already focused itself, so a capture that reads `document.activeElement` + // unconditionally records the drawer as its own opener; closing then focuses a + // node React has detached and focus falls to ``. Capture has to survive + // the replay, which is what the other tests here cannot see because they + // render without StrictMode. + const { act, drawer, opener, render } = await loadHarness(); + const view = render(drawer("strict-drawer"), { reactStrictMode: true }); + assert.equal(activeTestId(), "strict-drawer"); + + await act(async () => { + view.unmount(); + }); + await flushDeferredFocusRestore(act); + + assert.equal(dom.window.document.activeElement, opener); +}); + 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 From 80ba1f5ae060744e8a43c78dccbeb02d28ecfed7 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 14:21:06 -0700 Subject: [PATCH 5/8] test(desktop): cover Escape's yield to an in-progress edit in the cover drawer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** test `#6575` gave `FocusThreadDrawer` a `hasActiveEdit` prop so a capture-phase Escape claim yields to an edit composer inside the drawer instead of dismissing the surface and losing the draft. Rebasing this branch moved Escape ownership out of that file and into `CoverDrawer`, so the behaviour now lives behind `escapeYieldsToContent` — and `#6575`'s only regression test for it is an e2e case written against the thread drawer, which cannot see the agent drawer or the prop boundary itself. This adds jsdom coverage at the new seam: a press inside the drawer yields while content owns the key, closes when it does not, and a press from outside closes it either way, so the yield cannot wedge the drawer open. Verified to fail on the first case with the guard removed, while both negative controls keep passing. Signed-off-by: ss-dev-00 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- .../channels/ui/CoverDrawerEscape.test.mjs | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 desktop/src/features/channels/ui/CoverDrawerEscape.test.mjs diff --git a/desktop/src/features/channels/ui/CoverDrawerEscape.test.mjs b/desktop/src/features/channels/ui/CoverDrawerEscape.test.mjs new file mode 100644 index 0000000000..5f8132f2cb --- /dev/null +++ b/desktop/src/features/channels/ui/CoverDrawerEscape.test.mjs @@ -0,0 +1,114 @@ +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +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, + KeyboardEvent: dom.window.KeyboardEvent, + MutationObserver: dom.window.MutationObserver, + Node: dom.window.Node, + cancelAnimationFrame: dom.window.cancelAnimationFrame, + document: dom.window.document, + requestAnimationFrame: dom.window.requestAnimationFrame, + window: dom.window, + }); + Object.defineProperty(globalThis, "navigator", { + configurable: true, + value: dom.window.navigator, + writable: true, + }); + dom.window.matchMedia ??= () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, + }); +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); +}); + +after(() => dom.window.close()); + +/** + * Renders a drawer holding one focusable child, which stands in for the thread + * composer that owns Escape while an edit is in progress. + */ +async function renderDrawer({ escapeYieldsToContent }) { + const React = await import("react"); + const { render } = await import("@testing-library/react"); + const { CoverDrawer } = await import("./CoverDrawer.tsx"); + + const closes = []; + const view = render( + React.createElement( + CoverDrawer, + { + ariaLabel: "Thread", + escapeYieldsToContent, + onClose: () => closes.push("close"), + scrimLabel: "Back to #general", + testId: "cover-drawer", + }, + React.createElement("input", { "data-testid": "thread-composer" }), + ), + ); + + return { closes, view }; +} + +function pressEscapeOn(element) { + element.dispatchEvent( + new dom.window.KeyboardEvent("keydown", { + bubbles: true, + cancelable: true, + key: "Escape", + }), + ); +} + +function composer() { + return dom.window.document.querySelector('[data-testid="thread-composer"]'); +} + +test("Escape inside the drawer yields to content while it owns the key", async () => { + // The regression this guards (#6575): the drawer claims Escape in the capture + // phase, which runs before the composer's own handler. Without the yield, one + // press dismisses the entire drawer instead of cancelling the in-progress + // edit, and the unsaved draft goes with it. + const { closes } = await renderDrawer({ escapeYieldsToContent: true }); + + pressEscapeOn(composer()); + + assert.deepEqual(closes, []); +}); + +test("Escape inside the drawer closes it when content does not own the key", async () => { + // The default: with no active edit the same press is a dismissal, so the yield + // above must be conditional rather than a blanket exemption for the subtree. + const { closes } = await renderDrawer({ escapeYieldsToContent: false }); + + pressEscapeOn(composer()); + + assert.deepEqual(closes, ["close"]); +}); + +test("Escape from outside the drawer closes it even while content owns the key", async () => { + // The yield is scoped to the drawer's own subtree, so an active edit inside + // cannot wedge the drawer open against a press from the channel behind it. + const { closes } = await renderDrawer({ escapeYieldsToContent: true }); + + pressEscapeOn(dom.window.document.body); + + assert.deepEqual(closes, ["close"]); +}); From b8b94fba6b5b6840a937e4755e9e86513fc934dd Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 14:26:25 -0700 Subject: [PATCH 6/8] test(desktop): supply the thread edit guard to the activity exclusivity harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** test `#6575` made `requireThreadEditResolution` a required dependency of `useChannelAgentSessions`, which this branch's exclusivity harness predates — so after the rebase all five cases threw `requireThreadEditResolution is not a function` rather than exercising last-opened-wins. Production wiring was never affected; `ChannelScreen` supplies it from `useChannelPaneHandlers`. Rather than stub it silently, the harness takes it as an injectable so the guard's interaction with the replacement is itself covered: a refused open must leave the thread and its draft exactly as they were, with no half-applied replacement that cleared the thread before the guard turned the activity open away. Verified to fail with the guard's early return removed. Signed-off-by: ss-dev-00 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- ...useChannelAgentSessionExclusivity.test.mjs | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs b/desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs index b624a34617..066e7be605 100644 --- a/desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs +++ b/desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs @@ -36,7 +36,10 @@ const OTHER_THREAD_HEAD_ID = "thread-head-2"; * 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 } = {}) { +async function renderAgentSessionHandlers({ + openThreadHeadId = null, + requireThreadEditResolution = () => true, +} = {}) { const React = await import("react"); const { act, renderHook } = await import("@testing-library/react"); const { useChannelAgentSessions } = await import( @@ -82,6 +85,7 @@ async function renderAgentSessionHandlers({ openThreadHeadId = null } = {}) { openAgentSessionPubkey: state.openAgentSessionPubkey, openThreadHeadId: state.openThreadHeadId, profilePanelPubkey: state.profilePanelPubkey, + requireThreadEditResolution, setChannelManagementOpen: (open) => write("channelManagementOpen", open), setExpandedThreadReplyIds: () => {}, setOpenAgentSessionChannelId: (value) => @@ -175,3 +179,19 @@ test("back never resurrects a thread from an earlier replacement", async () => { assert.equal(state.openThreadHeadId, OTHER_THREAD_HEAD_ID); }); + +test("an unresolved thread edit blocks the replacement entirely", async () => { + // Last-opened-wins runs *after* the thread edit guard `#6575` added, so a + // refused open must leave both surfaces exactly as they were — the thread + // still on screen with its draft, and no half-applied replacement that + // cleared the thread before the guard turned the activity open away. + const { run, state } = await renderAgentSessionHandlers({ + openThreadHeadId: THREAD_HEAD_ID, + requireThreadEditResolution: () => false, + }); + + await run((handlers) => handlers.openAgentSession(AGENT_PUBKEY, "channel-1")); + + assert.equal(state.openAgentSessionPubkey, null); + assert.equal(state.openThreadHeadId, THREAD_HEAD_ID); +}); From 0d7b21b356c842ad167d54a874bc9f378fe3c706 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 14:54:10 -0700 Subject: [PATCH 7/8] refactor(desktop): extract the channel agent session surface from ChannelPane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** refactor The file-size ratchet grandfathers `ChannelPane.tsx` above the 1000-line cap but forbids growth, and `#6575` landed it at 1011 lines on main — so this branch's +35 became a hard gate failure on rebase rather than the +35 it was when the branch was cut. Extracting rather than trimming, and specifically extracting *this branch's* own additions rather than anything `#6575` just added: the agent-session branch of the auxiliary-surface chain moves into `ChannelAgentSessionSurface`, taking the cross-channel re-scoping rule, the cover-drawer-vs-split-pane choice, and its layout-prop resolution with it. `ChannelPane` keeps what it owns — the surface resolution, the shared `AnimatePresence` key, and the split pane's resize affordances, which it passes down as `wrapSplitPane`. No behaviour change: the same panel with the same props in both presentations. `ChannelPane.tsx` is now 1006 lines, back under the ratchet's recorded 1011. Signed-off-by: ss-dev-00 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- .../ui/ChannelAgentSessionSurface.tsx | 105 ++++++++++++++++++ .../src/features/channels/ui/ChannelPane.tsx | 83 ++++---------- 2 files changed, 127 insertions(+), 61 deletions(-) create mode 100644 desktop/src/features/channels/ui/ChannelAgentSessionSurface.tsx diff --git a/desktop/src/features/channels/ui/ChannelAgentSessionSurface.tsx b/desktop/src/features/channels/ui/ChannelAgentSessionSurface.tsx new file mode 100644 index 0000000000..64589bc316 --- /dev/null +++ b/desktop/src/features/channels/ui/ChannelAgentSessionSurface.tsx @@ -0,0 +1,105 @@ +import type * as React from "react"; + +import * as agentSessionSelection from "@/features/channels/ui/agentSessionSelection"; +import type { ChannelAgentSessionAgent } from "@/features/channels/ui/useChannelAgentSessions"; +import { getAgentSessionPanelPresentation } from "@/features/channels/lib/agentSessionPanelPresentation"; +import { AgentActivityDrawer } from "@/features/channels/ui/AgentActivityDrawer"; +import { AgentSessionThreadPanel } from "@/features/channels/ui/AgentSessionThreadPanel"; +import type { BotActivityAgent } from "@/features/channels/ui/BotActivityBar"; +import type { UserProfileLookup } from "@/features/profile/lib/identity"; +import type { Channel } from "@/shared/api/types"; + +type ChannelAgentSessionSurfaceProps = { + activeChannel: Channel; + activeChannelId: string | null; + activityAgents: BotActivityAgent[]; + agent: ChannelAgentSessionAgent; + /** Render the panel inside the cover drawer instead of the split pane. */ + isCoverDrawer: boolean; + isSinglePanelView: boolean; + onBack?: () => void; + onClose: () => void; + openAgentSessionChannelId: string | null; + profiles?: UserProfileLookup; + useSplitAuxiliaryPane: boolean; + widthPx: number; + /** + * Applies the split-pane presentation, including its resize affordances. + * Supplied by `ChannelPane` because that pane owns the resize state; the + * cover-drawer presentation is applied here. + */ + wrapSplitPane: (panel: React.ReactNode) => React.ReactNode; +}; + +/** + * The channel's agent activity surface: the session panel plus the channel + * re-scoping its content and actions depend on. + * + * Split out of `ChannelPane` so the re-scoping rule below has one home and is + * not another branch inside that component's auxiliary-surface chain. Which + * presentation this lands in is decided upstream and applied through `wrap`. + */ +export function ChannelAgentSessionSurface({ + activeChannel, + activeChannelId, + activityAgents, + agent, + isCoverDrawer, + isSinglePanelView, + onBack, + onClose, + openAgentSessionChannelId, + profiles, + useSplitAuxiliaryPane, + widthPx, + wrapSplitPane, +}: ChannelAgentSessionSurfaceProps) { + // When the panel was opened from a different channel than the currently + // active one, re-scope it to the active channel so that both the + // content/header AND channel-backed actions (e.g. Stop current turn) operate + // on the same channel object. + const effectiveAgentSessionChannelId = + openAgentSessionChannelId && activeChannel.id !== openAgentSessionChannelId + ? activeChannelId + : openAgentSessionChannelId; + const channel = effectiveAgentSessionChannelId + ? effectiveAgentSessionChannelId === activeChannel.id + ? activeChannel + : null + : agentSessionSelection.isAgentInActivityList({ + activityAgents, + selectedAgent: agent, + }) + ? activeChannel + : null; + + const layoutProps = getAgentSessionPanelPresentation({ + isCoverDrawer, + isSinglePanelView, + useSplitAuxiliaryPane, + }); + const panel = ( + + ); + + return isCoverDrawer ? ( + + {panel} + + ) : ( + wrapSplitPane(panel) + ); +} diff --git a/desktop/src/features/channels/ui/ChannelPane.tsx b/desktop/src/features/channels/ui/ChannelPane.tsx index 7827c5fe3e..5b7646d2b1 100644 --- a/desktop/src/features/channels/ui/ChannelPane.tsx +++ b/desktop/src/features/channels/ui/ChannelPane.tsx @@ -28,16 +28,12 @@ import { buildVideoReviewPresentationByMessageId } from "@/features/messages/lib import { isThreadReply } from "@/features/messages/lib/threading"; import { useComposerHeightPadding } from "@/features/messages/ui/useComposerHeightPadding"; import { UserProfilePanel } from "@/features/profile/ui/UserProfilePanel"; -import { AgentSessionThreadPanel } from "@/features/channels/ui/AgentSessionThreadPanel"; import { ChannelManagementAuxiliaryPanel } from "@/features/channels/ui/ChannelManagementAuxiliaryPanel"; import { RightAuxiliaryPane } from "@/features/channels/ui/RightAuxiliaryPane"; import { ThreadViewModeToggle } from "@/features/channels/ui/ThreadViewModeToggle"; -import { AgentActivityDrawer } from "@/features/channels/ui/AgentActivityDrawer"; +import { ChannelAgentSessionSurface } from "@/features/channels/ui/ChannelAgentSessionSurface"; import { FocusThreadDrawer } from "@/features/channels/ui/FocusThreadDrawer"; -import { - AGENT_SESSION_SURFACE_KEY, - getAgentSessionPanelPresentation, -} from "@/features/channels/lib/agentSessionPanelPresentation"; +import { AGENT_SESSION_SURFACE_KEY } from "@/features/channels/lib/agentSessionPanelPresentation"; import { resolveChannelAuxiliarySurface, resolveChannelCoverDrawer, @@ -597,20 +593,10 @@ export const ChannelPane = React.memo(function ChannelPane({ ) : ( wrapAux(panel, "message-thread-panel", { key: THREAD_SURFACE_KEY }) ); - const wrapAgentSessionPanel = (panel: React.ReactNode) => - useAgentActivityDrawer ? ( - - {panel} - - ) : ( - wrapAux(panel, "agent-session-thread-panel", { - key: AGENT_SESSION_SURFACE_KEY, - }) - ); + const wrapAgentSessionSplitPane = (panel: React.ReactNode) => + wrapAux(panel, "agent-session-thread-panel", { + key: AGENT_SESSION_SURFACE_KEY, + }); const threadHeaderLeading = useSplitAuxiliaryPane ? ( ) : undefined; @@ -620,11 +606,6 @@ export const ChannelPane = React.memo(function ChannelPane({ isSinglePanelView, useSplitAuxiliaryPane, }); - const agentSessionLayoutProps = getAgentSessionPanelPresentation({ - isCoverDrawer: useAgentActivityDrawer, - isSinglePanelView, - useSplitAuxiliaryPane, - }); const timelineReplyHandler = activeChannel?.archivedAt || isHuddleTranscript ? undefined : onOpenThread; return ( @@ -978,42 +959,22 @@ export const ChannelPane = React.memo(function ChannelPane({ ) : auxiliarySurface === "agent-session" && activeChannel && selectedAgent ? ( - (() => { - // When the panel was opened from a different channel than the - // currently active one, re-scope it to the active channel so - // that both the content/header AND channel-backed actions (e.g. - // Stop current turn) operate on the same channel object. - const effectiveAgentSessionChannelId = - openAgentSessionChannelId && - activeChannel.id !== openAgentSessionChannelId - ? activeChannelId - : openAgentSessionChannelId; - const panel = ( - - ); - return wrapAgentSessionPanel(panel); - })() + ) : auxiliarySurface === "profile" && profilePanelPubkey ? ( (() => { const panel = ( From d833c54ee6adc8e47fab1f0bc54fa9e0c8c5401c Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 17:19:46 -0700 Subject: [PATCH 8/8] 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, }) => {