From cc3ff31d5969f3153b187b708acd83f73160798a Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 2 Dec 2025 11:29:48 +0700 Subject: [PATCH] move golden retriever clear files logic (#6076) into #restore instead. we currently clear files when state transitions to all files complete, however there's an issue with that where if progress events come in after all files are marked as completed, it will overwrite the metadataStore, meaning the files that have been cleared will be re-added after they were cleared. this causes files to be restored (when e.g. refreshing the browser) when they should not (because they have already completed). i managed to reproduce this with the google drive picker plugin (but not with google drive non-picker) **Tip for review:** hide whitespace changes --- .changeset/cool-plants-compare.md | 6 + .../dashboard/src/components/Dashboard.tsx | 2 +- packages/@uppy/golden-retriever/src/index.ts | 111 ++++++++++-------- 3 files changed, 66 insertions(+), 53 deletions(-) create mode 100644 .changeset/cool-plants-compare.md diff --git a/.changeset/cool-plants-compare.md b/.changeset/cool-plants-compare.md new file mode 100644 index 000000000..08a5c946f --- /dev/null +++ b/.changeset/cool-plants-compare.md @@ -0,0 +1,6 @@ +--- +"@uppy/dashboard": patch +"@uppy/golden-retriever": patch +--- + +Move golden retriever clear files logic to the restore function. This prevents race condition bugs when storing state. diff --git a/packages/@uppy/dashboard/src/components/Dashboard.tsx b/packages/@uppy/dashboard/src/components/Dashboard.tsx index 401d48a5b..10456c924 100644 --- a/packages/@uppy/dashboard/src/components/Dashboard.tsx +++ b/packages/@uppy/dashboard/src/components/Dashboard.tsx @@ -242,7 +242,7 @@ export default function Dashboard( {showFileList && } - {numberOfFilesForRecovery && ( + {numberOfFilesForRecovery != null && numberOfFilesForRecovery > 0 && (
f.progress.complete && !f.error) + ? [] + : recoveredFiles, ) - if (Object.keys(recoveredState.files).length <= 0) { + const filesEntries = Object.entries(files) + + this.uppy.log( + `[GoldenRetriever] Recovered ${Object.keys(currentUploads).length} current uploads and ${filesEntries.length} files from Local Storage`, + ) + + const hasFiles = filesEntries.length > 0 + + if (!hasFiles) { this.uppy.log( '[GoldenRetriever] No files need to be loaded, restored only processing state...', ) - return } const [serviceWorkerBlobs, indexedDbBlobs] = await Promise.all([ @@ -122,43 +139,41 @@ export default class GoldenRetriever< UppyFileId, UppyFile > = Object.fromEntries( - Object.entries(files).map( - ([fileID, file]): [UppyFileId, UppyFile] => { - if (file.isRemote) { - return [ - fileID, + filesEntries.map(([fileID, file]): [UppyFileId, UppyFile] => { + if (file.isRemote) { + return [ + fileID, + { + ...file, + isRestored: true, + data: { size: null }, // todo shouldn't we save/restore the size too? + }, + ] + } + + const blob: Blob | undefined = blobs[fileID] + return [ + fileID, + !file.progress.uploadComplete && blob == null + ? // if we don’t have the blob (and the file is not completed uploading), mark the file as a ghost { ...file, isRestored: true, - data: { size: null }, // todo shouldn't we save/restore the size too? + isGhost: true, + data: undefined, + } + : { + ...file, + isRestored: true, + isGhost: false, + data: blob, }, - ] - } - - const blob: Blob | undefined = blobs[fileID] - return [ - fileID, - !file.progress.uploadComplete && blob == null - ? // if we don’t have the blob (and the file is not completed uploading), mark the file as a ghost - { - ...file, - isRestored: true, - isGhost: true, - data: undefined, - } - : { - ...file, - isRestored: true, - isGhost: false, - data: blob, - }, - ] - }, - ), + ] + }), ) this.uppy.setState({ - recoveredState, + recoveredState: hasFiles ? recoveredState : null, // recoveredState is used to control the UI (to show the "recovered" state), only set it if we actually have files currentUploads, files: filesWithBlobs, }) @@ -315,13 +330,6 @@ export default class GoldenRetriever< } if (nextState.files !== prevState.files) { - // If all files have completed *successfully*, remove the whole stored restoration state. - // This makes sure that if the upload was only partially successful, the user can still restore and upload the remaining files. - // Here are some scenarios we have to take into account: - // todo (make unit/e2e tests for these scenarios) - // - the user removes all uploads one by one (once all are removed, we should not restore anything after reloading page) - // - the user uploads files with Transloadit plugin enabled, uploads complete successfully, and the user refreshes the page while the assembly is still running. golden retriever should then restore the files, and the ongoing assembly should progress - // - once a file finishes uploading successfully, it should have it its blob removed (even if a post processing step remains). if not successful upload it should not be removed if ( Object.values(prevState.files).some((f) => !f.progress.complete) && (Object.values(nextState.files).length === 0 || @@ -333,18 +341,17 @@ export default class GoldenRetriever< `[GoldenRetriever] All files have been uploaded and processed successfully, clearing recovery state`, ) this.uppy.setState({ recoveredState: null }) - this.#metaDataStore.set(null) - } else { - // We don’t want to store file.data on local files, because the actual blob is too large and should therefore stored separately, - // and we want to avoid having weird properties in the serialized object (like file.preview). - const filesWithoutBlobs = Object.fromEntries( - Object.entries(nextState.files).map( - ([fileID, { data, preview, ...fileInfo }]) => [fileID, fileInfo], - ), - ) - this.#patchMetadata({ files: filesWithoutBlobs }) } + // We don’t want to store file.data on local files, because the actual blob is too large and should therefore stored separately, + // and we want to avoid having weird properties in the serialized object (like file.preview). + const filesWithoutBlobs = Object.fromEntries( + Object.entries(nextState.files).map( + ([fileID, { data, preview, ...fileInfo }]) => [fileID, fileInfo], + ), + ) + this.#patchMetadata({ files: filesWithoutBlobs }) + const addedFiles = Object.values(nextState.files).filter( (nextFile) => prevState.files[nextFile.id] == null, )