fix(query-core): remove focus check 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
from
39745de to
7250804
Compare
Hellol77
force-pushed
the
fix/retry-background-focus-issue
branch
from
7250804 to
56049b5
Compare
|
I fixed the conflicts and test issues! thanks for review. @TkDodo |
WalkthroughTests across core and adapters were updated to expect retries and refetch intervals to continue while unfocused/hidden. Core retry logic removed focus gating. A public option refetchIntervalInBackground was added to QueryObserverOptions. New tests cover background retries and cancellation behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant QueryObserver
participant Retryer
participant onlineManager
App->>QueryObserver: start query (refetchInterval, retry)
QueryObserver->>Retryer: execute with retry config
Retryer->>onlineManager: isOnline?
alt networkMode == "always" or online
Retryer-->>Retryer: schedule retries (tab may be hidden)
Retryer-->>QueryObserver: success or error after retries
else
Retryer-->>QueryObserver: wait until online
end
QueryObserver-->>App: notify result (continues even when hidden)
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
fixed #8353
Summary
focusManager.isFocused()check from retryer'scanContinue()function
test modifications
Summary by CodeRabbit
New Features
Tests