diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 6e22469840..7d5b00b3ef 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -254,12 +254,7 @@ jobs: run: pnpm exec playwright install - name: E2E Tests - uses: nick-fields/retry@v3 - with: - retry_on: error - max_attempts: 2 - timeout_minutes: 15 - command: pnpm test:e2e ${{ matrix.suite }} + run: pnpm test:e2e ${{ matrix.suite }} - uses: actions/upload-artifact@v4 if: always() diff --git a/packages/payload/src/admin/forms/Form.ts b/packages/payload/src/admin/forms/Form.ts index 8b9f657709..8501b5ff29 100644 --- a/packages/payload/src/admin/forms/Form.ts +++ b/packages/payload/src/admin/forms/Form.ts @@ -8,7 +8,7 @@ export type Data = { export type Row = { blockType?: string collapsed?: boolean - errorPaths?: Set + errorPaths?: string[] id: string } @@ -19,7 +19,7 @@ export type FilterOptionsResult = { export type FormField = { disableFormData?: boolean errorMessage?: string - errorPaths?: Set + errorPaths?: string[] fieldSchema?: Field filterOptions?: FilterOptionsResult initialValue: unknown diff --git a/packages/payload/src/fields/baseFields/baseIDField.ts b/packages/payload/src/fields/baseFields/baseIDField.ts index d66c418ee1..eadb170a8e 100644 --- a/packages/payload/src/fields/baseFields/baseIDField.ts +++ b/packages/payload/src/fields/baseFields/baseIDField.ts @@ -12,10 +12,8 @@ export const baseIDField: Field = { name: 'id', type: 'text', admin: { - disabled: true, - }, - hooks: { - beforeChange: [generateID], + hidden: true, }, + defaultValue: generateID, label: 'ID', } diff --git a/packages/ui/src/fields/Array/ArrayRow.tsx b/packages/ui/src/fields/Array/ArrayRow.tsx index 6540dd4238..f592a24a6a 100644 --- a/packages/ui/src/fields/Array/ArrayRow.tsx +++ b/packages/ui/src/fields/Array/ArrayRow.tsx @@ -70,7 +70,7 @@ export const ArrayRow: React.FC = ({ '0', )}` - const errorCount = row.errorPaths?.size + const errorCount = row.errorPaths?.length const fieldHasErrors = errorCount > 0 && hasSubmitted const classNames = [ diff --git a/packages/ui/src/fields/Array/index.tsx b/packages/ui/src/fields/Array/index.tsx index 71bdfa5417..6fdf028599 100644 --- a/packages/ui/src/fields/Array/index.tsx +++ b/packages/ui/src/fields/Array/index.tsx @@ -181,7 +181,7 @@ export const _ArrayField: React.FC = (props) => { const hasMaxRows = maxRows && rows.length >= maxRows const fieldErrorCount = - rows.reduce((total, row) => total + (row?.errorPaths?.size || 0), 0) + (valid ? 0 : 1) + rows.reduce((total, row) => total + (row?.errorPaths?.length || 0), 0) + (valid ? 0 : 1) const fieldHasErrors = submitted && fieldErrorCount > 0 diff --git a/packages/ui/src/fields/Blocks/BlockRow.tsx b/packages/ui/src/fields/Blocks/BlockRow.tsx index 763b9defb9..5b6518816c 100644 --- a/packages/ui/src/fields/Blocks/BlockRow.tsx +++ b/packages/ui/src/fields/Blocks/BlockRow.tsx @@ -67,7 +67,7 @@ export const BlockRow: React.FC = ({ const { i18n } = useTranslation() const hasSubmitted = useFormSubmitted() - const errorCount = row.errorPaths?.size + const errorCount = row.errorPaths?.length const fieldHasErrors = errorCount > 0 && hasSubmitted const classNames = [ diff --git a/packages/ui/src/fields/Blocks/index.tsx b/packages/ui/src/fields/Blocks/index.tsx index 4c783a0451..57828b27d6 100644 --- a/packages/ui/src/fields/Blocks/index.tsx +++ b/packages/ui/src/fields/Blocks/index.tsx @@ -192,7 +192,7 @@ const _BlocksField: React.FC = (props) => { const hasMaxRows = maxRows && rows.length >= maxRows - const fieldErrorCount = rows.reduce((total, row) => total + (row?.errorPaths?.size || 0), 0) + const fieldErrorCount = rows.reduce((total, row) => total + (row?.errorPaths?.length || 0), 0) const fieldHasErrors = submitted && fieldErrorCount + (valid ? 0 : 1) > 0 const showMinRows = rows.length < minRows || (required && rows.length === 0) diff --git a/packages/ui/src/forms/Form/fieldReducer.ts b/packages/ui/src/forms/Form/fieldReducer.ts index 3f362abe36..da4af32037 100644 --- a/packages/ui/src/forms/Form/fieldReducer.ts +++ b/packages/ui/src/forms/Form/fieldReducer.ts @@ -17,14 +17,13 @@ const ObjectId = (ObjectIdImport.default || export function fieldReducer(state: FormState, action: FieldAction): FormState { switch (action.type) { case 'REPLACE_STATE': { - const newState = {} - if (action.optimize !== false) { // Only update fields that have changed // by comparing old value / initialValue to new // .. // This is a performance enhancement for saving // large documents with hundreds of fields + const newState = {} Object.entries(action.state).forEach(([path, field]) => { const oldField = state[path] @@ -36,12 +35,10 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { newState[path] = oldField } }) - } else { - // If we're not optimizing, just set the state to the new state - return action.state + return newState } - - return newState + // If we're not optimizing, just set the state to the new state + return action.state } case 'REMOVE': { @@ -114,7 +111,7 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { const withNewRow = [...(state[path]?.rows || [])] const newRow: Row = { - id: new ObjectId().toHexString(), + id: (subFieldState?.id.value as string) || new ObjectId().toHexString(), blockType: blockType || undefined, collapsed: false, } diff --git a/packages/ui/src/forms/Form/index.tsx b/packages/ui/src/forms/Form/index.tsx index 3b2b705071..598b6655f0 100644 --- a/packages/ui/src/forms/Form/index.tsx +++ b/packages/ui/src/forms/Form/index.tsx @@ -5,7 +5,7 @@ import type { FormState } from 'payload/types' import isDeepEqual from 'deep-equal' import { useRouter } from 'next/navigation.js' import { serialize } from 'object-to-formdata' -import { wait } from 'payload/utilities' +import { deepCopyObject, wait } from 'payload/utilities' import QueryString from 'qs' import React, { useCallback, useEffect, useReducer, useRef, useState } from 'react' import { toast } from 'react-toastify' @@ -530,7 +530,7 @@ export const Form: React.FC = (props) => { () => { const executeOnChange = async () => { if (Array.isArray(onChange)) { - let revalidatedFormState: FormState = fields + let revalidatedFormState: FormState = contextRef.current.fields for (const onChangeFn of onChange) { revalidatedFormState = await onChangeFn({ @@ -538,10 +538,17 @@ export const Form: React.FC = (props) => { }) } - const { changed, newState } = mergeServerFormState(fields || {}, revalidatedFormState) + const { changed, newState } = mergeServerFormState( + contextRef.current.fields || {}, + revalidatedFormState, + ) if (changed) { - dispatchFields({ type: 'REPLACE_STATE', optimize: false, state: newState }) + dispatchFields({ + type: 'REPLACE_STATE', + optimize: false, + state: newState, + }) } } } @@ -549,7 +556,9 @@ export const Form: React.FC = (props) => { if (modified) void executeOnChange() }, 150, - [fields, dispatchFields, onChange], + // Make sure we trigger this whenever modified changes (not just when `fields` changes), otherwise we will miss merging server form state for the first form update/onChange. Here's why: + // `fields` updates before `modified`, because setModified is in a setTimeout. So on the first change, modified is false, so we don't trigger the effect even though we should. + [contextRef.current.fields, dispatchFields, onChange, modified], ) const actionString = diff --git a/packages/ui/src/forms/Form/mergeErrorPaths.ts b/packages/ui/src/forms/Form/mergeErrorPaths.ts new file mode 100644 index 0000000000..32b2148d71 --- /dev/null +++ b/packages/ui/src/forms/Form/mergeErrorPaths.ts @@ -0,0 +1,38 @@ +import { arraysHaveSameStrings } from '@payloadcms/ui/utilities/arraysHaveSameStrings' + +export const mergeErrorPaths = ( + existing?: string[], + incoming?: string[], +): { + changed: boolean + result?: string[] +} => { + if (!existing) { + return { + changed: false, + result: undefined, + } + } + + const existingErrorPaths: string[] = [] + const incomingErrorPaths: string[] = [] + + if (Array.isArray(incoming) && incoming?.length) { + incoming.forEach((path) => incomingErrorPaths.push(path)) + } + + if (Array.isArray(existing) && existing?.length) { + existing.forEach((path) => existingErrorPaths.push(path)) + } + + if (!arraysHaveSameStrings(existingErrorPaths, incomingErrorPaths)) { + return { + changed: true, + result: incomingErrorPaths, + } + } + return { + changed: false, + result: existing, + } +} diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index 9e4083e364..7906736dc6 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -1,8 +1,8 @@ import type { FormState } from 'payload/types' -import { arraysHaveSameStrings } from '../../utilities/arraysHaveSameStrings.js' +import { mergeErrorPaths } from './mergeErrorPaths.js' -const propsToCheck = ['passesCondition', 'valid', 'errorMessage'] +const serverPropsToAccept = ['passesCondition', 'valid', 'errorMessage'] /** * Merges certain properties from the server state into the client state. These do not include values, @@ -12,44 +12,70 @@ const propsToCheck = ['passesCondition', 'valid', 'errorMessage'] * is the thing we want to keep in sync with the server (where it's calculated) on the client. */ export const mergeServerFormState = ( - oldState: FormState, - newState: FormState, + existingState: FormState, + incomingState: FormState, ): { changed: boolean; newState: FormState } => { let changed = false - if (oldState) { - Object.entries(newState).forEach(([path, newFieldState]) => { - if (!oldState[path]) return + if (existingState) { + Object.entries(existingState).forEach(([path, newFieldState]) => { + if (!incomingState[path]) return - newFieldState.initialValue = oldState[path].initialValue - newFieldState.value = oldState[path].value + /** + * Handle error paths + */ - const oldErrorPaths: string[] = [] - const newErrorPaths: string[] = [] - - if (oldState[path].errorPaths instanceof Set) { - oldState[path].errorPaths.forEach((path) => oldErrorPaths.push(path)) + const errorPathsResult = mergeErrorPaths( + newFieldState.errorPaths, + incomingState[path].errorPaths as unknown as string[], + ) + if (errorPathsResult.result) { + if (errorPathsResult.changed) { + changed = errorPathsResult.changed + } + newFieldState.errorPaths = errorPathsResult.result } - if ( - newFieldState.errorPaths && - !Array.isArray(newFieldState.errorPaths) && - typeof newFieldState.errorPaths - ) { - Object.values(newFieldState.errorPaths).forEach((path) => newErrorPaths.push(path)) - } - - if (!arraysHaveSameStrings(oldErrorPaths, newErrorPaths)) { - changed = true - } - - propsToCheck.forEach((prop) => { - if (newFieldState[prop] != oldState[path][prop]) { + /** + * Handle the rest which is in serverPropsToAccept + */ + serverPropsToAccept.forEach((prop) => { + if (incomingState[path]?.[prop] !== newFieldState[prop]) { changed = true + if (!(prop in incomingState[path])) { + delete newFieldState[prop] + } else { + newFieldState[prop] = incomingState[path][prop] + } } }) + + /** + * Handle rows + */ + if (Array.isArray(newFieldState.rows) && newFieldState?.rows.length) { + let i = 0 + for (const row of newFieldState.rows) { + const incomingRow = incomingState[path]?.rows?.[i] + if (incomingRow) { + const errorPathsRowResult = mergeErrorPaths( + row.errorPaths, + incomingRow.errorPaths as unknown as string[], + ) + if (errorPathsRowResult.result) { + if (errorPathsResult.changed) { + changed = true + } + incomingRow.errorPaths = errorPathsRowResult.result + } + } + + i++ + } + } }) } - return { changed, newState } + // Conditions don't work if we don't memcopy the new state, as the object references would otherwise be the same + return { changed, newState: { ...existingState } } } diff --git a/packages/ui/src/forms/Form/types.ts b/packages/ui/src/forms/Form/types.ts index 7009b87f7d..590e30cb0a 100644 --- a/packages/ui/src/forms/Form/types.ts +++ b/packages/ui/src/forms/Form/types.ts @@ -174,7 +174,7 @@ export type Context = { disabled: boolean dispatchFields: Dispatch /** - * @deprecated Form context fields may be outdated and should not be relied on. Instead, prefer `useFormFields`. + * Form context fields may be outdated and should not be relied on. Instead, prefer `useFormFields`. */ fields: FormState formRef: React.MutableRefObject diff --git a/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts b/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts index 33ae11190b..67533c4f6d 100644 --- a/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts +++ b/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts @@ -23,7 +23,7 @@ export type AddFieldStatePromiseArgs = { */ anyParentLocalized?: boolean data: Data - errorPaths: Set + errorPaths: string[] field: NonPresentationalField /** * You can use this to filter down to only `localized` fields that require translation (type: text, textarea, etc.). Another plugin might want to look for only `point` type fields to do some GIS function. With the filter function you can go in like a surgeon. @@ -74,7 +74,7 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom id, anyParentLocalized = false, data, - errorPaths: parentErrorPaths, + errorPaths: parentErrorPaths = [], field, filter, forceFullValue = false, @@ -95,7 +95,7 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom const validate = operation === 'update' ? field.validate : undefined const fieldState: FormField = { - errorPaths: new Set(), + errorPaths: [], fieldSchema: includeSchema ? field : undefined, initialValue: undefined, passesCondition, @@ -144,7 +144,9 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom fieldState.valid = false // TODO: this is unpredictable, need to figure out why // It will sometimes lead to inconsistencies across re-renders - parentErrorPaths.add(`${path}${field.name}`) + if (!parentErrorPaths.includes(`${path}${field.name}`)) { + parentErrorPaths.push(`${path}${field.name}`) + } } else { fieldState.valid = true } diff --git a/packages/ui/src/forms/buildStateFromSchema/index.tsx b/packages/ui/src/forms/buildStateFromSchema/index.tsx index 3908327ded..74f6166261 100644 --- a/packages/ui/src/forms/buildStateFromSchema/index.tsx +++ b/packages/ui/src/forms/buildStateFromSchema/index.tsx @@ -40,7 +40,7 @@ export const buildStateFromSchema = async (args: Args): Promise => { await iterateFields({ id, data: fullData, - errorPaths: new Set(), + errorPaths: [], fields: fieldSchema, fullData, operation, diff --git a/packages/ui/src/forms/buildStateFromSchema/iterateFields.ts b/packages/ui/src/forms/buildStateFromSchema/iterateFields.ts index 7dbba5ed07..e7455f8d8f 100644 --- a/packages/ui/src/forms/buildStateFromSchema/iterateFields.ts +++ b/packages/ui/src/forms/buildStateFromSchema/iterateFields.ts @@ -12,7 +12,7 @@ type Args = { */ anyParentLocalized?: boolean data: Data - errorPaths: Set + errorPaths: string[] fields: FieldSchema[] filter?: (args: AddFieldStatePromiseArgs) => boolean /** diff --git a/packages/ui/src/providers/ComponentMap/buildComponentMap/mapFields.tsx b/packages/ui/src/providers/ComponentMap/buildComponentMap/mapFields.tsx index 2ed184110d..712e687d63 100644 --- a/packages/ui/src/providers/ComponentMap/buildComponentMap/mapFields.tsx +++ b/packages/ui/src/providers/ComponentMap/buildComponentMap/mapFields.tsx @@ -228,7 +228,6 @@ export const mapFields = (args: { disabled: field.admin?.disabled, fieldMap: mapFields({ config, - disableAddingID: true, fieldSchema: field.fields, filter, parentPath: path, @@ -251,7 +250,6 @@ export const mapFields = (args: { const blocks = field.blocks.map((block) => { const blockFieldMap = mapFields({ config, - disableAddingID: true, fieldSchema: block.fields, filter, parentPath: `${path}.${block.slug}`, diff --git a/test/access-control/e2e.spec.ts b/test/access-control/e2e.spec.ts index c81e739bbb..6c68500c8c 100644 --- a/test/access-control/e2e.spec.ts +++ b/test/access-control/e2e.spec.ts @@ -13,6 +13,7 @@ import { initPageConsoleErrorCatch, openDocControls, openNav, + saveDocAndAssert, } from '../helpers.js' import { AdminUrlUtil } from '../helpers/adminUrlUtil.js' import { initPayloadE2E } from '../helpers/initPayloadE2E.js' @@ -248,7 +249,7 @@ describe('access control', () => { await expect(deleteAction).toBeHidden() await page.locator('#field-approvedForRemoval').check() - await page.locator('#action-save').click() + await saveDocAndAssert(page) await openDocControls(page) const deleteAction2 = page.locator('#action-delete') @@ -272,7 +273,7 @@ describe('access control', () => { // Allow access to test global. await page.locator('.checkbox-input:has(#field-test) input').check() - await page.locator('#action-save').click() + await saveDocAndAssert(page) await openNav(page) diff --git a/test/fields/e2e.spec.ts b/test/fields/e2e.spec.ts index 73ae36a6da..1b3d3136ae 100644 --- a/test/fields/e2e.spec.ts +++ b/test/fields/e2e.spec.ts @@ -1386,7 +1386,8 @@ describe('fields', () => { await expect(dateField).toBeVisible() await dateField.fill('02/07/2023') await expect(dateField).toHaveValue('02/07/2023') - await page.locator('#action-save').click() + await saveDocAndAssert(page) + const clearButton = page.locator('#field-default .date-time-picker__clear-button') await expect(clearButton).toBeVisible() await clearButton.click() @@ -1409,10 +1410,7 @@ describe('fields', () => { // enter date in default date field await dateField.fill('02/07/2023') - await page.locator('#action-save').click() - - // wait for navigation to update route - await expect.poll(() => page.url(), { timeout: 1000 }).not.toContain('create') + await saveDocAndAssert(page) // get the ID of the doc const routeSegments = page.url().split('/') @@ -1473,10 +1471,7 @@ describe('fields', () => { // enter date in default date field await dateField.fill('02/07/2023') - await page.locator('#action-save').click() - - // wait for navigation to update route - await expect.poll(() => page.url(), { timeout: 1000 }).not.toContain('create') + await saveDocAndAssert(page) // get the ID of the doc const routeSegments = page.url().split('/') diff --git a/test/fields/lexical.e2e.spec.ts b/test/fields/lexical.e2e.spec.ts index 18d1d4fabe..255b22d6c0 100644 --- a/test/fields/lexical.e2e.spec.ts +++ b/test/fields/lexical.e2e.spec.ts @@ -32,7 +32,7 @@ async function navigateToLexicalFields() { const url: AdminUrlUtil = new AdminUrlUtil(serverURL, 'lexical-fields') await page.goto(url.list) const linkToDoc = page.locator('tbody tr:first-child .cell-title a').first() - await expect(() => expect(linkToDoc).toBeTruthy()).toPass({ timeout: 45000 }) + await expect(() => expect(linkToDoc).toBeTruthy()).toPass({ timeout: POLL_TOPASS_TIMEOUT }) const linkDocHref = await linkToDoc.getAttribute('href') await linkToDoc.click() @@ -718,7 +718,7 @@ describe('lexical', () => { await expect(nestedEditorParagraph).toHaveText('Some text below relationship node 12345') }) - test.skip('should respect row removal in nested array field', async () => { + test('should respect row removal in nested array field', async () => { await navigateToLexicalFields() const richTextField = page.locator('.rich-text-lexical').nth(1) // second await richTextField.scrollIntoViewIfNeeded()