Skip to content

Commit 284ebcc

Browse files
KyleAMathewsclaudeautofix-ci[bot]
authored
fix(db): track loadSubset promise for on-demand live queries (#1192)
* fix(db): track loadSubset promise for non-ordered on-demand live queries When a live query without orderBy/limit subscribes to an on-demand collection, the subscription was passing includeInitialState: true to subscribeChanges, which internally called requestSnapshot with trackLoadSubsetPromise: false. This prevented the subscription status from transitioning to 'loadingSubset', causing the live query to be marked ready before data actually loaded. The fix changes subscribeToMatchingChanges to manually call requestSnapshot() after creating the subscription (with default tracking enabled), ensuring the loadSubset promise is properly tracked and the live query waits for data before becoming ready. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(db): handle cleanup and cache edge cases for on-demand live queries This follow-up fix addresses edge cases that caused tests to fail after the initial isReady fix: 1. lifecycle.ts: Call pending onFirstReady callbacks during cleanup - Prevents preload() promises from hanging when cleanup happens - Ensures clean termination of pending preload operations 2. query.ts: Add fallback cache checks in createQueryFromOpts - Check QueryClient cache when observer state is out of sync - Handle cases where observer was deleted but data is still cached - Prevents hangs during cleanup/recreate cycles 3. Test updates: - Updated where clause tests to use synchronous loadSubset - These tests verify where clause passing, not async loading behavior Co-Authored-By: Claude Opus 4.5 <[email protected]> * chore: add changeset for on-demand isReady fix Co-Authored-By: Claude Opus 4.5 <[email protected]> * ci: apply automated fixes * fix(db): address review feedback - cleanup and error handling - Fix potential memory leak in trackCollectionLoading by registering cleanup callback for early unsubscribe scenarios - Add error handling for onFirstReady callbacks during cleanup to ensure all callbacks are attempted even if one throws Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: concurrent live queries now independently track loading state Previously, when multiple live queries subscribed to the same source collection, only the first would correctly track loading state due to the `!wasLoadingBefore` guard. This caused subsequent live queries to incorrectly report `isReady` before data finished loading. The fix removes this guard since each live query needs its own loading state tracking regardless of whether another query already triggered loading. Also adds a regression test for this scenario. Co-Authored-By: Claude Opus 4.5 <[email protected]> * Fix ordered live query paging offset tracking * ci: apply automated fixes * Add regression coverage for ordered on-demand paging * Avoid passing cross-alias orderBy to loadSubset * Avoid redundant ordered loads while snapshot pending * Refactor subscription loading tracking * refactor: simplify subscription and collection-subscriber code - Remove redundant has() check before Set.add() (idempotent operation) - Replace nested ternary with clearer if-statement for minValues Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 parent 43c7c9d commit 284ebcc

11 files changed

Lines changed: 577 additions & 77 deletions

File tree

‎.changeset/fix-on-demand-isready.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@tanstack/db': patch
3+
'@tanstack/query-db-collection': patch
4+
---
5+
6+
Fix `isReady` tracking for on-demand live queries without orderBy. Previously, non-ordered live queries using `syncMode: 'on-demand'` were incorrectly marked as ready before data finished loading. Also fix `preload()` promises hanging when cleanup occurs before the collection becomes ready. Additionally, fix concurrent live queries subscribing to the same source collection - each now independently tracks loading state.

‎packages/db-collection-e2e/src/suites/pagination.suite.ts‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,110 @@ export function createPaginationTestSuite(
264264
await query2.cleanup()
265265
})
266266

267+
it(`should load distinct windows across multiple live queries with multi-column orderBy`, async () => {
268+
const config = await getConfig()
269+
const usersCollection = config.collections.onDemand.users
270+
271+
const page1 = createLiveQueryCollection((q) =>
272+
q
273+
.from({ user: usersCollection })
274+
.orderBy(({ user }) => user.isActive, `desc`)
275+
.orderBy(({ user }) => user.age, `asc`)
276+
.limit(10)
277+
.offset(0),
278+
)
279+
280+
const page2 = createLiveQueryCollection((q) =>
281+
q
282+
.from({ user: usersCollection })
283+
.orderBy(({ user }) => user.isActive, `desc`)
284+
.orderBy(({ user }) => user.age, `asc`)
285+
.limit(10)
286+
.offset(10),
287+
)
288+
289+
await page1.preload()
290+
await waitForQueryData(page1, { minSize: 10 })
291+
await page2.preload()
292+
await waitForQueryData(page2, { minSize: 10 })
293+
294+
const page1Results = Array.from(page1.state.values())
295+
const page2Results = Array.from(page2.state.values())
296+
297+
expect(page1Results).toHaveLength(10)
298+
expect(page2Results).toHaveLength(10)
299+
300+
const page1Ids = new Set(page1Results.map((user) => user.id))
301+
for (const user of page2Results) {
302+
expect(page1Ids.has(user.id)).toBe(false)
303+
}
304+
305+
for (const page of [page1Results, page2Results]) {
306+
for (let i = 1; i < page.length; i++) {
307+
const prev = page[i - 1]!
308+
const curr = page[i]!
309+
310+
if (prev.isActive !== curr.isActive) {
311+
expect(prev.isActive ? 1 : 0).toBeGreaterThanOrEqual(
312+
curr.isActive ? 1 : 0,
313+
)
314+
} else {
315+
expect(prev.age).toBeLessThanOrEqual(curr.age)
316+
}
317+
}
318+
}
319+
320+
await page1.cleanup()
321+
await page2.cleanup()
322+
})
323+
324+
it(`should allow paging a second live query without affecting the first`, async () => {
325+
const config = await getConfig()
326+
const usersCollection = config.collections.onDemand.users
327+
328+
const baseQuery = createLiveQueryCollection((q) =>
329+
q
330+
.from({ user: usersCollection })
331+
.orderBy(({ user }) => user.isActive, `desc`)
332+
.orderBy(({ user }) => user.age, `asc`)
333+
.limit(10)
334+
.offset(0),
335+
)
336+
337+
const pagedQuery = createLiveQueryCollection((q) =>
338+
q
339+
.from({ user: usersCollection })
340+
.orderBy(({ user }) => user.isActive, `desc`)
341+
.orderBy(({ user }) => user.age, `asc`)
342+
.limit(10)
343+
.offset(0),
344+
)
345+
346+
await baseQuery.preload()
347+
await waitForQueryData(baseQuery, { minSize: 10 })
348+
await pagedQuery.preload()
349+
await waitForQueryData(pagedQuery, { minSize: 10 })
350+
351+
const baseIds = new Set(
352+
Array.from(baseQuery.state.values()).map((user) => user.id),
353+
)
354+
355+
const moveResult = pagedQuery.utils.setWindow({ offset: 10, limit: 10 })
356+
if (moveResult !== true) {
357+
await moveResult
358+
}
359+
await waitForQueryData(pagedQuery, { minSize: 10 })
360+
361+
const pagedResults = Array.from(pagedQuery.state.values())
362+
expect(pagedResults).toHaveLength(10)
363+
for (const user of pagedResults) {
364+
expect(baseIds.has(user.id)).toBe(false)
365+
}
366+
367+
await baseQuery.cleanup()
368+
await pagedQuery.cleanup()
369+
})
370+
267371
it(`should handle multi-column orderBy with duplicate values in first column`, async () => {
268372
const config = await getConfig()
269373
const usersCollection = config.collections.onDemand.users

‎packages/db-collection-e2e/src/suites/predicates.suite.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ export function createPredicatesTestSuite(
4242
)
4343

4444
await query.preload()
45-
4645
await waitForQueryData(query, { minSize: 1 })
4746

4847
const results = Array.from(query.state.values())

‎packages/db/src/collection/changes.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,12 @@ export class CollectionChangesManager<
135135
}
136136

137137
if (options.includeInitialState) {
138-
subscription.requestSnapshot({ trackLoadSubsetPromise: false })
138+
subscription.requestSnapshot({
139+
trackLoadSubsetPromise: false,
140+
orderBy: options.orderBy,
141+
limit: options.limit,
142+
onLoadSubsetResult: options.onLoadSubsetResult,
143+
})
139144
} else if (options.includeInitialState === false) {
140145
// When explicitly set to false (not just undefined), mark all state as "seen"
141146
// so that all future changes (including deletes) pass through unfiltered.

‎packages/db/src/collection/lifecycle.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,21 @@ export class CollectionLifecycleManager<
264264
}
265265

266266
this.hasBeenReady = false
267+
268+
// Call any pending onFirstReady callbacks before clearing them.
269+
// This ensures preload() promises resolve during cleanup instead of hanging.
270+
const callbacks = [...this.onFirstReadyCallbacks]
267271
this.onFirstReadyCallbacks = []
272+
callbacks.forEach((callback) => {
273+
try {
274+
callback()
275+
} catch (error) {
276+
console.error(
277+
`${this.config.id ? `[${this.config.id}] ` : ``}Error in onFirstReady callback during cleanup:`,
278+
error,
279+
)
280+
}
281+
})
268282

269283
// Set status to cleaned-up after everything is cleaned up
270284
// This fires the status:change event to notify listeners

‎packages/db/src/collection/subscription.ts‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ type RequestSnapshotOptions = {
2828
orderBy?: OrderBy
2929
/** Optional limit to pass to loadSubset for backend optimization */
3030
limit?: number
31+
/** Callback that receives the raw loadSubset result for external tracking */
32+
onLoadSubsetResult?: (result: Promise<void> | true) => void
3133
}
3234

3335
type RequestLimitedSnapshotOptions = {
@@ -37,6 +39,10 @@ type RequestLimitedSnapshotOptions = {
3739
minValues?: Array<unknown>
3840
/** Row offset for offset-based pagination (passed to sync layer) */
3941
offset?: number
42+
/** Whether to track the loadSubset promise on this subscription (default: true) */
43+
trackLoadSubsetPromise?: boolean
44+
/** Callback that receives the raw loadSubset result for external tracking */
45+
onLoadSubsetResult?: (result: Promise<void> | true) => void
4046
}
4147

4248
type CollectionSubscriptionOptions = {
@@ -365,6 +371,9 @@ export class CollectionSubscription
365371
}
366372
const syncResult = this.collection._sync.loadSubset(loadOptions)
367373

374+
// Pass the raw loadSubset result to the caller for external tracking
375+
opts?.onLoadSubsetResult?.(syncResult)
376+
368377
// Track this loadSubset call so we can unload it later
369378
this.loadedSubsets.push(loadOptions)
370379

@@ -416,6 +425,8 @@ export class CollectionSubscription
416425
limit,
417426
minValues,
418427
offset,
428+
trackLoadSubsetPromise: shouldTrackLoadSubsetPromise = true,
429+
onLoadSubsetResult,
419430
}: RequestLimitedSnapshotOptions) {
420431
if (!limit) throw new Error(`limit is required`)
421432

@@ -598,9 +609,14 @@ export class CollectionSubscription
598609
}
599610
const syncResult = this.collection._sync.loadSubset(loadOptions)
600611

612+
// Pass the raw loadSubset result to the caller for external tracking
613+
onLoadSubsetResult?.(syncResult)
614+
601615
// Track this loadSubset call
602616
this.loadedSubsets.push(loadOptions)
603-
this.trackLoadSubsetPromise(syncResult)
617+
if (shouldTrackLoadSubsetPromise) {
618+
this.trackLoadSubsetPromise(syncResult)
619+
}
604620
}
605621

606622
// TODO: also add similar test but that checks that it can also load it from the collection's loadSubset function
@@ -671,13 +687,21 @@ export class CollectionSubscription
671687

672688
for (const change of changes) {
673689
if (change.type === `delete`) {
674-
// Remove deleted keys from sentKeys so future re-inserts are allowed
675690
this.sentKeys.delete(change.key)
676691
} else {
677-
// For inserts and updates, track the key as sent
678692
this.sentKeys.add(change.key)
679693
}
680694
}
695+
696+
// Keep the limited snapshot offset in sync with keys we've actually sent.
697+
// This matters when loadSubset resolves asynchronously and requestLimitedSnapshot
698+
// didn't have local rows to count yet.
699+
if (this.orderByIndex) {
700+
this.limitedSnapshotRowCount = Math.max(
701+
this.limitedSnapshotRowCount,
702+
this.sentKeys.size,
703+
)
704+
}
681705
}
682706

683707
/**

‎packages/db/src/query/live/collection-config-builder.ts‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -830,17 +830,16 @@ export class CollectionConfigBuilder<
830830
return
831831
}
832832

833+
const subscribedToAll = this.currentSyncState?.subscribedToAllCollections
834+
const allReady = this.allCollectionsReady()
835+
const isLoading = this.liveQueryCollection?.isLoadingSubset
833836
// Mark ready when:
834837
// 1. All subscriptions are set up (subscribedToAllCollections)
835838
// 2. All source collections are ready
836839
// 3. The live query collection is not loading subset data
837840
// This prevents marking the live query ready before its data is processed
838841
// (fixes issue where useLiveQuery returns isReady=true with empty data)
839-
if (
840-
this.currentSyncState?.subscribedToAllCollections &&
841-
this.allCollectionsReady() &&
842-
!this.liveQueryCollection?.isLoadingSubset
843-
) {
842+
if (subscribedToAll && allReady && !isLoading) {
844843
markReady()
845844
}
846845
}

0 commit comments

Comments
 (0)