refactor: remove filter from entity mutation hooks #208
Conversation
|
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. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors mutation handling by replacing the mutationInterceptionFilter flow with explicit before/after entity mutation hooks, introducing MutationInfo and queryId correlation, adjusting transaction behavior for after hooks, augmenting return payloads for certain mutations, and updating plugin types and tests to the new load-based entity retrieval APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ 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 PR Review: Remove Filter from Entity Mutation HooksI've analyzed the changes in this refactoring PR. This is a significant simplification that removes the complexity around filtering and conditional loading in entity mutation hooks. Todo List:
SummaryThis PR removes the
Code Quality & Best Practices ✅Strengths:
Observations:
Potential Issues & Bugs 🟡Minor concerns:
Performance Considerations ✅Improvements:
Potential concerns:
Security Concerns ✅No security issues identified. The refactor:
Test Coverage 🟡Test updates align well with changes:
Observation: Specific Code Review
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors entity mutation hooks by removing the filter mechanism and simplifying the API. The mutation interception filter that determined whether mutations should be intercepted and what data to load has been replaced with direct function calls for loading entities.
- Removes the
mutationInterceptionFilterfunction and related types from the plugin system - Replaces pre-loaded entity data with lazy-loading functions (
loadBeforeMutationEntitiesandloadAfterMutationEntities) - Moves the
runAfterMutationWithinTransactionoption from the filter result to the top-level plugin configuration
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/runtime/test/plugin/entity-mutation-hooks.test.ts | Updates tests to use new lazy-loading API and removes filter-based test cases |
| packages/runtime/src/client/plugin.ts | Removes mutation interception filter types and updates plugin interface to use lazy-loading functions |
| packages/runtime/src/client/executor/zenstack-query-executor.ts | Refactors executor to remove filter logic and implement lazy entity loading |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/runtime/src/client/executor/zenstack-query-executor.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Claude finished @ymc9's task —— View job PR Review: Remove Filter from Entity Mutation HooksI've analyzed the changes in this refactoring PR. This is a significant simplification that removes the complexity around filtering and conditional loading in entity mutation hooks. Todo List:
SummaryThis PR removes the
Code Quality & Best Practices ✅Strengths:
Observations:
Potential Issues & Bugs 🟡Minor concerns:
Performance Considerations ✅Improvements:
Potential concerns:
Security Concerns ✅No security issues identified. The refactor:
Test Coverage 🟡Test updates align well with changes:
Observation: Specific Code Review
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
505-513: Incorrect composition of WHERE nodes (likely malformed AST and wrong results)andNodes is composing WhereNode instances directly inside AndNode.create and WhereNode.create. Kysely’s AndNode expects boolean-expression OperationNode(s), not WhereNode wrappers. This will produce an invalid AST and can lead to wrong or failing queries when loading pre-mutation entities.
Apply this diff:
- private andNodes(condition1: WhereNode | undefined, condition2: WhereNode | undefined) { - if (condition1 && condition2) { - return WhereNode.create(AndNode.create(condition1, condition2)) - } else if (condition1) { - return WhereNode.create(condition1) - } else { - return condition2 - } - } + private andNodes(condition1: WhereNode | undefined, condition2: WhereNode | undefined) { + if (condition1 && condition2) { + // Merge the inner predicates, not the WhereNode wrappers + return WhereNode.create(AndNode.create(condition1.where!, condition2.where!)); + } else if (condition1) { + return condition1; + } else { + return condition2; + } + }Additionally, consider guarding condition1.where/condition2.where with invariant checks to catch unexpected undefineds.
packages/runtime/test/plugin/entity-mutation-hooks.test.ts (1)
374-376: Comment contradicts expectationThe comment says “mutation is persisted” but the assertion expects length 0. Since the before hook throws, the mutation should be aborted. Fix the comment.
- // mutation is persisted + // mutation is not persisted
♻️ Duplicate comments (1)
packages/runtime/test/plugin/entity-mutation-hooks.test.ts (1)
300-316: Parameter naming consistency: prefer args over ctxThere’s a mix of ctx and args across tests. For consistency and readability (matching other newer tests), consider standardizing on args.
Example:
- async beforeEntityMutation(ctx) { - await ctx.client.profile.create({ + async beforeEntityMutation(args) { + await args.client.profile.create({ data: { bio: 'Bio1' }, }); },
🧹 Nitpick comments (8)
packages/runtime/src/client/plugin.ts (5)
126-136: Docs inconsistent: queryNode availability for delete mutationsThe doc on Line 127 says “Only available for create and update actions,” but both before/after hooks receive queryNode for delete as well (executor passes it for all three actions). Please update the docs for accuracy.
Apply this diff to adjust the docs:
- /** - * The mutation data. Only available for create and update actions. - */ + /** + * The mutation AST node. Available for all mutation actions (create, update, delete). + * For deletes, consumers typically use the WHERE clause to derive affected entities. + */
101-113: Clarify transactional semantics w.r.t. before hooksThe description is precise for after hooks. Consider adding one sentence to avoid ambiguity that beforeEntityMutation does NOT run inside the on-the-fly transaction created for after hooks; it only runs inside an outer transaction if one already exists.
Proposed doc tweak:
/** * Whether to run after-mutation hooks within the transaction that performs the mutation. @@ - * Defaults to `false`. + * Defaults to `false`. + * + * Note: beforeEntityMutation always executes before the mutation. It runs inside a transaction + * only if the mutation is already running inside one (i.e., when there is an outer transaction). + * An on-the-fly transaction is not created for the before hook. */
132-136: Document queryId uniqueness expectationsqueryId is great for correlating before/after invocations. It’s worth documenting that it’s unique per mutation operation to avoid misuse.
- /** - * A query ID that uniquely identifies the mutation operation. You can use it to correlate - * data between the before and after mutation hooks. - */ + /** + * A query ID that uniquely identifies a single mutation operation. + * Use it to correlate data between the before and after mutation hooks. + * The ID is unique per mutation invocation. + */
146-151: Spell out semantics for create vs update/delete in loaderloadBeforeMutationEntities returns undefined for creates. Consider making that explicit in the doc.
- /** - * Loads the entities that are about to be mutated. - */ + /** + * Loads the entities that are about to be mutated. + * - For update/delete: returns the rows matching the mutation's WHERE. + * - For create: returns undefined. + */
162-175: Be explicit that delete returns undefined for after loaderThe executor returns undefined for deletes. Small doc tweak improves clarity and avoids surprises.
- /** - * Loads the entities that have been mutated. - */ + /** + * Loads the entities that have been mutated. + * - For create/update: returns the rows affected by the mutation. + * - For delete: returns undefined. + */packages/runtime/src/client/executor/zenstack-query-executor.ts (2)
152-163: RETURNING * injection: confirm cross-dialect behavior and provide fallbackInjecting returning: select * for insert/update is great for Postgres/SQLite. For MySQL/MariaDB, RETURNING is not supported and queryResult.rows may be empty, breaking loadAfterMutationEntities for those dialects.
- Verify driver behavior for MySQL-family.
- If unsupported, gate the injection behind a capability check (e.g., driver.supportsReturning) and consider a fallback:
- For update: load affected entities via WHERE (like before-loader) after execution.
- For create: if unique fields are present in data, re-load via those; otherwise document the limitation in plugin.ts.
I can help draft a capability flag on ZenStackDriver and wire a fallback if desired.
Also applies to: 467-474
465-485: Minor: avoid recomputing model in after hooksYou already have mutationInfo.model at the call site. Avoid recomputing with getMutationModel(queryNode).
- const mutationModel = this.getMutationModel(queryNode); + const mutationModel = mutationInfo.model;packages/runtime/test/plugin/entity-mutation-hooks.test.ts (1)
71-114: Great coverage of queryId correlation; consider adding uniqueness check across operationsYou already assert before/after equality per operation. As a small enhancement, also assert that update and delete use different IDs to catch accidental reuse.
For example, after existing assertions:
expect(queryIds.update.before).toBe(queryIds.update.after); expect(queryIds.delete.before).toBe(queryIds.delete.after); + expect(queryIds.update.before).not.toBe(queryIds.delete.before);
📜 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 (3)
packages/runtime/src/client/executor/zenstack-query-executor.ts(10 hunks)packages/runtime/src/client/plugin.ts(4 hunks)packages/runtime/test/plugin/entity-mutation-hooks.test.ts(13 hunks)
🧰 Additional context used
🧠 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/executor/zenstack-query-executor.tspackages/runtime/src/client/plugin.ts
⏰ 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 (4)
packages/runtime/src/client/plugin.ts (1)
89-99: API shape looks solid; clear split of before/after hooks and transactional option
- The move from a filter to explicit before/after hooks is clearer and easier to reason about.
- The new runAfterMutationWithinTransaction flag gives good, explicit control over consistency vs. isolation.
Also applies to: 100-113
packages/runtime/test/plugin/entity-mutation-hooks.test.ts (3)
116-137: Nice validation of loadAfterMutationEntitiesGood job asserting concrete fields for create/update flows with the new loader API.
250-294: Good end-to-end check for multiple mutations in one transactionThis test nicely verifies ordering and payload shape for create/update/delete. Clear expectations.
486-513: Before-loader visibility within outer transaction is exercised wellThis confirms the loader sees uncommitted changes inside the transaction. Nice.
|
Claude finished @ymc9's task —— View job PR Review: Remove Filter from Entity Mutation HooksI've analyzed the changes in this refactoring PR. This is a significant simplification that removes the complexity around filtering and conditional loading in entity mutation hooks. Todo List:
SummaryThis PR removes the
Code Quality & Best Practices ✅Strengths:
Observations:
Potential Issues & Bugs 🟡Minor concerns:
Performance Considerations ✅Improvements:
Potential concerns:
Security Concerns ✅No security issues identified. The refactor:
Test Coverage 🟡Test updates align well with changes:
Observation: Specific Code Review
|
Summary by CodeRabbit