Repository navigation
fix(channels): keep a launcher's side panel open on Close all tabs - #2803
Conversation
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.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, with Rocket, Mantis and Groot coverage.
Reviewed 2f9b09b1ef5d41e7ac7fee3064eff5d61bc09f84 against 067db608fbc624e56161ccf3e4ff1c0f11c30baf. One P2 incomplete-fix finding is inline: a dismissal return can still reveal a hidden side panel when the destination channel visit saved an open pane. This is a remaining case of the stated fix, also present on the base. No other actionable findings in the reviewed diff and callers.
Source-only: no tests, builds or app workflows executed. Reviewed the added test source and sampled frames from the attached fixture video; the recording is labelled 2c81a4905, and the reported local runs predate the final rebase. Neither establishes runtime validation of this head. Native menu delivery, desktop behavior, both themes, narrow layouts and enlarged text were not exercised.
2f9b09b to
2dacb82
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, with Rocket, Mantis and Groot coverage.
Reviewed head 2dacb82c719906a4905c7a2caae0c1d44173f4f7, target base cae3922574c5c1f0ce5d32231810229e94b1bb40 (actual merge base 067db608fbc624e56161ccf3e4ff1c0f11c30baf). Prior coverage at 2f9b09b1 is retained; this pass covers the two-file repair.
The prior P2 in 4498855536 is addressed in source: the matching dismissal return now restores the side panel’s captured visibility, including false, after enterVisit; kept tabs still show and passive history snapshots remain unchanged. The added browser scenario covers the previously missed saved-open-channel sequence. No further required changes identified; one optional P3 ordering concern is inline.
Optional description cleanup: Fix item 2 and Related #1793 still describe the removed paneOpen guard. Update those passages to describe the captured-visibility marker used by this revision.
Source-only: no tests, builds or app workflows executed. The author reports that the new scenario was not run locally; we have not verified its execution or current CI results. The unchanged video is historical (2c81a4905); prior sampled-frame inspection does not validate this repair. Native menu delivery and rendered focus/theme/responsive behavior remain unexercised.
| else { | ||
| tabState.select(kept.id); | ||
| tabState.setPaneOpen(true); | ||
| tabState.setPaneOpen(kept.paneOpen); |
There was a problem hiding this comment.
🤖 [P3, optional] Preserve a newer launcher opening during the pending return
If Todos is reopened after a hidden-pane Close all but before the native history.go() return lands, the launcher still accepts the click: the old history attempt remains live until popstate (browser-history.ts:364–370). Its open callback sets paneOpen=true without changing the empty selected id (useConversationTabs.tsx:436–440), so retained still matches and this assignment reapplies the captured false. The accepted opening is then hidden again.
This is an optional ordering improvement: the impact is another launcher press, with no lost work, and native traversal duration was not measured. Consider updating the pending visibility intent on a newer launcher/pane action so the latest choice survives enterVisit, with a deliberately delayed-return regression case. The ordinary prior-P2 sequence is fixed.
There was a problem hiding this comment.
🤖 Leaving this optional one open; not addressed in this revision.
The window is between the dismissal and the native history.go() popstate, and the cost is one extra launcher press with nothing lost. Closing it properly means tracking pane actions during the pending return in both directions (a reopen and a hide), plus a delayed-return fixture to prove it. That is more machinery than this fix should carry without a measured traversal window. Happy to take it as a follow-up if it shows up in use.
|
🤖 Design review at Reviewed the diff, surrounding navigation/launcher code, regression-test source and supplied video. The video shows historical head |
2dacb82 to
6d78d49
Compare
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.
Bounded integration follow-up at 6d78d496832aec134f534c62898e947da7a22307 against base/merge-base 5d9d1ef33c9fb3127497c2ecfba9819c0ebf9826. Prior feature coverage is retained; this pass checks integration with the base’s retained side drawer and visit snapshots.
One new P2 test integration finding is inline. The prior hidden-pane dismissal repair remains intact in source; no additional production defect was identified in this bounded pass. The previously published optional pending-return launcher P3 remains unchanged and is not duplicated here.
Source-only: no tests, builds or app execution. The predicted browser failure is based on the current owners and their regression assertions, not an observed run. Historical browser runs and the attached older video do not validate this head; prior sampled-frame inspection is the extent of our video assessment.
Close all tabs (the tab menu, or ⌘⇧W) closed the whole right pane, so a launcher's side panel such as Todos vanished even though it isn't a tab. Its launcher still read pressed, so the first press did nothing visible. - closeTabs keeps the pane open when it was showing a launcher's side panel. A closed pane stays closed. - onCloseTabs, with nothing kept, dismisses toward the side pane while a launcher's side panel is selected, shown or hidden. The return to the channel visit then restores the pane as the close left it through the existing survivor path (#1767), the same way it does when the last tab closes with X. The kept-return marker records that visibility: a kept tab shows, and a side panel keeps its pre-close pane. Dismissing toward the main pane instead let a return to a channel visit that had saved an open pane, because a retained thread was in it, reveal the panel the user had hidden. - A side panel's launcher reads pressed only while the pane shows it. On a closed pane, one press brings back the pane and the panel. ChannelPanelTabs gains paneOpen for this. Close other tabs on a main-pane tab keeps the side panel beside it too. Tests: three useConversationTabs cases (Close all tabs, the launcher after Close tab pane, and Close other tabs keeping a main-pane tab), and a todos.spec.mjs journey that covers the history return in both an open and a closed pane. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
6d78d49 to
c2cf2ee
Compare
…ter Back Follow-up review for the Close all tabs side-panel fix. - useConversationTabs: the Close all tabs case now also runs with the pane closed. The side panel stays hidden and its launcher reads unpressed. This fails if closeTabs drops the paneOpen guard. - todos.spec.mjs: the journey goes Back to the channel visit. Since #2838 a channel visit showing Todos saves an open pane, so Back shows Todos with the retained thread tab. Hiding the pane there saves it closed: the next Back hides Todos with the launcher unpressed, one press shows Todos and the thread tab again, and Close all tabs from that channel visit keeps Todos shown, with the launcher pressed. This fails if closeTabs no longer keeps the pane open. - todos.spec.mjs: a channel visit showing Todos beside a retained thread saves an open pane when a second thread opens. Hiding the pane before Close all tabs still returns there with Todos hidden, and one press shows it. This fails if the return reapplies that saved open pane. - shell-design.md: Close all tabs leaves a side panel as it was, shown in an open pane or hidden in a closed one. Hiding the pane hides the panel without closing it. The launcher reads pressed only while the pane shows the panel, and one press brings back the pane with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up review for the Close all tabs side-panel fix. - plugin-architecture.md: for channelPlacement "side", a launcher's pressed now means the panel is shown in the open pane, not merely selected. While the pane is hidden, the panel stays selected but reads unpressed, and toggle shows it again. - useChannelTabState: the side-panel comment in closeTabs moves next to the paneOpen expression it describes. 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>
c2cf2ee to
82ed19e
Compare
Problem
Close all tabs (right-click a tab, ⌘⇧W, or the macOS Close All Tabs menu item) closes the whole right pane. A launcher's side panel such as Todos goes with it, even though it isn't a tab.
aria-expanded="true"), so the first press "closes" the hidden panel and nothing visible happens. Only the second press brings Todos back.Fix
Close all tabs now leaves a launcher's side panel as it was: shown in an open pane, hidden in a closed one. The launcher reads pressed only while the pane shows the panel, so one press always brings it back. Production change: 23 lines added and 5 removed, in 4 files.
closeTabs(useChannelTabState.ts):paneOpen: side || (s.paneOpen && !!s.sidePanel). A pane showing a launcher's panel stays open, and a closed pane stays closed.onCloseTabs(ChannelsPage.tsx): when nothing is kept and a launcher's side panel is selected, shown or hidden, it dismisses toward the"side"pane, not"main".dismissContentthen records the existing side-panel survivor from feat(navigation): distinguish passive traversal from explicit close #1767, so the return to the channel visit keeps Todos. That's the same path that keeps Todos when its last tab closes with X.paneOpenas it was. The matching return applies it afterenterVisit, so a channel visit that saved an open pane (a retained thread or Todos was in it) doesn't reveal the Todos the user had hidden before ⌘⇧W.pressedandtoggleviashown(useChannelPanels.tsx): a side panel counts asshownonly while the pane is open, andpressedandtoggleuseshown.onOpen(setPaneOpen(true)) and shows the pane with the panel.selectedis unchanged, so the panel stays retained while the pane is hidden.ChannelPanelTabsgainspaneOpen.useConversationTabs.tsxpassestabState.paneOpen, the flag its tab-placedpressedalready reads.Close other tabs on a main-pane tab now keeps the side panel beside it too.
Preserved:
Docs:
shell-design.mddescribes Close all tabs and hiding the pane for side panels.plugin-architecture.mddefinespressedforchannelPlacement: "side"as "shown in the open pane", not merely selected.Tests
Vitest (
useConversationTabs.test.tsx;launcherHarnessgains aplacementparameter):closeTabsdrops thes.paneOpen &&guard.Journey (
tests/browser/todos.spec.mjs): "Todos isn't a tab: Close all tabs keeps it, and its launcher tracks the pane". This is one new browser case; none were removed.closeTabschange reverted, it fails at Close all tabs from the channel visit.paneOpen, it fails at the ⌘⇧W step from a closed pane.Fail before, pass after:
aria-expandedis "true" after Toggle tab pane.useConversationTabs,useConversationTabs.panels,useChannelPanels,useChannelTabStateandChannelsPage.tabs.todos.spec.mjs,tab-close-others.spec.mjsandright-pane-dismissal.spec.mjspass in Chromium and WebKit, 32 tests in all. So does the layout.spec case "Todos stacks beside threads".tsc --noEmitandbiome check --error-on-warningsare clean.Not checked
buzz:close-all-tabs, the window event the native menu sends. The page handled it (preventDefault), and the result matched the right-click menu.Related
dismissContentkeeping(pane === "side" && tabState.sidePanel). Whichever of the two lands second must keep that condition.closeTabsand changesonCloseTabs. If it's revived, it needs the same two rules:onCloseTabsdismisses toward the side pane while a launcher's side panel is selected, and the return reapplies the pane's pre-close visibility.Before and after
Both runs used the same scripted steps on the same timeline. Each run checked the expected state at every step, and both passed.
2c81a4905 has the same fix and tests as this PR, from before the rebase. The only later commit changes a comment and docs. The video was recorded in Chromium against the fixture relay, with ⌘⇧W sent as the
buzz:close-all-tabsevent. Back, and Close all tabs from the earlier channel visit, are covered by the journey but not shown in the video.close-all-tabs-before-after.mp4
🤖 Generated with Claude Code