fix(query-core): focus check issue from retryer for refetchIntervallnBackground #9563
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 2m 3s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 21s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-08-14 08:50:26 UTC
|
plase have a look at the conflicts and failing tests |
Hellol77
force-pushed
the
fix/retry-background-focus-issue
branch
2 times, most recently
from
7250804 to
56049b5
Compare
|
I fixed the conflicts and test issues! thanks for review. @TkDodo |
WalkthroughImplements continuation of retry/backoff while a tab is unfocused when refetchIntervalInBackground is true by adding the option to QueryOptions, plumbing it into the Retryer config, updating Retryer.canContinue, and adding tests for true/false/undefined cases. Also fixes a comment typo. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Q as Query
participant R as Retryer
participant F as focusManager
participant O as onlineManager
Note over Q: start fetch with retry options<br/>(includes refetchIntervalInBackground)
Q->>R: createRetryer({ retry, retryDelay, networkMode, refetchIntervalInBackground })
loop retry loop
R->>F: isFocused()
R->>O: isOnline()
alt canContinue (refetchIntervalInBackground==true OR focused) AND online/networkMode allows
Note right of R #DFF7E0: Continue attempts (green)
R->>Q: attempt fetch
alt success
R-->>Q: resolve
else failure
R->>R: schedule retry after retryDelay
end
else paused
Note right of R #FFEFD5: Pause until focus/online (amber)
R-->>R: wait for visibilitychange/online
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/solid-query/src/__tests__/useQuery.test.tsx (3)
3203-3205: Ensure visibility state is restored even on test failure (wrap in try/finally)If an assertion fails, the mocked visibility may bleed into subsequent tests. Wrap the body in try/finally to guarantee cleanup.
- // make page unfocused - const visibilityMock = mockVisibilityState('hidden') + // make page unfocused + const visibilityMock = mockVisibilityState('hidden') + try { @@ - visibilityMock.mockRestore() + } finally { + visibilityMock.mockRestore() + }Also applies to: 3255-3256
3209-3216: Avoid unnecessary generic on Promise.reject
Promise.reject<unknown>(...)doesn’t add value here and can confuse readers regarding the error type. Returning a rejected promise without the generic keeps intent clear while preserving thestringerror typing set foruseQuery.- queryFn: () => { + queryFn: () => { count++ - return Promise.reject<unknown>(`fetching error ${count}`) + return Promise.reject(`fetching error ${count}`) },
3237-3251: Optional: consolidate waits to reduce flakiness and execution timeYou perform multiple sequential waitFor checks for related end-state assertions. Consolidating into one waitFor that verifies all required end-state texts reduces the number of polling cycles and makes the test slightly faster and less chatty.
- await vi.waitFor(() => - expect(rendered.getByText('failureCount 4')).toBeInTheDocument(), - ) - await vi.waitFor(() => - expect( - rendered.getByText('failureReason fetching error 4'), - ).toBeInTheDocument(), - ) - await vi.waitFor(() => - expect(rendered.getByText('status error')).toBeInTheDocument(), - ) - await vi.waitFor(() => - expect(rendered.getByText('error fetching error 4')).toBeInTheDocument(), - ) + await vi.waitFor(() => { + expect(rendered.getByText('failureCount 4')).toBeInTheDocument() + expect(rendered.getByText('failureReason fetching error 4')).toBeInTheDocument() + expect(rendered.getByText('status error')).toBeInTheDocument() + expect(rendered.getByText('error fetching error 4')).toBeInTheDocument() + })packages/react-query/src/__tests__/useQuery.test.tsx (1)
3434-3479: Guard visibility mock with try/finally to prevent cross-test leakageIf an assertion fails,
visibilityMock.mockRestore()won't run and could affect subsequent tests. Wrap the test body in a try/finally.it('should continue retry in background even when page is not focused', async () => { const key = queryKey() // make page unfocused const visibilityMock = mockVisibilityState('hidden') + try { let count = 0 function Page() { const query = useQuery<unknown, string>({ queryKey: key, queryFn: () => { count++ return Promise.reject<unknown>(`fetching error ${count}`) }, retry: 3, retryDelay: 1, }) return ( <div> <div>error {String(query.error)}</div> <div>status {query.status}</div> <div>failureCount {query.failureCount}</div> <div>failureReason {query.failureReason}</div> </div> ) } const rendered = renderWithClient(queryClient, <Page />) // With the new behavior, retries continue in background // Wait for all retries to complete await vi.advanceTimersByTimeAsync(4) expect(rendered.getByText('failureCount 4')).toBeInTheDocument() expect( rendered.getByText('failureReason fetching error 4'), ).toBeInTheDocument() expect(rendered.getByText('status error')).toBeInTheDocument() expect(rendered.getByText('error fetching error 4')).toBeInTheDocument() // Verify all retries were attempted expect(count).toBe(4) - - visibilityMock.mockRestore() + } finally { + visibilityMock.mockRestore() + } })packages/query-core/src/__tests__/query.test.tsx (4)
61-99: Solid background-retry assertion; add try/finally around visibility mockThe updated expectation aligns with removing focus gating. Minor robustness: ensure the visibility mock is restored even on failure.
- const visibilityMock = mockVisibilityState('hidden') + const visibilityMock = mockVisibilityState('hidden') + try { let count = 0 let result const promise = queryClient.fetchQuery({ queryKey: key, queryFn: () => { count++ if (count === 3) { return `data${count}` } throw new Error(`error${count}`) }, retry: 3, retryDelay: 1, }) promise.then((data) => { result = data }) // Check if we do not have a result initially expect(result).toBeUndefined() // With new behavior, retries continue in background // Wait for retries to complete await vi.advanceTimersByTimeAsync(10) expect(result).toBe('data3') expect(count).toBe(3) - - visibilityMock.mockRestore() + } finally { + visibilityMock.mockRestore() + }
149-178: New background-retry test looks good; ensure visibility cleanup via try/finallyMatches the core change to remove focus gating. Add try/finally so the visibility mock is always restored.
- const visibilityMock = mockVisibilityState('hidden') + const visibilityMock = mockVisibilityState('hidden') + try { let count = 0 let result const promise = queryClient.fetchQuery({ queryKey: key, queryFn: () => { count++ if (count === 3) { return `data${count}` } throw new Error(`error${count}`) }, retry: 3, retryDelay: 1, }) promise.then((data) => { result = data }) // Check if we do not have a result initially expect(result).toBeUndefined() // Unlike the old behavior, retry should continue in background // Wait for retries to complete await vi.advanceTimersByTimeAsync(10) expect(result).toBe('data3') expect(count).toBe(3) - visibilityMock.mockRestore() + } finally { + visibilityMock.mockRestore() + }
180-217: Simplify cancellation assertion and remove redundant checksYou can assert the cancellation more directly and drop the extra
resultplumbing and redundantinstanceof Errorcheck.- it('should throw a CancelledError when a retrying query is cancelled', async () => { + it('should throw a CancelledError when a retrying query is cancelled', async () => { const key = queryKey() let count = 0 - let result: unknown const promise = queryClient.fetchQuery({ queryKey: key, queryFn: (): Promise<unknown> => { count++ throw new Error(`error${count}`) }, retry: 3, - retryDelay: 100, // Longer delay to allow cancellation + retryDelay: 100, // Longer delay to allow cancellation }) - promise.catch((data) => { - result = data - }) - const query = queryCache.find({ queryKey: key })! // Wait briefly for first failure and start of retry await vi.advanceTimersByTimeAsync(1) expect(result).toBeUndefined() // Cancel query during retry query.cancel() - // Check if the error is set to the cancelled error - try { - await promise - expect.unreachable() - } catch { - expect(result instanceof CancelledError).toBe(true) - expect(result instanceof Error).toBe(true) - } + // Assert promise rejects with CancelledError + await expect(promise).rejects.toBeInstanceOf(CancelledError) })Note: if you keep the current style, consider adding
await vi.advanceTimersByTimeAsync(0)afterquery.cancel()to flush microtasks, but it isn’t strictly necessary when awaiting the promise.
258-294: Good coverage of refetchInterval + retries in background; add subscription cleanup and try/finally
- Capture and call the unsubscribe returned from
subscribe.- Wrap with try/finally to always destroy the observer and restore visibility, even if an assertion fails.
it('should continue refetchInterval with retries in background when tab is inactive', async () => { const key = queryKey() - const visibilityMock = mockVisibilityState('hidden') + const visibilityMock = mockVisibilityState('hidden') let totalRequests = 0 const queryObserver = new QueryObserver(queryClient, { queryKey: key, queryFn: () => { totalRequests++ // Always fail to simulate network offline throw new Error(`Network error ${totalRequests}`) }, refetchInterval: 60000, refetchIntervalInBackground: true, retry: 3, retryDelay: 1, }) - queryObserver.subscribe(() => {}) + const unsubscribe = queryObserver.subscribe(() => {}) - // First interval: t=0 to t=60s (initial query + retries) - await vi.advanceTimersByTimeAsync(60000) - expect(totalRequests).toBe(4) - - // Second interval: t=60s to t=120s (refetch + retries) - await vi.advanceTimersByTimeAsync(60000) - expect(totalRequests).toBe(8) - - // Third interval: t=120s to t=180s (refetch + retries) - await vi.advanceTimersByTimeAsync(60000) - expect(totalRequests).toBe(12) - - queryObserver.destroy() - visibilityMock.mockRestore() + try { + // First interval: t=0 to t=60s (initial query + retries) + await vi.advanceTimersByTimeAsync(60000) + expect(totalRequests).toBe(4) + + // Second interval: t=60s to t=120s (refetch + retries) + await vi.advanceTimersByTimeAsync(60000) + expect(totalRequests).toBe(8) + + // Third interval: t=120s to t=180s (refetch + retries) + await vi.advanceTimersByTimeAsync(60000) + expect(totalRequests).toBe(12) + } finally { + unsubscribe() + queryObserver.destroy() + visibilityMock.mockRestore() + } })
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
packages/query-core/src/__tests__/query.test.tsx(6 hunks)packages/query-core/src/retryer.ts(0 hunks)packages/react-query/src/__tests__/useQuery.test.tsx(3 hunks)packages/solid-query/src/__tests__/useQuery.test.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- packages/query-core/src/retryer.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/query-core/src/__tests__/query.test.tsx (3)
packages/query-core/src/queriesObserver.ts (1)
result(186-201)packages/query-core/src/query.ts (1)
promise(198-200)packages/query-core/src/retryer.ts (1)
CancelledError(57-65)
🔇 Additional comments (1)
packages/solid-query/src/__tests__/useQuery.test.tsx (1)
3200-3217: Test intent and expectations align with the new background-retry semanticsGood addition. The scenario accurately verifies that retries are not gated by focus anymore: failureCount/error state match the configured retry count (initial + 3 retries = 4). Assertions cover both failureCount and surfaced error, which is helpful.
Also applies to: 3235-3251
|
I discussed this with @DamianOsipiuk and we think the better fix would be to check the We might remove the check in the next major version as it’s technically a breaking change. |
Hellol77
force-pushed
the
fix/retry-background-focus-issue
branch
from
b4c70f1 to
a76783d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/query-core/src/__tests__/query.test.tsx (4)
89-97: Avoid magic timer values; flush timers deterministicallyUsing a fixed 10ms advance is brittle. Prefer running pending timers to completion in this test to avoid coupling to retryDelay:
-// With new behavior, retries continue in background -// Wait for retries to complete -await vi.advanceTimersByTimeAsync(10) -expect(result).toBe('data3') +// Flush scheduled retries deterministically +await vi.runAllTimersAsync() +expect(result).toBe('data3')If you want to keep time-based advancing, compute from the configured retryDelay and number of retries to document intent (e.g., advance by retryDelay * 3 + epsilon).
149-178: Duplicate of the previous test; consider consolidatingThis test appears to assert the same behavior as the earlier “continue retry … while hidden” case with identical setup and expectations. Consider merging into a single, descriptively named test or parameterizing to reduce duplication and test time.
Example consolidation (rename earlier test and remove this one), or parameterize with it.each over an array of titles if you need both names for historical clarity.
180-217: Tighten cancellation assertions and timing
- Use toBeInstanceOf for clarity/readability:
- expect(result instanceof CancelledError).toBe(true) - expect(result instanceof Error).toBe(true) + expect(result).toBeInstanceOf(CancelledError)
- To make “cancel during retry” explicit with retryDelay: 100, consider advancing to a point between attempts (e.g., 50ms) before cancelling:
-// Wait briefly for first failure and start of retry -await vi.advanceTimersByTimeAsync(1) +// Wait into the backoff window before the next retry +await vi.advanceTimersByTimeAsync(50)This reduces ambiguity between “after initial failure but before retry starts” vs “during a retry backoff window”.
258-293: Good coverage of refetchIntervalInBackground; minor cleanup suggested
- The test accurately validates background refetch+retry behavior when hidden with refetchIntervalInBackground: true. Nice.
- Minor test hygiene: subscribe returns an unsubscribe function—prefer using it to clean up instead of destroy(), which is more forceful than needed here:
- queryObserver.subscribe(() => {}) + const unsubscribe = queryObserver.subscribe(() => {}) … - queryObserver.destroy() + unsubscribe()
- Optional: make interval values self-documenting and avoid repeated literals:
- refetchInterval: 60000, + const interval = 60_000 + … + refetchInterval: interval, … - await vi.advanceTimersByTimeAsync(60000) + await vi.advanceTimersByTimeAsync(interval)
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
packages/query-core/src/__tests__/query.test.tsx(6 hunks)packages/query-core/src/retryer.ts(0 hunks)packages/react-query/src/__tests__/useQuery.test.tsx(3 hunks)packages/solid-query/src/__tests__/useQuery.test.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- packages/query-core/src/retryer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/solid-query/src/tests/useQuery.test.tsx
- packages/react-query/src/tests/useQuery.test.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/query-core/src/__tests__/query.test.tsx (2)
packages/query-core/src/query.ts (1)
promise(198-200)packages/query-core/src/retryer.ts (1)
CancelledError(57-65)
🔇 Additional comments (1)
packages/query-core/src/__tests__/query.test.tsx (1)
61-99: The search results didn’t reveal the Retryer implementation location directly. To determine whetherconfig.canRun()bypasses focus gating unconditionally or only whenrefetchIntervalInBackgroundis enabled, I’ll need to locate:
- Where
config.canRunis defined/passed intoRetryer.- How focus gating is implemented in
canRunbased on visibility state andrefetchIntervalInBackground.Let me search for where
Retryeris constructed to inspect theconfigobject.
Hellol77
changed the title
Hellol77
force-pushed
the
fix/retry-background-focus-issue
branch
from
a76783d to
a5ad6ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/query-core/src/types.ts (2)
279-281: TSDoc: clarify that this also affects retry gating, not just interval refetchingWith this PR, the flag now also allows retries to continue while unfocused. Consider clarifying the description and a tiny grammar tweak.
Apply this diff to update the comment:
/** - * If set to `true`, the query will continue to refetch while their tab/window is in the background. - * Defaults to `false`. + * If set to `true`, the query will continue to refetch and retry even when the tab/window is in the background. + * Defaults to `false`. */
278-283: Avoid duplicate declarations between QueryOptions and QueryObserverOptionsNow that QueryObserverOptions extends QueryOptions and this property lives on QueryOptions, the explicit redeclaration on QueryObserverOptions (Line 354–357) is redundant and risks docs drift.
Two options:
- Remove the property from QueryObserverOptions and let it inherit from QueryOptions (preferred).
- If you want to keep docs colocated on the observer interface, remove it from QueryOptions and fetch it from the first observer’s options when wiring the retryer (heavier change).
If you opt for the first approach, remove the member from QueryObserverOptions (outside this hunk).
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
packages/query-core/src/__tests__/query.test.tsx(1 hunks)packages/query-core/src/query.ts(2 hunks)packages/query-core/src/retryer.ts(3 hunks)packages/query-core/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/query-core/src/tests/query.test.tsx
- packages/query-core/src/retryer.ts
🔇 Additional comments (4)
packages/query-core/src/types.ts (1)
278-283: Adding refetchIntervalInBackground to QueryOptions is the right callThis unblocks passing the option into the retryer from Query, which only has QueryOptions available. Matches the PR’s objective and is backward-compatible.
packages/query-core/src/query.ts (3)
381-385: Typo fix in comment — LGTM“retryer” spelling fix reads well and clarifies the guard’s intent.
524-525: Plumbing refetchIntervalInBackground into createRetryer is correctThis is the crucial connection so canContinue can honor background retries. Looks good.
524-525: No changes needed:refetchIntervalInBackgroundis present andcanContinueimplements the intended semantics.The
RetryerConfiginterface declares:refetchIntervalInBackground?: booleanand
canContinueis defined as:const canContinue = () => (config.refetchIntervalInBackground === true || focusManager.isFocused()) && (config.networkMode === 'always' || onlineManager.isOnline()) && config.canRun()This matches the requested behavior.
|
Got it, I updated this PR to add a check for the refetchIntervalInBackground prop in the retryer instead of removing the focus check. @TkDodo |
Hellol77
force-pushed
the
fix/retry-background-focus-issue
branch
from
a5ad6ab to
2c351a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/query-core/src/types.ts (2)
278-283: Clarify that this flag governs retries as well, and avoid duplicate declarations drifting over timeGood addition. Since retryer.ts now uses this option to allow background retries, the JSDoc should reflect that it affects both retries and interval refetches. Also, this property is declared here and again in QueryObserverOptions (Lines 354-357) with slightly different wording; consider consolidating to a single source to prevent future drift.
Apply this doc tweak within the current hunk:
/** - * If set to `true`, the query will continue to refetch while their tab/window is in the background. - * Defaults to `false`. + * If set to `true`, the query will continue retries and refetches while the tab/window is in the background. + * Also influences retry behavior: background retries will proceed even when unfocused. Defaults to `false`. */ refetchIntervalInBackground?: booleanOptional consolidation (follow-up): Remove the duplicate property from QueryObserverOptions and rely on inheritance, or keep both but make their JSDoc identical. If your docs rely on the sub-interface re-declaration, keep it and sync the wording (see next comment).
354-357: Keep wording consistent here to match the new behaviorMirror the updated semantics so both declarations say the same thing.
Suggested text:
/** * If set to `true`, the query will continue retries and refetches while the tab/window is in the background. * Defaults to `false`. */ refetchIntervalInBackground?: boolean
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
packages/query-core/src/__tests__/query.test.tsx(1 hunks)packages/query-core/src/query.ts(2 hunks)packages/query-core/src/retryer.ts(3 hunks)packages/query-core/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/query-core/src/retryer.ts
- packages/query-core/src/tests/query.test.tsx
- packages/query-core/src/query.ts
🔇 Additional comments (1)
packages/query-core/src/types.ts (1)
278-283: Verified refetchIntervalInBackground forwarding and guard logic
- In
packages/query-core/src/query.ts(around line 524),this.options.refetchIntervalInBackgroundis passed directly intocreateRetryerasrefetchIntervalInBackground: this.options.refetchIntervalInBackground.- In
packages/query-core/src/retryer.ts(lines 103–106), thecanContinueguard correctly checks(config.refetchIntervalInBackground === true || focusManager.isFocused())before proceeding.The plumbing matches the intended behavior.
fixed #8353
Summary by CodeRabbit
New Features
Tests