fix(query): fix mutationInvalidates missing args and wrong import path (#3090)#3197
Closed
zeriong wants to merge 0 commit intoorval-labs:masterfrom
Closed
fix(query): fix mutationInvalidates missing args and wrong import path (#3090)#3197zeriong wants to merge 0 commit intoorval-labs:masterfrom
zeriong wants to merge 0 commit intoorval-labs:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTransforms import paths for tags-split implementation files and makes mutation invalidation generation OpenAPI-spec-aware: when no explicit params are provided it tries to derive a route prefix from the spec to emit predicate-based invalidation; adds/updates multiple React Query and Angular generated snapshot files. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Mutation Caller
participant M as Generated onSuccess
participant G as generateInvalidateCall
participant S as OpenAPI Spec
participant Q as QueryClient
Caller->>M: mutation succeeds (data, variables, context)
M->>G: call generateInvalidateCall(target, context.spec)
alt target.params present
G->>Q: invalidateQueries({ queryKey: get-<query>-query-key(generatedArgs) })
else target.params absent
G->>S: lookup operation by operationId
alt operation has required path params
G->>Q: invalidateQueries({ predicate: q => q.queryKey[0] startsWith("/derived/prefix") })
else
G->>Q: invalidateQueries({ queryKey: get-<query>-query-key() })
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
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 |
![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
🤖 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/query/src/mutation-generator.ts`:
- Around line 104-109: getRoutePrefix currently collapses templates with no
static prefix to '/' and the matching logic uses startsWith which matches
siblings like '/pets-stats'; update getRoutePrefix to return an empty string
when there is no static prefix (i.e., when prefix === '' after trimming the
trailing slash) instead of returning '/', and change the predicate used where
routes are matched (the same logic around lines 129-132) so it is path-segment
aware: if a static prefix is non-empty, require that the candidate route either
equals the prefix or startsWith(prefix + '/') (or prefix followed by
end-of-string), and if the prefix is empty treat it as “no static prefix” (do
not default to '/' and do not broadly match). Ensure you reference and update
getRoutePrefix and the matching code near lines 129-132 accordingly.
- Around line 129-136: The code currently treats a missing route (route ===
undefined) as safe for zero-arg invalidation; change the logic so you only emit
the zero-argument form when you have positively confirmed the route exists and
does NOT include path params. Concretely, update the conditional around
findOperationRoute(spec, target.query) to require route && !route.includes('{')
before returning the `queryClient.${method}({ queryKey: ${queryKeyFn}() })`
string; for the else branch (route missing or contains '{') keep or emit the
predicate-based invalidation (the existing `predicate: (query) => { const key =
query.queryKey[0]; return typeof key === 'string' &&
key.startsWith('${prefix}'); }`) or another conservative form so you don't
assume zero-arg safety. Reference symbols: findOperationRoute, getRoutePrefix,
route, queryKeyFn, method, and the queryClient invalidation string.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a85a4dd7-69ed-4ba6-9123-b1ad226058e2
📒 Files selected for processing (4)
packages/core/src/writers/split-tags-mode.tspackages/query/src/mutation-generator.tstests/__snapshots__/react-query/invalidates/endpoints.tstests/configs/react-query.config.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/query/src/mutation-generator.ts (2)
133-140:⚠️ Potential issue | 🟠 MajorDon't treat a missed route lookup as safe for zero-arg invalidation.
Line 140 assumes
findOperationRoute()returningundefinedmeans the target query has no required args, but lookup can miss whenevercontext.specis absent oroperationIdresolution fails. That recreates the original TS/runtime bug; only emitqueryKeyFn()after a positiveroute && !route.includes('{')check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query/src/mutation-generator.ts` around lines 133 - 140, The current logic assumes findOperationRoute(spec, target.query) returning undefined means the route has no path params and emits queryKeyFn(), which is unsafe; change the branch so you only emit `queryClient.<method>({ queryKey: ${queryKeyFn}() })` when there is a positive route AND the route does not include '{' (i.e., `route && !route.includes('{')`); for all other cases (route undefined or route includes '{') keep emitting the predicate-based invalidation that inspects query.queryKey[0] (the existing predicate using getRoutePrefix(route) when route is present, and a safe fallback predicate when route is missing) so you never treat a missed lookup as a zero-arg invalidation.
109-115:⚠️ Potential issue | 🟠 MajorMake the broad predicate path-segment aware.
Line 114 turns routes with no leading static segment into
'/', and Line 136 then matches almost the entire cache. Even with a non-empty prefix,startsWith('/pets')also catches siblings like/pets-stats; this should only matchkey === prefixorkey.startsWith(prefix + '/'), and an empty prefix should stay "no safe broad match."Also applies to: 134-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query/src/mutation-generator.ts` around lines 109 - 115, getRoutePrefix currently returns '/' for routes with no leading static segment, and the broad-match logic uses naive startsWith checks so keys like '/pets-stats' incorrectly match; change getRoutePrefix to return an empty string for no static prefix (do not coerce to '/'), and update the broad predicate that uses prefix (the code that checks key === prefix or key.startsWith(prefix + '/')) so it only matches when key equals the prefix or begins with the prefix plus a slash; also ensure that when prefix is empty you do not generate/allow a broad match (treat empty prefix as "no safe broad match").
🤖 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/query/src/mutation-generator.ts`:
- Around line 125-127: The code treats empty params mappings (e.g., params: []
or params: {}) as present because it only checks truthiness; update the guard
around target.params in mutation-generator.ts so empty arrays or empty objects
are treated as "no mapping" — replace the simple if (target.params) with a check
that ensures target.params is non-empty (e.g., target.params &&
!(Array.isArray(target.params) && target.params.length === 0) && !(typeof
target.params === 'object' && !Array.isArray(target.params) &&
Object.keys(target.params).length === 0)) before calling
generateParamArgs(target.params) and emitting queryClient.${method}({ queryKey:
${queryKeyFn}(${args}) }); so generateParamArgs/run of ${queryKeyFn} is only
used for truly non-empty mappings.
In `@tests/__snapshots__/angular/multi-content-query-params/endpoints.ts`:
- Around line 20-25: The params merging currently spreads options?.params
directly, which drops query params when a HttpParams instance is passed; change
the merge to mirror the headers fix: detect if options?.params instanceof
HttpParams and, if so, use it directly (or extract its serialized form) instead
of spreading, otherwise spread the plain object (i.e., replace the direct spread
of options?.params with a conditional based on HttpParams). Update the code that
builds query params where HttpClientOptions.params and options?.params are used
(the same area that handles headers) to handle both HttpParams and plain objects
consistently.
---
Duplicate comments:
In `@packages/query/src/mutation-generator.ts`:
- Around line 133-140: The current logic assumes findOperationRoute(spec,
target.query) returning undefined means the route has no path params and emits
queryKeyFn(), which is unsafe; change the branch so you only emit
`queryClient.<method>({ queryKey: ${queryKeyFn}() })` when there is a positive
route AND the route does not include '{' (i.e., `route &&
!route.includes('{')`); for all other cases (route undefined or route includes
'{') keep emitting the predicate-based invalidation that inspects
query.queryKey[0] (the existing predicate using getRoutePrefix(route) when route
is present, and a safe fallback predicate when route is missing) so you never
treat a missed lookup as a zero-arg invalidation.
- Around line 109-115: getRoutePrefix currently returns '/' for routes with no
leading static segment, and the broad-match logic uses naive startsWith checks
so keys like '/pets-stats' incorrectly match; change getRoutePrefix to return an
empty string for no static prefix (do not coerce to '/'), and update the broad
predicate that uses prefix (the code that checks key === prefix or
key.startsWith(prefix + '/')) so it only matches when key equals the prefix or
begins with the prefix plus a slash; also ensure that when prefix is empty you
do not generate/allow a broad match (treat empty prefix as "no safe broad
match").
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8eb0bd69-44fa-422a-ad25-68c850ee9bd2
📒 Files selected for processing (7)
packages/query/src/mutation-generator.tstests/__snapshots__/angular/multi-content-query-params/endpoints.tstests/__snapshots__/angular/multi-content-query-params/model/error.tstests/__snapshots__/angular/multi-content-query-params/model/index.tstests/__snapshots__/angular/multi-content-query-params/model/item.tstests/__snapshots__/angular/multi-content-query-params/model/items.tstests/__snapshots__/angular/multi-content-query-params/model/listItemsParams.ts
✅ Files skipped from review due to trivial changes (5)
- tests/snapshots/angular/multi-content-query-params/model/listItemsParams.ts
- tests/snapshots/angular/multi-content-query-params/model/item.ts
- tests/snapshots/angular/multi-content-query-params/model/items.ts
- tests/snapshots/angular/multi-content-query-params/model/error.ts
- tests/snapshots/angular/multi-content-query-params/model/index.ts
tests/__snapshots__/angular/multi-content-query-params/endpoints.ts
Outdated
Show resolved
Hide resolved
Collaborator
|
good CodeRabbit feedback and build did not pass. |
zeriong
force-pushed
the
fix/mutation-invalidates-codegen-bugs-3090
branch
from
d9693d0 to
b52e2cb
Compare
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
2 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
Closes #3090
Bug 1: Missing required arguments in generated invalidation code
When
mutationInvalidatestargets a query with required path parameters (e.g.showPetById) but noparamsmapping is specified, the generated code called the query key function without arguments:Now looks up the target query's route from the OpenAPI spec and generates predicate-based broad matching using the route prefix:
Queries without required path params (e.g.
listPets) and queries with explicitparamsmapping continue to usequeryKey: getXxxQueryKey(...)as before.Bug 2: Incorrect import path in tags-split mode
In
tags-splitmode, thefileoption for cross-tag invalidation generated import paths relative to the output root instead of the tag subdirectory:Changes
packages/query/src/mutation-generator.tsfindOperationRoute/getRoutePrefixhelpers; generatepredicate-based invalidation when params are missing for path-param queriespackages/core/src/writers/split-tags-mode.tsfileimportPath from./tagto../tag/tagin tags-split directory structuretests/configs/react-query.config.tsshowPetByIdwithout params tocreatePetsinvalidation for Bug 1 coveragetests/__snapshots__/react-query/invalidates/endpoints.tsTest plan
bun vitest run(2178/2181, 3 pre-existing failures inresolve-version.test.ts)bun run test:snapshots(67/67 tasks)predicate+startsWith('/pets')for params-lessshowPetByIdinvalidationdeletePetByIdwithparams: ['petId']still generatesqueryKey: getShowPetByIdQueryKey(variables.petId)Summary by CodeRabbit
New Features
skipInvalidationoption to mutation APIs for finer cache control.Bug Fixes