diff --git a/src/lib/api/feed/custom.test.ts b/src/lib/api/feed/custom.test.ts new file mode 100644 index 0000000000..a6fb459378 --- /dev/null +++ b/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 = 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 + } + }) +}) diff --git a/src/lib/api/feed/custom.ts b/src/lib/api/feed/custom.ts index edf10b0539..ade6c77d81 100644 --- a/src/lib/api/feed/custom.ts +++ b/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, diff --git a/src/lib/api/feed/merge.test.ts b/src/lib/api/feed/merge.test.ts new file mode 100644 index 0000000000..9bfe0bd40f --- /dev/null +++ b/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() + }) +}) diff --git a/src/lib/api/feed/merge.ts b/src/lib/api/feed/merge.ts index 06daaeaa0d..5e4443ee67 100644 --- a/src/lib/api/feed/merge.ts +++ b/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() 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) } diff --git a/src/state/queries/util.test.tsx b/src/state/queries/util.test.tsx index 360be2d05a..695ada0697 100644 --- a/src/state/queries/util.test.tsx +++ b/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) + }) }) diff --git a/src/state/queries/util.ts b/src/state/queries/util.ts index 0d97de6fe3..8e5d428484 100644 --- a/src/state/queries/util.ts +++ b/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) {