diff --git a/.changeset/fix-query-collection-remount-cache.md b/.changeset/fix-query-collection-remount-cache.md new file mode 100644 index 000000000..aca6956f6 --- /dev/null +++ b/.changeset/fix-query-collection-remount-cache.md @@ -0,0 +1,27 @@ +--- +"@tanstack/query-db-collection": patch +"@tanstack/db": patch +--- + +Fix data loss on component remount by implementing reference counting for QueryObserver lifecycle + +**What changed vs main:** + +Previously, when live query subscriptions unsubscribed, there was no tracking of which rows were still needed by other active queries. This caused data loss during remounts. + +This PR adds reference counting infrastructure to properly manage QueryObserver lifecycle: + +1. Pass same predicates to `unloadSubset` that were passed to `loadSubset` +2. Use them to compute the queryKey (via `generateQueryKeyFromOptions`) +3. Use existing machinery (`queryToRows` map) to find rows that query loaded +4. Decrement the ref count +5. GC rows where count reaches 0 (no longer referenced by any active query) + +**Impact:** + +- Navigation back to previously loaded pages shows cached data immediately +- No unnecessary refetches during quick remounts (< gcTime) +- Multiple live queries with identical predicates correctly share QueryObservers +- Proper row-level cleanup when last subscriber leaves +- TanStack Query's cache lifecycle (gcTime) is fully respected +- No data leakage from in-flight requests when unsubscribing diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..ee8056447 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,647 @@ +# Agent Coding Guidelines for TanStack DB + +This guide provides principles and patterns for AI agents contributing to the TanStack DB codebase. These guidelines are derived from PR review patterns and reflect the quality standards expected in this project. + +## Table of Contents + +1. [Type Safety](#type-safety) +2. [Code Organization](#code-organization) +3. [Algorithm Efficiency](#algorithm-efficiency) +4. [Semantic Correctness](#semantic-correctness) +5. [Abstraction Design](#abstraction-design) +6. [Code Clarity](#code-clarity) +7. [Testing Requirements](#testing-requirements) +8. [Function Design](#function-design) +9. [Modern JavaScript Patterns](#modern-javascript-patterns) +10. [Edge Cases and Corner Cases](#edge-cases-and-corner-cases) + +## Type Safety + +### Avoid `any` Types + +**❌ Bad:** + +```typescript +function processData(data: any) { + return data.value +} + +const result: any = someOperation() +``` + +**✅ Good:** + +```typescript +function processData(data: unknown) { + if (isDataObject(data)) { + return data.value + } + throw new Error("Invalid data") +} + +const result: TQueryData = someOperation() +``` + +**Key Principles:** + +- Use `unknown` instead of `any` when the type is truly unknown +- Provide proper type annotations for return values +- Use type guards to narrow `unknown` types safely +- If you find yourself using `any`, question whether there's a better type + +## Code Organization + +### Extract Common Logic + +**❌ Bad:** + +```typescript +// Duplicated logic in multiple places +function processA() { + const key = typeof value === "number" ? `__number__${value}` : String(value) + // ... +} + +function processB() { + const key = typeof value === "number" ? `__number__${value}` : String(value) + // ... +} +``` + +**✅ Good:** + +```typescript +function serializeKey(value: string | number): string { + return typeof value === "number" ? `__number__${value}` : String(value) +} + +function processA() { + const key = serializeKey(value) + // ... +} + +function processB() { + const key = serializeKey(value) + // ... +} +``` + +### Organize Utilities + +**Key Principles:** + +- Extract serialization/deserialization logic into utility files +- When you see identical or near-identical code blocks, extract to a helper function +- Prefer small, focused utility functions over large inline implementations +- Move reusable logic into utility modules (e.g., `utils/`, `helpers/`) + +### Function Size and Complexity + +**❌ Bad:** + +```typescript +function syncData() { + // 200+ lines of logic handling multiple concerns + // - snapshot phase + // - buffering + // - sync state management + // - error handling + // all inline... +} +``` + +**✅ Good:** + +```typescript +function syncData() { + handleSnapshotPhase() + manageBuffering() + updateSyncState() + handleErrors() +} + +function handleSnapshotPhase() { + // Focused logic for snapshot phase +} +``` + +**Key Principle:** If a function is massive, extract logical sections into separate functions. This improves readability and maintainability. + +## Algorithm Efficiency + +### Be Mindful of Time Complexity + +**❌ Bad: O(n²) Queue Processing:** + +```typescript +// Processes elements in queue, but elements may need multiple passes +while (queue.length > 0) { + const job = queue.shift() + if (hasUnmetDependencies(job)) { + queue.push(job) // Re-queue, causing O(n²) behavior + } else { + processJob(job) + } +} +``` + +**✅ Good: Dependency-Aware Processing:** + +```typescript +// Use a data structure that respects dependencies +// Process only jobs with no unmet dependencies +// Consider topological sort for DAG-like structures +const readyJobs = jobs.filter((job) => !hasUnmetDependencies(job)) +readyJobs.forEach(processJob) +``` + +### Use Appropriate Data Structures + +**❌ Bad:** + +```typescript +// O(n) lookup for each check +const items = ["foo", "bar", "baz" /* hundreds more */] +if (items.includes(searchValue)) { + // ... +} +``` + +**✅ Good:** + +```typescript +// O(1) lookup +const items = new Set(["foo", "bar", "baz" /* hundreds more */]) +if (items.has(searchValue)) { + // ... +} +``` + +**Key Principles:** + +- For membership checks on large collections, use `Set` instead of `Array.includes()` +- Be aware of nested loops and their complexity implications +- Consider the worst-case scenario, especially for operations that could process many items +- Use appropriate data structures (Set for lookups, Map for key-value, etc.) + +## Semantic Correctness + +### Ensure Logic Matches Intent + +**❌ Bad:** + +```typescript +// Intending to check if subset limit is more restrictive than superset +function isLimitSubset( + subset: number | undefined, + superset: number | undefined +) { + return subset === undefined || superset === undefined || subset <= superset +} + +// Problem: If subset has no limit but superset does, returns true (incorrect) +``` + +**✅ Good:** + +```typescript +function isLimitSubset( + subset: number | undefined, + superset: number | undefined +) { + // Subset with no limit cannot be a subset of one with a limit + return superset === undefined || (subset !== undefined && subset <= superset) +} +``` + +### Validate Intersections and Unions + +When merging predicates or combining queries, ensure the semantics are correct: + +**Example Problem:** + +```sql +-- Query 1: WHERE age >= 18 LIMIT 1 +-- Query 2: WHERE age >= 20 LIMIT 3 +-- Naive intersection: WHERE age >= 20 LIMIT 1 +-- Problem: This may not return the actual intersection of results +``` + +**Key Principle:** Think carefully about what operations like intersection, union, and subset mean for your specific use case. Consider edge cases with limits, ordering, and predicates. + +## Abstraction Design + +### Avoid Leaky Abstractions + +**❌ Bad:** + +```typescript +class Collection { + getViewKey(key: TKey): string { + // Caller needs to know internal representation + return `${this._state.viewKeyPrefix}${key}` + } +} + +// Usage exposes internals +const viewKey = collection.getViewKey(key) +if (viewKey.startsWith(PREFIX)) { + /* ... */ +} +``` + +**✅ Good:** + +```typescript +class Collection { + getViewKey(key: TKey): string { + // Delegate to state manager, hiding implementation + return this._state.getViewKey(key) + } +} + +class CollectionStateManager { + getViewKey(key: TKey): string { + return `${this.viewKeyPrefix}${key}` + } +} +``` + +**Key Principles:** + +- Encapsulate implementation details within the responsible class +- Don't expose internal data structures or representations +- Use delegation to maintain clean boundaries between components +- Keep internal properties private when possible + +### Proper Encapsulation + +**Key Principle:** If you need to access a property or method from outside a class, add a public method that delegates to the internal implementation rather than exposing the internal property directly. + +## Code Clarity + +### Prefer Positive Predicates + +**❌ Bad:** + +```typescript +if (!refs.some((ref) => ref.path[0] === outerAlias)) { + // treat as safe +} +``` + +**✅ Good:** + +```typescript +if (refs.every((ref) => ref.path[0] !== outerAlias)) { + // treat as safe +} +``` + +**Key Principle:** Positive conditions (every, all) are generally easier to understand than negated conditions (not some). + +### Simplify Complex Conditions + +**❌ Bad:** + +```typescript +const isLoadingNow = this.pendingLoadSubsetPromises.size > 0 +if (isLoadingNow && !isLoadingNow) { + // Confusing logic +} +``` + +**✅ Good:** + +```typescript +const wasLoading = this.pendingLoadSubsetPromises.size > 0 +this.pendingLoadSubsetPromises.add(promise) +const isLoadingNow = this.pendingLoadSubsetPromises.size === 1 + +if (isLoadingNow) { + // Started loading +} +``` + +### Use Descriptive Names + +**❌ Bad:** + +```typescript +const viewKeysMap = new Map() // Type in name is redundant +const dependencyBuilders = [] // Sounds like functions that build +``` + +**✅ Good:** + +```typescript +const viewKeys = new Map() // Data structure not in name +const dependentBuilders = [] // Accurately describes dependents +``` + +**Key Principles:** + +- Avoid Hungarian notation (encoding type in variable name) +- Use names that describe the role or purpose, not the data structure +- Choose names that make the code read like prose +- Prefer `dependentBuilders` over `dependencyBuilders` when referring to things that depend on something + +## Testing Requirements + +### Always Add Tests for Bugs + +**Key Principle:** If you're fixing a bug, add a unit test that reproduces the bug before fixing it. This ensures: + +- The bug is actually fixed +- The bug doesn't regress in the future +- The fix is validated + +**Example:** + +```typescript +// Found a bug with fetchSnapshot resolving after up-to-date message +// Should add a test: +test("ignores snapshot that resolves after up-to-date message", async () => { + // Reproduce the corner case + // Verify it's handled correctly +}) +``` + +### Test Corner Cases + +Common corner cases to consider: + +- Empty arrays or sets +- Single-element collections +- `undefined` vs `null` values +- Operations on already-resolved promises +- Race conditions between async operations +- Limit/offset edge cases (0, 1, very large numbers) +- IN predicates with 0 or 1 elements + +## Function Design + +### Prefer Explicit Parameters Over Closures + +**❌ Bad:** + +```typescript +function outer() { + const config = getConfig() + const state = getState() + + const updateFn = () => { + // Closes over config and state + applyUpdate(config, state) + } + + scheduler.schedule(updateFn) +} +``` + +**✅ Good:** + +```typescript +function updateEntry(entry: Entry, config: Config, state: State) { + applyUpdate(entry, config, state) +} + +function outer() { + const config = getConfig() + const state = getState() + + scheduler.schedule({ + config, + state, + update: updateEntry, + }) +} +``` + +**Key Principles:** + +- Functions that take dependencies as arguments are easier to test +- Explicit parameters make data flow clearer +- Closures can hide dependencies and make code harder to follow +- Use closures when they genuinely simplify the code, but be intentional + +### Return Type Precision + +**❌ Bad:** + +```typescript +function serializeKey(key: string | number): unknown { + return String(key) +} +``` + +**✅ Good:** + +```typescript +function serializeKey(key: string | number): string { + return String(key) +} +``` + +**Key Principle:** Always provide the most precise return type. Avoid `unknown` or `any` return types unless truly necessary. + +## Modern JavaScript Patterns + +### Use Modern Operators + +**❌ Bad:** + +```typescript +if (firstError === undefined) { + firstError = error +} + +const value = cached !== null && cached !== undefined ? cached : defaultValue + +if (obj[key] === undefined) { + obj[key] = value +} +``` + +**✅ Good:** + +```typescript +firstError ??= error + +const value = cached ?? defaultValue + +obj[key] ??= value +``` + +### Use Spread Operator + +**❌ Bad:** + +```typescript +const combined = [] +for (const item of currentItems) { + combined.push(item) +} +for (const item of newItems) { + combined.push(item) +} +``` + +**✅ Good:** + +```typescript +const combined = [...currentItems, ...newItems] +``` + +### Simplify Array Operations + +**❌ Bad:** + +```typescript +const filtered = [] +for (const item of items) { + if (item.value > 0) { + filtered.push(item) + } +} +``` + +**✅ Good:** + +```typescript +const filtered = items.filter((item) => item.value > 0) +``` + +## Edge Cases and Corner Cases + +### Common Patterns to Consider + +1. **Key Encoding**: When converting keys to strings, ensure no collisions + + ```typescript + // ❌ Bad: numeric 1 and string "__number__1" collide + const key = typeof val === "number" ? `__number__${val}` : String(val) + + // ✅ Good: proper encoding with type prefix + const key = `${typeof val}_${String(val)}` + ``` + +2. **Subset/Superset Logic**: Consider all cases + + ```typescript + // Consider: IN with 0, 1, or many elements + // Consider: EQ vs IN predicates + // Consider: Range predicates (>=, <=) vs equality + ``` + +3. **Limit and Offset**: Handle undefined, 0, and edge values + + ```typescript + // What happens when limit is 0? + // What happens when offset exceeds data length? + // What happens when limit is undefined? + ``` + +4. **Optional vs Required**: Be explicit about optionality + + ```typescript + // ❌ Why is this optional? + interface Config { + collection?: Collection + } + + // ✅ Document or make required if always needed + interface Config { + collection: Collection // Always required for query collections + } + ``` + +5. **Race Conditions**: Async operations may resolve in unexpected order + ```typescript + // Request snapshot before receiving up-to-date + // But snapshot resolves after up-to-date arrives + // Should ignore the stale snapshot + ``` + +## Package Versioning + +### Understand Semantic Versioning + +**Common Mistake:** + +```json +{ + "dependencies": { + "package": "^0.0.0" + } +} +``` + +**Problem:** `^0.0.0` restricts to exactly `0.0.0`, not "latest 0.0.x" as you might expect. + +From [npm semver docs](https://github.com/npm/node-semver): + +> Caret Ranges allow changes that do not modify the left-most non-zero element. For versions `0.0.X`, this means no updates. + +**Solutions:** + +- Use `*` for any version +- Use `latest` for the latest version +- Use a proper range like `^0.1.0` if that's what you mean + +## Documentation and Comments + +### Keep Useful Comments + +**Good Comment:** + +```typescript +// Returning false signals that callers should schedule another pass +return allDone +``` + +**Good Comment:** + +```typescript +// This step is necessary because the query function has captured +// the old subscription instance in its closure +``` + +### Remove Outdated Comments + +**Key Principle:** When refactoring code, update or remove comments that reference old function names or outdated logic. + +## General Principles + +1. **Question Optionality**: If a property is optional, understand why. Often it should be required. + +2. **Consider Performance**: Before implementing, think about time complexity, especially for operations that might process many items. + +3. **Validate Semantics**: Ensure that your implementation actually does what you think it does. Consider edge cases. + +4. **Avoid Premature Complexity**: Don't add ternaries, special cases, or checks for things that can't happen. + +5. **Test First for Bugs**: Reproduce bugs in tests before fixing them. + +6. **Be Consistent**: Follow naming conventions and patterns used elsewhere in the codebase. + +7. **Simplify**: Modern JavaScript provides many concise operators and methods. Use them. + +8. **Encapsulate**: Hide implementation details. Use delegation and proper abstraction boundaries. + +9. **Type Precisely**: Use the most specific type possible. Avoid `any`. + +10. **Extract When Duplicating**: If you're writing the same logic twice, extract it. + +## When in Doubt + +If you're unsure about an implementation decision: + +1. Look for similar patterns in the existing codebase +2. Consider the worst-case scenario for performance +3. Think about edge cases and corner cases +4. Ask: "Does this abstraction leak implementation details?" +5. Ask: "Would this be easy to test?" +6. Ask: "Is this as simple as it could be?" + +Remember: Simple, well-typed, well-tested code with clear abstractions is the goal. We raise the standard of code quality—not through complexity, but through clarity and correctness. diff --git a/packages/db/src/collection/subscription.ts b/packages/db/src/collection/subscription.ts index 33cedb946..01b6b5b6a 100644 --- a/packages/db/src/collection/subscription.ts +++ b/packages/db/src/collection/subscription.ts @@ -10,6 +10,7 @@ import type { BasicExpression, OrderBy } from "../query/ir.js" import type { IndexInterface } from "../indexes/base-index.js" import type { ChangeMessage, + LoadSubsetOptions, Subscription, SubscriptionEvents, SubscriptionStatus, @@ -47,6 +48,12 @@ export class CollectionSubscription // While `snapshotSent` is false we filter out all changes from subscription to the collection. private snapshotSent = false + /** + * Track all loadSubset calls made by this subscription so we can unload them on cleanup. + * We store the exact LoadSubsetOptions we passed to loadSubset to ensure symmetric unload. + */ + private loadedSubsets: Array = [] + // Keep track of the keys we've sent (needed for join and orderBy optimizations) private sentKeys = new Set() @@ -193,10 +200,14 @@ export class CollectionSubscription // Request the sync layer to load more data // don't await it, we will load the data into the collection when it comes in - const syncResult = this.collection._sync.loadSubset({ + const loadOptions: LoadSubsetOptions = { where: stateOpts.where, subscription: this, - }) + } + const syncResult = this.collection._sync.loadSubset(loadOptions) + + // Track this loadSubset call so we can unload it later + this.loadedSubsets.push(loadOptions) const trackLoadSubsetPromise = opts?.trackLoadSubsetPromise ?? true if (trackLoadSubsetPromise) { @@ -333,12 +344,16 @@ export class CollectionSubscription // Request the sync layer to load more data // don't await it, we will load the data into the collection when it comes in - const syncResult = this.collection._sync.loadSubset({ + const loadOptions1: LoadSubsetOptions = { where: whereWithValueFilter, limit, orderBy, subscription: this, - }) + } + const syncResult = this.collection._sync.loadSubset(loadOptions1) + + // Track this loadSubset call + this.loadedSubsets.push(loadOptions1) // Make parallel loadSubset calls for values equal to minValue and values greater than minValue const promises: Array> = [] @@ -348,10 +363,14 @@ export class CollectionSubscription const { expression } = orderBy[0]! const exactValueFilter = eq(expression, new Value(minValue)) - const equalValueResult = this.collection._sync.loadSubset({ + const loadOptions2: LoadSubsetOptions = { where: exactValueFilter, subscription: this, - }) + } + const equalValueResult = this.collection._sync.loadSubset(loadOptions2) + + // Track this loadSubset call + this.loadedSubsets.push(loadOptions2) if (equalValueResult instanceof Promise) { promises.push(equalValueResult) @@ -417,6 +436,13 @@ export class CollectionSubscription } unsubscribe() { + // Unload all subsets that this subscription loaded + // We pass the exact same LoadSubsetOptions we used for loadSubset + for (const options of this.loadedSubsets) { + this.collection._sync.unloadSubset(options) + } + this.loadedSubsets = [] + this.emitInner(`unsubscribed`, { type: `unsubscribed`, subscription: this, diff --git a/packages/db/src/collection/sync.ts b/packages/db/src/collection/sync.ts index 0485558a8..d0d963e85 100644 --- a/packages/db/src/collection/sync.ts +++ b/packages/db/src/collection/sync.ts @@ -43,6 +43,8 @@ export class CollectionSyncManager< public syncLoadSubsetFn: | ((options: LoadSubsetOptions) => true | Promise) | null = null + public syncUnloadSubsetFn: ((options: LoadSubsetOptions) => void) | null = + null private pendingLoadSubsetPromises: Set> = new Set() @@ -209,6 +211,9 @@ export class CollectionSyncManager< // Store loadSubset function if provided this.syncLoadSubsetFn = syncRes?.loadSubset ?? null + // Store unloadSubset function if provided + this.syncUnloadSubsetFn = syncRes?.unloadSubset ?? null + // Validate: on-demand mode requires a loadSubset function if (this.syncMode === `on-demand` && !this.syncLoadSubsetFn) { throw new CollectionConfigurationError( @@ -341,6 +346,16 @@ export class CollectionSyncManager< return true } + /** + * Notifies the sync layer that a subset is no longer needed. + * @param options Options that identify what data is being unloaded + */ + public unloadSubset(options: LoadSubsetOptions): void { + if (this.syncUnloadSubsetFn) { + this.syncUnloadSubsetFn(options) + } + } + public cleanup(): void { try { if (this.syncCleanupFn) { diff --git a/packages/db/src/types.ts b/packages/db/src/types.ts index 73944bf4e..aea66bcad 100644 --- a/packages/db/src/types.ts +++ b/packages/db/src/types.ts @@ -273,11 +273,14 @@ export type LoadSubsetOptions = { export type LoadSubsetFn = (options: LoadSubsetOptions) => true | Promise +export type UnloadSubsetFn = (options: LoadSubsetOptions) => void + export type CleanupFn = () => void export type SyncConfigRes = { cleanup?: CleanupFn loadSubset?: LoadSubsetFn + unloadSubset?: UnloadSubsetFn } export interface SyncConfig< T extends object = Record, diff --git a/packages/query-db-collection/e2e/query.e2e.test.ts b/packages/query-db-collection/e2e/query.e2e.test.ts index 7e31efdc6..7b44be9f6 100644 --- a/packages/query-db-collection/e2e/query.e2e.test.ts +++ b/packages/query-db-collection/e2e/query.e2e.test.ts @@ -160,24 +160,33 @@ describe(`Query Collection E2E Tests`, () => { // Mutations for Query collections - modify seed data and invalidate queries mutations: { insertUser: async (user) => { + console.log(`[mutation] insertUser called, id=${user.id}`) seedData.users.push(user) + console.log(`[mutation] calling invalidateQueries`) await queryClient.invalidateQueries({ queryKey: [`e2e`, `users`] }) + console.log(`[mutation] invalidateQueries completed`) }, updateUser: async (id, updates) => { + console.log(`[mutation] updateUser called, id=${id}`) const userIndex = seedData.users.findIndex((u) => u.id === id) if (userIndex !== -1) { seedData.users[userIndex] = { ...seedData.users[userIndex]!, ...updates, } + console.log(`[mutation] calling invalidateQueries`) await queryClient.invalidateQueries({ queryKey: [`e2e`, `users`] }) + console.log(`[mutation] invalidateQueries completed`) } }, deleteUser: async (id) => { + console.log(`[mutation] deleteUser called, id=${id}`) const userIndex = seedData.users.findIndex((u) => u.id === id) if (userIndex !== -1) { seedData.users.splice(userIndex, 1) + console.log(`[mutation] calling invalidateQueries`) await queryClient.invalidateQueries({ queryKey: [`e2e`, `users`] }) + console.log(`[mutation] invalidateQueries completed`) } }, insertPost: async (post) => { diff --git a/packages/query-db-collection/src/query.ts b/packages/query-db-collection/src/query.ts index 36c9e45d0..20d6aa48e 100644 --- a/packages/query-db-collection/src/query.ts +++ b/packages/query-db-collection/src/query.ts @@ -621,6 +621,23 @@ export function queryCollectionOptions( // queryKey → QueryObserver's unsubscribe function const unsubscribes = new Map void>() + // queryKey → reference count (how many loadSubset calls are active) + // Reference counting for QueryObserver lifecycle management + // ========================================================= + // Tracks how many live query subscriptions are using each QueryObserver. + // Multiple live queries with identical predicates share the same QueryObserver for efficiency. + // + // Lifecycle: + // - Increment: when createQueryFromOpts creates or reuses an observer + // - Decrement: when subscription.unsubscribe() passes predicates to collection._sync.unloadSubset() + // - Reset: when cleanupQuery() is triggered by TanStack Query's cache GC + // + // When refcount reaches 0, unloadSubset(): + // 1. Computes the same queryKey from the predicates + // 2. Uses existing machinery (queryToRows map) to find rows that query loaded + // 3. Decrements refcount and GCs rows where count reaches 0 + const queryRefCounts = new Map() + // Helper function to add a row to the internal state const addRow = (rowKey: string | number, hashedQueryKey: string) => { const rowToQueriesSet = rowToQueries.get(rowKey) || new Set() @@ -651,29 +668,44 @@ export function queryCollectionOptions( // Track whether sync has been started let syncStarted = false - const createQueryFromOpts = ( - opts: LoadSubsetOptions = {}, - queryFunction: typeof queryFn = queryFn - ): true | Promise => { - // Push the predicates down to the queryKey and queryFn - let key: QueryKey + /** + * Generate a consistent query key from LoadSubsetOptions. + * CRITICAL: Must use identical logic in both createQueryFromOpts and unloadSubset + * so that refcount increment/decrement operations target the same hashedQueryKey. + * Inconsistent keys would cause refcount leaks and prevent proper cleanup. + */ + const generateQueryKeyFromOptions = (opts: LoadSubsetOptions): QueryKey => { if (typeof queryKey === `function`) { // Function-based queryKey: use it to build the key from opts - key = queryKey(opts) + return queryKey(opts) } else if (syncMode === `on-demand`) { // Static queryKey in on-demand mode: automatically append serialized predicates // to create separate cache entries for different predicate combinations const serialized = serializeLoadSubsetOptions(opts) - key = serialized !== undefined ? [...queryKey, serialized] : queryKey + return serialized !== undefined ? [...queryKey, serialized] : queryKey } else { // Static queryKey in eager mode: use as-is - key = queryKey + return queryKey } + } + + const createQueryFromOpts = ( + opts: LoadSubsetOptions = {}, + queryFunction: typeof queryFn = queryFn + ): true | Promise => { + // Generate key using common function + const key = generateQueryKeyFromOptions(opts) const hashedQueryKey = hashKey(key) const extendedMeta = { ...meta, loadSubsetOptions: opts } if (state.observers.has(hashedQueryKey)) { // We already have a query for this queryKey + // Increment reference count since another consumer is using this observer + queryRefCounts.set( + hashedQueryKey, + (queryRefCounts.get(hashedQueryKey) || 0) + 1 + ) + // Get the current result and return based on its state const observer = state.observers.get(hashedQueryKey)! const currentResult = observer.getCurrentResult() @@ -732,6 +764,12 @@ export function queryCollectionOptions( hashToQueryKey.set(hashedQueryKey, key) state.observers.set(hashedQueryKey, localObserver) + // Increment reference count for this query + queryRefCounts.set( + hashedQueryKey, + (queryRefCounts.get(hashedQueryKey) || 0) + 1 + ) + // Create a promise that resolves when the query result is first available const readyPromise = new Promise((resolve, reject) => { const unsubscribe = localObserver.subscribe((result) => { @@ -751,13 +789,6 @@ export function queryCollectionOptions( subscribeToQuery(localObserver, hashedQueryKey) } - // Tell tanstack query to GC the query when the subscription is unsubscribed - // The subscription is unsubscribed when the live query is GCed. - const subscription = opts.subscription - subscription?.once(`unsubscribed`, () => { - queryClient.removeQueries({ queryKey: key, exact: true }) - }) - return readyPromise } @@ -874,10 +905,17 @@ export function queryCollectionOptions( hashedQueryKey: string ) => { if (!isSubscribed(hashedQueryKey)) { - const queryKey = hashToQueryKey.get(hashedQueryKey)! - const handleQueryResult = makeQueryResultHandler(queryKey) + const cachedQueryKey = hashToQueryKey.get(hashedQueryKey)! + const handleQueryResult = makeQueryResultHandler(cachedQueryKey) const unsubscribeFn = observer.subscribe(handleQueryResult) unsubscribes.set(hashedQueryKey, unsubscribeFn) + + // Process the current result immediately if available + // This ensures data is synced when resubscribing to a query with cached data + const currentResult = observer.getCurrentResult() + if (currentResult.isSuccess || currentResult.isError) { + handleQueryResult(currentResult) + } } } @@ -927,73 +965,181 @@ export function queryCollectionOptions( // Ensure we process any existing query data (QueryObserver doesn't invoke its callback automatically with initial state) state.observers.forEach((observer, hashedQueryKey) => { - const queryKey = hashToQueryKey.get(hashedQueryKey)! - const handleQueryResult = makeQueryResultHandler(queryKey) + const cachedQueryKey = hashToQueryKey.get(hashedQueryKey)! + const handleQueryResult = makeQueryResultHandler(cachedQueryKey) handleQueryResult(observer.getCurrentResult()) }) - // Subscribe to the query client's cache to handle queries that are GCed by tanstack query - const unsubscribeQueryCache = queryClient - .getQueryCache() - .subscribe((event) => { - const hashedKey = event.query.queryHash - if (event.type === `removed`) { - cleanupQuery(hashedKey) - } - }) - - function cleanupQuery(hashedQueryKey: string) { - // Unsubscribe from the query's observer + /** + * Perform row-level cleanup and remove all tracking for a query. + * Callers are responsible for ensuring the query is safe to cleanup. + */ + const cleanupQueryInternal = (hashedQueryKey: string) => { unsubscribes.get(hashedQueryKey)?.() + unsubscribes.delete(hashedQueryKey) - // Get all the rows that are in the result of this query const rowKeys = queryToRows.get(hashedQueryKey) ?? new Set() + const rowsToDelete: Array = [] - // Remove the query from these rows rowKeys.forEach((rowKey) => { - const queries = rowToQueries.get(rowKey) // set of queries that reference this row - if (queries && queries.size > 0) { - queries.delete(hashedQueryKey) - if (queries.size === 0) { - // Reference count dropped to 0, we can GC the row - rowToQueries.delete(rowKey) - - if (collection.has(rowKey)) { - begin() - write({ type: `delete`, value: collection.get(rowKey) }) - commit() - } + const queries = rowToQueries.get(rowKey) + + if (!queries) { + return + } + + queries.delete(hashedQueryKey) + + if (queries.size === 0) { + rowToQueries.delete(rowKey) + + if (collection.has(rowKey)) { + rowsToDelete.push(collection.get(rowKey)) } } }) - // Remove the query from the internal state - unsubscribes.delete(hashedQueryKey) + if (rowsToDelete.length > 0) { + begin() + rowsToDelete.forEach((row) => { + write({ type: `delete`, value: row }) + }) + commit() + } + state.observers.delete(hashedQueryKey) queryToRows.delete(hashedQueryKey) hashToQueryKey.delete(hashedQueryKey) + queryRefCounts.delete(hashedQueryKey) } + /** + * Attempt to cleanup a query when it appears unused. + * Respects refcounts and invalidateQueries cycles via hasListeners(). + */ + const cleanupQueryIfIdle = (hashedQueryKey: string) => { + const refcount = queryRefCounts.get(hashedQueryKey) || 0 + const observer = state.observers.get(hashedQueryKey) + + if (refcount <= 0) { + // Drop our subscription so hasListeners reflects only active consumers + unsubscribes.get(hashedQueryKey)?.() + unsubscribes.delete(hashedQueryKey) + } + + const hasListeners = observer?.hasListeners() ?? false + + if (hasListeners) { + // During invalidateQueries, TanStack Query keeps internal listeners alive. + // Leave refcount at 0 but keep observer so it can resubscribe. + queryRefCounts.set(hashedQueryKey, 0) + return + } + + // No listeners means the query is truly idle. + // Even if refcount > 0, we treat hasListeners as authoritative to prevent leaks. + // This can happen if subscriptions are GC'd without calling unloadSubset. + if (refcount > 0) { + console.warn( + `[cleanupQueryIfIdle] Invariant violation: refcount=${refcount} but no listeners. Cleaning up to prevent leak.`, + { hashedQueryKey } + ) + } + + cleanupQueryInternal(hashedQueryKey) + } + + /** + * Force cleanup used by explicit collection cleanup. + * Ignores refcounts/hasListeners and removes everything. + */ + const forceCleanupQuery = (hashedQueryKey: string) => { + cleanupQueryInternal(hashedQueryKey) + } + + // Subscribe to the query client's cache to handle queries that are GCed by tanstack query + const unsubscribeQueryCache = queryClient + .getQueryCache() + .subscribe((event) => { + const hashedKey = event.query.queryHash + if (event.type === `removed`) { + // Only cleanup if this is OUR query (we track it) + if (hashToQueryKey.has(hashedKey)) { + // TanStack Query GC'd this query after gcTime expired. + // Use the guarded cleanup path to avoid deleting rows for active queries. + cleanupQueryIfIdle(hashedKey) + } + } + }) + const cleanup = async () => { unsubscribeFromCollectionEvents() unsubscribeFromQueries() - const queryKeys = [...hashToQueryKey.values()] + const allQueryKeys = [...hashToQueryKey.values()] + const allHashedKeys = [...state.observers.keys()] - hashToQueryKey.clear() - queryToRows.clear() - rowToQueries.clear() - state.observers.clear() + // Force cleanup all queries (explicit cleanup path) + // This ignores hasListeners and always cleans up + for (const hashedKey of allHashedKeys) { + forceCleanupQuery(hashedKey) + } + + // Unsubscribe from cache events (cleanup already happened above) unsubscribeQueryCache() + // Remove queries from TanStack Query cache await Promise.all( - queryKeys.map(async (queryKey) => { - await queryClient.cancelQueries({ queryKey }) - queryClient.removeQueries({ queryKey }) + allQueryKeys.map(async (qKey) => { + await queryClient.cancelQueries({ queryKey: qKey, exact: true }) + queryClient.removeQueries({ queryKey: qKey, exact: true }) }) ) } + /** + * Unload a query subset - the subscription-based cleanup path (on-demand mode). + * + * Called when a live query subscription unsubscribes (via collection._sync.unloadSubset()). + * + * Flow: + * 1. Receives the same predicates that were passed to loadSubset + * 2. Computes the queryKey using generateQueryKeyFromOptions (same logic as loadSubset) + * 3. Decrements refcount + * 4. If refcount reaches 0: + * - Checks hasListeners() to detect invalidateQueries cycles + * - If hasListeners is true: resets refcount (TanStack Query keeping observer alive) + * - If hasListeners is false: calls forceCleanupQuery() to perform row-level GC + * + * The hasListeners() check prevents premature cleanup during invalidateQueries: + * - invalidateQueries causes temporary unsubscribe/resubscribe + * - During unsubscribe, our refcount drops to 0 + * - But observer.hasListeners() is still true (TanStack Query's internal listeners) + * - We skip cleanup and reset refcount, allowing resubscribe to succeed + * + * We don't cancel in-flight requests. Unsubscribing from the observer is sufficient + * to prevent late-arriving data from being processed. The request completes and is cached + * by TanStack Query, allowing quick remounts to restore data without refetching. + */ + const unloadSubset = (options: LoadSubsetOptions) => { + // 1. Same predicates → 2. Same queryKey + const key = generateQueryKeyFromOptions(options) + const hashedQueryKey = hashKey(key) + + // 3. Decrement refcount + const currentCount = queryRefCounts.get(hashedQueryKey) || 0 + const newCount = currentCount - 1 + + // Update refcount + if (newCount <= 0) { + queryRefCounts.set(hashedQueryKey, 0) + cleanupQueryIfIdle(hashedQueryKey) + } else { + // Still have other references, just decrement + queryRefCounts.set(hashedQueryKey, newCount) + } + } + // Create deduplicated loadSubset wrapper for non-eager modes // This prevents redundant snapshot requests when multiple concurrent // live queries request overlapping or subset predicates @@ -1002,6 +1148,7 @@ export function queryCollectionOptions( return { loadSubset: loadSubsetDedupe, + unloadSubset: syncMode === `eager` ? undefined : unloadSubset, cleanup, } } @@ -1025,9 +1172,9 @@ export function queryCollectionOptions( * @returns Promise that resolves when the refetch is complete, with QueryObserverResult */ const refetch: RefetchFn = async (opts) => { - const queryKeys = [...hashToQueryKey.values()] - const refetchPromises = queryKeys.map((queryKey) => { - const queryObserver = state.observers.get(hashKey(queryKey))! + const allQueryKeys = [...hashToQueryKey.values()] + const refetchPromises = allQueryKeys.map((qKey) => { + const queryObserver = state.observers.get(hashKey(qKey))! return queryObserver.refetch({ throwOnError: opts?.throwOnError, }) diff --git a/packages/query-db-collection/tests/query.test-d.ts b/packages/query-db-collection/tests/query.test-d.ts index b4f5140b0..59555e885 100644 --- a/packages/query-db-collection/tests/query.test-d.ts +++ b/packages/query-db-collection/tests/query.test-d.ts @@ -10,7 +10,7 @@ import { import { QueryClient } from "@tanstack/query-core" import { z } from "zod" import { queryCollectionOptions } from "../src/query" -import type { QueryCollectionConfig } from "../src/query" +import type { QueryCollectionConfig, QueryCollectionUtils } from "../src/query" import type { DeleteMutationFnParams, InsertMutationFnParams, @@ -70,15 +70,33 @@ describe(`Query collection type resolution tests`, () => { // Verify that the handlers are properly typed expectTypeOf(options.onInsert).parameters.toEqualTypeOf< - [InsertMutationFnParams] + [ + InsertMutationFnParams< + ExplicitType, + string | number, + QueryCollectionUtils + >, + ] >() expectTypeOf(options.onUpdate).parameters.toEqualTypeOf< - [UpdateMutationFnParams] + [ + UpdateMutationFnParams< + ExplicitType, + string | number, + QueryCollectionUtils + >, + ] >() expectTypeOf(options.onDelete).parameters.toEqualTypeOf< - [DeleteMutationFnParams] + [ + DeleteMutationFnParams< + ExplicitType, + string | number, + QueryCollectionUtils + >, + ] >() }) diff --git a/packages/query-db-collection/tests/query.test.ts b/packages/query-db-collection/tests/query.test.ts index 877bbe986..f867469a2 100644 --- a/packages/query-db-collection/tests/query.test.ts +++ b/packages/query-db-collection/tests/query.test.ts @@ -42,9 +42,10 @@ describe(`QueryCollection`, () => { queryClient = new QueryClient({ defaultOptions: { queries: { - // Setting a low staleTime and cacheTime to ensure queries can be refetched easily in tests + // Setting a low staleTime and gcTime to ensure queries can be refetched easily in tests // and GC'd quickly if not observed. staleTime: 0, + gcTime: 0, // Immediate GC for tests retry: false, // Disable retries for tests to avoid delays }, }, @@ -1000,8 +1001,11 @@ describe(`QueryCollection`, () => { expect(collection.status).toBe(`cleaned-up`) // Verify that cleanup methods are called regardless of subscriber state - expect(cancelQueriesSpy).toHaveBeenCalledWith({ queryKey }) - expect(removeQueriesSpy).toHaveBeenCalledWith({ queryKey }) + expect(cancelQueriesSpy).toHaveBeenCalledWith({ + queryKey, + exact: true, + }) + expect(removeQueriesSpy).toHaveBeenCalledWith({ queryKey, exact: true }) // Verify subscribers can be safely cleaned up after collection cleanup subscription1.unsubscribe() @@ -1143,8 +1147,11 @@ describe(`QueryCollection`, () => { expect(collection.status).toBe(`cleaned-up`) // Verify cleanup methods were called - expect(cancelQueriesSpy).toHaveBeenCalledWith({ queryKey }) - expect(removeQueriesSpy).toHaveBeenCalledWith({ queryKey }) + expect(cancelQueriesSpy).toHaveBeenCalledWith({ + queryKey, + exact: true, + }) + expect(removeQueriesSpy).toHaveBeenCalledWith({ queryKey, exact: true }) // Clear the spies to track new calls cancelQueriesSpy.mockClear() @@ -1162,8 +1169,11 @@ describe(`QueryCollection`, () => { await flushPromises() // Verify cleanup methods were called again for the restarted sync - expect(cancelQueriesSpy).toHaveBeenCalledWith({ queryKey }) - expect(removeQueriesSpy).toHaveBeenCalledWith({ queryKey }) + expect(cancelQueriesSpy).toHaveBeenCalledWith({ + queryKey, + exact: true, + }) + expect(removeQueriesSpy).toHaveBeenCalledWith({ queryKey, exact: true }) // Restore spies cancelQueriesSpy.mockRestore() @@ -3274,7 +3284,10 @@ describe(`QueryCollection`, () => { // Items 2 and 3 should remain because they're shared with other queries await query1.cleanup() - expect(collection.size).toBe(4) // Should have items 2, 3, 4, 5 + // Wait for async GC to complete (gcTime: 0 still schedules async removal) + await vi.waitFor(() => { + expect(collection.size).toBe(4) // Should have items 2, 3, 4, 5 + }) // Verify item 1 is removed (it was only in query 1) expect(collection.has(`1`)).toBe(false) @@ -3289,7 +3302,10 @@ describe(`QueryCollection`, () => { // Items 3 and 4 should remain because they are shared with query 3 await query2.cleanup() - expect(collection.size).toBe(3) // Should have items 3, 4, 5 + // Wait for async GC to complete + await vi.waitFor(() => { + expect(collection.size).toBe(3) // Should have items 3, 4, 5 + }) // Verify item 2 is removed (it was only in query 2) expect(collection.has(`2`)).toBe(false) @@ -3302,7 +3318,10 @@ describe(`QueryCollection`, () => { // GC query 3 (where: { category: 'C' }) - should remove all remaining items await query3.cleanup() - expect(collection.size).toBe(0) + // Wait for async GC to complete + await vi.waitFor(() => { + expect(collection.size).toBe(0) + }) // Verify all items are now removed expect(collection.has(`3`)).toBe(false) @@ -3413,7 +3432,10 @@ describe(`QueryCollection`, () => { // GC query 3 - should remove all items (no more queries reference them) await query3.cleanup() - expect(collection.size).toBe(0) + // Wait for async GC to complete + await vi.waitFor(() => { + expect(collection.size).toBe(0) + }) // All items should now be removed expect(collection.has(`1`)).toBe(false) @@ -3626,8 +3648,10 @@ describe(`QueryCollection`, () => { const proms = queries.map((query) => query.cleanup()) await Promise.all(proms) - // Collection should be empty after all queries are GCed - expect(collection.size).toBe(0) + // Wait for async GC to complete + await vi.waitFor(() => { + expect(collection.size).toBe(0) + }) // Verify all items are removed expect(collection.has(`1`)).toBe(false) @@ -3737,7 +3761,10 @@ describe(`QueryCollection`, () => { // GC the first query (all category A without limit) await query1.cleanup() - expect(collection.size).toBe(2) // Should only have items 1 and 2 because they are still referenced by query 2 + // Wait for async GC to complete + await vi.waitFor(() => { + expect(collection.size).toBe(2) // Should only have items 1 and 2 because they are still referenced by query 2 + }) // Verify that only row 3 is removed (it was only referenced by query 1) expect(collection.has(`1`)).toBe(true) // Still present (referenced by query 2) @@ -3748,7 +3775,521 @@ describe(`QueryCollection`, () => { await query2.cleanup() // Wait for final GC to process + await vi.waitFor(() => { + expect(collection.size).toBe(0) + }) + }) + + it(`should handle duplicate subset loads correctly (refcount bug)`, async () => { + // This test catches Bug 1: missing refcount increment when reusing existing observer + // When two subscriptions load the same subset, unloading one should NOT destroy + // the observer since another subscription still needs it + + const baseQueryKey = [`refcount-bug-test`] + const items: Array = [ + { id: `1`, name: `Item 1`, category: `A` }, + { id: `2`, name: `Item 2`, category: `A` }, + { id: `3`, name: `Item 3`, category: `A` }, + ] + + const queryFn = vi.fn().mockResolvedValue(items) + + const config: QueryCollectionConfig = { + id: `refcount-test`, + queryClient, + queryKey: baseQueryKey, + queryFn, + getKey: (item) => item.id, + startSync: true, + syncMode: `on-demand`, + onInsert: async () => ({ refetch: false }), + } + + const options = queryCollectionOptions(config) + const collection = createCollection(options) + + // Create two live queries that request the SAME subset + const query1 = createLiveQueryCollection({ + query: (q) => + q + .from({ item: collection }) + .where(({ item }) => eq(item.category, `A`)) + .select(({ item }) => ({ id: item.id, name: item.name })), + }) + + const query2 = createLiveQueryCollection({ + query: (q) => + q + .from({ item: collection }) + .where(({ item }) => eq(item.category, `A`)) + .select(({ item }) => ({ id: item.id, name: item.name })), + }) + + // Load both queries + await query1.preload() + await query2.preload() + + // Wait for data to load + await vi.waitFor(() => { + expect(collection.size).toBe(3) + }) + expect(queryFn).toHaveBeenCalledTimes(1) // Deduplicated + + // Cleanup query1 + await query1.cleanup() + await flushPromises() + + // BUG: Without refcount increment on reuse, the observer is destroyed + // and query2 stops receiving updates. Collection data is also removed. + // EXPECTED: query2 should still work since it's using the same observer + await vi.waitFor(() => { + expect(collection.size).toBe(3) // Should still have data for query2 + }) + + // Verify query2 still works by mutating data + await collection.insert({ id: `4`, name: `Item 4`, category: `A` }) + await vi.waitFor(() => { + expect(collection.size).toBe(4) + expect(collection.has(`4`)).toBe(true) + }) + + // Now cleanup query2 + await query2.cleanup() + await vi.waitFor(() => { + expect(collection.size).toBe(0) // NOW it should be cleaned up + }) + }) + + it(`should reset refcount after query GC and reload (stale refcount bug)`, async () => { + // This test catches Bug 2: stale refcounts after GC/remove + // When TanStack Query GCs a query, the refcount should be cleaned up + // Otherwise, reloading the same subset will start with a stale count + + const baseQueryKey = [`stale-refcount-test`] + const items: Array = [ + { id: `1`, name: `Item 1`, category: `A` }, + { id: `2`, name: `Item 2`, category: `A` }, + ] + + const queryFn = vi.fn().mockResolvedValue(items) + + const config: QueryCollectionConfig = { + id: `stale-refcount-test`, + queryClient, + queryKey: baseQueryKey, + queryFn, + getKey: (item) => item.id, + startSync: true, + syncMode: `on-demand`, + } + + const options = queryCollectionOptions(config) + const collection = createCollection(options) + + // Create and load a query + const query1 = createLiveQueryCollection({ + query: (q) => + q + .from({ item: collection }) + .where(({ item }) => eq(item.category, `A`)) + .select(({ item }) => ({ id: item.id, name: item.name })), + }) + + await query1.preload() + + // Wait for data to load + await vi.waitFor(() => { + expect(collection.size).toBe(2) + }) + + // Force GC by calling removeQueries (simulates gcTime expiry) + queryClient.removeQueries({ queryKey: baseQueryKey }) + await flushPromises() + + // BUG: queryRefCounts still has stale count, wasn't cleaned up by cleanupQuery + // When we load again, the refcount will be wrong (starts at 1 instead of 0, or accumulates) + + // Reload the same query + const query2 = createLiveQueryCollection({ + query: (q) => + q + .from({ item: collection }) + .where(({ item }) => eq(item.category, `A`)) + .select(({ item }) => ({ id: item.id, name: item.name })), + }) + + await query2.preload() + + // Wait for data to reload + await vi.waitFor(() => { + expect(collection.size).toBe(2) + }) + + // Cleanup - this should properly decrement from 1 to 0 and clean up + await query2.cleanup() + await vi.waitFor(() => { + expect(collection.size).toBe(0) // Should be cleaned up + }) + + // BUG SYMPTOM: If refcount was stale (e.g. was 2, decremented to 1), + // the observer won't be destroyed and data won't be cleaned up + }) + + it(`should handle mount/unmount/remount without breaking cache (destroyed observer bug)`, async () => { + // This test catches Bug 3: destroyed observer reuse + // When subscriberCount hits 0, unsubscribeFromQueries() destroys observers + // but leaves them in state.observers. On remount, subscribeToQueries() + // tries to reuse destroyed observers, which breaks cache processing + + const baseQueryKey = [`destroyed-observer-test`] + const items: Array = [ + { id: `1`, name: `Item 1` }, + { id: `2`, name: `Item 2` }, + ] + + const queryFn = vi.fn().mockResolvedValue(items) + + // Use a longer gcTime to ensure cache persists across unmount/remount + const customQueryClient = new QueryClient({ + defaultOptions: { + queries: { + gcTime: 5 * 60 * 1000, // 5 minutes + staleTime: 0, + retry: false, + }, + }, + }) + + const config: QueryCollectionConfig = { + id: `destroyed-observer-test`, + queryClient: customQueryClient, + queryKey: baseQueryKey, + queryFn, + getKey, + startSync: true, + syncMode: `on-demand`, + } + + const options = queryCollectionOptions(config) + const collection = createCollection(options) + + // Mount: create and subscribe to a query + const query1 = createLiveQueryCollection({ + query: (q) => q.from({ item: collection }).select(({ item }) => item), + }) + + await query1.preload() + + // Wait for initial data to load + await vi.waitFor(() => { + expect(collection.size).toBe(2) + }) + expect(queryFn).toHaveBeenCalledTimes(1) + + // Unmount: cleanup the query, triggering subscriberCount -> 0 + // This calls unsubscribeFromQueries() which destroys observers + await query1.cleanup() + await flushPromises() + + // At this point, observer.destroy() was called but observer is still in state.observers + + // Remount quickly (before gcTime expires): cache should still be valid + const query2 = createLiveQueryCollection({ + query: (q) => q.from({ item: collection }).select(({ item }) => item), + }) + + // BUG: subscribeToQueries() tries to subscribe to the destroyed observer + // QueryObserver.destroy() is terminal - reactivation isn't guaranteed + // This breaks cache processing on remount + + await query2.preload() + + // EXPECTED: Should process cached data immediately without refetch + await vi.waitFor(() => { + expect(collection.size).toBe(2) + }) + expect(queryFn).toHaveBeenCalledTimes(1) // No refetch! + + // BUG SYMPTOM: If destroyed observer doesn't process cached results, + // collection will be empty or queryFn will be called again + }) + + it(`should not leak data when unsubscribing while load is in flight`, async () => { + // Test the edge case where the last subscriber unsubscribes before queryFn resolves. + // We need to ensure that: + // 1. No late-arriving data is written after unsubscribe + // 2. No rows leak back into the collection + + const baseQueryKey = [`in-flight-unsubscribe-test`] + const items: Array = [ + { id: `1`, name: `Item 1` }, + { id: `2`, name: `Item 2` }, + ] + + // Create a delayed queryFn that we can control + let resolveQuery: ((value: Array) => void) | undefined + const queryFnPromise = new Promise>((resolve) => { + resolveQuery = resolve + }) + const queryFn = vi.fn().mockReturnValue(queryFnPromise) + + const customQueryClient = new QueryClient({ + defaultOptions: { + queries: { + gcTime: 5 * 60 * 1000, + staleTime: 0, + retry: false, + }, + }, + }) + + const config: QueryCollectionConfig = { + id: `in-flight-unsubscribe-test`, + queryClient: customQueryClient, + queryKey: baseQueryKey, + queryFn, + getKey, + startSync: true, + syncMode: `on-demand`, + } + + const options = queryCollectionOptions(config) + const collection = createCollection(options) + + // Create a live query and start loading + const query1 = createLiveQueryCollection({ + query: (q) => q.from({ item: collection }).select(({ item }) => item), + }) + + // Start preload but don't await - this triggers the queryFn + const preloadPromise = query1.preload() + + // Wait a bit to ensure queryFn has been called + await flushPromises() + expect(queryFn).toHaveBeenCalledTimes(1) + expect(collection.size).toBe(0) // No data yet + + // Unsubscribe while the query is still in flight (before queryFn resolves) + await query1.cleanup() + await flushPromises() + + // Collection should be empty after cleanup expect(collection.size).toBe(0) + + // Now resolve the query - this is the "late-arriving data" + resolveQuery!(items) + await flushPromises() + + // CRITICAL: After the late-arriving data is processed, the collection + // should still be empty. No rows should leak back in. + expect(collection.size).toBe(0) + + // Clean up + try { + await preloadPromise + } catch { + // Query was cancelled, this is expected + } + }) + }) + + describe(`Cache Persistence on Remount`, () => { + it(`should process cached results immediately when QueryObserver resubscribes`, async () => { + const queryKey = [`remount-cache-test`] + const items: Array = [ + { id: `1`, name: `Item 1` }, + { id: `2`, name: `Item 2` }, + { id: `3`, name: `Item 3` }, + ] + + const queryFn = vi.fn().mockResolvedValue(items) + + // Use a longer gcTime to simulate cache persistence + const customQueryClient = new QueryClient({ + defaultOptions: { + queries: { + gcTime: 5 * 60 * 1000, // 5 minutes + staleTime: 0, + retry: false, + }, + }, + }) + + const config: QueryCollectionConfig = { + id: `remount-cache-test`, + queryClient: customQueryClient, + queryKey, + queryFn, + getKey, + startSync: true, + syncMode: `on-demand`, + } + + const options = queryCollectionOptions(config) + const collection = createCollection(options) + + // Create first live query and load data + const query1 = createLiveQueryCollection({ + query: (q) => + q + .from({ item: collection }) + .select(({ item }) => ({ id: item.id, name: item.name })), + }) + + await query1.preload() + + // Wait for data to load + await vi.waitFor(() => { + expect(collection.size).toBe(3) + expect(queryFn).toHaveBeenCalledTimes(1) + }) + + // Verify all items are present before creating second query + expect(collection.has(`1`)).toBe(true) + expect(collection.has(`2`)).toBe(true) + expect(collection.has(`3`)).toBe(true) + + // Create second live query while first is still active + // This simulates multiple components using the same collection + // (e.g., list view and detail view both querying the same collection) + const query2 = createLiveQueryCollection({ + query: (q) => + q + .from({ item: collection }) + .select(({ item }) => ({ id: item.id, name: item.name })), + }) + + // Preload - this should use cached data and process it immediately + await query2.preload() + await flushPromises() + + // queryFn should still only have been called once (using cache) + // This verifies the fix: QueryObserver processes cached results immediately + expect(queryFn).toHaveBeenCalledTimes(1) + + // Data should be present in both queries + expect(collection.size).toBe(3) + expect(collection.has(`1`)).toBe(true) + expect(collection.has(`2`)).toBe(true) + expect(collection.has(`3`)).toBe(true) + + // Cleanup + await query1.cleanup() + await query2.cleanup() + customQueryClient.clear() + }) + + it(`should preserve cache and avoid refetch during quick remount`, async () => { + const queryKey = [`preserve-cache-remount`] + const items: Array = [{ id: `1`, name: `Item 1` }] + + const queryFn = vi.fn().mockResolvedValue(items) + + const config: QueryCollectionConfig = { + id: `preserve-cache-remount`, + queryClient, + queryKey, + queryFn, + getKey, + startSync: true, + syncMode: `on-demand`, + } + + const options = queryCollectionOptions(config) + const collection = createCollection(options) + + // Create and load first query + const query1 = createLiveQueryCollection({ + query: (q) => q.from({ item: collection }), + }) + + await query1.preload() + + await vi.waitFor(() => { + expect(collection.size).toBe(1) + expect(queryFn).toHaveBeenCalledTimes(1) + }) + + // Create second query while first is still active (simulating remount) + // In real-world React, the first component unmounts but cleanup is deferred + const query2 = createLiveQueryCollection({ + query: (q) => q.from({ item: collection }), + }) + + await query2.preload() + await flushPromises() + + // Cache should still be present in the collection + expect(collection.size).toBe(1) + + // We should NOT have refetched (used TanStack Query cache) + expect(queryFn).toHaveBeenCalledTimes(1) + + // Cleanup both + await query1.cleanup() + await query2.cleanup() + }) + + it(`should allow TanStack Query to manage cache lifecycle via gcTime`, async () => { + const queryKey = [`gctime-respect-test`] + const items: Array = [ + { id: `1`, name: `Item 1` }, + { id: `2`, name: `Item 2` }, + ] + + const queryFn = vi.fn().mockResolvedValue(items) + + // Use a longer gcTime to verify cache isn't prematurely removed + const customQueryClient = new QueryClient({ + defaultOptions: { + queries: { + gcTime: 5 * 60 * 1000, // 5 minutes + staleTime: 0, + retry: false, + }, + }, + }) + + const config: QueryCollectionConfig = { + id: `gctime-respect-test`, + queryClient: customQueryClient, + queryKey, + queryFn, + getKey, + startSync: true, + syncMode: `on-demand`, + } + + const options = queryCollectionOptions(config) + const collection = createCollection(options) + + // First mount + const query1 = createLiveQueryCollection({ + query: (q) => q.from({ item: collection }), + }) + + await query1.preload() + await vi.waitFor(() => { + expect(collection.size).toBe(2) + expect(queryFn).toHaveBeenCalledTimes(1) + }) + + // Create second query while first is active (simulating overlapping mount) + const query2 = createLiveQueryCollection({ + query: (q) => q.from({ item: collection }), + }) + + await query2.preload() + await flushPromises() + + // Should still use cache - no refetch + expect(queryFn).toHaveBeenCalledTimes(1) + expect(collection.size).toBe(2) + + // Cleanup both + await query1.cleanup() + await query2.cleanup() + customQueryClient.clear() }) })