[PROPOSAL] feat(tanstack-query): Improvements over PR #2637 useTransaction hook for sequential transactions#2643
Open
docloulou wants to merge 2 commits intozenstackhq:devfrom
Conversation
… references - Introduced `$stepRef`, `$get`, `$filter`, and `$map` for referencing results between transaction steps. - Updated the RPC API to handle sequential transactions, allowing operations to reference previous results. - Enhanced the Nuxt+Next.js sample application to demonstrate the new transaction capabilities. - Added tests to verify the functionality of step references in transactions.
Contributor
📝 WalkthroughWalkthroughThis PR introduces a client-side transaction step expression system enabling later transaction operations to reference and manipulate values produced by earlier steps. New composable expression constructors ( ChangesTransaction Step Expressions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
… validation - Added TransactionInputError class to handle user-facing resolution failures in transaction steps. - Updated path resolution and expression validation functions to throw TransactionInputError for invalid inputs. - Enhanced RPC API error handling to return bad input responses for TransactionInputError instances.
docloulou
changed the title
![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: 2
🧹 Nitpick comments (2)
packages/server/test/api/rpc.test.ts (2)
1096-1125: ⚡ Quick winThe “path omitted” scenario is not actually exercised.
At Line 1124, the test still passes a path (
$stepRef(2, 'id')), so it doesn’t validate the path-omitted behavior described in the test name.Proposed test adjustment
- { id: $stepRef(2, 'id') }, + { id: $get($stepRef(2), 'id') },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/test/api/rpc.test.ts` around lines 1096 - 1125, The test named "uses entire result of a step when path is omitted" currently still passes a path to the step reference; update the requestBody in that test so the OR condition uses $stepRef(2) (omit the 'id' path) instead of $stepRef(2, 'id') so the whole result of step 2 is used; keep using makeHandler(), rawClient, and the same transaction endpoint and then adjust any assertions if necessary to assert that the entire step result (not just its id) was applied.
1336-1359: ⚡ Quick winThis test title says “new syntax”, but the payload uses the helper syntax.
Line 1358 uses
$stepRef(...), so this case doesn’t directly validate raw$zenstackExpr: 'ref'parsing.Proposed test adjustment
- authorId: $stepRef(1, 'id'), + authorId: { $zenstackExpr: 'ref', step: 1, path: 'id' },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/test/api/rpc.test.ts` around lines 1336 - 1359, The test title claims "new syntax" but the payload uses the helper $stepRef helper; update the test to actually exercise the raw $zenstackExpr ref form or rename the title. Concretely, in the test that uses handleRequest in the "resolves $zenstackExpr: ref (new syntax, equivalent to old StepRef)" case, replace the helper call $stepRef(1, 'id') with the raw expression object { $zenstackExpr: { op: 'ref', step: 1, key: 'id' } } (or alternatively change the test title to say "helper syntax" if you prefer to keep $stepRef), so the code path parsing $zenstackExpr: 'ref' is truly validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/orm/src/client/transaction.ts`:
- Around line 323-329: The current visited WeakSet tracks every seen node and
causes false-positive circular-reference errors when the same subexpression
object is reused across different branches (e.g., the shared `posts` ref), so
update the resolver to track only the active recursion stack: inside the
function that takes `expr` and `visited` (the block using `const visited =
_visited ?? new WeakSet<object>();` and throwing `new
TransactionInputError('Circular reference detected in step expression.')`), add
`visited.add(expr)` before recursing and wrap the recursive resolution body in a
try/finally that calls `visited.delete(expr)` in the finally clause so nodes are
removed on unwind and only the current call path is treated as circular. Ensure
the circular check (visited.has(expr)) remains before adding.
In `@packages/server/test/api/rpc.test.ts`:
- Around line 1618-1652: The test currently only checks r.status >= 400 which is
too broad; update the assertions in the 'errors when filter targets an unknown
operator' test (using makeHandler and handleRequest) to assert the exact failure
and cause: expect r.status toBe(400) (or the precise error code your API
returns) and add an assertion on r.body (or r.data) that the error message
includes the unknown operator indicator (e.g., contains 'unknown operator' or
the token 'regex') so the failure is tied to the invalid operator rather than
any other error.
---
Nitpick comments:
In `@packages/server/test/api/rpc.test.ts`:
- Around line 1096-1125: The test named "uses entire result of a step when path
is omitted" currently still passes a path to the step reference; update the
requestBody in that test so the OR condition uses $stepRef(2) (omit the 'id'
path) instead of $stepRef(2, 'id') so the whole result of step 2 is used; keep
using makeHandler(), rawClient, and the same transaction endpoint and then
adjust any assertions if necessary to assert that the entire step result (not
just its id) was applied.
- Around line 1336-1359: The test title claims "new syntax" but the payload uses
the helper $stepRef helper; update the test to actually exercise the raw
$zenstackExpr ref form or rename the title. Concretely, in the test that uses
handleRequest in the "resolves $zenstackExpr: ref (new syntax, equivalent to old
StepRef)" case, replace the helper call $stepRef(1, 'id') with the raw
expression object { $zenstackExpr: { op: 'ref', step: 1, key: 'id' } } (or
alternatively change the test title to say "helper syntax" if you prefer to keep
$stepRef), so the code path parsing $zenstackExpr: 'ref' is truly validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41a9024b-de54-4370-8487-e36f5e8b648f
📒 Files selected for processing (9)
packages/clients/tanstack-query/src/common/types.tspackages/orm/src/client/index.tspackages/orm/src/client/transaction.tspackages/server/src/api/rpc/index.tspackages/server/test/api/rpc.test.tssamples/next.js/README.mdsamples/next.js/app/page.tsxsamples/nuxt/README.mdsamples/nuxt/app/pages/index.vue
Comment on lines
+323
to
+329
| const visited = _visited ?? new WeakSet<object>(); | ||
| if (typeof expr === 'object' && expr !== null) { | ||
| if (visited.has(expr as object)) { | ||
| throw new TransactionInputError('Circular reference detected in step expression.'); | ||
| } | ||
| visited.add(expr as object); | ||
| } |
Contributor
There was a problem hiding this comment.
Track the active recursion stack, not every visited node.
This WeakSet causes false-positive circular-reference errors when the same subexpression object is reused in two branches of one valid expression tree. For example, const posts = $stepRef<Post[]>(4); $filter(posts, 'id', 'in', $map(posts, 'id')) will visit posts through filter.ref and then fail when map.ref resolves the same object again.
Wrap the resolution body in try/finally and visited.delete(expr) on unwind so only the current recursion path is tracked.
Suggested fix
export function resolveExpr(
expr: StepExpr | StepRef,
results: unknown[],
_visited?: WeakSet<object>,
): unknown {
- // Cycle detection for client-side local expressions
const visited = _visited ?? new WeakSet<object>();
- if (typeof expr === 'object' && expr !== null) {
- if (visited.has(expr as object)) {
+ const exprObject = typeof expr === 'object' && expr !== null ? (expr as object) : undefined;
+
+ if (exprObject) {
+ if (visited.has(exprObject)) {
throw new TransactionInputError('Circular reference detected in step expression.');
}
- visited.add(expr as object);
+ visited.add(exprObject);
}
- // Handle old-style StepRef
- if (isStepRef(expr)) {
- const { step, path } = expr;
- validateInteger(step, 'step');
- const resultIndex = getResultIndex(step, results);
- let value = results[resultIndex];
- if (path) {
- if (typeof path !== 'string') {
- throw new TransactionInputError('"path" must be a string.');
- }
- value = resolvePath(value, parsePath(path));
- }
- return value;
- }
-
- // Accept plain objects with EXPR_SYMBOL
- if (!isStepExpr(expr)) {
- throw new TransactionInputError('Expression must be an object with a valid expression marker.');
- }
-
- validateExprRef(expr);
-
- // Handle new-style StepExpr
- const kind = (expr as Record<string, unknown>)[EXPR_SYMBOL] as string;
- switch (kind) {
- // existing cases...
- }
+ try {
+ // existing resolution logic
+ } finally {
+ if (exprObject) {
+ visited.delete(exprObject);
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/orm/src/client/transaction.ts` around lines 323 - 329, The current
visited WeakSet tracks every seen node and causes false-positive
circular-reference errors when the same subexpression object is reused across
different branches (e.g., the shared `posts` ref), so update the resolver to
track only the active recursion stack: inside the function that takes `expr` and
`visited` (the block using `const visited = _visited ?? new WeakSet<object>();`
and throwing `new TransactionInputError('Circular reference detected in step
expression.')`), add `visited.add(expr)` before recursing and wrap the recursive
resolution body in a try/finally that calls `visited.delete(expr)` in the
finally clause so nodes are removed on unwind and only the current call path is
treated as circular. Ensure the circular check (visited.has(expr)) remains
before adding.
Comment on lines
+1618
to
+1652
| it('errors when filter targets an unknown operator', async () => { | ||
| const handleRequest = makeHandler(); | ||
|
|
||
| const r = await handleRequest({ | ||
| method: 'post', | ||
| path: '/$transaction/sequential', | ||
| requestBody: [ | ||
| { | ||
| model: 'User', | ||
| op: 'findMany', | ||
| args: {}, | ||
| }, | ||
| { | ||
| model: 'User', | ||
| op: 'findFirst', | ||
| args: { | ||
| where: { | ||
| id: { | ||
| $zenstackExpr: 'get', | ||
| ref: { | ||
| $zenstackExpr: 'filter', | ||
| ref: $stepRef(1), | ||
| where: { field: 'email', op: 'regex', value: '.*' }, | ||
| }, | ||
| path: 'id', | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| client: rawClient, | ||
| }); | ||
|
|
||
| expect(r.status).toBeGreaterThanOrEqual(400); | ||
| }); |
Contributor
There was a problem hiding this comment.
Unknown-operator test is too broad and can pass for the wrong reason.
Only checking >= 400 (Line 1651) doesn’t prove the failure is caused by the invalid regex operator. Please assert the error details to lock intent.
Proposed assertion tightening
- expect(r.status).toBeGreaterThanOrEqual(400);
+ expect(r.status).toBe(400);
+ expect(r.error?.message).toMatch(/operator|regex/i);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('errors when filter targets an unknown operator', async () => { | |
| const handleRequest = makeHandler(); | |
| const r = await handleRequest({ | |
| method: 'post', | |
| path: '/$transaction/sequential', | |
| requestBody: [ | |
| { | |
| model: 'User', | |
| op: 'findMany', | |
| args: {}, | |
| }, | |
| { | |
| model: 'User', | |
| op: 'findFirst', | |
| args: { | |
| where: { | |
| id: { | |
| $zenstackExpr: 'get', | |
| ref: { | |
| $zenstackExpr: 'filter', | |
| ref: $stepRef(1), | |
| where: { field: 'email', op: 'regex', value: '.*' }, | |
| }, | |
| path: 'id', | |
| }, | |
| }, | |
| }, | |
| }, | |
| ], | |
| client: rawClient, | |
| }); | |
| expect(r.status).toBeGreaterThanOrEqual(400); | |
| }); | |
| it('errors when filter targets an unknown operator', async () => { | |
| const handleRequest = makeHandler(); | |
| const r = await handleRequest({ | |
| method: 'post', | |
| path: '/$transaction/sequential', | |
| requestBody: [ | |
| { | |
| model: 'User', | |
| op: 'findMany', | |
| args: {}, | |
| }, | |
| { | |
| model: 'User', | |
| op: 'findFirst', | |
| args: { | |
| where: { | |
| id: { | |
| $zenstackExpr: 'get', | |
| ref: { | |
| $zenstackExpr: 'filter', | |
| ref: $stepRef(1), | |
| where: { field: 'email', op: 'regex', value: '.*' }, | |
| }, | |
| path: 'id', | |
| }, | |
| }, | |
| }, | |
| }, | |
| ], | |
| client: rawClient, | |
| }); | |
| expect(r.status).toBe(400); | |
| expect(r.error?.message).toMatch(/operator|regex/i); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/test/api/rpc.test.ts` around lines 1618 - 1652, The test
currently only checks r.status >= 400 which is too broad; update the assertions
in the 'errors when filter targets an unknown operator' test (using makeHandler
and handleRequest) to assert the exact failure and cause: expect r.status
toBe(400) (or the precise error code your API returns) and add an assertion on
r.body (or r.data) that the error message includes the unknown operator
indicator (e.g., contains 'unknown operator' or the token 'regex') so the
failure is tied to the invalid operator rather than any other error.
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
1 participant
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.
This PR builds on the sequential transaction system introduced in #2637, adding typed constructors, composable expressions.
New Features
$stepRef<T>,$get,$item,$first,$filter,$map$get($filter($stepRef<Post[]>(1), 'title', 'eq', 'Foo'), 'id')— arbitrary chainingeq,neq,gt,gte,lt,lte,in,notIn,contains)Examples
1. Simple step ref (create user → linked post)
2. Pick the first item from a list
3. Publish all drafts via filter + map
4. Using the TanStack Query hook (Vue / React)
Summary by CodeRabbit
New Features
Documentation