feat(transaction): implement transaction handling with sequential operations and validation#2399
Open
jiashengguo wants to merge 4 commits intodevfrom
Open
feat(transaction): implement transaction handling with sequential operations and validation#2399jiashengguo wants to merge 4 commits intodevfrom
jiashengguo wants to merge 4 commits intodevfrom
Conversation
…rations and validation
Contributor
|
Claude finished @jiashengguo's task —— View job PR Review: Transaction Handling ImplementationReviewing this pull request for code quality, potential bugs, performance considerations, security concerns, and test coverage. Todo List
Code Review FeedbackThis PR adds transaction handling to the RPC API handler with sequential operations. Overall, it's a solid implementation with good validation and error handling. Here are my detailed findings: ✅ StrengthsCode Quality & Architecture
Input Validation
Test Coverage
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an RPC $transaction endpoint to execute multiple model operations in a single atomic transaction, plus API-level tests to validate request handling and rollback behavior.
Changes:
- Route
/$transaction/*to a newhandleTransactionhandler in the RPC API. - Validate transaction request bodies (method, shape, model/op) and run operations via
client.$transaction. - Add Jest tests covering success, validation failures, ORM validation errors, and rollback on failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/server/src/api/rpc/index.ts | Adds $transaction routing + transaction request validation/execution. |
| packages/server/test/api/rpc.test.ts | Adds test coverage for the new transaction endpoint behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/server/src/api/rpc/index.ts (2)
10-12: Constant declaration interleaved with import statements.
TRANSACTION_ROUTE_PREFIXon Line 11 is wedged between twoimportlines. Move it after all imports for clarity and to avoid linter issues.Proposed fix
- -const TRANSACTION_ROUTE_PREFIX = '$transaction' as const; import { loggerSchema } from '../common/schemas'; import { processSuperJsonRequestPayload, unmarshalQ } from '../common/utils'; import { log, registerCustomSerializers } from '../utils'; + +const TRANSACTION_ROUTE_PREFIX = '$transaction' as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/index.ts` around lines 10 - 12, The constant TRANSACTION_ROUTE_PREFIX is declared between import statements (e.g., between imports that include loggerSchema); move the declaration so all import statements come first and then declare TRANSACTION_ROUTE_PREFIX (e.g., place it immediately after the last import), ensuring imports remain grouped together to satisfy linters and improve clarity.
215-232:VALID_OPSis re-created on every call — hoist to module level.This
Setis constant data. Allocating it per request is wasteful. Consider making it a module-levelconst.Proposed fix
+const VALID_TRANSACTION_OPS = new Set([ + 'create', + 'createMany', + 'createManyAndReturn', + 'upsert', + 'findFirst', + 'findUnique', + 'findMany', + 'aggregate', + 'groupBy', + 'count', + 'exists', + 'update', + 'updateMany', + 'updateManyAndReturn', + 'delete', + 'deleteMany', +]);Then reference
VALID_TRANSACTION_OPSinhandleTransaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/index.ts` around lines 215 - 232, The VALID_OPS Set is being recreated inside the function on every call; hoist it to module scope as a top-level const (e.g., export or file-scoped const VALID_OPS = new Set([...])) and remove the local re-declaration, then update handleTransaction to reference this module-level VALID_OPS (and if you have a separate VALID_TRANSACTION_OPS concept, define it similarly at module level and use that identifier in handleTransaction) so the Set is allocated once instead of per-request.packages/server/test/api/rpc.test.ts (1)
766-983: Good test coverage overall, but consider adding two edge cases.The transaction test suite thoroughly validates input shape, routing, error propagation, and rollback. Two notable gaps:
No test for an invalid transaction type (e.g.,
/$transaction/parallel). Since the implementation currently ignores theopsegment of the path, this test would help enforce correct behavior once the validation is added.No test for SuperJSON-serialized args within a transaction (e.g., operations involving
BigInt,Date,Decimal). Thefield typestest covers this for regular CRUD, but the same path isn't exercised in the transaction handler — which would expose the missing deserialization noted in the implementation review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/api/rpc.test.ts` around lines 766 - 983, Add two tests into the existing describe('transaction') block using the same test harness (makeHandler and rawClient): (1) an "invalid transaction type" test that sends a POST to '/$transaction/parallel' (or another non-supported segment) and asserts a 400 response with an "invalid transaction type" / similar error message to ensure the handler rejects unknown transaction variants; (2) a "SuperJSON-serialized args" test that constructs a transaction requestBody containing operations with values requiring SuperJSON (e.g., BigInt, Date, Decimal), sends it to '/$transaction/sequential' via makeHandler, and asserts the operations are executed correctly (200 and expected results) to verify the transaction handler deserializes SuperJSON payloads; reference makeHandler, rawClient, and the '/$transaction/sequential' path to locate where to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/rpc/index.ts`:
- Around line 257-267: The mapping that builds promises from requestBody (using
lowerCaseFirst and calling (client as any)[model][op](args)) must be moved
inside the try block so synchronous throws are caught; relocate the promises
creation into the try before logging and awaiting client.$transaction (or wrap
the mapping in a try), ensuring the code still uses the same variable name
promises and then awaits client.$transaction(promises as any).
- Around line 234-262: The transaction path validates each operation but never
deserializes per-operation args with SuperJSON, so in the transaction invocation
you must pass item.args through this.processRequestPayload to restore
Date/BigInt/Decimal/Bytes etc. Update the validation/invocation in the
transaction handler (the loop and the promises mapping that reference item,
lowerCaseFirst, VALID_OPS and (client as any)[model][op](args)) to call
this.processRequestPayload(item.args) (or process the args with the same logic
used by processRequestPayload) before using them; keep using
makeBadInputErrorResponse for invalid arg shapes and ensure the mapped args
variable is the deserialized result when invoking (client as any)[model][op].
- Around line 76-82: handleRequest currently parses the op segment (op) from the
path but never passes it to handleTransaction, so any sub-path under
TRANSACTION_ROUTE_PREFIX is accepted; modify the call in handleRequest to pass
op to handleTransaction (e.g., include op in the object) and update
handleTransaction to validate op against allowed values (at least accept
"sequential" or a set of supported modes) and return a 4xx error for unsupported
ops; locate references to handleRequest, handleTransaction, op, and
TRANSACTION_ROUTE_PREFIX in the diff to implement the change and ensure behavior
is explicit rather than silently ignoring the operation segment.
---
Nitpick comments:
In `@packages/server/src/api/rpc/index.ts`:
- Around line 10-12: The constant TRANSACTION_ROUTE_PREFIX is declared between
import statements (e.g., between imports that include loggerSchema); move the
declaration so all import statements come first and then declare
TRANSACTION_ROUTE_PREFIX (e.g., place it immediately after the last import),
ensuring imports remain grouped together to satisfy linters and improve clarity.
- Around line 215-232: The VALID_OPS Set is being recreated inside the function
on every call; hoist it to module scope as a top-level const (e.g., export or
file-scoped const VALID_OPS = new Set([...])) and remove the local
re-declaration, then update handleTransaction to reference this module-level
VALID_OPS (and if you have a separate VALID_TRANSACTION_OPS concept, define it
similarly at module level and use that identifier in handleTransaction) so the
Set is allocated once instead of per-request.
In `@packages/server/test/api/rpc.test.ts`:
- Around line 766-983: Add two tests into the existing describe('transaction')
block using the same test harness (makeHandler and rawClient): (1) an "invalid
transaction type" test that sends a POST to '/$transaction/parallel' (or another
non-supported segment) and asserts a 400 response with an "invalid transaction
type" / similar error message to ensure the handler rejects unknown transaction
variants; (2) a "SuperJSON-serialized args" test that constructs a transaction
requestBody containing operations with values requiring SuperJSON (e.g., BigInt,
Date, Decimal), sends it to '/$transaction/sequential' via makeHandler, and
asserts the operations are executed correctly (200 and expected results) to
verify the transaction handler deserializes SuperJSON payloads; reference
makeHandler, rawClient, and the '/$transaction/sequential' path to locate where
to add these tests.
Contributor
|
Claude finished @jiashengguo's task —— View job PR Review: Transaction Handling ImplementationReviewing this pull request for code quality, potential bugs, performance considerations, security concerns, and test coverage. Todo List
Code Review FeedbackThis PR adds transaction handling to the RPC API handler with sequential operations. Overall, it's a solid implementation with good validation and error handling. Here are my detailed findings: ✅ StrengthsCode Quality & Architecture
Input Validation
SuperJSON Support
Transaction Type Validation
Test Coverage
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/server/test/api/rpc.test.ts (1)
810-876: Add a test for an unsupported transaction type (e.g./$transaction/interactive).
handleTransactioncorrectly rejects anytype !== 'sequential'with a 400, but there is no test exercising this branch. A GET-method test exists (lines 810-820), and model/op validation is covered, but the type guard athandleTransactionline 232-234 has no corresponding test.it('rejects unsupported transaction type', async () => { const handleRequest = makeHandler(); const r = await handleRequest({ method: 'post', path: '/$transaction/interactive', requestBody: [{ model: 'User', op: 'findMany', args: {} }], client: rawClient, }); expect(r.status).toBe(400); expect(r.error.message).toMatch(/unsupported transaction type/i); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/api/rpc.test.ts` around lines 810 - 876, Add a test exercising the handleTransaction branch that rejects unsupported transaction types: create a new it block (similar to existing tests) using makeHandler() and rawClient to send a POST to path '/$transaction/interactive' with a valid array body like [{ model: 'User', op: 'findMany', args: {} }], then assert r.status is 400 and r.error.message matches /unsupported transaction type/i; this ensures the type guard in handleTransaction is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/rpc/index.ts`:
- Around line 260-262: The current check lets arrays pass because typeof [] ===
'object'; update the validation around itemArgs (the variable checked and the
call to this.makeBadInputErrorResponse) to explicitly reject arrays by adding an
Array.isArray(itemArgs) check. Replace the condition with one that still permits
undefined but returns a bad-input response when itemArgs is null, not an object,
or is an array (e.g. if (itemArgs !== undefined && (itemArgs === null || typeof
itemArgs !== 'object' || Array.isArray(itemArgs))) return
this.makeBadInputErrorResponse(`operation at index ${i} has invalid "args"
field`);).
In `@packages/server/test/api/rpc.test.ts`:
- Around line 1024-1027: The rollback test currently asserts a broad error range
with expect(r.status).toBeGreaterThanOrEqual(400); tighten this to assert the
specific expected duplicate-key error code by changing the assertion to
expect(r.status).toBe(400) (or toEqual(400)) in the test that invokes the
rollback using rawClient and the response variable r so regressions to other
failure modes (e.g., 503) are caught.
---
Nitpick comments:
In `@packages/server/test/api/rpc.test.ts`:
- Around line 810-876: Add a test exercising the handleTransaction branch that
rejects unsupported transaction types: create a new it block (similar to
existing tests) using makeHandler() and rawClient to send a POST to path
'/$transaction/interactive' with a valid array body like [{ model: 'User', op:
'findMany', args: {} }], then assert r.status is 400 and r.error.message matches
/unsupported transaction type/i; this ensures the type guard in
handleTransaction is covered.
sanny-io
suggested changes
Feb 23, 2026
Contributor
|
Claude finished @jiashengguo's task —— View job PR Review: Transaction Handling ImplementationReviewing this pull request for code quality, potential bugs, performance considerations, security concerns, and test coverage. Todo List
Code Review FeedbackThis PR adds transaction handling to the RPC API handler with sequential operations. Overall, it's a solid implementation with good validation and error handling. Here are my detailed findings: ✅ StrengthsCode Quality & Architecture
Input Validation
SuperJSON Support
Transaction Type Validation
Test Coverage
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/server/src/api/rpc/index.ts (1)
243-245:⚠️ Potential issue | 🟡 Minor
argsvalidation still silently accepts arrays.
typeof [] === 'object', so an array payload forargspasses the current check. CRUD operations expect a plain object; arrays should be rejected explicitly.🛡️ Proposed fix
- if (itemArgs !== undefined && itemArgs !== null && typeof itemArgs !== 'object') { + if (itemArgs !== undefined && itemArgs !== null && (typeof itemArgs !== 'object' || Array.isArray(itemArgs))) { return this.makeBadInputErrorResponse(`operation at index ${i} has invalid "args" field`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/index.ts` around lines 243 - 245, The current validation for itemArgs in the RPC handler only checks typeof itemArgs === 'object', which wrongly allows arrays; update the validation in the block that references itemArgs and makeBadInputErrorResponse to explicitly reject arrays (e.g., add Array.isArray(itemArgs) check) or otherwise ensure itemArgs is a plain object before proceeding, and return this.makeBadInputErrorResponse(`operation at index ${i} has invalid "args" field`) when an array is detected.
🧹 Nitpick comments (1)
packages/server/src/api/rpc/index.ts (1)
15-15: Simplify the double cast forVALID_OPS.
CoreCrudOperationsis typed asstring[], so the double cast is unnecessary. Usenew Set<string>(CoreCrudOperations)instead.♻️ Proposed refactor
-const VALID_OPS = new Set(CoreCrudOperations as unknown as string[]); +const VALID_OPS = new Set<string>(CoreCrudOperations);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/index.ts` at line 15, VALID_OPS is created using an unnecessary double-cast; replace the double-cast expression new Set(CoreCrudOperations as unknown as string[]) with a typed Set construction using CoreCrudOperations directly (e.g., new Set<string>(CoreCrudOperations)) so VALID_OPS and its creation are simpler and type-safe; update the declaration where VALID_OPS is defined to use CoreCrudOperations without the as unknown as string[] cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/server/src/api/rpc/index.ts`:
- Around line 243-245: The current validation for itemArgs in the RPC handler
only checks typeof itemArgs === 'object', which wrongly allows arrays; update
the validation in the block that references itemArgs and
makeBadInputErrorResponse to explicitly reject arrays (e.g., add
Array.isArray(itemArgs) check) or otherwise ensure itemArgs is a plain object
before proceeding, and return this.makeBadInputErrorResponse(`operation at index
${i} has invalid "args" field`) when an array is detected.
---
Nitpick comments:
In `@packages/server/src/api/rpc/index.ts`:
- Line 15: VALID_OPS is created using an unnecessary double-cast; replace the
double-cast expression new Set(CoreCrudOperations as unknown as string[]) with a
typed Set construction using CoreCrudOperations directly (e.g., new
Set<string>(CoreCrudOperations)) so VALID_OPS and its creation are simpler and
type-safe; update the declaration where VALID_OPS is defined to use
CoreCrudOperations without the as unknown as string[] cast.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
Claude finished @jiashengguo's task —— View job PR Review: Transaction Handling ImplementationReviewing this pull request for code quality, potential bugs, performance considerations, security concerns, and test coverage. Todo List
Code Review FeedbackThis PR adds transaction handling to the RPC API handler with sequential operations. Overall, it's a solid implementation with good validation and error handling. Here are my detailed findings: ✅ StrengthsCode Quality & Architecture
Input Validation
SuperJSON Support
Transaction Type Validation
Test Coverage
|
ymc9
approved these changes
Feb 25, 2026
Member
ymc9
left a comment
ymc9
left a comment
There was a problem hiding this comment.
I think we can add transaction support to the tanstack hooks too. Shall we create a feature request first?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests