Skip to content

fix(timeline): cede the resize observer to the load-older index restore#1125

Draft
wpfleger96 wants to merge 15 commits into
mainfrom
duncan/timeline-virtualization-impl
Draft

fix(timeline): cede the resize observer to the load-older index restore#1125
wpfleger96 wants to merge 15 commits into
mainfrom
duncan/timeline-virtualization-impl

Conversation

@wpfleger96

Copy link
Copy Markdown
Collaborator

Draft PR to run the desktop e2e suite against the timeline-virtualization seam fix before it folds into #1123. This branch carries the load-older scroll-restore fix plus the e2e contract updates that virtualization requires. It merges nothing on its own — it exists to surface the behavioral CI gate.

What this branch changes vs 4ac28bfd (the cleared :190 base)

fix(timeline): cede the resize observer to the load-older index restore — During a load-older prepend, two scroll writers raced: the index-restore loop in useLoadOlderOnScroll (the correct single writer, which re-aims to getOffsetForIndex(newIndex) - anchorTop) and the ResizeObserver in useAnchoredScroll. The observer ceded only for convergence, not for load-older, so when the prepended rows measured late and grew scrollHeight, it fired with a now-windowed-out message anchor, hit restoreAnchorToMessage's all-gone fallback, pinned to the floor, and stomped the index restore. The fix adds a shared loadOlderRestoreInFlightRef the index loop holds while it owns scroll, checked at the top of the observer callback alongside the existing convergence cede; the flag is set before the prepend can commit and cleared at both waitForPrepend exits via finishPrepend(), so the cede releases exactly when the index loop relinquishes scroll ownership. The all-gone fallback and the restore math are unchanged — the observer simply defers to the single writer for the prepend duration.

test(timeline): align eva e2e assertions to the virtualized contract — Three e2e assertions encoded the pre-virtualization layout and break once the timeline windows rows out of the DOM and positions them with absolute/translateY:

  • relay-reconnect.spec.ts: the reconnect-backfill test expected the newest and the 260-rows-old message mounted simultaneously. A virtualized timeline windows the oldest rows out while the user sits at the bottom, so it now asserts the newest at the bottom, then scrolls to the top and polls until the oldest mounts — backfill depth proven by reachability.
  • channels.spec.ts (x2): expectIntroBalancedAroundDayDivider compared the intro-to-divider gap against the divider-to-message gap for equality. The intro is a flex sibling above the timeline while the divider and first row are virtualized items, so the two gaps are measured across different layout regimes and no longer match within a pixel. It now asserts reading order: intro, divider, then the first message, each cleanly separated.

Gating

Merges nothing. Stays gated on Will's two-human review and on useAnchoredScroll (#1123) landing on main first; the meaningful rebase is against post-#1123 main.

Related: #1123

npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d and others added 15 commits June 18, 2026 12:36
Replaces the two competing scroll writers (useTimelineScrollManager +
useLoadOlderOnScroll) with one anchor-based primitive, useAnchoredScroll,
owned by both the main timeline and the thread panel.

The prior design had two hooks both mutating scrollTop on prepend, which
is the root of the timeline-jumping bug: a fetch-older restore and a
scroll-anchor adjustment would fight over the same frame. The new hook
keeps a single anchor (the row the reader's eye is on, picked by a
top-crossing walk) and restores it relative to its prior top offset after
every render — prepends, appends, image loads, and embed expansions all
flow through that one path.

Also fixes three concrete bugs surfaced by the ported E2E suite:

- Deep-link targets in older history: the post-mount target effect bailed
  once when the row wasn't loaded yet and never retried. It now keys on
  `messages` and re-runs until the spliced-in row commits, then centers.
- Root-message deep-link reopen: the initial-mount target path scrolled
  and marked the target handled but never fired `onTargetReached`, so the
  `messageId` URL param stuck and re-clicking the same link was a no-op.
  The initial path now fires the callback too, matching the post-mount one.
- Inline image height reservation: images never read their NIP-92 `dim`
  tag, so a tall image grew from ~0 on load and shoved the timeline. We
  now stamp intrinsic width/height so the row reserves space before decode.

Verification: tsc clean, biome (764 files) clean, 989/989 unit tests,
scroll-history 6/6, full e2e smoke 253 passed (3 failures all reproduce
on a clean main checkout — pre-existing, unrelated to this change).

Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
useAnchoredScroll is meant to be the single owner of scrollTop, but both
live scroll containers had lost the [overflow-anchor:none] class (it
survived only on the thread loading skeleton, which never scrolls). With
it gone, Chromium's native scroll-anchoring heuristic re-engaged and
applied its own scrollTop correction on prepend — picking its own anchor
element — while the hook applied a second scrollBy correction on top. When
the two anchors diverged the corrections stacked, producing the residual
jiggle that survived the single-owner rewrite.

Restoring the class on both live containers (MessageTimeline timeline +
MessageThreadPanel body) hands scroll ownership back to the hook alone.

This is a real-wheel-only symptom: native scroll anchoring barely fires
under synthetic scrollTop= writes, so the headless suite stayed green
while a manual macOS scroll pass surfaced it. scroll-history e2e 6/6,
tsc clean.

Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
ChannelRouteScreen splices a getEventById-fetched target into targetMessageEvents so a deep link works in a channel whose feed doesn't already contain the message. The fetch effect wiped those events whenever the route target cleared — and onTargetReached clears the messageId URL param the moment the row is centered — so the only copy of the deep-linked message vanished and the timeline went blank.

Reset spliced events on channel/forum-post change only (guarded effect keyed on channelId/selectedPostId, seeded with the mount key so first-commit cache seeds survive), and have the fetch effect merge cached/fetched events additively instead of overwriting. e2eBridge: route the mock-engineering-shipped known event by 'h' tag so getChannelIdFromTags resolves it.

Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
The older-history fetch spinner renders above the anchor row but toggles on its own render commit (messages unchanged). The anchor-restoration layout effect was keyed only on `messages`, so it never re-ran when the spinner appeared or disappeared — leaving the spinner's height as an uncorrected shift above the reader's eye, the residual flicker on prepend. Thread isFetchingOlder into useAnchoredScroll as an extra restoration trigger so the existing scrollBy correction fires on the spinner toggle too, making the anchor the single owner of every layout change above the fold.

Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
…ssage

Each day group renders as a `<section>` whose React key was the exact
`createdAt` of its first message. Scrolling up prepends older messages,
and when a batch landed on a calendar day already on screen, the first
message of that day changed — flipping the section key. React can't diff
a changed key, so it unmounted and remounted the entire day section, all
its rows torn down and rebuilt above the reader's eye. That whole-section
remount is the residual flicker on scroll-up: the anchor restore was
correcting a full teardown instead of a clean prepend.

Key the section by the local start-of-day of its messages, which is
stable across same-day prepends, so an older message grows an existing
section's children (stably-keyed rows reorder, not remount) instead of
replacing the section. Fold the duplicate key derivation in the render
loop into the lib boundary's `key`, which was already documented as
"stable" but wasn't.

Pure helper `startOfLocalDaySeconds` plus lib tests for day-key stability
across a prepend and separation across calendar days. tsc, biome, 40 lib
unit, scroll-history e2e 6/6 green.

Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
…ssage count

Scrolling up fetched a fixed 100-message batch and stopped. Because thread
replies collapse into their parent's summary and non-content events never
render, a reply-heavy window could fetch 100 events but grow the timeline by
only a handful of rows — making one wheel-up feel like it did nothing.

fetchOlder now pages in batches until at least MIN_TOP_LEVEL_ROWS_PER_FETCH
(10) *visible* top-level rows have been added, or history is exhausted. A new
pure helper, countTopLevelTimelineRows, counts rows the same way
buildMainTimelineEntries renders them: content kinds, minus deletions, top-level
or broadcast replies only. The dedup/exhaustion guards already in place
terminate the loop when the window stops advancing.

Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
…d cost

An instrument, not a gate. Seeds a busy channel against the mock bridge and
measures the main-thread cost the headless correctness suite cannot feel.

Two measurements:
- Fast-wheel scroll of the bounded ~200-row window: compositor-cheap
  (~0.2ms layout + 1.3ms recalc over a 241-frame, 12k-px burst).
- Prepend re-render cost while scrolled up: ~9-13ms main-thread tick,
  attributable to the O(rendered-rows) parent walk (videoReviewContext
  rebuild + day-group boundaries + element construction), NOT layout
  and NOT leaf-row reconciliation (MessageRow memo bails correctly).
  Anchor holds at 0px drift on this path.

Scope limit: measures Chromium reconciliation, not the WKWebView
compositor feel — that remains the real-Tauri macOS pass.

Run: pnpm build && npx playwright test --config=playwright.perf.config.ts
Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
The ResizeObserver in useAnchoredScroll only re-pinned the scroller when
the user was at-bottom — when scrolled up reading older history and a row
above the reading row reflowed (link-card decode, async embed expand, late
font load, markdown that expands), the anchor row shifted on them and
nothing restored it. The PR description claimed image and embed loads all
flow through the anchor, but on a careful read only NIP-92 imeta images
are actually covered (their dim is reserved before decode, so no resize
fires). Every other in-viewport content growth fell into the gap.

Extract the anchor-restoration primitive — find the row (with the
nearest-newer fallback already used for prepends), measure its current
top, scrollBy the delta — into a shared restoreAnchorToMessage helper,
and call it from both the layout effect and the ResizeObserver. One
primitive serves the React-driven path (post-commit, on messages /
spinner change) and the non-React-driven path (image decode, embed
expand, font load), preserving the single-owner invariant. A messagesRef
is mirrored from the layout effect so the observer reads the same list
the DOM was last rendered from without resubscribing on every commit.

E2E coverage: scroll-history e2e adds an in-viewport reflow case that
seeds a scrollable channel, scrolls to a mid position, captures the
top-crossing row's offset, programmatically grows a row above the anchor
via style.minHeight, and asserts the anchor's offset is unchanged within
2px after the observer fires. Confirmed the assertion catches the
pre-fix behavior (80px drift) by reverting the implementation and
re-running.

tsc, biome (764 files), 998/998 unit, scroll-history e2e 7/7 green.

Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
…achball

Channel switch streamed up to 200 uncontained MessageRows (each with
synchronous shiki markdown), then scrollToBottom("auto") forced a
full-document scrollHeight read-then-write reflow before paint over
every row — the macOS beachball Will reported on v0.3.25.

Windows the main timeline on @tanstack/react-virtual. The day-grouped
section tree is flattened to a typed TimelineItem[] stream plus a
messageId->itemIndex map from one walk (cannot drift), and every
DOM-querySelector scroll path (deep-link, search-jump, jump-to-unread,
scrollToBottom, load-older anchor) is re-pathed onto the index model so
windowing does not silently break jumps to off-screen rows.

Scroll convergence is split: @tanstack/react-virtual owns offset
convergence (its rAF loop re-aims getOffsetForIndex as rows mount and
measure); a pure reducer owns only staleness re-resolution and
termination — re-resolving the target's index by id each frame so a
concurrent prepend/delete cannot strand the loop on a stale index, and
terminating when the target is deleted or a 32-frame cap is hit. The
breaking math lives in lib/ under the .mjs suite.

The thread reply list stays content-visibility:auto rather than
virtualized — it is bounded, unpaginated, ungrouped, and shares the
scroll hook, so virtualizing it would force a second index re-path and a
head/prologue split for no beachball gain. Phase-2 route-chunk preload
warms the agents/channel/lazy-view chunks on idle to clear the
Agents-menu first-visit stall.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… anchor

Loading older messages under virtualization let three writers fight over
scrollTop on overlapping frames, so the anchored row jittered or collapsed
to the top (~33% of prepends) and the library's reconcile spun the full 5s
MAX_RECONCILE_MS valve. Establish a single owner of scroll position across
the whole fetch+restore window:

- useLoadOlderOnScroll restores by scrollTop ONLY (drop scrollToIndex), via
  one getOffsetForIndex(anchorIndex + prepended, "start")[0] + intra-row gap
  write. getOffsetForIndex is a pure measurement-cache read, so no library
  scrollState is set and the reconcile loop has nothing to fight.
- The viewport ResizeObserver in useTimelineScrollManager no longer runs a
  competing restore during a fetch: it skips while isFetchingOlder is true
  (the spinner's clientHeight 720->590 mount-shift fires before the lock is
  set) and otherwise defers to lockedScrollTopRef when the load-older restore
  holds it. MessageTimeline threads isFetchingOlder into the manager.

The defect was invisible to unit tests (jsdom getBoundingClientRect -> 0) and
to static traces; the new load-older E2E drives a real prepend on six fresh
page loads and asserts the anchor holds every run, the scroller genuinely
grew, and the reconcile terminates. emitMockHistory now honors the relay
filter's until/limit so the mock relay paginates like a real one, which the
E2E needs to exercise a genuine older page.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
biome check enforces line-wrapping that biome lint does not. The
load-older test 07 had two over-width statements that passed local lint
but failed the Desktop Core biome check gate. Format-only, no behavior
change.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…line

Re-platforms the index-anchor scroll model onto eva's single-owner anchor
(useAnchoredScroll) so the virtualized timeline keeps one scroll writer
under windowing. Removes eva's older-history IntersectionObserver and makes
useLoadOlderOnScroll the sole top-sentinel/fetchOlder owner; eva's anchored
scrollBy cedes to the index path on a prepend via a front-id/tail-id
discriminator (new prevFirstMessageIdRef). Adds a minimal restoreScrollPosition
writer that re-derives the anchor after a programmatic scroll instead of
re-introducing a competing scroll-owner. Windowed-out jump targets converge
through the virtualizer (indexByMessageId + getOffsetForIndex) rather than a
querySelector that goes null off-screen. Load-older roles are gated on
fetchOlder/virtualizer presence so MessageThreadPanel degrades to the passive
anchor. Reserves a fixed-height spinner slot so the fetch indicator does not
shift the bottom row.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…r mid-jump

The rebase resolution wrapped the ResizeObserver at-bottom re-pin in an isAtBottomNow(container) guard. On initial load the virtualizer grows scrollHeight a frame after the first pin (off-screen rows measure late) with no messages change, so the guard read ">2px from the new floor" and skipped the legitimate load-time floor pin, leaving the timeline not at bottom (scroll-history:190). Cede the whole callback while convergingTargetIdRef.current !== null \u2014 the precise jump-in-flight signal the geometry proxy was approximating, mirroring the layout effect's existing bail \u2014 and restore eva's unconditional at-bottom re-pin.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The load-older index restore (useLoadOlderOnScroll) is the single scroll
writer across a prepend, and the layout effect cedes to it via the isPrepend
bail. The ResizeObserver in useAnchoredScroll did not: it ceded only for
convergence. So when the prepended rows measured late and grew scrollHeight,
the observer fired with the now-windowed-out message anchor, hit
restoreAnchorToMessage's all-gone fallback, pinned to the floor, and stomped
the index restore's correct offset.

Add a shared in-flight flag the index loop sets while it owns scroll, checked
at the top of the ResizeObserver callback alongside the convergence cede. The
restore math and the all-gone fallback are unchanged; the observer simply
defers to the single writer for the duration of the prepend.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Three eva-authored e2e assertions encoded the pre-virtualization layout and
break once the timeline windows rows out of the DOM and positions them with
absolute/translateY:

- relay-reconnect: the reconnect-backfill test expected both the newest and the
  260-rows-old message mounted at once. A virtualized timeline windows the
  oldest rows out while the user sits at the bottom, so assert the newest at the
  bottom, then scroll to the top and poll until the oldest mounts -- the backfill
  depth is now proven by reachability, not simultaneous mounting.
- channels (x2): expectIntroBalancedAroundDayDivider compared the intro->divider
  gap against the divider->message gap for equality. The intro is a flex sibling
  above the timeline while the divider and first row are virtualized items, so
  the two gaps are measured across different layout regimes and no longer match
  within a pixel. Assert the intended reading order instead: intro, divider,
  then the first message, cleanly separated with no overlap.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant