fix(react): allow using enabled when using useQuery().promise
#7
Conversation
|
View your CI Pipeline Execution ↗ for commit 2f69183.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8501 +/- ##
===========================================
+ Coverage 46.25% 63.04% +16.78%
===========================================
Files 199 135 -64
Lines 7530 4835 -2695
Branches 1719 1358 -361
===========================================
- Hits 3483 3048 -435
+ Misses 3668 1543 -2125
+ Partials 379 244 -135 |
WalkthroughThe changes introduce new test cases to both the core and React layers of the query package, focusing on the behavior of queries when toggling the Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useQuery
participant QueryCache
participant QueryObserver
Component->>useQuery: Render with enabled: false
useQuery->>QueryCache: Check for cache entry
QueryCache-->>useQuery: Return cache entry (possibly undefined)
useQuery->>QueryObserver: Subscribe (no fetch)
Component->>useQuery: Update enabled: true
useQuery->>QueryCache: Inspect cache entry state
alt Should fetch (no data or pending/idle)
useQuery->>QueryCache: fetchOptimistic()
else Already fetching or data present
useQuery->>QueryCache: Subscribe to existing promise
end
QueryCache-->>useQuery: Promise/data
useQuery-->>Component: Return data or suspense
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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 (1)
packages/react-query/src/useBaseQuery.ts (1)
144-152: Improved granular query fetch control with detailed state inspection.The code now examines the cache entry's internal state properties instead of simply checking if the entry exists. This allows for more precise control over when to fetch data during render, especially when toggling the
enabledflag.As noted in the comment, this logic might be better placed in
getOptimisticResult()in the future. Consider creating a follow-up issue to track this potential refactor if issue TanStack#8507 doesn't already cover it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/query-core/src/__tests__/queryObserver.test.tsx(3 hunks)packages/react-query/src/__tests__/useQuery.promise.test.tsx(1 hunks)packages/react-query/src/useBaseQuery.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/query-core/src/__tests__/queryObserver.test.tsx (3)
packages/query-core/src/__tests__/utils.ts (1)
queryKey(24-27)packages/query-core/src/queryObserver.ts (1)
QueryObserver(43-759)packages/query-core/src/thenable.ts (1)
pendingThenable(42-82)
packages/react-query/src/useBaseQuery.ts (1)
packages/query-core/src/query.ts (1)
promise(193-195)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (4)
packages/react-query/src/useBaseQuery.ts (1)
153-157: Improved conditional query fetching with the newshouldFetchcondition.The code now uses a more precise condition to determine when to fetch data or subscribe to an existing promise. This is particularly important for correctly handling queries when their
enabledstate changes.packages/react-query/src/__tests__/useQuery.promise.test.tsx (1)
1381-1447: Well-structured test forenabledstate changes with suspense.This test effectively validates that the
enabledflag works correctly with React Suspense:
- Initially the query doesn't run when disabled
- When enabled, the query shows the loading state
- The query function runs exactly once
- Data is properly rendered after fetching completes
The test is thorough and directly addresses the issue in the PR title regarding using
enabledwithuseQuery().promise.packages/query-core/src/__tests__/queryObserver.test.tsx (2)
1-12: Appropriate imports added to support new tests.The addition of
waitForfrom@testing-library/domandpendingThenablefrom the thenable module provides the necessary utilities for the new test.
1238-1270: Valuable test ensuring promise reuse when togglingenabledstate.This test verifies a critical behavior: when toggling a query from disabled to enabled, the same promise instance should be reused. This is important for React Suspense to work correctly with the promise returned by
useQuery().The test complements the React-level test by verifying the core behavior at the QueryObserver level, ensuring consistent promise handling throughout the library.
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| willFetch(result, isRestoring) | ||
| ) { | ||
| const promise = isNewCacheEntry | ||
| // This fetching in the render should likely be done as part of the getOptimisticResult() considering https://github.com/TanStack/query/issues/8507 |
There was a problem hiding this comment.
Logic Issue: The condition for shouldFetch has been modified to check cache entry state, but the comment "This fetching in the render should likely be done as part of the getOptimisticResult() considering TanStack#8507" suggests this may be a temporary solution. Consider addressing this architectural improvement in a future PR to ensure proper separation of concerns and better maintainability.
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Security Review: Query Promise👍 Well Done
📝 Additional Comments
|
| // This fetching in the render should likely be done as part of the getOptimisticResult() considering https://github.com/TanStack/query/issues/8507 | ||
| const state = cacheEntry?.state | ||
|
|
||
| const shouldFetch = |
There was a problem hiding this comment.
Race Condition Risk
Insufficient state validation creates race condition when enabled changes. Concurrent renders could trigger multiple fetches with inconsistent state. Potential memory leaks and unpredictable component behavior.
| // This fetching in the render should likely be done as part of the getOptimisticResult() considering https://github.com/TanStack/query/issues/8507 | |
| const state = cacheEntry?.state | |
| const shouldFetch = | |
| const promise = shouldFetch | |
| ? // Fetch immediately on render in order to ensure `.promise` is resolved even if the component is unmounted | |
| fetchOptimistic(defaultedOptions, observer, errorResetBoundary) | |
| : // subscribe to the "cache promise" so that we can finalize the currentThenable once data comes in | |
| observer.getOptimisticResult(defaultedOptions).promise |
Standards
- CWE-362
- OWASP-A06
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Promise Handling in React Query👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| // This fetching in the render should likely be done as part of the getOptimisticResult() considering https://github.com/TanStack/query/issues/8507 | ||
| const state = cacheEntry?.state | ||
|
|
||
| const shouldFetch = | ||
| !state || | ||
| (state.data === undefined && | ||
| state.status === 'pending' && | ||
| state.fetchStatus === 'idle') | ||
|
|
||
| const promise = shouldFetch |
There was a problem hiding this comment.
Inconsistent Fetch Condition
The condition for fetching changed from 'isNewCacheEntry' to 'shouldFetch', but the variable 'isNewCacheEntry' is no longer used. This creates inconsistency between variable declaration and usage, potentially causing unexpected behavior when the enabled state changes.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
1 similar comment
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: React Query Promise Handling👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| const shouldFetch = | ||
| !state || | ||
| (state.data === undefined && | ||
| state.status === 'pending' && | ||
| state.fetchStatus === 'idle') |
There was a problem hiding this comment.
Incorrect Fetch Condition
The fetch condition logic has been changed from a simple cache entry check to a more complex state-based condition. However, the new condition will trigger a fetch when state.data is undefined AND status is pending AND fetchStatus is idle, which is too restrictive and may prevent necessary fetches in other valid scenarios.
const shouldFetch =
!state ||
(state.data === undefined &&
(state.status !== 'success' || state.fetchStatus === 'idle'))
Commitable Suggestion
| const shouldFetch = | |
| !state || | |
| (state.data === undefined && | |
| state.status === 'pending' && | |
| state.fetchStatus === 'idle') | |
| const shouldFetch =, !state ||, (state.data === undefined &&, (state.status !== 'success' || state.fetchStatus === 'idle')) |
Standards
- Algorithm-Correctness-Conditional-Logic
- Business-Rule-State-Management
- Logic-Verification-Data-Flow
| const shouldFetch = | ||
| !state || | ||
| (state.data === undefined && | ||
| state.status === 'pending' && | ||
| state.fetchStatus === 'idle') |
There was a problem hiding this comment.
Inconsistent Fetch Condition
The shouldFetch condition doesn't account for enabled state changes. When enabled changes from false to true, the condition may not trigger a fetch if state exists with pending status, causing stale or missing data. This creates inconsistent behavior when toggling enabled state.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Predictable-Behavior
| const shouldFetch = | ||
| !state || | ||
| (state.data === undefined && | ||
| state.status === 'pending' && | ||
| state.fetchStatus === 'idle') |
There was a problem hiding this comment.
Redundant State Check
The condition checks for !state first, then accesses state properties without null check in the second condition. If state is null/undefined, the second condition would cause runtime errors. This creates potential for uncaught exceptions during state transitions.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Preconditions
| const shouldFetch = | ||
| !state || | ||
| (state.data === undefined && | ||
| state.status === 'pending' && | ||
| state.fetchStatus === 'idle') |
There was a problem hiding this comment.
Redundant State Check
The shouldFetch condition performs redundant checks that could be simplified. When state is undefined (!state), the subsequent condition is unnecessary. This creates extra boolean operations on every render affecting component performance under high frequency renders.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Conditional-Logic
- Algorithmic-Complexity-Boolean-Logic
| willFetch(result, isRestoring) | ||
| ) { | ||
| const promise = isNewCacheEntry | ||
| // This fetching in the render should likely be done as part of the getOptimisticResult() considering https://github.com/TanStack/query/issues/8507 |
There was a problem hiding this comment.
Inline Comment Placement
The comment is placed directly before the implementation logic it describes, breaking the flow of code. Moving this comment above the entire block (before line 143) would improve readability while maintaining the context.
Standards
- Clean-Code-Comments
- Maintainability-Quality-Readability
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: React Query Promise Handling👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| const shouldFetch = | ||
| !state || | ||
| (state.data === undefined && | ||
| state.status === 'pending' && | ||
| state.fetchStatus === 'idle') |
There was a problem hiding this comment.
Unnecessary Fetch Condition
The shouldFetch condition checks for pending status but idle fetchStatus, which is contradictory. This could cause unnecessary fetches or missed fetches in edge cases, affecting query reliability and causing inconsistent behavior.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| // This fetching in the render should likely be done as part of the getOptimisticResult() considering https://github.com/TanStack/query/issues/8507 | ||
| const state = cacheEntry?.state | ||
|
|
||
| const shouldFetch = | ||
| !state || | ||
| (state.data === undefined && | ||
| state.status === 'pending' && | ||
| state.fetchStatus === 'idle') |
There was a problem hiding this comment.
Incorrect Fetch Condition
The original condition used isNewCacheEntry to determine fetching, but the new logic checks state properties. This creates a logical inconsistency where a query with pending status but non-idle fetchStatus would be incorrectly skipped, potentially causing stale data to be used.
Standards
- Algorithm-Correctness-Conditional-Logic
- Business-Rule-State-Consistency
| expect(queryFn).toHaveBeenCalledTimes(0) | ||
| rendered.getByText('enable').click() | ||
|
|
||
| { | ||
| const result = await renderStream.takeRender() | ||
| result.withinDOM().getByText('loading..') | ||
| } | ||
|
|
There was a problem hiding this comment.
Incomplete Test Coverage
The test verifies queryFn is called exactly once, but doesn't test the case where enabled changes from true to false and back to true. This leaves a logical gap in test coverage since the promise reuse behavior isn't fully verified across multiple enabled state transitions.
Standards
- Algorithm-Correctness-Test-Coverage
- Logic-Verification-Edge-Cases
| willFetch(result, isRestoring) | ||
| ) { | ||
| const promise = isNewCacheEntry | ||
| // This fetching in the render should likely be done as part of the getOptimisticResult() considering https://github.com/TanStack/query/issues/8507 |
There was a problem hiding this comment.
Inline Comment Clarity
The comment indicates a potential architectural improvement but doesn't explain why the current implementation is problematic. Adding context about the architectural concern would improve future maintainability when revisiting this code.
Standards
- Clean-Code-Comments
- Maintainability-Quality-Documentation
4 participants
Closes TanStack#8499
Summary by CodeRabbit