fix(messages): reserve a stable scrollbar gutter on the channel feed - #451
Merged
Merged
Conversation
The vendored Virtua patch's interruptMomentum() sets the feed to overflow-y: hidden !important on every nonzero size correction on Mac WebKit. With classic, space-taking scrollbars that frame removes the scrollbar, widens the content box and re-wraps rows above the viewport, whose new heights produce the next correction, so the feed oscillates between two wrap widths. scrollbar-gutter: stable keeps the feed's inline size constant across the toggle and opens the ring, while the momentum interrupt itself stays because PR #428 measured that disabling it brings back the WebKit 262287 blank frames. The dead .feed::-webkit-scrollbar* rules are removed: with scrollbar-width and scrollbar-color set both engines ignore them, and if they ever became live again the gutter would reserve the classic width instead of the thin one. .threadHistory deliberately does not get the gutter. It has no Virtualizer and no overflow toggle, so it cannot loop, and reserving a gutter there would give short threads a right inset in legacy scrollbar mode as a design change rather than a bug fix. Its ::-webkit-scrollbar rules are left as they were. tests/browser/timeline-scrollbar-gutter.spec.mjs drops Chromium's --hide-scrollbars so platform scrollbars take space, detaches with upper(), applies the patch's exact inline toggle and restore, and asserts clientWidth and a fully visible paragraph width are equal in all three states. It skips where the scrollbar takes no space (Playwright WebKit on macOS uses overlay scrollbars) and guards the computed scrollbar-gutter in both projects. Run against the unchanged CSS it failed on Chromium (hidden state clientWidth 1106 vs 1095, paragraph 1006 vs 995) and the CSS guard failed in both engines; with the change it passes on Chromium and the width case skips on WebKit as intended. No existing browser spec asserting absolute widths or x positions broke from the 11 px gutter headless Chromium now reserves, so none changed. The standalone WKWebView probe under /tmp/wkprobe (patched Virtua bundle, 400 prose rows, AppKit reporting legacy scrollers) was re-run against the final .feed declarations. overflow-y hidden toggles seen by a MutationObserver on the scroller's style attribute: - at bottom, idle, 1 s: 40 without gutter, 0 with - after scrollBy(-700), detached, 2 s: 80 without, 0 with - after a +1 px content change on a row above the viewport, 3 s: 120 without, 1 with - total: 248 without gutter, 3 with Suites run (Hermit bin/pnpm): - bin/pnpm check: passed (biome, typecheck, design typecheck, design checks) - vitest virtua-compensation.test.mjs, ChannelTimeline.test.tsx, ChannelTimeline.restore.test.tsx, ChannelTimeline.report.test.tsx: 4 files, 129 tests passed - Playwright --project=chromium --project=webkit --no-deps: messages, layout, message-actions, message-actions-floating, message-navigation, navigation-thread-history, history-loading, image-scroll, initial-position, sidenav-polish, design-system, timeline-scrollbar-gutter: 165 passed, 1 skipped (the WebKit width case) - Playwright --project=chromium-measurements --project=webkit-measurements --no-deps --workers=1: scroll.spec.mjs, channel-opening.spec.mjs: 12 passed Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…th invariant Follow-up to 386a0746 addressing its review. patches/README.md now records in the momentum-interrupt section that the overflow-y: hidden frame removes a classic scrollbar, so every scroller the patched Virtualizer drives on Mac WebKit must keep a constant inline size across the toggle (scrollbar-gutter: stable, as .feed does), or re-wrapped rows feed new corrections back into the interrupt. The .feed comment's precondition is corrected: the global * rule in scrollbars.css also sets scrollbar-width/scrollbar-color, and what keeps ::-webkit-scrollbar dead is engine support for the standard properties (Chromium 121+, WebKit from Safari 18.2), not the declarations staying on this rule. tests/browser/timeline-scrollbar-gutter.spec.mjs reads offsetWidth minus clientWidth and decides the overlay-scrollbar skip right after open(), before paying for the wheel-gesture detach; selects the measured paragraph once and reuses that node for the before, hidden and restored reads so a re-wrap cannot swap the compared element; merges the config's launchOptions through a fixture function instead of replacing them; and its header states that Chromium never runs interruptMomentum (the MacIntel/Apple predicate is false), so the spec proves the CSS invariant by replaying the patch's exact inline toggle rather than the loop, and WebKit only runs the toHaveCSS guard. .threadHistory and its scrollbar rules are unchanged. Suites run (Hermit bin/pnpm): - bin/pnpm check: passed (biome, typecheck, design typecheck, design checks) - vitest virtua-compensation.test.mjs, ChannelTimeline.test.tsx, ChannelTimeline.restore.test.tsx, ChannelTimeline.report.test.tsx: 4 files, 129 tests passed - Playwright --project=chromium --project=webkit --no-deps timeline-scrollbar-gutter.spec.mjs: 3 passed, 1 skipped (the WebKit width case, now skipped before the detach). The Chromium width case passing with a nonzero scrollbar width confirms the merged launchOptions dropped --hide-scrollbars. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
reviewed
Sep 30, 2026
wesbillman
left a comment
Collaborator
There was a problem hiding this comment.
No source-level changes requested; the stable gutter addresses the overflow-toggle width feedback without changing Virtua’s momentum policy.
Star Lord’s automated source review via Wes’s account — head b68a91a1328e6bff4f51d19be55c1c7555eb4209, base 329fe2a1de0ad7f9aa8eebbc02e1a957bcafa25d.
Source-only: no tests/app execution; the author’s native probe was not independently reproduced, and legacy-scrollbar Mac acceptance remains unverified.
The CI snapshot is not green: WebKit panel-resize bottom-follow and Chromium image-strip focus-visibility tests failed, other jobs were still running, and causality is not established.
kalvinnchau
reviewed
Sep 30, 2026
kalvinnchau
left a comment
Contributor
There was a problem hiding this comment.
🤖 Two actionable issues below: a required image-strip journey fails at the narrower feed width, and the new geometry regression can silently skip its behavioral assertion. The Linux failure is corroborated by width emulation, not a local native-Linux reproduction.
Tabbing through a posted image strip relies on native focus scrolling, and both engines refuse to scroll horizontally when the newly focused element is already partly visible. Blink sets the partial-visibility behaviour to no-scroll in Element::UpdateSelectionOnFocus, and WebKit keeps its 32px legacy horizontal visibility threshold on the focus reveal path; only scrollIntoView() opts out of either rule. A thumbnail whose far edge is clipped therefore takes focus without ever coming fully into view, and its focus ring is cut off. On the base commit this already happens for every strip width from 330 to 390px. The feed's scrollbar-gutter narrows the strip by 10px (Linux) or 11px (legacy-scrollbar macOS) under headless Chromium, which moved image-strip.spec.mjs's fixed 768px viewport into that window and made the Chromium shard of PR #451 fail at the last-tile visibility check (review comment P2). MessageRow.tsx now handles onFocus on the strip group (React's onFocus is the bubbling focusin, so one handler covers every tile) and nudges only the strip's own scrollLeft until the focused tile lies inside the strip's scroll-padding box. In Blink the native reveal runs first and the handler corrects the clipped case; in WebKit the handler runs first and the timer-driven native reveal then finds a fully visible element and does nothing. A strip-local adjustment is used instead of scrollIntoView so that overflow: hidden ancestors such as the thread container are never scrolled. Inline start and end are treated as left and right, matching the app's LTR layout. The feed's scrollbar-gutter CSS is unchanged. tests/browser/image-strip.spec.mjs now asserts the focused last tile lies inside the strip's scroll-padding box rather than the parent's bounding rect. This also catches the base commit's 1px short-of-max state at 398px, where the tile sat inside the border box but past the scroll-padding end with its ring clipped. Where the strip is too narrow to hold a tile plus both paddings (the 390px viewport once a gutter is reserved) the tile's far edge must sit exactly on the scroll-padding end. No assertion was loosened. Suites run (Hermit bin/pnpm): - biome check --error-on-warnings MessageRow.tsx image-strip.spec.mjs: clean - typecheck: clean - vitest src/features/messages/MessageRow.test.tsx: 71 passed - Playwright --project=chromium --project=webkit --no-deps image-strip.spec.mjs on macOS with legacy scrollbars (Chromium reserves an 11px gutter and reproduced the CI failure before the fix): 2 passed - Playwright --project=chromium --project=webkit --no-deps gifs, image-scroll, image-strip, message-management, messages, startup: 64 passed - Linux container (mcr.microsoft.com/playwright:v1.63.0-noble, CI config) --project=chromium and --project=webkit --no-deps image-strip.spec.mjs: 1 passed each. Swapping the unpatched MessageRow.tsx back into the container fails the Chromium run at the same assertion (now line 131, previously 113), so the CI failure is gone rather than moved. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… project Resolves review comment P3 on PR #451. The scrollbar-gutter width case skipped silently wherever the scrollbar took no space, which is every Chromium run on an overlay-scrollbar Mac and every WebKit run, so a green sweep could not distinguish "the gutter holds" from "nothing was measured". The classic-scrollbar premise now fails loudly where it is guaranteed. tests/browser/timeline-scrollbar-gutter.spec.mjs tags the width case @classic-scrollbars, drops the launchOptions fixture override and the space === 0 skip, records the measured scrollbar width in the fixture's evidence and asserts it is greater than zero with a message naming the premise. The case is gated by platform rather than by measurement: unconditional on Linux, opt-in through BUZZ_CLASSIC_SCROLLBARS elsewhere, otherwise an explicit skip that says why. The toHaveCSS guard stays untagged so the chromium and webkit projects still run it. tests/browser/playwright.config.mjs adds the chromium-classic-scrollbars project: Chromium launched with ignoreDefaultArgs ["--hide-scrollbars"], grep on the tag, the measurement files ignored, and the inverse grep on the chromium and webkit projects so the tagged case never runs where classic scrollbars are not guaranteed. Two details go beyond the design note. The project depends on webkit-measurements like the engine projects do, because the local default gate runs every project and a dependency-free project would share phase 0 with chromium-measurements under two workers; CI passes --no-deps, so it is unaffected. The project also has its own outputDir, test-results/browser-classic-scrollbars: Playwright clears the outputDir of every project a run selects, so a second invocation sharing test-results/browser would delete the measurements' ci-report.json, ci-timing.json and per-test evidence.json before the artifact upload. Probe files confirmed this: a file under test-results/browser survives the classic run, a file under the classic outputDir does not. .github/workflows/ci.yml runs the project as a second step of the measurements job, after the serial measurements, through scripts/ci-test-report.mjs with its own report and evidence paths, and the Measurement evidence artifact uploads both directories. CI required already needs the measurements job, so the case blocks merges without a new runner. It is not added to the six-way engine matrix: one file across --shard=N/6 leaves five shards with no tests, which Playwright treats as an error. The '*-measurements' glob does not match the new project name, so the existing step is unchanged, and zizmor reports the same 8 findings (4 suppressed, 4 low) before and after. tests/integration/browser-ci.test.mjs gains a guard alongside the shard-coverage test: the step exists after the serial measurements, reports through ci-test-report.mjs into a directory disjoint from the measurements' outputDir, both directories are uploaded, the project's --list selects only tagged cases in that project, and the chromium and webkit discovery selects none of them. docs/browser-testing.md describes the project and the macOS opt-in. patches/README.md does not mention the spec's skip behaviour, so it is unchanged. Suites run (Hermit bin/pnpm): - bin/pnpm check: passed (biome, typecheck, design typecheck, design checks) - biome check --error-on-warnings on the spec, config and integration test: clean - Playwright --project chromium-classic-scrollbars --no-deps on this Mac (legacy scrollbars): with BUZZ_CLASSIC_SCROLLBARS=1, 1 passed with scrollbar space 11; without it, 1 skipped with the stated reason - Playwright --project=chromium --project=webkit --no-deps timeline-scrollbar-gutter.spec.mjs on this Mac: 2 passed; --list shows only the CSS guard in each engine project and only the tagged case in the classic project - Linux container (mcr.microsoft.com/playwright:v1.63.0-noble, CI config): --project chromium-classic-scrollbars --no-deps: 1 passed with scrollbar space 10; --project chromium and --project webkit --no-deps on the spec: 1 passed each; the exact workflow step command through ci-test-report.mjs exits 0 with a Complete summary and leaves a pre-seeded test-results/browser/ci-report.json intact - node --test tests/integration/browser-ci.test.mjs: 8 passed in the Linux container. On this Mac 7 pass and the untouched "browser Rust setup" case fails because bash resolves the temp dir through the /private symlink; that failure predates this change and does not occur on Linux. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
reviewed
Sep 30, 2026
wesbillman
left a comment
Collaborator
There was a problem hiding this comment.
No source-level changes requested on the focus-reveal and classic-scrollbar CI revisions.
Star Lord’s automated source review via Wes’s account — head 37fe84a654c7b34b992b7c1268e399f6ddfb7eb1, base 329fe2a1de0ad7f9aa8eebbc02e1a957bcafa25d.
Hosted measurement artifacts report 9/9 passes plus classic-scrollbar 1/1; full CI was unfinished at the status snapshot.
Source-only: no local tests or app execution; native Mac legacy-scrollbar momentum behavior and a before/after CI-cost comparison remain unverified.
kalvinnchau
reviewed
Sep 30, 2026
kalvinnchau
left a comment
Contributor
There was a problem hiding this comment.
🤖 The previous focus-visibility and silent-skip findings are addressed. One new pointer-focus regression remains below; its reproduction uses a standalone browser probe, not the candidate app.
Addresses the review of e4c38af and 37fe84a on PR #451. React's onFocus is the bubbling focusin, so revealFocusedThumbnail also ran for pointer focus, and Chromium focuses a link on mousedown (WebKit does not): pressing a clipped thumbnail on a Chromium host slid the strip under the held pointer before mouseup, so the release could land on a neighbour or on padding, and the strip visibly jumped on every such click. DESIGN.md wants pointer focus quiet, so MessageRow.tsx now gates the reveal on html[data-keyboard-navigation], the same modality fact MessageComposer.tsx and useFloatingActionBar.ts read. useKeyboardFocusVisibility sets that attribute from a capturing window keydown listener, which runs before Tab's default action moves focus, and clears it from the capturing pointerdown that precedes the mousedown focus, so a Tab reveal still runs and a press never does. The code comment records the pointer-focus rationale and that ordering. The relay-composer fixture already mounts the hook, so the spec's Tab path keeps revealing. tests/browser/image-strip.spec.mjs hoists the scroll-padding geometry into insideScrollPadding(el, edge) and adds two checks per viewport width. After the max-scroll assertion it Shift+Tabs back to the first tile (Shift+Alt+Tab on macOS WebKit, matching the forward Alt+Tab), asserting after every press that the focused tile's leading edge is at or past the scroll-padding start with the mirrored narrow-strip rule, which exercises the handler's left branch. It then presses a tile the strip's end clips with the mouse, holds for two frames, asserts scrollLeft is unchanged, releases, and closes the viewer the click opened. The width loop must press a clipped tile at least once. Negative runs confirm each assertion bites: with the handler disabled the forward pass fails at the last tile; with the left branch removed the backward pass fails on both Chromium and WebKit; with the gate removed the pointer check fails with the strip moved 13px on this Mac and 12px on Linux Chromium. tests/integration/browser-ci.test.mjs resolves the chromium-classic- scrollbars project through playwright.ci.config.mjs, which is what the workflow step runs, requires the local config's project to deep-equal it, and takes the measurements outputDir from the CI config too, so a CI-only override of the launch arguments or output directory cannot drift past the gate. docs/browser-testing.md states next to the test-results/browser paragraph that the classic project's evidence.json, failure screenshots and traces land in test-results/browser-classic-scrollbars/ and why. Suites run (Hermit bin/pnpm): - biome check --error-on-warnings MessageRow.tsx image-strip.spec.mjs browser-ci.test.mjs: clean - bin/pnpm check: passed (biome, typecheck, design typecheck, design checks) - vitest src/features/messages/MessageRow.test.tsx: 71 passed - Playwright --project=chromium --project=webkit --no-deps image-strip.spec.mjs on this Mac (legacy scrollbars): 2 passed - Linux container (mcr.microsoft.com/playwright:v1.63.0-noble, CI config) --project=chromium and --project=webkit --no-deps image-strip.spec.mjs: 1 passed each; the gate-removed MessageRow.tsx fails the Chromium run at the pointer check before the fixed file passes it - bin/node --test tests/integration/browser-ci.test.mjs: 8 passed in the Linux container. On this Mac 7 pass and the untouched "browser Rust setup" case fails on the /private temp-dir symlink, as before this change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
reviewed
Sep 30, 2026
wesbillman
left a comment
Collaborator
There was a problem hiding this comment.
No source-level changes requested on the keyboard-only thumbnail reveal, reverse-Tab/pointer regression checks, and CI-config follow-up.
Star Lord’s automated source review via Wes’s account — head e696436abd36510129d9408d3e3acaf228fd77a0, base 329fe2a1de0ad7f9aa8eebbc02e1a957bcafa25d.
The hosted CI snapshot has all required automatic checks passing, including the classic-scrollbar case.
Source-only: no local tests or app execution; native Mac legacy-scrollbar acceptance and a controlled before/after CI-cost comparison remain unverified.
wesbillman
approved these changes
Sep 30, 2026
TheSentinel454
pushed a commit
that referenced
this pull request
Sep 30, 2026
* origin/main: (27 commits) Let plugin pages publish NIP-AR artifacts and embed the host thread view (#434) test(app): migrate entity-navigation test off removed buzz://open locator API (#463) Show agent activity in navigation (#423) test(browser): hold motion when it commits, not on its start event (#459) fix(navigation): ignore unknown query parameters on Buzz links and remove the buzz://open locator (#457) feat(design-system): distinguish controls on floating surfaces (#429) feat(native): add community extras and media preparation (#450) Clone inventory identities through reviewed text and fresh identity creation (#289) feat(communities): add right-click actions to the community rail (#400) fix(messages): keep a send reveal pending until its scroll runs (#454) fix(messages): reserve a stable scrollbar gutter on the channel feed (#451) fix(sidebar): list plugin pages as sidebar rows via an opt-in primary flag (#401) feat(channels): surface canvas content in channel settings (#426) fix(profiles): remove redundant presence status row (#394) test(browser): count live retries once the page handles startup controls (#443) feat(composer): host-owned resource links for the Projects picker (#445) feat: support native read state and recent channel activity (#444) feat(native): serve relay media and uploads in packaged builds (#433) feat(channels): suggest joined channels in the composer (#446) feat: support native agent activity, library, memories, and community resolution (#441) ... Signed-off-by: Codex <noreply@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On Mac WebKit with classic (space-taking) scrollbars, the channel feed could jitter between two wrap widths. The vendored Virtua patch's
interruptMomentum()sets the feed tooverflow-y: hidden !importanton every nonzero size correction. That removes the scrollbar for a frame, which widens the content box and re-wraps rows above the viewport. Their new heights then trigger the next correction, and the cycle keeps going..feednow setsscrollbar-gutter: stable, so its inline size stays the same when overflow is toggled. The momentum interrupt stays, because fix(sidebar): paint channel rows with the scroller contents #428 measured that disabling it brings back the WebKit 262287 blank frames..feed::-webkit-scrollbar*rules. Both engines ignore them becausescrollbar-width/scrollbar-colorare set..threadHistoryis unchanged: it has no Virtualizer and no overflow toggle, so it can't loop.patches/README.mdrecords the rule that every scroller the patched Virtualizer drives on Mac WebKit must keep a constant inline size across the toggle.tests/browser/timeline-scrollbar-gutter.spec.mjs. It applies the patch's exact inline toggle and restore, asserts thatclientWidthand a visible paragraph's width match in all three states, and checks the computedscrollbar-gutterin both engines. Against the old CSS the width case failed on Chromium (hidden-stateclientWidth1106 vs 1095).Review follow-ups
--hide-scrollbars(10 px on Linux CI, 11 px on a legacy-scrollbar Mac). At 768 px that narrowed the image strip into a range where a pre-existing defect shows: Blink and WebKit both skip horizontal focus scrolling when the newly focused element is already partly visible, so Tab could leave the last thumbnail clipped.MessageRow.tsxnow reveals the focused tile within the strip's own scroll padding, only whilehtml[data-keyboard-navigation]is set so pointer focus stays quiet per DESIGN.md.image-strip.spec.mjsasserts against the scroll-padding box in both Tab directions and checks that a mouse press on a clipped tile does not move the strip. The under-scroll reproduces on the base commit for strip widths 330 to 390 px.@classic-scrollbarsand runs in a newchromium-classic-scrollbarsproject that drops--hide-scrollbarsand asserts the scrollbar takes space. It runs unconditionally on Linux, opts in on macOS viaBUZZ_CLASSIC_SCROLLBARS=1, and otherwise skips with a stated reason. CI runs it as a second step of the measurements job with its own output directory, so the required job blocks on it.Verification
overflow-y: hiddentoggles: 248 without the gutter, 3 with it.bin/pnpm checkpassed after every commit.MessageRowtests: 71 passed.scroll,channel-opening): 12 passed.mcr.microsoft.com/playwright:v1.63.0-noble:image-strip.spec.mjsfails at the CI assertion with the oldMessageRow.tsxand passes with the fix; the classic-scrollbar project passes with 10 px of scrollbar space; the exact new CI step exits 0 with the measurements report intact.layout.spec.mjs:398failure on the first CI attempt is a pre-existing load-sensitive flake, not related to this change: Linux WebKit reserves 0 px for the gutter, and the test passed 28 of 28 runs without the gutter and 19 of 20 with it under host load.🤖 Generated with Claude Code