tabshift/payload
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ui): nested fields disappear when manipulating rows in form state (#11906)

Browse Source 
Continuation of #11867. When rendering custom fields nested within
arrays or blocks, such as the Lexical rich text editor which is treated
as a custom field, these fields will sometimes disappear when form state
requests are invoked sequentially. This is especially reproducible on
slow networks.

This is different from the previous PR in that this issue is caused by
adding _rows_ back-to-back, whereas the previous issue was caused when
adding a single row followed by a change to another field.

Here's a screen recording demonstrating the issue:


https://github.com/user-attachments/assets/5ecfa9ec-b747-49ed-8618-df282e64519d

The problem is that `requiresRender` is never sent in the form state
request for row 2. This is because the [task
queue](https://github.com/payloadcms/payload/pull/11579) processes tasks
within a single `useEffect`. This forces React to batch the results of
these tasks into a single rendering cycle. So if request 1 sets state
that request 2 relies on, request 2 will never use that state since
they'll execute within the same lifecycle.

Here's a play-by-play of the current behavior:

1. The "add row" event is dispatched
    a. This sets `requiresRender: true` in form state
1. A form state request is sent with `requiresRender: true`
1. While that request is processing, another "add row" event is
dispatched
    a. This sets `requiresRender: true` in form state
    b. This adds a form state request into the queue
1. The initial form state request finishes
    a. This sets `requiresRender: false` in form state
1. The next form state request that was queued up in 3b is sent with
`requiresRender: false`
    a. THIS IS EXPECTED, BUT SHOULD ACTUALLY BE `true`!!

To fix this this, we need to ensure that the `requiresRender` property
is persisted into the second request instead of overridden. To do this,
we can add a new `serverPropsToIgnore` to form state which is read when
the processing results from the server. So if `requiresRender` exists in
`serverPropsToIgnore`, we do not merge it. This works because we
actually mutate form state in between requests. So request 2 can read
the results from request 1 without going through an additional rendering
cycle.

Here's a play-by-play of the fix:

1. The "add row" event is dispatched
    a. This sets `requiresRender: true` in form state
b. This adds a task in the queue to mutate form state with
`requiresRender: true`
1. A form state request is sent with `requiresRender: true`
1. While that request is processing, another "add row" event is
dispatched
a. This sets `requiresRender: true` in form state AND
`serverPropsToIgnore: [ "requiresRender" ]`
    c. This adds a form state request into the queue
1. The initial form state request finishes
a. This returns `requiresRender: false` from the form state endpoint BUT
IS IGNORED
1. The next form state request that was queued up in 3c is sent with
`requiresRender: true`
This commit is contained in:
Jacob Fletcher
2025-04-01 09:54:22 -04:00
committed by GitHub
parent 329cd0b876
commit 373f6d1032
13 changed files with 253 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
packages/payload/src/admin/forms/Form.ts
View File

@@ -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<keyof FieldState>
valid?: boolean
validate?: Validate
value?: unknown

16
packages/ui/src/fields/Array/index.tsx
View File

@@ -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(

16
packages/ui/src/fields/Blocks/index.tsx
View File

@@ -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 })
},

45
packages/ui/src/forms/Form/fieldReducer.ts
View File

@@ -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) {

35
packages/ui/src/forms/Form/index.tsx
View File

@@ -121,11 +121,11 @@ export const Form: React.FC<FormProps> = (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<FormProps> = (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<FormProps> = (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<FormProps> = (props) => {
refreshCookie()
},
15000,
[fields],
[formState],
)
useEffect(() => {
@@ -743,7 +760,7 @@ export const Form: React.FC<FormProps> = (props) => {
})
if (changed) {
prevFields.current = newState
prevFormState.current = newState
dispatchFields({
type: 'REPLACE_STATE',
@@ -757,14 +774,14 @@ export const Form: React.FC<FormProps> = (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<FormProps> = (props) => {
<FormContext value={contextRef.current}>
<FormWatchContext
value={{
fields,
fields: formState,
...contextRef.current,
}}
>

1
packages/ui/src/forms/Form/initContextState.ts
View File

@@ -41,6 +41,7 @@ export const initContextState: Context = {
getSiblingData,
initializing: undefined,
isValid: true,
moveFieldRow: () => undefined,
removeFieldRow: () => undefined,
replaceFieldRow: () => undefined,
replaceState: () => undefined,

35
packages/ui/src/forms/Form/mergeServerFormState.ts
View File

@@ -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<keyof FieldState> = [
'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

12
packages/ui/src/forms/Form/types.ts
View File

@@ -80,7 +80,9 @@ export type Submit = (
options?: SubmitOptions,
e?: React.FormEvent<HTMLFormElement>,
) => Promise<void>
export type ValidateForm = () => Promise<boolean>
export type CreateFormData = (
overrides?: Record<string, unknown>,
/**
@@ -89,6 +91,7 @@ export type CreateFormData = (
*/
options?: { mergeOverrideData?: boolean },
) => FormData | Promise<FormData>
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,

2
packages/ui/src/forms/fieldSchemasToFormState/addFieldStatePromise.ts
View File

@@ -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

1
test/eslint.config.js
View File

@@ -70,6 +70,7 @@ export const testEslintConfig = [
'saveDocAndAssert',
'runFilterOptionsTest',
'assertNetworkRequests',
'assertRequestBody',
],
},
],

90
test/form-state/e2e.spec.ts
View File

@@ -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<Post>): Promise<Post> {

28
test/helpers/e2e/assertRequestBody.ts Normal file
View File

@@ -0,0 +1,28 @@
import type { Page } from '@playwright/test'
import { expect } from '@playwright/test'
export const assertRequestBody = async <T>(
page: Page,
options: {
action: Promise<void> | void
expect?: (requestBody: T) => boolean | Promise<boolean>
},
): Promise<T | undefined> => {
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
}
}

2
tsconfig.base.json
View File

@@ -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"],