diff --git a/packages/payload/src/admin/forms/Form.ts b/packages/payload/src/admin/forms/Form.ts index 94242f0932..419087a6ec 100644 --- a/packages/payload/src/admin/forms/Form.ts +++ b/packages/payload/src/admin/forms/Form.ts @@ -49,6 +49,18 @@ export type FieldState = { passesCondition?: boolean requiresRender?: boolean rows?: Row[] + /** + * The `serverPropsToIgnore` obj is used to prevent the various properties from being overridden across form state requests. + * This can happen when queueing a form state request with `requiresRender: true` while the another is already processing. + * For example: + * 1. One "add row" action will set `requiresRender: true` and dispatch a form state request + * 2. Another "add row" action will set `requiresRender: true` and queue a form state request + * 3. The first request will return with `requiresRender: false` + * 4. The second request will be dispatched with `requiresRender: false` but should be `true` + * To fix this, only merge the `requiresRender` property if the previous state has not set it to `true`. + * See the `mergeServerFormState` function for implementation details. + */ + serverPropsToIgnore?: Array valid?: boolean validate?: Validate value?: unknown diff --git a/packages/ui/src/fields/Array/index.tsx b/packages/ui/src/fields/Array/index.tsx index 3a6ee75733..a230a93c82 100644 --- a/packages/ui/src/fields/Array/index.tsx +++ b/packages/ui/src/fields/Array/index.tsx @@ -58,7 +58,7 @@ export const ArrayFieldComponent: ArrayFieldClientComponent = (props) => { const minRows = (minRowsProp ?? required) ? 1 : 0 const { setDocFieldPreferences } = useDocumentInfo() - const { addFieldRow, dispatchFields, setModified } = useForm() + const { addFieldRow, dispatchFields, moveFieldRow, removeFieldRow, setModified } = useForm() const submitted = useFormSubmitted() const { code: locale } = useLocale() const { i18n, t } = useTranslation() @@ -153,18 +153,20 @@ export const ArrayFieldComponent: ArrayFieldClientComponent = (props) => { const removeRow = useCallback( (rowIndex: number) => { - dispatchFields({ type: 'REMOVE_ROW', path, rowIndex }) - setModified(true) + removeFieldRow({ path, rowIndex }) }, - [dispatchFields, path, setModified], + [removeFieldRow, path], ) const moveRow = useCallback( (moveFromIndex: number, moveToIndex: number) => { - dispatchFields({ type: 'MOVE_ROW', moveFromIndex, moveToIndex, path }) - setModified(true) + moveFieldRow({ + moveFromIndex, + moveToIndex, + path, + }) }, - [dispatchFields, path, setModified], + [path, moveFieldRow], ) const toggleCollapseAll = useCallback( diff --git a/packages/ui/src/fields/Blocks/index.tsx b/packages/ui/src/fields/Blocks/index.tsx index 8b68ebf6db..10d562742d 100644 --- a/packages/ui/src/fields/Blocks/index.tsx +++ b/packages/ui/src/fields/Blocks/index.tsx @@ -60,7 +60,7 @@ const BlocksFieldComponent: BlocksFieldClientComponent = (props) => { const minRows = (minRowsProp ?? required) ? 1 : 0 const { setDocFieldPreferences } = useDocumentInfo() - const { addFieldRow, dispatchFields, setModified } = useForm() + const { addFieldRow, dispatchFields, moveFieldRow, removeFieldRow, setModified } = useForm() const { code: locale } = useLocale() const { config: { localization }, @@ -141,23 +141,19 @@ const BlocksFieldComponent: BlocksFieldClientComponent = (props) => { const removeRow = useCallback( (rowIndex: number) => { - dispatchFields({ - type: 'REMOVE_ROW', + removeFieldRow({ path, rowIndex, }) - - setModified(true) }, - [path, dispatchFields, setModified], + [path, removeFieldRow], ) const moveRow = useCallback( (moveFromIndex: number, moveToIndex: number) => { - dispatchFields({ type: 'MOVE_ROW', moveFromIndex, moveToIndex, path }) - setModified(true) + moveFieldRow({ moveFromIndex, moveToIndex, path }) }, - [dispatchFields, path, setModified], + [moveFieldRow, path], ) const toggleCollapseAll = useCallback( @@ -166,6 +162,7 @@ const BlocksFieldComponent: BlocksFieldClientComponent = (props) => { collapsed, rows, }) + dispatchFields({ type: 'SET_ALL_ROWS_COLLAPSED', path, updatedRows }) setDocFieldPreferences(path, { collapsed: collapsedIDs }) }, @@ -179,6 +176,7 @@ const BlocksFieldComponent: BlocksFieldClientComponent = (props) => { rowID, rows, }) + dispatchFields({ type: 'SET_ROW_COLLAPSED', path, updatedRows }) setDocFieldPreferences(path, { collapsed: collapsedIDs }) }, diff --git a/packages/ui/src/forms/Form/fieldReducer.ts b/packages/ui/src/forms/Form/fieldReducer.ts index d96294413c..744463eb48 100644 --- a/packages/ui/src/forms/Form/fieldReducer.ts +++ b/packages/ui/src/forms/Form/fieldReducer.ts @@ -63,6 +63,14 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { requiresRender: true, rows: withNewRow, value: siblingRows.length, + ...(state[path]?.requiresRender === true + ? { + serverPropsToIgnore: [ + ...(state[path]?.serverPropsToIgnore || []), + 'requiresRender', + ], + } + : state[path]?.serverPropsToIgnore || []), }, } @@ -172,6 +180,14 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { requiresRender: true, rows: rowsMetadata, value: rows.length, + ...(state[path]?.requiresRender === true + ? { + serverPropsToIgnore: [ + ...(state[path]?.serverPropsToIgnore || []), + 'requiresRender', + ], + } + : state[path]?.serverPropsToIgnore || ([] as any)), }, } @@ -200,6 +216,14 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { ...state[path], requiresRender: true, rows: rowsWithinField, + ...(state[path]?.requiresRender === true + ? { + serverPropsToIgnore: [ + ...(state[path]?.serverPropsToIgnore || []), + 'requiresRender', + ], + } + : state[path]?.serverPropsToIgnore || ([] as any)), }, } @@ -218,9 +242,8 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { const copyOfMovingLabel = customComponents.RowLabels[moveFromIndex] - // eslint-disable-next-line @typescript-eslint/no-floating-promises customComponents.RowLabels.splice(moveFromIndex, 1) - // eslint-disable-next-line @typescript-eslint/no-floating-promises + customComponents.RowLabels.splice(moveToIndex, 0, copyOfMovingLabel) newState[path].customComponents = customComponents @@ -253,6 +276,14 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { requiresRender: true, rows: rowsMetadata, value: rows.length, + ...(state[path]?.requiresRender === true + ? { + serverPropsToIgnore: [ + ...(state[path]?.serverPropsToIgnore || []), + 'requiresRender', + ], + } + : state[path]?.serverPropsToIgnore || []), }, ...flattenRows(path, rows), } @@ -292,6 +323,14 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { disableFormData: true, rows: rowsMetadata, value: siblingRows.length, + ...(state[path]?.requiresRender === true + ? { + serverPropsToIgnore: [ + ...(state[path]?.serverPropsToIgnore || []), + 'requiresRender', + ], + } + : state[path]?.serverPropsToIgnore || []), }, } @@ -327,7 +366,7 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { return newState } - //TODO: Remove this in 4.0 - this is a temporary fix to prevent a breaking change + // TODO: Remove this in 4.0 - this is a temporary fix to prevent a breaking change if (action.sanitize) { for (const field of Object.values(action.state)) { if (field.valid !== false) { diff --git a/packages/ui/src/forms/Form/index.tsx b/packages/ui/src/forms/Form/index.tsx index 5470ea5a44..8e9246a93a 100644 --- a/packages/ui/src/forms/Form/index.tsx +++ b/packages/ui/src/forms/Form/index.tsx @@ -121,11 +121,11 @@ export const Form: React.FC = (props) => { const fieldsReducer = useReducer(fieldReducer, {}, () => initialState) - const [fields, dispatchFields] = fieldsReducer + const [formState, dispatchFields] = fieldsReducer - contextRef.current.fields = fields + contextRef.current.fields = formState - const prevFields = useRef(fields) + const prevFormState = useRef(formState) const validateForm = useCallback(async () => { const validatedFieldState = {} @@ -611,9 +611,25 @@ export const Form: React.FC = (props) => { [dispatchFields, getDataByPath], ) + const moveFieldRow: FormContextType['moveFieldRow'] = useCallback( + ({ moveFromIndex, moveToIndex, path }) => { + dispatchFields({ + type: 'MOVE_ROW', + moveFromIndex, + moveToIndex, + path, + }) + + setModified(true) + }, + [dispatchFields], + ) + const removeFieldRow: FormContextType['removeFieldRow'] = useCallback( ({ path, rowIndex }) => { dispatchFields({ type: 'REMOVE_ROW', path, rowIndex }) + + setModified(true) }, [dispatchFields], ) @@ -672,6 +688,7 @@ export const Form: React.FC = (props) => { contextRef.current.dispatchFields = dispatchFields contextRef.current.addFieldRow = addFieldRow contextRef.current.removeFieldRow = removeFieldRow + contextRef.current.moveFieldRow = moveFieldRow contextRef.current.replaceFieldRow = replaceFieldRow contextRef.current.uuid = uuid contextRef.current.initializing = initializing @@ -710,7 +727,7 @@ export const Form: React.FC = (props) => { refreshCookie() }, 15000, - [fields], + [formState], ) useEffect(() => { @@ -743,7 +760,7 @@ export const Form: React.FC = (props) => { }) if (changed) { - prevFields.current = newState + prevFormState.current = newState dispatchFields({ type: 'REPLACE_STATE', @@ -757,14 +774,14 @@ export const Form: React.FC = (props) => { useDebouncedEffect( () => { - if ((isFirstRenderRef.current || !dequal(fields, prevFields.current)) && modified) { + if ((isFirstRenderRef.current || !dequal(formState, prevFormState.current)) && modified) { executeOnChange(submitted) } - prevFields.current = fields + prevFormState.current = formState isFirstRenderRef.current = false }, - [modified, submitted, fields], + [modified, submitted, formState], 250, ) @@ -793,7 +810,7 @@ export const Form: React.FC = (props) => { diff --git a/packages/ui/src/forms/Form/initContextState.ts b/packages/ui/src/forms/Form/initContextState.ts index 30ba7295ce..ac3c9a9a11 100644 --- a/packages/ui/src/forms/Form/initContextState.ts +++ b/packages/ui/src/forms/Form/initContextState.ts @@ -41,6 +41,7 @@ export const initContextState: Context = { getSiblingData, initializing: undefined, isValid: true, + moveFieldRow: () => undefined, removeFieldRow: () => undefined, replaceFieldRow: () => undefined, replaceState: () => undefined, diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index 753972a4e7..f86ae1a6f5 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -1,4 +1,6 @@ 'use client' +import type { FieldState } from 'payload' + import { dequal } from 'dequal/lite' // lite: no need for Map and Set support import { type FormState } from 'payload' @@ -27,7 +29,7 @@ export const mergeServerFormState = ({ const newState = {} if (existingState) { - const serverPropsToAccept = [ + const serverPropsToAccept: Array = [ 'passesCondition', 'valid', 'errorMessage', @@ -46,6 +48,7 @@ export const mergeServerFormState = ({ if (!incomingState[path]) { continue } + let fieldChanged = false /** @@ -55,6 +58,7 @@ export const mergeServerFormState = ({ newFieldState.errorPaths, incomingState[path].errorPaths as unknown as string[], ) + if (errorPathsResult.result) { if (errorPathsResult.changed) { changed = errorPathsResult.changed @@ -76,18 +80,33 @@ export const mergeServerFormState = ({ /** * Handle adding all the remaining props that should be updated in the local form state from the server form state */ - serverPropsToAccept.forEach((prop) => { - if (!dequal(incomingState[path]?.[prop], newFieldState[prop])) { + serverPropsToAccept.forEach((propFromServer) => { + if (!dequal(incomingState[path]?.[propFromServer], newFieldState[propFromServer])) { changed = true fieldChanged = true - if (!(prop in incomingState[path])) { + + if (newFieldState?.serverPropsToIgnore?.includes(propFromServer)) { + // Remove the ignored prop for the next request + newFieldState.serverPropsToIgnore = newFieldState.serverPropsToIgnore.filter( + (prop) => prop !== propFromServer, + ) + + // if no keys left, remove the entire object + if (!newFieldState.serverPropsToIgnore.length) { + delete newFieldState.serverPropsToIgnore + } + + return + } + + if (!(propFromServer in incomingState[path])) { // Regarding excluding the customComponents prop from being deleted: the incoming state might not have been rendered, as rendering components for every form onchange is expensive. // Thus, we simply re-use the initial render state - if (prop !== 'customComponents') { - delete newFieldState[prop] + if (propFromServer !== 'customComponents') { + delete newFieldState[propFromServer] } } else { - newFieldState[prop] = incomingState[path][prop] + newFieldState[propFromServer as any] = incomingState[path][propFromServer] } } }) @@ -95,6 +114,7 @@ export const mergeServerFormState = ({ if (newFieldState.valid !== false) { newFieldState.valid = true } + if (newFieldState.passesCondition !== false) { newFieldState.passesCondition = true } @@ -106,7 +126,6 @@ export const mergeServerFormState = ({ // Now loop over values that are part of incoming state but not part of existing state, and add them to the new state. // This can happen if a new array row was added. In our local state, we simply add out stubbed `array` and `array.[index].id` entries to the local form state. // However, all other array sub-fields are not added to the local state - those will be added by the server and may be incoming here. - for (const [path, field] of Object.entries(incomingState)) { if (!existingState[path]) { changed = true diff --git a/packages/ui/src/forms/Form/types.ts b/packages/ui/src/forms/Form/types.ts index 7c0266441b..1a65ba2efb 100644 --- a/packages/ui/src/forms/Form/types.ts +++ b/packages/ui/src/forms/Form/types.ts @@ -80,7 +80,9 @@ export type Submit = ( options?: SubmitOptions, e?: React.FormEvent, ) => Promise + export type ValidateForm = () => Promise + export type CreateFormData = ( overrides?: Record, /** @@ -89,6 +91,7 @@ export type CreateFormData = ( */ options?: { mergeOverrideData?: boolean }, ) => FormData | Promise + export type GetFields = () => FormState export type GetField = (path: string) => FormField export type GetData = () => Data @@ -236,6 +239,15 @@ export type Context = { * For example the state could be submitted but invalid as field errors have been returned. */ isValid: boolean + moveFieldRow: ({ + moveFromIndex, + moveToIndex, + path, + }: { + moveFromIndex: number + moveToIndex: number + path: string + }) => void removeFieldRow: ({ path, rowIndex }: { path: string; rowIndex: number }) => void replaceFieldRow: ({ blockType, diff --git a/packages/ui/src/forms/fieldSchemasToFormState/addFieldStatePromise.ts b/packages/ui/src/forms/fieldSchemasToFormState/addFieldStatePromise.ts index 5760eb7c46..c1b17a9f14 100644 --- a/packages/ui/src/forms/fieldSchemasToFormState/addFieldStatePromise.ts +++ b/packages/ui/src/forms/fieldSchemasToFormState/addFieldStatePromise.ts @@ -356,8 +356,6 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom fieldState.rows = rows } - // Unset requiresRender - // so it will be removed from form state fieldState.requiresRender = false // Add values to field state diff --git a/test/eslint.config.js b/test/eslint.config.js index a39b80786b..1912a6b678 100644 --- a/test/eslint.config.js +++ b/test/eslint.config.js @@ -70,6 +70,7 @@ export const testEslintConfig = [ 'saveDocAndAssert', 'runFilterOptionsTest', 'assertNetworkRequests', + 'assertRequestBody', ], }, ], diff --git a/test/form-state/e2e.spec.ts b/test/form-state/e2e.spec.ts index 72b9ea243f..7c3ed9c0f4 100644 --- a/test/form-state/e2e.spec.ts +++ b/test/form-state/e2e.spec.ts @@ -1,9 +1,11 @@ import type { BrowserContext, Page } from '@playwright/test' import type { PayloadTestSDK } from 'helpers/sdk/index.js' +import type { FormState } from 'payload' import { expect, test } from '@playwright/test' import { addBlock } from 'helpers/e2e/addBlock.js' import { assertNetworkRequests } from 'helpers/e2e/assertNetworkRequests.js' +import { assertRequestBody } from 'helpers/e2e/assertRequestBody.js' import * as path from 'path' import { fileURLToPath } from 'url' @@ -180,7 +182,7 @@ test.describe('Form State', () => { await cdpSession.detach() }) - test('sequentially queued tasks not cause nested custom components to disappear', async () => { + test('should not cause nested custom fields to disappear when queuing form state (1)', async () => { await page.goto(postsUrl.create) const field = page.locator('#field-title') await field.fill('Test') @@ -191,14 +193,25 @@ test.describe('Form State', () => { delay: 'Slow 3G', }) + // Add a row and immediately type into another field + // Test that the rich text field within the row does not disappear await assertNetworkRequests( page, postsUrl.create, async () => { - await page.locator('#field-array .array-field__add-row').click() + // Ensure `requiresRender` is `true` is set for the first request + await assertRequestBody<{ args: { formState: FormState } }[]>(page, { + action: page.locator('#field-array .array-field__add-row').click(), + expect: (body) => body[0]?.args?.formState?.array?.requiresRender === true, + }) - await page.locator('#field-title').fill('Title 2') + // Ensure `requiresRender` is `false` for the second request + await assertRequestBody<{ args: { formState: FormState } }[]>(page, { + action: page.locator('#field-title').fill('Title 2'), + expect: (body) => body[0]?.args?.formState?.array?.requiresRender === false, + }) + // use `waitForSelector` to ensure the element doesn't appear and then disappear // eslint-disable-next-line playwright/no-wait-for-selector await page.waitForSelector('#field-array #array-row-0 .field-type.rich-text-lexical', { timeout: TEST_TIMEOUT, @@ -223,6 +236,77 @@ test.describe('Form State', () => { await cdpSession.detach() }) + + test('should not cause nested custom fields to disappear when queuing form state (2)', async () => { + await page.goto(postsUrl.create) + const field = page.locator('#field-title') + await field.fill('Test') + + const cdpSession = await throttleTest({ + page, + context, + delay: 'Slow 3G', + }) + + // Add two rows quickly + // Test that the rich text fields within the rows do not disappear + await assertNetworkRequests( + page, + postsUrl.create, + async () => { + // Ensure `requiresRender` is `true` is set for the first request + await assertRequestBody<{ args: { formState: FormState } }[]>(page, { + action: page.locator('#field-array .array-field__add-row').click(), + expect: (body) => body[0]?.args?.formState?.array?.requiresRender === true, + }) + + // Ensure `requiresRender` is `true` is set for the second request + await assertRequestBody<{ args: { formState: FormState } }[]>(page, { + action: page.locator('#field-array .array-field__add-row').click(), + expect: (body) => body[0]?.args?.formState?.array?.requiresRender === true, + }) + + // use `waitForSelector` to ensure the element doesn't appear and then disappear + // eslint-disable-next-line playwright/no-wait-for-selector + await page.waitForSelector('#field-array #array-row-0 .field-type.rich-text-lexical', { + timeout: TEST_TIMEOUT, + }) + + // use `waitForSelector` to ensure the element doesn't appear and then disappear + // eslint-disable-next-line playwright/no-wait-for-selector + await page.waitForSelector('#field-array #array-row-1 .field-type.rich-text-lexical', { + timeout: TEST_TIMEOUT, + }) + + await expect( + page.locator('#field-array #array-row-0 .field-type.rich-text-lexical'), + ).toBeVisible() + + await expect( + page.locator('#field-array #array-row-1 .field-type.rich-text-lexical'), + ).toBeVisible() + }, + { + allowedNumberOfRequests: 2, + timeout: 10000, + }, + ) + + // Ensure `requiresRender` is `false` for the third request + await assertRequestBody<{ args: { formState: FormState } }[]>(page, { + action: page.locator('#field-title').fill('Title 2'), + expect: (body) => body[0]?.args?.formState?.array?.requiresRender === false, + }) + + await cdpSession.send('Network.emulateNetworkConditions', { + offline: false, + latency: 0, + downloadThroughput: -1, + uploadThroughput: -1, + }) + + await cdpSession.detach() + }) }) async function createPost(overrides?: Partial): Promise { diff --git a/test/helpers/e2e/assertRequestBody.ts b/test/helpers/e2e/assertRequestBody.ts new file mode 100644 index 0000000000..9873092790 --- /dev/null +++ b/test/helpers/e2e/assertRequestBody.ts @@ -0,0 +1,28 @@ +import type { Page } from '@playwright/test' + +import { expect } from '@playwright/test' + +export const assertRequestBody = async ( + page: Page, + options: { + action: Promise | void + expect?: (requestBody: T) => boolean | Promise + }, +): Promise => { + const [request] = await Promise.all([ + page.waitForRequest((request) => request.method() === 'POST'), // Adjust condition as needed + await options.action, + ]) + + const requestBody = request.postData() + + if (typeof requestBody === 'string') { + const parsedBody = JSON.parse(requestBody) as T + + if (typeof options.expect === 'function') { + expect(await options.expect(parsedBody)).toBeTruthy() + } + + return parsedBody + } +} diff --git a/tsconfig.base.json b/tsconfig.base.json index daa36c7211..e2b64e3bc8 100644 --- a/tsconfig.base.json +++ b/tsconfig.base.json @@ -31,7 +31,7 @@ } ], "paths": { - "@payload-config": ["./test/query-presets/config.ts"], + "@payload-config": ["./test/form-state/config.ts"], "@payloadcms/admin-bar": ["./packages/admin-bar/src"], "@payloadcms/live-preview": ["./packages/live-preview/src"], "@payloadcms/live-preview-react": ["./packages/live-preview-react/src/index.ts"],