diff --git a/.changeset/fix-stale-optimistic-server-key.md b/.changeset/fix-stale-optimistic-server-key.md new file mode 100644 index 0000000000..0d6c43a0f1 --- /dev/null +++ b/.changeset/fix-stale-optimistic-server-key.md @@ -0,0 +1,5 @@ +--- +'@tanstack/db': patch +--- + +Fix stale optimistic rows persisting when sync confirms a different server-generated key. Previously, direct transactions (from `collection.insert()` etc.) had their optimistic rows exempted from stale-row cleanup, which prevented temp-key rows from being removed when the server returned a different primary key. diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index a7afbaab1a..3678cd19f2 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -28,7 +28,7 @@ jobs: - name: Setup Node.js uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: - node-version: '20' + node-version: '22.13' cache: 'pnpm' - name: Install dependencies diff --git a/AGENTS.md b/AGENTS.md index 6654f89bfc..a92ff46761 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,6 +14,7 @@ This guide provides principles and patterns for AI agents contributing to the Ta 8. [Function Design](#function-design) 9. [Modern JavaScript Patterns](#modern-javascript-patterns) 10. [Edge Cases and Corner Cases](#edge-cases-and-corner-cases) +11. [Git and PR Hygiene](#git-and-pr-hygiene) ## Type Safety @@ -563,6 +564,37 @@ const filtered = items.filter((item) => item.value > 0) // Should ignore the stale snapshot ``` +## Git and PR Hygiene + +### Never Rewrite Published Branch History + +**Do not amend, rebase, squash, or force-push a branch after it has been pushed or has an open PR.** This includes `git push --force` and `git push --force-with-lease`. + +Once a branch is visible to others, treat its history as shared. If CI fails or follow-up changes are needed, add a normal follow-up commit and push normally. + +**❌ Bad:** + +```bash +git commit --amend --no-edit +git push --force-with-lease +``` + +**✅ Good:** + +```bash +git add +git commit -m "fix: address CI failure" +git push +``` + +**Key Principles:** + +- Never force-push unless the user explicitly asks for it in that moment. +- Do not assume `--force-with-lease` is acceptable; it still rewrites shared history. +- Prefer small follow-up commits over rewritten history on PR branches. +- If a clean history is desired, let the human maintainer squash or rebase during merge. +- If you think history rewriting is necessary, stop and ask for explicit confirmation before running any command. + ## Package Versioning ### Understand Semantic Versioning diff --git a/packages/db/src/collection/mutations.ts b/packages/db/src/collection/mutations.ts index 963110801c..765e409ef6 100644 --- a/packages/db/src/collection/mutations.ts +++ b/packages/db/src/collection/mutations.ts @@ -231,9 +231,7 @@ export class CollectionMutationsManager< } else { // Create a new transaction with a mutation function that calls the onInsert handler const directOpTransaction = createTransaction({ - metadata: { - [DIRECT_TRANSACTION_METADATA_KEY]: true, - }, + metadata: { [DIRECT_TRANSACTION_METADATA_KEY]: true }, mutationFn: async (params) => { // Call the onInsert handler with the transaction and collection return await this.config.onInsert!({ @@ -430,9 +428,7 @@ export class CollectionMutationsManager< // Create a new transaction with a mutation function that calls the onUpdate handler const directOpTransaction = createTransaction({ - metadata: { - [DIRECT_TRANSACTION_METADATA_KEY]: true, - }, + metadata: { [DIRECT_TRANSACTION_METADATA_KEY]: true }, mutationFn: async (params) => { // Call the onUpdate handler with the transaction and collection return this.config.onUpdate!({ @@ -536,9 +532,7 @@ export class CollectionMutationsManager< // Create a new transaction with a mutation function that calls the onDelete handler const directOpTransaction = createTransaction({ autoCommit: true, - metadata: { - [DIRECT_TRANSACTION_METADATA_KEY]: true, - }, + metadata: { [DIRECT_TRANSACTION_METADATA_KEY]: true }, mutationFn: async (params) => { // Call the onDelete handler with the transaction and collection return this.config.onDelete!({ diff --git a/packages/db/src/collection/state.ts b/packages/db/src/collection/state.ts index af65cb8016..9cbdebb234 100644 --- a/packages/db/src/collection/state.ts +++ b/packages/db/src/collection/state.ts @@ -1160,6 +1160,33 @@ export class CollectionStateManager< } } + // A completed optimistic insert may have used a temporary client key while + // the sync confirmation used a different server-generated key. Once a + // sync commit has been applied, stop retaining completed optimistic keys + // that were not confirmed by this commit so the temporary row is removed. + for (const key of this.pendingOptimisticDirectUpserts) { + if (!changedKeys.has(key)) { + changedKeys.add(key) + if (!currentVisibleState.has(key)) { + const previousValue = previousOptimisticUpserts.get(key) + if (previousValue !== undefined) { + currentVisibleState.set(key, previousValue) + } + } + this.pendingOptimisticUpserts.delete(key) + this.pendingLocalOrigins.delete(key) + } + } + for (const key of this.pendingOptimisticDirectDeletes) { + if (!changedKeys.has(key)) { + changedKeys.add(key) + } + this.pendingOptimisticDeletes.delete(key) + this.pendingLocalOrigins.delete(key) + } + this.pendingOptimisticDirectUpserts.clear() + this.pendingOptimisticDirectDeletes.clear() + // Now check what actually changed in the final visible state for (const key of changedKeys) { const previousVisibleValue = currentVisibleState.get(key) diff --git a/packages/db/tests/collection.test.ts b/packages/db/tests/collection.test.ts index c5d09039e5..7fc5d67b46 100644 --- a/packages/db/tests/collection.test.ts +++ b/packages/db/tests/collection.test.ts @@ -11,6 +11,7 @@ import { MissingUpdateHandlerError, } from '../src/errors' import { createTransaction } from '../src/transactions' +import { createLiveQueryCollection, eq } from '../src/query/index.js' import { flushPromises, mockSyncCollectionOptionsNoInitialState, @@ -41,6 +42,95 @@ describe(`Collection`, () => { expect(() => createCollection()).toThrow(CollectionRequiresConfigError) }) + it(`removes optimistic insert when sync confirms with a different server-generated key`, async () => { + const options = mockSyncCollectionOptionsNoInitialState<{ + id: number + text: string + }>({ + id: `server-generated-key-test`, + getKey: (item) => item.id, + startSync: true, + }) + const collection = createCollection(options) + + options.utils.markReady() + await collection.stateWhenReady() + + const tx = collection.insert({ id: 4733, text: `two` }) + + expect(getStateEntries(collection)).toEqual([ + [4733, { id: 4733, text: `two` }], + ]) + + options.utils.begin() + options.utils.write({ type: `insert`, value: { id: 24, text: `two` } }) + options.utils.commit() + + // The sync commit is held while the local insert transaction is persisting. + expect(getStateEntries(collection)).toEqual([ + [4733, { id: 4733, text: `two` }], + ]) + + options.utils.resolveSync() + await tx.isPersisted.promise + await flushPromises() + + expect(getStateEntries(collection)).toEqual([[24, { id: 24, text: `two` }]]) + }) + + it(`updates live queries when an optimistic insert is replaced by a different server key`, async () => { + const options = mockSyncCollectionOptionsNoInitialState<{ + id: number + text: string + project_id: number + }>({ + id: `server-generated-key-live-query-test`, + getKey: (item) => item.id, + startSync: true, + }) + const collection = createCollection(options) + const liveCollection = createLiveQueryCollection((q) => + q + .from({ collection }) + .where(({ collection }) => eq(collection.project_id, 1)) + .select(({ collection }) => ({ + id: collection.id, + text: collection.text, + project_id: collection.project_id, + $synced: collection.$synced, + $origin: collection.$origin, + $key: collection.$key, + })), + ) + + options.utils.markReady() + await liveCollection.preload() + + const tx = collection.insert({ id: 4733, text: `two`, project_id: 1 }) + + expect(liveCollection.toArray.map((todo) => todo.id)).toEqual([4733]) + + options.utils.begin() + options.utils.write({ + type: `insert`, + value: { id: 24, text: `two`, project_id: 1 }, + }) + options.utils.commit() + + options.utils.resolveSync() + await tx.isPersisted.promise + await flushPromises() + + expect( + liveCollection.toArray.map((todo) => ({ + id: todo.id, + synced: todo.$synced, + origin: todo.$origin, + key: todo.$key, + })), + ).toEqual([{ id: 24, synced: true, origin: `remote`, key: 24 }]) + }) + it(`should throw an error when trying to use mutation operations outside of a transaction`, async () => { // Create a collection with sync but no mutationFn const collection = createCollection<{ value: string }>({ diff --git a/packages/db/tests/utils.ts b/packages/db/tests/utils.ts index 2b5c5d0c88..41fc65a9d8 100644 --- a/packages/db/tests/utils.ts +++ b/packages/db/tests/utils.ts @@ -311,6 +311,7 @@ type MockSyncCollectionConfigNoInitialState = { id: string getKey: (item: T) => string | number autoIndex?: `off` | `eager` + startSync?: boolean defaultIndexType?: IndexConstructor } diff --git a/packages/electric-db-collection/tests/electric.test.ts b/packages/electric-db-collection/tests/electric.test.ts index 30b15887b4..868c085213 100644 --- a/packages/electric-db-collection/tests/electric.test.ts +++ b/packages/electric-db-collection/tests/electric.test.ts @@ -739,6 +739,49 @@ describe(`Electric Integration`, () => { expect(testCollection._state.syncedData.size).toEqual(1) }) + it(`should remove optimistic insert when txid sync confirms a different server-generated key`, async () => { + const txid = 1234 + const onInsert = vi.fn().mockResolvedValue({ txid }) + + const testCollection = createCollection( + electricCollectionOptions({ + id: `test-server-generated-key-txid`, + shapeOptions: { + url: `http://test-url`, + params: { table: `test_table` }, + }, + startSync: true, + getKey: (item: Row) => item.id as number, + onInsert, + }), + ) + + const tx = testCollection.insert({ id: 4733, text: `two` }) + + expect(stripVirtualProps(testCollection.get(4733))).toEqual({ + id: 4733, + text: `two`, + }) + + subscriber([ + { + key: `24`, + value: { id: 24, text: `two` }, + headers: { operation: `insert`, txids: [txid] }, + }, + { headers: { control: `up-to-date` } }, + ]) + + await tx.isPersisted.promise + + expect(testCollection.has(4733)).toBe(false) + expect(stripVirtualProps(testCollection.get(24))).toEqual({ + id: 24, + text: `two`, + }) + expect(Array.from(testCollection.state.keys())).toEqual([24]) + }) + it(`should support void strategy when handler returns nothing`, async () => { const onInsert = vi.fn().mockResolvedValue(undefined)