Clone use query state #10
Conversation
the value of the ref can get out of sync if we only write the ref in the subscribe function
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Clone use query stateTL;DR: Refactors React Query hooks to use a new shared Refacto PR SummaryIntroduces Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant C as Component
participant UQS as useQueryState
participant QC as QueryCache
participant SE as SyncExternalStore
C->>UQS: useQueryState(filters)
UQS->>QC: getQueryCache()
UQS->>SE: useSyncExternalStore(subscribe, getSnapshot)
QC->>SE: subscribe to cache changes
QC->>QC: Query state changes
QC-->>SE: Notify subscribers (batched)
SE->>UQS: Trigger getSnapshot()
UQS->>UQS: replaceEqualDeep comparison
UQS-->>C: Return filtered query states
Testing GuideClick to expand
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a new hook useQueryState and exports it. Refactors useIsFetching to derive its value via useQueryState. Adjusts useMutationState internals to batch notifications and compute results lazily. Updates related tests: minor hook placement change in useIsFetching test and memoized components in useMutationState tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Component (Page)
participant UIF as useIsFetching
participant UQS as useQueryState
participant QC as QueryClient
participant QCache as QueryCache
rect rgba(230,245,255,0.5)
Note over C: Render phase
C->>UIF: call(filters, queryClient?)
UIF->>UQS: derive({ ...filters, fetchStatus: 'fetching' }, queryClient?)
end
rect rgba(240,255,240,0.5)
UQS->>QC: useQueryClient() / resolve client
UQS->>QCache: get queries matching filters
UQS-->>UIF: return Array<TResult>
UIF-->>C: return count (array.length)
end
rect rgba(255,248,230,0.5)
Note over UQS,QCache: Subscription (useSyncExternalStore)
UQS->>QCache: subscribe(batched)
QCache-->>UQS: notify on changes
UQS-->>UIF: recompute and memoize results
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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). (1)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new useQueryState hook and refactors useIsFetching to leverage it, which is a great abstraction. It also updates useMutationState to follow a similar implementation pattern for consistency. However, I've identified a potential issue in both useQueryState and the updated useMutationState regarding how options are handled within useSyncExternalStore. The current pattern of using a useRef updated via useEffect can lead to the component rendering with stale data for one cycle when options change. My review includes specific suggestions to resolve this by using the options prop directly in the getSnapshot function.
| () => { | ||
| const nextResult = replaceEqualDeep( | ||
| result.current, | ||
| getResult(mutationCache, optionsRef.current), |
There was a problem hiding this comment.
Using optionsRef.current here can lead to stale data. The optionsRef is updated in a useEffect, which runs after the render pass where getSnapshot is executed. This means getSnapshot might be using stale options.
You should use the options prop directly from the hook's arguments, as getSnapshot is recreated on each render and will have access to the latest options via its closure. This would also allow removing optionsRef and the corresponding useEffect from the hook.
| getResult(mutationCache, optionsRef.current), | |
| getResult(mutationCache, options), |
| const queryCache = useQueryClient(queryClient).getQueryCache() | ||
| const optionsRef = React.useRef(options) | ||
| const result = React.useRef<Array<TResult>>() | ||
| if (!result.current) { | ||
| result.current = getResult(queryCache, options) | ||
| } | ||
|
|
||
| React.useEffect(() => { | ||
| optionsRef.current = options | ||
| }) | ||
|
|
||
| return React.useSyncExternalStore( | ||
| React.useCallback( | ||
| (onStoreChange) => | ||
| queryCache.subscribe(notifyManager.batchCalls(onStoreChange)), | ||
| [queryCache], | ||
| ), | ||
| () => { | ||
| const nextResult = replaceEqualDeep( | ||
| result.current, | ||
| getResult(queryCache, optionsRef.current), | ||
| ) | ||
| if (result.current !== nextResult) { | ||
| result.current = nextResult | ||
| } | ||
|
|
||
| return result.current | ||
| }, | ||
| () => result.current, | ||
| )! |
There was a problem hiding this comment.
The current implementation using optionsRef and useEffect can lead to returning stale data for one render cycle when the options prop changes. This is because useEffect runs after rendering, so optionsRef.current is updated too late for useSyncExternalStore's getSnapshot function, which runs during rendering.
A better approach is to remove optionsRef and useEffect, and use the options prop directly inside getSnapshot. Since getSnapshot is redefined on each render, it will correctly close over the latest options.
const queryCache = useQueryClient(queryClient).getQueryCache()
const result = React.useRef<Array<TResult>>()
// Eagerly calculate result for first render and server render
if (result.current === undefined) {
result.current = getResult(queryCache, options)
}
return React.useSyncExternalStore(
React.useCallback(
(onStoreChange) =>
queryCache.subscribe(notifyManager.batchCalls(onStoreChange)),
[queryCache],
),
() => {
const nextResult = replaceEqualDeep(
result.current,
getResult(queryCache, options),
)
if (result.current !== nextResult) {
result.current = nextResult
}
return result.current
},
() => result.current,
)!![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/useQueryState.ts (1)
21-67: Consider extracting shared logic to reduce duplication.The implementation is correct and follows best practices for
useSyncExternalStore. However, this hook shares nearly identical structure withuseMutationState(lines 33-47 and 49-83 inuseMutationState.ts):
- Both use
useSyncExternalStorewith batched notifications- Both use
replaceEqualDeepfor efficient updates- Both maintain stable refs with identical update patterns
Consider extracting a generic helper hook like
useCacheStateto reduce duplication and simplify maintenance. This would centralize the subscription logic and ensure consistent behavior across both hooks.Example structure for a shared helper:
function useCacheState<TEntity, TResult>( cache: { subscribe: (callback: () => void) => () => void, findAll: (filters: any) => TEntity[] }, options: { filters?: any, select?: (entity: TEntity) => TResult }, defaultResult: (entity: TEntity) => any ): Array<TResult> { // Shared useSyncExternalStore logic here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/react-query/src/__tests__/useIsFetching.test.tsx(1 hunks)packages/react-query/src/__tests__/useMutationState.test.tsx(3 hunks)packages/react-query/src/index.ts(1 hunks)packages/react-query/src/useIsFetching.ts(1 hunks)packages/react-query/src/useMutationState.ts(1 hunks)packages/react-query/src/useQueryState.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/react-query/src/__tests__/useMutationState.test.tsx (1)
packages/react-query/src/useMutationState.ts (1)
useIsMutating(15-24)
packages/react-query/src/useQueryState.ts (1)
packages/query-core/src/types.ts (2)
DefaultError(18-22)QueryKey(24-24)
packages/react-query/src/useIsFetching.ts (2)
packages/query-core/src/queryClient.ts (1)
QueryClient(54-550)packages/react-query/src/useQueryState.ts (1)
useQueryState(33-67)
packages/react-query/src/__tests__/useIsFetching.test.tsx (2)
packages/query-core/src/queryClient.ts (1)
isFetching(102-105)packages/react-query/src/useIsFetching.ts (1)
useIsFetching(5-13)
⏰ 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). (1)
- GitHub Check: Preview
🔇 Additional comments (5)
packages/react-query/src/useMutationState.ts (1)
67-82: LGTM! Batching and deep equality optimization.The changes improve performance by batching cache notifications and using deep equality checks to prevent unnecessary re-renders. The pattern correctly leverages
useSyncExternalStorewith stable result references.packages/react-query/src/index.ts (1)
43-43: LGTM! Public API extension.The export makes
useQueryStateavailable as part of the public API, consistent with other state hooks likeuseMutationState.packages/react-query/src/__tests__/useIsFetching.test.tsx (1)
208-208: LGTM! Correct hook placement.Moving the
useIsFetchingcall inside thePagecomponent scope is correct and aligns with React hook rules. The explicitqueryClientparameter ensures the test uses the intended client instance.packages/react-query/src/__tests__/useMutationState.test.tsx (1)
69-76: LGTM! Test isolation with memoization.The
React.memowrappers correctly isolate theIsMutatingcomponent from parent re-renders triggered byuseMutationhooks. This ensures the test accurately measuresuseIsMutating's subscription behavior without noise from unrelated state changes.This pattern improves test reliability by focusing on the specific hook's behavior.
Also applies to: 110-119
packages/react-query/src/useIsFetching.ts (1)
9-12: LGTM! Clean refactor leveraging useQueryState.The refactor simplifies
useIsFetchingby delegating to the newuseQueryStatehook withfetchStatus: 'fetching'filter. This matches the previous behavior (as confirmed by thequeryClient.isFetchingimplementation in the relevant snippets) while reducing code duplication and maintenance burden.
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: React Query State Management👍 Well Done
📁 Selected files for review (6)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| () => { | ||
| const nextResult = replaceEqualDeep( | ||
| result.current, | ||
| getResult(queryCache, optionsRef.current), | ||
| ) | ||
| if (result.current !== nextResult) { | ||
| result.current = nextResult | ||
| } | ||
|
|
||
| return result.current | ||
| }, |
There was a problem hiding this comment.
Duplicated State Logic
Identical state management logic exists in both useQueryState and useMutationState hooks. This violates DRY principle creating maintenance overhead when state handling needs modification. Extract shared state management utility to eliminate code duplication.
() => {
return getStateResult(result, () => getResult(queryCache, optionsRef.current))
},
Commitable Suggestion
| () => { | |
| const nextResult = replaceEqualDeep( | |
| result.current, | |
| getResult(queryCache, optionsRef.current), | |
| ) | |
| if (result.current !== nextResult) { | |
| result.current = nextResult | |
| } | |
| return result.current | |
| }, | |
| () => { | |
| return getStateResult(result, () => getResult(queryCache, optionsRef.current)) | |
| }, |
Standards
- Org-Guideline-Reuse code wherever possible and avoid redundant code by refactoring with utils and static method across application
- Clean-Code-DRY
- Refactoring-Extract-Function
| return React.useSyncExternalStore( | ||
| React.useCallback( | ||
| (onStoreChange) => | ||
| queryCache.subscribe(notifyManager.batchCalls(onStoreChange)), | ||
| [queryCache], | ||
| ), |
There was a problem hiding this comment.
Duplicated Subscription Pattern
Subscription pattern with notifyManager.batchCalls is duplicated across useQueryState and useMutationState. This creates redundant implementation that must be maintained in multiple locations. Refactor into shared subscription utility function.
return React.useSyncExternalStore(
React.useCallback(
(onStoreChange) => createCacheSubscription(queryCache, onStoreChange),
[queryCache],
),
Commitable Suggestion
| return React.useSyncExternalStore( | |
| React.useCallback( | |
| (onStoreChange) => | |
| queryCache.subscribe(notifyManager.batchCalls(onStoreChange)), | |
| [queryCache], | |
| ), | |
| return React.useSyncExternalStore( | |
| React.useCallback( | |
| (onStoreChange) => createCacheSubscription(queryCache, onStoreChange), | |
| [queryCache], | |
| ), |
Standards
- Org-Guideline-Reuse code wherever possible and avoid redundant code by refactoring with utils and static method across application
- Clean-Code-DRY
- Design-Pattern-Template-Method
| return React.useSyncExternalStore( | ||
| React.useCallback( | ||
| (onStoreChange) => | ||
| mutationCache.subscribe(() => { | ||
| const nextResult = replaceEqualDeep( | ||
| result.current, | ||
| getResult(mutationCache, optionsRef.current), | ||
| ) | ||
| if (result.current !== nextResult) { | ||
| result.current = nextResult | ||
| notifyManager.schedule(onStoreChange) | ||
| } | ||
| }), | ||
| mutationCache.subscribe(notifyManager.batchCalls(onStoreChange)), | ||
| [mutationCache], | ||
| ), | ||
| () => result.current, | ||
| () => { | ||
| const nextResult = replaceEqualDeep( | ||
| result.current, | ||
| getResult(mutationCache, optionsRef.current), | ||
| ) | ||
| if (result.current !== nextResult) { | ||
| result.current = nextResult | ||
| } | ||
|
|
||
| return result.current | ||
| }, | ||
| () => result.current, | ||
| )! |
There was a problem hiding this comment.
Code Duplication Violation
Violates organization guideline requiring code reuse and redundancy elimination. The useSyncExternalStore pattern with cache subscription and result comparison is duplicated between useMutationState and useQueryState. This creates maintenance overhead and potential inconsistencies in state synchronization logic.
return React.useSyncExternalStore(
React.useCallback(
(onStoreChange) => createCacheSubscription(mutationCache, onStoreChange),
[mutationCache],
),
() => getStateResult(result, () => getResult(mutationCache, optionsRef.current)),
() => result.current,
)!
Commitable Suggestion
| return React.useSyncExternalStore( | |
| React.useCallback( | |
| (onStoreChange) => | |
| mutationCache.subscribe(() => { | |
| const nextResult = replaceEqualDeep( | |
| result.current, | |
| getResult(mutationCache, optionsRef.current), | |
| ) | |
| if (result.current !== nextResult) { | |
| result.current = nextResult | |
| notifyManager.schedule(onStoreChange) | |
| } | |
| }), | |
| mutationCache.subscribe(notifyManager.batchCalls(onStoreChange)), | |
| [mutationCache], | |
| ), | |
| () => result.current, | |
| () => { | |
| const nextResult = replaceEqualDeep( | |
| result.current, | |
| getResult(mutationCache, optionsRef.current), | |
| ) | |
| if (result.current !== nextResult) { | |
| result.current = nextResult | |
| } | |
| return result.current | |
| }, | |
| () => result.current, | |
| )! | |
| return React.useSyncExternalStore( | |
| React.useCallback( | |
| (onStoreChange) => createCacheSubscription(mutationCache, onStoreChange), | |
| [mutationCache], | |
| ), | |
| () => getStateResult(result, () => getResult(mutationCache, optionsRef.current)), | |
| () => result.current, | |
| )! |
Standards
- Org-Guideline-Reuse code wherever possible and avoid redundant code by refactoring with utils and static method across application
- ISO-IEC-25010-Performance-Efficiency-Resource-Utilization
| React.useEffect(() => { | ||
| optionsRef.current = options | ||
| }) |
There was a problem hiding this comment.
State Synchronization Race
Options reference update lacks dependency array causing potential stale closure issues. Effect runs after every render potentially creating race conditions between option updates and state synchronization. Missing dependency array prevents React optimization and creates reliability gaps.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
Co-authored-by: refacto-visz[bot] <230139763+refacto-visz[bot]@users.noreply.github.com>
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: React Query State Management👍 Well Done
📁 Selected files for review (6)
🎯 Custom Instructions
📝 Additional Comments
|
| React.useEffect(() => { | ||
| optionsRef.current = options | ||
| }, [options]) |
There was a problem hiding this comment.
Stale Options Reference
Options reference updated after render cycle creates timing window where getSnapshot uses stale options. This race condition can cause inconsistent query state during rapid option changes, leading to UI displaying outdated data.
// Update ref synchronously to avoid stale options in getSnapshot
optionsRef.current = options
React.useEffect(() => {
// Effect kept for dependency tracking
}, [options])
Commitable Suggestion
| React.useEffect(() => { | |
| optionsRef.current = options | |
| }, [options]) | |
| // Update ref synchronously to avoid stale options in getSnapshot | |
| optionsRef.current = options | |
| React.useEffect(() => { | |
| // Effect kept for dependency tracking | |
| }, [options]) |
Standards
- ISO-IEC-25010-Reliability
- SRE-State-Consistency
- DbC-Temporal-Safety
| () => { | ||
| const nextResult = replaceEqualDeep( | ||
| result.current, | ||
| getResult(mutationCache, optionsRef.current), |
There was a problem hiding this comment.
Mutation State Staleness
Similar stale options reference pattern in mutation state hook creates data consistency issues. The optionsRef update lag during render cycles can cause mutation state to reflect outdated filter criteria.
() => {
// Ensure we're using latest options by updating ref synchronously
optionsRef.current = options
const nextResult = replaceEqualDeep(
result.current,
getResult(mutationCache, optionsRef.current),
Commitable Suggestion
| () => { | |
| const nextResult = replaceEqualDeep( | |
| result.current, | |
| getResult(mutationCache, optionsRef.current), | |
| () => { | |
| // Ensure we're using latest options by updating ref synchronously | |
| optionsRef.current = options | |
| const nextResult = replaceEqualDeep( | |
| result.current, | |
| getResult(mutationCache, optionsRef.current), |
Standards
- ISO-IEC-25010-Reliability
- SRE-State-Consistency
- DbC-Temporal-Safety
| React.useEffect(() => { | ||
| optionsRef.current = options | ||
| }, [options]) |
There was a problem hiding this comment.
Stale Data Race
Race condition between useEffect updating optionsRef and useSyncExternalStore's getSnapshot function. During render cycles, getSnapshot may access stale options before useEffect runs, potentially exposing incorrect query state data to components.
Standards
- CWE-362
- CWE-367
| () => { | ||
| const nextResult = replaceEqualDeep( | ||
| result.current, | ||
| getResult(mutationCache, optionsRef.current), |
There was a problem hiding this comment.
Mutation State Race
Same race condition pattern as useQueryState where optionsRef updates lag behind render cycles. Components may receive stale mutation state data during option changes, potentially affecting UI security decisions based on mutation status.
Standards
- CWE-362
- CWE-367
| React.useEffect(() => { | ||
| optionsRef.current = options | ||
| }, [options]) |
There was a problem hiding this comment.
Missing Effect Dependencies
useEffect lacks dependency array causing it to run on every render instead of only when options change. This violates React hooks correctness rules and creates unnecessary ref updates, potentially causing performance issues and unpredictable synchronization behavior.
Standards
- Algorithm-Correctness-Effect-Dependencies
- Business-Rule-React-Hooks-Correctness
| return React.useSyncExternalStore( | ||
| React.useCallback( | ||
| (onStoreChange) => | ||
| queryCache.subscribe(notifyManager.batchCalls(onStoreChange)), |
There was a problem hiding this comment.
Hook Logic Duplication
Identical useSyncExternalStore pattern duplicated between useQueryState and useMutationState creates maintenance overhead when state synchronization logic needs updates. Extract shared hook utility to eliminate code duplication and ensure consistent behavior across query management hooks.
Standards
- Clean-Code-DRY
- Refactoring-Extract-Function
- SOLID-SRP
4 participants
Summary by CodeRabbit