Repository navigation
fix(channels): make the covered conversation inert while a narrow pane covers it - #2942
Conversation
|
🤖 Design follow-up at Evidence: pane-close focus, restored-panel focus, and regression journeys. The earlier review found no blockers in the covered-conversation behavior; this follow-up checks the two changed focus paths. Source and test definitions inspected; no app or tests run. The previously reviewed light-theme recording is from an earlier head and does not validate these fixes. Native WKWebView, live keyboard/VoiceOver, both themes, narrow/wide/enlarged-text layouts, long/empty/loading/error content and agent-heavy states remain unverified at this head. |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Source review of 81579e9c5dabc17eebe2a39e052e1c4b1fa61c34 against 11835b133f98449e963e191db3d72e6b5556e195: two P2 focus-restoration defects, detailed inline.
- Collapsing the tab pane while the channel companion remains open attempts to focus the still-inert conversation.
- Closing a retained side panel after leaving and returning to Messages skips deferred focus restoration when its original launcher is gone.
The CSS-derived coverage, observer cleanup, existing inert-aware read/dwell guards and PageTabs close-all handling otherwise showed no actionable defects in this source review. Both findings concern the channel board's existing dock; the stated Bestie/shell companion follow-up remains outside scope.
Validation limits: no tests, builds or app/browser workflows were executed. Sampled frames from both supplied videos support the intended behavior, but their after label is e73495675c, not this reviewed head. Current-head browser behavior, native WKWebView and VoiceOver remain unverified by this review.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, at 0f88f6287efcb99cff6c9b037bbcf2aafe614030 against unchanged base 11835b133f98449e963e191db3d72e6b5556e195.
Both prior P2 focus-restoration findings are addressed in source. Pane collapse now applies the inert-aware dock fallback to the remounted toggle; restored side panels resolve their connected launcher or page fallback after the close commits. The new browser assertions cover collapse beside a surviving companion and Messages → Activity → Messages → Escape with a restored Todos panel. No new actionable code findings in this bounded follow-up.
Prior coverage of inert/read-state, observer cleanup and PageTabs close-all is retained; those sources are unchanged. The stated Bestie/shell companion follow-up remains outside scope.
Optional evidence clarification: update the Testing section to name these two added scenarios and their current-revision fail-before/pass-after evidence, or identify those checks as deferred. The browser layer is appropriate for layout and native inert/focus behavior.
Source-only: no tests, builds or app/browser workflows were run by this review. The unchanged supplied videos were previously inspected through sampled frames, but show an earlier revision (e73495675c). Current-head browser behavior, native WKWebView and VoiceOver remain unverified.
0f88f62 to
ce1ee40
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source integration review via Wes’s account, at ce1ee40a5e0760873a2e91676ba6293aa1db5c7f against 80cb3a25569f763a2b36ce473320fe628a17c85f.
[P2] Keep bulk-close focus in the surviving dock when the conversation stays covered
Open Todos and Channel settings while wide, narrow the window so the dock covers the conversation, focus the Settings tab, then invoke native Close All Tabs. The new base deliberately keeps the launcher’s side panel open (useChannelTabState.ts:838–839), so Todos reappears and this PR keeps the conversation inert. Removing the focused Settings tab requires a new focus destination, but ChannelsPage.tsx:1393–1400 calls .focus() on the main conversation tab or page fallback inside that inert article. The call cannot focus it, leaving keyboard focus lost instead of on Todos.
Bulk close bypasses both repaired deferred-focus paths, and the retained drawer does not acquire focus when it unhides. The coverage observer only transfers focus from inside the conversation, so it cannot repair focus lost from the removed dock tab. Apply the existing inert-aware focusDock rule to this bulk-close fallback, preserving focus when surviving content already owns it. Add a narrow browser regression that asserts Todos receives focus while the conversation remains inert; the retained-Todos bulk-close assertions currently check visibility, while the narrow Activity close-all case ends uncovered.
Both previously reported P2 repairs remain present. The changed read-state consumer still gates acquisition/dwell on inert/hidden content, and the global reconnect notice’s visible-only selectors fit its portal placement. The stated Bestie/shell companion expansion remains outside scope.
Source-only: no tests, builds, apps or browser workflows were run. The unchanged supplied videos were previously inspected through sampled frames but show an earlier revision (e73495675c). Current-head browser behavior, native WKWebView and VoiceOver remain unverified.
|
🤖 Addressed the P2 bulk-close finding in ff08175. The close-all focus fallback in One correction to the scenario: Channel settings replaces a launcher's side panel in the pane, so Todos is not open when a Settings tab is. A thread tab does coexist with Todos, and that reproduces the loss. The new test in |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source-review follow-up via Wes’s account.
Reviewed head ff0817585603dcbfcc7f3532414ff809c2a1543c against base/merge-base 80cb3a25569f763a2b36ce473320fe628a17c85f, including the delta from ce1ee40a5e0760873a2e91676ba6293aa1db5c7f. No new actionable findings.
The published Close All Tabs P2 is repaired in source: the post-commit fallback now sends focus to the surviving dock when the conversation remains inert, while preserving focus already in surviving content. The retained-Todos/thread browser case checks the removed tab, surviving panel, inert conversation and focused Todos landing. Both earlier focus repairs remain intact.
The author’s scenario correction is right: opening Channel settings closes the launcher panel; the thread-plus-Todos case is the reachable reproduction. The updated Testing section also supplies the previously requested regression evidence.
Rocket, Mantis and Groot completed independent source coverage. We ran no tests or app/browser journeys. Chromium/WebKit fail-before/pass-after and full-spec results are author-reported, not independently reproduced by this review. Previously sampled videos show an earlier revision (e73495675c); current-head runtime behavior, native WKWebView and VoiceOver remain unverified. Bestie and the shell companion frame remain outside this review’s scope.
…e covers it In a narrow window, a thread, profile, Channel settings or link panel floats over the conversation, as a 420 px card or at full width. The conversation underneath stayed live. Shift+Tab from the thread's tab walked into the hidden "Message #Alpha" composer, Enter posted to a channel the user couldn't see, and the covered conversation was marked read while focus sat in it. The Activity page's tabs did the same. - useCoveredByDock reports whether the board's open dock floats, from the dock's computed position, measured in a layout effect and on ResizeObserver callbacks. CSS stays the only owner of the media and container query breakpoints. - ChannelsPage and PageTabs mark the covered conversation inert and aria-hidden, including the strip beside the card. The value is derived while rendering, so closing the pane uncovers it in the same commit. - PageTabs' close-all guard checks the board instead of the covered page, so the close-all-tabs command still works while a tab covers it. - useConversationTabs.afterClose waits for the close to commit before focusing an opener inside the covered conversation; focus() on an inert element does nothing. A new browser journey in focus-navigation.spec.mjs covers 1100, 600 and 900 px. It fails on unfixed code in Chromium and WebKit and passes with the fix. Existing tests that read covered content by role now use includeHidden. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to the review of the covered-conversation change. - When a pane starts to cover the conversation while focus is inside it, useCoveredByDock moves focus to the dock's selected tab first. Narrow Settings already does this for its details pane. Before, the inert fixup dropped focus to <body>. ChannelsPage and PageTabs both pass the covered element, so they behave the same way. - useConversationTabs.afterClose now waits for the close to commit before resolving its target, not just before focusing it. A disconnected opener still falls back to the page's own fallback element. If that target is still covered (by the companion, or a dock that stays open), focus goes to the dock's selected tab, or else its first live control, instead of <body>. - Channels.module.css notes, in both the @media and @container overlay blocks, that useCoveredByDock treats position: absolute on the dock as covering. No style values change. New and updated tests in focus-navigation.spec.mjs: - The narrowing journey now expects focus on the thread's tab and checks that a typed key still doesn't reach the draft. - A message that arrives while the conversation is covered isn't read until the pane closes, while the visible thread still reads. - At 600 px, a tab on the Activity page leaves the page inert and aria-hidden. Shift+Tab never enters it, and close-all-tabs still closes every tab. - Closing a tab under the companion keeps focus in the companion. - The covered-panes test now checks the main-to-side step at a width where nothing is covered. At narrow widths it starts from a live control. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to the second review of the covered-conversation change. - With no selected tab, focusDock fell back to the dock's first live button. For the companion that is "Close Companion fixture panel", so a Space typed after narrowing closed it. focusDock now picks the visible panel's own landing first, as the companion's PanelCard <aside> takes focus when it opens, then the selected tab. It never picks a button. The cover-start handoff and afterClose both use it. - A side panel such as Todos or Huddles had no landing, and narrowing put focus on its "About todos" button. Its section is now focusable, like PanelCard's aside. Tests in focus-navigation.spec.mjs: - The covered-panes test works out covering from the bounding boxes of the dock and the conversation, instead of repeating the hook's computed-position check. - The read-state test is retitled to what it checks: narrowing hands reading to the thread's tab, and the message is read once the close returns focus. The narrowing journey covers keeping focus out. - The Activity test opens its row at 1100 px, focuses it and narrows to 600 px, then expects the selected tab to have focus. It fails without the handoff. - The companion test narrows from the composer with only the companion open. It expects focus on the companion itself, not its Close button, and checks that Space leaves it open. It then closes Channel settings under the companion and expects the same. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…vered Follow-up to the third review of the covered-conversation change. - At narrow widths, Escape from a Todos or Huddles panel dropped focus to <body>. hide() focused the launcher straight away, while it was still inside the inert conversation. It now waits for the close to commit, as afterClose does, and then focuses the launcher. If a dock still covers the launcher, focus goes to the dock instead. - focusDock takes the first live match in document order. A stacked dock now focuses the top pane's selected tab before a lower panel's landing. Its comment now says the tab is a Base UI <button role="tab">, which Delete closes. Tests in focus-navigation.spec.mjs: - A new test opens Todos at 1440 px, clicks the composer, narrows to 600 px and presses Escape. It expects focus back on the launcher, and fails without the fix. - The covered-panes test adds a pass with normal-size text. It samples the pane at full width (600 px), as a card (900 px) and beside the conversation (1440 px), and asserts that both the covered and the uncovered branches ran. This pass replaces the separate 1440 px check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to the fourth review of the covered-conversation change. It adds one browser test in focus-navigation.spec.mjs and changes no source. - With Todos and the companion fixture open at 600 px, narrowing from the composer focuses Todos. Escape closes Todos while the companion still covers its launcher, so the companion's <aside> takes focus. This covers the still-covered branch of hide()'s deferral and its use of the cover's parent as the board. - With a thread tab above the companion, narrowing from the composer focuses the thread tab first, in page order. The test fails without the still-covered fallback in useChannelPanels.tsx, and with focusDock's earlier panel-before-tab order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… a restored panel Follow-up to the fifth review of the covered-conversation change. Two focus returns still ran against an inert conversation. - Toggle tab pane with a companion open at a narrow width collapsed the tabs, but the companion kept the dock, so the pane's toggle remounted inside the inert conversation and focus() on it did nothing. The layout effect in useConversationTabs now resolves the split target through the same inert check as an uncovered opener: a dock that still covers it takes focus through focusDock. - A side panel such as Todos restored after a page remount has no trigger element, so hide() focused side.fallbackFocus at once, while the conversation was still inert. hide() now resolves a connected trigger or the fallback through the deferred path, which focuses the dock if one still covers the target. Tests in focus-navigation.spec.mjs: - The companion test opens Channel settings, narrows to 600 px and collapses the pane. It expects the companion itself to hold focus. - A new test opens Todos, visits Activity and returns to Messages, then narrows from the composer and presses Escape. It expects focus on the Channel actions button once the close uncovers it. Both fail without the fix in Chromium and WebKit and pass with it. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…onversation Follow-up to the sixth review of the covered-conversation change. Close all tabs keeps a launcher's side panel such as Todos open, so at a narrow width the dock outlives the tabs and still covers the conversation. The closed tab's focus fallback in ChannelsPage called focus() on the conversation's main tab or page fallback inside that inert article, which does nothing, so keyboard focus was lost. The fallback now resolves through the same inert check as the other deferred focus returns: a dock that still covers the target takes focus through focusDock. A new browser test in focus-navigation.spec.mjs opens Todos and a thread tab at 1440 px, narrows to 600 px, focuses the thread tab and dispatches close-all. It expects Todos to stay, the conversation to stay inert, and Todos to hold focus. It fails without the fix in Chromium and WebKit and passes with it. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ff08175 to
3a7a67e
Compare
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One test regression needs fixing before merge.
…measures Follow-up to the seventh review of the covered-conversation change. It changes one browser test and no source. The agent-outcomes journey opens the Thread dock and then checks the expandable activity history at 800 px with 20 px text. Below the 813.75 px breakpoint the dock floats over the conversation, which the covered-conversation fix makes inert and aria-hidden, so settle(page)'s default scroller, the "Channel message history" region, matched nothing and timed out with "timeline geometry settles". Both browser CI shards failed on this journey. The geometry that moves between expanding the group and clicking the toggle again is the thread's, so the loop now settles the thread's "Thread messages" region, as activity-lifecycle.spec.mjs does. The conversation stays inert and hidden from role queries. Chromium, retries disabled: the journey no longer fails at settle. Under a load average above 16, a separate toBeFocused() check after Enter on the activity toggle flakes at the same rate on base 3a1eb56 (2 of 6) and on this branch (2 of 10), so it is pre-existing and not addressed here. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Follow-up at 3a9623e23ac728a87e2e6f81a04ae299979c3760, base/merge-base 3a1eb5601e997811009d01601786df8af9393a7f: no new actionable findings in the test repair.
agent-outcomes.spec.mjs now passes the visible thread’s exact Thread messages region to the existing geometry-settling helper. This follows the scrolling owner at both widths and avoids querying the covered channel history at 800 px. Failure-history, message-anchor, transcript, bounds, keyboard-focus and reload assertions remain intact. No browser cases or production behavior were added or removed. Prior production review coverage and all three focus repairs remain preserved.
Source-only: we ran no tests, builds or app journeys. Current-head Chromium/WebKit behavior, desktop WKWebView and VoiceOver remain independently unverified; reported test results belong to the author. The unchanged supplied videos retain their earlier-revision sampled-inspection limitation. Bestie/shell companion expansion remains outside this review.
kazemek21
left a comment
There was a problem hiding this comment.
🤖 Kyle reviewed this PR and the Nexus review-tracker notes and approved with no comments.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
## Why Clicking an image in a channel timeline cleared Channel settings and unmounted the whole side pane for the length of the review. When the viewer closed, Channel settings was gone with no announcement, and any side panel that clears state on unmount had lost it: the native Huddle chat panel releases files you had attached but not sent. The side thread only survived because its text draft is saved per keystroke. The `setSettings(undefined)` in `openMediaReview` dates from #163, when settings was a transient panel auto-closed by any other opening. #413 made it a retained tab and added the dock guards but left that clear behind. ## What - `openMediaReview` no longer clears Channel settings. - The side pane stays mounted under the viewer. `PanelDock` already renders a closed dock as `hidden`, `inert` and `aria-hidden` when `keepMounted` holds, so `keepMounted` now also holds during a review (only when there is a panel to keep) and the pane's child guard no longer unmounts on it. Settings, side plugin panels (Huddle) and the Solo companion keep their state. - Side thread tabs still leave during a review, via a new `hostPanes` option on `useConversationTabs`. This is load-bearing, not a leftover: the viewer's comment composer reads the thread draft when it mounts and the thread composer reads the viewer's edits back when it remounts, so keeping the side thread mounted would hand back a stale draft. Threads moved to the main pane keep rendering through a review, as they did before. ## Before / after Left: `main` (3a1eb56). Right: this branch. Same steps: open ⋯ Channel actions → View channel details, click the image, press Escape. https://github.com/user-attachments/assets/564548a1-7479-4855-9eb4-4090bfa1a844  ## Tests - New browser journey in `tests/browser/channel-settings.spec.mjs`: Channel settings stays open with its tab selected and focus returns to the tile after Escape; the dock is `hidden` and `inert` under the modal; a side-thread draft reaches the viewer's comment composer and comes back edited; a thread moved to the main pane keeps the same composer element connected through the review. Fails on `main` at the first pane assertion. - New `useConversationTabs` case: `hostPanes: ["main"]` withholds only the side host and re-hosts it afterwards, including a tab the visit showed but no longer selects. - `channel-settings.spec.mjs` and the existing "media review hands off the thread draft" journey pass in Chromium and WebKit; `tsc --noEmit`, Biome and Vitest for `src/bundled/channels` and `src/features/panels` are clean. Not verified here: the native-only Huddle case (attached-but-unsent files surviving a review) needs a manual check in the native app. ## Notes - Merges with #2942 as a one-line, same-meaning conflict: that PR writes `open={docked}`; keep that plus this branch's `keepMounted`. - The other three `setSettings(undefined)` sites (Reply/open thread, opening a link into a side panel, opening a bottom drawer) are unchanged and belong in a follow-up once Channel settings is treated as a fully retained tab. - A main-pane thread on the same root as the reviewed image still keeps its pre-viewer draft, as on `main`; the composer's conflict banner surfaces it on the next keystroke. Unmounting only the reviewed root's thread is a possible follow-up. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: mtoohey <mtoohey@squareup.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Summary
In a narrow window a thread, profile, Channel settings or link pane slides over the conversation, but the covered conversation stayed live. Shift+Tab from the pane walked into the hidden
Message #Alphacomposer, Enter posted to a channel you couldn't see, screen readers could still browse it, and messages arriving underneath were marked read. The same happened on the Activity page (PageTabs) and when the window narrowed while you were typing.This makes the covered conversation
inertandaria-hiddenwhile the dock floats over it, and keeps keyboard focus somewhere live.useCoveredByDock(board, open)(new) reports whether the open dock covers the conversation by reading its computedposition: absolute.Channels.module.csskeeps owning the breakpoints (@mediaand@containerboth set it; comments there name the contract). It measures in a layout effect and onResizeObserverfor the board and the dock, and resets when the dock closes.ChannelsPageandPageTabsmark the conversationinert+aria-hiddenwhile covered, including the visible strip beside the 420 px card (that strip is already treated as covered by the pane-focus shortcuts).PageTabs' close-all guard checks the board instead of the now-inert page, so the native close-all-tabs command still works while a tab covers it.<aside tabIndex=-1>) or its selected tab, in document order. Never a button. Same precedent as narrow Settings. Todos and Huddles sections gettabIndex={-1}so they have a landing.afterCloseinuseConversationTabsandhide()inuseChannelPanelsused to focus the opener synchronously, before the close committed, while it was still inert. They now resolve and focus the target in the post-commit layout effect, and fall back to the dock if something else (e.g. the companion) still covers the opener. This fixes Close from Channel settings, Escape from a profile opened from Manage members, and Escape from Todos/Huddles at narrow widths.inert:use-readingalready skips anything under[inert], so messages that arrive while covered aren't read until the conversation is uncovered.Not in this PR: Bestie and the shell companion frame (
Panels.module.css). Those float the card at a fixed 1000 px breakpoint with plenty of conversation still usable, so they need a different rule (inert only on full cover). Follow-up.Heads-up for #2609: it changes the same
afterClosetarget line. Whichever lands second swaps one expression in the resolver (successor(trigger) ?? fallbackFocus?.current); the deferral already runs after the commit, sosuccessor()sees a live history.Before / after
Chromium 600×800, same scripted journey on both sides (left:
mainat3bcf1c4bb1, right: this branch). The badge at the bottom showsdocument.activeElement, whether it is hit-testable, and the live text of theMessage #Alphacomposer.Part 1: seven Shift+Tab presses from the thread tab, type "Wrong channel!", Enter, close the thread. On the left every stop is "HIDDEN under the thread pane" and Enter posts to #Alpha. On the right focus stays on visible chrome and the draft is untouched. Part 2 (from 22 s): typing at 1000 px, then narrowing to 600 px. Left keeps typing into the hidden composer; right hands focus to the "Thread root 1" tab and later keystrokes go nowhere.
side-by-side.mp4
Part 2 at full resolution:
narrow-while-typing-side-by-side.mp4
Testing
tests/browser/focus-navigation.spec.mjs(browser-only: jsdom evaluates neither media nor container queries):mainat the narrowing step and at the Shift+Tab walk, passes here, Chromium and WebKit);coveredderived from geometry (dock box intersects conversation box), asserting both branches ran;<aside>, Space leaves it open;ce1ee40a5with focus lost, passes atff0817585, Chromium and WebKit).includeHidden: true(inbox,layout,thread-resize,live-status,channel-details, two infocus-navigation).channel-settings.spec.mjs:117andprofiles.spec.mjs:1297fail without theafterClosedeferral and pass with it.ChannelsPage*,PageTabs*,useConversationTabs*,useChannelPanels*: 170 tests pass.tsc --noEmit, Biome,design:checkclean.typing.spec.mjs:290, React Add complete reaction toggles to the message menu #185; fails 8–10 of 15 runs onmaintoo).🤖 Generated with Claude Code