Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…-auth/core instance Without an explicit kysely devDependency, pnpm resolved @better-auth/core's kysely peer dep to 0.28.8 (from better-auth's bundled copy) instead of the workspace-catalog 0.28.16, creating two incompatible @better-auth/core instances and causing TypeScript type errors at build time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…xist` error when the model is explicitly included (#2539) Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lude relation fields from makeModelSchema by default + add "optionality" setting to control runtime optionality of fields (#2525)
…ilters (#2579) Co-authored-by: evgenovalov <evgenii@flowlity.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…use cases (#2583) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
Claude finished @ymc9's task —— View job Code Review for v3.6.0 Release (dev → main)I've completed my comprehensive code review of this major release. Here are my findings: Review Checklist:
🏗️ Build System Migration Analysis ✅Positive Changes:
📦 Dependency Updates & Package Changes ✅Key Observations:
🔧 Code Quality & Best Practices ✅Strengths:
🐛 Potential Issues & Bugs
|
Contributor
📝 WalkthroughWalkthroughThe PR migrates build tooling from tsup to tsdown (new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/zod/src/utils.ts (1)
447-466:⚠️ Potential issue | 🟠 MajorSkip
ABSENTfor membership predicates too.Line 439 skips omitted fields for ordered comparisons, but
?,!,^, andinstill treatABSENTas a real operand and returnfalse. A partial payload can therefore fail rules like@@validate(tags ? "x")or@@validate(status in ["A"])even when the field was not provided.🐛 Proposed fix
case '?': + if (left === ABSENT || right === ABSENT) return true; if (!Array.isArray(left)) { return false; } return left.some((item) => item === right); case '!': + if (left === ABSENT || right === ABSENT) return true; if (!Array.isArray(left)) { return false; } return left.every((item) => item === right); case '^': + if (left === ABSENT || right === ABSENT) return true; if (!Array.isArray(left)) { return false; } return !left.some((item) => item === right); case 'in': + if (left === ABSENT || right === ABSENT) return true; if (!Array.isArray(right)) { return false; } return right.includes(left);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod/src/utils.ts` around lines 447 - 466, The membership predicate cases ('?', '!', '^', 'in') currently treat the ABSENT sentinel as a real value and fail; update each case to early-return true when the relevant operand is ABSENT (i.e., for '?', '!', '^' if left === ABSENT return true; for 'in' if left === ABSENT or right === ABSENT return true / or at minimum if the operand you expect to be omitted equals ABSENT return true) so omitted fields are skipped like the ordered-comparison branch does; use the existing ABSENT constant and modify the switch cases for '?', '!', '^', and 'in' accordingly.packages/zod/src/factory.ts (1)
135-166:⚠️ Potential issue | 🟠 MajorDon’t deprecate these helpers to a non-equivalent replacement.
Line 136 and Line 165 point users to
makeModelSchema(...), but that path still includes computed and discriminator fields, whilemakeModelCreateSchema/makeModelUpdateSchemaexplicitly exclude them. Following this deprecation would make create/update validators reject valid payloads or require output-only fields.Proposed fix
/** - * `@deprecated` Use `makeModelSchema(model, { optionality: 'defaults' })` instead. + * Creates a schema for create-input payloads. This is not equivalent to + * `makeModelSchema(model, { optionality: 'defaults' })` because create inputs + * exclude computed and discriminator fields. */ makeModelCreateSchema<Model extends GetModels<Schema>>( /** - * `@deprecated` Use `makeModelSchema(model, { optionality: 'all' })` instead. + * Creates a schema for update-input payloads. This is not equivalent to + * `makeModelSchema(model, { optionality: 'all' })` because update inputs + * exclude computed and discriminator fields. */ makeModelUpdateSchema<Model extends GetModels<Schema>>(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod/src/factory.ts` around lines 135 - 166, The deprecation note incorrectly points users to makeModelSchema(...) which still includes computed and discriminator fields, unlike makeModelCreateSchema and makeModelUpdateSchema that explicitly exclude them; update the API or docs so behavior is preserved: either stop deprecating makeModelCreateSchema/makeModelUpdateSchema or change makeModelSchema signature to accept flags (e.g., excludeComputed/excludeDiscriminator or a mode for create/update) and update callers to call makeModelSchema(model, { optionality: 'defaults', excludeComputed: true, excludeDiscriminator: true }) (or equivalent) so computed and isDiscriminator fields are omitted, and adjust applyDescription/addCustomValidation usage in makeModelCreateSchema/makeModelUpdateSchema to delegate to the new option-based makeModelSchema implementation; reference the functions makeModelCreateSchema, makeModelUpdateSchema, and makeModelSchema and the field properties computed and isDiscriminator when making the change.packages/zod/src/types.ts (1)
318-332:⚠️ Potential issue | 🟡 MinorKeep select typing aligned with
true-only runtime validation.
ModelSchemaOptionsnow rejectsfalse, butSelectEntryToZodstill maps anybooleanto an included field. If a caller’s option type is widened or passed through a generic,falsecan produce a selected field at the type level while runtime validation rejects it.Proposed type tightening
-> = Value extends boolean - ? // `true` or widened `boolean` — use the default shape for this field. - // Handling `boolean` (not just literal `true`) prevents the type from - // collapsing to `never` when callers use a boolean variable instead of - // a literal (e.g. `const pick: boolean = true`). +> = Value extends true + ? GetAllModelFieldsShape<Schema, Model>[FieldInShape<Schema, Model, Field>]Also applies to: 338-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod/src/types.ts` around lines 318 - 332, SelectEntryToZod currently treats any boolean as a truthy selection which can type-select fields even when runtime accepts only literal true; change the boolean branch to require the literal true (i.e. use Value extends true) so only explicit true maps to GetAllModelFieldsShape via FieldInShape, and similarly tighten the analogous type(s) around lines 338-340 to use Value extends true instead of Value extends boolean; keep RelationFieldZodWithOptions handling for object values unchanged.packages/orm/src/client/zod/factory.ts (1)
1367-1377:⚠️ Potential issue | 🟠 MajorDon’t make
createManyAndReturnargs optional.Line 1375 makes the schema accept
undefined, butcreateManyAndReturnrequires adatapayload and the return type is not| undefined. This can let an invalid mutation through validation.Proposed fix
- const schema = this.refineForSelectHasTruthyField( - this.refineForSelectOmitMutuallyExclusive(result), - ).optional() as ZodType<CreateManyAndReturnArgs<Schema, Model, Options, ExtQueryArgs>>; + const schema = this.refineForSelectHasTruthyField( + this.refineForSelectOmitMutuallyExclusive(result), + ) as ZodType<CreateManyAndReturnArgs<Schema, Model, Options, ExtQueryArgs>>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/factory.ts` around lines 1367 - 1377, The generated schema for createManyAndReturn (registered as `${model}CreateManyAndReturnArgs`) is being marked optional, allowing undefined through validation; remove the .optional() call so the final schema produced by makeCreateManyPayloadSchema / mergePluginArgsSchema and then refined by refineForSelectOmitMutuallyExclusive and refineForSelectHasTruthyField is required, ensuring the required data payload is validated for createManyAndReturn; update the construction where result is refined and registered (the code calling refineForSelectHasTruthyField(...).optional()) to drop .optional() so registerSchema and the exported CreateManyAndReturnArgs type no longer accept undefined.
🧹 Nitpick comments (9)
tests/e2e/orm/client-api/timezone/pg-timezone.test.ts (1)
634-642: Tighten direct-select assertions for all time fields.The direct-select path should validate
closeandopenTznormalization, not just theirDateinstance type. Keep the loosereffectiveOncheck as-is.Proposed assertions
expect(windows[0].open).toBeInstanceOf(Date); expect(windows[0].open.toISOString()).toBe('1970-01-01T09:30:00.000Z'); expect(windows[0].close).toBeInstanceOf(Date); + expect(windows[0].close.toISOString()).toBe('1970-01-01T16:00:00.000Z'); expect(windows[0].openTz).toBeInstanceOf(Date); + expect(windows[0].openTz.toISOString()).toBe('1970-01-01T09:30:00.000Z'); // On direct select pg's default DATE parser returns a Date anchored in local🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/timezone/pg-timezone.test.ts` around lines 634 - 642, The direct-select assertions are too loose for the time fields — tighten them by asserting that windows[0].close and windows[0].openTz are Date instances and that their ISO strings match the expected normalized UTC values (similar to the existing check for open.toISOString()), while keeping windows[0].effectiveOn only asserted as a Date; update the assertions referring to windows[0].close and windows[0].openTz in the pg-timezone.test.ts block so they validate both instance type and toISOString equality.tests/e2e/orm/client-api/timezone/mysql-timezone.test.ts (1)
80-84: Assert the direct-selectclosevalue too.
Dateinstance checks can still pass for invalid dates. The nested test assertsclose.toISOString(), so the direct-select path should do the same to fully cover the regression.Proposed assertion
expect(windows[0].open).toBeInstanceOf(Date); expect(windows[0].open.toISOString()).toBe('1970-01-01T09:30:00.000Z'); expect(windows[0].close).toBeInstanceOf(Date); + expect(windows[0].close.toISOString()).toBe('1970-01-01T16:00:00.000Z');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/timezone/mysql-timezone.test.ts` around lines 80 - 84, The test currently only asserts that windows[0].close is a Date instance; update the assertion for the direct-select path to assert the actual ISO string as well by calling windows[0].close.toISOString() and comparing it to the expected value '1970-01-01T16:00:00.000Z' (matching the nested test's check), so the variable windows and its element windows[0].close are validated for the correct timestamp rather than just Date validity.tests/regression/test/issue-2578.test.ts (1)
50-51: Nit: trivial sanity assertions can be dropped.Lines 50-51 and 71 just re-assert ids exist after
createalready returned them. They don't strengthen the test and only make intent slightly noisier — consider removing if not needed for readability.🧹 Proposed trim
- // keep references live so the test intent is readable - expect([p1.id, p2.id, p3.id].length).toBe(3);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2578.test.ts` around lines 50 - 51, Remove the trivial sanity assertions that only re-check created ids (e.g., the expect([p1.id, p2.id, p3.id].length).toBe(3) line and the similar assertion around line 71); locate the assertions referencing p1.id, p2.id, p3.id (and the analogous id-check later) in the test and delete those expect(...) lines so the test reads cleaner while preserving the meaningful assertions that follow create.packages/clients/client-helpers/tsdown.config.ts (1)
8-8: Minor: explicitformat: ['esm']may be redundant.Most other packages in this PR (e.g.,
packages/sdk,packages/orm,packages/auth-adapters/better-auth) rely on the default from@zenstackhq/tsdown-configwithout overridingformat. If the shared config already defaults to ESM, consider dropping this override for consistency; if ESM-only is intentional here (vs. other packages that also emit CJS), a short comment would help readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/clients/client-helpers/tsdown.config.ts` at line 8, The explicit format: ['esm'] setting in tsdown.config.ts appears redundant with the shared `@zenstackhq/tsdown-config` default; either remove the override to align with other packages (delete the format property) or, if you intentionally require ESM-only output for this package, keep the format: ['esm'] entry and add a one-line comment explaining why ESM-only is required so readers understand the deviation from the shared config.packages/config/tsdown-config/src/index.ts (1)
9-17: Shallow merge may silently drop array defaults when overriding.
...overridesshallow-merges over the defaults, so a caller passingformat: ['esm']replaces the default['cjs','esm'](probably intended), but passing a partial config object like{ dts: { ... } }also wholly replacesdts: true. This is fine for the current usage pattern, but worth documenting that overrides are shallow. Also, sincedefineConfigaccepts an array/function form, typingConfig = Parameters<typeof defineConfig>[0]permits non-object inputs that would break the spread — constrainingoverridesto the object variant would be safer.Proposed tightening
-type Config = Parameters<typeof defineConfig>[0]; +type Config = Extract<Parameters<typeof defineConfig>[0], Record<string, unknown>>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/tsdown-config/src/index.ts` around lines 9 - 17, The createConfig function currently accepts a broad Config type derived from Parameters<typeof defineConfig>[0], which can include non-object forms and makes the spread merge unsafe and shallow; tighten the overrides parameter to only accept the object form of defineConfig's parameter (so callers can’t pass an array/function) and either document that the merge is shallow (so keys like format/dts are replaced entirely) or implement a deep-merge if true merging is required; update the signature and add a short comment above createConfig mentioning "shallow merge: overrides replace defaults" and reference the overrides parameter, createConfig function, defineConfig, and keys like format and dts when making the change.packages/config/tsdown-config/package.json (1)
1-19: Missingtsdownas a dependency.
src/index.tsimportsdefineConfigfromtsdownand re-exports a wrapper, buttsdownis not declared independenciesorpeerDependencieshere. Since this is a private workspace package and consumers always depend ontsdownthemselves, it works in practice, but declaring it (likely as apeerDependency) would make the contract explicit and avoid hoisting surprises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/tsdown-config/package.json` around lines 1 - 19, The package.json for `@zenstackhq/tsdown-config` is missing the tsdown runtime contract; update package.json to declare "tsdown" as a peerDependency (so consumers must provide it) and also add it to devDependencies for local development/testing; ensure the version spec matches the project's tsdown range and keep the existing "exports" and scripts intact. Locate src/index.ts which imports defineConfig from "tsdown" to confirm the symbol and to validate the version chosen.packages/server/src/api/rest/openapi.ts (1)
186-186: Optional: minor consistency/readability nits in refactored path builders.Two small things while this area is being reshaped:
buildCollectionPath(modelName, modelDef, tag)takes bothmodelNameandmodelDef, whereasbuildSinglePath(modelDef, tag)derivesmodelNamefrommodelDef.name. Aligning them (dropmodelName, usemodelDef.name) reduces the risk of the two going out of sync.- In
buildNestedSinglePath,mayDenyUpdate/mayDenyDeleteare computed up-front regardless of whether the corresponding operations end up being included. Not a correctness issue; you could move each computation inside its respectiveif (isOperationIncluded(...))branch for clarity and to avoid unnecessary work when ops are filtered out.Neither is blocking — purely optional polish.
Also applies to: 431-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rest/openapi.ts` at line 186, Change buildCollectionPath to derive the model name from modelDef.name instead of accepting a separate modelName parameter (aligning its signature with buildSinglePath) to avoid divergence between the two values; in buildNestedSinglePath, defer computing mayDenyUpdate and mayDenyDelete by moving their evaluations inside the respective if (isOperationIncluded(...)) branches so they are only computed when the update/delete operations will actually be included; update any callers to stop passing modelName into buildCollectionPath and rely on modelDef.name instead.packages/language/src/ast.ts (1)
1-2: Make generated AST imports type-only.These symbols are only used in type positions in this file. Converting them to type-only imports improves clarity. The const values remain available to consumers via the
export * from './generated/ast'wildcard export.♻️ Proposed refinement
import type { AstNode } from 'langium'; -import { AbstractDeclaration, BinaryExpr, DataModel, type ExpressionType } from './generated/ast'; +import type { AbstractDeclaration, BinaryExpr, DataModel, ExpressionType } from './generated/ast';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/language/src/ast.ts` around lines 1 - 2, The imported generated AST symbols (AbstractDeclaration, BinaryExpr, DataModel, ExpressionType) are only used as types and should be imported as type-only; update the import so those names are brought in via a type-only import (e.g., use an import type for AbstractDeclaration, BinaryExpr, DataModel, and ExpressionType) while leaving the existing type import for AstNode unchanged so runtime exports remain available via the wildcard export.packages/orm/src/client/zod/factory.ts (1)
758-760: Remove the duplicate cache decorator.
makeArrayFilterSchemais decorated with@cache()twice. That is easy to miss and can wrap/cache the method redundantly.Proposed cleanup
- `@cache`() `@cache`() private makeArrayFilterSchema(fieldType: string, allowedFilterKinds: string[] | undefined) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/factory.ts` around lines 758 - 760, The method makeArrayFilterSchema is decorated twice with `@cache`(), causing redundant decoration; remove the duplicated `@cache`() so the method has a single `@cache`() decorator applied to it (locate the makeArrayFilterSchema declaration and delete the extra decorator instance), verifying there are no other accidental duplicate decorators nearby and that imports/usages remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 17: The packageManager field was changed to "pnpm@10.33.0" but CLAUDE.md
documents pinning to "pnpm@10.12.1"; update either the package.json or the
documentation so they agree. Restore the packageManager value to "pnpm@10.12.1"
in package.json if you want to follow the existing policy, or edit CLAUDE.md to
document the intentional bump to "pnpm@10.33.0" and update any subpackage notes
(e.g., VSCode subpackage) so all references to the pnpm pin are consistent.
Ensure the change mentions the packageManager key and the specific pnpm version
string you choose.
In `@packages/language/scripts/patch-generated-ast.ts`:
- Around line 14-27: The patch currently does blind includes/replace for
AttributeArg/$resolvedParam and DataModel/$allFields which can silently fail;
after attempting each replace (the blocks modifying content for 'AttributeArg'
and 'DataModel'), validate that the replacement actually occurred (e.g., check
that content now includes '$resolvedParam' and '$allFields' or that the original
pattern was removed) and if not throw an explicit Error (or exit non-zero) with
a clear message referencing the affected symbols (AttributeArg, $resolvedParam,
DataModel, $allFields) so the script fails fast when the AST patch target is
missing.
In `@packages/orm/src/client/crud/dialects/mysql.ts`:
- Around line 180-182: The MySQL transformOutputDate currently returns new
Date(...) directly which can produce an unhelpful "Invalid Date" object; update
the code in transformOutputDate to construct the Date from anchored (append 'Z'
if missing), store it in a local variable (e.g., const d = new Date(...)), then
check if the date is invalid (Number.isNaN(d.getTime())) and return null in that
case, otherwise return the valid Date instance — matching the PostgreSQL
dialect's invalid-date guard.
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 223-226: The toJSONSchema() path eagerly builds args schemas for
every procedure by calling makeProcedureArgsSchema for each key in
this.schema.procedures, which can register `${procName}ProcArgs` components for
procedures that should be hidden; add an isProcedureAllowed check (reusing the
same QueryOptions slicing logic used by server spec path generation) and only
call makeProcedureArgsSchema(procName) when isProcedureAllowed(procName,
queryOptions) returns true so that args components for excluded procedures are
not registered in the JSON-schema components.
In `@packages/orm/src/common-types.ts`:
- Line 3: The JsonArray type must accept readonly arrays to avoid breaking
assignability to JsonValue; update the definition of JsonArray (in
common-types.ts) from Array<JsonValue | null> to ReadonlyArray<JsonValue | null>
so readonly JSON arrays remain compatible (mutable arrays will still be
assignable to this readonly type).
In `@packages/server/src/api/rpc/openapi.ts`:
- Around line 531-546: The response-side model schemas can reference enum
components that aren't being emitted, causing dangling $refs; in
generateSharedSchemas() include enum schemas as well (e.g. build each enum from
this.schema.enums via a buildEnumSchema(enumDef) and merge them into the
returned object alongside typeDefSchemas and modelEntitySchemas) so
buildModelEntitySchema() refs resolve; apply the same change in the analogous
block around the other occurrence (the similar function at lines ~684-691) so
enums are always present for response and procedure return types.
- Around line 469-475: The OpenAPI operation currently sets op['requestBody']
with content when hasParams is true but does not mark the request body as
required; update the code that builds the requestBody (the block that references
op, hasParams, JSON_CT, and envelopeSchema) to set requestBody.required = true
whenever hasParams is true so clients will be forced to include the envelope
(with args) for mutation procedures. Ensure you only change the requestBody
creation for the branch that uses envelopeSchema/JSON_CT and leave other
branches untouched.
In `@tests/regression/test/issue-2550/regression.test.ts`:
- Around line 1-6: The test file is in the wrong path/name; rename
tests/regression/test/issue-2550/regression.test.ts to
tests/regression/test/issue-2550.test.ts and update the schema import so it
points to the fixture directory (change the import of './schema' to
'./issue-2550/schema' or the correct relative path to the moved fixture
directory); ensure the test still imports createTestClient, createSchemaFactory,
and schema from their updated locations and run the tests.
---
Outside diff comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 1367-1377: The generated schema for createManyAndReturn
(registered as `${model}CreateManyAndReturnArgs`) is being marked optional,
allowing undefined through validation; remove the .optional() call so the final
schema produced by makeCreateManyPayloadSchema / mergePluginArgsSchema and then
refined by refineForSelectOmitMutuallyExclusive and
refineForSelectHasTruthyField is required, ensuring the required data payload is
validated for createManyAndReturn; update the construction where result is
refined and registered (the code calling
refineForSelectHasTruthyField(...).optional()) to drop .optional() so
registerSchema and the exported CreateManyAndReturnArgs type no longer accept
undefined.
In `@packages/zod/src/factory.ts`:
- Around line 135-166: The deprecation note incorrectly points users to
makeModelSchema(...) which still includes computed and discriminator fields,
unlike makeModelCreateSchema and makeModelUpdateSchema that explicitly exclude
them; update the API or docs so behavior is preserved: either stop deprecating
makeModelCreateSchema/makeModelUpdateSchema or change makeModelSchema signature
to accept flags (e.g., excludeComputed/excludeDiscriminator or a mode for
create/update) and update callers to call makeModelSchema(model, { optionality:
'defaults', excludeComputed: true, excludeDiscriminator: true }) (or equivalent)
so computed and isDiscriminator fields are omitted, and adjust
applyDescription/addCustomValidation usage in
makeModelCreateSchema/makeModelUpdateSchema to delegate to the new option-based
makeModelSchema implementation; reference the functions makeModelCreateSchema,
makeModelUpdateSchema, and makeModelSchema and the field properties computed and
isDiscriminator when making the change.
In `@packages/zod/src/types.ts`:
- Around line 318-332: SelectEntryToZod currently treats any boolean as a truthy
selection which can type-select fields even when runtime accepts only literal
true; change the boolean branch to require the literal true (i.e. use Value
extends true) so only explicit true maps to GetAllModelFieldsShape via
FieldInShape, and similarly tighten the analogous type(s) around lines 338-340
to use Value extends true instead of Value extends boolean; keep
RelationFieldZodWithOptions handling for object values unchanged.
In `@packages/zod/src/utils.ts`:
- Around line 447-466: The membership predicate cases ('?', '!', '^', 'in')
currently treat the ABSENT sentinel as a real value and fail; update each case
to early-return true when the relevant operand is ABSENT (i.e., for '?', '!',
'^' if left === ABSENT return true; for 'in' if left === ABSENT or right ===
ABSENT return true / or at minimum if the operand you expect to be omitted
equals ABSENT return true) so omitted fields are skipped like the
ordered-comparison branch does; use the existing ABSENT constant and modify the
switch cases for '?', '!', '^', and 'in' accordingly.
---
Nitpick comments:
In `@packages/clients/client-helpers/tsdown.config.ts`:
- Line 8: The explicit format: ['esm'] setting in tsdown.config.ts appears
redundant with the shared `@zenstackhq/tsdown-config` default; either remove the
override to align with other packages (delete the format property) or, if you
intentionally require ESM-only output for this package, keep the format: ['esm']
entry and add a one-line comment explaining why ESM-only is required so readers
understand the deviation from the shared config.
In `@packages/config/tsdown-config/package.json`:
- Around line 1-19: The package.json for `@zenstackhq/tsdown-config` is missing
the tsdown runtime contract; update package.json to declare "tsdown" as a
peerDependency (so consumers must provide it) and also add it to devDependencies
for local development/testing; ensure the version spec matches the project's
tsdown range and keep the existing "exports" and scripts intact. Locate
src/index.ts which imports defineConfig from "tsdown" to confirm the symbol and
to validate the version chosen.
In `@packages/config/tsdown-config/src/index.ts`:
- Around line 9-17: The createConfig function currently accepts a broad Config
type derived from Parameters<typeof defineConfig>[0], which can include
non-object forms and makes the spread merge unsafe and shallow; tighten the
overrides parameter to only accept the object form of defineConfig's parameter
(so callers can’t pass an array/function) and either document that the merge is
shallow (so keys like format/dts are replaced entirely) or implement a
deep-merge if true merging is required; update the signature and add a short
comment above createConfig mentioning "shallow merge: overrides replace
defaults" and reference the overrides parameter, createConfig function,
defineConfig, and keys like format and dts when making the change.
In `@packages/language/src/ast.ts`:
- Around line 1-2: The imported generated AST symbols (AbstractDeclaration,
BinaryExpr, DataModel, ExpressionType) are only used as types and should be
imported as type-only; update the import so those names are brought in via a
type-only import (e.g., use an import type for AbstractDeclaration, BinaryExpr,
DataModel, and ExpressionType) while leaving the existing type import for
AstNode unchanged so runtime exports remain available via the wildcard export.
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 758-760: The method makeArrayFilterSchema is decorated twice with
`@cache`(), causing redundant decoration; remove the duplicated `@cache`() so the
method has a single `@cache`() decorator applied to it (locate the
makeArrayFilterSchema declaration and delete the extra decorator instance),
verifying there are no other accidental duplicate decorators nearby and that
imports/usages remain unchanged.
In `@packages/server/src/api/rest/openapi.ts`:
- Line 186: Change buildCollectionPath to derive the model name from
modelDef.name instead of accepting a separate modelName parameter (aligning its
signature with buildSinglePath) to avoid divergence between the two values; in
buildNestedSinglePath, defer computing mayDenyUpdate and mayDenyDelete by moving
their evaluations inside the respective if (isOperationIncluded(...)) branches
so they are only computed when the update/delete operations will actually be
included; update any callers to stop passing modelName into buildCollectionPath
and rely on modelDef.name instead.
In `@tests/e2e/orm/client-api/timezone/mysql-timezone.test.ts`:
- Around line 80-84: The test currently only asserts that windows[0].close is a
Date instance; update the assertion for the direct-select path to assert the
actual ISO string as well by calling windows[0].close.toISOString() and
comparing it to the expected value '1970-01-01T16:00:00.000Z' (matching the
nested test's check), so the variable windows and its element windows[0].close
are validated for the correct timestamp rather than just Date validity.
In `@tests/e2e/orm/client-api/timezone/pg-timezone.test.ts`:
- Around line 634-642: The direct-select assertions are too loose for the time
fields — tighten them by asserting that windows[0].close and windows[0].openTz
are Date instances and that their ISO strings match the expected normalized UTC
values (similar to the existing check for open.toISOString()), while keeping
windows[0].effectiveOn only asserted as a Date; update the assertions referring
to windows[0].close and windows[0].openTz in the pg-timezone.test.ts block so
they validate both instance type and toISOString equality.
In `@tests/regression/test/issue-2578.test.ts`:
- Around line 50-51: Remove the trivial sanity assertions that only re-check
created ids (e.g., the expect([p1.id, p2.id, p3.id].length).toBe(3) line and the
similar assertion around line 71); locate the assertions referencing p1.id,
p2.id, p3.id (and the analogous id-check later) in the test and delete those
expect(...) lines so the test reads cleaner while preserving the meaningful
assertions that follow create.
🪄 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: 234cd176-0c10-422d-9853-b9fe001579c1
⛔ Files ignored due to path filters (2)
packages/language/src/generated/ast.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (137)
.vscode/launch.json.vscode/settings.jsonBREAKINGCHANGES.mdCLAUDE.mdREADME.mdpackage.jsonpackages/auth-adapters/better-auth/package.jsonpackages/auth-adapters/better-auth/test/cli-generate.test.tspackages/auth-adapters/better-auth/tsdown.config.tspackages/auth-adapters/better-auth/tsup.config.tspackages/cli/bin/clipackages/cli/package.jsonpackages/cli/scripts/post-build.tspackages/cli/test/utils.tspackages/cli/tsdown.config.tspackages/cli/tsup.config.tspackages/clients/client-helpers/package.jsonpackages/clients/client-helpers/tsconfig.jsonpackages/clients/client-helpers/tsconfig.test.jsonpackages/clients/client-helpers/tsdown.config.tspackages/clients/client-helpers/tsup.config.tspackages/clients/tanstack-query/package.jsonpackages/clients/tanstack-query/tsconfig.general.jsonpackages/clients/tanstack-query/tsconfig.svelte.jsonpackages/clients/tanstack-query/tsconfig.test.jsonpackages/common-helpers/package.jsonpackages/common-helpers/tsconfig.jsonpackages/common-helpers/tsdown.config.tspackages/common-helpers/tsup.config.tspackages/config/eslint-config/package.jsonpackages/config/tsdown-config/package.jsonpackages/config/tsdown-config/src/index.tspackages/config/tsdown-config/tsconfig.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/bin/clipackages/create-zenstack/package.jsonpackages/create-zenstack/tsconfig.jsonpackages/create-zenstack/tsdown.config.tspackages/create-zenstack/tsup.config.tspackages/ide/vscode/package.jsonpackages/ide/vscode/scripts/post-build.tspackages/ide/vscode/tsdown.config.tspackages/ide/vscode/tsup.config.tspackages/language/package.jsonpackages/language/scripts/patch-generated-ast.tspackages/language/src/ast.tspackages/language/src/utils.tspackages/language/src/validators/datamodel-validator.tspackages/language/src/validators/typedef-validator.tspackages/language/test/mixin.test.tspackages/language/tsdown.config.tspackages/language/tsup.config.tspackages/orm/package.jsonpackages/orm/src/client/client-impl.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/index.tspackages/orm/src/client/crud/dialects/lateral-join-dialect-base.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/aggregate.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/crud/operations/count.tspackages/orm/src/client/crud/operations/create.tspackages/orm/src/client/crud/operations/delete.tspackages/orm/src/client/crud/operations/exists.tspackages/orm/src/client/crud/operations/find.tspackages/orm/src/client/crud/operations/group-by.tspackages/orm/src/client/crud/operations/update.tspackages/orm/src/client/crud/validator/validator.tspackages/orm/src/client/executor/name-mapper.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/orm/src/client/helpers/schema-db-pusher.tspackages/orm/src/client/index.tspackages/orm/src/client/options.tspackages/orm/src/client/plugin.tspackages/orm/src/client/promise.tspackages/orm/src/client/query-builder.tspackages/orm/src/client/query-utils.tspackages/orm/src/client/result-processor.tspackages/orm/src/client/zod/factory.tspackages/orm/src/client/zod/index.tspackages/orm/src/common-types.tspackages/orm/src/utils/schema-utils.tspackages/orm/tsdown.config.tspackages/plugins/policy/package.jsonpackages/plugins/policy/tsdown.config.tspackages/plugins/policy/tsup.config.tspackages/schema/package.jsonpackages/schema/tsdown.config.tspackages/schema/tsup.config.tspackages/sdk/package.jsonpackages/sdk/src/ts-schema-generator.tspackages/sdk/tsconfig.jsonpackages/sdk/tsdown.config.tspackages/sdk/tsup.config.tspackages/server/package.jsonpackages/server/src/api/common/spec-utils.tspackages/server/src/api/rest/openapi.tspackages/server/src/api/rpc/index.tspackages/server/src/api/rpc/openapi.tspackages/server/test/openapi/baseline/rpc.baseline.yamlpackages/server/test/openapi/rest-openapi.test.tspackages/server/test/openapi/rpc-openapi.test.tspackages/server/tsdown.config.tspackages/testtools/package.jsonpackages/testtools/tsdown.config.tspackages/testtools/tsup.config.tspackages/zod/package.jsonpackages/zod/src/factory.tspackages/zod/src/types.tspackages/zod/src/utils.tspackages/zod/test/factory.test.tspackages/zod/tsdown.config.tspackages/zod/tsup.config.tspnpm-workspace.yamlsamples/orm/package.jsonsamples/orm/zenstack/schema.zmodelscripts/test-generate.tstests/e2e/orm/client-api/mixin.test.tstests/e2e/orm/client-api/timezone/mysql-timezone.test.tstests/e2e/orm/client-api/timezone/pg-timezone.test.tstests/e2e/orm/client-api/zod.test.tstests/e2e/package.jsontests/regression/package.jsontests/regression/test/issue-2540.test.tstests/regression/test/issue-2550/regression.test.tstests/regression/test/issue-2550/schema.tstests/regression/test/issue-2550/schema.zmodeltests/regression/test/issue-2578.test.tstests/regression/test/issue-2588.test.tstests/regression/test/issue-274/regression.test.tstests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
💤 Files with no reviewable changes (13)
- packages/auth-adapters/better-auth/tsup.config.ts
- packages/cli/tsup.config.ts
- packages/clients/client-helpers/tsup.config.ts
- packages/create-zenstack/tsup.config.ts
- packages/ide/vscode/tsup.config.ts
- packages/schema/tsup.config.ts
- packages/plugins/policy/tsup.config.ts
- packages/testtools/tsup.config.ts
- packages/zod/tsup.config.ts
- tests/e2e/orm/client-api/zod.test.ts
- packages/language/tsup.config.ts
- packages/sdk/tsup.config.ts
- packages/common-helpers/tsup.config.ts
…I spec - ZodSchemaFactory.toJSONSchema() now skips makeProcedureArgsSchema for procedures excluded by slicing (includedProcedures / excludedProcedures), preventing dangling component registrations for hidden procedures - RPCApiSpecGenerator.generateSharedSchemas() now emits enum schemas so $ref pointers from model entity schemas and typedef schemas resolve correctly - RPCApiSpecGenerator requestBody.required is now only set when at least one procedure param is non-optional, matching the existing GET q-param behavior - Add tests covering all three fixes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Claude finished @ymc9's task —— View job Code Review for v3.6.0 Release (dev → main)I've completed my comprehensive code review of this major release. Here are my findings: Review Checklist:
🏗️ Build System Migration Analysis ✅Positive Changes:
📦 Dependency Updates & Package Changes ✅Key Observations:
🔧 Code Quality & Best Practices ✅Strengths:
🐛 Potential Issues & Bugs
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server/src/api/rpc/openapi.ts`:
- Around line 720-727: The unknown-type fallback in buildModelEntitySchema
currently creates a typed SchemaObject and blindly marks the field as required;
update that block to respect fieldDef.optional by wrapping the typed schema with
an anyOf including { type: 'null' } when fieldDef.optional is true (i.e.,
produce either typed or nullable typed), and keep the same
required.push(fieldName) behavior as the other branches so the field is emitted
as required-but-nullable consistent with scalar/enum/typedef handling; modify
the logic around variables typed, properties[fieldName], and
required.push(fieldName) accordingly.
🪄 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: fcf5367d-41d8-4315-8598-51330481a6fa
📒 Files selected for processing (4)
packages/orm/src/client/zod/factory.tspackages/server/src/api/rpc/openapi.tspackages/server/test/openapi/baseline/rpc.baseline.yamlpackages/server/test/openapi/rpc-openapi.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/orm/src/client/zod/factory.ts
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
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
optionalityto control field optionality.Breaking Changes
makeModelSchema()excludes relation fields by default; opt in viainclude/select.Bug Fixes
Chores
Tests