custom procedures #551
custom procedures #551
Conversation
📝 WalkthroughWalkthroughAdds first-class "procedures": grammar/validator updates, schema/type changes (array flags), SDK ts-schema emission, ORM $procs runtime with input validation and plugin hooks, server REST/RPC procedure routing and SuperJSON handling, TanStack Query per-procedure hooks, and tests/fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/orm/src/client/crud/validator/index.ts (1)
74-128: Validation logic is sound, but parsed values are discarded.The
validateProcedureArgsmethod validates each parameter but returns the originalargsarray rather than the Zod-parsed values. This means:
Type coercion is lost: Zod transformations (e.g.,
z.iso.datetime()coercing strings to Date objects) won't propagate to the procedure handler.Refinement side effects: Any
.transform()or.refine()results are discarded.If this is intentional (validation-only, no coercion), consider adding a comment. Otherwise, consider accumulating parsed values:
🔎 Proposed fix to return coerced values
+ const coercedArgs: unknown[] = []; + // Validate each parameter using the procedure signature. Missing trailing optional parameters are allowed. for (let i = 0; i < procDef.params.length; i++) { const param = procDef.params[i]!; const value = args[i]; if (typeof value === 'undefined') { // When a value is not provided, allow it only if the parameter is optional. // Note: callers can still explicitly pass `undefined`. if (i >= args.length) { if (param.optional) { + coercedArgs.push(undefined); continue; } throw createInvalidInputError( `Missing procedure argument: ${param.name}`, `$procedures.${proc}`, ); } if (param.optional) { + coercedArgs.push(undefined); continue; } throw createInvalidInputError( `Invalid procedure argument: ${param.name} is required`, `$procedures.${proc}`, ); } const schema = this.makeProcedureParamSchema(param); const parsed = schema.safeParse(value); if (!parsed.success) { throw createInvalidInputError( `Invalid procedure argument: ${param.name}: ${formatError(parsed.error)}`, `$procedures.${proc}`, ); } + coercedArgs.push(parsed.data); } - return args; + return coercedArgs;packages/language/src/validators/procedure-validator.ts (1)
6-10: Consider expanding reserved names for JavaScript runtime safety.The reserved names list covers critical prototype-related names, which is good for preventing prototype pollution. Consider whether additional reserved names like
hasOwnProperty,toString, orvalueOfshould be included to prevent potential runtime conflicts when procedures are exposed as object properties.packages/language/src/utils.ts (1)
107-131: LGTM! Consider adding exhaustiveness check.The expanded signature correctly handles both
BuiltinTypeandExpressionTypeinputs, making the function idempotent for expression types. The switch now covers all cases from theExpressionTypeunion.For future maintainability, consider adding an exhaustiveness check to catch new types at compile time:
🔎 Optional: Add exhaustiveness check
case 'Json': case 'Bytes': return 'Any'; + default: + const _exhaustive: never = type; + return _exhaustive; } }packages/server/src/api/rest/index.ts (1)
548-556: Consider simplifying SuperJSON payload detection.The condition
argsPayload && typeof argsPayload === 'object' && !Array.isArray(argsPayload) && 'meta' in (argsPayload as any)is duplicated between REST and RPC handlers. TheprocessSuperJsonRequestPayloadfunction already handles non-object cases gracefully (returns{ result: payload }unchanged).🔎 Suggested simplification
- let processedArgsPayload = argsPayload; - if (argsPayload && typeof argsPayload === 'object' && !Array.isArray(argsPayload) && 'meta' in (argsPayload as any)) { - const { result, error } = await processSuperJsonRequestPayload(argsPayload); - if (error) { - return this.makeProcBadInputErrorResponse(error); - } - processedArgsPayload = result; + const { result: processedArgsPayload, error: superJsonError } = await processSuperJsonRequestPayload(argsPayload); + if (superJsonError) { + return this.makeProcBadInputErrorResponse(superJsonError); }packages/server/src/api/rpc/index.ts (1)
180-292: Consider extracting shared procedure handling logic.The
handleProcedureRequestmethod is nearly identical toprocessProcedureRequestin the REST handler. Both perform:
- Procedure definition lookup
- HTTP method validation
- Argument extraction (body/query)
- SuperJSON payload processing
- Argument mapping and validation
- Procedure invocation
- Response serialization
Consider extracting this to the shared
procedures.tsmodule to reduce duplication and ensure consistent behavior.packages/orm/src/client/client-impl.ts (1)
217-227: Consider caching the $procs object.The
$procsgetter creates a new object with wrapped handlers on every access. While this ensures fresh closure overthis, repeated access patterns likeclient.$procs.foo(); client.$procs.bar();will create the wrapper object multiple times.🔎 Optional: Cache the procedures object
+ private _procs: Record<string, (...args: unknown[]) => Promise<unknown>> | undefined; + get $procs() { + if (this._procs) { + return this._procs; + } - return Object.keys(this.$schema.procedures ?? {}).reduce((acc, name) => { + this._procs = Object.keys(this.$schema.procedures ?? {}).reduce((acc, name) => { acc[name] = (...args: unknown[]) => this.handleProc(name, args); return acc; }, {} as any); + return this._procs; }Note: If caching, ensure the cache is invalidated when plugins change (in
$use/$unuse).packages/clients/tanstack-query/test/schemas/procedures/schema-lite.ts (1)
5-5: Minor: Remove extra blank line.There's an extra blank line (line 5) that could be removed for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/language/src/generated/ast.tsis excluded by!**/generated/**packages/language/src/generated/grammar.tsis excluded by!**/generated/**
📒 Files selected for processing (33)
packages/cli/test/ts-schema-gen.test.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-typing-test.tspackages/clients/tanstack-query/test/schemas/procedures/schema-lite.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/clients/tanstack-query/test/vue-typing-test.tspackages/language/src/utils.tspackages/language/src/validator.tspackages/language/src/validators/common.tspackages/language/src/validators/function-invocation-validator.tspackages/language/src/validators/procedure-validator.tspackages/language/src/zmodel.langiumpackages/language/test/procedure-validation.test.tspackages/orm/src/client/client-impl.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/index.tspackages/orm/src/client/plugin.tspackages/orm/test/procedures.test.tspackages/schema/src/schema.tspackages/sdk/src/ts-schema-generator.tspackages/server/src/api/common/procedures.tspackages/server/src/api/rest/index.tspackages/server/src/api/rpc/index.tspackages/server/test/api/procedures.e2e.test.tspackages/server/test/api/rest.test.tspackages/server/test/api/rpc.test.tstests/e2e/orm/schemas/procedures/input.tstests/e2e/orm/schemas/procedures/models.tstests/e2e/orm/schemas/procedures/schema.tstests/e2e/orm/schemas/procedures/schema.zmodel
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/schemas/procedures/schema.tstests/e2e/orm/schemas/procedures/input.tstests/e2e/orm/schemas/procedures/models.ts
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
tests/e2e/orm/schemas/procedures/schema.zmodel
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Database migrations should use Prisma CLI under the hood via ZenStack commands
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/cli/test/ts-schema-gen.test.tspackages/language/test/procedure-validation.test.tstests/e2e/orm/schemas/procedures/schema.tspackages/clients/tanstack-query/test/vue-typing-test.tspackages/clients/tanstack-query/test/schemas/procedures/schema-lite.tspackages/orm/test/procedures.test.tspackages/server/test/api/rpc.test.tspackages/clients/tanstack-query/test/react-typing-test.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/server/test/api/procedures.e2e.test.tspackages/server/test/api/rest.test.tstests/e2e/orm/schemas/procedures/models.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
packages/cli/test/ts-schema-gen.test.tspackages/language/test/procedure-validation.test.tspackages/clients/tanstack-query/test/vue-typing-test.tspackages/clients/tanstack-query/test/react-typing-test.tspackages/clients/tanstack-query/test/svelte-typing-test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/cli/test/ts-schema-gen.test.tstests/e2e/orm/schemas/procedures/schema.tspackages/orm/src/client/contract.tspackages/clients/tanstack-query/test/schemas/procedures/schema-lite.tstests/e2e/orm/schemas/procedures/schema.zmodeltests/e2e/orm/schemas/procedures/models.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/language/test/procedure-validation.test.tspackages/orm/src/client/index.tspackages/clients/tanstack-query/test/vue-typing-test.tspackages/orm/test/procedures.test.tspackages/server/test/api/rpc.test.tspackages/clients/tanstack-query/test/react-typing-test.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/server/test/api/procedures.e2e.test.tspackages/server/test/api/rest.test.tstests/e2e/orm/schemas/procedures/models.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/index.tstests/e2e/orm/schemas/procedures/schema.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/test/schemas/procedures/schema-lite.tspackages/orm/test/procedures.test.tspackages/orm/src/client/plugin.tspackages/clients/tanstack-query/src/react.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/clients/tanstack-query/src/svelte/index.svelte.tstests/e2e/orm/schemas/procedures/models.tspackages/orm/src/client/client-impl.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:
tests/e2e/orm/schemas/procedures/schema.tspackages/clients/tanstack-query/test/schemas/procedures/schema-lite.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/test/schemas/procedures/schema-lite.tspackages/orm/test/procedures.test.tspackages/clients/tanstack-query/test/react-typing-test.tspackages/clients/tanstack-query/src/react.tspackages/clients/tanstack-query/test/svelte-typing-test.tstests/e2e/orm/schemas/procedures/input.tstests/e2e/orm/schemas/procedures/models.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/cli/**/*.test.{ts,tsx} : CLI package tests should focus on action-specific tests for each command
Applied to files:
packages/orm/test/procedures.test.ts
🧬 Code graph analysis (18)
packages/language/src/validators/function-invocation-validator.ts (1)
packages/language/src/utils.ts (2)
mapBuiltinTypeToExpressionType(107-131)typeAssignable(84-102)
packages/language/src/validator.ts (2)
packages/language/src/generated/ast.ts (2)
Procedure(647-655)Procedure(657-657)packages/language/src/validators/procedure-validator.ts (1)
ProcedureValidator(15-55)
packages/orm/src/client/contract.ts (2)
packages/schema/src/schema.ts (6)
SchemaDef(11-19)GetEnums(137-137)GetEnum(139-141)GetModels(122-122)GetTypeDefs(143-143)ProcedureDef(82-87)packages/orm/src/utils/type-utils.ts (1)
OrUndefinedIf(80-80)
packages/clients/tanstack-query/test/vue-typing-test.ts (1)
packages/clients/tanstack-query/src/vue.ts (1)
useClientQueries(276-332)
packages/clients/tanstack-query/src/vue.ts (5)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/contract.ts (1)
ProcedureFunc(261-263)packages/clients/tanstack-query/src/react.ts (5)
ProcedureHooks(192-199)useInternalProcedureMutation(457-470)useInternalProcedureInfiniteQuery(428-442)useInternalQuery(588-609)useInternalInfiniteQuery(634-661)packages/clients/tanstack-query/src/svelte/index.svelte.ts (5)
ProcedureHooks(171-178)useInternalProcedureMutation(339-352)useInternalProcedureInfiniteQuery(326-337)useInternalQuery(444-474)useInternalInfiniteQuery(476-508)packages/clients/tanstack-query/src/common/types.ts (2)
QueryContext(9-24)ExtraQueryOptions(29-34)
packages/language/src/validators/common.ts (1)
packages/language/src/ast.ts (2)
AstNode(4-4)AstNode(66-71)
packages/clients/tanstack-query/test/schemas/procedures/schema-lite.ts (2)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)
packages/orm/test/procedures.test.ts (3)
packages/orm/src/client/index.ts (1)
ZenStackClient(1-1)packages/orm/src/dialects/sqlite.ts (1)
SqliteDialect(1-1)packages/orm/src/client/plugin.ts (1)
definePlugin(58-60)
packages/server/test/api/rpc.test.ts (2)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)packages/server/src/api/rpc/index.ts (1)
RPCApiHandler(36-377)
packages/orm/src/client/plugin.ts (2)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/contract.ts (1)
ClientContract(65-199)
packages/clients/tanstack-query/test/react-typing-test.ts (3)
packages/clients/tanstack-query/test/schemas/procedures/schema-lite.ts (1)
schema(62-62)tests/e2e/orm/schemas/procedures/schema.ts (1)
schema(173-173)packages/clients/tanstack-query/test/schemas/basic/schema-lite.ts (1)
schema(172-172)
packages/clients/tanstack-query/src/react.ts (4)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/contract.ts (1)
ProcedureFunc(261-263)packages/clients/tanstack-query/src/common/types.ts (2)
QueryContext(9-24)ExtraQueryOptions(29-34)packages/clients/tanstack-query/src/common/query-key.ts (1)
getQueryKey(25-35)
packages/clients/tanstack-query/test/svelte-typing-test.ts (2)
packages/clients/tanstack-query/src/svelte/index.svelte.ts (1)
useClientQueries(264-314)packages/clients/tanstack-query/test/schemas/procedures/schema-lite.ts (1)
schema(62-62)
packages/language/src/utils.ts (2)
packages/schema/src/schema.ts (1)
BuiltinType(89-99)packages/language/src/generated/ast.ts (2)
BuiltinType(103-103)ExpressionType(125-125)
packages/orm/src/client/crud/validator/index.ts (4)
packages/schema/src/schema.ts (2)
ProcedureDef(82-87)BuiltinType(89-99)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)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/server/test/api/procedures.e2e.test.ts (3)
packages/orm/src/client/contract.ts (1)
ClientContract(65-199)packages/server/src/api/rest/index.ts (1)
schema(285-287)packages/testtools/src/client.ts (1)
createTestClient(101-248)
packages/schema/src/schema.ts (1)
packages/language/src/generated/ast.ts (2)
ProcedureParam(663-669)ProcedureParam(671-671)
packages/server/src/api/rpc/index.ts (2)
packages/server/src/api/common/procedures.ts (3)
getProcedureDef(8-14)processSuperJsonRequestPayload(44-66)validateProcedureArgs(154-166)packages/orm/src/client/errors.ts (1)
ORMError(66-104)
🪛 GitHub Actions: Build and Test
packages/clients/tanstack-query/test/react-typing-test.ts
[error] 119-119: test:typecheck failed. TypeScript error TS2345: Argument of type '"bob"' is not assignable to parameter of type '[] | [string | undefined] | undefined'.
⏰ 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). (1)
- GitHub Check: GitGuardian Security Checks
🔇 Additional comments (57)
packages/sdk/src/ts-schema-generator.ts (1)
1126-1204: LGTM! Array flag propagation for procedures is correctly implemented.The changes properly emit:
array: truein the parameter object literal whenparam.type.arrayis true (line 1134)- An
arrayproperty signature in the TypeScript tuple type (lines 1165-1174)returnArray: trueon the procedure object whenproc.returnType.arrayis true (lines 1197-1199)The implementation follows the established patterns for optional flags and aligns with the type definitions in
packages/schema/src/schema.ts.packages/schema/src/schema.ts (1)
80-87: LGTM! Type definitions correctly extended for procedure array support.The updates to
ProcedureParamandProcedureDefproperly introduce:
array?: booleanflag for parametersreadonlymodifier forparamstuple (appropriate for immutable schema definitions)returnArray?: booleanfor return type arraysThese changes align with the TS schema generator and enable proper type inference across the codebase.
packages/cli/test/ts-schema-gen.test.ts (1)
187-209: LGTM! Good test coverage for procedure array flags.The test case properly validates:
- Array-typed parameters (
ids: Int[]) emitarray: true- Array return types emit
returnArray: true- Both single-parameter and zero-parameter procedures are covered
This aligns with the learnings that TypeScript type coverage should be validated through tests.
packages/orm/src/client/contract.ts (3)
242-259: Good backward-compatible API extension with$procsand$proceduresaliases.The dual exposure pattern maintains backward compatibility while introducing the preferred
$procsnamespace. Documentation comments clearly indicate the preferred API.
265-287: Well-designed type mapping for procedure parameters and returns.The type helpers correctly handle:
MapProcedureReturn: Array wrapping based onreturnArrayflagMapProcedureParam: Array wrapping based onarrayflag with conditional undefined for optional paramsMapProcedureParams: Recursive union construction allowing trailing optional params to be omittedThe recursive tuple prefix union in
MapProcedureParams(lines 281-287) is a clever approach for modeling optional trailing parameters in function signatures.
227-240: LGTM! Enhanced type mapping with enum support.The
EnumValuetype and updatedMapTypeproperly resolve enum types to their value union, enabling correct type inference for procedures returning enum values.packages/language/src/zmodel.langium (2)
228-229: LGTM! Optional mutation flag enables query procedures.Making
mutationoptional allows procedures to default to query behavior (non-mutation), which aligns with the common pattern where most procedures are read operations.
236-237: LGTM! Extended type vocabulary for procedures.The additions of
BigInt,Decimal,Json,Bytes,Void, andUndefinedto bothRegularIDWithTypeNamesandExpressionTypeenable richer type expressions in procedure signatures, consistent with the_TypeMapextensions inpackages/orm/src/client/contract.ts.Also applies to: 268-269
packages/server/src/api/common/procedures.ts (3)
1-14: LGTM! Well-structured procedure lookup utility.The
getProcedureDeffunction uses safe property access viaObject.prototype.hasOwnProperty.callto avoid prototype pollution issues. ThePROCEDURE_ROUTE_PREFIXESconstant centralizes the endpoint paths for consistency.
68-152: Comprehensive argument mapping with good edge case handling.The
mapProcedureArgsfunction handles multiple input formats elegantly:
- Zero-parameter procedures with empty/undefined payloads (lines 73-84)
- All-optional parameters with undefined payload (lines 87-89)
- Single array-typed parameter disambiguation (lines 93-96)
- Positional array arguments (lines 91-111)
- Single parameter with object wrapping detection (lines 113-128)
- Named object arguments with unknown key rejection (lines 136-141)
The unknown key rejection (lines 137-140) is a good defensive measure to catch user mistakes early.
154-166: LGTM! Validation respects global client options.The
validateProcedureArgsfunction correctly defers to the client'svalidateInputoption, maintaining consistency with the ORM's validation behavior.packages/clients/tanstack-query/src/vue.ts (4)
65-83: LGTM! Procedure type utilities follow established patterns.The type helpers (
ExtractProcedures,ProcedureArgsTuple,ProcedureReturn,ProcedurePayload) correctly derive procedure types from the schema and align with the React client implementation for cross-framework consistency.
148-188: LGTM! Well-typed procedure hook group.The
ProcedureHookGrouptype correctly branches based on themutationflag:
- Mutation procedures expose
useMutationonly- Non-mutation procedures expose
useQueryanduseInfiniteQueryThis mirrors the React implementation and provides a consistent API across frameworks.
300-331: LGTM! Dynamic procedure hook building.The
buildProcedureHooksfunction correctly iterates over schema procedures and wires:
useMutationfor mutation proceduresuseQuery/useInfiniteQueryfor non-mutation proceduresThe hooks are attached to the
$procsnamespace, matching the ORM contract.
362-375: LGTM! Procedure mutation hook implementation.The
useInternalProcedureMutationcorrectly:
- Uses
makeUrlwith$procsendpoint model- Sends POST request with procedure data
- Integrates with Vue's reactivity via
toValuepackages/clients/tanstack-query/src/svelte/index.svelte.ts (4)
66-83: LGTM! Procedure type utilities consistent with Vue/React implementations.The type helpers are identical across frameworks, ensuring consistent type inference for procedure arguments and returns.
147-178: LGTM! Svelte-adapted procedure hook types.The
ProcedureHookGroupcorrectly uses Svelte'sAccessorpattern (instead of Vue'sMaybeRefOrGetter) while maintaining the same mutation/non-mutation branching logic.
288-314: LGTM! Procedure hook building consistent with Vue implementation.The dynamic hook building follows the same pattern as Vue, correctly wiring mutation and non-mutation procedures to appropriate hooks.
339-352: LGTM! Svelte procedure mutation hook.Uses
createMutation(Svelte's equivalent ofuseMutation) with the correct endpoint model and request format.packages/orm/src/client/index.ts (1)
6-6: LGTM! Public API extension for procedure validation.The export of
InputValidatoris a clean addition to the ORM client's public API surface, enabling procedure argument validation in REST/RPC server layers.packages/language/src/validators/function-invocation-validator.ts (1)
23-23: LGTM! Consistent type mapping for scalar argument validation.The addition of
mapBuiltinTypeToExpressionTypeensures that scalar argument types (likeBigInt,Decimal,Json,Bytes) are consistently mapped to their expression equivalents (Int,Float,Any) before type compatibility checks. This aligns with the broader type resolution behavior introduced across the validator subsystem.Also applies to: 177-179
packages/language/test/procedure-validation.test.ts (1)
1-43: LGTM! Good baseline test coverage for procedure validation.The test file covers the essential validation scenarios:
- Unknown parameter types
- Unknown return types
- Reserved procedure names
The tests appropriately use
loadSchemaWithErrorwith regex matching to verify error reporting.packages/language/src/validators/common.ts (1)
22-27: LGTM! Important security improvement to prevent prototype pollution.Using
Object.create(null)creates a null-prototype map that prevents collisions with built-in prototype properties like__proto__andconstructor. This is particularly important for procedure validation where reserved names must be safely grouped by name.packages/clients/tanstack-query/test/svelte-typing-test.ts (1)
3-3: LGTM! Comprehensive procedure typing tests for Svelte.The test additions cover:
- Query procedures (
greet,greetMany) with optional arguments and data transformations- Mutation procedures (
sum) with array arguments- Type safety checks for invalid usage (e.g., attempting
useMutationon a query procedure)- Both
$procsand$proceduresalias paths- Query key validation for procedures
The tests mirror existing patterns and provide strong type coverage for the new procedure support.
Also applies to: 6-6, 112-131
packages/server/test/api/rest.test.ts (1)
3167-3401: LGTM! Comprehensive REST procedure test coverage.The new test suite thoroughly exercises the REST procedure endpoints with excellent coverage:
- Serialization: SuperJSON with metadata for complex types (Decimal)
- Argument handling: Optional params, required params, array types, enums, typedefs
- HTTP methods: GET for queries, POST for mutations
- Error cases: Missing required args, wrong types, too many args, unknown arg keys
- Path variants: Both
/$procsand alias pathsThe tests are well-structured, clear, and follow established patterns in the file.
packages/language/src/validator.ts (1)
12-12: LGTM! Clean integration of procedure validation.The changes properly wire
ProcedureValidatorinto the validation flow:
- Added necessary imports
- Registered
Procedurevalidation check in the checks mapping- Implemented
checkProceduremethod following the established patternThe integration is consistent with existing validators and maintains the validator architecture.
Also applies to: 24-24, 45-45, 96-98
packages/clients/tanstack-query/test/react-typing-test.ts (1)
117-136: The React test code is correct and matches the hook type signatures.The current implementation in
react-typing-test.tsline 119 (useQuery('bob')) is valid. React's procedure hooks acceptProcedurePayload<Schema, Name>directly (not wrapped), which for thegreetprocedure resolves tostring | undefined. The test passing a string value is correct.The differences between React, Svelte, and Vue are intentional and framework-specific:
- React: Accepts direct values (
args?: ProcedurePayload<Schema, Name>)- Svelte: Requires function accessors (
args?: Accessor<ProcedurePayload<Schema, Name>>) due to Svelte's reactivity model- Vue: Accepts flexible reactive patterns (
args?: MaybeRefOrGetter<ProcedurePayload<Schema, Name>>)These patterns are not bugs but correct framework-specific implementations. No changes are needed.
packages/server/test/api/procedures.e2e.test.ts (1)
1-103: Well-structured E2E test for procedure handling.The test suite covers essential procedure scenarios:
- Query procedure with valid/invalid input
- Mutation procedure with transactional rollback verification
- HTTP method enforcement (GET for queries, POST for mutations)
A few observations:
Line 86: The
requestBody: ['a@a.com', 'b@b.com']format tests array-to-positional-args mapping, which is good coverage for the RPC layer.Consider adding edge cases: The test could be extended to cover:
- Procedure not found (404 or similar)
- Empty/null arguments for optional parameters
Based on learnings, E2E tests should validate real-world schema compatibility. This test uses an inline schema which is acceptable for unit-style E2E tests, but consider adding tests against established project schemas in a separate suite.
packages/clients/tanstack-query/test/vue-typing-test.ts (1)
114-134: Comprehensive procedure typing tests for Vue client.The type-level tests effectively validate:
- Query procedures expose
useQueryanduseInfiniteQuerybut notuseMutation(Line 122-123)- Mutation procedures expose
useMutationwith correct argument tuple types (Line 126-128)- Both
$procsand$proceduresaliases are accessible (Line 121, 134)- Optional parameters work correctly (Line 116 with
undefined)queryKeyaccess is typed (Lines 118, 120)The
@ts-expect-errorguards appropriately enforce type constraints at compile time.packages/orm/src/client/crud/validator/index.ts (1)
130-159: Model-typed parameters use shallow validation.Line 139 accepts
z.record(z.string(), z.unknown())for model-typed parameters, which only validates the value is an object with string keys. This is documented inline but worth noting:
- No field-level validation against the model schema
- Allows extra/missing fields
This is reasonable for procedure inputs where the handler may perform custom logic, but callers should be aware that model-typed procedure arguments aren't deeply validated.
tests/e2e/orm/schemas/procedures/schema.ts (1)
1-173: Auto-generated schema with comprehensive procedure definitions.This generated file provides good test coverage for procedure variants:
- Various return types:
User,User[],Role,Role[],Overview,Null,Void,Undefined,String,Json,Bytes- Parameter variations: required, optional (
greet), array (findByIds)- No-param procedures for testing edge cases
The tuple type assertions (e.g.,
as [ id: { "name": "id"; "type": "Int"; } ]) ensure type-level param info is preserved.tests/e2e/orm/schemas/procedures/models.ts (1)
1-13: Standard auto-generated models file.The file correctly re-exports:
Usertype via$ModelResultOverviewtype via$TypeDefResultRoleas both a const (values object) and type (union)This pattern enables consumers to import typed model shapes and enum values from the generated schema.
packages/orm/src/client/plugin.ts (2)
39-43: Well-designed procedure interception hook.The
onProcedurehook follows the established pattern ofonQueryand provides a clean interception point for procedure invocations.
64-95: Comprehensive hook context with clear documentation.The
OnProcedureHookContexttype is well-documented and provides:
name: Procedure identifiermutation: Distinguishes query vs mutation semanticsargs: Immutable (readonly) input argumentsproceed: Continuation function allowing argument transformationclient: Access to the invoking client for additional operationsThe
readonly unknown[]type forargsandproceedparameter enforces immutability at the type level, encouraging plugins to create new arrays rather than mutate.Based on learnings, implementing plugin hooks at the ORM level for query interception is expected, and this aligns with that pattern.
packages/server/test/api/rpc.test.ts (2)
128-213: Thorough RPC procedure test coverage.This test block validates core procedure RPC behavior:
- HTTP method enforcement: GET for queries (Lines 170-177), POST for mutations (Lines 188-194)
- Error messaging for method violations (Lines 179-186, 196-202)
- Falsy return serialization (
falsepreserved, Line 207)- Undefined serialization with meta (Lines 209-212)
The helper
handleProcRequestpattern keeps tests readable.
215-325: Excellent edge case coverage for procedure argument handling.The test block covers important edge cases:
Scenario Lines Status Code Multi-arg (>2) object mapping 267-273 200 All-optional params, no payload 276-278 200 Array-typed single param 281-287 200 Enum param 290-292 200 TypeDef param (object) 295-301 200 Wrong type input 304-306 422 Too many args 309-315 400 Unknown keys in args 318-324 400 This provides confidence in the argument mapping and validation logic.
packages/orm/test/procedures.test.ts (4)
15-35: Good coverage for procedure alias exposure.Tests verify both
$procsand$proceduresaccessors work identically, ensuring backward compatibility and alias consistency.
37-66: Error handling tests for configuration issues.Properly tests two distinct error scenarios:
- Procedures defined in schema but no
proceduresoption provided (Line 49)- Procedures option provided but specific handler missing (Line 65)
These provide clear error messages for common configuration mistakes.
89-133: Hook execution order demonstrates LIFO plugin stack.The test verifies:
- Plugins execute in reverse order (p2 before p1, Line 132)
- Argument mutations propagate through the chain (1+1=2 passed to handler, result = 2+2 = 4, Line 131)
This LIFO ordering (last plugin registered executes first) should be documented, as it affects how users compose plugins. Consider adding a brief comment in the plugin.ts documentation or here.
Please verify the intended plugin execution order (LIFO vs FIFO) is documented somewhere for users. The test shows
[p1, p2]registration results in['p2:add', 'p1:add']execution order.
135-154: Transaction wrapping verification for mutation procedures.The test elegantly uses
c.isTransactionto verify:
- Non-mutation procedures (
qry) run outside transactions (Line 152)- Mutation procedures (
mut) are automatically wrapped in transactions (Line 153)This validates the transactional semantics mentioned in the PR context without requiring actual database state verification.
packages/language/src/validators/procedure-validator.ts (2)
36-44: LGTM!The parameter type validation correctly checks for unresolved type references by verifying
typeRef && !typeRef.ref. This properly handles the case where a type is referenced but the reference cannot be resolved.
46-54: LGTM!Return type validation mirrors parameter validation logic, maintaining consistency. The error message properly includes the reference text for debugging.
packages/server/src/api/rest/index.ts (4)
346-349: LGTM!Clean integration of procedure handling into the REST API path routing. The
/$procs/prefix is a sensible convention that clearly distinguishes procedure calls from model-based REST endpoints.
501-519: LGTM!Proper enforcement of HTTP method semantics: mutations require POST, queries require GET. This aligns with REST conventions where mutations are non-idempotent operations.
619-653: LGTM!Well-structured ORM error handling with proper HTTP status code mapping. The use of
ts-pattern'smatchprovides clear, exhaustive handling of error reasons.
577-577: Remove the suggestion for explicit validation of$procedures.The
$proceduresproperty is a getter that always returns an object (neverundefined), even when no procedures are configured. The optional chaining is already defensive, and procedure validation is handled byvalidateProcedureArgs()before this point.packages/server/src/api/rpc/index.ts (2)
56-64: LGTM!Clean integration point for procedure handling. The
$procsmodel name convention clearly distinguishes procedure calls from model operations.
155-161: Good improvement to response serialization.Always applying SuperJSON serialization ensures consistent handling of special types (Date, BigInt, etc.) and falsy values. The comment noting falsy value handling is helpful.
packages/orm/src/client/client-impl.ts (2)
254-278: LGTM! Well-structured plugin pipeline.The plugin chain correctly:
- Wraps the original handler in a
proceedfunction- Iterates plugins and wraps each
onProcedurehook- Passes through
argsallowing plugins to modify them- Maintains the chain by capturing previous
proceedreferenceThis mirrors the CRUD operation plugin pattern, ensuring consistency.
280-286: LGTM! Sensible transaction semantics.Auto-wrapping mutation procedures in transactions provides atomic behavior by default, which is the expected behavior for data-modifying operations. The check for
!this.isTransactioncorrectly prevents nested transaction creation.tests/e2e/orm/schemas/procedures/schema.zmodel (1)
1-35: Comprehensive test fixture for procedure types.This schema provides excellent coverage for procedure validation and type generation:
- Scalar returns (
getRole,greet)- Array returns (
listUsers,findByIds,getRoles)- Complex type returns (
overview)- Special types (
getNull,getVoid,getUndefined)- Array parameters (
findByIds)- Optional parameters (
greet)- Special data types (
echoJson,echoBytes)As per coding guidelines, remember to run
zenstack generateafter any modifications to this schema.packages/clients/tanstack-query/test/schemas/procedures/schema-lite.ts (1)
38-56: LGTM! Good coverage of procedure variants.The procedure definitions cover the key typing scenarios:
greet: Optional parameter, scalar returngreetMany: Optional parameter, array return (returnArray: true)sum: Required parameters, mutation flagThis provides good coverage for validating TanStack Query hook type inference.
tests/e2e/orm/schemas/procedures/input.ts (1)
1-30: LGTM!This is auto-generated code following the established pattern for ORM input types. The type aliases correctly parameterize the generic ORM types with the schema and model name.
packages/clients/tanstack-query/src/react.ts (4)
65-83: LGTM! Well-structured procedure type utilities.The type definitions correctly extract procedure information from the schema and handle the various argument configurations (none, single, multiple) appropriately.
158-199: LGTM! Clean conditional hook group types.The
ProcedureHookGroupcorrectly differentiates between mutation and query procedures, andProcedureHooksproperly uses conditional typing to only expose$procswhen procedures exist in the schema.
341-377: LGTM! Runtime procedure hook wiring is correctly implemented.The implementation correctly:
- Detects procedures from the schema
- Differentiates between mutation and query procedures
- Merges root options with hook-specific options
- Follows the same pattern as model query hooks
391-394: Appropriate: Optimistic updates disabled for procedure queries.Procedures correctly disable optimistic updates (
optimisticUpdate: false) since they're arbitrary functions rather than entity CRUD operations, so there's no predictable data structure for optimistic state management.
|
Insane turnaround time on this @mwillbanks. Well done. What do you all think of also having the ability to define procedures under models? model User {
...
mutation procedure signUp(info: SignUpInfo): User
}I bring this up because I foresee that there will be a lot of unrelated functions grouped together both in the procedures' implementation, and in the intellisense when you're trying to call a procedure, and if this feature is not considered at this stage, it could be a breaking change. For example, the above procedure really has no business being grouped together with the below procedure. model Invoice {
...
mutation procedure process(id: String): Void
}In larger code bases especially, things will become very difficult to follow because procedures simply will not be organized. With these "model-scoped procedures" (I don't know what else to call them lol), they could be invoked like Model-scoped procedures could be disambiguated from "top-level" procedures by simply automatically prefixing the procedure's name with the model name and using a dot like That leaves the question of how the procedures should then be defined. Something like this maybe? const db = new ZenStackClient(schema, {
...,
procedures: {
// db.$procs.user.signUp or db.user.$procs.signUp
user: {
async signUp(client, args) {
...
},
},
// db.$procs.topLevelProcedure
$global: {
async topLevelProcedure(client, args) {
...
},
},
},
};) |
Thanks :) I have a very big need for this so need to get something going here quickly that I can start leveraging.
You bring up a very good point... I don't think it would be too difficult to add it but certainly could be something that could follow. I was thinking through that while I was working on this last night and I was thinking about some type of logical grouping. The problem I foresee here will be dealing with the policy plugin as now we're talking about nesting and that'll certainly have some various changes. This may be better to do in a future release, I like the idea of it but the implementation of it can make things far more challenging. What I think could be difficult here is that if you do this inside of a model, I believe the expected return should more than likely then be either the model or a value within a particular record.
So this is where it gets really interesting... at this point I feel like it leads to instance based procs where you could say do something like: Alternatively, it could be possible to allow more freehand organization of the top level to basically achieve the same thing: const db = new ZenStackClient(schema, {
...,
procedures: {
async foo(client, args) {
// ..
},
bar: {
async baz(client, args) {
// ...
},
},
},
});Which achieves a similar design as to what you mention but allows for categorization without purely tying it to the model and is safer in that respect to be able to organize it without other potential issues. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Hi @mwillbanks , this is truly great work done at an amazing pace! I haven't thoroughly gone through all details yet, but it overall looks very solid. Here are my thoughts about your RFC. In fact, I feel what you implemented is already very good as the first release of this feature. Access ControlMy original thought was that procedures don't involve a new kind of access control. Instead, if a proc calls into the ORM client (in most cases, it should), it's automatically governed by the underlying CRUD rules, provided the call is initiated with a policy-enabled ORM client. Conceptually, it's equivalent to a Postgres stored procedure + RLS. The additional challenge with "execute" policy kind, if we were to introduce it, is that since procs are not bound to specific models, the policy rules can pretty much only access GroupingI do share @sanny-io 's concern that the TS API can become cluttered when there are many procs. However, I also feel the logical grouping may not necessarily align with model boundaries, since procs don't have a natural model context. If we were to introduce grouping just for organization, maybe we could simply do it with naming? mutate procedure user.signUp(email: String) { ... }And at TS level (ORM and hooks), it's mapped to QaaS VisibilityZModel is pretty flexible for expressing many different aspects of an app, but I'm worried putting too many things into it will result in a bloated, hard-to-read model. My principle so far is that, hopefully, everything in it has an effect on every layer of the stack: ORM -> QaaS -> Hooks. This makes me feel proc's visibility to QaaS should be configured at the QaaS layer (as it has no effect to the ORM). We need more control flexibility there anyway, but details should be for a separate RFC. Basically, something like the following? API Handler: const handler = new RPCApiHandler({
...,
visibility: {
models: {
User: true, Post: { delete: false, deleteMany: false }, Comment: false
},
procs: {
'user.signUp': false
}
});Hooks: // client hooks creation use the same `visibility` settings to control its typing
const client = useClientQueries(schema, { ..., visibility: { ... } });Proc Client APIThis is not from the RFC but some change I'd like to propose. Instead of calling the await db.$procs.signUp({ args: { email } });The accidental benefit is that we don't need to introduce Looking forward to hearing your thoughts. |
- Switch procedure calls to `db.$procs.name({ args: {...} })` (no positional args)
- Remove legacy `$procedures` alias entirely (client API + server routing/logging)
- Validate procedure envelope input (`args` object, required/unknown keys)
- Keep TanStack Query procedure hooks as `(args, options)` (with conditional args optionality)
- Update server/ORM/client tests for the envelope API
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive support for custom procedures across the ZenStack platform, enabling developers to define type-safe, reusable server-side operations beyond standard CRUD. The implementation includes schema language extensions, ORM client integration, API handlers, and TanStack Query hooks.
Key Changes
- Extended the schema language to support procedure declarations with parameters and return types (including arrays)
- Implemented procedure invocation in the ORM client with validation, transaction support for mutations, and plugin hooks
- Added REST and RPC API endpoints for procedures with SuperJSON serialization and proper HTTP method enforcement
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/language/src/zmodel.langium | Grammar updates to support procedure syntax with optional mutation keyword |
| packages/language/src/validators/procedure-validator.ts | New validator for procedure declarations including reserved name checks |
| packages/schema/src/schema.ts | Updated ProcedureDef type to support array parameters and return types |
| packages/orm/src/client/contract.ts | Type definitions for procedure functions and envelopes |
| packages/orm/src/client/client-impl.ts | Core procedure invocation logic with validation and transaction handling |
| packages/orm/src/client/plugin.ts | New onProcedure hook for plugin integration |
| packages/orm/src/client/crud/validator/index.ts | Input validation for procedure arguments |
| packages/server/src/api/common/procedures.ts | Shared utilities for argument mapping and validation |
| packages/server/src/api/rest/index.ts | REST API handler for procedure endpoints |
| packages/server/src/api/rpc/index.ts | RPC API handler for procedure endpoints |
| packages/clients/tanstack-query/src/react.ts | React hooks for procedures (query and mutation) |
| packages/clients/tanstack-query/src/vue.ts | Vue hooks for procedures |
| packages/clients/tanstack-query/src/svelte/index.svelte.ts | Svelte hooks for procedures |
| tests/e2e/orm/schemas/procedures/* | Test schema and generated types |
| packages/server/test/api/*.test.ts | Comprehensive test coverage for procedure APIs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/clients/tanstack-query/src/react.ts (2)
438-452: Minor: Inconsistent indentation in parameter list.The
endpointModelparameter has extra indentation compared to other parameters.🔎 Proposed fix
export function useInternalProcedureInfiniteQuery<TQueryFnData, TData>( schema: SchemaDef, - endpointModel: '$procs', + endpointModel: '$procs', procedure: string, args: unknown,
454-465: Minor: Inconsistent indentation in parameter list.Same indentation issue as the infinite query hook.
🔎 Proposed fix
export function useInternalProcedureSuspenseInfiniteQuery<TQueryFnData, TData>( schema: SchemaDef, - endpointModel: '$procs', + endpointModel: '$procs', procedure: string, args: unknown,packages/clients/tanstack-query/src/svelte/index.svelte.ts (1)
276-282: Consider extracting duplicatemergehelper.The
mergehelper function is duplicated in bothuseClientQueries(lines 276-282) anduseModelQueries(lines 384-390). Consider extracting it to a shared location to reduce duplication.packages/clients/tanstack-query/src/vue.ts (2)
288-294: Samemergehelper duplication as Svelte.The
mergehelper is duplicated inuseClientQueries(lines 288-294) anduseModelQueries(lines 407-411). Consider extracting to a shared utility.
376-376: Reactive options not fully unwrapped in mutation hook.Unlike
useInternalMutation(line 598-602) which wraps the final options in acomputed()to maintain reactivity,useInternalProcedureMutationonly callstoValue(options)once at hook creation time. This means changes tooptionswon't be reactive.Consider wrapping in
computed()for consistency:🔎 Proposed fix
export function useInternalProcedureMutation<TArgs, R = any>( _schema: SchemaDef, endpointModel: '$procs', procedure: string, options?: MaybeRefOrGetter<Omit<UnwrapRef<UseMutationOptions<R, DefaultError, TArgs>>, 'mutationFn'> & QueryContext>, ) { - const { endpoint, fetch } = useFetchOptions(toValue(options)); const mutationFn = (data: any) => { + const { endpoint, fetch } = useFetchOptions(toValue(options)); const reqUrl = makeUrl(endpoint, endpointModel, procedure); const fetchInit: RequestInit = { method: 'POST', headers: { 'content-type': 'application/json', }, body: marshal(data), }; return fetcher<R>(reqUrl, fetchInit, fetch) as Promise<R>; }; - return useMutation({ ...(toValue(options) as any), mutationFn }); + const finalOptions = computed(() => ({ + ...(toValue(options) as any), + mutationFn, + })); + return useMutation(finalOptions); }packages/orm/src/client/contract.ts (1)
300-309: Consider consolidatingProcedureFuncandProcedureHandlerFunc.Both types have identical signatures. If they serve the same purpose, consider keeping only one to reduce API surface complexity. If they differ semantically (e.g., one for client invocation, one for handler registration), consider adding JSDoc comments to clarify the distinction.
packages/server/src/api/rpc/index.ts (1)
274-274: Consider handling missing$procsgracefully.The optional chaining
client.$procs?.[proc]will returnundefinedif$procsdoesn't exist, which would then fail silently or throw a less informative error when called.Consider adding an explicit check before invocation to provide a clearer error message if procedures are not configured on the client.
Proposed improvement
try { log(this.options.log, 'debug', () => `handling "$procs.${proc}" request`); - const clientResult = await (client as any).$procs?.[proc](procInput); + const procFn = (client as any).$procs?.[proc]; + if (!procFn) { + return this.makeBadInputErrorResponse(`procedure "${proc}" is not available on the client`); + } + const clientResult = await procFn(procInput);packages/server/src/api/rest/index.ts (1)
577-577: Consider adding defensive check for missing $procs.Similar to the RPC handler, the direct access
client.$procs[proc]could fail if procedures aren't configured. Consider adding an explicit check.Proposed improvement
- const clientResult = await (client as any).$procs[proc](procInput); + const procFn = (client as any).$procs?.[proc]; + if (!procFn) { + return this.makeProcBadInputErrorResponse(`procedure "${proc}" is not available on the client`); + } + const clientResult = await procFn(procInput);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/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-typing-test.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/clients/tanstack-query/test/vue-typing-test.tspackages/orm/src/client/client-impl.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/options.tspackages/orm/src/client/plugin.tspackages/orm/test/procedures.test.tspackages/server/src/api/common/procedures.tspackages/server/src/api/rest/index.tspackages/server/src/api/rpc/index.tspackages/server/test/api/procedures.e2e.test.tspackages/server/test/api/rest.test.tspackages/server/test/api/rpc.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/clients/tanstack-query/test/react-typing-test.ts
- packages/server/test/api/procedures.e2e.test.ts
- packages/server/test/api/rest.test.ts
- packages/clients/tanstack-query/test/svelte-typing-test.ts
- packages/orm/src/client/crud/validator/index.ts
- packages/clients/tanstack-query/test/vue-typing-test.ts
- packages/orm/src/client/client-impl.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/plugin.tspackages/orm/test/procedures.test.tspackages/orm/src/client/options.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/src/react.tspackages/clients/tanstack-query/src/svelte/index.svelte.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/orm/src/client/plugin.tspackages/orm/src/client/options.tspackages/clients/tanstack-query/src/vue.tspackages/server/src/api/common/procedures.tspackages/clients/tanstack-query/src/react.tspackages/orm/src/client/contract.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/server/src/api/rpc/index.tspackages/server/src/api/rest/index.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/orm/test/procedures.test.tspackages/orm/src/client/contract.tspackages/server/test/api/rpc.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/test/procedures.test.tspackages/orm/src/client/options.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/src/react.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/cli/**/*.test.{ts,tsx} : CLI package tests should focus on action-specific tests for each command
Applied to files:
packages/orm/test/procedures.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/orm/test/procedures.test.tspackages/server/test/api/rpc.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/orm/src/client/contract.ts
🧬 Code graph analysis (7)
packages/orm/src/client/plugin.ts (2)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/contract.ts (1)
ClientContract(65-199)
packages/orm/test/procedures.test.ts (4)
packages/orm/src/client/client-impl.ts (1)
ZenStackClient(46-52)packages/orm/src/client/index.ts (1)
ZenStackClient(1-1)packages/orm/src/dialects/sqlite.ts (1)
SqliteDialect(1-1)packages/orm/src/client/plugin.ts (1)
definePlugin(58-60)
packages/orm/src/client/options.ts (1)
packages/orm/src/client/contract.ts (1)
ProcedureHandlerFunc(307-309)
packages/clients/tanstack-query/src/vue.ts (4)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/contract.ts (1)
ProcedureFunc(300-302)packages/clients/tanstack-query/src/common/types.ts (2)
QueryContext(9-24)ExtraQueryOptions(29-34)packages/clients/client-helpers/src/fetch.ts (2)
marshal(86-93)fetcher(15-37)
packages/server/src/api/common/procedures.ts (2)
packages/server/src/api/rest/index.ts (3)
schema(285-287)unmarshalQ(602-604)mapProcedureArgs(595-600)packages/schema/src/schema.ts (2)
SchemaDef(11-19)ProcedureDef(82-87)
packages/server/test/api/rpc.test.ts (2)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)packages/server/src/api/rpc/index.ts (1)
RPCApiHandler(36-377)
packages/server/src/api/rpc/index.ts (4)
packages/server/src/api/common/procedures.ts (3)
getProcedureDef(8-14)processSuperJsonRequestPayload(44-66)validateProcedureArgs(143-155)packages/orm/src/client/errors.ts (1)
ORMError(66-104)packages/server/src/api/utils.ts (1)
log(9-27)packages/common-helpers/src/safe-json-stringify.ts (1)
safeJSONStringify(4-12)
⏰ 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). (2)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
🔇 Additional comments (27)
packages/clients/tanstack-query/src/react.ts (5)
65-88: LGTM! Type utilities are well-designed.The procedure type utilities correctly extract and normalize procedure arguments and return types from the schema. The
NormalizeProcedurePayloadtype handles empty args, single arg, and multi-arg cases appropriately, andProcedureHookFncorrectly makes args optional whenundefined extends Payload.
162-209: LGTM! Procedure hooks types are comprehensive.The
ProcedureHookGroupcorrectly differentiates between mutation and non-mutation procedures, providing appropriate hook signatures for each. The conditional extension ofClientHookswithProcedureHooksensures the$procsnamespace only appears when procedures exist in the schema.
351-390: LGTM! Runtime procedure hook wiring is correct.The
buildProcedureHookshelper correctly iterates over schema procedures and wires each to the appropriate hook type based on themutationflag. Options are properly merged with root options.
392-413: Procedure query hooks hardcodeoptimisticUpdate: false- verify this is intentional.The procedure query hooks set
optimisticUpdate: falsein the query key options, unlike model queries which respect theoptions?.optimisticUpdate !== falsepattern. This means procedure queries won't participate in optimistic update invalidation.If this is intentional (procedures may have side effects that make optimistic updates unsafe), consider adding a brief comment explaining the design decision.
467-487: LGTM! Mutation now correctly sends data in request body.The mutation hook properly constructs the request with the payload in the body using
marshal(data)and sets the appropriatecontent-typeheader. This addresses the previous review concern about data being sent in URL query parameters.packages/clients/tanstack-query/src/svelte/index.svelte.ts (3)
66-89: LGTM! Type utilities consistent with React implementation.The Svelte procedure type utilities mirror the React implementation correctly, with the appropriate Svelte-specific
Accessorwrapper for theProcedureHookFntype.
347-367: LGTM! Svelte mutation hook correctly sends data in body.The mutation implementation properly marshals data to the request body with appropriate headers, consistent with the React implementation.
153-186: Svelte procedure hooks intentionally omit suspense variants—this is expected.@tanstack/svelte-query does not provide suspense hooks like
useSuspenseQueryoruseSuspenseInfiniteQuery. Suspense is a React-specific feature in TanStack Query; Svelte uses different query primitives (createQuery / store-based APIs) that don't integrate with React's Suspense API. The React implementation's inclusion of suspense variants is therefore not relevant to parity with the Svelte implementation.packages/clients/tanstack-query/src/vue.ts (3)
65-88: LGTM! Type utilities consistent across frameworks.The Vue procedure type utilities correctly use
MaybeRefOrGetterfor the reactive wrapper pattern, maintaining consistency with the React and Svelte implementations.
154-196: Vue procedure hooks also missing suspense variants - same consideration as Svelte.The Vue
ProcedureHookGrouponly providesuseQueryanduseInfiniteQueryfor non-mutation procedures, omitting suspense variants. Verify if@tanstack/vue-querysupports suspense hooks and whether they should be included for API parity with React.
370-390: LGTM! Vue mutation hook correctly sends data in body.The implementation properly marshals data to the request body with
marshal(data)and sets thecontent-typeheader, consistent with React and Svelte implementations.packages/orm/src/client/contract.ts (3)
217-224: LGTM - Extended type map covers procedure return types.The additions to
_TypeMapproperly support all ZModel primitive types that procedures can return, includingVoidandUndefinedfor procedures with no meaningful return value.
227-240: LGTM - Clean enum and type resolution.The
EnumValuetype correctly extracts enum member values, and the updatedMapTypeproperly chains resolution for models, typedefs, and enums.
254-298: Well-designed envelope typing with proper optionality handling.The conditional types correctly distinguish between procedures with required vs optional parameters, making the
argskey required only when there are required parameters. The pattern allows callingdb.$procs.noArgProc()ordb.$procs.noArgProc({ args: {} })interchangeably.packages/orm/src/client/options.ts (1)
134-143: LGTM - Handler type alignment.The switch to
ProcedureHandlerFuncproperly types procedure handlers that receive the client as the first parameter viaPrependParameter.packages/server/test/api/rpc.test.ts (2)
128-213: Comprehensive procedure RPC tests.Good coverage of:
- HTTP method enforcement (GET for queries, POST for mutations)
- Argument passing via query string and request body
- Falsy/undefined return value serialization with SuperJSON metadata
The test for
getUndefinedcorrectly verifies thatundefinedis serialized tonullwith SuperJSON metadata present.
215-338: Thorough edge case coverage.The tests properly validate:
- Multi-parameter procedures with object mapping
- All-optional parameters allowing omitted payloads
- Array-typed parameters
- Enum and typedef (object) parameters
- Input type validation (422 status)
- Unknown argument rejection (400 status)
- Invalid args payload structure (400 status)
packages/orm/src/client/plugin.ts (1)
64-98: Well-designed procedure interception hook.The
OnProcedureHookContextprovides all necessary context for plugins to:
- Identify the procedure (
name,mutation)- Access and mutate input before proceeding
- Control execution flow via
proceed- Access the client for additional operations
This mirrors the
OnQueryHookContextpattern, maintaining API consistency.packages/orm/test/procedures.test.ts (1)
139-158: Good transaction behavior verification.The test correctly validates that:
- Query procedures (
qry) execute outside transactions (isTransaction === false)- Mutation procedures (
mut) are automatically wrapped in transactions (isTransaction === true)This aligns with the PR objective to provide default transactional wrapping for mutations.
packages/server/src/api/rpc/index.ts (2)
56-64: Clean routing for procedure requests.The
$procspath prefix is correctly detected and delegated to the specialized handler, keeping the CRUD and procedure flows separated.
180-292: Robust procedure request handling.The implementation correctly:
- Validates procedure existence via
getProcedureDef- Enforces HTTP method constraints (POST for mutations, GET for queries)
- Supports argument passing via both request body and query string
- Processes SuperJSON payloads when metadata is present
- Maps and validates arguments before invocation
- Serializes responses with SuperJSON for type preservation
packages/server/src/api/common/procedures.ts (3)
68-141: Thorough argument mapping with helpful error messages.The
mapProcedureArgsfunction handles multiple scenarios well:
- Zero-parameter procedures (lines 73-92)
- All-optional parameter procedures (lines 95-97)
- Missing/invalid payload structures
- Unknown argument rejection (lines 124-128)
- Required parameter validation (lines 131-138)
The error messages are clear and actionable for API consumers.
143-155: InputValidator integration respects client options.Good defensive check for
validateInput === falsebefore running validation, allowing users to bypass validation when needed.
157-159: Verify thatisOrmErroris used externally before keeping this export.The
isOrmErrortype guard is exported but its usage across the codebase should be verified. If it's not imported or used in other files, consider removing it to reduce the public API surface. Run a codebase search for all references toisOrmErrorto confirm its necessity.packages/server/src/api/rest/index.ts (3)
346-349: Clean procedure routing in REST handler.The
/$procs/path prefix detection appropriately routes procedure requests to the specialized handler, maintaining separation from JSON:API resource routes.
488-593: Consistent procedure handling with REST conventions.The
processProcedureRequestmethod mirrors the RPC implementation while integrating with REST-specific:
- Error response format (JSON:API style via
makeError)- Logging patterns
- Response structure
This maintains a consistent developer experience across both API styles.
619-653: Well-structured ORM error mapping for procedures.The
makeProcORMErrorResponsemethod properly maps ORM error reasons to appropriate HTTP status codes and includes relevant context (model,rejectReason,dbErrorCode) in the response body.Good use of
paramCasefor consistent error reason formatting.
ymc9
left a comment
ymc9
left a comment
There was a problem hiding this comment.
I've finished reviewing the changes to "packages/language", "packages/schema", "packages/sdk", and "packages/orm". Please find my comments attached. Thanks!
Will complete the review tomorrow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/language/src/zmodel.langium (1)
269-269: Verify whetherUndefinedtype is needed.A previous reviewer (ymc9) asked whether
Undefinedshould be removed if it's not needed. TheVoidtype makes sense for procedures that don't return a value, butUndefinedseems redundant. Could you clarify the use case forUndefinedas a distinct expression type?packages/orm/src/client/client-impl.ts (1)
217-222: Consider usingObject.create(null)for the accumulator.While
Object.keys()only returns own enumerable properties (not inherited ones), usingObject.create(null)for the accumulator provides additional safety against prototype pollution when building the procedures map:🔎 Suggested improvement
get $procs() { - return Object.keys(this.$schema.procedures ?? {}).reduce((acc, name) => { + return Object.keys(this.$schema.procedures ?? {}).reduce((acc, name) => { acc[name] = (input?: unknown) => this.handleProc(name, input); return acc; - }, {} as any); + }, Object.create(null) as any); }packages/server/src/api/rest/index.ts (1)
574-592: Consider explicit procedure existence check and avoid broad type assertion.Two concerns:
Type assertion: Using
(client as any)at line 577 bypasses TypeScript's type safety. Consider using a more specific type assertion or extending the ClientContract type to include$procsif it's not already present.Defensive check: While optional chaining
$procs?.[proc]handles undefined gracefully, the subsequent call could throw a generic error if the procedure doesn't exist at runtime. Consider an explicit check to provide a clearer error message.Based on learnings, avoid broad type assertions in production code.
🔎 Suggested improvements
try { log(this.log, 'debug', () => `handling "$procs.${proc}" request`); - const clientResult = await (client as any).$procs?.[proc](procInput); + const procs = (client as any).$procs; + const procFn = procs?.[proc]; + if (!procFn || typeof procFn !== 'function') { + return this.makeProcBadInputErrorResponse(`unknown procedure: ${proc}`); + } + + const clientResult = await procFn(procInput); const { json, meta } = SuperJSON.serialize(clientResult);For the type assertion issue, verify if
$procsshould be part of the ClientContract type definition:#!/bin/bash # Search for ClientContract type definition to check if $procs is included ast-grep --pattern $'type ClientContract<$$$> = $$$' # Also search for where $procs is added to the client rg -n --type=ts -C3 '\$procs'packages/orm/test/procedures.test.ts (1)
87-137: LGTM: Plugin execution order test is correct.The test correctly validates that:
- Plugins execute in reverse order of registration (line 136:
['p2:add', 'p1:add']for registration[p1, p2])- Plugins can mutate procedure inputs (p2 adds 1 to parameter
a)- Mutations propagate correctly through the chain (final result:
(1+1) + 2 = 4)This aligns with the middleware-style wrapping pattern where the last-registered plugin becomes the outermost wrapper and executes first.
Note: A previous review comment flagged that this execution order behavior should be documented in
packages/orm/src/client/client-impl.ts(lines 256-269) for future maintainers.
🧹 Nitpick comments (3)
packages/server/src/api/rest/index.ts (1)
521-546: Refactor duplicated query parameter extraction.The logic for extracting and unmarshaling the 'q' query parameter is duplicated between the POST (lines 527-534) and GET (lines 538-545) branches. Consider extracting this into a helper method to reduce duplication and improve maintainability.
🔎 Suggested refactor
+ private extractQueryArgs(query?: Record<string, string | string[]>): unknown { + return query?.['q'] + ? this.unmarshalQ(query['q'] as string, query['meta'] as string | undefined) + : undefined; + } + private async processProcedureRequest({ // ... existing code ... let argsPayload: unknown; if (method === 'POST') { if (typeof requestBody !== 'undefined') { argsPayload = requestBody; } else { try { - argsPayload = query?.['q'] - ? this.unmarshalQ(query['q'] as string, query['meta'] as string | undefined) - : undefined; + argsPayload = this.extractQueryArgs(query); } catch (err) { return this.makeProcBadInputErrorResponse( err instanceof Error ? err.message : 'invalid "q" query parameter', ); } } } else { try { - argsPayload = query?.['q'] - ? this.unmarshalQ(query['q'] as string, query['meta'] as string | undefined) - : undefined; + argsPayload = this.extractQueryArgs(query); } catch (err) { return this.makeProcBadInputErrorResponse( err instanceof Error ? err.message : 'invalid "q" query parameter', ); } }packages/orm/test/procedures.test.ts (2)
66-85: Add a success case for procedure argument validation.The test correctly validates that invalid arguments are rejected (passing a string when an Int is expected). However, it only tests the error path. Add a companion assertion or test case that verifies valid arguments pass validation and execute successfully, e.g.,
await expect(client.$procs.echoInt({ args: { n: 1 } })).resolves.toBe(1).🔎 Suggested addition
}); await expect(client.$procs.echoInt({ args: { n: '1' } })).rejects.toThrow(/invalid input/i); + await expect(client.$procs.echoInt({ args: { n: 1 } })).resolves.toBe(1); });
1-159: Consider additional test coverage for RFC requirements.The current test suite provides solid coverage of core procedure functionality: exposure, error handling, validation, plugin hooks, and transactions. However, based on the RFC outlined in PR comments and the learning that "ORM package tests should include comprehensive client API tests and policy tests," consider adding tests for:
- Policy integration: Test procedure execution with policy-enabled clients and the "execute" policy scope mentioned in the RFC
- Grouped procedures: Validate nested organization patterns discussed in PR comments (e.g.,
db.$procs.user.signUp)- Return type validation: Currently only input validation is tested; verify return values are validated against schema
- Complex argument types: Test procedures with nested objects, arrays, or enum parameters
- Error propagation: Verify that errors thrown in handlers are properly wrapped/reported
Note: Some of these may be covered in other test files (e.g.,
packages/server/test/api/procedures.e2e.test.ts), but ORM-level policy and validation tests would strengthen this suite.Based on learnings, as per coding guidelines: ORM package tests should include comprehensive client API tests and policy tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/language/src/generated/ast.tsis excluded by!**/generated/**packages/language/src/generated/grammar.tsis excluded by!**/generated/**
📒 Files selected for processing (12)
packages/language/src/validators/procedure-validator.tspackages/language/src/zmodel.langiumpackages/orm/src/client/client-impl.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/options.tspackages/orm/src/client/plugin.tspackages/orm/test/procedures.test.tspackages/server/src/api/rest/index.tspackages/server/test/api/procedures.e2e.test.tspackages/server/test/api/rest.test.tspackages/server/test/api/rpc.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/orm/src/client/crud/validator/index.ts
- packages/server/test/api/rpc.test.ts
- packages/language/src/validators/procedure-validator.ts
- packages/server/test/api/procedures.e2e.test.ts
- packages/orm/src/client/options.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/plugin.tspackages/orm/test/procedures.test.tspackages/orm/src/client/client-impl.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/orm/src/client/plugin.tspackages/server/src/api/rest/index.tspackages/orm/src/client/client-impl.tspackages/orm/src/client/contract.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/server/test/api/rest.test.tspackages/orm/test/procedures.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/server/test/api/rest.test.tspackages/orm/test/procedures.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
packages/server/test/api/rest.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/test/procedures.test.tspackages/orm/src/client/contract.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/cli/**/*.test.{ts,tsx} : CLI package tests should focus on action-specific tests for each command
Applied to files:
packages/orm/test/procedures.test.ts
🧬 Code graph analysis (4)
packages/orm/src/client/plugin.ts (2)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/contract.ts (1)
ClientContract(65-199)
packages/server/test/api/rest.test.ts (2)
packages/server/src/api/rest/index.ts (2)
schema(285-287)RestApiHandler(126-2254)packages/orm/src/client/contract.ts (2)
ClientContract(65-199)Procedures(242-252)
packages/orm/test/procedures.test.ts (2)
packages/orm/src/client/client-impl.ts (1)
ZenStackClient(46-52)packages/orm/src/client/plugin.ts (1)
definePlugin(58-60)
packages/server/src/api/rest/index.ts (7)
packages/orm/src/client/contract.ts (1)
ClientContract(65-199)packages/server/src/types.ts (1)
Response(52-62)packages/server/src/api/common/procedures.ts (3)
getProcedureDef(8-14)processSuperJsonRequestPayload(44-66)validateProcedureArgs(143-155)packages/orm/src/client/errors.ts (1)
ORMError(66-104)packages/server/src/api/rpc/index.ts (1)
log(43-45)packages/server/src/api/utils.ts (1)
log(9-27)packages/common-helpers/src/param-case.ts (1)
paramCase(5-22)
⏰ 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). (2)
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (19)
packages/orm/src/client/plugin.ts (2)
39-42: LGTM! The newonProcedurehook follows existing plugin patterns.The hook integrates cleanly with the existing plugin architecture, mirroring the structure of
onQueryandonKyselyQueryhooks.
64-96: Well-structured hook context type with clear documentation.The
OnProcedureHookContexttype provides all necessary fields for procedure interception: procedure name, mutation flag, input envelope, proceed callback, and client reference. This aligns with the plugin hook patterns established for query interception. Based on learnings, this correctly implements plugin hooks at the ORM level for procedure interception.packages/language/src/zmodel.langium (1)
229-229: Good change: mutation flag is now optional for procedures.This allows both query procedures (
procedure foo(): String) and mutation procedures (mutation procedure foo(): String), providing cleaner syntax for read-only procedures.packages/orm/src/client/contract.ts (3)
242-252: Good design:$procswith envelope-based invocation.The rename from
$proceduresto$procsand the envelope-based call signature ({ args: {...} }) aligns with the RFC discussion in the PR comments. This provides a consistent and extensible API surface.
254-298: Well-designed procedure parameter typing with envelope support.The type utilities correctly handle:
- Required vs optional parameter detection
- Envelope optionality based on parameter requirements
- Proper type mapping for procedure arguments
The conditional types ensure type safety while maintaining good DX.
309-318:MaybePromisereturn type addresses flexibility concern.The
ProcedureFuncandProcedureHandlerFuncnow returnMaybePromise<...>, allowing handlers to be synchronous or asynchronous. This addresses the previous review feedback from ymc9.packages/orm/src/client/client-impl.ts (2)
224-245: Good validation flow: schema check followed by input validation.The implementation correctly:
- Validates procedures are configured on the client
- Verifies the procedure exists in the schema
- Validates handler is configured
- Runs input validation via
InputValidatorThis provides clear error messages at each failure point.
249-282: Well-structured plugin pipeline and transaction support.The implementation correctly:
- Chains
onProcedurehooks in plugin order- Provides mutation metadata to plugins
- Wraps mutation procedures in transactions when not already inside one
- Extracts args from the envelope format
This aligns with the existing query plugin architecture. Based on learnings, this properly implements plugin hooks at the ORM level for procedure interception.
packages/server/test/api/rest.test.ts (4)
3167-3192: Comprehensive procedure schema for testing.The test schema covers a good variety of procedure types:
- Scalar types (Int, String, Decimal)
- Optional parameters
- Array parameters
- Enum parameters
- TypeDef/object parameters
- Mutation procedures
This provides good coverage for the REST procedures feature.
3285-3332: Good test coverage for optional and required parameter handling.Tests correctly validate:
- SuperJSON serialization for complex types (Decimal)
- Optional args allowing omission of envelope
- Required args enforcing presence of arguments
The error messages are specific and helpful for debugging.
3382-3399: Type validation error test ensures proper input coercion.This test verifies that passing incorrect types (string instead of Int) returns a 422 validation error, ensuring the input validation pipeline works correctly for procedures.
3418-3456: Good error scenario coverage for malformed payloads.Tests verify:
- Array as
argsvalue (should be object) → 400 error- Unknown argument keys → 400 error with descriptive message
This ensures the API provides clear feedback for invalid procedure calls.
packages/server/src/api/rest/index.ts (4)
346-349: Clean procedure routing logic.The path-based routing for procedures is straightforward and delegates appropriately to the dedicated handler.
595-604: Good delegation to shared utilities.The helper methods appropriately delegate to common procedure utilities, promoting code reuse across REST and RPC handlers.
606-653: Excellent error handling and response builders.The error response builders provide comprehensive coverage:
- Clear separation of input errors (400), generic errors (500), and ORM-specific errors
- Pattern matching elegantly maps
ORMErrorReasonto appropriate HTTP status codes- Proper logging of error responses aids debugging
- Structured error messages with metadata (reason, model, rejection details)
548-556: Concern already addressed in the implementation.The
processSuperJsonRequestPayloadfunction correctly checks formeta?.serializationbefore attempting SuperJSON deserialization, not just the presence of the 'meta' property. When a payload contains a 'meta' property without a 'serialization' key, the function safely drops it (as intended) rather than misinterpreting it as SuperJSON format. False positives are prevented by the specific structure validation.packages/orm/test/procedures.test.ts (3)
35-64: LGTM: Error handling tests cover key failure modes.The tests properly validate two distinct error scenarios:
- Procedures not configured at all (lines 35-48)
- Specific handler missing from configuration (lines 50-64)
The error messages are appropriately descriptive and assertions use case-insensitive matching for robustness.
139-158: The test correctly validates that mutation procedures run in transactions by default while non-mutation procedures don't. Theclient.isTransactionproperty (lines 151-152) is a stable public getter that delegates to Kysely's transaction state and is documented through inline comments in the implementation (see client-impl.ts:276-277). The test properly verifies the documented transactional behavior.
16-33: Handler signatures must matchProcedureHandlerFunctype contract with context object.Based on the type definitions in
contract.ts, all procedure handlers must receive a context objectctx: ProcedureHandlerCtx<Schema, Params>containing at least{ client: ClientContract<Schema> }. However, this test violates that contract:
- Lines 16-33: Handler
async () => 'ok'takes no parameters but should receive context object withclientproperty to matchProcedureHandlerFuncsignature.- Lines 139-158: Handlers destructure
{ client }from context but the invocations (client.$procs.qry(),client.$procs.mut()) don't pass an envelope. For consistency, either update handlers to not destructure context, or pass the envelope object in invocations.Ensure all procedure handlers conform to the enforced type contract:
(ctx: { client: ClientContract<Schema>, args?: ... }) => MaybePromise<Return>.⛔ Skipped due to learnings
Learnt from: CR Repo: zenstackhq/zenstack-v3 PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T01:55:04.540Z Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy testsLearnt from: CR Repo: zenstackhq/zenstack-v3 PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T01:55:04.540Z Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
ymc9
left a comment
ymc9
left a comment
There was a problem hiding this comment.
Hi @mwillbanks , I've finished reviewing the remaining code. Please take a look at the new comments. Thanks!
|
@ymc9 i've updated the code based on your review and updated relevant tests. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/server/test/api/rest.test.ts (1)
3285-3461: Excellent test coverage with one suggested addition.The test cases comprehensively cover:
- SuperJSON serialization/deserialization
- Optional and required arguments
- Array, enum, and custom type parameters
- Error scenarios (invalid types, unknown arguments, missing args)
- Both GET query and POST mutation procedures
Consider adding test cases for:
- Procedure invocation with policy enforcement (if execute policies are supported)
- Context propagation to procedure handlers
- Nested/complex typedef parameters
These would provide additional confidence in the procedure execution flow.
packages/server/src/api/common/utils.ts (1)
7-29: Simplify explicit undefined type annotations.The explicit type annotations
error: undefined as string | undefinedon lines 9, 17, and 28 are redundant. TypeScript can infer the union type from the context.Proposed simplification
export async function processSuperJsonRequestPayload(payload: unknown) { if (!payload || typeof payload !== 'object' || Array.isArray(payload) || !('meta' in (payload as any))) { - return { result: payload, error: undefined as string | undefined }; + return { result: payload, error: undefined }; } const { meta, ...rest } = payload as any; if (meta?.serialization) { try { return { result: SuperJSON.deserialize({ json: rest, meta: meta.serialization }), - error: undefined as string | undefined, + error: undefined, }; } catch (err) { return { result: undefined, error: `failed to deserialize request payload: ${(err as Error).message}`, }; } } // drop meta when no serialization info is present - return { result: rest, error: undefined as string | undefined }; + return { result: rest, error: undefined }; }The union type will be correctly inferred by TypeScript from the different return paths.
packages/server/src/api/rpc/index.ts (1)
157-157: Remove trailing whitespace.Line 157 contains trailing whitespace.
const clientResult = await (client as any)[model][op](processedArgs); let responseBody: any = { data: clientResult }; - + // superjson serialize responsepackages/clients/tanstack-query/src/react.ts (2)
145-181: Consider whether all query variants are necessary for procedures.Procedures expose four query hook variants (useQuery, useSuspenseQuery, useInfiniteQuery, useSuspenseInfiniteQuery) mirroring model hooks. However, procedures are custom operations with potentially different semantics than standard CRUD queries:
- Suspense variants may not align with typical procedure usage patterns
- Infinite queries might be useful for paginated custom queries (per past discussion), but suspense + infinite is a niche combination
Consider starting with just
useQueryanduseInfiniteQueryfor non-mutation procedures, adding suspense variants only if user demand materializes. This reduces initial API surface and avoids committing to patterns that may not fit procedure semantics.[scratchpad_end] -->
332-332: Unsafe type assertion when accessing procedures.The code uses
(schema as any).procedureswhich bypasses type safety. While there's a runtime check for undefined, this pattern could mask type errors.🔎 Safer approach
- const procedures = (schema as any).procedures as Record<string, { mutation?: boolean }> | undefined; + const procedures = 'procedures' in schema ? schema.procedures : undefined;This leverages TypeScript's type narrowing via the
inoperator to safely access the optional property.packages/server/src/api/rest/index.ts (2)
539-539: Optional chaining may be redundant after validation.Line 539 uses optional chaining for
client.$procs?.[proc], but the procedure has already been validated to exist in the schema (line 505-508). Ifclient.$procsis guaranteed to exist whenschema.proceduresexists, the optional chaining could be replaced with a more assertive access pattern.However, if there's a possibility of schema-client mismatch (e.g., outdated client), the defensive check is appropriate. Consider adding a runtime assertion if
$procsis missing to surface configuration issues early:🔎 More explicit error handling
try { log(this.log, 'debug', () => `handling "$procs.${proc}" request`); - const clientResult = await (client as any).$procs?.[proc](procInput); + const procs = (client as any).$procs; + if (!procs) { + throw new Error('Client does not support procedures - schema/client version mismatch'); + } + const clientResult = await procs[proc](procInput);
558-563: Consider inlining trivial delegation.The
mapProcedureArgsmethod simply delegates tomapProcedureArgsCommonwithout any additional logic. Unless you anticipate REST-specific argument mapping in the future, this could be inlined at the call site (line 531):procInput = mapProcedureArgsCommon(procDef, processedArgsPayload);This reduces indirection and makes the code flow clearer. If REST-specific mapping is planned, a comment explaining the future intent would be helpful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/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/orm/src/client/client-impl.tspackages/orm/test/procedures.test.tspackages/server/src/api/common/procedures.tspackages/server/src/api/common/utils.tspackages/server/src/api/rest/index.tspackages/server/src/api/rpc/index.tspackages/server/test/api/procedures.e2e.test.tspackages/server/test/api/rest.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/server/test/api/procedures.e2e.test.ts
- packages/orm/src/client/client-impl.ts
- packages/orm/test/procedures.test.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/src/common/types.tspackages/clients/tanstack-query/src/react.tspackages/clients/tanstack-query/src/svelte/index.svelte.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/src/common/types.tspackages/clients/tanstack-query/src/react.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/clients/tanstack-query/src/vue.tspackages/server/src/api/common/utils.tspackages/clients/tanstack-query/src/common/types.tspackages/clients/tanstack-query/src/react.tspackages/server/src/api/common/procedures.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/server/src/api/rest/index.tspackages/server/src/api/rpc/index.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/server/test/api/rest.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/server/test/api/rest.test.tspackages/clients/tanstack-query/src/common/types.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/clients/tanstack-query/src/common/types.ts
🧬 Code graph analysis (6)
packages/clients/tanstack-query/src/vue.ts (2)
packages/clients/tanstack-query/src/react.ts (1)
ProcedureHooks(183-190)packages/clients/tanstack-query/src/common/types.ts (4)
ExtractProcedures(80-82)ProcedureReturn(88-90)ProcedurePayload(98-99)QueryContext(9-24)
packages/clients/tanstack-query/src/common/types.ts (2)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/contract.ts (1)
ProcedureFunc(309-311)
packages/clients/tanstack-query/src/react.ts (3)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/clients/tanstack-query/src/common/types.ts (3)
ExtractProcedures(80-82)ProcedureReturn(88-90)ProcedurePayload(98-99)packages/clients/tanstack-query/src/common/query-key.ts (1)
QueryKey(6-15)
packages/server/src/api/common/procedures.ts (2)
packages/schema/src/schema.ts (2)
SchemaDef(11-19)ProcedureDef(82-87)packages/orm/src/client/index.ts (1)
ORMError(7-7)
packages/clients/tanstack-query/src/svelte/index.svelte.ts (3)
packages/clients/tanstack-query/src/react.ts (1)
ProcedureHooks(183-190)packages/clients/tanstack-query/src/common/types.ts (3)
ExtractProcedures(80-82)ProcedureReturn(88-90)ProcedurePayload(98-99)packages/clients/tanstack-query/src/common/query-key.ts (1)
QueryKey(6-15)
packages/server/src/api/rest/index.ts (4)
packages/server/src/types.ts (1)
Response(52-62)packages/server/src/api/common/procedures.ts (1)
getProcedureDef(6-12)packages/server/src/api/common/utils.ts (1)
processSuperJsonRequestPayload(7-29)packages/server/src/api/utils.ts (1)
log(9-27)
⏰ 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). (2)
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (17)
packages/server/test/api/rest.test.ts (2)
3167-3193: LGTM! Well-structured test schema.The test schema provides comprehensive coverage of procedure parameter types (Decimal, String, Int, arrays, enums, custom types) and both query and mutation procedures.
3194-3283: LGTM! Clear and type-safe procedure implementations.The procedure implementations are well-typed using ProcCtx and ProcCtxOptionalArgs patterns, providing good examples for procedure usage.
packages/server/src/api/common/procedures.ts (3)
4-4: LGTM! Clear constant definition.The PROCEDURE_ROUTE_PREFIXES constant uses proper const assertion for type safety.
6-12: LGTM! Safe procedure lookup.The function correctly uses
Object.prototype.hasOwnProperty.call()to avoid prototype pollution when checking for procedure existence.
89-91: LGTM! Clean type guard.The
isOrmErrortype guard correctly usesinstanceoffor runtime type checking.packages/server/src/api/common/utils.ts (1)
34-56: LGTM! Clean query parameter unmarshaling.The
unmarshalQfunction properly handles JSON parsing errors and optional SuperJSON deserialization with clear error messages.packages/server/src/api/rpc/index.ts (6)
8-15: LGTM! Proper utility reuse.The imports correctly leverage the new shared utilities from
common/proceduresandcommon/utilsmodules, promoting code reuse across REST and RPC handlers.
57-65: LGTM! Clean procedure routing.The early return pattern for
$procsrouting is clear and doesn't disrupt the existing CRUD operation flow.
99-99: LGTM! Consistent utility usage.Replacing direct unmarshalQ usage with the imported utility centralizes SuperJSON handling and adds serialization support to existing RPC operations.
Also applies to: 126-126
184-262: LGTM! Well-structured procedure request handling.The
handleProcedureRequestmethod implements a clean flow:
- Validates procedure existence
- Enforces HTTP method constraints per procedure type
- Processes SuperJSON payloads
- Maps and validates arguments
- Invokes procedures via
client.$procs[proc]- Serializes responses with proper metadata
- Handles errors with appropriate logging
The implementation follows the existing RPC handler patterns and integrates well with the new utilities.
264-269: LGTM! Clean delegation to shared utility.The private
mapProcedureArgswrapper properly delegates to the shared utility, maintaining encapsulation while reusing common logic.
315-315: LGTM! Enhanced error details.Adding
rejectReasonto the policy rejection error response provides valuable context for debugging policy-related failures.packages/clients/tanstack-query/src/vue.ts (4)
55-63: LGTM! Proper type imports for procedures.The new type imports (
ExtractProcedures,ProcedurePayload,ProcedureReturn) enable strongly-typed procedure hooks aligned with the schema definitions.
67-69: LGTM! Ergonomic hook signature.The
ProcedureHookFnconditional type correctly makes theargsparameter optional when the procedure has no required parameters, improving the developer experience.
133-177: LGTM! Well-structured procedure hook types.The type definitions correctly:
- Map mutation procedures to
useMutationhooks- Map query procedures to
useQueryanduseInfiniteQueryhooks- Conditionally expose the
$procsnamespace only when procedures exist- Integrate with the
ClientHookstypeThe types mirror the React and Svelte implementations, maintaining consistency across client frameworks.
269-320: LGTM! Clean procedure runtime wiring.The implementation correctly:
- Introduces a
mergeutility for combining root and per-call options with Vue reactivity- Detects procedures in the schema
- Wires mutation procedures to
useInternalMutation- Wires query procedures to both
useInternalQueryanduseInternalInfiniteQuery- Mounts all procedure hooks under the
$procsnamespaceThe runtime wiring aligns with the type definitions and maintains consistency with the React and Svelte client implementations.
packages/clients/tanstack-query/src/common/types.ts (1)
80-99: LGTM! Well-structured procedure type utilities.The type helpers provide a clean abstraction for extracting procedure signatures from the schema:
NormalizeProcedurePayloadelegantly handles the common case of single-argument procedures by unwrapping the tuple, improving developer experience- Proper use of
Awaitedensures async procedure return types are correctly resolved- Type constraints are well-specified and type-safe
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/src/api/rpc/index.ts (1)
314-314: Optional: clarify empty otherwise block.The empty
.otherwise()handler is functional but could be clearer with a comment explaining that no additional error fields are needed for other error reasons.🔎 Optional improvement
- .otherwise(() => { }); + .otherwise(() => { + // No additional error fields for other error reasons + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/clients/tanstack-query/src/svelte/index.svelte.tspackages/server/src/api/common/procedures.tspackages/server/src/api/common/utils.tspackages/server/src/api/rest/index.tspackages/server/src/api/rpc/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/api/common/utils.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/server/src/api/rpc/index.tspackages/server/src/api/common/procedures.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/server/src/api/rest/index.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/clients/tanstack-query/src/svelte/index.svelte.ts
🧬 Code graph analysis (3)
packages/server/src/api/rpc/index.ts (2)
packages/server/src/api/common/utils.ts (2)
unmarshalQ(34-56)processSuperJsonRequestPayload(7-29)packages/server/src/api/common/procedures.ts (2)
getProcedureDef(6-12)mapProcedureArgs(61-134)
packages/server/src/api/common/procedures.ts (2)
packages/schema/src/schema.ts (2)
SchemaDef(11-19)ProcedureDef(82-87)packages/orm/src/client/index.ts (1)
ORMError(7-7)
packages/clients/tanstack-query/src/svelte/index.svelte.ts (4)
packages/clients/tanstack-query/src/vue.ts (1)
ProcedureHooks(170-177)packages/clients/tanstack-query/src/react.ts (1)
ProcedureHooks(183-190)packages/clients/tanstack-query/src/common/types.ts (3)
ExtractProcedures(80-82)ProcedureReturn(88-90)ProcedurePayload(98-99)packages/clients/tanstack-query/src/common/query-key.ts (1)
QueryKey(6-15)
⏰ 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). (2)
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (15)
packages/server/src/api/common/procedures.ts (3)
1-12: LGTM: Clean utility setup.The imports, constant export, and
getProcedureDeffunction are well-structured. The use ofObject.prototype.hasOwnProperty.callis the correct defensive pattern for checking object properties.
14-134: Well-documented complex validation logic.The extensive JSDoc documentation added in a previous commit successfully addresses the complexity of this function. The validation paths are now clear, and the rationale for rejecting null/falsey payloads is well-explained.
136-138: LGTM: Correct type guard implementation.packages/clients/tanstack-query/src/svelte/index.svelte.ts (4)
57-70: LGTM: Well-designed conditional type for procedure hooks.The
ProcedureHookFntype correctly handles optional arguments when the payload is undefined, providing good DX for procedures with no required parameters.
98-104: Good refactor: eliminated code duplication.The extraction of the
mergehelper eliminates the duplication that existed betweenuseClientQueriesanduseModelQueries. Verified that it's used consistently at lines 322, 326, and 330.
138-175: LGTM: Type definitions are consistent across framework adapters.The procedure hook types properly distinguish between mutations and queries, and the structure mirrors the React and Vue implementations mentioned in relevant code snippets, ensuring consistency.
265-302: LGTM: Procedure hooks generation is well-structured.The runtime wiring correctly:
- Iterates over schema procedures when present
- Distinguishes mutations from queries
- Uses the extracted
mergehelper consistently- Routes to appropriate internal hook implementations
packages/server/src/api/rpc/index.ts (4)
8-15: LGTM: Appropriate imports of shared utilities.The imports of procedure utilities from the common modules promote code reuse between RPC and REST handlers.
57-65: LGTM: Clean routing for procedure requests.The early detection and routing of
$procsrequests before regular model operations is a clean pattern, consistent with the REST handler implementation.
184-262: LGTM: Comprehensive procedure request handling.The implementation properly:
- Validates procedure existence and HTTP methods
- Uses shared utilities for argument mapping
- Employs optional chaining for defensive access to
$procs- Handles SuperJSON serialization consistently
- Distinguishes ORM errors from generic errors
308-308: Good addition: propagates policy rejection reason.Including
rejectReasonin the error response provides valuable debugging context for policy rejections.packages/server/src/api/rest/index.ts (4)
12-18: LGTM: Consistent imports with RPC handler.Using the same shared utilities as the RPC handler promotes consistency and reduces duplication.
346-349: LGTM: Early routing with proper validation.The early detection of procedure paths is clean. The downstream validation at line 501 properly handles missing procedure names.
488-556: LGTM: Well-structured procedure request handling.The implementation is consistent with the RPC handler while adapting to REST-specific patterns:
- Uses optional chaining defensively (line 539) - consistent with RPC
- Calls
toPlainObjectbefore SuperJSON (line 540) - necessary for REST serialization- Rethrows
ORMError(line 552) which is caught by top-level handler (line 476)The optional chaining mentioned in a past review comment is appropriate here, as it prevents runtime errors if
$procsis undefined.
558-569: LGTM: Clean error response helpers.The procedure-specific error response helpers appropriately delegate to the existing
makeErrorinfrastructure and include proper logging.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/e2e/orm/schemas/procedures/schema.zmodel (1)
24-29: Good test coverage across procedure variations.The procedure definitions cover a comprehensive range of scenarios: single/array returns, custom types, optional/array parameters, and both query and mutation operations.
Optionally, consider adding
@@allow('execute', ...)policies on some procedures in future test iterations to validate policy enforcement integration mentioned in the PR objectives.tests/e2e/orm/client-api/procedures.test.ts (2)
188-200: Consider more specific error assertions for transaction rollback test.The test verifies transactional rollback behavior but uses a generic
.toThrow()assertion. For better test precision and debugging, consider asserting the specific error type and code.🔎 Example of more specific error assertion
- await expect(client.$procs.createMultiple({ args: { names: ['Alice', 'Alice'] } })).rejects.toThrow(); + await expect( + client.$procs.createMultiple({ args: { names: ['Alice', 'Alice'] } }) + ).rejects.toThrow(/unique constraint|Unique constraint/i); await expect(client.user.count()).resolves.toBe(0);This makes test failures easier to diagnose and ensures you're catching the expected constraint violation rather than any arbitrary error.
7-201: Consider expanding test coverage for real-world scenarios.The current tests cover core happy paths effectively. To align with the coding guideline that "E2E tests should validate real-world schema compatibility with established projects" and the RFC's emphasis on policy integration and QaaS visibility, consider adding tests for:
- Procedure argument validation failures (invalid/missing required args)
- Error handling and custom error propagation from procedures
- Authorization context and policy enforcement (mentioned in PR objectives/RFC)
- Procedures with complex nested transactions or error recovery
- QaaS visibility and handler-specific context features (if applicable at this layer)
These additions would make the test suite more comprehensive and better validate the procedures feature against real-world usage patterns.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/orm/client-api/procedures.test.tstests/e2e/orm/schemas/procedures/schema.tstests/e2e/orm/schemas/procedures/schema.zmodel
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/orm/schemas/procedures/schema.ts
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/client-api/procedures.test.ts
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
tests/e2e/orm/schemas/procedures/schema.zmodel
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/procedures.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/procedures.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
tests/e2e/orm/client-api/procedures.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
tests/e2e/orm/schemas/procedures/schema.zmodel
🧬 Code graph analysis (1)
tests/e2e/orm/client-api/procedures.test.ts (3)
packages/orm/src/client/contract.ts (1)
ClientContract(65-199)packages/testtools/src/client.ts (1)
createTestClient(101-248)packages/plugins/policy/src/plugin.ts (1)
name(11-13)
⏰ 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). (2)
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (5)
tests/e2e/orm/schemas/procedures/schema.zmodel (5)
1-4: LGTM!The SQLite datasource configuration is appropriate for an e2e test schema.
6-9: LGTM!The Role enum is well-defined and provides necessary values for testing role-based scenarios in procedures.
11-16: LGTM!The Overview type effectively tests array field support (Int[], Role[]) and optional fields, which aligns well with the procedure feature's array handling capabilities.
18-22: LGTM!The User model provides a clean test entity for procedure operations with appropriate constraints.
1-29: Reminder: Runzenstack generateafter modifying this schema.As per coding guidelines, ensure you run
zenstack generateto compile this ZModel schema to TypeScript after any modifications.Based on coding guidelines.
- Simplified procedure's params definition from a tuple an object, since procs are now called with an envelop now - Refactored procedure related typing to make them more consistent with other CURD types (that usually takes the schema as the first type parameter, and a name as the second) - Moved detailed procedure's types to "crud-types" where other ORM client detailed types are defined - Removed some type duplication from hooks side - Updated the "orm" sample to demonstrate procedures
refactor: procedure params generation fix and type refactors
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @packages/orm/src/client/crud/validator/index.ts:
- Around line 76-157: validateProcedureInput currently iterates only known
params and doesn't reject unknown keys in argsPayload; replace the manual
per-param loop with a single Zod strict object validation: build a mapping from
procDef.params to Zod schemas by calling makeProcedureParamSchema for each param
name, create a z.strictObject schema (so unknown keys are rejected), run
safeParse against argsPayload, and on failure throw createInvalidInputError with
formatError; keep the existing early handling for zero-params and undefined
argsPayload but use the strict schema for all non-empty param sets to ensure
extraneous/misspelled args are rejected.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/cli/test/ts-schema-gen.test.tspackages/clients/tanstack-query/src/common/types.tspackages/clients/tanstack-query/src/react.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/test/react-typing-test.tspackages/clients/tanstack-query/test/schemas/procedures/schema-lite.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/crud/validator/index.tspackages/orm/src/client/options.tspackages/orm/src/utils/type-utils.tspackages/schema/src/schema.tspackages/sdk/src/ts-schema-generator.tspackages/server/src/api/common/procedures.tssamples/orm/main.tssamples/orm/zenstack/schema.tssamples/orm/zenstack/schema.zmodeltests/e2e/orm/schemas/procedures/schema.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/clients/tanstack-query/src/common/types.ts
- packages/clients/tanstack-query/src/vue.ts
- packages/clients/tanstack-query/test/schemas/procedures/schema-lite.ts
- packages/orm/src/client/options.ts
- packages/schema/src/schema.ts
- tests/e2e/orm/schemas/procedures/schema.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
samples/orm/zenstack/schema.zmodel
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
samples/orm/zenstack/schema.tssamples/orm/zenstack/schema.zmodelpackages/orm/src/client/contract.tspackages/cli/test/ts-schema-gen.test.tspackages/clients/tanstack-query/src/react.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
samples/orm/zenstack/schema.tssamples/orm/main.tspackages/clients/tanstack-query/src/react.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/orm/src/client/crud-types.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/clients/tanstack-query/test/react-typing-test.tssamples/orm/main.tspackages/orm/src/client/crud-types.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/clients/tanstack-query/test/react-typing-test.tspackages/cli/test/ts-schema-gen.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/clients/tanstack-query/test/react-typing-test.tspackages/orm/src/client/contract.tspackages/clients/tanstack-query/src/react.tspackages/orm/src/client/crud/validator/index.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/orm/src/client/crud-types.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/orm/src/utils/type-utils.tspackages/orm/src/client/contract.tspackages/clients/tanstack-query/src/react.tspackages/orm/src/client/crud/validator/index.tspackages/server/src/api/common/procedures.tspackages/clients/tanstack-query/src/svelte/index.svelte.tspackages/sdk/src/ts-schema-generator.tspackages/orm/src/client/crud-types.ts
🧬 Code graph analysis (6)
packages/clients/tanstack-query/test/react-typing-test.ts (3)
packages/clients/tanstack-query/src/react.ts (1)
useClientQueries(327-382)packages/clients/tanstack-query/src/svelte/index.svelte.ts (1)
useClientQueries(274-316)packages/clients/tanstack-query/src/vue.ts (1)
useClientQueries(278-328)
packages/orm/src/client/contract.ts (2)
packages/schema/src/schema.ts (2)
SchemaDef(11-19)ProcedureDef(82-87)packages/orm/src/client/crud-types.ts (2)
GetProcedureNames(1986-1988)ProcedureFunc(2041-2045)
packages/cli/test/ts-schema-gen.test.ts (1)
packages/testtools/src/schema.ts (1)
generateTsSchema(40-74)
packages/clients/tanstack-query/src/react.ts (3)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/crud-types.ts (3)
GetProcedureNames(1986-1988)ProcedureEnvelope(2021-2032)GetProcedure(1996-2000)packages/clients/tanstack-query/src/common/types.ts (1)
ProcedureReturn(80-82)
packages/orm/src/client/crud/validator/index.ts (4)
packages/schema/src/schema.ts (2)
ProcedureDef(82-87)BuiltinType(89-99)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/query-utils.ts (2)
isTypeDef(201-203)isEnum(193-195)
packages/server/src/api/common/procedures.ts (2)
packages/schema/src/schema.ts (3)
SchemaDef(11-19)ProcedureDef(82-87)ProcedureParam(80-80)packages/orm/src/client/index.ts (1)
ORMError(7-7)
⏰ 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). (2)
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (15)
packages/orm/src/client/crud/validator/index.ts (2)
12-12: LGTM: Import additions support procedure validation.The new imports (ProcedureDef, isEnum, isTypeDef) are properly utilized in the validateProcedureInput and makeProcedureParamSchema methods.
Also applies to: 43-44
159-188: LGTM: Per-parameter schema builder is well-structured.The helper correctly:
- Uses
isTypeDefandisEnumhelpers (addressing past review feedback)- Handles TypeDefs, Enums, Models, and builtin scalar types
- Applies array and optional modifiers appropriately
- Throws an internal error for unsupported types
The permissive validation for model-typed parameters (line 168:
z.record(z.string(), z.unknown())) is a reasonable trade-off that avoids deep object validation while still ensuring the value is an object.packages/clients/tanstack-query/test/react-typing-test.ts (2)
3-6: LGTM! Procedures client setup follows established patterns.The import and client initialization for procedures testing mirrors the existing pattern used for the basic schema, ensuring consistency in test structure.
117-137: Excellent comprehensive test coverage for procedures API.The test cases thoroughly exercise the new
$procsnamespace across multiple scenarios:
- Query procedures with optional arguments and type validation
- Infinite query support for array-returning procedures
- Mutation procedures with envelope-style invocation (
{ args: { ... } })- Type safety enforcement via
@ts-expect-errorannotations- Both synchronous and async (thenable) execution patterns
The tests align well with the existing model-based test patterns and validate the strongly-typed per-procedure hook surface introduced by this PR.
packages/cli/test/ts-schema-gen.test.ts (1)
187-209: LGTM! Essential test coverage for array parameter and return type handling.This test validates that the TypeScript schema generator correctly captures array semantics for procedures:
- Array parameters are marked with
array: trueon the param definition- Array return types set
returnArray: truewhile keepingreturnTypeas the element typeThis metadata is crucial for both client-side type generation and runtime input validation in the ORM layer. The test complements the broader procedure typing system introduced in this PR.
packages/orm/src/utils/type-utils.ts (1)
34-50: LGTM! TypeMap extension supports expanded procedure type coverage.The changes enhance the type mapping system appropriately:
- Export addition - Making
TypeMapexported enables reuse across the codebase (referenced incrud-types.tsand elsewhere)- New type mappings:
Void- Essential for procedures with side effects that return no valueNull,Undefined- Support nullable types in procedure signaturesObject- Maps toRecord<string, unknown>for generic object parametersAny,Unsupported- Reasonable fallbacks for types that cannot be precisely mappedThese additions align with the expanded procedure type system and enable more comprehensive ZModel type coverage.
samples/orm/zenstack/schema.ts (1)
242-257: Generated code correctly reflects ZModel procedure definitions.The generated
proceduresproperty accurately represents the procedure declarations fromschema.zmodel:
signUpwith properly typed parameters (including optional flags) andmutation: truelistPublicPostswithreturnArray: truefor array return typeAs this is auto-generated code (per the file header warning), no manual changes should be made. The generation appears to be working correctly.
samples/orm/zenstack/schema.zmodel (1)
58-59: Procedure declarations are correct and properly generated.The
signUpandlistPublicPostsprocedures follow ZModel conventions with correct syntax:
signUpcorrectly defined as a mutation with requiredname/roleparameters, returningUserlistPublicPostscorrectly returns an array ofPostmodelsThe generated
schema.tsfile reflects these definitions accurately with proper parameter configurations and type mappings.samples/orm/main.ts (1)
19-30: Procedures wiring and sample usage look consistent end‑to‑end
proceduresregistration matches the new$procscontract, and the example calls viaauthDb.$procs.signUp/authDb.$procs.listPublicPostscorrectly demonstrate mutation vs query semantics while flowing through policy-enabled clients.Also applies to: 116-121
packages/server/src/api/common/procedures.ts (1)
4-12: Server‑side procedure utilities are coherent and defensively implementedRoute prefixes, schema lookup, and
mapProcedureArgs’ validation logic (zero/all‑optional/required params, envelope handling, and strict object checks) align with the documented behavior and provide a solid contract for REST/RPC layers;isOrmErroris a minimal, correct type guard.Also applies to: 14-133, 135-137
packages/sdk/src/ts-schema-generator.ts (1)
1126-1158: Procedure schema emission now matches runtimeProcedureDefshapeObject‑keyed
paramswith{ name, type, optional?, array? }entries plus the newreturnArrayflag correctly mirror the updatedProcedureDef/ProcedureParamtypes and should make downstream procedure typing much simpler.packages/orm/src/client/contract.ts (1)
12-12: Typed$procssurface integrates cleanly intoClientContract
ProcedureOperations<Schema>conditionally adds$procskeyed byGetProcedureNames<Schema>withProcedureFuncsignatures, so client instances gain strongly typed custom procedures only when the schema defines them, without impacting existing CRUD typings.Also applies to: 29-30, 33-33, 195-217
packages/clients/tanstack-query/src/react.ts (1)
39-41: Procedure hooks for React reuse existing plumbing and preserve typing semanticsThe new
ProcedureHookFn/ProcedureHookGroup/ProcedureHookstypes plus$procsintegration inuseClientQueriescleanly extend model hooks to procedures, correctly distinguish mutation vs query procedures, and enforce the envelope contract while reusing the existing internal query/mutation helpers and$procsendpoint routing.Also applies to: 44-44, 61-65, 68-77, 148-151, 152-201, 327-382
packages/clients/tanstack-query/src/svelte/index.svelte.ts (1)
40-45: Svelte procedure hooks and sharedmergehelper are well‑structuredAccessor‑aware
ProcedureHookFn/ProcedureHookGroup/ProcedureHooks, the extractedmergehelper, and the$procswiring inuseClientQueriesextend the model hook pattern to procedures cleanly while avoiding duplication and keeping options merging consistent.Also applies to: 62-66, 69-78, 105-111, 145-148, 149-189, 274-316
packages/orm/src/client/crud-types.ts (1)
26-34: Procedure typing utilities provide a solid, composable foundation
GetProcedureNames/GetProcedureParams/GetProcedure,ProcedureEnvelope, andProcedureFunc/ProcedureHandlerFunctogether give strongly typed procedure inputs and outputs, whileMapProcedureReturn/MapProcedureParam+MapTypecorrectly resolve schema string types to model/typedef/enum/builtin shapes; this should make both client$procsand server handlers type‑safe without extra casting.Also applies to: 35-50, 51-53, 1984-2067, 2085-2099
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/clients/tanstack-query/src/vue.ts (1)
314-320: Runtime exposesuseInfiniteQueryfor procedures but types exclude it.The runtime wiring at lines 318-319 creates
useInfiniteQueryfor non-mutation procedures, but theProcedureHookGrouptype (lines 174-181) has this commented out. This inconsistency means the hook exists at runtime but TypeScript won't recognize it.Consider removing the runtime wiring to match the type definition, keeping behavior consistent with the decision to defer infinite queries for procedures.
♻️ Proposed fix to align runtime with types
} else { acc[name] = { useQuery: (args?: any, hookOptions?: any) => useInternalQuery(schema, endpointModel, name, args, merge(options, hookOptions)), - useInfiniteQuery: (args?: any, hookOptions?: any) => - useInternalInfiniteQuery(schema, endpointModel, name, args, merge(options, hookOptions)), }; }packages/clients/tanstack-query/src/react.ts (1)
364-374: Runtime wires infinite query hooks for procedures but types exclude them.Lines 364-373 create
useInfiniteQueryanduseSuspenseInfiniteQueryhooks for procedures at runtime, but the corresponding types inProcedureHookGroup(lines 178-192) are commented out. Users cannot access these hooks with type safety.Remove this dead runtime code to match the type definitions and the decision to defer infinite queries for procedures.
♻️ Proposed fix to remove untyped runtime hooks
} else { acc[name] = { useQuery: (args?: any, hookOptions?: any) => useInternalQuery(schema, endpointModel, name, args, { ...options, ...hookOptions }), useSuspenseQuery: (args?: any, hookOptions?: any) => useInternalSuspenseQuery(schema, endpointModel, name, args, { ...options, ...hookOptions, }), - useInfiniteQuery: (args?: any, hookOptions?: any) => - useInternalInfiniteQuery(schema, endpointModel, name, args, { - ...options, - ...hookOptions, - }), - useSuspenseInfiniteQuery: (args?: any, hookOptions?: any) => - useInternalSuspenseInfiniteQuery(schema, endpointModel, name, args, { - ...options, - ...hookOptions, - }), }; }packages/clients/tanstack-query/src/svelte/index.svelte.ts (1)
304-308: Runtime exposesuseInfiniteQueryfor procedures but types exclude it.Similar to React and Vue, line 306-307 creates
useInfiniteQueryfor non-mutation procedures at runtime, but theProcedureHookGrouptype (lines 172-179) has this commented out.For consistency across all three frameworks and to match the decision to defer infinite queries, remove this runtime wiring.
♻️ Proposed fix
} else { acc[name] = { useQuery: (args?: any, hookOptions?: any) => useInternalQuery(schema, endpointModel, name, args, merge(options, hookOptions)), - useInfiniteQuery: (args?: any, hookOptions?: any) => - useInternalInfiniteQuery(schema, endpointModel, name, args, merge(options, hookOptions)), }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/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-typing-test.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/clients/tanstack-query/test/vue-typing-test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clients/tanstack-query/test/vue-typing-test.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/clients/tanstack-query/test/react-typing-test.tspackages/clients/tanstack-query/test/svelte-typing-test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/clients/tanstack-query/test/react-typing-test.tspackages/clients/tanstack-query/test/svelte-typing-test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/clients/tanstack-query/src/svelte/index.svelte.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/src/react.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/clients/tanstack-query/src/svelte/index.svelte.tspackages/clients/tanstack-query/test/svelte-typing-test.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/src/react.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/clients/tanstack-query/src/svelte/index.svelte.tspackages/clients/tanstack-query/src/vue.tspackages/clients/tanstack-query/src/react.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
packages/clients/tanstack-query/test/svelte-typing-test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/clients/tanstack-query/src/react.ts
🧬 Code graph analysis (4)
packages/clients/tanstack-query/test/react-typing-test.ts (5)
packages/clients/tanstack-query/src/react.ts (1)
useClientQueries(329-384)packages/clients/tanstack-query/test/schemas/procedures/schema-lite.ts (1)
schema(60-60)samples/orm/zenstack/schema.ts (1)
schema(260-260)tests/e2e/orm/schemas/procedures/schema.ts (1)
schema(121-121)packages/clients/tanstack-query/test/schemas/basic/schema-lite.ts (1)
schema(172-172)
packages/clients/tanstack-query/src/svelte/index.svelte.ts (3)
packages/orm/src/client/crud-types.ts (3)
GetProcedureNames(1986-1988)ProcedureEnvelope(2021-2032)GetProcedure(1996-2000)packages/clients/tanstack-query/src/react.ts (2)
useInternalMutation(613-700)useInternalQuery(502-523)packages/clients/tanstack-query/src/common/types.ts (1)
ProcedureReturn(80-82)
packages/clients/tanstack-query/test/svelte-typing-test.ts (1)
packages/clients/tanstack-query/src/svelte/index.svelte.ts (1)
useClientQueries(276-318)
packages/clients/tanstack-query/src/vue.ts (4)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/crud-types.ts (3)
GetProcedureNames(1986-1988)ProcedureEnvelope(2021-2032)GetProcedure(1996-2000)packages/clients/tanstack-query/src/common/types.ts (1)
ProcedureReturn(80-82)packages/clients/tanstack-query/src/common/query-key.ts (1)
QueryKey(6-15)
⏰ 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). (2)
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (9)
packages/clients/tanstack-query/test/svelte-typing-test.ts (1)
123-147: Comprehensive procedure typing tests for Svelte client.Good coverage of the new
$procsAPI surface:
- Query procedures with optional and required args
- Mutation procedures with envelope-style invocation
- Type constraint validation via
@ts-expect-errorcomments- Proper use of Svelte's Accessor pattern with arrow functions
The commented-out infinite query tests align with the decision to defer that feature.
packages/clients/tanstack-query/test/react-typing-test.ts (1)
117-140: Solid typing tests for React procedure hooks.Tests comprehensively validate the
$procsAPI:
- Query hooks with args, options, and queryKey access
- Type errors for wrong arg shapes and missing required args
- Mutation hooks with both
mutateandmutateAsync- Proper detection of query vs mutation procedures via
@ts-expect-errorpackages/clients/tanstack-query/src/vue.ts (2)
68-77: Well-designed conditional type for procedure hook signatures.The
ProcedureHookFntype elegantly handles optional vs required args using conditional typing based on theProcedureEnvelope. Vue'sMaybeRefOrGetterwrapper is correctly applied for reactivity.
142-193: Clean procedure hook type definitions.The
ProcedureHookGroupandProcedureHookstypes properly:
- Differentiate mutation vs query procedures via conditional mapping
- Exclude
optimisticUpdatefrom query options (not applicable to procedures)- Use conditional extension to only add
$procswhen schema has procedurespackages/clients/tanstack-query/src/react.ts (2)
68-77: Good conditional type for procedure hook function signatures.The
ProcedureHookFntype correctly uses{ args: undefined } extends Inputto determine if input is optional, providing appropriate overloads for procedures with and without required arguments.
148-204: Well-structured procedure hook types for React.The type definitions properly:
- Map procedure names to their appropriate hook shapes
- Include
useSuspenseQueryfor React (correctly matching runtime)- Exclude
optimisticUpdatefrom procedure query options- Conditionally add
$procsonly when schema defines procedurespackages/clients/tanstack-query/src/svelte/index.svelte.ts (3)
105-111: Good extraction of merge helper to module level.The
mergehelper is now properly defined at module scope, eliminating the duplication that was previously flagged. The implementation correctly handles both function and non-function values for Svelte's reactive patterns.
69-78: Correct Accessor-wrapped procedure hook function type.The
ProcedureHookFnfor Svelte properly wraps both args and options inAccessor<>, aligning with Svelte's reactive patterns. The conditional logic for optional vs required args is correctly implemented.
149-191: Clean procedure hook type definitions for Svelte.The types correctly:
- Use Svelte-specific
CreateQueryResultandCreateMutationResult- Wrap options in
Accessor<>for reactivity- Exclude
optimisticUpdatefrom procedure query options- Conditionally expose
$procsbased on schema.procedures
3 participants
draft of an implementation of custom procedures based on the blog article: https://zenstack.dev/blog/next-chapter-2#custom-procedures
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.