fix: deeply merge array and block rows from server form state

packages/payload/src/admin/forms/Form.ts

@@ -11,6 +11,7 @@ export type Data = {
 }
 
 export type Row = {
+  addedByServer?: FieldState['addedByServer']
   blockType?: string
   collapsed?: boolean
   customComponents?: {

packages/ui/src/forms/Form/mergeServerFormState.ts

@@ -89,30 +89,38 @@ export const mergeServerFormState = ({
     }
 
     /**
-     * Intelligently merge the rows array to ensure changes to local state are not lost while the request was pending
+     * Deeply merge the rows array to ensure changes to local state are not lost while the request was pending
      * 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
      */
-    if (Array.isArray(incomingField.rows)) {
-      if (acceptValues === true) {
-        newState[path].rows = incomingField.rows
-      } else if (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 (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,
           }
-        })
-      }
+        } 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)
+        }
+      })
     }
 
     // If `valid` is `undefined`, mark it as `true`

packages/ui/src/forms/fieldSchemasToFormState/addFieldStatePromise.ts

@@ -352,7 +352,10 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom
               newRow.lastRenderedPath = previousRow.lastRenderedPath
             }
 
-            acc.rows.push(newRow)
+            // add addedByServer flag
+            if (!previousRow) {
+              newRow.addedByServer = true
+            }
 
             const isCollapsed = isRowCollapsed({
               collapsedPrefs: preferences?.fields?.[path]?.collapsed,
@@ -362,9 +365,11 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom
             })
 
             if (isCollapsed) {
-              acc.rows[acc.rows.length - 1].collapsed = true
+              newRow.collapsed = true
             }
 
+            acc.rows.push(newRow)
+
             return acc
           },
           {

test/form-state/collections/Posts/ArrayRowLabel.tsx

@@ -1,5 +1,5 @@
 import React from 'react'
 
 export const ArrayRowLabel = () => {
-  return <p>This is a custom component</p>
+  return <p id="custom-array-row-label">This is a custom component</p>
 }

test/form-state/int.spec.ts

@@ -1,4 +1,5 @@
 import type { FieldState, FormState, Payload, User } from 'payload'
+import type React from 'react'
 
 import { buildFormState } from '@payloadcms/ui/utilities/buildFormState'
 import path from 'path'
@@ -10,6 +11,7 @@ import type { NextRESTClient } from '../helpers/NextRESTClient.js'
 import { devUser } from '../credentials.js'
 import { initPayloadInt } from '../helpers/initPayloadInt.js'
 import { postsSlug } from './collections/Posts/index.js'
+
 // eslint-disable-next-line payload/no-relative-monorepo-imports
 import { mergeServerFormState } from '../../packages/ui/src/forms/Form/mergeServerFormState.js'
 
@@ -21,6 +23,14 @@ const { email, password } = devUser
 const filename = fileURLToPath(import.meta.url)
 const dirname = path.dirname(filename)
 
+const DummyReactComponent: React.ReactNode = {
+  // @ts-expect-error - can ignore, needs to satisfy `typeof value.$$typeof === 'symbol'`
+  $$typeof: Symbol.for('react.element'),
+  type: 'div',
+  props: {},
+  key: null,
+}
+
 describe('Form State', () => {
   beforeAll(async () => {
     ;({ payload, restClient } = await initPayloadInt(dirname, undefined, true))
@@ -566,7 +576,7 @@ describe('Form State', () => {
     expect(newState === currentState).toBe(true)
   })
 
-  it('should accept all values from the server regardless of local modifications, e.g. on submit', () => {
+  it('should accept all values from the server regardless of local modifications, e.g. `acceptAllValues` on submit', () => {
     const title: FieldState = {
       value: 'Test Post (modified on the client)',
       initialValue: 'Test Post',
@@ -577,14 +587,39 @@ describe('Form State', () => {
     const currentState: Record<string, FieldState> = {
       title: {
         ...title,
-        isModified: true,
+        isModified: true, // This is critical, this is what we're testing
       },
       computedTitle: {
         value: 'Test Post (computed on the client)',
         initialValue: 'Test Post',
         valid: true,
         passesCondition: true,
       },
+      array: {
+        rows: [
+          {
+            id: '1',
+            customComponents: {
+              RowLabel: DummyReactComponent,
+            },
+            lastRenderedPath: 'array.0.customTextField',
+          },
+        ],
+        valid: true,
+        passesCondition: true,
+      },
+      'array.0.id': {
+        value: '1',
+        initialValue: '1',
+        valid: true,
+        passesCondition: true,
+      },
+      'array.0.customTextField': {
+        value: 'Test Post (modified on the client)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
     }
 
     const incomingStateFromServer: Record<string, FieldState> = {
@@ -600,6 +635,29 @@ describe('Form State', () => {
         valid: true,
         passesCondition: true,
       },
+      array: {
+        rows: [
+          {
+            id: '1',
+            lastRenderedPath: 'array.0.customTextField',
+            // Omit `customComponents` because the server did not re-render this row
+          },
+        ],
+        passesCondition: true,
+        valid: true,
+      },
+      'array.0.id': {
+        value: '1',
+        initialValue: '1',
+        valid: true,
+        passesCondition: true,
+      },
+      'array.0.customTextField': {
+        value: 'Test Post (modified on the client)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
     }
 
     const newState = mergeServerFormState({
@@ -614,10 +672,14 @@ describe('Form State', () => {
         ...incomingStateFromServer.title,
         isModified: true,
       },
+      array: {
+        ...incomingStateFromServer.array,
+        rows: currentState?.array?.rows,
+      },
     })
   })
 
-  it('should not accept values from the server if they have been modified locally since the request was made, e.g. on autosave', () => {
+  it('should not accept values from the server if they have been modified locally since the request was made, e.g. `overrideLocalChanges: false` on autosave', () => {
     const title: FieldState = {
       value: 'Test Post (modified on the client 1)',
       initialValue: 'Test Post',