Clone feature/imperative infinite queries #18
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 · |
|
@refacto-visz |
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 enhances the flexibility of infinite queries by allowing 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
|
|
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. WalkthroughThe changes enable explicit Changes
Sequence DiagramsequenceDiagram
actor User
participant Observer as InfiniteQueryObserver
participant Behavior as infiniteQueryBehavior
participant Query as Query Engine
User->>Observer: fetchNextPage({ pageParam: 5 })
Observer->>Behavior: fetch with meta.fetchMore = { direction: 'forward', pageParam: 5 }
alt pageParam provided in meta.fetchMore
Behavior->>Behavior: Use explicit pageParam: 5
else pageParam not provided
Behavior->>Behavior: Compute nextPageParam via getNextPageParam()
end
Behavior->>Query: Execute queryFn with pageParam: 5, direction: 'forward'
Query-->>Behavior: Return data for page 5
Behavior-->>Observer: Update pages and pageParams
Observer-->>User: Return InfiniteQueryObserverResult
User->>Observer: fetchPreviousPage({ pageParam: -1 })
Observer->>Behavior: fetch with meta.fetchMore = { direction: 'backward', pageParam: -1 }
Behavior->>Behavior: Use explicit pageParam: -1
Behavior->>Query: Execute queryFn with pageParam: -1, direction: 'backward'
Query-->>Behavior: Return data for page -1
Behavior-->>Observer: Update pages and pageParams
Observer-->>User: Return InfiniteQueryObserverResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
feat: Allow imperative page param for infinite queriesTL;DR: This change makes Refacto PR SummaryThis pull request introduces a significant enhancement to infinite queries by enabling an imperative fetching model. Previously, infinite queries required If Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant C as Component
participant O as InfiniteQueryObserver
participant B as InfiniteQueryBehavior
participant QF as queryFn
C->>O: fetchNextPage({ pageParam: 2 })
Note over O: getNextPageParam is not defined
O->>B: fetch(meta: { fetchMore: { direction: 'forward', pageParam: 2 } })
B->>B: Prioritize explicit pageParam over getNextPageParam
B->>QF: Execute with { pageParam: 2 }
QF-->>B: Return page data for page 2
B-->>O: New state with appended page
O-->>C: Notify with updated data { pages: [page1, page2] }
Testing GuideClick to expand
|
codeant-ai
bot
added
the
size:L
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
CodeAnt AI finished reviewing your PR. |
![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 an imperative API for infinite queries, allowing pageParam to be passed directly to fetchNextPage and fetchPreviousPage. This is a great feature for pagination patterns where the next page's identifier isn't easily derived. The changes are well-implemented across the core logic, types, and are accompanied by good tests. I've found one critical issue in the refetch logic for imperatively-fetched pages, which could lead to incorrect data being refetched. My review includes a suggested fix for this issue.
| 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 logic for determining the param for refetching seems to have a bug when getNextPageParam is not provided. For subsequent pages (currentPage > 0), if oldPageParams[currentPage] is undefined or null, it incorrectly falls back to options.initialPageParam. This would cause a refetch with the wrong page parameter. For example, if pageParams were [1, undefined], a refetch would request pages for 1 and then for initialPageParam, instead of 1 and undefined.
The fallback to initialPageParam should only happen for the very first page (currentPage === 0) when there are no previous page params.
const param =
currentPage === 0
? oldPageParams[0] ?? options.initialPageParam
: !options.getNextPageParam
? oldPageParams[currentPage]
: getNextPageParam(options, result)
Code Review: Imperative Infinite Query Enhancements👍 Well Done
📁 Selected files for review (6)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| fetchPreviousPage( | ||
| options?: FetchPreviousPageOptions, | ||
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | ||
| 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.
New Feature Lacks Required Feature Flag
This change introduces a significant new feature (imperative page parameters) without being gated by a feature flag. This violates the organization guideline for new functionality, which is crucial for controlled rollouts and ensuring a clear rollback plan. The new API surface should be conditionally enabled via a feature flag.
fetchNextPage(
options?: FetchNextPageOptions,
): Promise<InfiniteQueryObserverResult<TData, TError>> {
return this.fetch({
...options,
meta: {
fetchMore: { direction: 'forward' },
},
})
}
fetchPreviousPage(
options?: FetchPreviousPageOptions,
): Promise<InfiniteQueryObserverResult<TData, TError>> {
return this.fetch({
...options,
meta: {
fetchMore: { direction: 'backward' },
},
})
}
Commitable Suggestion
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | |
| InfiniteQueryObserverResult<TData, TError> | |
| > { | |
| return this.fetch({ | |
| ...options, | |
| meta: { | |
| fetchMore: { direction: 'forward' }, | |
| fetchMore: { direction: 'forward', pageParam }, | |
| }, | |
| }) | |
| } | |
| fetchPreviousPage( | |
| options?: FetchPreviousPageOptions, | |
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | |
| fetchPreviousPage({ | |
| pageParam, | |
| ...options | |
| }: FetchPreviousPageOptions = {}): Promise< | |
| InfiniteQueryObserverResult<TData, TError> | |
| > { | |
| return this.fetch({ | |
| ...options, | |
| meta: { | |
| fetchMore: { direction: 'backward' }, | |
| fetchMore: { direction: 'backward', pageParam }, | |
| }, | |
| }) | |
| } | |
| fetchNextPage( | |
| options?: FetchNextPageOptions, | |
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | |
| return this.fetch({ | |
| ...options, | |
| meta: { | |
| fetchMore: { direction: 'forward' }, | |
| }, | |
| }) | |
| } | |
| fetchPreviousPage( | |
| options?: FetchPreviousPageOptions, | |
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | |
| return this.fetch({ | |
| ...options, | |
| meta: { | |
| fetchMore: { direction: 'backward' }, | |
| }, | |
| }) | |
| } |
Standards
- Org-Guideline-Use feature flags for new functionality and include a clear rollback plan.
| getNextPageParam?: GetNextPageParamFunction<TPageParam, TQueryFnData> |
There was a problem hiding this comment.
API Contract Changed Without Feature Flag
A significant new feature allowing imperative page fetching has been introduced by making getNextPageParam optional. This changes a core API contract without being placed behind a feature flag, violating company policy. This makes rollbacks difficult and increases risk if unforeseen issues arise from the new behavior.
getNextPageParam: GetNextPageParamFunction<TPageParam, TQueryFnData>
Commitable Suggestion
| getNextPageParam?: GetNextPageParamFunction<TPageParam, TQueryFnData> | |
| getNextPageParam: GetNextPageParamFunction<TPageParam, TQueryFnData> |
Standards
- Org-Guideline-Use feature flags for new functionality and include a clear rollback plan.
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam |
There was a problem hiding this comment.
Type Definition and Runtime Behavior Mismatch
The runtime logic allows pageParam to override the result of getNextPageParam if both are provided. However, the accompanying type definitions explicitly forbid providing pageParam when getNextPageParam is defined. This discrepancy between the typed API contract and the actual runtime behavior can lead to confusion and unexpected behavior, undermining the reliability of the type system.
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| expectTypeOf<typeof observer.fetchNextPage>() | ||
| .parameter(0) | ||
| .toEqualTypeOf< | ||
| | { pageParam: number; cancelRefetch?: boolean; throwOnError?: boolean } | ||
| | undefined | ||
| >() |
There was a problem hiding this comment.
Incorrect Type Test Allows Undefined Arguments
The type test asserts that the options argument for fetchNextPage can be undefined, which contradicts the test's stated goal to 'require pageParam'. This lenient type allows calling fetchNextPage() without arguments when getNextPageParam is missing, which can lead to a silent runtime failure where the queryFn receives pageParam: undefined. The type should enforce that the options object containing pageParam is mandatory.
Standards
- Type Safety & API Contracts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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 (2)
packages/query-core/src/infiniteQueryObserver.ts (1)
InfiniteQueryObserver(27-198)packages/query-core/src/__tests__/utils.ts (1)
queryKey(24-27)
packages/query-core/src/infiniteQueryObserver.ts (1)
packages/query-core/src/types.ts (2)
FetchNextPageOptions(586-596)FetchPreviousPageOptions(598-608)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (2)
packages/query-core/src/__tests__/utils.ts (1)
queryKey(24-27)packages/query-core/src/infiniteQueryObserver.ts (1)
InfiniteQueryObserver(27-198)
🪛 GitHub Actions: pr
packages/query-core/src/infiniteQueryObserver.ts
[error] 122-122: TS2339: Property 'pageParam' does not exist on type 'FetchNextPageOptions'.
| const previous = fetchMore.direction === 'backward' | ||
| const pageParamFn = previous ? getPreviousPageParam : getNextPageParam | ||
| const oldData = { | ||
| pages: oldPages, | ||
| pageParams: oldPageParams, | ||
| } | ||
| const param = pageParamFn(options, oldData) | ||
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam | ||
|
|
||
| result = await fetchPage(oldData, param, previous) | ||
| } else { |
There was a problem hiding this comment.
Explicit pageParam is dropped on the very first fetch
If the consumer starts an infinite query with no cached pages (e.g. enabled: false) and then calls fetchNextPage({ pageParam }), fetchMore is populated but oldPages.length is 0. Because of the if (fetchMore && oldPages.length) guard we fall into the else branch, completely ignoring the caller-supplied pageParam and defaulting back to initialPageParam. That makes it impossible to bootstrap a query from a runtime-provided cursor—the main use case this feature is supposed to unlock.
Please let the fetchMore branch run even when there are no cached pages and treat an explicit pageParam override as the source of truth. Something like the diff below preserves the old behavior for existing pages while honoring manual cursors for the first fetch:
- if (fetchMore && oldPages.length) {
- const previous = fetchMore.direction === 'backward'
- const pageParamFn = previous ? getPreviousPageParam : getNextPageParam
- const oldData = {
- pages: oldPages,
- pageParams: oldPageParams,
- }
- const param =
- fetchMore.pageParam === undefined
- ? pageParamFn(options, oldData)
- : fetchMore.pageParam
-
- result = await fetchPage(oldData, param, previous)
+ if (fetchMore) {
+ const previous = fetchMore.direction === 'backward'
+ const pageParamFn = previous ? getPreviousPageParam : getNextPageParam
+ const baseData = oldPages.length
+ ? { pages: oldPages, pageParams: oldPageParams }
+ : { pages: [], pageParams: [] }
+ const param =
+ fetchMore.pageParam !== undefined
+ ? fetchMore.pageParam
+ : oldPages.length
+ ? pageParamFn(options, baseData)
+ : options.initialPageParam
+
+ result = await fetchPage(baseData, param, previous)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/query-core/src/infiniteQueryBehavior.ts around lines 85 to 97, the
current guard skips the fetchMore branch when there are no cached pages causing
an explicit fetchMore.pageParam to be ignored; change the condition so fetchMore
is processed even if oldPages.length === 0 when the caller provided a pageParam.
Specifically, always enter the fetchMore logic when fetchMore exists, compute
previous and pageParamFn as before, and derive param by preferring
fetchMore.pageParam when it is defined (use that value even if oldPages is
empty); only fall back to calling pageParamFn(options, oldData) when
fetchMore.pageParam is undefined (and for existing pages preserve current
behavior).
| 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.
Critical: Add pageParam to FetchNextPageOptions type definition.
The pipeline is failing because pageParam is being destructured from FetchNextPageOptions, but the type definition doesn't include this property. The relevant code snippets show that FetchNextPageOptions in types.ts (lines 585-595) only includes cancelRefetch.
To fix this, update the FetchNextPageOptions interface in packages/query-core/src/types.ts:
export interface FetchNextPageOptions extends ResultOptions {
+ /**
+ * Optional page parameter to use when fetching the next page.
+ * If provided, this will override the value computed from getNextPageParam.
+ */
+ pageParam?: unknown
/**
* 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
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/query-core/src/types.ts around lines 585 to 595, the
FetchNextPageOptions interface is missing the pageParam property but code in
infiniteQueryObserver.ts destructures pageParam from FetchNextPageOptions;
update the FetchNextPageOptions type to include an optional pageParam (e.g.
pageParam?: unknown or a properly generic page param type used across the
package), so the compiler knows this property exists and type-checks correctly.
| 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.
Critical: Add pageParam to FetchPreviousPageOptions type definition.
The same issue applies here—pageParam is being destructured from FetchPreviousPageOptions, but the type definition doesn't include this property.
Update the FetchPreviousPageOptions interface in packages/query-core/src/types.ts:
export interface FetchPreviousPageOptions extends ResultOptions {
+ /**
+ * Optional page parameter to use when fetching the previous page.
+ * If provided, this will override the value computed from getPreviousPageParam.
+ */
+ pageParam?: unknown
/**
* 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
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/query-core/src/infiniteQueryObserver.ts around lines 138 to 150,
fetchPreviousPage destructures pageParam from FetchPreviousPageOptions but the
type definition lacks pageParam; update packages/query-core/src/types.ts to add
an optional pageParam?: unknown (or generic TPageParam) to the
FetchPreviousPageOptions interface (and any related
FetchNextPageOptions/FetchPageOptions types if present) so the destructured
property is correctly typed and preserves proper generics/optional usage.
CodeAnt-AI Description
Allow imperative infinite queries with manual page params
What Changed
Impact
✅ Manual paging for infinite lists✅ Consistent refetch of previously loaded pages✅ Clearer manual pagination requirements💡 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
fetchNextPage()andfetchPreviousPage()now accept an explicitpageParamparameter for manual pagination control.getNextPageParamis now optional, enabling manual page parameter management without required callback functions.Tests
✏️ Tip: You can customize this high-level summary in your review settings.