fix: address performance, testing, and documentation issues #10091
Conversation
Performance: - Convert observers from Array to Set for O(1) lookups - Add countMatching() to queryCache to avoid creating intermediate arrays - Cache Object.keys result in shallowEqualObjects Testing: - Add tests for vue-query-devtools, svelte-query-devtools, react-query-next-experimental - Replace placeholder test in query-devtools with actual component tests - Update existing tests for Set-based observers API Documentation: - Add JSDoc comments to QueryClient, QueryObserver, and core types Architecture: - Split types.ts (1,391 lines) into focused modules (query, mutation, observer, common, client) - Maintain backwards compatibility via re-exports Other: - Improve input validation and error handling in persistence layer Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughRefactors observer storage from arrays to Sets; expands QueryClient with many imperative cache/data methods; promotes QueryObserver to a subscribable class with lifecycle and optimistic tracking; reorganizes and centralizes query-core types; adds persister validation/protection and improves tests and test configs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/query-core/src/types/common.ts`:
- Around line 44-50: Add a Biome ignore comment immediately above the empty
Register interface to silence the no-empty-interface rule used for module
augmentation; e.g. place a line like "// biome-ignore-next-line
no-empty-interface" directly above the "export interface Register { }"
declaration so the lint rule is skipped for the Register interface while
preserving the empty interface for augmentation.
In `@packages/query-persist-client-core/src/createPersister.ts`:
- Around line 104-129: The validateDeserializedData function uses Object.hasOwn
(ES2022) which breaks ES2020 targets; update the forbidden-key checks in
validateDeserializedData to use Object.prototype.hasOwnProperty.call(obj,
'<key>') for '__proto__', 'constructor', and 'prototype' (preserving the
existing error throw) so the check is compatible with ES2020 environments and
still prevents prototype pollution.
🧹 Nitpick comments (2)
packages/vue-query-devtools/src/__tests__/devtools.test.ts (1)
4-24: Consider expanding tests to exercise the mocks.The mocks for
TanstackQueryDevtoolslifecycle methods (mount,unmount,setButtonPosition, etc.) are comprehensive but currently unused. If you plan to expand test coverage later, consider adding tests that actually render the components and verify these methods are called appropriately.packages/query-core/src/queryClient.ts (1)
154-158: Consider addingcountMatching()to MutationCache for consistency.The
isFetching()method was optimized to usecountMatching(), butisMutating()still usesfindAll().length. While mutations are typically fewer than queries (making this less impactful), adding a similar optimization to MutationCache would maintain consistency.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The Register interface is intentionally empty for module augmentation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/query-persist-client-core/src/createPersister.ts (2)
299-317:⚠️ Potential issue | 🟡 MinorConsider wrapping validation in try-catch for resilience.
If
validateDeserializedDatathrows on one corrupted entry, the entire GC loop stops and remaining stale entries won't be cleaned up. UnlikeretrieveQuery, this function lacks error handling around the deserialization/validation step.🛡️ Proposed fix to continue GC on corrupted entries
for (const [key, value] of entries) { if (key.startsWith(prefix)) { - const persistedQuery = await deserialize(value) - validateDeserializedData(persistedQuery) - - if (isExpiredOrBusted(persistedQuery)) { + try { + const persistedQuery = await deserialize(value) + validateDeserializedData(persistedQuery) + + if (isExpiredOrBusted(persistedQuery)) { + await storage.removeItem(key) + } + } catch { + // Remove corrupted entries during GC await storage.removeItem(key) } } }
319-361:⚠️ Potential issue | 🟡 MinorSame resilience concern as
persisterGc.Similar to the GC function, a single corrupted entry will abort the entire restoration loop. Consider whether partial restoration with logged warnings would provide better user experience than failing entirely.
The parameter rename from
filterstoqueryFiltersimproves clarity.🛡️ Proposed fix to continue restoration on corrupted entries
for (const [key, value] of entries) { if (key.startsWith(prefix)) { - const persistedQuery = await deserialize(value) - validateDeserializedData(persistedQuery) + let persistedQuery: PersistedQuery + try { + persistedQuery = await deserialize(value) + validateDeserializedData(persistedQuery) + } catch (err) { + if (process.env.NODE_ENV === 'development') { + console.error(err) + console.warn('Skipping corrupted persisted query:', key) + } + await storage.removeItem(key) + continue + } if (isExpiredOrBusted(persistedQuery)) {
🧹 Nitpick comments (2)
ANALYSIS_REPORT.md (2)
1-10: Clarify which issues are addressed by this PR.The report presents findings as open issues requiring action, but the PR objectives indicate several items are being fixed in this PR (observer arrays → Sets, QueryCache.countMatching(), types.ts splitting, JSDoc additions, test coverage). This creates confusion about what's already resolved versus what remains outstanding.
Consider adding a note in the Executive Summary or a new section indicating which recommendations are implemented by the accompanying PR changes.
📝 Suggested addition to Executive Summary
Five specialized agents reviewed the TanStack Query codebase from different perspectives. This report consolidates findings across architecture, testing, performance, security, and documentation. The project is well-engineered overall but has accumulated technical debt that warrants attention. + +**Note:** Several high-priority items identified in this analysis are addressed by the accompanying PR changes, including: +- Performance: Observer collections converted from Array to Set (Section 3.4) +- Performance: QueryCache.countMatching() added to reduce N+1 patterns (Section 3.3) +- Architecture: types.ts split into focused modules (Section 1.1) +- Testing: Coverage added for previously untested packages (Section 2.1) +- Documentation: JSDoc comments added to core types (Section 5.1) + +Remaining recommendations require follow-up work as outlined in Section 7.
378-385: Add language specification to fenced code block.The fenced code block is missing a language identifier, which triggers a linting warning.
🔧 Proposed fix
**Location:** `examples/react/basic/README.md` -``` +```markdown # Basic To run this example:
1 participant
Summary
This PR addresses multiple issues identified through automated code analysis:
Changes
Performance
observersfromArraytoSetfor O(1) add/remove/has operationscountMatching()to QueryCache to avoid creating intermediate arrays inisFetching()Object.keys()result inshallowEqualObjects()instead of calling twiceTesting
vue-query-devtools(was 0% coverage)svelte-query-devtools(was 0% coverage)react-query-next-experimental(was 0% coverage)query-devtoolswith actual component testsDocumentation
QueryClientclass and public methodsQueryObserverclass and key methodsArchitecture
types.ts(1,391 lines) into focused modules:types/query.ts- Query-related typestypes/mutation.ts- Mutation-related typestypes/observer.ts- Observer-related typestypes/common.ts- Shared utility typestypes/client.ts- Client config typesOther
Test plan
pnpm run test:types)pnpm run test:eslint)pnpm run format)pnpm run test:lib- all 20 packages)🤖 Generated with Claude Code
Summary by CodeRabbit