fix(query-core): correctly refetch resetQueries matches#10837
Open
raashish1601 wants to merge 3 commits into
Open
fix(query-core): correctly refetch resetQueries matches#10837raashish1601 wants to merge 3 commits into
raashish1601 wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds pending task tracking to Angular Query to stabilize Angular's ChangesAngular pending task tracking for whenStable stability
Query Core resetQueries predicate filtering
Sequence DiagramsequenceDiagram
participant Component
participant createBaseQuery
participant QueryFn
participant PendingTask
participant whenStable
Component->>createBaseQuery: inject query, access data signal before render
createBaseQuery->>QueryFn: wrapped call, register pending task
QueryFn->>PendingTask: promise created
Component->>whenStable: called immediately
whenStable->>PendingTask: check pending (task incomplete)
whenStable->>whenStable: remains pending
Component->>Component: advance timers
QueryFn->>PendingTask: promise resolves
PendingTask->>createBaseQuery: complete pending task ref
createBaseQuery->>whenStable: stability check
whenStable->>Component: resolves
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/angular-query-experimental/src/__tests__/pending-tasks.test.ts`:
- Around line 329-349: The test may be a false positive because it never asserts
that app.whenStable() (stableResult) is pending before destroy; update the test
around NeverResolvesComponent to first create stableResult = app.whenStable()
after fixture.detectChanges(), then assert it has not yet resolved (e.g., race
it against a short timeout and expect the timeout to win) before calling
fixture.destroy(), and only then advance timers and assert stableResult
resolves; reference the symbols TestBed.inject(ApplicationRef),
fixture.detectChanges(), stableResult, fixture.destroy(), and
vi.advanceTimersByTimeAsync to find and modify the code paths.
In `@packages/angular-query-experimental/src/create-base-query.ts`:
- Around line 67-75: The wrapper for defaultedOptions.queryFn currently calls
markPendingQueryFnTask() and only invokes complete() on the promise settle,
leaving the pending task open if the AbortSignal aborts before the promise
settles; update the wrapper around originalQueryFn to also listen to
context.signal (or context.signal.addEventListener('abort', ...)) and call
complete() when the signal aborts, and ensure the abort listener is removed when
the promise settles (or when complete() runs) so markPendingQueryFnTask() is
always completed on either promise settle or signal abort.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e45f78c7-ae8b-4ad6-ae5c-4632e2a7476e
📒 Files selected for processing (6)
.changeset/angular-query-preaccess-whenstable.mdpackages/angular-query-experimental/src/__tests__/inject-query.test.tspackages/angular-query-experimental/src/__tests__/pending-tasks.test.tspackages/angular-query-experimental/src/create-base-query.tspackages/query-core/src/__tests__/queryClient.test.tsxpackages/query-core/src/queryClient.ts
Comment on lines
+329
to
+349
| it('should cleanup query promise pending task when component with unresolved query is destroyed', async () => { | ||
| const app = TestBed.inject(ApplicationRef) | ||
| const fixture = TestBed.createComponent(NeverResolvesComponent) | ||
|
|
||
| fixture.detectChanges() | ||
|
|
||
| const stableResult = app.whenStable().then(() => true) | ||
|
|
||
| fixture.destroy() | ||
|
|
||
| const timeoutResult = new Promise<boolean>((resolve) => { | ||
| setTimeout(() => { | ||
| resolve(false) | ||
| }, 10) | ||
| }) | ||
|
|
||
| await vi.advanceTimersByTimeAsync(10) | ||
|
|
||
| const isStableResolved = await Promise.race([stableResult, timeoutResult]) | ||
| expect(isStableResolved).toBe(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
Assert instability before destroy so this can't pass as a false positive.
As written, this still passes if whenStable() resolves immediately after detectChanges() and destroy cleanup never does anything. Check that the promise is still pending before fixture.destroy(), then assert it resolves after destroy.
Suggested test tightening
const app = TestBed.inject(ApplicationRef)
const fixture = TestBed.createComponent(NeverResolvesComponent)
fixture.detectChanges()
- const stableResult = app.whenStable().then(() => true)
+ let stableResolved = false
+ const stableResult = app.whenStable().then(() => {
+ stableResolved = true
+ return true
+ })
+
+ await Promise.resolve()
+ expect(stableResolved).toBe(false)
fixture.destroy()
const timeoutResult = new Promise<boolean>((resolve) => {
setTimeout(() => {📝 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('should cleanup query promise pending task when component with unresolved query is destroyed', async () => { | |
| const app = TestBed.inject(ApplicationRef) | |
| const fixture = TestBed.createComponent(NeverResolvesComponent) | |
| fixture.detectChanges() | |
| const stableResult = app.whenStable().then(() => true) | |
| fixture.destroy() | |
| const timeoutResult = new Promise<boolean>((resolve) => { | |
| setTimeout(() => { | |
| resolve(false) | |
| }, 10) | |
| }) | |
| await vi.advanceTimersByTimeAsync(10) | |
| const isStableResolved = await Promise.race([stableResult, timeoutResult]) | |
| expect(isStableResolved).toBe(true) | |
| }) | |
| it('should cleanup query promise pending task when component with unresolved query is destroyed', async () => { | |
| const app = TestBed.inject(ApplicationRef) | |
| const fixture = TestBed.createComponent(NeverResolvesComponent) | |
| fixture.detectChanges() | |
| let stableResolved = false | |
| const stableResult = app.whenStable().then(() => { | |
| stableResolved = true | |
| return true | |
| }) | |
| await Promise.resolve() | |
| expect(stableResolved).toBe(false) | |
| fixture.destroy() | |
| const timeoutResult = new Promise<boolean>((resolve) => { | |
| setTimeout(() => { | |
| resolve(false) | |
| }, 10) | |
| }) | |
| await vi.advanceTimersByTimeAsync(10) | |
| const isStableResolved = await Promise.race([stableResult, timeoutResult]) | |
| expect(isStableResolved).toBe(true) | |
| }) |
🤖 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/angular-query-experimental/src/__tests__/pending-tasks.test.ts`
around lines 329 - 349, The test may be a false positive because it never
asserts that app.whenStable() (stableResult) is pending before destroy; update
the test around NeverResolvesComponent to first create stableResult =
app.whenStable() after fixture.detectChanges(), then assert it has not yet
resolved (e.g., race it against a short timeout and expect the timeout to win)
before calling fixture.destroy(), and only then advance timers and assert
stableResult resolves; reference the symbols TestBed.inject(ApplicationRef),
fixture.detectChanges(), stableResult, fixture.destroy(), and
vi.advanceTimersByTimeAsync to find and modify the code paths.
Comment on lines
+67
to
+75
| defaultedOptions.queryFn = (context) => { | ||
| const result = originalQueryFn(context) | ||
|
|
||
| if (result && typeof result.then === 'function') { | ||
| const complete = markPendingQueryFnTask() | ||
| void result.then( | ||
| complete, | ||
| complete, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify query function contexts expose AbortSignal:"
rg -n -C2 'signal:\s*AbortSignal' packages/query-core/src
echo
echo "Inspect the wrapped queryFn and PendingTask cleanup paths:"
sed -n '64,96p' packages/angular-query-experimental/src/create-base-query.ts
sed -n '183,193p' packages/angular-query-experimental/src/create-base-query.tsRepository: TanStack/query
Length of output: 3458
Complete queryFn PendingTask on AbortSignal abort (not only promise settle)
packages/angular-query-experimental/src/create-base-query.ts wraps defaultedOptions.queryFn so thenables call markPendingQueryFnTask() but only complete() inside result.then(complete, complete). Since @tanstack/query-core provides queryFn context with an AbortSignal, a cancelled/superseded request whose promise never settles will keep the pending task open until Angular cleanup/destroy, which can keep whenStable() waiting. Hook complete() to context.signal abort too.
Suggested fix
defaultedOptions.queryFn = (context) => {
const result = originalQueryFn(context)
if (result && typeof result.then === 'function') {
const complete = markPendingQueryFnTask()
+ const onAbort = () => {
+ complete()
+ }
+
+ context.signal.addEventListener('abort', onAbort, { once: true })
void result.then(
- complete,
- complete,
+ () => {
+ context.signal.removeEventListener('abort', onAbort)
+ complete()
+ },
+ () => {
+ context.signal.removeEventListener('abort', onAbort)
+ complete()
+ },
)
}
return result
}🤖 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/angular-query-experimental/src/create-base-query.ts` around lines 67
- 75, The wrapper for defaultedOptions.queryFn currently calls
markPendingQueryFnTask() and only invokes complete() on the promise settle,
leaving the pending task open if the AbortSignal aborts before the promise
settles; update the wrapper around originalQueryFn to also listen to
context.signal (or context.signal.addEventListener('abort', ...)) and call
complete() when the signal aborts, and ensure the abort listener is removed when
the promise settles (or when complete() runs) so markPendingQueryFnTask() is
always completed on either promise settle or signal abort.
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.
Fix
esetQueries so it refetches queries selected by predicates that depend on pre-reset state (eg status === 'error').
The implementation now snapshots the initially-matched query set before calling
eset(), then refetches only those query hashes so status mutations during reset do not mutate filtering behavior.
Included regression test:
Summary by CodeRabbit
Bug Fixes
whenStable()from resolving prematurely.resetQueries()to correctly refetch only the queries that were initially matched by the provided filter.Tests
resetQueries()predicate-based filtering.