merge dev to main (v3.0.0-alpha.24) #166
Conversation
WalkthroughConsolidates the runtime plugin model: replaces nested per-model/per-operation onQuery hooks with a single onQuery callback; nests entity-mutation hooks under onEntityMutation; adds AllModelOperations type; updates executor/client/plugin code and tests; bumps many package versions to 3.0.0-alpha.24; minor VSCode launch tweak and a TODO entry. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1){packages,samples,tests}/**📄 CodeRabbit Inference Engine (CLAUDE.md)
Files:
🧠 Learnings (1)📓 Common learnings⏰ 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)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude finished @ymc9's task —— View job Pull Request Review Complete ✅Reviewing PR #166: merge dev to main (3.0.0-alpha.24 release) Review Checklist:
📋 SummaryThis is a significant alpha release that includes major plugin system refactoring for TypeScript performance improvements. While the changes are well-structured, there are some important considerations for the team.
|
There was a problem hiding this comment.
Pull Request Overview
This PR merges development branch to main, implementing a significant API refactoring for ZenStack runtime plugins along with version bumping from alpha.23 to alpha.24. The main changes simplify the plugin API structure by flattening nested hook configurations and reorganizing entity mutation hooks.
Key changes:
- Simplifies onQuery plugin API from nested model/operation structure to a single callback function
- Reorganizes entity mutation hooks under an
onEntityMutationproperty - Updates all package versions from 3.0.0-alpha.23 to 3.0.0-alpha.24
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Multiple package.json files | Version bump from alpha.23 to alpha.24 across all packages |
| samples/blog/main.ts | Updates plugin usage to use simplified onQuery API |
| packages/runtime/src/client/plugin.ts | Major API refactor: flattens onQuery structure and moves entity mutation hooks |
| packages/runtime/test/ files | Updates test files to use new plugin API and adds new test structure |
| packages/runtime/src/client/ files | Implementation updates to support new plugin API structure |
| README.md | Documentation updates reflecting the new plugin API |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
TODO.md (1)
102-102: Clarify SQL phrasing/casing for consistencyConsider using the canonical SQL form and consistent casing with other entries.
Apply this diff:
- - [ ] Inject "insert into select from" + - [ ] Inject "INSERT INTO ... SELECT ..."packages/runtime/src/client/plugin.ts (5)
96-112: Fix JSDoc: references to old API and singular/plural names.
- “return value of {@link RuntimePlugin.mutationInterceptionFilter}” is outdated; it should refer to
onEntityMutation.mutationInterceptionFilter.- “args.entity” should be “args.entities”.
Apply this diff:
- * @param args.entity Only available if `loadBeforeMutationEntities` is set to true in the - * return value of {@link RuntimePlugin.mutationInterceptionFilter}. + * @param args.entities Only available if `loadBeforeMutationEntities` is set to true in the + * return value of {@link EntityMutationHooksDef.mutationInterceptionFilter}.And here:
- * @param args.beforeMutationEntity Only available if `loadBeforeMutationEntities` is set to true in the - * return value of {@link RuntimePlugin.mutationInterceptionFilter}. - * @param args.afterMutationEntity Only available if `loadAfterMutationEntities` is set to true in the - * return value of {@link RuntimePlugin.mutationInterceptionFilter}. + * @param args.beforeMutationEntities Only available if `loadBeforeMutationEntities` is set to true in the + * return value of {@link EntityMutationHooksDef.mutationInterceptionFilter}. + * @param args.afterMutationEntities Only available if `loadAfterMutationEntities` is set to true in the + * return value of {@link EntityMutationHooksDef.mutationInterceptionFilter}.
113-129: Fix JSDoc: clarify queryNode description.The comment says “The mutation data. Only available for create and update actions.” but the field is
queryNode: OperationNodeand is always present. Reword to avoid confusion.Apply this diff:
- /** - * The mutation data. Only available for create and update actions. - */ + /** + * The raw Kysely operation node for the mutation (Insert/Update/Delete). + */
162-169: Fix JSDoc: update link target and plural form.Apply this diff:
- * Entities that are about to be mutated. Only available if `loadBeforeMutationEntities` is set to - * true in the return value of {@link RuntimePlugin.mutationInterceptionFilter}. + * Entities that are about to be mutated. Only available if `loadBeforeMutationEntities` is set to + * true in the return value of {@link EntityMutationHooksDef.mutationInterceptionFilter}.
170-182: Fix JSDoc: update link targets to new API.Apply this diff:
- * Entities that are about to be mutated. Only available if `loadBeforeMutationEntities` is set to - * true in the return value of {@link RuntimePlugin.mutationInterceptionFilter}. + * Entities that are about to be mutated. Only available if `loadBeforeMutationEntities` is set to + * true in the return value of {@link EntityMutationHooksDef.mutationInterceptionFilter}. @@ - * Entities mutated. Only available if `loadAfterMutationEntities` is set to true in the return - * value of {@link RuntimePlugin.mutationInterceptionFilter}. + * Entities mutated. Only available if `loadAfterMutationEntities` is set to true in the return + * value of {@link EntityMutationHooksDef.mutationInterceptionFilter}.
188-194: Tighten schema typing in OnKyselyQueryArgs.
schemashould carry the genericSchema, not the baseSchemaDef, for better type inference in plugins.Apply this diff:
-export type OnKyselyQueryArgs<Schema extends SchemaDef> = { - kysely: ToKysely<Schema>; - schema: SchemaDef; +export type OnKyselyQueryArgs<Schema extends SchemaDef> = { + kysely: ToKysely<Schema>; + schema: Schema;samples/blog/main.ts (1)
20-24: Log in finally to capture durations even on errors.If the query throws, current code skips logging. Wrap with try/finally.
Apply this diff:
- onQuery: async ({ model, operation, args, query }) => { - const start = Date.now(); - const result = await query(args); - console.log(`[cost] ${model} ${operation} took ${Date.now() - start}ms`); - return result; - }, + onQuery: async ({ model, operation, args, query }) => { + const start = Date.now(); + try { + return await query(args); + } finally { + console.log(`[cost] ${model} ${operation} took ${Date.now() - start}ms`); + } + },packages/runtime/test/plugin/on-query-hooks.test.ts (1)
124-153: Safer result mutation patternDirectly mutating the result may have side effects if the ORM returns frozen/proxied objects in the future. Prefer a shallow copy when annotating.
- const result = await ctx.query(ctx.args); - (result as any).happy = true; - return result; + const result = await ctx.query(ctx.args); + return result ? { ...(result as any), happy: true } : result;packages/runtime/test/policy/client-extensions.test.ts (2)
33-35: Duplicate assertions offer little additional valueThese four tests now exercise the same path (top-level onQuery) with identical logic and repeated duplicate assertions. Consider parameterizing into a single test or differentiating by explicitly filtering on model/operation in the hook to validate scoping.
Also applies to: 64-66, 95-97, 126-128
148-154: Prefer non-mutating result transformationUsing a functional style avoids unintended side effects.
- onQuery: async ({ args, query }: any) => { - const r: any = await query(args); - for (let i = 0; i < r.length; i++) { - r[i].value = r[i].value + 1; - } - return r; - }, + onQuery: async ({ args, query }: any) => { + const r: any[] = await query(args); + return r.map((item) => ({ ...item, value: item.value + 1 })); + },README.md (2)
281-291: Typo: use db.$use, not ZenStackdb.$useThe instance variable in examples is consistently named db.
-V3 introduces a new runtime plugin mechanism that allows you to tap into the ORM's query execution in various ways. A plugin implements the [RuntimePlugin](./packages/runtime/src/client/plugin.ts#L121) interface, and can be installed with the `ZenStackdb.$use` API. +V3 introduces a new runtime plugin mechanism that allows you to tap into the ORM's query execution in various ways. A plugin implements the [RuntimePlugin](./packages/runtime/src/client/plugin.ts) interface, and can be installed with the `db.$use` API.
301-303: Wording: refer to the query callback, not proceedThe onQuery examples use query(args) as the continuation. Align the prose.
-Usually a plugin would call the `proceed` callback to trigger the execution of the original query, but you can choose to completely override the query behavior with custom logic. +Usually a plugin would call the `query` callback to trigger the execution of the original query, but you can choose to completely override the query behavior with custom logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.vscode/launch.json(2 hunks)README.md(3 hunks)TODO.md(1 hunks)package.json(1 hunks)packages/cli/package.json(1 hunks)packages/common-helpers/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/eslint-config/package.json(1 hunks)packages/ide/vscode/package.json(1 hunks)packages/language/package.json(1 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/client-impl.ts(2 hunks)packages/runtime/src/client/contract.ts(1 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(4 hunks)packages/runtime/src/client/plugin.ts(2 hunks)packages/runtime/test/plugin/entity-mutation-hooks.test.ts(10 hunks)packages/runtime/test/plugin/on-kysely-query.test.ts(1 hunks)packages/runtime/test/plugin/on-query-hooks.test.ts(1 hunks)packages/runtime/test/plugin/query-lifecycle.test.ts(0 hunks)packages/runtime/test/policy/client-extensions.test.ts(5 hunks)packages/sdk/package.json(1 hunks)packages/tanstack-query/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/typescript-config/package.json(1 hunks)packages/vitest-config/package.json(1 hunks)packages/zod/package.json(1 hunks)samples/blog/main.ts(1 hunks)samples/blog/package.json(1 hunks)tests/e2e/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime/test/plugin/query-lifecycle.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/typescript-config/package.jsonpackages/ide/vscode/package.jsonpackages/create-zenstack/package.jsontests/e2e/package.jsonpackages/zod/package.jsonpackages/dialects/sql.js/package.jsonpackages/eslint-config/package.jsonpackages/tanstack-query/package.jsonpackages/common-helpers/package.jsonpackages/testtools/package.jsonsamples/blog/package.jsonpackages/runtime/package.jsonpackages/runtime/test/plugin/on-kysely-query.test.tspackages/language/package.jsonpackages/vitest-config/package.jsonpackages/runtime/src/client/client-impl.tspackages/cli/package.jsonsamples/blog/main.tspackages/sdk/package.jsonpackages/runtime/test/plugin/on-query-hooks.test.tspackages/runtime/src/client/contract.tspackages/runtime/src/client/executor/zenstack-query-executor.tspackages/runtime/test/policy/client-extensions.test.tspackages/runtime/test/plugin/entity-mutation-hooks.test.tspackages/runtime/src/client/plugin.ts
tests/e2e/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
E2E tests are in
tests/e2e/directory
Files:
tests/e2e/package.json
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Plugin Architecture: Runtime plugins for query interception and entity mutation hooks
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: `zenstack generate` compiles ZModel to TypeScript schema (`schema.ts`)
Applied to files:
packages/language/package.json.vscode/launch.json
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Plugin Architecture: Runtime plugins for query interception and entity mutation hooks
Applied to files:
packages/runtime/src/client/client-impl.tspackages/runtime/test/plugin/on-query-hooks.test.tspackages/runtime/src/client/executor/zenstack-query-executor.tsREADME.mdpackages/runtime/src/client/plugin.ts
🧬 Code Graph Analysis (3)
packages/runtime/test/plugin/on-query-hooks.test.ts (3)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/src/client/client-impl.ts (1)
ZenStackClient(40-46)packages/runtime/src/client/plugin.ts (1)
definePlugin(45-47)
packages/runtime/src/client/contract.ts (4)
packages/sdk/src/schema/schema.ts (2)
SchemaDef(10-18)GetModels(108-108)packages/runtime/src/client/crud-types.ts (10)
FindArgs(624-643)FindUniqueArgs(648-650)UpdateArgs(794-800)UpdateManyArgs(802-805)CountArgs(956-961)CountResult(967-977)AggregateArgs(983-997)AggregateResult(1023-1051)GroupByArgs(1070-1087)GroupByResult(1089-1123)packages/runtime/src/client/promise.ts (1)
ZenStackPromise(7-13)packages/runtime/src/utils/type-utils.ts (1)
Simplify(11-17)
packages/runtime/src/client/plugin.ts (5)
packages/sdk/src/schema/schema.ts (2)
SchemaDef(10-18)GetModels(108-108)packages/runtime/src/client/crud/operations/base.ts (1)
CrudOperation(54-69)packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/src/utils/type-utils.ts (1)
MaybePromise(70-70)packages/runtime/src/client/query-builder.ts (1)
ToKysely(19-19)
⏰ 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 (20.x)
- GitHub Check: claude-review
🔇 Additional comments (34)
samples/blog/package.json (1)
3-3: Version bump to 3.0.0-alpha.24 looks goodMatches the monorepo release bump context.
tests/e2e/package.json (1)
3-3: All package.json versions are 3.0.0-alpha.24 — no mismatches foundQuick check completed: the repo has 17 package.json files and all are set to 3.0.0-alpha.24.
Files scanned (all matched):
- package.json
- packages/cli/package.json
- packages/common-helpers/package.json
- packages/create-zenstack/package.json
- packages/dialects/sql.js/package.json
- packages/eslint-config/package.json
- packages/ide/vscode/package.json
- packages/language/package.json
- packages/runtime/package.json
- packages/sdk/package.json
- packages/tanstack-query/package.json
- packages/testtools/package.json
- packages/typescript-config/package.json
- packages/vitest-config/package.json
- packages/zod/package.json
- samples/blog/package.json
- tests/e2e/package.json
No changes required.
packages/zod/package.json (1)
3-3: LGTM on version bumpNo other changes; aligns with the release bump.
packages/eslint-config/package.json (1)
3-3: LGTM on version bumpPackage remains private; no functional impact beyond versioning.
packages/sdk/package.json (1)
3-3: Version bump looks goodNo manifest inconsistencies spotted. This aligns with the repo-wide alpha.24 release.
package.json (1)
3-3: Root version bump verified — all packages are 3.0.0-alpha.24Checks run against the repo show:
- All package.json files (root, packages, samples, tests) report "version": "3.0.0-alpha.24".
- No occurrences of "alpha.23" remain.
- All internal @zenstackhq/* dependencies use workspace:*.
Snippet (unchanged):
"version": "3.0.0-alpha.24",packages/common-helpers/package.json (1)
3-3: Version bump alignedManifest remains unchanged otherwise. No issues.
packages/language/package.json (1)
4-4: Version bump acknowledgedNo functional/package structure changes here; safe to proceed.
packages/create-zenstack/package.json (1)
3-3: Version bump to 3.0.0-alpha.24: LGTMChange is metadata-only and consistent with the PR’s release bump.
packages/testtools/package.json (1)
3-3: Version bump to 3.0.0-alpha.24: LGTMNo functional changes; aligns with monorepo versioning.
packages/cli/package.json (1)
6-6: Version bump to 3.0.0-alpha.24: LGTMCLI manifest change only; consistent with the coordinated prerelease.
packages/vitest-config/package.json (1)
4-4: Version bump to 3.0.0-alpha.24: LGTMPrivate package; safe metadata update.
packages/runtime/package.json (1)
3-3: Runtime package version bump verified — monorepo alignedAll package.json files under packages/, samples/, and tests/ are set to 3.0.0-alpha.24 and all internal @zenstackhq/* dependencies use workspace:* — no action required.
- Primary file checked: packages/runtime/package.json
"version": "3.0.0-alpha.24",packages/typescript-config/package.json (1)
3-3: Version bump looks goodPackage version updated to 3.0.0-alpha.24 as part of the release sweep. No concerns.
packages/dialects/sql.js/package.json (1)
3-3: Version bump looks goodPackage version updated to 3.0.0-alpha.24 in sync with the monorepo. No concerns.
packages/tanstack-query/package.json (1)
3-3: Version bump looks goodPackage version updated to 3.0.0-alpha.24; no functional changes. No concerns.
.vscode/launch.json (3)
13-13: No-op formatting change to argsCollapsing the args array to one line is fine; no functional impact.
43-45: Changing cwd for TSX debug could affect runtime behaviorSetting cwd to ${fileDirname} changes process.cwd() and can influence relative path resolution and tsconfig discovery for tsx. If the current file lives deep under the repo, this can be desirable for relative file reads, but it may break code that expects cwd to be the repo root (e.g., reading configs, schema files, or relying on workspace-root-relative paths).
- If you mainly need per-file relative reads, this is fine.
- If some scripts expect workspace-root cwd, consider keeping cwd as ${workspaceFolder} and passing explicit paths, or set env vars to reference the workspace root.
Please verify typical debug flows (blog sample, CLI entry points) still work as expected with this change.
50-50: No-op formatting change to argsCollapsing the args array to one line for the extension host is fine; no functional impact.
packages/runtime/src/client/client-impl.ts (1)
1-1: Import cleanup looks goodUsing invariant from @zenstackhq/common-helpers is appropriate; no issues.
packages/runtime/src/client/contract.ts (1)
773-777: Deriving ModelOperations from AllModelOperations is a good consolidation.Using
Omitto exclude delegate-inapplicable ops centralizes the surface and removes duplication. This improves maintainability and consistency.packages/runtime/src/client/executor/zenstack-query-executor.ts (4)
233-234: Correct hook presence check for new API.Switching
hasMutationHooksto detectonEntityMutationaligns with the new plugin shape.
275-293: Confirm default interception semantics when filter is absent.When a plugin defines
onEntityMutationbut notmutationInterceptionFilter, the executor defaults tointercept = true(without loading entities). This changes behavior vs “no interception” and will run before/after hooks for every mutation.If this is intentional for backward-compatibility, consider adding an inline comment explaining the rationale to avoid confusion for future maintainers.
Do the tests cover:
- a plugin with
onEntityMutationbut nomutationInterceptionFilterreceiving before/after hooks; and- a plugin with
mutationInterceptionFilterreturningintercept: falsenot receiving hooks?
323-331: Before-mutation hooks: correct routing via onEntityMutation.beforeEntityMutation.Invocation and payload composition look consistent with the new types.
349-352: After-mutation hooks: collection and binding updated correctly.Collecting hooks from
onEntityMutation.afterEntityMutationand binding to plugin preserves prior semantics.packages/runtime/src/client/plugin.ts (1)
10-47: New RuntimePlugin contract and definePlugin helper look solid.The top-level
onQuery, nestedonEntityMutation, andonKyselyQueryprovide a clean, unified plugin API.definePluginis a small but useful DX improvement.packages/runtime/test/plugin/on-kysely-query.test.ts (1)
7-7: Test description rename is fine.Minor wording tweak; no behavioral impact.
packages/runtime/test/plugin/on-query-hooks.test.ts (2)
186-209: LGTM: plugin encapsulation via definePlugin is correctGood use of definePlugin with schema typing and per-client installation. This validates plugin-scoped behavior as intended.
116-122: Custom matcher present — no change neededI ran a repo search and found the custom matchers implemented and typed, so .toResolveNull is registered.
- packages/runtime/test/vitest-ext.ts — expect.extend(...) defines toResolveTruthy, toResolveNull, toResolveWithLength (matcher implementations).
- packages/runtime/test/vitest.d.ts — type declarations for toResolveTruthy, toResolveFalsy, toResolveNull, toResolveWithLength, etc.
packages/runtime/test/plugin/entity-mutation-hooks.test.ts (1)
313-332: Intentional behavior: afterEntityMutation errors should not affect DB operationThis test asserts that throwing in afterEntityMutation does not propagate nor roll back the operation. It’s a good contract check and complements the onQuery behavior test where you explicitly expect a throw to surface. No change needed.
If the intended contract changes in the future (e.g., surfacing errors), this test must be updated accordingly.
packages/runtime/test/policy/client-extensions.test.ts (1)
26-30: Guard against spreading undefined where clauseSpreading undefined throws at runtime. Use nullish coalescing to default to an empty object.
- onQuery: async ({ args, query }: any) => { - args = args ?? {}; - args.where = { ...args.where, y: { lt: 300 } }; - return query(args); - }, + onQuery: async ({ args, query }: any) => { + args = args ?? {}; + args.where = { ...(args.where ?? {}), y: { lt: 300 } }; + return query(args); + },Likely an incorrect or invalid review comment.
README.md (3)
290-299: Great: onQuery example reflects the new unified signatureThe code sample correctly demonstrates the unified top-level onQuery hook with model, operation, args, and query.
346-369: Solid: onEntityMutation examples match the new nested APIThe examples clearly illustrate mutationInterceptionFilter, beforeEntityMutation, and afterEntityMutation under onEntityMutation with load-before/after options.
412-413: Potential doc inconsistency: raw SQL API supportDocs state raw SQL APIs are not supported, but the client contract appears to expose $queryRaw/$executeRaw. Please confirm and update the docs or implementation accordingly.
If the APIs are implemented/available behind a feature flag, clarify their current support status here to avoid confusion.
* chore: rename query function to proceed for onquery plugin * update test
|
Claude finished @ymc9's task —— View job Pull Request Review Complete ✅Reviewing PR #166: merge dev to main (3.0.0-alpha.24 release) Review Checklist:
📋 SummaryThis is a significant alpha release that includes major plugin system refactoring for TypeScript performance improvements. While the changes are well-structured, there are some important considerations for the team.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/runtime/src/client/client-impl.ts (1)
361-373: Post-processing uses stale args; use effective args after plugin overrides.If a plugin overrides args, result post-processing should reflect the actual args used for execution. Currently it always uses the original args.
- let proceed = async (_args?: unknown) => { - const _handler = txClient ? handler.withClient(txClient) : handler; - const r = await _handler.handle(operation, _args ?? args); + let proceed = async (_args?: unknown) => { + const effectiveArgs = _args ?? args; + const _handler = txClient ? handler.withClient(txClient) : handler; + const r = await _handler.handle(operation, effectiveArgs); if (!r && throwIfNoResult) { throw new NotFoundError(model); } let result: unknown; if (r && postProcess) { - result = resultProcessor.processResult(r, model, args); + result = resultProcessor.processResult(r, model, effectiveArgs); } else { result = r ?? null; } return result; };
♻️ Duplicate comments (2)
packages/runtime/test/plugin/on-query-hooks.test.ts (1)
3-3: Add afterEach to close the in-memory SQLite connection (duplicate of prior feedback).Each test opens a new DB; none closes it. Add afterEach and import it from vitest to avoid leaking connections and keep suites consistent.
-import { beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; @@ beforeEach(async () => { _client = new ZenStackClient(schema, { dialect: new SqliteDialect({ database: new SQLite(':memory:') }), }); await _client.$pushSchema(); }); + + afterEach(async () => { + await _client?.$disconnect(); + });Also applies to: 10-15
packages/runtime/src/client/client-impl.ts (1)
379-383: onQuery wrapper drops overridden args and breaks multi-plugin propagation.The wrapper replaces proceed with a zero-arg function and passes the original args into onQuery. Later plugins cannot propagate modified args to earlier ones.
- const onQuery = plugin.onQuery; - if (onQuery) { - const _proceed = proceed; - proceed = () => onQuery({ client, model, operation, args, proceed: _proceed }) as Promise<unknown>; - } + const onQuery = plugin.onQuery; + if (onQuery) { + const _proceed = proceed; + // preserve proceed's signature and propagate arg overrides across the chain + proceed = (_args?: unknown) => + onQuery({ + client, + model, + operation, + args: _args ?? args, + proceed: (nextArgs: unknown) => _proceed(nextArgs ?? _args ?? args), + }) as Promise<unknown>; + }
🧹 Nitpick comments (3)
packages/runtime/src/client/client-impl.ts (1)
376-384: Document plugin invocation order for predictability.Clarify that the last registered plugin is invoked first, which matters for composing behaviors.
- // apply plugins + // apply plugins (the last registered plugin is invoked first) const plugins = [...(client.$options.plugins ?? [])];packages/runtime/src/client/plugin.ts (2)
124-128: Fix doc comment for queryNode to reflect actual semantics.The current comment suggests it's only for create/update, but the type exists for all actions and represents the Kysely AST node.
- /** - * The mutation data. Only available for create and update actions. - */ + /** + * Mutation query AST node (Kysely OperationNode). + * Note: available for all actions; contents vary by action. + */ queryNode: OperationNode;
188-194: Preserve generic Schema on OnKyselyQueryArgs.schema for stronger typing.Use Schema instead of SchemaDef to keep the actual schema type.
export type OnKyselyQueryArgs<Schema extends SchemaDef> = { kysely: ToKysely<Schema>; - schema: SchemaDef; + schema: Schema; client: ClientContract<Schema>; query: RootOperationNode; proceed: ProceedKyselyQueryFunction; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)packages/runtime/src/client/client-impl.ts(2 hunks)packages/runtime/src/client/plugin.ts(2 hunks)packages/runtime/test/plugin/on-query-hooks.test.ts(1 hunks)packages/runtime/test/policy/client-extensions.test.ts(5 hunks)samples/blog/main.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- samples/blog/main.ts
- packages/runtime/test/policy/client-extensions.test.ts
- README.md
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/runtime/src/client/client-impl.tspackages/runtime/test/plugin/on-query-hooks.test.tspackages/runtime/src/client/plugin.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Plugin Architecture: Runtime plugins for query interception and entity mutation hooks
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Plugin Architecture: Runtime plugins for query interception and entity mutation hooks
Applied to files:
packages/runtime/src/client/client-impl.tspackages/runtime/test/plugin/on-query-hooks.test.tspackages/runtime/src/client/plugin.ts
🧬 Code Graph Analysis (2)
packages/runtime/test/plugin/on-query-hooks.test.ts (3)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/src/client/client-impl.ts (1)
ZenStackClient(40-46)packages/runtime/src/client/plugin.ts (1)
definePlugin(45-47)
packages/runtime/src/client/plugin.ts (5)
packages/sdk/src/schema/schema.ts (2)
SchemaDef(10-18)GetModels(108-108)packages/runtime/src/client/crud/operations/base.ts (1)
CrudOperation(54-69)packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/src/utils/type-utils.ts (1)
MaybePromise(70-70)packages/runtime/src/client/query-builder.ts (1)
ToKysely(19-19)
⏰ 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 (20.x)
- GitHub Check: claude-review
|
Claude finished @ymc9's task —— View job Pull Request Review Complete ✅Reviewing PR #166: merge dev to main (3.0.0-alpha.24 release) Review Checklist:
📋 SummaryThis is a significant alpha release that includes major plugin system refactoring for TypeScript performance improvements. While the changes are well-structured, there are some important considerations for the team.
|
|
Claude finished @ymc9's task —— View job Pull Request Review for #166: merge dev to main (v3.0.0-alpha.24) ✅Review Checklist:
📋 SummaryThis is a significant alpha release that includes major plugin system refactoring for TypeScript performance improvements. While the changes are well-structured, there are some important considerations for the team.
|
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Chores