From db8e805a967ba5f494bab07106f77ca884de1402 Mon Sep 17 00:00:00 2001 From: Alessio Gravili Date: Wed, 27 Mar 2024 15:55:19 -0400 Subject: [PATCH] fix: improve error path merging from server, make sure no new or removed rows/values coming from the server are being considered outside addFieldRow --- packages/payload/src/admin/forms/Form.ts | 4 +- packages/ui/src/fields/Array/ArrayRow.tsx | 2 +- packages/ui/src/fields/Array/index.tsx | 2 +- packages/ui/src/fields/Blocks/index.tsx | 2 +- packages/ui/src/forms/Form/fieldReducer.ts | 11 +-- packages/ui/src/forms/Form/index.tsx | 8 +- packages/ui/src/forms/Form/mergeErrorPaths.ts | 38 ++++++++ .../ui/src/forms/Form/mergeServerFormState.ts | 89 +++++++++++-------- .../addFieldStatePromise.ts | 10 ++- .../src/forms/buildStateFromSchema/index.tsx | 2 +- .../buildStateFromSchema/iterateFields.ts | 2 +- test/access-control/e2e.spec.ts | 2 - test/fields/lexical.e2e.spec.ts | 2 +- 13 files changed, 115 insertions(+), 59 deletions(-) create mode 100644 packages/ui/src/forms/Form/mergeErrorPaths.ts diff --git a/packages/payload/src/admin/forms/Form.ts b/packages/payload/src/admin/forms/Form.ts index 8b9f65770..8501b5ff2 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/ui/src/fields/Array/ArrayRow.tsx b/packages/ui/src/fields/Array/ArrayRow.tsx index 6540dd423..f592a24a6 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 71bdfa541..6fdf02859 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/index.tsx b/packages/ui/src/fields/Blocks/index.tsx index 4c783a045..57828b27d 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 a9dd46260..da4af3203 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': { diff --git a/packages/ui/src/forms/Form/index.tsx b/packages/ui/src/forms/Form/index.tsx index 80b43a450..598b6655f 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' @@ -544,7 +544,11 @@ export const Form: React.FC = (props) => { ) if (changed) { - dispatchFields({ type: 'REPLACE_STATE', optimize: false, state: newState }) + dispatchFields({ + type: 'REPLACE_STATE', + optimize: false, + state: newState, + }) } } } diff --git a/packages/ui/src/forms/Form/mergeErrorPaths.ts b/packages/ui/src/forms/Form/mergeErrorPaths.ts new file mode 100644 index 000000000..32b2148d7 --- /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 b5f55ad2c..7906736dc 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -1,10 +1,8 @@ import type { FormState } from 'payload/types' -import isDeepEqual from 'deep-equal' +import { mergeErrorPaths } from './mergeErrorPaths.js' -import { arraysHaveSameStrings } from '../../utilities/arraysHaveSameStrings.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, @@ -14,51 +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] + } } }) - if ( - Array.isArray(newFieldState.rows) && - !isDeepEqual(newFieldState.rows, oldState[path].rows) - ) { - changed = true + /** + * 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/buildStateFromSchema/addFieldStatePromise.ts b/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts index 33ae11190..67533c4f6 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 3908327de..74f616626 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 7dbba5ed0..e7455f8d8 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/test/access-control/e2e.spec.ts b/test/access-control/e2e.spec.ts index 7f43984cb..4da63e281 100644 --- a/test/access-control/e2e.spec.ts +++ b/test/access-control/e2e.spec.ts @@ -9,7 +9,6 @@ import type { ReadOnlyCollection, RestrictedVersion } from './payload-types.js' import { closeNav, - delayNetwork, exactText, initPageConsoleErrorCatch, openDocControls, @@ -60,7 +59,6 @@ describe('access control', () => { const context = await browser.newContext() page = await context.newPage() initPageConsoleErrorCatch(page) - await delayNetwork({ context, page, delay: 'Slow 4G' }) }) test('field without read access should not show', async () => { diff --git a/test/fields/lexical.e2e.spec.ts b/test/fields/lexical.e2e.spec.ts index 7ab19e703..255b22d6c 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()