Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensures autosaves run sequentially rather than ever in parallel #9809

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions packages/ui/src/elements/Autosave/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import type { ClientCollectionConfig, ClientGlobalConfig } from 'payload'

import { versionDefaults } from 'payload/shared'
import React, { useEffect, useRef, useState } from 'react'
import React, { useCallback, useEffect, useRef, useState } from 'react'
import { toast } from 'sonner'

import {
Expand Down Expand Up @@ -64,6 +64,9 @@ export const Autosave: React.FC<Props> = ({ id, collection, global: globalDoc })
interval = versionsConfig.drafts.autosave.interval
}

const [currentAutosave, setCurrentAutosave] = useState<(() => Promise<void>) | null>(null)
const [isProcessing, setIsProcessing] = useState(false)

const [saving, setSaving] = useState(false)
const debouncedFields = useDebounce(fields, interval)
const modified = useDebounce(formModified, interval)
Expand All @@ -88,6 +91,29 @@ export const Autosave: React.FC<Props> = ({ id, collection, global: globalDoc })
// can always retrieve the most to date locale
localeRef.current = locale

const processCurrentAutosave = useCallback(async () => {
if (!currentAutosave || isProcessing) {
return
}

setIsProcessing(true)

try {
await currentAutosave()
} catch (error) {
console.error('Error autosaving:', error)
} finally {
setCurrentAutosave(null)
setIsProcessing(false)
}
}, [currentAutosave, isProcessing])

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this without a useEffect? Seems like it should be possible

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to learn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it cant, I hate it but will live

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just call processCurrentAutosave and have it take the autosave function as an arg and leave everything else as it is? Removing the autosave state and the useEffect?

if (!isProcessing) {
void processCurrentAutosave()
}
}, [currentAutosave, isProcessing, processCurrentAutosave])

// When debounced fields change, autosave
useIgnoredEffect(
() => {
Expand All @@ -97,7 +123,7 @@ export const Autosave: React.FC<Props> = ({ id, collection, global: globalDoc })
let startTimestamp = undefined
let endTimestamp = undefined

const autosave = () => {
const autosave = async () => {
if (modified) {
startTimestamp = new Date().getTime()

Expand Down Expand Up @@ -129,7 +155,7 @@ export const Autosave: React.FC<Props> = ({ id, collection, global: globalDoc })
submitted && !valid && versionsConfig?.drafts && versionsConfig?.drafts?.validate

if (!skipSubmission) {
void fetch(url, {
await fetch(url, {
body: JSON.stringify(data),
credentials: 'include',
headers: {
Expand Down Expand Up @@ -229,7 +255,7 @@ export const Autosave: React.FC<Props> = ({ id, collection, global: globalDoc })
}
}

void autosave()
setCurrentAutosave(() => autosave)

return () => {
if (autosaveTimeout) {
Expand Down
Loading