Repository navigation
Fix multi-second mobile tap freeze from sidebar swipe layout probe - #1281
Conversation
…1269) The mobile sidebar swipe recognizer ran isInsideHorizontalScrollRegion on every document-level pointerdown/touchstart. Each ancestor probe pairs getComputedStyle with a scrollWidth read, which forces a synchronous style+layout pass of the whole document. On a large streamed thread that flush blocks the main thread for seconds per tap. The path became hot in #1026, which removed the data-no-sidebar-swipe opt-out on message prose. Defer the probe to the first touchmove that shows horizontal intent, so it runs at most once per real swipe gesture and never on a tap. The wheel path keeps the probe but only after its direction test, so vertical trackpad scrolls stop paying it. Gesture semantics are unchanged: a swipe that starts inside a horizontal scroll region still yields to native scroll. Measured on a 7.5k-node DOM with dirty styles: tap dispatch went from ~75ms median (110ms max) to ~0ms on a desktop CPU. Fixes #1269 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request for security, code quality, performance, architecture, and product behavior. I will post one final review after the parallel checks finish. |
|
|
||
| if ( | ||
| session.startTarget !== null && | ||
| isInsideHorizontalScrollRegion(session.startTarget) |
There was a problem hiding this comment.
🚨 slopcop/review — Cancel the swipe if startTarget is no longer connected before this deferred probe.
A live timeline update can replace the target between the gesture start and the first horizontal move. Chromium then returns an empty overflowX value for the removed element. The probe fails open, the sidebar opens, and native horizontal scroll stops.
I reproduced this in the mobile app. I removed a CSS-class scroll region after touchstart. The probe made zero scrollWidth reads, and the sidebar opened on touchmove.
Please check !session.startTarget.isConnected, clear the session, and return before preventDefault. Add touch and pointer tests that replace the scroll region before horizontal intent.
There was a problem hiding this comment.
Fixed in a1615d5 — the deferred probe now cancels the swipe when the start target is no longer connected, with a regression test covering the detach case.
|
|
||
| fireTouch(window, "touchmove", createTouch(260, 164)); | ||
|
|
||
| expect(scrollWidthReads).toBeGreaterThan(0); |
There was a problem hiding this comment.
🚨 slopcop/review — Require exactly one deferred probe.
toBeGreaterThan(0) accepts a regression that probes on every move. The intended touch and pointer limit is one probe per gesture. Please use toBe(1) and apply the same bound to pointer coverage.
There was a problem hiding this comment.
Fixed in a1615d5 — the touch and pointer tests both assert exactly one probe per gesture (toBe(1)), including a second move event to catch per-move probing.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5 summary
The app used to inspect page layout on every mobile tap. This change waits until a person swipes.
The main fix works. However, a live message update can remove the original scroll target during a swipe.
The app can then open the sidebar instead of letting the person scroll code.
Findings
-
P2 — Fix the removed-target race before merge. A live render can disconnect
startTargetbefore horizontal intent. Chromium then reports no computed overflow. The guard fails open and captures native horizontal scroll. I reproduced this in the mobile app. Fail closed when the target is not connected, then add touch and pointer regression tests. -
P3 — Make the probe-count test exact.
toBeGreaterThan(0)allows many layout probes. UsetoBe(1)to protect the one-probe limit.
Other review results
The security review found no issue. The architecture scan found no duplicate code that this pull request must refactor.
The wheel path can still probe once per horizontal wheel event. That cost existed before this change. This change removes the cost from vertical wheel events.
A later change can mark horizontal overflow with an observer-backed attribute. The sidebar could then use closest() without a layout read.
Validation
- The focused sidebar tests passed: 5 of 5.
- The
@bb/apptypecheck passed through Turbo. - A mobile Chromium test used a 390 by 844 viewport.
- Tap starts caused zero
scrollWidthreads. - A connected scroll region kept native horizontal scroll.
- Normal prose opened the sidebar.
- A removed scroll region reproduced the race.
I posted this as a comment-only review. I did not approve the pull request or request changes.
…tests A live timeline update can detach the gesture's start target before the deferred probe runs; a detached element reports empty computed style, so the probe would wrongly pass. Cancel the swipe instead. Tests now require exactly one probe per gesture, cover the pointer path, and cover the detached-target cancel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lection listeners (#1285) Follow-up #1 from the #1269 tap-freeze diagnosis (primary fix: #1281). Two re-render/tap amplifiers on the thread timeline. ## Root cause **1. Every realtime event re-rendered every timeline row.** `useSenderThreadMetadataById` rebuilt a fresh `Map` on every matching query-cache event with no equality check, and its predicate also fired on `observerResultsUpdated` (observer churn with no data change). The map fed `TimelineRendererStaticContext`, and context identity changes bypass `React.memo` — so every `sidebarNavigation`/`threads`/`thread` cache event re-rendered all rows of the un-virtualized timeline, keeping style/layout dirty and amplifying any main-thread work on mobile. **2. Per-tap handler work was O(N messages).** Each `SelectableMessageProse` (one per assistant message) registered 6 document-level listeners, so every tap and every `selectionchange` dispatched N sets of handlers and scheduled up to N rAF callbacks. ## Fix Commit 1 (`useSenderThreadMetadataById` + `ThreadTimelineRows`): - The hook keeps the previous map reference when the rebuild is value-equal (all metadata fields are primitives, so shallow per-entry compare is exact). - The sync predicate drops `observerResultsUpdated` — the map is built exclusively from `query.state.data`, which only changes via `updated` dispatches. - The map moves out of the wide static context into a dedicated `SenderThreadMetadataContext`, so a *genuine* metadata change (any thread title anywhere in the cache) re-renders only the conversation rows that resolve sender labels, not every row. Commit 2 (`SelectableMessageProse`): - One module-level registry owns a single shared set of 6 document listeners and one shared report frame. Each instance keeps its own selection state machine (the `hadSelection` null-once gate, pending/last-release anchors, multi-click timer) and its node-scoped `click`/`dblclick` listeners. An instance inside its multi-click delay is skipped by shared report passes and reports when its own timer fires, preserving double/triple-click semantics. ## Measurement React Profiler over a 21-row timeline in jsdom, 10 value-equal `threads` cache events (the shape realtime refetches produce): | | full-timeline commits | React update work | |---|---|---| | before | 11 | ~136 ms | | after | **0** | **0 ms** | Document listeners for N assistant messages: before 6·N, after 6 total. ## Validation - `pnpm exec turbo run typecheck --filter=@bb/app` — pass - `pnpm exec turbo run test --filter=@bb/app --force` — 328 files, 2472 tests, all pass (including the 51 pre-existing selection/timeline behavior tests, unchanged) - `pnpm exec turbo run lint --filter=@bb/app` — pass - New regression tests were each verified to fail against the pre-fix code: - `useSenderThreadMetadataById.test.tsx` — stable reference on equal rebuilds, new reference + value on real changes, stability across unrelated query keys - `ThreadTimelineRows.render-stability.test.tsx` — profiler-counted zero commits on equal cache events; sender title updates flow through to the DOM - `SelectableMessageProse.events.test.tsx` — mounting additional messages adds zero document listeners; a selection moving between two messages clears the old one exactly once Timeline virtualization remains explicitly out of scope. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…et-bb#1281) ## Summary - Defer the sidebar swipe recognizer's horizontal-scroll-region probe off the tap path. It now runs at most once per gesture, at the first `touchmove` that shows horizontal intent — never on `pointerdown`/`touchstart`. - Keep the probe on the wheel path, but only after the direction test, so vertical trackpad scrolls stop paying it. - Add regression tests: the tap path performs zero forced-layout reads, and a swipe that starts inside a horizontal scroll region still yields to native scroll. Fixes get-bb#1269 ## Root cause The mobile swipe recognizer listens for `pointerdown` and `touchstart` on `document` (capture). On every tap it called `isInsideHorizontalScrollRegion`, which pairs `getComputedStyle(...).overflowX` with a `scrollWidth`/`clientWidth` read for each ancestor. Each pair forces a synchronous style + layout pass of the whole document, and the handler runs for both events, so one tap paid the flush twice. On a large streamed thread (un-virtualized timeline, styles dirty almost every frame) that flush blocks a mobile main thread for seconds — which also freezes the running animations, exactly as reported. The path became hot in get-bb#1026, which replaced the blanket `data-no-sidebar-swipe` opt-out on message prose with runtime arbitration. After that, taps on prose — most of the mobile screen — fell through to the expensive walk. ## Performance Measured in Chromium at a 390×844 viewport against a 7.5k-node DOM inside `SidebarInset`, with styles dirtied before each tap (as a streamed thread does), 15 trials per variant on the same DOM: | Variant | Tap dispatch median | Tap dispatch max | |---|---|---| | Before | 75.3 ms | 110.7 ms | | After | ~0 ms | 0.5 ms | The pre-fix cost equals one full forced style+layout flush (~75–95 ms on a desktop CPU); phone CPUs are several times slower and real threads have far more nodes, which produces the multi-second freezes in the issue. ## Behavior verification In a real browser at a compact viewport: a right swipe over message prose still opens the sidebar; a swipe that starts inside a horizontally scrollable region still does not. ## Testing - `pnpm exec turbo run typecheck --filter=@bb/app` - `pnpm exec turbo run test --filter=@bb/app` — 326 files, 2,459 tests pass, including two new regression tests in `sidebar.test.tsx`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…lection listeners (get-bb#1285) Follow-up get-bb#1 from the get-bb#1269 tap-freeze diagnosis (primary fix: get-bb#1281). Two re-render/tap amplifiers on the thread timeline. ## Root cause **1. Every realtime event re-rendered every timeline row.** `useSenderThreadMetadataById` rebuilt a fresh `Map` on every matching query-cache event with no equality check, and its predicate also fired on `observerResultsUpdated` (observer churn with no data change). The map fed `TimelineRendererStaticContext`, and context identity changes bypass `React.memo` — so every `sidebarNavigation`/`threads`/`thread` cache event re-rendered all rows of the un-virtualized timeline, keeping style/layout dirty and amplifying any main-thread work on mobile. **2. Per-tap handler work was O(N messages).** Each `SelectableMessageProse` (one per assistant message) registered 6 document-level listeners, so every tap and every `selectionchange` dispatched N sets of handlers and scheduled up to N rAF callbacks. ## Fix Commit 1 (`useSenderThreadMetadataById` + `ThreadTimelineRows`): - The hook keeps the previous map reference when the rebuild is value-equal (all metadata fields are primitives, so shallow per-entry compare is exact). - The sync predicate drops `observerResultsUpdated` — the map is built exclusively from `query.state.data`, which only changes via `updated` dispatches. - The map moves out of the wide static context into a dedicated `SenderThreadMetadataContext`, so a *genuine* metadata change (any thread title anywhere in the cache) re-renders only the conversation rows that resolve sender labels, not every row. Commit 2 (`SelectableMessageProse`): - One module-level registry owns a single shared set of 6 document listeners and one shared report frame. Each instance keeps its own selection state machine (the `hadSelection` null-once gate, pending/last-release anchors, multi-click timer) and its node-scoped `click`/`dblclick` listeners. An instance inside its multi-click delay is skipped by shared report passes and reports when its own timer fires, preserving double/triple-click semantics. ## Measurement React Profiler over a 21-row timeline in jsdom, 10 value-equal `threads` cache events (the shape realtime refetches produce): | | full-timeline commits | React update work | |---|---|---| | before | 11 | ~136 ms | | after | **0** | **0 ms** | Document listeners for N assistant messages: before 6·N, after 6 total. ## Validation - `pnpm exec turbo run typecheck --filter=@bb/app` — pass - `pnpm exec turbo run test --filter=@bb/app --force` — 328 files, 2472 tests, all pass (including the 51 pre-existing selection/timeline behavior tests, unchanged) - `pnpm exec turbo run lint --filter=@bb/app` — pass - New regression tests were each verified to fail against the pre-fix code: - `useSenderThreadMetadataById.test.tsx` — stable reference on equal rebuilds, new reference + value on real changes, stability across unrelated query keys - `ThreadTimelineRows.render-stability.test.tsx` — profiler-counted zero commits on equal cache events; sender title updates flow through to the DOM - `SelectableMessageProse.events.test.tsx` — mounting additional messages adds zero document listeners; a selection moving between two messages clears the old one exactly once Timeline virtualization remains explicitly out of scope. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
touchmovethat shows horizontal intent — never onpointerdown/touchstart.Fixes #1269
Root cause
The mobile swipe recognizer listens for
pointerdownandtouchstartondocument(capture). On every tap it calledisInsideHorizontalScrollRegion, which pairsgetComputedStyle(...).overflowXwith ascrollWidth/clientWidthread for each ancestor. Each pair forces a synchronous style + layout pass of the whole document, and the handler runs for both events, so one tap paid the flush twice. On a large streamed thread (un-virtualized timeline, styles dirty almost every frame) that flush blocks a mobile main thread for seconds — which also freezes the running animations, exactly as reported.The path became hot in #1026, which replaced the blanket
data-no-sidebar-swipeopt-out on message prose with runtime arbitration. After that, taps on prose — most of the mobile screen — fell through to the expensive walk.Performance
Measured in Chromium at a 390×844 viewport against a 7.5k-node DOM inside
SidebarInset, with styles dirtied before each tap (as a streamed thread does), 15 trials per variant on the same DOM:The pre-fix cost equals one full forced style+layout flush (~75–95 ms on a desktop CPU); phone CPUs are several times slower and real threads have far more nodes, which produces the multi-second freezes in the issue.
Behavior verification
In a real browser at a compact viewport: a right swipe over message prose still opens the sidebar; a swipe that starts inside a horizontally scrollable region still does not.
Testing
pnpm exec turbo run typecheck --filter=@bb/apppnpm exec turbo run test --filter=@bb/app— 326 files, 2,459 tests pass, including two new regression tests insidebar.test.tsx.🤖 Generated with Claude Code