From 74fb1ade04b2a35a11b49b1967d7e0bccce021cf Mon Sep 17 00:00:00 2001 From: Sam Willis Date: Mon, 29 Sep 2025 19:08:56 +0100 Subject: [PATCH 1/5] fix repeated renders when markReady called when already ready --- packages/db/src/collection/lifecycle.ts | 10 +- .../tests/electric-live-query.test.ts | 113 ++++++++++++++++++ 2 files changed, 118 insertions(+), 5 deletions(-) diff --git a/packages/db/src/collection/lifecycle.ts b/packages/db/src/collection/lifecycle.ts index bdc207670..3ebceedf3 100644 --- a/packages/db/src/collection/lifecycle.ts +++ b/packages/db/src/collection/lifecycle.ts @@ -146,12 +146,12 @@ export class CollectionLifecycleManager< this.onFirstReadyCallbacks = [] callbacks.forEach((callback) => callback()) } - } - // Always notify dependents when markReady is called, after status is set - // This ensures live queries get notified when their dependencies become ready - if (this.changes.changeSubscriptions.size > 0) { - this.changes.emitEmptyReadyEvent() + // Notify dependents when markReady is called, after status is set + // This ensures live queries get notified when their dependencies become ready + if (this.changes.changeSubscriptions.size > 0) { + this.changes.emitEmptyReadyEvent() + } } } diff --git a/packages/electric-db-collection/tests/electric-live-query.test.ts b/packages/electric-db-collection/tests/electric-live-query.test.ts index 253a8fb12..3b3ff4f66 100644 --- a/packages/electric-db-collection/tests/electric-live-query.test.ts +++ b/packages/electric-db-collection/tests/electric-live-query.test.ts @@ -144,6 +144,11 @@ describe.each([ subscriber(messages) } + function simulateUpToDateOnly() { + // Send only an up-to-date message with no data changes + subscriber([{ headers: { control: `up-to-date` } }]) + } + beforeEach(() => { electricCollection = createElectricUsersCollection() }) @@ -338,4 +343,112 @@ describe.each([ expect(testElectricCollection.status).toBe(`ready`) expect(liveQuery.status).toBe(`ready`) }) + + it.only(`should not emit changes on up-to-date messages with no data changes`, async () => { + // Test to verify that up-to-date messages without actual data changes + // don't trigger unnecessary renders in live query collections + + // Create a live query collection + const liveQuery = createLiveQueryCollection({ + startSync: true, + query: (q) => + q + .from({ user: electricCollection }) + .where(({ user }) => eq(user.active, true)) + .select(({ user }) => ({ + id: user.id, + name: user.name, + active: user.active, + })), + }) + + // Track changes emitted by the live query + const changeNotifications: Array = [] + const subscription = liveQuery.subscribeChanges((changes) => { + changeNotifications.push(changes) + }) + + // Initial sync with data + simulateInitialSync() + expect(liveQuery.status).toBe(`ready`) + expect(liveQuery.size).toBe(3) // Only active users + + // Clear any initial change notifications + changeNotifications.length = 0 + + // Send an up-to-date message with no data changes + // This simulates the scenario where Electric sends up-to-date + // but there are no actual changes to the data + simulateUpToDateOnly() + + // Wait a tick to ensure any async operations complete + await new Promise((resolve) => setTimeout(resolve, 0)) + + // The live query should not have emitted any changes + // because there were no actual data changes + // BUG: Currently emits empty change arrays on up-to-date messages + if (changeNotifications.length > 0) { + console.log( + `BUG DETECTED: Live query emitted ${changeNotifications.length} change notifications on up-to-date with no data changes:`, + changeNotifications + ) + } + expect(changeNotifications).toHaveLength(0) + + // Verify the collection is still in ready state + expect(liveQuery.status).toBe(`ready`) + expect(liveQuery.size).toBe(3) + + // Clean up + subscription.unsubscribe() + }) + + it(`should not emit changes on multiple consecutive up-to-date messages with no data changes`, async () => { + // Test to verify that multiple consecutive up-to-date messages + // without data changes don't accumulate unnecessary renders + + const liveQuery = createLiveQueryCollection({ + startSync: true, + query: (q) => q.from({ user: electricCollection }), + }) + + // Track changes emitted by the live query + const changeNotifications: Array = [] + const subscription = liveQuery.subscribeChanges((changes) => { + changeNotifications.push(changes) + }) + + // Initial sync + simulateInitialSync() + expect(liveQuery.status).toBe(`ready`) + expect(liveQuery.size).toBe(4) + + // Clear initial change notifications + changeNotifications.length = 0 + + // Send multiple up-to-date messages with no data changes + simulateUpToDateOnly() + simulateUpToDateOnly() + simulateUpToDateOnly() + + // Wait for any async operations + await new Promise((resolve) => setTimeout(resolve, 0)) + + // Should not have emitted any changes despite multiple up-to-date messages + // BUG: Currently emits empty change arrays for each up-to-date message + if (changeNotifications.length > 0) { + console.log( + `BUG DETECTED: Live query emitted ${changeNotifications.length} change notifications on multiple up-to-date messages with no data changes:`, + changeNotifications + ) + } + expect(changeNotifications).toHaveLength(0) + + // Verify collection state is still correct + expect(liveQuery.status).toBe(`ready`) + expect(liveQuery.size).toBe(4) + + // Clean up + subscription.unsubscribe() + }) }) From b5754456ef406ccdceca34dc44741f82748e8e22 Mon Sep 17 00:00:00 2001 From: Sam Willis Date: Mon, 29 Sep 2025 19:10:31 +0100 Subject: [PATCH 2/5] changeset --- .changeset/polite-eels-laugh.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/polite-eels-laugh.md diff --git a/.changeset/polite-eels-laugh.md b/.changeset/polite-eels-laugh.md new file mode 100644 index 000000000..381432686 --- /dev/null +++ b/.changeset/polite-eels-laugh.md @@ -0,0 +1,6 @@ +--- +"@tanstack/electric-db-collection": patch +"@tanstack/db": patch +--- + +Fix repeated renders when markReady called when the collection was already ready. This would occur after each long poll on a Electric collection. From d854f2b69a3f68dc5f4d696ecf7de8fffa47155e Mon Sep 17 00:00:00 2001 From: Sam Willis Date: Mon, 29 Sep 2025 19:18:06 +0100 Subject: [PATCH 3/5] tidy --- .../tests/electric-live-query.test.ts | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/packages/electric-db-collection/tests/electric-live-query.test.ts b/packages/electric-db-collection/tests/electric-live-query.test.ts index 3b3ff4f66..e5ccd490d 100644 --- a/packages/electric-db-collection/tests/electric-live-query.test.ts +++ b/packages/electric-db-collection/tests/electric-live-query.test.ts @@ -386,13 +386,6 @@ describe.each([ // The live query should not have emitted any changes // because there were no actual data changes - // BUG: Currently emits empty change arrays on up-to-date messages - if (changeNotifications.length > 0) { - console.log( - `BUG DETECTED: Live query emitted ${changeNotifications.length} change notifications on up-to-date with no data changes:`, - changeNotifications - ) - } expect(changeNotifications).toHaveLength(0) // Verify the collection is still in ready state @@ -435,13 +428,6 @@ describe.each([ await new Promise((resolve) => setTimeout(resolve, 0)) // Should not have emitted any changes despite multiple up-to-date messages - // BUG: Currently emits empty change arrays for each up-to-date message - if (changeNotifications.length > 0) { - console.log( - `BUG DETECTED: Live query emitted ${changeNotifications.length} change notifications on multiple up-to-date messages with no data changes:`, - changeNotifications - ) - } expect(changeNotifications).toHaveLength(0) // Verify collection state is still correct From 72eef560c0b5e12f5ba7edb50a2a938046dbaa52 Mon Sep 17 00:00:00 2001 From: Sam Willis Date: Mon, 29 Sep 2025 20:07:38 +0100 Subject: [PATCH 4/5] fix the regression --- packages/db/src/collection/lifecycle.ts | 15 ++++++++++++--- packages/db/src/collection/state.ts | 2 +- .../tests/electric-live-query.test.ts | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/db/src/collection/lifecycle.ts b/packages/db/src/collection/lifecycle.ts index 3ebceedf3..7af8bd81d 100644 --- a/packages/db/src/collection/lifecycle.ts +++ b/packages/db/src/collection/lifecycle.ts @@ -1,5 +1,6 @@ import { CollectionInErrorStateError, + CollectionStateError, InvalidCollectionStatusTransitionError, } from "../errors" import { @@ -90,7 +91,15 @@ export class CollectionLifecycleManager< * Safely update the collection status with validation * @private */ - public setStatus(newStatus: CollectionStatus): void { + public setStatus( + newStatus: CollectionStatus, + allowReady: boolean = false + ): void { + if (newStatus === `ready` && !allowReady) { + throw new CollectionStateError( + `Cannot set status to anything other than ready, must use markReady instead` + ) + } this.validateStatusTransition(this.status, newStatus) const previousStatus = this.status this.status = newStatus @@ -129,9 +138,10 @@ export class CollectionLifecycleManager< * @private - Should only be called by sync implementations */ public markReady(): void { + this.validateStatusTransition(this.status, `ready`) // Can transition to ready from loading or initialCommit states if (this.status === `loading` || this.status === `initialCommit`) { - this.setStatus(`ready`) + this.setStatus(`ready`, true) // Call any registered first ready callbacks (only on first time becoming ready) if (!this.hasBeenReady) { @@ -146,7 +156,6 @@ export class CollectionLifecycleManager< this.onFirstReadyCallbacks = [] callbacks.forEach((callback) => callback()) } - // Notify dependents when markReady is called, after status is set // This ensures live queries get notified when their dependencies become ready if (this.changes.changeSubscriptions.size > 0) { diff --git a/packages/db/src/collection/state.ts b/packages/db/src/collection/state.ts index 10a7c0491..a077c0a4a 100644 --- a/packages/db/src/collection/state.ts +++ b/packages/db/src/collection/state.ts @@ -644,7 +644,7 @@ export class CollectionStateManager< // Ensure listeners are active before emitting this critical batch if (this.lifecycle.status !== `ready`) { - this.lifecycle.setStatus(`ready`) + this.lifecycle.markReady() } } diff --git a/packages/electric-db-collection/tests/electric-live-query.test.ts b/packages/electric-db-collection/tests/electric-live-query.test.ts index e5ccd490d..b387f1756 100644 --- a/packages/electric-db-collection/tests/electric-live-query.test.ts +++ b/packages/electric-db-collection/tests/electric-live-query.test.ts @@ -344,7 +344,7 @@ describe.each([ expect(liveQuery.status).toBe(`ready`) }) - it.only(`should not emit changes on up-to-date messages with no data changes`, async () => { + it(`should not emit changes on up-to-date messages with no data changes`, async () => { // Test to verify that up-to-date messages without actual data changes // don't trigger unnecessary renders in live query collections From 581ada61f0218e72cbeb585565b2913bc8862db3 Mon Sep 17 00:00:00 2001 From: Sam Willis Date: Mon, 29 Sep 2025 22:40:45 +0100 Subject: [PATCH 5/5] address review --- .changeset/polite-eels-laugh.md | 2 +- packages/db/src/collection/lifecycle.ts | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.changeset/polite-eels-laugh.md b/.changeset/polite-eels-laugh.md index 381432686..472390b3b 100644 --- a/.changeset/polite-eels-laugh.md +++ b/.changeset/polite-eels-laugh.md @@ -3,4 +3,4 @@ "@tanstack/db": patch --- -Fix repeated renders when markReady called when the collection was already ready. This would occur after each long poll on a Electric collection. +Fix repeated renders when markReady called when the collection was already ready. This would occur after each long poll on an Electric collection. diff --git a/packages/db/src/collection/lifecycle.ts b/packages/db/src/collection/lifecycle.ts index 7af8bd81d..f425f4d68 100644 --- a/packages/db/src/collection/lifecycle.ts +++ b/packages/db/src/collection/lifecycle.ts @@ -96,8 +96,11 @@ export class CollectionLifecycleManager< allowReady: boolean = false ): void { if (newStatus === `ready` && !allowReady) { + // setStatus('ready') is an internal method that should not be called directly + // Instead, use markReady to transition to ready triggering the necessary events + // and side effects. throw new CollectionStateError( - `Cannot set status to anything other than ready, must use markReady instead` + `You can't directly call "setStatus('ready'). You must use markReady instead.` ) } this.validateStatusTransition(this.status, newStatus)