Merge pull request #10763 from owncloud/batch-delete-perf

fix: flickering loading indicator on batch delete
owncloud · Apr 18, 2024 · d8532e6 · d8532e6
2 parents 4a05abb + 9584ab2
commit d8532e6
Show file tree

Hide file tree

Showing 3 changed files with 100 additions and 91 deletions.
diff --git a/changelog/unreleased/bugfix-flickering-loading-indicator b/changelog/unreleased/bugfix-flickering-loading-indicator
@@ -0,0 +1,7 @@
+Bugfix: Flickering loading indicator
+
+The flickering loading indicator when batch deleting a lot of files has been fixed.
+
+We also added a request limit that stops the network from being overrun with concurrent requests.
+
+https://github.com/owncloud/web/pull/10763
diff --git a/packages/web-app-files/src/store/actions.ts b/packages/web-app-files/src/store/actions.ts
@@ -1,6 +1,6 @@
 import PQueue from 'p-queue'
 
-import { getParentPaths } from '@ownclouders/web-pkg'
+import { ConfigurationManager, getParentPaths } from '@ownclouders/web-pkg'
 import {
   buildShare,
   buildCollaboratorShare,
@@ -18,7 +18,7 @@ import {
   SpaceResource
 } from '@ownclouders/web-client/src/helpers'
 import { WebDAV } from '@ownclouders/web-client/src/webdav'
-import { ClientService, LoadingTaskCallbackArguments } from '@ownclouders/web-pkg'
+import { ClientService } from '@ownclouders/web-pkg'
 import { Language } from 'vue3-gettext'
 import { eventBus } from '@ownclouders/web-pkg'
 import { AncestorMetaData } from '@ownclouders/web-pkg'
@@ -165,7 +165,7 @@ export default {
       space: SpaceResource
       files: Resource[]
       clientService: ClientService
-      loadingCallbackArgs: LoadingTaskCallbackArguments
+      configurationManager: ConfigurationManager
       firstRun: boolean
     } & Language
   ) {
@@ -175,47 +175,52 @@ export default {
       space,
       files,
       clientService,
-      loadingCallbackArgs,
+      configurationManager,
       firstRun = true
     } = options
-    const { setProgress } = loadingCallbackArgs
-    const promises = []
-    const removedFiles = []
-    for (const [i, file] of files.entries()) {
-      const promise = clientService.webdav
-        .deleteFile(space, file)
-        .then(() => {
-          removedFiles.push(file)
-        })
-        .catch((error) => {
-          let title = $gettext('Failed to delete "%{file}"', { file: file.name })
-          if (error.statusCode === 423) {
-            if (firstRun) {
-              return context.dispatch('deleteFiles', {
-                ...options,
-                space,
-                files: [file],
-                clientService,
-                firstRun: false
+    const removedFiles: Resource[] = []
+
+    const queue = new PQueue({
+      concurrency: configurationManager.options.concurrentRequests.resourceBatchActions
+    })
+
+    const promises = files.map((file) =>
+      queue.add(() =>
+        clientService.webdav
+          .deleteFile(space, file)
+          .then(() => {
+            removedFiles.push(file)
+          })
+          .catch((error) => {
+            let title = $gettext('Failed to delete "%{file}"', { file: file.name })
+            if (error.statusCode === 423) {
+              if (firstRun) {
+                return context.dispatch('deleteFiles', {
+                  ...options,
+                  space,
+                  files: [file],
+                  clientService,
+                  configurationManager,
+                  firstRun: false
+                })
+              }
+
+              title = $gettext('Failed to delete "%{file}" - the file is locked', {
+                file: file.name
               })
             }
+            context.dispatch(
+              'showErrorMessage',
+              {
+                title,
+                error
+              },
+              { root: true }
+            )
+          })
+      )
+    )
 
-            title = $gettext('Failed to delete "%{file}" - the file is locked', { file: file.name })
-          }
-          context.dispatch(
-            'showErrorMessage',
-            {
-              title,
-              error
-            },
-            { root: true }
-          )
-        })
-        .finally(() => {
-          setProgress({ total: files.length, current: i + 1 })
-        })
-      promises.push(promise)
-    }
     return Promise.all(promises).then(() => {
       context.commit('REMOVE_FILES', removedFiles)
       context.commit('REMOVE_FILES_FROM_SEARCHED', removedFiles)

diff --git a/packages/web-pkg/src/composables/actions/helpers/useFileActionsDeleteResources.ts b/packages/web-pkg/src/composables/actions/helpers/useFileActionsDeleteResources.ts
@@ -179,61 +179,58 @@ export const useFileActionsDeleteResources = ({ store }: { store?: Store<any> })
 
     return Object.values(resourceSpaceMapping).map(
       ({ space: spaceForDeletion, resources: resourcesForDeletion }) => {
-        return loadingService.addTask(
-          (loadingCallbackArgs) => {
-            return store
-              .dispatch('Files/deleteFiles', {
-                ...language,
-                space: spaceForDeletion,
-                files: resourcesForDeletion,
-                clientService,
-                loadingCallbackArgs
-              })
-              .then(async () => {
-                // Load quota
-                if (
-                  isLocationSpacesActive(router, 'files-spaces-generic') &&
-                  !['public', 'share'].includes(spaceForDeletion?.driveType)
-                ) {
-                  if (unref(hasSpacesEnabled)) {
-                    const graphClient = clientService.graphAuthenticated
-                    const driveResponse = await graphClient.drives.getDrive(
-                      unref(resources)[0].storageId
-                    )
-                    store.commit('runtime/spaces/UPDATE_SPACE_FIELD', {
-                      id: driveResponse.data.id,
-                      field: 'spaceQuota',
-                      value: driveResponse.data.quota
-                    })
-                  } else {
-                    const user = await owncloudSdk.users.getUser(store.getters.user.id)
-                    store.commit('SET_QUOTA', user.quota)
-                  }
-                }
-
-                if (
-                  unref(resourcesToDelete).length &&
-                  isSameResource(unref(resourcesToDelete)[0], store.getters['Files/currentFolder'])
-                ) {
-                  // current folder is being deleted
-                  return router.push(
-                    createFileRouteOptions(spaceForDeletion, {
-                      path: dirname(unref(resourcesToDelete)[0].path),
-                      fileId: unref(resourcesToDelete)[0].parentFolderId
-                    })
+        return loadingService.addTask(() => {
+          return store
+            .dispatch('Files/deleteFiles', {
+              ...language,
+              space: spaceForDeletion,
+              files: resourcesForDeletion,
+              clientService,
+              configurationManager
+            })
+            .then(async () => {
+              // Load quota
+              if (
+                isLocationSpacesActive(router, 'files-spaces-generic') &&
+                !['public', 'share'].includes(spaceForDeletion?.driveType)
+              ) {
+                if (unref(hasSpacesEnabled)) {
+                  const graphClient = clientService.graphAuthenticated
+                  const driveResponse = await graphClient.drives.getDrive(
+                    unref(resources)[0].storageId
                   )
+                  store.commit('runtime/spaces/UPDATE_SPACE_FIELD', {
+                    id: driveResponse.data.id,
+                    field: 'spaceQuota',
+                    value: driveResponse.data.quota
+                  })
+                } else {
+                  const user = await owncloudSdk.users.getUser(store.getters.user.id)
+                  store.commit('SET_QUOTA', user.quota)
                 }
+              }
 
-                const activeFilesCount = store.getters['Files/activeFiles'].length
-                const pageCount = Math.ceil(unref(activeFilesCount) / unref(itemsPerPage))
-                if (unref(currentPage) > 1 && unref(currentPage) > pageCount) {
-                  // reset pagination to avoid empty lists (happens when deleting all items on the last page)
-                  currentPageQuery.value = pageCount.toString()
-                }
-              })
-          },
-          { indeterminate: false }
-        )
+              if (
+                unref(resourcesToDelete).length &&
+                isSameResource(unref(resourcesToDelete)[0], store.getters['Files/currentFolder'])
+              ) {
+                // current folder is being deleted
+                return router.push(
+                  createFileRouteOptions(spaceForDeletion, {
+                    path: dirname(unref(resourcesToDelete)[0].path),
+                    fileId: unref(resourcesToDelete)[0].parentFolderId
+                  })
+                )
+              }
+
+              const activeFilesCount = store.getters['Files/activeFiles'].length
+              const pageCount = Math.ceil(unref(activeFilesCount) / unref(itemsPerPage))
+              if (unref(currentPage) > 1 && unref(currentPage) > pageCount) {
+                // reset pagination to avoid empty lists (happens when deleting all items on the last page)
+                currentPageQuery.value = pageCount.toString()
+              }
+            })
+        })
       }
     )
   }