Repository navigation
fix(settings): link a plugin's details to its Settings sections - #2419
Conversation
2fcc428 to
e6620be
Compare
joahg
left a comment
There was a problem hiding this comment.
🤖 Read the Settings changes and tests; sections are derived from live cards/providers so retired or disabled plugins drop their links, and the scroll-into-view is limited to link-opened sections. Merge state is DIRTY, needs a rebase.
A plugin's details pane in Manage plugins gave no way to reach the plugin's own Settings section; GitHub comment authors, for example, was only reachable from the sidebar. Settings now works out the selected plugin's sections from the sidebar's current sections whose contributed card is owned by that plugin, and the pane shows them as "Settings: …" with SettingsSectionLink. A plugin with no section, a disabled plugin (it registers no cards) and a community card in Personal space get no link. Following a link moves focus into the opened section through the existing contentFocus step, which the Decisions hint now shares as openSection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com>
A decision provider's settings live under the built-in Decisions API section, not a section the plugin adds, so TypeSafe's details pane had no link. The pane's sections now also include Decisions API when the selected plugin has a registered decision provider (matched on the provider contribution's pluginId). It keeps its sidebar position, appears once however many providers the plugin registers, and appears only when the section is in the current sidebar sections. The link line is now a group named "<Plugin> settings", so screen reader users hear what the links are for. Tests find the line by that group role, cover a plugin whose cards retire while its pane is open (the line goes, the pane stays), and cover a plugin that only registers decision providers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com>
In the narrow Settings modal, navigation stacks above the content in one scrollport. A section opened from a link inside the content (a plugin's details pane, or the Decisions API hint) kept that scrollport's offset, and the focus step uses preventScroll, so after scrolling down to the link the new section opened partway down with its heading out of view. The existing content-focus layout effect now scrolls the focused section to its start, as the Dialog already does for stacked sidebar clicks, but only when a scrollport around it has scrolled past that start. Where the heading is already in view nothing scrolls. The scroll is instant, so reduced motion is unaffected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com>
The stacked-scroll test only changed section by following a link, so it could not tell whether the scroll was limited to content links. Add a row with the same hidden-start geometry where the route changes the section without the link (as Back or a sidebar click does) and assert nothing is scrolled into view. The new row fails if the scroll is moved above the contentFocus early return, and passes with the current code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com>
e6620be to
b1fabf3
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 passes.
Reviewed full feature at head b1fabf356f72c43d431d9de886cf9bb402914321 against target base 25e3282fe248317a87ae87f1e1cf7c99495341fa (feature merge base b99951af32f143376404fbcf8b50064aedf39d08). Recommend approval at this revision; no blocking source finding. This is a COMMENT recommendation, with integration and validation limits below.
The links reuse the sidebar's live sections and card/provider ownership. Inactive revisions and withdrawn providers disappear through the existing registries; provider plugins get one Decisions API link in sidebar order. The real Settings opener preserves viewer/community scope. The shared focus helper scrolls a linked destination only when its start is above an ancestor scrollport. Codex, Back closing the details pane, and TypeSafe opening the backend picker when Builderlab is selected are accurately disclosed limits.
Optional P3 follow-up, extending an existing pattern: src/app/Settings.tsx:763-783 retains contentFocus if a section request is superseded or becomes unavailable before selection. If the same Settings instance later visits that id via the sidebar, the old intent can move focus and scroll as though the original content link had just opened it. Consider expiring the intent on cancelled/superseding navigation, preserving the normal intermediate render before selected adopts the requested section. A useful regression is request X → supersede with Y → later sidebar X, with no stale link-specific focus/scroll. This is conditional source inference, not a reproduced runtime failure.
Validation: reviewed all three changed files and relevant owners/callers, test sources and documentation. Hosted snapshot: 41 successful checks, two skipped. The new scroll regression mocks geometry; real 480×800/200% text heading visibility remains unverified. Two fixture screenshots inspected; video not replayed. No tests, builds or app runs performed by this review; native/WebKit behavior, assistive technology and human acceptance remain unverified.
The earlier DIRTY report is historical: captured metadata varies from mergeable/blocked to unknown. This review does not establish a current merge conflict or validate a rebased combination; recheck integration against the landing base.
## What
Test-only. Fixes an intermittent Chromium failure in
`tests/browser/bestie-conversation.spec.mjs:165`, "Bestie stays within a
block host with multiline history at narrow widths".
`expect(getByRole('textbox', { name: 'Reply to thread'
})).toBeInViewport()` failed with "element(s) not found" because no
thread had opened. It failed on [run
42133368195](https://github.com/block/buzz-app/actions/runs/42133368195/job/127617350396)
(#2419) and on an unrelated branch's [run
42133084134](https://github.com/block/buzz-app/actions/runs/42133084134/job/127615558182),
both in Browser journeys chromium 7/9.
## Why it failed
With `?long` at 480 px, the timeline opens at its latest message. The
first "View thread: 1 reply" button starts out of view (viewport ratio
0), so `locator.click()` scrolled the timeline to reach it. After every
scroll event, Virtua sets `pointer-events: none` on the list for 150 ms.
Playwright hit-tested the button and pressed it. Under CPU load, React
committed Virtua's scrolling style before the release. The release then
reached the history `<section>`, and `click` went to the section.
Playwright checks only the first pointer event, so `click()` reported
success and the next assertion timed out.
- In both CI traces, the snapshot at the click's input has no
`pointer-events` style on the list, and the snapshot after the click has
`pointer-events: none`.
- Reproduced locally with event logging. Every logged failure showed the
same sequence: `pointerdown` on the button, the list's `style` changes,
then `pointerup` and `click` on the history section. Meanwhile the
button stays connected at the same position, and `elementFromPoint`
returns the section. The thread handler is never called.
This is the race #2560 diagnosed for the channel timeline.
## Change
The test now opens the latest message's thread with `clickRow` from
`tests/browser/timeline.mjs`. `clickRow` waits for Virtua to restore
pointer events and for the geometry to settle. Then it requires the
control to be fully in view and clicks without scrolling. The first
button can't be used with `clickRow` because it is out of view when the
conversation opens. Which thread opens doesn't matter to this test:
every `?long` root has the same multiline body, and the assertions cover
the block host, the dock and the reply box.
## Validation
Chromium with `--repeat-each`, 4 workers pinned to 2 cores (`taskset`)
under an 8-CPU quota. The two runs ran side by side:
| | Runs | Lost click (this failure) | `page.goto` 15 s timeout | Passed
|
|---|---|---|---|---|
| main | 200 | 2 | 12 | 186 |
| this change | 200 | 0 | 5 | 195 |
The `page.goto` timeouts are noise from this setup: cold Vite servers on
overloaded cores. Across all loaded runs, the old click lost the click 6
times in 380 runs; this approach lost it 0 times in 324. Without load,
main passed 60 of 60, and CDP CPU throttling did not reproduce it
either.
- The whole Bestie spec passed 90 of 90 on Chromium and Linux WebKit (5
repeats).
- `biome check` and `git diff --check` pass.
Not addressed: as #2560 notes, `clickRow` can't stop an app scroll that
starts after its idle wait.
🤖 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>
## Problem Follow a plugin's "Settings: GitHub" link from its details in Manage plugins, then press Back (⌘[). Manage plugins comes back with no row expanded and no details pane, so you have to find the plugin again. The selection was local component state: the Settings visit never recorded which plugin's details it showed, so Back/Forward had nothing to restore (#2419 left this open). ## Change Each Manage plugins visit records the plugin whose details it shows (or `null`) through the existing per-visit store, `navigation.savePosition("settings.pluginDetails")`, the same way `ChannelTimeline` and `ThreadPanel` keep their positions. Back/Forward to that visit (`navigation.traversal`) reopens those details with focus on **Close plugin details**. Unchanged: closing details is kept, so Forward and Back to that visit show the list; choosing Manage plugins from another section starts at the list; addressed details links (`pluginId` in the target) behave as before. Three commits, in landing order: 1. `8a14c4ae1` — record and restore the shown plugin per visit. 2. `2818a4e0d` — adjacent visits: a stored `null` clears a selection still showing; each new visit records what it starts with; Back from an addressed visit (for example the plugin-update toast) restores the earlier visit's details, while Close through `dismissPluginDetails` still lands on the list; a removed plugin records `null`; "Open in Buzz" from a plugin import keeps its plugin. 3. `35349385c` — import paths: "Open in Buzz" from a link opened outside Manage plugins records on the visit that replaced the import; a stale `onClosed` after Install + Back can't leave the closing flag set; the effect no longer writes under other sections' entries. Production change: about 65 lines in `src/app/Settings.tsx`. Docs: one sentence each in `docs/navigation-history-behavior.md` and `docs/buzz-help.md`. ## Before and after Same steps on both builds, Chromium, local fixture data. Left: `main` @ `3bcf1c4bb`. Right: this branch @ `35349385c`. At step 3 (about 0:20), ⌘[ on `main` lands on the list with nothing selected; on the branch GitHub's details reopen with focus on Close. Steps 5 and 6 show the unchanged behaviour: Close is kept across Forward/Back, and a sidebar visit starts at the list. https://github.com/user-attachments/assets/23493f50-69e7-44a6-89e1-070f337904f9  ## Tests - `src/app/Settings.test.tsx`: 11 new cases. **Fail-then-pass:** with only the test changes on `main`, 6 fail (the Back restore at `toBeVisible`, both addressed-return cases, and the three import-path cases); the other 5 guard existing behaviour (inherited details on an adjacent visit, Close of addressed details landing on the list, Open in Buzz). All 65 pass on the branch. - Fixture: one navigation request object per visit (as App's `route.request`), with `entryId`, `signal`, `position`/`savePosition`, and `pluginId`/`pluginImport` in the target. A `useSyncExternalStore` host covers the dismissal that lands synchronously (replace) versus later (`history.go()`). - Mutation check: reverting each behavioural change fails at least one named test. The exception is the redundant flag reset when leaving Plugins, which is unobservable and kept as belt and braces. - Suites: `Settings.test.tsx` + `App.profile.test.tsx` (89), App-level navigation and settings suites, `SettingsSidebar`, `DecisionsSettings`, `PluginImport`, `src/features/navigation` (19 files, 480). Biome, `tsc --noEmit`, `git diff --check` clean. - No browser cases added or removed. The recording above is a harness flow (fixture relay, Playwright), not a repo journey. ## Review Two independent agent review rounds. Round 1 found the adjacent-visit and addressed-visit cases; round 2 reproduced the two import-path cases and the stray writes. All were fixed with fail-before tests. ## Manual test 1. Settings → Manage plugins → View GitHub details → click **GitHub** in the "Settings: GitHub" line. Press ⌘[. GitHub's details should be open, the row expanded, focus on Close plugin details. ⌘] then ⌘[ should do it again. 2. Close the details. ⌘] then ⌘[: the list, no details. 3. Open any details, click Appearance, then Manage plugins in the sidebar: the list. 4. With Settings on Manage plugins showing details, trigger a plugin-update notice and open its details from the toast, then ⌘[: the earlier details should be back. ## Deferred - WebKit and the desktop app (WKWebView) weren't re-run on the final commits; the change is state logic, not layout. - Branch base is `3bcf1c4bb`; `main` has moved 3 commits (#2587, #2718, #2682), none touching these files. - #2621 (draft) uses the changed effect-dependency line as context and will need a small hand rebase. 🤖 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>
Problem
In Settings → Manage plugins, a plugin's details pane had no way to reach that plugin's own settings. To change who can wake agents from GitHub comments, for example, you had to know the rules live in a GitHub comment authors section and find it in the sidebar yourself.
What the pane shows now
Under the Enable/Disable button, the pane lists the Settings sections that hold the plugin's settings, for example Settings: GitHub comment authors. Each name links to its section. Clicking it or pressing Enter opens the section and moves focus into it.
openSectionhelper that opens a section and moves focus into it.docs/buzz-help.mdmentions the links.Opening a linked section at its start
In a narrow window, Settings stacks the sidebar above the content in one scroll container. A section opened from a link inside the content kept that container's offset, and the focus step uses
preventScroll. So after scrolling down to the link, the section opened partway down with its heading out of view (423 px in, at 480×800 with 200% text).The focus step now scrolls the opened section to its start, but only after a link inside the content, and only when a scroll container around it has scrolled past that start. A sidebar click, Back or other route change never triggers it. The scroll is instant, so reduced motion is unaffected. The same fix applies at full width to the Decisions API hints that open Manage plugins, where the plugin list could open scrolled with its focused start out of view.
Before and after
media-before-after.mp4
Left:
mainat 0b2dfbf. Right: this branch at d4c6ad5. Both sides play the same steps in sync, in the browser fixture build at 1280×800:The pink outline marks the control about to be clicked, then the element that has focus once the section opens. The later commits only change narrow-window scrolling, which these steps don't hit.
Dark theme, with keyboard focus on the link:
480 px wide, where the pane covers the plugin list:
Checks
102 Settings Vitest tests and 17 Settings browser tests (Chromium) pass, and
pnpm lintis clean. Typecheck anddesign:checkwere clean at af551cd; b06c71f only adds a test. Not run: the desktop app and WebKit.Not in this PR
🤖 Generated with Claude Code