@uppy/provider-views: fix race conditions with folder loading (#4578)

* fix multiple race conditions multiple race conditions / bugs: - If you open a folder in e.g. facebook and you scroll down, triggering the infinity scroll, and then go back (one folder up) before the infinity scroll has finished loading, you could end up with the contents of the scrolled folder instead of the previous folder - if you select a folder structure (e.g. deep structure with many files) and then click "cancel" while loading, it will still continue loading recursively in the background even after cancelled and potentially cause more issues down the road * Apply suggestions from code review Co-authored-by: Antoine du Hamel <antoine@transloadit.com> * merge conditions --------- Co-authored-by: Antoine du Hamel <antoine@transloadit.com> Co-authored-by: Murderlon <merlijn@soverin.net>
2026-07-24 10:47:44 +00:00 · 2023-07-20 15:20:48 +02:00 · 2023-07-20 15:20:48 +02:00 · ecdecd1dd6
commit ecdecd1dd6
parent 49cf48d541
3 changed files with 146 additions and 122 deletions
--- a/packages/@uppy/companion-client/src/Provider.js
+++ b/packages/@uppy/companion-client/src/Provider.js
@ -135,8 +135,8 @@ export default class Provider extends RequestClient {
    return this.get(`${this.id}/list/${directory || ''}`, options)
  }

-  async logout () {
-    const response = await this.get(`${this.id}/logout`)
+  async logout (options) {
+    const response = await this.get(`${this.id}/logout`, options)
    await this.#removeAuthToken()
    return response
  }
--- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx
+++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx
@ -95,6 +95,29 @@ export default class ProviderView extends View {
    // Nothing.
  }

+  #abortController
+
+  async #withAbort (op) {
+    // prevent multiple requests in parallel from causing race conditions
+    this.#abortController?.abort()
+    const abortController = new AbortController()
+    this.#abortController = abortController
+    const cancelRequest = () => {
+      abortController.abort()
+      this.clearSelection()
+    }
+    try {
+      this.plugin.uppy.on('dashboard:close-panel', cancelRequest)
+      this.plugin.uppy.on('cancel-all', cancelRequest)
+
+      await op(abortController.signal)
+    } finally {
+      this.plugin.uppy.off('dashboard:close-panel', cancelRequest)
+      this.plugin.uppy.off('cancel-all', cancelRequest)
+      this.#abortController = undefined
+    }
+  }
+
  async #list ({ requestPath, absDirPath, signal }) {
    const { username, nextPagePath, items } = await this.provider.list(requestPath, { signal })
    this.username = username || this.username
@ -139,58 +162,45 @@ export default class ProviderView extends View {
   * @returns {Promise}   Folders/files in folder
   */
  async getFolder (requestPath, name) {
-    const controller = new AbortController()
-    const cancelRequest = () => {
-      controller.abort()
-      this.clearSelection()
-    }
-
-    this.plugin.uppy.on('dashboard:close-panel', cancelRequest)
-    this.plugin.uppy.on('cancel-all', cancelRequest)
-
    this.setLoading(true)
    try {
-      this.lastCheckbox = undefined
+      await this.#withAbort(async (signal) => {
+        this.lastCheckbox = undefined

-      let { breadcrumbs } = this.plugin.getPluginState()
+        let { breadcrumbs } = this.plugin.getPluginState()

-      const index = breadcrumbs.findIndex((dir) => requestPath === dir.requestPath)
+        const index = breadcrumbs.findIndex((dir) => requestPath === dir.requestPath)

-      if (index !== -1) {
-        // means we navigated back to a known directory (already in the stack), so cut the stack off there
-        breadcrumbs = breadcrumbs.slice(0, index + 1)
-      } else {
-        // we have navigated into a new (unknown) folder, add it to the stack
-        breadcrumbs = [...breadcrumbs, { requestPath, name }]
-      }
+        if (index !== -1) {
+          // means we navigated back to a known directory (already in the stack), so cut the stack off there
+          breadcrumbs = breadcrumbs.slice(0, index + 1)
+        } else {
+          // we have navigated into a new (unknown) folder, add it to the stack
+          breadcrumbs = [...breadcrumbs, { requestPath, name }]
+        }

-      this.nextPagePath = requestPath
-      let files = []
-      let folders = []
-      do {
-        const { files: newFiles, folders: newFolders } = await this.#listFilesAndFolders({
-          breadcrumbs, signal: controller.signal,
-        })
+        this.nextPagePath = requestPath
+        let files = []
+        let folders = []
+        do {
+          const { files: newFiles, folders: newFolders } = await this.#listFilesAndFolders({
+            breadcrumbs, signal,
+          })

-        files = files.concat(newFiles)
-        folders = folders.concat(newFolders)
+          files = files.concat(newFiles)
+          folders = folders.concat(newFolders)

-        this.setLoading(this.plugin.uppy.i18n('loadedXFiles', { numFiles: files.length + folders.length }))
-      } while (
-        this.opts.loadAllFiles && this.nextPagePath
-      )
+          this.setLoading(this.plugin.uppy.i18n('loadedXFiles', { numFiles: files.length + folders.length }))
+        } while (
+          this.opts.loadAllFiles && this.nextPagePath
+        )

-      this.plugin.setPluginState({ folders, files, breadcrumbs, filterInput: '' })
+        this.plugin.setPluginState({ folders, files, breadcrumbs, filterInput: '' })
+      })
    } catch (err) {
-      if (err.cause?.name === 'AbortError') {
-        // Expected, user clicked “cancel”
-        return
-      }
      this.handleError(err)
    } finally {
      this.setLoading(false)
-      this.plugin.uppy.off('dashboard:close-panel', cancelRequest)
-      this.plugin.uppy.off('cancel-all', cancelRequest)
    }
  }

@ -207,9 +217,10 @@ export default class ProviderView extends View {
  /**
   * Removes session token on client side.
   */
-  logout () {
-    this.provider.logout()
-      .then((res) => {
+  async logout () {
+    try {
+      await this.#withAbort(async (signal) => {
+        const res = await this.provider.logout({ signal })
        if (res.ok) {
          if (!res.revoked) {
            const message = this.plugin.uppy.i18n('companionUnauthorizeHint', {
@ -228,7 +239,10 @@ export default class ProviderView extends View {
          }
          this.plugin.setPluginState(newState)
        }
-      }).catch(this.handleError)
+      })
+    } catch (err) {
+      this.handleError(err)
+    }
  }

  filterQuery (input) {
@ -286,14 +300,18 @@ export default class ProviderView extends View {
      this.isHandlingScroll = true

      try {
-        const { files, folders, breadcrumbs } = this.plugin.getPluginState()
+        await this.#withAbort(async (signal) => {
+          const { files, folders, breadcrumbs } = this.plugin.getPluginState()

-        const { files: newFiles, folders: newFolders } = await this.#listFilesAndFolders({ breadcrumbs })
+          const { files: newFiles, folders: newFolders } = await this.#listFilesAndFolders({
+            breadcrumbs, signal,
+          })

-        const combinedFiles = files.concat(newFiles)
-        const combinedFolders = folders.concat(newFolders)
+          const combinedFiles = files.concat(newFiles)
+          const combinedFolders = folders.concat(newFolders)

-        this.plugin.setPluginState({ folders: combinedFolders, files: combinedFiles })
+          this.plugin.setPluginState({ folders: combinedFolders, files: combinedFiles })
+        })
      } catch (error) {
        this.handleError(error)
      } finally {
@ -302,11 +320,11 @@ export default class ProviderView extends View {
    }
  }

-  async #recursivelyListAllFiles ({ requestPath, absDirPath, relDirPath, queue, onFiles }) {
+  async #recursivelyListAllFiles ({ requestPath, absDirPath, relDirPath, queue, onFiles, signal }) {
    let curPath = requestPath

    while (curPath) {
-      const res = await this.#list({ requestPath: curPath, absDirPath })
+      const res = await this.#list({ requestPath: curPath, absDirPath, signal })
      curPath = res.nextPagePath

      const files = res.items.filter((item) => !item.isFolder)
@ -322,6 +340,7 @@ export default class ProviderView extends View {
          relDirPath: prependPath(relDirPath, folder.name),
          queue,
          onFiles,
+          signal,
        })
      )))
      await Promise.all(promises) // in case we get an error
@ -331,87 +350,90 @@ export default class ProviderView extends View {
  async donePicking () {
    this.setLoading(true)
    try {
-      const { currentSelection } = this.plugin.getPluginState()
+      await this.#withAbort(async (signal) => {
+        const { currentSelection } = this.plugin.getPluginState()

-      const messages = []
-      const newFiles = []
+        const messages = []
+        const newFiles = []

-      for (const selectedItem of currentSelection) {
-        const { requestPath } = selectedItem
+        for (const selectedItem of currentSelection) {
+          const { requestPath } = selectedItem

-        const withRelDirPath = (newItem) => ({
-          ...newItem,
-          // calculate the file's path relative to the user's selected item's path
-          // see https://github.com/transloadit/uppy/pull/4537#issuecomment-1614236655
-          relDirPath: newItem.absDirPath.replace(selectedItem.absDirPath, '').replace(/^\//, ''),
-        })
-
-        if (selectedItem.isFolder) {
-          let isEmpty = true
-          let numNewFiles = 0
-
-          const queue = new PQueue({ concurrency: 6 })
-
-          const onFiles = (files) => {
-            for (const newFile of files) {
-              const tagFile = this.getTagFile(newFile)
-              const id = getSafeFileId(tagFile)
-              // If the same folder is added again, we don't want to send
-              // X amount of duplicate file notifications, we want to say
-              // the folder was already added. This checks if all files are duplicate,
-              // if that's the case, we don't add the files.
-              if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) {
-                newFiles.push(withRelDirPath(newFile))
-                numNewFiles++
-                this.setLoading(this.plugin.uppy.i18n('addedNumFiles', { numFiles: numNewFiles }))
-              }
-              isEmpty = false
-            }
-          }
-
-          await this.#recursivelyListAllFiles({
-            requestPath,
-            absDirPath: prependPath(selectedItem.absDirPath, selectedItem.name),
-            relDirPath: selectedItem.name,
-            queue,
-            onFiles,
+          const withRelDirPath = (newItem) => ({
+            ...newItem,
+            // calculate the file's path relative to the user's selected item's path
+            // see https://github.com/transloadit/uppy/pull/4537#issuecomment-1614236655
+            relDirPath: newItem.absDirPath.replace(selectedItem.absDirPath, '').replace(/^\//, ''),
          })
-          await queue.onIdle()

-          let message
-          if (isEmpty) {
-            message = this.plugin.uppy.i18n('emptyFolderAdded')
-          } else if (numNewFiles === 0) {
-            message = this.plugin.uppy.i18n('folderAlreadyAdded', {
-              folder: selectedItem.name,
+          if (selectedItem.isFolder) {
+            let isEmpty = true
+            let numNewFiles = 0
+
+            const queue = new PQueue({ concurrency: 6 })
+
+            const onFiles = (files) => {
+              for (const newFile of files) {
+                const tagFile = this.getTagFile(newFile)
+                const id = getSafeFileId(tagFile)
+                // If the same folder is added again, we don't want to send
+                // X amount of duplicate file notifications, we want to say
+                // the folder was already added. This checks if all files are duplicate,
+                // if that's the case, we don't add the files.
+                if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) {
+                  newFiles.push(withRelDirPath(newFile))
+                  numNewFiles++
+                  this.setLoading(this.plugin.uppy.i18n('addedNumFiles', { numFiles: numNewFiles }))
+                }
+                isEmpty = false
+              }
+            }
+
+            await this.#recursivelyListAllFiles({
+              requestPath,
+              absDirPath: prependPath(selectedItem.absDirPath, selectedItem.name),
+              relDirPath: selectedItem.name,
+              queue,
+              onFiles,
+              signal,
            })
+            await queue.onIdle()
+
+            let message
+            if (isEmpty) {
+              message = this.plugin.uppy.i18n('emptyFolderAdded')
+            } else if (numNewFiles === 0) {
+              message = this.plugin.uppy.i18n('folderAlreadyAdded', {
+                folder: selectedItem.name,
+              })
+            } else {
+              // TODO we don't really know at this point whether any files were actually added
+              // (only later after addFiles has been called) so we should probably rewrite this.
+              // Example: If all files fail to add due to restriction error, it will still say "Added 100 files from folder"
+              message = this.plugin.uppy.i18n('folderAdded', {
+                smart_count: numNewFiles, folder: selectedItem.name,
+              })
+            }
+
+            messages.push(message)
          } else {
-            // TODO we don't really know at this point whether any files were actually added
-            // (only later after addFiles has been called) so we should probably rewrite this.
-            // Example: If all files fail to add due to restriction error, it will still say "Added 100 files from folder"
-            message = this.plugin.uppy.i18n('folderAdded', {
-              smart_count: numNewFiles, folder: selectedItem.name,
-            })
+            newFiles.push(withRelDirPath(selectedItem))
          }
-
-          messages.push(message)
-        } else {
-          newFiles.push(withRelDirPath(selectedItem))
        }
-      }

-      // Note: this.plugin.uppy.addFiles must be only run once we are done fetching all files,
-      // because it will cause the loading screen to disappear,
-      // and that will allow the user to start the upload, so we need to make sure we have
-      // finished all async operations before we add any file
-      // see https://github.com/transloadit/uppy/pull/4384
-      this.plugin.uppy.log('Adding remote provider files')
-      this.plugin.uppy.addFiles(newFiles.map((file) => this.getTagFile(file)))
+        // Note: this.plugin.uppy.addFiles must be only run once we are done fetching all files,
+        // because it will cause the loading screen to disappear,
+        // and that will allow the user to start the upload, so we need to make sure we have
+        // finished all async operations before we add any file
+        // see https://github.com/transloadit/uppy/pull/4384
+        this.plugin.uppy.log('Adding remote provider files')
+        this.plugin.uppy.addFiles(newFiles.map((file) => this.getTagFile(file)))

-      this.plugin.setPluginState({ filterInput: '' })
-      messages.forEach(message => this.plugin.uppy.info(message))
+        this.plugin.setPluginState({ filterInput: '' })
+        messages.forEach(message => this.plugin.uppy.info(message))

-      this.clearSelection()
+        this.clearSelection()
+      })
    } catch (err) {
      this.handleError(err)
    } finally {
--- a/packages/@uppy/provider-views/src/View.js
+++ b/packages/@uppy/provider-views/src/View.js
@ -48,7 +48,9 @@ export default class View {

    uppy.log(error.toString())

-    if (error.isAuthError) {
+    if (error.isAuthError || error.cause?.name === 'AbortError') {
+      // authError just means we're not authenticated, don't show to user
+      // AbortError means the user has clicked "cancel" on an operation
      return
    }