Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Zod-based schema factories and validators, exposes a client-level Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 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 |
|
Claude finished @ymc9's task —— View job PR Review: Zod Schemas for ZModel Constructs and ORM Query Input ValidationsThis is a substantial and well-architected PR that introduces comprehensive Zod integration for ZModel validation. Here's my detailed review: ✅ Code Quality & ArchitectureExcellent Design Patterns:
Well-Structured Code Organization:
✅ Implementation StrengthsComprehensive Type Safety:
Robust Validation Architecture:
Performance Considerations:
✅ Feature CompletenessCRUD Operation Coverage:
Advanced Features:
✅ TestingComprehensive Test Coverage:
|
|
Claude finished @ymc9's task —— View job PR Review: Zod Schemas for ZModel Constructs and ORM Query Input ValidationsThis is a substantial and well-architected PR that introduces comprehensive Zod integration for ZModel validation. Here's my detailed review: ✅ Code Quality & ArchitectureExcellent Design Patterns:
Well-Structured Code Organization:
✅ Implementation StrengthsComprehensive Type Safety:
Robust Validation Architecture:
Performance Considerations:
✅ Feature CompletenessCRUD Operation Coverage:
Advanced Features:
✅ TestingComprehensive Test Coverage:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (9)
packages/zod/src/error.ts (1)
4-7: Missingthis.nameassignment —error.namewill read'Error'instead of'ZodSchemaError'.Without explicitly setting
this.name, the inheritedError.prototype.namevalue ('Error') is used, which makes the class indistinguishable from a plainErrorin logs and catch-blockerror.namechecks.🛡️ Proposed fix
export class ZodSchemaError extends Error { constructor(message: string) { super(message); + this.name = 'ZodSchemaError'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod/src/error.ts` around lines 4 - 7, The ZodSchemaError constructor doesn't set this.name so caught errors show as generic "Error"; update the ZodSchemaError constructor to assign this.name = "ZodSchemaError" (and for proper subclassing across transpiled targets also call Object.setPrototypeOf(this, ZodSchemaError.prototype)) so instances report the correct name in logs and error checks.packages/orm/src/client/constants.ts (1)
1-4: Dead commented-out code — remove or add a TODO if restoration is planned.Leaving the entire
CONTEXT_COMMENT_PREFIXdeclaration as a comment block adds noise with no actionable context. If it's permanently removed, delete the lines; if it's temporarily disabled, add a// TODO: re-enable when ...note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/constants.ts` around lines 1 - 4, Remove the dead commented-out declaration of CONTEXT_COMMENT_PREFIX from the file (or, if you intend to restore it later, replace the block comment with a single-line comment containing a TODO explaining when/why it will be re-enabled, e.g., "// TODO: re-enable CONTEXT_COMMENT_PREFIX when query annotation with context is needed"); ensure the symbol name CONTEXT_COMMENT_PREFIX is referenced in the comment/TODO if kept so future readers know what's being preserved.scripts/test-generate.ts (1)
24-29: Hardcoded flags override any caller-supplied equivalents.
--generate-models=false --generate-input=falseis appended afteroptions.join(' '). For last-wins CLI parsers (yargs, commander), a caller passing--generate-models=trueviaoptionswill be silently overridden. If this is intentional, a brief comment explaining why model/input generation is always suppressed here would help future maintainers.✏️ Suggested clarification
execSync( - `${RUNTIME} ${cliPath} generate --schema ${schemaPath} ${options.join(' ')} --generate-models=false --generate-input=false`, + // --generate-models=false --generate-input=false are intentionally forced last so + // test-generate only produces Zod schema output, never model or input files. + `${RUNTIME} ${cliPath} generate --schema ${schemaPath} ${options.join(' ')} --generate-models=false --generate-input=false`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-generate.ts` around lines 24 - 29, The execSync call currently appends hardcoded flags `--generate-models=false --generate-input=false` after `options.join(' ')`, which will override any equivalent flags passed in `options`; update the invocation so it does not silently override caller-supplied flags: either remove the hardcoded flags from the command and rely on callers to provide them, or conditionally append them only when `options` does not already include `--generate-models`/`--no-generate-models`/`--generate-input`/`--no-generate-input` (check the `options` array before building the command); use the existing symbols `execSync`, `RUNTIME`, `cliPath`, `schemaPath`, and `options` to locate and change the code, and if you intentionally must force these values add a brief comment above the `execSync` call explaining that model/input generation must always be suppressed.packages/orm/src/client/zod/zod-schema-factory.ts (3)
980-980: Inconsistent parameter naming:WithAggregationuses PascalCase.The parameter
WithAggregationshould bewithAggregation(camelCase) to match the sibling parameterwithRelationand project conventions.Proposed fix
- private makeOrderBySchema(model: string, withRelation: boolean, WithAggregation: boolean) { + private makeOrderBySchema(model: string, withRelation: boolean, withAggregation: boolean) {And update all usages of
WithAggregationwithin the method body towithAggregation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/zod-schema-factory.ts` at line 980, Rename the PascalCase parameter WithAggregation to camelCase withAggregation in the method signature of makeOrderBySchema and update every usage inside that method body to reference withAggregation (ensure any internal conditionals, returns, or helper calls that currently use WithAggregation are changed to withAggregation to preserve behavior and match project naming conventions).
1471-1487:z.object()(non-strict) used for numeric/array update operators.Lines 1475 and 1494 use
z.object(...)instead ofz.strictObject(...). While the.refine()checks enforceObject.keys(v).length === 1, an unknown key alongside a known one (e.g.,{ set: 1, typo: 2 }) would also have length > 1 and be rejected — so behavior is correct. However, the error message would be about mutual exclusivity rather than "unrecognized key," which may confuse users. Consider usingz.strictObject()for a clearer error.Also applies to: 1489-1500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/zod-schema-factory.ts` around lines 1471 - 1487, Replace the non-strict Zod objects used for update-operator unions with strict objects so unexpected keys surface with a clearer "unrecognized key" error; specifically, in the branch that builds the numeric update union (the z.union that adds the object with keys set/increment/decrement/multiply/divide) and the corresponding array-update branch, swap z.object(...) for z.strictObject(...), keeping the same optional/nullability wrappers and the existing refine that enforces a single operator key.
124-142: Unused debug methods with@ts-ignoreandconsole.log.Same concern as in
accessor.ts:getCache,setCache, andprintCacheStatsare suppressed with@ts-ignoreand appear unused.printCacheStatswrites toconsole.logwhich could leak internal state if invoked. Consider removing these or, if needed for debugging, guarding behind a debug flag and using@ts-expect-error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/zod-schema-factory.ts` around lines 124 - 142, The three private debug helpers getCache, setCache, and printCacheStats (which access schemaCache) are unused and suppress TypeScript errors with `@ts-ignore` and use console.log; either remove them entirely or convert them to a guarded debug feature: delete the unused methods if not needed, or wrap their bodies behind a ZOD_SCHEMA_DEBUG (or similar) flag, replace console.log with the project's logger, and drop `@ts-ignore` in favor of correct typings (or `@ts-expect-error` if an unavoidable typing gap remains). Update references to schemaCache access only via approved methods and ensure no leftover `@ts-ignore` comments remain.packages/schema/src/accessor.ts (2)
82-86:InvalidSchemaErrordoes not setname.Without setting
this.name, the error'snameproperty defaults to"Error"rather than"InvalidSchemaError", which makes debugging and programmatic error handling harder.Proposed fix
export class InvalidSchemaError extends Error { constructor(message: string) { super(message); + this.name = 'InvalidSchemaError'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/schema/src/accessor.ts` around lines 82 - 86, The InvalidSchemaError class currently doesn't set this.name, so caught errors report as "Error"; update the constructor of InvalidSchemaError to set this.name = "InvalidSchemaError" and restore the proper prototype chain (e.g., via Object.setPrototypeOf(this, InvalidSchemaError.prototype)) and optionally capture the stack (Error.captureStackTrace(this, InvalidSchemaError)) so the error's type and stack are correct for debugging and programmatic checks.
124-142: Debug/internal methods suppressed with@ts-ignore.
getCache,setCache, andprintCacheStatsare all@ts-ignore-suppressed and appear unused by the runtime code.printCacheStatswrites toconsole.log, which is a concern if accidentally invoked in production. Consider removing these or guardingprintCacheStatswith a debug flag, and replacing@ts-ignorewith@ts-expect-error(which will error if the suppression becomes unnecessary).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/schema/src/accessor.ts` around lines 124 - 142, The three internal/debug methods getCache, setCache, and printCacheStats are suppressed with `@ts-ignore` and appear unused; remove the unused methods or at minimum replace the suppression comments with `@ts-expect-error` and prevent console output in production by guarding printCacheStats behind a debug flag (e.g., process.env.DEBUG_SCHEMA or an exported debug boolean) so it only logs when enabled; update the definitions for getCache, setCache, printCacheStats accordingly (or delete them) and ensure any `@ts-ignore` near those functions is changed to `@ts-expect-error` so the build fails if the suppression is no longer needed.packages/orm/src/client/crud/validator/validator.ts (1)
186-267: Procedure input validation does not reject extraneous argument keys.The
validateProcedureInputmethod validates that all required parameters are present and each parameter value matches its Zod schema, but it never checks for unknown/extra keys in theargsPayloadobject. For example, if a procedure accepts{ userId: String }but the caller passes{ userId: "x", admin: true }, the extraadminkey is silently accepted and forwarded.This could mask typos in argument names (e.g.,
userIDvsuserId) or allow unexpected data to flow through. Consider adding an extraneous-key check after the param loop:💡 Suggested addition after line 264
+ const knownParams = new Set(params.map((p) => p.name)); + const extraKeys = Object.keys(obj).filter((k) => !knownParams.has(k)); + if (extraKeys.length > 0) { + throw createInvalidInputError( + `Unknown procedure argument(s): ${extraKeys.join(', ')}`, + `$procs.${proc}`, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/validator/validator.ts` around lines 186 - 267, validateProcedureInput currently validates required params and types but doesn't reject unknown keys in the argsPayload, letting extraneous properties through; after the loop that iterates params in validateProcedureInput, add a check that collects Object.keys(obj) and compares against params.map(p => p.name) and if any keys remain, throw createInvalidInputError with a message like "Unexpected procedure argument(s): <keys>" and path `$procs.${proc}`; reference the argsPayload variable (obj), the params array and this.zodFactory.makeProcedureParamSchema to locate where to insert the check.
🤖 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/orm/src/client/crud/validator/validator.ts`:
- Around line 29-34: InputValidator is losing the client's Options and
ExtQueryArgs type parameters which narrows zodFactory to generic ClientOptions;
update InputValidator to be generic over Schema, Options, ExtQueryArgs (e.g.,
InputValidator<Schema, Options, ExtQueryArgs>) and use those types when
declaring zodFactory = ZodSchemaFactory<Schema, Options, ExtQueryArgs> and the
constructor ClientContract to preserve the client's precise types (so methods
like makeFindFirstSchema return FindFirstArgs<Schema, Model, Options,
ExtQueryArgs> and client.$zod exposes the narrowed Options/ExtQueryArgs).
In `@packages/orm/src/client/zod/zod-schema-factory.ts`:
- Around line 1722-1735: The refine on schema in zod-schema-factory.ts
incorrectly assumes value.orderBy is an object; update the refinement in the
schema.refine callback (the block that uses bys and AggregateOperators) to
normalize value.orderBy to an array of order objects when it's not already an
array, then iterate each order object to collect its property keys (excluding
AggregateOperators) and ensure every such key is included in bys; keep handling
for value.by (string vs array) as is and return false only if any non-aggregate
order key is missing from bys.
In `@packages/schema/package.json`:
- Line 7: The build script in package.json currently chains "pnpm test:generate"
into the "build" command, creating a dependency on the CLI dist that may not
exist yet; remove "pnpm test:generate" from the "build" script (the "build" npm
script entry) and instead expose "test:generate" as a standalone script (e.g.,
keep the existing "test:generate" npm script) and run it explicitly in CI after
all packages including the CLI are built, or document it as an optional
post-build step to be invoked only after the CLI dist is available.
In `@packages/schema/src/accessor.ts`:
- Around line 172-193: The current typeof guard in getUniqueFields lets null
pass because typeof null === 'object', which leads to uncontrolled errors when
accessing value.type; update the validation to explicitly reject null (e.g.,
check value === null || typeof value !== 'object') and throw InvalidSchemaError
for such cases, and then when branching on the singular unique field guard the
property's presence safely (e.g., verify 'type' in value or value.type !==
undefined before using typeof) so accesses to value.type and Object.keys(value)
are safe; adjust the logic around getUniqueFields, InvalidSchemaError, and the
compound branch that calls _requireModelField to use these stronger null/object
checks.
In `@packages/schema/vitest.config.ts`:
- Around line 1-4: The CI failure is caused because scripts/test-generate.ts
(invoked by the package.json "test:generate" script) expects
packages/cli/dist/index.js (the cliPath variable) to exist; ensure the build
graph requires `@zenstackhq/cli` to be built before packages/schema by either
updating the CI workflow to run the packages/cli build before packages/schema,
or by declaring an explicit build dependency so the package build/task graph
enforces it (e.g., add a turbo/pnpm pipeline dependency or make
packages/schema's build script depend on `@zenstackhq/cli` build) so
scripts/test-generate.ts can find cliPath at runtime.
In `@packages/zod/package.json`:
- Line 39: The package.json has inconsistent versioning: change
"json-stable-stringify" from a careted version to use the catalog alias (e.g.,
"json-stable-stringify": "catalog:json-stable-stringify") and make the
devDependency for "zod" use the same catalog alias as the peerDependency
(replace "^4.1.0" with the same "catalog:zod" entry), then add an entry for
json-stable-stringify to the root pnpm-workspace.yaml catalog so the catalog
reference resolves consistently across peerDependencies and devDependencies.
In `@packages/zod/src/factory.ts`:
- Around line 117-125: applyCardinality currently returns schema.array() early
and thus ignores fieldDef.optional when fieldDef.array is true; update
applyCardinality to handle both flags by returning
schema.array().nullable().optional() (or schema.array().optional().nullable(),
preserving Zod chaining) when both fieldDef.array and fieldDef.optional are
true, keep the existing schema.array() when only array is true and
schema.nullable().optional() when only optional is true; reference the
applyCardinality method, FieldDef.array/optional, and the
schema.array()/schema.nullable().optional() chains when making the change.
- Around line 51-56: The relation branch constructs a z.lazy that already
contains applyCardinality and then wraps that lazy with .optional(), causing
double-optional nesting for optional relations; fix by moving applyCardinality
out of the z.lazy so that you call z.lazy(() =>
this.makeModelSchema(relatedModelName as GetModels<Schema>)) and then apply
this.applyCardinality(...) (and .optional() only when appropriate) to the
resulting lazy schema so applyCardinality is invoked exactly once for
relatedModelName via makeModelSchema and fields[fieldName] gets a single
correctly-cardinalized schema.
- Around line 127-129: The parameter type for makeTypeSchema prevents TypeScript
from inferring the specific Type generic when multiple typedefs exist; change
the signature to accept the generic Type itself (i.e., makeTypeSchema<Type
extends GetTypeDefs<Schema>>(type: Type)) and return
z.ZodObject<GetTypeDefFieldsShape<Schema, Type>, z.core.$strict> so the
parameter is typed as the specific Type rather than the full union; update the
function declaration of makeTypeSchema accordingly so callers like
makeTypeSchema('Address') produce a schema narrowed to the 'Address' fields.
In `@packages/zod/test/factory.test.ts`:
- Around line 63-64: The inline comment above the birthdate assertion is
incorrect: FieldTypeZodMap does include a DateTime key (DateTime:
z.ZodType<Date>) and the test rightly asserts User['birthdate'] is Date | null |
undefined; remove or replace the misleading comment with an accurate one (e.g.,
noting DateTime maps to Date) next to the
expectTypeOf<User['birthdate']>().toEqualTypeOf<Date | null | undefined>() so
the comment matches the implementation and assertion.
In `@tests/e2e/orm/client-api/zod.test.ts`:
- Around line 1397-1402: The test creates a local Prisma test client via
createTestClient(schema) that shadows the outer client and is never
disconnected; update the test so the locally-created client is properly
disconnected after use (e.g., call await client.$disconnect() at the end of the
it block or reuse the outer client instead of creating a new one) so the
connection created by createTestClient is closed; reference the local const
client in this test and ensure its $disconnect() is awaited (or remove the local
creation and use the shared client cleaned up by the existing afterEach).
---
Nitpick comments:
In `@packages/orm/src/client/constants.ts`:
- Around line 1-4: Remove the dead commented-out declaration of
CONTEXT_COMMENT_PREFIX from the file (or, if you intend to restore it later,
replace the block comment with a single-line comment containing a TODO
explaining when/why it will be re-enabled, e.g., "// TODO: re-enable
CONTEXT_COMMENT_PREFIX when query annotation with context is needed"); ensure
the symbol name CONTEXT_COMMENT_PREFIX is referenced in the comment/TODO if kept
so future readers know what's being preserved.
In `@packages/orm/src/client/crud/validator/validator.ts`:
- Around line 186-267: validateProcedureInput currently validates required
params and types but doesn't reject unknown keys in the argsPayload, letting
extraneous properties through; after the loop that iterates params in
validateProcedureInput, add a check that collects Object.keys(obj) and compares
against params.map(p => p.name) and if any keys remain, throw
createInvalidInputError with a message like "Unexpected procedure argument(s):
<keys>" and path `$procs.${proc}`; reference the argsPayload variable (obj), the
params array and this.zodFactory.makeProcedureParamSchema to locate where to
insert the check.
In `@packages/orm/src/client/zod/zod-schema-factory.ts`:
- Line 980: Rename the PascalCase parameter WithAggregation to camelCase
withAggregation in the method signature of makeOrderBySchema and update every
usage inside that method body to reference withAggregation (ensure any internal
conditionals, returns, or helper calls that currently use WithAggregation are
changed to withAggregation to preserve behavior and match project naming
conventions).
- Around line 1471-1487: Replace the non-strict Zod objects used for
update-operator unions with strict objects so unexpected keys surface with a
clearer "unrecognized key" error; specifically, in the branch that builds the
numeric update union (the z.union that adds the object with keys
set/increment/decrement/multiply/divide) and the corresponding array-update
branch, swap z.object(...) for z.strictObject(...), keeping the same
optional/nullability wrappers and the existing refine that enforces a single
operator key.
- Around line 124-142: The three private debug helpers getCache, setCache, and
printCacheStats (which access schemaCache) are unused and suppress TypeScript
errors with `@ts-ignore` and use console.log; either remove them entirely or
convert them to a guarded debug feature: delete the unused methods if not
needed, or wrap their bodies behind a ZOD_SCHEMA_DEBUG (or similar) flag,
replace console.log with the project's logger, and drop `@ts-ignore` in favor of
correct typings (or `@ts-expect-error` if an unavoidable typing gap remains).
Update references to schemaCache access only via approved methods and ensure no
leftover `@ts-ignore` comments remain.
In `@packages/schema/src/accessor.ts`:
- Around line 82-86: The InvalidSchemaError class currently doesn't set
this.name, so caught errors report as "Error"; update the constructor of
InvalidSchemaError to set this.name = "InvalidSchemaError" and restore the
proper prototype chain (e.g., via Object.setPrototypeOf(this,
InvalidSchemaError.prototype)) and optionally capture the stack
(Error.captureStackTrace(this, InvalidSchemaError)) so the error's type and
stack are correct for debugging and programmatic checks.
- Around line 124-142: The three internal/debug methods getCache, setCache, and
printCacheStats are suppressed with `@ts-ignore` and appear unused; remove the
unused methods or at minimum replace the suppression comments with
`@ts-expect-error` and prevent console output in production by guarding
printCacheStats behind a debug flag (e.g., process.env.DEBUG_SCHEMA or an
exported debug boolean) so it only logs when enabled; update the definitions for
getCache, setCache, printCacheStats accordingly (or delete them) and ensure any
`@ts-ignore` near those functions is changed to `@ts-expect-error` so the build
fails if the suppression is no longer needed.
In `@packages/zod/src/error.ts`:
- Around line 4-7: The ZodSchemaError constructor doesn't set this.name so
caught errors show as generic "Error"; update the ZodSchemaError constructor to
assign this.name = "ZodSchemaError" (and for proper subclassing across
transpiled targets also call Object.setPrototypeOf(this,
ZodSchemaError.prototype)) so instances report the correct name in logs and
error checks.
In `@scripts/test-generate.ts`:
- Around line 24-29: The execSync call currently appends hardcoded flags
`--generate-models=false --generate-input=false` after `options.join(' ')`,
which will override any equivalent flags passed in `options`; update the
invocation so it does not silently override caller-supplied flags: either remove
the hardcoded flags from the command and rely on callers to provide them, or
conditionally append them only when `options` does not already include
`--generate-models`/`--no-generate-models`/`--generate-input`/`--no-generate-input`
(check the `options` array before building the command); use the existing
symbols `execSync`, `RUNTIME`, `cliPath`, `schemaPath`, and `options` to locate
and change the code, and if you intentionally must force these values add a
brief comment above the `execSync` call explaining that model/input generation
must always be suppressed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/orm/src/client/crud/validator/validator.ts (1)
185-186: Consider caching procedure input schemas.Line 185 notes a TODO to turn procedure input validation into a cached Zod schema. Caching would align with other schema paths and avoid rebuilding per call. If you'd like, I can draft a cached schema helper for this path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/validator/validator.ts` around lines 185 - 186, The validateProcedureInput(proc: string, input: unknown) method currently rebuilds/validates inputs per call; convert this to use a cached Zod schema per procedure name to avoid repeated schema construction. Implement a small cache (e.g., Map<string, ZodSchema>) keyed by proc, generate or retrieve the Zod schema once when validateProcedureInput is first called for that proc, then reuse it for subsequent validations, and ensure errors are thrown or returned in the same shape as before; update validateProcedureInput to look up the schema in the cache, compile it on miss, and validate input using the cached ZodSchema.
🤖 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/orm/src/client/zod/factory.ts`:
- Around line 1722-1735: The refine validator in schema.refine incorrectly
assumes value.orderBy is an object and calls Object.keys directly, rejecting
valid array forms; update the check to normalize orderBy into an array (e.g.,
const orderBys = Array.isArray(value.orderBy) ? value.orderBy : [value.orderBy])
and then collect field names from each element (e.g., orderBys.flatMap(ob =>
Object.keys(ob))) before filtering against AggregateOperators and comparing
against the bys array (value.by) so groupBy accepts both object and array
orderBy shapes; adjust the logic around value.orderBy, AggregateOperators, bys,
and the final .some(...) accordingly and keep the same error message.
- Around line 1471-1485: The numeric update operator union currently forces
operands to z.number(), rejecting Decimal/bigint/numeric-string types; update
the operator payloads inside the isNumericField branch (the object with keys
set/increment/decrement/multiply/divide) to reuse the field's scalar schema
instead of z.number() — e.g., replace z.number() with the scalar schema produced
by makeScalarSchema for the current fieldDef (respecting existing nullableIf
usage and optional flags) so set/increment/decrement/multiply/divide accept the
same types as the field itself; keep the same single-key refine check and
optional wrapping.
---
Duplicate comments:
In `@packages/orm/src/client/crud/validator/validator.ts`:
- Around line 1-54: InputValidator is currently generic only over Schema which
forces zodFactory to use ClientOptions<Schema> and collapses client-specific
Options/ExtQueryArgs (losing the narrowed types and causing use of any in
validate* return types); fix by making the class generic as
InputValidator<Schema extends SchemaDef, Options, ExtQueryArgs> (or matching the
generics on ClientContract), update the constructor type to
ClientContract<Schema, Options, ExtQueryArgs>, change zodFactory to
ZodSchemaFactory<Schema, ClientOptions<Schema, Options, ExtQueryArgs>> (or the
correct ClientOptions instantiation), and thread Options/ExtQueryArgs through
all validate* signatures (e.g., validateFindArgs, validateCreateArgs, etc.)
replacing the any return-type placeholders with the proper generics so the
methods preserve the client's narrowed types.
In `@packages/schema/package.json`:
- Line 7: The build script "build" in package.json was updated to "tsc --noEmit
&& tsup-node" and no longer chains test:generate; no change required—confirm the
"build" npm script in package.json reflects this new value and proceed with
approval/merge (verify the "build" script string matches "tsc --noEmit &&
tsup-node").
In `@tests/e2e/orm/client-api/zod.test.ts`:
- Around line 1397-1402: The test creates a local client with
createTestClient(schema) that shadows the outer `client` and is never
disconnected, leaking DB connections; update the test (the `it('can be
constructed directly from client'...)` block) to either rename the local
variable (e.g., `localClient`) and call await localClient.disconnect() in a
finally block or explicitly await disconnect on the locally created `client` at
the end of the test to ensure it is closed (referencing createTestClient and the
local `client` variable).
---
Nitpick comments:
In `@packages/orm/src/client/crud/validator/validator.ts`:
- Around line 185-186: The validateProcedureInput(proc: string, input: unknown)
method currently rebuilds/validates inputs per call; convert this to use a
cached Zod schema per procedure name to avoid repeated schema construction.
Implement a small cache (e.g., Map<string, ZodSchema>) keyed by proc, generate
or retrieve the Zod schema once when validateProcedureInput is first called for
that proc, then reuse it for subsequent validations, and ensure errors are
thrown or returned in the same shape as before; update validateProcedureInput to
look up the schema in the cache, compile it on miss, and validate input using
the cached ZodSchema.
|
Claude finished @ymc9's task —— View job PR Review: Zod Schemas for ZModel Constructs and ORM Query Input ValidationsTodo List:
OverviewThis is a substantial and well-architected PR that introduces comprehensive Zod integration for ZModel validation. The implementation adds a new ✅ Architecture & Design ExcellenceOutstanding Design Patterns:
Well-Structured Organization:
✅ Implementation StrengthsComprehensive Type Safety:
Robust Validation Architecture:
Performance Considerations:
✅ Test CoverageComprehensive Testing:
|
There was a problem hiding this comment.
🧹 Nitpick comments (7)
packages/schema/src/accessor.ts (2)
216-232: Consider adding asettrap to prevent silent misuse.The proxy has no
settrap, so assignments likeaccessor.models = {}would silently succeed on the internal{ schema }target object rather than mutating the originalschema. If this is intended as a read-only accessor, an explicitsettrap (returningfalseor throwing) would prevent confusing bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/schema/src/accessor.ts` around lines 216 - 232, The Proxy returned by SchemaAccessor currently only defines a get trap, allowing assignments (e.g. accessor.models = {}) to silently mutate the internal { schema } target; add a set trap to the Proxy handler in SchemaAccessor that prevents mutation by either throwing a TypeError or returning false (so strict mode will throw) when any property is assigned, and ensure it references the same symbols (handler in SchemaAccessor, the target containing schema, and accessors) so attempts to set properties on the accessor are blocked rather than applied to the internal target.
82-86: Setthis.namefor proper error identification.
Errorsubclasses defaultnameto"Error". Without setting it,error.name,toString(), and stack traces won't reflect the actual error class.Proposed fix
export class InvalidSchemaError extends Error { constructor(message: string) { super(message); + this.name = 'InvalidSchemaError'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/schema/src/accessor.ts` around lines 82 - 86, The InvalidSchemaError class doesn't set this.name or restore the prototype chain, so error.name and stack traces show "Error" and instanceof may fail; in the InvalidSchemaError constructor set this.name = "InvalidSchemaError" and call Object.setPrototypeOf(this, InvalidSchemaError.prototype) (or equivalent) after super(message) to preserve the custom error identity and proper instanceof behavior.packages/orm/src/client/zod/factory.ts (3)
980-980: Inconsistent parameter casing:WithAggregationshould bewithAggregation.The parameter
WithAggregationon line 980 uses PascalCase while all other parameters in this file use camelCase (e.g.,withRelation,withAggregations,withoutRelationFields).Suggested fix
- private makeOrderBySchema(model: string, withRelation: boolean, WithAggregation: boolean) { + private makeOrderBySchema(model: string, withRelation: boolean, withAggregation: boolean) {And update all references within the method (lines 1018, 1021).
🤖 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` at line 980, The parameter name in makeOrderBySchema is using PascalCase (`WithAggregation`); rename it to camelCase `withAggregation` and update every reference inside the makeOrderBySchema function (currently referencing `WithAggregation`) to use `withAggregation` instead so casing is consistent with other parameters like `withRelation` and `withAggregations`; ensure both the parameter declaration and all internal usages (including any conditional checks or passed-through values) are updated to the new identifier.
124-142: Debug infrastructure uses@ts-ignoreand includesconsole.log.
getCache,setCache, andprintCacheStatsall suppress TypeScript errors with@ts-ignore.printCacheStatswrites to console, which could leak into production. If these are only for the@cache()decorator's internal use, consider making the decorator access the map directly, or at minimum ensureprintCacheStatsis behind an environment check.🤖 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 124 - 142, The three helper methods (getCache, setCache, printCacheStats) suppress TypeScript errors with `@ts-ignore` and print to console; remove the `@ts-ignore` by giving the underlying schemaCache a proper type (e.g., declare schemaCache: Map<string, ZodType> or Record<string, ZodType>) and update getCache(cacheKey: string): ZodType | undefined and setCache(cacheKey: string, schema: ZodType): this|void signatures accordingly so TypeScript errors are resolved; then either (a) let the `@cache`() decorator access the typed schemaCache directly (export or expose a protected accessor) or (b) keep the methods but gate printCacheStats(detailed = false) behind a runtime flag (e.g., process.env.NODE_ENV === 'development' or a DEBUG flag) so console.log is not executed in production. Ensure references to getCache, setCache, and printCacheStats in the decorator are updated to the new typed API.
1392-1404: Fragile cast chain:makeUpdateManySchemaresult →ZodObjectfor.extend().
makeUpdateManySchemareturnsZodType(after casting), and line 1397 casts it back toZodObjectto call.extend(). This works becausemergePluginArgsSchemareturns.strict()(aZodObject), but if that implementation changes or a.refine()is added upstream, the cast will silently produce a runtime error when.extend()is called on a non-object schema.Consider having
mergePluginArgsSchemareturnZodObjectexplicitly, or restructuremakeUpdateManyAndReturnSchemato build its own base schema rather than extending the updateMany schema.🤖 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 1392 - 1404, The current implementation of makeUpdateManyAndReturnSchema casts the result of makeUpdateManySchema to ZodObject to call .extend(), which is brittle if makeUpdateManySchema/mergePluginArgsSchema ever return a non-object ZodType; update the code so you do not blindly cast: either change mergePluginArgsSchema (the upstream producer used by makeUpdateManySchema) to have a return type of ZodObject explicitly, or in makeUpdateManyAndReturnSchema validate the returned ZodType from makeUpdateManySchema is a ZodObject (e.g., check instance/type) and then safely call .extend(), or build a fresh base object schema instead of extending the updateMany schema; keep references to makeUpdateManyAndReturnSchema, makeUpdateManySchema, mergePluginArgsSchema, makeSelectSchema, makeOmitSchema and refineForSelectOmitMutuallyExclusive when applying the fix.packages/zod/src/factory.ts (1)
105-114:makeJsonSchemacreates a new schema instance on every call — no caching.Unlike
makeModelSchema/makeTypeSchema,makeJsonSchemais not memoized. Since it's recursive (viaz.lazy), each call returns a fresh schema. Consider caching if this is called frequently.🤖 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 105 - 114, makeJsonSchema currently builds a new recursive Zod schema on every call; add memoization similar to makeModelSchema/makeTypeSchema by introducing a private cache field (e.g., jsonSchemaCache or _jsonSchema) on the class, check and return it if present, otherwise construct the union schema, assign it to that cache, and return it; ensure the z.lazy closures still reference the cached schema (build and assign the schema before any external returns so the recursion sees the same instance).packages/orm/src/client/crud/validator/validator.ts (1)
184-266:validateProcedureInputhas a TODO to convert manual validation to Zod schema.The current implementation manually validates envelope structure, required/optional params, and per-param types. While thorough, it's ~80 lines of imperative checks that could be expressed as a single Zod schema (strict object with optional/required fields, each typed by
makeProcedureParamSchema). This would also enable caching.Would you like me to open an issue to track converting
validateProcedureInputto a Zod-schema-based approach?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/validator/validator.ts` around lines 184 - 266, The validateProcedureInput implementation should be replaced by a Zod-schema-based validation: build a cached Zod schema per procedure (keyed by proc) using zodFactory.makeProcedureParamSchema(param) for each param to compose a strict object with required vs optional fields, then validate the incoming envelope (allowing undefined input only when params.length === 0 or all params optional) by extracting envelope.args and running schema.safeParse; on failure throw createInvalidInputError with formatError(parsed.error). Implement caching (e.g., this._procSchemas: Map<string, ZodType>) to avoid rebuilding schemas, keep special-case handling for procedures with zero params (accept empty args) and ensure the schema enforces no extra keys (strict). Ensure all references use validateProcedureInput, zodFactory.makeProcedureParamSchema, createInvalidInputError, and formatError so reviewers can locate code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/orm/src/client/crud/validator/validator.ts`:
- Around line 28-33: InputValidator is only generic over Schema, losing Options
and ExtQueryArgs precision; update the class to forward the client generics so
zodFactory and methods keep correct types. Change InputValidator to accept the
same Options and ExtQueryArgs generics used by ClientContract and
ZodSchemaFactory (e.g., InputValidator<Schema, Options extends
ClientOptions<Schema>, ExtQueryArgs = {}>) and update the zodFactory declaration
to ZodSchemaFactory<Schema, Options, ExtQueryArgs> and the constructor signature
to accept ClientContract<Schema, Options, ExtQueryArgs>, ensuring all validate
methods and any return types use those generics rather than any.
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 1471-1487: The numeric update branch uses z.number() for
set/increment/decrement/multiply/divide which rejects Decimal/BigInt; replace
z.number() with the field's actual scalar schema (the same schema used for the
field's base type) so those operators accept Decimal/BigInt where applicable.
Locate the branch guarded by isNumericField(fieldDef) and use the field's scalar
schema (from whatever helper/method supplies the field's base zod schema for
fieldDef) in nullableIf(...) and for each operator's .optional() type,
preserving the refine(...) single-key check and existing optional/null handling
via nullableIf and fieldDef.optional. Ensure behavior remains identical for
plain numeric fields by falling back to z.number() if the scalar schema is not
available.
In `@packages/zod/src/factory.ts`:
- Around line 41-65: The relation field construction in makeModelSchema creates
a lazy schema then calls applyCardinality(lazySchema, fieldDef).optional(),
which redundantly adds a second .optional() because applyCardinality already
applies nullable/optional for optional relations; update makeModelSchema to use
the result of applyCardinality directly (without appending .optional()) for
relation fields so you don't wrap optionals twice (refer to makeModelSchema and
applyCardinality to locate the change).
---
Nitpick comments:
In `@packages/orm/src/client/crud/validator/validator.ts`:
- Around line 184-266: The validateProcedureInput implementation should be
replaced by a Zod-schema-based validation: build a cached Zod schema per
procedure (keyed by proc) using zodFactory.makeProcedureParamSchema(param) for
each param to compose a strict object with required vs optional fields, then
validate the incoming envelope (allowing undefined input only when params.length
=== 0 or all params optional) by extracting envelope.args and running
schema.safeParse; on failure throw createInvalidInputError with
formatError(parsed.error). Implement caching (e.g., this._procSchemas:
Map<string, ZodType>) to avoid rebuilding schemas, keep special-case handling
for procedures with zero params (accept empty args) and ensure the schema
enforces no extra keys (strict). Ensure all references use
validateProcedureInput, zodFactory.makeProcedureParamSchema,
createInvalidInputError, and formatError so reviewers can locate code to change.
In `@packages/orm/src/client/zod/factory.ts`:
- Line 980: The parameter name in makeOrderBySchema is using PascalCase
(`WithAggregation`); rename it to camelCase `withAggregation` and update every
reference inside the makeOrderBySchema function (currently referencing
`WithAggregation`) to use `withAggregation` instead so casing is consistent with
other parameters like `withRelation` and `withAggregations`; ensure both the
parameter declaration and all internal usages (including any conditional checks
or passed-through values) are updated to the new identifier.
- Around line 124-142: The three helper methods (getCache, setCache,
printCacheStats) suppress TypeScript errors with `@ts-ignore` and print to
console; remove the `@ts-ignore` by giving the underlying schemaCache a proper
type (e.g., declare schemaCache: Map<string, ZodType> or Record<string,
ZodType>) and update getCache(cacheKey: string): ZodType | undefined and
setCache(cacheKey: string, schema: ZodType): this|void signatures accordingly so
TypeScript errors are resolved; then either (a) let the `@cache`() decorator
access the typed schemaCache directly (export or expose a protected accessor) or
(b) keep the methods but gate printCacheStats(detailed = false) behind a runtime
flag (e.g., process.env.NODE_ENV === 'development' or a DEBUG flag) so
console.log is not executed in production. Ensure references to getCache,
setCache, and printCacheStats in the decorator are updated to the new typed API.
- Around line 1392-1404: The current implementation of
makeUpdateManyAndReturnSchema casts the result of makeUpdateManySchema to
ZodObject to call .extend(), which is brittle if
makeUpdateManySchema/mergePluginArgsSchema ever return a non-object ZodType;
update the code so you do not blindly cast: either change mergePluginArgsSchema
(the upstream producer used by makeUpdateManySchema) to have a return type of
ZodObject explicitly, or in makeUpdateManyAndReturnSchema validate the returned
ZodType from makeUpdateManySchema is a ZodObject (e.g., check instance/type) and
then safely call .extend(), or build a fresh base object schema instead of
extending the updateMany schema; keep references to
makeUpdateManyAndReturnSchema, makeUpdateManySchema, mergePluginArgsSchema,
makeSelectSchema, makeOmitSchema and refineForSelectOmitMutuallyExclusive when
applying the fix.
In `@packages/schema/src/accessor.ts`:
- Around line 216-232: The Proxy returned by SchemaAccessor currently only
defines a get trap, allowing assignments (e.g. accessor.models = {}) to silently
mutate the internal { schema } target; add a set trap to the Proxy handler in
SchemaAccessor that prevents mutation by either throwing a TypeError or
returning false (so strict mode will throw) when any property is assigned, and
ensure it references the same symbols (handler in SchemaAccessor, the target
containing schema, and accessors) so attempts to set properties on the accessor
are blocked rather than applied to the internal target.
- Around line 82-86: The InvalidSchemaError class doesn't set this.name or
restore the prototype chain, so error.name and stack traces show "Error" and
instanceof may fail; in the InvalidSchemaError constructor set this.name =
"InvalidSchemaError" and call Object.setPrototypeOf(this,
InvalidSchemaError.prototype) (or equivalent) after super(message) to preserve
the custom error identity and proper instanceof behavior.
In `@packages/zod/src/factory.ts`:
- Around line 105-114: makeJsonSchema currently builds a new recursive Zod
schema on every call; add memoization similar to makeModelSchema/makeTypeSchema
by introducing a private cache field (e.g., jsonSchemaCache or _jsonSchema) on
the class, check and return it if present, otherwise construct the union schema,
assign it to that cache, and return it; ensure the z.lazy closures still
reference the cached schema (build and assign the schema before any external
returns so the recursion sees the same instance).
|
Claude finished @ymc9's task —— View job PR Review: Zod Schemas for ZModel Constructs and ORM Query Input ValidationsTodo List:
OverviewThis is a substantial and well-architected PR that introduces comprehensive Zod integration for ZModel validation. The implementation adds a new ✅ Architecture & Design ExcellenceOutstanding Design Patterns:
Well-Structured Organization:
✅ Implementation StrengthsComprehensive Type Safety:
Robust Validation Architecture:
Performance Considerations:
✅ Test CoverageComprehensive Testing:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/zod.test-d.ts (1)
208-225:makeExistsSchemalacks a schema-shape testEvery other factory method (
makeFindManySchema,makeCreateSchema, …makeGroupBySchema) has a dedicateditblock asserting the fields present on its inferredArgstype.makeExistsSchemais only verified as a function (line 224). Consider adding a shape test (e.g. asserting the presence of awhereproperty) for completeness and to catch any future regression in the schema's structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/zod.test-d.ts` around lines 208 - 225, Add a shape test for makeExistsSchema similar to the other factory method tests: infer the Args type from client.$zod.makeExistsSchema and assert it contains the expected properties (at minimum a where property) using expectTypeOf(...).toHaveProperty or the same pattern used for other tests (e.g., verifying Args["where"] exists); update the it block for makeExistsSchema to perform this assertion so regressions to the schema shape are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/orm/client-api/zod.test-d.ts`:
- Around line 208-225: Add a shape test for makeExistsSchema similar to the
other factory method tests: infer the Args type from
client.$zod.makeExistsSchema and assert it contains the expected properties (at
minimum a where property) using expectTypeOf(...).toHaveProperty or the same
pattern used for other tests (e.g., verifying Args["where"] exists); update the
it block for makeExistsSchema to perform this assertion so regressions to the
schema shape are caught.
|
Claude finished @ymc9's task —— View job PR Review: Zod Schemas for ZModel Constructs and ORM Query Input ValidationsTodo List:
OverviewThis is a substantial and well-architected PR that introduces comprehensive Zod integration for ZModel validation. The implementation adds a new ✅ Architecture & Design ExcellenceOutstanding Design Patterns:
Well-Structured Organization:
✅ Implementation StrengthsComprehensive Type Safety:
Robust Validation Architecture:
Performance Considerations:
✅ Test CoverageComprehensive Testing:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/orm/src/client/zod/factory.ts (2)
984-985:WithAggregationparameter uses PascalCase — rename towithAggregation.Inconsistent with the adjacent
withRelationparameter (camelCase). Minor but inconsistent across the parameter list.♻️ Proposed fix
- private makeOrderBySchema(model: string, withRelation: boolean, WithAggregation: boolean) { + private makeOrderBySchema(model: string, withRelation: boolean, withAggregation: boolean) {All internal references to
WithAggregation(lines 995, 996, 1023, 1026) should be updated accordingly.🤖 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 984 - 985, The parameter name in makeOrderBySchema is inconsistent: rename the PascalCase parameter WithAggregation to camelCase withAggregation and update all internal references to that parameter (occurrences of WithAggregation inside makeOrderBySchema and any callers that pass or reference it) as well as any local variables that depend on it so the function signature and its internal usage remain consistent (search for symbols "makeOrderBySchema" and "WithAggregation" and replace with "withAggregation").
136-143: Remove or gate theprintCacheStatsdebug artifact.This private method exists purely for debugging, but the
console.logcalls in it will execute in production if the@cache()decorator ever surfaces this method. Either remove it or guard it with a debug/verbose option.🤖 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 136 - 143, The private debug method printCacheStats (which iterates this.schemaCache.keys()) must not emit console.log in production: either remove the method entirely or replace its direct console.log calls with gated logging—e.g., check a debug/verbose flag or environment variable (process.env.DEBUG or an injected logger like this.logger?.debug/isVerbose) before printing, or switch to using the existing logger instead of console; ensure no unconditional console.log remains so that `@cache-decorated` surfaces won't leak debug output.
🤖 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/orm/src/client/zod/factory.ts`:
- Around line 1033-1038: makeDistinctSchema currently calls
z.enum(nonRelationFields as any) without guarding for the empty-array case,
which will throw in Zod v4; mirror the pattern used in makeGroupBySchema by
checking nonRelationFields.length and return z.never() when there are no
non-relation fields. Update makeDistinctSchema (and keep the
requireModel(this.schema, model) / nonRelationFields calculation) so the
returned schema is nonRelationFields.length > 0 ?
this.orArray(z.enum(nonRelationFields as any), true) : z.never().
In `@tests/e2e/orm/client-api/zod.test-d.ts`:
- Around line 216-233: Add an assertion to the existing type-check block to
verify the missing factory method: the test that ensures client.$zod is a
ZodSchemaFactory should include
expectTypeOf(client.$zod.makeDeleteManySchema).toBeFunction(); so that the
makeDeleteManySchema factory method is covered alongside
makeDeleteSchema/makeUpdateManySchema etc., ensuring its existence is tested;
update the it block that references client.$zod factory methods (the one
containing makeFindManySchema...makeExistsSchema) to include this additional
check.
---
Duplicate comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 1728-1742: The groupBy orderBy handling issue has already been
addressed: in the schema.refine predicate replace any direct access to
value.orderBy with enumerate(value.orderBy) and iterate over each normalized
orderBy object (as done), filtering Object.keys(orderBy) by AggregateOperators
and ensuring every non-aggregate key appears in the enumerate(value.by) list; no
further code changes are required to fix this specific problem in factory.ts
(see schema.refine, enumerate, AggregateOperators).
- Around line 1476-1493: The numeric-update branch in the zod factory uses
z.number() for set/increment/decrement/multiply/divide which breaks
Decimal/BigInt/string numeric fields; in the isNumericField handling inside the
class (see makeScalarSchema, isNumericField, nullableIf and
fieldDef/fieldSchema), replace the hardcoded z.number() with the scalar schema
produced by makeScalarSchema(fieldDef) (and wrap it with
nullableIf/...optional() exactly as done for the original fieldSchema/set
variant) so set and the arithmetic ops accept the same per-field type
(Decimal/bigint/string/number); keep the single-key refine check unchanged.
---
Nitpick comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 984-985: The parameter name in makeOrderBySchema is inconsistent:
rename the PascalCase parameter WithAggregation to camelCase withAggregation and
update all internal references to that parameter (occurrences of WithAggregation
inside makeOrderBySchema and any callers that pass or reference it) as well as
any local variables that depend on it so the function signature and its internal
usage remain consistent (search for symbols "makeOrderBySchema" and
"WithAggregation" and replace with "withAggregation").
- Around line 136-143: The private debug method printCacheStats (which iterates
this.schemaCache.keys()) must not emit console.log in production: either remove
the method entirely or replace its direct console.log calls with gated
logging—e.g., check a debug/verbose flag or environment variable
(process.env.DEBUG or an injected logger like this.logger?.debug/isVerbose)
before printing, or switch to using the existing logger instead of console;
ensure no unconditional console.log remains so that `@cache-decorated` surfaces
won't leak debug output.
|
Claude finished @ymc9's task —— View job PR Review: Zod Schemas for ZModel Constructs and ORM Query Input ValidationsTodo List:
OverviewThis is a substantial and well-architected PR that introduces comprehensive Zod integration for ZModel validation. The implementation adds a new ✅ Architecture & Design ExcellenceOutstanding Design Patterns:
Well-Structured Organization:
✅ Implementation StrengthsComprehensive Type Safety:
Robust Validation Architecture:
Performance Considerations:
✅ Test CoverageComprehensive Testing:
|
|
Claude finished @ymc9's task —— View job PR Review: Zod Schemas for ZModel Constructs and ORM Query Input ValidationsTodo List:
OverviewThis is a substantial and well-architected PR that introduces comprehensive Zod integration for ZModel validation. The implementation adds a new ✅ Architecture & Design ExcellenceOutstanding Design Patterns:
Well-Structured Organization:
✅ Implementation StrengthsComprehensive Type Safety:
Robust Validation Architecture:
Performance Considerations:
✅ Test CoverageComprehensive Testing:
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests