diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index 20286799b..ded4993ef 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -245,6 +245,7 @@ const MessageTimelineBase = React.forwardRef< syncScrollState, } = useTimelineScrollManager({ channelId, + isFetchingOlder, isLoading: showTimelineSkeleton, messages: deferredMessages, onTargetReached, diff --git a/desktop/src/features/messages/ui/useLoadOlderOnScroll.ts b/desktop/src/features/messages/ui/useLoadOlderOnScroll.ts index cb36f2a86..bbf190111 100644 --- a/desktop/src/features/messages/ui/useLoadOlderOnScroll.ts +++ b/desktop/src/features/messages/ui/useLoadOlderOnScroll.ts @@ -98,12 +98,17 @@ export function useLoadOlderOnScroll({ (virtualizerRef.current?.itemCount ?? previousCount) - previousCount; if (after && anchorIndex !== null && prepended > 0) { - after.scrollToIndex(anchorIndex + prepended, { - align: "start", - }); - // scrollToIndex aligns the row's top to the viewport top; - // re-apply the captured gap so the view doesn't nudge by a - // partial row. + // Restore by scrollTop ONLY — a single writer. Compute the + // anchored row's top via getOffsetForIndex (a pure read of + // the measurement cache, no scrollState) and add back the + // captured intra-row gap. Calling scrollToIndex here too + // would set the library's scrollState aiming at the row TOP + // while this restore aims at row top + gap; the two write + // scrollTop to different values on overlapping rAF frames, + // so the library's reconcile never reaches approxEqual, + // never re-scrolls (its target is unchanged), and spins one + // rAF/frame for the full 5s MAX_RECONCILE_MS valve on every + // prepend. One mechanism, no fight. const target = after.getOffsetForIndex( anchorIndex + prepended, "start", diff --git a/desktop/src/features/messages/ui/useTimelineScrollManager.ts b/desktop/src/features/messages/ui/useTimelineScrollManager.ts index f5413612a..32e319a19 100644 --- a/desktop/src/features/messages/ui/useTimelineScrollManager.ts +++ b/desktop/src/features/messages/ui/useTimelineScrollManager.ts @@ -12,6 +12,7 @@ import { useConvergentScrollToMessage } from "./useConvergentScrollToMessage"; type UseTimelineScrollManagerOptions = { channelId?: string | null; + isFetchingOlder?: boolean; isLoading: boolean; messages: TimelineMessage[]; onTargetReached?: (messageId: string) => void; @@ -37,6 +38,7 @@ type PinToBottomOptions = { export function useTimelineScrollManager({ channelId, + isFetchingOlder = false, isLoading, messages, onTargetReached, @@ -63,6 +65,12 @@ export function useTimelineScrollManager({ // a streaming-in list is what makes the timeline thrash on entry. const isLoadingRef = React.useRef(isLoading); isLoadingRef.current = isLoading; + // Mirror isFetchingOlder so the viewport ResizeObserver (subscribes once) can + // see the live value: the load-older path owns scroll position across its + // whole fetch+restore window, so the observer must not run a competing + // restore while a fetch is in flight (see the resize handler below). + const isFetchingOlderRef = React.useRef(isFetchingOlder); + isFetchingOlderRef.current = isFetchingOlder; const [isAtBottom, setIsAtBottom] = React.useState(true); const [highlightedMessageId, setHighlightedMessageId] = React.useState< string | null @@ -307,7 +315,27 @@ export function useTimelineScrollManager({ return; } - restoreScrollPosition(previousScrollTopRef.current); + // The load-older path owns scroll position across its whole window. Two + // guards keep this observer from running a competing restore — without + // them the spinner's clientHeight 720->590 shift fires here and restores + // to previousScrollTopRef.current (0, since the user scrolled to the top + // to trigger), collapsing the anchor. + // + // Guard 1 — fetch in flight, lock not yet set: the spinner mounts BEFORE + // the fetch resolves and calls restoreScrollPosition, so lockedScrollTop + // is still null on this fire. Skip entirely; the load-older path restores + // once the page arrives. + if (isFetchingOlderRef.current) { + return; + } + + // Guard 2 — restore running, lock set: a later shift (e.g. spinner + // unmount) can fire while restoreScrollPosition's rAF loop holds its + // target in lockedScrollTopRef. Defer to that target so both aim at the + // same scrollTop instead of fighting frame-by-frame. + restoreScrollPosition( + lockedScrollTopRef.current ?? previousScrollTopRef.current, + ); }); observer.observe(timeline); diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 743e40ded..197c89b41 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -466,6 +466,8 @@ type MockFilter = { "#h"?: string[]; authors?: string[]; kinds?: number[]; + limit?: number; + until?: number; }; type MockSocket = { @@ -1491,6 +1493,37 @@ const mockChannels: MockChannel[] = [ createMockMember(MOCK_IDENTITY_PUBKEY, "member", 700), ], }), + // Deep history channel for the load-older-under-virtualization E2E. Seeded + // with more messages than CHANNEL_HISTORY_LIMIT (200) so the initial load + // windows to the newest page and a `fetchOlder` (until-cursor) prepend has + // genuinely older rows to add — exercising the scroll-restore anchor under + // virtualization. Its own channel so existing channels' row-index and unread + // assertions stay undisturbed. + createMockChannel({ + id: "feedf00d-0000-4000-8000-000000000007", + name: "deep-history", + channel_type: "stream", + visibility: "open", + description: "Channel with paginated history for load-older tests", + topic: null, + purpose: null, + last_message_at: isoMinutesAgo(1), + archived_at: null, + created_by: ALICE_PUBKEY, + topic_set_by: null, + topic_set_at: null, + purpose_set_by: null, + purpose_set_at: null, + topic_required: false, + max_members: null, + nip29_group_id: null, + created_minutes_ago: 2000, + updated_minutes_ago: 1, + members: [ + createMockMember(ALICE_PUBKEY, "owner", 2000), + createMockMember(MOCK_IDENTITY_PUBKEY, "member", 1900), + ], + }), ]; const mockMessages = new Map(); @@ -2257,15 +2290,50 @@ function getMockMessageStore(channelId: string): RelayEvent[] { sig: "mocksig".repeat(20).slice(0, 128), }, ] - : []; + : channelId === "feedf00d-0000-4000-8000-000000000007" + ? // 600 messages > CHANNEL_HISTORY_LIMIT (200): the initial load + // windows to the newest 200, leaving 400 older behind the until + // cursor — enough for several full fetchOlder pages (batch 100), + // so the load-older anchor restore is exercised across REPEATED + // prepend cycles, not a single lucky pass. created_at increases + // with index (oldest first) so message N+1 is newer than N — the + // anchor restores the first-visible row across each prepend. + Array.from({ length: 600 }, (_, index) => ({ + id: `mock-deep-history-${index}`, + pubkey: index % 2 === 0 ? ALICE_PUBKEY : MOCK_IDENTITY_PUBKEY, + created_at: Math.floor(Date.now() / 1000) - (600 - index) * 60, + kind: 9, + tags: [["h", channelId]], + content: `Deep history message #${index}`, + sig: "mocksig".repeat(20).slice(0, 128), + })) + : []; mockMessages.set(channelId, seeded); return seeded; } -function emitMockHistory(socket: MockSocket, subId: string, channelId: string) { - const events = getMockMessageStore(channelId); - for (const event of events) { +function emitMockHistory( + socket: MockSocket, + subId: string, + channelId: string, + filter?: MockFilter, +) { + // Honor the relay window so load-older paginates instead of replaying the + // whole store. A real relay returns the newest `limit` events at or before + // `until` (inclusive — the client dedupes the boundary message by id). Cap at + // `limit` after the `until` filter so the page is genuinely older content. + const events = getMockMessageStore(channelId).filter( + (event) => filter?.until === undefined || event.created_at <= filter.until, + ); + const limit = filter?.limit ?? events.length; + const windowed = + events.length > limit + ? [...events] + .sort((left, right) => right.created_at - left.created_at) + .slice(0, limit) + : events; + for (const event of windowed) { sendWsText(socket.handler, ["EVENT", subId, event]); } sendWsText(socket.handler, ["EOSE", subId]); @@ -5847,7 +5915,7 @@ function sendToMockSocket(args: { return; } - emitMockHistory(socket, subId, channelId); + emitMockHistory(socket, subId, channelId, filter); return; } diff --git a/desktop/tests/e2e/virtualization-screenshots.spec.ts b/desktop/tests/e2e/virtualization-screenshots.spec.ts index 09a5e90dd..adef5493c 100644 --- a/desktop/tests/e2e/virtualization-screenshots.spec.ts +++ b/desktop/tests/e2e/virtualization-screenshots.spec.ts @@ -196,4 +196,136 @@ test.describe("list virtualization screenshots", () => { await page.screenshot({ path: `${SHOTS}/06b-sections-after-reorder.png` }); }); + + test("07 — load-older prepend holds the anchored row without jitter or reconcile spin", async ({ + page, + }) => { + // Install once: addInitScript re-runs on every navigation in this page, so + // each page.goto in the loop below re-applies the mock bridge. + await installMockBridge(page); + + // The deep-history channel seeds 600 messages; the initial load windows to + // the newest 200, leaving 400 older behind the until cursor — enough that + // every run lands a genuine prepend. Reads the first row at/below the + // viewport top and returns scrollTop, scrollHeight, and that row's on-screen + // VIEWPORT position in ONE settled snapshot — the position the single-writer + // restore must hold steady across the prepend. + // + // Waits inside the browser for a measurement-settled frame before reading. + // The virtualizer re-windows after a scroll: for a few rAFs the mounted rows + // can all sit above the viewport top (their absolute offsets lag the new + // scrollTop) until the library mounts rows at the current position. That is + // a measurement transient, NOT the scrollTop race — scrollTop is already + // correct on those frames. Reading on such a frame would throw "no row"; + // polling for a settled frame removes the flake without touching any + // race-detection threshold below (scrollTop value + viewportPos stability), + // and snapshots all three fields together so they can't skew across reads. + const sampleAnchor = (timeline: Locator) => + timeline.evaluate(async (scroller) => { + const s = scroller as HTMLElement; + for (let frame = 0; frame < 60; frame += 1) { + const scrollerTop = s.getBoundingClientRect().top; + const row = Array.from( + s.querySelectorAll("[data-message-id]"), + ).find((r) => r.getBoundingClientRect().top - scrollerTop >= 0); + if (row) { + return { + viewportPos: row.getBoundingClientRect().top - scrollerTop, + scrollTop: s.scrollTop, + scrollHeight: s.scrollHeight, + }; + } + await new Promise((resolve) => requestAnimationFrame(resolve)); + } + throw new Error("no anchor row mounted after 60 frames"); + }); + + // Determinism is the bar, not pass-once. The original defect was a RACE: a + // second restore loop (the resize-observer restoring to the pre-fetch + // scrollTop of 0, fired by the load-older spinner's clientHeight shift) + // fought the anchor restore frame-by-frame; last writer won, so the anchor + // held only ~2 of 3 runs and on its losing runs scrollTop collapsed to ~0 + // (view stuck at the top, anchor lost). A single prepend can go green on a + // lucky scheduling order, so this drives the prepend on SIX fresh page loads + // and asserts the anchor holds on every one — a flaky-pass fails the run. + // Fresh navigation each iteration resets the virtualizer's measurement state, + // matching the run-to-run conditions under which the race surfaced. + for (let run = 0; run < 6; run += 1) { + // Force a full document reload each iteration. Navigating straight to the + // same hash route is a same-document hash change, not a reload, so the + // virtualizer + paginated history would carry over and later runs would + // exhaust the older pages — defeating the per-run fresh-prepend premise. + await page.goto("about:blank"); + await page.goto("/#/channels/feedf00d-0000-4000-8000-000000000007"); + const timeline = page.getByTestId("message-timeline"); + await expect(timeline).toBeVisible(); + await expect( + page.locator('[data-message-id^="mock-deep-history-"]').first(), + ).toBeVisible(); + + // Scroll up to mount mid-history rows while staying clear of the load-older + // sentinel zone (trips within 200px of the top), then let the windowed rows + // measure off their 80px estimate so the pre-prepend anchor reading is + // stable. The single trigger is the deliberate scrollTop = 0 below. + await timeline.evaluate((el) => { + el.scrollTop = 4000; + }); + await page.waitForTimeout(300); + await timeline.evaluate((el) => { + el.scrollTop = 4000; + }); + await page.waitForTimeout(150); + const before = await sampleAnchor(timeline); + expect(before.scrollTop).toBeGreaterThan(200); + + // Trigger exactly one prepend. Scrolling to 150 trips the load-older + // sentinel (its rootMargin reaches 200px past the top) with + // previousScrollTopRef pinned near the top — the condition under which the + // resize-observer's competing restore collapsed the anchor pre-fix. After + // the single fetchOlder lands, the anchor restore carries scrollTop deep + // into the content, clear of the 200px sentinel zone, so the observer does + // NOT re-fire: one clean prepend, not the re-trigger storm that scrollTop + // 0 produces (0 keeps the sentinel tripped across every paged window down + // to the small exhaustion-tail page, which legitimately lands the top row + // near the top — masking the hold signal). + await timeline.evaluate((el) => { + el.scrollTop = 150; + }); + + // Anchor-hold gate (the race signal): poll until the restore has carried + // scrollTop deep into the content — past where it sat before the prepend. + // Pre-fix, the competing resize-observer restore (firing on the spinner's + // clientHeight shift, restoring to previousScrollTopRef ~150) won often + // enough that scrollTop stayed pinned near the top; this poll would then + // time out, failing the run. scrollHeight grows several frames BEFORE the + // restore moves scrollTop, so a scrollHeight gate would read mid-cycle + // near the top — the race lives in scrollTop, so the gate watches it. + await expect + .poll(async () => (await sampleAnchor(timeline)).scrollTop, { + timeout: 10_000, + }) + .toBeGreaterThan(before.scrollTop); + + // One settled snapshot for the remaining checks so scrollHeight and + // viewportPos come from the same frame as the held scrollTop: + // (a) the scroller grew by the prepended rows' height (genuine prepend), + // (b) the first-visible row sits where it did before the prepend. + const after = await sampleAnchor(timeline); + expect(after.scrollHeight).toBeGreaterThan(before.scrollHeight + 800); + expect(Math.abs(after.viewportPos - before.viewportPos)).toBeLessThan(120); + + // Reconcile terminates: two equal scrollTop reads 600ms apart prove the + // rAF loop stopped. Under the double-writer bug the library re-scheduled + // one rAF per frame for the full 5s MAX_RECONCILE_MS valve — still churning + // 600ms apart. + const settled1 = await timeline.evaluate((el) => el.scrollTop); + await page.waitForTimeout(600); + const settled2 = await timeline.evaluate((el) => el.scrollTop); + expect(Math.abs(settled1 - settled2)).toBeLessThan(2); + + if (run === 0) { + await page.screenshot({ path: `${SHOTS}/07-load-older-anchor-hold.png` }); + } + } + }); });