Window large mobile timelines - #1384
Conversation
SawyerHood
changed the title
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am now reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
| [location.state], | ||
| ); | ||
| const initialWindowCenterIndex = useMemo(() => { | ||
| if (searchTarget !== null) { |
There was a problem hiding this comment.
🚨 slopcop/review — Search state from another thread can override the saved scroll anchor.
This condition uses the search sequence before it checks the target thread. An overlapping sequence centers the wrong thread and skips its saved anchor.
The always-realized path and the scroll hook already check the thread ID. Apply the same check here. Add a mismatched-thread test with a saved anchor.
| return; | ||
| } | ||
| const scrollElement = getScrollElement?.() ?? null; | ||
| if (scrollElement === null || scrollElement.clientHeight === 0) { |
There was a problem hiding this comment.
🚨 slopcop/review — A zero-height scroll root permanently disables window updates.
The effect returns when the root first has zero height. No dependency changes when a hidden or initial flex layout later gives it height.
The initial estimated window then stays mounted, and other placeholders stay empty during a scroll. Remove this height check, or start the observer after a resize event.
| aria-hidden={isRealized ? undefined : true} | ||
| style={isRealized ? undefined : { height: placeholderHeight }} | ||
| > | ||
| {isRealized ? renderItem(item) : null} |
There was a problem hiding this comment.
🚨 slopcop/review — Window removal resets user-controlled row state.
This line removes the row subtree after the row leaves the margin. Several children store user state in the component. They include row expansion, long-message expansion, and tool-argument expansion.
A user can expand an old row, scroll away, and return to a collapsed row. Store the state above the window, or keep stateful rows mounted. Add an exit and return test.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: This change keeps only nearby mobile thread rows active. It uses empty boxes for other rows, which reduces browser work.
I found three issues.
- A row loses its local user state after it leaves the window. An expanded row can return in a collapsed state.
- A zero-height scroll root can prevent all later window updates. This can leave empty rows after the surface becomes visible.
- Search state for another thread can override the correct saved scroll anchor.
I left inline comments at each affected line.
The security review found no issue. The change adds no network, authorization, storage, code execution, or HTML injection path.
The new window code repeats the design in SidebarWindowedItems.tsx. A shared keyed, in-flow window hook could reduce future behavior differences.
I tested a compact mobile route with a seeded 1,202-event thread. The route kept 25 of 73 top-level rows active and reached both ends without a page error.
The focused test passed all 34 tests. The Turbo app type check also passed.
14056ad to
0c88e34
Compare
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and product behavior. I started three parallel reviews. I will add one final review after the code and product checks finish. |
e04cde8 to
00b7a7b
Compare
00b7a7b to
31491a0
Compare
During a scroll the windowing observer skipped its anchor compensation and trusted native scroll anchoring, but WebKit has none. A placeholder that realized above or over the viewport shifted visible content by the placeholder-vs-real height delta, so the timeline skipped and jittered. Now, while the scroll marker is set, only height-neutral updates apply immediately: derealizations swap in their measured height, and realizations fully below the viewport push layout downward only. Realizations at or above the viewport wait in a pending set; when scroll events stay silent past the idle delay, they flush through the existing capture-anchor / flushSync / restore path, which can no longer kill momentum because the scroll has stopped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Waiting for full scroll idle made rows blank for the whole momentum tail: iOS emits scroll events through the entire deceleration, so every event pushed the idle flush out and everything the user scrolled up into stayed empty until the scroll fully stopped. Rows above the viewport now realize the moment they enter the margin, even mid-scroll. The realized height delta is balanced against placeholder donors that also sit fully above the viewport, in a second synchronous commit in the same task, so the net height above the viewport stays zero: nothing visible moves, no scrollTop write happens, and momentum survives. Donors shrink topmost-first and self-correct when they later realize or derealize. A row with no donor capacity (near the top of the timeline) reverts to its unchanged placeholder and keeps the idle-flush fallback with scrollTop compensation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SawyerHood
changed the title
|
🚨 SLOP COP 🚨 · review (manual run) I’m the Slop Cop for this manual run. I’m reviewing this PR now against the security, code quality, architecture, performance, and end-to-end guidelines. I will post one final comment after the parallel review and browser check finish. |
|
🚨 SLOP COP 🚨 · review (manual run) ELI5: This change keeps nearby mobile timeline rows active. It replaces distant rows with empty boxes, which reduces browser work while preserving scroll height. I found five blockers at head f7d54c2.
The security review found no new security issue. This change adds no network, storage, authorization, code execution, or unsafe HTML path. The performance review also found two non-blocking costs. Row updates rebuild the observer for all wrappers, and donor selection can approach quadratic work. The final donor commits also need a new iOS WebKit trace because they add synchronous work during active scroll. The new code repeats the observer maps, wrapper registration, and placeholder design from SidebarWindowedItems.tsx. A shared in-flow window primitive would reduce drift. The timeline-specific momentum policy should remain separate. I tested a 390×844 route with a seeded 1,368-event thread. It loaded 124 wrappers and kept 15–35 complete rows during 122 scroll samples. The route had no console or server errors. The focused timeline file passed 38 tests. The Turbo app type check passed. All current GitHub checks passed. |
- No-donor realizations no longer leave blank rows: instead of reverting to a placeholder and waiting for scroll idle, the row realizes immediately and a clamped scrollTop write absorbs whatever residual the donors could not. The idle-flush machinery is gone. - Late content growth (lazy images, async rendering) in realized rows above the viewport is now compensated: a ResizeObserver diffs each realized wrapper against its last committed height and balances the delta into donors between layout and paint. - Terminal auto-expansion ids accumulate at the timeline root, so a latched frontier-error row that windowed eviction unmounted re-expands when it remounts. - Interaction pins are capped at 24 in recency order; the oldest pin releases and its row derealizes on its next window exit. - The Intersection/Resize observers no longer recreate on every streamed row: callbacks read the current key order through a ref, so row updates stop re-firing initial entries for all wrappers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the SlopCop findings in f3bd288:
Also took the performance note: the Intersection/Resize observers no longer recreate on every streamed row — callbacks read the current key order through a ref, so row updates stop re-firing initial entries for every wrapper. Typecheck passes and the timeline + scroll-body suites pass (171 tests, including new coverage for the residual write, late-growth re-balancing, and the pin cap).
|
The react-hooks lint flags ref reads in an effect cleanup; the map ref never changes identity, so a local capture is equivalent and quiets the rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Placeholder estimates undershoot real row heights, so upward scrolling drained the donor pool monotonically; once dry, every realization wrote scrollTop and stopped WebKit momentum. Two changes keep the pool solvent and the writes rare: - Idle re-budgeting: track the running average of first-measured row heights, and once the scroll idles re-seed every never-realized placeholder to that average through the anchor-compensated path (a scrollTop write is free at idle). This refills drained donors and keeps the estimate budget tracking the timeline's real heights. - Residual tolerance: a shortfall of 48px or less shifts content instead of writing scrollTop — a sub-threshold stutter reads better than a momentum stop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The SlopCop-driven changes traded the momentum-preserving revert fallback for mid-scroll scrollTop writes and tolerated sub-threshold shifts, and let ResizeObserver rebalance donors during active scrolls. All three mutate what the user sees mid-gesture, which reads as snapping. Restore the invariant: while a scroll is active, the only geometry change is the exact net-zero donor swap. Everything else waits for the idle pass: - An insolvent realization reverts to its unchanged placeholder and mounts at idle with anchor compensation (brief near-top blanks, made rare by re-budgeting). - Late content growth above the viewport compensates with a direct scrollTop nudge at idle only; during a scroll just the baseline updates. - The pending-realize flush and the placeholder re-budget share one idle pass and one anchor-compensated commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
iOS Safari results
Tested an 85-row mobile timeline from a seeded thread with 1,368 event rows.
Verification
pnpm exec turbo run lint typecheck --filter=@bb/apppnpm exec turbo run build --filter=@bb/appUpdate: scroll-time realization (2026-08-12)
Two follow-up commits fix scroll defects that field testing found after the results above.
Skips and jitters during momentum scrolls. The scroll owner sets
data-scrollbar-scrollingon every scroll event, so window updates during a scroll skipped the anchor compensation and trusted native scroll anchoring — which WebKit does not implement. A placeholder that realized above the viewport shifted visible content by the placeholder-vs-real height delta.Fix: donor-compensated realization. While a scroll is active:
scrollTopwrite happens, and momentum survives. Donors shrink topmost-first and self-correct when they later realize or derealize.An intermediate approach that deferred all above-viewport realizations to scroll idle was rejected: iOS emits scroll events through the whole momentum tail, so rows stayed blank until the scroll fully stopped.
New unit tests cover scroll-time below-viewport realization, donor shrink across multiple placeholders, donor growth for shorter-than-placeholder content, and the no-donor revert + idle-flush path. Typecheck passes and the timeline and scroll-body suites pass (170 tests).
🤖 Generated with Claude Code