fix(desktop): single-owner anchored scroll for dynamic loading#1115
Merged
Conversation
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>
Co-authored-by: npub1mprnacetjua2xx3p5eddmhxyk6wv929ymm5py8kd2xfxurxahspqqlgyta <d8473ee32b973aa31a21a65adddcc4b69cc2a8a4dee8121ecd51926e0cddbc02@sprout-oss.stage.blox.sqprod.co> Signed-off-by: npub1mprnacetjua2xx3p5eddmhxyk6wv929ymm5py8kd2xfxurxahspqqlgyta <d8473ee32b973aa31a21a65adddcc4b69cc2a8a4dee8121ecd51926e0cddbc02@sprout-oss.stage.blox.sqprod.co>
…oll PR Brings Max's flat semantic render rows + lazy per-row video review context (max/render-model-step1, e8500c4) onto the useAnchoredScroll single-owner branch. Zero file overlap with the anchor + ResizeObserver work (TimelineMessageList.tsx + videoReviewContext.* vs useAnchoredScroll.ts + perf harness), so the merge is conflict-free. This makes #1115 carry what the milestone described: anchor redesign + Sami's resize fix + flat rows + lazy video context. Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
…airline The flat-render merge left the day row wrapped in a section with gap-2.5 py-1 and a before:top-[19px] hairline, plus mb-0.5 on both intro cards. That asymmetry broke expectIntroBalancedAroundDayDivider (gap diff 2 > 1). Move the hairline onto DayDivider's own before: pseudo-element, centered with top-1/2 + -translate-y-1/2 so it no longer depends on a magic 19px offset; drop the now-purposeless single-child wrapper; remove the compensating mb-0.5 from both intro cards. Spacing is symmetric, the line decoration is preserved and self-contained, no magic pixels remain. Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
…t fight it scrollToMessageImperative read el.getBoundingClientRect() immediately after scrollIntoView. With behavior:"smooth" the animation is async, so the rect still reflects the pre-animation position — the anchor was saved at the row's start-of-scroll offset. Any messages commit during the ~300ms animation re-ran restoreAnchorToMessage, found the row mid-flight, computed a delta against the stale offset, and scrollBy'd it back toward the start. Find-in-channel next-match in a live channel jerked back. Compute the clamped destination offset analytically before kicking off the animation (block:"center" lands the row at (clientHeight - height)/2, clamped to the real scroll range), and save that as the anchor. Mid-animation restoration is now a no-op because the saved offset is the eventual offset. No timers, no listeners. Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.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.
Problem
PR #1113 fixed one of two scroll writers but the timeline-jumping bug persisted, because the old design had two hooks both mutating
scrollTopon prepend (useTimelineScrollManager+useLoadOlderOnScroll). A fetch-older restore and a scroll-anchor adjustment fought over the same frame — that race is the jump.Approach
Replace both with a single anchor-based primitive,
useAnchoredScroll, owned by both the main timeline and the thread panel.The hook keeps one anchor — the row the reader's eye is on, chosen by a top-crossing walk (the first row whose bottom edge has crossed below the container top) — and after every render restores it to the same top-relative offset it had before. Prepends, appends, image loads, and embed expansions all flow through that single restoration path instead of competing writers.
Bugs fixed along the way (surfaced by the ported E2E suite)
messagesa render later; the effect now keys onmessagesand re-runs until the row commits, then centers.onTargetReached, so themessageIdURL param stuck and re-clicking the same link was a no-op. The initial path now fires the callback too, matching the post-mount path. (Root-caused fromnavigation.spec.ts:301.)dimtag, so a tall image grew from ~0 on load and shoved the timeline. We now stamp intrinsicwidth/heightso the row reserves aspect-ratio space before decode.9b18ad01) — the flat-render merge wrapped the day row in asectionwithgap-2.5 py-1and abefore:top-[19px]hairline, plusmb-0.5on both intro cards, breakingexpectIntroBalancedAroundDayDivider(gap diff 2 > 1). The hairline now lives onDayDivider's ownbefore:pseudo-element, self-centered withtop-1/2 / -translate-y-1/2(no magic 19px); the wrapper section and bothmb-0.5s are gone. Symmetric spacing, hairline preserved.f652745b) —useAnchoredScroll'sscrollToMessageImperativesaved the anchor by readinggetBoundingClientRect()immediately afterscrollIntoView({ behavior: "smooth" }), capturing the pre-animation offset; a live message commit mid-scroll restored against the stale value and yanked the view back (find-in-channel next-match jerked backward in a live channel). Now the clamped destination offset is computed analytically before the animation starts and saved as the anchor — mid-animation restore is a no-op. No timers, no listeners.Verification
tscclean ·biome check .— 764 files, no fixeschannels.spec+scroll-history.spec: 43/43 (intro/day-divider balance + find-bar active-match scroll/highlight)f652745b: 17/17 checks pass, zero failures (Desktop Core, Build (macOS), E2E Integration ×2, E2E Relay, Smoke E2E ×4, security checks)Earlier full-smoke baseline (pre-spacing/smooth-scroll fixes): 253 passed, 3 failed — all 3 (
navigation:340,video-attachment:127,custom-emoji-screenshots:73) reproduce on a cleanmaincheckout, i.e. pre-existing and unrelated to this change.Not done / follow-ups
navigation:340("deep links survive reload") is a mock-bridge fixture gap (mock-engineering-shippedisn't seeded into the engineering timeline) rather than a scroll bug.