fix(core): never revert to undefined data #9577
Conversation
WalkthroughAdds a CancelledError export and modifies Query.fetch so canceling an initial (no-data) fetch triggers a CancelledError; tests updated to import CancelledError, simplify cancelQueries to a single key scenario, and add a test asserting imperative fetchQuery rejects with CancelledError. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Client as QueryClient
participant Cache as QueryCache
participant Query as Query
Test->>Client: fetchQuery(key, fn) -> Promise
Client->>Cache: get/create Query
Cache->>Query: fetch()
Note right of Query: fetch starts (initial/no data)
Test->>Client: cancelQueries(key)
Client->>Query: cancel()
Query->>Query: revert state to previous
alt previous data exists
Query-->>Client: resolve(existing data) and return data
else no previous data (initial fetch)
Query->>Client: set error = CancelledError
Query-->>Client: throw CancelledError (promise rejects)
end
Test-->>Test: expect promise to reject instanceOf CancelledError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ 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 (
|
|
View your CI Pipeline Execution ↗ for commit 06e178f
☁️ Nx Cloud last updated this comment at |
|
Sizes for commit 06e178f:
|
![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 (2)
packages/query-core/src/__tests__/queryClient.test.tsx (1)
1065-1081: Great addition: asserts cancel-on-initial-fetch rejects with CancelledErrorThis directly guards against regressing to “resolve with undefined” on cancellation. Consider strengthening the test by also asserting the resulting query state transitions to error with a CancelledError.
Apply this diff to extend the assertions:
await queryClient.cancelQueries({ queryKey: key }) - await expect(promise).rejects.toBeInstanceOf(CancelledError) + await expect(promise).rejects.toBeInstanceOf(CancelledError) + const state = queryClient.getQueryState(key) + expect(state?.status).toBe('error') + expect(state?.fetchStatus).toBe('idle') + expect(state?.data).toBeUndefined() + expect(state?.error).toBeInstanceOf(CancelledError) })packages/query-core/src/query.ts (1)
561-565: Minor clarity: read from revert snapshot instead of mutable stateYou’ve just restored state from #revertState; checking the snapshot directly makes the intent explicit and decouples the return value from any subsequent state churn.
Apply this small refactor:
- if (this.state.data !== undefined) { - return this.state.data - } + const revertedData = this.#revertState?.data + if (revertedData !== undefined) { + return revertedData + }
📜 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 (2)
packages/query-core/src/__tests__/queryClient.test.tsx(2 hunks)packages/query-core/src/query.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/query-core/src/__tests__/queryClient.test.tsx (3)
packages/query-core/src/query.ts (1)
promise(198-200)packages/query-core/src/utils.ts (1)
sleep(355-359)packages/query-core/src/retryer.ts (1)
CancelledError(58-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/query-core/src/__tests__/queryClient.test.tsx (1)
4-4: Verified public API export of CancelledError
- Defined in packages/query-core/src/retryer.ts (line 58)
- Re-exported in packages/query-core/src/index.ts (line 3)
You can safely import CancelledError from the package root in your tests.
packages/query-core/src/query.ts (1)
561-565: Fix is correct: don’t revert to undefined data on cancel of initial fetchBehavior now reverts only if there’s prior data; otherwise it enters the error path and rethrows CancelledError. This matches the intended contract for initial fetch cancellation and prevents accidental resolution with undefined.
| return this.state.data! | ||
| // if the initial fetch was cancelled, we have no data, so we have | ||
| // to get reject with a CancelledError | ||
| if (this.state.data === undefined) { | ||
| throw error | ||
| } | ||
| return this.state.data |
There was a problem hiding this comment.
this changes makes sure we never resolve with undefined for imperative methods like queryClient.fetchQuery(...). The bang operator ! was a dead giveaway that this could happen 😅
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9577 +/- ##
===========================================
+ Coverage 45.15% 59.26% +14.11%
===========================================
Files 208 137 -71
Lines 8318 5558 -2760
Branches 1875 1489 -386
===========================================
- Hits 3756 3294 -462
+ Misses 4115 1960 -2155
+ Partials 447 304 -143 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/queryClient.test.tsx (1)
997-1013: Make the pending fetch explicit to avoid potential unhandled rejections and assert the new behavior.
Today this is fine, but if cancel semantics regress to a rejection in the “has data” case, this could turn into an unhandled rejection. Capturing the Promise lets us assert it resolves to previous data after revert.Apply:
- void queryClient.fetchQuery({ + const pending = queryClient.fetchQuery({ queryKey: key1, queryFn: () => sleep(1000).then(() => 'data2'), }) await vi.advanceTimersByTimeAsync(10) await queryClient.cancelQueries() + // with previous data present, imperative fetch should resolve to that data after cancel + await expect(pending).resolves.toBe('data') + const state1 = queryClient.getQueryState(key1) expect(state1).toMatchObject({ data: 'data', status: 'success', })
📜 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 (2)
packages/query-core/src/__tests__/queryClient.test.tsx(3 hunks)packages/query-core/src/query.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/query-core/src/query.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/query-core/src/__tests__/queryClient.test.tsx (2)
packages/query-core/src/utils.ts (1)
sleep(355-359)packages/query-core/src/retryer.ts (1)
CancelledError(58-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: autofix
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/query-core/src/__tests__/queryClient.test.tsx (2)
4-4: Importing CancelledError for instance checks looks right; verify re-export path.
AssumingCancelledErroris re-exported from the package root (..), this keeps tests aligned with the public API surface. If it isn't re-exported, switch the import to its source module to avoidinstanceofmismatches.
1033-1059: Solid regression test for cancelled initial fetch -> CancelledError and reverted state.
Asserts both rejection type and final state (pending/idle/no data/no error). This aligns with the intended “never revert to undefined data” behavior for initial fetches.
Summary by CodeRabbit
New Features
Bug Fixes
Tests