Improve subscription performance by 10-20 times #9817
Conversation
🦋 Changeset detectedLatest commit: 7dd1e63 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@MrFlashAccount has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughIntroduces a timestamp-based GC system: new GCManager centralizes scheduling; Removable switched from timers to gcTime/gcMarkedAt; QueryClient, Query, and Mutation integrate GCManager; tests updated to assert timestamp-based GC marks and adjust minor timings. Changes
Sequence Diagram(s)sequenceDiagram
participant R as Removable (Query/Mutation)
participant GM as GCManager
participant TM as Timer/TimeoutManager
participant Micro as MicrotaskQueue
R->>R: markForGc() (set gcMarkedAt)
R->>GM: trackEligibleItem(this)
GM->>Micro: enqueue scheduleScan (if not already)
Micro->>GM: run #scheduleScan()
GM->>GM: compute min(getGcAtTimestamp - now)
GM->>TM: setTimeout(#performScan, delay)
TM-->>GM: timeout fires
GM->>GM: #performScan()
loop each eligible item
GM->>R: isEligibleForGc()
alt eligible
R-->>GM: true
GM->>R: optionalRemove()
R-->>GM: removed
GM->>GM: untrack item
else not eligible
R-->>GM: false
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
MrFlashAccount
changed the title
MrFlashAccount
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/solid-query/src/__tests__/useQuery.test.tsx (1)
3953-3966: Make GC timestamp assertion deterministic without fake timersvi.setSystemTime usually requires fake timers; here timers stay real, so Date.now used by markForGc won’t follow your set time. Stub Date.now instead and drop useRealTimers.
Apply:
- vi.setSystemTime(new Date(1970, 0, 1, 0, 0, 0, 0)) + const fixedNow = new Date(1970, 0, 1, 0, 0, 0, 0).getTime() + const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(fixedNow) rendered.unmount() expect(query!.gcEligibleAt).not.toBeNull() expect(query!.gcEligibleAt).toBe( new Date(1970, 0, 1, 0, 0, 0, gcTime).getTime(), ) - - vi.useRealTimers() + nowSpy.mockRestore() await vi.waitFor(() => { return queryClient.getQueryCache().find({ queryKey: key }) === undefined })packages/query-core/src/mutationCache.ts (1)
252-259: Avoid mutating the Set while iterating itoptionalRemove() can delete from #mutations; iterating the same Set risks skipping elements. Iterate a snapshot.
- performGarbageCollection(): void { - for (const mutation of this.#mutations) { + performGarbageCollection(): void { + for (const mutation of Array.from(this.#mutations)) { if (mutation.isEligibleForGc()) { mutation.optionalRemove() } } }packages/query-core/src/gcManager.ts (1)
134-147: Operational refinements to reduce idle overhead
- Auto-stop scanning when no caches registered; restart on first register.
- Optional: adapt scan interval based on cache size/load.
startScanning(): void { if (this.#isScanning || isServer) { return } @@ this.#intervalId = timeoutManager.setInterval(() => { - if (!this.#isPaused) { + if (!this.#isPaused) { this.#performScan() + if (this.#caches.size === 0) { + this.stopScanning() + } } }, this.#scanInterval) }Also applies to: 199-201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/query-core/src/__tests__/mutationObserver.test.tsx(2 hunks)packages/query-core/src/gcManager.ts(1 hunks)packages/query-core/src/index.ts(2 hunks)packages/query-core/src/mutation.ts(7 hunks)packages/query-core/src/mutationCache.ts(3 hunks)packages/query-core/src/query.ts(10 hunks)packages/query-core/src/queryCache.ts(3 hunks)packages/query-core/src/queryClient.ts(5 hunks)packages/query-core/src/removable.ts(2 hunks)packages/query-core/src/types.ts(2 hunks)packages/react-query/src/__tests__/useQuery.test.tsx(2 hunks)packages/react-query/src/__tests__/useSuspenseQueries.test.tsx(1 hunks)packages/solid-query/src/__tests__/useQuery.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
packages/solid-query/src/__tests__/useQuery.test.tsx (1)
packages/solid-query/src/useQuery.ts (1)
useQuery(36-50)
packages/query-core/src/removable.ts (1)
packages/query-core/src/utils.ts (1)
isValidTimeout(93-95)
packages/query-core/src/mutationCache.ts (3)
packages/vue-query/src/mutationCache.ts (1)
MutationCache(10-25)packages/query-core/src/subscribable.ts (1)
Subscribable(1-30)packages/query-core/src/gcManager.ts (1)
GarbageCollectable(8-13)
packages/query-core/src/queryCache.ts (3)
packages/query-core/src/subscribable.ts (1)
Subscribable(1-30)packages/query-core/src/gcManager.ts (1)
GarbageCollectable(8-13)packages/query-core/src/queryObserver.ts (1)
query(704-721)
packages/query-core/src/mutation.ts (2)
packages/query-core/src/gcManager.ts (1)
GCManager(60-247)packages/query-core/src/index.ts (1)
GCManager(17-17)
packages/react-query/src/__tests__/useQuery.test.tsx (2)
packages/query-core/src/queryObserver.ts (1)
query(704-721)packages/react-query/src/__tests__/utils.tsx (1)
renderWithClient(9-23)
packages/query-core/src/gcManager.ts (2)
packages/query-core/src/index.ts (5)
GCManagerConfig(53-53)GCManager(17-17)ManagedTimerId(25-25)isServer(31-31)timeoutManager(24-24)packages/query-core/src/timeoutManager.ts (1)
systemSetTimeoutZero(133-135)
packages/query-core/src/types.ts (2)
packages/query-core/src/gcManager.ts (1)
GCManager(60-247)packages/query-core/src/index.ts (1)
GCManager(17-17)
packages/query-core/src/query.ts (2)
packages/query-core/src/gcManager.ts (1)
GCManager(60-247)packages/query-core/src/index.ts (1)
GCManager(17-17)
packages/query-core/src/queryClient.ts (2)
packages/query-core/src/gcManager.ts (1)
GCManager(60-247)packages/query-core/src/index.ts (1)
GCManager(17-17)
🔇 Additional comments (16)
packages/query-core/src/__tests__/mutationObserver.test.tsx (1)
61-61: LGTM! Timer adjustments align with new GC timing model.The doubled wait time (10ms → 20ms) correctly accounts for the new GC timing behavior:
gcTime(10ms) +scanInterval(10ms default). Under the centralized GC system, items become eligible atgcTimebut are removed during the next periodic scan.Also applies to: 83-83
packages/react-query/src/__tests__/useSuspenseQueries.test.tsx (1)
651-656: LGTM! Test correctly updated for new GC timing and verification.Two appropriate changes:
- Timer adjustment: 1000ms → 1010ms accounts for
gcTime(1000ms) +scanInterval(10ms).- Verification method: Checking
cache.find()directly is more accurate for GC verification thangetQueryData, as it confirms cache-level removal rather than just data absence.packages/query-core/src/removable.ts (5)
3-21: LGTM! Clear documentation and appropriate field types.The JSDoc clearly explains the shift from individual timeouts to timestamp-based GC eligibility. The
gcEligibleAt: number | nulltype correctly models the tri-state: not marked (null), marked with timestamp (number), or Infinity gcTime (remains null).
26-47: LGTM! Correct GC mark implementation.The
markForGc()method correctly handles all gcTime scenarios:
- Valid finite gcTime: sets future timestamp
Infinity: setsnull(never eligible)gcTime: 0: setsDate.now()(immediately eligible, triggers immediate scan per PR description)
56-76: LGTM! Eligibility check implementation is correct.
isEligibleForGc()properly determines eligibility by comparing current time against the marked timestamp. The null check ensures unmarked items and those withgcTime: Infinityare never collected.
83-105: LGTM! Time calculation and update logic are sound.
getTimeUntilGc()correctly calculates remaining time with proper null handling and negative-to-zero clamping.updateGcTime()appropriately usesMath.max()to honor the "longest gcTime wins" rule documented earlier, with sensible defaults (5min client / Infinity server).
107-116: LGTM! Appropriate abstraction and visibility change.Making
optionalRemove()public and abstract is correct: the GCManager and cache implementations need to call this method from outside the class hierarchy, and subclasses (Query, Mutation) provide the specific removal logic.packages/query-core/src/types.ts (1)
6-6: LGTM! Clean API extension with backward compatibility.The optional
gcManagerfield inQueryClientConfigenables custom GCManager injection while maintaining full backward compatibility. Type-only import is appropriate.Also applies to: 1355-1355
packages/query-core/src/index.ts (1)
17-17: LGTM! Appropriate public API exposure.Exporting
GCManagerandGCManagerConfigenables external configuration and custom GC management while following the package's established export patterns. No breaking changes introduced.Also applies to: 53-53
packages/query-core/src/queryClient.ts (3)
15-15: LGTM! Consistent initialization pattern.GCManager initialization follows the established pattern for
queryCacheandmutationCache, using the config value or creating a default instance. The private field encapsulation is appropriate.Also applies to: 63-63, 76-76
82-84: LGTM! Proper cache registration and scan initialization.Both caches are registered with GCManager and scanning starts automatically. The automatic start is safe because
GCManager.startScanning()includes anisServerguard that prevents scanning on SSR.
464-466: LGTM! Consistent accessor pattern.The
getGcManager()method follows the established pattern ofgetQueryCache()andgetMutationCache(), enabling external access to GC controls for advanced use cases and testing.packages/query-core/src/queryCache.ts (3)
16-16: LGTM! Proper interface implementation.QueryCache correctly implements the
GarbageCollectableinterface, enabling integration with the centralized GCManager system. Type-only import is appropriate.Also applies to: 93-96
235-242: LGTM! Correct GC implementation.
performGarbageCollection()properly iterates eligible queries and delegates removal toquery.optionalRemove(), which handles the actual removal and notifications through the existingremove()path.
247-250: LGTM! Proper cleanup implementation.The
destroy()method correctly delegates toclear(), which handles batch notification and resource cleanup for all queries. This provides a clean shutdown path for the cache.packages/query-core/src/query.ts (1)
374-379: GC integration verified—all requirements confirmedQueryClient correctly registers both QueryCache and MutationCache with GCManager and immediately starts scanning on initialization (queryClient.ts:82–84). Query and Mutation classes properly access GCManager through
getGcManager(). The marking logic and immediate scans for gcTime=0 are consistent with the registration flow.
MrFlashAccount
changed the title
MrFlashAccount
changed the title
|
Random observer here! 👋 I'm not seeing where the GC ever stops running. This would be running every 10ms for the life of the application, even after everything has been GC'ed or is settled. That might keep the page active and prevent some backgrounding behaviors for inactive pages/tabs. There should probably be some settling behavior or backoff for the sweeps. |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 3m 38s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 19s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-27 19:29:33 UTC
|
@timdorr Hi! That's a good observation. Thank you. Initially, implementation was to start the GC manager on mount and stop on unmount. But it works only if you mount the queryClient using the QueryClientProvider. I think you concern makes sense. Will try to find out a solution for this. One of the solutions could be to store queries to collect in a map, and start the timer only when |
…ss caches - Improved performance by 5-10 times - Tests left intact (no breaking changes expected)
- Added `scheduleImmediateScan` method to `GCManager` for immediate garbage collection scans. - Integrated `GCManager` into `Mutation` and `Query` classes to trigger immediate scans when conditions are met. - Updated tests to verify new garbage collection behavior.
MrFlashAccount
force-pushed
the
improve-gc-performance
branch
from
e1bc8d3 to
c514ec8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/query-core/src/mutation.ts (2)
150-153: removeObserver: GC mark only when safe — matches guidanceAddresses the earlier concern about scheduling scans while pending.
386-389: Dispatch-time GC mark without early‑return is correctKeeps notifications consistent while still enabling GC when safe.
🧹 Nitpick comments (7)
packages/query-core/src/__tests__/query.test.tsx (1)
561-562: Double 0ms advances to flush GC schedulingGood call adding two 0ms advances to flush the microtask that schedules the timeout and the timeout itself. Consider a tiny helper (e.g., flushGcScheduling) to avoid repetition across tests.
packages/query-core/src/queryClient.ts (1)
64-82: GCManager integration and SSR gating look correct
- Initializing GCManager with forceDisable: isServer prevents clientless environments from scheduling timers.
- Public getGcManager accessor and clear() cleanup are appropriate.
Optional: allow injecting a custom GCManager via QueryClientConfig for advanced testing or alternative strategies.
Also applies to: 461-463, 656-657
packages/query-core/src/__tests__/gcManager.test.tsx (1)
49-56: Harden scheduling assertions to avoid flakinessGC scheduling uses a microtask to schedule the timeout. In places you assert isScanning() right after unsubscribe, use a helper to flush both microtask and the potential 0ms timeout:
+async function flushGcScheduling() { + await Promise.resolve(); // flush microtasks + await vi.advanceTimersByTimeAsync(0); // run any 0ms timers +}Then replace single 0ms advances like:
- await vi.advanceTimersByTimeAsync(0) + await flushGcScheduling()This reduces flakes across environments.
Also applies to: 71-76, 101-124, 160-169, 198-207, 255-265, 286-296
packages/query-core/src/removable.ts (1)
40-48: Mark-for-GC logic is correct; consider unified time sourceUsing Date.now() is fine, but if getTimeUntilGc() relies on a different clock, consider centralizing through a shared time utility to avoid drift in tests or non-DOM runtimes.
packages/query-core/src/gcManager.ts (3)
49-95: On‑demand scheduling fixes “always‑on interval” concernMicrotask + min(gcAt) timeout = no continuous timer; nothing runs when no eligibles. This addresses background tab/idle concerns.
Consider documenting this deviation from “global interval” in the PR description/docs so expectations match behavior.
145-163: Minor: cancel pending microtask when set becomes empty and no active timerIf getEligibleItemCount() hits 0 while only a microtask is queued (#isScheduledScan true, #isScanning false), you can proactively flip #isScheduledScan=false here to avoid a no‑op microtask.
untrackEligibleItem(item: Removable): void { if (this.#forceDisable) { return } @@ - if (this.isScanning()) { - if (this.getEligibleItemCount() === 0) { - this.stopScanning() - } else { - this.#scheduleScan() - } - } + const count = this.getEligibleItemCount() + if (this.isScanning()) { + if (count === 0) this.stopScanning() + else this.#scheduleScan() + } else if (count === 0) { + // No timer yet, but a microtask may be queued + this.#isScheduledScan = false + } }
172-190: performScan(): safe iteration; minor clarity nitDeleting from a Set during iteration is fine. If you prefer defensive clarity, snapshot the set first to avoid any future semantic surprises.
- for (const item of this.#eligibleItems) { + for (const item of Array.from(this.#eligibleItems)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/query-core/src/__tests__/gcManager.test.tsx(1 hunks)packages/query-core/src/__tests__/mutationCache.test.tsx(1 hunks)packages/query-core/src/__tests__/query.test.tsx(1 hunks)packages/query-core/src/gcManager.ts(1 hunks)packages/query-core/src/index.ts(1 hunks)packages/query-core/src/mutation.ts(6 hunks)packages/query-core/src/query.ts(8 hunks)packages/query-core/src/queryClient.ts(7 hunks)packages/query-core/src/removable.ts(2 hunks)packages/react-query/src/__tests__/useMutation.test.tsx(1 hunks)packages/react-query/src/__tests__/useQuery.test.tsx(2 hunks)packages/react-query/src/__tests__/useSuspenseQueries.test.tsx(1 hunks)packages/solid-query/src/__tests__/useMutation.test.tsx(1 hunks)packages/solid-query/src/__tests__/useQuery.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/solid-query/src/tests/useMutation.test.tsx
- packages/query-core/src/tests/mutationCache.test.tsx
- packages/query-core/src/index.ts
- packages/react-query/src/tests/useSuspenseQueries.test.tsx
- packages/react-query/src/tests/useMutation.test.tsx
- packages/solid-query/src/tests/useQuery.test.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
packages/query-core/src/__tests__/gcManager.test.tsx (2)
packages/query-core/src/queryClient.ts (1)
QueryClient(63-658)packages/query-core/src/__tests__/utils.ts (1)
executeMutation(13-22)
packages/query-core/src/queryClient.ts (3)
packages/query-core/src/gcManager.ts (1)
GCManager(38-196)packages/query-core/src/index.ts (2)
GCManager(17-17)isServer(31-31)packages/query-core/src/utils.ts (1)
isServer(78-78)
packages/react-query/src/__tests__/useQuery.test.tsx (1)
packages/query-core/src/gcManager.ts (1)
item(172-190)
packages/query-core/src/removable.ts (3)
packages/query-core/src/utils.ts (1)
isValidTimeout(93-95)packages/query-core/src/gcManager.ts (1)
GCManager(38-196)packages/query-core/src/index.ts (1)
GCManager(17-17)
packages/query-core/src/gcManager.ts (1)
packages/query-core/src/index.ts (3)
GCManager(17-17)ManagedTimerId(25-25)timeoutManager(24-24)
packages/query-core/src/mutation.ts (2)
packages/query-core/src/gcManager.ts (1)
GCManager(38-196)packages/query-core/src/mutationObserver.ts (1)
MutationObserver(23-211)
packages/query-core/src/query.ts (2)
packages/query-core/src/gcManager.ts (1)
GCManager(38-196)packages/query-core/src/types.ts (1)
QueryOptions(225-278)
🔇 Additional comments (18)
packages/query-core/src/__tests__/query.test.tsx (2)
4024-4028: Infinity gcTime should not set gc markAssertion that gcMarkedAt remains null after unmount when gcTime is Infinity is correct and safeguards against accidental tracking.
4032-4063: Timestamp-based GC verification looks solidSetting system time, verifying gcMarkedAt at unmount, then advancing by gcTime to assert removal aligns with the new GC model. No changes needed.
packages/react-query/src/__tests__/useQuery.test.tsx (1)
4024-4063: React tests align with new GC semanticsThe added checks for gcMarkedAt (Infinity vs finite) and the timestamp-based removal window are correct and stable under fake timers.
packages/query-core/src/removable.ts (1)
104-110: “Longest gcTime wins” defaulting is correctDefaults (Infinity on server, 5 minutes on client) and max-merge semantics are spot on.
packages/query-core/src/query.ts (7)
207-209: Protected getGcManager accessor looks goodProvides the hook Removable needs without widening surface area.
231-239: optionalRemove(): correct boolean contract and unmarking
- Removes when safe, returns boolean.
- Else clears GC mark to untrack in manager.
LGTM.
241-243: isSafeToRemove(): criteria make senseNo observers and idle fetchStatus matches previous semantics.
366-366: addObserver: clearGcMark() is correctPrevents premature collection while observed.
387-389: removeObserver: gate GC mark on safetyOnly mark when safe to remove; avoids useless scans during in‑flight fetches.
622-626: Fetch finally: re-mark for GC when safeEnsures unused queries after settle are eligible without extra timers.
184-191: Verified: GC marking semantics are correctBoth concerns have been verified:
- Query is added to QueryCache (via
queryCache.build()→this.add(query)) before the constructor completes and callsmarkForGc(), preventing any early collection race.markForGc()is properly a no-op forgcTime=Infinity:isValidTimeout()explicitly excludes Infinity, causingmarkForGc()to callclearGcMark()instead of tracking the item with the GCManager.packages/query-core/src/mutation.ts (5)
113-114: Ctor-level GC mark: verify Infinity handlingSame note as Query: ensure markForGc skips tracking for gcTime=Infinity.
Use the same script shared in Query comment to inspect Removable.
128-131: Protected getGcManager accessor looks goodKeeps GC integration encapsulated.
137-138: addObserver: clearGcMark() is correctPrevents GC while the mutation is observed.
161-164: isSafeToRemove(): pending check is essentialEnsures we don’t collect in‑flight mutations.
165-176: optionalRemove(): smart deferral for pending, removes otherwiseRe‑marking when pending pushes gcAt forward and avoids tight 0ms rescan loops; removing when idle returns true for manager cleanup.
packages/query-core/src/gcManager.ts (2)
96-111: stopScanning(): state reset is correctClears timer and cancels any pending scan via flags.
198-204: getTimeUntilGc(): simple and correctInfinity handling defers scheduling. Ensure Removable.getGcAtTimestamp() returns null for unmarked or Infinity gcTime.
Use the earlier script to validate Removable’s return contract.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/query-core/src/__tests__/gcManager.test.tsx (1)
206-207: Consider adding final state assertions for completeness.The test correctly validates that resubscribing untracts the query (line 202). For completeness and explicit test self-containment, consider adding final assertions after the last unsubscribe:
unsubscribe2() +await vi.advanceTimersByTimeAsync(0) +expect(gcManager.isScanning()).toBe(true) +expect(gcManager.getEligibleItemCount()).toBe(1)This would make the test's final state explicit rather than relying solely on afterEach cleanup.
packages/solid-query/src/__tests__/useQuery.test.tsx (2)
3907-3907: Tighten the Infinity case: explicit existence assert; drop fake timers (optional)
- Add an explicit assertion for the query object to avoid masking failures behind non-null assertion.
- Fake timers aren’t needed here (no time-based behavior exercised). Optional removal keeps the test simpler.
- vi.useFakeTimers() + // No timers needed for this test @@ - const item = queryClient.getQueryCache().find({ queryKey: key }) - expect(item!.gcMarkedAt).toBeNull() + const item = queryClient.getQueryCache().find({ queryKey: key }) + expect(item).toBeDefined() + expect(item!.gcMarkedAt).toBeNull()Also applies to: 3929-3931
3934-3937: Make eviction timing resilient to GC scan intervalThe GC runs on a periodic sweep. Advance a bit beyond gcTime to avoid edge flakiness if scanInterval changes.
- await vi.advanceTimersByTimeAsync(gcTime) + // Advance slightly beyond gcTime to ensure at least one sweep runs + await vi.advanceTimersByTimeAsync(gcTime + 50)Also applies to: 3943-3944, 3956-3971
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/query-core/src/__tests__/gcManager.test.tsx(1 hunks)packages/query-core/src/__tests__/query.test.tsx(1 hunks)packages/solid-query/src/__tests__/useQuery.test.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/query-core/src/tests/query.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/query-core/src/__tests__/gcManager.test.tsx (2)
packages/query-core/src/queryClient.ts (1)
QueryClient(63-658)packages/query-core/src/__tests__/utils.ts (1)
executeMutation(13-22)
packages/solid-query/src/__tests__/useQuery.test.tsx (2)
packages/query-core/src/gcManager.ts (1)
item(172-190)packages/query-core/src/queryObserver.ts (1)
query(704-721)
🔇 Additional comments (5)
packages/query-core/src/__tests__/gcManager.test.tsx (4)
1-18: LGTM: Test setup is well-structured.The setup and teardown properly initialize fake timers, mount the QueryClient, and ensure test isolation through cleanup.
42-43: Past review issue resolved.The debug console.log that was flagged in the previous review has been successfully removed from this section.
231-265: Excellent test that addresses the continuous scanning concern.This test validates the fix for reviewer timdorr's concern about the GC running continuously (every 10ms) for the life of the application. It confirms that:
- GC doesn't auto-start when idle
- GC only starts when queries are marked for collection
- GC stops after collecting all items
- GC remains stopped during subsequent idle periods
This behavior aligns with the author's proposed solution to track items in a map and start the timer only when map.size > 0, preventing interference with browser backgrounding behaviors.
267-293: LGTM: Comprehensive test coverage for GCManager.The mutation test correctly validates that mutations are tracked by the GC manager. The entire test suite provides excellent coverage of:
- Initial idle state
- Marking and tracking for GC
- Collection timing and scanning lifecycle
- Multiple items with different gcTime values
- Reactivation/untracking
- Edge cases (infinite gcTime, idle behavior)
- Both queries and mutations
The tests properly use fake timers and validate the new timestamp-based GC approach.
packages/solid-query/src/__tests__/useQuery.test.tsx (1)
41-43: Good call resetting timers after each testPrevents fake-timer leakage and cross-test flakiness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/gcManager.test.tsx (1)
1-1341: LGTM: Excellent test suite structure and coverage.The test suite is well-organized with clear describe blocks, comprehensive coverage of initialization, basic behavior, edge cases, error handling, and integration scenarios. The progression from simple to complex tests is logical and thorough.
Optional: Consider extracting magic numbers to constants.
Throughout the tests, gcTime values like 10, 20, 50, 100 are hardcoded. While this is acceptable, extracting them to named constants (e.g.,
SHORT_GC_TIME = 10,MEDIUM_GC_TIME = 50) could improve readability and maintainability, especially if these values need to be adjusted.Example:
const SHORT_GC_TIME = 10 const MEDIUM_GC_TIME = 50 const LONG_GC_TIME = 100 const observer = new QueryObserver(queryClient, { queryKey: key, queryFn: () => 'data', gcTime: SHORT_GC_TIME, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-core/src/__tests__/gcManager.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/gcManager.test.tsx (2)
packages/query-core/src/queryClient.ts (1)
QueryClient(63-658)packages/query-core/src/__tests__/utils.ts (1)
executeMutation(13-22)
🔇 Additional comments (10)
packages/query-core/src/__tests__/gcManager.test.tsx (10)
1-18: LGTM: Clean test setup with appropriate timer management.The setup correctly uses fake timers for deterministic testing of time-based GC behavior, and properly cleans up in afterEach.
563-597: Excellent test that addresses the continuous scanning concern.This test validates the solution to reviewer timdorr's concern about GC running continuously. It confirms that:
- GC doesn't start automatically when idle
- GC only starts when items are marked for collection
- GC stops after collecting all items
- GC remains stopped until new items are marked
This proves the implementation won't prevent browser backgrounding behaviors.
804-837: LGTM: Appropriate testing of environment-dependent logging behavior.The test correctly saves and restores
process.env.NODE_ENV. While environment manipulation can be fragile, this is a standard pattern for testing environment-dependent behavior, and the restoration is properly handled.
361-376: LGTM: Appropriate use of mocks for unit testing internal behavior.These tests use mocks to test GCManager's internal tracking and timer management in isolation. This is a good practice for unit testing, and the test suite appropriately complements these with integration tests using real Query and Mutation objects.
Also applies to: 443-481
704-802: LGTM: Comprehensive error handling coverage.These tests validate that the GC system is resilient to failures in individual items, ensuring that one failing item doesn't prevent collection of others. The tests properly mock and restore console.error, and verify that errors are logged in development but don't propagate.
1176-1340: LGTM: Excellent integration test coverage.These integration tests cover real-world scenarios including rapid subscribe/unsubscribe cycles, many items, client remounts, and dynamic state changes during scanning. This coverage is essential for validating the GCManager's behavior in production-like conditions.
954-1067: LGTM: Comprehensive mutations integration.These tests validate that the GCManager correctly handles mutations alongside queries, confirming the unified garbage collection approach works for all Removable types.
484-701: LGTM: Thorough edge case coverage.The edge case tests comprehensively validate behavior across the full range of
gcTimevalues, including Infinity, zero, very large, very small, and mixed scenarios. This ensures the GCManager handles all valid configurations correctly.
1069-1174: LGTM: Comprehensive scheduling behavior validation.These tests validate the internal scheduling state machine, including prevention of duplicate scheduling, proper rescheduling with timeout cancellation, and correct handling of race conditions. This ensures the GCManager's scheduling logic is robust.
304-482: LGTM: Comprehensive tracking lifecycle validation.These tests thoroughly validate the tracking and untracking lifecycle, including reactivation, duplicate prevention, safe handling of non-existent items, and proper timer cleanup. This ensures the GCManager correctly manages item tracking state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/query-core/src/__tests__/gcManager.test.tsx (6)
41-49: Baseline system time for determinism with fake timersSet a fixed system time in beforeEach to make Date.now()-based eligibility checks fully deterministic across runs.
beforeEach(() => { vi.useFakeTimers() + // Ensure deterministic Date.now() across tests + vi.setSystemTime(new Date(0)) gcManager = new GCManager() }) afterEach(() => { gcManager.clear() vi.useRealTimers() })
58-66: Add coverage for forceDisable configurationGCManager supports forceDisable; track/untrack should no-op and never scan when enabled. Add a test here.
Example:
test('should not track or scan when forceDisable=true', async () => { const disabled = new GCManager({ forceDisable: true }) const item = createMockRemovable({ gcTime: 10 }) disabled.trackEligibleItem(item) await vi.advanceTimersByTimeAsync(0) expect(disabled.getEligibleItemCount()).toBe(0) expect(disabled.isScanning()).toBe(false) disabled.clear() })
324-338: Clarify Infinity handling commentThe current comment contradicts itself. Keep it concise and accurate.
- // Item with infinite gcTime should not be tracked (returns Infinity from getGcAtTimestamp) - // But actually, it will be tracked, just won't schedule a scan + // Items with Infinity gcTime remain tracked, but no scan is scheduled
510-535: Protect NODE_ENV mutation with try/finallyEnsure environment is restored even if the test fails midway.
- const originalEnv = process.env.NODE_ENV - process.env.NODE_ENV = 'production' + const prevEnv = process.env.NODE_ENV + try { + process.env.NODE_ENV = 'production' const item = createMockRemovable({ gcTime: 10 }) // Mock item to throw item.isEligibleForGc = vi.fn(() => { throw new Error('Test error') }) gcManager.trackEligibleItem(item) await vi.advanceTimersByTimeAsync(0) const consoleErrorSpy = vi .spyOn(console, 'error') .mockImplementation(() => {}) await vi.advanceTimersByTimeAsync(15) // Should not log in production expect(consoleErrorSpy).not.toHaveBeenCalled() consoleErrorSpy.mockRestore() - process.env.NODE_ENV = originalEnv + } finally { + process.env.NODE_ENV = prevEnv + }
1-36: Optional: microtask vs timers flushing helperMany tests use await vi.advanceTimersByTimeAsync(0) to flush the queueMicrotask scheduling. To make intent explicit and avoid inadvertently firing zero-delay timers where not needed, add and use a tiny helper for microtasks only.
Example helper at top-level:
const flushMicrotasks = () => Promise.resolve()Then replace occurrences where only the microtask needs flushing (not a 0ms timeout) with:
await flushMicrotasks()
759-787: Optional: cover “untrack before schedule” pathAdd a test where you track an item and immediately untrack it before flushing the microtask; assert that no timeout is scheduled and nothing runs after advancing time.
Example:
test('should no-op if item is untracked before schedule completes', async () => { const item = createMockRemovable({ gcTime: 50, isEligibleFn: () => true }) gcManager.trackEligibleItem(item) // Untrack before microtask runs gcManager.untrackEligibleItem(item) await vi.advanceTimersByTimeAsync(0) await vi.advanceTimersByTimeAsync(100) expect(gcManager.isScanning()).toBe(false) expect(gcManager.getEligibleItemCount()).toBe(0) expect(item.isEligibleForGc).not.toHaveBeenCalled() expect(item.optionalRemove).not.toHaveBeenCalled() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-core/src/__tests__/gcManager.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/gcManager.test.tsx (1)
packages/query-core/src/gcManager.ts (2)
GCManager(38-196)item(172-190)
🔇 Additional comments (4)
packages/query-core/src/__tests__/gcManager.test.tsx (4)
69-79: LGTM: scanning starts only when neededStarts scanning after the first eligible item and asserts state correctly. This aligns with GCManager’s on-demand scheduling (no idle churn).
289-320: Great: verifies timeout cleanup on untrackSolid assertion that no scan fires after untracking (isEligibleForGc/optionalRemove never called).
372-398: Addresses idle-tab concern: no continuous scanning when idleThis test explicitly ensures scanning doesn’t run in the absence of eligible items, addressing reviewer feedback about background tab behavior.
If you have an integration test harness for the browser environment, consider a follow-up test that simulates page backgrounding (document.hidden) to ensure no unexpected timers leak.
661-679: Nice: stopScanning before microtask completesCovers the race where stopScanning is called prior to scheduled timeout creation; behavior matches GCManager’s #isScheduledScan guard.
|
Closing to recreate this PR, as the implementation has been significantly changed. |
2 participants
This PR is closed in favor of the #9827
Outdated description
PR is ready for a review/discussion. Once I address all issues, will create changesets.
🎯 Changes
TLDR: this PR improves mount/subscription performance by 5-10 times by using a single GC interval instead of a timeout per query.
This PR does not introduce any breaking changes, as edge cases behave exactly the same and the other cases are garbage collected within [
gcTime,gcTime + 10ms] interval.Demo: https://react-19-query-demo-git-tanstac-d7ec69-mrflashaccounts-projects.vercel.app/
Performance difference
Before:After:
Long explanation
Open to view
Overview
This PR refactors TanStack Query's garbage collection from individual timeout-based approach to a unified periodic scan approach, dramatically improving performance and scalability for applications with many queries.
The Problem
Before (Timeout-Based)
setTimeout(gcTime)which is expensive.setTimeout(gcTime)Performance Impact
The Solution
After (Periodic Scan)
Performance Improvement
Scan Interval Configuration
Users can now configure the scan interval:
Behavioral Change
GC Timing Precision
Before: Items removed after
gcTimeexpiration (atgcTime+ event loop delay)After: Items removed within
scanIntervalafter becoming eligible (atgcTime+scanInterval+ event loop delay)Example (
gcTime: 5 minutes,scanInterval: 10ms):In practice, this 10ms delay is negligible for most applications.
Immediate GC
For
gcTime: 0, queries trigger an immediate scan:Timer Reduction
For an app with 1000 inactive queries:
Scalability
API Compatibility
✅ No Breaking Changes
gcTimeoption behavior is preserved (within 10ms). 0 and Infinity are unchanged.Benefits
Performance
Reliability
Trade-offs
scanIntervallonger (10ms default)Migration Guide
For Library Users
No changes required! Existing code continues to work.
Future Enhancements
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Release Notes