Clone use query state #12
Conversation
the value of the ref can get out of sync if we only write the ref in the subscribe function
Co-authored-by: refacto-visz[bot] <230139763+refacto-visz[bot]@users.noreply.github.com>
Summary of ChangesHello @DDShantanuBadmanji, 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 powerful 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
|
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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, which is then used to simplify the implementation of useIsFetching. This is a great abstraction that improves code reuse. The related changes in useMutationState and the tests are also solid improvements. I have one suggestion regarding the new useQueryState hook to enhance its type safety.
| export function useQueryState<TResult = QueryState>( | ||
| options: QueryStateOptions<TResult> = {}, | ||
| queryClient?: QueryClient, | ||
| ): Array<TResult> { |
There was a problem hiding this comment.
The current typings for useQueryState can lead to type-safety issues. If a consumer provides a generic type for TResult but omits the select function, the hook will return an array of QueryState objects that are incorrectly typed as TResult[]. This can lead to runtime errors that are not caught by the TypeScript compiler.
To improve type safety, I recommend using function overloads to ensure that if a custom TResult is specified, a select function returning that type is also required. This pattern is used elsewhere in React Query and would make this new API more robust.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-query/src/useQueryState.ts (1)
44-46: Nit: align optionsRef update semantics with useMutationStateTo avoid edge cases where options are mutated in place (identity doesn’t change), consider removing the dependency array so the ref updates every render, matching useMutationState.
- React.useEffect(() => { - optionsRef.current = options - }, [options]) + React.useEffect(() => { + optionsRef.current = options + })
📜 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 (2)
packages/query-core/src/queryClient.ts (1)
isMutating(107-109)packages/react-query/src/useMutationState.ts (1)
useIsMutating(15-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/useQueryState.ts (2)
packages/query-core/src/types.ts (2)
DefaultError(18-22)QueryKey(24-24)packages/react-query/src/index.ts (2)
useQueryState(43-43)useQueryClient(31-31)
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)
🪛 GitHub Actions: pr
packages/react-query/src/useQueryState.ts
[error] 39-39: src/useQueryState.ts(39,24): TS2554: Expected 1 arguments, but got 0.
🔇 Additional comments (7)
packages/react-query/src/index.ts (1)
43-43: Expose useQueryState: LGTMNew public export looks correct and consistent with internal usage.
Please add a brief entry to changelog/docs for the new public hook.
packages/react-query/src/__tests__/useIsFetching.test.tsx (1)
208-209: Inline isFetching with custom client: LGTMCorrectly scopes isFetching and uses the provided queryClient.
packages/react-query/src/__tests__/useMutationState.test.tsx (1)
69-77: Memoizing test component to isolate renders: LGTMUsing React.memo avoids unrelated re-renders from sibling hooks, stabilizing expected sequences.
Also applies to: 110-120
packages/react-query/src/useMutationState.ts (1)
67-67: Batched subscription + structural sharing: LGTM
- notifyManager.batchCalls in subscribe reduces redundant renders.
- replaceEqualDeep + ref memoization avoids useless updates.
Also applies to: 70-81
packages/react-query/src/useQueryState.ts (1)
48-66: Approve subscription and snapshot logic
- Batching via notifyManager.batchCalls(onStoreChange) is correct
- replaceEqualDeep with ref memoization ensures stable identities and minimal re-renders
packages/react-query/src/useIsFetching.ts (2)
2-2: LGTM!The import of
useQueryStateis correct and aligns with the refactored implementation.
9-12: LGTM! Clean refactor touseQueryState.The implementation correctly delegates to
useQueryStateand returns the count of fetching queries. The spread operator on line 10 ensuresfetchStatusis always set to'fetching', which is the expected behavior for this hook.
| const result = React.useRef<Array<TResult>>() | ||
| if (!result.current) { | ||
| result.current = getResult(queryCache, options) | ||
| } |
There was a problem hiding this comment.
Build break: useRef called without initial value (TS2554)
TS error matches pipeline: Expected 1 arguments, but got 0. Initialize the ref to null (or undefined) and keep the first‑render assignment.
Apply this diff:
- const result = React.useRef<Array<TResult>>()
+ const result = React.useRef<Array<TResult> | null>(null)
if (!result.current) {
result.current = getResult(queryCache, options)
}This preserves runtime behavior and fixes typing by using non-null assertion on the hook return.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = React.useRef<Array<TResult>>() | |
| if (!result.current) { | |
| result.current = getResult(queryCache, options) | |
| } | |
| const result = React.useRef<Array<TResult> | null>(null) | |
| if (!result.current) { | |
| result.current = getResult(queryCache, options) | |
| } |
🧰 Tools
🪛 GitHub Actions: pr
[error] 39-39: src/useQueryState.ts(39,24): TS2554: Expected 1 arguments, but got 0.
🤖 Prompt for AI Agents
In packages/react-query/src/useQueryState.ts around lines 39 to 42, useRef is
called with no initial value which breaks TS (expects 1 arg); change the ref to
be initialized to null (e.g., useRef<Array<TResult> | null>(null)) so typing is
satisfied, keep the existing first-render assignment result.current =
getResult(...), and use a non-null assertion when reading result.current later
(result.current!) to preserve runtime behavior and satisfy TypeScript.
4 participants
Summary by CodeRabbit