From 6454ff62b1bb48ae7d84ee8e68f171ab49214b2d Mon Sep 17 00:00:00 2001 From: Eduardo Costa Date: Wed, 11 Mar 2026 12:20:46 +0000 Subject: [PATCH 1/5] fix(ui): array ghost item after reorder and delete with autosave --- .../ui/src/forms/Form/mergeServerFormState.ts | 65 +++++++++++ test/form-state/int.spec.ts | 105 +++++++++++++++++- 2 files changed, 169 insertions(+), 1 deletion(-) diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index d9fcfd3ba89..f940254c956 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -43,16 +43,70 @@ export const mergeServerFormState = ({ }: Args): FormState => { const newState = { ...currentState } + /** + * Pre-compute row ID maps for all array/block fields with rows. + * This allows us to detect when a row has been moved or deleted on the client + * while a request was in-flight, so that flattened row fields (e.g. `array.1.text`) + * are not overwritten with stale server data from a different row at the same index. + */ + const serverRowIdAtIndex = new Map>() // arrayPath -> (serverIndex -> rowId) + const clientRowIdAtIndex = new Map>() // arrayPath -> (clientIndex -> rowId) + + for (const [path, incomingField] of Object.entries(incomingState || {})) { + if (Array.isArray(incomingField.rows) && path in currentState) { + const serverMap = new Map() + incomingField.rows.forEach((row, i) => serverMap.set(i, row.id)) + serverRowIdAtIndex.set(path, serverMap) + + const clientMap = new Map() + ;(currentState[path]?.rows || []).forEach((row, i) => clientMap.set(i, row.id)) + clientRowIdAtIndex.set(path, clientMap) + } + } + for (const [path, incomingField] of Object.entries(incomingState || {})) { if (!(path in currentState) && !incomingField.addedByServer) { continue } + /** + * Check if this is a flattened row field (e.g. `array.1.text`) whose row index on the server + * points to a different row than the same index on the client. This can happen when the user + * reorders or deletes rows while an autosave request is in-flight: the server responds with + * stale index-based data that no longer corresponds to the client's row order. + * In that case, do not accept the server's value to avoid overwriting the correct local data. + */ + let rowIndexMismatch = false + if ( + typeof acceptValues === 'object' && + acceptValues !== null && + acceptValues.overrideLocalChanges === false + ) { + for (const [arrayPath, serverMap] of serverRowIdAtIndex) { + if (path.startsWith(`${arrayPath}.`)) { + const remainder = path.slice(arrayPath.length + 1) + const dotIdx = remainder.indexOf('.') + if (dotIdx > -1) { + const idx = parseInt(remainder.slice(0, dotIdx), 10) + if (!isNaN(idx)) { + const serverRowId = serverMap.get(idx) + const clientRowId = clientRowIdAtIndex.get(arrayPath)?.get(idx) + if (serverRowId !== clientRowId) { + rowIndexMismatch = true + } + } + } + break + } + } + } + /** * If it's a new field added by the server, always accept the value. * Otherwise: * a. accept all values when explicitly requested, e.g. on submit * b. only accept values for unmodified fields, e.g. on autosave + * — but not when the row at this index has changed (reorder/delete race condition) */ const shouldAcceptValue = incomingField.addedByServer || @@ -61,6 +115,7 @@ export const mergeServerFormState = ({ acceptValues !== null && // Note: Must be explicitly `false`, allow `null` or `undefined` to mean true acceptValues.overrideLocalChanges === false && + !rowIndexMismatch && !currentState[path]?.isModified) let sanitizedIncomingField = incomingField @@ -121,6 +176,16 @@ export const mergeServerFormState = ({ newState[path].rows.push(newRow) } }) + + /** + * Ensure the `value` (row count) always reflects the actual number of rows after the merger. + * When the server sends a `value` that differs from the corrected rows count (e.g. because + * rows were deleted on the client while the request was in-flight), override it with the + * actual count to keep `value` in sync with `rows.length`. + */ + if ('value' in incomingField) { + newState[path].value = newState[path].rows.length + } } // If `valid` is `undefined`, mark it as `true` diff --git a/test/form-state/int.spec.ts b/test/form-state/int.spec.ts index 344f0cdc64a..34c06d5916f 100644 --- a/test/form-state/int.spec.ts +++ b/test/form-state/int.spec.ts @@ -9,8 +9,8 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest' import type { NextRESTClient } from '../__helpers/shared/NextRESTClient.js' -import { devUser } from '../credentials.js' import { initPayloadInt } from '../__helpers/shared/initPayloadInt.js' +import { devUser } from '../credentials.js' import { postsSlug } from './collections/Posts/index.js' // eslint-disable-next-line payload/no-relative-monorepo-imports @@ -732,6 +732,109 @@ describe('Form State', () => { }) }) + it('should not overwrite local row field values with stale server data when rows were reordered and deleted during autosave (overrideLocalChanges: false)', () => { + /** + * Regression test for the "ghost item" bug: + * 1. User has array [A, B, C] and reorders to [C, A, B] + * 2. Autosave fires with state [C, A, B] (request in-flight) + * 3. User deletes row A (index 1) — client state becomes [C, B] + * 4. Server responds with state based on [C, A, B] (3 rows) + * 5. mergeServerFormState should preserve the local delete and not + * overwrite array.1.* (B's data) with the server's array.1.* (A's data) + */ + const currentState: FormState = { + array: { + value: 2, + rows: [ + { id: 'C' }, // index 0 + { id: 'B' }, // index 1 (A was deleted, B shifted from index 2 to 1) + ], + }, + 'array.0.text': { value: 'C text', initialValue: 'C text' }, + 'array.1.text': { value: 'B text', initialValue: 'B text' }, + } + + // Server had [C, A, B] before the delete happened + const serverState: FormState = { + array: { + value: 3, + rows: [ + { id: 'C' }, + { id: 'A' }, // A was deleted on the client + { id: 'B' }, + ], + }, + 'array.0.text': { value: 'C text', initialValue: 'C text' }, + 'array.1.text': { value: 'A text', initialValue: 'A text' }, // Stale: A's data at index 1 + 'array.2.text': { value: 'B text', initialValue: 'B text' }, + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: serverState, + }) + + // rows should reflect the client's state (A deleted) + expect(newState.array?.rows).toHaveLength(2) + expect(newState.array?.rows?.[0]).toMatchObject({ id: 'C' }) + expect(newState.array?.rows?.[1]).toMatchObject({ id: 'B' }) + + // value should match rows.length + expect(newState.array?.value).toBe(2) + + // array.1.text should keep B's data, not be overwritten with A's stale data + expect(newState['array.1.text']?.value).toBe('B text') + + // array.0.text should be unaffected (row C is at index 0 in both server and client) + expect(newState['array.0.text']?.value).toBe('C text') + }) + + it('should not overwrite local row field values with stale server data when rows were reordered during autosave (overrideLocalChanges: false)', () => { + /** + * Regression test: user reorders [A, B] to [B, A] during autosave. + * The server responds with the pre-reorder state [A, B]. + * array.0.* should keep B's data, not revert to A's data. + */ + const currentState: FormState = { + array: { + value: 2, + rows: [ + { id: 'B' }, // moved to index 0 + { id: 'A' }, // moved to index 1 + ], + }, + 'array.0.text': { value: 'B text', initialValue: 'B text' }, + 'array.1.text': { value: 'A text', initialValue: 'A text' }, + } + + // Server had [A, B] — before the reorder + const serverState: FormState = { + array: { + value: 2, + rows: [{ id: 'A' }, { id: 'B' }], + }, + 'array.0.text': { value: 'A text', initialValue: 'A text' }, // Stale at index 0 + 'array.1.text': { value: 'B text', initialValue: 'B text' }, // Stale at index 1 + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: serverState, + }) + + // rows should reflect the client's reordered state + expect(newState.array?.rows).toHaveLength(2) + expect(newState.array?.rows?.[0]).toMatchObject({ id: 'B' }) + expect(newState.array?.rows?.[1]).toMatchObject({ id: 'A' }) + + // array.0.text should keep B's data (not revert to A's stale data from server) + expect(newState['array.0.text']?.value).toBe('B text') + // array.1.text should keep A's data (not revert to B's stale data from server) + expect(newState['array.1.text']?.value).toBe('A text') + }) + it('should set rows to empty array for empty array fields', async () => { const req = await createLocalReq({ user }, payload) From 395316cf4b252867af951bd216402a9f8ac733a4 Mon Sep 17 00:00:00 2001 From: Patrik Kozak <35232443+PatrikKozak@users.noreply.github.com> Date: Wed, 11 Mar 2026 10:55:37 -0400 Subject: [PATCH 2/5] test: adds failing tests --- test/form-state/int.spec.ts | 153 ++++++++++++++++++++++++++---------- 1 file changed, 111 insertions(+), 42 deletions(-) diff --git a/test/form-state/int.spec.ts b/test/form-state/int.spec.ts index 34c06d5916f..0fa6eedddbf 100644 --- a/test/form-state/int.spec.ts +++ b/test/form-state/int.spec.ts @@ -732,40 +732,29 @@ describe('Form State', () => { }) }) - it('should not overwrite local row field values with stale server data when rows were reordered and deleted during autosave (overrideLocalChanges: false)', () => { + it('should preserve client row data after reorder and delete during autosave', () => { /** - * Regression test for the "ghost item" bug: - * 1. User has array [A, B, C] and reorders to [C, A, B] - * 2. Autosave fires with state [C, A, B] (request in-flight) - * 3. User deletes row A (index 1) — client state becomes [C, B] - * 4. Server responds with state based on [C, A, B] (3 rows) - * 5. mergeServerFormState should preserve the local delete and not - * overwrite array.1.* (B's data) with the server's array.1.* (A's data) + * Regression test for the "ghost item" bug. + * User reorders [A, B, C] → [C, A, B], autosave fires, then user deletes A. + * Server responds with stale [C, A, B]. Client should preserve [C, B] and + * not overwrite array.1 (B's data) with server's array.1 (A's data). */ const currentState: FormState = { array: { value: 2, - rows: [ - { id: 'C' }, // index 0 - { id: 'B' }, // index 1 (A was deleted, B shifted from index 2 to 1) - ], + rows: [{ id: 'C' }, { id: 'B' }], }, 'array.0.text': { value: 'C text', initialValue: 'C text' }, 'array.1.text': { value: 'B text', initialValue: 'B text' }, } - // Server had [C, A, B] before the delete happened const serverState: FormState = { array: { value: 3, - rows: [ - { id: 'C' }, - { id: 'A' }, // A was deleted on the client - { id: 'B' }, - ], + rows: [{ id: 'C' }, { id: 'A' }, { id: 'B' }], }, 'array.0.text': { value: 'C text', initialValue: 'C text' }, - 'array.1.text': { value: 'A text', initialValue: 'A text' }, // Stale: A's data at index 1 + 'array.1.text': { value: 'A text', initialValue: 'A text' }, 'array.2.text': { value: 'B text', initialValue: 'B text' }, } @@ -775,47 +764,35 @@ describe('Form State', () => { incomingState: serverState, }) - // rows should reflect the client's state (A deleted) expect(newState.array?.rows).toHaveLength(2) expect(newState.array?.rows?.[0]).toMatchObject({ id: 'C' }) expect(newState.array?.rows?.[1]).toMatchObject({ id: 'B' }) - - // value should match rows.length expect(newState.array?.value).toBe(2) - - // array.1.text should keep B's data, not be overwritten with A's stale data - expect(newState['array.1.text']?.value).toBe('B text') - - // array.0.text should be unaffected (row C is at index 0 in both server and client) expect(newState['array.0.text']?.value).toBe('C text') + expect(newState['array.1.text']?.value).toBe('B text') }) - it('should not overwrite local row field values with stale server data when rows were reordered during autosave (overrideLocalChanges: false)', () => { + it('should preserve client row data after reorder during autosave', () => { /** - * Regression test: user reorders [A, B] to [B, A] during autosave. - * The server responds with the pre-reorder state [A, B]. - * array.0.* should keep B's data, not revert to A's data. + * User reorders [A, B] → [B, A] during autosave. + * Server responds with stale [A, B]. Field values should remain with correct rows. */ const currentState: FormState = { array: { value: 2, - rows: [ - { id: 'B' }, // moved to index 0 - { id: 'A' }, // moved to index 1 - ], + rows: [{ id: 'B' }, { id: 'A' }], }, 'array.0.text': { value: 'B text', initialValue: 'B text' }, 'array.1.text': { value: 'A text', initialValue: 'A text' }, } - // Server had [A, B] — before the reorder const serverState: FormState = { array: { value: 2, rows: [{ id: 'A' }, { id: 'B' }], }, - 'array.0.text': { value: 'A text', initialValue: 'A text' }, // Stale at index 0 - 'array.1.text': { value: 'B text', initialValue: 'B text' }, // Stale at index 1 + 'array.0.text': { value: 'A text', initialValue: 'A text' }, + 'array.1.text': { value: 'B text', initialValue: 'B text' }, } const newState = mergeServerFormState({ @@ -824,17 +801,109 @@ describe('Form State', () => { incomingState: serverState, }) - // rows should reflect the client's reordered state expect(newState.array?.rows).toHaveLength(2) expect(newState.array?.rows?.[0]).toMatchObject({ id: 'B' }) expect(newState.array?.rows?.[1]).toMatchObject({ id: 'A' }) - - // array.0.text should keep B's data (not revert to A's stale data from server) expect(newState['array.0.text']?.value).toBe('B text') - // array.1.text should keep A's data (not revert to B's stale data from server) expect(newState['array.1.text']?.value).toBe('A text') }) + it('should preserve nested array row data after reorder during autosave', () => { + /** + * User reorders nested array blocks[0].items from [A, B] → [B, A] during autosave. + * Outer block unchanged. Server responds with stale inner array [A, B]. + * Field values should remain with correct rows at nested level. + */ + const currentState: FormState = { + blocks: { + value: 1, + rows: [{ id: 'block-1' }], + }, + 'blocks.0.items': { + value: 2, + rows: [{ id: 'B' }, { id: 'A' }], + }, + 'blocks.0.items.0.text': { value: 'B text', initialValue: 'B text' }, + 'blocks.0.items.1.text': { value: 'A text', initialValue: 'A text' }, + } + + const serverState: FormState = { + blocks: { + value: 1, + rows: [{ id: 'block-1' }], + }, + 'blocks.0.items': { + value: 2, + rows: [{ id: 'A' }, { id: 'B' }], + }, + 'blocks.0.items.0.text': { value: 'A text', initialValue: 'A text' }, + 'blocks.0.items.1.text': { value: 'B text', initialValue: 'B text' }, + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: serverState, + }) + + expect(newState.blocks?.rows).toHaveLength(1) + expect(newState.blocks?.rows?.[0]).toMatchObject({ id: 'block-1' }) + expect(newState['blocks.0.items']?.rows).toHaveLength(2) + expect(newState['blocks.0.items']?.rows?.[0]).toMatchObject({ id: 'B' }) + expect(newState['blocks.0.items']?.rows?.[1]).toMatchObject({ id: 'A' }) + expect(newState['blocks.0.items.0.text']?.value).toBe('B text') + expect(newState['blocks.0.items.1.text']?.value).toBe('A text') + }) + + it('should preserve nested array row data after reorder and delete during autosave', () => { + /** + * User reorders nested array blocks[0].items from [A, B, C] → [C, A, B], then deletes A. + * Outer block unchanged. Server responds with stale inner array [C, A, B]. + * Client should preserve [C, B] and not overwrite with stale data. + */ + const currentState: FormState = { + blocks: { + value: 1, + rows: [{ id: 'block-1' }], + }, + 'blocks.0.items': { + value: 2, + rows: [{ id: 'C' }, { id: 'B' }], + }, + 'blocks.0.items.0.text': { value: 'C text', initialValue: 'C text' }, + 'blocks.0.items.1.text': { value: 'B text', initialValue: 'B text' }, + } + + const serverState: FormState = { + blocks: { + value: 1, + rows: [{ id: 'block-1' }], + }, + 'blocks.0.items': { + value: 3, + rows: [{ id: 'C' }, { id: 'A' }, { id: 'B' }], + }, + 'blocks.0.items.0.text': { value: 'C text', initialValue: 'C text' }, + 'blocks.0.items.1.text': { value: 'A text', initialValue: 'A text' }, + 'blocks.0.items.2.text': { value: 'B text', initialValue: 'B text' }, + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: serverState, + }) + + expect(newState.blocks?.rows).toHaveLength(1) + expect(newState.blocks?.rows?.[0]).toMatchObject({ id: 'block-1' }) + expect(newState['blocks.0.items']?.rows).toHaveLength(2) + expect(newState['blocks.0.items']?.rows?.[0]).toMatchObject({ id: 'C' }) + expect(newState['blocks.0.items']?.rows?.[1]).toMatchObject({ id: 'B' }) + expect(newState['blocks.0.items']?.value).toBe(2) + expect(newState['blocks.0.items.0.text']?.value).toBe('C text') + expect(newState['blocks.0.items.1.text']?.value).toBe('B text') + }) + it('should set rows to empty array for empty array fields', async () => { const req = await createLocalReq({ user }, payload) From 7c2e7924b999133ebb772919f57f21d27b08817e Mon Sep 17 00:00:00 2001 From: Eduardo Costa Date: Wed, 11 Mar 2026 15:16:19 +0000 Subject: [PATCH 3/5] chore: fix nested array check to use deepest array level --- packages/ui/src/forms/Form/mergeServerFormState.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index f940254c956..ed6c7f598d1 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -96,7 +96,6 @@ export const mergeServerFormState = ({ } } } - break } } } From f43dc84e4f5d2ec6b7acecb9fb791157ae59286e Mon Sep 17 00:00:00 2001 From: Patrik Kozak <35232443+PatrikKozak@users.noreply.github.com> Date: Wed, 11 Mar 2026 13:06:57 -0400 Subject: [PATCH 4/5] fix(ui): validates row IDs at each index before accepting server values to prevent stale data from overwriting the wrong row during autosave merges --- .../ui/src/forms/Form/mergeServerFormState.ts | 126 +++++++++--------- 1 file changed, 66 insertions(+), 60 deletions(-) diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index ed6c7f598d1..a5b8354f1b9 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -24,6 +24,49 @@ type Args = { incomingState: FormState } +/** + * Parses an array field path to extract the array path, row index, and field path. + * Handles nested arrays by finding the deepest numeric index. + * + * @param path - The field path to parse (e.g., 'array.1.text' or 'blocks.0.items.1.title') + * @returns Object with arrayPath, rowIndex, and fieldPath, or null if not an array field + * + * @example + * parseArrayFieldPath('array.1.text') + * // Returns: { arrayPath: 'array', rowIndex: 1, fieldPath: 'text' } + * + * @example + * parseArrayFieldPath('blocks.0.items.1.title') + * // Returns: { arrayPath: 'blocks.0.items', rowIndex: 1, fieldPath: 'title' } + */ +function parseArrayFieldPath(path: string): { + arrayPath: string + fieldPath: string + rowIndex: number +} | null { + const segments = path.split('.') + + // Find the last numeric index (indicates array row) + let lastNumericIndex = -1 + for (let i = segments.length - 1; i >= 0; i--) { + if (/^\d+$/.test(segments[i])) { + lastNumericIndex = i + break + } + } + + // Not an array field if no numeric index or nothing after it + if (lastNumericIndex === -1 || lastNumericIndex === segments.length - 1) { + return null + } + + return { + arrayPath: segments.slice(0, lastNumericIndex).join('.'), + fieldPath: segments.slice(lastNumericIndex + 1).join('.'), + rowIndex: parseInt(segments[lastNumericIndex], 10), + } +} + /** * This function receives form state from the server and intelligently merges it into the client state. * The server contains extra properties that the client may not have, e.g. custom components and error states. @@ -43,80 +86,45 @@ export const mergeServerFormState = ({ }: Args): FormState => { const newState = { ...currentState } - /** - * Pre-compute row ID maps for all array/block fields with rows. - * This allows us to detect when a row has been moved or deleted on the client - * while a request was in-flight, so that flattened row fields (e.g. `array.1.text`) - * are not overwritten with stale server data from a different row at the same index. - */ - const serverRowIdAtIndex = new Map>() // arrayPath -> (serverIndex -> rowId) - const clientRowIdAtIndex = new Map>() // arrayPath -> (clientIndex -> rowId) - - for (const [path, incomingField] of Object.entries(incomingState || {})) { - if (Array.isArray(incomingField.rows) && path in currentState) { - const serverMap = new Map() - incomingField.rows.forEach((row, i) => serverMap.set(i, row.id)) - serverRowIdAtIndex.set(path, serverMap) - - const clientMap = new Map() - ;(currentState[path]?.rows || []).forEach((row, i) => clientMap.set(i, row.id)) - clientRowIdAtIndex.set(path, clientMap) - } - } - for (const [path, incomingField] of Object.entries(incomingState || {})) { if (!(path in currentState) && !incomingField.addedByServer) { continue } - /** - * Check if this is a flattened row field (e.g. `array.1.text`) whose row index on the server - * points to a different row than the same index on the client. This can happen when the user - * reorders or deletes rows while an autosave request is in-flight: the server responds with - * stale index-based data that no longer corresponds to the client's row order. - * In that case, do not accept the server's value to avoid overwriting the correct local data. - */ - let rowIndexMismatch = false - if ( - typeof acceptValues === 'object' && - acceptValues !== null && - acceptValues.overrideLocalChanges === false - ) { - for (const [arrayPath, serverMap] of serverRowIdAtIndex) { - if (path.startsWith(`${arrayPath}.`)) { - const remainder = path.slice(arrayPath.length + 1) - const dotIdx = remainder.indexOf('.') - if (dotIdx > -1) { - const idx = parseInt(remainder.slice(0, dotIdx), 10) - if (!isNaN(idx)) { - const serverRowId = serverMap.get(idx) - const clientRowId = clientRowIdAtIndex.get(arrayPath)?.get(idx) - if (serverRowId !== clientRowId) { - rowIndexMismatch = true - } - } - } - } - } - } - /** * If it's a new field added by the server, always accept the value. * Otherwise: * a. accept all values when explicitly requested, e.g. on submit * b. only accept values for unmodified fields, e.g. on autosave - * — but not when the row at this index has changed (reorder/delete race condition) */ - const shouldAcceptValue = + let shouldAcceptValue = incomingField.addedByServer || acceptValues === true || (typeof acceptValues === 'object' && acceptValues !== null && // Note: Must be explicitly `false`, allow `null` or `undefined` to mean true acceptValues.overrideLocalChanges === false && - !rowIndexMismatch && !currentState[path]?.isModified) + /** + * For array row fields, verify the row IDs match at the given index before accepting + * server values. If rows were reordered or deleted while the request was in-flight, + * the same index may refer to different rows, and accepting the server value would + * overwrite the wrong row's data. + */ + if (shouldAcceptValue && !incomingField.addedByServer) { + const parsed = parseArrayFieldPath(path) + if (parsed) { + const { arrayPath, rowIndex } = parsed + const clientRowId = currentState[arrayPath]?.rows?.[rowIndex]?.id + const serverRowId = incomingState[arrayPath]?.rows?.[rowIndex]?.id + + if (clientRowId === undefined || (serverRowId && clientRowId !== serverRowId)) { + shouldAcceptValue = false + } + } + } + let sanitizedIncomingField = incomingField if (!shouldAcceptValue) { @@ -177,12 +185,10 @@ export const mergeServerFormState = ({ }) /** - * Ensure the `value` (row count) always reflects the actual number of rows after the merger. - * When the server sends a `value` that differs from the corrected rows count (e.g. because - * rows were deleted on the client while the request was in-flight), override it with the - * actual count to keep `value` in sync with `rows.length`. + * Sync the value field to match the actual row count after merging. + * Client is source of truth for row count when rows were added/removed during the request. */ - if ('value' in incomingField) { + if ('value' in incomingField && newState[path].rows.length !== incomingField.value) { newState[path].value = newState[path].rows.length } } From e8b81fd82ba6872d150570f577672af95763e4e1 Mon Sep 17 00:00:00 2001 From: Patrik Kozak <35232443+PatrikKozak@users.noreply.github.com> Date: Wed, 11 Mar 2026 18:43:36 -0400 Subject: [PATCH 5/5] fix(ui): apply guard only during autosave --- .../ui/src/forms/Form/mergeServerFormState.ts | 79 ++++--- test/form-state/int.spec.ts | 204 ++++++++++++++++++ 2 files changed, 251 insertions(+), 32 deletions(-) diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index a5b8354f1b9..a17e9b591ad 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -111,8 +111,11 @@ export const mergeServerFormState = ({ * server values. If rows were reordered or deleted while the request was in-flight, * the same index may refer to different rows, and accepting the server value would * overwrite the wrong row's data. + * + * This guard only applies during autosave. On explicit save (acceptValues === true), + * the server response is authoritative and this check is bypassed. */ - if (shouldAcceptValue && !incomingField.addedByServer) { + if (shouldAcceptValue && !incomingField.addedByServer && acceptValues !== true) { const parsed = parseArrayFieldPath(path) if (parsed) { const { arrayPath, rowIndex } = parsed @@ -155,41 +158,53 @@ export const mergeServerFormState = ({ * For example, the server response could come back with a row which has been deleted on the client * Loop over the incoming rows, if it exists in client side form state, merge in any new properties from the server * Note: read `currentState` and not `newState` here, as the `rows` property have already been merged above + * + * On explicit save (acceptValues === true), the server is authoritative - accept server row order. + * On autosave (acceptValues !== true), the client is source of truth - use ID-based matching. */ if (Array.isArray(incomingField.rows) && path in currentState) { - newState[path].rows = [...(currentState[path]?.rows || [])] // shallow copy to avoid mutating the original array - - incomingField.rows.forEach((row) => { - const indexInCurrentState = currentState[path].rows?.findIndex( - (existingRow) => existingRow.id === row.id, - ) - - if (indexInCurrentState > -1) { - newState[path].rows[indexInCurrentState] = { - ...currentState[path].rows[indexInCurrentState], - ...row, + if (acceptValues === true) { + // Explicit save: server is authoritative, use index-based merging + newState[path].rows = incomingField.rows.map((serverRow, index) => ({ + ...(currentState[path]?.rows?.[index] || {}), + ...serverRow, + })) + } else { + // Autosave: client is source of truth, use ID-based matching + newState[path].rows = [...(currentState[path]?.rows || [])] // shallow copy to avoid mutating the original array + + incomingField.rows.forEach((row) => { + const indexInCurrentState = currentState[path].rows?.findIndex( + (existingRow) => existingRow.id === row.id, + ) + + if (indexInCurrentState > -1) { + newState[path].rows[indexInCurrentState] = { + ...currentState[path].rows[indexInCurrentState], + ...row, + } + } else if (row.addedByServer) { + /** + * Note: This is a known limitation of computed array and block rows + * If a new row was added by the server, we append it to the _end_ of this array + * This is because the client is the source of truth, and it has arrays ordered in a certain position + * For example, the user may have re-ordered rows client-side while a long running request is processing + * This means that we _cannot_ slice a new row into the second position on the server, for example + * By the time it gets back to the client, its index is stale + */ + const newRow = { ...row } + delete newRow.addedByServer + newState[path].rows.push(newRow) } - } else if (row.addedByServer) { - /** - * Note: This is a known limitation of computed array and block rows - * If a new row was added by the server, we append it to the _end_ of this array - * This is because the client is the source of truth, and it has arrays ordered in a certain position - * For example, the user may have re-ordered rows client-side while a long running request is processing - * This means that we _cannot_ slice a new row into the second position on the server, for example - * By the time it gets back to the client, its index is stale - */ - const newRow = { ...row } - delete newRow.addedByServer - newState[path].rows.push(newRow) + }) + + /** + * Sync the value field to match the actual row count after merging. + * Client is source of truth for row count when rows were added/removed during the request. + */ + if ('value' in incomingField && newState[path].rows.length !== incomingField.value) { + newState[path].value = newState[path].rows.length } - }) - - /** - * Sync the value field to match the actual row count after merging. - * Client is source of truth for row count when rows were added/removed during the request. - */ - if ('value' in incomingField && newState[path].rows.length !== incomingField.value) { - newState[path].value = newState[path].rows.length } } diff --git a/test/form-state/int.spec.ts b/test/form-state/int.spec.ts index 0fa6eedddbf..5c76061fdd1 100644 --- a/test/form-state/int.spec.ts +++ b/test/form-state/int.spec.ts @@ -904,6 +904,210 @@ describe('Form State', () => { expect(newState['blocks.0.items.1.text']?.value).toBe('B text') }) + it('should accept server values on explicit save without row ID guard', () => { + /** + * On explicit save (acceptValues: true), server values should be accepted + * without the row ID guard interfering. This test ensures the guard is + * scoped to autosave only. + * + * Edge case: Client has [B, A], server responds with [A, B] (perhaps due to + * server-side re-sorting). Without acceptValues !== true check, the guard + * would see mismatched IDs and reject server values. With the check, server + * values are accepted because explicit save responses are authoritative. + */ + const currentState: FormState = { + array: { + value: 2, + rows: [{ id: 'B' }, { id: 'A' }], + }, + 'array.0.text': { value: 'B text modified locally', initialValue: 'B text' }, + 'array.1.text': { value: 'A text modified locally', initialValue: 'A text' }, + } + + const serverState: FormState = { + array: { + value: 2, + rows: [{ id: 'A' }, { id: 'B' }], + }, + 'array.0.text': { value: 'A text from server', initialValue: 'A text' }, + 'array.1.text': { value: 'B text from server', initialValue: 'B text' }, + } + + const newState = mergeServerFormState({ + acceptValues: true, + currentState, + incomingState: serverState, + }) + + expect(newState.array?.rows).toHaveLength(2) + expect(newState.array?.rows?.[0]).toMatchObject({ id: 'A' }) + expect(newState.array?.rows?.[1]).toMatchObject({ id: 'B' }) + // Server values should be accepted even though row IDs don't match at same indexes + expect(newState['array.0.text']?.value).toBe('A text from server') + expect(newState['array.1.text']?.value).toBe('B text from server') + }) + + it('should preserve client-added row during autosave', () => { + /** + * Client adds row D during autosave. Server responds with stale [A, B, C]. + * Row D should be preserved with its client value. + */ + const currentState: FormState = { + array: { + value: 4, + rows: [{ id: 'A' }, { id: 'B' }, { id: 'C' }, { id: 'D' }], + }, + 'array.0.text': { value: 'A text', initialValue: 'A text' }, + 'array.1.text': { value: 'B text', initialValue: 'B text' }, + 'array.2.text': { value: 'C text', initialValue: 'C text' }, + 'array.3.text': { value: 'D text', initialValue: 'D text' }, + } + + const serverState: FormState = { + array: { + value: 3, + rows: [{ id: 'A' }, { id: 'B' }, { id: 'C' }], + }, + 'array.0.text': { value: 'A text', initialValue: 'A text' }, + 'array.1.text': { value: 'B text', initialValue: 'B text' }, + 'array.2.text': { value: 'C text', initialValue: 'C text' }, + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: serverState, + }) + + expect(newState.array?.rows).toHaveLength(4) + expect(newState.array?.rows?.[3]).toMatchObject({ id: 'D' }) + expect(newState.array?.value).toBe(4) + expect(newState['array.3.text']?.value).toBe('D text') + }) + + it('should append server-added row with addedByServer flag', () => { + /** + * Server adds a new row via hook with addedByServer: true. + * Row should be appended and not blocked by row ID guard. + */ + const currentState: FormState = { + array: { + value: 2, + rows: [{ id: 'A' }, { id: 'B' }], + }, + 'array.0.text': { value: 'A text', initialValue: 'A text' }, + 'array.1.text': { value: 'B text', initialValue: 'B text' }, + } + + const serverState: FormState = { + array: { + value: 3, + rows: [{ id: 'A' }, { id: 'B' }, { id: 'C', addedByServer: true }], + }, + 'array.0.text': { value: 'A text', initialValue: 'A text' }, + 'array.1.text': { value: 'B text', initialValue: 'B text' }, + 'array.2.text': { value: 'C text', initialValue: 'C text', addedByServer: true }, + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: serverState, + }) + + expect(newState.array?.rows).toHaveLength(3) + expect(newState.array?.rows?.[2]).toMatchObject({ id: 'C' }) + expect(newState.array?.rows?.[2]).not.toHaveProperty('addedByServer') + expect(newState.array?.value).toBe(3) + expect(newState['array.2.text']?.value).toBe('C text') + expect(newState['array.2.text']).not.toHaveProperty('addedByServer') + }) + + it('should preserve empty client array when server has rows', () => { + /** + * Client deleted all rows during autosave. Server responds with [A, B]. + * Client should stay empty. + */ + const currentState: FormState = { + array: { + value: 0, + rows: [], + }, + } + + const serverState: FormState = { + array: { + value: 2, + rows: [{ id: 'A' }, { id: 'B' }], + }, + 'array.0.text': { value: 'A text', initialValue: 'A text' }, + 'array.1.text': { value: 'B text', initialValue: 'B text' }, + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: serverState, + }) + + expect(newState.array?.rows).toHaveLength(0) + expect(newState.array?.value).toBe(0) + expect(newState['array.0.text']).toBeUndefined() + expect(newState['array.1.text']).toBeUndefined() + }) + + it('should handle 3-level nested array reordering', () => { + /** + * Verify parseArrayFieldPath works at depth 3+. + * blocks.0.items.1.subItems reordered from [X, Y] → [Y, X]. + */ + const currentState: FormState = { + blocks: { + value: 1, + rows: [{ id: 'block-1' }], + }, + 'blocks.0.items': { + value: 2, + rows: [{ id: 'item-1' }, { id: 'item-2' }], + }, + 'blocks.0.items.1.subItems': { + value: 2, + rows: [{ id: 'Y' }, { id: 'X' }], + }, + 'blocks.0.items.1.subItems.0.text': { value: 'Y text', initialValue: 'Y text' }, + 'blocks.0.items.1.subItems.1.text': { value: 'X text', initialValue: 'X text' }, + } + + const serverState: FormState = { + blocks: { + value: 1, + rows: [{ id: 'block-1' }], + }, + 'blocks.0.items': { + value: 2, + rows: [{ id: 'item-1' }, { id: 'item-2' }], + }, + 'blocks.0.items.1.subItems': { + value: 2, + rows: [{ id: 'X' }, { id: 'Y' }], + }, + 'blocks.0.items.1.subItems.0.text': { value: 'X text', initialValue: 'X text' }, + 'blocks.0.items.1.subItems.1.text': { value: 'Y text', initialValue: 'Y text' }, + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: serverState, + }) + + expect(newState['blocks.0.items.1.subItems']?.rows).toHaveLength(2) + expect(newState['blocks.0.items.1.subItems']?.rows?.[0]).toMatchObject({ id: 'Y' }) + expect(newState['blocks.0.items.1.subItems']?.rows?.[1]).toMatchObject({ id: 'X' }) + expect(newState['blocks.0.items.1.subItems.0.text']?.value).toBe('Y text') + expect(newState['blocks.0.items.1.subItems.1.text']?.value).toBe('X text') + }) + it('should set rows to empty array for empty array fields', async () => { const req = await createLocalReq({ user }, payload)