Skip to content

Instantly share code, notes, and snippets.

@KyleAMathews
Created February 10, 2026 17:21
Show Gist options
  • Select an option

  • Save KyleAMathews/769caeeb219fea0b2cb2ae22e3aaaad9 to your computer and use it in GitHub Desktop.

Select an option

Save KyleAMathews/769caeeb219fea0b2cb2ae22e3aaaad9 to your computer and use it in GitHub Desktop.
TanStack/db community PR reviews & issue investigations (2026-02-10)

Now I have a complete picture. Let me write the investigation.


Summary

When consumers use declaration: true in their tsconfig.json (common with TS project references), TypeScript emits errors like "The inferred type of 'xyz' cannot be named without a reference to .../node_modules/@tanstack/db/dist/esm/query/builder/types". This happens specifically when using query joins and select. The root problem is that several types used in public method return signatures are not re-exported from the package's public API.

Root cause analysis

The BaseQueryBuilder class (packages/db/src/query/builder/index.ts) uses numerous types from ./types.ts in its public method return types, but only a small subset of those types are re-exported through the public API chain.

The export chain today:

File Types re-exported
packages/db/src/query/builder/index.ts:911-917 Context, Source, GetResult, RefLeaf as Ref, InferResultType
packages/db/src/query/index.ts:4-15 Same + ExtractContext, QueryResult, QueryBuilder, InitialQueryBuilder
packages/db/src/index.ts:11 Re-exports everything via export * from './query/index.js'

Types used in public return types but NOT exported:

Missing Type Used in method
SchemaFromSource<T> from() return type (line 139-140), all join() variants
MergeContextWithJoinType<...> join() return type (line 188-189), leftJoin(), rightJoin(), innerJoin(), fullJoin()
MergeContextForJoinCallback<...> join() callback parameter (line 185)
ResultTypeFromSelect<T> select() return type (line 475)
WithResult<...> select() return type (line 475)
JoinOnCallback<T> join() parameter type (line 184)
RefsForContext<T> select() callback parameter (line 474), where(), orderBy(), groupBy()
WhereCallback<T> where() parameter type
OrderByCallback<T> orderBy() parameter type
SelectObject<T> select() constraint
Prettify<T> Used transitively in multiple return types

When TypeScript tries to generate .d.ts files for a consumer, it cannot name these types because they aren't accessible through the package's public exports — hence the error.

Suggested fix

Add the missing types to the re-export chain at packages/db/src/query/builder/index.ts:910-917:

export type {
  Context,
  Source,
  GetResult,
  RefLeaf as Ref,
  InferResultType,
  // Add these to fix declaration emit:
  SchemaFromSource,
  MergeContextWithJoinType,
  MergeContextForJoinCallback,
  ResultTypeFromSelect,
  WithResult,
  JoinOnCallback,
  RefsForContext,
  WhereCallback,
  OrderByCallback,
  GroupByCallback,
  SelectObject,
  Prettify,
  ContextSchema,
  ApplyJoinOptionalityToMergedSchema,
} from './types.js'

Then similarly update packages/db/src/query/index.ts:4-15 and verify they flow through packages/db/src/index.ts (which already uses export * so it should pick them up automatically once query/index.ts exports them).

Verification approach: Set up a test project with declaration: true and composite: true in tsconfig, import the query builder, use from() + join() + select(), and confirm tsc --noEmit passes.

Effort estimate

Small — This is purely an additive change to type re-exports. No runtime code changes needed. The main work is identifying which types to export and verifying the fix with the reproduction case.

Suggested comment

Thanks for the clear repro, @marbemac! This is caused by several types (like SchemaFromSource, MergeContextWithJoinType, WithResult, etc.) being used in public method return types of BaseQueryBuilder but not re-exported from the package's public API. When declaration: true is set, TypeScript can't generate valid .d.ts files because it can't reference those internal types. The fix is to add the missing type re-exports at packages/db/src/query/builder/index.ts and propagate them through the export chain. This is similar to the issue TanStack Query had with unexported symbols (#8453 in that repo).

Now I have everything I need. Here's the investigation:


Summary

Users get a Missing getServerSnapshot runtime error when using useLiveQuery in Next.js App Router, even with "use client" directives. This is because Next.js still performs server-side rendering of client components, and React's useSyncExternalStore requires a third getServerSnapshot argument when rendering on the server. The reporter found an awkward workaround using next/dynamic to force purely client-side rendering.

Root Cause Analysis

The fix is straightforward. In packages/react-db/src/useLiveQuery.ts:483-486, the call to useSyncExternalStore only passes two arguments:

const snapshot = useSyncExternalStore(
  subscribeRef.current,
  getSnapshotRef.current,
  // ← missing 3rd argument: getServerSnapshot
)

React's useSyncExternalStore requires a third parameter — getServerSnapshot — when the component renders during SSR. Next.js App Router always SSRs "use client" components on the first request, so this omission causes the error.

This is the only call to useSyncExternalStore in the entire repo (confirmed via grep), so the fix surface is small.

★ Insight ───────────────────────────────────── A common misconception: "use client" in Next.js does not mean "skip SSR." It means "this is the client/server boundary" — the component still renders on the server during the initial HTML pass. Only next/dynamic with ssr: false truly skips server rendering, which is why the reporter's workaround works. This is the same pattern that tripped up many libraries adopting React 18's concurrent features — like the early days of Zustand and Jotai adapting to useSyncExternalStore. ─────────────────────────────────────────────────

Suggested Fix

Minimal fix — add a getServerSnapshot that returns a stable empty-state object at packages/react-db/src/useLiveQuery.ts:483:

// Stable server snapshot — on the server there is no collection data
const serverSnapshot = { collection: null, version: 0 }

const snapshot = useSyncExternalStore(
  subscribeRef.current,
  getSnapshotRef.current,
  () => serverSnapshot,    // ← add this
)

This returns { collection: null, version: 0 } during SSR, meaning useLiveQuery will return { data: undefined } on the server and then hydrate with real data on the client. The object must be defined outside the hook call (or at module scope) so referential identity is stable across renders.

Files to change:

  • packages/react-db/src/useLiveQuery.ts — line 483, add third argument to useSyncExternalStore

Full SSR support (prefetching, hydration) is a larger effort tracked separately in the SSR branch (claude/implement-issue-545-*), but the minimal fix above unblocks basic Next.js usage immediately.

Effort Estimate

Small — This is a one-line fix (plus one line for the stable snapshot constant). No new files, no API changes, no breaking changes. The downstream behavior is correct: queries return undefined during SSR and populate on the client.

Suggested Comment

Thanks for the detailed report! The root cause is that useLiveQuery calls useSyncExternalStore without the third getServerSnapshot argument, which React requires during SSR — and Next.js SSRs "use client" components by default. The fix is small: we need to provide a server snapshot that returns empty state. We'll get a fix out for this. Full SSR support with server-side prefetching is a separate, larger effort being tracked elsewhere.

Good. Now the full trace:

  1. Render #1: createLiveQueryCollection({ startSync: true, ... }) is called. The constructor calls startSync() (line 378-379 of index.ts), which sets the live query collection to loading. When the hook returns, snapshot.collection.status is loading. User sees: "loading".

  2. Between Render 1 and 2: The subscribeChanges callback at line 437 sets up the subscription. The addSubscriber() call won't re-trigger startSync since status is already loading. The sync function (syncFn) subscribes to the source Electric collection. The Electric collection starts loading data. The live query's graph does an initial run. At this point, the source collection isn't ready yet, so updateLiveQueryStatus doesn't call markReady.

  3. Render #2: But something bumps the version causing a second loading render. Let me trace what triggers onStoreChange between renders 1 and 2...

The syncFn calls scheduleGraphRun(loadSubsetDataCallbacks) at line 601. The maybeRunGraph at line 311 runs and at lines 350-353, if messagesCount === 0, it does begin() then commit(). The commit() writes to the live query collection, which triggers emitEvents on the change subscription, which calls onStoreChange() — giving us the second loading render.

Then later the source Electric collection becomes ready, which triggers handleSourceStatusChangeupdateLiveQueryStatusmarkReady()emitEmptyReadyEvent()onStoreChange()Render #3 with "ready".

So we get: loading → loading → ready (three renders, matching the user's report).

Summary

The user reports that useLiveQuery renders three times: loadingloadingready, even with reactStrictMode: false. This is not a React StrictMode issue — it's caused by the internal lifecycle of how live query collections initialize and subscribe to source collections.

Root cause analysis

There are two collections in play when using useLiveQuery with a query builder:

  1. The source Electric collection (workspaceContentCollection) — created by the user
  2. The live query collection — created internally by createLiveQueryCollection inside useLiveQuery

The three renders happen as follows:

Render 1 (loading): createLiveQueryCollection({ startSync: true }) is called during render (useLiveQuery.ts:389-393). The collection constructor synchronously calls startSync() (collection/index.ts:378-379), which sets status to loading (sync.ts:85). The first render reads snapshot.collection.status === 'loading'.

Render 2 (loading): The live query's syncFn (collection-config-builder.ts:551-639) subscribes to source collections and calls scheduleGraphRun() (line 601). Inside maybeRunGraph (line 311), when messagesCount === 0, it executes an empty begin() / commit() cycle (lines 350-353). This commit() triggers the change subscription callback, which bumps versionRef.current and calls onStoreChange() (useLiveQuery.ts:439-440). The source Electric collection hasn't reported ready yet, so the live query collection's status is still loading.

Render 3 (ready): The Electric collection finishes its initial sync and calls markReady(). The live query's handleSourceStatusChange (collection-config-builder.ts:793-820) calls updateLiveQueryStatusmarkReady() (lifecycle.ts:145-170), which calls emitEmptyReadyEvent() (line 167), triggering another onStoreChange().

The unnecessary render is Render 2 — the empty begin()/commit() in maybeRunGraph when messagesCount === 0 (line 350-353 of collection-config-builder.ts). This initial empty commit is meant to ensure the collection is initialized, but it triggers a re-render while still in loading state, producing a visually redundant update.

Suggested fix

The fix should prevent the empty begin()/commit() from notifying subscribers when the collection isn't ready yet. Two approaches:

  1. In collection-config-builder.ts:350-353: Guard the empty commit so it only runs after the collection is ready, or batch it with the readiness transition. Currently:

    if (syncState.messagesCount === 0) {
      begin()
      commit()
    }

    This could be deferred until markReady is about to be called, or the commit could use a flag to suppress change notifications while still in loading state.

  2. In useLiveQuery.ts:437-440: The subscribe callback could skip bumping the version / calling onStoreChange() when the collection status hasn't meaningfully changed (i.e., data hasn't actually changed and status is still loading). Something like checking if collection.status transitioned before bumping.

Approach 1 is cleaner — the empty commit at lines 350-353 serves no purpose before the collection is ready since there's no real data to show.

Key files:

  • packages/db/src/query/live/collection-config-builder.ts:350-353 — empty commit triggering redundant render
  • packages/react-db/src/useLiveQuery.ts:437-440 — change subscription callback
  • packages/db/src/collection/lifecycle.ts:145-170markReady() and emitEmptyReadyEvent()

Effort estimate

Small — The fix is localized to 1-2 files and the behavior is well-understood. The empty-commit guard or batching with readiness is straightforward.

Suggested comment

Thanks for the report! This is a known behavior — it's not related to React StrictMode but rather how the internal live query collection initializes. The extra loading render comes from an empty initialization commit that fires before the source Electric collection has reported ready. We're tracking this for a fix to batch that initialization with the readiness transition so you'll see just one loadingready cycle.

This confirms the bug. Here's my investigation:


Summary

When using usePacedMutations (with debounce/throttle/queue strategies), the UI briefly flickers back to the old (pre-mutation) value while the mutation is persisting. The optimistic update is visible while the transaction is pending, but once the mutationFn completes and the transaction transitions to completed, the optimistic state is dropped before the sync layer has delivered the server-confirmed data back into syncedData. This creates a visible regression window.

Root Cause Analysis

The bug is a timing gap between optimistic state removal and synced data arrival.

Here's the exact sequence:

  1. User calls mutate(newValue) — a Transaction is created in pending state, optimistic update applied via recomputeOptimisticState(). UI shows newValue. (paced-mutations.ts:142)

  2. Strategy timer fires → commitCallback() calls transaction.commit(). (paced-mutations.ts:118)

  3. Inside commit() (transactions.ts:472-518):

    • State set to persisting (line 477) — no touchCollection() call, so optimistic state remains. Good.
    • await mutationFn(...) executes — server write happens.
    • mutationFn returns.
    • State set to completed (line 495).
    • touchCollection() called (line 496).
  4. touchCollection()onTransactionStateChange()recomputeOptimisticState(false) (state.ts:891-899):

    • Iterates this.transactions — the now-completed transaction is excluded (state.ts:253).
    • optimisticUpserts is cleared of this transaction's mutations.
    • No other active transaction provides coverage for the key.
  5. get(key) now falls through to syncedData.get(key) (state.ts:114-115), which still holds the old pre-mutation value because the sync layer hasn't processed the server's confirmation yet.

  6. UI regresses to old value. Only when the sync response arrives and commitPendingTransactions() updates syncedData does the value become correct again.

★ Insight ───────────────────────────────────── This is the same fundamental problem documented in createOptimisticAction's JSDoc (optimistic-action.ts:14-17): "you must ensure that your server writes have synced back before you return [from mutationFn], as the optimistic state is dropped when you return." However, with paced mutations the user has less control — the commit is strategy-driven and the API doesn't make this requirement obvious. The createOptimisticAction docs explicitly tell users to await collection.utils.refetch() or awaitTxId() inside mutationFn, but paced mutations have no such guidance. ─────────────────────────────────────────────────

Why regular (non-paced) mutations don't have this problem: With createOptimisticAction, the mutationFn signature is (variables, params) => Promise, giving the user direct access to variables so they can construct a proper await-sync-back call. The documentation explicitly instructs them to do so. With paced mutations, users often write a simple fire-and-forget mutationFn because the pacing API suggests the framework handles the timing.

Suggested Fix

The most robust fix would be to keep the optimistic state for a completed transaction until the sync layer has confirmed the data. Two approaches:

Approach A: Add a completed-awaiting-sync state (Recommended)

In transactions.ts / state.ts, introduce logic so that when a transaction completes, it doesn't immediately drop its optimistic state. Instead:

  1. In recomputeOptimisticState() (state.ts:252-256), also include completed transactions whose mutations haven't yet been reflected in syncedData.
  2. When commitPendingTransactions() processes sync data that covers a completed transaction's mutations, then remove the transaction from the active set and trigger a final recompute.

Key files:

  • packages/db/src/collection/state.ts:252-256 — Change the filter to retain completed transactions until their keys are confirmed in sync.
  • packages/db/src/collection/state.ts:834-850 (scheduleTransactionCleanup) — Delay cleanup until sync confirmation.
  • packages/db/src/transactions.ts:495-496 — Potentially delay touchCollection() or add metadata about awaiting sync.

Approach B: Documentation + mutationFn guidance

Add clear documentation to createPacedMutations that mutationFn must await sync confirmation before returning (matching createOptimisticAction's contract). This is simpler but puts the burden on every user.

Approach C: Hold optimistic state until isPersisted + sync confirmation

In the commit() method, after mutationFn returns, don't transition to completed until the collection confirms the sync data includes the mutated keys. This could be done by having the transaction check for its mutations' keys in syncedData before completing.

I recommend Approach A as it fixes the problem at the framework level without requiring users to understand the sync timing contract.

Effort Estimate

Medium — The core logic change is isolated to recomputeOptimisticState() and scheduleTransactionCleanup() in state.ts, but getting the "when is sync confirmed" heuristic right requires careful thought about different collection backends (Electric, Query, etc.), and thorough testing for edge cases (failed syncs, overlapping transactions, rollbacks).

Suggested Comment

Thanks for the detailed repro @fcannizzaro! We've confirmed this is a timing issue: when a paced mutation's mutationFn completes, the transaction moves to completed and its optimistic state is immediately dropped — but the sync layer hasn't delivered the confirmed data yet, causing the brief regression to the old value. This is the same underlying issue as #904. We're looking at keeping the optimistic state until sync confirmation arrives, rather than dropping it at transaction completion.

Here's the investigation:


Summary

When a user installs @tanstack/query-db-collection alongside @tanstack/query, the package's module augmentation globally overrides the meta type on all TanStack Query queries — not just those created by Query Collections. This forces every meta: { ... } usage across the entire app to include the required loadSubsetOptions property, causing widespread TypeScript errors.

Root cause analysis

The problem is in packages/query-db-collection/src/global.ts:36-39:

declare module '@tanstack/query-core' {
  interface Register {
    queryMeta: QueryCollectionMeta  // ← globally replaces QueryMeta for ALL queries
  }
}

TanStack Query's Register interface is a global singleton — there can only be one queryMeta augmentation across the entire project. The derived QueryMeta type uses a conditional:

type QueryMeta = Register extends { queryMeta: infer TQueryMeta }
  ? TQueryMeta extends Record<string, unknown> ? TQueryMeta : Record<string, unknown>
  : Record<string, unknown>;

Because QueryCollectionMeta declares loadSubsetOptions: LoadSubsetOptions as a required property, every meta in every useQuery, useMutation, etc. across the app now requires that field. This is the "interface pollution" — a DB-specific required field leaking into the global query type system.

Suggested fix

Make loadSubsetOptions optional in the QueryCollectionMeta interface:

File: packages/query-db-collection/src/global.ts:31

 export interface QueryCollectionMeta extends Record<string, unknown> {
-  loadSubsetOptions: LoadSubsetOptions
+  loadSubsetOptions?: LoadSubsetOptions
 }

This is the simplest fix that resolves the issue: non-DB queries can omit loadSubsetOptions in their meta, while DB-originated queries still get it populated at runtime (line 671 of query.ts: const extendedMeta = { ...meta, loadSubsetOptions: opts }).

The trade-off is that ctx.meta.loadSubsetOptions inside query collection queryFn callbacks would become LoadSubsetOptions | undefined instead of LoadSubsetOptions. But the docs already recommend the pattern ctx.meta?.loadSubsetOptions (with optional chaining), and parseLoadSubsetOptions already handles undefined input — so this is fully compatible with existing usage. The type test file (packages/query-db-collection/tests/query.test-d.ts) would need minor updates to expect the optional type.

An alternative approach would be to remove the module augmentation entirely and instead type meta locally within queryCollectionOptions using generics. This would be cleaner architecturally but is a larger change that requires reworking how queryFn's context is typed within the collection abstraction.

Effort estimate

Small — Making loadSubsetOptions optional is a one-line change plus test updates. The alternative (removing module augmentation entirely) would be Medium.

Suggested comment

Thanks for reporting this, @Nick-Motion. You're right that the required loadSubsetOptions field on the global Register.queryMeta augmentation bleeds into all TanStack Query usage. The simplest fix is making loadSubsetOptions optional in QueryCollectionMeta — the runtime code and parseLoadSubsetOptions already handle undefined gracefully. We'll get a PR up for this.

Now I understand the full bug. Here's the investigation:


Summary

When using syncMode: 'on-demand' collections in a join query, the "lazy" side of the join never receives data. The workaround is to separately query the lazy collection first to trigger its sync. This is because the lazy alias subscription skips calling requestSnapshot() (and therefore loadSubset()) during initial setup, so on-demand collections never fetch data from the backend.

Root Cause Analysis

There are two compounding issues in how lazy aliases interact with on-demand sync:

Issue 1: loadSubset never called for lazy on-demand collections

The chain of events:

  1. joins.ts:248-249 — During join compilation, one side is marked as "lazy" via lazySources.add(lazyAlias). For a leftJoin, the right/joined collection is lazy.

  2. collection-subscriber.ts:110-113 — When subscribing, includeInitialState is set to false for lazy aliases:

    const includeInitialState = !this.collectionConfigBuilder.isLazyAlias(this.alias)
  3. collection-subscriber.ts:241 — The false value is conditionally spread, resulting in includeInitialState being undefined (not false) in the options:

    ...(includeInitialState && { includeInitialState }),
  4. changes.ts:137-148 — Since options.includeInitialState is undefined:

    • The if (options.includeInitialState) branch (which calls requestSnapshot()loadSubset()) is skipped
    • The else if (options.includeInitialState === false) branch (which calls markAllStateAsSeen()) is also skipped

    For eager sync collections, this is fine — data arrives via push. For on-demand collections, data only arrives when explicitly requested via loadSubset(), which never happens.

Issue 2: The tap mechanism partially compensates but has a filtering problem

The tap operator at joins.ts:280-317 fires when active-side data flows and calls requestSnapshot() on the lazy subscription. This does call loadSubset() (at subscription.ts:372), so data is fetched from the backend.

However, because markAllStateAsSeen() was never called (Issue 1, point 4), skipFiltering remains false on the lazy subscription. The filterAndFlipChanges method at subscription.ts:631-634 then filters out changes that weren't part of a prior snapshot. Since the subscription never received an initial snapshot, the data loaded by loadSubset may arrive as change events that get filtered out, preventing them from reaching the join pipeline.

The net result

Lazy on-demand collections in joins either never load data, or load it but filter it out before it reaches the query pipeline.

Suggested Fix

The fix should be in collection-subscriber.ts:230-241. For on-demand sync mode collections, even when includeInitialState is false (lazy alias), we still need to:

  1. Call loadSubset() to trigger the backend fetch — or at minimum, ensure the tap mechanism's requestSnapshot works correctly by calling markAllStateAsSeen() on the lazy subscription.

  2. The most targeted fix: In collection-subscriber.ts, when building the options for subscribeChanges, explicitly pass includeInitialState: false (not omit it) so that changes.ts:144 triggers markAllStateAsSeen(). This ensures the tap-driven requestSnapshotloadSubset path works correctly for on-demand collections:

    // Line 240-247 in collection-subscriber.ts
    const subscription = this.collection.subscribeChanges(sendChanges, {
      includeInitialState,  // Always pass the value (true or false), don't conditionally spread
      whereExpression,
      onStatusChange,
      orderBy: orderByForSubscription,
      limit: limitForSubscription,
      onLoadSubsetResult,
    })

    This single change ensures that when includeInitialState is false, markAllStateAsSeen() gets called, so when the tap operator later calls requestSnapshot() and loadSubset(), the resulting change events won't be filtered out.

Key files:

  • packages/db/src/query/live/collection-subscriber.ts — lines 230-247 (the conditional spread)
  • packages/db/src/collection/changes.ts — lines 137-148 (the includeInitialState branching)
  • packages/db/src/collection/subscription.ts — lines 631-634 (filterAndFlipChanges)
  • packages/db/src/query/compiler/joins.ts — lines 277-317 (the tap lazy loading mechanism)

Effort Estimate

Small — The core fix is a one-line change to pass includeInitialState explicitly rather than conditionally spreading it. However, testing across different join types (left, right, inner) with on-demand sync would be advisable.

Suggested Comment

Thanks for the detailed report and the clear reproduction, @ibarakaiev! We've traced this to a subtle interaction between the lazy alias subscription setup and on-demand sync — the lazy side's subscription doesn't call markAllStateAsSeen() because includeInitialState is omitted (undefined) rather than explicitly set to false, which prevents the join's lazy-loading tap mechanism from working correctly with on-demand collections. The fix looks straightforward — we'll get a PR up.

Now I have a clear picture. Let me write the investigation report.


Summary

The issue requests splitting the single schema property on collections into three separate schemas (selectSchema, insertSchema, updateSchema) to support different validation/transformation behavior for reads vs. writes. The core motivation is that schema transforms (e.g., z.string().transform(val => new Date(val))) work for client-side mutations (insert/update) but are never applied to data arriving from sync streams (e.g., ElectricSQL), leaving synced data in its raw form.

Root cause analysis

The schema is only applied in one direction — on client mutations — and completely bypassed on the sync read path:

  • Mutations path (packages/db/src/collection/mutations.ts:73-119): CollectionMutationsManager.validateData() calls schema['~standard'].validate(data) on every insert and update, applying transforms and defaults.
  • Sync path (packages/db/src/collection/sync.ts:159-164): The write() method in the sync transaction pushes messageWithOptionalKey.value directly into pendingTransaction.operations with zero schema validation or transformation. The raw value from the sync source flows untouched into syncedData.

This asymmetry is by design (performance, trust in backend data), but it means any Zod .transform(), .default(), or .coerce() calls in the schema have no effect on synced data. The schema field at packages/db/src/types.ts:513 in BaseCollectionConfig is used for type inference on the sync side but never for runtime transformation.

★ Insight ───────────────────────────────────── This is a common tension in local-first systems — like the difference between the Urim and Thummim serving distinct revelatory purposes, a single schema tries to serve two fundamentally different roles: validation of user intent (mutations) and deserialization of external truth (sync). The current design chose to trust the sync source, but that breaks down when the application type (Date) differs from the wire type (string). ─────────────────────────────────────────────────

Suggested fix

Rather than the full 3-schema split (which adds significant API surface), the minimum viable change is adding a selectSchema (or deserializationSchema) that transforms sync data. This could be implemented incrementally:

  1. packages/db/src/types.ts (~line 499-513): Add an optional selectSchema?: TSelectSchema to BaseCollectionConfig alongside the existing schema.

  2. packages/db/src/collection/sync.ts (~line 159-164): In the sync transaction's write() method, if config.selectSchema exists, run selectSchema['~standard'].validate(message.value) before pushing the operation. This transforms sync data (e.g., string→Date).

  3. packages/db/src/collection/mutations.ts: Optionally support separate insertSchema / updateSchema overrides, falling back to the existing schema. This is a nice-to-have and lower priority.

  4. Type updates: Add InferSelectSchemaOutput<T> type helper and thread the new generic through CollectionSyncManager, CollectionStateManager, and the Collection type itself so TOutput can differ between sync and mutation paths (or unify via selectSchema output).

  5. packages/electric-db-collection/src/electric.ts: Surface the new selectSchema option in electricCollectionOptions.

Key concern: The sync path is performance-sensitive and processes many messages. Schema validation on every sync message adds overhead. Consider making it opt-in (only runs if selectSchema is provided) and documenting the perf tradeoff. Also, Kyle already noted Electric has its own custom parsing — so the question is whether TanStack DB should duplicate that or defer to sync providers.

Effort estimate

Medium — The minimum viable change (add optional selectSchema applied in the sync write path) is relatively contained (~100-200 lines across types, sync, and one collection provider). The full 3-schema split with proper type inference threading would be Large due to the generic type cascade across the entire collection system.

Suggested comment

This is a reasonable request. The core issue is that schema transforms are only applied on the mutation path (CollectionMutationsManager.validateData) and bypassed entirely in the sync write path (CollectionSyncManager). Adding an optional selectSchema that transforms incoming sync data would be the minimal fix — similar to PowerSync's deserializationSchema. Worth noting that Electric's client already supports custom parsing at the stream level, so there's a question of whether this belongs in TanStack DB's core or in the sync provider layer. We're open to a PR if someone wants to take a crack at the selectSchema approach.

Now I have a complete picture. Here's the investigation:


Summary

When using electricCollectionOptions with syncMode: 'progressive', browser visibility changes (tab switch, minimize/restore) cause SyncTransactionAlreadyCommittedError or SyncTransactionAlreadyCommittedWriteError. The Electric client's visibilityHandler triggers resume_fn, which re-delivers messages through the stream subscriber. If the local transactionStarted flag is out of sync with the actual transaction state, the code attempts to write() or commit() against an already-committed transaction.

Root Cause Analysis

There are two bugs in packages/electric-db-collection/src/electric.ts, both related to how the transactionStarted flag tracks the actual transaction state:

Bug 1: Wrong transactionStarted value during atomic swap (line 1501)

During the progressive-mode atomic swap (lines 1460–1507), begin() is called at line 1470 to start a transaction, but transactionStarted is never set to true. When processMoveOutEvent is called at line 1497 with transactionStarted (which is false), it sees no transaction is active and calls begin() again (line 1120 in processMoveOutEvent), creating a second nested transaction. The subsequent commit() at line 1507 only commits the last transaction, leaving orphaned transaction state.

// Line 1470: begin() starts TX1 — but transactionStarted stays false
begin()
// ...
// Line 1497-1502: processMoveOutEvent gets transactionStarted=false
// If it needs to delete rows, it calls begin() AGAIN → TX2
processMoveOutEvent(bufferedMsg.headers.patterns, begin, write, transactionStarted)
// Line 1507: commit() commits TX2, TX1 is orphaned
commit()

Bug 2: transactionStarted reset AFTER commit() (lines 1519–1521)

In the normal commit path:

if (transactionStarted) {
  commit()                   // If this throws, next line never runs
  transactionStarted = false // Too late if commit() threw
}

If commit() throws (e.g., due to Bug 1 leaving stale transactions), transactionStarted stays true. The next batch of messages then skips begin() (since transactionStarted is already true) and calls write() on the committed transaction → SyncTransactionAlreadyCommittedWriteError.

How visibility change triggers it

  1. Initial progressive sync completes with atomic swap, potentially leaving orphaned transaction state (Bug 1)
  2. User switches tabs → Electric client pauses
  3. User returns → visibilityHandler calls resume_fn, re-subscribing to the shape stream
  4. New messages arrive and hit the subscriber callback
  5. The stale transactionStarted flag causes write()/commit() to target an already-committed transaction

Suggested Fix

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

Fix 1 (line 1501): Pass true instead of transactionStarted to processMoveOutEvent during the atomic swap, since begin() was already called at line 1470:

processMoveOutEvent(
  bufferedMsg.headers.patterns,
  begin,
  write,
  true,  // Transaction is already active from line 1470
)

Fix 2 (lines 1519–1521): Reset transactionStarted before calling commit() to prevent stale state on failure:

if (transactionStarted) {
  transactionStarted = false
  commit()
}

★ Insight ───────────────────────────────────── This is a classic state-synchronization bug — like when the Nephite record-keepers had to carefully track which plates were current vs. sealed. The transactionStarted boolean is a shadow of the real transaction state inside sync.ts, and the two got out of alignment. A more robust design would track transaction state in one place (e.g., expose an isTransactionActive() method from the sync layer) rather than maintaining a separate boolean. But the minimal fix above addresses the immediate issue. ─────────────────────────────────────────────────

Effort Estimate

Small — Two targeted line changes in a single file, plus a test for the visibility-resume scenario in progressive mode.

Suggested Comment

Thanks for the detailed report and stack trace @sudoskys — this was very helpful for pinpointing the issue. We've identified two bugs in the transactionStarted flag management in electric.ts: (1) during the progressive-mode atomic swap, processMoveOutEvent is passed false even though a transaction is already active (line 1501), which can create a duplicate transaction; and (2) transactionStarted is reset after commit() instead of before (line 1519-1521), so if anything goes wrong the flag stays stale. Both issues compound when a visibility change triggers resume_fn and re-delivers messages. We're working on a fix — the changes are small and targeted.

I now have a thorough understanding of the issue. Here's the investigation:


Summary

In React StrictMode (dev mode), useLiveQuery creates 2 subscribers on mount but only cleans up 1 on unmount, leaving an orphaned subscriber that keeps the collection syncing permanently. The activeSubscribersCount never returns to 0, so garbage collection never triggers even after the component is gone. This affects all hooks built on useLiveQuery including useLiveInfiniteQuery and useLiveSuspenseQuery.

Root cause analysis

★ Insight ───────────────────────────────────── React StrictMode double-invokes render functions to surface side effects. useSyncExternalStore is designed to handle this — it calls subscribe, then calls the returned unsubscribe, then calls subscribe again. The problem here is that the collection creation also happens during render, which useSyncExternalStore has no awareness of. ─────────────────────────────────────────────────

There are two interleaved problems, both in packages/react-db/src/useLiveQuery.ts:

Problem 1: Collection creation during render (lines 351–418)

When needsNewCollection is true, the hook creates a new collection during render with startSync: true (line 391) or calls startSyncImmediate() (line 369/384). In StrictMode's first render pass, a collection is created and stored in collectionRef.current. React then throws away this render, but the collection persists in the ref. On the second render, needsNewCollection is false (because collectionRef.current already exists and deps match), so the same collection is reused. No collection leaks here — but it sets up the subscription problem.

Problem 2: Subscription via useSyncExternalStore (lines 426–486)

The subscribe function (lines 431–450) calls collectionRef.current.subscribeChanges() — which increments activeSubscribersCount in packages/db/src/collection/changes.ts:161. The StrictMode lifecycle goes:

  1. First subscribe callsubscribeChanges() called → activeSubscribersCount = 1
  2. StrictMode cleanupsubscription.unsubscribe() called → activeSubscribersCount = 0
  3. Second subscribe callsubscribeChanges() called → activeSubscribersCount = 1

So far so good — useSyncExternalStore handles its own cleanup correctly during mount. However, the commenter @hugiex and the reporter both observe +2 / -1 behavior. This suggests the actual behavior may differ from the spec in certain React versions, or there's a timing issue where subscribeChanges() triggers an immediate onStoreChange callback (line 443–445: when status === 'ready'), causing React to re-subscribe before the first unsubscribe completes.

The most likely concrete scenario: when the collection is already ready at subscribe time, the immediate onStoreChange() call (line 445) triggers a React re-render inside the subscribe callback. This can cause useSyncExternalStore to call subscribe a second time before the first subscription's cleanup runs, resulting in 2 live subscriptions but only 1 cleanup path tracked.

Suggested fix

The fix should move subscription side effects out of the render phase. Two approaches:

Approach A (Recommended): Move subscribeChanges to be lazy/deferred

In useLiveQuery.ts lines 431–450, instead of calling subscribeChanges() directly in the subscribe callback, ensure the collection's sync is started only in the commit phase. The subscribe function passed to useSyncExternalStore is the commit phase (React guarantees it's called after commit), so the subscription itself is fine. The issue is the synchronous onStoreChange() call at lines 443–445 that fires during subscribe when the collection is already ready. This should be deferred:

// Instead of synchronously calling onStoreChange during subscribe:
if (collectionRef.current.status === `ready`) {
  versionRef.current += 1
  onStoreChange()  // <-- This triggers re-render during subscribe
}

// Defer it with queueMicrotask or setTimeout:
if (collectionRef.current.status === `ready`) {
  queueMicrotask(() => {
    versionRef.current += 1
    onStoreChange()
  })
}

Approach B: Move collection creation to useEffect

Move the createLiveQueryCollection({ startSync: true }) calls (lines 389–393, 396–400, 410–414) and startSyncImmediate() calls (lines 369, 384) into a useEffect, so they only run after commit. This is a larger refactor and may introduce a flash of empty data on first render.

Files to modify:

  • packages/react-db/src/useLiveQuery.ts — lines 437–450 (primary fix)

Effort estimate

Small — The core fix is deferring the synchronous onStoreChange() in the subscribe callback (a ~3-line change). Testing in StrictMode and verifying no regressions in non-StrictMode will take the bulk of the effort.

Suggested comment

Thanks for the detailed report @tec27, and good analysis @hugiex! We've identified the root cause — the subscribe callback passed to useSyncExternalStore fires a synchronous onStoreChange() when the collection is already ready, which can cause React to re-subscribe before cleaning up the first subscription in StrictMode. We're working on a fix to defer that notification. This is dev-only but we want to get it right since leaked subscribers prevent proper GC.

Now I have all the evidence. Here's the investigation:


Summary

The utils property on collections created via electricCollectionOptions, trailBaseCollectionOptions, powerSyncCollectionOptions, and localOnlyCollectionOptions resolves to the generic UtilsRecord type instead of the specific utils type (e.g. ElectricCollectionUtils<T>). The utils are present at runtime, but TypeScript doesn't know their shape. Collections created via localStorageCollectionOptions and queryCollectionOptions are correctly typed.

Root Cause Analysis

CollectionConfig in packages/db/src/types.ts takes 4 generic parameters: <T, TKey, TSchema, TUtils>. When TUtils (the 4th parameter) is omitted, it defaults to UtilsRecord — the untyped fallback.

Working collections pass all 4 parameters:

  • local-storage.ts:343CollectionConfig<T, TKey, never, LocalStorageCollectionUtils>
  • query.ts:413-417CollectionConfig<..., QueryCollectionUtils<...>>

Broken collections omit the 4th parameter:

  • local-only.ts:67CollectionConfig<T, TKey, TSchema> (missing 4th param)
  • trailbase.ts:116CollectionConfig<TItem, TKey> (missing 3rd and 4th params)
  • powersync definitions.ts:263CollectionConfig<OutputType, string, TSchema> (missing 4th param)
  • electric.ts:514,525Omit<CollectionConfig<..., T>, 'utils'> — uses Omit to remove utils then re-adds it via intersection, but the CollectionConfig itself still only has 3 generic params, so the underlying TUtils stays as UtilsRecord. The implementation overload at line 534 does pass 4 params, but the public-facing overloads at lines 514 and 525 do not.

In each case, the specific utils type is added via & { utils: SpecificUtils } intersection, but that doesn't propagate into the Collection interface's TUtils generic — createCollection reads TUtils from CollectionConfig's generic parameter, not from the intersection.

Suggested Fix

Add the specific utils type as the 4th generic parameter to CollectionConfig in each broken return type:

File Line(s) Change
packages/db/src/local-only.ts 67 CollectionConfig<T, TKey, TSchema>CollectionConfig<T, TKey, TSchema, LocalOnlyCollectionUtils>
packages/trailbase-db-collection/src/trailbase.ts 116 CollectionConfig<TItem, TKey>CollectionConfig<TItem, TKey, never, TrailBaseCollectionUtils>
packages/powersync-db-collection/src/definitions.ts 263 CollectionConfig<OutputType, string, TSchema>CollectionConfig<OutputType, string, TSchema, PowerSyncCollectionUtils<TTable>>
packages/electric-db-collection/src/electric.ts 514 Omit<CollectionConfig<InferSchemaOutput<T>, string | number, T>, 'utils'>Omit<CollectionConfig<InferSchemaOutput<T>, string | number, T, ElectricCollectionUtils<InferSchemaOutput<T>>>, 'utils'>
packages/electric-db-collection/src/electric.ts 525 Same pattern for the no-schema overload

The Electric one could also be simplified to drop the Omit + intersection pattern entirely and just pass all 4 generic params directly (matching the local-storage pattern), but that's optional.

Effort Estimate

Small — Each fix is a one-line type parameter addition. The pattern is identical across all four packages. Testing should involve type-checking against the reporter's reproduction repo or a simple tsc check.

Suggested Comment

Thanks for the thorough report and reproduction, Paweł! The root cause is that the affected collection factory functions omit the TUtils generic parameter from their CollectionConfig return types, so it defaults to UtilsRecord. The working collections (local-storage, query) pass it explicitly as the 4th parameter. The fix is straightforward — we just need to add the specific utils type as the 4th generic arg in each of the four affected packages. We'll get a PR up for this.

I now have all the information I need to write a comprehensive investigation. The key insight is about how the solid-db useLiveQuery manages state differently from react-db, with a dual subscription model that can cause stale/inconsistent data when where + orderBy interact. Let me write the final report.

Summary

When using useLiveQuery in @tanstack/solid-db with both where and orderBy clauses across multiple components, the UI displays incorrect/broken data. The reporter confirms: orderBy alone works, where alone works, and everything works with a single <List> component. Manually subscribing via collection.subscribeChanges works correctly — the bug is isolated to the solid-db useLiveQuery hook.

Root Cause Analysis

The bug is in packages/solid-db/src/useLiveQuery.ts — specifically, a dual-subscription race condition combined with Solid's reconcile function misinterpreting state during ordered, filtered updates.

Primary Issue: Dual syncDataFromCollection calls

The useLiveQuery implementation has two independent paths that both call syncDataFromCollection:

  1. createResource (line 322–351): Awaits toArrayWhenReady(), then calls syncDataFromCollection inside a batch().
  2. createEffect (line 353–392): Subscribes with includeInitialState: true, which fires the callback immediately with all current items, calling syncDataFromCollection inside a separate batch().

Both paths run when the collection changes. They execute in separate Solid batch() scopes, meaning Solid can render between them. This causes:

  • First sync: resource resolves → sets data via reconcile
  • Second sync: effect fires with initial state → sets data via reconcile again
  • The reconcile function (from solid-js/store) uses key: "id" by default and does a positional diff. When the same data arrives twice through different code paths with slightly different timing, reconcile can produce phantom entries or miss deletions.

Amplifying Factor: WeakMap-based ordering + reconcile interaction

The live query collection's SortedMap uses a WeakMap<object, string> (orderByIndices at collection-config-builder.ts:84) to store fractional indices for ordering. This WeakMap is keyed by object reference. When:

  1. An item is updated (e.g., "decrement all" changes an index field), the D2 graph produces a delete of the old object + insert of a new object
  2. The new object gets a new WeakMap entry; the old one is eligible for GC
  3. syncDataFromCollection calls Array.from(currentCollection.values()) which iterates the SortedMap in sorted order
  4. Solid's reconcile sees a new array and diffs it against the previous store state

When multiple components each call useLiveQuery with the same query (each creating an independent live query collection instance — there's no dedup, see live-query-collection.ts:136), and both where and orderBy are active:

  • Items moving in/out of the where filter produce insert/delete change messages
  • orderBy causes the collection to re-sort on each update
  • The dual sync paths race, and reconcile sees intermediate states where some items have moved position while others haven't yet been processed
  • .filter(Boolean) at line 318 masks the symptom (undefined entries from reconcile mismatches) but doesn't fix the root cause

Why it only breaks with multiple components

Each component creates its own live query collection instance (no caching/dedup). But they all subscribe to the same underlying source collection. When a mutation occurs:

  • All live query collections receive the change event simultaneously
  • Each runs its D2 graph and updates its SortedMap
  • Each solid-db hook fires both the resource and effect paths
  • With N components, there are 2N concurrent syncDataFromCollection calls, all fighting over their respective Solid stores during the same microtask batch

Comparison with react-db

React's useLiveQuery (packages/react-db/src/useLiveQuery.ts) avoids this entirely — it uses useSyncExternalStore with a single subscription, and reads data directly from collection.entries() during render (line 518). There is no dual-path, no reconcile, and no ReactiveMap intermediary.

Suggested Fix

The fix should simplify the solid-db hook to eliminate the dual subscription. Two approaches:

Option A (Minimal fix): Remove the includeInitialState from the createEffect subscription

In packages/solid-db/src/useLiveQuery.ts, line 383–386:

// Change from:
{ includeInitialState: true }
// To:
{ }  // Let createResource handle initial state

This removes the double-sync but may leave a gap where changes that arrive between the resource resolving and the effect subscribing are missed.

Option B (Recommended): Restructure to single subscription path

Refactor to use a single subscribeChanges call (with includeInitialState: true) as the sole data source, removing the createResource toArrayWhenReady() path entirely. The subscription callback should be the single source of truth for state updates. Use createSignal or a custom Suspense trigger for loading state instead of createResource.

Key files to modify:

  • packages/solid-db/src/useLiveQuery.ts — lines 297–392 (the entire state management section)

Reference implementation: The react-db useLiveQuery at packages/react-db/src/useLiveQuery.ts demonstrates the simpler single-subscription pattern.

Effort Estimate

Medium — The core fix is conceptually straightforward (eliminate dual subscription), but requires careful handling of Suspense integration (the createResource is there for Solid Suspense support) and testing across all the edge cases (loading states, disabled queries, collection swaps, cleanup).

Suggested Comment

Thanks for the detailed reproduction and analysis @LQR471814! We've identified the root cause — the solid-db useLiveQuery has a dual-subscription pattern (both a createResource and a createEffect with includeInitialState: true) that race against each other when processing changes. When where + orderBy are combined, items moving in/out of the filter while being re-sorted cause reconcile to see inconsistent intermediate states. This is amplified with multiple components since each creates an independent live query collection. We're working on a fix to consolidate to a single subscription path.

I see something critical here. On line 138-143, when includeInitialState is true, it calls requestSnapshot with trackLoadSubsetPromise: false. This is fine because the onLoadSubsetResult callback handles tracking separately. But let me now check the subscribeToMatchingChanges path more carefully — lines 230-238 in collection-subscriber.ts.

The key insight: In subscribeToMatchingChanges (line 230-238), onLoadSubsetResult is set only when includeInitialState is true. And in changes.ts line 139, trackLoadSubsetPromise: false is passed. So the loading state is tracked through the onLoadSubsetResult callback which calls trackLoadPromise on the live query collection's sync.

This all looks correct for the current code. The investigation confirms the bug does not exist in the current codebase because the if (limit) guard at order-by.ts:129 prevents subscribeToOrderedChanges from being called when there's no limit. The code falls through to subscribeToMatchingChanges which handles undefined limit correctly.

The bug was likely present in v0.5.21 (what the reporter was using) and was fixed by commit 284ebcc8 which improved loading state tracking.


Summary

The reporter observes that useLiveQuery with .orderBy() but no .limit() against an on-demand sync collection immediately reports isLoading: false with data: [], instead of staying in a loading state while the API fetch completes. Adding .limit(N) as a workaround fixes the behavior. The issue was filed against v0.5.21.

Root Cause Analysis

This bug does not exist in the current codebase (v0.5.25+). Here's why:

The reporter's proposed root cause — offset + limit = NaN in subscribeToOrderedChanges — cannot occur because of a guard at order-by.ts:129:

if (limit) {  // only creates OrderByOptimizationInfo when limit is truthy
    orderByOptimizationInfo = { ... }
}

Without a limit, orderByOptimizationInfo is never created. In collection-subscriber.ts:102, the code checks:

if (orderByInfo) {
    subscription = this.subscribeToOrderedChanges(...)  // only when limit exists
} else {
    subscription = this.subscribeToMatchingChanges(...)  // used for orderBy-only queries
}

subscribeToMatchingChanges (line 207-209) safely handles undefined limit:

const effectiveLimit =
    limit !== undefined && offset !== undefined ? limit + offset : limit

The real fix was likely commit 284ebcc8 ("fix(db): track loadSubset promise for on-demand live queries") which added direct load promise tracking via trackLoadPromise and the onLoadSubsetResult callback pattern. Before that fix, the loading state transitions for on-demand collections could race and resolve prematurely — which is exactly what the reporter described. The subscription's requestSnapshot now properly flows through onLoadSubsetResulttrackLoadPromiseisLoadingSubset, ensuring updateLiveQueryStatus doesn't call markReady() until the fetch completes.

Suggested Fix

No code changes needed. The bug was fixed in v0.5.25 by commit 284ebcc8. The reporter should upgrade.

Key files for reference:

  • packages/db/src/query/compiler/order-by.ts:129 — guard preventing NaN path
  • packages/db/src/query/live/collection-subscriber.ts:102-120 — dispatch to correct subscribe method
  • packages/db/src/query/live/collection-subscriber.ts:207-249subscribeToMatchingChanges safe limit handling
  • packages/db/src/collection/subscription.ts:291-301trackLoadSubsetPromise loading state management

Effort Estimate

Small — No code changes required, just version upgrade confirmation.

Suggested Comment

Thanks for the detailed report! We believe this was fixed in v0.5.25 by commit 284ebcc8 which improved loading state tracking for on-demand live queries. The orderBy-without-limit path correctly falls through to subscribeToMatchingChanges which handles undefined limit safely. Could you try upgrading to the latest version and let us know if the issue persists?

Now I have a thorough understanding of the implementation. Let me write the investigation.


Summary

The issue requests that useLiveQuery in the solid-db package return Solid Stores instead of plain arrays, enabling fine-grained reactivity consistent with the Solid.js philosophy. Currently, when a single item changes, the entire data array is reconciled and consumers using <For each={query()}> may re-render more than necessary. The author notes that TanStack DB already provides per-item change deltas, which should make granular Store updates feasible.

Root Cause Analysis

This is more of an enhancement request than a bug. The current implementation in packages/solid-db/src/useLiveQuery.ts is actually partially already Store-based — it uses createStore + reconcile internally (lines 301, 317-319). However, the issue is nuanced:

  1. The returned data is a resource accessor wrapping a store array (line 344: return data, line 396: return getDataResource()). The getData() function returns the result of createResource, which wraps the store value. This means consumers calling query() get the unwrapped array from the resource, losing the store proxy's fine-grained tracking.

  2. reconcile() is used on every change (line 317-318) — this rebuilds the entire array from currentCollection.values() and diffs it against the previous store state. While reconcile is smarter than full replacement, it still does an O(n) structural comparison on the whole array, rather than surgically applying only the known deltas.

  3. The delta information is already availablesubscribeChanges at line 361 provides per-item insert/update/delete changes with keys, which are already used to update the ReactiveMap granularly (lines 366-374). But the data array sync ignores these deltas and rebuilds from scratch.

  4. The state property (ReactiveMap) is already granular — ironically, query.state already provides the fine-grained per-item reactivity the issue requests, but it's a Map, not an ordered array, so it can't be used directly with <For>.

Suggested Fix

The fix should apply known deltas directly to the store array using produce() from solid-js/store instead of rebuilding via reconcile():

File: packages/solid-db/src/useLiveQuery.ts

  1. Import produce from solid-js/store (line 15) alongside the existing createStore and reconcile.

  2. Replace syncDataFromCollection (lines 313-320) with a delta-aware update function. When processing changes from subscribeChanges (lines 361-387), instead of calling syncDataFromCollection which does a full reconcile, use produce() to:

    • For insert: push/splice the new item into the store array at the correct sorted position
    • For update: find and replace the item in-place by key (preserving the store proxy reference)
    • For delete: splice the item out by key
  3. Keep reconcile for initial population (line 341 in the resource creator) where there's no previous state to diff against, but switch the subscribeChanges handler to use produce().

  4. Return type stays the same — the accessor already returns a store-backed array; the change is about how it's updated (surgical vs. full reconcile). The Accessor<Array<...>> return type could optionally be tightened to indicate it's a store, but this is backwards-compatible.

  5. Consider exposing the store directly — rather than wrapping through createResource (which strips the store proxy), expose the store array more directly so <For> can leverage Solid's keyed diffing on store arrays.

Key concern: Maintaining correct sort order when applying inserts/updates via produce() requires knowing where to place items. The collection already maintains order, so the implementation could reference currentCollection.indexOf(key) or rebuild the position from the collection's sorted state. Alternatively, after applying produce() mutations, a lightweight re-sort within the store (using produce) would still be more efficient than full reconcile.

Effort Estimate

Medium — The core change is localized to one function in one file (~30 lines of logic change), but requires careful handling of:

  • Sort order maintenance when applying deltas
  • Suspense integration (the createResource wrapping complicates direct store exposure)
  • Ensuring <For> properly leverages store granularity
  • Comprehensive test updates to verify per-item reactivity (not just correctness)

Suggested Comment

Thanks for this well-articulated issue! You're right that the current implementation doesn't fully leverage Solid's fine-grained reactivity — we use createStore + reconcile internally but the full-array reconcile on every change misses the opportunity to apply surgical deltas via produce(). The good news is that the subscribeChanges API already provides per-item insert/update/delete deltas, so the plumbing is there. We'll look into switching the change handler from reconcile to produce-based updates to enable true granular store reactivity. In the meantime, the query.state ReactiveMap property already provides per-item granular reactivity if you need it.

Now I have a thorough understanding of the issue and codebase. Let me write the investigation.


Summary

When useLiveQuery is called multiple times with the same query (e.g., 20 components on a page each calling useLiveVendorsQuery()), each call creates a completely independent live query collection — its own CollectionConfigBuilder, D2 incremental computation graph, and source collection subscriptions. The reporter is asking for TanStack Query-style deduplication, where multiple callers sharing the same query key would share a single underlying execution, similar to how QueryCache/QueryObserver work in TanStack Query.

Root cause analysis

There is no global query cache or deduplication layer for live queries. The path is:

  1. useLiveQuery (packages/react-db/src/useLiveQuery.ts:389) calls createLiveQueryCollection every time deps change (or on first mount).
  2. createLiveQueryCollection (packages/db/src/query/live-query-collection.ts:136-178) instantiates a fresh CollectionConfigBuilder each time.
  3. Each CollectionConfigBuilder (packages/db/src/query/live/collection-config-builder.ts) compiles its own D2 graph and sets up independent subscriptions to the source collections.

The only deduplication that exists is at the data loading levelDeduplicatedLoadSubset (packages/db/src/query/subset-dedupe.ts) prevents redundant network/sync calls for overlapping WHERE predicates on the source collection. But the query computation graph itself (joins, filters, projections, ordering) is fully duplicated per call.

The gcTime default is 1ms (useLiveQuery.ts:21), meaning collections are garbage collected almost instantly on unmount — there's no opportunity for a subsequent mount to reuse a prior query result.

Suggested fix

Two possible approaches:

Approach A: Query collection cache (Medium effort)

Introduce a cache keyed by a query identity (serialized query IR or user-supplied id) that returns the same Collection instance when an identical query is requested. This would sit in createLiveQueryCollection (or a new LiveQueryCache class) and use a reference-counting or subscriber-counting approach:

  • packages/db/src/query/live-query-collection.ts — add lookup-or-create logic before instantiating CollectionConfigBuilder
  • packages/react-db/src/useLiveQuery.ts — when a query function produces the same key, return the cached collection rather than creating a new one
  • Need a stable serialization of the query IR from packages/db/src/query/ir.ts for automatic keying, or accept a user-supplied id

Approach B: Shared collection pattern (already works, needs better docs)

Kyle's comment on the issue already points to the recommended workaround: create a single createLiveQueryCollection outside of components and pass it into multiple useLiveQuery calls (overloads 7/8 at useLiveQuery.ts:276-313 accept pre-created collections). This works today but requires users to restructure their code.

The pragmatic path is probably Approach A with user-supplied id — if two useLiveQuery calls share the same id, return the same underlying collection. This avoids the complexity of automatic query IR serialization.

Effort estimate

Medium — The core mechanism (a cache map keyed by ID, reference counting for cleanup) is straightforward, but it touches the collection lifecycle (gcTime, cleanup, sync start/stop) which needs careful handling, especially around React StrictMode double-mount behavior.

Suggested comment

Thanks for the detailed write-up! This is a reasonable feature request. Currently live queries are designed to be lightweight (no network calls, just in-memory computation), but we recognize that duplicating the D2 computation graph 20x is wasteful. The recommended pattern today is to create a shared collection with createLiveQueryCollection outside your components and pass it to multiple useLiveQuery calls — that gives you full deduplication. Longer term, we're considering adding an id-based cache so identical queries can automatically share a single underlying collection without manual hoisting.

Now I have a thorough understanding. Let me write the investigation.

Summary

The issue reports that vue-db's useLiveQuery API was modeled after the React implementation rather than the Solid implementation. Since Vue and Solid are both fine-grained reactivity frameworks, the Vue adapter should follow Solid's patterns (accessor-based return type, implicit dependency tracking, Suspense support) rather than React's snapshot/useSyncExternalStore model.

Root cause analysis

There are several key architectural differences where Vue currently follows React's patterns instead of Solid's:

1. API shape: explicit deps array (React-like) vs implicit reactivity (Solid-like)

  • React (react-db/src/useLiveQuery.ts:318): Takes a raw value + explicit deps array — necessary because React has no built-in reactivity system.
  • Solid (solid-db/src/useLiveQuery.ts:255-256): All arguments are accessor functions (Accessor<...> or function with arity). Dependencies are tracked implicitly by Solid's reactivity system via createMemo.
  • Vue (vue-db/src/useLiveQuery.ts:247-250): Takes raw values + an explicit deps: Array<MaybeRefOrGetter<unknown>> — this is the React pattern. Vue's computed/watchEffect can auto-track ref/reactive dependencies without an explicit deps array.

2. Return type: plain object with ComputedRefs (React-like) vs callable accessor with properties (Solid-like)

  • React: Returns a plain object { data, state, status, ... } with static values.
  • Solid (solid-db/src/useLiveQuery.ts:395-446): Returns a callable function (accessor) with Object.defineProperties for state, status, etc. Data is accessed via query(), and the function itself integrates with <Suspense> via createResource.
  • Vue (vue-db/src/useLiveQuery.ts:465-477): Returns a plain object with ComputedRef properties. While ComputedRef is Vue-idiomatic, the overall shape mirrors React's destructured object rather than Solid's callable accessor.

3. No Suspense support (React-like omission)

  • Solid (solid-db/src/useLiveQuery.ts:322-351): Uses createResource for Suspense integration — data loading suspends the component until ready.
  • Vue: No equivalent <Suspense> integration. Vue 3 has experimental <Suspense> support with async setup(), but the composable doesn't leverage it.
  • React: Also lacks built-in Suspense (has a separate useLiveSuspenseQuery hook in react-db/src/useLiveSuspenseQuery.ts).

4. Collection detection: duck-typing (React-like) vs instanceof (Solid-like)

  • Solid (solid-db/src/useLiveQuery.ts:283): Uses instanceof CollectionImpl to detect pre-created collections.
  • React (react-db/src/useLiveQuery.ts:321-326): Duck-types by checking for subscribeChanges/startSyncImmediate/id.
  • Vue (vue-db/src/useLiveQuery.ts:269-274): Duck-types identically to React.

5. Disabled query detection: try/catch with sentinel error (React-like) vs direct null check (Solid-like)

  • Solid (solid-db/src/useLiveQuery.ts:260-268): Probes the query function with a BaseQueryBuilder, checks if the result is null/undefined.
  • React (react-db/src/useLiveQuery.ts:374-381): Same pattern as Solid (probe + null check).
  • Vue (vue-db/src/useLiveQuery.ts:306-328): Wraps the query function and throws a "__DISABLED_QUERY__" sentinel error, then catches it. This is fragile and unlike either implementation.

6. Data diffing: splice-and-push (Vue) vs reconcile (Solid)

  • Solid (solid-db/src/useLiveQuery.ts:317-320): Uses createStore + reconcile() for efficient structural diffing that preserves object identity.
  • Vue (vue-db/src/useLiveQuery.ts:362-366): Clears the array (length = 0) and pushes all values — this destroys identity and triggers excessive reactivity updates.

Suggested fix

A proper rewrite of packages/vue-db/src/useLiveQuery.ts to align with Solid's patterns:

  1. Remove the deps parameter — use Vue's auto-tracking via computed/watchEffect for implicit dependency tracking (like Solid's createMemo). The query function parameter should be wrapped in a getter/accessor.
  2. Match Solid's overload signatures — accept () => queryFn, () => config, or () => collection (all wrapped in getters), not raw values.
  3. Use instanceof CollectionImpl for collection detection (solid-db/src/useLiveQuery.ts:283), not duck-typing.
  4. Probe the query function with BaseQueryBuilder for null-check (solid-db/src/useLiveQuery.ts:261-264), removing the sentinel-error hack.
  5. Consider Suspense integration — Vue 3's <Suspense> is experimental but growing; a createResource-like wrapper using async setup could be added.
  6. Improve data sync — replace the clear-and-push pattern with a more efficient structural diff or at minimum a shallow comparison before updating.

The primary files to modify:

  • packages/vue-db/src/useLiveQuery.ts — main rewrite
  • packages/vue-db/src/index.ts — update exports if return types change

Effort estimate

Medium — The core logic already exists in Solid's implementation as a reference. The main work is translating Solid primitives (createMemocomputed, createSignalref, createEffectwatchEffect, ReactiveMapreactive(new Map()), batch → implicit in Vue) and redesigning the API surface. The type overloads also need updating.

Suggested comment

Thanks for flagging this! You're right — since Vue and Solid are both fine-grained reactivity frameworks, the Vue adapter should follow Solid's patterns (implicit dependency tracking, accessor-style API) rather than React's explicit deps/snapshot model. The main areas to align are: removing the explicit deps array in favor of Vue's auto-tracking, using instanceof CollectionImpl for collection detection, and removing the sentinel-error pattern for disabled queries. This is on our radar as we continue maturing the framework adapters.

Now I have a complete picture. Let me write the investigation.

Summary

Users want to create collections that fetch data scoped to a parameter (e.g., project ID, user ID) rather than fetching all data. The current queryFn in collection options takes no arguments, so there's no built-in way to parameterize a collection. Kyle responded with a "collection factory pattern" (caching createCollection calls keyed by parameter), but this pattern has no dedicated documentation page.

Root cause analysis

This is not a code bug — it's a documentation and discoverability gap. The library already supports two approaches for parameterized data:

  1. Singleton collection + live query filtering — Create one collection that syncs all data, then use useLiveQuery with .where() to filter by parameter. This is the recommended approach and is demonstrated in examples/react/projects/ but not explicitly called out in docs as the answer to "how do I parameterize?"

  2. Collection factory pattern — Create collections on-demand keyed by parameter, with a Map cache and status:change cleanup listener. The key lifecycle bug (event handlers cleared before 'cleaned-up' status was emitted) was fixed in PR #714 (packages/db/src/collection/lifecycle.ts:283-288). The pattern works correctly now but is only documented in the CHANGELOG (packages/db/CHANGELOG.md:863) and GitHub issue comments.

Additionally, "query-driven sync" / on-demand sync (referenced in issue #612) would be the first-class solution — letting collections load data based on live query predicates — but that's still in progress.

Suggested fix

Documentation additions (no code changes needed):

  1. Create docs/guides/parameterized-collections.md — A new guide covering:

    • The recommended approach (singleton + useLiveQuery .where() filtering)
    • The collection factory pattern with full typed example (from Kyle's comment)
    • gcTime configuration for controlling factory cache lifetime
    • When to use each approach (factory for large datasets where fetching everything is impractical, singleton for smaller datasets)
  2. Add a section to the existing docs/guides/live-queries.md explaining how .where() clauses enable parameterized data access without parameterized collections.

  3. Reference from the main docs/README so new users find it quickly.

Relevant existing files:

  • docs/guides/live-queries.md — add "Parameterized Data" section
  • docs/guides/collection-options-creator.md — could reference factory pattern
  • examples/react/projects/ — already demonstrates the singleton + filter approach
  • packages/db/src/collection/lifecycle.ts:176-196 — gcTime/GC timer logic
  • packages/db/src/collection/lifecycle.ts:248-296 — cleanup flow (fixed in PR #714)

Effort estimate

Small — This is purely a documentation task. The code already works correctly. Writing the guide and adding cross-references would take a few hours.

Suggested comment

This is a common question! We should add a dedicated docs guide covering both approaches: (1) using a singleton collection with useLiveQuery .where() filters (recommended for most cases), and (2) the collection factory pattern for when you need separate collections per parameter. The factory pattern has been working correctly since the cleanup fix in PR #714. I'll work on getting a "Parameterized Collections" guide added to the docs.

I now have a complete picture. Here's the investigation:


Summary

When using max() (or min()) on a field backed by a Temporal object (e.g. Temporal.PlainDate), the aggregate returns NaN. This happens because the compiler's value extractor for min/max doesn't recognize Temporal types, causing them to be coerced via Number(), and the > / < comparison in the reduce function doesn't work on Temporal objects (they don't support implicit conversion to primitive).

Root Cause Analysis

The problem has two layers:

Layer 1 — Compiler value extractor (packages/db/src/query/compiler/group-by.ts:362-378):

The valueExtractorForMinMax function checks for number, string, bigint, and Date instances, passing them through as-is. Anything else falls through to Number(value). Temporal objects (like Temporal.PlainDate) are not instanceof Date, so they hit the fallback Number(value), which returns NaN.

// line 369-377 — no Temporal check exists
if (
  typeof value === `number` ||
  typeof value === `string` ||
  typeof value === `bigint` ||
  value instanceof Date
) {
  return value
}
return value != null ? Number(value) : 0  // ← Temporal objects land here → NaN

Layer 2 — Aggregate reduce function (packages/db-ivm/src/operators/groupBy.ts:264-270):

Even if Temporal objects made it through the extractor, the max() reduce uses native > for comparison (value > maxValue). Temporal objects don't support implicit primitive coercion, so this comparison would also fail. Temporal types require explicit comparison methods like Temporal.PlainDate.compare().

Layer 3 — Type constraint (packages/db-ivm/src/operators/groupBy.ts:214):

The CanMinMax type is number | Date | bigint | string — Temporal types aren't included.

Notably, the codebase already has isTemporal() detection in packages/db/src/utils.ts:230-233 and treats Temporal objects as immutable pass-throughs in packages/db/src/proxy.ts:586. The infrastructure to detect them exists — it just wasn't wired into aggregates.

Suggested Fix

  1. packages/db/src/query/compiler/group-by.ts:362-378 — Add a Temporal check to valueExtractorForMinMax. The simplest approach: since Temporal date/time types have a toString() that produces ISO 8601 strings (lexicographically sortable for dates), convert Temporal objects to strings:

    if (isTemporal(value)) {
      return value.toString()  // ISO 8601 — lexicographically comparable
    }

    This works correctly for PlainDate, PlainDateTime, Instant, and ZonedDateTime. Import isTemporal from ../../utils.

  2. Alternatively, for a more robust approach: add Temporal-aware comparison to the min()/max() reduce functions in packages/db-ivm/src/operators/groupBy.ts:234-274. Use Temporal.PlainDate.compare(a, b) etc. This is more correct but requires knowing the specific Temporal type to dispatch the right compare() call.

  3. Type updates — Expand CanMinMax in packages/db-ivm/src/operators/groupBy.ts:214 to include Temporal types (or a generic { toString(): string } constraint).

  4. packages/db/src/utils/comparison.ts:52-55 — The ascComparator (used for ORDER BY) also lacks Temporal support. Should add a Temporal branch alongside the existing Date branch for consistency.

Recommended approach: The string-conversion approach (option 1) is simplest, works immediately for the most common Temporal types, and requires changes in only one file. A follow-up could add native Temporal comparison for correctness with edge cases.

Effort Estimate

Small — The core fix is adding a Temporal check to valueExtractorForMinMax (3-5 lines). Adding Temporal support to the comparison utility and expanding the type constraint adds a bit more, but the total is still well-scoped. Tests should cover min()/max() with Temporal.PlainDate and Temporal.PlainDateTime values.

Suggested Comment

Thanks for the detailed report, @evanheckert! The root cause is that the min/max value extractor in the query compiler only recognizes Date, string, number, and bigint — Temporal objects fall through to a Number() coercion which produces NaN. We already have isTemporal() detection in the codebase, so the fix should be straightforward: detect Temporal values in the extractor and convert them to their ISO 8601 string representation (which is lexicographically sortable for date types). We'll also want to add Temporal awareness to the ordering comparator for consistency.

Now I have the full picture. Here's my investigation:


Summary

The issue requests a public utility type (e.g., CollectionItem<TCollection>) that lets users extract the item/row type from a collection without importing internal generics. This is a common need when passing collections into UI components and wanting to type props against the collection's item type.

Root cause analysis

This isn't a bug — it's a missing public API. The good news is that the utility already exists internally as InferCollectionType at packages/db/src/query/builder/types.ts:85-86:

export type InferCollectionType<T> =
  T extends CollectionImpl<infer TOutput, any, any, any, any> ? TOutput : never

It's just never re-exported from the package's public entry points. The packages/db/src/query/index.ts file (line 49) only re-exports Ref from ./builder/types.js, and neither packages/db/src/index.ts nor packages/db/src/types.ts surface InferCollectionType.

As kevin-dp noted in the comments, the simpler approach using Collection (rather than CollectionImpl) works just as well — and since Collection extends CollectionImpl, the existing internal type already handles both.

Suggested fix

Two changes needed, both small:

  1. Re-export InferCollectionType from packages/db/src/query/index.ts — add it to the type export on line 49 alongside Ref:

    export type { Ref, InferCollectionType } from './builder/types.js'

    This makes it available from @tanstack/db since packages/db/src/index.ts:11 does export * from './query/index.js'.

  2. Optionally, add a CollectionItem alias in packages/db/src/types.ts (near line 60 alongside InferSchemaOutput/InferSchemaInput) for a more user-friendly name that matches the issue's proposal. However, simply exporting the existing InferCollectionType may be sufficient — it follows the Infer* naming convention already used (InferSchemaOutput, InferSchemaInput, InferResultType).

No new logic is needed. This is purely a re-export / public API surface change.

Effort estimate

Small — a one-line re-export, plus optionally a type alias. Could also warrant a brief addition to docs/examples showing usage.

Suggested comment

Great suggestion! We actually already have this internally as InferCollectionType in our query builder types — it just wasn't exported publicly. The fix is straightforward: we need to re-export it from the package entry point. We could also add a CollectionItem alias if that naming feels more natural. Would you be interested in sending a PR for this?

Now I have all the pieces. Here's my investigation:


Summary

When a live query uses chained LEFT JOINs (A→B→C) and a middle collection (B) starts empty and is populated later, the subscription emits 3 deletes + 6 inserts instead of 3 updates. Additionally, providing a custom getKey to work around this causes a DuplicateKeySyncError. The reporter suspects this is a regression from #658 (eager execution during sync).

Root Cause Analysis

The bug is in composite key instability for LEFT JOIN results when the joined side transitions from null to populated.

Key generation happens in packages/db/src/query/compiler/joins.ts:599:

const resultKey = `[${mainKey},${joinedKey}]`

The sequence of events:

  1. First graph step — Players (A) and Balances (C) are loaded, Clients (B) is empty. The first LEFT JOIN (A→B) produces results with joinedKey = undefined, giving composite keys like [player1,undefined]. The second LEFT JOIN then produces keys like [[player1,undefined],undefined].

  2. Second graph step — Clients (B) arrives. The D2 IVM engine retracts the old null-extended rows (multiplicity -1) and inserts new matched rows (multiplicity +1). But the retracted rows have key [player1,undefined] while the inserted rows have key [player1,client1].

  3. accumulateChanges (collection-config-builder.ts:1104-1133) merges inserts/deletes by key. Since the old and new keys differ, they're tracked separately — the old key gets {deletes: 1, inserts: 0}delete event, and the new key gets {deletes: 0, inserts: 1}insert event.

  4. With chained joins (A→B→C), this multiplies: the B insertion changes the first join's keys, which cascades into the second join, producing even more key mismatches — hence 3 deletes + 6 inserts.

Why #658 caused this: Before #658, the graph waited until all source collections were ready before executing. The joined collection would already be populated at first execution, so the key would never contain undefined. After #658, eager execution means the graph runs with partial data, producing null-extended keys that later change.

Why custom getKey fails: When getKey: (r) => r.player_name is provided, the first graph step inserts rows with key player1. The second step retracts the old composite key (triggering a delete) and inserts with the new composite key — but since the custom getKey maps both to player1, it tries to insert a key that already exists (the delete hasn't been processed yet in the same accumulation batch), triggering DuplicateKeySyncError at packages/db/src/collection/sync.ts:150.

Suggested Fix

Primary fix — stabilize LEFT JOIN keys in packages/db/src/query/compiler/joins.ts:598-599:

For LEFT JOINs, the result key should be based only on the main (non-nullable) side's key, since the main side is what determines result row identity in a LEFT JOIN. Something like:

// For left joins, use only the main key since the left side determines identity
const resultKey = joinType === 'left' 
  ? mainKey 
  : joinType === 'right'
    ? joinedKey
    : `[${mainKey},${joinedKey}]`

However, this only works for 1:1 or many:1 joins. For 1:many LEFT JOINs (where one left row matches multiple right rows), you'd still need a composite key to distinguish results. The fix needs to be more nuanced — possibly using the main key as the identity key for change tracking while keeping the composite key for deduplication.

Alternative approach — key reconciliation in accumulateChanges: Track key lineage so that when a retraction for [player1,undefined] and insertion for [player1,client1] arrive in the same flush, they can be recognized as updates to the same logical row. This is more complex but handles all join types correctly.

Most pragmatic approach: In flushPendingChanges, before committing, look for delete+insert pairs where the main key (extracted from the composite key) matches, and merge them into updates. This would be in collection-config-builder.ts around lines 718-725.

Effort Estimate

Medium — The fix requires careful handling of the key stability problem for different join cardinalities (1:1 vs 1:many) and join types (left, right, full, inner). Testing across all join scenarios will be important. The DuplicateKeySyncError with custom getKey is a downstream symptom that should resolve once key stability is fixed.

Suggested Comment

Thanks for the detailed report and reproduction! This is indeed related to the eager execution change from #658. The root cause is that LEFT JOIN composite keys include the joined side's key, which changes from undefined to an actual value when a previously-empty collection gets populated — causing the system to treat it as a delete+insert rather than an update. We'll work on stabilizing the key identity for outer join results so that null-to-matched transitions are correctly recognized as updates.

Now I have the full picture. The problem is:

  1. isUndefined(r.payload) is a single-source clause touching only r, with hasNamespaceOnlyRef = false
  2. It gets pushed down into r's subquery (line 649-653), filtering the right collection before the join
  3. The code does add a residual WHERE clause post-join (line 678-680) because hasOuterJoins is true
  4. But the residual filter is semantically different after pushdown: rows that originally matched (l1↔r1, l2↔r2) now have no right match, so r is missing from the merged row, and isUndefined(undefined?.payload) returns true

The fix: for outer joins, WHERE clauses targeting the nullable side should NOT be pushed down. They should only remain as post-join filters.

Now I have everything I need to write the investigation.

Summary

When using .leftJoin(...).where(({r}) => isUndefined(r?.payload)), the WHERE predicate is supposed to filter entire joined rows post-join. Instead, it incorrectly filters rows from the right collection before the join (via predicate pushdown), then lets the now-unmatched left rows pass through the post-join residual filter. The result is that rows like l1 and l2 appear with right: undefined instead of being excluded entirely.

Root cause analysis

The bug is in the query optimizer's predicate pushdown logic in packages/db/src/query/optimizer.ts.

Here's the chain of events:

  1. Expression capture: When the user writes .where(({r}) => isUndefined(r?.payload)), the r proxy is always truthy (it's a Proxy object), so r?.payload is equivalent to r.payload. This captures Func('isUndefined', [PropRef(['r', 'payload'])]).

  2. Optimizer analysis (analyzeWhereClause, line ~517): The PropRef path ['r', 'payload'] has length 2, so hasNamespaceOnlyRef stays false. It touches only source 'r'.

  3. Grouping (groupWhereClauses, line ~590): Since touchedSources.size === 1 and !hasNamespaceOnlyRef, the clause is classified as single-source — eligible for pushdown.

  4. Pushdown (applyOptimizations, line ~646-654): The clause is pushed into the right-side subquery, filtering the r collection before the join executes. This removes r1 (payload: 'ok') and r2 (payload: null) from the right side entirely.

  5. Residual filter (line ~678-680): The code correctly adds a residual WHERE clause post-join when outer joins are present. But after pushdown removed r1/r2, left rows l1 and l2 have no right match. In the merged row, namespacedRow['r'] is undefined. The evaluator (evaluators.ts:148-149) computes undefined?.payloadundefined, so isUndefined(undefined)true. These rows pass the residual filter.

The net effect: pushdown changes semantics for outer joins. Filtering the nullable side before a left join converts "matched but filtered" rows into "unmatched" rows, which then incorrectly survive the residual filter.

★ Insight ───────────────────────────────────── This is a classic SQL optimization pitfall. In SQL, pushing a WHERE clause on the outer side of a LEFT JOIN into the join subquery converts the LEFT JOIN into an effectively INNER JOIN — it changes semantics. The same principle applies here. The existing hasNamespaceOnlyRef guard (line 540) correctly prevents pushdown for isUndefined(r) (whole-namespace checks), but doesn't extend to property-level checks like isUndefined(r.payload) on the nullable side of an outer join. ─────────────────────────────────────────────────

Suggested fix

File: packages/db/src/query/optimizer.ts, in applyOptimizations (around line 631-683)

The fix should prevent pushdown of single-source WHERE clauses that target the nullable side of an outer join. Specifically:

  1. Before pushing down into join subqueries (line 646-654), check if the join is an outer join and the clause targets the nullable side:

    • For left joins: don't push down clauses targeting the joined (right) source
    • For right joins: don't push down clauses targeting the main (left) source
    • For full joins: don't push down clauses targeting either side
  2. When a clause can't be pushed down for this reason, add it to remainingWhereClauses as a regular (non-residual) WHERE clause instead.

Concretely, the fix is in applyOptimizations: build a set of "outer-nullable aliases" from the join clauses, then in the loop at line 646-654, skip pushdown for those aliases. Something like:

// Collect aliases that are on the nullable side of outer joins
const outerNullableAliases = new Set<string>()
if (query.join) {
  for (const join of query.join) {
    if (join.type === 'left' || join.type === 'full') {
      outerNullableAliases.add(join.from.alias)
    }
    if (join.type === 'right' || join.type === 'full') {
      outerNullableAliases.add(query.from.alias)
    }
  }
}

// Then when optimizing joins:
const optimizedJoins = query.join
  ? query.join.map((joinClause) => ({
      ...joinClause,
      from: outerNullableAliases.has(joinClause.from.alias)
        ? joinClause.from  // Don't push down - nullable side of outer join
        : optimizeFromWithTracking(
            joinClause.from,
            groupedClauses.singleSource,
            actuallyOptimized,
          ),
    }))
  : undefined

And similarly guard optimizedFrom for right/full joins.

An existing test at packages/db/tests/query/join.test.ts:1443 covers the isUndefined(namespace) case — a new test should be added for the isUndefined(namespace.property) case from this issue.

Effort estimate

Small — The fix is a focused change to the optimizer's applyOptimizations function (~10-15 lines), plus a test case. The logic is well-isolated and the existing residual-WHERE mechanism already handles the post-join filtering correctly once pushdown is prevented.

Suggested comment

Thanks for the excellent bug report with a clear reproduction case! This is a predicate pushdown issue in the query optimizer — isUndefined(r.payload) gets incorrectly pushed into the right-side subquery before the left join executes, which changes the join semantics. The fix is to prevent pushdown of WHERE clauses targeting the nullable side of outer joins. We're tracking this and will get a fix in.

Now I have a clear picture. Let me verify the exact flow by understanding what useSyncExternalStore does with the subscribe callback:

The key issue is in useLiveQuery.ts lines 442-446. Let me write the investigation.


Summary

When mounting a useLiveQuery hook against a collection that is already in ready status, React triggers an unnecessary extra re-render. The data reference also changes between renders despite no underlying data change. This is a performance issue — not a correctness bug — but it breaks referential stability guarantees that users rely on for memoization.

Root cause analysis

The issue is in packages/react-db/src/useLiveQuery.ts lines 442-446, inside the subscribe callback passed to useSyncExternalStore:

// Collection may be ready and will not receive initial `subscribeChanges()`
if (collectionRef.current.status === `ready`) {
  versionRef.current += 1
  onStoreChange()
}

This code was introduced in PR #485 to fix a race condition: if a collection transitions to ready before subscribeChanges is called, the subscriber would never receive the initial data notification. The fix unconditionally bumps the version and calls onStoreChange() whenever the collection is already ready at subscribe-time.

The problem: When the collection is already ready and has data, the initial getSnapshot call during React's render already produces a snapshot with version: 0 and the collection. But then subscribe fires, bumps to version: 1, and calls onStoreChange(), which causes React to call getSnapshot again. Since the version changed (0 → 1), a new snapshot object is created, which triggers a re-render. During that re-render, a new entries array is captured (line 518), creating new data/state objects — breaking referential stability.

In essence: the version bump is unconditional and doesn't check whether the data actually changed since the initial snapshot was produced.

Suggested fix

The fix needs to preserve the PR #485 race condition protection while avoiding the spurious re-render when data hasn't changed. A few approaches:

Approach A (simplest): In the subscribe callback, only bump the version if it's still at 0 (initial) AND the snapshot hasn't yet captured any data. This can be done by checking if snapshotRef.current already has a version that reflects the ready state:

// In subscribe callback (~line 442):
if (collectionRef.current.status === `ready` && versionRef.current === 0) {
  // Only bump if getSnapshot hasn't been called yet with data
  // (i.e., the initial render already saw the ready state)
  // Check if the snapshot already reflects the current state
  if (!snapshotRef.current || snapshotRef.current.version === 0) {
    // Don't bump — the initial getSnapshot already captured version 0
    // with the ready collection, so React already has the right data
  } else {
    versionRef.current += 1
    onStoreChange()
  }
}

Actually, a cleaner approach:

Approach B (recommended): The root issue is that getSnapshot at version 0 with a ready collection already returns valid data. So the subscribe callback shouldn't bump the version in that case. The fix: initialize versionRef.current to 1 when the collection is already ready at creation time (around line 422), and then in subscribe, only call onStoreChange if the current version is still 0 (meaning the collection wasn't ready at creation):

In useLiveQuery.ts around lines 421-424:

if (needsNewCollection) {
  // If collection is already ready, start at version 1 so subscribe
  // doesn't redundantly bump it
  versionRef.current = collectionRef.current?.status === 'ready' ? 1 : 0
  snapshotRef.current = null
}

And around lines 442-446:

if (collectionRef.current.status === `ready` && versionRef.current === 0) {
  versionRef.current += 1
  onStoreChange()
}

This way, if the collection was already ready when we created it, versionRef starts at 1, and the === 0 guard prevents the redundant bump. If the collection becomes ready after creation (the PR #485 race), versionRef starts at 0 and the bump fires correctly.

Files to modify: packages/react-db/src/useLiveQuery.ts — only ~4 lines changed.

Effort estimate

Small — The fix is a few lines in a single file. However, as collaborator @samwillis noted, this area is hard to test outside a browser, so careful manual testing is needed to avoid regressions on the PR #485 race condition.

Suggested comment

Thanks for the detailed report and reproduction! You correctly identified the root cause — the subscribe callback unconditionally bumps the version when the collection is already ready, even though useSyncExternalStore's initial getSnapshot already captured the correct data. The fix is to skip the bump when the version already reflects the ready state. We'll need to be careful not to regress the race condition that PR #485 solved (collection becomes ready between render and subscribe).

I now have a complete understanding. Here's my investigation:


Summary

When an aggregate function like count(...) is wrapped in another expression (e.g., coalesce(count(item.id), 0)), the query compiler throws QueryCompilationError: Unknown expression type: agg. Bare aggregates work fine. The reporter also notes that the nested form works inside useLiveQuery() but fails when constructing a new Query() outside of a hook.

Root Cause Analysis

There are two separate bugs working together:

Bug 1: Aggregate detection only checks top-level SELECT entries

In packages/db/src/query/compiler/index.ts:270-272, the implicit single-group aggregation detection does a shallow check:

const hasAggregates = Object.values(query.select).some(
  (expr) => expr.type === `agg`,  // Only checks top-level type!
)

When the SELECT is { vehicleDispatchCount: coalesce(count(...), 0) }, the top-level expression type is func (for coalesce), not agg. So the groupBy processing is never triggered, and the query proceeds as if there are no aggregates at all.

Bug 2: compileExpressionInternal doesn't handle agg type

In packages/db/src/query/compiler/evaluators.ts:68-92, the expression compiler only handles three types: val, ref, func. When the coalesce function tries to compile its arguments (line ~195), it recurses into compileExpressionInternal with the nested count(...) aggregate, which falls through to the default case and throws UnknownExpressionTypeError("agg").

Similarly in select.ts

addFromObject at select.ts:229 only checks isAggregateExpression(expression) at the top level. A Func containing a nested Aggregate is treated as a normal expression and passed to compileExpression, which ultimately hits the same bug in evaluators.ts.

Why live queries behave differently

The reporter's useLiveQuery example includes an explicit .groupBy(...) clause, which routes through processGroupBy — a code path that handles aggregates more directly. The standalone new Query() example relies on implicit single-group aggregation detection, which fails to detect nested aggregates.

Suggested Fix

The fix requires changes in three areas:

  1. packages/db/src/query/compiler/index.ts (~line 270) — Add a recursive helper containsAggregate(expr) that walks the expression tree to detect aggregates nested inside functions, not just top-level ones. Replace the shallow .some(expr => expr.type === 'agg') check with this recursive version. The same pattern appears again at line 288-289.

  2. packages/db/src/query/compiler/group-by.ts (~lines 89-96, 207-212) — The aggregate scanning loops (for (const [alias, expr] of Object.entries(selectClause))) only extract aggregates when expr.type === 'agg'. They need to recursively walk Func expressions to find and extract nested aggregates. For expressions like coalesce(count(x), 0), the system needs to:

    • Extract the inner count(x) as an aggregate with a synthetic alias
    • After groupBy computes the aggregate value, re-evaluate the outer coalesce(...) wrapping using the computed aggregate result (similar to how replaceAggregatesByRefs already works for HAVING clauses)
  3. packages/db/src/query/compiler/select.ts (~line 229)addFromObject needs to handle expressions that contain aggregates (not just are aggregates), deferring them as placeholders for group-by processing the same way direct aggregates are deferred.

The pattern in replaceAggregatesByRefs (group-by.ts:416-458) is a good reference — it already correctly walks Func nodes and recursively replaces nested aggregates with refs. A similar approach should be used for the SELECT/groupBy aggregate extraction.

Effort Estimate

Medium — The conceptual fix is well-scoped (recursive aggregate detection + extraction + post-groupBy re-evaluation), and there's a clear pattern to follow in replaceAggregatesByRefs. However, it touches the core compilation pipeline in three files, and both single-group and multi-group aggregation paths need updating. Testing edge cases (multiple nested aggregates, deeply nested, aggregates in HAVING with nesting) adds complexity.

Suggested Comment

Thanks for the clear repro and detailed write-up! The root cause is that the compiler only detects aggregates at the top level of SELECT expressions — when an aggregate is nested inside another function like coalesce(count(...), 0), it's invisible to the implicit GROUP BY detection, and the expression compiler doesn't know how to evaluate an agg node directly. We'd need to add recursive aggregate detection and extraction, similar to how the HAVING clause compiler already handles this via replaceAggregatesByRefs. Marking this as confirmed.

Confirmed! At line 370, collection is a value copy of the local variable at the time of return. At that point, collection is null because sync() hasn't been called yet. The sync callback and confirmOperationsSync capture the variable by reference through closures, but the returned object at line 370 captures the current value (null).

Then in acceptMutations (line 252), m.collection === syncResult.collection compares against the returned value which is permanently null.

Meanwhile, the confirmOperationsSync function correctly captures collection by closure reference (but it uses the captured sync functions, not the collection variable). The problem is specifically in acceptMutations trying to filter mutations by collection identity.

Now I have everything I need for the write-up.

Summary

When using a local-only collection with a manual transaction (autoCommit: false), items inserted inside tx.mutate() appear immediately (optimistically) but vanish on the next tick after tx.commit(). The acceptMutations utility meant to persist mutations for manual transactions is silently a no-op due to a bug, so the synced data is never written — when the transaction completes and optimistic state is cleared, the items disappear.

Root cause analysis

The bug is in createLocalOnlySync() in packages/db/src/local-only.ts:367-371.

The function declares a local let collection = null (line 299) and later captures it by closure reference inside the sync callback (line 314: collection = params.collection). However, the return statement at line 370 copies the current value of collection into the returned object — which is null at that point, since sync() hasn't been called yet.

Then in acceptMutations (line 249-253), mutations are filtered by:

const collectionMutations = transaction.mutations.filter(
  (m) => m.collection === syncResult.collection,  // always null!
)

This comparison always fails because syncResult.collection is permanently null. The filter returns an empty array, acceptMutations returns early, and confirmOperationsSync is never called.

Without synced data being written, when the transaction completes:

  1. recomputeOptimisticState clears optimistic state (transaction is completed, not active)
  2. commitPendingTransactions has nothing to commit (no sync write was ever made)
  3. The item disappears from both optimistic and synced state

The wrappedOnInsert/wrappedOnUpdate/wrappedOnDelete handlers work correctly for direct operations because they call confirmOperationsSync directly with params.transaction.mutations, bypassing the broken collection filter. That's why the bug only manifests with manual transactions.

Suggested fix

In packages/db/src/local-only.ts, the acceptMutations function needs to access the closure variable collection instead of the stale syncResult.collection value. Two approaches:

Option A (simplest): Change acceptMutations to use a getter that reads the closure variable:

In the acceptMutations function (line 249-253), replace syncResult.collection with a direct reference to the closure variable collection:

const collectionMutations = transaction.mutations.filter(
  (m) => m.collection === collection,  // use closure variable, not returned value
)

This requires moving acceptMutations into the createLocalOnlySync function body (or making collection accessible via a getter). Since acceptMutations is defined at line 245 in localOnlyCollectionOptionsoutside of createLocalOnlySync — it can't access the closure.

Option B (better): Return a getter for collection from createLocalOnlySync instead of the value:

return {
  sync,
  confirmOperationsSync,
  getCollection: () => collection,  // getter reads live closure value
}

Then update acceptMutations (line 252) to use syncResult.getCollection().

Option C: Move acceptMutations into createLocalOnlySync so it captures collection by closure reference.

Effort estimate

Small — The fix is a one-line change to how the collection reference is exposed, plus updating the single consumer. No architectural changes needed.

Suggested comment

Thanks for the clear reproduction @marbemac! The root cause is that acceptMutations compares mutations against a stale null collection reference — the createLocalOnlySync function returns the collection value before it's been assigned by the sync callback. This means the mutation filter always produces an empty array and the sync write never happens. Fix is straightforward — we need to expose the collection via a getter (or move acceptMutations into the closure). Working on a PR.

Now I have the full picture. Here's the investigation:


Summary

When a useLiveQuery uses a join with a collection configured with progressive syncMode (Electric collections), an error is thrown: "Snapshot requests are not supported in full mode, as the consumer is guaranteed to observe all data". The error also affects useLiveSuspenseQuery. Switching to on-demand mode works around it.

Root Cause Analysis

The error originates from the interaction between joins and progressive-mode Electric streams:

  1. Joins trigger requestSnapshot: When a join executes, the join compiler's tap operator (packages/db/src/query/compiler/joins.ts ~line 307) calls requestSnapshot on the lazy-side collection's subscription to load matching rows.

  2. Progressive mode uses a full-mode stream: Progressive collections create a ShapeStream in full mode (no log: 'changes_only'), because they need to sync the complete shape log. The @electric-sql/client's ShapeStream.requestSnapshot() explicitly rejects calls on full-mode streams, throwing the observed error.

  3. The old loadSubset didn't handle the buffering phase correctly: Before the fix, the electric collection's loadSubset callback had a single code path for both progressive and on-demand modes. It only guarded against calls after the initial sync completed (if (syncMode === 'progressive' && hasReceivedUpToDate) return). During the buffering phase (before full sync completes), stream.requestSnapshot() was called on a full-mode stream — which throws.

Call chain: Join tap → CollectionSubscription.requestSnapshot()CollectionSyncManager.loadSubset()DeduplicatedLoadSubset → electric's loadSubsetstream.requestSnapshot() → ERROR

Suggested Fix

This issue is already fixed by PR #852 (commit acb3e4f1, merged Nov 21, 2025).

The fix introduced createLoadSubsetDedupe() in packages/electric-db-collection/src/electric.ts:343 which correctly handles three phases:

Phase Behavior
Buffering (initial sync in progress) Uses stream.fetchSnapshot() (HTTP call, works in any mode) instead of requestSnapshot
Progressive, after sync No-op return (full stream already delivers all data)
On-demand Uses stream.requestSnapshot() (stream is in changes_only mode, so it works)

The key insight: fetchSnapshot is a standalone HTTP request that works regardless of stream mode, while requestSnapshot is a stream-level operation restricted to changes_only mode.

Effort Estimate

Already resolved — the fix was a Medium effort change, involving a new helper function with proper phase-aware logic and error handling.

Suggested Comment

This was fixed in PR #852 (released in subsequent versions). The root cause was that during the buffering phase of progressive sync, stream.requestSnapshot() was being called on a full-mode stream, which isn't supported. The fix uses stream.fetchSnapshot() (a standalone HTTP call) during buffering instead. If you're still seeing this, please update to the latest packages and let us know!

Now I have a thorough understanding of the codebase. Here's my investigation:

Summary

The author built a createSupabaseCollection helper that bridges TanStack DB's collection/query system with Supabase's PostgREST client. The core challenge is mapping TanStack DB's expression-based filter/sort DSL (via parseLoadSubsetOptions) to Supabase's query builder methods (.eq(), .order(), etc.) while preserving TypeScript type safety. The author reports significant pain around type inference (lots of any/unknown casts), Supabase-specific join handling, and incomplete operator coverage.

Root cause analysis

This isn't a bug — it's a feature/integration gap. The existing infrastructure is actually well-suited for a Supabase collection package:

  1. Expression helpers already exist and work well. parseLoadSubsetOptions in packages/db/src/query/expression-helpers.ts returns { filters: SimpleComparison[], sorts: ParsedOrderBy[], limit? } — the exact shape the author's code consumes. The more powerful parseWhereExpression with custom handlers could eliminate the author's manual switch/case operator mapping entirely.

  2. The query-db-collection package is the right foundation. The author is already using queryCollectionOptions + on-demand sync mode correctly. The pattern matches what TrailBase, RxDB, and PowerSync collections do.

  3. The typing problems stem from bridging two incompatible type systems. Supabase generates deep generic types (Database["public"]["Tables"][T]["Row"] etc.) that don't align with TanStack DB's simpler T extends Record<string, unknown> generic. The author's GenericDatabase/GenericTable types are a workaround but lose Supabase's precise type inference.

  4. Join handling is fundamentally a Supabase-specific concern. TanStack DB's expression system deals with flat field paths (['users', 'profile_slug']), but Supabase needs foreign key alias syntax (users!pages_created_by_fkey.profile_slug). This mapping can't be generalized without schema metadata.

Suggested fix

This should become a new supabase-db-collection package in the monorepo, following the existing collection package patterns:

  1. Create packages/supabase-db-collection/ modeled after packages/trailbase-db-collection/ — it's the closest analog (REST-like API, on-demand sync, single-table queries).

  2. Use parseWhereExpression with Supabase-specific handlers instead of parseLoadSubsetOptions + manual switch. This would look like:

    const filters = parseWhereExpression(where, {
      handlers: {
        eq: (field, value) => (q) => q.eq(field.join('.'), value),
        gt: (field, value) => (q) => q.gt(field.join('.'), value),
        // ...
        and: (...fns) => (q) => fns.reduce((q, fn) => fn(q), q),
      }
    })

    This eliminates the switch/case entirely and is extensible.

  3. For types, accept Supabase's generated Database type as a generic parameter and use conditional types to extract Row/Insert/Update per table. The key insight is to constrain the collection's T to the Row type and provide typed mutation callbacks.

  4. For joins, keep them out of the generic helper. The author's approach of conditional select strings based on filter content is reasonable — document it as a pattern rather than trying to generalize it.

  5. Optionally add Supabase Realtime subscription support for eager sync mode, similar to how electric-db-collection subscribes to Electric's ShapeStream.

Key files to reference:

  • packages/db/src/query/expression-helpers.ts — the parseWhereExpression API (line 201) is more powerful than parseLoadSubsetOptions and should be the primary tool
  • packages/query-db-collection/src/query.tsqueryCollectionOptions config interface
  • packages/trailbase-db-collection/src/trailbase.ts — closest existing pattern for a REST-backed collection

Effort estimate

Medium — The core wrapper is ~200 lines (the author already has a working draft). The real work is:

  • Setting up the package scaffolding (copy from trailbase-db-collection)
  • Getting types right with Supabase's generated Database type
  • Adding Supabase Realtime support for eager mode
  • Writing tests and docs

Suggested comment

Thanks for the thorough exploration and working code, @damien-schneider! Your createSupabaseCollection approach looks solid — using queryCollectionOptions with on-demand sync is exactly the right pattern.

One suggestion: instead of the manual switch/case for operators, try using parseWhereExpression with custom handlers — it composes Supabase query builder calls directly and handles and/not nesting automatically. For types, constraining the generic to Supabase's Database["public"]["Tables"][T] type and letting Row/Insert/Update flow from there should reduce the casting. We'd welcome this as a supabase-db-collection community package if you're interested in contributing it!

I now have a complete picture. Here's the investigation:


Summary

After signing in for the first time in the React "projects" example, the user is redirected to a URL like /project/34853 and hits a crash: Cannot read properties of undefined (reading 'name'). The random project ID comes from an auto-created "Default" project, but the project detail component renders before the data is available, accessing properties on undefined.

Root cause analysis

There are two interacting problems:

1. Race condition between project creation and navigation

The sign-in flow works like this:

  1. Login → hard navigate to / (_authenticated/index.tsx:32-54)
  2. AuthenticatedLayout (_authenticated.tsx:27-39) fires a useEffect that creates a default project with id: Math.floor(Math.random() * 100000) when projects.length === 0
  3. IndexRedirect (_authenticated/index.tsx:36-44) fires a separate useEffect that navigates to the first project once projects.length > 0

These two effects are in separate components (parent layout vs. child route). The insert happens in the layout's effect, then the child's useLiveQuery reactively picks it up and navigates. This sequence generally works, but the project detail page it navigates to has no guard.

2. No null check on project in $projectId.tsx

At $projectId.tsx:59, const project = projects[0] can be undefined when:

  • The query hasn't resolved yet (initial render before data loads)
  • The projectId in the URL doesn't match any project in the collection

The component then unconditionally accesses project.name (line 100), project.description (lines 108, 117), project.owner_id (lines 181, 187), and project.id (lines 94, 111, 199, 206) — all of which crash with TypeError: Cannot read properties of undefined.

The same pattern applies to usersInProject at line 50 — usersInProjects[0] can also be undefined.

Suggested fix

File: examples/react/projects/src/routes/_authenticated/project/$projectId.tsx

Add an early return guard after line 59:

const project = projects[0]
const usersInProject = usersInProjects[0]

if (!project || !usersInProject) {
  return (
    <div className="p-6">
      <div className="text-center py-8">
        <p className="text-gray-500">Project not found.</p>
      </div>
    </div>
  )
}

This addresses both the race condition (data not yet loaded) and the case where a stale/invalid project ID is in the URL.

Optionally, the random ID generation (Math.floor(Math.random() * 100000)) in _authenticated.tsx:30 and _authenticated.tsx:49 could be replaced with something more robust (e.g., a UUID or timestamp-based ID) to avoid collisions, but that's a secondary concern.

Effort estimate

Small — a single guard clause in one file.

Suggested comment

Thanks for reporting this! The root cause is a missing null check in the project detail component — when you first sign in, a default project is auto-created with a random ID, and the component tries to render before the data is available from the query. The fix is a simple early-return guard in $projectId.tsx when project is undefined. We'll get a PR up for this.

Now I have a complete picture. Here's the investigation:


Summary

When two collections are joined on a Uint8Array key (e.g., a ULID stored as bytes), the join fails to match rows if the two sides hold different Uint8Array instances with identical content. This is because the Index class in db-ivm uses a JavaScript Map for key lookups, and Map uses === (reference equality) for object keys. PR #779 fixed this for BTree indexes and WHERE clause evaluation in the db package, but the same normalization was never applied to the db-ivm Index class used by joins.

Root cause analysis

The Index class in packages/db-ivm/src/indexes.ts backs all join operations. It stores join keys directly in a Map<TKey, ...> (#inner at line 152 and #consolidatedMultiplicity at line 153).

When the JoinOperator calls Index.fromMultiSets() (line 164), keys from each side of the join are inserted as-is. Later, Index.join() (line 440) iterates one index and calls other.has(key) / other.get(key) on the other. These are plain Map.has() / Map.get() calls, which use === for object keys.

Two separate Uint8Array instances with the same bytes are !==, so they never match.

★ Insight ───────────────────────────────────── The hashing layer (PR #779) already hashes small Uint8Arrays by content in hash.ts:52-58, and the db package's normalizeValue() in comparison.ts:152-175 converts them to strings for Map key usage. But neither of these is invoked when keys enter the db-ivm Index — there's a normalization gap between the two packages. The Index class is generic and has no knowledge of Uint8Array normalization. ─────────────────────────────────────────────────

Specifically, every method that touches #inner or #consolidatedMultiplicity as a Map (.get(key), .has(key), .set(key, ...), .delete(key)) uses the raw key without normalization:

  • has() — line 203
  • hasPresence() — line 212
  • getConsolidatedMultiplicity() — line 221
  • getIterator() — line 247
  • addValue() — lines 307, 309, 314, 318

Suggested fix

Add a normalizeKey() function inside packages/db-ivm/src/indexes.ts (similar to the existing normalizeValue() in packages/db/src/utils/comparison.ts) that converts small Uint8Arrays (≤128 bytes) to a deterministic string representation (e.g., "__u8__1,2,3,..."). Apply it at every point where keys are used for Map operations in the Index class.

Key changes needed:

  1. packages/db-ivm/src/indexes.ts: Add a normalizeKey<K>(key: K): K | string helper. Wrap all #inner.get/has/set/delete(key) and #consolidatedMultiplicity.get/has/set/delete(key) calls to use normalizeKey(key).

  2. Preserve original keys for iteration: The entriesIterators() and getPresenceKeys() methods yield keys back to callers (the join operator uses them). Maintain a parallel #originalKeys: Map<normalized, original> to return the original Uint8Array instances rather than the normalized strings.

  3. Test: Add a test in packages/db-ivm/tests/operators/join.test.ts that creates two separate Uint8Array instances with identical bytes, puts them in different sides of a join, and verifies the join matches by content.

A dangling commit ac158fb0 already implements this exact fix but was never merged to any branch.

Effort estimate

Small — The normalization function is straightforward (already proven in the db package), and the changes are localized to one file. The dangling commit ac158fb0 can serve as a reference implementation.

Suggested comment

Thanks for reporting this with a clear repro! The root cause is that the Index class in db-ivm uses JavaScript Map lookups with raw Uint8Array keys, which compare by reference (===) rather than content. We already have content-based normalization for Uint8Arrays in the db package's BTree indexes (from PR #779), but it wasn't extended to the db-ivm join index. We'll apply the same normalization pattern there.

Now I have a complete understanding of the issue. Let me write the investigation.

Summary

When using powersync-db-collection, tx.isPersisted.promise resolves when the mutation is written to the local PowerSync SQLite table and the change is observed back via the diff trigger — NOT when the data has been uploaded to the server and synced back with a server-confirmed ID. This means there's no way to await the full server round-trip, making it impossible to use the view-key mapping pattern to swap a temporary optimistic ID for a server-confirmed ID.

Root Cause Analysis

The flow works as follows:

  1. collection.insert() → creates a transaction whose mutationFn calls PowerSyncTransactor.applyTransaction() (powersync.ts:446-448)
  2. applyTransaction() writes to the local PowerSync SQLite table (PowerSyncTransactor.ts:174-182), then waits for the local diff trigger to observe the write (PowerSyncTransactor.ts:138-149)
  3. Once the diff trigger fires and PendingOperationStore.resolvePendingFor() resolves the pending operation (powersync.ts:348), applyTransaction() returns
  4. Back in transactions.ts:498, isPersisted.resolve(this) fires

The critical gap: PowerSync uploads to the server happen in the background via PowerSync's upload queue — completely decoupled from this flow. The isPersisted promise resolves after step 3 (local SQLite write observed), not after the server has processed the upload and synced the confirmed data back.

This means:

  • await tx.isPersisted.promise resolves before the server has the data
  • There is no signal for when the server-confirmed record (with potentially a different ID) arrives back via sync
  • The view-key mapping pattern from the docs can't work because you never know when the "real" record appears

As stevensJourney (PowerSync contributor) confirmed in the comments: the current architecture is offline-first, and mutations resolve once the local SQLite table reflects the change, not once the server confirms it.

Suggested Fix

This needs a configurable "online-first" mode for the PowerSync collection, as stevensJourney suggested. The implementation would involve:

  1. packages/powersync-db-collection/src/PowerSyncTransactor.ts: After writing to local SQLite and observing the diff trigger, optionally wait for PowerSync's upload queue to complete for that specific CRUD entry. PowerSync's SDK likely exposes upload status/completion events that could be used.

  2. packages/powersync-db-collection/src/powersync.ts: Add a configuration option (e.g., awaitServerSync: boolean or syncMode: 'offline-first' | 'online-first') to BasePowerSyncCollectionConfig. When enabled, the onInsert/onUpdate/onDelete handlers would additionally await the server upload confirmation before returning.

  3. packages/powersync-db-collection/src/powersync.ts (sync callback, ~line 307-363): The diff trigger observer would need to distinguish between "local write observed" and "server-confirmed sync observed" — the latter being when a record arrives through PowerSync's downstream sync (not the local write echo).

  4. For the ID swap mechanism: once the server sync is awaitable, the existing view-key mapping pattern from the docs would work. No core DB changes needed — this is purely a PowerSync collection concern.

The main challenge is that PowerSync's upload queue is asynchronous and decoupled. The PowerSync SDK would need to provide a way to correlate a specific CRUD entry with its upload completion, which may require coordination with the PowerSync team.

Effort Estimate

Large — This requires:

  • PowerSync SDK-level coordination (may need new APIs from @powersync/common)
  • New configuration surface for the collection
  • Careful handling of the offline case (what happens when there's no connectivity?)
  • Testing across the upload → server confirm → sync-back lifecycle
  • Close collaboration with the PowerSync team (stevensJourney)

Suggested Comment

Thanks for the detailed report, Nick. You've identified the core issue accurately — isPersisted currently resolves after the local SQLite write is observed, not after the full server round-trip. As stevensJourney noted, this is a limitation of the current offline-first architecture. Supporting an online-first mode where mutations can optionally await server confirmation is something we'd like to enable, but it requires coordination with the PowerSync SDK for upload completion tracking. We'll keep this open and work with the PowerSync team on a path forward.

Now I have the complete root cause. The key line is 671:

const extendedMeta = { ...meta, loadSubsetOptions: opts }

Where opts is a LoadSubsetOptions that includes a subscription property — which is a full CollectionSubscription instance with methods, event emitters, callbacks, etc. This non-serializable object ends up in query.meta, and when @tanstack/react-query-persist-client tries to persist the query cache to IndexedDB using structuredClone / IDBObjectStore.put(), it fails because functions can't be cloned.

Additionally, LoadSubsetOptions.where is a BasicExpression<boolean> which itself could contain non-serializable data depending on its structure. And LoadSubsetOptions.cursor with CursorExpressions could also be problematic.

The error message from the issue — (event) => options.onUnsubscribe(event) could not be cloned — points directly to the CollectionSubscription object's event handler registered at subscription.ts:115.

Here's my investigation report:


Summary

When using syncMode: "on-demand" with @tanstack/query-db-collection, the full LoadSubsetOptions object — including a live CollectionSubscription instance — gets attached to TanStack Query's query.meta. When @tanstack/react-query-persist-client tries to serialize the query cache to IndexedDB via structuredClone, it fails with a DataCloneError because functions and class instances can't be cloned.

Root Cause Analysis

★ Insight ───────────────────────────────────── The root cause is a classic serialization boundary violation — like trying to put a living thing into the Ark of the Covenant rather than just a description of it. Internal runtime state is being stored where only serializable data should go. ─────────────────────────────────────────────────

The chain of events:

  1. CollectionSubscription calls loadSubset with subscription: this — at packages/db/src/collection/subscription.ts:367 and :608, the subscription passes itself as part of LoadSubsetOptions.

  2. LoadSubsetOptions includes the subscription field — defined at packages/db/src/types.ts:311:

    subscription?: Subscription

    This Subscription is a full CollectionSubscription instance — an EventEmitter subclass with methods, closures, and registered event handlers (including the onUnsubscribe callback at subscription.ts:115).

  3. The raw opts object is placed into query meta — at packages/query-db-collection/src/query.ts:671:

    const extendedMeta = { ...meta, loadSubsetOptions: opts }

    This attaches the entire LoadSubsetOptions (including the subscription property) to the TanStack Query observer's metadata.

  4. Persist client tries to serialize query.meta@tanstack/react-query-persist-client calls IDBObjectStore.put() on the dehydrated query cache, which uses structuredClone internally. The subscription object and its (event) => options.onUnsubscribe(event) handler cannot be cloned, producing the exact error from the issue.

The error message (event) => options.onUnsubscribe(event) could not be cloned maps directly to subscription.ts:115:

this.on(`unsubscribed`, (event) => options.onUnsubscribe!(event))

Suggested Fix

File: packages/query-db-collection/src/query.ts, line 671

The loadSubsetOptions stored in meta should only contain serializable data. The fix is to strip the subscription property (and any other non-serializable fields) before storing in meta:

// Before (broken):
const extendedMeta = { ...meta, loadSubsetOptions: opts }

// After (fixed):
const { subscription, ...serializableOpts } = opts
const extendedMeta = { ...meta, loadSubsetOptions: serializableOpts }

Alternatively, a more defensive approach would be to only store the serializable subset of LoadSubsetOptions that the meta actually needs downstream:

const extendedMeta = {
  ...meta,
  loadSubsetOptions: {
    where: opts.where,
    orderBy: opts.orderBy,
    limit: opts.limit,
    offset: opts.offset,
    cursor: opts.cursor,
  }
}

Secondary consideration: Verify that where (BasicExpression<boolean>) and cursor (CursorExpressions) are themselves serializable. If they contain function references, those would need similar treatment. A quick check of serializeLoadSubsetOptions in packages/query-db-collection/src/serialization.ts shows these are JSON-serializable expression objects, so they should be fine.

Verify usage: Check whether anything downstream reads meta.loadSubsetOptions.subscription. If so, that code path would need an alternative way to access the subscription (e.g., via a WeakMap keyed by query hash). A grep for loadSubsetOptions usage sites would confirm.

Effort Estimate

Small — This is a one-line destructuring fix at the point where loadSubsetOptions is placed in meta. The subscription field was added for sync layer use (LRU caching, reference counting), not for persistence in query metadata. Stripping it before placing in meta preserves all intended functionality while making the data serializable.

Suggested Comment

Thanks for the detailed report and reproduction, @HaleyBoles! The root cause is that the LoadSubsetOptions object placed into query.meta at query.ts:671 carries a live CollectionSubscription instance (via the subscription field), which contains functions that can't be serialized by IndexedDB's structuredClone. The fix is to strip non-serializable properties (particularly subscription) before attaching to meta. This should be a small, targeted fix.

Now I have a thorough understanding. Let me write the investigation report.

Summary

When using syncMode: "on-demand" with a query collection, calling liveQuery.preload() followed by liveQuery.stateWhenReady() returns immediately with 0 items. The stateWhenReady() promise resolves before the async loadSubset fetch has completed, so the data appears empty until a manual delay (e.g., sleep(2000)).

Root cause analysis

This has been largely fixed across multiple PRs. The core issue was a race condition: the live query collection was marked "ready" before its on-demand loadSubset promise had resolved. The chain of fixes:

  1. PR #1081 (c1247e81) — The primary fix. Added an isLoadingSubset gate to updateLiveQueryStatus() in collection-config-builder.ts:842. The live query now checks three conditions before calling markReady(): all subscriptions set up, all source collections ready, AND !isLoadingSubset. A loadingSubset:change event listener (line 582-590) re-evaluates readiness when loading completes.

  2. PR #1192 (284ebcc8) — Follow-up fix for non-ordered on-demand live queries. When subscribeToMatchingChanges was called with includeInitialState: true, the loadSubset promise wasn't being tracked on the live query collection, so isLoadingSubset remained false. Fixed by piping the onLoadSubsetResult callback through collection-subscriber.ts:228-238.

The preload() method on CollectionSyncManager (sync.ts:248-294) resolves via onFirstReady, which now correctly waits for loadSubset to finish. stateWhenReady() (collection/index.ts:771-779) delegates to preload(), so both are gated properly.

Suggested fix

This issue appears to already be fixed on main by the combination of PR #1081 and PR #1192. The key code paths are:

  • packages/db/src/query/live/collection-config-builder.ts:825-845updateLiveQueryStatus() with the 3-condition gate
  • packages/db/src/query/live/collection-config-builder.ts:578-591loadingSubset:change listener registered before subscriptions
  • packages/db/src/query/live/collection-subscriber.ts:73-80, 228-238trackLoadPromise propagation for both ordered and non-ordered queries
  • packages/db/src/collection/sync.ts:307-337trackLoadPromise managing isLoadingSubset state

One gap: There are no dedicated integration tests for the exact reproduction case in the issue (calling preload() then stateWhenReady() on a live query backed by an on-demand source collection). Adding a test like the one in the issue would prevent regression.

Effort estimate

Small — The bug is already fixed. The remaining work is: (1) verify the fix works against the reporter's reproduction, (2) optionally add a targeted integration test, (3) close the issue.

Suggested comment

This should now be fixed on main through PR #1081 and PR #1192. The root cause was that the live query was being marked "ready" before the on-demand loadSubset fetch completed — we now gate readiness on isLoadingSubset being false. If you're still seeing this on the latest release, please let us know and we'll reopen. Thanks for the detailed reproduction!

That confirms it — normalizeValue is only called inside the eq function evaluator (used in WHERE clauses), and never in the db-ivm join operator. Now I have a complete picture.


Summary

When using eq() with Date objects in a join condition, the comparison fails because Date objects are compared by reference (===) rather than by value (getTime()). The eq() function in WHERE clauses correctly normalizes Dates via normalizeValue(), but the join operator bypasses this normalization entirely — it uses raw keys in a Map for matching, which relies on JavaScript's referential equality.

Root cause analysis

The join pipeline in packages/db/src/query/compiler/joins.ts (lines 188-215) extracts join keys from both sides using compileExpression() and passes them directly to the IVM join operator:

const mainKey = compiledMainExpr(namespacedRow)    // line 191 — raw Date object
const joinedKey = compiledJoinedExpr(namespacedRow) // line 208 — different Date object

These keys flow into packages/db-ivm/src/indexes.ts, where the Index class stores them in a Map<TKey, ...>. The join matching at Index.join() (~line 448) uses Map.has(key), which does === comparison. Two Date objects with the same timestamp are different references, so they never match.

Meanwhile, the eq function used in WHERE/filter clauses (packages/db/src/query/compiler/evaluators.ts:205-206) does call normalizeValue() which converts Dates to epoch numbers — so eq() works fine in filters, just not in joins.

This also explains why the reporter's concat() workaround worked: when either join expression is a func type, hasComputedJoinExpr becomes true (joins.ts:235-236), which disables the optimized index-based join path and falls back to a slower path that presumably evaluates eq() through the normal expression evaluator (which does normalize).

Suggested fix

Normalize join keys before they enter the Map-based join. The most surgical fix is to apply normalizeValue() to the extracted join keys in packages/db/src/query/compiler/joins.ts, in the two map() pipelines that prepare data for the join operator:

  1. Line 191 — wrap compiledMainExpr(namespacedRow) with normalizeValue():

    const mainKey = normalizeValue(compiledMainExpr(namespacedRow))
  2. Line 208 — wrap compiledJoinedExpr(namespacedRow) with normalizeValue():

    const joinedKey = normalizeValue(compiledJoinedExpr(namespacedRow))

This ensures Date objects become epoch numbers (and Uint8Arrays become string keys) before they're used as Map keys in the join index. It mirrors what eq() already does in evaluators.ts.

Additionally, the lazy-loading path (line 305) extracts join keys for index lookups:

const joinKeys = data.getInner().map(([[joinKey]]) => joinKey)

These keys are already normalized at this point (since they came through the map() pipeline above), but the index on the lazy collection stores raw (unnormalized) values. The index lookup in requestSnapshot / inArray would also need to normalize, or the BTree index itself would need to store normalized keys. Check packages/db/src/indexes/auto-index.ts and how ensureIndexForField builds the index — the index key extraction likely needs normalizeValue() applied too.

Files to modify:

  • packages/db/src/query/compiler/joins.ts — normalize keys in the map pipelines (lines ~191, ~208)
  • Possibly packages/db/src/indexes/ — ensure BTree indexes used for lazy join lookups also normalize keys consistently

Effort estimate

Medium — The core fix (normalizing join keys) is straightforward, but ensuring consistency with the lazy-loading index path and adding proper test coverage for Date joins (and Uint8Array joins) adds scope. Need to verify the fix doesn't break the optimized join path where index lookups must match the normalized keys.

Suggested comment

This is confirmed — the root cause is that join keys bypass normalizeValue(). In WHERE clauses, eq() normalizes Dates to epoch timestamps (via normalizeValue in evaluators.ts), but the join operator extracts keys directly and uses them in a Map which compares by reference. The fix is to apply normalizeValue() to join keys in joins.ts lines ~191 and ~208 before they enter the join pipeline. Your concat() workaround happened to work because it made hasComputedJoinExpr true, which disables the optimized index-based join and falls back to the expression-evaluator path that does normalize. We'll get a fix in — thanks for the thorough investigation, @bradleyayers!

Now I have a clear picture. Let me write the investigation.


Summary

When using PowerSync with the OPFSCoopSyncVFS (OPFS-based SQLite), mutations made in one tab are not consistently visible in other tabs' TanStack DB collections. The first tab sees all changes, and each mutating tab sees its own changes, but sibling tabs miss each other's updates. The issue does not occur with IDBBatchAtomicVFS (IndexedDB-backed).

Root cause analysis

This was an upstream PowerSync issue, not a TanStack DB bug. The cross-tab change detection depends on PowerSync's internal SQLite trigger/diff mechanism (database.triggers.createDiffTrigger and database.onChangeWithCallback in packages/powersync-db-collection/src/powersync.ts:307-362). Each tab creates its own uniquely-named tracking table (line 282-286) and listens for changes via onChangeWithCallback scoped to that tracking table (line 361).

The problem was that when using OPFS, PowerSync's trigger mechanism didn't properly fire across tabs — the diff triggers that populate the tracking tables weren't being invoked in non-originating tabs. This meant the onChange callback (line 309) never fired in other tabs, so TanStack DB collections never received the updates, even though the underlying SQLite data was correct.

PowerSync maintainer @stevensJourney confirmed this in the upstream issue powersync-js#787 and shipped the fix in @powersync/[email protected].

Suggested fix

No code changes needed in this repo. This is purely an upstream dependency fix. The resolution for end users is to upgrade to @powersync/web >= 1.32.0 in their applications.

Since @tanstack/powersync-db-collection uses @powersync/common as a peer dependency (currently ^1.41.0 in package.json:62), the fix is in the user's @powersync/web package, not something TanStack DB controls.

Optionally, the docs at docs/collections/powersync-collection.md could note the minimum recommended @powersync/web version for OPFS cross-tab support.

Effort estimate

Small — This issue can be closed as resolved upstream. The only optional action is a docs note about minimum PowerSync version.

Suggested comment

This has been resolved upstream in @powersync/[email protected] as @stevensJourney noted. Users experiencing this should upgrade their @powersync/web dependency. Closing this issue — thanks for the report @Nick-Motion and thanks @stevensJourney for the quick fix!

Good. The id field is optional in BaseCollectionConfig. Now let me check what restConfig would contain — since id is not destructured, it passes through in restConfig. But importantly, acceptMutations at line 249-252 compares m.collection === syncResult.collection, and syncResult.collection is the stale null captured at object creation time (line 370).

Here's my investigation:


Summary

When using localOnlyCollectionOptions with manual transactions that call utils.acceptMutations(), mutations appear during optimistic state but vanish when the transaction completes. The same pattern works correctly with localStorageCollectionOptions. The bug was reported in Dec 2025 and confirmed by a maintainer, but has not been fixed.

Root cause analysis

The bug is a stale closure / captured-by-value issue in packages/db/src/local-only.ts.

In createLocalOnlySync() (line 291), a local collection variable starts as null (line 299). It gets updated inside the sync() callback (line 314) when the collection initializes. However, the return object at line 367-371 captures collection by value at creation time — always null:

return {
  sync,
  confirmOperationsSync,
  collection, // ← captured as null, never updates
}

Then in acceptMutations (line 249-252), the filter compares:

m.collection === syncResult.collection  // always null → never matches

Since no mutations match, confirmOperationsSync is never called, and optimistic mutations are cleared when the transaction completes.

Why localStorage works: local-storage.ts (lines 554-561) has a fallback that matches by collection ID (m.collection.id === collectionId), so it doesn't rely on the potentially-stale collection reference.

Suggested fix

File: packages/db/src/local-only.ts

Two changes needed:

  1. Lines 182+: Extract and track the id from config, similar to local-storage.ts:

    const { initialData, onInsert, onUpdate, onDelete, id, ...restConfig } = config
    const collectionId = id ?? crypto.randomUUID()
  2. Lines 249-253: Add the ID-based fallback to acceptMutations, matching the pattern in local-storage.ts:

    const collectionMutations = transaction.mutations.filter((m) => {
      if (syncResult.collection && m.collection === syncResult.collection) {
        return true
      }
      return m.collection.id === collectionId
    })
  3. Add id: collectionId to the return object (line 266) to ensure the collection has a stable, known ID.

Alternatively, createLocalOnlySync could return a getter for collection instead of a stale value, but the ID-based fallback is simpler, matches the localStorage pattern, and is the approach already validated by the reporter's patch and PR #723.

Effort estimate

Small — ~10 lines changed in one file, well-understood fix with a known-good pattern in the sibling implementation.

Suggested comment

This is still open — the root cause is confirmed: syncResult.collection is captured as null by value when createLocalOnlySync returns, so acceptMutations never matches any mutations. The fix is to add an ID-based fallback in acceptMutations, matching the pattern already used in localStorageCollectionOptions. We'll get a PR up for this.

Now I have the full picture. Let me write the investigation.


Summary

The reporter uses useLiveInfiniteQuery with a queryCollectionOptions-backed collection (eager sync mode) expecting TanStack Query-style server-side pagination — passing getNextPageParam, initialPageParam, and fetchNextPage. Once all items from the initial fetch are displayed, scrolling further does nothing: no additional server requests are made, and the list stops updating.

Root Cause Analysis

There are two interrelated problems:

1. getNextPageParam / pageParam are accepted in the types but never used

useLiveInfiniteQuery accepts getNextPageParam and initialPageParam in its config type (UseLiveInfiniteQueryConfig at packages/react-db/src/useLiveInfiniteQuery.ts:26-31), but the implementation never calls getNextPageParam. Pagination is driven entirely by an internal loadedPageCount counter and a peek-ahead mechanism:

  • The hook requests loadedPageCount * pageSize + 1 items via setWindow (line 198)
  • hasNextPage is true only if the returned data exceeds loadedPageCount * pageSize (line 261)
  • fetchNextPage simply increments loadedPageCount (line 291)

So pageParam is always undefined from the user's perspective — the hook never computes or passes page params to the collection's queryFn.

2. Eager sync mode ignores loadSubset — no server pagination

The default syncMode for queryCollectionOptions is eager (packages/query-db-collection/src/query.ts:550). In eager mode:

  • loadSubset is set to undefined (line 1124)
  • When the pipeline's loadMoreIfNeeded fires, CollectionSyncManager.loadSubset() returns true immediately (packages/db/src/collection/sync.ts:347-348)
  • No additional server request is ever made

The data flow dead-ends:

fetchNextPage() → setWindow({ limit: larger }) → pipeline re-runs → 
loadMoreIfNeeded() finds dataNeeded > 0 → loadSubset() → returns true (no-op in eager mode) → 
no new data arrives → hasNextPage stays false → pagination stops

The fundamental mismatch

useLiveInfiniteQuery is designed for client-side windowed pagination over data already in the collection. It assumes expanding the window will surface more local data (or trigger loadSubset in on-demand mode). It is not wired up for TanStack Query-style server-side page-param-based pagination, despite accepting getNextPageParam in its type signature.

The reporter expects the behavior of TanStack Query's useInfiniteQuery (where fetchNextPage triggers queryFn with the next pageParam), but useLiveInfiniteQuery operates on a completely different model.

Suggested Fix

This needs a design decision. Two possible directions:

Option A: Make useLiveInfiniteQuery support server-side pagination (Medium effort)

  • Wire up getNextPageParam to actually compute page params and pass them through to the collection's loadSubset or directly to the queryFn
  • This would require changes in packages/react-db/src/useLiveInfiniteQuery.ts to call getNextPageParam, and potentially in the query-db-collection package to support parameterized fetching
  • Would match user expectations coming from TanStack Query

Option B: Rely on on-demand sync mode + better docs (Small effort)

  • Document that useLiveInfiniteQuery requires syncMode: 'on-demand' for server-paginated collections
  • Ensure on-demand mode's loadSubsetcreateQueryFromOpts properly handles the cursor/offset-based loading that the pipeline already requests
  • Remove or deprecate getNextPageParam/initialPageParam from the config type if they're not used, to avoid confusion

Immediate cleanup regardless of direction

  • Remove getNextPageParam from UseLiveInfiniteQueryConfig or actually implement it — currently it's a misleading API that accepts params it ignores
  • Clarify in docs that pageParam is not passed to queryFn

Key files:

  • packages/react-db/src/useLiveInfiniteQuery.ts — the hook implementation
  • packages/query-db-collection/src/query.ts:550,1124 — syncMode default and loadSubset wiring
  • packages/db/src/collection/sync.ts:345-361loadSubset eager bypass

Effort Estimate

Medium — The core issue is an API/design mismatch. Fixing the misleading types is small, but properly supporting server-side pagination through useLiveInfiniteQuery requires coordinating changes across react-db, db core, and query-db-collection.

Suggested Comment

Thanks for the detailed report! You've identified a real gap — useLiveInfiniteQuery currently does client-side windowed pagination over data already in the collection, and doesn't trigger new server fetches via pageParam the way TanStack Query's useInfiniteQuery does. The getNextPageParam config is accepted in the types but isn't actually wired up in the implementation yet. We need to either properly support server-side pagination or clarify the expected usage with syncMode: 'on-demand'. We'll look into the best path forward here.

Summary

This is a request to list TanStack DB on the Local-First Landscape — a community directory of local-first software projects. It's not a bug report. The commenter clarifies that the process requires the TanStack org to create a dedicated data repository (following a specific format) and submit a PR to the landscape project.

Root cause analysis

There's no code issue here. This is purely an external marketing/community task. The Local-First Landscape project works by having each listed project maintain a repo with a data.js file containing structured metadata (name, description, features, links, etc.). The landscape site then periodically fetches from these repos. TanStack DB simply hasn't submitted one yet.

The referenced example PR (localfirstfm/local-first-landscape#55) and the CouchDB example (neighbourhoodie/couchdb-landscape-data) show what's needed:

  1. Create a new repo (e.g., TanStack/tanstack-db-landscape-data) with a data.js file describing TanStack DB's features, categories, and metadata.
  2. Open a PR to localfirstfm/local-first-landscape registering the new data repo.

Suggested fix

No code changes needed in this repository. The action items are:

  1. Create a new repo under the TanStack GitHub org (e.g., TanStack/tanstack-db-landscape-data) containing a data.js that fills out the landscape schema — categories like sync approach, conflict resolution, storage, supported frameworks, etc.
  2. Submit a PR to localfirstfm/local-first-landscape pointing to the new data repo (similar to PR #55).

Key metadata to include based on the codebase:

  • Sync: Supports multiple sync backends (Electric SQL, TanStack Query, RxDB, PowerSync, TrailBase)
  • Storage: In-memory with optional IndexedDB persistence (proposed)
  • Conflict resolution: Pluggable via collection implementations
  • Frameworks: React, Solid, Svelte, Vue, Angular
  • License: MIT

Effort estimate

Small — This is a 1-2 hour task of filling out a structured data file and opening a PR. No code changes to this repo.

Suggested comment

Thanks for the suggestion and for explaining how the process works! We'll create a landscape data repo under the TanStack org and submit a PR to get listed. Appreciate the nudge — it's a great directory for local-first projects.

Now I have a thorough understanding of the codebase. Let me write the investigation.

Summary

The author requests a way to enforce that collection mutations (insert, update, delete) can only be called inside createOptimisticAction (or transactions), preventing "inline" mutations directly in event handlers. The motivation is that most real-world mutations have side effects, and team members / coding agents often bypass actions for direct mutations. One community commenter suggests solving this at the module-boundary level with tools like jsboundaries.

Root cause analysis

This isn't a bug — it's a missing feature. Currently, the mutation methods in CollectionMutationsManager (packages/db/src/collection/mutations.ts) check for an ambient transaction via getActiveTransaction() (from packages/db/src/transactions.ts:175-181), but only to decide how to route the mutation:

  • With ambient transaction (inside an action/transaction mutate() callback): mutations join the transaction (mutations.ts:214-221)
  • Without ambient transaction: mutations create their own standalone transaction using onInsert/onUpdate/onDelete handlers (mutations.ts:222+)

The infrastructure to detect "are we inside an action?" already exists — getActiveTransaction() returns undefined when called outside a transaction.mutate() callback. What's missing is a collection-level config option to reject mutations when no ambient transaction is active.

Suggested fix

Approach: Add an actionsOnly option to BaseCollectionConfig.

  1. packages/db/src/types.ts (~line 509, BaseCollectionConfig) — Add an optional actionsOnly?: boolean field to the collection config interface.

  2. packages/db/src/errors.ts — Add a new error class like MutationOutsideActionError with a helpful message explaining that this collection requires mutations to be performed inside an action/transaction.

  3. packages/db/src/collection/mutations.ts — In each of the three mutation methods (insert at line 159, update at ~line 255, delete at ~line 450), add a check right after getActiveTransaction():

    if (!ambientTransaction && this.config.actionsOnly) {
      throw new MutationOutsideActionError()
    }

    This is a ~3-line addition per method, slotting naturally into the existing pattern where each method already checks !ambientTransaction && !this.config.onInsert.

  4. packages/db/src/collection/index.ts — The createCollection factory should pass through the new option. This likely happens automatically since it spreads config.

Alternative (lighter-weight, non-breaking): Instead of a config option, provide a utility assertInsideAction() that users call themselves at the top of wrapper functions. But the config-level approach is cleaner and matches the author's request.

Effort estimate

Small — The ambient transaction detection (getActiveTransaction()) already exists. The implementation is essentially: one new config field, one new error class, and a 3-line guard clause added to each of the three mutation methods. Total ~20-30 lines of new code plus tests.

Suggested comment

Thanks for the feature request! The infrastructure for this is already in place — getActiveTransaction() in our transaction system already knows whether a mutation is happening inside an action or standalone. Adding an actionsOnly config option to createCollection that throws when mutations are called outside a transaction/action would be straightforward. We'd be happy to review a PR for this, or we can pick it up when we get to it.

Good — queryClient is indeed required (throws QueryClientRequiredError if missing). Now let me check one more thing: the PR creates new QueryClient() inline in each collection definition. The query-collection docs show the correct pattern of creating a shared instance.

Summary

This PR fixes several incomplete code snippets in docs/overview.md and docs/quick-start.md. It adds the required queryClient parameter, adds missing getKey and eq imports, fixes queryFn return types (adding await and .json()), and adds a Todo interface to the quick-start example for type inference.

Strengths

  • Correctly identifies real bugs in the docs. queryClient is a required field that throws QueryClientRequiredError if omitted — the existing snippets would fail at runtime. Similarly, the queryFn in overview.md was returning a Response object instead of parsed JSON. These are legitimate first-time-user stumbling blocks.
  • The getKey addition to the on-demand snippet and the eq import fix are both correct and necessary.
  • The Todo interface + type annotation on toggleTodo makes the quick-start example more complete and educational.
  • Good PR description explaining the "why" behind each change.

Concerns

  1. new QueryClient() inline per collection is an anti-pattern. The PR creates a separate new QueryClient() inside each queryCollectionOptions({...}) call. This means each collection gets its own isolated query client, which defeats the purpose of shared caching, deduplication, and coordinated invalidation. The query-collection docs correctly show the pattern: create one const queryClient = new QueryClient() at module scope, then pass it to all collections. The overview snippet creates three separate QueryClient instances across the two collections and the on-demand example — this would confuse users.

  2. The import import { QueryClient } from "@tanstack/react-query" should be "@tanstack/query-core". The query-collection package and its own docs use @tanstack/query-core. Using react-query works (it re-exports from query-core) but is inconsistent with the rest of the docs and pulls in React-specific code unnecessarily.

  3. Minor: semicolons inconsistency. The PR adds lines with semicolons (e.g., import { QueryClient } from "@tanstack/react-query";) in a file that otherwise doesn't use trailing semicolons.

Verdict

Needs work — The core intent is correct and valuable (the docs genuinely need these fixes), but the new QueryClient() inline pattern is actively misleading and the import source is inconsistent with existing docs.

Suggested comment

Thanks for catching these gaps — queryClient is indeed required and the missing .json() calls were real bugs. Two things to fix before merging: (1) create a single shared const queryClient = new QueryClient() at the top of each snippet rather than inlining new QueryClient() per collection (see docs/collections/query-collection.md for the pattern), and (2) import from @tanstack/query-core instead of @tanstack/react-query to be consistent with the rest of the docs. Would also be good to drop the trailing semicolons on the new import lines to match the file style.

OK, the write call passes type: mutation.type — the sync engine's write function expects insert, update, or delete. The PR correctly decomposes the upsert into those types, so that's fine.

Now I have all the information I need. Let me write the review.

Summary

Adds a writeUpsert utility to the localStorage collection that inserts or updates items directly into the synced data store, modeled after the existing writeUpsert in the query-db-collection. The implementation checks lastKnownData for existence, merges data on update, persists to localStorage, and confirms via confirmOperationsSync. Includes 4 tests covering insert, update, merge, and batch operations.

Strengths

  • Correctly follows the existing acceptMutations pattern: validate → mutate lastKnownDatasaveToStorageconfirmOperationsSync
  • Decomposes upsert into insert/update types before passing to confirmOperationsSync, matching what the sync engine expects
  • Tests are thorough — covers single insert, single update, partial merge, and batch operations
  • Good JSDoc with examples on the interface type

Concerns

Bug: original reads stale data after mutation

This is the most significant issue. In the writeUpsert implementation:

lastKnownData.set(key, storedItem)       // mutates the map

mutations.push({
  original: existsInStorage
    ? (lastKnownData.get(key)?.data as T) // reads AFTER set — gets the NEW data
    : ({} as T),
})

original is supposed to capture the previous value, but because lastKnownData.set() runs first, lastKnownData.get(key) now returns the newly written data. The fix is to capture the original data before the set:

const originalData = existsInStorage ? lastKnownData.get(key)?.data : undefined
// ... then set ...
lastKnownData.set(key, storedItem)
// ... then use originalData in the mutation

Currently confirmOperationsSync doesn't use original for non-delete mutations (line 863: it uses mutation.modified), so this bug is latent — it doesn't cause incorrect behavior today. But it violates the PendingMutation contract and would break if original is ever consumed.

API type mismatch with query collection

The query collection's writeUpsert accepts Partial<T> (appropriate for upserts where you may only supply some fields). This PR's signature requires the full T:

// query-db-collection
writeUpsert: (data: Partial<TItem> | Array<Partial<TItem>>) => void

// this PR
writeUpsert: <T extends object>(data: T | Array<T>) => void

Since the merge test (should merge data when updating existing item) explicitly exercises partial updates by casting as Partial<Todo>, the function already supports partials at runtime — the type just doesn't reflect it. Consider using Partial<T> for consistency.

Minor: {} as T for original on insert

Using {} as T for the original field of an insert mutation is a type lie. Looking at acceptMutations, it doesn't set original at all for inserts (it comes from the PendingMutation parameter). Consider whether original should be undefined or whether insert mutations even need it.

Missing: no test for writeUpsert on an item that was inserted via normal mutation

All tests upsert items that were themselves upserted. There's no test verifying that writeUpsert correctly updates an item that was originally inserted through the normal collection.insert() / onInsert path.

Verdict

Needs work — the original data bug should be fixed, and the type signature should be aligned with the query collection's Partial<T> convention.

Suggested comment

Thanks for the contribution! The implementation follows the existing patterns well and the tests are solid. Two things to address: (1) there's a bug where original in the mutation reads from lastKnownData after it's been overwritten — capture the old data before calling lastKnownData.set(). (2) Consider using Partial<T> in the type signature to match the query collection's writeUpsert API. Happy to help if you have questions!

I have a thorough understanding of the code now. Here's the review:

Summary

When loadPendingTransactions() is called (e.g., upon regaining tab leadership), transactions are loaded from the outbox, optionally filtered by the beforeRetry hook, then scheduled. The bug: filtered-out transactions were removed from the outbox but not from the scheduler's in-memory pendingTransactions array, so they'd still execute. The fix adds this.scheduler.clear() before re-scheduling the filtered set.

Strengths

  • Correct fix in the right place. The one-liner this.scheduler.clear() before the re-scheduling loop is the minimal, precise change needed. It mirrors the pattern already used by TransactionExecutor.clear() (line 315).
  • Good test. The test covers the exact scenario — leadership loss/regain with a beforeRetry filter — and asserts both that the filtered transaction doesn't execute and that the kept transaction does. The assertions on the outbox state are a nice touch.
  • Changeset included with an accurate description.

Concerns

  1. Race condition with in-flight transactions. KeyScheduler.clear() sets isRunning = false and empties the queue. If a transaction is currently mid-execution when loadPendingTransactions() is called, clearing isRunning could allow the scheduler to hand out a second concurrent transaction (violating the sequential-processing invariant). In practice this may be safe because leadership changes likely don't happen mid-execution, but it's worth verifying. Consider whether loadPendingTransactions should guard against being called while a transaction is running, or whether the clear should preserve the isRunning flag.

  2. scheduler.clear() runs even without beforeRetry. The clear + re-schedule now happens on every loadPendingTransactions() call, not just when beforeRetry filters things. This is fine (idempotent), but it's worth noting the behavior change — the scheduler is now always rebuilt from the outbox on reload, not just appended to. This is arguably better behavior, but should be intentional.

  3. Minor: comment placement. The added comment says "Clear scheduler before reloading to ensure filtered-out transactions are removed" — it could be slightly misleading since the clear happens unconditionally. A tweak like "Clear scheduler before reloading to rebuild from the authoritative outbox state" would be more accurate.

Verdict

Merge with minor fixes — the core bug fix is correct and well-tested. The race condition in concern #1 should at least be discussed (even if the conclusion is "it's fine in practice"), and the comment could be tightened.

Suggested comment

Nice catch — this is a real bug with a clean fix. One thing worth double-checking: KeyScheduler.clear() resets isRunning to false. Could loadPendingTransactions() ever be called while a transaction is mid-execution? If so, clearing isRunning could briefly allow concurrent transaction dispatch. If you're confident the leadership handoff prevents that, a brief comment noting it would be helpful. Otherwise, LGTM — thanks for the test coverage!

Now I have full context. Let me verify one critical detail — the current main branch already has isOnline() on the implementation but not the interface, and the PR adds it to the interface.

Summary

This PR adds isOnline() to the OnlineDetector interface (it already exists on the WebOnlineDetector implementation) and uses it to skip transaction execution when the device is offline. It also gates notifyOnline() to no-op when offline, and passes the onlineDetector into TransactionExecutor so it can check status before each execution run.

Strengths

  • Real problem, clean solution. The data-loss scenario described in the PR body is legitimate — burning through 10 retries while offline is wasteful and risks discarding queued mutations. Checking isOnline() before runExecution() is a low-risk guard.
  • Good test coverage. Three new e2e tests covering: custom detector config, offline-to-online transition, and queued-while-offline flow. The FakeOnlineDetector harness is well-structured.
  • Interface already aligned. WebOnlineDetector.isOnline() already existed; promoting it to the interface is the correct move and makes custom implementations (React Native, Expo) contractually require it.
  • README documentation is thorough — shows a custom polling detector example which is the primary use case.

Concerns

  1. notifyOnline() behavior change is breaking for custom implementations. The PR adds isOnline() gate inside WebOnlineDetector.notifyOnline(), but the README's custom detector example does not gate notifyOnline() — it calls listeners unconditionally. This creates an inconsistency: the built-in detector silently swallows notifyOnline() calls when offline, but a naively-written custom detector won't. The contract is unclear. Either:

    • Document that notifyOnline() must check isOnline() first (and show it in the example), or
    • Move the isOnline() guard into OfflineExecutor.setupEventListeners() (the subscriber callback) so it's enforced regardless of detector implementation.
  2. FakeOnlineDetector.notifyOnline() duplicates the guard but the README example doesn't. The fake detector checks isOnline() before notifying, which means the tests pass even if the real guard were only in the detector. But a user copying the README example would get different behavior. The README custom detector example should include the isOnline() check in notifyOnline().

  3. Retry timer still fires while offline. The scheduleNextRetry() timer in TransactionExecutor continues to fire. When it triggers executeAll()runExecution(), the isOnline() check will bail out, but the timer-fire-and-bail loop repeats every retry interval. This isn't harmful but is wasteful. Consider clearing the retry timer when offline and relying solely on the notifyOnline() subscriber to restart execution.

  4. Missing test for retry-while-offline scenario. The PR description claims "network/connectivity errors no longer count toward retry limit" but there's no code implementing this — the handleError path still increments retryCount unconditionally. Either the claim should be removed from the description or the feature needs to be implemented.

  5. Extra blank line added at line 237 of TransactionExecutor.ts (between beforeRetry block and the for loop in loadPendingTransactions). Minor style nit.

  6. Changeset mentions rename (DefaultOnlineDetectorWebOnlineDetector) but this rename already landed on main. The changeset note is misleading about what this PR actually changes.

Verdict

Needs work. The core idea is sound, but the notifyOnline() contract inconsistency (#1) and the false claim about retry count behavior (#4) need to be addressed before merge. The fix for #1 is small — either move the guard to the subscriber or document it as a requirement. #4 just needs the description corrected (or the feature implemented).

Suggested comment

Thanks for tackling this — the offline guard in runExecution() is a solid improvement. Two things to address: (1) the isOnline() check in notifyOnline() creates an implicit contract that custom detectors must replicate but the README example doesn't show — consider moving that guard into the OfflineExecutor subscriber callback so it works regardless of detector implementation; (2) the PR description claims network errors no longer count toward the retry limit, but I don't see that implemented in the diff — can you either add that or update the description?

So delete-update is not explicitly handled either — it falls through to the default exhaustiveness check which throws. That's consistent with the updated comment.

Now I have all the context needed for a thorough review.


Summary

Adds handling for the delete-insert mutation combination within a single transaction. When a user deletes a record and then re-inserts one with the same key (e.g., undo in an editor), the merge logic either cancels both mutations (if the data matches the original) or converts to an update mutation. Previously this threw Unhandled mutation combination: delete-insert.

Strengths

  • Real use case: The undo/redo workflow in a long-lived editor session is a legitimate pattern. The collection layer already allows this path — has(key) returns false after an optimistic delete (state.ts:124-125), so insert() passes the duplicate-key check.
  • Consistent merge semantics: The null-return for exact restore mirrors the existing insert-delete → null pattern. The fallback to update is the right semantic choice.
  • Good tests: Both the "cancel both" and "convert to update" cases are covered, including verification that changes only contains actually-differing properties.
  • Changeset included and well-worded.

Concerns

  1. Missing fields in the update result: The insert-update merge (line 48-66) carefully preserves key, globalKey, mutationId, updatedAt, optimistic, createdAt, and collection. The new delete-insert case uses ...incoming spread which does carry these, but it's worth noting that mutationId and timestamps come from the insert mutation, not the delete — which seems correct but differs from how insert-update explicitly assigns mutationId: incoming.mutationId. This is fine because the spread achieves the same thing, but the inconsistency in style may make future maintenance harder.

  2. existing.original as T cast: At line 107, the code casts existing.original to T with a comment that delete mutations always have the full original. This is true today (see mutations.ts:488), but it's a type-level assumption with no runtime guard. The PendingMutation type defines original as TOperation extends 'insert' ? {} : T, so the generic PendingMutation<T> type doesn't narrow this for the delete case. This cast is safe but could be made unnecessary if mergePendingMutations accepted typed discriminated union overloads. Minor concern.

  3. deepEquals import adds a dependency: transactions.ts was previously self-contained with respect to utils. Now it imports deepEquals. This is fine, but deepEquals is a non-trivial function (handles Maps, Sets, Temporal objects, circular references). For simple record comparison in the "cancel both" path, this is appropriate, but worth noting.

  4. No test for nested/complex objects: The tests use flat { id, value } objects. A test with nested objects would increase confidence that deepEquals integration works correctly for the change-diffing logic (the Object.entries(...).filter(...) at line 109-112 only does a shallow key iteration with deepEquals per-property, which is correct but worth stress-testing).

  5. Comment in truth table not fully updated: The applyMutations docstring at line 311-320 still lists only the old merge rules and doesn't mention delete-insert. Should be updated for consistency.

Verdict

Merge with minor fixes — the logic is correct and the use case is real. The docstring on applyMutations should be updated to mention delete-insert, and a test with a nested object would be nice but not blocking.

Suggested comment

Clean PR — the logic is sound and the tests cover the key cases well. Two small requests: (1) update the applyMutations JSDoc (around line 311) to include the delete-insert rule alongside the others, and (2) consider adding a test case with nested object values to exercise deepEquals in the change-diffing path. Otherwise LGTM.

Now I have full context on the existing doc and the PR changes. Let me write the review.

Summary

This PR adds documentation about Zod 4.1's z.codec() API to the schemas guide. It adds an inline code example inside the existing "TInput superset" section and a new standalone "Codecs" section after the mutation handler example block, with good/bad patterns and links to Zod's codec docs.

Strengths

  • Useful topic — codecs are the cleaner way to express bi-directional transformations, and documenting them alongside the existing z.union().transform() pattern helps users who are on Zod 4.1+.
  • Good/bad pattern comparison mirrors the existing doc style.
  • Links to upstream Zod docs for further reading.

Concerns

  1. Placement is awkward. The new "Codecs" H2 section (line 197+) is injected between the mutation handler code example and the --- separator — it disrupts the flow of the "Why This Matters" section. It should either be its own top-level section (e.g., after "Transformation Patterns") or folded into the existing "Use Union Types for Transformations (Essential)" best practice.

  2. Duplicate example. The inline codec example added inside the existing code block (lines 144–157) repeats nearly the same code as the standalone section. Pick one location; having both is redundant and makes the doc longer without adding value.

  3. Stray blank line added (line 184). A trailing whitespace change inside the onInsert code block — should be removed.

  4. Excessive trailing blank lines. Four extra blank lines at the end of the new section (lines 233–236). Trim to one.

  5. Zod version gating is unclear. The PR says "Requires [email protected]" in comments but never states what happens for users on Zod 3.x (the version most TanStack DB users are likely on today). The doc should clarify the version requirement prominently and note that the z.union().transform() approach works on all versions.

  6. No mention of StandardSchema compatibility. The existing doc emphasizes StandardSchema. It would be worth noting whether z.codec() outputs conform to StandardSchema or if there are caveats.

  7. Minor style issues:

    • Inconsistent spacing around type annotations: (val : string | Date ) vs the rest of the codebase using (val: string | Date).
    • "Bi-Directional Dataflow" — unusual capitalization. Use "bidirectional data flow".
    • Parenthetical style ( Requires [email protected] ) has spaces inside parens, inconsistent with the rest of the doc.
    • "compatiblity" → "compatibility" (typo).

Verdict

Needs work. The content is valuable but needs structural reorganization (remove the duplicate, find a better home for the section), cleanup of style inconsistencies, and clearer version-gating guidance.

Suggested comment

Thanks for adding codec documentation — this is a useful addition! A few things to address before merge: (1) the codec example appears twice (inline in the existing code block and in the new section) — please consolidate to one location; (2) the new section feels misplaced mid-flow — consider moving it after "Transformation Patterns" or into "Best Practices"; (3) there are some style nits (spacing in type annotations, a typo in "compatiblity", extra blank lines) that need cleanup. Also worth adding a note that z.codec() requires Zod 4.1+ and that the z.union().transform() pattern remains the approach for Zod 3.x users.

Now I have everything I need. Here's the review:

Summary

This PR makes substantial changes to trailbase-db-collection: it adds e2e test infrastructure (Docker setup, global-setup, 700-line e2e test file, a vitest e2e config), rewrites significant portions of trailbase.ts to add progressive/on-demand sync modes plus loadSubset, broadens the type signatures for parse/serialize to support function-based converters, and removes two of three existing unit tests (insert/delete events, local mutations). Despite the title "remove redundant unit tests," the actual scope is a large feature addition.

Strengths

  • Adds e2e test coverage using the shared db-collection-e2e suite framework — this is the right pattern used by electric-db-collection and query-db-collection.
  • The progressive sync mode with loadSubset is a useful feature, providing fast initial data for queries while background-syncing the rest.
  • UUID base64 encode/decode helpers are necessary for TrailBase's BLOB UUID storage.
  • Cancellation is improved via AbortController instead of manual reader tracking.

Concerns

  1. Committed binary: testing-bin-linux/trail is a checked-in Linux binary. This should never be committed to the repo. It bloats git history permanently and is a security concern (untrusted binary). The README says to download it — the binary itself should be .gitignored and fetched in CI.

  2. Deleted unit tests are not redundant: The PR removes "receive inserts and delete updates" and "local inserts, updates and deletes" tests. These test fundamental mock-level behavior (event stream processing, optimistic mutations via createBulk/update/delete). The e2e tests do not replace them — e2e tests require a running TrailBase server and won't run in most CI or local dev workflows. The remaining single unit test only covers cancellation.

  3. Verbose console.log in production code: trailbase.ts now has ~10 console.log statements in the listen() function logging every event with JSON stringification. These are clearly debug leftovers and should be removed before merge.

  4. markReady() no longer called on error in eager mode: The original code had markReady() in a finally block specifically to avoid blocking apps on .preload(). The rewrite only calls markReady() in the success path for eager mode — if initialFetch() throws, markReady() is never called, and .preload() will hang forever.

  5. Test hooks in production API surface: TRAILBASE_TEST_HOOKS symbol and TrailBaseTestHooks type are exported from index.ts. Test-only infrastructure should not be part of the public API.

  6. Type signature changes are breaking: The generic constraints on TrailBaseCollectionConfig changed from TItem extends ShapeOf<TRecord> to TItem extends object. This is a public API change that relaxes type safety and could affect downstream consumers. The syncMode is also removed from the Omit list and re-added with a different type.

  7. decodeIdForSorting uses atob: The atob function is not available in all Node.js environments (only available since Node 16 with some caveats). The codebase should use Buffer.from() or a cross-platform approach.

  8. Misleading PR title: "remove redundant unit tests" significantly understates what this PR does. It's really "add sync modes + e2e tests + refactor trailbase collection."

  9. No changeset: The trailbase.ts changes alter the public API (new exports, changed types, new sync modes). This needs a changeset.

  10. serializePartial fallback is fragile: The fallback for function-based serializers does config.serialize(item as TItem) which will likely fail or produce wrong results for partial objects with missing required fields.

Verdict

Needs work. The binary must be removed, the deleted unit tests should be restored, console.log debug statements need cleanup, the markReady regression needs fixing, and test hooks shouldn't be exported publicly. The feature additions (sync modes, e2e tests) have value but need to be separated from the unit test deletions and polished.

Suggested comment

Thanks for the work on e2e tests and sync mode support! A few things need addressing before this can merge: (1) the committed testing-bin-linux/trail binary must be removed from the repo — add it to .gitignore and fetch it in CI instead, (2) please restore the deleted unit tests — they cover mock-level behavior that e2e tests don't replace, (3) remove the console.log debug statements from trailbase.ts, and (4) the markReady() call was moved out of the finally block for eager mode, which means .preload() will hang if initialFetch() throws — this needs to be fixed. Also, given the scope of changes to the public API, this needs a changeset.

This confirms a critical issue. The Electric loadSubset properly accepts LoadSubsetOptions (with where, orderBy, limit, cursor, offset) and uses them to fetch the specific data needed for a query. The PR's TrailBase loadSubset ignores all of these — it just fetches the first N records with no filtering or ordering. This means on-demand mode won't actually work correctly for filtered or sorted queries.

Now I have everything I need for a thorough review.

Summary

Adds syncMode support (eager, on-demand, progressive) to trailbase-db-collection and introduces an e2e test suite using the shared db-collection-e2e test harness. The implementation changes the type signatures for parse/serialize to support full function-based transformations (not just per-field Conversions), adds a loadSubset function for on-demand/progressive modes, and wires up an AbortController-based cancellation model. 4 of 118 e2e tests fail.

Strengths

  • Uses the shared e2e harness — correctly integrates with db-collection-e2e test suites, which is the right pattern for new collection backends.
  • Good UUID handling — the base64-to-UUID and UUID-to-base64 conversions are correct and necessary for TrailBase's BLOB UUID storage.
  • AbortController migration — replacing the mutable eventReader ref with an AbortController is a cleaner cancellation model and fixes the prior double-cancel edge case.
  • Docker + binary fallback — the global setup supports multiple ways to start TrailBase, which is practical for different CI environments.

Concerns

Critical

  1. loadSubset ignores query parameters — The loadSubset implementation (line ~570 of the new trailbase.ts) only accepts { limit?: number } and fetches unfiltered, unsorted records. The framework's LoadSubsetFn type expects (options: LoadSubsetOptions) => true | Promise<void> where LoadSubsetOptions includes where, orderBy, cursor, offset, and subscription. This means on-demand queries with filters or sorting will get wrong/incomplete data. Compare to Electric's loadSubset which compiles these options into SQL and requests a filtered snapshot. This is likely the root cause of the 4 failing tests.

  2. loadSubset is one-shot — Once loadSubsetPromise is set, all subsequent calls return the same promise. On-demand mode expects loadSubset to be callable with different predicates for different queries. Two live queries with different where clauses would both get the result of whichever loaded first.

  3. Binary committed to repotesting-bin-linux/trail is a binary blob (likely 10s of MB) committed to git. This will bloat the repo permanently. Should be downloaded in CI instead or added to .gitignore.

Significant

  1. Sorting records by decoded ID — The new decodeIdForSorting + allRecords.sort() in initialFetch forces a specific insertion order. This is papering over what seems to be a nondeterminism issue. If TanStack DB's internal ordering depends on insertion order (which it shouldn't for correctness), that's a framework issue, not something the sync layer should compensate for.

  2. Verbose console.log left in production code — The listen() function has extensive console.log statements ([TrailBase] Received event:, [TrailBase] Insert event for item with key:, etc.) that should use the debug module or be removed.

  3. Unit tests removed, not replaced — Three existing unit tests (receive inserts and delete updates, local inserts, updates and deletes, and half of the initial-fetch test) were deleted. The PR description doesn't justify their removal, and the e2e tests aren't a substitute since they require a running TrailBase server.

  4. Progressive mode implementation is incomplete — Progressive mode in TanStack DB is meant to provide fast initial query responses via loadSubset while syncing the full dataset in background, then atomically swapping. The PR maps progressive to on-demand internally but the loadSubset doesn't support filtered queries (see #1), so the progressive behavior doesn't actually work as designed.

Minor

  1. SyncMode re-exportedSyncMode is imported from @tanstack/db but not listed in the original imports. The new TrailBaseSyncMode = SyncMode | 'progressive' type is fine, but the export from index.ts exposes internal detail.

  2. Unused progressive test hook infrastructure — The test file sets up usersUpToDateControl, postsUpToDateControl, commentsUpToDateControl for progressive mode but never uses them (the progressive collections are created without TRAILBASE_TEST_HOOKS).

  3. config.recordApi.list doesn't support filtering — If the TrailBase list() API doesn't accept where/orderBy parameters, then on-demand mode fundamentally can't work with TrailBase. This should be documented clearly or on-demand should be removed as an option.

Verdict

Needs work. The core loadSubset implementation doesn't conform to the LoadSubsetFn contract (ignores where, orderBy, cursor, offset), which makes on-demand and progressive modes non-functional for any query that isn't a simple "give me everything." The committed binary should be removed. The removed unit tests should be preserved.

Suggested comment

Thanks for the effort here — the e2e harness integration and UUID handling are solid! The main blocker is that loadSubset ignores the where/orderBy/cursor fields from LoadSubsetOptions, so on-demand and progressive modes won't serve correct data for filtered queries (this is likely why 4 tests fail). If TrailBase's list() API doesn't support server-side filtering, the pragmatic path may be to only support eager mode for now and document that limitation. Also, please remove the committed binary (testing-bin-linux/trail) — it should be downloaded in CI instead.

Good — the afterEach is handled at the test runner level in the e2e test file, not inside the shared suites. The shared suites expect the config to provide afterEach and the wrapper calls it. Now I have everything I need.


Summary

Adds syncMode option (eager | on-demand) to the TrailBase collection adapter and introduces a full e2e test suite using Docker-based TrailBase with shared test suites from db-collection-e2e. In on-demand mode, loadSubset fetches all records (ignoring filter/pagination options) and subscribes for live updates, while eager mode retains existing behavior.

Strengths

  • Well-structured e2e infrastructure: Dockerfile, global setup with health checks, cleanup, and teardown follow the established Electric e2e pattern closely. The global-setup.ts is clean and robust.
  • Good use of shared test suites: Reuses db-collection-e2e suites (predicates, joins, deduplication, collation, mutations, live-updates, progressive) which ensures consistency across collection adapters.
  • Clean serialize/parse pattern: The bidirectional type conversion between TrailBase records and app types is well-designed with proper UUID base64 encoding.
  • Correct sync ordering: Subscribes before initial fetch to avoid gaps — matching the pre-existing pattern.

Concerns

  1. Missing afterEach cleanup for on-demand collections — Electric's e2e tests provide an afterEach that calls cleanup() + startSyncImmediate() + preload() on all on-demand collections between tests. The TrailBase config has no afterEach, which means on-demand collections accumulate stale state across tests. This could cause flaky or incorrect test results.

  2. loadSubset ignores all options — The implementation logs a warning and then fetches everything anyway. This makes on-demand mode functionally identical to eager mode (just deferred). While the PR title says "partial" support, this should be documented more clearly in the code and ideally throw or return early for unsupported options rather than silently fetching everything.

  3. getSyncMetadata is duplicated — It's returned both from the sync function callback and as a top-level property on the sync config. If state drifts (e.g. fullSyncCompleted changes), both closures capture the same mutable variable so it works, but the duplication is unnecessary and confusing. Electric only returns it from the sync callback in on-demand mode.

  4. TrailBaseSyncMode type alias is unnecessaryexport type TrailBaseSyncMode = SyncMode adds no value. Other adapters (Electric) define their own extended type (SyncMode | 'progressive'). If TrailBase doesn't extend it, just use SyncMode directly.

  5. Missing LoadSubsetOptions and SyncMode imports in the diff — The diff adds imports for LoadSubsetOptions and SyncMode but removes syncMode from the Omit on the config interface. This means the base syncMode is hidden from consumers, replaced by the TrailBase-specific one. This is fine but worth confirming intent since both types are identical.

  6. posts_e2e.userId is TEXT NOT NULL — But the users table uses BLOB UUIDs for id. Foreign key references won't work across these types. This may cause issues if join tests depend on referential integrity.

  7. Unrelated vue-db type test formatting change — The useLiveQuery.test-d.ts change is just whitespace reformatting and shouldn't be in this PR.

Verdict

Merge with minor fixes — The core approach is sound and follows existing patterns well. The missing afterEach cleanup (#1) is the most important fix needed; the rest are minor.

Suggested comment

Nice work on the e2e infrastructure and on-demand support — this follows the Electric adapter patterns well. One important gap: the test config is missing an afterEach that calls cleanup() + startSyncImmediate() + preload() on the on-demand collections between tests (see how Electric does it). Without this, on-demand collections will accumulate state across tests. Also, could you drop the unrelated vue-db formatting change into a separate PR?

Now I have all the context needed. Here's the review:


Summary

Adds on-demand sync mode support to the TrailBase collection adapter, implementing loadSubset with pagination (cursor and offset), orderBy translation, and partial where clause support (simple binary comparisons). Also adds a comprehensive e2e test suite with Docker-based TrailBase setup.

Strengths

  • Well-structured e2e tests: Docker-based global setup with proper cleanup, uses the shared db-collection-e2e test suites — follows the same pattern as other collection adapters.
  • Incremental where support: Rather than claiming full support, honestly handles only simple binary comparisons (eq, ne, gt, etc.) and logs warnings for unsupported patterns. This is pragmatic.
  • Cursor caching: TrailBase uses encrypted cursors that can't be derived from keys, so the cursors Map is a reasonable workaround with a clear comment explaining why.

Concerns

  1. Import from dist path (trailbase.ts:9):

    import type { OrderByClause } from '../../db/dist/esm/query/ir'

    This imports from the built dist directory rather than the source. It will break if db hasn't been built first. Should use import { IR } from '@tanstack/db' and reference IR.OrderByClause, or import from the source path that the tsconfig aliases already set up (@tanstack/db). The other collection packages (e.g. electric) use import type { IR } from '@tanstack/db'.

  2. CompareOp and FilterOrComposite imported from trailbase client (trailbase.ts:9): This is fine, but verify these types are actually exported from the trailbase npm package — they look like they might be internal types.

  3. syncMode removed from Omit list then re-added (trailbase.ts:98): The config interface omits syncMode from BaseCollectionConfig but then re-adds its own syncMode with the same SyncMode type (re-exported as TrailBaseSyncMode). This indirection adds complexity for no benefit — the re-export and the omit/re-add could be skipped.

  4. buildFilters only handles func type where clauses: If a composed and/or where clause comes through, it silently falls through to the warning. The Func with name and/or that wraps multiple conditions is the common case for multi-condition filters. Worth a TODO comment at minimum.

  5. buildOrder type annotation (trailbase.ts:429): The .map callback annotates the parameter as (o: OrderByClause) which is redundant given the input type, and uses == instead of === for direction comparison.

  6. No changeset: The PR checklist shows the "affects published code" box is unchecked, but the source file trailbase.ts is modified with new public API surface (TrailBaseSyncMode export, new syncMode config option). This needs a changeset.

  7. Vue type test change (useLiveQuery.test-d.ts): Unrelated formatting-only change — should be in a separate commit or PR.

Verdict

Needs work — the dist import path is a blocker that will cause build failures in CI and for consumers. The rest are minor.

Suggested Comment

Nice work adding on-demand sync and e2e tests for TrailBase! One blocker: the import at trailbase.ts:9 references ../../db/dist/esm/query/ir — this should use @tanstack/db (e.g. import { IR } from '@tanstack/db' and then IR.OrderByClause), matching how other collection packages import these types. Also, this PR needs a changeset since it adds new public API surface. Happy to re-review once those are addressed!

Now I have both implementations. Let me compare them closely and write the review.

Summary

Ports useLiveInfiniteQuery from react-db to vue-db, providing infinite pagination backed by the live query system's setWindow API. Adds the composable (~289 lines), tests (~630 lines), and re-exports from the package index.

Strengths

  • Core pagination logic (peek-ahead for hasNextPage, page splitting, fetchNextPage gating) faithfully matches the React reference implementation.
  • Good test coverage: initial page, multi-page fetch, end-of-data detection, live updates, dependency reset, pre-created collections, missing orderBy error, and window-skip optimization.
  • Properly uses Vue primitives (ref, computed, watchEffect, watch) and returns ComputedRef values as expected by the vue-db API surface.

Concerns

Bugs / Correctness

  1. Overly complex and fragile collection detection in queryInput (lines 85–127). The React version does a simple instanceof CollectionImpl check and branches once. The Vue version probes zero-arg functions, instantiates a BaseQueryBuilder dummy, and has nested try/catch blocks. This is brittle — a query function that happens to have .length === 0 (e.g., (q) => ... with default params or TypeScript compilation artifacts) could be misidentified as a getter. The heuristic raw.length === 0 is not reliable. This entire section should be simplified to match the React approach more closely.

  2. getNextPageParam is never called. The config accepts getNextPageParam but it's completely unused in the implementation — hasNextPage is determined purely by the peek-ahead check (dataArray.length > totalItemsRequested). The React implementation also doesn't call it, so this is inherited tech debt, but it's misleading API surface. At minimum, the types and docs should be honest about this, or the callback should actually be wired up.

  3. Missing input validation. The React version validates the first argument and throws a clear error for invalid types (lines 142–147). The Vue version silently passes through whatever it gets.

  4. Missing isReady gate for query functions. React has if (!isCollection && !queryResult.isReady) return before calling setWindow (line 229). The Vue watchEffect will fire immediately and may call setWindow before the collection is ready.

  5. Missing pre-created collection window validation warning. React logs a console.warn when a pre-created collection's existing window doesn't match expectations (lines 213–226). Vue skips this entirely.

Style / Structure

  1. Unused import: BaseQueryBuilder is imported solely for the dummy-probe hack — removing the hack removes this dependency.

  2. Return type discrepancy: The Vue return type wraps everything in ComputedRef, but UseLiveInfiniteQueryReturn defines data as ComputedRef<InferResultType<TContext>> while UseLiveQueryReturn defines it differently. The Omit<UseLiveQueryReturn, 'data'> spread means the actual shape depends on what useLiveQuery returns at runtime, which could be surprising if useLiveQuery's return type evolves.

  3. Double blank lines scattered through the test file (minor formatting nit).

Tests

  1. Tests use setTimeout(resolve, 50) polling which is fragile and slow. This matches existing vue-db test patterns though, so not a blocker.

  2. No test for the isFetchingNextPage state being true during an async setWindow call.

Verdict

Needs work. The core pagination logic is correct but the collection-detection heuristic (concern #1) is a real bug risk that needs simplification. Missing the isReady gate (#4) could cause runtime errors. These should be fixed before merging.

Suggested comment

Thanks for porting this, @1bod — the core pagination logic looks solid and the tests are thorough. Two things to address before merge: (1) the collection-detection logic in queryInput (lines 85–127) is fragile with its zero-arg probe and dummy builder — please simplify it to match the React version's instanceof CollectionImpl approach, and (2) add the isReady guard before calling setWindow for query-function inputs (React ref line 229) to avoid calling setWindow on an unready collection. Happy to re-review once those are updated!

Good. Now I have a comprehensive understanding of the architecture. Let me write the review.

Summary

Adds union() and unionFromLiveQuery() utilities for combining multiple collections into a single collection with fail-fast key conflict detection. union() merges static collections; unionFromLiveQuery() dynamically adds/removes source collections based on live query results using ref-counted tracking. Addresses issue #58.

Strengths

  • Solid architecture: UnionCollectionManager cleanly encapsulates sync protocol, key ownership tracking, and source lifecycle. The SourceRefTracker is a well-designed abstraction for ref-counted dynamic unions.
  • Correct use of sync protocol: Properly uses begin/write/commit/markReady and returns a cleanup function from sync. rowUpdateMode: 'full' is the right choice since union sources forward complete rows.
  • Fail-fast key conflicts: UnionKeyConflictError with clear messaging that includes both collection IDs. Conflict checking happens both at seed time and on change application.
  • Good cleanup handling: unionFromLiveQuery listens for cleaned-up on the union collection to unsubscribe the live query — prevents leaks.
  • Tests cover the key scenarios: basic union, key conflicts, dynamic add/remove via live query, and ref-counting.

Concerns

API Design (Major)

  1. Monkey-patching add/remove/hasSource/sources directly onto the collection instance breaks the established utils pattern. Every other collection extension in the codebase (live query collections, test utilities) adds methods under collection.utils.*. This should use the same convention:
    collection.utils.add(source)
    collection.utils.remove(source)
    collection.utils.hasSource(source)
    collection.utils.sources()
    The current approach also sidesteps TypeScript — the cast through unknown to UnionCollection is a red flag.

Potential Bugs

  1. subscribeChanges callback receives ChangeMessage but the code accesses change.key as if always present. For the ChangeMessage type, key is always present (key: TKey), so this works. However, in applyChange, the write call for inserts/updates passes { type, value } without a key — this relies on getKey resolving it internally. Verify that the collection's state manager handles keyless writes correctly with rowUpdateMode: 'full'.

  2. seedKeyOwners runs before subscribeToSource, but subscribeToSource also calls assertNoKeyConflicts which re-checks the same data. This is redundant work on initial setup — not a bug, but worth noting.

  3. transitionToError calls console.error then sets lifecycle status to error, but the error also propagates via thrown UnionKeyConflictError. The applyChanges method catches the error after commit() and re-throws — but commit() has already been called on partial data. If 3 of 5 changes applied before a conflict, the union collection now has inconsistent state.

  4. No cleanup of the liveQueryCollection itself in unionFromLiveQuery. When the union is cleaned up, the subscription is unsubscribed, but the underlying liveQueryCollection created internally is never cleaned up / disposed. This leaks the live query collection and its subscriptions to source collections.

Missing Tests

  1. No test for add/remove after creation — the dynamic add/remove API on union() is untested outside of the unionFromLiveQuery path.
  2. No test for source collection entering error or cleaned-up state — the status:change handler transitions to error but this path is untested.
  3. No test for updates — only inserts and deletes are tested. An update to a source collection item should propagate correctly.
  4. No test for cleanup/disposal — verifying that unsubscription happens when the union collection is cleaned up.

Minor

  1. Global mutable counter unionCollectionCounter — fine for now, but could cause flaky test ordering if tests run in parallel in the same process. Consider accepting an optional id parameter.
  2. WeakMap for resultKeys — clever for avoiding memory leaks, but means getKeyFromItem silently falls back to primarySource.getKey for items not in the map. If primarySource is removed, the fallback could return wrong keys.

Verdict

Needs work — The core algorithm is sound and well-structured, but the API should follow the utils pattern rather than monkey-patching, the partial-commit-on-conflict bug needs addressing, the liveQueryCollection leak needs fixing, and test coverage should be expanded for the additional code paths.

Suggested Comment

Nice work on the core design — the UnionCollectionManager and SourceRefTracker are well-structured. A few things to address before merge: (1) methods like add/remove should go under collection.utils.* to match the existing extension pattern (see how live query collections do it), (2) the liveQueryCollection created inside unionFromLiveQuery needs cleanup when the union is disposed, and (3) if a key conflict occurs mid-batch in applyChanges, the transaction has already partially committed — consider validating the full batch before writing. Happy to pair on any of these if helpful!

Let me work with the diff provided in the prompt and the research from the subagents. I have enough information to write a thorough review.

Summary

Adds a new @tanstack/indexeddb-db-collection package that persists TanStack DB collection data to IndexedDB with cross-tab synchronization via BroadcastChannel. The package includes a low-level IndexedDB wrapper (wrapper.ts), the collection integration (indexeddb.ts), custom error classes, and extensive tests (~3,100 lines of tests). This is a first-time contributor's PR, authored with Claude assistance.

Strengths

  • Useful feature — IndexedDB persistence is a common need for offline-first apps and fills a real gap in the TanStack DB ecosystem.
  • Shared database patterncreateIndexedDB() creating a shared IDBDatabase instance that multiple collections reference is a good design. It avoids the version-upgrade coordination headache that would come from each collection managing its own database.
  • Good test coverage — The PR includes unit tests, type tests (test-d.ts), cross-tab tests, and wrapper tests.
  • Comprehensive README — Clear docs with usage examples, API reference, and error handling guidance.
  • Out-of-line keys — Correct choice to avoid keyPath, keeping stored objects clean.

Concerns

Error hierarchy doesn't follow monorepo conventions

Existing collection packages (electric, query, powersync) all extend TanStackDBError from @tanstack/db:

TanStackDBError -> ElectricDBCollectionError -> SpecificError

This PR defines standalone error classes (IndexedDBError, DatabaseRequiredError, etc.) that extend Error directly. They should extend TanStackDBError for consistency and to enable consumers to catch all TanStack DB errors uniformly.

Sync protocol concerns

  1. queueMicrotask for sync confirmation is fragile — After persisting to IndexedDB (async), mutations are confirmed via queueMicrotask(() => { syncBegin(); syncWrite(); syncCommit() }). This fires sync confirmation before the IndexedDB write actually completes in some cases, because queueMicrotask runs before the IDB transaction oncomplete. Other collections confirm sync through their backend's acknowledgment path (Electric awaits txid, Query refetches). Here the confirmation should happen after the IDB write succeeds, not on a microtask.

  2. Mutation handlers always fire even without user handlers — In electric/query collections, if the user doesn't provide onInsert/onUpdate/onDelete, the wrapped handler is undefined. This PR always defines wrapped handlers (wrappedOnInsert, etc.) that persist to IDB regardless, which means every mutation goes through IDB persistence even if the user wants to handle persistence themselves. This conflates "sync from IDB" with "persist to IDB" — the sync function should handle loading data from IDB, but writes to IDB happen unconditionally in mutation handlers without a way to opt out.

  3. collection.get(key) called during cross-tab sync — Inside the BroadcastChannel handler, collection.get(key) is called to find existing items for delete operations. This creates a coupling where the sync handler reads from the collection it's writing to, which can lead to race conditions if multiple cross-tab messages arrive rapidly.

  4. Initial load doesn't await version entries before markReady() — The initial load uses raw cursor iteration with manual onsuccess callbacks inside an executeTransaction callback that the transaction machinery expects to handle. Mixing promise-based await with raw IDB request callbacks inside executeTransaction is risky — the transaction may auto-commit before the cursor iteration completes.

API design issues

  1. wrapper.ts is a large generic IDB wrapper (671 lines) that shouldn't be in this package — It's a general-purpose IndexedDB utility library, not specific to TanStack DB. The collection should use it internally but probably shouldn't export it as public API. Existing collection packages don't export their backend's low-level primitives.

  2. acceptMutations util is confusing — It writes to IDB and triggers sync confirmation, but the semantics overlap with the normal mutation flow. It's unclear when a user would use this vs. the standard collection insert/update/delete methods.

  3. No loadSubset / unloadSubset — Both electric and query collections support on-demand sync modes. While not required for an initial version, the return type from sync should at minimum return { cleanup } (a SyncConfigRes), not rely on the cleanup being returned via a side channel.

Package structure deviations

  1. tsconfig.json has extra paths — The PR's tsconfig adds custom paths entries; other collection packages use a simpler structure extending the root tsconfig.
  2. No vitest.e2e.config.ts — Minor, but other packages include this.
  3. Version 0.0.2 — Should be 0.0.1 (or 0.0.0) since this is a new unreleased package. The changeset system will handle versioning.
  4. Missing changeset — The PR checklist shows it hasn't been generated.
  5. tests/.gitkeep — Unnecessary since the tests directory has files.

Code quality

  1. _operation parameter in writeWithVersion — The operation parameter is accepted but never used (prefixed with underscore). Either use it or remove it.
  2. _TKey in IndexedDBCollectionUtils — Unused generic parameter.
  3. any types scattered throughoutsyncWrite typed as ((message: ChangeMessageOrDeleteKeyMessage<any, string | number>) => void), mutation handler params all typed as any. Should use the proper generic types.
  4. No cleanup of error listeners — The BroadcastChannel onerror handler is never set. If the channel errors, it silently fails.

Verdict

Needs work. The core idea is solid and the contribution is welcome, but the sync protocol integration needs rethinking. The confirmation-via-microtask pattern, the mandatory IDB persistence in mutation handlers (conflating sync source with persistence target), and the error hierarchy divergence are significant enough to require revision before merge.

Suggested comment

Thanks for the contribution — an IndexedDB collection is something the community has been asking for! A few things need addressing before this can merge: (1) error classes should extend TanStackDBError from @tanstack/db to match the other collection packages, (2) the sync confirmation currently fires via queueMicrotask which may confirm before the IDB write actually completes — the confirmation should be tied to the transaction's oncomplete, and (3) the mutation handlers (wrappedOnInsert, etc.) unconditionally persist to IDB, which means users can't opt out of the built-in persistence if they want custom handling. Take a look at how electric-db-collection and query-db-collection structure their sync and handler wrapping for reference patterns. Happy to discuss any of these in more detail!

Good. Now I have full context. Here's the review:


Summary

This PR forwards the structuralSharing option from QueryCollectionConfig through to the underlying TanStack Query QueryObserver. Previously, structuralSharing was hard-coded to true, preventing consumers from disabling it (e.g., for non-serializable data or performance tuning). The PR adds the type, destructures the option, and conditionally spreads it into observer options — following the exact same pattern as staleTime, retry, etc.

Strengths

  • Follows established patterns exactly. The type definition uses the same QueryObserverOptions<...>['structuralSharing'] indexed access pattern as staleTime, retry, retryDelay, and refetchInterval. The conditional spread ...(structuralSharing !== undefined && { structuralSharing }) matches how all other optional observer options are forwarded.
  • Correct behavioral change. Removing the hard-coded structuralSharing: true default means TanStack Query's own default (true) still applies when the option is omitted, so existing consumers are unaffected. But consumers can now explicitly set false.
  • Test is focused and verifiable. The test creates a collection with structuralSharing: false, then asserts the option landed on the cached query's options — directly verifying forwarding without over-testing.
  • Changeset included with correct scope (patch) and package.

Concerns

  1. Missing JSDoc comment. Every other forwarded option in QueryCollectionConfig (enabled, refetchInterval, retry, retryDelay, staleTime) has a brief JSDoc comment. structuralSharing has none. This is a minor consistency issue — adding something like /** Whether to use structural sharing for query data (default: true) */ would match the pattern.

  2. Alphabetical import sort in builder/index.ts. The diff shows GetResult and FunctionalHavingRow swapped to alphabetical order. This is a harmless cleanup but unrelated to the fix — ideally would be in a separate commit or mentioned in the description. Very minor.

  3. No concern about the default change. I verified that TanStack Query's own default for structuralSharing is true, so removing the hard-coded true and only passing it when explicitly set does not change behavior for existing users. This is correct.

Verdict

Merge with minor fixes — ask for a one-line JSDoc comment on the structuralSharing property for consistency with the other options.

Suggested comment

Clean PR — follows the established forwarding pattern perfectly and the test is well-scoped. One minor ask: could you add a JSDoc comment on structuralSharing in the QueryCollectionConfig interface to match the other options (e.g., /** Whether to use structural sharing for query data (default: true) */)? Otherwise this is good to merge.

Good — now I have all the context I need. The PR adds refetchOnWindowFocus, refetchOnReconnect, and networkMode to the destructuring and observer options, but also sneaks in a removal of structuralSharing: true.

Summary

Adds three new pass-through options (refetchOnWindowFocus, refetchOnReconnect, networkMode) to the QueryCollectionConfig interface and forwards them to the underlying TanStack Query QueryObserver. Follows the existing pattern of conditionally spreading only when explicitly defined. Includes 317 lines of new tests.

Strengths

  • Follows established patterns exactly — the type definitions, destructuring, and conditional spreading all match the existing staleTime/retry/etc. pattern line-for-line. Clean contribution.
  • Changeset included with correct minor bump.
  • Good config interface design — uses indexed access types (QueryObserverOptions[...]) rather than re-declaring types, keeping the contract tied to TanStack Query's types.

Concerns

  1. Unrelated removal of structuralSharing: true — The diff removes structuralSharing: true from the observer options (line 729 on main). This is not mentioned in the PR description and is a behavioral change: it switches structural sharing from explicitly enabled to whatever the QueryClient default is (which happens to be true, but could differ if someone sets defaultOptions). At minimum this should be called out; ideally it should be reverted or split into its own PR/changeset.

  2. Unrelated import sort change — The packages/db/src/query/builder/index.ts diff reorders GetResult and FunctionalHavingRow alphabetically. Harmless, but unrelated to this feature and adds noise.

  3. Tests don't verify the options actually reach the observer — Every test creates a collection, waits for the query to resolve, then asserts collection.size. None of them inspect the actual QueryObserver to confirm refetchOnWindowFocus, refetchOnReconnect, or networkMode were forwarded. They only prove the collection doesn't crash with these options set. A single test that checks observer.options.refetchOnWindowFocus === true (or similar) would be far more meaningful.

  4. High test-to-value ratio — 317 lines of tests for ~10 lines of implementation, yet the tests are essentially identical copy-paste with different option values. These could be parameterized with it.each to cover the same ground in ~30-40 lines.

Verdict

Merge with minor fixes — The structuralSharing removal needs to be reverted (or justified and changesetted separately). The tests work but would benefit from actually asserting the options reach the observer.

Suggested comment

Thanks for the clean contribution — this follows the existing patterns well! Two things before merge: (1) The removal of structuralSharing: true appears unintentional and should be reverted since it's an unrelated behavioral change. (2) The tests currently only verify the collection doesn't break — could you add at least one assertion that checks the option actually reached the QueryObserver (e.g., inspecting observer.options)? The tests could also be condensed with it.each to reduce duplication.

Good — solid-db has the exact same pattern at line 278. The PR correctly aligns svelte-db with react-db and solid-db.

Summary

When useLiveQuery receives a Svelte getter that returns null (common during SSR), toValue() unwraps it to null, which falls through to the else branch where ...null throws a runtime error. This PR adds a null/undefined guard before the spread, returning null early to signal a disabled query. The downstream $effect already handles a null collection correctly (clears state, sets status: 'disabled').

Strengths

  • Correct root cause fix. The null check is placed exactly where the crash occurs — in the else branch at the spread site. The isCollection guard and the typeof === 'function' branch already handle null safely; only this code path was missing.
  • Consistent with other framework adapters. Both react-db (line 379) and solid-db (line 278) have equivalent null checks in the same logical position. This is a genuine parity fix.
  • Good test. The test simulates the actual SSR scenario: starts with null, then sets the real collection, and verifies both the empty state and the reactive update.
  • Changeset included.

Concerns

  • Minor: The test name says "nullable collection getter returning null" but what it actually tests is a $state variable that transitions from null to a live query collection. This is the right thing to test, but the name could be slightly more precise (e.g., "should handle getter returning null then becoming available"). Very minor — not blocking.
  • No concern about the code change itself. Three lines, well-placed, matches established patterns.

Verdict

Merge. Clean, minimal fix for a real SSR crash. Matches existing patterns in react-db and solid-db exactly.

Suggested comment

Clean fix — the null guard is placed exactly where the spread would crash, and it matches the same pattern in react-db and solid-db. LGTM, merging. Thanks @elohmeier!

This is the key finding. Line 78-81 of the scheduler:

if (typeof contextId === `undefined`) {
  run()
  return
}

When contextId is undefined (i.e., not within a transaction), the graph run executes immediately and synchronously. Looking back at the scheduleGraphRun call:

const contextId = options?.contextId ?? getActiveTransaction()?.id

If there's no active transaction, contextId is undefined, so scheduleGraphRuntransactionScopedScheduler.schedule() → runs synchronously.

This means that for the SSR case (no active transaction), when createLiveQueryCollection is called with startSync: true and the source collection already has data synchronously:

  1. Source collection's sync fires synchronously (begin/write/commit/markReady)
  2. Live query collection subscribes to source, gets initial state
  3. D2 graph runs synchronously via the scheduler
  4. Data is available in collection.entries()/values() immediately

So the PR's approach does work correctly when the source collection has synchronous data. The tests are valid. Now I have everything I need for the review.

Summary

When Svelte renders on the server, $effect never runs, so useLiveQuery returned empty state and data even when the underlying collection was populated with initial data. This PR adds a synchronous initialization block that copies collection.entries() and collection.values() into the reactive state map and internalData array immediately after their declarations, before any effects. Three new tests cover the SSR path.

Strengths

  • Correct diagnosis: $effect genuinely does not run during Svelte SSR, so synchronous seeding is necessary. This matches how react-db's useSyncExternalStore provides data during the render phase.
  • Safe on the client: The $effect block (lines 408-417) clears and reinitializes state/internalData when it runs, so the SSR block is simply overwritten — no double-data bugs.
  • Data is filtered: Because collection is the live query collection (post-D2 pipeline), collection.entries() already contains filtered/projected results, not raw source data. The scheduler runs the D2 graph synchronously when there's no active transaction, so data flows through the pipeline immediately when the source has synchronous initialData.
  • Good test coverage: Tests verify the critical "before flushSync" assertion, pre-created collections, and the empty-collection case.
  • Includes changeset.

Concerns

  1. Comment on the $effect is misleading: The diff changes the comment from "Watch for collection changes and subscribe to updates" to "Watch for collection changes and subscribe to updates (client-side only)". The effect does still run on the client, so "(client-side only)" is technically accurate about when it runs, but it reads like the effect is somehow conditional. A clearer comment would be something like "runs on client only; SSR data is initialized synchronously above".

  2. No status initialization in the SSR block: The SSR block initializes state and internalData, but status is already initialized on line 367 from collection.status. If the collection is ready at that point, status will be correct. But if the collection is still loading (because markReady hasn't been called yet), SSR consumers checking isReady would see false even though data is present. This seems like a pre-existing edge case, not introduced by this PR, but worth noting.

  3. Minor duplication: The SSR init block duplicates the logic in the $effect body (lines 409-417). If the initialization logic ever changes (e.g., adding filtering or transformations), it would need updating in two places. Consider extracting a shared helper, though this is a minor style point.

Verdict

Merge with minor fixes — the SSR init comment on the $effect should be clarified. The core fix is correct and well-tested.

Suggested comment

Clean fix, and the tests cover the important cases well. One small thing: the comment (client-side only) on the $effect could be slightly confusing — maybe rephrase to clarify that SSR initialization happens synchronously above. Otherwise this looks good to merge!

Summary

The initial useLiveQuery call for query-function mode fetched exactly pageSize items, but the setWindow effect immediately requests pageSize + 1 (for peek-ahead). On syncMode: "on-demand", this one-item gap causes a timing issue where the peek-ahead item isn't available yet, making hasNextPage incorrectly return false on initial load. The fix changes the initial .limit(pageSize) to .limit(pageSize + 1) so the first fetch already includes the peek-ahead item.

Strengths

  • Correctly identified root cause: The mismatch between initial limit (pageSize) and the setWindow expectation (pageSize + 1) is real — line 198 clearly shows loadedPageCount * pageSize + 1.
  • Minimal, targeted change: Only one line of logic changed; the fix aligns the initial query with what the window management already expects.
  • Good changeset & description: The PR body clearly explains the problem, the mechanism, and why the returned data is unaffected (the useMemo at line 254-285 already slices to exclude the peek-ahead item).
  • Contributor track record: melvinhagberg has been consistently submitting well-scoped infinite query fixes.

Concerns

  1. Collection path is unaffected — The fix only applies to the query-function branch (line 189-191). When isCollection is true (line 188), the pre-created collection's initial limit is set by the caller. The setWindow effect handles this case, but it's worth noting that the same class of bug could appear there if a user sets .limit(pageSize) in their pre-created collection. This is already somewhat addressed by the console.warn at line 220, but could use a docs callout.

  2. No test — This is a race-condition-style bug specific to syncMode: "on-demand" with predicate push-down. It would be hard to unit test without mocking the async loading pipeline, so the lack of a test is understandable but worth flagging.

  3. Changeset checked box mismatch — The "This change is docs/CI/dev-only" checkbox is unchecked but a changeset is included and the "affects published code" box is also unchecked. The changeset is correct; the checkboxes are just slightly inconsistent (minor).

Verdict

Merge — The fix is correct, minimal, and well-explained. The initial limit now matches what setWindow expects on the very next render, eliminating the race condition.

Suggested comment

Clean fix — the initial limit now correctly matches the peek-ahead expectation from setWindow. LGTM. One minor note: the "Release Impact" checkboxes are slightly inconsistent (changeset is included but the "affects published code" box is unchecked), so you may want to tick that box.

Now I have a complete picture. Here's the review:

Summary

When JOIN queries use on-demand sync mode, the tap() operator in processJoin fires on every stream emission, calling requestSnapshot() with the same join keys repeatedly. This causes loadedSubsets to grow unboundedly, eventually leading to "too many SQL variables" errors. The fix adds two layers of deduplication: a loadedJoinKeys Set in joins.ts to skip already-requested keys, and an isPredicateSubset check in subscription.ts before pushing to loadedSubsets.

Strengths

  • Correct root cause identification. The two-layer fix is appropriate — filtering at the join level (where keys are known) avoids unnecessary requestSnapshot calls entirely, and the subscription-level check is a safety net for other callers.
  • Good use of existing isPredicateSubset. Rather than inventing new dedup logic, it reuses the well-tested predicate comparison infrastructure.
  • Tests cover the right scenarios: repeated graph runs from updates, new users with no matching orders, and multi-update loops. The assertions check for bounded growth rather than exact counts, which is resilient.

Concerns

  1. loadedJoinKeys is never cleared on truncation. The Set lives in processJoin's closure — it persists for the subscription's entire lifetime. When handleTruncate() fires, subscription.loadedSubsets is cleared and re-requested (lines 154-202), but the loadedJoinKeys Set in joins.ts still contains the old keys. This means after a truncate/must-refetch, the lazy loading will think those keys are already loaded and skip re-requesting them. This is a correctness bug that would manifest in 409 must-refetch scenarios.

  2. Asymmetry between load and unload. Before this PR, every loadSubset() call was matched by an unloadSubset() call on cleanup (line 728). With the subscription-level dedup, some loadSubset() calls to the sync layer still happen (the actual sync request fires before the loadedSubsets.push check), but the matching unloadSubset() is skipped. If the sync layer uses reference counting for active subsets, this creates a load/unload mismatch. Worth verifying the sync layer handles this correctly.

  3. isPredicateSubset is directional — but the check may need both directions. The code checks isPredicateSubset(loadOptions, existingOptions) — "is the new request a subset of something already loaded?" This is correct for preventing duplicates. But it doesn't handle the case where a new, wider request supersedes an old narrower one (e.g., inArray(ref, [1]) followed by inArray(ref, [1, 2])). The second call would correctly be added, but the first entry remains, meaning both get unloaded separately. This is fine for correctness but leaves the stale narrow entry in the array.

  4. Minor: test assertions are loose. expect(afterInsertCallCount).toBeLessThanOrEqual(initialCallCount + 1) — the +1 slack makes the tests pass even if dedup is partially broken. A tighter assertion (e.g., toEqual(initialCallCount)) would catch regressions more effectively, at least for the cases where no new keys are introduced.

Verdict

Needs work. Concern #1 (truncation invalidation) is a real correctness bug that would cause stale data after must-refetch. The loadedJoinKeys Set needs to be cleared when the subscription resets state. This could be done via a callback/event from the subscription, or by tying the Set's lifecycle to something that gets recreated on truncation.

Suggested comment

Nice catch on the unbounded growth issue — this is a real problem in production. Two things to address before merge: (1) the loadedJoinKeys Set in joins.ts isn't cleared when a truncation/must-refetch happens, which means after a 409 the lazy loader will skip re-requesting keys it already has cached, leading to missing data. Consider adding a reset mechanism tied to handleTruncate(). (2) Could you tighten the test assertions — e.g., toEqual(initialCallCount) instead of toBeLessThanOrEqual(initialCallCount + 1) for the cases where no genuinely new keys are introduced?

Now I have a thorough understanding of the PR. Here's the review:

Summary

When using useLiveInfiniteQuery with syncMode: 'on-demand', navigating between different queries on the same collection (e.g., Inbox → All Emails) causes stale data from the previous query to appear on the first page. The fix adds a hasLoadSubsetBeenCalled boolean flag on CollectionSyncManager and checks it in requestLimitedSnapshot — if a prior loadSubset has occurred and this is the subscription's first snapshot, it skips reading from the local index (which may contain another query's data) and lets loadSubset provide fresh data instead.

Strengths

  • Well-scoped: Only 3 files changed, minimal surface area for the fix.
  • Clear PR description: The root cause and fix rationale are well explained.
  • Correct heuristic: Distinguishing "initial sync data (valid for all queries)" from "loadSubset data (query-specific)" is a sound approach.
  • Changeset included.

Concerns

  1. hasLoadSubsetBeenCalled is never reset on cleanup. CollectionSyncManager.cleanup() (line 373 of sync.ts) doesn't reset the flag. If a collection is cleaned up and re-synced, the flag stays true permanently, meaning all future first-snapshots will skip local data — even when the local index contains fresh, valid initial-sync data. This could cause unnecessary empty first renders after reconnection.

  2. No tests. This is a subtle race condition fix in a critical code path. There are no tests demonstrating the contamination or verifying the fix. Even a unit test showing that keys stays empty when hasLoadSubsetBeenCalled=true && loadedSubsets.length===0 would increase confidence.

  3. Global flag is coarse-grained. hasLoadSubsetBeenCalled is a one-way latch on the entire collection. Once any subscription calls loadSubset, every new subscription's first snapshot is affected forever. A more precise approach might be to check if the local index actually contains data relevant to the current subscription's query, but I acknowledge that's significantly more complex.

  4. Comment heaviness. The 8-line block comment explaining the fix in subscription.ts is verbose for what amounts to two boolean checks. The PR description already captures the rationale — a 2-line comment would suffice inline.

  5. The // else: trailing comment (line after takeFromStart) explains control flow that's already clear from the conditionals. Minor nit.

Verdict

Needs work — the missing reset in cleanup() is a correctness issue, and the absence of tests for a subtle data-integrity bug is concerning. Fix the cleanup reset, add at least one test, and this is ready to merge.

Suggested comment

Thanks for tracking this down, Melvin — the root cause analysis is solid. Two things before merge: (1) hasLoadSubsetBeenCalled should be reset in CollectionSyncManager.cleanup() so it doesn't persist across sync restarts, and (2) could you add a test that demonstrates the contamination scenario? Even a focused unit test would give us confidence this stays fixed.

Summary

Adds a README.md to the @tanstack/rxdb-db-collection package so it displays on the npm page, and enhances package.json with a better description, more keywords, a bugs URL, and a more specific homepage link pointing to the RxDB collection docs.

Strengths

  • Practical improvement — npm packages without READMEs look abandoned; this is a simple quality-of-life fix.
  • The README is concise and links to the right docs.
  • The package.json enrichments (keywords, bugs URL, specific homepage) are all reasonable and improve discoverability.
  • PR author is the RxDB maintainer, so the "About RxDB" description is authoritative.

Concerns

  • typescript keyword removed — The diff drops "typescript" from keywords. This package is written in TypeScript and that keyword helps discoverability. Likely unintentional.
  • Author field unchanged"author": "Kyle Mathews" stays, which is fine for this PR, but worth noting the README was written by the RxDB author (pubkey). Not a blocker.
  • Minor: The README links to https://tanstack.com/db/latest/docs/collections/rxdb-collection — worth verifying this URL actually resolves. If the docs page doesn't exist yet, the link will 404.
  • No changeset needed — PR correctly notes this is docs-only with no release impact. Confirmed: README and package.json metadata changes don't affect published code behavior.

Verdict

Merge with minor fixes — Just restore the typescript keyword.

Suggested Comment

Thanks for adding this, @pubkey! Looks great. One small thing: the "typescript" keyword got dropped from package.json — could you add it back? It helps with npm discoverability. Otherwise this is good to merge.

Now I have a thorough understanding of the codebase. Let me write the review.

Summary

Adds an optional querySchema field to the query builder's Context interface, enabling users to restrict which fields are available in where/orderBy callbacks while keeping the full document type for results. Provides two API surfaces: a queryable config option on createLiveQueryCollection and a withQueryableFields() utility that brands a collection. This is purely additive type-level work — no runtime behavior changes.

Strengths

  • Real use case: On-demand sync collections where only some fields have backend indexes is a legitimate problem. This is a practical feature request.
  • Good test coverage: 214 lines of test-d.ts type tests covering happy paths, disallowed fields, empty lists, and subquery propagation.
  • Backwards compatible: querySchema defaults to schema via ResolveQuerySchema, so existing code is unaffected.
  • Refactored RefsForContext well: Extracting RefsForSchema<TSchema, TResult> as a shared implementation for both RefsForContext and QueryRefsForContext is clean.

Concerns

1. Three as any casts added to builder/index.ts (lines 747, 810, 836, 864)

findOne(), fn.select(), fn.where(), and fn.having() all gained as any casts to suppress type errors from the new querySchema field. This is a red flag — it means the Context type threading is incomplete in these paths. Adding querySchema to Context introduced type incompatibilities that were papered over rather than fixed. These should be properly resolved.

2. Duplicate type definitions between queryable-fields.ts and live-query-collection.ts

QueryableFieldList, KeysFromList, and ResolveQueryableSchema are defined in both files with identical or near-identical implementations. This should be consolidated into one location.

3. Two parallel API surfaces for the same thing

withQueryableFields(collection, config) brands a collection, while queryable: { filterable, sortable } on createLiveQueryCollection config does the same thing differently. Having both creates confusion about which to use and when. The PR body doesn't explain why both are needed. I'd recommend picking one approach — the config-based queryable option seems more natural since it lives at the query definition site.

4. ResolveQueryableSchema logic bug with undefined filterable

type ResolveQueryableSchema<TDocument, TFilterable, TSortable> =
  [TFilterable] extends [undefined]
    ? [TSortable] extends [undefined]
      ? TDocument  // both undefined → full document ✓
      : Pick<TDocument, Extract<KeysFromList<TFilterable> | KeysFromList<TSortable>, keyof TDocument>>
    : Pick<...>

When TFilterable is undefined but TSortable is defined, the Pick includes KeysFromList<undefined> which resolves to never — so it works by accident. But the intent seems wrong: the true branch (filterable is undefined, sortable is defined) should only pick from TSortable, not union with TFilterable.

5. queryable property added to LiveQueryCollectionConfig interface but typed loosely

In live/types.ts, the new queryable field uses ReadonlyArray<string> instead of constraining keys to the document type. The type safety only comes from the overloads in live-query-collection.ts, making the base interface misleading.

6. The QuerySchemaBrand approach is clever but adds permanent complexity

The branded CollectionWithQuerySchema type adds a phantom field to collections. This means any utility type that pattern-matches on CollectionImpl now has a hidden extra field. The InferCollectionQuerySchema fallback to InferCollectionType is correct but this pattern will surprise future contributors.

Verdict

Needs work. The core idea is sound, but the as any casts indicate the type threading is incomplete, the duplicate types need consolidation, and the two parallel APIs (config queryable vs withQueryableFields) should be rationalized. Fixing the as any casts is the most important item — they could mask real type errors in user code flowing through findOne() or fn.* methods.

Suggested comment

Thanks for this PR — restricting queryable fields for on-demand sync is a great idea and a real need! A few things to address before merge: (1) the as any casts in builder/index.ts suggest the querySchema threading through findOne/fn.* is incomplete — those should be properly typed rather than cast, (2) ResolveQueryableSchema/KeysFromList/QueryableFieldList are duplicated between queryable-fields.ts and live-query-collection.ts and should be consolidated, and (3) having both withQueryableFields() and the config queryable option as parallel APIs may be more surface area than needed — could we start with just one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment