Fix pagination edge cases

src/lib/api/feed/custom.test.ts

@@ -0,0 +1,45 @@
+import {type AtpAgent} from '@atproto/api'
+
+import {CustomFeedAPI} from './custom'
+
+jest.mock('@atproto/api', () => ({
+  AtpAgent: {appLabelers: []},
+  jsonStringToLex: (value: string) => JSON.parse(value),
+}))
+
+jest.mock('#/state/preferences/languages', () => ({
+  getAppLanguageAsContentLanguage: () => '',
+  getContentLanguages: () => [],
+}))
+
+jest.mock('./utils', () => ({
+  createBskyTopicsHeader: () => ({}),
+  isBlueskyOwnedFeed: () => false,
+}))
+
+describe('CustomFeedAPI', () => {
+  it('preserves the cursor from an empty logged-out fallback page', async () => {
+    const originalFetch = global.fetch
+    const fetchMock: jest.MockedFunction<typeof fetch> = jest
+      .fn()
+      .mockResolvedValueOnce(new Response(JSON.stringify({feed: []})))
+      .mockResolvedValueOnce(
+        new Response(JSON.stringify({feed: [], cursor: 'next'})),
+      )
+    global.fetch = fetchMock
+    const api = new CustomFeedAPI({
+      agent: {did: undefined} as unknown as AtpAgent,
+      feedParams: {feed: 'at://did:example:feed/app.bsky.feed.generator/test'},
+    })
+
+    try {
+      await expect(api.fetch({cursor: undefined, limit: 10})).resolves.toEqual({
+        cursor: 'next',
+        feed: [],
+      })
+      expect(fetchMock).toHaveBeenCalledTimes(2)
+    } finally {
+      global.fetch = originalFetch
+    }
+  })
+})

src/lib/api/feed/custom.ts

@@ -148,7 +148,7 @@ async function loggedOutFetch({
   data = res.ok
     ? (jsonStringToLex(await res.text()) as GetCustomFeed.OutputSchema)
     : null
-  if (data?.feed?.length) {
+  if (data) {
     return {
       success: true,
       data,

src/lib/api/feed/merge.test.ts

@@ -0,0 +1,131 @@
+import {type AppBskyFeedDefs, type AtpAgent} from '@atproto/api'
+
+import {MergeFeedAPI} from './merge'
+
+const post = {} as AppBskyFeedDefs.FeedViewPost
+
+describe('MergeFeedAPI', () => {
+  it('drains a terminal following queue without restarting the source', async () => {
+    const api = new MergeFeedAPI({
+      agent: {} as AtpAgent,
+      feedParams: {},
+      feedTuners: [],
+    })
+    api.reset()
+    api.following.queue = [post, post, post]
+    api.following.hasMore = false
+
+    const first = await api.fetch({cursor: 'started', limit: 2})
+    const second = await api.fetch({cursor: first.cursor, limit: 2})
+
+    expect(first.feed).toHaveLength(2)
+    expect(first.cursor).toBeDefined()
+    expect(second.feed).toHaveLength(1)
+    expect(second.cursor).toBeUndefined()
+  })
+
+  it('stops when the following source repeats its cursor', async () => {
+    const getTimeline = jest
+      .fn()
+      .mockResolvedValueOnce({
+        success: true,
+        headers: {},
+        data: {feed: [], cursor: 'a'},
+      })
+      .mockResolvedValueOnce({
+        success: true,
+        headers: {},
+        data: {feed: [], cursor: 'a'},
+      })
+    const api = new MergeFeedAPI({
+      agent: {getTimeline} as unknown as AtpAgent,
+      feedParams: {},
+      feedTuners: [],
+    })
+
+    const first = await api.fetch({cursor: undefined, limit: 1})
+    const second = await api.fetch({cursor: first.cursor, limit: 1})
+
+    expect(first.cursor).toBeDefined()
+    expect(second.cursor).toBeUndefined()
+    expect(getTimeline).toHaveBeenCalledTimes(2)
+  })
+
+  it('stops when the following source returns an empty cursor', async () => {
+    const getTimeline = jest.fn().mockResolvedValue({
+      success: true,
+      headers: {},
+      data: {feed: [], cursor: ''},
+    })
+    const api = new MergeFeedAPI({
+      agent: {getTimeline} as unknown as AtpAgent,
+      feedParams: {},
+      feedTuners: [],
+    })
+
+    const result = await api.fetch({cursor: undefined, limit: 1})
+
+    expect(result.cursor).toBeUndefined()
+    expect(getTimeline).toHaveBeenCalledTimes(1)
+  })
+
+  it('stops when the following source cycles to an earlier cursor', async () => {
+    const getTimeline = jest
+      .fn()
+      .mockResolvedValueOnce({
+        success: true,
+        headers: {},
+        data: {feed: [], cursor: 'a'},
+      })
+      .mockResolvedValueOnce({
+        success: true,
+        headers: {},
+        data: {feed: [], cursor: 'b'},
+      })
+      .mockResolvedValueOnce({
+        success: true,
+        headers: {},
+        data: {feed: [], cursor: 'a'},
+      })
+    const api = new MergeFeedAPI({
+      agent: {getTimeline} as unknown as AtpAgent,
+      feedParams: {},
+      feedTuners: [],
+    })
+
+    const first = await api.fetch({cursor: undefined, limit: 1})
+    const second = await api.fetch({cursor: first.cursor, limit: 1})
+    const third = await api.fetch({cursor: second.cursor, limit: 1})
+
+    expect(third.cursor).toBeUndefined()
+    expect(getTimeline).toHaveBeenCalledTimes(3)
+  })
+
+  it('drains a terminal custom-feed queue without restarting the source', async () => {
+    const getFeed = jest.fn()
+    const api = new MergeFeedAPI({
+      agent: {
+        app: {bsky: {feed: {getFeed}}},
+      } as unknown as AtpAgent,
+      feedParams: {
+        mergeFeedEnabled: true,
+        mergeFeedSources: [
+          'at://did:example:feed/app.bsky.feed.generator/test',
+        ],
+      },
+      feedTuners: [],
+    })
+    api.reset()
+    api.following.hasMore = false
+    api.customFeeds[0].queue = [post, post, post]
+    api.customFeeds[0].hasMore = false
+
+    const first = await api.fetch({cursor: 'started', limit: 2})
+    const second = await api.fetch({cursor: first.cursor, limit: 2})
+
+    expect(first.feed).toHaveLength(2)
+    expect(second.feed).toHaveLength(1)
+    expect(second.cursor).toBeUndefined()
+    expect(getFeed).not.toHaveBeenCalled()
+  })
+})

src/lib/api/feed/merge.ts

@@ -100,8 +100,8 @@ export class MergeFeedAPI implements FeedAPI {
 
     const promises = []
 
-    // always keep following topped up
-    if (this.following.numReady < limit) {
+    // always keep following topped up while the source has another page
+    if (this.following.hasMore && this.following.numReady < limit) {
       await this.following.fetchNext(60)
     }
 
@@ -117,7 +117,7 @@ export class MergeFeedAPI implements FeedAPI {
       !this.following.hasMore && this.following.numReady < limit
     if (this.params.mergeFeedEnabled || outOfFollows) {
       for (const feed of feeds) {
-        if (feed.numReady < 5) {
+        if (feed.hasMore && feed.numReady < 5) {
           promises.push(feed.fetchNext(10))
         }
       }
@@ -137,8 +137,12 @@ export class MergeFeedAPI implements FeedAPI {
       }
     }
 
+    const hasMore =
+      this.following.hasMore ||
+      this.following.numReady > 0 ||
+      this.customFeeds.some(feed => feed.hasMore || feed.numReady > 0)
     return {
-      cursor: String(this.itemCursor),
+      cursor: hasMore ? String(this.itemCursor) : undefined,
       feed: posts,
     }
   }
@@ -147,8 +151,8 @@ export class MergeFeedAPI implements FeedAPI {
     const i = this.itemCursor++
     const candidateFeeds = this.customFeeds.filter(f => f.numReady > 0)
     const canSample = candidateFeeds.length > 0
-    const hasFollows = this.following.hasMore
     const hasFollowsReady = this.following.numReady > 0
+    const hasFollows = this.following.hasMore || hasFollowsReady
 
     // this condition establishes the frequency that custom feeds are woven into follows
     const shouldSample =
@@ -179,6 +183,7 @@ class MergeFeedSource {
   feedTuners: FeedTunerFn[]
   sourceInfo: ReasonFeedSource | undefined
   cursor: string | undefined = undefined
+  seenCursors = new Set<string>()
   queue: AppBskyFeedDefs.FeedViewPost[] = []
   hasMore = true
 
@@ -213,7 +218,13 @@ class MergeFeedSource {
     const res = await this._getFeed(this.cursor, n)
     if (res.success) {
       this.cursor = res.data.cursor
-      this.hasMore = Boolean(this.cursor)
+      const cursor = this.cursor
+      if (cursor) {
+        this.hasMore = !this.seenCursors.has(cursor)
+        this.seenCursors.add(cursor)
+      } else {
+        this.hasMore = false
+      }
       if (res.data.feed.length) {
         this.queue = this.queue.concat(res.data.feed)
       }

src/state/queries/util.test.tsx

@@ -38,4 +38,63 @@ describe('useAutoPagination', () => {
 
     expect(value.fetchNextPage).not.toHaveBeenCalled()
   })
+
+  it('stops when structured page params repeat a cursor', () => {
+    const value = query({
+      data: {pageParams: [undefined, {cursor: 'a'}, {cursor: 'a'}]},
+    })
+
+    renderHook(() => useAutoPagination(value, 0, 10))
+
+    expect(value.fetchNextPage).not.toHaveBeenCalled()
+  })
+
+  it('stops when a cursor repeats non-adjacently', () => {
+    const value = query({data: {pageParams: [undefined, 'a', 'b', 'a']}})
+
+    renderHook(() => useAutoPagination(value, 0, 10))
+
+    expect(value.fetchNextPage).not.toHaveBeenCalled()
+  })
+
+  it('fills one page after switching to a smaller cached query', () => {
+    const first = query({hasNextPage: false})
+    const second = query()
+    let value = first
+    let itemCount = 10
+    const {rerender} = renderHook(() => useAutoPagination(value, itemCount, 10))
+
+    value = second
+    itemCount = 5
+    rerender(undefined)
+
+    expect(second.fetchNextPage).toHaveBeenCalledTimes(1)
+  })
+
+  it('resets the attempt limit after switching to cached data with the same item count', () => {
+    const fetchNextPage = jest.fn().mockResolvedValue(undefined)
+    const data = {pageParams: [undefined]}
+    let value = query({fetchNextPage, data})
+    const itemCount = 0
+    const {rerender} = renderHook(() => useAutoPagination(value, itemCount, 10))
+
+    for (let i = 1; i < 50; i++) {
+      value = query({
+        fetchNextPage,
+        data,
+      })
+      rerender(undefined)
+    }
+    expect(fetchNextPage).toHaveBeenCalledTimes(49)
+
+    const second = query({
+      data: {
+        pageParams: Array.from({length: 51}, (_, i) => `new-cursor-${i}`),
+      },
+    })
+    value = second
+    rerender(undefined)
+
+    expect(second.fetchNextPage).toHaveBeenCalledTimes(1)
+  })
 })

src/state/queries/util.ts

@@ -30,30 +30,55 @@ export function useAutoPagination(
   pageSize: number,
 ) {
   const lastItemCount = useRef(0)
+  const lastPageParams = useRef(query.data?.pageParams)
   const wantedItemCount = useRef(pageSize)
   const attemptCount = useRef(0)
 
   useEffect(() => {
+    const cursorOf = (param: unknown) =>
+      param && typeof param === 'object' && 'cursor' in param
+        ? param.cursor
+        : param
+    const pageParams = query.data?.pageParams
+    const previousPageParams = lastPageParams.current
+    const continuedPagination =
+      pageParams &&
+      previousPageParams &&
+      pageParams.length > previousPageParams.length &&
+      previousPageParams.every((param, index) =>
+        Object.is(cursorOf(param), cursorOf(pageParams[index])),
+      )
+    if (
+      pageParams !== previousPageParams &&
+      previousPageParams &&
+      !continuedPagination
+    ) {
+      wantedItemCount.current = pageSize
+      attemptCount.current = 0
+    }
+    lastPageParams.current = pageParams
+
     if (itemCount !== lastItemCount.current) {
+      attemptCount.current = 0
       if (itemCount < lastItemCount.current) {
-        wantedItemCount.current = itemCount
+        wantedItemCount.current = Math.max(itemCount, pageSize)
       }
       lastItemCount.current = itemCount
     }
 
     if (query.isLoading || query.isRefetching) {
       wantedItemCount.current = pageSize
+      attemptCount.current = 0
     } else if (query.isFetchingNextPage) {
       if (itemCount > wantedItemCount.current) {
         wantedItemCount.current = itemCount + pageSize
       }
     } else if (query.hasNextPage) {
       if (itemCount < wantedItemCount.current) {
-        const pageParams = query.data?.pageParams
-        const repeatedCursor =
-          pageParams &&
-          pageParams.length > 1 &&
-          Object.is(pageParams.at(-1), pageParams.at(-2))
+        const currentCursor = cursorOf(pageParams?.at(-1))
+        const repeatedCursor = pageParams
+          ?.slice(0, -1)
+          .some(param => Object.is(cursorOf(param), currentCursor))
         if (repeatedCursor) return
         attemptCount.current++
         if (attemptCount.current < 50) {