Rework desktop message-timeline scrolling: de-virtualize + native overflow-anchor#1338
Open
wesbillman wants to merge 10 commits into
Open
Rework desktop message-timeline scrolling: de-virtualize + native overflow-anchor#1338wesbillman wants to merge 10 commits into
wesbillman wants to merge 10 commits into
Conversation
…ll anchoring Render all loaded timeline rows directly via a `.map` wrapped in the `ln-auto` (content-visibility) utility instead of `VirtualizedList`, and remove `[overflow-anchor:none]` from the scroll container so the browser's native scroll anchoring holds the reading position across reflow/prepend. Collapses the virtualizer-consumer wiring in MessageTimeline: drops the index-model deep-link convergence, the index bottom-pin, and the virtualizer option fed into the scroll hooks. Jump-to-message is now purely DOM-based (all rows are mounted). The floating active-day header (which was portaled and driven by the virtualizer's top-visible index) is removed here; a scroll-driven replacement lands in a follow-up. First commit of the timeline scroll rework. Hook-internal cleanup (useAnchoredScroll / useLoadOlderOnScroll virtualizer branches, the orphaned convergence modules) and the JS prepend-delta removal follow in later commits, gated on the native-anchoring de-risk probe. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…horing With the timeline de-virtualized and `overflow-anchor` re-enabled, the browser holds the reading position across an older-history prepend on its own. The non-virtualized load-older handler's `capture height -> double-rAF -> restoreScrollPosition(prev + delta)` was a second scroll writer that would double-correct the same reflow once native anchoring is live. Remove it: the sentinel now only triggers `fetchOlder` and re-arms the observer. This is the native-anchoring de-risk gate — the no-shift probe re-runs against this commit to confirm the <=2px hold survives with no JS scroll write masking it. The dead virtualizer branch in this hook is removed in a follow-up cleanup. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
The native-anchoring de-risk gate passed (no-shift probe: 0px drift, 0 missing samples, 240 frames, with no JS prepend restore), so the windowing-era scroll machinery is now dead code. Remove it: - useLoadOlderOnScroll: drop the entire virtualizer prepend-restore branch (index re-aim, abandon-to-bottom, ResizeObserver cede plumbing) and the options it needed. The hook is now just an IntersectionObserver that triggers fetchOlder and re-arms; native anchoring holds the position. - Delete useConvergentScrollToMessage and scrollConvergence (+ its test): deep-link is pure DOM scrollIntoView now that all rows are mounted. - MessageTimeline: stop consuming the now-removed hook return values. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
With native scroll anchoring proven (no-shift probe: 0px through prepend + above-anchor reflow, JS delta gone), the hook's JS message-anchor restore is the redundant second writer the one-owner rule forbids. Remove it and the windowing-era machinery it implied: - delete restoreAnchorToMessage / findNearestNewerMessageId (the computeAnchor -> scrollBy(0, delta) restore) and both call sites (layout effect + ResizeObserver); - drop the convergence cede, the load-older cede, the isPrepend cede, and the settle-window (settlingRef); - drop the convergeToTarget / pinToBottomByIndex options and the restoreScrollPosition / setLoadOlderRestoreInFlight / getAnchorIsAtBottom result surface (all unused after the prior cleanup). Preserved behaviors (native anchoring does NOT cover these): mount bottom-pin, stick-to-bottom-on-append, send-snap, at-bottom re-pin on reflow, DOM deep-link + its retry-on-commit target effect, isAtBottom / new-message-count. 822 -> 439 lines. tsc + biome clean. Gated on the final no-shift probe pass. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…-out Accumulate every relay batch of a scrollback pass and commit to the cache once, so a reply-heavy window grows in a single bounded step instead of painting in several small increments under one spinner. Remove the [overflow-anchor:none] opt-out from the thread panel body so native scroll anchoring can hold the reading position there too. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
The de-virtualization rework relied on native `overflow-anchor` to hold scroll position across an older-history prepend, but the browser suppresses scroll anchoring at the top edge (no anchor node above the viewport at scrollTop ~0), so a prepend left the user pinned at offset 0 staring at the oldest of the new page. And because nothing pushed scrollTop off 0, the load-older observer re-armed into the still-intersecting sentinel and cascaded page-after-page to the channel start. Three coordinated changes, one concern each: - useAnchoredScroll: when a prepend lands while anchored mid-history, re-pin the anchored row to its saved offset by id (single scroll writer for the prepend). Runs in the post-commit layout effect, after the deferred snapshot commits, so the row's true position is known. Native anchoring keeps in-viewport reflow. - useLoadOlderOnScroll: one long-lived observer that fires once on entering the trigger band then disarms until the sentinel leaves and re-enters — a fresh scroll-up gesture. Replaces the re-arm-on-resolve that double-fired because the prepend commits a few frames late. One gesture -> one page. - Widen the trigger band to 600px (PRELOAD_MARGIN_PX) so the next page preloads while scrolling. Kept below a typical prepend height so the landed page still pushes the sentinel out of the band and re-arms the gate. Adds a regression test asserting the reading row stays fixed across a prepend, and a fetch-count probe in the e2e bridge for the one-page-per-gesture test. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
A channel shorter than the viewport painted top-anchored: the scroll content column only got min-h-full for the intro/empty surfaces, so the message-list case had no slack for its mt-auto to consume and the list sat snug at the top. Extend min-h-full to the list surface so mt-auto bottom-pins a short channel from the first paint, matching a full channel — and so a late older-history prepend lands above the content without shifting the rows the reader is on. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…tualization
The de-virtualization rework left behind machinery that only existed to
serve the old windowed renderer, now unreferenced:
- ActiveDayHeader: a floating day label needed only because in-stream day
dividers unmounted when scrolled out of the virtual window. Rows persist
in the DOM now, so the sticky in-stream DayDivider holds the viewport top
again and the portaled label is dead. Delete the component.
- resolveActiveDayTimestamp + the indexByMessageId map: the former fed the
floating label; the latter was the virtualizer's O(1) deep-link resolver.
Scroll is DOM-based now (querySelector), so nothing consumes either.
Remove both, simplify buildTimelineItems to return { items }, and drop
their unit tests. Refresh the stale "virtualizer" doc comments.
Also fix scroll-history e2e "preserves user scroll while older channel
history loads": its floating-day-header assertions targeted the removed
ActiveDayHeader and failed on a null lookup. Drop those assertions and the
getActiveDayHeader helper; the scroll-preservation poll (the test's real
guarantee) is unchanged and still passes.
Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Collaborator
Author
|
@codex please review |
Open channel timelines with a smaller recent-history window so native-scroll timelines do not synchronously build hundreds of rows on cold entry. Older history still streams in via the existing scroll-up pagination path. Fetch/splice find-in-channel hits that live outside the initial window so search targets can still be scrolled into view, and retry DOM target scrolling after the row commits. Keep the no-shift coverage aligned with the smaller initial window. Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Collaborator
Author
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f870c12f61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (messagesArrived > 0) { | ||
| setNewMessageCount((current) => current + messagesArrived); | ||
| } else if (messagesArrived > 0) { |
There was a problem hiding this comment.
When the reader is scrolled up and fetchOlder prepends older history, messages.length - prevCount is positive even though no latest message arrived (the tail id is unchanged). Because this branch now runs for every positive growth, the scroll-to-latest pill reports the fetched history page as “new messages” instead of staying at “Jump to latest”; this happens on normal scrollback pagination and was previously avoided by the prepend check. Please gate the counter on an actual append/new latest message, not on prepends.
Useful? React with 👍 / 👎.
Comment on lines
+334
to
+335
| if (!jumpToMessage(searchActiveMessageId, { behavior: "smooth" })) { | ||
| pendingSearchTargetRef.current = searchActiveMessageId; |
There was a problem hiding this comment.
If a relay search hit is outside the rendered window, this stores it in pendingSearchTargetRef; but a later successful jump to another match or closing the find bar never clears that ref. When the async spliced event for the old hit eventually commits, the retry effect below will jump back to that stale match even though it is no longer active. Clear the pending target whenever searchActiveMessageId changes to a different value/null or when the immediate jump succeeds.
Useful? React with 👍 / 👎.
Do not count older-history prepends as new bottom messages while keeping the anchored-row restore for scrollback pagination. Clear stale pending find targets when the active match changes or clears, so a delayed splice cannot jump back to an inactive search hit. Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rework desktop message-timeline scrolling: native scroll + smaller cold window
What & why
The desktop message list was virtualized, which made scroll position fragile: the reading row jumped on older-history loads, pages could cascade off a single scroll gesture, and short channels cold-loaded top-anchored then shifted down. This reworks the timeline to keep loaded rows in the DOM and lean on browser/native scroll anchoring instead of a virtualizer-specific restore stack.
The first de-virtualized version fixed the scroll feel but cold-mounted ~300 rows and produced a visible longtask (~600ms in earlier measurement). The final commit keeps the native-scroll behavior while opening channels on a smaller recent window so cold entry does not synchronously build hundreds of rows.
Changes
content-visibilityinstead of virtualizing the main message timeline.overflow-anchoron the timeline and delete the virtualizer-era convergence / restore machinery.Behavior change to review
Oldest history is no longer pre-built on channel open. A channel opens on the recent window and loads older history as the user scrolls up. Find/deep-link targets outside the 60-row window are fetched/spliced on demand so they still land in the timeline.
Out of scope / follow-up
The deep-history top-edge bounce is tracked separately and is not part of this PR.
Validation
Local + reviewer-verified before push:
pnpm exec biome check ...✅pnpm exec tsc --noEmit --pretty false✅pnpm build✅cold-switch-longtask.perf.ts: median longest longtask ~115–133ms (down from ~630ms de-virt baseline) ✅scroll-history.spec.ts: 14/14 ✅timeline-no-shift.spec.ts: timelinemaxDrift: 0; thread-panelmaxDrift: 0with meaningful rAF sample count ✅virtualization.spec.tsnon-timeline coverage ✅