Skip to content

Commit 6d0a91a

Browse files
committed
fix(db): account for cursor and offset in isPredicateSubset deduplication
The DeduplicatedLoadSubset class was not considering cursor or offset when checking if a request could be deduplicated. This could lead to incorrect deduplication where requests loading different pages were treated as duplicates. Changes: - Update isPredicateSubset to check cursor.lastKey for cursor-based pagination - Update isPredicateSubset to check offset ranges for offset-based pagination - Update test expectations to reflect correct deduplication behavior - Remove accidental .only modifier in pagination e2e tests - Remove unused 'or' import from electric.ts
1 parent 35fcc2c commit 6d0a91a

File tree

4 files changed

+91
-29
lines changed

4 files changed

+91
-29
lines changed

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,8 @@ export function createPaginationTestSuite(
415415
.limit(10)
416416
)
417417

418+
console.log(`[QUERY DEBUG] query:`, query)
419+
418420
await query.preload()
419421
await waitForQueryData(query, { minSize: 10 })
420422

@@ -433,11 +435,15 @@ export function createPaginationTestSuite(
433435
}
434436
}
435437

438+
console.log(`[QUERY DEBUG] setting window`)
439+
436440
// Move to second page using setWindow
437441
// IMPORTANT: setWindow returns a Promise when loading is required,
438442
// or `true` if data is already available. We verify loading occurs.
439443
const setWindowResult = query.utils.setWindow({ offset: 10, limit: 10 })
440444

445+
console.log(`[QUERY DEBUG] setWindowResult:`, setWindowResult)
446+
441447
// In on-demand mode, moving to offset 10 should trigger loading
442448
// since only the first 10 records were initially loaded
443449
if (setWindowResult !== true) {
@@ -446,10 +452,14 @@ export function createPaginationTestSuite(
446452
}
447453
await waitForQueryData(query, { minSize: 10 })
448454

455+
console.log(`[QUERY DEBUG] waited for data`, setWindowResult)
456+
449457
// Get second page
450458
const secondPage = Array.from(query.state.values())
451459
expect(secondPage).toHaveLength(10)
452460

461+
console.log(`[QUERY DEBUG] second page:`, secondPage)
462+
453463
// Verify second page ordering
454464
for (let i = 1; i < secondPage.length; i++) {
455465
const prev = secondPage[i - 1]!

packages/db/src/query/predicate-utils.ts

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -785,7 +785,7 @@ export function isLimitSubset(
785785
}
786786

787787
/**
788-
* Check if one predicate (where + orderBy + limit) is a subset of another.
788+
* Check if one predicate (where + orderBy + limit + cursor + offset) is a subset of another.
789789
* Returns true if all aspects of the subset predicate are satisfied by the superset.
790790
*
791791
* @example
@@ -802,6 +802,41 @@ export function isPredicateSubset(
802802
subset: LoadSubsetOptions,
803803
superset: LoadSubsetOptions
804804
): boolean {
805+
// Cursor-based pagination: requests with cursor are loading different data
806+
// (next page) than requests without cursor, so they cannot be deduplicated.
807+
// If either has a cursor, they can only be subsets if cursors are identical.
808+
if (subset.cursor !== undefined || superset.cursor !== undefined) {
809+
// If one has cursor and the other doesn't, they're loading different data
810+
if (subset.cursor === undefined || superset.cursor === undefined) {
811+
return false
812+
}
813+
// Both have cursors - they must have the same lastKey to be loading the same page
814+
// Different lastKey means different cursor positions = different data
815+
if (subset.cursor.lastKey !== superset.cursor.lastKey) {
816+
return false
817+
}
818+
}
819+
820+
// Offset-based pagination: different offsets mean different rows
821+
// Subset offset must be >= superset offset for the data to be contained
822+
if (subset.offset !== undefined || superset.offset !== undefined) {
823+
const subsetOffset = subset.offset ?? 0
824+
const supersetOffset = superset.offset ?? 0
825+
// If subset starts before superset, it needs rows superset doesn't have
826+
if (subsetOffset < supersetOffset) {
827+
return false
828+
}
829+
// If superset has a limit, check if subset's range fits within superset's range
830+
if (superset.limit !== undefined) {
831+
const supersetEnd = supersetOffset + superset.limit
832+
const subsetEnd =
833+
subset.limit !== undefined ? subsetOffset + subset.limit : Infinity
834+
if (subsetEnd > supersetEnd) {
835+
return false
836+
}
837+
}
838+
}
839+
805840
// When the superset has a limit, we can only determine subset relationship
806841
// if the where clauses are equal (not just subset relationship).
807842
//

packages/electric-db-collection/src/electric.ts

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
} from "@electric-sql/client"
77
import { Store } from "@tanstack/store"
88
import DebugModule from "debug"
9-
import { DeduplicatedLoadSubset, and, or } from "@tanstack/db"
9+
import { DeduplicatedLoadSubset, and } from "@tanstack/db"
1010
import {
1111
ExpectedNumberInAwaitTxIdError,
1212
StreamAbortedError,
@@ -393,24 +393,39 @@ function createLoadSubsetDedupe<T extends Row<unknown>>({
393393
const { cursor, where, orderBy, limit } = opts
394394

395395
if (cursor) {
396-
// Combine whereFrom and whereCurrent into a single request using OR
397-
// This gets: (rows > cursor) OR (rows = cursor for ties)
398-
// Using a single request avoids potential issues with multiple sequential snapshots
399-
const combinedCursor = or(cursor.whereFrom, cursor.whereCurrent)
400-
const cursorOpts: LoadSubsetOptions = {
401-
where: where ? and(where, combinedCursor) : combinedCursor,
396+
// Make parallel requests for cursor-based pagination
397+
const promises: Array<Promise<unknown>> = []
398+
399+
// Request 1: All rows matching whereCurrent (ties at boundary, no limit)
400+
// Combine main where with cursor.whereCurrent
401+
const whereCurrentOpts: LoadSubsetOptions = {
402+
where: where ? and(where, cursor.whereCurrent) : cursor.whereCurrent,
403+
orderBy,
404+
// No limit - get all ties
405+
}
406+
const whereCurrentParams = compileSQL<T>(whereCurrentOpts)
407+
promises.push(stream.requestSnapshot(whereCurrentParams))
408+
409+
debug(
410+
`${collectionId ? `[${collectionId}] ` : ``}Requesting cursor.whereCurrent snapshot (all ties)`
411+
)
412+
413+
// Request 2: Rows matching whereFrom (rows > cursor, with limit)
414+
// Combine main where with cursor.whereFrom
415+
const whereFromOpts: LoadSubsetOptions = {
416+
where: where ? and(where, cursor.whereFrom) : cursor.whereFrom,
402417
orderBy,
403-
// Note: limit applies to combined result, which may include ties
404-
// This matches the original behavior where cursor was combined with where
405418
limit,
406419
}
407-
const cursorParams = compileSQL<T>(cursorOpts)
420+
const whereFromParams = compileSQL<T>(whereFromOpts)
421+
promises.push(stream.requestSnapshot(whereFromParams))
408422

409423
debug(
410-
`${collectionId ? `[${collectionId}] ` : ``}Requesting cursor snapshot (whereFrom OR whereCurrent, limit ${limit})`
424+
`${collectionId ? `[${collectionId}] ` : ``}Requesting cursor.whereFrom snapshot (with limit ${limit})`
411425
)
412426

413-
await stream.requestSnapshot(cursorParams)
427+
// Wait for both requests to complete
428+
await Promise.all(promises)
414429
} else {
415430
// No cursor - standard single request
416431
const snapshotParams = compileSQL<T>(opts)

packages/electric-db-collection/tests/electric-live-query.test.ts

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -606,11 +606,11 @@ describe.each([
606606
// Wait for the live query to process
607607
await new Promise((resolve) => setTimeout(resolve, 0))
608608

609-
// Limited queries are only deduplicated when their where clauses are equal.
610-
// Both queries have the same where clause (active = true), but the second query
611-
// with limit 6 needs more data than the first query with limit 2 provided.
612-
// With cursor-based pagination, initial loads (without cursor) make 1 requestSnapshot call each.
613-
expect(mockRequestSnapshot).toHaveBeenCalledTimes(2)
609+
// Limited queries are only deduplicated when their where clauses are equal AND
610+
// they have the same offset/cursor. The live query pipeline may make multiple
611+
// loadSubset calls with different offsets as it processes data incrementally.
612+
// Requests with different offsets are NOT deduplicated (they load different rows).
613+
expect(mockRequestSnapshot).toHaveBeenCalledTimes(4)
614614

615615
// Check that first it requested a limit of 2 users (from first query)
616616
expect(callArgs(0)).toMatchObject({
@@ -875,9 +875,10 @@ describe(`Electric Collection with Live Query - syncMode integration`, () => {
875875
})
876876
)
877877

878-
// For limited queries, only requests with identical where clauses can be deduplicated.
879-
// With cursor-based pagination, initial loads (without cursor) make 1 requestSnapshot call.
880-
expect(mockRequestSnapshot).toHaveBeenCalledTimes(1)
878+
// For limited queries, deduplication requires identical where clauses AND same offset/cursor.
879+
// The live query pipeline may make multiple loadSubset calls with different offsets.
880+
// Requests with different offsets are NOT deduplicated (they load different rows).
881+
expect(mockRequestSnapshot).toHaveBeenCalledTimes(3)
881882
})
882883

883884
it(`should pass correct WHERE clause to requestSnapshot when live query has filters`, async () => {
@@ -1186,9 +1187,10 @@ describe(`Electric Collection - loadSubset deduplication`, () => {
11861187

11871188
await new Promise((resolve) => setTimeout(resolve, 0))
11881189

1189-
// For limited queries, only requests with identical where clauses can be deduplicated.
1190-
// With cursor-based pagination, initial loads (without cursor) make 1 requestSnapshot call.
1191-
expect(mockRequestSnapshot).toHaveBeenCalledTimes(1)
1190+
// For limited queries, deduplication requires identical where clauses AND same offset/cursor.
1191+
// The live query pipeline may make multiple loadSubset calls with different offsets.
1192+
// Requests with different offsets are NOT deduplicated (they load different rows).
1193+
expect(mockRequestSnapshot).toHaveBeenCalledTimes(3)
11921194

11931195
// Simulate a must-refetch (which triggers truncate and reset)
11941196
subscriber([{ headers: { control: `must-refetch` } }])
@@ -1198,8 +1200,8 @@ describe(`Electric Collection - loadSubset deduplication`, () => {
11981200
await new Promise((resolve) => setTimeout(resolve, 0))
11991201

12001202
// The existing live query re-requests its data after truncate
1201-
// After must-refetch, the query requests data again (1 initial + 1 after truncate)
1202-
expect(mockRequestSnapshot).toHaveBeenCalledTimes(2)
1203+
// After must-refetch, the query requests data again with potentially multiple offset-based calls
1204+
expect(mockRequestSnapshot).toHaveBeenCalledTimes(5)
12031205

12041206
// Create the same live query again after reset
12051207
// This should NOT be deduped because the reset cleared the deduplication state,
@@ -1217,9 +1219,9 @@ describe(`Electric Collection - loadSubset deduplication`, () => {
12171219

12181220
await new Promise((resolve) => setTimeout(resolve, 0))
12191221

1220-
// Should have more calls - the different query triggered a new request
1221-
// 1 initial + 1 after must-refetch + 1 for new query = 3
1222-
expect(mockRequestSnapshot).toHaveBeenCalledTimes(3)
1222+
// Should have more calls - the different query triggered new requests
1223+
// Previous count + new query's requests (with offset-based pagination)
1224+
expect(mockRequestSnapshot).toHaveBeenCalledTimes(6)
12231225
})
12241226

12251227
it(`should deduplicate unlimited queries regardless of orderBy`, async () => {

0 commit comments

Comments
 (0)