feat(query-db-collection): support more top-level query options#1665
Open
KyleAMathews wants to merge 4 commits into
Open
feat(query-db-collection): support more top-level query options#1665KyleAMathews wants to merge 4 commits into
KyleAMathews wants to merge 4 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughQuery Collection now accepts selected top-level TanStack Query observer options, forwards only defined values, preserves adapter-owned fields, and allows ChangesQuery Collection options and synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant queryCollectionOptions
participant createQueryFromOpts
participant QueryClient
queryCollectionOptions->>createQueryFromOpts: provide supported observer options
createQueryFromOpts->>createQueryFromOpts: omit undefined values and preserve adapter-owned fields
createQueryFromOpts->>QueryClient: create query with merged observer options
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: 0 B Total Size: 125 kB ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 4.22 kB ℹ️ View Unchanged
|
KyleAMathews
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/query-db-collection/tests/query.test.ts (2)
194-223: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing
collection.cleanup()leaves stale state in the sharedqueryClient.Unlike the refetch test at line 267 which cleans up in a
finallyblock, this test never callscollection.cleanup(). The query with key['query-options-pass-through']remains in the sharedqueryClientcache, and theQueryClientmount fromenhancedInternalSyncis never unmounted.🧹 Proposed fix: add cleanup after assertions
expect(options.networkMode).toBe(`online`) + + await collection.cleanup() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query-db-collection/tests/query.test.ts` around lines 194 - 223, Add collection cleanup to the “should pass through additional top-level Query observer options” test after its assertions, preferably in a finally block so it always runs; call the collection’s cleanup method to remove the cached query and unmount the enhanced internal sync.
273-315: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing
collection.cleanup()leaks the localQueryClientmount.
clientWithDefaults.clear()only clears the query cache — it does not unmount theQueryClient. Withoutcollection.cleanup(), theenhancedInternalSynccleanup (which callsunmountQueryClient()) never runs, leaving the localQueryClientmounted with active subscriptions to the globalfocusManagerandonlineManager.🧹 Proposed fix: await cleanup before clearing client
expect(options.refetchOnWindowFocus).toBe(true) + await collection.cleanup() clientWithDefaults.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query-db-collection/tests/query.test.ts` around lines 273 - 315, Add await collection.cleanup() at the end of the test before clientWithDefaults.clear(), ensuring the collection’s enhancedInternalSync cleanup unmounts the local QueryClient and removes focusManager/onlineManager subscriptions.packages/query-db-collection/src/query.ts (1)
1949-2005: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
queryClientmount leaks if setup orinternalSyncthrows.
mountQueryClient()is called at line 1965, but if any code between there and thereturn(e.g.,queryKey({})on line 1970, orinternalSync(params)on line 1988) throws, the error propagates without a cleanup path. The returnedcleanupwrapper — which callsunmountQueryClient()in afinally— is never created, so theQueryClientstays mounted with lingering focus/online event subscriptions.🔒️ Proposed fix: wrap body in try/catch to unmount on error
mountQueryClient() + + try { // Get the base query key for the context (handle both static and function-based keys) const contextQueryKey = typeof queryKey === `function` ? (queryKey({}) as unknown as Array<unknown>) : (queryKey as unknown as Array<unknown>) // Store references for manual write operations writeContext = { collection, queryClient, queryKey: contextQueryKey, getKey: getKey as (item: any) => string | number, begin, write, commit, updateCacheData, } // Call the original internalSync logic, pairing QueryClient mount with the // collection sync lifecycle so focus/reconnect managers dispatch events for // standalone QueryClient usage. const syncResult = internalSync(params) const sync = typeof syncResult === `function` ? { cleanup: syncResult } : typeof syncResult === `object` ? syncResult : {} return { ...sync, cleanup: () => { try { sync.cleanup?.() } finally { unmountQueryClient() } }, } + } catch (error) { + unmountQueryClient() + throw error + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query-db-collection/src/query.ts` around lines 1949 - 2005, queryClient remains mounted when setup or internalSync fails before the cleanup wrapper is returned. In the sync setup containing mountQueryClient, contextQueryKey creation, writeContext assignment, and internalSync(params), wrap the post-mount work in try/catch; on any exception, call unmountQueryClient() and rethrow the original error, while preserving the existing returned cleanup behavior for successful setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/query-db-collection/src/query.ts`:
- Around line 1949-2005: queryClient remains mounted when setup or internalSync
fails before the cleanup wrapper is returned. In the sync setup containing
mountQueryClient, contextQueryKey creation, writeContext assignment, and
internalSync(params), wrap the post-mount work in try/catch; on any exception,
call unmountQueryClient() and rethrow the original error, while preserving the
existing returned cleanup behavior for successful setup.
In `@packages/query-db-collection/tests/query.test.ts`:
- Around line 194-223: Add collection cleanup to the “should pass through
additional top-level Query observer options” test after its assertions,
preferably in a finally block so it always runs; call the collection’s cleanup
method to remove the cached query and unmount the enhanced internal sync.
- Around line 273-315: Add await collection.cleanup() at the end of the test
before clientWithDefaults.clear(), ensuring the collection’s
enhancedInternalSync cleanup unmounts the local QueryClient and removes
focusManager/onlineManager subscriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93a0f24e-a102-47ce-b0cb-4109fd01074b
📒 Files selected for processing (2)
packages/query-db-collection/src/query.tspackages/query-db-collection/tests/query.test.ts
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.
Adds top-level Query Collection support for selected TanStack Query observer options:
refetchOnWindowFocus,refetchOnReconnect,refetchOnMount, andnetworkMode. This broadens Query option support while keeping the public API flat and preserving existingQueryClient.defaultOptionsbehavior for omitted or explicitly undefined options.Related RFC: #1643
Approach
queryCollectionOptionsfields, matching existing fields likeenabled,refetchInterval,retry,retryDelay,staleTime,gcTime, andmeta.queryObserverOptionKeyslist pluspickDefinedQueryObserverOptions(...)helper so future supported fields are not forwarded one-off in multiple places.undefined, allowingQueryClient.defaultOptionsto continue applying when collection config omits a value.QueryClientfor the active collection sync lifecycle and unmount during sync cleanup so Query Core focus/online manager events are installed for standalonenew QueryClient()usage.queryKey/queryFnstay controlled by the adapter.selectremains Query Collection row extraction, not TanStack Query observerselect.metaremains merged by Query Collection for on-demandloadSubsetOptions.subscribed,structuralSharing, andnotifyOnChangePropsremain adapter-owned to preserve synchronization behavior.Key invariants
queryOptions/querybag and therefore no duplicate option locations or precedence rules.undefinedvalues are not passed to TanStack Query, so defaults can still apply.Non-goals
queryOptionsorquerybag.selectas TanStack Query observerselect.initialDataorplaceholderData.Verification
Notes:
pnpm --filter @tanstack/query-db-collection test tests/query.test-d.tscommand is stale for the current Vitest include pattern and exits with “No test files found”; type coverage is exercised through the typecheckedquery.test.tsrun and package build.PLAN.mdis an untracked local planning file and is not part of this PR.Files changed
packages/query-db-collection/src/query.ts— adds flat top-level option fields, centralizes defined-option forwarding, and pairs QueryClient mount/unmount with collection sync lifecycle.packages/query-db-collection/tests/query.test.ts— covers top-level forwarding and default preservation.packages/query-db-collection/tests/query.test-d.ts— covers accepted top-level fields and rejected adapter-owned fields.docs/collections/query-collection.md— documents the flat option shape, default behavior, and adapter-owned/deferred fields..changeset/query-options-pass-through.md— records the minor release note for@tanstack/query-db-collection.Summary by CodeRabbit
refetchOnWindowFocus,refetchOnReconnect,refetchOnMount,networkMode).undefinedno longer overwritingQueryClientdefaults), while Collection-owned options keep precedence.