Repository navigation
Fix timeline re-render amplifiers: stable sender metadata + shared selection listeners - #1285
Conversation
…ine row useSenderThreadMetadataById rebuilt a fresh Map on every matching query cache event and fed it into TimelineRendererStaticContext, so every realtime event re-rendered every timeline row past React.memo (the timeline is un-virtualized). Three changes: - The hook keeps the previous map reference when a rebuild is value-equal, so no state update propagates for no-op cache events. - The sync predicate drops "observerResultsUpdated": the map is built from query.state.data, which only changes via "updated" dispatches. - The metadata map moves out of the wide static renderer context into a dedicated SenderThreadMetadataContext, so a genuine metadata change (any thread title anywhere) re-renders only the conversation rows that resolve sender labels, not every row. Measured with a React Profiler over a 21-row timeline in jsdom: 10 equal cache events caused 11 full-timeline commits (~136ms of render work) before, 0 commits after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each assistant message registered 6 document-level listeners, so per-tap and per-selectionchange handler work was O(N messages). A module-level registry now owns one shared set of document listeners and one report frame; each instance keeps its own selection state (hadSelection gate, pending/last 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 the double/triple-click semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
| instance.onSelectRef.current?.(next); | ||
| } | ||
|
|
||
| function reportAllInstances(): void { |
There was a problem hiding this comment.
🚨 slopcop/review — The shared callback still does O(N) work for each selection event.
reportAllInstances calls getSelection() through readSelectionWithinNode for every mounted message. The shared pointer-down callback also scans every instance.
A browser test with 30 messages produced 30 getSelection() calls after one selectionchange event. This change reduces the listener count, but it keeps the main per-message scan.
Track the active instance from the pointer target or selection range. Report only that instance and the previous selected instance.
There was a problem hiding this comment.
Fixed in 06a80dc. reportAllInstances now reads the live range once and pre-filters with range.intersectsNode; non-intersecting instances take a null path with no selection read (both acceptance paths in readSelectionWithinNode require containment or intersection, so results are unchanged). The pointerdown handler now resolves the active instance with one ancestor walk over a node→instance map instead of a per-instance contains() probe. One selectionchange with N messages now does 1 getSelection() + N intersectsNode boundary checks + 1 full read.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: This change gives all timeline messages one event listener set. It also stops unchanged thread data from redrawing the timeline.
I found one medium performance concern.
- The shared listener still scans every mounted message. A browser test with 30 messages produced 30
getSelection()calls after oneselectionchangeevent. The change reduces listener and frame counts, but it keeps the main O(N) scan. I left an inline comment with a focused fix.
The security review found no security issues. The sender metadata change works as claimed. Equal cache writes kept the same map reference, while real title changes updated the sender label.
I also checked related selection code in SecondaryPanelSelectionActions. That component accepts different selection boundaries, so a shared reader would mix two policies. The useful refactor belongs in the prose registry: track the active and previous selected instances.
Validation was clean:
- 50 focused and regression tests passed locally.
- The
@bb/appTurbo type check passed. - All GitHub checks passed.
- The pull request is mergeable.
- An isolated browser test moved selection between two messages. Add to chat used only the latest text and closed the menu.
The browser check used a programmatic range. It did not cover a physical drag, an iOS long-press, or real multi-click input.
Address the SlopCop review finding: the shared report frame called getSelection() once per mounted message, and the shared pointerdown handler probed node.contains(target) per message. - reportAllInstances reads the live range once; instances the range does not intersect take a cheap null path with no selection read. Both acceptance paths in readSelectionWithinNode require containment or intersection, so the pre-filter cannot change results. - pointerdown resolves the active instance with one ancestor walk over a node-to-instance map, replacing the per-instance contains() probe and the per-instance pointerStartedInNode flag. 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>
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.
useSenderThreadMetadataByIdrebuilt a freshMapon every matching query-cache event with no equality check, and its predicate also fired onobserverResultsUpdated(observer churn with no data change). The map fedTimelineRendererStaticContext, and context identity changes bypassReact.memo— so everysidebarNavigation/threads/threadcache 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 everyselectionchangedispatched N sets of handlers and scheduled up to N rAF callbacks.Fix
Commit 1 (
useSenderThreadMetadataById+ThreadTimelineRows):observerResultsUpdated— the map is built exclusively fromquery.state.data, which only changes viaupdateddispatches.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):hadSelectionnull-once gate, pending/last-release anchors, multi-click timer) and its node-scopedclick/dblclicklisteners. 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
threadscache events (the shape realtime refetches produce):Document listeners for N assistant messages: before 6·N, after 6 total.
Validation
pnpm exec turbo run typecheck --filter=@bb/app— passpnpm 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— passuseSenderThreadMetadataById.test.tsx— stable reference on equal rebuilds, new reference + value on real changes, stability across unrelated query keysThreadTimelineRows.render-stability.test.tsx— profiler-counted zero commits on equal cache events; sender title updates flow through to the DOMSelectableMessageProse.events.test.tsx— mounting additional messages adds zero document listeners; a selection moving between two messages clears the old one exactly onceTimeline virtualization remains explicitly out of scope.
🤖 Generated with Claude Code