refactor(sdk): drop the dead hasNextPage page param and stabilise the display select - #1327
Conversation
… display select Follow-up to #1324, both review notes from that PR. hasNextPage became vestigial once getNextPageParam returned undefined to terminate: it was never set to false again, so the query function's short-circuit on it was unreachable. Field and guard both removed. The test that covered that guard goes with it, since the behaviour it described no longer exists; termination is covered by the empty-page case. `select` was an inline arrow, and React Query only reuses its previous result when both the data and the select function are unchanged. These options are rebuilt on every render, so every page was re-sorted on every render. One stable select per sort restores the memoisation.
|
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 (7)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe ranked-post query now uses cursor-only pagination. It removes ChangesRanked post query
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.spec.ts (1)
243-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest React Query behavior instead of selector reference identity.
a.select === b.selectasserts the factory implementation. Test that rebuilding options with unchanged query data does not cause another display transformation. Keep the existing ordering tests for visible output.As per coding guidelines, use Vitest for tests and “test user-visible behavior rather than implementation details.”
🤖 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.spec.ts` around lines 243 - 257, Replace the selector reference-identity assertions in the “select identity” tests with Vitest coverage of React Query’s behavior: rebuild options for the same sort with unchanged query data and verify the display transformation is not rerun. Preserve the existing ordering/output tests, and assert observable transformation behavior rather than function identity.Source: Coding guidelines
🤖 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.
Inline comments:
In
`@packages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.spec.ts`:
- Line 56: Remove the explicit any casts in the affected tests and use typed
helpers to narrow options.queryFn before invoking it. Preserve type-safe access
to getNextPageParam and select while keeping the existing test behavior
unchanged.
---
Nitpick comments:
In
`@packages/sdk/src/modules/posts/queries/get-posts-ranked-query-options.spec.ts`:
- Around line 243-257: Replace the selector reference-identity assertions in the
“select identity” tests with Vitest coverage of React Query’s behavior: rebuild
options for the same sort with unchanged query data and verify the display
transformation is not rerun. Preserve the existing ordering/output tests, and
assert observable transformation behavior rather than function identity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee777c23-e5f7-4f9f-90f5-40220ecc6d44
📒 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
|
|
||
| const options = getPostsRankedInfiniteQueryOptions('created', 'hive') | ||
| const result = await (options.queryFn as any)(makeInfiniteContext(options, { hasNextPage: true })) | ||
| const result = await (options.queryFn as any)(makeInfiniteContext(options, {})) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the explicit any casts.
These changed lines produce configured ESLint errors. Use typed helpers that narrow options.queryFn before invocation. Preserve typed access to getNextPageParam and select.
Also applies to: 66-66, 80-80, 111-111, 129-129, 141-141, 225-225, 227-227, 233-233
🧰 Tools
🪛 ESLint
[error] 56-56: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 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.spec.ts`
at line 56, Remove the explicit any casts in the affected tests and use typed
helpers to narrow options.queryFn before invoking it. Preserve type-safe access
to getNextPageParam and select while keeping the existing test behavior
unchanged.
Source: Linters/SAST tools
There was a problem hiding this comment.
Checked this against the configured lint scope rather than acting on it, and it does not hold as a finding against this PR.
Two things. First, the casts are the existing convention in this file, not something this PR introduces: develop has 15 occurrences of as any in it and this branch has 14, so the change reduces them by one. The lines cited are almost all restored original code.
Second, the SDK's own linter does not currently run. packages/sdkhas"lint": "eslint ."and aneslint.config.mjs, but invoking it throws from @typescript-eslint/utils` RuleCreator: the workspace resolves eslint 8.57.1 while the installed typescript-eslint build targets eslint 9. So nothing is reporting these errors today, which is also why the CI lint job is green.
Narrowing the types here would mean typing queryFn, getNextPageParam and select off infiniteQueryOptions through their generics, which is worth doing for the whole file rather than for the two lines this PR touched. The eslint version mismatch is worth fixing first, since otherwise the convention will keep drifting with nothing to catch it. Happy to open an issue for both if useful.
1 participant
Both CodeRabbit notes from #1324, which were correct and not merge blockers.
The page param field was dead
hasNextPageon the page param existed so the query function could short-circuit once the feed ended. Termination now happens bygetNextPageParamreturningundefined, which means the field is never set to false again and the guard reading it is unreachable. Field and guard both removed.The test that covered that guard goes with it. Flagging that deliberately rather than quietly, because a test was already lost once on this file: the behaviour it described no longer exists, and termination is covered by the empty-page case added in #1324.
The select was rebuilt every render
selectwas an inline arrow. React Query reuses its previous result only when both the data and the select function are unchanged, and these options are constructed during render, so the arrow was a new function each time and every page was re-sorted on every render of every community feed.One stable select per sort restores the memoisation. Keyed by sort rather than a single shared function because the ordering differs:
hotkeeps the bridge order, everything else sorts by creation date.Verification
596 SDK tests pass, typecheck clean, package builds. Two new tests cover the select identity, and inlining the arrow again fails the first of them.
Summary by CodeRabbit