fix electric collection progressive mode #852
Conversation
🦋 Changeset detectedLatest commit: 1527028 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
samwillis
force-pushed
the
samwillis/fix-electric-progressive
branch
from
85a5bd7 to
b702326
Compare
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: 0 B Total Size: 85.8 kB ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 3.34 kB ℹ️ View Unchanged
|
samwillis
force-pushed
the
samwillis/fix-electric-progressive
branch
from
3941a16 to
132b57f
Compare
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
Looks great! All that work you did on truncate is coming in handy! Very nice semantics to be able to easily do this clean swap.
|
Mostly note to self during review:
--> Need to check that we also cancel in-flight fetchSnapshot requests and/or ignore fetchSnapshot requests that resolve after the swap. |
| const wrappedMarkReady = () => { | ||
| // Only create gate if we're in buffering phase (first up-to-date) | ||
| if ( | ||
| isBufferingInitialSync && |
There was a problem hiding this comment.
I don't like that we are closing over the isBufferingInitialSync which is actually define way down. I would rather explicitly pass it as an argument.
| return | ||
| // In progressive mode, use fetchSnapshot during snapshot phase | ||
| if (syncMode === `progressive`) { | ||
| if (hasReceivedUpToDate) { |
There was a problem hiding this comment.
aren't hasReceivedUpToDate and isBufferingInitialSync equivalent when the syncMode is 'progressive'?
There was a problem hiding this comment.
Essentially, i think isBufferingInitialSync should be a function that evaluates syncMode === 'progressive' && !hasReceivedUpToDate
| // Snapshot phase: fetch and apply immediately | ||
| const snapshotParams = compileSQL<T>(opts) | ||
| try { | ||
| const snapshot = await stream.fetchSnapshot(snapshotParams) |
There was a problem hiding this comment.
nit: since we don't use snapshot let's immediately destructure it const { data: rows } = ...
There was a problem hiding this comment.
I think there is a bug here. When we call fetchSnapshot we still didn't receive the up-to-date. But, when this call resolves it could be that we received the up-to-date message in the meantime. We should then ignore this snapshot.
We should have a unit test for this corner case.
| // Full sync complete, no need to load more | ||
| return | ||
| } | ||
| // Snapshot phase: fetch and apply immediately |
There was a problem hiding this comment.
I would extract the logic that handles the entire snapshot phase (L800-L830) into a separate function. This sync function is already massive and inlining it here only makes it worse. Small and clean functions will improve readability a lot here.
Depends on electric-sql/electric#3463, must have deps bumped to new electric client version when that is released.doneFix progressive mode to use fetchSnapshot and atomic swap
Progressive mode was broken because
requestSnapshot()injected snapshots into the stream in causally correct position, which didn't work properly with thefullmode stream. This release fixes progressive mode by:Core Changes:
fetchSnapshot()during initial sync to fetch and apply snapshots immediately in sync transactionsisBufferingInitialSync)up-to-date: truncate snapshot data → apply buffered messages → mark readyTest Infrastructure:
ELECTRIC_TEST_HOOKSsymbol for test control (hidden from public API)progressiveTestControl.releaseInitialSync()to E2E test config for explicit transition controlBug Fixes: