fix(sdk): stop ranked-posts pagination at the end of the feed - #1324
Conversation
getNextPageParam returned an object for every page, including an empty one. React Query reads "there is no next page" from undefined alone, so hasNextPage stayed true forever. An infinite list therefore keeps calling fetchNextPage once the reader reaches the bottom. The query function short-circuits on the stale hasNextPage flag in the page param, so no further RPCs go out, but each call still appends an empty page: the query state churns and the cache grows for as long as the reader sits there. Returning undefined on an empty page ends it. Restoring the old return fails the new test.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRanked post queries preserve bridge response order for pagination cursors. Display ordering runs separately through React Query’s ChangesRanked pagination behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant BridgeAPI
participant QueryFunction
participant ReactQuery
BridgeAPI->>QueryFunction: Return ranked posts in bridge order
QueryFunction->>ReactQuery: Return filtered page and bridge-order cursor
ReactQuery->>ReactQuery: Apply orderForDisplay
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The termination tests were written into the spec file rather than added to it, dropping ten existing cases: null and malformed responses, the created-desc sort, pinned ordering and multi-pin retention, the hot exception, network error propagation and the finite query. All are restored and the two new cases sit alongside them.
getNextPageParam reads the cursor from the last entry of the page the query function returns, and the bridge continues a ranked feed from that entry in ITS ranking. The page was re-sorted by creation date first, so for trending, payout and muted the cursor named the oldest entry by date rather than the last by rank: the next request started from the middle of the previous ranked page, and scrolling repeated some posts and skipped others. The display ordering, pinned entries first then created descending, moves into `select`, which React Query applies after pagination. Nothing a reader sees changes; the cursor is now taken from the bridge's own order. This closes the second half of the ranked-posts problem, so #1322 can move community feeds onto this query without inheriting it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.ts (2)
104-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
pageParam.hasNextPageis now vestigial.
getNextPageParamonly returnsundefined(Line 117) or an object withhasNextPage: true(Line 123). It never produceshasNextPage: false. Combined withinitialPageParam.hasNextPage: trueat Line 107,pageParam.hasNextPageis alwaystruewheneverqueryFnruns. The early-return guard at Line 62 (if (!pageParam.hasNextPage) return [];) is now unreachable dead code.Leaving this field in place can mislead a future maintainer into believing that toggling
hasNextPageon the cursor object controls pagination, when only theundefinedvs. object return value ofgetNextPageParamactually does. Remove the field and the dead guard, or document why it is intentionally retained.♻️ Proposed cleanup
- queryFn: async ({ pageParam, signal }: { pageParam: PageParam; signal: AbortSignal }) => { - if (!pageParam.hasNextPage) { - return []; - } - - let sanitizedTag = tag; + queryFn: async ({ pageParam, signal }: { pageParam: PageParam; signal: AbortSignal }) => { + let sanitizedTag = tag; @@ initialPageParam: { author: undefined, permlink: undefined, - hasNextPage: true, } as PageParam, getNextPageParam: (lastPage: Entry[]) => { const last = lastPage?.[lastPage.length - 1]; if (!last) { return undefined; } return { author: last.author, permlink: last.permlink, - hasNextPage: true, }; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.ts` around lines 104 - 125, Remove the vestigial pageParam.hasNextPage field from initialPageParam and the object returned by getNextPageParam, then delete the unreachable early-return guard in the query function. Preserve pagination termination through getNextPageParam returning undefined when the last page is empty.
99-102: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
selectre-sorts every loaded page on each fetch.
data.pages.map((page) => orderForDisplay(page, sort))runsorderForDisplay(which itself does afilter+sort) over every page indata.pages, not just the newest one, each timeselectruns. As a reader scrolls a community feed and accumulates more pages, this becomes repeated O(n log n) work over already-ordered pages, and the cost compounds across the scrolling session.Cache per-page results keyed by page reference so unchanged pages are not re-sorted.
⚡ Proposed memoization
export function getPostsRankedInfiniteQueryOptions( sort: string, tag: string, limit = 20, observer = "", enabled = true, _options: GetPostsRankedOptions = {} ) { + const displayOrderCache = new WeakMap<Entry[], Entry[]>(); + return infiniteQueryOptions< Entry[], Error, InfiniteData<Entry[], PageParam>, (string | number)[], PageParam >({ queryKey: QueryKeys.posts.postsRanked(sort, tag, limit, observer), queryFn: async (... select: (data) => ({ ...data, - pages: data.pages.map((page) => orderForDisplay(page, sort)), + pages: data.pages.map((page) => { + const cached = displayOrderCache.get(page); + if (cached) return cached; + const ordered = orderForDisplay(page, sort); + displayOrderCache.set(page, ordered); + return ordered; + }), }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.ts` around lines 99 - 102, Update the select transformation in the ranked-post query options to memoize orderForDisplay results per page reference, keyed by the page object and sort value. Reuse cached ordering for unchanged pages and only invoke orderForDisplay for new or changed pages while preserving the existing pages output shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.ts`:
- Around line 104-125: Remove the vestigial pageParam.hasNextPage field from
initialPageParam and the object returned by getNextPageParam, then delete the
unreachable early-return guard in the query function. Preserve pagination
termination through getNextPageParam returning undefined when the last page is
empty.
- Around line 99-102: Update the select transformation in the ranked-post query
options to memoize orderForDisplay results per page reference, keyed by the page
object and sort value. Reuse cached ordering for unchanged pages and only invoke
orderForDisplay for new or changed pages while preserving the existing pages
output shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c5eeba8-2900-40cc-903e-030ccecf68d2
📒 Files selected for processing (2)
packages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.spec.tspackages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.ts
1 participant
Found while reviewing #1322, which switches the self-hosted community feed onto this query. The bug is pre-existing and affects every community feed on the main app too.
getNextPageParamreturned an object for every page, including an empty one:React Query reads "there is no next page" from
undefinedalone. A returned object means there is one, whatever it contains, sohasNextPagestayed true forever and thehasNextPagefield inside the param was only ever read by the query function.An infinite list therefore keeps calling
fetchNextPageonce the reader reaches the bottom. The query function does short-circuit on that stale flag, so no further RPCs go out, but each call still appends an empty page: the query state churns and the cache grows for as long as the reader sits at the end of the feed.Returning
undefinedon an empty page ends it. Termination is on an empty page rather than a short one, since a short page is not reliably the end and one extra request that comes back empty is cheap.584 SDK tests pass, typecheck clean. Restoring the old return fails the new test.
Related, not fixed here
The same function re-sorts every non-
hotpage by creation date beforegetNextPageParampicks the cursor, so fortrending,payoutandmutedthe cursor is the oldest post in the page rather than the last in the bridge ranked order. The next request then starts from the middle of the previous ranked page, which duplicates and skips posts while scrolling. Fixing that means deciding whether the date sort should apply to ranked sorts at all, which changes visible feed ordering on the main app, so it is filed separately as #1325 rather than folded in here.Summary by CodeRabbit
Bug Fixes
Chores