address review

Author: samwillis

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

@@ -83,12 +83,12 @@ type InferSchemaOutput<T> = T extends StandardSchemaV1
  *   - collection will be marked as ready once the sync is complete
  *   - there is no incremental sync
  * - `on-demand`:
- *   - syncs data synced in incremental snapshots as the collection is queried
+ *   - syncs data in incremental snapshots when the collection is queried
  *   - collection will be marked as ready immediately after the first snapshot is synced
  * - `progressive`:
- *   - syncs all data in the shape in the background
+ *   - syncs all data for the collection in the background
  *   - uses incremental snapshots during the initial sync to provide a fast path to the data required for queries
- *   - collection will be marked as ready once the initial sync is complete
+ *   - collection will be marked as ready once the full sync is complete
  */
 export type SyncMode = `eager` | `on-demand` | `progressive`
 
@@ -851,7 +851,7 @@ function createElectricSync<T extends Row<unknown>>(
       })
 
       // Only set onLoadMore if the sync mode is not eager, this indicates to the sync
-      // layer can load more data on demand via the requestSnapshot method when,
+      // layer that it can load more data on demand via the requestSnapshot method when,
       // the syncMode = `on-demand` or `progressive`
       const onLoadMore =
         syncMode === `eager`

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

@@ -471,25 +471,25 @@ describe.each([
       // Create a new electric collection with on-demand syncMode for this test
       vi.clearAllMocks()
 
-      const testSubscriber = vi.fn<(messages: Array<Message<User>>) => void>()
+      let testSubscriber: (messages: Array<Message<User>>) => void = () => {}
       mockSubscribe.mockImplementation((callback) => {
-        testSubscriber.mockImplementation(callback)
+        testSubscriber = callback
         return () => {}
       })
 
-      const testElectricCollection = createCollection({
-        ...electricCollectionOptions({
+      const testElectricCollection = createCollection(
+        electricCollectionOptions({
           id: `test-incremental-loading`,
           shapeOptions: {
             url: `http://test-url`,
             params: { table: `users` },
           },
           syncMode: `on-demand`,
           getKey: (user: User) => user.id,
-        }),
-        startSync: true,
-        autoIndex: `eager` as const,
-      })
+          startSync: true,
+          autoIndex: `eager` as const,
+        })
+      )
 
       mockRequestSnapshot.mockResolvedValue({
         data: [],
@@ -718,8 +718,15 @@ describe(`Electric Collection with Live Query - syncMode integration`, () => {
     // Wait for the live query to process
     await new Promise((resolve) => setTimeout(resolve, 0))
 
-    // Should have requested more data from Electric
-    expect(mockRequestSnapshot).toHaveBeenCalled()
+    // Should have requested more data from Electric with correct parameters
+    expect(mockRequestSnapshot).toHaveBeenCalledWith(
+      expect.objectContaining({
+        limit: 5, // Requests full limit from Electric
+        orderBy: `age NULLS FIRST`,
+        where: `active = $1`,
+        params: { 1: `true` }, // Parameters are stringified
+      })
+    )
     expect(liveQuery.size).toBeGreaterThan(2)
   })
 
@@ -757,8 +764,15 @@ describe(`Electric Collection with Live Query - syncMode integration`, () => {
     // Wait for the live query to process
     await new Promise((resolve) => setTimeout(resolve, 0))
 
-    // Should have requested more data from Electric
-    expect(mockRequestSnapshot).toHaveBeenCalled()
+    // Should have requested more data from Electric with correct parameters
+    // First request asks for the full limit
+    expect(mockRequestSnapshot).toHaveBeenCalledWith(
+      expect.objectContaining({
+        limit: 3, // Requests full limit from Electric
+        orderBy: `id NULLS FIRST`,
+        params: {},
+      })
+    )
   })
 
   it(`should NOT trigger requestSnapshot in eager mode even when live query needs more data`, async () => {