Skip to content

Commit 8173180

Browse files
authored
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
1 parent 3258e78 commit 8173180

File tree

10 files changed

+214
-25
lines changed

10 files changed

+214
-25
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,9 @@ export const Autosave: React.FC<Props> = ({ id, collection, global: globalDoc })
150150

151151
if (!skipSubmission && modifiedRef.current && url) {
152152
const result = await submit({
153+
acceptValues: {
154+
overrideLocalChanges: false,
155+
},
153156
action: url,
154157
context: {
155158
incrementVersionCount: false,

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,11 +189,12 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState {
189189
}
190190

191191
case 'MERGE_SERVER_STATE': {
192-
const { acceptValues, prevStateRef, serverState } = action
192+
const { acceptValues, formStateAtTimeOfRequest, prevStateRef, serverState } = action
193193

194194
const newState = mergeServerFormState({
195195
acceptValues,
196196
currentState: state || {},
197+
formStateAtTimeOfRequest,
197198
incomingState: serverState,
198199
})
199200

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

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ export const Form: React.FC<FormProps> = (props) => {
203203
const submit = useCallback<Submit>(
204204
async (options, e) => {
205205
const {
206+
acceptValues = true,
206207
action: actionArg = action,
207208
context,
208209
disableFormWhileProcessing = true,
@@ -263,19 +264,23 @@ export const Form: React.FC<FormProps> = (props) => {
263264
await wait(100)
264265
}
265266

267+
/**
268+
* Take copies of the current form state and data here. This will ensure it is consistent.
269+
* For example, it is possible for the form state ref to change in the background while this submit function is running.
270+
* TODO: can we send the `formStateCopy` through `reduceFieldsToValues` to even greater consistency? Doing this currently breaks uploads.
271+
*/
272+
const formStateCopy = deepCopyObjectSimpleWithoutReactComponents(contextRef.current.fields)
273+
const data = reduceFieldsToValues(contextRef.current.fields, true)
274+
266275
// Execute server side validations
267276
if (Array.isArray(beforeSubmit)) {
268277
let revalidatedFormState: FormState
269278

270-
const serializableFields = deepCopyObjectSimpleWithoutReactComponents(
271-
contextRef.current.fields,
272-
)
273-
274279
await beforeSubmit.reduce(async (priorOnChange, beforeSubmitFn) => {
275280
await priorOnChange
276281

277282
const result = await beforeSubmitFn({
278-
formState: serializableFields,
283+
formState: formStateCopy,
279284
})
280285

281286
revalidatedFormState = result
@@ -319,17 +324,11 @@ export const Form: React.FC<FormProps> = (props) => {
319324

320325
// If submit handler comes through via props, run that
321326
if (onSubmit) {
322-
const serializableFields = deepCopyObjectSimpleWithoutReactComponents(
323-
contextRef.current.fields,
324-
)
325-
326-
const data = reduceFieldsToValues(serializableFields, true)
327-
328327
for (const [key, value] of Object.entries(overrides)) {
329328
data[key] = value
330329
}
331330

332-
onSubmit(serializableFields, data)
331+
onSubmit(formStateCopy, data)
333332
}
334333

335334
if (!hasFormSubmitAction) {
@@ -342,6 +341,7 @@ export const Form: React.FC<FormProps> = (props) => {
342341
}
343342

344343
const formData = await contextRef.current.createFormData(overrides, {
344+
data,
345345
mergeOverrideData: Boolean(typeof overridesFromArgs !== 'function'),
346346
})
347347

@@ -384,7 +384,8 @@ export const Form: React.FC<FormProps> = (props) => {
384384
if (newFormState) {
385385
dispatchFields({
386386
type: 'MERGE_SERVER_STATE',
387-
acceptValues: true,
387+
acceptValues,
388+
formStateAtTimeOfRequest: formStateCopy,
388389
prevStateRef: prevFormState,
389390
serverState: newFormState,
390391
})
@@ -503,8 +504,8 @@ export const Form: React.FC<FormProps> = (props) => {
503504
)
504505

505506
const createFormData = useCallback<CreateFormData>(
506-
async (overrides, { mergeOverrideData = true }) => {
507-
let data = reduceFieldsToValues(contextRef.current.fields, true)
507+
async (overrides, { data: dataFromArgs, mergeOverrideData = true }) => {
508+
let data = dataFromArgs || reduceFieldsToValues(contextRef.current.fields, true)
508509

509510
let file = data?.file
510511

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

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,25 @@ import type { FormState } from 'payload'
33

44
import { dequal } from 'dequal/lite' // lite: no need for Map and Set support
55

6+
/**
7+
* If true, will accept all values from the server, overriding any current values in local state.
8+
* Can also provide an options object for more granular control.
9+
*/
10+
export type AcceptValues =
11+
| {
12+
/**
13+
* When `false`, will accept the values from the server _UNLESS_ the value has been modified locally since the request was made.
14+
* 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.
15+
* @default undefined
16+
*/
17+
overrideLocalChanges?: boolean
18+
}
19+
| boolean
20+
621
type Args = {
7-
acceptValues?: boolean
22+
acceptValues?: AcceptValues
823
currentState?: FormState
24+
formStateAtTimeOfRequest?: FormState
925
incomingState: FormState
1026
}
1127

@@ -21,6 +37,7 @@ type Args = {
2137
export const mergeServerFormState = ({
2238
acceptValues,
2339
currentState = {},
40+
formStateAtTimeOfRequest,
2441
incomingState,
2542
}: Args): FormState => {
2643
const newState = { ...currentState }
@@ -30,7 +47,20 @@ export const mergeServerFormState = ({
3047
continue
3148
}
3249

33-
if (!acceptValues && !incomingField.addedByServer) {
50+
/**
51+
* If it's a new field added by the server, always accept the value.
52+
* Otherwise, only accept the values if explicitly requested, e.g. on submit.
53+
* Can also control this granularly by only accepting unmodified values, e.g. for autosave.
54+
*/
55+
if (
56+
!incomingField.addedByServer &&
57+
(!acceptValues ||
58+
// See `acceptValues` type definition for more details
59+
(typeof acceptValues === 'object' &&
60+
acceptValues !== null &&
61+
acceptValues?.overrideLocalChanges === false &&
62+
currentState[path]?.value !== formStateAtTimeOfRequest?.[path]?.value))
63+
) {
3464
delete incomingField.value
3565
delete incomingField.initialValue
3666
}

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import type {
1010
import type React from 'react'
1111
import type { Dispatch } from 'react'
1212

13+
import type { AcceptValues } from './mergeServerFormState.js'
14+
1315
export type Preferences = {
1416
[key: string]: unknown
1517
}
@@ -69,6 +71,7 @@ export type FormProps = {
6971
)
7072

7173
export type SubmitOptions = {
74+
acceptValues?: AcceptValues
7275
action?: string
7376
/**
7477
* @experimental - Note: this property is experimental and may change in the future. Use as your own discretion.
@@ -113,7 +116,13 @@ export type CreateFormData = (
113116
* If mergeOverrideData true, the data will be merged with the existing data in the form state.
114117
* @default true
115118
*/
116-
options?: { mergeOverrideData?: boolean },
119+
options?: {
120+
/**
121+
* If provided, will use this instead of of derived data from the current form state.
122+
*/
123+
data?: Data
124+
mergeOverrideData?: boolean
125+
},
117126
) => FormData | Promise<FormData>
118127

119128
export type GetFields = () => FormState
@@ -175,7 +184,8 @@ export type ADD_ROW = {
175184
}
176185

177186
export type MERGE_SERVER_STATE = {
178-
acceptValues?: boolean
187+
acceptValues?: AcceptValues
188+
formStateAtTimeOfRequest?: FormState
179189
prevStateRef: React.RefObject<FormState>
180190
serverState: FormState
181191
type: 'MERGE_SERVER_STATE'

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
11
import type {
2-
ArrayField,
3-
BlocksField,
42
BuildFormStateArgs,
53
ClientFieldSchemaMap,
6-
CollapsedPreferences,
74
Data,
85
DocumentPreferences,
96
Field,
@@ -153,6 +150,7 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom
153150
} = args
154151

155152
if (!args.clientFieldSchemaMap && args.renderFieldFn) {
153+
// eslint-disable-next-line no-console
156154
console.warn(
157155
'clientFieldSchemaMap is not passed to addFieldStatePromise - this will reduce performance',
158156
)

test/form-state/int.spec.ts

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -565,4 +565,109 @@ describe('Form State', () => {
565565

566566
expect(newState === currentState).toBe(true)
567567
})
568+
569+
it('should accept all values from the server regardless of local modifications, e.g. on submit', () => {
570+
const currentState = {
571+
title: {
572+
value: 'Test Post (modified on the client)',
573+
initialValue: 'Test Post',
574+
valid: true,
575+
passesCondition: true,
576+
},
577+
computedTitle: {
578+
value: 'Test Post (computed on the client)',
579+
initialValue: 'Test Post',
580+
valid: true,
581+
passesCondition: true,
582+
},
583+
}
584+
585+
const formStateAtTimeOfRequest = {
586+
...currentState,
587+
title: {
588+
value: 'Test Post (modified on the client 2)',
589+
initialValue: 'Test Post',
590+
valid: true,
591+
passesCondition: true,
592+
},
593+
}
594+
595+
const incomingStateFromServer = {
596+
title: {
597+
value: 'Test Post (modified on the server)',
598+
initialValue: 'Test Post',
599+
valid: true,
600+
passesCondition: true,
601+
},
602+
computedTitle: {
603+
value: 'Test Post (computed on the server)',
604+
initialValue: 'Test Post',
605+
valid: true,
606+
passesCondition: true,
607+
},
608+
}
609+
610+
const newState = mergeServerFormState({
611+
acceptValues: true,
612+
currentState,
613+
formStateAtTimeOfRequest,
614+
incomingState: incomingStateFromServer,
615+
})
616+
617+
expect(newState).toStrictEqual(incomingStateFromServer)
618+
})
619+
620+
it('should not accept values from the server if they have been modified locally since the request was made, e.g. on autosave', () => {
621+
const currentState = {
622+
title: {
623+
value: 'Test Post (modified on the client 1)',
624+
initialValue: 'Test Post',
625+
valid: true,
626+
passesCondition: true,
627+
},
628+
computedTitle: {
629+
value: 'Test Post',
630+
initialValue: 'Test Post',
631+
valid: true,
632+
passesCondition: true,
633+
},
634+
}
635+
636+
const formStateAtTimeOfRequest = {
637+
...currentState,
638+
title: {
639+
value: 'Test Post (modified on the client 2)',
640+
initialValue: 'Test Post',
641+
valid: true,
642+
passesCondition: true,
643+
},
644+
}
645+
646+
const incomingStateFromServer = {
647+
title: {
648+
value: 'Test Post (modified on the server)',
649+
initialValue: 'Test Post',
650+
valid: true,
651+
passesCondition: true,
652+
},
653+
computedTitle: {
654+
value: 'Test Post (modified on the server)',
655+
initialValue: 'Test Post',
656+
valid: true,
657+
passesCondition: true,
658+
},
659+
}
660+
661+
const newState = mergeServerFormState({
662+
acceptValues: { overrideLocalChanges: false },
663+
currentState,
664+
formStateAtTimeOfRequest,
665+
incomingState: incomingStateFromServer,
666+
})
667+
668+
expect(newState).toStrictEqual({
669+
...currentState,
670+
computedTitle: incomingStateFromServer.computedTitle, // This field was not modified locally, so should be updated from the server
671+
})
672+
})
568673
})

test/versions/collections/Autosave.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ const AutosavePosts: CollectionConfig = {
1616
maxPerDoc: 35,
1717
drafts: {
1818
autosave: {
19-
interval: 2000,
19+
interval: 100,
2020
},
2121
schedulePublish: true,
2222
},
@@ -67,6 +67,16 @@ const AutosavePosts: CollectionConfig = {
6767
type: 'textarea',
6868
required: true,
6969
},
70+
{
71+
name: 'array',
72+
type: 'array',
73+
fields: [
74+
{
75+
name: 'text',
76+
type: 'text',
77+
},
78+
],
79+
},
7080
],
7181
}
7282

test/versions/e2e.spec.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1286,12 +1286,31 @@ describe('Versions', () => {
12861286
page.removeListener('dialog', acceptAlert)
12871287
})
12881288

1289-
test('- with autosave - applies afterChange hooks to form state after autosave runs', async () => {
1289+
test('- with autosave - applies field hooks to form state after autosave runs', async () => {
12901290
const url = new AdminUrlUtil(serverURL, autosaveCollectionSlug)
12911291
await page.goto(url.create)
12921292
const titleField = page.locator('#field-title')
12931293
await titleField.fill('Initial')
1294+
1295+
await waitForAutoSaveToRunAndComplete(page)
1296+
1297+
const computedTitleField = page.locator('#field-computedTitle')
1298+
await expect(computedTitleField).toHaveValue('Initial')
1299+
})
1300+
1301+
test('- with autosave - does not override local changes to form state after autosave runs', async () => {
1302+
const url = new AdminUrlUtil(serverURL, autosaveCollectionSlug)
1303+
await page.goto(url.create)
1304+
const titleField = page.locator('#field-title')
1305+
1306+
// press slower than the autosave interval, but not faster than the response and processing
1307+
await titleField.pressSequentially('Initial', {
1308+
delay: 150,
1309+
})
1310+
12941311
await waitForAutoSaveToRunAndComplete(page)
1312+
1313+
await expect(titleField).toHaveValue('Initial')
12951314
const computedTitleField = page.locator('#field-computedTitle')
12961315
await expect(computedTitleField).toHaveValue('Initial')
12971316
})

0 commit comments

Comments
 (0)