Repository navigation
Fix mobile sidebar swipe over timeline prose - #1026
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request under the configured security, code quality, performance, architecture, and end-to-end rules. Stack: #1026 only. I will post a comment after each review phase. I will not approve this pull request or request changes. |
|
🚨 SLOP COP 🚨 · Performance phase complete. No confirmed performance finding exists. The new global Checks: diff inspection, gesture hot-path review, listener lifecycle review, retained-reference review, and |
|
🚨 SLOP COP 🚨 · Security phase complete. No security finding exists. The selection check reads only DOM endpoints. It does not read or transmit selected text. Message HTML still passes through the existing sanitizer, and the document listeners have correct cleanup. Checks: diff inspection, pointer and touch flow review, sanitization boundary review, listener cleanup review, and unsafe HTML path search. |
|
🚨 SLOP COP 🚨 · Code quality and architecture phase complete. Medium finding: a late The new test covers selection before the first move. It does not cover The repository has other selection listeners, but they serve different policies. A shared refactor would not improve this change. |
|
🚨 SLOP COP 🚨 · End-to-end phase complete on the top stack state. I ran the dev app at SHA The browser also confirmed the medium finding. After The sidebar therefore stays partly open after the cancellation path. |
| selectionRoot !== undefined && | ||
| hasExpandedTextSelectionWithin(selectionRoot) | ||
| ) { | ||
| clearSwipeSession(); |
There was a problem hiding this comment.
🚨 slopcop/review — A late text selection can leave the mobile drawer partly open.
A qualified move already opens the drawer and applies drag styles. This call removes the move and end listeners, but it does not close the drawer, settle it, or clear those styles. The browser reproduced the sequence touchstart → qualified touchmove → expanded selection → selectionchange → touchend; the panel stayed at translate3d(-56.25%, 0px, 0px) with transition: none.
Please settle or reset an active drag before removing its end listeners. Add a regression test with this event order and verify the drawer closes and clears all drag styles.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Stack reviewed: #1026 only.
High finding: a late native text selection can leave the mobile sidebar partly open. A qualified swipe applies partial drag styles. The new cancellation path then removes the move and end listeners without closing or settling the drawer. The browser reproduced the full event order and left the panel at translate3d(-56.25%, 0px, 0px) with transition: none.
Please settle or reset an active drag before listener removal. Add a regression test with touchstart, a qualified touchmove, expanded selection, selectionchange, and touchend.
No security or performance finding exists. The repository scan found no useful shared refactor because the other selection listeners serve different policies.
Validation passed:
pnpm exec turbo run typecheck --filter=@bb/apppnpm exec turbo run test --filter=@bb/app --force: 312 files and 2,347 tests- All applicable GitHub checks
- Mobile-width dev-browser normal swipe and late-selection scenarios
- GPT-5.6 synthesis gate
This is a comment-only review under the configured rule.
…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 #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 #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>
## Summary - allow the compact sidebar swipe gesture to begin over selectable timeline message prose - cancel a pending sidebar swipe when native text selection appears within that message - preserve existing exclusions for controls and horizontal scroll regions - add regression coverage for swipe opening and long-press selection arbitration ## Root cause Selectable message prose was blanket-marked with `data-no-sidebar-swipe`, so most of the thread timeline was a gesture dead zone. This replaces the blanket opt-out with intent-based selection arbitration. ## Validation - `pnpm exec turbo run test --filter=@bb/app --force` (312 files, 2,347 tests) - `pnpm exec turbo run typecheck --filter=@bb/app` - mobile dev preview verified healthy through bb Connect
…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>
Summary
Root cause
Selectable message prose was blanket-marked with
data-no-sidebar-swipe, so most of the thread timeline was a gesture dead zone. This replaces the blanket opt-out with intent-based selection arbitration.Validation
pnpm exec turbo run test --filter=@bb/app --force(312 files, 2,347 tests)pnpm exec turbo run typecheck --filter=@bb/app