merge dev to main #209
merge dev to main #209
Conversation
Co-authored-by: ymc9 <[email protected]>
* refactor: remove filter from entity mutation hooks * addressing comments * update
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. ✨ 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 ReviewI've completed a comprehensive review of this dev -> main merge PR. Here are my findings: Review Tasks:
✅ Code Quality & Best PracticesPositive aspects:
|
There was a problem hiding this comment.
Pull Request Overview
This PR merges dev branch to main, updating package versions from 3.0.0-alpha.31 to 3.0.0-alpha.32 across the entire project and includes significant enhancements to the entity mutation hooks system.
- Version bump for all packages to alpha.32
- Major refactoring of entity mutation hooks to support more flexible async data loading patterns
- Enhanced testing coverage with multi-provider support (SQLite and PostgreSQL)
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Version bump to 3.0.0-alpha.32 |
| packages/*/package.json | Consistent version updates across all packages |
| packages/runtime/src/client/plugin.ts | API redesign for entity mutation hooks with new async loading functions |
| packages/runtime/src/client/executor/zenstack-query-executor.ts | Complete refactor of mutation hook execution logic with transaction support |
| packages/runtime/test/plugin/entity-mutation-hooks.test.ts | Extensive test updates with multi-provider support and new hook functionality |
| packages/runtime/test/client-api/name-mapping.test.ts | Test updates for multi-provider support |
| TODO.md | Documentation updates reflecting completed features |
| .coderabbit.yaml | Configuration update for code review base branches |
Comments suppressed due to low confidence (1)
packages/runtime/src/client/executor/zenstack-query-executor.ts:84
- The error handling strategy for commit callbacks should be more explicit. Consider adding configuration options to control whether commit callback errors should fail the transaction or just be logged, as different applications may have different requirements.
const hooks: OnKyselyQueryCallback<Schema>[] = [];
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (19)
packages/tanstack-query/package.json (1)
4-7: Optional: add a short descriptionEven though the package is private, a brief description helps new contributors and improves workspace listing clarity.
- "description": "", + "description": "Adapters/utilities for integrating ZenStack with TanStack Query (React Query)",packages/runtime/src/client/crud/operations/base.ts (1)
292-293: Default isolation level: sensible, but confirm dialect support pathsDefaulting to Repeatable Read is reasonable. For engines that don’t support configurable isolation (e.g., some SQLite setups), Kysely typically no-ops, but verify that our drivers don’t throw. If there’s any risk, consider conditionally setting the isolation level based on dialect.
packages/runtime/src/client/executor/zenstack-driver.ts (1)
82-87: Swallowing post-commit callback errors hides failures; log via driver and consider surfacing aggregated errorsCatching and
console.error-ing commit-callback failures prevents callers and observability pipelines from noticing issues. Two improvements:
- Use the driver’s logger (
#log.error) for consistency with query/error logs.- Optionally aggregate callback errors and expose them (e.g., by throwing a non-fatal
PostCommitCallbackErrorafter commit or returning them via a side-channel) so callers can act on them while keeping the commit intact.- } catch (err) { - // errors in commit callbacks are logged but do not fail the commit - console.error(`Error executing transaction commit callback: ${err}`); - } + } catch (error) { + // errors in commit callbacks are logged but do not fail the commit + await this.#log.error(() => ({ + level: 'error', + message: 'Error executing transaction commit callback', + error, + })); + }Please confirm behavior change is intentional and documented as a breaking semantics change (callbacks no longer abort commit). Also ensure tests cover both: (1) commit succeeds, (2) callback failures are observable via logging.
packages/runtime/src/client/executor/kysely-utils.ts (1)
1-14: Good centralization of alias handling; add explicit return type for stronger contractsThe helper looks solid and the invariant improves safety. Minor polish: declare the return type to help downstream inference and avoid accidental widening.
-export function stripAlias(node: OperationNode) { +export function stripAlias(node: OperationNode): { alias?: string; node: OperationNode } { if (AliasNode.is(node)) { invariant(IdentifierNode.is(node.alias), 'Expected identifier as alias'); return { alias: node.alias.name, node: node.node }; } else { return { alias: undefined, node }; } }packages/runtime/src/client/executor/name-mapper.ts (2)
98-106: Prefer safe narrowing without non-null assertion in transformJoin
innerNode!is unnecessary and weakens type safety. Let the type guard handle undefined.- const { alias, node: innerNode } = stripAlias(node.table); - if (TableNode.is(innerNode!)) { + const { alias, node: innerNode } = stripAlias(node.table); + if (innerNode && TableNode.is(innerNode)) { const modelName = innerNode.table.identifier.name; if (this.hasMappedColumns(modelName)) { // create a nested query with all fields selected and names mapped const select = this.createSelectAll(modelName); return { ...super.transformJoin(node), table: this.wrapAlias(select, alias ?? modelName) }; } }
301-315: createScopesFromFroms: clear scope extraction; minor optional micro-optimizationImplementation is fine. If desired, you can avoid the
filter(Boolean)cast by returningScope | undefinedtype and narrowing, but this is purely optional here.packages/runtime/test/client-api/name-mapping.test.ts (2)
7-8: Use per-provider, unique database names to avoid cross-run/test interferenceA single
TEST_DBfor both sqlite and postgresql risks collisions across providers or parallel runs. Consider parameterizing by provider and adding a random/worker suffix.-const TEST_DB = 'client-api-name-mapper-test'; +const TEST_DB_BASE = 'client-api-name-mapper-test';And inside each provider block (see the beforeEach below):
const dbName = `${TEST_DB_BASE}-${provider}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;Then pass
dbNametocreateTestClient.If your CI runs tests in parallel, this change prevents spurious failures due to DB reuse.
146-158: Reduce flakiness: don’t couple Post.id to User.id in nested select assertion
SELECT ... FROM Postreturningidcompared touser.idassumes both sequences align. It happens to pass with fresh DBs, but it’s brittle. Assertexpect.any(Number)instead.- ).resolves.toMatchObject({ - id: user.id, - title: 'Post1', - email: '[email protected]', - }); + ).resolves.toMatchObject({ + id: expect.any(Number), + title: 'Post1', + email: '[email protected]', + });packages/runtime/test/plugin/entity-mutation-hooks.test.ts (4)
7-8: DB name reuse may cause cross-test interference on PostgreSQL in parallel runsUsing a single constant DB name for all tests can lead to DROP/CREATE clashes or flaky isolation when tests run in parallel across files/workers. Consider appending a per-worker suffix (e.g., process.env.VITEST_WORKER_ID) or a timestamp for Postgres runs, while still cleaning up in beforeEach/afterAll to avoid DB bloat.
Would you confirm your Vitest concurrency settings and whether other test files reuse the same TEST_DB? If parallelism is enabled, I can propose a worker-scoped naming helper.
Example patch:
-const TEST_DB = 'client-api-entity-mutation-hooks-test'; +const TEST_DB = `client-api-entity-mutation-hooks-test-${process.env.VITEST_WORKER_ID ?? 'w0'}`;
14-18: Potentially heavy Postgres setup in beforeEachcreateTestClient drops/recreates the database for provider='postgresql' on each test. Great for isolation, but slow for large suites. If runtime allows, moving DB bootstrap to beforeAll and truncating tables between tests can significantly speed up CI. Keeping it as-is is acceptable if current runtime is within SLA.
210-248: Avoid relying on index 0 for nested after-entitiesUsing afterEntities![0] assumes a single-entity result and ordering. Prefer asserting arrayContaining with title match to avoid brittleness if future changes return multiple rows or different order.
Example:
-const afterEntities = await args.loadAfterMutationEntities(); -if ((afterEntities![0] as any).title === 'Post1') { ... } +const afterEntities = (await args.loadAfterMutationEntities()) ?? []; +if (afterEntities.some((e: any) => e.title === 'Post1')) { ... }
358-376: Comment contradicts the assertion; fix wordingThe comment says "mutation is persisted" but the assertion checks that nothing was persisted. Update the comment for clarity.
- // mutation is persisted + // mutation is NOT persistedpackages/runtime/src/client/plugin.ts (4)
101-113: Document cross-plugin semantics for runAfterMutationWithinTransactionBehavior in a mixed-plugin environment is “any-true implies in-tx execution is needed.” Consider adding a sentence: “If any installed plugin sets runAfterMutationWithinTransaction=true, the executor will run the in-transaction portion for all such plugins and defer others until after commit.”
/** * Whether to run after-mutation hooks within the transaction that performs the mutation. @@ - * Defaults to `false`. + * Defaults to `false`. If multiple plugins are installed, the executor will: + * - run callbacks of plugins with `runAfterMutationWithinTransaction=true` inside the mutation transaction, and + * - run callbacks of other plugins after the transaction commits.
126-136: Clarify queryNode documentationThe current text suggests availability only for create/update, but tests assert it for delete too. Reword to avoid confusion.
- /** - * The mutation data. Only available for create and update actions. - */ + /** + * The mutation query node (InsertQueryNode | UpdateQueryNode | DeleteQueryNode). + * For delete operations, the node represents the delete statement. + */
146-161: Explain return for create in loadBeforeMutationEntitiesloadBeforeMutationEntities() returns undefined for create actions. Making this explicit helps plugin authors.
/** - * Loads the entities that are about to be mutated. The db operation that loads the entities is executed + * Loads the entities that are about to be mutated. The db operation that loads the entities is executed * within the same transaction context as the mutation. */ loadBeforeMutationEntities(): Promise<Record<string, unknown>[] | undefined>; @@ - * Mutations initiated from this client will NOT trigger entity mutation hooks to avoid infinite loops. + * Mutations initiated from this client will NOT trigger entity mutation hooks to avoid infinite loops. + * + * Note: for "create" actions there are no pre-existing entities; this method resolves to `undefined`.
163-176: Mention delete semantics in loadAfterMutationEntities docsFor delete operations, loadAfterMutationEntities() returns undefined. Worth stating explicitly.
/** - * Loads the entities that have been mutated. + * Loads the entities that have been mutated. + * Note: for "delete" actions this resolves to `undefined`. */packages/runtime/src/client/executor/zenstack-query-executor.ts (3)
233-244: Guard async post-commit callbacks to avoid unhandled rejectionsIf registerTransactionCommitCallback doesn’t await the callback, rejections can become unhandled. Wrap with try/catch.
- this.driver.registerTransactionCommitCallback(connection, () => - this.callAfterMutationHooks( - result, - finalQuery, - mutationInfo, - connectionClient, - 'outTx', - queryId, - ), - ); + this.driver.registerTransactionCommitCallback(connection, async () => { + try { + await this.callAfterMutationHooks( + result, + finalQuery, + mutationInfo, + connectionClient, + 'outTx', + queryId, + ); + } catch { + // optionally route to a logger + } + });
288-289: Avoid leaking full error objects in QueryError messageInterpolating the entire error object may expose stack details in logs. Use err.message when available.
- const message = `Failed to execute query: ${err}, sql: ${compiled?.sql}`; + const message = `Failed to execute query: ${ + err instanceof Error ? err.message : String(err) + }, sql: ${compiled?.sql}`;
487-503: Where-node reuse may break if mutation WHERE contains table aliasesloadEntities merges the mutation’s WhereNode into a fresh selectFrom(model) without aliasing. If WHERE predicates reference aliased tables/columns, the merged WhereNode might not bind correctly. Consider normalizing references (e.g., removing table aliases for the target model) before merging, or building the select by cloning the original table/alias context.
I can prototype a utility to rebind column references for the target model if you’d like.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
.coderabbit.yaml(1 hunks)TODO.md(3 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/crud/operations/base.ts(2 hunks)packages/runtime/src/client/executor/kysely-utils.ts(1 hunks)packages/runtime/src/client/executor/name-mapper.ts(6 hunks)packages/runtime/src/client/executor/zenstack-driver.ts(1 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(12 hunks)packages/runtime/src/client/plugin.ts(3 hunks)packages/runtime/test/client-api/name-mapping.test.ts(2 hunks)packages/runtime/test/plugin/entity-mutation-hooks.test.ts(1 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/package.json(1 hunks)tests/e2e/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/cli/package.jsonpackages/runtime/package.jsonpackages/testtools/package.jsonpackages/zod/package.jsonpackages/typescript-config/package.jsonpackages/eslint-config/package.jsonpackages/dialects/sql.js/package.jsonpackages/ide/vscode/package.jsonpackages/runtime/src/client/crud/operations/base.tspackages/sdk/package.jsonpackages/language/package.jsonpackages/tanstack-query/package.jsonpackages/runtime/src/client/executor/zenstack-driver.tspackages/runtime/src/client/executor/kysely-utils.tspackages/create-zenstack/package.jsonpackages/vitest-config/package.jsonpackages/runtime/src/client/client-impl.tssamples/blog/package.jsontests/e2e/package.jsonpackages/runtime/test/client-api/name-mapping.test.tspackages/runtime/src/client/executor/zenstack-query-executor.tspackages/runtime/test/plugin/entity-mutation-hooks.test.tspackages/common-helpers/package.jsonpackages/runtime/src/client/plugin.tspackages/runtime/src/client/executor/name-mapper.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests are in
tests/e2e/directory
Files:
tests/e2e/package.json
🧠 Learnings (4)
📚 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
📚 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: Kysely-Based ORM: V3 uses Kysely as query builder instead of Prisma runtime dependency
Applied to files:
packages/runtime/src/client/client-impl.ts
📚 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: Language-First Design: ZModel DSL compiles to TypeScript, not runtime code generation
Applied to files:
TODO.md
📚 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/executor/zenstack-query-executor.tspackages/runtime/src/client/plugin.ts
🧬 Code graph analysis (6)
packages/runtime/src/client/executor/kysely-utils.ts (1)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/test/client-api/name-mapping.test.ts (3)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/schemas/name-mapping/schema.ts (1)
schema(9-87)packages/runtime/test/utils.ts (1)
createTestClient(78-194)
packages/runtime/src/client/executor/zenstack-query-executor.ts (4)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/executor/kysely-utils.ts (1)
stripAlias(7-14)packages/runtime/src/client/plugin.ts (1)
AfterEntityMutationCallback(142-144)
packages/runtime/test/plugin/entity-mutation-hooks.test.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/utils.ts (1)
createTestClient(78-194)
packages/runtime/src/client/plugin.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)
packages/runtime/src/client/executor/name-mapper.ts (1)
packages/runtime/src/client/executor/kysely-utils.ts (1)
stripAlias(7-14)
🪛 markdownlint-cli2 (0.17.2)
TODO.md
12-12: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
68-68: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
69-69: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
⏰ 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: build-test (20.x)
🔇 Additional comments (44)
packages/runtime/package.json (1)
3-3: Version bump consistency verifiedAll
package.jsonfiles have been correctly updated to3.0.0-alpha.32. No mismatches were found.• Please ensure the CHANGELOG/release notes document the new runtime public surface (e.g.
forceTransaction, hook changes) and any behavioral nuances for consumers.packages/eslint-config/package.json (1)
3-3: LGTM: version bump onlyNo concerns—private package, scoped manifest change.
packages/tanstack-query/package.json (1)
3-3: LGTM: version bump to 3.0.0-alpha.32No other manifest changes; dependency on @zenstackhq/runtime remains workspace-pinned, which will track the new runtime version.
tests/e2e/package.json (1)
3-3: LGTM: e2e workspace version alignedMatches the repo-wide bump; no script or dependency drift detected in this file.
packages/testtools/package.json (1)
3-3: LGTM: version bump to 3.0.0-alpha.32Peer deps on better-sqlite3/pg unchanged, which is fine for this release. No further action needed here.
packages/vitest-config/package.json (1)
4-4: Version bump to 3.0.0-alpha.32 looks goodNo functional changes; safe metadata update. Matches the monorepo-wide bump.
packages/typescript-config/package.json (1)
3-3: Version alignment confirmedBumped to 3.0.0-alpha.32; consistent with the rest of the repo. No API impact.
packages/language/package.json (1)
4-4: Language package version bump LGTMOnly the version changed to 3.0.0-alpha.32. Exports/files/scripts unchanged; no downstream risk.
packages/create-zenstack/package.json (1)
3-3: CLI package version bump approvedUpdate to 3.0.0-alpha.32 with no behavior changes.
package.json (1)
3-3: Root version bump consistency verifiedNo occurrences of
3.0.0-alpha.31were found in anypackage.jsonacross packages/, samples/, or tests/. All workspaces are now at3.0.0-alpha.32.✅ Version bump approved.
packages/zod/package.json (1)
3-3: Monorepo version consistency verifiedNo remaining references to
3.0.0-alpha.31were found across anypackage.json—all workspace packages now uniformly use3.0.0-alpha.32.samples/blog/package.json (1)
3-3: Version bump verified; sample apps aligned
No leftover references to3.0.0-alpha.31found insamples/ortests/package.json files—everything’s updated to3.0.0-alpha.32.packages/ide/vscode/package.json (1)
4-4: ✅ Extension pre-release version validated and ready for publish
- packages/ide/vscode/package.json:
versionis correctly set to"3.0.0-alpha.32".No further changes detected. You’re all set to publish the pre-release.
packages/common-helpers/package.json (1)
3-3: Approve version bump: common-helpers 3.0.0-alpha.32 verifiedAll checks passed:
- No lingering references to “3.0.0-alpha.31” across any package.json
- Every dependent package (runtime, sdk, language, cli) uses
workspace:*for@zenstackhq/common-helpersSafe to merge.
packages/cli/package.json (1)
6-6: Updated script to verify all @zenstackhq packages are at 3.0.0-alpha.32Version bump to 3.0.0-alpha.32: LGTM
No concerns with the manifest change.Run the following adjusted script to catch any straggling package versions:
#!/usr/bin/env bash set -euo pipefail expected="3.0.0-alpha.32" echo "Packages with version NOT matching ${expected}:" # Find all package.json files outside node_modules/dist find . \ -type f \ -name package.json \ ! -path "*/node_modules/*" \ ! -path "*/dist/*" \ | while read -r file; do name=$(jq -r .name "$file" 2>/dev/null || echo "") version=$(jq -r .version "$file" 2>/dev/null || echo "") if [[ "$name" == "@zenstackhq/"* ]] && [[ "$version" != "$expected" ]]; then printf '%s@%s :: %s\n' "$name" "$version" "$file" fi doneThis uses standard
jqfilters (.nameand.version) and shell pattern matching (avoiding the unsupportedstarts_with) to list any internal package not yet bumped. Please run this and confirm there are no mismatches.packages/dialects/sql.js/package.json (1)
3-3: Version bump: looks goodNothing else changed; dependencies/peers remain stable.
packages/sdk/package.json (1)
3-3: SDK version bump only: LGTMNo API surface or export map changes here.
packages/runtime/src/client/crud/operations/base.ts (1)
25-25: Type alignment confirmed for TransactionIsolationLevelThe string literals in your
TransactionIsolationLevelenum exactly match Kysely’sisolationLevelunion ("read uncommitted" | "read committed" | "repeatable read" | "serializable" | "snapshot"), so there is no subtle type mismatch and no casting is required. (kysely-org.github.io)TODO.md (1)
12-14: ConfigureMD007indent to 4 and address the top-level headingThe lint run confirms that
TODO.mdhas numerous MD007 (unordered-list indentation) violations under the default 2-space indent, plus an MD041 first-line heading error:
- MD041 at line 1: “## V3 Alpha Todo” should be a level-1 heading
- MD007 at many lines: expected 2 spaces but actual 4, 8, or 12 spaces
To align with the file’s existing style and eliminate these errors, update your markdownlint configuration (e.g.
.markdownlint.jsonormarkdownlint-cli2.json) and/or adjust the file:• Option A: Configure the rules
{ + "MD007": { "indent": 4 }, + "MD041": false }This sets list indentation levels to 4 spaces per level and disables the first-line-heading rule.
• Option B: Fix the file
- ## V3 Alpha Todo + # V3 Alpha TodoThen normalize all list items to 4 spaces per indent level (or adjust as needed to match the new rule).
After making your changes, verify no errors remain:
npx -y markdownlint-cli2 TODO.mdpackages/runtime/src/client/client-impl.ts (1)
159-163: I’ve requested the definitions of both theTransactionclass and the$transactionmethod to confirm whether the currentforceTransactiontruly omits transaction semantics and to see how$transactionworks. Once you have those snippets, we can finalize the API redesign recommendation.packages/runtime/src/client/executor/name-mapper.ts (4)
22-22: Importing shared stripAlias is a good moveCentralizing alias logic reduces duplication and keeps invariant checks consistent across transformers.
154-169: Guarding update without table node is correct; nice resilienceThe
!node.tableshort-circuit avoids crashes on atypical update shapes. The subsequentstripAlias+ scope setup looks consistent with other paths.
176-186: transformDeleteQuery: consistent alias handling and table mappingReplacing ad-hoc logic with
stripAliaskeeps behavior uniform across JOIN/FROM/DELETE. Looks good.
318-338: processFrom: alias-preserving nested select is correctThe
ensureAliasfallback to original table name preserves outer references transparently. Good alignment with JOIN and SELECT* expansion logic.packages/runtime/test/client-api/name-mapping.test.ts (3)
26-81: Create path coverage is comprehensive across ORM and QB APIsNice coverage: nested create,
returning, qualifiedreturningwith table prefix, andreturningAll. This should exercise the name-mapper thoroughly.
194-206: Update with alias and returningAll covers the tricky path—goodThis validates
UPDATE ... FROM 'User as u'alias handling through the mapper. Great inclusion.
208-237: Delete coverage looks solid (QB and ORM)Validates name mapping for delete with
returningand relation cleanup through ORM delete. Good end-to-end assertions.packages/runtime/test/plugin/entity-mutation-hooks.test.ts (14)
29-49: Good coverage of query node typing across actionsAsserting Insert/Update/Delete node types in beforeEntityMutation validates the new API surface well. Nice.
71-114: before-entities loader usage is correctThe test exercises loadBeforeMutationEntities() only for update/delete and verifies queryId correlation. This aligns with the new contract.
116-152: after-entities loader usage is correctValidates create/update flows and ensures returned entities reflect post-mutation state. Looks good.
296-357: Transactional behavior without outer tx is well coveredBoth out-of-tx and in-tx after-hook behaviors are validated. Nice separation of concerns.
378-398: After-hook throws out-of-tx: correct persistence semanticsVerifies that mutation persists even if after-hook throws when executed out of transaction. Good.
400-424: In-tx after-hook exception: correct rollback semanticsThe test correctly expects the entire operation to fail and rollback when runAfterMutationWithinTransaction is true. Good coverage.
426-483: Rollback behavior vs. after-hook placement validatedNice pair of tests ensuring afterEntityMutation is skipped on rollback by default, and still invoked when configured to run within the transaction (while effects don’t persist). Solid.
486-513: Visibility of intra-transaction changes before mutationValidates that beforeEntityMutation reads the latest uncommitted state inside the outer transaction. Good.
514-540: Before-hook runs within outer transaction as expectedSide-effects are rolled back with the transaction, as intended. Looks correct.
542-594: Out-of-tx after-hook with outer tx: intended persistence and visibility checksThe test nicely proves that before-hook sees in-tx data and that after-hook runs post-commit and persists. Good.
596-634: In-tx after-hook with outer tx persists as a unitConfirms that the hook runs and persists with the transaction. LGTM.
636-660: Out-of-tx after-hook throwing doesn't rollback committed workGood assertion that both the mutation and side-effects are persisted despite the thrown error in the after-hook. LGTM.
662-689: In-tx after-hook throwing rolls back everythingCorrect expectation for runAfterMutationWithinTransaction: true. Nice.
323-325: Custom matchertoResolveWithLengthis present – no changes requiredThe custom Jest-style matcher
toResolveWithLengthis defined and extended in the test setup, so all usages are valid and don’t need to be replaced:
- Defined in
packages/runtime/test/vitest-ext.tsviaexpect.extend({ async toResolveWithLength(received, length) { … } })- Declared in
packages/runtime/test/vitest.d.tsfor proper typingsAll occurrences of
await expect(...).toResolveWithLength(n)across your tests (including the lines inentity-mutation-hooks.test.tsand the other mentioned locations) are supported by this extension.packages/runtime/src/client/plugin.ts (1)
89-99: API shape looks consistent and minimalThe before/after hooks, queryId correlation, and client-in-transaction semantics are coherent and align with the tests.
packages/runtime/src/client/executor/zenstack-query-executor.ts (2)
317-331: Propagation of suppressMutationHooks is correct across executor factoriesAll withPlugin/withPlugins/withPluginAtFront/withoutPlugins/withConnectionProvider preserve the suppression flag. Good attention to detail.
Also applies to: 333-343, 345-354, 357-367, 369-383
116-135: Hook orchestration and queryId plumbing look solidgetMutationInfo, callBeforeMutationHooks, and callAfterMutationHooks are cohesive and match the new public API contract and tests.
Also applies to: 384-405, 407-431, 433-485
|
Claude finished @ymc9's task —— View job Pull Request ReviewI've completed a comprehensive review of this dev -> main merge PR. Here are my findings: Review Tasks:
✅ Code Quality & Best PracticesPositive aspects:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
7-7: Nit: prefer block-style list for readability and cleaner diffsInline arrays are valid, but block lists make future edits and reviews easier.
Apply this diff:
- base_branches: ['dev', 'main'] + base_branches: + - dev + - main
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.coderabbit.yaml(1 hunks)
⏰ 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: claude-review
- GitHub Check: build-test (20.x)
🔇 Additional comments (1)
.coderabbit.yaml (1)
7-7: Resolved: base_branches correctly nested under reviews.auto_reviewMatches the schema (array of strings under reviews.auto_review.base_branches). Configuration will now be honored. LGTM.
Summary by CodeRabbit