Clone feature/imperative infinite queries #19
Conversation
let's bring back imperative infinite queries, but only allow them in an all-or-nothing mode, dependent on if `getNextPageParam` has been passed
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request enhances infinite query pagination by adding support for explicit pageParam overrides in fetchNextPage and fetchPreviousPage methods, making the getNextPageParam option optional in the type system, and propagating pageParam metadata through the query pipeline. The changes maintain backward compatibility while enabling callers to override page parameters when fetching additional pages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature that allows developers to imperatively specify the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@refacto-visz |
codeant-ai
bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: imperative fetching for infinite queries. This allows developers to manually specify pageParam when calling fetchNextPage or fetchPreviousPage, which is useful when automatic page param detection via getNextPageParam is not suitable. The changes span across core logic, types, and tests to support this new capability.
My review identifies a potential bug in the refetch logic for these new imperative queries which could lead to incorrect data being fetched. I've also included suggestions to improve the developer experience by adding development-mode warnings for incorrect usage of the new imperative API, which currently fails silently.
| const param = | ||
| currentPage === 0 | ||
| ? (oldPageParams[0] ?? options.initialPageParam) | ||
| currentPage === 0 || !options.getNextPageParam | ||
| ? (oldPageParams[currentPage] ?? options.initialPageParam) | ||
| : getNextPageParam(options, result) |
There was a problem hiding this comment.
The current logic for determining the param for refetching has a potential bug. When !options.getNextPageParam is true, the expression oldPageParams[currentPage] ?? options.initialPageParam is used for all pages. This is incorrect for pages other than the first one (currentPage > 0). If oldPageParams[currentPage] is undefined for some reason, it will incorrectly fall back to options.initialPageParam, which could lead to fetching the wrong page data during a refetch.
The fallback to initialPageParam should only apply to the very first page (currentPage === 0). For subsequent pages in manual mode, we should rely on oldPageParams[currentPage] exclusively. I've suggested a refactor of this logic to make it safer and more explicit.
const param =
currentPage === 0
? oldPageParams[0] ?? options.initialPageParam
: !options.getNextPageParam
? oldPageParams[currentPage]
: getNextPageParam(options, result)| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
When getNextPageParam is not defined, fetchNextPage must be called with a pageParam. Currently, if it's called without one (e.g., fetchNextPage()), it results in a silent no-op. This can be confusing and hard to debug.
To improve the developer experience, I suggest adding a check that logs an error in development mode if fetchNextPage is called without a pageParam when getNextPageParam is not provided. This will make the API usage clearer and prevent silent failures.
fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise<
InfiniteQueryObserverResult<TData, TError>
> {
if (
process.env.NODE_ENV !== 'production' &&
pageParam === undefined &&
!this.options.getNextPageParam
) {
console.error(
`'pageParam' is required for 'fetchNextPage' when 'getNextPageParam' is not defined.`,
)
}
return this.fetch({
...options,
meta: {
fetchMore: { direction: 'forward', pageParam },
},
})
}| fetchPreviousPage({ | ||
| pageParam, | ||
| ...options | ||
| }: FetchPreviousPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'backward' }, | ||
| fetchMore: { direction: 'backward', pageParam }, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Similar to fetchNextPage, fetchPreviousPage will fail silently if called without a pageParam when getPreviousPageParam is not defined. This can lead to a poor developer experience.
I recommend adding a development-mode warning to alert the user when they call fetchPreviousPage without the required pageParam in manual pagination mode. This will help catch configuration errors early.
fetchPreviousPage({
pageParam,
...options
}: FetchPreviousPageOptions = {}): Promise<
InfiniteQueryObserverResult<TData, TError>
> {
if (
process.env.NODE_ENV !== 'production' &&
pageParam === undefined &&
!this.options.getPreviousPageParam
) {
console.error(
`'pageParam' is required for 'fetchPreviousPage' when 'getPreviousPageParam' is not defined.`,
)
}
return this.fetch({
...options,
meta: {
fetchMore: { direction: 'backward', pageParam },
},
})
}
![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
♻️ Duplicate comments (1)
packages/query-core/src/infiniteQueryObserver.ts (1)
138-148: Same type issue applies toFetchPreviousPageOptions
fetchPreviousPageuses the same{ pageParam, ...options }: FetchPreviousPageOptions = {}pattern, so it’s affected by the same missingpageParamproperty onFetchPreviousPageOptions. Once you addpageParam?: …to both option interfaces inpackages/query-core/src/types.ts(as suggested above), this method’s typing will be consistent with its implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx(1 hunks)packages/query-core/src/__tests__/infiniteQueryObserver.test-d.tsx(1 hunks)packages/query-core/src/infiniteQueryBehavior.ts(4 hunks)packages/query-core/src/infiniteQueryObserver.ts(1 hunks)packages/query-core/src/query.ts(1 hunks)packages/query-core/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/query-core/src/__tests__/infiniteQueryObserver.test-d.tsx (1)
packages/query-core/src/infiniteQueryObserver.ts (1)
InfiniteQueryObserver(27-198)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (4)
packages/query-core/src/__tests__/utils.ts (1)
queryKey(24-27)packages/query-core/src/queriesObserver.ts (2)
observer(255-261)result(185-202)packages/query-core/src/infiniteQueryObserver.ts (1)
InfiniteQueryObserver(27-198)packages/query-core/src/types.ts (1)
InfiniteQueryObserverResult(1049-1057)
packages/query-core/src/infiniteQueryObserver.ts (1)
packages/query-core/src/types.ts (3)
FetchNextPageOptions(586-596)InfiniteQueryObserverResult(1049-1057)FetchPreviousPageOptions(598-608)
🪛 GitHub Actions: pr
packages/query-core/src/infiniteQueryObserver.ts
[error] 122-122: TS2339: Property 'pageParam' does not exist on type 'FetchNextPageOptions'.
🔇 Additional comments (8)
packages/query-core/src/query.ts (1)
91-93: LGTM!The addition of an optional
pageParam?: unknowntoFetchMeta.fetchMorecorrectly enables passing explicit page parameters through the fetch pipeline while maintaining backward compatibility.packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (1)
204-306: LGTM!This test comprehensively validates the new
pageParamoverride feature:
- Tests explicit
pageParamin bothfetchNextPageandfetchPreviousPage- Verifies that refetch correctly uses the stored pageParams from previous manual fetches
- Properly cleans up the subscription
packages/query-core/src/types.ts (1)
274-274: MakinggetNextPageParamoptional enables imperative pagination.This change allows users to omit
getNextPageParamand instead providepageParamexplicitly when callingfetchNextPage/fetchPreviousPage.Note: When
getNextPageParamis undefined,hasNextPagewill always returnfalse(seeinfiniteQueryBehavior.ts:168). This is likely intentional since automatic next-page detection isn't possible without the callback.Please confirm this is the intended behavior for
hasNextPagewhen using imperative pagination withoutgetNextPageParam.packages/query-core/src/__tests__/infiniteQueryObserver.test-d.tsx (2)
76-95: Type test validates pageParam exclusion whengetNextPageParamis defined.This correctly enforces that when automatic pagination is configured via
getNextPageParam, thepageParamoption should not be available, preventing accidental override of the derived page parameter.
97-117: The type test expectations indicate that FetchNextPageOptions and FetchPreviousPageOptions need conditional typing based on getNextPageParam presence.The test expects
pageParamto be conditionally required whengetNextPageParamis missing and conditionally forbidden whengetNextPageParamis defined. However, the currentFetchNextPageOptionsandFetchPreviousPageOptionsinterfaces (types.ts lines 586-608) are static and don't includepageParamat all, nor do they have conditional properties based on query options.The runtime methods destructure
pageParamfrom options, but the types don't reflect this capability. The type definitions need to be updated to support conditionalpageParambased on whethergetNextPageParamis provided in the observer configuration.packages/query-core/src/infiniteQueryBehavior.ts (3)
84-96: LGTM!The updated logic correctly handles:
- Explicit
pageParamoverride fromfetchMore.pageParamwhen provided- Fallback to
pageParamFnwhenpageParamisundefinedThis distinction allows callers to explicitly pass
nullto stop pagination (handled by line 53'sparam == nullcheck).
102-111: Good fallback for missinggetNextPageParam.When
getNextPageParamis undefined, the code correctly falls back to storedoldPageParamsorinitialPageParam, enabling refetch of previously fetched pages even without automatic page parameter derivation.
136-149: Optional chaining aligns with updated type definitions.Using
options.getNextPageParam?.(...)correctly handles the case wheregetNextPageParamis now optional inInfiniteQueryPageParamsOptions.
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine infiniteQueryObserver.ts around the specified lines
fd infiniteQueryObserver.ts --type fRepository: coderabbit-test/query
Length of output: 113
🏁 Script executed:
# Also find types.ts in the same directory structure
fd types.ts packages/query-core/srcRepository: coderabbit-test/query
Length of output: 97
🏁 Script executed:
# Once found, let me check the exact content of infiniteQueryObserver.ts at lines 127-148
cat -n packages/query-core/src/infiniteQueryObserver.ts | sed -n '120,150p'Repository: coderabbit-test/query
Length of output: 993
🏁 Script executed:
# Now check the types.ts file for FetchNextPageOptions and FetchPreviousPageOptions
cat -n packages/query-core/src/types.ts | grep -A 20 "FetchNextPageOptions\|FetchPreviousPageOptions"Repository: coderabbit-test/query
Length of output: 2604
🏁 Script executed:
# Verify there are no other uses of pageParam in FetchNextPageOptions or related types
rg "pageParam" packages/query-core/src/types.tsRepository: coderabbit-test/query
Length of output: 306
🏁 Script executed:
# Check if there are any imports or extensions that might add pageParam to these interfaces
rg "FetchNextPageOptions\|FetchPreviousPageOptions" packages/query-core/src/types.ts | head -20Repository: coderabbit-test/query
Length of output: 47
🏁 Script executed:
# Let me check the full context around where pageParam appears in types.ts
rg -B 5 -A 5 "pageParam" packages/query-core/src/types.ts | head -100Repository: coderabbit-test/query
Length of output: 931
🏁 Script executed:
# Check if the interfaces have been updated - get more context around lines 586-608
cat -n packages/query-core/src/types.ts | sed -n '580,620p'Repository: coderabbit-test/query
Length of output: 1829
🏁 Script executed:
# Check if there's a tsconfig.json or build configuration that might suppress these errors
find packages/query-core -name "tsconfig.json" -o -name "tsconfig.*.json" | head -5Repository: coderabbit-test/query
Length of output: 178
🏁 Script executed:
# Let me verify the actual method signature once more and check if there are any type assertions or other overloads
rg -B 10 -A 10 "fetchNextPage\(" packages/query-core/src/infiniteQueryObserver.ts | head -40Repository: coderabbit-test/query
Length of output: 566
🏁 Script executed:
# Check what ResultOptions contains
rg -B 5 -A 15 "interface ResultOptions" packages/query-core/src/types.tsRepository: coderabbit-test/query
Length of output: 594
🏁 Script executed:
# Let me also search for any declaration merging or augmentation of FetchNextPageOptions
rg "FetchNextPageOptions" packages/query-core/src/ -A 5Repository: coderabbit-test/query
Length of output: 2018
🏁 Script executed:
# Check the entire infiniteQueryObserver.ts file for any type definitions or overloads that might affect this
wc -l packages/query-core/src/infiniteQueryObserver.tsRepository: coderabbit-test/query
Length of output: 117
🏁 Script executed:
# Check if there are any function overloads or declarations in the file
rg "fetchNextPage|fetchPreviousPage" packages/query-core/src/infiniteQueryObserver.ts -B 3Repository: coderabbit-test/query
Length of output: 558
🏁 Script executed:
# Let's verify the exact line numbers and see if there are any decorators or type annotations that might bypass the type check
cat -n packages/query-core/src/infiniteQueryObserver.ts | sed -n '120,155p'Repository: coderabbit-test/query
Length of output: 1107
🏁 Script executed:
# Let's check if the code actually compiles by looking for build errors or CI configuration
cat packages/query-core/tsconfig.json | head -50Repository: coderabbit-test/query
Length of output: 285
🏁 Script executed:
# Check if there's a way the types could be different - let me search for any conditional types or generics that might make pageParam available
ast-grep --pattern 'export interface FetchNextPageOptions {
$$$
}'Repository: coderabbit-test/query
Length of output: 47
🏁 Script executed:
# Final verification: does TypeScript actually report an error? Let me check the complete method signature with full context
sed -n '127,136p' packages/query-core/src/infiniteQueryObserver.tsRepository: coderabbit-test/query
Length of output: 329
Type mismatch: pageParam not declared on FetchNextPageOptions and FetchPreviousPageOptions
The methods destructure { pageParam, ...options } from FetchNextPageOptions and FetchPreviousPageOptions (lines 127–128 and 138–141), but neither interface declares a pageParam property. This causes a TypeScript TS2339 error and blocks compilation.
Add pageParam?: unknown to both interfaces in packages/query-core/src/types.ts:
export interface FetchNextPageOptions extends ResultOptions {
/**
* If set to `true`, calling `fetchNextPage` repeatedly will invoke `queryFn` every time,
* whether the previous invocation has resolved or not. Also, the result from previous invocations will be ignored.
*
* If set to `false`, calling `fetchNextPage` repeatedly won't have any effect until the first invocation has resolved.
*
* Defaults to `true`.
*/
cancelRefetch?: boolean
+ pageParam?: unknown
}
export interface FetchPreviousPageOptions extends ResultOptions {
/**
* If set to `true`, calling `fetchPreviousPage` repeatedly will invoke `queryFn` every time,
* whether the previous invocation has resolved or not. Also, the result from previous invocations will be ignored.
*
* If set to `false`, calling `fetchPreviousPage` repeatedly won't have any effect until the first invocation has resolved.
*
* Defaults to `true`.
*/
cancelRefetch?: boolean
+ pageParam?: unknown
}If you need stronger typing, make the interfaces generic (e.g., FetchNextPageOptions<TPageParam>) and update the method signatures accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/query-core/src/infiniteQueryObserver.ts around lines 127 to 134 and
138 to 141, the methods destructure pageParam from
FetchNextPageOptions/FetchPreviousPageOptions but those interfaces lack a
pageParam property causing TS2339; fix by adding pageParam?: unknown to both
FetchNextPageOptions and FetchPreviousPageOptions in
packages/query-core/src/types.ts (or, if stronger typing is desired, convert
them to generic interfaces like FetchNextPageOptions<TPageParam> and
FetchPreviousPageOptions<TPageParam> and update the method signatures in
infiniteQueryObserver.ts to accept the corresponding generic type).
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
CodeAnt-AI Description
Allow explicit pageParam when fetching infinite query pages
What Changed
Impact
✅ Clearer infinite pagination control when using manual cursors✅ Fewer unexpected page param derivations during imperative fetches✅ Stronger TypeScript guarantees for manual vs automatic paging💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
fetchNextPageandfetchPreviousPagenow accept an optionalpageParamparameter to explicitly control pagination behavior.getNextPageParamconfiguration is now optional, providing more flexibility in infinite query setups.Tests
pageParamusage in infinite query pagination.✏️ Tip: You can customize this high-level summary in your review settings.