From b40c581a2799795ea85ec92ebb3c3a7abd96fd53 Mon Sep 17 00:00:00 2001 From: Jacob Fletcher Date: Wed, 2 Jul 2025 15:11:38 -0400 Subject: [PATCH] fix(ui): autosave infinite loop within document drawer (#13007) Required for #13005. Opening an autosave-enabled document within a drawer triggers an infinite loop when the root document is also autosave-enabled. This was for two reasons: 1. Autosave would run and change the `updatedAt` timestamp. This would trigger another run of autosave, and so on. The timestamp is now removed before comparison to ensure that sequential autosave runs are skipped. 2. The `dequal()` call was not being given the `.current` property off the ref object. This meant that is was never evaluate to `true` and therefore never skip unnecessary autosaves to begin with. --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1210697235723932 --- packages/ui/src/elements/Autosave/index.tsx | 50 +++++++------------ packages/ui/src/hooks/useQueues.ts | 2 +- ...lidate.ts => AutosaveWithDraftValidate.ts} | 12 ++--- test/versions/config.ts | 4 +- test/versions/e2e.spec.ts | 26 +++++----- test/versions/payload-types.ts | 14 ++++++ test/versions/seed.ts | 4 +- test/versions/slugs.ts | 2 +- 8 files changed, 59 insertions(+), 55 deletions(-) rename test/versions/collections/{AutosaveWithValidate.ts => AutosaveWithDraftValidate.ts} (57%) diff --git a/packages/ui/src/elements/Autosave/index.tsx b/packages/ui/src/elements/Autosave/index.tsx index 65b435012f..2215f7e659 100644 --- a/packages/ui/src/elements/Autosave/index.tsx +++ b/packages/ui/src/elements/Autosave/index.tsx @@ -60,9 +60,9 @@ export const Autosave: React.FC = ({ id, collection, global: globalDoc }) const { onSave: onSaveFromDocumentDrawer } = useDocumentDrawerContext() const { reportUpdate } = useDocumentEvents() - const { dispatchFields, isValid, setBackgroundProcessing, setIsValid, setSubmitted } = useForm() + const { dispatchFields, isValid, setBackgroundProcessing, setIsValid } = useForm() - const [fields] = useAllFormFields() + const [formState] = useAllFormFields() const modified = useFormModified() const submitted = useFormSubmitted() @@ -81,32 +81,27 @@ export const Autosave: React.FC = ({ id, collection, global: globalDoc }) ) const [_saving, setSaving] = useState(false) + const saving = useDeferredValue(_saving) - const debouncedFields = useDebounce(fields, interval) - const fieldRef = useRef(fields) + + const debouncedFormState = useDebounce(formState, interval) + + const formStateRef = useRef(formState) const modifiedRef = useRef(modified) const localeRef = useRef(locale) - /** - * Track the validation internally so Autosave can determine when to run queue processing again - * Helps us prevent infinite loops when the queue is processing and the form is invalid - */ - const isValidRef = useRef(isValid) // Store fields in ref so the autosave func // can always retrieve the most to date copies // after the timeout has executed - // eslint-disable-next-line react-compiler/react-compiler -- TODO: fix - fieldRef.current = fields + formStateRef.current = formState // Store modified in ref so the autosave func // can bail out if modified becomes false while // timing out during autosave - // eslint-disable-next-line react-compiler/react-compiler -- TODO: fix modifiedRef.current = modified // Store locale in ref so the autosave func // can always retrieve the most to date locale - // eslint-disable-next-line react-compiler/react-compiler -- TODO: fix localeRef.current = locale const { queueTask } = useQueues() @@ -158,14 +153,14 @@ export const Autosave: React.FC = ({ id, collection, global: globalDoc }) if (url) { if (modifiedRef.current) { - const { data, valid } = reduceFieldsToValuesWithValidation(fieldRef.current, true) + const { data, valid } = reduceFieldsToValuesWithValidation(formStateRef.current, true) data._status = 'draft' const skipSubmission = submitted && !valid && versionsConfig?.drafts && versionsConfig?.drafts?.validate - if (!skipSubmission && isValidRef.current) { + if (!skipSubmission) { let res try { @@ -250,10 +245,7 @@ export const Autosave: React.FC = ({ id, collection, global: globalDoc }) toast.error(err.message || i18n.t('error:unknown')) }) - // Set valid to false internally so the queue doesn't process - isValidRef.current = false setIsValid(false) - setSubmitted(true) hideIndicator() return } @@ -264,9 +256,6 @@ export const Autosave: React.FC = ({ id, collection, global: globalDoc }) // Manually update the data since this function doesn't fire the `submit` function from useForm if (document) { setIsValid(true) - - // Reset internal state allowing the queue to process - isValidRef.current = true updateSavedDocumentData(document) } } @@ -282,11 +271,6 @@ export const Autosave: React.FC = ({ id, collection, global: globalDoc }) setBackgroundProcessing(false) }, beforeProcess: () => { - if (!isValidRef.current) { - isValidRef.current = true - return false - } - setBackgroundProcessing(true) }, }, @@ -294,7 +278,8 @@ export const Autosave: React.FC = ({ id, collection, global: globalDoc }) }) const didMount = useRef(false) - const previousDebouncedFieldValues = useRef(reduceFieldsToValues(debouncedFields)) + const previousDebouncedData = useRef(reduceFieldsToValues(debouncedFormState)) + // When debounced fields change, autosave useEffect(() => { /** @@ -307,16 +292,19 @@ export const Autosave: React.FC = ({ id, collection, global: globalDoc }) /** * Ensure autosave only runs if the form data changes, not every time the entire form state changes + * Remove `updatedAt` from comparison as it changes on every autosave interval. */ - const debouncedFieldValues = reduceFieldsToValues(debouncedFields) - if (dequal(debouncedFieldValues, previousDebouncedFieldValues)) { + const { updatedAt: _, ...formData } = reduceFieldsToValues(debouncedFormState) + const { updatedAt: __, ...prevFormData } = previousDebouncedData.current + + if (dequal(formData, prevFormData)) { return } - previousDebouncedFieldValues.current = debouncedFieldValues + previousDebouncedData.current = formData handleAutosave() - }, [debouncedFields]) + }, [debouncedFormState]) /** * If component unmounts, clear the autosave timeout diff --git a/packages/ui/src/hooks/useQueues.ts b/packages/ui/src/hooks/useQueues.ts index 5dcaf849eb..853f789c64 100644 --- a/packages/ui/src/hooks/useQueues.ts +++ b/packages/ui/src/hooks/useQueues.ts @@ -14,7 +14,7 @@ type QueuedTaskOptions = { * Can also be used to perform side effects before processing the queue * @returns {boolean} If `false`, the queue will not process */ - beforeProcess?: () => boolean + beforeProcess?: () => boolean | void } type QueueTask = (fn: QueuedFunction, options?: QueuedTaskOptions) => void diff --git a/test/versions/collections/AutosaveWithValidate.ts b/test/versions/collections/AutosaveWithDraftValidate.ts similarity index 57% rename from test/versions/collections/AutosaveWithValidate.ts rename to test/versions/collections/AutosaveWithDraftValidate.ts index f412a75e36..3869d3048b 100644 --- a/test/versions/collections/AutosaveWithValidate.ts +++ b/test/versions/collections/AutosaveWithDraftValidate.ts @@ -1,12 +1,12 @@ import type { CollectionConfig } from 'payload' -import { autosaveWithValidateCollectionSlug } from '../slugs.js' +import { autosaveWithDraftValidateSlug } from '../slugs.js' -const AutosaveWithValidatePosts: CollectionConfig = { - slug: autosaveWithValidateCollectionSlug, +const AutosaveWithDraftValidate: CollectionConfig = { + slug: autosaveWithDraftValidateSlug, labels: { - singular: 'Autosave with Validate Post', - plural: 'Autosave with Validate Posts', + singular: 'Autosave with Draft Validate', + plural: 'Autosave with Draft Validate', }, admin: { useAsTitle: 'title', @@ -30,4 +30,4 @@ const AutosaveWithValidatePosts: CollectionConfig = { ], } -export default AutosaveWithValidatePosts +export default AutosaveWithDraftValidate diff --git a/test/versions/config.ts b/test/versions/config.ts index 665abff203..daa998f572 100644 --- a/test/versions/config.ts +++ b/test/versions/config.ts @@ -5,7 +5,7 @@ const dirname = path.dirname(filename) import { buildConfigWithDefaults } from '../buildConfigWithDefaults.js' import AutosavePosts from './collections/Autosave.js' import AutosaveWithDraftButtonPosts from './collections/AutosaveWithDraftButton.js' -import AutosaveWithValidate from './collections/AutosaveWithValidate.js' +import AutosaveWithDraftValidate from './collections/AutosaveWithDraftValidate.js' import CustomIDs from './collections/CustomIDs.js' import { Diff } from './collections/Diff/index.js' import DisablePublish from './collections/DisablePublish.js' @@ -41,7 +41,7 @@ export default buildConfigWithDefaults({ Posts, AutosavePosts, AutosaveWithDraftButtonPosts, - AutosaveWithValidate, + AutosaveWithDraftValidate, DraftPosts, DraftWithMax, DraftsWithValidate, diff --git a/test/versions/e2e.spec.ts b/test/versions/e2e.spec.ts index 3887931fb1..25ee858c13 100644 --- a/test/versions/e2e.spec.ts +++ b/test/versions/e2e.spec.ts @@ -38,6 +38,7 @@ import { exactText, initPageConsoleErrorCatch, saveDocAndAssert, + // throttleTest, } from '../helpers.js' import { AdminUrlUtil } from '../helpers/adminUrlUtil.js' import { assertNetworkRequests } from '../helpers/e2e/assertNetworkRequests.js' @@ -50,7 +51,7 @@ import { autoSaveGlobalSlug, autosaveWithDraftButtonGlobal, autosaveWithDraftButtonSlug, - autosaveWithValidateCollectionSlug, + autosaveWithDraftValidateSlug, customIDSlug, diffCollectionSlug, disablePublishGlobalSlug, @@ -82,7 +83,7 @@ describe('Versions', () => { let serverURL: string let autosaveURL: AdminUrlUtil let autosaveWithDraftButtonURL: AdminUrlUtil - let autosaveWithValidateURL: AdminUrlUtil + let autosaveWithDraftValidateURL: AdminUrlUtil let draftWithValidateURL: AdminUrlUtil let disablePublishURL: AdminUrlUtil let customIDURL: AdminUrlUtil @@ -103,11 +104,12 @@ describe('Versions', () => { }) beforeEach(async () => { - /* await throttleTest({ - page, - context, - delay: 'Slow 4G', - }) */ + // await throttleTest({ + // page, + // context, + // delay: 'Fast 4G', + // }) + await reInitializeDB({ serverURL, snapshotKey: 'versionsTest', @@ -121,7 +123,7 @@ describe('Versions', () => { url = new AdminUrlUtil(serverURL, draftCollectionSlug) autosaveURL = new AdminUrlUtil(serverURL, autosaveCollectionSlug) autosaveWithDraftButtonURL = new AdminUrlUtil(serverURL, autosaveWithDraftButtonSlug) - autosaveWithValidateURL = new AdminUrlUtil(serverURL, autosaveWithValidateCollectionSlug) + autosaveWithDraftValidateURL = new AdminUrlUtil(serverURL, autosaveWithDraftValidateSlug) disablePublishURL = new AdminUrlUtil(serverURL, disablePublishSlug) customIDURL = new AdminUrlUtil(serverURL, customIDSlug) postURL = new AdminUrlUtil(serverURL, postCollectionSlug) @@ -1059,7 +1061,7 @@ describe('Versions', () => { describe('Collections with draft validation', () => { beforeAll(() => { - autosaveWithValidateURL = new AdminUrlUtil(serverURL, autosaveWithValidateCollectionSlug) + autosaveWithDraftValidateURL = new AdminUrlUtil(serverURL, autosaveWithDraftValidateSlug) draftWithValidateURL = new AdminUrlUtil(serverURL, draftWithValidateCollectionSlug) }) @@ -1173,7 +1175,7 @@ describe('Versions', () => { }) test('- with autosave - can save', async () => { - await page.goto(autosaveWithValidateURL.create) + await page.goto(autosaveWithDraftValidateURL.create) const titleField = page.locator('#field-title') await titleField.fill('Initial') @@ -1191,7 +1193,7 @@ describe('Versions', () => { test('- with autosave - can safely trigger validation errors and then continue editing', async () => { // This test has to make sure we don't enter an infinite loop when draft.validate is on and we have autosave enabled - await page.goto(autosaveWithValidateURL.create) + await page.goto(autosaveWithDraftValidateURL.create) const titleField = page.locator('#field-title') await titleField.fill('Initial') @@ -1213,7 +1215,7 @@ describe('Versions', () => { }) test('- with autosave - shows a prevent leave alert when form is submitted but invalid', async () => { - await page.goto(autosaveWithValidateURL.create) + await page.goto(autosaveWithDraftValidateURL.create) // Flag to check against if window alert has been displayed and dismissed since we can only check via events let alertDisplayed = false diff --git a/test/versions/payload-types.ts b/test/versions/payload-types.ts index d5b3d5fc35..3172effc09 100644 --- a/test/versions/payload-types.ts +++ b/test/versions/payload-types.ts @@ -536,6 +536,13 @@ export interface User { hash?: string | null; loginAttempts?: number | null; lockUntil?: string | null; + sessions?: + | { + id: string; + createdAt?: string | null; + expiresAt: string; + }[] + | null; password?: string | null; } /** @@ -1049,6 +1056,13 @@ export interface UsersSelect { hash?: T; loginAttempts?: T; lockUntil?: T; + sessions?: + | T + | { + id?: T; + createdAt?: T; + expiresAt?: T; + }; } /** * This interface was referenced by `Config`'s JSON-Schema diff --git a/test/versions/seed.ts b/test/versions/seed.ts index 9a7d921f7e..7cbc9463a4 100644 --- a/test/versions/seed.ts +++ b/test/versions/seed.ts @@ -8,7 +8,7 @@ import { devUser } from '../credentials.js' import { executePromises } from '../helpers/executePromises.js' import { generateLexicalData } from './collections/Diff/generateLexicalData.js' import { - autosaveWithValidateCollectionSlug, + autosaveWithDraftValidateSlug, diffCollectionSlug, draftCollectionSlug, media2CollectionSlug, @@ -141,7 +141,7 @@ export async function seed(_payload: Payload, parallel: boolean = false) { }) await _payload.create({ - collection: autosaveWithValidateCollectionSlug, + collection: autosaveWithDraftValidateSlug, data: { title: 'Initial seeded title', }, diff --git a/test/versions/slugs.ts b/test/versions/slugs.ts index e8dac96bb5..d460444a94 100644 --- a/test/versions/slugs.ts +++ b/test/versions/slugs.ts @@ -2,7 +2,7 @@ export const autosaveCollectionSlug = 'autosave-posts' export const autosaveWithDraftButtonSlug = 'autosave-with-draft-button-posts' -export const autosaveWithValidateCollectionSlug = 'autosave-with-validate-posts' +export const autosaveWithDraftValidateSlug = 'autosave-with-validate-posts' export const customIDSlug = 'custom-ids'