Skip to content

Commit 1b832ff

Browse files
samwillissamwillis
authored
fix repeated renders when markReady called when collection already ready (#604)
* fix repeated renders when markReady called when already ready * changeset * tidy * fix the regression * address review
1 parent 51c6bc5 commit 1b832ff

4 files changed

Lines changed: 126 additions & 9 deletions

File tree

‎.changeset/polite-eels-laugh.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@tanstack/electric-db-collection": patch
3+
"@tanstack/db": patch
4+
---
5+
6+
Fix repeated renders when markReady called when the collection was already ready. This would occur after each long poll on an Electric collection.

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

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import {
22
CollectionInErrorStateError,
3+
CollectionStateError,
34
InvalidCollectionStatusTransitionError,
45
} from "../errors"
56
import {
@@ -90,7 +91,18 @@ export class CollectionLifecycleManager<
9091
* Safely update the collection status with validation
9192
* @private
9293
*/
93-
public setStatus(newStatus: CollectionStatus): void {
94+
public setStatus(
95+
newStatus: CollectionStatus,
96+
allowReady: boolean = false
97+
): void {
98+
if (newStatus === `ready` && !allowReady) {
99+
// setStatus('ready') is an internal method that should not be called directly
100+
// Instead, use markReady to transition to ready triggering the necessary events
101+
// and side effects.
102+
throw new CollectionStateError(
103+
`You can't directly call "setStatus('ready'). You must use markReady instead.`
104+
)
105+
}
94106
this.validateStatusTransition(this.status, newStatus)
95107
const previousStatus = this.status
96108
this.status = newStatus
@@ -129,9 +141,10 @@ export class CollectionLifecycleManager<
129141
* @private - Should only be called by sync implementations
130142
*/
131143
public markReady(): void {
144+
this.validateStatusTransition(this.status, `ready`)
132145
// Can transition to ready from loading or initialCommit states
133146
if (this.status === `loading` || this.status === `initialCommit`) {
134-
this.setStatus(`ready`)
147+
this.setStatus(`ready`, true)
135148

136149
// Call any registered first ready callbacks (only on first time becoming ready)
137150
if (!this.hasBeenReady) {
@@ -146,12 +159,11 @@ export class CollectionLifecycleManager<
146159
this.onFirstReadyCallbacks = []
147160
callbacks.forEach((callback) => callback())
148161
}
149-
}
150-
151-
// Always notify dependents when markReady is called, after status is set
152-
// This ensures live queries get notified when their dependencies become ready
153-
if (this.changes.changeSubscriptions.size > 0) {
154-
this.changes.emitEmptyReadyEvent()
162+
// Notify dependents when markReady is called, after status is set
163+
// This ensures live queries get notified when their dependencies become ready
164+
if (this.changes.changeSubscriptions.size > 0) {
165+
this.changes.emitEmptyReadyEvent()
166+
}
155167
}
156168
}
157169

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -644,7 +644,7 @@ export class CollectionStateManager<
644644

645645
// Ensure listeners are active before emitting this critical batch
646646
if (this.lifecycle.status !== `ready`) {
647-
this.lifecycle.setStatus(`ready`)
647+
this.lifecycle.markReady()
648648
}
649649
}
650650

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,11 @@ describe.each([
144144
subscriber(messages)
145145
}
146146

147+
function simulateUpToDateOnly() {
148+
// Send only an up-to-date message with no data changes
149+
subscriber([{ headers: { control: `up-to-date` } }])
150+
}
151+
147152
beforeEach(() => {
148153
electricCollection = createElectricUsersCollection()
149154
})
@@ -338,4 +343,98 @@ describe.each([
338343
expect(testElectricCollection.status).toBe(`ready`)
339344
expect(liveQuery.status).toBe(`ready`)
340345
})
346+
347+
it(`should not emit changes on up-to-date messages with no data changes`, async () => {
348+
// Test to verify that up-to-date messages without actual data changes
349+
// don't trigger unnecessary renders in live query collections
350+
351+
// Create a live query collection
352+
const liveQuery = createLiveQueryCollection({
353+
startSync: true,
354+
query: (q) =>
355+
q
356+
.from({ user: electricCollection })
357+
.where(({ user }) => eq(user.active, true))
358+
.select(({ user }) => ({
359+
id: user.id,
360+
name: user.name,
361+
active: user.active,
362+
})),
363+
})
364+
365+
// Track changes emitted by the live query
366+
const changeNotifications: Array<any> = []
367+
const subscription = liveQuery.subscribeChanges((changes) => {
368+
changeNotifications.push(changes)
369+
})
370+
371+
// Initial sync with data
372+
simulateInitialSync()
373+
expect(liveQuery.status).toBe(`ready`)
374+
expect(liveQuery.size).toBe(3) // Only active users
375+
376+
// Clear any initial change notifications
377+
changeNotifications.length = 0
378+
379+
// Send an up-to-date message with no data changes
380+
// This simulates the scenario where Electric sends up-to-date
381+
// but there are no actual changes to the data
382+
simulateUpToDateOnly()
383+
384+
// Wait a tick to ensure any async operations complete
385+
await new Promise((resolve) => setTimeout(resolve, 0))
386+
387+
// The live query should not have emitted any changes
388+
// because there were no actual data changes
389+
expect(changeNotifications).toHaveLength(0)
390+
391+
// Verify the collection is still in ready state
392+
expect(liveQuery.status).toBe(`ready`)
393+
expect(liveQuery.size).toBe(3)
394+
395+
// Clean up
396+
subscription.unsubscribe()
397+
})
398+
399+
it(`should not emit changes on multiple consecutive up-to-date messages with no data changes`, async () => {
400+
// Test to verify that multiple consecutive up-to-date messages
401+
// without data changes don't accumulate unnecessary renders
402+
403+
const liveQuery = createLiveQueryCollection({
404+
startSync: true,
405+
query: (q) => q.from({ user: electricCollection }),
406+
})
407+
408+
// Track changes emitted by the live query
409+
const changeNotifications: Array<any> = []
410+
const subscription = liveQuery.subscribeChanges((changes) => {
411+
changeNotifications.push(changes)
412+
})
413+
414+
// Initial sync
415+
simulateInitialSync()
416+
expect(liveQuery.status).toBe(`ready`)
417+
expect(liveQuery.size).toBe(4)
418+
419+
// Clear initial change notifications
420+
changeNotifications.length = 0
421+
422+
// Send multiple up-to-date messages with no data changes
423+
simulateUpToDateOnly()
424+
simulateUpToDateOnly()
425+
simulateUpToDateOnly()
426+
427+
// Wait for any async operations
428+
await new Promise((resolve) => setTimeout(resolve, 0))
429+
430+
// Should not have emitted any changes despite multiple up-to-date messages
431+
expect(changeNotifications).toHaveLength(0)
432+
433+
// Verify collection state is still correct
434+
expect(liveQuery.status).toBe(`ready`)
435+
expect(liveQuery.size).toBe(4)
436+
437+
// Clean up
438+
subscription.unsubscribe()
439+
})
341440
})

0 commit comments

Comments
 (0)