Open
Conversation
|
github-actions
bot
added
documentation
Contributor
📝 WalkthroughWalkthroughThis change introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 4206f79
☁️ Nx Cloud last updated this comment at |
Contributor
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/vue-query/src/__tests__/queryClient.test.ts (1)
23-23: This change appears unrelated to the environmentManager feature.The type improvement for the filter predicate is a good change, but it doesn't seem related to the
environmentManagerfeature being introduced in this PR. Consider whether this should be in a separate PR for cleaner history, or note in the PR description that this is an incidental type improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-query/src/__tests__/queryClient.test.ts` at line 23, The change to the filter predicate type guard in queryClient.test.ts (the line casting to QueryClientPrototypeMethod) is unrelated to the environmentManager feature; either revert that type-only change from this branch and leave the environmentManager PR focused, or extract the type improvement into its own commit/branch and open a separate PR; update the current PR description to mention the incidental type tweak if you intend to keep it here. Ensure you reference the test file's filter predicate that narrows to QueryClientPrototypeMethod when making the change.packages/query-core/src/__tests__/environmentManager.test.tsx (1)
21-24: Consider consolidating or differentiating this test.This test ("should allow overriding isServer with a function") is essentially identical to the first part of the test on lines 13-15. Both tests call
setIsServer(() => true)and assert the result. The test name suggests testing "with a function" but that's already what the API requires—setIsServeronly accepts a function per the typeIsServerValue = () => boolean.Consider either removing this test as it's redundant, or differentiating it by testing a more dynamic function (e.g., one that returns different values based on some condition).
Example of a more meaningful test
test('should allow overriding isServer with a function', () => { - environmentManager.setIsServer(() => true) - expect(environmentManager.isServer()).toBe(true) + let value = true + environmentManager.setIsServer(() => value) + expect(environmentManager.isServer()).toBe(true) + + value = false + expect(environmentManager.isServer()).toBe(false) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/environmentManager.test.tsx` around lines 21 - 24, The test is redundant because environmentManager.setIsServer(() => true) is already asserted earlier; either remove this duplicate test or change it to exercise a dynamic function: replace the static () => true with a function that flips or returns different values based on external state, then assert environmentManager.isServer() reflects that changing state (use environmentManager.setIsServer and environmentManager.isServer to locate code to modify). Ensure the new test verifies the dynamic behavior across state changes rather than repeating the same static assertion.packages/query-core/src/onlineManager.ts (1)
17-17: Inconsistent with environmentManager pattern used elsewhere.This change uses
typeof window !== 'undefined'directly instead ofenvironmentManager.isServer(). While this works for detecting the browser environment, it creates an inconsistency with the rest of the codebase migration:
- If a user calls
environmentManager.setIsServer(() => true)to simulate server-side behavior (e.g., for testing), theOnlineManagerwill still register window event listeners becausewindowexists.- Other files (
retryer.ts,queryObserver.ts,removable.ts) useenvironmentManager.isServer()consistently.If the intent is to check specifically for
window.addEventListeneravailability (as the comment suggests for React Native compatibility), the current approach is fine. However, if the goal is to respect theenvironmentManageroverride, consider:Option: Use environmentManager for consistency
- if (typeof window !== 'undefined' && window.addEventListener) { + if (!environmentManager.isServer() && typeof window !== 'undefined' && window.addEventListener) {This would require adding the import:
+import { environmentManager } from './environmentManager'Please clarify the intended behavior: should
OnlineManagerrespectenvironmentManager.setIsServer()overrides, or is checking forwindowexistence the correct approach here?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/onlineManager.ts` at line 17, Replace the direct "typeof window !== 'undefined' && window.addEventListener" check in OnlineManager with the environmentManager-based guard so the module respects environmentManager.setIsServer() overrides; import environmentManager and change the condition to "!environmentManager.isServer() && typeof window !== 'undefined' && window.addEventListener" (so you still guard for environments like React Native while honoring test/server overrides) and update any surrounding comment to reflect the combined intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/query-core/src/__tests__/focusManager.test.tsx`:
- Around line 63-68: The test currently calls subscribe twice (once via
expect(subscribe).not.toThrow() which invokes subscribe, and again on the next
line) but only calls unsubscribe once, leaving a dangling listener; change the
test so both subscriptions are unsubscribed: capture the first unsubscribe by
invoking subscribe inside the expect (e.g. let firstUnsub; expect(() => {
firstUnsub = subscribe(); }).not.toThrow()), then call const secondUnsub =
subscribe() and finally call firstUnsub() and secondUnsub(); reference the
subscribe constant and focusManager.subscribe to locate the code and ensure
removeEventListener gets exercised.
In `@packages/query-core/src/__tests__/onlineManager.test.tsx`:
- Around line 72-77: The test currently calls onlineManager.subscribe twice
(once via expect(subscribe).not.toThrow() and again when assigning unsubscribe)
which leaves the first listener active and masks cleanup; fix by calling
subscribe only once: replace the expect + second call with a single call to
onlineManager.subscribe(() => undefined) assigned to unsubscribe, then assert
the returned unsubscribe is a function (e.g.,
expect(unsubscribe).toBeInstanceOf(Function)) and finally call unsubscribe() so
onUnsubscribe() can be exercised; refer to onlineManager.subscribe, unsubscribe,
and onUnsubscribe to locate the code to change.
---
Nitpick comments:
In `@packages/query-core/src/__tests__/environmentManager.test.tsx`:
- Around line 21-24: The test is redundant because
environmentManager.setIsServer(() => true) is already asserted earlier; either
remove this duplicate test or change it to exercise a dynamic function: replace
the static () => true with a function that flips or returns different values
based on external state, then assert environmentManager.isServer() reflects that
changing state (use environmentManager.setIsServer and
environmentManager.isServer to locate code to modify). Ensure the new test
verifies the dynamic behavior across state changes rather than repeating the
same static assertion.
In `@packages/query-core/src/onlineManager.ts`:
- Line 17: Replace the direct "typeof window !== 'undefined' &&
window.addEventListener" check in OnlineManager with the
environmentManager-based guard so the module respects
environmentManager.setIsServer() overrides; import environmentManager and change
the condition to "!environmentManager.isServer() && typeof window !==
'undefined' && window.addEventListener" (so you still guard for environments
like React Native while honoring test/server overrides) and update any
surrounding comment to reflect the combined intent.
In `@packages/vue-query/src/__tests__/queryClient.test.ts`:
- Line 23: The change to the filter predicate type guard in queryClient.test.ts
(the line casting to QueryClientPrototypeMethod) is unrelated to the
environmentManager feature; either revert that type-only change from this branch
and leave the environmentManager PR focused, or extract the type improvement
into its own commit/branch and open a separate PR; update the current PR
description to mention the incidental type tweak if you intend to keep it here.
Ensure you reference the test file's filter predicate that narrows to
QueryClientPrototypeMethod when making the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/config.jsondocs/reference/environmentManager.mdpackages/preact-query/src/__tests__/utils.tsxpackages/preact-query/src/useBaseQuery.tspackages/query-core/src/__tests__/environmentManager.test.tsxpackages/query-core/src/__tests__/focusManager.test.tsxpackages/query-core/src/__tests__/onlineManager.test.tsxpackages/query-core/src/__tests__/utils.tspackages/query-core/src/environmentManager.tspackages/query-core/src/focusManager.tspackages/query-core/src/index.tspackages/query-core/src/onlineManager.tspackages/query-core/src/queryObserver.tspackages/query-core/src/removable.tspackages/query-core/src/retryer.tspackages/query-core/src/utils.tspackages/react-query/src/__tests__/utils.tsxpackages/react-query/src/useBaseQuery.tspackages/vue-query/src/__tests__/queryClient.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/query-core/src/__tests__/focusManager.test.tsx (1)
57-60: Usetry/finallyfor spy restoration to prevent cross-test leakage.If an assertion fails before Line 76, the mocked
globalThis.windowgetter remains active and can pollute following tests.Suggested hardening
test('cleanup (removeEventListener) should not be called if window is not defined', () => { const windowSpy = vi.spyOn(globalThis, 'window', 'get') windowSpy.mockImplementation( () => undefined as unknown as Window & typeof globalThis, ) const removeEventListenerSpy = vi.spyOn(globalThis, 'removeEventListener') - const subscribe = () => focusManager.subscribe(() => undefined) - let firstUnsubscribe: (() => void) | undefined - - expect(() => { - firstUnsubscribe = subscribe() - }).not.toThrow() - const secondUnsubscribe = subscribe() - - firstUnsubscribe?.() - secondUnsubscribe() - - expect(removeEventListenerSpy).not.toHaveBeenCalled() - - windowSpy.mockRestore() + try { + const subscribe = () => focusManager.subscribe(() => undefined) + let firstUnsubscribe: (() => void) | undefined + + expect(() => { + firstUnsubscribe = subscribe() + }).not.toThrow() + const secondUnsubscribe = subscribe() + + firstUnsubscribe?.() + secondUnsubscribe() + + expect(removeEventListenerSpy).not.toHaveBeenCalled() + } finally { + removeEventListenerSpy.mockRestore() + windowSpy.mockRestore() + } })Also applies to: 76-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/focusManager.test.tsx` around lines 57 - 60, Wrap the test code that mocks globalThis.window (the windowSpy created via vi.spyOn(globalThis, 'window', 'get')) in a try/finally block so you always restore the spy; move windowSpy.mockImplementation(...) into the try and call windowSpy.mockRestore() in the finally to ensure the mocked getter is removed even if assertions fail (apply the same pattern for the other instance around line 76).packages/query-core/src/__tests__/onlineManager.test.tsx (1)
66-69: Guard spy restoration withfinallyfor deterministic test cleanup.If anything fails before Line 79, the mocked
windowgetter may leak into later tests.Suggested hardening
test('cleanup (removeEventListener) should not be called if window is not defined', () => { const windowSpy = vi.spyOn(globalThis, 'window', 'get') windowSpy.mockImplementation( () => undefined as unknown as Window & typeof globalThis, ) const removeEventListenerSpy = vi.spyOn(globalThis, 'removeEventListener') - const unsubscribe = onlineManager.subscribe(() => undefined) - expect(unsubscribe).toBeInstanceOf(Function) - - unsubscribe() - - expect(removeEventListenerSpy).not.toHaveBeenCalled() - - windowSpy.mockRestore() + try { + const unsubscribe = onlineManager.subscribe(() => undefined) + expect(unsubscribe).toBeInstanceOf(Function) + unsubscribe() + expect(removeEventListenerSpy).not.toHaveBeenCalled() + } finally { + removeEventListenerSpy.mockRestore() + windowSpy.mockRestore() + } })Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/onlineManager.test.tsx` around lines 66 - 69, The test creates a spy via vi.spyOn(globalThis, 'window', 'get') assigned to windowSpy and mocks it with windowSpy.mockImplementation(...); ensure the spy is always restored to avoid leaking into other tests by wrapping the test body so that windowSpy.mockRestore() is called in a finally block (or register windowSpy.mockRestore() in an afterEach/teardown hook) — update the test that uses windowSpy, mockImplementation, and windowSpy.mockRestore() to guarantee restoration even if assertions throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/query-core/src/__tests__/focusManager.test.tsx`:
- Around line 57-60: Wrap the test code that mocks globalThis.window (the
windowSpy created via vi.spyOn(globalThis, 'window', 'get')) in a try/finally
block so you always restore the spy; move windowSpy.mockImplementation(...) into
the try and call windowSpy.mockRestore() in the finally to ensure the mocked
getter is removed even if assertions fail (apply the same pattern for the other
instance around line 76).
In `@packages/query-core/src/__tests__/onlineManager.test.tsx`:
- Around line 66-69: The test creates a spy via vi.spyOn(globalThis, 'window',
'get') assigned to windowSpy and mocks it with
windowSpy.mockImplementation(...); ensure the spy is always restored to avoid
leaking into other tests by wrapping the test body so that
windowSpy.mockRestore() is called in a finally block (or register
windowSpy.mockRestore() in an afterEach/teardown hook) — update the test that
uses windowSpy, mockImplementation, and windowSpy.mockRestore() to guarantee
restoration even if assertions throw.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/query-core/src/__tests__/environmentManager.test.tsxpackages/query-core/src/__tests__/focusManager.test.tsxpackages/query-core/src/__tests__/onlineManager.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/query-core/src/tests/environmentManager.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
alternative fix for #10139
Summary by CodeRabbit
New Features
environmentManageras a new public API for runtime server-detection management, allowing queries to check and override server environment status.isServer()andsetIsServer()methods for flexible server-state control.Documentation
environmentManagerwith usage examples and configuration guidance.Deprecations
isServerexport as deprecated in favor ofenvironmentManager.isServer().