merge dev to main (v3.0.0-beta.20) #382
Conversation
* refactor: revised error system * addressing PR comments
* fix(lsp): get language server on par with v2 * fix typo * addressing PR comments
* fix(orm): update to kysely 0.28.x and simplify typing * fix deprecation warning * fix `orderBy` deprecation * fix tests
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
WalkthroughThis PR implements a comprehensive refactor of the error handling system, replacing individual error classes with a unified ORMError architecture using factory functions. It introduces new language server providers (formatters, definition, completion, semantic tokens), simplifies type signatures by replacing GetModels with string identifiers, refactors QueryId handling from strings to typed objects, and updates dependency versions. Documentation is also updated with improved setup instructions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 Code Review for ZenStack v3.0.0-beta.20I've completed a comprehensive review of this substantial release merge covering 84 files with significant changes to the ORM, language server, and error handling systems. Review Tasks
🚀 Overall Assessment: STRONGThis is a well-architected release with significant improvements across the codebase. The transition from Prisma runtime dependency to a pure Kysely-based implementation shows careful engineering. ✅ StrengthsArchitecture & Design
Error Handling System (
|
There was a problem hiding this comment.
Pull Request Overview
This PR represents a version bump from 3.0.0-beta.19 to 3.0.0-beta.20 with several major improvements:
- Refactored error handling system from multiple error classes to a unified
ORMErrorwith reason codes for better type safety and consistency - Updated dependencies: kysely from 0.27.6 to 0.28.8, @types/node from 20.17.24 to 20.19.24
- Added comprehensive LSP features to the language package (formatting, semantic tokens, code actions, etc.)
- Cleaned up debug console.log statements from test files
- Enhanced query execution with proper QueryId handling from Kysely
Reviewed Changes
Copilot reviewed 83 out of 84 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/orm/src/client/errors.ts | Complete refactoring of error system from multiple classes to unified ORMError with reason enum |
| packages/testtools/src/vitest-ext.ts | Updated test matchers to work with new ORMError system |
| packages/orm/src/client/executor/zenstack-query-executor.ts | Fixed QueryId handling to use Kysely's createQueryId |
| packages/orm/src/client/plugin.ts | Updated plugin API to use QueryId type from Kysely |
| packages/language/src/index.ts | Added ZModelCodeGenerator to public exports |
| packages/sdk/src/index.ts | Removed ZModelCodeGenerator export (moved to language package) |
| pnpm-workspace.yaml | Updated kysely to ~0.28.8 and @types/node to ^20.19.0 |
| Multiple test files | Removed debug console.log statements and updated error assertions |
| packages/language/src/* | Added new LSP features: formatter, semantic tokens, code actions, completion provider, etc. |
| README.md | Minor documentation improvements |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/base.ts (1)
1238-1319: Fix updateMany return shape when base-only fields changeWhen
updateManyis invoked withreturnData = trueand all updated columns live on a delegate base model,updateFieldsbecomes empty at Line 1260. We then returnresultFromBaseModel, but that value comes fromprocessBaseModelUpdateMany, which still callsthis.updateMany(..., false, ...). The result is{ count: ... }, not an array of rows, so callers expecting a list blow up at runtime. Please propagate the caller’sreturnDataflag through the base-model path so we only return arrays whenreturnDatais true.@@ - const baseResult = await this.processBaseModelUpdateMany( + const baseResult = await this.processBaseModelUpdateMany( kysely, modelDef.baseModel, where, updateFields, - filterModel, + filterModel, + returnData, ); @@ - private async processBaseModelUpdateMany( + private async processBaseModelUpdateMany( kysely: AnyKysely, model: string, where: any, updateFields: any, - filterModel: string, + filterModel: string, + returnData: boolean, ) { @@ - const baseResult: any = await this.updateMany( + const baseResult: any = await this.updateMany( kysely, model as GetModels<Schema>, where, thisUpdateFields, undefined, - false, - filterModel, + returnData, + filterModel, );
🧹 Nitpick comments (10)
packages/language/tsconfig.json (1)
4-6: Consider addressing unused code instead of disabling the compiler check.Setting
noUnusedLocals: falsesuppresses warnings about unused local variables rather than resolving them. In a comprehensive refactor adding new language server providers and expanding the public API, this could mask dead code, debugging artifacts, or incomplete cleanup.Either:
- Enable the check and refactor unused variables in the new code, or
- Document why this exception is necessary during the beta phase and plan to re-enable it before release.
This maintains better code hygiene and prevents technical debt from accumulating in critical tooling layers.
tests/regression/test/issue-204/regression.test.ts (1)
10-11: Optional: Consider if the no-op function is necessary.The
checkfunction serves to maintain the test structure and ensure property access is executed. If these values don't need to be accessed at all, you could remove both the function and its invocations. However, keeping them ensures the code path is exercised for type checking.tests/e2e/orm/plugin-infra/on-kysely-query.test.ts (1)
214-214: Error assertion correctly updated for ORMError wrapping.The change from
.toThrow('test error')to checking.cause.messageproperly validates that errors thrown in interceptors are wrapped and propagated through transactions.Consider verifying the error type for better type safety:
- ).rejects.toSatisfy((e) => (e as any).cause.message === 'test error'); + ).rejects.toSatisfy((e) => e instanceof Error && (e as any).cause?.message === 'test error');packages/language/src/zmodel-comment-provider.ts (1)
6-20: Consider edge cases for container names.The implementation uses optional chaining for
$container?.name(lines 14-15), which will produce comments like "Value of enum undefined" if the container is missing or has no name. Consider adding a fallback or validation.- .when(isEnumField, (f) => `/**\n * Value of enum *${f.$container?.name}*\n */`) - .when(isDataField, (f) => `/**\n * Field of *${f.$container?.name}*\n */`) + .when(isEnumField, (f) => `/**\n * Value of enum *${f.$container?.name ?? 'Unknown'}*\n */`) + .when(isDataField, (f) => `/**\n * Field of *${f.$container?.name ?? 'Unknown'}*\n */`)packages/orm/src/client/errors.ts (1)
66-74: SetnameforORMErrorto preserve diagnostic clarity.Without explicitly setting
this.name, logged errors surface as plainError, losing the new type’s identity and regressing prior class-based diagnostics. Addthis.name = 'ORMError';aftersuper(...)so logs, telemetry, and callers inspectingerror.namekeep the expected signal.) { super(message, options); + this.name = 'ORMError'; }packages/orm/src/client/crud/validator/index.ts (1)
476-476: Consider passing the model parameter for consistency.While the error message includes the model name, the
error.modelproperty won't be set. For consistency with other error factory usages in this file and to enable programmatic error handling, consider passing the model parameter.Apply this diff:
- throw createInternalError(`Model "${model}" has no unique fields`); + throw createInternalError(`Model "${model}" has no unique fields`, model);packages/plugins/policy/src/utils.ts (1)
162-175: Consider importing error factories instead of re-implementing them.These functions duplicate the error factory implementations from
@zenstackhq/orm/client/errors.ts(createRejectedByPolicyErrorandcreateNotSupportedError). Unless there's a circular dependency concern, consider importing and re-exporting the existing factories to reduce code duplication.Example:
-export function createRejectedByPolicyError( - model: string | undefined, - reason: RejectedByPolicyReason, - message?: string, -) { - const err = new ORMError(ORMErrorReason.REJECTED_BY_POLICY, message ?? 'operation is rejected by access policies'); - err.rejectedByPolicyReason = reason; - err.model = model; - return err; -} - -export function createUnsupportedError(message: string) { - return new ORMError(ORMErrorReason.NOT_SUPPORTED, message); -} +export { createRejectedByPolicyError, createNotSupportedError as createUnsupportedError } from '@zenstackhq/orm';tests/e2e/orm/policy/crud/update.test.ts (2)
760-761: Add defensive null check for error cause.The assertion accesses
e.cause.messagewithout verifying thatcauseexists. If the error structure changes or an unexpected error is thrown, this could fail with a confusing error message.Consider adding a null check:
-).rejects.toSatisfy((e) => e.cause.message.toLowerCase().includes('constraint')); +).rejects.toSatisfy((e) => e.cause?.message?.toLowerCase()?.includes('constraint'));
1126-1127: Add defensive null check for error cause.Same issue as line 761 - the assertion accesses
e.cause.messagewithout verifying thatcauseexists.Apply the same fix:
-).rejects.toSatisfy((e) => e.cause.message.toLowerCase().includes('constraint')); +).rejects.toSatisfy((e) => e.cause?.message?.toLowerCase()?.includes('constraint'));packages/language/src/zmodel-documentation-provider.ts (1)
9-11: Consider making the regex more specific for triple-slash comments.The current regex
/^[/]*\s*/will strip any number of leading slashes, which means it will match//,///,////, etc. If the intent is specifically to support triple-slash comments, the regex could be more precise.Consider using a more specific pattern:
-return node.comments.map((c: string) => c.replace(/^[/]*\s*/, '')).join('\n'); +return node.comments.map((c: string) => c.replace(/^\/\/\/?\s*/, '')).join('\n');This pattern matches
///(triple-slash) or//(double-slash) followed by optional whitespace, which is more explicit about the supported comment formats.
📜 Review details
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 (83)
README.md(3 hunks)package.json(1 hunks)packages/cli/package.json(1 hunks)packages/clients/tanstack-query/package.json(1 hunks)packages/common-helpers/package.json(1 hunks)packages/config/eslint-config/package.json(1 hunks)packages/config/typescript-config/package.json(1 hunks)packages/config/vitest-config/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/ide/vscode/package.json(1 hunks)packages/language/package.json(2 hunks)packages/language/res/stdlib.zmodel(1 hunks)packages/language/src/index.ts(1 hunks)packages/language/src/module.ts(3 hunks)packages/language/src/validators/attribute-application-validator.ts(0 hunks)packages/language/src/validators/function-invocation-validator.ts(0 hunks)packages/language/src/zmodel-code-action.ts(1 hunks)packages/language/src/zmodel-code-generator.ts(1 hunks)packages/language/src/zmodel-comment-provider.ts(1 hunks)packages/language/src/zmodel-completion-provider.ts(1 hunks)packages/language/src/zmodel-definition.ts(1 hunks)packages/language/src/zmodel-documentation-provider.ts(1 hunks)packages/language/src/zmodel-formatter.ts(1 hunks)packages/language/src/zmodel-semantic.ts(1 hunks)packages/language/tsconfig.json(1 hunks)packages/orm/package.json(1 hunks)packages/orm/src/client/client-impl.ts(7 hunks)packages/orm/src/client/contract.ts(2 hunks)packages/orm/src/client/crud/dialects/base-dialect.ts(12 hunks)packages/orm/src/client/crud/dialects/postgresql.ts(2 hunks)packages/orm/src/client/crud/dialects/sqlite.ts(3 hunks)packages/orm/src/client/crud/operations/base.ts(54 hunks)packages/orm/src/client/crud/operations/create.ts(2 hunks)packages/orm/src/client/crud/operations/delete.ts(2 hunks)packages/orm/src/client/crud/operations/group-by.ts(1 hunks)packages/orm/src/client/crud/operations/update.ts(4 hunks)packages/orm/src/client/crud/validator/index.ts(3 hunks)packages/orm/src/client/crud/validator/utils.ts(2 hunks)packages/orm/src/client/errors.ts(2 hunks)packages/orm/src/client/executor/error-processor.ts(1 hunks)packages/orm/src/client/executor/zenstack-query-executor.ts(12 hunks)packages/orm/src/client/index.ts(1 hunks)packages/orm/src/client/plugin.ts(2 hunks)packages/orm/src/client/query-utils.ts(9 hunks)packages/orm/src/utils/kysely-utils.ts(5 hunks)packages/plugins/policy/package.json(1 hunks)packages/plugins/policy/src/expression-transformer.ts(20 hunks)packages/plugins/policy/src/policy-handler.ts(21 hunks)packages/plugins/policy/src/utils.ts(2 hunks)packages/schema/package.json(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/index.ts(0 hunks)packages/sdk/src/prisma/prisma-schema-generator.ts(2 hunks)packages/server/package.json(1 hunks)packages/server/src/api/rest/index.ts(4 hunks)packages/server/src/api/rpc/index.ts(3 hunks)packages/server/test/api/rest.test.ts(0 hunks)packages/testtools/package.json(1 hunks)packages/testtools/src/vitest-ext.ts(4 hunks)packages/zod/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)samples/next.js/package.json(2 hunks)samples/orm/package.json(1 hunks)tests/e2e/orm/client-api/delegate.test.ts(1 hunks)tests/e2e/orm/client-api/error-handling.test.ts(1 hunks)tests/e2e/orm/client-api/find.test.ts(4 hunks)tests/e2e/orm/client-api/mixin.test.ts(1 hunks)tests/e2e/orm/client-api/name-mapping.test.ts(1 hunks)tests/e2e/orm/client-api/update.test.ts(1 hunks)tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts(1 hunks)tests/e2e/orm/plugin-infra/on-kysely-query.test.ts(1 hunks)tests/e2e/orm/policy/crud/update.test.ts(2 hunks)tests/e2e/orm/policy/migrated/auth.test.ts(1 hunks)tests/e2e/orm/policy/migrated/deep-nested.test.ts(1 hunks)tests/e2e/orm/policy/migrated/multi-field-unique.test.ts(3 hunks)tests/e2e/orm/policy/migrated/todo-sample.test.ts(0 hunks)tests/e2e/orm/validation/toplevel.test.ts(0 hunks)tests/e2e/package.json(1 hunks)tests/regression/package.json(1 hunks)tests/regression/test/issue-204/regression.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1271.test.ts(0 hunks)tests/regression/test/v2-migrated/issue-1698.test.ts(1 hunks)
💤 Files with no reviewable changes (7)
- tests/regression/test/v2-migrated/issue-1271.test.ts
- packages/sdk/src/index.ts
- packages/language/src/validators/function-invocation-validator.ts
- tests/e2e/orm/validation/toplevel.test.ts
- packages/server/test/api/rest.test.ts
- packages/language/src/validators/attribute-application-validator.ts
- tests/e2e/orm/policy/migrated/todo-sample.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T16:04:56.292Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/crud/dialects/base-dialect.ts:745-747
Timestamp: 2025-10-21T16:04:56.292Z
Learning: In packages/runtime/src/client/crud/dialects/base-dialect.ts, it's intentional that buildCursorFilter applies default ordering (via makeDefaultOrderBy fallback) while buildOrderBy does not. This ensures cursor-based pagination always has stable ordering for correctness, while regular queries remain unordered unless explicitly specified. This design is to be consistent with Prisma's pagination requirements.
Applied to files:
tests/e2e/orm/client-api/name-mapping.test.tspackages/orm/src/client/crud/dialects/base-dialect.ts
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
packages/orm/src/client/query-utils.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/executor/zenstack-query-executor.ts
🧬 Code graph analysis (31)
packages/orm/src/client/executor/error-processor.ts (1)
packages/orm/src/client/executor/zenstack-driver.ts (1)
error(172-179)
tests/e2e/orm/client-api/error-handling.test.ts (2)
packages/testtools/src/client.ts (2)
createTestClient(52-169)createPolicyTestClient(179-190)packages/orm/src/client/errors.ts (1)
ORMError(66-104)
packages/orm/src/client/crud/validator/utils.ts (1)
packages/orm/src/client/errors.ts (1)
createNotSupportedError(143-145)
packages/language/src/zmodel-semantic.ts (1)
packages/language/src/generated/ast.ts (19)
isDataModel(391-393)isDataSource(417-419)isGeneratorDecl(516-518)isPlugin(628-630)isTypeDef(737-739)isDataField(344-346)isConfigField(303-305)isAttributeArg(221-223)isPluginField(641-643)isEnumField(446-448)isDataFieldType(373-375)isDataModelAttribute(404-406)isDataFieldAttribute(357-359)isInternalAttribute(529-531)isInvocationExpr(542-544)isFunctionDecl(475-477)isAttribute(208-210)isReferenceExpr(697-699)isMemberAccessExpr(555-557)
packages/orm/src/client/query-utils.ts (2)
packages/orm/src/client/errors.ts (1)
createInternalError(147-151)packages/orm/src/client/crud/operations/base.ts (2)
schema(98-100)requireField(129-131)
tests/e2e/orm/policy/migrated/multi-field-unique.test.ts (2)
packages/orm/src/client/errors.ts (1)
ORMError(66-104)packages/orm/src/client/index.ts (2)
ORMError(6-6)ORMErrorReason(6-6)
packages/orm/src/client/crud/validator/index.ts (2)
packages/orm/src/client/errors.ts (2)
createInvalidInputError(116-120)createInternalError(147-151)packages/orm/src/utils/zod-utils.ts (1)
formatError(7-9)
packages/orm/src/client/crud/dialects/base-dialect.ts (1)
packages/orm/src/client/errors.ts (3)
createNotSupportedError(143-145)createInvalidInputError(116-120)createConfigError(106-108)
packages/orm/src/client/crud/dialects/sqlite.ts (1)
packages/orm/src/client/errors.ts (1)
createInternalError(147-151)
packages/language/src/zmodel-definition.ts (2)
packages/language/src/generated/ast.ts (1)
isModelImport(579-581)packages/language/src/utils.ts (1)
resolveImport(398-416)
packages/orm/src/client/crud/operations/update.ts (2)
packages/orm/src/client/errors.ts (1)
createRejectedByPolicyError(131-141)packages/plugins/policy/src/utils.ts (1)
createRejectedByPolicyError(162-171)
packages/orm/src/client/crud/dialects/postgresql.ts (1)
packages/orm/src/client/errors.ts (1)
createInternalError(147-151)
packages/language/src/zmodel-comment-provider.ts (1)
packages/language/src/generated/ast.ts (5)
isDataModel(391-393)isTypeDef(737-739)isEnumField(446-448)isDataField(344-346)isFunctionDecl(475-477)
packages/language/src/zmodel-formatter.ts (1)
packages/language/src/module.ts (1)
ZModelLanguageMetaData(27-27)
packages/language/src/zmodel-completion-provider.ts (3)
packages/language/src/generated/ast.ts (11)
isDataModelAttribute(404-406)isDataFieldAttribute(357-359)isArrayExpr(193-195)isMemberAccessExpr(555-557)isEnumField(446-448)isInvocationExpr(542-544)isFunctionDecl(475-477)isDataField(344-346)isDataModel(391-393)isTypeDef(737-739)isAttribute(208-210)packages/language/src/utils.ts (1)
isEnumFieldReference(136-138)packages/language/src/zmodel-code-generator.ts (1)
ZModelCodeGenerator(72-389)
packages/plugins/policy/src/utils.ts (1)
packages/orm/src/client/errors.ts (2)
createRejectedByPolicyError(131-141)ORMError(66-104)
packages/orm/src/client/contract.ts (1)
packages/orm/src/utils/kysely-utils.ts (1)
AnyKysely(412-412)
packages/orm/src/client/crud/operations/delete.ts (1)
packages/orm/src/client/errors.ts (2)
createNotFoundError(110-114)createRejectedByPolicyError(131-141)
packages/orm/src/client/crud/operations/create.ts (2)
packages/orm/src/client/errors.ts (1)
createRejectedByPolicyError(131-141)packages/plugins/policy/src/utils.ts (1)
createRejectedByPolicyError(162-171)
packages/language/src/zmodel-code-action.ts (4)
packages/language/src/zmodel-formatter.ts (1)
ZModelFormatter(7-114)packages/language/src/validators/datamodel-validator.ts (1)
MissingOppositeRelationData(494-503)packages/language/src/generated/ast.ts (1)
isDataModel(391-393)packages/language/src/utils.ts (2)
getAllFields(511-536)getDocument(570-577)
packages/orm/src/client/client-impl.ts (2)
packages/orm/src/utils/kysely-utils.ts (1)
AnyKysely(412-412)packages/orm/src/client/errors.ts (2)
createConfigError(106-108)createNotFoundError(110-114)
packages/testtools/src/vitest-ext.ts (1)
packages/orm/src/client/errors.ts (1)
ORMError(66-104)
packages/orm/src/client/errors.ts (2)
packages/orm/src/client/executor/error-processor.ts (1)
getDbErrorCode(6-12)packages/plugins/policy/src/utils.ts (1)
createRejectedByPolicyError(162-171)
packages/server/src/api/rpc/index.ts (2)
packages/orm/src/client/errors.ts (1)
ORMError(66-104)packages/orm/src/client/index.ts (2)
ORMError(6-6)ORMErrorReason(6-6)
tests/regression/test/issue-204/regression.test.ts (1)
tests/regression/test/issue-204/models.ts (1)
Configuration(11-11)
packages/orm/src/client/executor/zenstack-query-executor.ts (1)
packages/orm/src/client/errors.ts (3)
ORMError(66-104)createDBQueryError(122-129)createInternalError(147-151)
packages/language/src/module.ts (6)
packages/language/src/zmodel-formatter.ts (1)
ZModelFormatter(7-114)packages/language/src/zmodel-definition.ts (1)
ZModelDefinitionProvider(7-37)packages/language/src/zmodel-completion-provider.ts (1)
ZModelCompletionProvider(35-397)packages/language/src/zmodel-semantic.ts (1)
ZModelSemanticTokenProvider(27-111)packages/language/src/zmodel-comment-provider.ts (1)
ZModelCommentProvider(5-21)packages/language/src/zmodel-documentation-provider.ts (1)
ZModelDocumentationProvider(6-15)
packages/server/src/api/rest/index.ts (1)
packages/orm/src/client/errors.ts (1)
ORMError(66-104)
packages/plugins/policy/src/expression-transformer.ts (4)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/schema/src/expression.ts (6)
ArrayExpression(17-20)FieldExpression(22-25)BinaryExpression(39-44)UnaryExpression(33-37)CallExpression(46-50)MemberExpression(27-31)packages/plugins/policy/src/utils.ts (1)
createUnsupportedError(173-175)packages/schema/src/schema.ts (2)
GetModels(110-110)FieldDef(61-76)
packages/plugins/policy/src/policy-handler.ts (4)
packages/orm/src/client/errors.ts (1)
createRejectedByPolicyError(131-141)packages/plugins/policy/src/utils.ts (3)
createRejectedByPolicyError(162-171)getTableName(144-156)createUnsupportedError(173-175)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/orm/src/client/contract.ts (2)
CRUD_EXT(230-230)CRUD_EXT(240-240)
packages/orm/src/client/crud/operations/base.ts (5)
packages/orm/src/utils/kysely-utils.ts (1)
AnyKysely(412-412)packages/orm/src/client/errors.ts (6)
createDBQueryError(122-129)createInternalError(147-151)createNotSupportedError(143-145)createNotFoundError(110-114)createInvalidInputError(116-120)ORMError(66-104)packages/schema/src/schema.ts (2)
GetModels(110-110)FieldDef(61-76)packages/orm/src/client/query-utils.ts (2)
requireIdFields(70-77)flattenCompoundUniqueFilters(266-308)packages/common-helpers/src/enumerable.ts (1)
enumerate(9-17)
🪛 Biome (2.1.2)
packages/plugins/policy/src/expression-transformer.ts
[error] 123-123: This parameter is used before its declaration.
The parameter is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 450-450: This parameter is used before its declaration.
The parameter is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (70)
tests/regression/test/v2-migrated/issue-1698.test.ts (1)
50-56: LGTM! Clean removal of unused variables.The house records from
create()calls were not being used after creation, so removing the variable assignments is appropriate. The test still validates the same behavior by querying the created records later.packages/language/res/stdlib.zmodel (2)
385-386: Documentation improvement looks good.The terminology update from "Prisma Client" to "ORM Client" aligns with the broader refactoring objectives, and the added clarification about schema migration behavior is helpful for users to understand the scope of the
@ignoreattribute.
391-392: Consistent documentation update.The changes mirror the field-level
@ignoreattribute updates, maintaining consistency across the documentation. The clarification about schema migrations applies equally well at the model level.tests/regression/test/issue-204/regression.test.ts (1)
6-8: Good practice: Removing console logging from tests.Replacing
console.logwith the no-opcheckfunction keeps tests cleaner while preserving the property access expressions for type checking and ensuring no runtime errors occur during access.tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts (1)
80-85: LGTM! QueryId access correctly updated.Both changes consistently access the
queryIdproperty from the typedQueryIdobject, aligning with the broader refactor from string identifiers to structured QueryId objects. The test correctly validates that the same query identifier is maintained throughout the mutation lifecycle.pnpm-workspace.yaml (2)
13-13: @types/node 20.19.0 is compatible with the LSP implementation.Verification confirms:
- Version exists on npm and is a standard DefinitelyTyped publish for Node.js v20.19.0
- All LSP providers (ZModelDefinitionProvider, ZModelCompletionProvider, etc.) rely on langium 3.5.0 type definitions, not direct @types/node types
- Codebase uses modern ESM imports (
node:fs,node:path,node:url), unaffected by Node.js 20.19.0's require(esm) default enablement- No breaking changes or type incompatibilities detected
6-6: Kysely 0.28.8 update is compatible and safe.No explicit breaking changes exist in kysely v0.28.0-0.28.8, and the exported types used by this codebase (OperationNode, WithNode, PrimaryKeyConstraintNode) are stable in v0.28.8. The version spec change from
^0.27.6to~0.28.8is more restrictive (patch-only updates on 0.28.x rather than allowing minor versions), which is an intentional stability measure and does not pose compatibility risks. The update is safe to proceed.packages/orm/src/client/crud/operations/group-by.ts (1)
15-15: The type assertion is correct and necessary; the original concern about inconsistency is unfounded.The cast
as stringinselectFrom(this.model)is not inconsistent. Unlike other methods in the file that acceptGetModels<Schema>union types, Kysely'sselectFrom()requires strict string typing for table names. Only group-by.ts usesselectFrom()withthis.modeldirectly; other operation handlers (count.ts, aggregate.ts) useselectFrom()with callback functions instead, so there is no pattern to verify across handlers. The cast is correct.Likely an incorrect or invalid review comment.
packages/common-helpers/package.json (1)
3-3: Version bump is correct and consistent with monorepo strategy.Line 3 correctly updates the version to 3.0.0-beta.20, aligning with the broader coordinated version bump across packages. No functional changes to exports, dependencies, or scripts.
samples/orm/package.json (1)
3-3: Version bump is correct and aligned with the release.Line 3 correctly updates the version to 3.0.0-beta.20. All dependencies and devDependencies remain unchanged, maintaining compatibility with the workspace packages.
packages/ide/vscode/package.json (1)
4-4: Version bump is correct and uses independent versioning.Line 4 correctly increments the VS Code extension version to 3.0.12, maintaining the independent semantic versioning scheme for this user-facing package while other monorepo packages use 3.0.0-beta.20. The workspace dependency reference to
@zenstackhq/languageremains correct.packages/dialects/sql.js/package.json (1)
3-3: Version bump is correct and consistent.Line 3 correctly updates the version to 3.0.0-beta.20. Dependencies and peer dependencies remain stable, maintaining compatibility with the Kysely dialect ecosystem.
package.json (1)
3-3: Version bump is correct for monorepo root.Line 3 correctly updates the root package version to 3.0.0-beta.20. All build scripts, development tooling, and monorepo configuration remain unchanged. This aligns the root version with the child packages in the coordinated release.
tests/regression/package.json (1)
3-3: Version bump is correct and maintains test integrity.Line 3 correctly updates the version to 3.0.0-beta.20. The test package's scripts and workspace dependencies remain unchanged, ensuring the regression test suite continues to work with the updated packages.
packages/cli/package.json (1)
6-6: Version bump is correct and maintains CLI functionality.Line 6 correctly updates the CLI package version to 3.0.0-beta.20. The CLI bin entries, build scripts, and all dependencies remain unchanged, ensuring seamless CLI operation with the updated version.
packages/config/eslint-config/package.json (1)
3-3: Version bump is correct for config package.Line 3 correctly updates the version to 3.0.0-beta.20, maintaining consistency across shared configuration packages in the monorepo.
packages/sdk/package.json (1)
3-3: LGTM! Standard version bump for beta.20 release.samples/next.js/package.json (2)
3-3: LGTM! Standard version bump for beta.20 release.
28-28: Good standardization with catalog reference.Migrating
@types/nodeto usecatalog:improves consistency across the monorepo by centralizing version management.packages/testtools/package.json (1)
3-3: LGTM! Standard version bump for beta.20 release.packages/plugins/policy/package.json (1)
3-3: LGTM! Standard version bump for beta.20 release.packages/create-zenstack/package.json (1)
3-3: LGTM! Standard version bump for beta.20 release.packages/zod/package.json (1)
3-3: LGTM! Standard version bump for beta.20 release.tests/e2e/package.json (1)
3-3: LGTM! Standard version bump for beta.20 release.tests/e2e/orm/client-api/mixin.test.ts (1)
78-78: The error structure verification confirms proper cause property chaining.The ORMError system correctly handles the error structure:
- ORMError constructor passes
ErrorOptionstosuper(message, options), which sets the standardError.causeproperty- Constraint violations caught during query execution invoke
createDBQueryError(message, dbError, ...), which passes{ cause: dbError }to ORMError- The test assertion
e.cause.message.toLowerCase().includes('constraint')accesses the original database error's message, which is the expected behaviorThe code is correct and no changes are needed.
packages/server/package.json (1)
3-3: LGTM - Version bump for beta release.The version update to 3.0.0-beta.20 is consistent with the PR-wide release preparation.
packages/language/src/zmodel-code-generator.ts (1)
43-44: LGTM - Import path refactor.The changes consolidate imports to use local module paths (
./astand./utils) instead of external package references. This improves module cohesion with no functional impact.packages/config/typescript-config/package.json (1)
3-3: LGTM - Version bump for beta release.The version update to 3.0.0-beta.20 aligns with the PR-wide release cycle.
tests/e2e/orm/client-api/update.test.ts (1)
1053-1053: LGTM - Updated error assertion for new error structure.The test now correctly validates errors using the new ORMError structure by checking
error.cause.messageinstead of the top-level error message. This aligns with the PR-wide migration to unified error handling.packages/clients/tanstack-query/package.json (1)
3-3: LGTM - Version bump for beta release.The version update to 3.0.0-beta.20 is consistent with the PR-wide release preparation.
tests/e2e/orm/client-api/delegate.test.ts (1)
185-185: LGTM - Updated error assertion for new error structure.The test now validates errors using the new ORMError structure by inspecting
error.cause.message, consistent with the PR-wide error handling refactor.packages/schema/package.json (1)
3-3: LGTM - Version bump for beta release.The version update to 3.0.0-beta.20 aligns with the PR-wide release cycle.
packages/language/package.json (2)
4-4: LGTM - Version bump for beta release.The version update to 3.0.0-beta.20 is consistent with the PR-wide release preparation.
61-61: LGTM - New dependency for language server support.Adding
vscode-languageserversupports the new LSP/IDE integration features introduced in this PR, including formatters, definition providers, completion providers, and semantic token providers.packages/orm/package.json (1)
3-3: LGTM!Version bump to 3.0.0-beta.20 is consistent with the PR's objective to release a new beta version.
packages/config/vitest-config/package.json (1)
4-4: LGTM!Version bump to 3.0.0-beta.20 aligns with the package-wide beta release.
packages/orm/src/utils/kysely-utils.ts (3)
14-14: LGTM!Added imports for additional Kysely node types to support more comprehensive AST traversal.
Also applies to: 52-52, 64-64, 77-77, 79-79
398-409: LGTM!The new visitor methods correctly delegate to
defaultVisitfor the additional Kysely node types, maintaining consistency with the existing visitor pattern.
412-412: LGTM!The
AnyKyselytype alias provides a cleaner, more consistent way to referenceKysely<any>throughout the codebase.tests/e2e/orm/client-api/find.test.ts (3)
3-4: LGTM!Import reordering improves readability.
55-55: LGTM!Using the custom matcher
toBeRejectedByValidation()provides clearer test intent and aligns with the new ORMError architecture.
391-391: LGTM!Migration to the custom matcher
toBeRejectedNotFound()is more expressive and aligns with the unified error handling approach introduced in this PR.Also applies to: 405-405
README.md (2)
38-38: LGTM!Title case for headings improves consistency and readability.
Also applies to: 52-52
75-84: LGTM!The expanded manual setup instructions with explicit installation commands and file creation steps provide much clearer guidance for users setting up ZenStack manually.
packages/language/src/zmodel-comment-provider.ts (1)
1-21: LGTM!The comment provider implementation is clean and provides helpful default JSDoc-style comments for AST nodes. The delegation to
super.getComment(node)first ensures user-provided comments take precedence over generated ones.tests/e2e/orm/client-api/name-mapping.test.ts (1)
124-124: The review comment is based on a misunderstanding of the codebase's dual APIs.The
.orderBy('email')on line 124 is part of the query builder API (db.$qb.selectFrom(...).orderBy(...)), which uses Kysely syntax and naturally accepts scalar string arguments. This is distinct from the Prisma client API used elsewhere (orderBy: { field: 'asc' }), which uses object arguments. These are two different APIs with different signatures, and the change is correct for its context. There are no other query builderorderBycalls in the test suite to create inconsistency.Likely an incorrect or invalid review comment.
packages/orm/src/client/executor/error-processor.ts (1)
1-12: Verify whether node-pg native bindings will be used and confirm this limitation is acceptable.The assumption partially holds: both PostgreSQL (node-pg) and SQLite (better-sqlite3) expose error codes via a property on thrown errors. However, when using node-pg native bindings, the SQLSTATE is exposed under
sqlStateinstead ofcode. The current function only checks forcode, so it would silently returnundefinedfor native binding errors. Verify whether native bindings will be enabled in your environment and update the function to handlesqlStateif needed.packages/orm/src/client/crud/dialects/postgresql.ts (1)
438-442: Nice alignment with the unified error factory.Switching to
createInternalErrorkeeps this guard consistent with the new ORM error surface while preserving the existing behavior.packages/language/src/index.ts (1)
1-3: Good call exposingZModelCodeGeneratorhere.Re-exporting from the language package lets consumers rely on the public surface instead of internal paths.
tests/e2e/orm/policy/migrated/deep-nested.test.ts (1)
468-486: Assertion now matches the new ORMError shape.Checking
error.cause.messageensures the test targets the database constraint while staying aligned with the refactored error surface.packages/sdk/src/prisma/prisma-schema-generator.ts (1)
1-38: Public API import looks correct.Pulling
ZModelCodeGeneratorfrom@zenstackhq/languagereflects the newly exposed export and keeps SDK consumers on the supported path.packages/orm/src/client/crud/validator/utils.ts (1)
16-16: LGTM!The migration from
QueryErrortocreateNotSupportedErroris correct and aligns with the centralized error handling refactor.Also applies to: 455-455
packages/orm/src/client/crud/validator/index.ts (1)
233-239: LGTM!The error factory usage correctly passes the model name and chains the original Zod validation error as the cause. This provides better error context for debugging.
packages/orm/src/client/crud/dialects/sqlite.ts (2)
118-128: LGTM!The error handling correctly uses
createInternalErrorwith cause chaining to preserve the original JSON parse error.
376-404: LGTM!The error handling migration to
createInternalErroris correct. The function doesn't have model context available, so passingundefinedfor the model parameter is appropriate.packages/orm/src/client/crud/operations/delete.ts (1)
4-4: LGTM!The migration to error factory functions is correct. Both
createNotFoundErrorandcreateRejectedByPolicyErrorproperly pass the model context and provide clear error messages.Also applies to: 37-37, 43-47
packages/orm/src/client/plugin.ts (1)
1-1: LGTM! Improved type safety.The change from
stringtoQueryIdimproves type safety and aligns with Kysely's native query identification system. This provides better type checking for plugin authors working with mutation hooks.Also applies to: 141-141
tests/e2e/orm/policy/migrated/multi-field-unique.test.ts (1)
1-1: LGTM!The test assertions have been correctly updated to validate the new
ORMErrortype withDB_QUERY_ERRORreason. The pattern is consistent and properly checks database constraint violations.Also applies to: 3-3, 23-25, 88-90
packages/orm/src/client/query-utils.ts (1)
16-16: LGTM! Consistent error handling refactor.The migration from direct error class instantiation to the
createInternalErrorfactory function is consistently applied throughout the file. The model context is appropriately passed to improve error diagnostics.Also applies to: 35-35, 49-49, 57-57, 62-62, 74-74, 83-83, 88-88, 100-100, 106-106, 109-109, 112-112, 156-156, 176-176, 331-331
packages/orm/src/client/index.ts (1)
6-6: LGTM! Improved API surface control.Narrowing the error exports from a wildcard to specific named exports improves API clarity and makes the public contract more explicit.
packages/orm/src/client/contract.ts (1)
3-3: LGTM! Type alias improves consistency.The change from
ToKysely<any>toAnyKyselyimproves type naming consistency across the codebase.Also applies to: 120-120
packages/orm/src/client/crud/operations/create.ts (1)
4-4: LGTM! Consistent error factory usage.The migration to the error factory pattern maintains the same error information while centralizing error construction.
Also applies to: 51-55
packages/orm/src/client/crud/dialects/base-dialect.ts (2)
17-17: LGTM! Comprehensive error handling refactor.The changes consistently migrate to the new error factory pattern throughout the dialect:
createNotSupportedErrorfor unsupported featurescreateInvalidInputErrorfor invalid user inputscreateConfigErrorfor configuration issuesThe signature change from
GetModels<Schema>tostringfor the model parameter simplifies the type and aligns with the broader refactor.Also applies to: 67-67, 98-98, 485-485, 513-513, 516-516, 592-592, 645-645, 820-820, 1089-1089
770-770: No issues found — callback refactor correctly maintains ordering behavior.The implementation properly handles all three concerns:
- Sort direction negation: Correctly applied via
this.negateSort(value.sort, negated)within the callback (line 808).- Null positioning: Properly implemented with
ob.nullsFirst()/ob.nullsLast()calls (line 811).- Kysely query builder interaction: Correct fluent API usage; the callback properly chains
ob.asc()/ob.desc()with null positioning methods.The callback pattern is appropriately isolated to regular scalar fields with nulls support. Aggregations correctly retain the older pattern since they don't support null positioning. Cursor pagination uses
makeDefaultOrderByindependently and functions as designed. Tests confirm nulls positioning works correctly.tests/e2e/orm/client-api/error-handling.test.ts (1)
1-52: LGTM! Comprehensive error handling test coverage.This test file provides excellent coverage of the new ORMError system:
- Invalid input validation errors
- Not found errors
- Policy rejection errors with specific reason codes
- Database query errors with provider-specific error codes
The tests appropriately use the new error types and matchers, and account for database provider differences.
packages/language/src/zmodel-definition.ts (1)
18-35: Definition links for imported models are wired correctly
resolveImport+LangiumDocumentsmakes these model import jumps reliable, and the fallback to the base provider keeps other symbols covered. Nicely done.packages/orm/src/client/crud/operations/update.ts (1)
64-176: Great move to the shared policy-error factorySwitching to
createRejectedByPolicyErrorkeeps this path consistent with the new ORMError surface and avoids duplicating metadata wiring.packages/testtools/src/vitest-ext.ts (1)
8-125: Vitest helpers now align with ORMErrorReasonThe new matcher logic cleanly enforces
ORMError+ reason checks and still honors optional message assertions—appreciate the tidy migration.packages/language/src/zmodel-semantic.ts (1)
27-110: Semantic token mapping looks comprehensiveCovering models, fields, attributes, invocations, and member access in one pass gives the LSP a solid initial highlight set.
packages/language/src/module.ts (1)
17-142: Module wiring scales well with the new providersRegistering the formatter/definition/completion/semantic/documentation providers and only indexing freshly created plugin docs keeps startup lean without missing symbol exposure.
Summary by CodeRabbit
New Features
Bug Fixes
Chores