feat(desktop): cover the channel with the agent activity drawer - #6542
Open
baxen wants to merge 2 commits into
Open
feat(desktop): cover the channel with the agent activity drawer#6542baxen wants to merge 2 commits into
baxen wants to merge 2 commits into
Conversation
**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. <details> <summary>File changes</summary> **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. </details> Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a877508075
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useAgentActivityDrawer ? ( | ||
| <AgentActivityDrawer | ||
| channelName={activeChannel?.name ?? "channel"} | ||
| key={AGENT_SESSION_SURFACE_KEY} |
There was a problem hiding this comment.
When activity is opened from a focus-mode thread, or its Back action restores that thread, this distinct key makes the agent drawer enter while the thread drawer with THREAD_SURFACE_KEY remains mounted for its exit animation. Because the surrounding AnimatePresence uses its default synchronous mode, two full-size z-41 overlays exist and animate simultaneously, despite the single-surface resolver; the exiting thread drawer can also continue claiming Escape during that interval. Use a shared cover-slot key or a coordinated/waiting presence transition so the replacement drawer does not mount until the previous one has left.
Useful? React with 👍 / 👎.
**Category:** fix **User Impact:** Opening a thread over agent activity (or activity over a thread) now leaves keyboard focus inside the drawer that just opened. Before, the outgoing drawer's deferred focus restore fired after the new drawer had already taken focus and pushed focus into the covered channel, which is `inert` — so focus landed on `<body>` with no visible focus ring and no keyboard path back into the visible drawer. **Problem:** Two review findings, one root cause. `CoverDrawer` decided at teardown whether to restore focus by calling a caller-supplied predicate that read thread view-mode state, so a presentation-only primitive was interpreting a specific caller's presentation, and the predicate could not see the replacement race at all. Separately, last-opened-wins was only demonstrated through the resolver's fixed thread-beats-agent priority; the e2e "exclusivity" test closed one drawer before opening the other and its final step opened nothing, so no test exercised a real open-over-open transition or asserted the replaced surface's URL param was cleared. **Solution:** Replace the predicate with a module-level focus slot: a drawer claims the slot when it captures focus and restores only if its claim is still current, so any successor's claim — from any source, with no mount ordering assumptions — invalidates the loser's pending restore. The thread view-mode switch releases the slot from the thread side, keeping that decision out of the primitive while leaving plain thread close identical. Document the resolver's priority as a stale-simultaneous-param safety net and test the open handlers directly for both orderings, which is where the real rule lives. <details> <summary>File changes</summary> **desktop/src/features/channels/lib/coverDrawerFocusSlot.ts** New single-slot coordinator: `claimCoverDrawerFocus`, `hasCoverDrawerFocusClaim`, `releaseCoverDrawerFocus`. A monotonic generation answers "was I superseded?" without any drawer naming its successor. **desktop/src/features/channels/ui/CoverDrawer.tsx** Drops the `shouldRestoreFocusOnClose` prop. Claims the slot on focus capture and gates the deferred restore on still holding it. **desktop/src/features/channels/ui/FocusThreadDrawer.tsx** No longer reads thread view-mode state to decide focus restore. **desktop/src/features/channels/ui/useThreadViewModeSwitch.ts** Releases the focus slot when the switch places focus itself. **desktop/src/features/channels/lib/channelAuxiliarySurface.ts** Documents the priority order as a safety net for stale/simultaneous params, not the product rule. **desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs** Drives `CoverDrawer` in jsdom: plain close restores, a replaced drawer leaves focus with its successor, a chain of replacements keeps only the last, and a released slot leaves focus where the caller put it. **desktop/src/features/channels/lib/coverDrawerFocusSlot.test.mjs** Claim/supersede/release semantics, including that claims are never reused. **desktop/src/features/channels/ui/useChannelAgentSessionExclusivity.test.mjs** Tests the open handlers directly in both orderings, plus the breadcrumb that keeps the replaced thread recoverable. **desktop/tests/e2e/agent-activity-cover.spec.ts** Rewrites the exclusivity test as genuine open-over-open transitions with no closed intermediate: thread over activity via a `messageId` deep link, activity over thread via the thread composer's activity bar (the one ingress reachable while the channel is inert). Asserts the replaced param is cleared and exactly one overlay exists. </details> Note on the e2e focus assertions: they are positive checks, not the regression guard. Because the covered channel is `inert`, a wrongly-restored focus is silently refused and lands on `<body>` rather than visibly stealing focus — mutating the guard to a no-op still passes e2e. The discriminating assertions therefore live in `CoverDrawerFocusHandoff.test.mjs`, where disabling the guard fails 3 of 4 tests while the plain-close test keeps passing. Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 already 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
CoverDrawerand 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. Thread drawer behavior is unchanged.Before / after
Same viewport (1280x800), same ingress (composer activity bar -> View activity), captured from the e2e mock bridge:
Screenshots are on the relay rather than inline here, since relay media needs auth and would render broken in GitHub. I posted the before/after pair in the originating Buzz thread.
Notes for review
Exclusivity is structural, not defensive.
resolveChannelAuxiliarySurfaceresolves the one surface a channel shows, andresolveChannelCoverDrawerresolves which one covers. Because that returns a single value, two drawers cannot stack by construction — there is one covered slot and the resolved surface owns it.ChannelPanenow branches off that discriminator instead of the previous implicitthreadHeadMessage ? … : shouldShowThreadSkeleton ? … : selectedAgent ? …fall-through.Escape ownership is opt-out.
CoverDrawerclaims Escape in the capture phase by default, which the thread drawer needs because the composer's mention autocomplete would otherwise swallow the press. The agent drawer setsownsEscape={false}and delegates to the panel's existinguseEscapeKey, so the settings dropdown dismisses on the first press and the drawer closes on the second. The e2e test asserts exactly that two-press sequence.One real bug found while wiring this up:
AgentSessionThreadPanelnever accepted or forwardedenterMotion, even thoughAuxiliaryPanelhas supported it all along. MyenterMotion: falsewas being silently dropped by the JSX spread (spreads skip excess-property checks, so typecheck was happy), and the panel was double-sliding inside the already-animating drawer. Fixed additively, and the e2e test asserts the absence ofbuzz-side-panel-enter— I verified that assertion fails if the prop stops being forwarded.One test fixture updated, and it was a genuine regression on my side:
activity-scope-label-screenshots.spec.tsasserts the header agent name truncates. Its long-name fixture was calibrated to the narrow split pane (745px name in a 745px box), so at the cover drawer's 899px it no longer overflowed and the assertion went745 > 745. Truncation CSS is untouched; I lengthened the fixture name so it still clamps at the widest presentation. Confirmed the test passes on cleanorigin/mainand failed on my branch before the fixture change.Follow-up seam (#6538)
Slice B adds an explicit
transcriptVariantprop with aconversationvariant that beats the width heuristic, and the intended contract is for this drawer to pintranscriptVariant="conversation". That variant does not exist onmainyet (#6538 is still open), so it is not wired here — there is aTODO(#6538)ongetAgentSessionPanelPresentation, which is the single place that needs to return the prop once it lands. Panel prop surfaces are additive only, so that follow-up is a one-liner.Validation
Run at
a877508:pnpm typecheck— cleanpnpm lint— clean (remaining biome warnings are pre-existing files I did not touch)pnpm test— 5368/5368 pass, 81 suites, 0 failagent-activity-cover(3, new),thread-focus-mode(2),activity-scope-label-screenshots(2) — 7 passed. Also ran the fullchannels.spec.tsalongside these plusobserver-feed-screenshots— 105 passed.I did not run the entire smoke project in one go — it is 1160 tests on a single worker and exceeds my shell timeout, so I ran the specs covering the features I touched.
New unit coverage:
channelAuxiliarySurface.test.mjs(surface + cover resolution, including "every candidate open at once still resolves to exactly one surface") andagentSessionPanelPresentation.test.mjs.