diff --git a/src/components/ScrollToTop.restore.test.tsx b/src/components/ScrollToTop.restore.test.tsx new file mode 100644 index 00000000..0d05bf44 --- /dev/null +++ b/src/components/ScrollToTop.restore.test.tsx @@ -0,0 +1,380 @@ +// ABOUTME: Tests scroll restoration when content lays out after the restore attempt +// ABOUTME: divine-web#379 — profile grid reset to top because the page was still short + +import { fireEvent, render, screen } from '@testing-library/react'; +import { Link, MemoryRouter, Route, Routes, useNavigate } from 'react-router-dom'; +import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'; + +function BackButton() { + const navigate = useNavigate(); + return ; +} + +// ScrollToTop keeps saved positions in module scope, so each test needs a fresh +// import or one test's saved position leaks into the next. +async function loadScrollToTop() { + vi.resetModules(); + return (await import('./ScrollToTop')).ScrollToTop; +} + +function makeTestApp(ScrollToTop: React.ComponentType) { + return () => ( + + + + +

Feed

+ Details + + } + /> + +

Details

+ + + } + /> +
+
+ ); +} + +const nextFrame = () => new Promise(resolve => setTimeout(resolve, 32)); + +/** `window.scrollTo` is overloaded; a mock has to handle either call shape. */ +type ScrollToArgs = [x: number, y: number] | [options?: ScrollToOptions]; + +/** The requested offset, from either `scrollTo(x, y)` or `scrollTo({ top })`. */ +function requestedOffset(args: ScrollToArgs): number { + const [first, second] = args; + return Number(typeof first === 'number' ? second : first?.top); +} + +describe('ScrollToTop restoration against late-loading content', () => { + let scrollY = 0; + /** Tallest position the "browser" will accept — grows as content lays out. */ + let maxScroll = 0; + + beforeEach(() => { + scrollY = 0; + maxScroll = 10_000; + Object.defineProperty(window, 'scrollY', { + configurable: true, + get: () => scrollY, + }); + // Real browsers clamp a scroll request to the current document height. An + // infinite grid that hasn't rendered its rows yet is short, so the restore + // lands near the top. + vi.mocked(window.scrollTo).mockImplementation((...args: ScrollToArgs) => { + const landed = Math.min(requestedOffset(args), maxScroll); + if (landed === scrollY) return; + scrollY = landed; + // A programmatic scroll fires a `scroll` event in a real browser exactly + // as a viewer's does. The restore loop's own writes therefore have to be + // distinguishable from the viewer taking the page over; a mock that moved + // the offset silently would let a broken guard pass. + window.dispatchEvent(new Event('scroll')); + }); + }); + + afterEach(() => { + vi.mocked(window.scrollTo).mockReset(); + }); + + it('keeps trying until the grown page can hold the saved position', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + expect(screen.getByRole('heading', { name: 'Details' })).toBeInTheDocument(); + + // Coming back, the profile grid has not rendered its rows yet. + scrollY = 0; + maxScroll = 150; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + expect(screen.getByRole('heading', { name: 'Feed' })).toBeInTheDocument(); + + // The first attempt is clamped — this is the reported bug. + expect(window.scrollY).toBe(150); + + // Cached pages render and the document grows. + maxScroll = 10_000; + await nextFrame(); + + expect(window.scrollY).toBe(1800); + }); + + // `html { scroll-behavior: smooth }` applies app-wide, and the positional + // `scrollTo(x, y)` form scrolls with behavior "auto", which resolves to it. + // That animates the restore, so every frame reads short of the target and the + // loop chases its own animation instead of the page height — and stopping the + // loop does not stop the animation, so the page keeps travelling after the + // viewer takes over. The restore must opt out of it explicitly. + it('restores without starting the page-level smooth scroll', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 1800, behavior: 'instant' }); + }); + + it('stops fighting the user if they scroll during restoration', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + maxScroll = 150; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + expect(window.scrollY).toBe(150); + + // User grabs the page before the grid finishes loading. + fireEvent.wheel(window); + maxScroll = 10_000; + scrollY = 300; + await nextFrame(); + + expect(window.scrollY).toBe(300); + }); + + // Grabbing the scrollbar fires neither wheel nor touchstart nor keydown, and + // it is the natural way out of a page the retry loop cannot satisfy. + it('stops fighting the user if they drag the scrollbar during restoration', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + maxScroll = 150; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + expect(window.scrollY).toBe(150); + + fireEvent.mouseDown(window); + maxScroll = 10_000; + scrollY = 300; + await nextFrame(); + + expect(window.scrollY).toBe(300); + }); + + // An interrupted restore is holding a clamped position, not a real one. + // Saving it would overwrite the offset the restore was chasing and walk the + // feed toward the top on every interrupted back-navigation. + it('does not overwrite the saved position when a restore is interrupted', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + // Back, but the grid has not laid out, so the restore clamps to 150. + scrollY = 0; + maxScroll = 150; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + expect(window.scrollY).toBe(150); + + // Navigate away again before the retry loop can land the position. + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + // Back once more, this time with the page fully laid out. The original + // 1800 must have survived the interrupted attempt. + scrollY = 0; + maxScroll = 10_000; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + await nextFrame(); + + expect(window.scrollY).toBe(1800); + }); + + // Cancelling the retry loop is not the same as moving the page. A click hands + // control back without scrolling anywhere, so what is on screen is still the + // clamped offset the loop wrote — persisting it loses the one being chased. + // The case above passes either way because `fireEvent.click` dispatches no + // `mousedown`, and the scrollbar-drag case sets a new `scrollY` afterwards, + // which is the branch where the viewer really did move. + it('does not overwrite the saved position when a click cancels a restore', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + maxScroll = 150; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + expect(window.scrollY).toBe(150); + + // The viewer clicks a video while the grid is still filling in. The + // viewport never moved off the clamped 150. + fireEvent.mouseDown(window); + maxScroll = 10_000; + await nextFrame(); + expect(window.scrollY).toBe(150); + + // Follow the click through, then come back to a fully laid out page. + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + scrollY = 0; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + await nextFrame(); + + expect(window.scrollY).toBe(1800); + }); + + // The mirror of the two cases above. Not persisting an interrupted restore + // must not cost the viewer a position they really did choose — and the + // offset most at risk is the one the interrupted loop happened to leave + // behind, because a clamped restore on a grid that has not laid out leaves + // exactly `0`. Asking whether the loop stopped, or comparing the final offset + // against what the loop last wrote, both read this as "not the viewer's". + it('persists the top of the feed after an interrupted restore', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + // Back onto a grid with no rows yet: the restore is clamped all the way to + // the top, which is the offset the loop is now holding. + scrollY = 0; + maxScroll = 0; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + expect(window.scrollY).toBe(0); + + // The viewer clicks, taking control without moving the page. + fireEvent.mouseDown(window); + maxScroll = 10_000; + await nextFrame(); + + // They read down, then come back to the top and open a video from there. + scrollY = 3000; + fireEvent.scroll(window); + scrollY = 0; + fireEvent.scroll(window); + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + // The top is where they left the feed, so that is where they come back to. + scrollY = 900; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + await nextFrame(); + + expect(window.scrollY).toBe(0); + }); + + it('persists a position the viewer scrolls to after a restore lands', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + await nextFrame(); + expect(window.scrollY).toBe(1800); + + // The restore is done; the viewer reads on and leaves from 4000. + fireEvent.wheel(window); + scrollY = 4000; + fireEvent.scroll(window); + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + await nextFrame(); + + expect(window.scrollY).toBe(4000); + }); + + // Not every scroll is the viewer's. Scroll anchoring, the browser clamping + // `scrollY` when the document shrinks, and focus-driven scrolling on mount all + // fire one — and a restore is in flight precisely while content is still + // laying out, which is when those are most likely. Treating a bare `scroll` as + // intent would let a stray event hand the clamped offset back into storage. + it('ignores a scroll the viewer did not cause', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + await nextFrame(); + expect(window.scrollY).toBe(1800); + + // The page reflows and the browser moves the viewport on its own. No wheel, + // no touch, no key, no mousedown. + scrollY = 300; + fireEvent.scroll(window); + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + await nextFrame(); + + expect(window.scrollY).toBe(1800); + }); + + // `pagehide` fires on tab and app switches and on entry to the back-forward + // cache. The module scope survives bfcache, so a save from there outlives the + // event exactly as an in-app one does and has to clear the same guard. + // + // Only the negative direction is testable here: `pagehide` shares + // `isViewerChosen` with the layout-effect cleanup, and any in-app navigation + // that would let a test read the stored value runs that cleanup afterwards + // and overwrites whatever `pagehide` wrote. The positive direction of the + // guard is pinned by the two tests above. + it('does not let pagehide persist an in-flight restore', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + + scrollY = 1800; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + scrollY = 0; + maxScroll = 150; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + expect(window.scrollY).toBe(150); + + // The viewer switches tabs while the grid is still filling in. + fireEvent(window, new Event('pagehide')); + + // Back to a laid-out page: the clamped 150 must not have replaced 1800. + maxScroll = 10_000; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + scrollY = 0; + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + await nextFrame(); + + expect(window.scrollY).toBe(1800); + }); + + it('still lands at the top on forward navigation', async () => { + const TestApp = makeTestApp(await loadScrollToTop()); + render(); + expect(window.scrollY).toBe(0); + + scrollY = 900; + fireEvent.click(screen.getByRole('link', { name: 'Details' })); + + expect(window.scrollY).toBe(0); + await nextFrame(); + expect(window.scrollY).toBe(0); + }); +}); diff --git a/src/components/ScrollToTop.test.tsx b/src/components/ScrollToTop.test.tsx index dfcbfb25..019ef3d4 100644 --- a/src/components/ScrollToTop.test.tsx +++ b/src/components/ScrollToTop.test.tsx @@ -46,9 +46,13 @@ describe('ScrollToTop', () => { configurable: true, get: () => scrollY, }); - vi.mocked(window.scrollTo).mockImplementation((_x, y) => { - scrollY = Number(y); - }); + // `window.scrollTo` is overloaded; the restore uses the options form. + vi.mocked(window.scrollTo).mockImplementation( + (...args: [x: number, y: number] | [options?: ScrollToOptions]) => { + const [first, second] = args; + scrollY = Number(typeof first === 'number' ? second : first?.top); + }, + ); }); it('scrolls to the top on forward (PUSH) navigation, even with a saved position', () => { @@ -84,6 +88,8 @@ describe('ScrollToTop', () => { fireEvent.click(screen.getByRole('button', { name: 'Back' })); expect(screen.getByRole('heading', { name: 'Feed' })).toBeInTheDocument(); - expect(window.scrollTo).toHaveBeenLastCalledWith(0, 420); + // The restore opts out of `html { scroll-behavior: smooth }`; an animated + // restore would read short of its target on every frame. + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 420, behavior: 'instant' }); }); }); diff --git a/src/components/ScrollToTop.tsx b/src/components/ScrollToTop.tsx index 3be31957..53be2f34 100644 --- a/src/components/ScrollToTop.tsx +++ b/src/components/ScrollToTop.tsx @@ -3,15 +3,133 @@ import { useLocation, useNavigationType } from 'react-router-dom'; const scrollPositions = new Map(); +/** How long to keep chasing a saved position while content loads in. */ +const RESTORE_TIMEOUT_MS = 3000; + function getScrollKey(pathname: string, search: string) { return `${pathname}${search}`; } +/** + * Scroll to `target`, retrying while the document is too short to honour it. + * + * Feeds restore into a page whose rows have not laid out yet, so a single + * `scrollTo` gets clamped to the current document height and the viewer lands + * near the top. Retrying across frames lets the position land once the cached + * pages render. Returns a function that stops the attempt. + */ +interface ScrollRestoration { + /** + * True once the viewer has moved the page themselves. Only a position the + * viewer chose is worth persisting. + */ + isViewerChosen: () => boolean; + stop: () => void; +} + +function restoreScrollPosition(target: number): ScrollRestoration { + if (target <= 0) { + // Nothing to chase, so wherever the viewer ends up on this route is theirs. + window.scrollTo(0, 0); + return { stop: () => {}, isViewerChosen: () => true }; + } + + // The offset this loop last left on the page, read back after the write so it + // holds what the browser accepted rather than what we asked for. + let written = 0; + let frame: number | null = null; + let stopped = false; + let viewerMoved = false; + let viewerTookOver = false; + const deadline = Date.now() + RESTORE_TIMEOUT_MS; + + // A scroll only counts once the viewer has taken the page with a real input, + // and only when it lands somewhere other than the offset the loop last wrote. + // + // The input requirement matters because plenty of scrolls are not the + // viewer's: scroll anchoring (`overflow-anchor: auto` is the default), the + // browser clamping `scrollY` when the document shrinks, and focus-driven + // scrolling on mount all fire one. A restore times out precisely *because* + // content is still laying out, which is when those are most likely — so + // without the gate, one stray event would let an interrupted restore's + // clamped offset overwrite the position it was chasing. + // + // The offset comparison then separates "the viewer settled here" from "the + // loop was interrupted here" even when the two end up equal: a viewer who + // reads down and comes back to the top passed through other offsets on the + // way, and each one fired this. Reading the offsets equal at teardown cannot + // tell those apart. The loop's own writes never latch — they land on + // `written`, and by the time the listener exists the loop has stopped. + const noteViewerScroll = () => { + if (viewerTookOver && window.scrollY !== written) viewerMoved = true; + }; + + // Once the viewer takes over, stop dragging them back to where they were. + // `mousedown` covers grabbing the scrollbar, which fires none of the others + // and is exactly how someone escapes a page the loop cannot satisfy. + const inputEvents = ['wheel', 'touchstart', 'keydown', 'mousedown'] as const; + + const noteViewerInput = () => { + viewerTookOver = true; + handOver(); + }; + + // Stops the loop writing. The input listeners outlive it: handover can also + // come from reaching the target or from the deadline, and a viewer who takes + // the page after either of those still needs to be recognised. + function handOver() { + if (stopped) return; + stopped = true; + if (frame !== null) { + window.cancelAnimationFrame(frame); + frame = null; + } + window.addEventListener('scroll', noteViewerScroll, { passive: true }); + } + + const stop = () => { + handOver(); + window.removeEventListener('scroll', noteViewerScroll); + for (const event of inputEvents) window.removeEventListener(event, noteViewerInput); + }; + + for (const event of inputEvents) { + window.addEventListener(event, noteViewerInput, { passive: true }); + } + + const attempt = () => { + if (stopped) return; + // Options form, not `scrollTo(0, target)`. The positional form scrolls with + // behavior "auto", which resolves to the root's computed `scroll-behavior` — + // and that is `smooth` app-wide (src/index.css:233). An animated restore + // reads short of its target on every frame, so the loop ends up chasing its + // own animation rather than the page's height: measured in Chromium at 154 + // frames over 1.3s on a page already tall enough to honour the offset in + // one. Worse, cancelling the loop does not cancel the animation, so the + // page kept travelling to the target after the viewer had taken over, + // defeating the handover listeners above. + window.scrollTo({ top: target, behavior: 'instant' }); + written = window.scrollY; + + if (written >= target || Date.now() > deadline) { + handOver(); + return; + } + + frame = window.requestAnimationFrame(attempt); + }; + + attempt(); + + return { stop, isViewerChosen: () => viewerMoved }; +} + export function ScrollToTop() { const { pathname, search, hash } = useLocation(); const navigationType = useNavigationType(); const scrollKey = getScrollKey(pathname, search); const timeoutRef = useRef(null); + const restorationRef = useRef(null); useEffect(() => { if ('scrollRestoration' in window.history) { @@ -26,6 +144,13 @@ export function ScrollToTop() { useEffect(() => { const saveCurrentPosition = () => { + // Same guard as the layout-effect cleanup below, for the same reason. + // `pagehide` fires on tab and app switches and on entry to the + // back-forward cache, where this module scope — and so `scrollPositions` + // — survives, so a save here outlives the event just as an in-app one + // does. Writing an in-flight restore's clamped offset would overwrite the + // position that restore is still chasing. + if (restorationRef.current && !restorationRef.current.isViewerChosen()) return; scrollPositions.set(scrollKey, window.scrollY); }; @@ -68,10 +193,25 @@ export function ScrollToTop() { // so footer/sidebar/nav links always land at the top of the destination. const savedPosition = navigationType === 'POP' ? (scrollPositions.get(scrollKey) ?? 0) : 0; - window.scrollTo(0, savedPosition); + const restoration = restoreScrollPosition(savedPosition); + restorationRef.current = restoration; return () => { - scrollPositions.set(scrollKey, window.scrollY); + // Only persist an offset the viewer chose. A restore that never reached + // its target is holding a value clamped by a page that had not finished + // laying out; saving that would overwrite the offset the restore was + // chasing and walk the feed toward the top on every interrupted + // back-navigation. Cancelling the loop is not the same as moving the + // page — a click or a keystroke hands control back without scrolling + // anywhere — so "did the loop stop" cannot stand in for "is this the + // viewer's position". Ask whether the viewer actually scrolled instead. + const viewerChose = restoration.isViewerChosen(); + restoration.stop(); + restorationRef.current = null; + + if (viewerChose) { + scrollPositions.set(scrollKey, window.scrollY); + } }; }, [scrollKey, hash, navigationType]); diff --git a/src/hooks/useInfiniteVideosFunnelcake.ts b/src/hooks/useInfiniteVideosFunnelcake.ts index b2296077..b665f68d 100644 --- a/src/hooks/useInfiniteVideosFunnelcake.ts +++ b/src/hooks/useInfiniteVideosFunnelcake.ts @@ -38,6 +38,13 @@ interface UseInfiniteVideosFunnelcakeOptions { interface FunnelcakeVideoPage { videos: ParsedVideoData[]; + /** + * Rows this page fetched, as the API returned them. Required, not optional: + * infinite scroll paginates on this (divine-web#380), and `countFetchedVideos` + * falls back to `videos.length` when it is absent, so an optional field would + * let a dropped assignment revert the count to the parsed length silently. + */ + fetchedRows: number; nextCursor: number | undefined; offset?: number; /** Opaque cursor string for recommendations pagination */ @@ -269,6 +276,7 @@ export function useInfiniteVideosFunnelcake({ }); return { videos: page.videos, + fetchedRows: page.fetchedRows, nextCursor: page.nextCursor, offset: page.offset, recommendationsCursor: feedType === 'recommendations' ? page.rawCursor : undefined, @@ -341,7 +349,7 @@ export function useInfiniteVideosFunnelcake({ case 'home': if (!user?.pubkey) { debugLog('[useInfiniteVideosFunnelcake] No user logged in for home feed'); - return { videos: [], nextCursor: undefined }; + return { videos: [], fetchedRows: 0, nextCursor: undefined }; } response = await fetchUserFeed(effectiveApiUrl, { ...options, @@ -352,7 +360,7 @@ export function useInfiniteVideosFunnelcake({ case 'recommendations': { if (!user?.pubkey) { debugLog('[useInfiniteVideosFunnelcake] No user logged in for recommendations feed'); - return { videos: [], nextCursor: undefined }; + return { videos: [], fetchedRows: 0, nextCursor: undefined }; } if (isPopularFallback) { responseMode = 'popular'; @@ -464,6 +472,7 @@ export function useInfiniteVideosFunnelcake({ return { videos: enrichedVideos, + fetchedRows: page.fetchedRows, nextCursor: page.nextCursor, offset: page.offset, recommendationsCursor: responseMode === 'recommendations' ? page.rawCursor : undefined, diff --git a/src/hooks/useVideoProvider.fetchedCount.test.ts b/src/hooks/useVideoProvider.fetchedCount.test.ts new file mode 100644 index 00000000..b4692272 --- /dev/null +++ b/src/hooks/useVideoProvider.fetchedCount.test.ts @@ -0,0 +1,124 @@ +// ABOUTME: Tests that useVideoProvider reports a fetched-row count from unfiltered pages +// ABOUTME: divine-web#380 — infinite scroll stalls when the rendered length stops growing + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { renderHook } from '@testing-library/react'; +import { useVideoProvider } from './useVideoProvider'; + +const BLOCKED_AUTHOR = 'b'.repeat(64); +const OK_AUTHOR = 'a'.repeat(64); + +let mockBlocklist: ReadonlySet = new Set(); + +function makeVideo(pubkey: string, id: string) { + return { + id, + pubkey, + kind: 34236, + createdAt: 1700000000, + content: '', + videoUrl: `https://cdn.example/${id}.mp4`, + hashtags: [], + vineId: id, + reposts: [], + }; +} + +let funnelcakeData: { pages: Array<{ videos: ReturnType[]; nextCursor: undefined }>; pageParams: unknown[] }; + +vi.mock('@/hooks/useFeedBlocklist', () => ({ + useFeedBlocklist: () => mockBlocklist, +})); + +vi.mock('@/hooks/useInfiniteVideosFunnelcake', () => ({ + useInfiniteVideosFunnelcake: () => ({ + data: funnelcakeData, + fetchNextPage: vi.fn(), + hasNextPage: true, + isLoading: false, + error: null, + refetch: vi.fn(), + }), +})); + +vi.mock('@/hooks/useInfiniteVideos', () => ({ + useInfiniteVideos: () => ({ + data: undefined, + fetchNextPage: vi.fn(), + hasNextPage: false, + isLoading: false, + error: null, + refetch: vi.fn(), + }), +})); + +vi.mock('@/hooks/useFeaturedTabVideos', () => ({ + useFeaturedTabVideos: () => ({ + data: undefined, + fetchNextPage: vi.fn(), + hasNextPage: false, + isLoading: false, + error: null, + refetch: vi.fn(), + }), +})); + +vi.mock('@/hooks/useAppContext', () => ({ + useAppContext: () => ({ config: { relayUrl: 'wss://relay.divine.video' } }), +})); + +vi.mock('@/hooks/useRelayCapabilities', () => ({ + useResolvedRelayCapabilities: () => ({ supportsVideoSorts: true }), +})); + +describe('useVideoProvider fetchedCount', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockBlocklist = new Set(); + funnelcakeData = { + pages: [{ videos: [makeVideo(OK_AUTHOR, 'ok-1'), makeVideo(OK_AUTHOR, 'ok-2')], nextCursor: undefined }], + pageParams: [undefined], + }; + }); + + it('counts every fetched row', () => { + const { result } = renderHook(() => + useVideoProvider({ feedType: 'profile', pubkey: OK_AUTHOR }) + ); + expect(result.current.fetchedCount).toBe(2); + }); + + it('is 0 before the first page arrives', () => { + funnelcakeData = { pages: [], pageParams: [] }; + const { result } = renderHook(() => + useVideoProvider({ feedType: 'profile', pubkey: OK_AUTHOR }) + ); + expect(result.current.fetchedCount).toBe(0); + }); + + // The stall in divine-web#380: a fetched page whose authors are all blocked + // disappears from `data` entirely. If the scroll trigger keyed off the + // rendered length it would never re-arm and the grid would stop paginating. + it('grows when a fetched page is entirely blocked authors', () => { + mockBlocklist = new Set([BLOCKED_AUTHOR]); + const { result, rerender } = renderHook(() => + useVideoProvider({ feedType: 'profile', pubkey: OK_AUTHOR }) + ); + const before = result.current.fetchedCount; + const renderedBefore = result.current.data?.pages.flatMap(p => p.videos).length ?? 0; + + funnelcakeData = { + pages: [ + ...funnelcakeData.pages, + { videos: [makeVideo(BLOCKED_AUTHOR, 'blocked-1'), makeVideo(BLOCKED_AUTHOR, 'blocked-2')], nextCursor: undefined }, + ], + pageParams: [undefined, undefined], + }; + rerender(); + + const renderedAfter = result.current.data?.pages.flatMap(p => p.videos).length ?? 0; + expect(renderedAfter).toBe(renderedBefore); // nothing new is displayed + expect(result.current.fetchedCount).toBeGreaterThan(before); // but the fetch counts + expect(result.current.fetchedCount).toBe(4); + }); +}); diff --git a/src/hooks/useVideoProvider.ts b/src/hooks/useVideoProvider.ts index 4cbfbcab..37aaa2ac 100644 --- a/src/hooks/useVideoProvider.ts +++ b/src/hooks/useVideoProvider.ts @@ -11,6 +11,7 @@ import { useFeaturedTabVideos } from '@/hooks/useFeaturedTabVideos'; import { useFeedBlocklist } from '@/hooks/useFeedBlocklist'; import { FEED_PAGE_SIZE } from '@/config/feed'; import { filterBlockedVideoPages } from '@/lib/blocklistFilter'; +import { countFetchedVideos } from '@/lib/feedPagination'; import { hasFunnelcake, getFunnelcakeUrl } from '@/config/relays'; import { debugLog } from '@/lib/debug'; import type { RelayCapabilities } from '@/lib/relayCapabilities'; @@ -41,6 +42,12 @@ interface VideoProviderResult { isLoading: boolean; error: Error | null; refetch: () => void; + /** + * Rows fetched across every loaded page, as the API returned them — before + * transform failures, dedup, and block filtering take any of them out. + * Infinite-scroll triggers must key off this rather than the rendered length, + * which can stay flat when a page collapses and would stall pagination. + */ fetchedCount: number; // Additional metadata dataSource: 'funnelcake' | 'websocket'; @@ -302,20 +309,21 @@ export function useVideoProvider({ // unfiltered pages inside the underlying query hooks. const blockedPubkeys = useFeedBlocklist(); const rawData = activeQuery.data; - const fetchedCount = rawData?.pages.reduce((sum, page) => sum + page.videos.length, 0) ?? 0; const filteredData = useMemo( () => filterBlockedVideoPages(rawData, blockedPubkeys), [rawData, blockedPubkeys] ); + // Counted from the unfiltered pages on purpose (divine-web#380). + const fetchedCount = useMemo(() => countFetchedVideos(rawData?.pages), [rawData]); return { data: filteredData, + fetchedCount, fetchNextPage: activeQuery.fetchNextPage, hasNextPage: activeQuery.hasNextPage, isLoading: activeQuery.isLoading, error: activeQuery.error, refetch: activeQuery.refetch, - fetchedCount, dataSource: activeDataSource, apiUrl: shouldUseFunnelcake ? decision.apiUrl : undefined, }; diff --git a/src/lib/feedPagination.test.ts b/src/lib/feedPagination.test.ts new file mode 100644 index 00000000..f4d01874 --- /dev/null +++ b/src/lib/feedPagination.test.ts @@ -0,0 +1,74 @@ +// ABOUTME: Tests for infinite-feed page accounting helpers +// ABOUTME: Covers the fetched-item count that keeps infinite scroll re-arming + +import { describe, it, expect } from 'vitest'; +import { countFetchedVideos } from './feedPagination'; + +describe('countFetchedVideos', () => { + it('returns 0 when no pages have been fetched', () => { + expect(countFetchedVideos(undefined)).toBe(0); + expect(countFetchedVideos([])).toBe(0); + }); + + it('sums videos across every fetched page', () => { + expect(countFetchedVideos([ + { videos: [{ id: 'a' }, { id: 'b' }] }, + { videos: [{ id: 'c' }] }, + ])).toBe(3); + }); + + it('tolerates pages that carry no videos array', () => { + expect(countFetchedVideos([ + { videos: [{ id: 'a' }] }, + {} as { videos: unknown[] }, + ])).toBe(1); + }); + + // divine-web#380: react-infinite-scroll-component only re-arms its internal + // `actionTriggered` guard when `dataLength` changes. A page whose rows all + // collapse under addressable-key dedup (or per-viewer block filtering) adds + // nothing to the rendered list, so a deduped length would stall the feed + // permanently even though `hasNextPage` is still true. The fetched count has + // to keep climbing. + it('keeps climbing when a whole page dedupes away to nothing', () => { + const duplicateRow = { id: 'a' }; + const firstPageOnly = [{ videos: [duplicateRow, { id: 'b' }] }]; + const withCollapsingPage = [ + ...firstPageOnly, + { videos: [duplicateRow, duplicateRow] }, + ]; + + expect(countFetchedVideos(firstPageOnly)).toBe(2); + expect(countFetchedVideos(withCollapsingPage)).toBe(4); + expect(countFetchedVideos(withCollapsingPage)) + .toBeGreaterThan(countFetchedVideos(firstPageOnly)); + }); + + // A page's `videos` has already lost rows to within-page dedup and to any + // transform drop, so counting it makes pagination depend on how much of the + // page survived — the same mistake as counting the rendered list, one layer + // down. `fetchedRows` is the count the API actually returned. + it('prefers the raw fetched-row count over the parsed length', () => { + expect(countFetchedVideos([ + { fetchedRows: 20, videos: [{ id: 'a' }] }, + { fetchedRows: 20, videos: [] }, + ])).toBe(40); + }); + + it('falls back to the parsed length for pages without a raw count', () => { + expect(countFetchedVideos([ + { fetchedRows: 20, videos: [{ id: 'a' }] }, + { videos: [{ id: 'b' }, { id: 'c' }] }, + ])).toBe(22); + }); + + // Callers must feed this the *unfiltered* query pages. Per-viewer block/mute + // filtering runs before the page reaches the component, so counting filtered + // pages would stall again the moment a page is entirely blocked authors. + it('reports 0 for a page that was emptied before counting', () => { + expect(countFetchedVideos([ + { videos: [{ id: 'a' }] }, + { videos: [] }, + ])).toBe(1); + }); +}); diff --git a/src/lib/feedPagination.ts b/src/lib/feedPagination.ts new file mode 100644 index 00000000..b80b3732 --- /dev/null +++ b/src/lib/feedPagination.ts @@ -0,0 +1,36 @@ +// ABOUTME: Page accounting helpers for infinite video feeds +// ABOUTME: Counts fetched rows so infinite scroll re-arms on every loaded page + +interface CountablePage { + /** Rows the API returned, before any were dropped. Preferred when present. */ + fetchedRows?: number; + videos?: readonly unknown[]; +} + +/** + * Total rows fetched across every loaded page. + * + * `react-infinite-scroll-component` re-arms its internal trigger only when + * `dataLength` changes, so feeds must report a count that grows with each + * fetched page rather than the length of the rendered list. Rendered lists are + * deduplicated by addressable key and filtered per viewer, either of which can + * collapse a whole page to nothing and stall the feed permanently. + * + * Pass the unfiltered query pages: block/mute filtering happens downstream, and + * counting already-filtered pages reintroduces the stall. + * + * `fetchedRows` is preferred over `videos.length` because a page's `videos` has + * already lost the rows that duplicated within that page and any the transform + * had to drop. Keying pagination on what survived that is the same mistake as + * keying it on the rendered list, one layer down; the count the API returned + * cannot collapse. Pages without it (the websocket path) fall back to the + * parsed length. + */ +export function countFetchedVideos(pages: readonly CountablePage[] | undefined): number { + return ( + pages?.reduce( + (total, page) => total + (page.fetchedRows ?? page.videos?.length ?? 0), + 0 + ) ?? 0 + ); +} diff --git a/src/lib/funnelcakeTransform.test.ts b/src/lib/funnelcakeTransform.test.ts index 6f01affd..eac535ba 100644 --- a/src/lib/funnelcakeTransform.test.ts +++ b/src/lib/funnelcakeTransform.test.ts @@ -574,6 +574,29 @@ describe('transformToVideoPage', () => { }); }); + // divine-web#380: infinite scroll re-arms only when its `dataLength` moves, + // and `videos` has already lost the rows that duplicated within the page and + // any that `transformFunnelcakeResponse` had to drop. The count the feed + // paginates on should not depend on how many rows survived that, so the row + // count the API returned travels alongside the parsed list. + describe('fetchedRows', () => { + it('reports the rows the API returned, not the parsed ones', () => { + const duplicate = makeRawVideo(); + const page = transformToVideoPage(makeResponse({ videos: [duplicate, duplicate] })); + + expect(page.videos).toHaveLength(1); + expect(page.fetchedRows).toBe(2); + }); + + it('counts bare edge-injected arrays too', () => { + expect(transformToVideoPage([makeRawVideo()]).fetchedRows).toBe(1); + }); + + it('is 0 when the response carries no videos array', () => { + expect(transformToVideoPage(makeResponse({ videos: undefined })).fetchedRows).toBe(0); + }); + }); + it('stops pagination when has_more is false', () => { const page = transformToVideoPage(makeResponse({ has_more: false })); expect(page.hasMore).toBe(false); diff --git a/src/lib/funnelcakeTransform.ts b/src/lib/funnelcakeTransform.ts index ebb06efb..a0581611 100644 --- a/src/lib/funnelcakeTransform.ts +++ b/src/lib/funnelcakeTransform.ts @@ -309,6 +309,14 @@ export function transformToVideoPage( cursorType: 'timestamp' | 'offset' | 'cursor' = 'timestamp' ): { videos: ParsedVideoData[]; + /** + * Rows the API returned for this page, before any of them were dropped. + * + * `videos` is what survived within-page dedup and any transform drop, so it + * under-reports what the page carried. Infinite-scroll accounting needs a + * count that tracks fetching rather than survival (divine-web#380). + */ + fetchedRows: number; nextCursor: number | undefined; offset?: number; /** Raw opaque cursor string for cursor-based pagination (recommendations) */ @@ -316,6 +324,8 @@ export function transformToVideoPage( hasMore: boolean; } { const videos = transformFunnelcakeResponse(response); + const rawRows = Array.isArray(response) ? response : response.videos; + const fetchedRows = Array.isArray(rawRows) ? rawRows.length : 0; // Parse next cursor based on pagination type let nextCursor: number | undefined; @@ -343,6 +353,7 @@ export function transformToVideoPage( return { videos, + fetchedRows, nextCursor, offset, rawCursor, diff --git a/src/pages/ProfilePage.infiniteScroll.test.tsx b/src/pages/ProfilePage.infiniteScroll.test.tsx new file mode 100644 index 00000000..94dcd89a --- /dev/null +++ b/src/pages/ProfilePage.infiniteScroll.test.tsx @@ -0,0 +1,147 @@ +// ABOUTME: Tests that the profile grid feeds infinite scroll a fetched-row count +// ABOUTME: divine-web#380 — a page that dedupes away must still re-arm the trigger + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { TestApp } from '@/test/TestApp'; + +const PUBKEY = 'a'.repeat(64); + +// Two rows that collapse to one under `pubkey:kind:d-tag` dedup, plus one unique +// row. Rendered length is 2; fetched length is 3. +function makeVideo(vineId: string, id: string) { + return { + id, + pubkey: PUBKEY, + kind: 34236, + createdAt: 1700000000, + content: '', + videoUrl: `https://cdn.example/${id}.mp4`, + thumbnailUrl: undefined, + title: vineId, + hashtags: [], + vineId, + reposts: [], + }; +} + +const videoProviderResult = { + data: { + pages: [ + { videos: [makeVideo('v1', 'e1'), makeVideo('v2', 'e2')], nextCursor: undefined }, + // Whole page is a repeat of v1 — dedup drops it entirely. + { videos: [makeVideo('v1', 'e3')], nextCursor: undefined }, + ], + pageParams: [undefined, undefined], + }, + fetchNextPage: vi.fn(), + hasNextPage: true, + isLoading: false, + error: null, + refetch: vi.fn(), + fetchedCount: 3, + dataSource: 'funnelcake' as const, + apiUrl: 'https://api.example', +}; + +const infiniteScrollProps: Array> = []; + +vi.mock('react-infinite-scroll-component', () => ({ + default: (props: Record) => { + infiniteScrollProps.push(props); + return
{props.children as React.ReactNode}
; + }, +})); + +vi.mock('@/hooks/useVideoProvider', () => ({ + useVideoProvider: () => videoProviderResult, +})); + +vi.mock('@/hooks/useFunnelcakeProfile', () => ({ + useFunnelcakeProfile: () => ({ data: { video_count: 3, name: 'tester' }, isLoading: false }), +})); + +vi.mock('@/hooks/useAuthor', () => ({ + useAuthor: () => ({ data: { metadata: { name: 'tester' } }, isLoading: false }), +})); + +vi.mock('@/hooks/useProfileJoinedDate', () => ({ + useProfileJoinedDate: () => ({ data: null, isLoading: false }), +})); + +vi.mock('@/hooks/useClassicVineArchiveStats', () => ({ + useClassicVineArchiveStats: () => ({ data: undefined, isLoading: false }), +})); + +vi.mock('@/hooks/useNip05Validation', () => ({ + useNip05Validation: () => ({ isValid: false }), +})); + +vi.mock('@/hooks/useRssFeedAvailable', () => ({ + useRssFeedAvailable: () => ({ data: false }), +})); + +vi.mock('@/hooks/useResolveSubdomainPubkey', () => ({ + useResolveSubdomainPubkey: () => ({ pubkey: undefined, isLoading: false }), +})); + +vi.mock('@/hooks/useNip05Pubkey', () => ({ + useNip05Pubkey: () => ({ data: undefined, isLoading: false }), +})); + +vi.mock('@/hooks/useSubdomainUser', () => ({ + getSubdomainUser: () => null, +})); + +// Everything below is chrome around the grid. Stubbing it keeps this test on +// the scroll wiring instead of the whole profile page's data fetching. +vi.mock('@/components/ProfileHeader', () => ({ + ProfileHeader: () =>
, +})); + +vi.mock('@/components/PinnedVideosSection', () => ({ + PinnedVideosSection: () => null, +})); + +vi.mock('@/components/ProfileListsSection', () => ({ + ProfileListsSection: () => null, +})); + +vi.mock('@/components/VideoGrid', () => ({ + VideoGrid: ({ videos }: { videos: unknown[] }) => ( +
+ ), +})); + +vi.mock('@/components/VideoFeed', () => ({ + VideoFeed: () => null, +})); + +describe('ProfilePage infinite scroll', () => { + beforeEach(() => { + infiniteScrollProps.length = 0; + vi.clearAllMocks(); + }); + + it('passes the fetched row count as dataLength, not the deduplicated length', async () => { + const { ProfilePage } = await import('./ProfilePage'); + + render( + + + + ); + + const props = infiniteScrollProps.at(-1); + expect(props).toBeDefined(); + // Rendered grid shows 2 unique videos; 3 rows were fetched. Using the + // rendered length here is what stalls the feed, because the second page + // leaves it unchanged and the scroll trigger never re-arms. + expect(props?.dataLength).toBe(3); + expect(props?.hasMore).toBe(true); + // ...while the grid itself still renders only the deduplicated videos. + expect(screen.getByTestId('video-grid')).toHaveAttribute('data-count', '2'); + // Rendering the whole page pulls in a large module graph; the default 5s + // budget is tight for it when the suite runs in parallel. + }, 20_000); +}); diff --git a/src/pages/ProfilePage.tsx b/src/pages/ProfilePage.tsx index b70c2298..80c24b84 100644 --- a/src/pages/ProfilePage.tsx +++ b/src/pages/ProfilePage.tsx @@ -512,6 +512,9 @@ export function ProfilePage({ pubkeyOverride }: { pubkeyOverride?: string } = {} ) : viewMode === 'grid' ? (