From 9584ab2ef6e222075995d5977b366159f22cd994 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Thu, 11 Apr 2024 10:35:56 +0200 Subject: [PATCH] fix: flickering loading indicator on batch delete Fixes the flickering loading indicator when batch deleting a bunch of files. Since delete operations are running in parallel, using the `setProgress` method for the loading indicator won't work. This is only suited for serial requests. Also adds a concurrent request limit via pqueue. This stops the network from being overrun with concurrent requests. --- .../bugfix-flickering-loading-indicator | 7 ++ packages/web-app-files/src/store/actions.ts | 83 +++++++------- .../helpers/useFileActionsDeleteResources.ts | 101 +++++++++--------- 3 files changed, 100 insertions(+), 91 deletions(-) create mode 100644 changelog/unreleased/bugfix-flickering-loading-indicator diff --git a/changelog/unreleased/bugfix-flickering-loading-indicator b/changelog/unreleased/bugfix-flickering-loading-indicator new file mode 100644 index 00000000000..d2cb449006b --- /dev/null +++ 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 index 17abdbd8aad..9deadff48d7 100644 --- 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 index 88382380b17..71ecb95fafb 100644 --- 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 }) 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() + } + }) + }) } ) }