fix(web): restore unified activity logs and composer banners - #8734

💭 I want to check the actual diff between the previously reviewed commits and the current head at 35e6b66, so let me pull the commit list to see what's new.

1. git_tools — 8b817cb..35e6b66

2. github_api_read_only — repos/{owner}/{repo}/pulls/{pull_number}

💭 Two new commits since the last review point at cfb3851 — one restoring combined loading/syncing banners, another discarding doc changes. I want to focus on what's new since cfb3851 without re-flagging previously addressed issues, so I'll diff cfb3851..35e6b66 to see exactly what changed.

3. git_tools — cfb3851..35e6b66

💭 The docs revert is out of scope, so I'm focusing on the real changes: a new ComposerActivityStatus.tsx, removal of ThreadSyncStatusPill, and updates to ChatComposer. The previous hand-rolled banner issue looks resolved now that ComposerActivityRow properly composes Row/Icon/Content, so I'm checking the new row component for how it applies the status role to its content span. Placing role="status" on the inner span rather than the outer container seems fine for live-region announcements, and I don't think it's a meaningful regression compared to before. The icon sizing differs slightly from the old pill's size-3.5, and I notice the spinner now animates with motion-safe classes even though the old pill's tests explicitly checked for no "animate-" class — that test file has been deleted, and the PR description confirms the spinner is meant to respect reduced motion preferences, so this seems intentional. Now I want to look at how threadSyncPhase gates the activeTasksProgress logic. I'm checking whether removing activeTasksProgress and activeTaskSteps from the destructuring creates shadowing issues elsewhere, but it looks like they're safely redefined. I'm also examining the width prop behavior on ComposerBanner.Root — when width is "content" there's no @container wrapper, so any @max-[400px] container queries in child rows wouldn't resolve unless there's an ancestor container, but since the sync row uses the default "inline" layout without those queries, this isn't an issue. The tasks badge case uses width="fill" which does get @container, so that path is covered too. Now I'm tracing how ComposerBanner.Root with width="content" behaves when nested inside ComposerBanner.Column and Attachment — since Attachment is a block element and Root becomes w-fit, it naturally left-aligns and shrinks to its content, giving the desired compact pill appearance for the activity strip. I'm also considering whether the attachment seam-joining logic, which uses negative margins and border adjustments to visually merge adjacent attachments, still works correctly when the activity strip attachment is narrower than the full width. So the pill's open bottom seam rests against the composer's top border, with left/right edges ending abruptly — that matches the old shoulder-tab look, likely intentional given the PR's narrow-layout screenshot. Same pattern applies to the stash tab. Still, I'm noticing there might be an inconsistency worth checking. Tracing the width and content logic: content width kicks in only when threadSyncPhase is active and neither the top drawer nor mobile-collapsed state applies, and the activity row swaps in place of the inline tasks badge whenever threadSyncPhase is set. Since activeTasksProgress gets nulled out whenever threadSyncPhase isn't null, the badge and activity row can't both appear — that's consistent. Now I want to check how ComposerActivityRow itself is implemented to see if there's a real issue there. I should figure out if that's separate from the current change. Since I shouldn't re-comment on prior findings, I need to check whether they're actually resolved: the InfoIcon static state issue, the ThreadSyncStatusPill hand-rolled component (now fixed since the file was deleted and uses Row/Icon/Content), and another task-related item I need to verify. I need to check the current head files against several remaining concerns: the missing role="list" on ComposerTasksBadge's list, alignment issues in ComposerBannerStack, the deleted CSS variable that ComposerCommandMenuLayer still reads, the deleted MessagesTimeline test, ToggleIcon's pointer-coarse inheritance, and ComposerPendingUserInputPanel's padding.

4. git_tools — chat-composer-drawer-inset

5. git_tools — role="list"|render={

}

💭 Findings C and D2 remain unaddressed, but the fallback behaves the same as before since parsing an empty string yields NaN, which triggers the same 1.375 default—so it's a latent issue but not behaviorally different, not worth re-flagging. There's a tension between the instruction to output exactly "All clear" and the schema requiring JSON—I'll treat "All clear" as the summary when there are no findings and state is success, otherwise failure. Since prior findings remain unaddressed, this check should be failure, though I should only post new comments for issues introduced by the latest commits or not yet flagged. I want to look more carefully at the newest changes for a genuinely new finding, focusing on the combined loading/syncing banner and the ComposerBanner Root width around that region of ChatComposer, checking whether the status span's data attributes introduce any fresh issue. I'm walking through several potential concerns: the removed pointer-events-none no longer matters since the new row is a plain div; the activity item's priority placement and spinner content seem fine; the spin animation now respecting reduced-motion settings is consistent with the rules; and I'm now considering how the w-fit Root combined with the row's w-full grid columns affects how the flexible middle column shrinks to content width. But I'm noticing a real problem with the width="content" mode's border styling — since the narrow strip is width-fit, a following full-width attachment drawer would lose its top border and radius across its entire width, except where the narrow strip happens to sit above it, creating a visible broken edge on the top of the drawer. I should check whether isTasksDrawerOpen could remain stale true from a prior state, so I need to trace that useEffect that resets it when task progress data becomes null. I also want to confirm the tasks tab badge stays hidden when threadSyncPhase is active, since activeTasksProgress would be null in that case, which checks out. I'm also verifying the interaction between hasBannerItems and threadSyncPhase: when banner items exist, the sync appears as a full-width stack item and the strip doesn't render, while the tasks tab correctly stays hidden since progress is null either way. The race between threadSyncPhase becoming truthy and the drawer-closing effect firing also seems to resolve correctly since the render conditions naturally exclude the drawer before the effect even runs. Now I'm scanning the broader diff for real issues beyond what I've already flagged, particularly around the stash menu close logic in ChatComposer.tsx -- it checks whether there's any candidate in the stash queue other than the current entry, closing the menu if not. This behavior looks correct for closing the stash when the last entry is removed. I'm also checking ComposerStashBadge's button rendering to make sure attributes like data-prompt-stash-badge are still preserved for tests or other consumers.

6. browse_code — apps/web/src/components/chat/ComposerStashBadge.tsx

7. git_tools — data-prompt-stash-badge|data-composer-tasks-badge|data-chat-composer-activity-strip|data-composer-sync-status|data-thread-sync-drawer

💭 My grep on ChatComposer.tsx seems unreliable since it stopped after the first match, treating it like a binary file. I should view the relevant portions of that file directly instead, and also check if there's a previous test file to reference. Four test files got deleted — ComposerStashBadge, ComposerTasksBadge, ComposerBannerStack, and ThreadSyncStatusPill — while the PR adds new interactive behavior like priority sorting, expand state, Escape handling, and a peek button to ComposerBannerStack, with no replacement tests added. That's worth flagging given the rule requiring focused tests for primitive composition changes affecting behavior, prop forwarding, or keyboard interactions. I want to check whether this same issue was caught in a prior run.

8. browse_code — apps/web/src/components/chat/*.test.tsx

9. browse_code — apps/web/src/components/chat/ChatComposer.tsx

💭 No test files exist for the new ComposerBanner/ComposerBannerStack components, and the four deleted test files have no replacements. This matters because the new stack component introduces real behavioral complexity—expand/collapse state, Escape key handling, aria-expanded/aria-controls on the peek button, and priority ordering—all of which should have focused tests. Since prior review runs didn't flag this and it's existed since the first commit, I want to see if the latest commit introduces something more concrete. Looking at ComposerActivityRow, it's reused both as full-width stack content and inside a fill-width grid cell in the strip, but it uses shrink-0 whitespace-nowrap instead of truncate like the old pill did—so on narrow widths the label could overflow the grid column instead of truncating cleanly. At mobile widths the labels still fit within the available space though, so this seems marginal rather than a hard regression—I'd lean toward not flagging it unless I find another instance where it truly breaks. I'm also checking whether the accessibility role="status" and aria-label placement carried over correctly from the old pill implementation. That layout matches the screenshot with the sync pill left and stash tab right. I should double-check that ChatView no longer imports the removed ThreadSyncStatusPill and that nothing else still references it. Now I want to grep for any leftover references to the deleted CSS classes—chat-composer-drawer-surface, chat-composer-top-drawer, chat-composer-shoulder-tab, chat-composer-glass-shell—since dangling class names would be a real ownership issue.

10. git_tools — chat-composer-drawer-surface|chat-composer-top-drawer|chat-composer-shoulder-tab|chat-composer-glass|chat-composer-drawer-slot|chat-composer-stash-tab|chat-composer-tasks-tab|chat-composer-banner-stack-cap|chat-composer-context-strip

11. git_tools — chat-composer-attachment-overlap|chat-composer-glass-surface|chat-composer-outline

💭 I'm tracing how --chat-composer-attachment-overlap flows from ComposerBanner.Surface into ComposerCommandMenu's padding, confirming the custom property is set and consumed on elements where it correctly applies. Checking further, the command menu now uses Surface instead of the old drawer classes, and the highlight variable and context-extension media query overrides seem to have migrated correctly into the Tailwind arbitrary values. I'm also verifying the backdrop-filter fallback and the shell's stacking context with isolate and before-pseudo-element z-index behave as expected. Looking for genuinely new findings, I check whether there are duplicate live regions between the stack and strip renderings of ComposerActivityRow, but confirm only one renders at a time. I also verify the role="status" span nesting inside ComposerBanner.Content doesn't create nested alert roles, since content-type banners skip role="alert" on the root, and I'm examining the grid-cols layout logic for ComposerBanner.Row depending on whether actions are present. Given the newest commit looks clean, the real question is whether earlier flagged issues remain unresolved at head — checking each one: the InfoIcon usage across all server-update states, the missing role="list" on the tasks badge ul, the description alignment in ComposerBannerStack, the drawer-inset variable fallback, and whether the deleted MessagesTimeline "Thinking row" test is still absent. Most of these appear to still be present unaddressed in the current diff. Continuing that check: the ToggleIcon still uses ghost/icon-xs button variants with pointer-coarse handling after the overlay, and the ComposerPendingUserInputPanel padding still only applies to Body. Since these findings remain open, I'll mark the state as failure without duplicating prior comments, though I'm considering flagging one new issue — the deletion of four component test files covering behavior this PR now handles differently, with no replacement tests added, which fits squarely within the check's mandate around testing behavior changes. The strongest candidate is ComposerBannerStack.tsx, which now owns hover/focus/Escape/peek state previously governed by CSS and verified by a deleted test file. I'm leaning toward posting an inline comment on that new state/keyboard block, noting the lost coverage and requesting a focused test, though I want to see if there's an even more concrete behavioral issue in how that stack handles interactions before finalizing.

12. browse_code — apps/web/src/components/chat/ComposerBannerStack.tsx

💭 I'm looking at whether items.length checks collapse the stack correctly, and now I'm checking whether applying role="alert" to every ComposerBanner.Root, including hidden collapsed ones, might cause accessibility noise since it fires for notices that aren't actually visible. I'm noticing that the hidden stacked notices use invisible pointer-events-none without aria-hidden, matching prior behavior, so that's consistent. More importantly, since invisible removes elements from the tab order, keyboard users can no longer Tab directly into hidden notices — they now have to activate the Peek button first, which seems like an intentional accessibility improvement given the aria-expanded/aria-controls wiring and the focus-capture logic that expands the stack when focus lands on it. Tabbing to the Peek button auto-expands the notices via onFocusCapture, and then a subsequent Tab moves focus into the revealed items — that flow seems coherent. The Escape handler calls stopPropagation on the notices container, so pressing Escape while focus is inside blocks broader composer/global Escape handling, though if the stack was only expanded via hover rather than focus, Escape won't collapse it, which is a minor edge case. I'm now checking how Tailwind v4 handles translate-y-16/translate-y-28 alongside the transition-[translate,opacity] utility, since v4 changed how the translate property is generated compared to earlier versions. Verifying that duration-220 works as a bare-value Tailwind v4 duration utility and lines up with the DISMISS_TRANSITION_MS = 220 constant, and confirming the new translate-based exit animation is functionally equivalent to the old translate3d/transform-gpu approach. Now I'm looking at the ComposerBanner.Peek button's sizing (absolute inset-x-0 bottom-0 z-0 mx-auto h-3 w-[96%]), since a 12px-tall hit target seems quite small. Since pointerType !== "touch" skips the hover-expand behavior, tapping this thin Peek button becomes the only way for touch users to trigger expansion — which is a real accessibility concern given the design system elsewhere enforces a minimum 44px touch target. This does seem like a genuine, novel finding since the peek strip is the only touch affordance for revealing hidden notices and lacks the same coarse-pointer expansion the design system applies elsewhere. For the fix I'm considering an expanded pseudo-element hit area on coarse pointers, but I need to be careful it doesn't extend upward and overlap the front banner's dismiss button. Now I'm tracing the layout logic to figure out which direction is actually safe to expand into — checking how the flex-col-reverse ordering places notices above the front banner, and how the peek's absolute positioning at the bottom of the collapsed notices region relates to the front banner beneath it. Extending the hit area downward would overlap the front banner's dismiss/action controls, and extending upward risks overlapping scrollable timeline content near the composer — so neither direction is clean. A safer fix might be to just increase the peek's actual height on coarse pointers, though that would change how the visible cap looks. I'm also reconsidering whether the peek is really the only touch affordance here — checking the pointer/focus handlers confirms it skips hover-based interaction on touch, and it seems so. But I realize before this PR, mobile browsers applying sticky :hover on tap meant tapping anywhere on the front banner (the whole group) would reveal the stack, giving a much larger tap target than what exists now. Now I'm confirming this regression is real: the front banner explicitly collapses on pointer-down, leaving only a thin 12px strip as the sole way to reveal hidden notices on touch devices, which matters since mobile is an explicitly supported surface. I'm double-checking the Peek component's styling to see if there's anything else relevant. I also notice Peek accepts a ref prop but isn't wrapped in forwardRef—this only works correctly if the repo is on React 19, since in React 18 that ref would be silently dropped, breaking focus restoration on Escape. I need to check the package.json to confirm which React version is in use.

13. browse_code — What React version does apps/web depend on (check package.json / root package.json for "react")? Also, are there other components in apps/web/src/components/ui that accept ref as a plain prop without forwardRef?

💭 React 19 handles ref as a prop fine, so that's not the issue. I'm now confirming whether the peek touch target behavior is a regression by checking the pre-PR code: the ComposerBannerStack used group-hover and group-focus-within on an outer wrapper containing both the front banner and stacked items, so tapping the front banner on touch devices would trigger sticky hover and reveal the stack. Now the new implementation adds an explicit onPointerDownCapture that collapses the stack on front-banner tap, leaving just a 12px peek area. While the old sticky-hover reliance was accidental, the new explicit Peek button with aria-expanded is a genuine accessibility win—the real concern is just that 12px height falls well below any reasonable coarse-pointer hit target guidance, so this seems worth flagging as a concrete finding. I'm weighing possible fixes, like enlarging the hit area, but the space above the peek is occupied by timeline content, so there's no clean place to extend it. I think I'll flag this as an issue and suggest a modest fix like pointer-coarse:h-6, while cautioning against overlapping the front banner's controls, but hold off on a full diff since the ideal solution isn't obvious. The peek strip sits within the composer overlay, but since it's just a button rather than something with restricted touch-action, normal scroll gestures should still pass through fine. I think this coarse-pointer issue with the Peek component in ComposerBanner.tsx is worth flagging.

14. browse_code — apps/web/src/components/chat/ComposerBanner.tsx

💭 Since it's a new file, all lines 1-348 are commentable, so I can target line 91 where the h-3 element sits. I'm drafting the comment noting that the "Peek" affordance is now the only way to reveal hidden notices, but its hit area is too small at just 12px. I want to suggest growing the coarse-pointer target with something like pointer-coarse:h-6, but that only gets to 24px, still short of the 44px standard used elsewhere. I'm weighing whether to instead suggest a pseudo-element that extends the hit area upward without overlapping the front banner's dismiss controls. Writing the pseudo-element hit area code... That gives a total tappable height of 32px, which stays clear of the banner below since the bottom edge lines up correctly — a safe, minimal fix. Now I need to draft the suggestion diff and also double-check which of the previously reported findings are still open before summarizing the final count.

15. browse_code — apps/web/src/components/chat/ComposerBanner.tsx

16. browse_code — apps/web/src/components/chat/ComposerPendingUserInputPanel.tsx

💭 Both prior findings are still open, confirming the failure state. Now I'm drafting the new inline comment for ComposerBanner.tsx line 91, noting that "Peek" is the only pointer affordance for opening hidden notices but has just a 12px hit target — I'll reference the earlier finding about 44px touch targets lightly while composing this. I'm suggesting extending the tappable area upward via a pointer-coarse pseudo-element so it never overlaps the front banner's dismiss/action controls below. Once that's drafted, I'll submit the review with a summarizing body and the final JSON marking the failure.

17. modify_pr — UI consistency: one new finding on the latest push (combined loading/syncing banner). The earlier ThreadSyncStatusPill hand-rolled-row finding is resolved — ComposerActivityRow now composes Row/Icon/Content and shares the --composer-banner-icon-column grid. Findings from previous runs on this PR (server-update InfoIcon, banner description/action alignment, missing role="list" on the migrated task and stash lists, ToggleIcon inheriting buttonVariants' coarse-pointer overlay, ComposerBanner.Body dropping the clipped panel's end/bottom padding, the deleted --chat-composer-drawer-inset declaration still read by ComposerCommandMenuLayer, and the removed Thinking-row test) are still open and are not repeated here.