Move completed uploads exclusion logic (#6058)

into uploader plugins

fixes #6051

also [fix deprecated usage of done
callback](2511142276)

.changeset/polite-cougars-itch.md

@@ -0,0 +1,9 @@
+---
+"@uppy/aws-s3": patch
+"@uppy/core": patch
+"@uppy/tus": patch
+"@uppy/utils": patch
+"@uppy/xhr-upload": patch
+---
+
+Fix: Move completed uploads exclusion logic into uploaders. This fixes the problem where postprocessors would not run for already uploaded files.

packages/@uppy/aws-s3/src/index.ts

@@ -16,7 +16,7 @@ import type {
 import {
   createAbortError,
   filterFilesToEmitUploadStarted,
-  filterNonFailedFiles,
+  filterFilesToUpload,
   getAllowedMetaFields,
   RateLimitedQueue,
 } from '@uppy/utils'
@@ -938,7 +938,7 @@ export default class AwsS3Multipart<
     if (fileIDs.length === 0) return undefined
 
     const files = this.uppy.getFilesByIds(fileIDs)
-    const filesFiltered = filterNonFailedFiles(files)
+    const filesFiltered = filterFilesToUpload(files)
     const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)
 
     this.uppy.emit('upload-start', filesToEmit)

packages/@uppy/core/src/Uppy.test.ts

@@ -2609,7 +2609,7 @@ describe('src/Core', () => {
       expect(infoVisibleEvent.mock.calls.length).toEqual(1)
     })
 
-    it('should set an info message to be displayed for a period of time before hiding', (done) => {
+    it('should set an info message to be displayed for a period of time before hiding', async () => {
       const infoVisibleEvent = vi.fn()
       const infoHiddenEvent = vi.fn()
       const core = new Core()
@@ -2618,12 +2618,10 @@ describe('src/Core', () => {
 
       core.info('This is the message', 'info', 100)
       expect(infoHiddenEvent.mock.calls.length).toEqual(0)
-      setTimeout(() => {
-        expect(infoHiddenEvent.mock.calls.length).toEqual(1)
-        expect(core.getState().info).toEqual([])
-        // @ts-ignore
-        done()
-      }, 110)
+
+      await new Promise((resolve) => setTimeout(resolve, 110))
+      expect(infoHiddenEvent.mock.calls.length).toEqual(1)
+      expect(core.getState().info).toEqual([])
     })
 
     it('should hide an info message', () => {

packages/@uppy/core/src/Uppy.ts

@@ -2233,9 +2233,6 @@ export class Uppy<
     ]
     try {
       for (let step = currentUpload.step || 0; step < steps.length; step++) {
-        if (!currentUpload) {
-          break
-        }
         const fn = steps[step]
 
         this.setState({
@@ -2248,13 +2245,7 @@ export class Uppy<
           },
         })
 
-        // when restoring (e.g. using golden retriever), we don't need to re-upload already successfully uploaded files
-        // so let's exclude them here:
-        // https://github.com/transloadit/uppy/issues/5930
-        const fileIDs = currentUpload.fileIDs.filter((fileID) => {
-          const file = this.getFile(fileID)
-          return !file.progress.uploadComplete
-        })
+        const { fileIDs } = currentUpload
 
         // TODO give this the `updatedUpload` object as its only parameter maybe?
         // Otherwise when more metadata may be added to the upload this would keep getting more parameters
@@ -2262,6 +2253,9 @@ export class Uppy<
 
         // Update currentUpload value in case it was modified asynchronously.
         currentUpload = getCurrentUpload()
+        if (!currentUpload) {
+          break
+        }
       }
     } catch (err) {
       this.#removeUpload(uploadID)

packages/@uppy/golden-retriever/src/goldenRetriever.browser.test.ts

@@ -20,7 +20,9 @@ function createUppy(
 
   const root = document.createElement('div')
   document.body.appendChild(root)
-  return new Uppy().use(Dashboard, {
+  return new Uppy({
+    debug: true,
+  }).use(Dashboard, {
     target: root,
     inline: true,
     ...opts,
@@ -36,9 +38,11 @@ beforeEach(async () => {
   document.body.innerHTML = ''
 })
 
+let fileIndex = 0
+
 const createMockFile = ({
   size,
-  name = `${Date.now()}.txt`,
+  name = `${fileIndex++}.txt`,
   type = 'text/plain',
 }: {
   name?: string
@@ -99,7 +103,9 @@ describe('Golden retriever', () => {
           requestAt += 1
           return HttpResponse.json({})
         }
-        // never reply to subsequent requests (leave them hanging)
+        // fail subsequent requests
+        requestAt += 1
+        return HttpResponse.json({}, { status: 400 })
       }),
     )
 
@@ -117,10 +123,15 @@ describe('Golden retriever', () => {
       uppy.once('upload-success', () => resolve()),
     )
 
+    const uploadCompletePromise = new Promise<void>((resolve) =>
+      uppy.once('complete', () => resolve()),
+    )
+
     // Start the upload
     await page.getByRole('button', { name: 'Upload 2 files' }).click()
 
     await uploadFirstFileCompletePromise
+    await uploadCompletePromise
 
     // reload page and recreate Uppy instance
     uppy = createUppy({ withPageReload: true })
@@ -140,7 +151,8 @@ describe('Golden retriever', () => {
           return HttpResponse.json({})
         }
         // don't allow more than 1 request
-        return new HttpResponse({ status: 400 })
+        requestAt += 1
+        return HttpResponse.json({}, { status: 400 })
       }),
     )
 
@@ -152,6 +164,7 @@ describe('Golden retriever', () => {
       .toBeVisible()
 
     expect(uppy.getFiles().length).toBe(2)
+    expect(requestAt).toBe(1) // only the first file upload should have happened so far
   })
 
   test('Should not clean up files upon completion if there were failed uploads and it should only make the failed file a ghost', async ({

packages/@uppy/tus/src/index.ts

@@ -10,7 +10,7 @@ import type {
 import { BasePlugin, EventManager } from '@uppy/core'
 import {
   filterFilesToEmitUploadStarted,
-  filterNonFailedFiles,
+  filterFilesToUpload,
   getAllowedMetaFields,
   hasProperty,
   isNetworkError,
@@ -563,7 +563,7 @@ export default class Tus<M extends Meta, B extends Body> extends BasePlugin<
   }
 
   async #uploadFiles(files: UppyFile<M, B>[]) {
-    const filesFiltered = filterNonFailedFiles(files)
+    const filesFiltered = filterFilesToUpload(files)
     const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)
     this.uppy.emit('upload-start', filesToEmit)
 

packages/@uppy/utils/src/fileFilters.ts

@@ -1,12 +1,17 @@
 import type { UppyFile } from './UppyFile.js'
 
-export function filterNonFailedFiles(
+const hasError = (file: UppyFile<any, any>) => 'error' in file && !!file.error
+
+// We don't need to re-upload already successfully uploaded files
+// so let's exclude them here:
+// https://github.com/transloadit/uppy/issues/5930
+// This happens for example when restoring a partially finished session (e.g. using golden retriever).
+const isCompleted = (file: UppyFile<any, any>) => file.progress.uploadComplete
+
+export function filterFilesToUpload(
   files: UppyFile<any, any>[],
 ): UppyFile<any, any>[] {
-  const hasError = (file: UppyFile<any, any>): boolean =>
-    'error' in file && !!file.error
-
-  return files.filter((file) => !hasError(file))
+  return files.filter((file) => !hasError(file) && !isCompleted(file))
 }
 
 // Don't double-emit upload-started for Golden Retriever-restored files that were already started

packages/@uppy/utils/src/index.ts

@@ -37,7 +37,7 @@ export { default as fetchWithNetworkError } from './fetchWithNetworkError.js'
 
 export {
   filterFilesToEmitUploadStarted,
-  filterNonFailedFiles,
+  filterFilesToUpload,
 } from './fileFilters.js'
 
 export { default as findAllDOMElements } from './findAllDOMElements.js'

packages/@uppy/xhr-upload/src/index.ts

@@ -13,7 +13,7 @@ import {
   type FetcherOptions,
   fetcher,
   filterFilesToEmitUploadStarted,
-  filterNonFailedFiles,
+  filterFilesToUpload,
   getAllowedMetaFields,
   internalRateLimitedQueue,
   isNetworkError,
@@ -535,7 +535,7 @@ export default class XHRUpload<
     this.uppy.log('[XHRUpload] Uploading...')
     const files = this.uppy.getFilesByIds(fileIDs)
 
-    const filesFiltered = filterNonFailedFiles(files)
+    const filesFiltered = filterFilesToUpload(files)
     const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)
     this.uppy.emit('upload-start', filesToEmit)