Conversation
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
…tibility (#2562) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Co-authored-by: Yiming Cao <ymc9@users.noreply.github.com>
…FindMany (#2561) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Claude finished @ymc9's task —— View job Code Review Analysis
|
Contributor
📝 WalkthroughWalkthroughPatch release 3.5.6 updates all package versions and introduces type safety improvements for TanStack Query infinite queries with configurable page-parameter generics. ORM query construction is enhanced to properly align DISTINCT ON with ORDER BY fields, and orderBy validation restricts objects to single keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/clients/tanstack-query/src/vue.ts (1)
543-550:⚠️ Potential issue | 🔴 CriticalUse
pageParamwhen building paginated requests inqueryFn.In TanStack Vue Query's
useInfiniteQuery, thequeryFnreceives apageParamargument that TanStack Query automatically manages for pagination. Your current implementation ignores it and always builds the URL fromargsValue, causingfetchNextPage()to repeatedly fetch the same initial page instead of advancing through results.Suggested fix
- queryFn: ({ signal }: any) => { - const reqUrl = makeUrl(endpoint, model, operation, argsValue); + queryFn: ({ pageParam, signal }: any) => { + const reqUrl = makeUrl(endpoint, model, operation, pageParam ?? argsValue); return fetcher<TQueryFnData>(reqUrl, { signal }, fetch); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/clients/tanstack-query/src/vue.ts` around lines 543 - 550, The queryFn inside finalOptions ignores TanStack Query's pageParam so infinite queries always request the same page; update the queryFn signature to accept ({ signal, pageParam }) and incorporate pageParam into the request arguments when building reqUrl (use the pageParam to override/augment argsValue or pass it to makeUrl) so makeUrl(endpoint, model, operation, ...) uses the current page; update any call sites using finalOptions/queryFn to expect the pageParam handling (symbols: finalOptions, queryFn, argsValue, makeUrl, pageParam).
🧹 Nitpick comments (1)
tests/regression/test/issue-2529.test.ts (1)
6-21: Consider adding explicit cleanup to prevent connection leaks.The
setup()function creates a database client but doesn't clean up. WhilecreateTestClientmay handle some cleanup, adding explicit$disconnect()in anafterEachhook would be more robust and consistent with patterns in other test files (e.g.,find.test.tslines 14-16).♻️ Suggested improvement
+import { afterEach, describe, expect, it } from 'vitest'; + // https://github.com/zenstackhq/zenstack/issues/2529 describe('Regression for issue `#2529`', () => { + let db: any; + + afterEach(async () => { + await db?.$disconnect(); + }); + async function setup() { - const db = await createTestClient( + db = await createTestClient( ` model Post { id Int `@id` `@default`(autoincrement())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2529.test.ts` around lines 6 - 21, The setup() helper creates a Prisma client via createTestClient but never disconnects it, risking connection leaks; add an afterEach hook in this test file that calls await db.$disconnect() for the client returned by setup() (or store the db in a file-scoped variable set by setup()), ensuring the Prisma client created by createTestClient is explicitly disconnected between tests; reference the setup() function, createTestClient, and the $disconnect() method when implementing this cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/clients/tanstack-query/src/react.ts`:
- Around line 615-619: The infinite-query helper is incorrectly using the full
args object as initialPageParam while queryFn treats pageParam as a cursor
passed to makeUrl, making TPageParam unsound; update the types and API so
TPageParam represents the full request shape (the same type as args) or add a
public option to let callers set initialPageParam explicitly. Concretely, change
the generic/alias used for TPageParam to the args-type used by makeUrl (so
initialPageParam: args is type-correct) or add an options parameter (exposed
from the infinite-query helper) that maps to react-query's initialPageParam and
use that instead of always assigning args to initialPageParam; ensure queryFn,
makeUrl, fetcher, and the public helper signature are updated to keep types
aligned.
In `@packages/clients/tanstack-query/src/svelte/index.svelte.ts`:
- Around line 527-533: The pagination loses original filters because
initialPageParam is set from args()?. Fix by preserving the full initial request
args for subsequent pages: either change TPageParam to include both cursor and
full request shape (so getNextPageParam returns { cursor, initialArgs }) or
capture a stable initialArgs value in the closure and use that in queryFn when
pageParam is a cursor-only value; update queryFn, initialPageParam and
getNextPageParam handling (references: queryFn, initialPageParam,
getNextPageParam, TPageParam, args, makeUrl, fetcher) so every page request
composes makeUrl(endpoint, model, operation, { ...initialArgs,
...pageParamCursor }) rather than recomputing args() on later pages.
In `@packages/clients/tanstack-query/src/vue.ts`:
- Around line 551-552: The current code sets initialPageParam from argsValue
(via initialPageParam: toValue(argsValue) as TPageParam) which forces TPageParam
to represent the entire request args when stored in data.pageParams[0]; fix by
either (A) adding a new explicit option (e.g., initialPageArgs or
initialPageParamArgs) that callers can provide to override the full-args initial
page value and defaulting it to argsValue when absent, or (B) tighten the type
by constraining the generic so TPageParam extends the full request args type
(bind TPageParam to the request args shape) so callers cannot supply a narrower
cursor-only type; update references to initialPageParam, argsValue, options and
any code that reads data.pageParams[0] accordingly so the runtime page param
always matches the full request args shape or is explicitly overridden by the
new option.
---
Outside diff comments:
In `@packages/clients/tanstack-query/src/vue.ts`:
- Around line 543-550: The queryFn inside finalOptions ignores TanStack Query's
pageParam so infinite queries always request the same page; update the queryFn
signature to accept ({ signal, pageParam }) and incorporate pageParam into the
request arguments when building reqUrl (use the pageParam to override/augment
argsValue or pass it to makeUrl) so makeUrl(endpoint, model, operation, ...)
uses the current page; update any call sites using finalOptions/queryFn to
expect the pageParam handling (symbols: finalOptions, queryFn, argsValue,
makeUrl, pageParam).
---
Nitpick comments:
In `@tests/regression/test/issue-2529.test.ts`:
- Around line 6-21: The setup() helper creates a Prisma client via
createTestClient but never disconnects it, risking connection leaks; add an
afterEach hook in this test file that calls await db.$disconnect() for the
client returned by setup() (or store the db in a file-scoped variable set by
setup()), ensuring the Prisma client created by createTestClient is explicitly
disconnected between tests; reference the setup() function, createTestClient,
and the $disconnect() method when implementing this cleanup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de8b2d5e-804d-40e7-b8d6-c182b81bde72
📒 Files selected for processing (34)
package.jsonpackages/auth-adapters/better-auth/package.jsonpackages/cli/package.jsonpackages/clients/client-helpers/package.jsonpackages/clients/tanstack-query/package.jsonpackages/clients/tanstack-query/src/react.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/test/react-typing.test-d.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/clients/tanstack-query/test/vue-typing-test.tspackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/ide/vscode/package.jsonpackages/language/package.jsonpackages/orm/package.jsonpackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/zod/factory.tspackages/plugins/policy/package.jsonpackages/schema/package.jsonpackages/sdk/package.jsonpackages/server/package.jsonpackages/testtools/package.jsonpackages/zod/package.jsonsamples/orm/package.jsontests/e2e/orm/client-api/find.test.tstests/e2e/package.jsontests/regression/package.jsontests/regression/test/issue-2529.test.tstests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
orderByvalidation to enforce single-field objects, improving query clarity.Chores