Conversation
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
- Add "create" and "update" variants to model schemas - Simplify dependencies - Translate ZModel meta description into zod meta
…in support in child updates (#2369)
…#2401) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…) (#2402) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ing from a to-one relation (#2403)
…rations and validation (#2399) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
|
Claude finished @ymc9's task —— View job Code Review for v3.4.0 ReleaseI've completed a comprehensive review of this large release PR. Here's my analysis: Review Tasks
🔧 Code Quality & ArchitecturePositive Changes
Areas for Improvement
🚨 Security ConcernsHigh Priority
Medium Priority
⚡ Performance ConsiderationsOptimizations
Concerns
🧪 Test CoverageWell Tested
Missing Coverage
🔄 New Features AnalysisMajor Additions
Breaking Changes
📋 Specific RecommendationsSecurity Fixes Needed// packages/cli/src/actions/proxy.ts:107-122
function evaluateUrl(schemaUrl: ConfigExpr) {
if (isInvocationExpr(schemaUrl)) {
const envFunction = schemaUrl as InvocationExpr;
const envName = getStringLiteral(envFunction.args[0]?.value as LiteralExpr)!;
// ADD: Validate environment variable name
if (!/^[A-Z_][A-Z0-9_]*$/.test(envName)) {
throw new CliError(`Invalid environment variable name: ${envName}`);
}
const envValue = process.env[envName];
// ... rest of function
}
}Code Quality Improvements
Performance Optimizations
✅ Overall AssessmentThis is a substantial and well-architected release that significantly improves the developer experience. The type system enhancements, client slicing, and RPC functionality are excellent additions. Primary concerns: Address the security issue with environment variable validation and consider the TypeScript compilation performance impact of complex types. Recommendation: ✅ Approve after addressing the security concern above The codebase shows excellent engineering practices with proper error handling, extensive testing, and thoughtful architectural decisions. |
📝 WalkthroughWalkthroughAdds a standalone schema package and Zod schema factory, threads per-query/options slicing through ORM types and runtime, integrates Zod-based input validation and InputValidator relocation, normalizes temporary SQL aliases, extends CLI generation flags and usage tips, updates dialect loading to dynamic imports, and bumps many packages to 3.4.0. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orm/src/client/crud/operations/delete.ts (1)
24-50:⚠️ Potential issue | 🟠 MajorMove policy rejection inside the transaction to keep delete atomic.
At Line 42,
createRejectedByPolicyErroris thrown aftersafeTransactionresolves. If the delete succeeds but read-back is disallowed, the row is already committed as deleted while the API returns an error.💡 Suggested fix
- const result = await this.safeTransaction(async (tx) => { + return await this.safeTransaction(async (tx) => { let preDeleteRead: any = undefined; if (needReadBack) { preDeleteRead = await this.readUnique(tx, this.model, { select: args.select, include: args.include, omit: args.omit, where: args.where, }); } const deleteResult = await this.delete(tx, this.model, args.where, undefined, undefined, selectedFields); if (!deleteResult.numAffectedRows) { throw createNotFoundError(this.model); } - - return needReadBack ? preDeleteRead : deleteResult.rows[0]; + const result = needReadBack ? preDeleteRead : deleteResult.rows[0]; + if (!result && this.hasPolicyEnabled) { + throw createRejectedByPolicyError( + this.model, + RejectedByPolicyReason.CANNOT_READ_BACK, + 'result is not allowed to be read back', + ); + } + return result; }); - - if (!result && this.hasPolicyEnabled) { - throw createRejectedByPolicyError( - this.model, - RejectedByPolicyReason.CANNOT_READ_BACK, - 'result is not allowed to be read back', - ); - } - - return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/operations/delete.ts` around lines 24 - 50, The policy rejection must be thrown inside the safeTransaction callback to keep the delete atomic: move the createRejectedByPolicyError check into the async function passed to safeTransaction (in the same block that calls readUnique, delete, and builds the return value). Specifically, inside the safeTransaction callback in the delete operation (the code path using readUnique, delete, deleteResult, preDeleteRead, needReadBack), after computing the value to return (either preDeleteRead or deleteResult.rows[0]) but before returning it, check this.hasPolicyEnabled and whether the computed result is disallowed (matching the existing check: result falsy / cannot read back) and throw createRejectedByPolicyError(this.model, RejectedByPolicyReason.CANNOT_READ_BACK, 'result is not allowed to be read back') so the transaction aborts instead of committing.
🧹 Nitpick comments (13)
tests/regression/test/issue-2375.test.ts (1)
74-76: Assert result ordering explicitly to harden the regression test.On Line 74, the query is executed but its output is not validated. Capturing and asserting ordered results will prevent silent behavior regressions.
Proposed test hardening
-import { describe, it } from 'vitest'; +import { describe, expect, it } from 'vitest'; @@ - await db.testRunStepResults.findMany({ + const rows = await db.testRunStepResults.findMany({ + include: { + step: { select: { order: true } }, + sharedStepItem: { select: { order: true } }, + }, orderBy: [{ step: { order: 'asc' } }, { sharedStepItem: { order: 'asc' } }], }); + + expect(rows).toHaveLength(2); + expect(rows.map((r: any) => [r.step.order, r.sharedStepItem?.order ?? null])).toEqual([ + [1, 1], + [2, 2], + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2375.test.ts` around lines 74 - 76, Capture the output of db.testRunStepResults.findMany into a variable (e.g., results) and add assertions that verify the ordering by step.order then sharedStepItem.order: map the results to the ordering keys (result.step.order and result.sharedStepItem?.order) and assert the sequence is non-decreasing (or matches an expected array of orders) so the test explicitly validates the sort applied by findMany.tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts (1)
698-698: Remove leftover debug logging from the E2E test.
console.log('RUNNING!!!!')is a debug artifact and will add noise to CI test output.Proposed cleanup
- console.log('RUNNING!!!!');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts` at line 698, Remove the leftover debug log by deleting the stray console.log('RUNNING!!!!') statement found in the entity-mutation-hooks.test E2E test; simply remove that line (or replace with a proper test assertion/log helper if needed) so CI output is not cluttered by debug noise.packages/zod/src/error.ts (1)
4-7: Consider settingthis.namefor better error identification.Custom error classes should set the
nameproperty to improve debugging and error handling. Without it, the error name defaults to "Error" in stack traces.♻️ Proposed fix
export class SchemaFactoryError extends Error { constructor(message: string) { super(message); + this.name = 'SchemaFactoryError'; } }🤖 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 SchemaFactoryError class constructor should set the error name for clearer identification: inside the constructor of SchemaFactoryError, assign this.name = 'SchemaFactoryError' (and optionally call Error.captureStackTrace(this, SchemaFactoryError) for V8 stack traces) so stack traces and error checks reflect the correct custom error type.tests/regression/test/issue-2344.test.ts (1)
4-43: Consider adding cleanup to prevent resource leaks.Similar to the issue-2378 test, this test creates a database client but doesn't disconnect after completion. Consider adding cleanup for consistency with other test files.
♻️ Proposed fix to add cleanup
describe('Regression for issue 2344', () => { + let db: Awaited<ReturnType<typeof createTestClient>>; + + afterEach(async () => { + await db?.$disconnect(); + }); + it('should reject select with only false fields', async () => { - const db = await createTestClient( + db = await createTestClient(Import
afterEach:-import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2344.test.ts` around lines 4 - 43, The test creates a DB client via createTestClient and never disconnects, causing resource leaks; add a cleanup that calls db.disconnect() after the test (use afterEach or equivalent test teardown) so the client created in the test's scope is closed; locate the db variable returned by createTestClient in this file (tests/regression/test/issue-2344.test.ts) and ensure an afterEach(async () => { await db.disconnect(); }) or equivalent is added and imported if necessary.tests/regression/test/issue-2378.test.ts (1)
4-103: Consider adding cleanup to prevent resource leaks.The test creates a database client but doesn't disconnect after the test completes. Other tests in the codebase (e.g.,
find.test.ts) useafterEachto ensure proper cleanup.♻️ Proposed fix to add cleanup
describe('Regression for issue `#2378`', () => { + let db: Awaited<ReturnType<typeof createTestClient>>; + + afterEach(async () => { + await db?.$disconnect(); + }); + it('deep nested include should not generate alias names exceeding 63 bytes', async () => { - const db = await createTestClient( + db = await createTestClient(You'll also need to import
afterEachfrom vitest:-import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2378.test.ts` around lines 4 - 103, The test opens a Prisma client via createTestClient but never disconnects it; add proper cleanup by importing afterEach from vitest and calling await db.$disconnect() in an afterEach hook (guarded if db is defined) so the client created by createTestClient is always disconnected after the test; if you prefer, declare let db at the test scope, assign it from createTestClient inside the it block, and ensure the afterEach calls await db.$disconnect() to prevent resource leaks.packages/orm/src/client/constants.ts (1)
1-4: Remove stale commented-out export block.These lines are dead code and can be removed to keep the constants module clean.
🤖 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 stale commented-out export block for CONTEXT_COMMENT_PREFIX in constants.ts: delete the three commented lines that define/export CONTEXT_COMMENT_PREFIX (the comment and the export statement) so the module contains only active constants and no dead commented code.packages/cli/src/actions/action-utils.ts (1)
260-271: Consider adding a guard against multiple callback invocations.The returned callback modifies shared closure state (
fetchComplete) but doesn't prevent the tip from being printed multiple times if the callback is invoked more than once. While likely called once in practice, a simple guard would make this more robust.♻️ Optional: Add invocation guard
export function startUsageTipsFetch() { let fetchedData: z.infer<typeof usageTipsSchema> | undefined = undefined; let fetchComplete = false; + let callbackInvoked = false; const start = Date.now(); const controller = new AbortController(); // ... fetch logic ... return async () => { + if (callbackInvoked) return; + callbackInvoked = true; + const elapsed = Date.now() - start; // ... rest of callback ... }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/actions/action-utils.ts` around lines 260 - 271, The callback returned by the function uses shared closure state (fetchComplete) and may be invoked multiple times, allowing controller.abort() and the tip-print logic to run repeatedly; add an invocation guard (e.g., a local boolean like "callbackInvoked" or set fetchComplete immediately) at the start of the returned async () => to short-circuit on subsequent calls so controller.abort() and related logic execute only once; update the returned callback that references fetchComplete, controller.abort(), and FETCH_CLI_MAX_TIME accordingly to check and set the guard before proceeding.packages/schema/test/accessor.test.ts (1)
5-117: Consider adding direct tests forgetField/requireFieldandgetDelegateDiscriminator.Those accessor paths were introduced in the same feature but aren’t directly exercised in this suite yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/schema/test/accessor.test.ts` around lines 5 - 117, Add tests to exercise SchemaAccessor.getField and SchemaAccessor.requireField (call them for an existing model like 'User' and assert returned field objects and that requireField throws InvalidSchemaError with the expected message for a missing field), and add tests for SchemaAccessor.getDelegateDiscriminator (verify it returns the expected discriminator for a model that uses delegation and returns undefined for models without one); use existing schema/models (e.g., 'User' and 'Post') and reference the exact method names getField, requireField, and getDelegateDiscriminator so the assertions cover both success and failure paths.tests/e2e/orm/client-api/slicing.test.ts (1)
162-167: Remove stale commented config and align the test title with actual setup.This case currently tests non-included models, not explicitly excluded models. Keeping title/config aligned will reduce ambiguity.
🧹 Suggested cleanup
- it('prevents excluded models from being used in include clause', async () => { + it('prevents models not in includedModels from being used in include clause', async () => { const options = { slicing: { includedModels: ['User', 'Post'] as const, - // excludedModels: ['Profile', 'Comment'] as const, }, dialect: {} as any, } as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/slicing.test.ts` around lines 162 - 167, Update the test so its description matches what it actually asserts and remove the stale commented config: change the it(...) title from 'prevents excluded models from being used in include clause' to something like 'prevents non-included models from being used in include clause', and delete the commented-out excludedModels line from the options object (the test uses slicing.includedModels: ['User','Post'] in the options variable). Ensure any assertions or variable names referring to excludedModels are updated to reflect "non-included" semantics so the test and its setup are consistent.packages/orm/src/client/crud/validator/validator.ts (1)
185-266:validateProcedureInputreturns original input rather than validated data.Unlike the CRUD validators that return
datafrom Zod'ssafeParseresult (which may include transformations),validateProcedureInputreturns the originalinputobject. If procedure parameter schemas include transformations (e.g., coercion, defaults), these won't be reflected in the returned value.Consider aligning this behavior with the other validators by constructing the validated args object from the parsed results, or document this as intentional if transformations are not expected for procedure params.
🤖 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 - 266, validateProcedureInput currently returns the original input (losing Zod transformations); update validateProcedureInput to return the validated/transformed data from each schema.safeParse call (use parsed.data) instead of the raw value: for procedures with params, build a new args object by iterating params, using parsed.data for each param (and preserving optional/defaulted values), then return the envelope with its args replaced by this validated args object (or return the args object directly if the function should return only args); also handle the params.length === 0 and missing-args branches consistently (return transformed envelope/args when present). Reference: validateProcedureInput, zodFactory.makeProcedureParamSchema, schema.safeParse, parsed.data.packages/orm/src/client/crud/operations/create.ts (1)
38-38:as anycasts for where clause construction.The casts are necessary because
getIdValuesreturnsRecord<string, any>, which doesn't satisfy the strongly-typedWhereUniqueInput. The TODO at line 86 acknowledges this as a future improvement area.Consider creating a helper type or utility that constructs a properly-typed where clause from ID values, or documenting why the cast is safe (i.e.,
getIdValuesguarantees the shape matches the model's unique fields).Also applies to: 84-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/operations/create.ts` at line 38, The cast "as any" used when passing getIdValues(this.schema, this.model, createResult) into the strongly-typed WhereUniqueInput should be replaced by a small typed helper that validates/constructs a proper WhereUniqueInput: add a function (e.g., buildWhereUniqueFromIdValues(schema, model, idValues): WhereUniqueInput<ModelType>) which accepts the schema, model and getIdValues output, verifies the required unique keys are present (or maps id fields to the model's unique field names) and returns a correctly-typed object; then replace the inline casts in create.ts (and the other occurrences around the TODO) to call buildWhereUniqueFromIdValues(this.schema, this.model, createResult) so the compiler sees a proper WhereUniqueInput and you no longer need "as any" (alternatively, if runtime validation is undesirable, add a narrowly-scoped typed wrapper function that documents and asserts the guarantee that getIdValues returns the required shape and returns it as WhereUniqueInput).packages/clients/tanstack-query/test/react-sliced-client.test-d.ts (1)
13-16: Clarify slicing configuration intent.The
_dbconfiguration includes'update'in bothincludedOperationsandexcludedOperations. While this appears intentional for testing type extraction viaGetQueryOptions<typeof _db>, it's worth noting that:
- The
_dbinstance is only used for type extraction in the "sliced models" test (line 23)- The "sliced operations" test (lines 30-48) uses a different inline type that includes
'update'without excluding itConsider adding a brief comment clarifying this is intentional for demonstrating exclusion precedence behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/clients/tanstack-query/test/react-sliced-client.test-d.ts` around lines 13 - 16, Add a clarifying comment where the `_db` test config is defined to explain that including 'update' in both includedOperations and excludedOperations is intentional to demonstrate exclusion precedence when extracting types with GetQueryOptions<typeof _db>; mention that `_db` is only used for the "sliced models" test while the "sliced operations" test uses a different inline type that retains 'update'. This comment should be placed next to the `_db` object and reference includedOperations, excludedOperations, and GetQueryOptions so future readers understand the test intent.packages/cli/src/index.ts (1)
46-52: Unreachable error handling in argParser.The
throw new CliError(...)on line 50 is unreachable because.choices(['true', 'false'])already validates input values and throws its own error for invalid choices beforeargParseris invoked. While not harmful, this dead code could be removed for clarity.♻️ Proposed simplification
function triStateBooleanOption(flag: string, description: string) { return new Option(flag, description).choices(['true', 'false']).argParser((value) => { if (value === undefined || value === 'true') return true; - if (value === 'false') return false; - throw new CliError(`Invalid value for ${flag}: ${value}`); + return false; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/index.ts` around lines 46 - 52, The argParser in triStateBooleanOption contains an unreachable throw because .choices(['true','false']) already rejects invalid values; remove the final throw branch and simplify the parser to only handle undefined/'true' => true and 'false' => false (keep .choices if you want built-in validation/help), e.g., update triStateBooleanOption's argParser to return true for undefined or 'true' and false for 'false' and drop the throw new CliError(...) line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/clients/tanstack-query/test/react-typing.test-d.ts`:
- Around line 17-18: The test's `@ts-expect-error` is ambiguous because
client.user.useFindUnique({ select: { email: true } }).data.name can error
either from an omitted unique filter or from accessing an unselected field;
update the call to include a proper unique filter (e.g., add a where: { id: ...
} matching your model) so the only TypeScript error left is the unselected-field
access, keeping the `@ts-expect-error` on the same check(...) line and referencing
client.user.useFindUnique, select, and data.name to locate the change.
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 1545-1551: The update schema builder (makeUpdateDataSchema)
currently iterates over Object.keys(modelDef.fields) and includes fields that
should be read-only/internal; change the loop logic to mirror
makeCreateDataSchema by skipping fields where fieldDef.computed === true or
fieldDef.discriminator === true (in addition to existing withoutFields checks).
Locate the iteration that calls requireField(this.schema, model, field) and add
a guard that returns early for fieldDef.computed or fieldDef.discriminator so
computed/discriminator fields are excluded from the update validation schema.
- Around line 324-327: The Biome lint rule flags the arrow callback inside
parseResult.error.issues.forEach because it currently uses an expression body
that implicitly returns a value; change the callback to a block body without
returning anything. Locate the safeParse handling around schema.safeParse(value)
where parseResult is checked and update the forEach callback that calls
ctx.addIssue (i.e., parseResult.error.issues.forEach((issue) =>
ctx.addIssue(issue as any))) so it uses a block body: open a block, call
ctx.addIssue(issue as any), and close the block to eliminate any implicit
return.
In `@packages/zod/src/utils.ts`:
- Around line 424-441: The code uses getArgValue for the second argument
(search) inside evalCall which only handles literals and breaks for non-literal
expressions; replace getArgValue with evalCall<string>(expr.args?.[1]) (or the
existing evalCall helper) to evaluate the search argument at runtime, add an
invariant to ensure the evaluated search is present, and use the evaluated value
when applying case-insensitive logic in applyStringOp; update references to
"search" and the caseInsensitive branch so you call toLowerCase() on the
evaluated string rather than on a potentially undefined literal.
In `@tests/e2e/orm/client-api/slicing.test.ts`:
- Around line 494-509: The test uses an order-dependent assertion after
db.user.create by indexing user.posts (expect(user.posts[0]!.title).toBe('Post
1')) which can be flaky; update the assertions in the block that creates the
user (and the similar block at 535-549) to either (a) assert membership of
titles (e.g., check user.posts' titles contain 'Post 1' and 'Post 2' using
arrayContaining-style assertions) or (b) perform an explicit deterministic
ordering (sort user.posts by title/id) before indexing, replacing the direct
index-based expectation to remove implicit ordering dependence.
In `@tests/e2e/orm/client-api/zod.test.ts`:
- Around line 1006-1014: The test is disconnecting the wrong Prisma client in
the finally block; the client created by createTestClient(schema) inside the try
(symbol: createTestClient and the local variable currently named client) must be
closed. Fix by renaming the inner client to a distinct identifier (e.g.,
localClient) or hoisting a separate variable for it, update usages
(createQuerySchemaFactory(localClient) and s.safeParse calls remain same), and
call await localClient.$disconnect() in the finally block so the locally created
client is properly disconnected instead of the outer client.
In `@tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts`:
- Around line 699-735: The test currently disconnects the DB client only on the
happy path; wrap the test body that calls createTestClient(), client.$use(...),
withPlugin.child.create/update, assertions, etc. in a try/finally and move
client.$disconnect() into the finally block so the Prisma client (client) is
always closed even if an assertion or operation throws; locate the setup around
createTestClient, the withPlugin variable, and the created/updated operations to
enclose them in the try block and ensure client.$disconnect() runs in finally.
---
Outside diff comments:
In `@packages/orm/src/client/crud/operations/delete.ts`:
- Around line 24-50: The policy rejection must be thrown inside the
safeTransaction callback to keep the delete atomic: move the
createRejectedByPolicyError check into the async function passed to
safeTransaction (in the same block that calls readUnique, delete, and builds the
return value). Specifically, inside the safeTransaction callback in the delete
operation (the code path using readUnique, delete, deleteResult, preDeleteRead,
needReadBack), after computing the value to return (either preDeleteRead or
deleteResult.rows[0]) but before returning it, check this.hasPolicyEnabled and
whether the computed result is disallowed (matching the existing check: result
falsy / cannot read back) and throw createRejectedByPolicyError(this.model,
RejectedByPolicyReason.CANNOT_READ_BACK, 'result is not allowed to be read
back') so the transaction aborts instead of committing.
---
Nitpick comments:
In `@packages/cli/src/actions/action-utils.ts`:
- Around line 260-271: The callback returned by the function uses shared closure
state (fetchComplete) and may be invoked multiple times, allowing
controller.abort() and the tip-print logic to run repeatedly; add an invocation
guard (e.g., a local boolean like "callbackInvoked" or set fetchComplete
immediately) at the start of the returned async () => to short-circuit on
subsequent calls so controller.abort() and related logic execute only once;
update the returned callback that references fetchComplete, controller.abort(),
and FETCH_CLI_MAX_TIME accordingly to check and set the guard before proceeding.
In `@packages/cli/src/index.ts`:
- Around line 46-52: The argParser in triStateBooleanOption contains an
unreachable throw because .choices(['true','false']) already rejects invalid
values; remove the final throw branch and simplify the parser to only handle
undefined/'true' => true and 'false' => false (keep .choices if you want
built-in validation/help), e.g., update triStateBooleanOption's argParser to
return true for undefined or 'true' and false for 'false' and drop the throw new
CliError(...) line.
In `@packages/clients/tanstack-query/test/react-sliced-client.test-d.ts`:
- Around line 13-16: Add a clarifying comment where the `_db` test config is
defined to explain that including 'update' in both includedOperations and
excludedOperations is intentional to demonstrate exclusion precedence when
extracting types with GetQueryOptions<typeof _db>; mention that `_db` is only
used for the "sliced models" test while the "sliced operations" test uses a
different inline type that retains 'update'. This comment should be placed next
to the `_db` object and reference includedOperations, excludedOperations, and
GetQueryOptions so future readers understand the test intent.
In `@packages/orm/src/client/constants.ts`:
- Around line 1-4: Remove the stale commented-out export block for
CONTEXT_COMMENT_PREFIX in constants.ts: delete the three commented lines that
define/export CONTEXT_COMMENT_PREFIX (the comment and the export statement) so
the module contains only active constants and no dead commented code.
In `@packages/orm/src/client/crud/operations/create.ts`:
- Line 38: The cast "as any" used when passing getIdValues(this.schema,
this.model, createResult) into the strongly-typed WhereUniqueInput should be
replaced by a small typed helper that validates/constructs a proper
WhereUniqueInput: add a function (e.g., buildWhereUniqueFromIdValues(schema,
model, idValues): WhereUniqueInput<ModelType>) which accepts the schema, model
and getIdValues output, verifies the required unique keys are present (or maps
id fields to the model's unique field names) and returns a correctly-typed
object; then replace the inline casts in create.ts (and the other occurrences
around the TODO) to call buildWhereUniqueFromIdValues(this.schema, this.model,
createResult) so the compiler sees a proper WhereUniqueInput and you no longer
need "as any" (alternatively, if runtime validation is undesirable, add a
narrowly-scoped typed wrapper function that documents and asserts the guarantee
that getIdValues returns the required shape and returns it as WhereUniqueInput).
In `@packages/orm/src/client/crud/validator/validator.ts`:
- Around line 185-266: validateProcedureInput currently returns the original
input (losing Zod transformations); update validateProcedureInput to return the
validated/transformed data from each schema.safeParse call (use parsed.data)
instead of the raw value: for procedures with params, build a new args object by
iterating params, using parsed.data for each param (and preserving
optional/defaulted values), then return the envelope with its args replaced by
this validated args object (or return the args object directly if the function
should return only args); also handle the params.length === 0 and missing-args
branches consistently (return transformed envelope/args when present).
Reference: validateProcedureInput, zodFactory.makeProcedureParamSchema,
schema.safeParse, parsed.data.
In `@packages/schema/test/accessor.test.ts`:
- Around line 5-117: Add tests to exercise SchemaAccessor.getField and
SchemaAccessor.requireField (call them for an existing model like 'User' and
assert returned field objects and that requireField throws InvalidSchemaError
with the expected message for a missing field), and add tests for
SchemaAccessor.getDelegateDiscriminator (verify it returns the expected
discriminator for a model that uses delegation and returns undefined for models
without one); use existing schema/models (e.g., 'User' and 'Post') and reference
the exact method names getField, requireField, and getDelegateDiscriminator so
the assertions cover both success and failure paths.
In `@packages/zod/src/error.ts`:
- Around line 4-7: The SchemaFactoryError class constructor should set the error
name for clearer identification: inside the constructor of SchemaFactoryError,
assign this.name = 'SchemaFactoryError' (and optionally call
Error.captureStackTrace(this, SchemaFactoryError) for V8 stack traces) so stack
traces and error checks reflect the correct custom error type.
In `@tests/e2e/orm/client-api/slicing.test.ts`:
- Around line 162-167: Update the test so its description matches what it
actually asserts and remove the stale commented config: change the it(...) title
from 'prevents excluded models from being used in include clause' to something
like 'prevents non-included models from being used in include clause', and
delete the commented-out excludedModels line from the options object (the test
uses slicing.includedModels: ['User','Post'] in the options variable). Ensure
any assertions or variable names referring to excludedModels are updated to
reflect "non-included" semantics so the test and its setup are consistent.
In `@tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts`:
- Line 698: Remove the leftover debug log by deleting the stray
console.log('RUNNING!!!!') statement found in the entity-mutation-hooks.test E2E
test; simply remove that line (or replace with a proper test assertion/log
helper if needed) so CI output is not cluttered by debug noise.
In `@tests/regression/test/issue-2344.test.ts`:
- Around line 4-43: The test creates a DB client via createTestClient and never
disconnects, causing resource leaks; add a cleanup that calls db.disconnect()
after the test (use afterEach or equivalent test teardown) so the client created
in the test's scope is closed; locate the db variable returned by
createTestClient in this file (tests/regression/test/issue-2344.test.ts) and
ensure an afterEach(async () => { await db.disconnect(); }) or equivalent is
added and imported if necessary.
In `@tests/regression/test/issue-2375.test.ts`:
- Around line 74-76: Capture the output of db.testRunStepResults.findMany into a
variable (e.g., results) and add assertions that verify the ordering by
step.order then sharedStepItem.order: map the results to the ordering keys
(result.step.order and result.sharedStepItem?.order) and assert the sequence is
non-decreasing (or matches an expected array of orders) so the test explicitly
validates the sort applied by findMany.
In `@tests/regression/test/issue-2378.test.ts`:
- Around line 4-103: The test opens a Prisma client via createTestClient but
never disconnects it; add proper cleanup by importing afterEach from vitest and
calling await db.$disconnect() in an afterEach hook (guarded if db is defined)
so the client created by createTestClient is always disconnected after the test;
if you prefer, declare let db at the test scope, assign it from createTestClient
inside the it block, and ensure the afterEach calls await db.$disconnect() to
prevent resource leaks.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (141)
CLAUDE.mdpackage.jsonpackages/auth-adapters/better-auth/package.jsonpackages/auth-adapters/better-auth/src/adapter.tspackages/cli/package.jsonpackages/cli/src/actions/action-utils.tspackages/cli/src/actions/generate.tspackages/cli/src/actions/proxy.tspackages/cli/src/index.tspackages/cli/src/plugins/typescript.tspackages/cli/src/utils/version-utils.tspackages/cli/test/generate.test.tspackages/clients/client-helpers/package.jsonpackages/clients/tanstack-query/package.jsonpackages/clients/tanstack-query/src/common/types.tspackages/clients/tanstack-query/src/react.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/test/react-sliced-client.test-d.tspackages/clients/tanstack-query/test/react-typing-test.tspackages/clients/tanstack-query/test/react-typing.test-d.tspackages/clients/tanstack-query/test/schemas/basic/schema-lite.tspackages/clients/tanstack-query/test/schemas/procedures/schema-lite.tspackages/clients/tanstack-query/test/svelte-sliced-client.test-d.tspackages/clients/tanstack-query/test/vue-sliced-client.test-d.tspackages/clients/tanstack-query/vitest.config.tspackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/ide/vscode/package.jsonpackages/language/package.jsonpackages/orm/package.jsonpackages/orm/src/client/client-impl.tspackages/orm/src/client/constants.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/dialects/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/base.tspackages/orm/src/client/crud/operations/create.tspackages/orm/src/client/crud/operations/delete.tspackages/orm/src/client/crud/operations/find.tspackages/orm/src/client/crud/operations/update.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/crud/validator/validator.tspackages/orm/src/client/executor/temp-alias-transformer.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/orm/src/client/index.tspackages/orm/src/client/options.tspackages/orm/src/client/query-utils.tspackages/orm/src/client/type-utils.tspackages/orm/src/client/zod/cache-decorator.tspackages/orm/src/client/zod/factory.tspackages/orm/src/client/zod/index.tspackages/plugins/policy/package.jsonpackages/plugins/policy/src/expression-transformer.tspackages/plugins/policy/src/policy-handler.tspackages/schema/package.jsonpackages/schema/src/accessor.tspackages/schema/src/index.tspackages/schema/src/schema.tspackages/schema/test/accessor.test.tspackages/schema/test/schema/schema.tspackages/schema/test/schema/schema.zmodelpackages/schema/vitest.config.tspackages/sdk/package.jsonpackages/sdk/src/prisma/prisma-schema-generator.tspackages/sdk/src/ts-schema-generator.tspackages/server/package.jsonpackages/server/src/api/rpc/index.tspackages/server/test/api/rpc.test.tspackages/testtools/package.jsonpackages/testtools/src/project.tspackages/zod/package.jsonpackages/zod/src/error.tspackages/zod/src/factory.tspackages/zod/src/index.tspackages/zod/src/types.tspackages/zod/src/utils.tspackages/zod/test/factory.test.tspackages/zod/test/schema/schema.tspackages/zod/test/schema/schema.zmodelpackages/zod/vitest.config.tspnpm-workspace.yamlsamples/next.js/package.jsonsamples/next.js/zenstack/schema-lite.tssamples/next.js/zenstack/schema.tssamples/nuxt/package.jsonsamples/nuxt/zenstack/schema-lite.tssamples/nuxt/zenstack/schema.tssamples/orm/package.jsonsamples/orm/zenstack/schema.tssamples/sveltekit/package.jsonsamples/sveltekit/src/zenstack/schema-lite.tssamples/sveltekit/src/zenstack/schema.tsscripts/test-generate.tstests/e2e/apps/rally/zenstack/schema.tstests/e2e/github-repos/cal.com/schema.tstests/e2e/github-repos/formbricks/schema.tstests/e2e/github-repos/trigger.dev/schema.tstests/e2e/orm/client-api/create-many.test.tstests/e2e/orm/client-api/find.test.tstests/e2e/orm/client-api/slicing.test.tstests/e2e/orm/client-api/zod.test-d.tstests/e2e/orm/client-api/zod.test.tstests/e2e/orm/plugin-infra/entity-mutation-hooks.test.tstests/e2e/orm/plugin-infra/ext-query-args/schema.tstests/e2e/orm/schemas/auth-type/schema.tstests/e2e/orm/schemas/basic/schema.tstests/e2e/orm/schemas/basic/schema.zmodeltests/e2e/orm/schemas/default-auth/schema.tstests/e2e/orm/schemas/delegate/schema.tstests/e2e/orm/schemas/json/schema.tstests/e2e/orm/schemas/name-mapping/schema.tstests/e2e/orm/schemas/omit/schema.tstests/e2e/orm/schemas/petstore/schema.tstests/e2e/orm/schemas/procedures/schema.tstests/e2e/orm/schemas/todo/schema.tstests/e2e/orm/schemas/typed-json/schema.tstests/e2e/orm/schemas/typing/schema.tstests/e2e/package.jsontests/e2e/vitest.config.tstests/regression/package.jsontests/regression/test/issue-204/schema.tstests/regression/test/issue-2344.test.tstests/regression/test/issue-2375.test.tstests/regression/test/issue-2376.test.tstests/regression/test/issue-2378.test.tstests/regression/test/issue-2385.test.tstests/regression/test/issue-422/schema.tstests/regression/test/issue-503/schema.tstests/runtimes/bun/package.jsontests/runtimes/bun/schemas/schema.tstests/runtimes/edge-runtime/package.jsontests/runtimes/edge-runtime/schemas/schema.tsturbo.json
💤 Files with no reviewable changes (2)
- packages/clients/tanstack-query/test/react-typing-test.ts
- tests/e2e/orm/client-api/create-many.test.ts
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
|
Claude finished @ymc9's task —— View job Claude finished @ymc9's task —— View job Code Review for v3.4.0 ReleaseI've completed a comprehensive review of this large release PR. Here's my analysis: Review Tasks
🔧 Code Quality & ArchitecturePositive Changes
Areas for Improvement
🚨 Security ConcernsHigh Priority
Medium Priority
⚡ Performance ConsiderationsOptimizations
Concerns
🧪 Test CoverageWell Tested
Missing Coverage
🔄 New Features AnalysisMajor Additions
Breaking Changes
📋 Specific RecommendationsSecurity Fixes Needed// packages/cli/src/actions/proxy.ts:107-122
function evaluateUrl(schemaUrl: ConfigExpr) {
if (isInvocationExpr(schemaUrl)) {
const envFunction = schemaUrl as InvocationExpr;
const envName = getStringLiteral(envFunction.args[0]?.value as LiteralExpr)!;
// ADD: Validate environment variable name
if (!/^[A-Z_][A-Z0-9_]*$/.test(envName)) {
throw new CliError(`Invalid environment variable name: ${envName}`);
}
const envValue = process.env[envName];
// ... rest of function
}
}Code Quality Improvements
Performance Optimizations
✅ Overall AssessmentThis is a substantial and well-architected release that significantly improves the developer experience. The type system enhancements, client slicing, and RPC functionality are excellent additions. Primary concerns: Address the security issue with environment variable validation and consider the TypeScript compilation performance impact of complex types. Recommendation: ✅ Approve after addressing the security concern above The codebase shows excellent engineering practices with proper error handling, extensive testing, and thoughtful architectural decisions. |
|
Claude finished @ymc9's task —— View job Code Review for v3.4.0 Release I've completed a comprehensive review of this large release PR. Here's my analysis: Review Tasks
🔧 Code Quality & ArchitecturePositive Changes
Areas for Improvement
🚨 Security ConcernsHigh Priority
Medium Priority
⚡ Performance ConsiderationsOptimizations
Concerns
🧪 Test CoverageWell Tested
Missing Coverage
🔄 New Features AnalysisMajor Additions
Breaking Changes
📋 Specific RecommendationsSecurity Fixes Needed// packages/cli/src/actions/proxy.ts:107-122
function evaluateUrl(schemaUrl: ConfigExpr) {
if (isInvocationExpr(schemaUrl)) {
const envFunction = schemaUrl as InvocationExpr;
const envName = getStringLiteral(envFunction.args[0]?.value as LiteralExpr)!;
// ADD: Validate environment variable name
if (!/^[A-Z_][A-Z0-9_]*$/.test(envName)) {
throw new CliError(`Invalid environment variable name: ${envName}`);
}
const envValue = process.env[envName];
// ... rest of function
}
}Code Quality Improvements
Performance Optimizations
✅ Overall AssessmentThis is a substantial and well-architected release that significantly improves the developer experience. The type system enhancements, client slicing, and RPC functionality are excellent additions. Primary concerns: Address the security issue with environment variable validation and consider the TypeScript compilation performance impact of complex types. Recommendation: ✅ Approve after addressing the security concern above The codebase shows excellent engineering practices with proper error handling, extensive testing, and thoughtful architectural decisions. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/orm/client-api/zod.test.ts (2)
10-13: Remove redundant sqlite guard inside the already gated suite.
describeis already switched to_describe.skipwhen provider isn’t sqlite (Line 8), so the early-return block is duplicate logic.🧹 Suggested simplification
describe('Zod schema factory test', () => { - if (getTestDbProvider() !== 'sqlite') { - return; - } - let client: ClientContract<typeof schema>;🤖 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.ts` around lines 10 - 13, The suite contains a redundant runtime guard: inside the 'Zod schema factory test' describe block there is an unnecessary if (getTestDbProvider() !== 'sqlite') return; — remove that early-return block and rely on the outer describe switching to _describe.skip when the provider isn’t sqlite, leaving the describe body only with the tests and setup (references: 'Zod schema factory test', describe, getTestDbProvider).
3-3: PreferbeforeAll/afterAllfor this suite to cut test runtime.This suite creates/disconnects a full test client for every case, but most cases are pure schema-validation checks and don’t mutate DB state. Reusing one shared client per suite will significantly reduce e2e runtime.
♻️ Suggested change
-import { afterEach, beforeEach, describe as _describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, describe as _describe, expect, it } from 'vitest'; @@ - beforeEach(async () => { + beforeAll(async () => { client = await createTestClient(schema); }); - afterEach(async () => { + afterAll(async () => { await client?.$disconnect(); });Also applies to: 17-23
🤖 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.ts` at line 3, Replace per-test lifecycle hooks with suite-level hooks: change the import to include beforeAll and afterAll (add beforeAll, afterAll to the import list alongside describe/it/expect) and update the test setup/teardown code that currently uses beforeEach and afterEach to use beforeAll (to create/connect the shared test client once) and afterAll (to disconnect/cleanup once). Ensure the shared client variable used across tests is declared in the suite scope and that the teardown calls the existing disconnect/cleanup routine (the same function currently used in afterEach) so tests still tear down once at the end.
🤖 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.ts`:
- Around line 10-13: The suite contains a redundant runtime guard: inside the
'Zod schema factory test' describe block there is an unnecessary if
(getTestDbProvider() !== 'sqlite') return; — remove that early-return block and
rely on the outer describe switching to _describe.skip when the provider isn’t
sqlite, leaving the describe body only with the tests and setup (references:
'Zod schema factory test', describe, getTestDbProvider).
- Line 3: Replace per-test lifecycle hooks with suite-level hooks: change the
import to include beforeAll and afterAll (add beforeAll, afterAll to the import
list alongside describe/it/expect) and update the test setup/teardown code that
currently uses beforeEach and afterEach to use beforeAll (to create/connect the
shared test client once) and afterAll (to disconnect/cleanup once). Ensure the
shared client variable used across tests is declared in the suite scope and that
the teardown calls the existing disconnect/cleanup routine (the same function
currently used in afterEach) so tests still tear down once at the end.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/orm/client-api/slicing.test.tstests/e2e/orm/client-api/zod.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/orm/client-api/slicing.test.ts
|
Claude finished @ymc9's task —— View job Code Review for v3.4.0 ReleaseI've completed a comprehensive review of this large release PR. Here's my analysis: Review Tasks
🔧 Code Quality & ArchitecturePositive Changes
Areas for Improvement
🚨 Security ConcernsHigh Priority
Medium Priority
⚡ Performance ConsiderationsOptimizations
Concerns
🧪 Test CoverageWell Tested
Missing Coverage
🔄 New Features AnalysisMajor Additions
Breaking Changes
📋 Specific RecommendationsSecurity Fixes Needed// packages/cli/src/actions/proxy.ts:107-122
function evaluateUrl(schemaUrl: ConfigExpr) {
if (isInvocationExpr(schemaUrl)) {
const envFunction = schemaUrl as InvocationExpr;
const envName = getStringLiteral(envFunction.args[0]?.value as LiteralExpr)!;
// ADD: Validate environment variable name
if (!/^[A-Z_][A-Z0-9_]*$/.test(envName)) {
throw new CliError(`Invalid environment variable name: ${envName}`);
}
const envValue = process.env[envName];
// ... rest of function
}
}Code Quality Improvements
Performance Optimizations
✅ Overall AssessmentThis is a substantial and well-architected release that significantly improves the developer experience. The type system enhancements, client slicing, and RPC functionality are excellent additions. Primary concerns: Address the security issue with environment variable validation and consider the TypeScript compilation performance impact of complex types. Recommendation: ✅ Approve after addressing the security concern above The codebase shows excellent engineering practices with proper error handling, extensive testing, and thoughtful architectural decisions. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/zod/src/factory.ts (1)
149-155: Verify Zod 4 union behavior with mixed validation schemas.The Decimal field creates a union of three schemas with validation applied. The type casts (
as z.ZodNumber,as z.ZodString) may lose type information after validation is applied sinceaddNumberValidationandaddDecimalValidationreturnz.ZodSchema. Consider whether the casts are necessary or if the union inference is sufficient.💡 Consider removing explicit casts if union inference is sufficient
case 'Decimal': base = z.union([ - addNumberValidation(z.number(), attributes) as z.ZodNumber, - addDecimalValidation(z.string(), attributes, true) as z.ZodString, + addNumberValidation(z.number(), attributes), + addDecimalValidation(z.string(), attributes, true), addDecimalValidation(z.instanceof(Decimal), attributes, true), ]); break;🤖 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 149 - 155, The union for 'Decimal' uses explicit casts (as z.ZodNumber, as z.ZodString) after calling addNumberValidation and addDecimalValidation which both currently return z.ZodSchema, risking loss of precise types; either remove the casts and let z.union infer types (preferable) or change the signatures of addNumberValidation and addDecimalValidation to return the specific Zod types (e.g., z.ZodNumber, z.ZodString, z.ZodType<Decimal>) so the union in the Decimal case can be built without unsafe casts — update the Decimal case in the switch (and the addNumberValidation/addDecimalValidation function signatures/returns) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/zod/src/factory.ts`:
- Around line 149-155: The union for 'Decimal' uses explicit casts (as
z.ZodNumber, as z.ZodString) after calling addNumberValidation and
addDecimalValidation which both currently return z.ZodSchema, risking loss of
precise types; either remove the casts and let z.union infer types (preferable)
or change the signatures of addNumberValidation and addDecimalValidation to
return the specific Zod types (e.g., z.ZodNumber, z.ZodString,
z.ZodType<Decimal>) so the union in the Decimal case can be built without unsafe
casts — update the Decimal case in the switch (and the
addNumberValidation/addDecimalValidation function signatures/returns)
accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/zod/src/factory.tspackages/zod/src/types.tspackages/zod/test/factory.test.tspackages/zod/test/schema/schema.tspackages/zod/test/schema/schema.zmodelpackages/zod/vitest.config.ts
…riminator fields (#2419)
|
Claude finished @ymc9's task —— View job Code Review for v3.4.0 ReleaseI've completed a comprehensive review of this large release PR. Here's my analysis: Review Tasks
🔧 Code Quality & ArchitecturePositive Changes
Areas for Improvement
🚨 Security ConcernsHigh Priority
Medium Priority
⚡ Performance ConsiderationsOptimizations
Concerns
🧪 Test CoverageWell Tested
Missing Coverage
🔄 New Features AnalysisMajor Additions
Breaking Changes
📋 Specific RecommendationsSecurity Fixes Needed// packages/cli/src/actions/proxy.ts:107-122
function evaluateUrl(schemaUrl: ConfigExpr) {
if (isInvocationExpr(schemaUrl)) {
const envFunction = schemaUrl as InvocationExpr;
const envName = getStringLiteral(envFunction.args[0]?.value as LiteralExpr)!;
// ADD: Validate environment variable name
if (!/^[A-Z_][A-Z0-9_]*$/.test(envName)) {
throw new CliError(`Invalid environment variable name: ${envName}`);
}
const envValue = process.env[envName];
// ... rest of function
}
}Code Quality Improvements
Performance Optimizations
✅ Overall AssessmentThis is a substantial and well-architected release that significantly improves the developer experience. The type system enhancements, client slicing, and RPC functionality are excellent additions. Primary concerns: Address the security issue with environment variable validation and consider the TypeScript compilation performance impact of complex types. Recommendation: ✅ Approve after addressing the security concern above The codebase shows excellent engineering practices with proper error handling, extensive testing, and thoughtful architectural decisions. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/orm/src/client/zod/factory.ts (1)
323-327:⚠️ Potential issue | 🟡 MinorFix Biome lint:
forEachcallback should not return a value.The arrow function uses an expression body that implicitly returns the result of
ctx.addIssue(). Switch to a block body.Proposed fix
const finalSchema = z.any().superRefine((value, ctx) => { const parseResult = schema.safeParse(value); if (!parseResult.success) { - parseResult.error.issues.forEach((issue) => ctx.addIssue(issue as any)); + parseResult.error.issues.forEach((issue) => { + ctx.addIssue(issue as any); + }); } });🤖 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 323 - 327, The forEach callback used after schema.safeParse is an arrow expression that implicitly returns ctx.addIssue, triggering the Biome lint; update the callback in the block that iterates parseResult.error.issues to use a block body (e.g., (issue) => { ctx.addIssue(issue as any); }) so it does not return a value—locate the parseResult variable and the forEach call where schema.safeParse(value) is used and change the arrow function accordingly.
🧹 Nitpick comments (3)
tests/e2e/orm/client-api/delegate.test.ts (1)
652-659: Use a valid discriminator literal to isolate immutability behavior.At
Line 657,videoType: 'MyVideo'may fail due to invalid literal/value validation, which can mask whether the “discriminator cannot be updated” rule is enforced.♻️ Suggested tweak
await expect( client.ratedVideo.update({ where: { id: 2 }, // `@ts-expect-error` - data: { videoType: 'MyVideo' }, + data: { videoType: 'RatedVideo' }, }), ).toBeRejectedByValidation(['videoType']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/delegate.test.ts` around lines 652 - 659, The test uses an invalid discriminator value ('MyVideo') which can trigger literal validation instead of the immutability rule; update the assertion to use a valid discriminator literal that's part of the model's allowed values (referencing client.ratedVideo.update and the videoType discriminator) so the failure reflects "discriminator cannot be updated" rather than an invalid-value validation error—replace the invalid 'MyVideo' with an actual enum/literal used by the RatedVideo/Video model and keep the same toBeRejectedByValidation(['videoType']) expectation.packages/orm/src/client/zod/factory.ts (2)
144-162: Debug utilities with@ts-ignoremay be unintended artifacts.The
getCache,setCache, andprintCacheStatsmethods are decorated with@ts-ignoreand appear to be internal debugging utilities.printCacheStatsusesconsole.logwhich could leak to production.If these are intentional debugging hooks, consider:
- Adding documentation explaining their purpose
- Guarding
console.logbehind a debug flagIf they're artifacts, consider removing them.
🤖 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 144 - 162, The three debug helper methods getCache, setCache, and printCacheStats are annotated with `@ts-ignore` and use console.log (printCacheStats) which looks like an unintended debug artifact; either remove them or make them explicit debug hooks: delete getCache/setCache/printCacheStats if not needed, or remove the `@ts-ignore` annotations, add JSDoc comments describing their purpose, guard printCacheStats behind a runtime debug flag (e.g., an internal boolean) or route logs through the library logger instead of console, and ensure schemaCache (the underlying Map) access remains encapsulated when you change these methods.
2128-2137: Consider using Set for O(n+m) filter kind exclusion.The current implementation creates a new array on each excluded kind, resulting in O(n*m) complexity. While the arrays are typically small, a Set-based approach would be more efficient.
Optional optimization
if (excluded !== undefined) { if (!result) { // If no inclusion list, start with all filter kinds result = [...this.allFilterKinds]; } // Remove excluded kinds - for (const kind of excluded) { - result = result.filter((k) => k !== kind); - } + const excludedSet = new Set(excluded); + result = result.filter((k) => !excludedSet.has(k)); }🤖 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 2128 - 2137, The loop that removes excluded kinds (using excluded and result built from this.allFilterKinds) repeatedly filters the array causing O(n*m) cost; replace it with a Set-based approach: build a Set from excluded and then produce result by filtering this.allFilterKinds once against that Set (or by deleting from a Set of all kinds), so exclusion runs in O(n + m) time while keeping the same semantics for result and excluded.
🤖 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/zod/factory.ts`:
- Around line 323-327: The forEach callback used after schema.safeParse is an
arrow expression that implicitly returns ctx.addIssue, triggering the Biome
lint; update the callback in the block that iterates parseResult.error.issues to
use a block body (e.g., (issue) => { ctx.addIssue(issue as any); }) so it does
not return a value—locate the parseResult variable and the forEach call where
schema.safeParse(value) is used and change the arrow function accordingly.
---
Nitpick comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 144-162: The three debug helper methods getCache, setCache, and
printCacheStats are annotated with `@ts-ignore` and use console.log
(printCacheStats) which looks like an unintended debug artifact; either remove
them or make them explicit debug hooks: delete getCache/setCache/printCacheStats
if not needed, or remove the `@ts-ignore` annotations, add JSDoc comments
describing their purpose, guard printCacheStats behind a runtime debug flag
(e.g., an internal boolean) or route logs through the library logger instead of
console, and ensure schemaCache (the underlying Map) access remains encapsulated
when you change these methods.
- Around line 2128-2137: The loop that removes excluded kinds (using excluded
and result built from this.allFilterKinds) repeatedly filters the array causing
O(n*m) cost; replace it with a Set-based approach: build a Set from excluded and
then produce result by filtering this.allFilterKinds once against that Set (or
by deleting from a Set of all kinds), so exclusion runs in O(n + m) time while
keeping the same semantics for result and excluded.
In `@tests/e2e/orm/client-api/delegate.test.ts`:
- Around line 652-659: The test uses an invalid discriminator value ('MyVideo')
which can trigger literal validation instead of the immutability rule; update
the assertion to use a valid discriminator literal that's part of the model's
allowed values (referencing client.ratedVideo.update and the videoType
discriminator) so the failure reflects "discriminator cannot be updated" rather
than an invalid-value validation error—replace the invalid 'MyVideo' with an
actual enum/literal used by the RatedVideo/Video model and keep the same
toBeRejectedByValidation(['videoType']) expectation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/orm/src/client/zod/factory.tstests/e2e/orm/client-api/computed-fields.test.tstests/e2e/orm/client-api/delegate.test.ts
Summary by CodeRabbit
New Features
Bug Fixes
Tests