From a87750807532c11055c2dbc368a62ee2f2b45460 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Fri, 21 Aug 2026 19:25:24 -0700 Subject: [PATCH 1/2] 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 | 113 ++++--- .../src/features/channels/ui/CoverDrawer.tsx | 276 ++++++++++++++++++ .../channels/ui/FocusThreadDrawer.tsx | 240 ++------------- ...rPresence.ts => useCoverDrawerPresence.ts} | 6 +- .../activity-scope-label-screenshots.spec.ts | 5 +- .../tests/e2e/agent-activity-cover.spec.ts | 234 +++++++++++++++ 20 files changed, 1111 insertions(+), 329 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 ff8a0e7703b..a79cd74dfc1 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -92,6 +92,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 e111f93ca0e..0cf787647d9 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 00000000000..75a3bf70dbe --- /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 00000000000..6e8f2b705ef --- /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 6f30d7ec6de..00000000000 --- 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 3628d707676..00000000000 --- 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 00000000000..9294d134a41 --- /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 00000000000..e7380da5112 --- /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 00000000000..bbad854850f --- /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 00000000000..7b419e5aa3a --- /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 00000000000..00aa710f0f8 --- /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 f14f3399bd7..edfbb3a0cde 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 00000000000..e0121baa446 --- /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 c1933f14bb7..ce4ca8d2049 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({ @@ -478,13 +472,37 @@ export const ChannelPane = React.memo(function ChannelPane({ }), [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 { changeThreadViewMode, layoutScrollTargetId, resolveScrollTarget } = + useThreadViewModeSwitch({ + activeThreadHeadId: threadHeadMessage?.id ?? null, + externalScrollTargetId: threadScrollTargetId, + onExternalTargetResolved: onThreadScrollTargetResolved, + onModeChange: markExitComplete, + }); const hasSplitAuxiliaryPane = - useSplitAuxiliaryPane && - (channelManagementOpen || - Boolean(threadHeadMessage) || - shouldShowThreadSkeleton || - Boolean(activeChannel && selectedAgent) || - Boolean(profilePanelPubkey)); + useSplitAuxiliaryPane && auxiliarySurface !== null; const wrapAux = ( panel: React.ReactNode, testId: string, @@ -516,6 +534,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; @@ -525,6 +557,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 ( @@ -759,15 +796,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(); @@ -869,7 +906,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 @@ -897,20 +936,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; + /** 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, + 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; + event.preventDefault(); + event.stopImmediatePropagation(); + onClose(); + } + + window.addEventListener("keydown", handleEscape, { capture: true }); + return () => { + window.removeEventListener("keydown", handleEscape, { capture: true }); + }; + }, [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 1aaad0e6093..b3c73f83ab8 100644 --- a/desktop/src/features/channels/ui/FocusThreadDrawer.tsx +++ b/desktop/src/features/channels/ui/FocusThreadDrawer.tsx @@ -1,12 +1,7 @@ -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; @@ -15,231 +10,34 @@ type FocusThreadDrawerProps = { }; /** - * 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, children, 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; - event.preventDefault(); - event.stopImmediatePropagation(); - onClose(); - } - - window.addEventListener("keydown", handleEscape, { capture: true }); - return () => { - window.removeEventListener("keydown", handleEscape, { capture: true }); - }; - }, [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 271c867ae39..3d725cc979b 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 4ed0a211e54..242f58d1529 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 00000000000..ae4c5e189a7 --- /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 5fbe7c676c73b6417a2703d6086855c13ec3ac00 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Fri, 21 Aug 2026 20:41:21 -0700 Subject: [PATCH 2/2] 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 7b419e5aa3a..c461146ca61 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 00000000000..33dee31be37 --- /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 00000000000..21a7c802af9 --- /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 d934964ed02..3810d625238 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, @@ -23,13 +27,6 @@ type CoverDrawerProps = { ownsEscape?: 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. @@ -168,17 +165,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; @@ -201,13 +193,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 00000000000..74b94f8a9d7 --- /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 b3c73f83ab8..5f793c8eff5 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 = { @@ -9,20 +8,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, @@ -34,7 +27,6 @@ export function FocusThreadDrawer({ ariaLabel="Thread" 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 00000000000..b624a346175 --- /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 8dd41cfc51e..5fdb9c62ddf 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 ae4c5e189a7..214ff9799c2 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 ({