Repository navigation
Keep Messages selected across its sidebar destinations - #848
wesbillman wants to merge 4 commits into
Conversation
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
One change requested: keep keyboard focus on a stable element when retrying a failed Messages opening (inline). Automated source review by Star Lord via Wes’s account, head a9e753b26ec9fbadeda2213e7b8554667bd5472a, base 37dcd836fbcf1613ddd27cece5a99e04d601bd64. Source and both attached images inspected; no tests or app execution performed. The hosted snapshot has required CI failing on a 5-second WorkflowsPage.test.tsx:335 timeout; causality is unestablished, and native acceptance remains unverified.
| !current.readOnly && !requestedMessage && !requestedThread | ||
| // The bound request arrives after the composer's first render. | ||
| (navigation?.focus ?? | ||
| navigator?.snapshot().attempt.focus) !== "preserve" && |
There was a problem hiding this comment.
[P2] Hand off focus when the preserved opening is retried
This suppresses the recovery focus as well as the initial tab-to-page autofocus. If opening Messages from a header tab fails (for example, its channel window errors), App.tsx:253–267 replaces the page with “Retry navigation”. A keyboard user can focus and activate that button, but route.retry() calls navigation.retry(), which retains the original focus: "preserve" (controller.ts:263). The retry button is then unmounted; this new guard prevents the remounted composer from focusing, and the wide shell has no retry focus handoff, leaving focus on the document body rather than the tab or recovered content.
Keep suppression for activation from the persistent header tab, but explicitly hand off focus to a stable main/panel or tab when the retry control itself owns focus, before removing it. Add a failed-opening → keyboard Retry → successful recovery regression asserting that focus remains on a meaningful element; the existing live-update retry already has an explicit handoff. This follows DESIGN.md’s requirement to preserve focus restoration, without changing retry’s stored focus intent.
wesbillman
left a comment
There was a problem hiding this comment.
The existing retry-focus finding remains unresolved. No new findings in the test-only follow-up: bulk fixture input preserves both replacement cycles and their assertions without changing production code or timeouts.
Star Lord’s automated source review via Wes; head b3f8f8d8ced618f3046b80d042ec2f4d8187c559, base 37dcd836fbcf1613ddd27cece5a99e04d601bd64. No tests/app execution; hosted CI was still running and native acceptance is unverified. Dark attachment inspected; the light attachment could not be freshly fetched (HTTP 504), so its previously saved copy was inspected instead.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Thanks for this. Blocking: one correctness issue at b3f8f8d8ced618f3046b80d042ec2f4d8187c559. It is the same issue as the open retry-focus thread (r4239536720), confirmed independently from the source.
Blocking: keyboard Retry after a failed Messages opening leaves focus on the page body
- The new guard at
ChannelsPage.tsx:1527-1533turns off composer mount focus for any opening whose intent ispreserve. Header-tab activation and the new Messages reselect both sendpreserve(PageNavigation.tsx:32-35). - If that opening fails,
App.tsx:253-274renders the "Retry navigation" alert. A keyboard user focuses Retry and activates it.route.retry()(navigation.ts:301-318) callsnavigation.retry(), which restarts with the storedpreserveintent (controller.ts:263). - On success the alert and its focused button unmount. The composer mounts without focus, and nothing else takes it, so focus falls to
document.body. Before this change the composer took focus on that path. - Fix: before the alert unmounts, move focus from the Retry control to a stable element (the Messages tab, the shell panel, or
#main-content), or give the retry its own focus intent. Add a test: a failed header opening, then keyboard Retry, then a successful recovery, asserting the final focused element. The existing reconnect test (tests/browser/navigation-repairs.spec.mjs:40-59) checks requests and composer visibility, not focus.
What holds
headerPageSelection(presentation.ts:74-93) maps only registered sidebar/default-placement pages to Messages, and only when Messages is a primary topbar page. Me, Settings, toolbar and independent header pages, and unregistered keys keep exact selection. The sidebar still receives the exactselected, so routes and sidebararia-current="page"are unchanged.onReselectis opt-in.PageNavigationis its only caller, so the otherTabsconsumers get no new behavior.Tabs.tsx:97-99compares against the current render's value, so activating an unselected tab never also reselects. Reselecting the exact Messages destination does nothing.- The panel id follows
panelKeyas an attribute, not a React key, so nothing remounts andaria-controls/aria-labelledbystay paired. Back/forward and channel-row openings keep thecontentintent, so the composer still autofocuses there. Join completion and the live-status Retry keep their explicitfocusRequesthandoffs. - The
WorkflowsPage.test.tsxfixture change still exercises both replacement cycles and every assertion.
Optional
- The compact browser journey (
tests/browser/page-placement.spec.mjs:213-232) checks the Messages area marker in More pages, then dismisses the menu without activating Messages. The RTL compact branch (AppShell.test.tsx:508-516) checks only the callback and immediate main focus against a mocked destination. Activating Messages from compact overflow and asserting the returned conversation, the draft, and the settled focus would cover the real path. - CI at this head: Browser journeys (webkit, 1/6) failed
me-workspace.spec.mjs:110(the activity popup button was never found). The previous head has the same production code and passed that shard, so it looks like a flake. The failed jobs are rerunning. - The description has no
buzz-review-completed.
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
No actionable changes requested. The retry-focus finding is addressed in source; the keyboard-preview follow-up introduces no new source-demonstrated defect.
Star Lord’s automated source review via Wes’s account; head db46f0202b7fe93dd867b28040a2252606d9341e, base 37dcd836fbcf1613ddd27cece5a99e04d601bd64. Both attachments inspected; no tests or app execution. Required CI still fails on two 5-second ChannelMembersDialog.test.tsx timeouts; causality is unestablished and native acceptance remains unverified.
Summary
Messages now stays selected when navigating to Inbox and other pages in its sidebar. The header previously matched only the exact destination, so leaving the conversation page cleared its selection even though the user was still inside Messages.
No new routing state, last-child memory, persistence changes, or sidebar redesign.
Validation
Local macOS checks passed on the final source tree committed as
a9e753b26ec9fbadeda2213e7b8554667bd5472a(changed-file hashes verified after commit):Browser command:
bin/pnpm test:browser tests/browser/page-placement.spec.mjs --project chromium --project webkit --no-deps.Extended the existing page-placement journey; no browser cases added or removed. Real browser focus, pointer/Enter/Space activation, draft retention, reload/history, and responsive overflow require this integration layer. Both themes, 390px width, and 200% root-font scaling are covered; this is not full browser zoom. Plugin/registration permutations remain in the pure-helper tests, with shell and shared Tabs wiring covered in RTL. Negative control: reverting the AppShell area-selection wiring made the new Inbox assertion fail (
aria-selected=false); restoring it passed both engines.Independent agent reviews found no material blockers: Mordecai reviewed ownership/focus source, Princess Donut reviewed the diff and light/dark captures, and Mongo reviewed coverage and ran Biome on touched files. They did not independently run suites. Documentation and the additional Me → Messages focus assertion address their feedback.
Visual evidence
Synthetic browser fixtures, not a live account or native acceptance. The extra fixture plugins and loading/retry sidebar are test-only content. Captures below wait for the selection indicator to settle.
Light
Dark
CI timeout repair
b3f8f8d8ced618f3046b80d042ec2f4d8187c559changes only fixture input in the workflow same-revision draft-replacement test: onefireEvent.changeper invalid value instead of per-character typing. Real mounted React, user clicks, both replacement cycles, and every assertion remain. No timeout/retry changes, skipped cases, or production edits.BUZZ_TEST_WORKERS=2 bin/pnpm exec vitest run src/bundled/workflows/WorkflowsPage.test.tsx: 22/22 before and after. Before (a9e753b26): import 0.93 s, environment 0.47 s, wall 6.50 s, summed tests 5.02 s, affected test 1.45 s. After (same head plus the committed test-only patch): import 0.62 s, environment 0.26 s, wall 5.72 s, summed tests 4.75 s, affected test 1.15 s. These single local observations do not establish hosted timing margin.b3f8f8d8c: 654 files / 9,220 tests passed, zero skipped; reporting-wrapper wall 362.12 s, summed test execution 465.57 s. Affected test 1.19 s. Slowest test: Vite-config import 4.60 s; slowest file: ChannelMembersDialog 24.45 s. Required commit/push checks and current-head DCO passed.second-partial). Production source restored before final validation. Independent source review found no coverage blockers. No browser cases added or removed by this repair.New hosted CI is running: https://github.com/block/buzz-app/actions/runs/38094795996. CI is not yet verified green for this head. Existing human/native acceptance and approval requirements are unchanged; no running app was modified.
Remaining acceptance gate
Human confirmation, native-app acceptance, hosted checks, and required approvals remain acceptance requirements. GitHub currently marks the PR ready for review; this CI-only repair does not attest those gates. No running app or live identity was changed. Full browser/native suites remain with CI; local browser evidence is limited to the page-placement file and screenshot capture.
On this branch in an agreed native test setup:
Implemented by Carl, an automated agent; PR opened through Wes’s GitHub account.