Conversation
…to UpdateInput (#2627) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…er the wire Register SuperJSON custom serializers for the three JSON null sentinels in both the client-helpers fetch layer and the server's registerCustomSerializers, so they survive HTTP round-trips when used as query filters or mutation input. Re-export the singletons from all tanstack-query framework entry points (react, vue, svelte) for convenient user access. Fixes #2278 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
…tions (#2645) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…stackhq/orm types (#2641) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…signature (#2650) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `pnpm test:generate` to the build scripts of @zenstackhq/orm, @zenstackhq/schema, and @zenstackhq/zod so test fixtures are regenerated as part of `pnpm build`. Includes the resulting regeneration of packages/schema/test/schema/schema.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces a Prisma-style full-text search capability gated by a new
field-level `@fullText` ZModel attribute. PostgreSQL only — MySQL/SQLite
throw NotSupported. Mirrors the existing `@fuzzy` design.
- Filter operator: `where: { title: { fts: { search, config? } } }`
emits `to_tsvector(field) @@ to_tsquery(query)` (or with a `::regconfig`
cast when `config` is provided; otherwise Postgres uses the database's
`default_text_search_config`).
- OrderBy operator: `_ftsRelevance: { fields, search, config?, sort }`
emits a single `ts_rank(...)`. Multi-field combines fields with
`concat_ws(' ', ...)` so AND queries match terms across fields
(matches Prisma's behavior).
- Type-level gating: the `fts` operator and `_ftsRelevance` orderBy
appear only on String fields annotated with `@fullText` and only when
the schema's provider is `postgresql`. Slicing's `'FullText'` filter
kind controls availability of the runtime operator.
- Cursor pagination is rejected when combined with `_ftsRelevance`
(parallel to `_fuzzyRelevance`).
Also refactors `buildOrderBy` to dispatch to small per-branch helpers
(`applyScalarOrderBy`, `applyAggregationOrderBy`, `applyRelationOrderBy`,
`applyFuzzyRelevanceOrderBy`, `applyFtsRelevanceOrderBy`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`to_tsvector(NULL)` returns NULL and `ts_rank(NULL, ...)` therefore
returns NULL — under Postgres's default `NULLS FIRST` for `ORDER BY DESC`
this would surface NULL-valued rows ahead of any matching ones, an
asymmetry with the multi-field path where `concat_ws(' ', ...)` already
skips NULLs and yields a 0.0 rank. Coalescing the field to `''` aligns
the two paths.
Adds `subtitle: String? @fullText` to the test fixture and a regression
test that orders a NULL-subtitle row against a matching one — without
the fix, the NULL row ranks first under DESC.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#2655) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds fuzzy/full-text search and relevance ordering, checked/unchecked CRUD inputs, a typed fetch client, transaction typing and $transaction hooks in TanStack bindings, Changes3.7.0 Unified Update Core checkpoints
Estimated code review effort: Possibly related PRs:
Poem:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
|
Claude finished @ymc9's task —— View job Code Review for v3.7.0 Release PRThis is a substantial release with 100+ changed files including new packages, major enhancements, and important bug fixes. I've conducted a comprehensive review focusing on code quality, security, performance, and potential issues. Review Tasks:
✅ Strengths and Positive Changes🎯 New Fetch Client Package
🔧 ORM Enhancements
📊 TanStack Query Improvements
|
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
17-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate pnpm version to match coding guidelines.
The
packageManagerfield specifiespnpm@10.33.0, but coding guidelines requirepnpm@10.12.1. Using a different pnpm version may lead to inconsistencies in dependency resolution and lockfile generation.As per coding guidelines, the pnpm version should be pinned to
pnpm@10.12.1.📦 Proposed fix
- "packageManager": "pnpm@10.33.0", + "packageManager": "pnpm@10.12.1",🤖 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 `@package.json` at line 17, Update the packageManager entry in package.json from "pnpm@10.33.0" to the required "pnpm@10.12.1" to comply with coding guidelines; locate the packageManager field in package.json and change its value accordingly, then regenerate/install dependencies with the updated pnpm version (e.g., run pnpm install using pnpm@10.12.1) to ensure the lockfile and node_modules match the pinned manager.
🧹 Nitpick comments (3)
packages/clients/tanstack-query/test/react/optimistic-mutation.test.tsx (1)
263-264: ⚡ Quick winReplace TODO with an explicit assertion to lock behavior.
This TODO leaves expected nested relation behavior undefined. Please assert either inclusion of
categoryobject or explicit absence, so regressions are detectable.If you want, I can draft the exact assertion once you confirm the intended behavior.
🤖 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/clients/tanstack-query/test/react/optimistic-mutation.test.tsx` around lines 263 - 264, The test in optimistic-mutation.test.tsx leaves a TODO instead of a concrete assertion about nested relation behavior; replace the commented line near the optimistic mutation assertion with an explicit expectation that either checks for the presence of the category object (e.g., that the mutation result or cache contains category: { $optimistic: true, id: '1', name: 'category1' }) or explicitly asserts its absence, so the test locks the intended behavior; update the assertion surrounding the mutation result (the optimistic mutation assertion in the test) to reflect the chosen behavior and remove the TODO comment.packages/orm/src/client/client-impl.ts (1)
271-276: ⚡ Quick winReturn the validated variable instead of re-accessing the property.
Line 275 re-accesses
(promise as any).cbinstead of returning the already-validatedcbvariable from line 273, causing a redundant property lookup.♻️ Proposed fix
private getPromiseCallback(promise: ZenStackPromise<any>) { invariant((promise as any).cb, 'Invalid ZenStackPromise, missing cb property'); const cb = (promise as any).cb; invariant(typeof cb === 'function', 'Invalid ZenStackPromise, cb property is not a function'); - return (promise as any).cb; + return cb; }🤖 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/orm/src/client/client-impl.ts` around lines 271 - 276, In getPromiseCallback, you're re-accessing (promise as any).cb on return instead of returning the validated local variable; change the return to return the already-validated cb from line where const cb is declared to avoid redundant property lookup and ensure the function returns the validated function value (references: getPromiseCallback, variable cb, ZenStackPromise).packages/clients/fetch-client/test/fetch-client.test.ts (1)
444-463: ⚡ Quick winUse real special values in the SuperJSON tests.
These cases only pass plain strings, so they never hit the serialization path the test names are asserting. A regression in Decimal/Date/SuperJSON marshaling would still pass here.
Also applies to: 558-567
🤖 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/clients/fetch-client/test/fetch-client.test.ts` around lines 444 - 463, Tests named "serializes args with special types into query string" and "marshals args with Decimal into POST body" are using plain strings so they never exercise the SuperJSON/Decimal serialization paths; update those tests to pass real special values (e.g., Date objects and Decimal instances) instead of strings when calling client.user.findMany and client.user.createMany, and similarly fix the other affected test block later in the file that checks Decimal/Date marshaling; ensure mockFetch assertions still read the URL query (?q=) and the POST init.body JSON after these real special values are passed so the serialization path is actually exercised.
🤖 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/clients/fetch-client/src/index.ts`:
- Around line 159-169: normalizeEndpoint currently accepts fully qualified URLs
that include query strings or fragments which later cause malformed concatenated
paths; update normalizeEndpoint to reject endpoints containing '?' or '#' by
validating the parsed URL (use new URL(endpoint)) and throwing an Error if
url.search or url.hash are non-empty, keeping the existing checks for non-empty
string and trailing slash trimming; reference the function normalizeEndpoint and
the URL constructor to locate and implement this change.
In `@packages/clients/fetch-client/test/fetch-client.test.ts`:
- Around line 22-29: The tests replace globalThis.fetch with mockFetch but never
restore it, risking cross-test pollution; update the setup/teardown around
beforeEach/afterEach to save the original fetch (e.g., const originalFetch =
globalThis.fetch) before assigning mockFetch in beforeEach and then restore
globalThis.fetch = originalFetch in afterEach, and keep vi.resetAllMocks() there
as well; refer to the existing beforeEach, afterEach, and mockFetch symbols to
locate where to add the save/restore.
In `@packages/clients/tanstack-query/test/react/helpers.tsx`:
- Around line 24-26: The URL builder currently skips serializing valid falsy
values because it checks `if (args)`; update the condition to explicitly test
for undefined (e.g., `if (args !== undefined)`) so values like null, 0, false,
or empty string are preserved and still get serialized with
`encodeURIComponent(JSON.stringify(args))` — change the `if (args)` check around
the `r += \`?q=${encodeURIComponent(JSON.stringify(args))}\`` line accordingly.
In `@packages/orm/src/client/contract.ts`:
- Around line 384-391: The createManyAndReturn method signature mistakenly makes
its args parameter optional, allowing invalid calls that lack the required data
to typecheck; update the signature(s) for createManyAndReturn (and the other
occurrence noted) to require args by removing the optional marker so args:
SelectSubset<...> (not args?: ...), ensuring the mapped CrudArgsType that
requires data is enforced at compile time.
In `@packages/orm/src/client/crud/dialects/base-dialect.ts`:
- Around line 1227-1250: The applyScalarOrderBy currently only handles
object-based ordering when both 'sort' and 'nulls' exist; update it so an object
with a valid 'sort' is honored even if 'nulls' is omitted: in applyScalarOrderBy
check for an object with 'sort' === 'asc'|'desc' and if 'nulls' is present call
buildOrderByField(query, fieldRef, this.negateSort(value.sort, negated),
value.nulls) but if 'nulls' is absent call query.orderBy(fieldRef,
this.negateSort(value.sort, negated)); keep using the existing helper negateSort
and buildOrderByField to locate the code paths.
In `@packages/orm/src/client/crud/dialects/postgresql.ts`:
- Around line 651-676: The GREATEST(...) call can return NULL if any per-field
similarity is NULL; update buildFuzzyRelevanceOrderBy so multi-field similarity
expressions are wrapped with COALESCE(..., 0) before passing to GREATEST (and
optionally do the same for the single-field path) — modify the mapping where
similarities is built (and the single-field orderBy call) to use COALESCE around
each buildSimilarity(...) result (referencing the buildFuzzyRelevanceOrderBy
function and the buildSimilarity helper) so NULL scores become 0 and don't null
out GREATEST.
In `@tests/e2e/orm/client-api/full-text-search.test.ts`:
- Around line 428-462: The test creates a temporary client (db via
createTestClient) and may leak it if an assertion throws before the final
db.$disconnect(); wrap the lifecycle for the temporary client in a try/finally:
after creating db (from createTestClient) run the test logic in try and always
call await db.$disconnect() in finally; also keep the existing suite-level
client.$disconnect() behavior untouched and apply the same try/finally pattern
to the other two tests referenced (the blocks around lines with createTestClient
and db.$disconnect()).
In `@tests/e2e/orm/schemas/delegate/typecheck.ts`:
- Around line 104-114: The tests for discriminator field assignments were
commented out because TS6 changed type assignability for create/update/upsert
but updateMany still compiles; investigate by comparing the generated Prisma
client parameter types for the affected operations (inspect Parameters<typeof
client.ratedVideo.create>, Parameters<typeof client.ratedVideo.update>,
Parameters<typeof client.ratedVideo.upsert>, and Parameters<typeof
client.ratedVideo.updateMany>) to see which input types enforce
discriminated-union constraints vs. more permissive UpdateMany input types, then
verify whether the proposed Parameters<typeof ...> wrapper (using those
Parameters types to construct a correctly-typed arg) actually allows the calls
under TS6; if it does, un-comment and replace the tests to use that pattern (or
add precise type-assertion tests using a type-testing tool if needed), otherwise
adjust the tests to use the correct shape the client expects (or mark only the
failing sites with `@ts-expect-error` and add a comment explaining the TS6
incompatibility) and add a short note in the test describing the root cause and
the chosen fix.
In `@tests/e2e/performance/tsc-torture/main.ts`:
- Around line 21-68: The cleanupAll function's deletion order is not FK-safe:
move db.commentReaction.deleteMany() (and any other tables that reference
Comment such as db.reviewComment.deleteMany()) to run before
db.comment.deleteMany(), and ensure self-referential Comment replies are removed
before parent comments by deleting child-related rows (replies/reactions) prior
to calling db.comment.deleteMany(); update the sequence inside cleanupAll so all
tables that have non-cascading relations to Comment (e.g., commentReaction,
reviewComment, any reply/junction rows) are deleted first.
---
Outside diff comments:
In `@package.json`:
- Line 17: Update the packageManager entry in package.json from "pnpm@10.33.0"
to the required "pnpm@10.12.1" to comply with coding guidelines; locate the
packageManager field in package.json and change its value accordingly, then
regenerate/install dependencies with the updated pnpm version (e.g., run pnpm
install using pnpm@10.12.1) to ensure the lockfile and node_modules match the
pinned manager.
---
Nitpick comments:
In `@packages/clients/fetch-client/test/fetch-client.test.ts`:
- Around line 444-463: Tests named "serializes args with special types into
query string" and "marshals args with Decimal into POST body" are using plain
strings so they never exercise the SuperJSON/Decimal serialization paths; update
those tests to pass real special values (e.g., Date objects and Decimal
instances) instead of strings when calling client.user.findMany and
client.user.createMany, and similarly fix the other affected test block later in
the file that checks Decimal/Date marshaling; ensure mockFetch assertions still
read the URL query (?q=) and the POST init.body JSON after these real special
values are passed so the serialization path is actually exercised.
In `@packages/clients/tanstack-query/test/react/optimistic-mutation.test.tsx`:
- Around line 263-264: The test in optimistic-mutation.test.tsx leaves a TODO
instead of a concrete assertion about nested relation behavior; replace the
commented line near the optimistic mutation assertion with an explicit
expectation that either checks for the presence of the category object (e.g.,
that the mutation result or cache contains category: { $optimistic: true, id:
'1', name: 'category1' }) or explicitly asserts its absence, so the test locks
the intended behavior; update the assertion surrounding the mutation result (the
optimistic mutation assertion in the test) to reflect the chosen behavior and
remove the TODO comment.
In `@packages/orm/src/client/client-impl.ts`:
- Around line 271-276: In getPromiseCallback, you're re-accessing (promise as
any).cb on return instead of returning the validated local variable; change the
return to return the already-validated cb from line where const cb is declared
to avoid redundant property lookup and ensure the function returns the validated
function value (references: getPromiseCallback, variable cb, ZenStackPromise).
🪄 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: 98a7d68d-5c4d-436c-8d89-f6442500d1d7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (145)
package.jsonpackages/auth-adapters/better-auth/package.jsonpackages/auth-adapters/better-auth/src/adapter.tspackages/auth-adapters/better-auth/tsconfig.jsonpackages/auth-adapters/better-auth/tsdown.config.tspackages/cli/package.jsonpackages/cli/src/actions/proxy.tspackages/cli/test/plugins/custom-plugin.test.tspackages/cli/tsdown.config.tspackages/clients/client-helpers/package.jsonpackages/clients/client-helpers/src/constants.tspackages/clients/client-helpers/src/fetch.tspackages/clients/client-helpers/src/index.tspackages/clients/client-helpers/src/invalidation.tspackages/clients/client-helpers/src/nested-write-visitor.tspackages/clients/client-helpers/src/transaction.tspackages/clients/client-helpers/src/types.tspackages/clients/client-helpers/test/nested-write-visitor.test.tspackages/clients/fetch-client/eslint.config.jspackages/clients/fetch-client/package.jsonpackages/clients/fetch-client/src/index.tspackages/clients/fetch-client/test/fetch-client.test.tspackages/clients/fetch-client/test/schemas/basic/schema-lite.tspackages/clients/fetch-client/test/schemas/basic/schema.zmodelpackages/clients/fetch-client/test/schemas/no-procs/schema-lite.tspackages/clients/fetch-client/test/schemas/no-procs/schema.zmodelpackages/clients/fetch-client/test/typing.test-d.tspackages/clients/fetch-client/tsconfig.jsonpackages/clients/fetch-client/tsconfig.test.jsonpackages/clients/fetch-client/tsdown.config.tspackages/clients/fetch-client/vitest.config.tspackages/clients/tanstack-query/package.jsonpackages/clients/tanstack-query/src/common/client.tspackages/clients/tanstack-query/src/common/constants.tspackages/clients/tanstack-query/src/common/transaction.tspackages/clients/tanstack-query/src/common/types.tspackages/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/crud-and-invalidation.test.tsxpackages/clients/tanstack-query/test/react/helpers.tsxpackages/clients/tanstack-query/test/react/json-null-serialization.test.tsxpackages/clients/tanstack-query/test/react/optimistic-mutation.test.tsxpackages/clients/tanstack-query/test/react/react-sliced-client.test-d.tspackages/clients/tanstack-query/test/react/react-typing.test-d.tspackages/clients/tanstack-query/test/react/sequential-transaction.test.tsxpackages/clients/tanstack-query/test/svelte/svelte-sliced-client.test-d.tspackages/clients/tanstack-query/test/svelte/svelte-typing-test.tspackages/clients/tanstack-query/test/vue/vue-sliced-client.test-d.tspackages/clients/tanstack-query/test/vue/vue-typing-test.tspackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/tsdown-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/ide/vscode/package.jsonpackages/language/package.jsonpackages/language/res/stdlib.zmodelpackages/language/src/document.tspackages/language/src/utils.tspackages/language/src/validators/attribute-application-validator.tspackages/language/test/attribute-application.test.tspackages/orm/package.jsonpackages/orm/src/client/client-impl.tspackages/orm/src/client/constants.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/dialects/mysql.tspackages/orm/src/client/crud/dialects/postgresql.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/plugin.tspackages/orm/src/client/promise.tspackages/orm/src/client/zod/factory.tspackages/orm/src/utils/type-utils.tspackages/orm/test/schema/models.tspackages/orm/test/schema/schema.tspackages/orm/test/schema/schema.zmodelpackages/orm/test/zod-compat.test.tspackages/orm/tsdown.config.tspackages/plugins/policy/package.jsonpackages/schema/package.jsonpackages/schema/src/schema.tspackages/schema/test/schema/schema.tspackages/sdk/package.jsonpackages/sdk/src/ts-schema-generator.tspackages/server/package.jsonpackages/server/src/api/utils.tspackages/testtools/package.jsonpackages/testtools/src/project.tspackages/zod/package.jsonpackages/zod/src/index.tspackages/zod/src/types.tspackages/zod/test/factory.test.tspackages/zod/test/schema/schema.tspnpm-workspace.yamlsamples/next.js/app/global.d.tssamples/next.js/zenstack/input.tssamples/nuxt/zenstack/input.tssamples/orm/package.jsonsamples/orm/zenstack/input.tssamples/sveltekit/src/zenstack/input.tsscripts/test-generate.tstests/e2e/orm/client-api/checked-unchecked.test-d.tstests/e2e/orm/client-api/checked-unchecked.test.tstests/e2e/orm/client-api/delegate.test.tstests/e2e/orm/client-api/full-text-search.test.tstests/e2e/orm/client-api/fuzzy-search.test.tstests/e2e/orm/client-api/json-filter.test.tstests/e2e/orm/client-api/slicing.test.tstests/e2e/orm/client-api/unsupported.test.tstests/e2e/orm/client-api/update-many.test.tstests/e2e/orm/client-api/update.test.tstests/e2e/orm/schemas/basic/input.tstests/e2e/orm/schemas/delegate/typecheck.tstests/e2e/orm/schemas/full-text-search/schema.tstests/e2e/orm/schemas/full-text-search/schema.zmodeltests/e2e/orm/schemas/fuzzy-search/index.tstests/e2e/orm/schemas/fuzzy-search/schema.tstests/e2e/orm/schemas/fuzzy-search/schema.zmodeltests/e2e/package.jsontests/e2e/performance/tsc-torture/main.tstests/e2e/performance/tsc-torture/tsconfig.test.jsontests/e2e/performance/tsc-torture/zenstack/schema.tstests/e2e/performance/tsc-torture/zenstack/schema.zmodeltests/regression/package.jsontests/regression/test/issue-2567/input.tstests/regression/test/issue-2567/models.tstests/regression/test/issue-2567/regression.test.tstests/regression/test/issue-2567/schema.tstests/regression/test/issue-2567/schema.zmodeltests/regression/test/issue-2631.test.tstests/regression/test/issue-2633.test.tstests/regression/test/issue-2639/regression.test.tstests/regression/test/issue-2639/schema.tstests/regression/test/issue-2639/schema.zmodeltests/regression/test/issue-2647/regression.test.tstests/regression/test/issue-2647/schema.tstests/regression/test/issue-2647/schema.zmodeltests/regression/test/issue-2654.test.tstests/regression/test/policy-plugin-detection.test.tstests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
💤 Files with no reviewable changes (3)
- tests/e2e/orm/client-api/delegate.test.ts
- packages/clients/client-helpers/src/nested-write-visitor.ts
- packages/clients/client-helpers/test/nested-write-visitor.test.ts
Comment on lines
+1227
to
+1250
| private applyScalarOrderBy( | ||
| query: SelectQueryBuilder<any, any, any>, | ||
| model: string, | ||
| modelAlias: string, | ||
| field: string, | ||
| value: any, | ||
| negated: boolean, | ||
| buildFieldRef: (model: string, field: string, modelAlias: string) => Expression<any>, | ||
| ): SelectQueryBuilder<any, any, any> { | ||
| const fieldRef = buildFieldRef(model, field, modelAlias); | ||
| if (value === 'asc' || value === 'desc') { | ||
| return query.orderBy(fieldRef, this.negateSort(value, negated)); | ||
| } | ||
| if ( | ||
| typeof value === 'object' && | ||
| 'nulls' in value && | ||
| 'sort' in value && | ||
| (value.sort === 'asc' || value.sort === 'desc') && | ||
| (value.nulls === 'first' || value.nulls === 'last') | ||
| ) { | ||
| return this.buildOrderByField(query, fieldRef, this.negateSort(value.sort, negated), value.nulls); | ||
| } | ||
| return query; | ||
| } |
Contributor
There was a problem hiding this comment.
Honor { sort } when nulls is omitted.
The public OrderBy shape allows { sort: 'asc' | 'desc', nulls?: ... }, but this branch only applies object-based ordering when nulls is present. orderBy: { deletedAt: { sort: 'desc' } } will currently be ignored instead of sorting.
Suggested fix
if (
typeof value === 'object' &&
- 'nulls' in value &&
'sort' in value &&
(value.sort === 'asc' || value.sort === 'desc') &&
- (value.nulls === 'first' || value.nulls === 'last')
- ) {
- return this.buildOrderByField(query, fieldRef, this.negateSort(value.sort, negated), value.nulls);
+ (!('nulls' in value) || value.nulls === 'first' || value.nulls === 'last')
+ ) {
+ const sort = this.negateSort(value.sort, negated);
+ return 'nulls' in value && value.nulls
+ ? this.buildOrderByField(query, fieldRef, sort, value.nulls)
+ : query.orderBy(fieldRef, sort);
}🤖 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/orm/src/client/crud/dialects/base-dialect.ts` around lines 1227 -
1250, The applyScalarOrderBy currently only handles object-based ordering when
both 'sort' and 'nulls' exist; update it so an object with a valid 'sort' is
honored even if 'nulls' is omitted: in applyScalarOrderBy check for an object
with 'sort' === 'asc'|'desc' and if 'nulls' is present call
buildOrderByField(query, fieldRef, this.negateSort(value.sort, negated),
value.nulls) but if 'nulls' is absent call query.orderBy(fieldRef,
this.negateSort(value.sort, negated)); keep using the existing helper negateSort
and buildOrderByField to locate the code paths.
Comment on lines
+651
to
+676
| override buildFuzzyRelevanceOrderBy( | ||
| query: SelectQueryBuilder<any, any, any>, | ||
| fieldRefs: Expression<any>[], | ||
| search: string, | ||
| sort: SortOrder, | ||
| mode: FuzzyFilterOptions['mode'], | ||
| unaccent: boolean, | ||
| ): SelectQueryBuilder<any, any, any> { | ||
| const valueExpr = this.normalizeForTrigram(sql.val(search), unaccent); | ||
| const buildSimilarity = (fieldRef: Expression<any>) => { | ||
| const fieldExpr = this.normalizeForTrigram(fieldRef, unaccent); | ||
| switch (mode) { | ||
| case 'simple': | ||
| return sql`similarity(${fieldExpr}, ${valueExpr})`; | ||
| case 'word': | ||
| return sql`word_similarity(${valueExpr}, ${fieldExpr})`; | ||
| case 'strictWord': | ||
| return sql`strict_word_similarity(${valueExpr}, ${fieldExpr})`; | ||
| } | ||
| }; | ||
|
|
||
| if (fieldRefs.length === 1) { | ||
| return query.orderBy(buildSimilarity(fieldRefs[0]!), sort); | ||
| } | ||
| const similarities = fieldRefs.map((ref) => buildSimilarity(ref)); | ||
| return query.orderBy(sql`GREATEST(${sql.join(similarities)})`, sort); |
Contributor
There was a problem hiding this comment.
Coalesce nullable scores before GREATEST(...).
For multi-field _fuzzyRelevance, one nullable field is enough to make GREATEST(...) evaluate to NULL, which discards the other field scores and can put partial-NULL rows ahead of real matches under DESC.
Suggested fix
- const similarities = fieldRefs.map((ref) => buildSimilarity(ref));
+ const similarities = fieldRefs.map((ref) => sql`coalesce(${buildSimilarity(ref)}, 0)`);
return query.orderBy(sql`GREATEST(${sql.join(similarities)})`, sort);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| override buildFuzzyRelevanceOrderBy( | |
| query: SelectQueryBuilder<any, any, any>, | |
| fieldRefs: Expression<any>[], | |
| search: string, | |
| sort: SortOrder, | |
| mode: FuzzyFilterOptions['mode'], | |
| unaccent: boolean, | |
| ): SelectQueryBuilder<any, any, any> { | |
| const valueExpr = this.normalizeForTrigram(sql.val(search), unaccent); | |
| const buildSimilarity = (fieldRef: Expression<any>) => { | |
| const fieldExpr = this.normalizeForTrigram(fieldRef, unaccent); | |
| switch (mode) { | |
| case 'simple': | |
| return sql`similarity(${fieldExpr}, ${valueExpr})`; | |
| case 'word': | |
| return sql`word_similarity(${valueExpr}, ${fieldExpr})`; | |
| case 'strictWord': | |
| return sql`strict_word_similarity(${valueExpr}, ${fieldExpr})`; | |
| } | |
| }; | |
| if (fieldRefs.length === 1) { | |
| return query.orderBy(buildSimilarity(fieldRefs[0]!), sort); | |
| } | |
| const similarities = fieldRefs.map((ref) => buildSimilarity(ref)); | |
| return query.orderBy(sql`GREATEST(${sql.join(similarities)})`, sort); | |
| override buildFuzzyRelevanceOrderBy( | |
| query: SelectQueryBuilder<any, any, any>, | |
| fieldRefs: Expression<any>[], | |
| search: string, | |
| sort: SortOrder, | |
| mode: FuzzyFilterOptions['mode'], | |
| unaccent: boolean, | |
| ): SelectQueryBuilder<any, any, any> { | |
| const valueExpr = this.normalizeForTrigram(sql.val(search), unaccent); | |
| const buildSimilarity = (fieldRef: Expression<any>) => { | |
| const fieldExpr = this.normalizeForTrigram(fieldRef, unaccent); | |
| switch (mode) { | |
| case 'simple': | |
| return sql`similarity(${fieldExpr}, ${valueExpr})`; | |
| case 'word': | |
| return sql`word_similarity(${valueExpr}, ${fieldExpr})`; | |
| case 'strictWord': | |
| return sql`strict_word_similarity(${valueExpr}, ${fieldExpr})`; | |
| } | |
| }; | |
| if (fieldRefs.length === 1) { | |
| return query.orderBy(buildSimilarity(fieldRefs[0]!), sort); | |
| } | |
| const similarities = fieldRefs.map((ref) => sql`coalesce(${buildSimilarity(ref)}, 0)`); | |
| return query.orderBy(sql`GREATEST(${sql.join(similarities)})`, sort); |
🤖 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/orm/src/client/crud/dialects/postgresql.ts` around lines 651 - 676,
The GREATEST(...) call can return NULL if any per-field similarity is NULL;
update buildFuzzyRelevanceOrderBy so multi-field similarity expressions are
wrapped with COALESCE(..., 0) before passing to GREATEST (and optionally do the
same for the single-field path) — modify the mapping where similarities is built
(and the single-field orderBy call) to use COALESCE around each
buildSimilarity(...) result (referencing the buildFuzzyRelevanceOrderBy function
and the buildSimilarity helper) so NULL scores become 0 and don't null out
GREATEST.
Member
Author
There was a problem hiding this comment.
Are you sure one null value will cause GREATEST to return null?
Contributor
|
Claude finished @ymc9's task —— View job Code Review for v3.7.0 Release PRThis is a substantial release with 100+ changed files including new packages, major enhancements, and important bug fixes. I've conducted a comprehensive review focusing on code quality, security, performance, and potential issues. Review Tasks:
✅ Strengths and Positive Changes🎯 New Fetch Client Package
🔧 ORM Enhancements
📊 TanStack Query Improvements
|
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
5 participants
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
@fuzzy) and full‑text (@fullText) search support (Postgres)Bug Fixes / Improvements