Repository navigation
Conversation
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One non-blocking maintainability finding; no functional or security defect established in the exercised flows.
a19bb9c to
9555c2e
Compare
…2562) ## Summary Closing a routed panel tab calls `history.go(-1)` to retire its visit, and that return lands on a later popstate. Until it lands, the navigator still shows the closing visit, but the opener already has focus. If a Profile was reopened from the opener in that window, it was hidden when the return arrived, because entering the channel visit restored that visit's saved selection. This PR treats a reopen made before the return lands as superseding the close, so the panel stays open with a fresh visit of its own. ### fix(channels): keep a panel reopened before its close's return lands **Problem** Two WebKit journeys lost a Profile that was reopened right after its tab closed. Both failed on attempt 1 on unrelated branches, and the same SHA passed on rerun. - `profiles.spec.mjs:321` "profile plumbing: exact avatar/mention targets, thread enrichment, lifecycle and recovery", branch peon/participant-collator, run 41752442750 (webkit 13/14): https://github.com/block/buzz-app/actions/runs/41752442750/job/126320885261. After the Mic tab closed, Space on the focused mention never reopened the Profile, and the test failed at :479. - `solo-workspace.spec.mjs:159` "Solo chat Activity opens the agent's profile locally, restores per conversation and retires on plugin disable", branch peon/dnd-stable-transform, run 41757728323 attempt 1 (webkit 5/14): https://github.com/block/buzz-app/actions/runs/41757728323/job/126338025674. After the agent's Profile closed, Enter on the focused Activity trigger never brought back "Profile details". **Cause** - Closing a routed panel tab removes it locally. `dismissContent` then calls `navigator.dismiss`. The browser driver retires the visit and calls `history.go(-1)`, which lands on a later popstate. Until then the navigator still shows the retiring visit, while `afterClose` has already focused the opener. - A reopen in that window runs `open()` and then `visitPanel`. `visitPanel` sees the navigator already on that Profile's detail, so it doesn't navigate. When the return lands, the route effect enters the channel visit, and `enterVisit` restores that visit's saved selection, which hides the reopened panel. Reopening a different panel pushes its visit on top of the retiring one, and the late return then removes it. - Both traces show this sequence: - Profiles run: the close finished at 80128 ms and Space ran at 80148–80193 ms with the URL still on Mic's detail. By 80383 ms the URL was the channel and the dock was closing. - Solo run: the close finished at 251783 ms and Enter ran at 251808–251872 ms. By 251999 ms the URL was the channel and the dock was hidden. - The dock isn't the cause. `PanelDock` keeps a reopened subtree through its exit (`PanelDock.test.tsx`). **Fix** - When a dismissal retires the current visit and the navigator hasn't moved yet, `dismissContent` records a pending return. A synchronous driver or a replacing dismissal moves the navigator at once, so neither records anything. - While that return is pending, `visitPanel` records the panel as the reopen instead of navigating. - When the return lands on the dismissal's fallback, the route effect checks whether that panel is still selected. If it is, the effect reselects and opens it, which gives it its own visit. If the panel has closed again, or the return landed somewhere else, nothing is revived. - A later close before the return lands, such as closing the thread beneath a closed Profile, moves where the return lands. The record follows the latest fallback and keeps the reopen. - While the return is pending, the route effect no longer reapplies the retiring visit's detail. Otherwise a re-render could reopen the panel that was just closed. - Two new cases in `profiles.spec.mjs` hold `history.go` and pause the dock's exit while a Profile closes. One reopens Mic by click and by Space. The other closes a thread's Profile and then the thread, and opens Mic. Each asserts that the Profile shows while the return is held, the navigator stays on the closed visit with exactly one return held, a fresh Mic visit lands after the release, and Back returns to the channel and closes the panel. The existing journeys are unchanged, and no assertions are loosened. **Residual limit** Thread and other explicit navigation during a pending return is not deferred, so a late return can still undo it. #2113 lists the same limit. **Validation** Local Linux workstation, Playwright 1.63.0, `--no-deps --retries=0`: - New cases without the fix: 4/4 failed. The reopen case failed at :406 (no fresh visit). The thread case failed at :442, with the click stacking a visit on the closed one. With the fix but without the latest-fallback step, the thread case failed 2/2 at :452. With the fix, all pass. - Adversarial ordering, with temporary changes that delayed `history.go` calls from click or key handlers by 250 ms, 6 repeats per engine: - Without the fix: 9/24 failed, seven with the CI signatures. - With the fix: 24/24 passed, twice. - Natural contention, with 4 CPUs shared with 4 busy loops, 2 workers and 10 repeats per engine: 40/40 passed with the fix. - The 38 browser specs that close panel tabs, both engines: all passed at 6 workers. At 12 workers, 42 failed on timeouts because the test server had stopped serving under load, and those 42 passed on rerun. - Vitest for channels, panels, navigation and channel-navigation: 87 files, 1539 tests passed. - Rebased on main `f4805605f5`: - `tsc --noEmit`, `biome check --error-on-warnings` on the changed files and `git diff --check` are clean. - Vitest `src/bundled/channels`: 61 files, 1097 tests passed. - `profiles.spec.mjs` and `solo-workspace.spec.mjs` in full, WebKit and Chromium, `--workers=4 --retries=0`: 48/48 passed. A green run doesn't prove the flake is gone. Hosted CI covers broad validation. ## Coordination - #2415 (open) rewrites the same route effect in `ChannelsPage.tsx` around a `dismissalReturn` record. Its head `9555c2ed` conflicts textually with this branch. Whichever PR lands second should fold this PR's pending `reopen`, and the guard against reapplying the retiring visit's detail, into the other's record. - #2113 (open) restores panel opener focus after history returns. It doesn't overlap these hunks, its head merges cleanly with this branch, and it lists the same residual limit described above. - Draft #1793 (stale since 2026-10-29) also edits `ChannelsPage.tsx`, and its head conflicts textually with this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: mtoohey <mtoohey@squareup.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord / Guardians full-feature review: recommend approval at the reviewed base, subject to integration with #2562; no new blocking source finding.
Reviewed head 9555c2edebad83f73ea7be11c0d93047dc6c6d96 against base/merge base 0db9aded728df70e2aa4848d2b3233238b730181. Star Lord, Rocket, Mantis and Groot reviewed the complete 12-file feature and prior feedback.
The prior consolidation P3 is repaired: one dismissalReturn record carries kept-tab and Solo-opener recovery, and registered header refs replace DOM-selector coupling. Both Solo headers route profiles and human DMs through the existing page/dialog owners and lifetime guards. Membership overflow entries use validated targets, preserve the overflow opener for focus return, and defer button/target construction until the popover opens. The open list still mounts every member; this is not virtualization or a new profile-read cache.
Integration requirement: this branch predates merged #2562. Preserve its pending-reopen handling and retiring-detail guard when combining the changes. Its separately reported P2 remains unresolved: close profile B with the native return held, reopen C, close inactive thread T, then release the return. This review does not establish a combined fix. Also cover Share access → close → reopen before the held return lands: the reopened panel must retain selection and appropriate focus instead of restoring the old header opener. That latter concern is conditional on integration, not a reproduced defect at this head.
Optional description correction: update the references to data-solo-opener selectors and the one-line dialog change to describe registered refs and the unified return record.
The committed unit/browser sources cover profile/DM wiring, refused targets, keyboard/pointer overflow selection, main/side header recovery, same-profile reopen, owner handover and Back/Forward opener identity. Browser hover-to-profile and overflow focus return are present. Source-only: we ran no tests, builds or apps. The hosted check snapshot at this head has 45 successful checks and one skipped; this is status evidence, not independent reproduction of the author's repeat counts. Three videos were not viewed; native, assistive-technology and human acceptance remain unverified. Sidebar Share access, Solo-region and Huddle paths and Send message for agents remain disclosed exclusions.
9555c2e to
2976be3
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord / Guardians focused integration review.
Reviewed head 2976be37b8fac586361fd7c0997d7ec7bcb02608 against base 4123ea85846aa167013cd1da4aa448413be97cde (merge-base 2552bf65f9f2165e5fa5746a3f5c8b14bb65c1d8). Rocket, Mantis, Groot and Star Lord reviewed the integration with pending native returns and the unified Solo header, preserving the prior full-feature review.
One new P3 focus-recovery finding, detailed inline: a profile reopened through Share access while the previous close is waiting for native history loses its opener association when replay creates the fresh visit. Its subsequent close therefore returns focus to history rather than Share access or Chat actions.
The registered main/side header refs and updated browser selectors match the current header structure. The pending-reopen and retiring-detail guards are retained. The separately reported #2562 inactive-thread issue remains outside this finding; this pass does not establish a fix for it. The prior optional PR-description correction still applies.
Source-only: no tests, builds or app execution; the finding is inferred from source, not reproduced at runtime. The existing Solo browser cases do not cover this held-return sequence. The public PR description and one thumbnail from each of its three historical videos were inspected; the complete videos, current-head runtime behavior, native accessibility and human acceptance remain unverified.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord / Guardians focused follow-up: the prior P3 is repaired in source; no new actionable findings in this two-file revision.
Reviewed head d82458994eff7b337b4952fa53ef148a2299f4fd against base 4123ea85846aa167013cd1da4aa448413be97cde, comparing the repair with our reviewed 2976be37b8fac586361fd7c0997d7ec7bcb02608. Rocket, Mantis, Groot and Star Lord completed the focused review.
After replay creates the fresh visit, the repair replaces the retiring ID only in the owning Solo opener record and preserves its other visit associations. The added browser case holds the native history return, confirms the reopen remains queued, releases it, and checks the fresh visit, selection/focus and final opener recovery through both Share access and Chat actions. Existing feature coverage is retained. The PR description now describes registered refs and the unified dismissal record.
This preserves the prior full-feature review; #2562's separate inactive-thread issue is not fixed or re-reviewed here. Source-only: no tests, builds or app execution. The new regression was inspected, not run. The author reports fail-before/pass-after in Chromium and WebKit in the inline reply; that evidence was not independently reproduced. Native/human acceptance remains unverified. The three unchanged historical videos retain the previous inspection limit: one thumbnail from each was viewed, not complete playback or current-head evidence.
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One remaining focus-restoration defect when a queued different profile is closed before the pending return lands.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord / Guardians focused source follow-up: the queued different-profile close defect reported in review 5795926405 is repaired in source. No new actionable findings in this two-file revision.
Reviewed head 54c54f1faba915f963b55f1c251e4ff046a92432 against base 4123ea85846aa167013cd1da4aa448413be97cde, comparing with previously reviewed d82458994eff7b337b4952fa53ef148a2299f4fd. Rocket, Mantis, Groot and Star Lord completed the focused review.
A queued profile has no visit of its own. Its close now uses the matching pending return to retain the latest Solo opener, while the existing target, trigger-identity, selection and waiting-focus guards remain in place. The added browser case holds the native return through both profile closes and checks both opener orders, header remount, no profile resurrection and final focus on the second opener. The browser layer fits the native history/focus boundary; prior feature and side-chat coverage is retained.
Source-only: we inspected the regression but ran no tests, builds or app flows. Runtime, native and human acceptance remain unverified. The separate inherited #2562 issue remains outside this repair. The PR description and three historical video URLs are unchanged; previous inspection covered one thumbnail per video, not full playback or current-head behavior.
54c54f1 to
188229b
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord / Guardians focused integration review: no new actionable findings.
Reviewed head 188229bc4c217a3558f9f024c5a57632e173806b against base/merge-base 3bcf1c4bb1111d924a84d9b710c1da2db80ccf0b. All 12 feature patches match the previously reviewed 54c54f1faba915f963b55f1c251e4ff046a92432; this pass checks their integration with shared navigation replacement, draft visits, contextual timeline seeking and member lookup.
Profile opens still reach the navigation controller through visitPanel. The new pass-through policy is registered for draft visit IDs, not existing chat/profile visits, and replacements receive fresh IDs. Explicit profile dismissal and the queued-return opener repairs remain intact. The contextual seek preserves thread focus, and the existing Share access/Chat actions and membership browser cases are unchanged.
Rocket, Mantis, Groot and Star Lord completed the focused source review. Prior feature/repair coverage and the separate #2562 exclusion remain. We ran no tests, builds or app flows; runtime, native and human acceptance remain unverified. The unchanged three historical videos retain the prior inspection limit: one thumbnail each, not full playback or current-head behavior.
188229b to
e280a4f
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord / Guardians focused integration review: no new actionable findings.
Reviewed head e280a4f64d48a656541a71c4658f1bc6f977edd1 against base/merge-base cae3922574c5c1f0ce5d32231810229e94b1bb40. The twelve feature and repair patches from the previously reviewed 188229bc4c217a3558f9f024c5a57632e173806b are retained: ten identical and two with context-only changes.
This pass checked their interaction with header subscriptions, member matching and confirmation focus, retained menu/drawer lifetimes, navigation and compact message styles. Header subscriptions retain the registered trigger refs; the queued-reopen visit remap and latest-opener recovery remain intact in source. The membership popover and existing browser regressions are unchanged. Prior feature/repair coverage and the separate #2562 exclusion remain.
Rocket, Mantis and Groot completed independent lanes, integrated by Star Lord. Source-only: no tests, builds or app flows were run; performance, runtime, native and human acceptance remain unverified. The PR description and three historical video links are unchanged; the videos retain the prior inspection limit of one thumbnail each, without full playback or current-head behavior verification.
|
🤖 Design review at Checked the main/side Solo headers, agent-owner handoff, refused profile targets, scrollable membership list, and focus guards for Back/Forward and queued reopening. Source: people list, focus return. Limits: inspected test source and six sampled frames per recording; recordings show |
Solo Share access listed people with no route to their profile: names and "Managed by" were plain text, View profile was disabled and person rows had no Send message. ChannelWorkspace now builds the team dialog's three member link props once and passes them to the team Manage members dialog and to both Solo headers, the main-pane SoloHeading and the side-tab SoloToolbar. SoloToolbar forwards them to its header Share access button and its Chat actions dialog. The side-tab toolbar renders inside useConversationTabs, before canOpenLink and openLink are declared, so the props wrap them in arrows that read them only when called. A membership row's "N others" list showed plain names. Its entries now use the lead name's button, shared through a local profileName helper: each opens that member's profile (the agent profile for agents) and closes the list. "N others" moves into a small MembershipPreview component so it has a ref; a chosen entry focuses it before opening, so closing the profile returns focus there. Entries the host cannot open stay plain text, and the list takes the lead names' quiet .membershipName look. The sidebar and Solo-region Share access mounts and Huddle members still open the dialog without links; they live outside ChannelsPage. Tests: new RTL cases for the Solo header, Chat actions and side-tab paths and for the "N others" list, each failing before the fix; the join-burst journey now asserts 602 entry buttons and opens Pinky's profile from the list, returning focus to "601 others". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The "N others" list built every member's profile button whenever its membership row rendered, even with the list closed: an npub encode plus the host's canOpenLink per member, about 3.75 ms per pass for a 602-member burst and repeated on each memo miss while the burst's profiles load. The list moves into a MembershipPeople component inside PopoverPopup, whose portal mounts it only while the list is open. The list's scrollport clipped its entries: each .link overhangs by space-half, so the hover fill lost 2px on the left and the 2px-offset keyboard ring was cut on the left of every entry and the top of the first. Following DESIGN.md "Focus recipes", the scrollport gains space-half of inline padding (offset by a matching negative margin, so names stay aligned with the title) and the entries' ring is inset with outline-offset: -2px. Member names now rest at currentColor instead of text-subtle. Row names still inherit the caption's text-subtle, while listed names take the popover's text-standard rather than reading dimmer than the plain entries around them. Hover and keyboard focus keep the inline-reference colour. The comment no longer says medium weight is the only resting cue; the .link underline stays. Tests: the "N others" RTL case asserts that a closed list asks the host about its lead name only, which fails against the inline map. The join-burst journey checks that a resting entry matches the list's colour, Tabs to the first name, asserts its drawn ring fits inside the scrollport and saves a focus screenshot. The colour and clip assertions fail before their fixes (rgb(82, 82, 82) vs rgb(15, 15, 15); 6px clipped). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Closing a profile opened from Solo Share access, either the header button or Chat actions → Share access, left focus on the "Chat <name>" section instead of the control that opened the dialog. Team Manage members already returns there. The main-pane SoloHeading is keyed on navigation.entryId. Since #784 (moved to SoloHeading in #1273) each visit remounts the header. That ends the visit's header dialogs, menus and toasts, which are scoped to its signal and don't close themselves on abort. The key stays. Opening the profile is a new visit, so the header remounts and detaches the opener that panelTrigger holds. afterClose then falls back to the history section. The dismissal's async return to the chat remounts the header again. This follows keptTabReturn. onClosePanel records a dismissal-scoped return when the panel's opener is detached and marked data-solo-opener. The visit layout effect consumes it only on this dismissal's matching return, and only while focus is still on the history section. It then focuses the same control in the remounted header. A Solo side tab's toolbar isn't keyed per visit. When its profile closes, the pane keeps the chat's tab and focus stays with it, so that outcome is unchanged. The RTL tests "opens a profile from the header's Share access and returns focus to it" and the Chat actions variant passed despite the bug. Their fixture's navigation is static (entryId "visit", open is a vi.fn), so no visit is ever applied. The header never remounted and the original trigger stayed connected. Their focus assertions are removed and the header test is renamed. A new solo-workspace.spec.mjs journey covers both openers across real navigation entries. It also asserts that opening the profile changes the entry. On 7ae9be9 its two focus assertions fail in Chromium and WebKit, and they pass with this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Closing a profile could focus a Share access or Chat actions control that never opened it. 9aba9a2 chose the control from panelTrigger when the profile closed. That ref lives as long as the session's ChannelWorkspace, and a detail visit re-applied by Back/Forward doesn't refresh it. Open a profile from a message author in Solo chat B, then Sam's profile from chat A's Share access. Back twice shows B's profile again, and Escape focused B's Share access instead of B's history section. openLink now records a Solo header's opener when it opens a panel, while the opener is still connected. The record keeps the opening's chat and target, plus the side tab whose toolbar held it. Every onClosePanel consumes the record. It applies only to the panel it opened, with the same chat and target, while it is still the panel's trigger and detached from its header. The dismissal's return keeps its existing gates. Side tabs change behaviour. 9aba9a2 left focus on a Solo side tab's own tab when a profile opened from its toolbar closed. Now the toolbar's Share access or Chat actions takes it back, as the main header's does. This follows onCloseThread: while tabs remain, an opener takes focus only from within their pane. The profile covers the chat's tab and unmounts its toolbar. On close the dock reselects and focuses the chat's tab. On the dismissal's return, and only while focus is still on that tab, the same control in the dock's remounted toolbar takes focus in place of focusKept's tab focus. No rule in docs/, AGENTS.md or the tab strip keeps focus on the tabs. The side-tab RTL comment that said so came from 764e4c2. The lookups stay apart. The main one searches [data-solo-focus-pane]. Side-tab toolbars live in the PanelDock, outside it, which the lookup comment now says instead of implying document order. The side lookup starts from the focused tab's [data-panel-workspace], which shows only its selected tab's toolbar. Tests: two solo-workspace.spec.mjs journeys. One restores Sam with Back then Forward and closes him, and focus returns to Share access. It then goes Back to Beta's author profile and closes it. On 9aba9a2 focus lands on Beta's Share access, failing the Chat Beta assertion in Chromium and WebKit. The other opens Beta in a side tab of team channel Gamma and closes profiles opened from its toolbar's Share access and Chat actions. On 9aba9a2 focus stays on the Beta tab, and both focus assertions fail in both engines. The side-tab RTL test drops its Gamma tab focus assertion, because its fixture profile isn't a history panel and its visit never changes. It now covers opening from both Share access and Chat actions → Share access. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
dc922d4 records a Solo header's opener and lets only the closing panel it opened return focus. Two flows lost that return. An agent's profile opened from Share access can open its owner's ("Open owner profile"). The owner's profile replaces the agent's tab: the push records a visit, and applying it reopens the panel alone. Its close never matched the record's target, so focus went to the history section. 9aba9a2 returned it to Share access, like team Manage members. visitPanel now hands the record to a panel opened in place of the recorded one, in the same chat. Every close consumed the record, even of an unrelated tab. A Solo side tab's profile can sit beside a retained channel tool, such as Gamma's Canvas. Closing the Canvas cleared the record, and the profile's close left focus on the Beta tab. Now only a close matching the record's chat and target consumes it. The channel, target and trigger-identity checks still reject a stale record. The review's repro, closing the owner's tab and then the agent's, does not occur in either Solo host, since the owner's profile replaces the agent's. The review's clear-on-match fix alone leaves the nested step failing. serveProfile takes an optional owner key and signs a real NIP-OA attestation. The fixture's soloAgentNames carry a placeholder signature, so their profiles show no "Managed by". Tests (solo-workspace.spec.mjs, Chromium and WebKit): - The Share access journey opens Scout, an agent Sam manages, then Sam from Scout's profile, and closes Sam. Focus returns to Share access. It fails on dc922d4 and with only the clear-on-match change, and passes on 9aba9a2. - The side-tab journey opens Gamma's Canvas, then Sam from Beta's Share access, closes Canvas, then Sam. Focus returns to Share access. It fails on dc922d4 and with only the hand-over. - The Back/Forward journey reads the entry before Escape and polls for the dismissal's return before asserting Chat Beta stays focused. On 9aba9a2 that assertion now fails, with focus on Beta's Share access. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
d7aa710 records a Solo header's opener with its chat and profile target, so it can't tell two visits of the same profile apart. Wrong opener: open Sam from a message author (V1). With Sam open, open Scout from Share access (V2), then "Open owner profile: Sam Rivera" (V3). Go Back twice to V1 and press Escape. Focus went to Share access, though the author opened V1. Without the handover, author Sam, author Fixture Reader, Share access Sam, Back twice and Escape did the same. Lost opener: Share access opens Scout (V1), and the owner link opens Sam (V2), which moved the record to Sam. Go Back to V1 and press Escape. Focus landed on the history section instead of Share access. Closing Sam first, then going Back to Scout and pressing Escape, lost it too, because Sam's close consumed the whole record. The record now holds the navigation visits its opener produced, not a channel and target. openLink records the visit that shows the panel once open() returns, whether pushed or already current. Both host drivers publish pushes synchronously. visitPanel adds a visit pushed from one of the record's visits in the same chat, which covers the owner handover. onClosePanel returns focus only when the closing visit is in the record, together with the existing trigger-identity and detached checks. Each history-panel close removes the visits it dismisses and drops the record once none remain. A non-history close, such as Gamma's Canvas, leaves it. The channelId and target fields and the target-based handover go. Tests (solo-workspace.spec.mjs, Chromium and WebKit): - The Share access journey, after closing Sam, goes Back to Scout and presses Escape. It then reopens Scout, follows the owner link, goes Back to Scout and presses Escape. Both expect Share access focused. - The Back/Forward journey publishes a message from Sam in Alpha and runs both wrong-opener sequences. Each polls entry() past the dismissal's return, then expects Chat Alpha focused. A soft-assertion copy run against d7aa710's ChannelsPage.tsx fails exactly these four steps in both engines and no others. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Choosing the profile that is already showing, from Solo Share access or Chat actions → Share access, lost focus to <body> when that profile closed. Reopening it pushes no visit, so the main SoloHeading doesn't remount and the opener stays connected. onClosePanel recorded a return only for a detached opener, so it recorded none. afterClose focused the connected button, then the dismissal's return remounted the keyed header and removed it. The gap dates from 764e4c2. For a main-pane record, onClosePanel no longer requires a detached opener. The dismissal retires the current visit, so its return lands on another entry: the previous one, or a fresh replacement. SoloHeading's `${current.id}:${entryId}` key therefore always remounts the header. The return's layout effect, which runs in that commit, now accepts focus on <body> as well as on the history section. The remount leaves it there when it removes the focused opener. Accepting <body> leaves the earlier flows alone. In each journey the opener was detached at the close, so focus waited on the history section. When a tab remains beside the profile, such as a Solo thread, the tab strip focuses it at once, and a probe found it still focused after the return, with or without a reopen. The side-tab path keeps its detached check. Selecting the profile tab unmounts the toolbar, so the opener is detached even when Share access picks the profile already open in a side tab, which pushes no visit. A probe confirmed focus returns to Share access there. Comments now say what the record adopts: any visit pushed from one of its visits in the same chat, not only a panel opened in place of its own. A same-profile reopen takes over the current visit instead of owning one it pushed. Tests (solo-workspace.spec.mjs, Chromium and WebKit): - The Share access journey reopens Sam's profile while it shows, first from Share access, then from Chat actions → Share access. It checks entry() is unchanged, presses Escape, waits for the old header control to detach and expects the opener focused. A soft-assertion copy run against 40b3a49's ChannelsPage.tsx fails exactly these two assertions in both engines, and passes with the fix. - The Back/Forward journey's negative checks, the Beta step and closeRestored, now wait for the old Share access button to detach instead of polling history.state. That detach happens in the commit whose layout effect consumes soloOpenerReturn. With these barriers the journey still fails on d7aa710 (both closeRestored steps) and on 9aba9a2 (the Beta step too). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The side-tab return keeps its detached-opener check because selecting a profile tab unmounts the Solo chat's toolbar. 618d195 confirmed with a probe that this holds even when Share access picks the profile already open beside the chat, which pushes no visit. No journey covered it. The side-tab journey now opens Sam from Beta's Share access, reselects Beta, and picks Sam again from Share access. It checks that entry() is still Sam's visit, presses Escape on Sam's tab and expects Share access focused. It reuses the existing steps' barriers: Sam's tab is gone and Beta is selected. The close focuses the Beta tab, the toolbar is keyed on its chat, and afterClose doesn't focus the trigger while tabs remain. Only the dismissal's return can therefore focus Share access, and the positive assertion can't pass on a transient. A soft-assertion copy fails the new focus assertion in Chromium and WebKit against three throwaway sources: - The dock keeps a Solo chat's toolbar mounted while another tab is selected (PanelWorkspace): it fails with every existing side-tab focus assertion and the toolbar-unmounted check. - The side-tab return is dropped (ChannelsPage): it fails with every existing side-tab focus assertion. - A side-tab opener is recorded only when its opening pushed a visit: it is the only assertion that fails. The step passes on 618d195. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
A review comment on PR 2415 (r4469565101) found that soloOpenerReturn duplicated keptTabReturn's from/target matching in the same layout effect, then cleared focusKept to override it. The opener lookups also depended on DOM markers owned by the toolbar and dock. One dismissalReturn record now holds the dismissal's from and target, with an optional kept tab and an optional Solo opener control. dismissContent sets it for either, under its existing check that the dismissal retires the current visit. The visit layout effect consumes it in one place. On the matching return the kept tab is reselected as before. The opener's control takes focus while focus still waits where the close left it; otherwise the kept tab does. focusKept is no longer set and then cleared. SoloToolbar owns its Share access and Chat actions refs and registers them with the page through registerOpeners, by pane: the main SoloHeading and the dock's toolbar, which shows only its selected tab. ChannelMembersButton takes the toolbar's ref for its button. openLink finds the trigger among the registered refs instead of reading data-solo-opener and walking to [data-solo-focus-pane] or [data-panel-workspace]. The return focuses the same control in the pane's current registration. Registration runs in a layout effect, so a header remounted by the return registers before the page's layout effect consumes the record. The data-solo-opener attributes go. The side-tab gate still reads the focused tab's data-tab-value, the shared Tabs marker that the page's other tab focus lookups use. No behaviour change. Tests: the ChannelsPage solo, tabs and page RTL files pass, as do ChannelMembersDialog, MemberAdministration, SoloRegions, App.profile, navigation and useConversationTabs. MemberAdministration failed one toBeVisible under load and passed alone. solo-workspace.spec.mjs passes 22/22 in Chromium and WebKit. right-pane-dismissal, navigation, navigation-boundaries and profiles pass in both; one WebKit page.goto timeout under load passed on rerun. With registration made a no-op, all three Share access journeys fail in Chromium. With it moved to a passive effect, the two main-pane journeys fail and the side-tab one passes, since the dock's toolbar mounts at the close. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Main's unified conversation headers (8b908c8) name a Solo chat's main-pane Share access control "Share access to <chat>". The header Share access tests now look up that name. On the rebased base, the side-tab profile test's navigation stub, which returned a new snapshot object per call, loops renders with "The result of getSnapshot should be cached". It now returns one cached snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Review thread r4488965070 on PR 2415 found a lost focus return. Close a profile opened from Solo Share access while the close's native return is still pending, then reopen it from Share access or Chat actions. Closing the reopened profile focused the chat's history section instead of the opener. While the return is pending, visitPanel queues the reopen without pushing a visit (7d13663), so openLink records the retiring visit for the opener. When the return lands, the route effect replays the reopen and pushes a fresh visit from the landing entry. visitPanel adopts only visits pushed from one of the record's own, so the fresh visit was never added. Its close then failed the record's visit check. The route effect now gives the fresh visit to a record that holds the retiring one, replacing it, right after the replay pushes it. The existing checks are unchanged: trigger identity, the dismissal's waiting focus, and the retained-selection gate. The reopened profile keeps its selection and focus when the return lands, and the earlier close's opener does not take focus. The effect now lists navigator as a dependency; it is the plugin's navigation and does not change while the page is mounted. Tests: a solo-workspace.spec.mjs journey holds history.go, closes Sam and reopens him from Share access, then from Chat actions. It expects the navigator still on the closed visit, exactly one held return and, after release, a fresh visit for Sam. Sam's tab stays selected and focused. Escape then waits for the old header to detach and expects the opener focused. On 2976be3 the final focus assertion fails in Chromium and WebKit, and it passes with this change. solo-workspace.spec.mjs and right-pane-dismissal.spec.mjs pass 50/50 in both engines, as do profiles.spec.mjs's two pending-reopen cases. The five ChannelsPage RTL files pass 95/95. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Review thread r4489182646 on PR 2415 found a lost focus return. Close person A's profile from Solo Share access while that close's native return is still pending. Then open person B's profile from Chat actions → Share access and close it before the return lands. Chat actions has focus right after the close. The return then remounts the header and leaves focus on <body>. B's open is queued behind the pending return, so B has no visit and its close retires none. dismissContent recorded a dismissal return only when the close retired the current visit. Every close cleared the record first, so B's close dropped A's record and recorded none of its own. The fresh-visit replacement from d824589 never runs, because the closed reopen isn't replayed. A close made while an earlier close's return is pending now joins that return, as pendingReturn already treats it, and records its kept tab and opener. A dismiss with no visits leaves the native return unchanged, so it still lands where the first close sent it. The latest close's opener and kept tab then apply. The trigger and visit checks in onClosePanel are unchanged, as are the waiting-focus and retained-selection gates on the return. A queued profile opened from somewhere other than a Solo header records no opener. Its close still clears the record, so A's opener doesn't take focus. Tests: a solo-workspace.spec.mjs journey opens Sam from one Solo header control and holds history.go. It closes Sam, opens Scout from the other control and closes Scout. It expects the navigator still on Sam's closed visit and exactly one held return. After release it expects the chat's entry, the old header detached, neither profile back, and Scout's opener focused. It runs Share access then Chat actions, then the reverse. On d824589 the final focus assertion fails in Chromium and WebKit, and it passes with this change. solo-workspace.spec.mjs and right-pane-dismissal.spec.mjs pass 52/52 in both engines. So do profiles.spec.mjs's pending-reopen and transient-profile cases, 8/8. The five ChannelsPage RTL files pass 95/95. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
e280a4f to
75869c3
Compare
…2803) ## 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. - Its launcher still reads pressed (`aria-expanded="true"`), so the first press "closes" the hidden panel and nothing visible happens. Only the second press brings Todos back. - **Toggle tab pane** and **Back** to the channel visit end the same way: Todos is hidden and the launcher reads pressed. Close all tabs from that earlier channel visit also hides Todos. ## 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. 1. **`closeTabs`** (`useChannelTabState.ts`): `paneOpen: side || (s.paneOpen && !!s.sidePanel)`. A pane showing a launcher's panel stays open, and a closed pane stays closed. 2. **`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"`. - `dismissContent` then records the existing side-panel survivor from #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. - The kept-return marker records the pane's pre-close visibility: a kept tab shows, and a side panel keeps `paneOpen` as it was. The matching return applies it after `enterVisit`, 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. 3. **Launcher `pressed` and `toggle` via `shown`** (`useChannelPanels.tsx`): a side panel counts as `shown` only while the pane is open, and `pressed` and `toggle` use `shown`. - On a hidden panel, one press goes through `onOpen` (`setPaneOpen(true)`) and shows the pane with the panel. - `selected` is unchanged, so the panel stays retained while the pane is hidden. - `ChannelPanelTabs` gains `paneOpen`. `useConversationTabs.tsx` passes `tabState.paneOpen`, the flag its tab-placed `pressed` already reads. Close other tabs on a main-pane tab now keeps the side panel beside it too. **Preserved:** - [Navigation history](https://github.com/block/buzz-app/blob/main/docs/navigation-history.md): closed thread visits are still retired together. Back and Forward still skip them, and no visit's saved pane state changes. Back to a channel visit still restores that visit's saved pane. Where it was hidden, Todos stays hidden, but the launcher now reads unpressed and one press shows it. - No visual change. Pressed now matches what's on screen. **Docs:** - `shell-design.md` describes Close all tabs and hiding the pane for side panels. - `plugin-architecture.md` defines `pressed` for `channelPlacement: "side"` as "shown in the open pane", not merely selected. ## Tests **Vitest** (`useConversationTabs.test.tsx`; `launcherHarness` gains a `placement` parameter): - "Close all tabs leaves a launcher's side panel as the open/closed pane showed it": - The open case fails on main. - The closed case fails if `closeTabs` drops the `s.paneOpen &&` guard. - "a side panel's launcher reads unpressed while the closed pane hides it, and one press shows it": catches the launcher change. - "Close other tabs on a main-pane tab keeps a launcher's side panel open beside it." **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. - **Why it needs a browser:** Close all tabs retires the thread's history visit, and the page's return to the channel visit has to keep the side panel. jsdom page tests don't route visits through navigation. - **What it covers:** 1. Toggle tab pane hides Todos with the launcher unpressed, and one press shows it. 2. Close all tabs from a thread visit keeps Todos. 3. Back to a channel visit that saved a closed pane hides Todos, one press shows it, and Close all tabs from there keeps it. Since #2838 a channel visit showing Todos saves an open pane, so the journey hides the pane there first. 4. ⌘⇧W from a thread visit with the pane closed doesn't reveal Todos, even when the channel visit it returns to saved an open pane. - **What fails without each part:** - On main, it fails at the first launcher check after Toggle tab pane. - With the `closeTabs` change reverted, it fails at Close all tabs from the channel visit. - With only the launcher change, Todos is gone after Close all tabs. - If the return drops the kept-return marker's `paneOpen`, it fails at the ⌘⇧W step from a closed pane. **Fail before, pass after:** - With only the first commit's tests applied to main, Vitest fails 2 of 42. The journey fails in Chromium and WebKit, where `aria-expanded` is "true" after Toggle tab pane. - With the fix: - 139 Vitest tests pass across `useConversationTabs`, `useConversationTabs.panels`, `useChannelPanels`, `useChannelTabState` and `ChannelsPage.tabs`. - `todos.spec.mjs`, `tab-close-others.spec.mjs` and `right-pane-dismissal.spec.mjs` pass in Chromium and WebKit, 32 tests in all. So does the layout.spec case "Todos stacks beside threads". - `tsc --noEmit` and `biome check --error-on-warnings` are clean. - These local runs were on the same three patches before the rebase onto 067db60. That rebase brought in only #2587 (inbox, thread and unread files), which touches none of this PR's files. The later rebase onto be67480 brought in #2838 (retained side panels); the journey's Back setups were reconciled with it in 2a30ea9 and have not been run locally since. ## Not checked - The native ⌘⇧W menu. The browser runs dispatch `buzz:close-all-tabs`, the window event the native menu sends. The page handled it (`preventDefault`), and the result matched the right-click menu. - The desktop app. ## Related - **#2415** reworks the same dismissal-return code. This change depends on `dismissContent` keeping `(pane === "side" && tabState.sidePanel)`. Whichever of the two lands second must keep that condition. - **#1793** (draft, conflicting) rewrites `closeTabs` and changes `onCloseTabs`. If it's revived, it needs the same two rules: - A pane showing a launcher's side panel stays open on Close all tabs. - `onCloseTabs` dismisses 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. | Step (starts at) | Before: main @ 3bcf1c4 | After: this branch @ 2c81a4905 | |---|---|---| | 1. Press **Toggle channel todos**, then open the thread from its reply chip (3.6 s) | Thread tab above Todos. Launcher pressed. | Same. | | 2. Right-click the thread tab, choose **Close all tabs**, hold about 3.5 s (10.0 s) | The whole pane closes, and Todos goes with it. The launcher still reads pressed, so the badge turns red. | The tab closes. Todos stays and fills the pane. Launcher pressed. | | 3. Open the thread again, then ⌘⇧W (16.2 s) | Reopening brings back the thread with Todos, because Todos was never deselected. ⌘⇧W then hides both, and the launcher still reads pressed. | ⌘⇧W closes the tab and Todos stays. Launcher pressed. | | 4. Open the thread again, then **Toggle tab pane** (23.2 s) | Pane and Todos hidden. Launcher **still pressed**. | Pane and Todos hidden. Launcher **not pressed**. | | 5. Press the Todos launcher once (30.0 s) | Nothing visible happens. The launcher flips to not pressed and Todos stays hidden. | The pane comes back with the thread tab above Todos. Launcher pressed. | 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-tabs` event. Back, and Close all tabs from the earlier channel visit, are covered by the journey but not shown in the video. https://github.com/user-attachments/assets/c40bd79d-7668-4511-b065-915e533ea6e6 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot reviews.
No new actionable findings at 75869c36f2c5acd9f34c42213dabe0a710661b73 against base/merge-base c9239f737f4985979eb5939e84c33ab3da2b1321.
This follow-up retains the prior twelve feature/repair patches (eleven identical; one import-context-only change) and checks their integration with live refusal, session/workspace teardown and recovery, profile demand, first-channel discovery, Share confirmation focus and changed message rows. The opener and queued-return repairs remain intact in source; old session callbacks are fenced and recovery reconstructs read-only addresses with fresh authority. Prior coverage and the separate #2562 exclusion remain.
Source-only: no tests, builds or app flows were run. The unchanged three historical videos retain the previous thumbnail-only inspection limit; they do not verify this head. Native behavior, full playback and human acceptance remain unverified.
Summary
Two people lists had no route to a profile: the Solo chat's Share access dialog and the list behind "N others" in membership rows. Both now open profiles, and closing a profile opened from Share access returns focus to the control that opened the dialog.
SoloHeading(main pane) andSoloToolbar(side tab) now passcanOpenLink,onOpenLinkandonOpenConversation. Names, the "Managed by" owner and the row menu's View profile open profiles, and the row menu offers Send message. The side-tab toolbar uses the page'sopenLink, as the main header does.docs/buzz-help.mdis updated.SoloHeadingis keyed on that visit, so it remounts and detaches the button that opened the dialog.ChannelsPage.tsxnow records the opener (its trigger, its side tab and the visits it produced). When one of those visits closes, the page's single dismissal-return record, which also handles kept tabs, carries the control, and the return focuses the same control in the remounted header.SoloToolbarregisters its Share access and Chat actions refs with the page for each pane (the main header and the dock's toolbar), so the page finds the control by ref, not by a DOM selector. The opener record adopts the visit pushed by an agent profile's Open owner profile, takes over the current visit when Share access reopens the profile already showing, and moves to the fresh visit when a reopen queued behind a pending native return replays. It is dropped once all its visits close. This covers Back/Forward across chats and Solo chats open as side tabs.Out of scope: Share access in the sidebar (
SoloWorkspaces.tsx), the Solo region (SoloDestination.tsx), Huddle members (HuddleLauncher.tsx) and Send message for agents.Before and after
Side by side: base
8668dbd46on the left,618d19557on the right. Both panes get the same clicks and keys at the same times, and each pill shows what the page reported. The only later commit,4cc8f4dd2, is test-only. Fixture data only.Solo Share access. Left: clicking a name, the owner, or a name reached through Chat actions → Share access opens nothing, and the row menu has View profile greyed out with no Send message. Right: each click opens the profile and closes the dialog, the menu offers View profile and Send message, and Esc returns focus to Share access, then to Chat actions.
staged-image-6fd891da-d31d-41b8-845f-5dc8ddcd7eaa.mp4
"Pinky and 601 others were added". Left: the list is plain text, clicking Pinky does nothing, Tab leaves the list. Right: all 602 entries are profile buttons, clicking Pinky opens the profile, Esc returns focus to "601 others", Tab focuses Pinky with a visible ring.
staged-image-62b7db95-a873-4207-99ad-addea6da7830.mp4
Solo chat Beta in a side tab beside #Gamma. Left: Share access in the side tab's toolbar opens the dialog, but clicking Sam Rivera does nothing. Right: Sam Rivera's profile opens as a tab; Esc closes it, reselects Beta and focuses the side tab's Share access.
staged-image-2a89e5dc-e71c-4c7b-9c61-f51c68b23359.mp4
Validation
Run locally on macOS at
4cc8f4dd2(unit tests at618d19557):bin/pnpm typecheck: pass.bin/pnpm lint: pass (26 info, no warnings).ChannelsPage.solo.test.tsx,ChannelsPage.tabs.test.tsx,membership-rows.test.tsx: 83/83.bin/pnpm test:browser --project=chromium --project=webkit --no-deps tests/browser/solo-workspace.spec.mjs tests/browser/membership.spec.mjs: 30/30, run twice.Browser cases added
Vitest's static navigator cannot remount the keyed header, so the focus-return contract needs real history and a real remount. Added to
tests/browser/solo-workspace.spec.mjs(andmembership.spec.mjsfor N others):7ae9be9389aba9a2389aba9a238dc922d455dc922d455d7aa71060d7aa7106040b3a492c40b3a492c618d19557618d195579aba9a238dc922d4558668dbd46764e4c261Negative focus checks wait on a barrier that proves the work which could violate them has run: the pre-Escape header element handle is polled until it is no longer connected, which happens in the same commit as the layout effect that consumes the return.
Deferred checks and known gaps
ChannelMembersDialog.tsxchanges by a few lines:ChannelMembersButtontakes an optionaltriggerref from its host, whichSoloToolbarregisters. Low conflict risk with feat(solo): share chats as view only #2375 and fix(members): preserve relay-recorded agent classification #2363.Review checklist
The review-completed attestation marker from AGENTS.md is intentionally left out of this description until step 3 holds.
🤖 Generated with Claude Code