feat(desktop): cover the channel with the agent activity drawer - #6542
Closed
baxen wants to merge 9 commits into
Closed
feat(desktop): cover the channel with the agent activity drawer#6542baxen wants to merge 9 commits into
baxen wants to merge 9 commits into
Conversation
![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".
baxen
force-pushed
the
ss-dev-00/agent-activity-cover
branch
from
August 24, 2026 19:16
5fbe7c6 to
7f526d9
Compare
Contributor
Author
|
Updated PR #6542 to Validation at the same checked-out HEAD:
The cover drawer now pins |
baxen
force-pushed
the
ss-dev-00/agent-activity-cover
branch
from
August 24, 2026 19:29
7f526d9 to
a114340
Compare
**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>
**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>
…enshot spec The throwaway spec used to compare presentation options has now paid for itself twice, so give it a permanent home next to the other activity screenshot specs and register it in the smoke project. One seeded turn with the shape a real investigation has: a `@mention` that starts it, thinking, three file reads, a shell command, a relay post, a step that failed, a plan, and an answer containing code. The prompt is framed the way the harness frames it — a `[Buzz event: ...]` section with `From:`/`Content:` lines — because `parsePromptText` reads the author pubkey and the user-visible text out of exactly that shape; a plain text prompt would render as an unattributed bubble and would not exercise the header the drawer exists to make readable. Three frames, all at the drawer's design width: the full window as it opens (the drawer against the sliver and the scrimmed channel, which a panel-only shot cannot show), then the expanded turn at its head and at its tail. Expanding matters — the transcript opens folded, so the default frame is a stack of one-line summaries that shows none of the turn's shape. Expanded it is taller than the drawer, hence two frames. The PNGs are the deliverable, not a pixel baseline, so the assertions are scoped to what this slice owns: the drawer covers, the panel is mounted inside it, there is no split resize handle, and the turn actually rendered. Transcript structure, grouping, and styling belong to the transcript variants being restyled in parallel and are asserted by their own specs; asserting them here would make the reference shots fail for reasons unrelated to the drawer. For the same reason the helpers reach for DOM contracts rather than transcript markup — the scroller is found by computed overflow, and folds are opened through the native `<details>` `open` property rather than by clicking a variant-owned toggle. Two things the helpers guard rather than assume. The expander asserts it found at least one collapsed row, so a moved fold cannot silently turn the expanded shots into duplicates of the collapsed one. And the scroll-to-head polls a value read two frames after the write: the panel is tail-anchored and expanding pins it back to the bottom, so an immediate read would report the write instead of the outcome and the shot would quietly become a second tail frame. Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
…ode replay
The drawer captured `document.activeElement` in a layout effect with an
empty dependency list. The app root is wrapped in `React.StrictMode`, which
in development replays every effect as setup -> cleanup -> setup, so by the
second setup the drawer had already focused itself and recorded *itself* as
the element it had covered. On a real close the deferred restore then
focused a node React had detached, which is a silent no-op that drops focus
to `<body>` -- keyboard-stranded, no visible focus ring, in the exact dev
build people use the feature in.
A one-shot ref guards the capture. Refs survive the replay, so the second
setup keeps the opener the first one recorded. The flag is deliberately not
reset in cleanup: the only cleanup it sees before a real close is the
simulated one, which is precisely what it exists to ignore.
The focus-slot claim stays unconditional. Re-claiming on the replayed setup
is what makes the first cleanup's deferred restore stand down, so the
coordinator was already behaving correctly here -- only the capture was
wrong.
The existing focus-handoff tests could not see this because they render
without StrictMode, so the new case renders under it via
`render(..., { reactStrictMode: true })`. Verified to fail at the previous
head (focus lands on `HTMLBodyElement` instead of the opener) and to pass
with the guard.
Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
Co-authored-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
…er drawer **Category:** test `#6575` gave `FocusThreadDrawer` a `hasActiveEdit` prop so a capture-phase Escape claim yields to an edit composer inside the drawer instead of dismissing the surface and losing the draft. Rebasing this branch moved Escape ownership out of that file and into `CoverDrawer`, so the behaviour now lives behind `escapeYieldsToContent` — and `#6575`'s only regression test for it is an e2e case written against the thread drawer, which cannot see the agent drawer or the prop boundary itself. This adds jsdom coverage at the new seam: a press inside the drawer yields while content owns the key, closes when it does not, and a press from outside closes it either way, so the yield cannot wedge the drawer open. Verified to fail on the first case with the guard removed, while both negative controls keep passing. Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
…ty harness **Category:** test `#6575` made `requireThreadEditResolution` a required dependency of `useChannelAgentSessions`, which this branch's exclusivity harness predates — so after the rebase all five cases threw `requireThreadEditResolution is not a function` rather than exercising last-opened-wins. Production wiring was never affected; `ChannelScreen` supplies it from `useChannelPaneHandlers`. Rather than stub it silently, the harness takes it as an injectable so the guard's interaction with the replacement is itself covered: a refused open must leave the thread and its draft exactly as they were, with no half-applied replacement that cleared the thread before the guard turned the activity open away. Verified to fail with the guard's early return removed. Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
…nnelPane **Category:** refactor The file-size ratchet grandfathers `ChannelPane.tsx` above the 1000-line cap but forbids growth, and `#6575` landed it at 1011 lines on main — so this branch's +35 became a hard gate failure on rebase rather than the +35 it was when the branch was cut. Extracting rather than trimming, and specifically extracting *this branch's* own additions rather than anything `#6575` just added: the agent-session branch of the auxiliary-surface chain moves into `ChannelAgentSessionSurface`, taking the cross-channel re-scoping rule, the cover-drawer-vs-split-pane choice, and its layout-prop resolution with it. `ChannelPane` keeps what it owns — the surface resolution, the shared `AnimatePresence` key, and the split pane's resize affordances, which it passes down as `wrapSplitPane`. No behaviour change: the same panel with the same props in both presentations. `ChannelPane.tsx` is now 1006 lines, back under the ratchet's recorded 1011. Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
baxen
force-pushed
the
ss-dev-00/agent-activity-cover
branch
from
August 24, 2026 22:01
2f6c9dc to
0d7b21b
Compare
**Category:** fix
Replacing one cover drawer with another swallowed the next Escape, so leaving
the drawer that just arrived took two presses. `AnimatePresence` holds the
replaced surface mounted through its exit animation (~210ms measured), and for
that whole window two surfaces are listening for the key — with the outgoing
one, registered first, winning it.
Both layers had to stand down, which is why fixing one was not enough:
- `CoverDrawer`'s capture-phase claim consumes the press with
`stopImmediatePropagation`, invisibly to the successor.
- `useEscapeKey` ignores an event that is already `defaultPrevented`, so an
exiting *panel*'s `preventDefault` swallows the press from its successor's
panel just as completely. This is the path agent activity actually takes: it
sets `ownsEscape={false}` and routes the key through its panel, so no
`CoverDrawer` Escape code runs at all.
Both now gate on `useIsPresent`. Presence rather than the focus slot is
deliberate: the slot is claimed only by a surface that captures focus, and a
successor whose content takes focus instead never claims it — leaving the
outgoing claim current, so a slot check passes for exactly the surface that
must stand down. That version was tried and falsified in the browser. Outside
`AnimatePresence` there is no presence context and `useIsPresent` is `true`, so
the many panels that never animate out are unaffected; `useEscapeKey` also
keeps its `escapeSurfaces` registration for the exit's duration, so background
shortcuts still yield.
Tests, each falsified by removing the guard it covers rather than assumed:
two jsdom cases drive the real primitives under a real `AnimatePresence` with a
keyed child swap, one per layer, asserting the overlap is live before pressing;
a third pins the no-presence-context case. An e2e case presses once inside the
window, reaching activity by navigation so no dismissable layer of the test's
own can absorb it. Removing either guard fails exactly the new coverage.
The visual half of the overlap — both panels cross-fading in the same window —
is unchanged here and stays open in #6734 as a design question.
Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
Co-authored-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
…ity-cover * origin/main: feat(desktop): simplify the message action rail (#6529) fix(desktop): restore icon-only remote marker (#6491) fix(ci): prevent poisoned Rust caches (#6618) docs(security): route reports through private advisories (#6728) fix(composer): wrap Buzz chip labels without orphaning icons (#6581) Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
Contributor
Author
|
Superseded by #6911, which consolidates the full agent-activity focus-view stack (plus tho's polish) into one PR against current main, per baxen's call. No further changes will land here. |
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.