fix(ui): autosave form state discards local changes (#13438)

Follow-up to #13416. Supersedes #13434.

When autosave is triggered and the user continues to modify fields,
their changes are overridden by the server's value, i.e. the value at
the time the form state request was made. This makes it almost
impossible to edit fields when using a small autosave interval and/or a
slow network.

This is because autosave is now merged into form state, which by default
uses `acceptValues: true`. This does exactly what it sounds like,
accepts all the values from the server—which may be stale if underlying
changes have been made. We ignore these values for onChange events,
because the user is actively making changes. But during form
submissions, we can accept them because the form is disabled while
processing anyway.

This pattern allows us to render "computed values" from the server, i.e.
a field with an `beforeChange` hook that modifies its value.

Autosave, on the other hand, happens in the background _while the form
is still active_. This means changes may have been made since sending
the request. We still need to accept computed values from the server,
but we need to avoid doing this if the user has active changes since the
time of the request.

---
- To see the specific tasks where the Asana app for GitHub is being
used, see below:
  - https://app.asana.com/0/0/1211027929253429

Author: jacobsfletch

packages/ui/src/elements/Autosave/index.tsx

@@ -150,6 +150,9 @@ export const Autosave: React.FC<Props> = ({ id, collection, global: globalDoc })
 
           if (!skipSubmission && modifiedRef.current && url) {
             const result = await submit({
+              acceptValues: {
+                overrideLocalChanges: false,
+              },
               action: url,
               context: {
                 incrementVersionCount: false,

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

@@ -189,11 +189,12 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState {
     }
 
     case 'MERGE_SERVER_STATE': {
-      const { acceptValues, prevStateRef, serverState } = action
+      const { acceptValues, formStateAtTimeOfRequest, prevStateRef, serverState } = action
 
       const newState = mergeServerFormState({
         acceptValues,
         currentState: state || {},
+        formStateAtTimeOfRequest,
         incomingState: serverState,
       })
 

packages/ui/src/forms/Form/index.tsx

@@ -203,6 +203,7 @@ export const Form: React.FC<FormProps> = (props) => {
   const submit = useCallback<Submit>(
     async (options, e) => {
       const {
+        acceptValues = true,
         action: actionArg = action,
         context,
         disableFormWhileProcessing = true,
@@ -263,19 +264,23 @@ export const Form: React.FC<FormProps> = (props) => {
         await wait(100)
       }
 
+      /**
+       * Take copies of the current form state and data here. This will ensure it is consistent.
+       * For example, it is possible for the form state ref to change in the background while this submit function is running.
+       * TODO: can we send the `formStateCopy` through `reduceFieldsToValues` to even greater consistency? Doing this currently breaks uploads.
+       */
+      const formStateCopy = deepCopyObjectSimpleWithoutReactComponents(contextRef.current.fields)
+      const data = reduceFieldsToValues(contextRef.current.fields, true)
+
       // Execute server side validations
       if (Array.isArray(beforeSubmit)) {
         let revalidatedFormState: FormState
 
-        const serializableFields = deepCopyObjectSimpleWithoutReactComponents(
-          contextRef.current.fields,
-        )
-
         await beforeSubmit.reduce(async (priorOnChange, beforeSubmitFn) => {
           await priorOnChange
 
           const result = await beforeSubmitFn({
-            formState: serializableFields,
+            formState: formStateCopy,
           })
 
           revalidatedFormState = result
@@ -319,17 +324,11 @@ export const Form: React.FC<FormProps> = (props) => {
 
       // If submit handler comes through via props, run that
       if (onSubmit) {
-        const serializableFields = deepCopyObjectSimpleWithoutReactComponents(
-          contextRef.current.fields,
-        )
-
-        const data = reduceFieldsToValues(serializableFields, true)
-
         for (const [key, value] of Object.entries(overrides)) {
           data[key] = value
         }
 
-        onSubmit(serializableFields, data)
+        onSubmit(formStateCopy, data)
       }
 
       if (!hasFormSubmitAction) {
@@ -342,6 +341,7 @@ export const Form: React.FC<FormProps> = (props) => {
       }
 
       const formData = await contextRef.current.createFormData(overrides, {
+        data,
         mergeOverrideData: Boolean(typeof overridesFromArgs !== 'function'),
       })
 
@@ -384,7 +384,8 @@ export const Form: React.FC<FormProps> = (props) => {
             if (newFormState) {
               dispatchFields({
                 type: 'MERGE_SERVER_STATE',
-                acceptValues: true,
+                acceptValues,
+                formStateAtTimeOfRequest: formStateCopy,
                 prevStateRef: prevFormState,
                 serverState: newFormState,
               })
@@ -503,8 +504,8 @@ export const Form: React.FC<FormProps> = (props) => {
   )
 
   const createFormData = useCallback<CreateFormData>(
-    async (overrides, { mergeOverrideData = true }) => {
-      let data = reduceFieldsToValues(contextRef.current.fields, true)
+    async (overrides, { data: dataFromArgs, mergeOverrideData = true }) => {
+      let data = dataFromArgs || reduceFieldsToValues(contextRef.current.fields, true)
 
       let file = data?.file
 

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

@@ -3,9 +3,25 @@ import type { FormState } from 'payload'
 
 import { dequal } from 'dequal/lite' // lite: no need for Map and Set support
 
+/**
+ * If true, will accept all values from the server, overriding any current values in local state.
+ * Can also provide an options object for more granular control.
+ */
+export type AcceptValues =
+  | {
+      /**
+       * When `false`, will accept the values from the server _UNLESS_ the value has been modified locally since the request was made.
+       * This is useful for autosave, for example, where hooks may have modified the field's value on the server while you were still making changes.
+       * @default undefined
+       */
+      overrideLocalChanges?: boolean
+    }
+  | boolean
+
 type Args = {
-  acceptValues?: boolean
+  acceptValues?: AcceptValues
   currentState?: FormState
+  formStateAtTimeOfRequest?: FormState
   incomingState: FormState
 }
 
@@ -21,6 +37,7 @@ type Args = {
 export const mergeServerFormState = ({
   acceptValues,
   currentState = {},
+  formStateAtTimeOfRequest,
   incomingState,
 }: Args): FormState => {
   const newState = { ...currentState }
@@ -30,7 +47,20 @@ export const mergeServerFormState = ({
       continue
     }
 
-    if (!acceptValues && !incomingField.addedByServer) {
+    /**
+     * If it's a new field added by the server, always accept the value.
+     * Otherwise, only accept the values if explicitly requested, e.g. on submit.
+     * Can also control this granularly by only accepting unmodified values, e.g. for autosave.
+     */
+    if (
+      !incomingField.addedByServer &&
+      (!acceptValues ||
+        // See `acceptValues` type definition for more details
+        (typeof acceptValues === 'object' &&
+          acceptValues !== null &&
+          acceptValues?.overrideLocalChanges === false &&
+          currentState[path]?.value !== formStateAtTimeOfRequest?.[path]?.value))
+    ) {
       delete incomingField.value
       delete incomingField.initialValue
     }

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

@@ -10,6 +10,8 @@ import type {
 import type React from 'react'
 import type { Dispatch } from 'react'
 
+import type { AcceptValues } from './mergeServerFormState.js'
+
 export type Preferences = {
   [key: string]: unknown
 }
@@ -69,6 +71,7 @@ export type FormProps = {
 )
 
 export type SubmitOptions = {
+  acceptValues?: AcceptValues
   action?: string
   /**
    * @experimental - Note: this property is experimental and may change in the future. Use as your own discretion.
@@ -113,7 +116,13 @@ export type CreateFormData = (
    * If mergeOverrideData true, the data will be merged with the existing data in the form state.
    * @default true
    */
-  options?: { mergeOverrideData?: boolean },
+  options?: {
+    /**
+     * If provided, will use this instead of of derived data from the current form state.
+     */
+    data?: Data
+    mergeOverrideData?: boolean
+  },
 ) => FormData | Promise<FormData>
 
 export type GetFields = () => FormState
@@ -175,7 +184,8 @@ export type ADD_ROW = {
 }
 
 export type MERGE_SERVER_STATE = {
-  acceptValues?: boolean
+  acceptValues?: AcceptValues
+  formStateAtTimeOfRequest?: FormState
   prevStateRef: React.RefObject<FormState>
   serverState: FormState
   type: 'MERGE_SERVER_STATE'

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

@@ -1,9 +1,6 @@
 import type {
-  ArrayField,
-  BlocksField,
   BuildFormStateArgs,
   ClientFieldSchemaMap,
-  CollapsedPreferences,
   Data,
   DocumentPreferences,
   Field,
@@ -153,6 +150,7 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom
   } = args
 
   if (!args.clientFieldSchemaMap && args.renderFieldFn) {
+    // eslint-disable-next-line no-console
     console.warn(
       'clientFieldSchemaMap is not passed to addFieldStatePromise - this will reduce performance',
     )

test/form-state/int.spec.ts

@@ -565,4 +565,109 @@ describe('Form State', () => {
 
     expect(newState === currentState).toBe(true)
   })
+
+  it('should accept all values from the server regardless of local modifications, e.g. on submit', () => {
+    const currentState = {
+      title: {
+        value: 'Test Post (modified on the client)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+      computedTitle: {
+        value: 'Test Post (computed on the client)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+    }
+
+    const formStateAtTimeOfRequest = {
+      ...currentState,
+      title: {
+        value: 'Test Post (modified on the client 2)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+    }
+
+    const incomingStateFromServer = {
+      title: {
+        value: 'Test Post (modified on the server)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+      computedTitle: {
+        value: 'Test Post (computed on the server)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+    }
+
+    const newState = mergeServerFormState({
+      acceptValues: true,
+      currentState,
+      formStateAtTimeOfRequest,
+      incomingState: incomingStateFromServer,
+    })
+
+    expect(newState).toStrictEqual(incomingStateFromServer)
+  })
+
+  it('should not accept values from the server if they have been modified locally since the request was made, e.g. on autosave', () => {
+    const currentState = {
+      title: {
+        value: 'Test Post (modified on the client 1)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+      computedTitle: {
+        value: 'Test Post',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+    }
+
+    const formStateAtTimeOfRequest = {
+      ...currentState,
+      title: {
+        value: 'Test Post (modified on the client 2)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+    }
+
+    const incomingStateFromServer = {
+      title: {
+        value: 'Test Post (modified on the server)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+      computedTitle: {
+        value: 'Test Post (modified on the server)',
+        initialValue: 'Test Post',
+        valid: true,
+        passesCondition: true,
+      },
+    }
+
+    const newState = mergeServerFormState({
+      acceptValues: { overrideLocalChanges: false },
+      currentState,
+      formStateAtTimeOfRequest,
+      incomingState: incomingStateFromServer,
+    })
+
+    expect(newState).toStrictEqual({
+      ...currentState,
+      computedTitle: incomingStateFromServer.computedTitle, // This field was not modified locally, so should be updated from the server
+    })
+  })
 })

test/versions/collections/Autosave.ts

@@ -16,7 +16,7 @@ const AutosavePosts: CollectionConfig = {
     maxPerDoc: 35,
     drafts: {
       autosave: {
-        interval: 2000,
+        interval: 100,
       },
       schedulePublish: true,
     },
@@ -67,6 +67,16 @@ const AutosavePosts: CollectionConfig = {
       type: 'textarea',
       required: true,
     },
+    {
+      name: 'array',
+      type: 'array',
+      fields: [
+        {
+          name: 'text',
+          type: 'text',
+        },
+      ],
+    },
   ],
 }
 

test/versions/e2e.spec.ts

@@ -1286,12 +1286,31 @@ describe('Versions', () => {
       page.removeListener('dialog', acceptAlert)
     })
 
-    test('- with autosave - applies afterChange hooks to form state after autosave runs', async () => {
+    test('- with autosave - applies field hooks to form state after autosave runs', async () => {
       const url = new AdminUrlUtil(serverURL, autosaveCollectionSlug)
       await page.goto(url.create)
       const titleField = page.locator('#field-title')
       await titleField.fill('Initial')
+
+      await waitForAutoSaveToRunAndComplete(page)
+
+      const computedTitleField = page.locator('#field-computedTitle')
+      await expect(computedTitleField).toHaveValue('Initial')
+    })
+
+    test('- with autosave - does not override local changes to form state after autosave runs', async () => {
+      const url = new AdminUrlUtil(serverURL, autosaveCollectionSlug)
+      await page.goto(url.create)
+      const titleField = page.locator('#field-title')
+
+      // press slower than the autosave interval, but not faster than the response and processing
+      await titleField.pressSequentially('Initial', {
+        delay: 150,
+      })
+
       await waitForAutoSaveToRunAndComplete(page)
+
+      await expect(titleField).toHaveValue('Initial')
       const computedTitleField = page.locator('#field-computedTitle')
       await expect(computedTitleField).toHaveValue('Initial')
     })