From 63362d3af36021018afe518cae48d305dc73c622 Mon Sep 17 00:00:00 2001 From: Lucas Vasconcelos Date: Thu, 6 Aug 2026 18:14:47 -0300 Subject: [PATCH] fix(ui): prevent autosave from overwriting recently edited fields and remounting relationship/upload list drawers Switching to a different field while a previous autosave request was still in flight cleared that field's modified protection, so a stale response could overwrite the newer local edit. Track a per-field last-modified timestamp and compare it against when each request was sent, so a response is only accepted if no local edit happened after it was dispatched. Also preserve the value/initialValue reference for unmodified fields when the incoming server value is content-identical, since object/array-shaped values (e.g. relationship and upload fields) otherwise get a new reference on every autosave merge, causing their list drawers to needlessly recompute filter options and remount. --- packages/payload/src/admin/forms/Form.ts | 9 + packages/ui/src/forms/Form/fieldReducer.ts | 5 +- packages/ui/src/forms/Form/index.tsx | 6 + .../ui/src/forms/Form/mergeServerFormState.ts | 46 +++++- packages/ui/src/forms/Form/types.ts | 6 + .../form-state/collections/Autosave/index.tsx | 13 ++ test/form-state/e2e.spec.ts | 156 ++++++++++++++++-- test/form-state/int.spec.ts | 57 +++++++ test/form-state/payload-types.ts | 4 + 9 files changed, 287 insertions(+), 15 deletions(-) diff --git a/packages/payload/src/admin/forms/Form.ts b/packages/payload/src/admin/forms/Form.ts index 8b70baaa9b0..0939215cd89 100644 --- a/packages/payload/src/admin/forms/Form.ts +++ b/packages/payload/src/admin/forms/Form.ts @@ -79,6 +79,15 @@ export type FieldState = { * from the current path of a given field, the field's components will be re-rendered. */ lastRenderedPath?: string + /** + * Timestamp (from `Date.now()`) of the last time this field's value was changed locally. Unlike `isModified`, + * this is never reset. It is used to detect whether a field was edited _after_ a given autosave/submit request + * was sent, so that a response for an older, now-stale request cannot overwrite a newer local edit even after + * `isModified` has already been cleared for this field. + * + * @experimental This property is experimental and may change in the future. Use at your own risk. + */ + modifiedAt?: number passesCondition?: boolean rows?: Row[] /** diff --git a/packages/ui/src/forms/Form/fieldReducer.ts b/packages/ui/src/forms/Form/fieldReducer.ts index 9e8d0c673ba..c868ca942c0 100644 --- a/packages/ui/src/forms/Form/fieldReducer.ts +++ b/packages/ui/src/forms/Form/fieldReducer.ts @@ -217,12 +217,13 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { } case 'MERGE_SERVER_STATE': { - const { acceptValues, prevStateRef, serverState } = action + const { acceptValues, prevStateRef, requestSnapshotTakenAt, serverState } = action const newState = mergeServerFormState({ acceptValues, currentState: state || {}, incomingState: serverState, + requestSnapshotTakenAt, }) if (prevStateRef) { @@ -414,7 +415,7 @@ export function fieldReducer(state: FormState, action: FieldAction): FormState { return { ...field, [key]: value, - ...(key === 'value' ? { isModified: true } : {}), + ...(key === 'value' ? { isModified: true, modifiedAt: Date.now() } : {}), } } diff --git a/packages/ui/src/forms/Form/index.tsx b/packages/ui/src/forms/Form/index.tsx index 3f742b0dce5..edd309f55d3 100644 --- a/packages/ui/src/forms/Form/index.tsx +++ b/packages/ui/src/forms/Form/index.tsx @@ -311,6 +311,11 @@ export const Form: React.FC = (props) => { await wait(100) } + // Captured now, before any further local edits can happen, so that the merge of this request's + // response can later detect whether a field was edited _after_ this point - and therefore is not + // yet reflected in the response we're about to receive. + const requestSnapshotTakenAt = Date.now() + const data = reduceFieldsToValues(contextRef.current.fields, true) const serializableFormState = deepCopyObjectSimpleWithoutReactComponents( @@ -442,6 +447,7 @@ export const Form: React.FC = (props) => { type: 'MERGE_SERVER_STATE', acceptValues, prevStateRef: prevFormState, + requestSnapshotTakenAt, serverState: newFormState, }) } diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index a17e9b591ad..570fec701b6 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -22,6 +22,13 @@ type Args = { acceptValues?: AcceptValues currentState?: FormState incomingState: FormState + /** + * The timestamp (from `Date.now()`) captured when the form state snapshot being merged was sent to the + * server. Fields that were locally modified _after_ this timestamp are never overwritten, even if + * `isModified` has since been cleared for them (e.g. because the user moved on to edit a different field + * while this request was still in flight). + */ + requestSnapshotTakenAt?: number } /** @@ -83,6 +90,7 @@ export const mergeServerFormState = ({ acceptValues, currentState = {}, incomingState, + requestSnapshotTakenAt, }: Args): FormState => { const newState = { ...currentState } @@ -96,7 +104,17 @@ export const mergeServerFormState = ({ * Otherwise: * a. accept all values when explicitly requested, e.g. on submit * b. only accept values for unmodified fields, e.g. on autosave + * + * For (b), `isModified` alone is not enough: it is cleared as soon as the user moves on to edit a + * _different_ field, even if this response corresponds to an older request sent before that field's + * latest local edit. To guard against that race, also confirm this field was not modified locally + * after this response's request was sent - otherwise this stale response would clobber the newer edit. */ + const wasModifiedAfterThisRequestWasSent = + typeof requestSnapshotTakenAt === 'number' && + typeof currentState[path]?.modifiedAt === 'number' && + currentState[path].modifiedAt > requestSnapshotTakenAt + let shouldAcceptValue = incomingField.addedByServer || acceptValues === true || @@ -104,7 +122,8 @@ export const mergeServerFormState = ({ acceptValues !== null && // Note: Must be explicitly `false`, allow `null` or `undefined` to mean true acceptValues.overrideLocalChanges === false && - !currentState[path]?.isModified) + !currentState[path]?.isModified && + !wasModifiedAfterThisRequestWasSent) /** * For array row fields, verify the row IDs match at the given index before accepting @@ -138,6 +157,31 @@ export const mergeServerFormState = ({ */ const { initialValue, value, ...rest } = incomingField sanitizedIncomingField = rest + } else if ( + currentState[path] && + dequal(currentState[path].value, incomingField.value) && + dequal(currentState[path].initialValue, incomingField.initialValue) + ) { + /** + * The incoming value is content-identical to the current one, but the server always sends a freshly + * (de)serialized object/array - a new reference even when nothing changed. Preserve the existing + * reference instead of swapping in the equal-but-different one. Otherwise, consumers that memoize + * on this value's identity (e.g. a relationship/upload field's list drawer filter options) see it as + * "changed" on every autosave cycle and needlessly recompute or remount, even though the field itself + * was never touched. + * + * Only swap in the reference for keys that were actually present on the incoming field, so we don't + * introduce a `value`/`initialValue` key (set to `undefined`) where there wasn't one before. + */ + sanitizedIncomingField = { ...incomingField } + + if ('value' in incomingField) { + sanitizedIncomingField.value = currentState[path].value + } + + if ('initialValue' in incomingField) { + sanitizedIncomingField.initialValue = currentState[path].initialValue + } } newState[path] = { diff --git a/packages/ui/src/forms/Form/types.ts b/packages/ui/src/forms/Form/types.ts index b8ce6ac6d9b..3b73017c790 100644 --- a/packages/ui/src/forms/Form/types.ts +++ b/packages/ui/src/forms/Form/types.ts @@ -202,6 +202,12 @@ export type ADD_ROW = { export type MERGE_SERVER_STATE = { acceptValues?: AcceptValues prevStateRef?: React.RefObject + /** + * The timestamp (from `Date.now()`) captured when the form state snapshot for this request was taken, + * i.e. right before it was sent to the server. Used to detect fields that were edited locally _after_ + * this request was sent, so their newer value isn't overwritten by this now-stale response. + */ + requestSnapshotTakenAt?: number serverState: FormState type: 'MERGE_SERVER_STATE' } diff --git a/test/form-state/collections/Autosave/index.tsx b/test/form-state/collections/Autosave/index.tsx index 51dd8aad41c..6cbb1c2615c 100644 --- a/test/form-state/collections/Autosave/index.tsx +++ b/test/form-state/collections/Autosave/index.tsx @@ -12,6 +12,10 @@ export const AutosavePostsCollection: CollectionConfig = { name: 'title', type: 'text', }, + { + name: 'subtitle', + type: 'text', + }, { name: 'computedTitle', type: 'text', @@ -19,6 +23,15 @@ export const AutosavePostsCollection: CollectionConfig = { beforeChange: [({ data }) => data?.title], }, }, + { + name: 'relatedPosts', + type: 'relationship', + admin: { + appearance: 'drawer', + }, + hasMany: true, + relationTo: autosavePostsSlug, + }, ], versions: { drafts: { diff --git a/test/form-state/e2e.spec.ts b/test/form-state/e2e.spec.ts index acd748d7b08..749c4702897 100644 --- a/test/form-state/e2e.spec.ts +++ b/test/form-state/e2e.spec.ts @@ -78,18 +78,14 @@ test.describe('Form State', () => { await expect(page.locator('#field-title')).toBeDisabled() }) - test( - 'should render the create form ready to edit', - { framework: 'tanstack-start' }, - async () => { - await page.goto(postsUrl.create) - // No client-init disabled phase: the RSC payload arrives with form state - // already initialized, so the field is immediately enabled and editable. - await expect(page.locator('#field-title')).toBeEnabled() - await page.locator('#field-title').fill(title) - await expect(page.locator('#field-title')).toHaveValue(title) - }, - ) + test('should render the create form ready to edit', { framework: 'tanstack-start' }, async () => { + await page.goto(postsUrl.create) + // No client-init disabled phase: the RSC payload arrives with form state + // already initialized, so the field is immediately enabled and editable. + await expect(page.locator('#field-title')).toBeEnabled() + await page.locator('#field-title').fill(title) + await expect(page.locator('#field-title')).toHaveValue(title) + }) test('should disable fields while processing', async () => { const doc = await createPost() @@ -524,6 +520,142 @@ test.describe('Form State', () => { await expect(computedTitleField).toHaveValue('Test Title 2') }) + test('autosave - should not overwrite a field with a stale response after switching to another field mid-flight', async () => { + const doc = await payload.create({ + collection: autosavePostsSlug, + data: { + title: 'Initial Title', + }, + }) + + await page.goto(autosavePostsUrl.edit(doc.id)) + await waitForFormReady(page) + + const titleField = page.locator('#field-title') + const subtitleField = page.locator('#field-subtitle') + + let releaseFirstRequest: () => void + const firstRequestReleased = new Promise((resolve) => { + releaseFirstRequest = resolve + }) + + let resolveFirstRequestHandled: () => void + const firstRequestHandled = new Promise((resolve) => { + resolveFirstRequestHandled = resolve + }) + + let patchRequestCount = 0 + + const isAutosaveRequest = (url: URL) => url.pathname.endsWith(`/${autosavePostsSlug}/${doc.id}`) + + await page.route(isAutosaveRequest, async (route) => { + if (route.request().method() !== 'PATCH') { + await route.continue() + return + } + + patchRequestCount += 1 + + // Hold the first autosave request open, simulating a slow round-trip. This lets + // us keep editing the form (moving to a different field) while it is still in + // flight, and later resolve it with the stale, partially-typed value it was sent + // with. Autosave requests are queued and sent strictly one at a time, so a second + // request can only be dispatched (and land here) once the first one's response has + // already been merged into form state - at which point we hold it forever. This + // isolates the effect of the first (stale) response's merge from any subsequent, + // corrective autosave that would otherwise overwrite it with the fresher value and + // mask the bug. + if (patchRequestCount === 1) { + await firstRequestReleased + await route.continue() + // Signal that this route has already been continued, so the test knows it's + // safe to unroute without racing Playwright's own auto-continue-on-unroute. + resolveFirstRequestHandled() + return + } + + await new Promise(() => { + // Never resolves - intentionally holds all subsequent autosave requests forever. + }) + }) + + // Type a partial value into the title field. Once the debounce elapses, this + // fires the first (held) autosave request with the incomplete text "Hel". + await titleField.fill('Hel') + + await expect.poll(() => patchRequestCount).toBe(1) + + // Finish typing in the title field, then immediately move to another field and + // keep typing - all while the first autosave request for "Hel" is still pending. + await titleField.pressSequentially('lo') + await subtitleField.fill('World') + + // Now let the stale first request resolve. Before the fix, this reverted the + // title field back to "Hel" because switching to `subtitle` had already cleared + // `title`'s modified protection. + releaseFirstRequest() + await firstRequestHandled + + // Wait for the second (corrective) autosave request to be dispatched and held. + // Since requests are processed strictly one at a time, reaching this point + // guarantees the first response has already been merged into form state. + await expect.poll(() => patchRequestCount).toBe(2) + + await expect(titleField).toHaveValue('Hello') + await expect(subtitleField).toHaveValue('World') + + await page.unroute(isAutosaveRequest) + }) + + test('autosave - should not remount an open relationship list drawer when an unmodified field is merged', async () => { + const relatedDoc = await payload.create({ + collection: autosavePostsSlug, + data: { + title: 'Related Post', + }, + }) + + const doc = await payload.create({ + collection: autosavePostsSlug, + data: { + title: 'Initial Title', + // The relationship field must already hold a value for this to reproduce the bug: an + // empty/untouched value has nothing to get a new (but content-equal) reference on merge. + relatedPosts: [relatedDoc.id], + }, + }) + + await page.goto(autosavePostsUrl.edit(doc.id)) + await waitForFormReady(page) + + // Open the relationship field's list drawer (appearance: 'drawer') and wait for it to load. + await page.locator('#field-relatedPosts').click() + const listDrawerContent = page.locator('.list-drawer .drawer__content') + await expect(listDrawerContent).toBeVisible() + await expect(listDrawerContent.locator('table tbody tr').first()).toBeVisible() + + // Tag the currently-rendered drawer content so we can detect if it gets unmounted and + // replaced with a fresh element later - which is what a "remount" (the reported flicker) + // would look like, even if the drawer stays open and its content looks the same. + await listDrawerContent.evaluate((el) => { + el.setAttribute('data-test-remount-marker', 'still-here') + }) + + // Edit a field unrelated to the relationship field and let autosave run to completion. + // Before the fix, the relationship field's unmodified (but object/array-valued) value would + // get a brand new, content-identical reference on every accepted autosave merge, causing its + // list drawer to recompute its filter options and remount - even though nothing about the + // relationship field itself changed. + await page.locator('#field-title').fill('Updated Title') + await waitForAutoSaveToRunAndComplete(page) + + await expect(listDrawerContent).toBeVisible() + await expect(listDrawerContent).toHaveAttribute('data-test-remount-marker', 'still-here') + + await payload.delete({ collection: autosavePostsSlug, id: doc.id }) + await payload.delete({ collection: autosavePostsSlug, id: relatedDoc.id }) + }) + test('array and block rows and maintain consistent row IDs across duplication', async () => { await page.goto(postsUrl.create) await waitForFormReady(page) diff --git a/test/form-state/int.spec.ts b/test/form-state/int.spec.ts index 9776c83f380..3e1c936d5ec 100644 --- a/test/form-state/int.spec.ts +++ b/test/form-state/int.spec.ts @@ -927,6 +927,63 @@ describe('Form State', () => { }) }) + it('should preserve the existing value reference for an unmodified field when the incoming value is content-identical', () => { + /** + * The server always sends a freshly (de)serialized value, so an object/array-valued field (e.g. a + * relationship or upload field) would otherwise get a brand new reference on every autosave merge, even + * though nothing about it changed. Consumers that memoize on this value's identity (e.g. a relationship + * field's list drawer filter options) would then needlessly recompute or remount. + * + * Includes a second, actually-changed field (`title`) so the top-level "return the same object if nothing + * changed" optimization in `mergeServerFormState` doesn't short-circuit and mask whether the untouched + * `relationshipField` value's reference was itself preserved within the new state object. + */ + const currentRelationshipValue = { relationTo: 'uploads', value: '1' } + + const currentState: Record = { + title: { + value: 'Test Post', + initialValue: 'Test Post', + valid: true, + passesCondition: true, + }, + relationshipField: { + value: currentRelationshipValue, + initialValue: currentRelationshipValue, + valid: true, + passesCondition: true, + }, + } + + const incomingStateFromServer: Record = { + title: { + // Actually changed on the server, e.g. by a hook - ensures the overall state is not a no-op. + value: 'Test Post (modified on the server)', + initialValue: 'Test Post', + valid: true, + passesCondition: true, + }, + relationshipField: { + // Content-identical to `currentRelationshipValue`, but a different object reference - + // simulating a value freshly deserialized from the server response. + value: { relationTo: 'uploads', value: '1' }, + initialValue: { relationTo: 'uploads', value: '1' }, + valid: true, + passesCondition: true, + }, + } + + const newState = mergeServerFormState({ + acceptValues: { overrideLocalChanges: false }, + currentState, + incomingState: incomingStateFromServer, + }) + + expect(newState.title.value).toBe('Test Post (modified on the server)') + expect(newState.relationshipField.value).toBe(currentRelationshipValue) + expect(newState.relationshipField.initialValue).toBe(currentRelationshipValue) + }) + it('should preserve client row data after reorder and delete during autosave', () => { /** * Regression test for the "ghost item" bug. diff --git a/test/form-state/payload-types.ts b/test/form-state/payload-types.ts index 90529f6558e..48097330892 100644 --- a/test/form-state/payload-types.ts +++ b/test/form-state/payload-types.ts @@ -192,7 +192,9 @@ export interface Number { export interface AutosavePost { id: string; title?: string | null; + subtitle?: string | null; computedTitle?: string | null; + relatedPosts?: (string | AutosavePost)[] | null; updatedAt: string; createdAt: string; _status?: ('draft' | 'published') | null; @@ -377,7 +379,9 @@ export interface PostsSelect { */ export interface AutosavePostsSelect { title?: T; + subtitle?: T; computedTitle?: T; + relatedPosts?: T; updatedAt?: T; createdAt?: T; _status?: T;