fix: skip empty non-list queries during fetchAll pagination #1395
Conversation
📦 Packages
|
github-actions
bot
added
qa:success
janb87
changed the title
github-actions
bot
added
qa:failed
janb87
requested review from
SilVdB,
daanporon,
pmualaba,
roderik and
snigdha920
github-actions
bot
added
qa:running
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider combining the pruning in createNonListQuery with the executable-field counting into a single AST visit to eliminate the extra traversal and streamline the logic.
- Instead of always returning a filtered document and relying on countExecutableFields externally, have createNonListQuery itself return null when no executable fields remain to simplify its API.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider combining the pruning in createNonListQuery with the executable-field counting into a single AST visit to eliminate the extra traversal and streamline the logic.
- Instead of always returning a filtered document and relying on countExecutableFields externally, have createNonListQuery itself return null when no executable fields remain to simplify its API.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
github-actions
bot
added
qa:success
|
To view in Slack, search for: 1761921589.180549 |
|
To view in Slack, search for: 1761921613.129709 |
github-actions
bot
added
status:draft
📝 WalkthroughWalkthroughThis change enhances pagination logic in the GraphQL utility. A new test case validates behavior when only Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
sdk/thegraph/src/utils/pagination.test.ts(1 hunks)sdk/thegraph/src/utils/pagination.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Do not use default exports in TypeScript (except when a framework requires it)
Useimport typefor type-only imports in TypeScript
Prefer interfaces over type aliases for object shapes
Never useany; useunknownor proper types instead
Use discriminated unions for error handling
File names should be kebab-case
Use camelCase for variable and function names
Use PascalCase for types, interfaces, and classes
Use UPPER_SNAKE_CASE for constants
Prefer nullish coalescing (??) over logical OR (||)
Use early returns to reduce nesting
Extract complex logic into well-named functions
Keep functions small and focused
Use structured logging with proper context
**/*.{ts,tsx}: Inside generic functions, using any for concise type narrowing is acceptable; outside of generics, use any extremely sparingly
Avoid default exports unless explicitly required by the framework (e.g., Next.js pages)
Use discriminated unions to model variant data and avoid 'bag of optionals'
Handle discriminated unions with switch statements over the discriminant
Do not introduce new enums; prefer as const objects for enum-like behavior; retain existing enums
Use top-level import type when importing types instead of inline import { type ... }
Prefer interface extends over intersection types (&) for modeling inheritance; use & only when extends is not possible
Use JSDoc comments to annotate functions and types when behavior isn’t obvious; keep comments concise
Use JSDoc inline @link to reference related functions and types within the same file
File names should be kebab-case (e.g., my-component.ts)
Use camelCase for variables and function names
Use UpperCamelCase (PascalCase) for classes, types, and interfaces
Use ALL_CAPS for constants (and enum values where enums already exist)
Prefix generic type parameters with T (e.g., TKey, TValue)
Be mindful of noUncheckedIndexedAccess: treat indexed object/array access as possibly undefined
Use optional properties sparingly; prefer required prop...
Files:
sdk/thegraph/src/utils/pagination.test.tssdk/thegraph/src/utils/pagination.ts
sdk/{viem,portal,thegraph,eas,blockscout,js}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
sdk/{viem,portal,thegraph,eas,blockscout,js}/**/*.{ts,tsx}: Always validate blockchain addresses before use
Handle chain-specific differences properly
Implement proper gas estimation for transactions
Use type-safe contract interactions (e.g., via Viem/Ethers types)
Files:
sdk/thegraph/src/utils/pagination.test.tssdk/thegraph/src/utils/pagination.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{ts,tsx}: Write unit tests using Vitest
Mock external API calls in tests
Test error scenarios explicitly
Files:
sdk/thegraph/src/utils/pagination.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/bun.mdc)
Use
Bun.File()for file reading and writing
Files:
sdk/thegraph/src/utils/pagination.test.tssdk/thegraph/src/utils/pagination.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: QA
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Cursor Bugbot
- GitHub Check: Sourcery review
- GitHub Check: QA
- GitHub Check: QA
- GitHub Check: Sourcery review
🔇 Additional comments (1)
sdk/thegraph/src/utils/pagination.test.ts (1)
356-377: Appreciate the regression coverageNew test nails the scenario where only paginated fields are selected and ensures the client still returns structured data while tracking request count.
| Field: (node) => { | ||
| if (!node.name.value.startsWith("__")) { | ||
| if (!node.selectionSet || node.selectionSet.selections.length > 0) { | ||
| fieldCount += 1; | ||
| } | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Don't skip __typename-only follow-up queries
countExecutableFields ignores fields whose names start with "__", so a query like token { __typename holders @fetchAll { ... } } now short-circuits before the non-list request runs. The merged result never includes __typename, even though the caller explicitly asked for it. Previously we still executed the non-list fetch and returned the correct payload. Please count meta fields as executable (at least __typename) so we don't drop requested data.
Apply this diff to ensure __typename keeps the non-list query alive:
- Field: (node) => {
- if (!node.name.value.startsWith("__")) {
- if (!node.selectionSet || node.selectionSet.selections.length > 0) {
- fieldCount += 1;
- }
- }
- },
+ Field: (node) => {
+ if (!node.selectionSet || node.selectionSet.selections.length > 0) {
+ fieldCount += 1;
+ }
+ },🤖 Prompt for AI Agents
In sdk/thegraph/src/utils/pagination.ts around lines 284-291, the current logic
skips all fields whose names start with "__" causing queries that only request
__typename (e.g. token { __typename holders @fetchAll { ... } }) to
short-circuit and drop requested data; update the conditional so that meta
fields are still counted when executable, specifically treat "__typename" as
executable: do not exclude it from the fieldCount check (i.e., only skip
double-underscore fields except for "__typename"), and keep the existing
selectionSet/selections length check so __typename-only follow-up queries keep
the non-list query alive.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="sdk/thegraph/src/utils/pagination.ts">
<violation number="1" location="sdk/thegraph/src/utils/pagination.ts:285">
countExecutableFields excludes meta fields like __typename, which can cause the non-list request to be skipped and omit requested data when __typename is the only remaining field. Include __typename in the count so the follow-up query executes.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| visit(document, { | ||
| Field: (node) => { | ||
| if (!node.name.value.startsWith("__")) { |
There was a problem hiding this comment.
countExecutableFields excludes meta fields like __typename, which can cause the non-list request to be skipped and omit requested data when __typename is the only remaining field. Include __typename in the count so the follow-up query executes.
Prompt for AI agents
Address the following comment on sdk/thegraph/src/utils/pagination.ts at line 285:
<comment>countExecutableFields excludes meta fields like __typename, which can cause the non-list request to be skipped and omit requested data when __typename is the only remaining field. Include __typename in the count so the follow-up query executes.</comment>
<file context>
@@ -263,17 +262,35 @@ function createNonListQuery(document: DocumentNode, listFields: ListFieldWithFet
+
+ visit(document, {
+ Field: (node) => {
+ if (!node.name.value.startsWith("__")) {
+ if (!node.selectionSet || node.selectionSet.selections.length > 0) {
+ fieldCount += 1;
</file context>
Summary
@fetchAllso it also verifies no extra non-list request is issued@fetchAllTesting
gql.tadabecause packages cannot be downloaded in the execution environment)https://chatgpt.com/codex/tasks/task_b_6904799dd93883208eb61e4f90b5c5ab
Summary by Sourcery
Prevent empty non-list queries during @fetchall pagination by checking for executable fields and extend tests to cover queries with only fetchAll lists
Bug Fixes:
Enhancements:
Tests:
Note
Skips executing non-list GraphQL requests when only @fetchall list fields remain, with AST pruning and a regression test to verify behavior.
createNonListQuery(...): removes list fields and prunes empty selection sets; always returns a filtered document.countExecutableFields(...): new helper to detect empty documents; used to skip non-list requests when no executable fields remain.FieldNodefor leave-visitor pruning.@fetchAlllist fields returns data and does not issue an extra non-list request.Written by Cursor Bugbot for commit 95573b2. This will update automatically on new commits. Configure here.