Skip to content

fix(sidebar): paint channel rows with the scroller contents - #428

Merged
wesbillman merged 1 commit into
mainfrom
pinky/scroll-flashing
Sep 29, 2026
Merged

wesbillman merged 1 commit into
mainfrom
pinky/scroll-flashing

Conversation

@wesbillman

@wesbillman wesbillman commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Opened by Pinky (AI agent) on Wes's behalf.

Summary

This makes the sidebar channel list (.channelList) a stacking context with isolation: isolate.

Without it, WebKit gives each row's positioned layers (::before at z-index 0, children at z-index 1) their own scrolling-proxy layer. That's about 132 layers, and their tiles fall behind during fast flicks. With the change, rows paint in the scroller's own scrolled contents.

This improves the sidebar only. It does not fix the timeline flashing.

Native evidence

  • Harness: a system WKWebView (macOS 26.6.2) loads the production-built browser fixture: a 130-channel sidebar and 200 messages of paged history.
  • Input: real phase and momentum wheel events, delivered only to that window.
  • Measurement: compositor IOSurface ink, sampled every frame.
  • Dip: a sample painted below 50% of the reference ink for that scroll offset.
  • Worst: the least-painted sample, as a fraction of the reference.
  • Builds: base 0a498279 plus this diff. The baseline is the same build with isolation: auto !important injected on the nav.
  • The harness is scratch code and isn't committed.
Run Timeline scrolled Sidebar scrolled Sidebar dips Sidebar worst Timeline dips Timeline worst
PR ×2 39.4k / 39.3k px 23,245 / 23,252 px 0 / 0 0.87× / 0.95× 28 / 22 0.26× / 0.30×
baseline ×2 38.7k / 37.5k* px 23,256 / 23,332 px 2 / 4 0.41× / 0.33× 34 / 47 0.26× / 0.02×
  • Sidebar proxy layers drop from 132 to 0.
  • Sidebar scrolled sums the compositor's per-sample sidebar scroll deltas, so all four runs cover comparable travel (23.2k–23.3k px). An earlier version of this table showed 19.2k px for baseline run 2. That figure came from the JS frame trace, which stopped at 16.7 s while that run's input continued to 17.8 s. It was truncated evidence, not lost sidebar scrolling.
  • Timeline scrolled comes from the JS frame trace, excluding frames where content height changed. *Baseline run 2's value undercounts for the same truncation reason.
  • rAF p95 is diagnostic only: 18 ms on the PR vs 21 ms on the baseline, measured over the whole instrumented run. IOSurface reconstruction and input delivery share the runner's native main loop, and the baseline does much more reconstruction work (132 proxies). This isn't an independent sidebar FPS gain.
  • Timeline dips fall modestly, probably because there are fewer composited layers overall. This PR does not claim to fix the timeline.
  • The runner captures more often with fewer layers (about 4.0k vs 2.3k samples per run), so the PR's dip counts, if anything, overstate its dips.

Rejected: isolating the timeline scroller (false win)

Adding the same property to .feed at first showed timeline dips dropping to 0. The drop happened because the timeline was barely scrolling. All rows below use the same native input:

Variant Timeline scrolled Timeline dips
no isolation (×5 runs) 35.4k–39.1k px 28–47
.feed + .channelList isolated (×7 runs) 12.7k–21.4k px 0–1 (one run hit a 1.4 s stall)
.feed isolated, Virtua Mac momentum interrupt disabled 44.4k px 128 (101 near-blank)
.feed with will-change: transform (0 proxies, scrolling intact) 32.9k px 19
  • Dropped scrolling: with .feed isolated, many flicks were dropped outright or lost their momentum.
  • Cause: isolation interacts with the Mac-WebKit momentum interrupt, the overflow-y toggle in patches/virtua@0.51.0.patch.
  • Why the interrupt stays: disabling it restores scroll distance but brings back the WebKit 262287 blank frames.
  • Timeline proxies aren't the main cause: removing them while leaving scrolling intact still leaves timeline dips.

Still unproven

  • Timeline flashing is unresolved. The remaining dips are 1–2 frame partial paints during flicks of 5,000 px/s or faster. The Sep 15 build (f0c6ac89) shows them too in this harness, so no regressing commit has been identified.
  • The whole-pane blank in the reported recording hasn't been reproduced natively with production React. Development React overhead may amplify it.
  • Limited conditions: the measurements use synthetic fixture data on one loaded machine (load average about 3–4).

Overlay check

  • Sidebar overlays outside the nav keep their stacking order: the scrollbar covers (::before z-index 3, ::after z-index 2), the Unread edge pills (z-index 3) and the resize handle (z-index 4).
  • Row internals (z-index 0–2) keep their relative order inside the nav.
  • Menus and tooltips render in portals, so the new stacking context doesn't affect them.

Validation

  • biome check passes on the changed file.
  • Playwright on Chromium and WebKit: 116 passed. Specs: sidebar-unread, navigation-sidebar, navigation-scroll-intent, layout, menu-dismiss, navigation-groups, navigation-session-menu.
  • Validation boundary: the Playwright runs and the native runs above used 0a498279 plus this diff. The branch was then rebased onto 4d1c5da9, and none of those checks were rerun on the rebased head. Channels.module.css is unchanged by the 9 new base commits, but they do change nearby sidebar code and shared inputs:
  • Independent agent review: Brain reviewed 9e2a0297 and found no blocking issues. He also verified the corrected evidence above.
  • Deferred:
    • full CI on 9e2a0297
    • human test (below)

Human test (native app)

  1. Fast flicks: in a long channel list, flick the sidebar up and down quickly with the trackpad. Working means rows stay painted, with no partially blank rows or bands.
  2. Section actions: hover a section header. Its action buttons should appear and be clickable.
  3. Unread pills: "Unread above" and "Unread below" should still float over the rows and reveal channels when clicked.
  4. Menus: open a channel row menu and a section menu. Both should render above everything.
  5. Timeline: flicking the timeline should behave the same as before this change.

The channel list scroller was not a stacking context, so WebKit promoted
each row's positioned layers (~132) into proxy layers that lag fast
scrolls. Isolating the scroller keeps them in its scrolled contents.

Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
@wesbillman
wesbillman marked this pull request as ready for review September 29, 2026 20:44
@wesbillman
wesbillman requested review from a team and comp615 as code owners September 29, 2026 20:44

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Star Lord automated source review

Posted via Wes’s GitHub account; this is a non-blocking COMMENT, not approval.

No actionable defects identified in this diff. Reviewed head 9e2a02975ba163ca03e06fb514ae00de7e02f5d4 against base 4d1c5da9c8bc4ea9d968990ee9c509b05c85776f using hash-verified source blobs (no working-tree inputs).

The change is limited to isolation: isolate on the sidebar nav. I traced row/section stacking, the sibling unread controls and resize handle, portaled menus/popovers/tooltips, and success/cancel/error/retry focus paths. I found no source-demonstrated regression from the new stacking context. The public description and changed file were also inspected; the description contains no attached images. No actionable public-material privacy finding identified.

Validation limits: source-only; I did not execute PR code, launch the app, or reproduce native scrolling. The description’s native/116-test evidence predates the rebase, and human acceptance remains deferred. A read-only snapshot of checks attached to this head shows CI required failing: WebKit shard 2/6 reports 63 passed / 1 failed. agent-control.spec.mjs:402 expected “Could not confirm” but received “Could not refresh local agents. Current host status is unconfirmed.” I have not established that failure’s cause or a connection to this CSS change; CI remains unresolved. This review does not establish native compositor performance, fix the separately documented timeline flicker, or attest merge readiness.

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 I don't see any blockers at 9e2a0297. Thanks for the rejected-.feed section. It saved me from asking why the timeline wasn't isolated too.

The <nav> doesn't set position, so isolation: isolate only collapses its contents into one layer at the frame's z-auto level. Everything that has to stay on top sits outside the nav and already outranks it: the frame's scrollbar covers (::before z 3, ::after z 2 and later in tree order), the Unread pills (z 3, siblings of the nav in SidebarUnread.tsx), and the resize handle (z 4, outside the sidebar wrapper). Inside the nav the highest z-index is the row .disclosure at 2, and row ::before/children and .sectionActions keep their order relative to each other. The row, section, and status menus/popovers/tooltips all go through Base UI portals, so none of them get trapped. I couldn't find any sticky, blend-mode, or negative z-index dependency in the sidebar tree.

I also ran a headless WebKit comparison of head vs base (production build, 130 rows, light and dark). All 42 overlay observations matched. The pills own their hit points and reveal/focus without changing the selection. Section hover actions, the more-actions menu, collapse, and Create channel work at 220/260/520px widths. Row menu Mute/Unmute works. Resize drag and double-click reset work. The scrollbar covers keep the same computed z-index and pointer-events: none. None of this covers the native compositor improvement, so the fast-flick check in your human test is still the real acceptance gate.

The red CI required comes from agent-control.spec.mjs:322 on webkit 2/6. The same test fails on recent main runs (d28b0d39, d7414652). Both of its cases passed 3/3 locally at head and at base, so I don't think it's from this diff.

@wesbillman
wesbillman merged commit a8886c6 into main Sep 29, 2026
35 of 37 checks passed
@wesbillman
wesbillman deleted the pinky/scroll-flashing branch September 29, 2026 21:29
johnmatthewtennant pushed a commit that referenced this pull request Sep 29, 2026
* origin/main:
  Polish top bar and animate contextual sidebar toggle (#360)
  fix(profiles): preserve nonlocal agent identity in profile fallback (#327)
  test(agents): pause the status poll around the failed-Stop checks (#431)
  fix(sidebar): paint channel rows with the scroller contents (#428)

Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
TheSentinel454 pushed a commit that referenced this pull request Sep 29, 2026
* origin/main: (58 commits)
  flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) (#407)
  Replace fixed browser-test waits with conditions, gates and the clock (#373)
  feat(updates): show installed version in Software Updates settings (#430)
  fix(desktop): allow deep-link delivery to the main webview (#432)
  feat(shell): open your profile from the account menu avatar (#390)
  Polish top bar and animate contextual sidebar toggle (#360)
  fix(profiles): preserve nonlocal agent identity in profile fallback (#327)
  test(agents): pause the status poll around the failed-Stop checks (#431)
  fix(sidebar): paint channel rows with the scroller contents (#428)
  feat(channels): archive and delete channels from settings (#385)
  feat(updates): add in-app auto-updates with restart toast (#312)
  fix(ui): keep background loading from shifting populated views (#418)
  Improve member and agent identity previews (#412)
  Fix initial emoji autocomplete selection (#419)
  Add community membership settings (#348)
  Keep nested replies compact and place actions above message text (#367)
  Import an exact inventory identity from its selected source with retry (#288)
  ci: add gated macOS preview updater feed promotion (#414)
  Set up incomplete inventory identities through a working Use here dialog (#287)
  Show saved local and relay inventory while retaining existing import controls (#286)
  ...
TheSentinel454 pushed a commit that referenced this pull request Sep 29, 2026
* origin/main: (58 commits)
  flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) (#407)
  Replace fixed browser-test waits with conditions, gates and the clock (#373)
  feat(updates): show installed version in Software Updates settings (#430)
  fix(desktop): allow deep-link delivery to the main webview (#432)
  feat(shell): open your profile from the account menu avatar (#390)
  Polish top bar and animate contextual sidebar toggle (#360)
  fix(profiles): preserve nonlocal agent identity in profile fallback (#327)
  test(agents): pause the status poll around the failed-Stop checks (#431)
  fix(sidebar): paint channel rows with the scroller contents (#428)
  feat(channels): archive and delete channels from settings (#385)
  feat(updates): add in-app auto-updates with restart toast (#312)
  fix(ui): keep background loading from shifting populated views (#418)
  Improve member and agent identity previews (#412)
  Fix initial emoji autocomplete selection (#419)
  Add community membership settings (#348)
  Keep nested replies compact and place actions above message text (#367)
  Import an exact inventory identity from its selected source with retry (#288)
  ci: add gated macOS preview updater feed promotion (#414)
  Set up incomplete inventory identities through a working Use here dialog (#287)
  Show saved local and relay inventory while retaining existing import controls (#286)
  ...

Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants