fix: keep upload batch out of uppy while the conflict dialog is open

packages/web-app-files/src/HandleUpload.ts

@@ -539,9 +539,21 @@ export class HandleUpload extends BasePlugin<PluginOpts, OcUppyMeta, OcUppyBody>
           ;(dashboard[0] as HTMLElement).style.display = 'none'
         }
 
-        const result = await conflictHandler.displayOverwriteDialog(filesToUpload, conflicts)
-        const remainingIds = result.files.map(({ id }) => id)
-        this.removeFilesFromUpload(filesToUpload.filter(({ id }) => !remainingIds.includes(id)))
+        // keep batch out of uppy while the dialog is open, another batch's upload() would upload it.
+        // kept files are re-added below.
+        const batchIds = filesToUpload.map(({ id }) => id)
+        this.uppyService.holdBackFiles(batchIds)
+        const result = await conflictHandler
+          .displayOverwriteDialog(filesToUpload, conflicts)
+          .finally(() => this.uppyService.releaseHeldBackFiles(batchIds))
+        if (result.dismissed) {
+          const { $gettext } = this.language
+          this.messageStore.showMessage({
+            title: $gettext('Upload cancelled'),
+            desc: $gettext('The conflict dialog was closed, no files were uploaded.'),
+            status: 'warning'
+          })
+        }
         if (result.files.length === 0) {
           return this.uppyService.clearInputs()
         }

packages/web-app-files/src/helpers/resource/actions/upload.ts

@@ -1,4 +1,4 @@
-import { markRaw } from 'vue'
+import { markRaw, watch } from 'vue'
 import { escapeRegExp } from 'lodash-es'
 import { Language } from 'vue3-gettext'
 import { Resource } from '@opencloud-eu/web-client'
@@ -37,6 +37,8 @@ function renameUploadFolderInEndpoint(endpoint: string, segments: string[], newF
 
 export class UploadResourceConflict extends ConflictDialog {
   resourcesStore: ResourcesStore
+  // a dialog was closed without an answer, e.g. on route change
+  private dismissed = false
 
   constructor(resourcesStore: ResourcesStore, language: Language) {
     const { $gettext, $ngettext } = language
@@ -52,10 +54,10 @@ export class UploadResourceConflict extends ConflictDialog {
     separateSkipHandling = false, // separate skip-handling between files and folders
     existingIsFolder = resource.isFolder
   ): Promise<ResolveConflict> {
-    const { dispatchModal } = useModals()
+    const modalStore = useModals()
 
     return new Promise<ResolveConflict>((resolve) => {
-      dispatchModal({
+      const modal = modalStore.dispatchModal({
         title: existingIsFolder
           ? this.$gettext('Folder already exists')
           : this.$gettext('File already exists'),
@@ -72,10 +74,23 @@ export class UploadResourceConflict extends ConflictDialog {
           existingIsFolder,
           hideSecondaryAction: existingIsFolder !== resource.isFolder,
           callbackFn: (conflict: ResolveConflict) => {
+            stop()
             resolve(conflict)
           }
         })
       })
+
+      const stop = watch(
+        () => modalStore.modals.some(({ id }) => id === modal.id),
+        (isOpen) => {
+          if (isOpen) {
+            return
+          }
+          stop()
+          this.dismissed = true
+          resolve({ strategy: ResolveStrategy.SKIP, doForAllConflicts: true })
+        }
+      )
     })
   }
 
@@ -109,7 +124,7 @@ export class UploadResourceConflict extends ConflictDialog {
   async displayOverwriteDialog(
     files: OcUppyFile[],
     conflicts: ConflictedResource[]
-  ): Promise<{ files: OcUppyFile[]; mergedFolders: string[] }> {
+  ): Promise<{ files: OcUppyFile[]; mergedFolders: string[]; dismissed?: boolean }> {
     const resolvedFileConflicts: { name: string; strategy: ResolveStrategy }[] = []
     const resolvedFolderConflicts: { name: string; strategy: ResolveStrategy }[] = []
     const allStrategies: Record<
@@ -155,6 +170,9 @@ export class UploadResourceConflict extends ConflictDialog {
         true,
         conflict.existingIsFolder
       )
+      if (this.dismissed) {
+        return { files: [], mergedFolders: [], dismissed: true }
+      }
       if (resolvedConflict.doForAllConflicts) {
         strategies[typeMismatch ? 'typeMismatch' : 'regular'] = resolvedConflict.strategy
       }

packages/web-app-files/tests/unit/HandleUpload.spec.ts

@@ -21,6 +21,7 @@ import {
   OcUppyBody
 } from '@opencloud-eu/web-pkg'
 import { Language } from 'vue3-gettext'
+import { omit } from 'lodash-es'
 import { createTestingPinia } from '@opencloud-eu/web-test-helpers'
 
 let getConflictsMock = vi.fn()
@@ -545,37 +546,103 @@ describe('HandleUpload', () => {
         await instance.handleUpload([file])
         expect(getConflictsMock).not.toHaveBeenCalled()
       })
+      function useUppyFileState(
+        { uppy, opts }: ReturnType<typeof getWrapper>['mocks'],
+        files: OcUppyFile[]
+      ) {
+        let state = { files: Object.fromEntries(files.map((file) => [file.id, file])) }
+        uppy.getState.mockImplementation(() => state as State<OcUppyMeta, OcUppyBody>)
+        uppy.setState.mockImplementation((patch) => {
+          state = { ...state, ...patch }
+        })
+        vi.mocked(opts.uppyService.holdBackFiles).mockImplementation((ids) => {
+          state = { ...state, files: omit(state.files, ids) }
+        })
+        return () => Object.keys(state.files)
+      }
       it('does not start upload if all files were skipped in conflict handling', async () => {
+        const file = mock<OcUppyFile>({ id: 'skipped', name: 'name' })
         const { instance, mocks } = getWrapper({ conflicts: [{}], conflictHandlerResult: [] })
-        const removeFilesFromUploadSpy = vi.spyOn(instance, 'removeFilesFromUpload')
+        const uppyFileIds = useUppyFileState(mocks, [file])
 
-        await instance.handleUpload([mock<OcUppyFile>({ name: 'name' })])
+        await instance.handleUpload([file])
         expect(mocks.opts.uppyService.uploadFiles).not.toHaveBeenCalled()
         expect(mocks.opts.uppyService.clearInputs).toHaveBeenCalled()
-        expect(removeFilesFromUploadSpy).toHaveBeenCalled()
+        expect(uppyFileIds()).toEqual([])
+      })
+      it('informs the user if the conflict dialog was closed without an answer', async () => {
+        const file = mock<OcUppyFile>({ id: 'dismissed', name: 'name' })
+        const { instance, mocks } = getWrapper({ conflicts: [{}] })
+        displayOverwriteDialogMock.mockResolvedValue({
+          files: [],
+          mergedFolders: [],
+          dismissed: true
+        })
+        const showMessageSpy = vi.spyOn(mocks.opts.messageStore, 'showMessage')
+
+        await instance.handleUpload([file])
+        expect(showMessageSpy).toHaveBeenCalledWith(expect.objectContaining({ status: 'warning' }))
+        expect(mocks.opts.uppyService.uploadFiles).not.toHaveBeenCalled()
+      })
+      it('does not show a message if all files were skipped in the conflict dialog', async () => {
+        const file = mock<OcUppyFile>({ id: 'skipped', name: 'name' })
+        const { instance, mocks } = getWrapper({ conflicts: [{}], conflictHandlerResult: [] })
+        const showMessageSpy = vi.spyOn(mocks.opts.messageStore, 'showMessage')
+
+        await instance.handleUpload([file])
+        expect(showMessageSpy).not.toHaveBeenCalled()
       })
-      it('removes skipped files from uppy and uploads the remaining ones', async () => {
+      it('keeps the batch out of uppy while the conflict dialog is open', async () => {
+        const conflictFile = mock<OcUppyFile>({ id: 'conflict', name: 'one.txt' })
+        const freshFile = mock<OcUppyFile>({ id: 'fresh', name: 'fresh.txt' })
+        const otherBatchFile = mock<OcUppyFile>({ id: 'other', name: 'other.txt' })
+        const { instance, mocks } = getWrapper({
+          conflicts: [{}],
+          conflictHandlerResult: [conflictFile, freshFile]
+        })
+        vi.spyOn(instance, 'prepareFiles').mockReturnValue([conflictFile, freshFile])
+        const uppyFileIds = useUppyFileState(mocks, [conflictFile, freshFile, otherBatchFile])
+        const { releaseHeldBackFiles } = mocks.opts.uppyService
+        let idsWhileDialogOpen: string[]
+        displayOverwriteDialogMock.mockImplementation(() => {
+          idsWhileDialogOpen = uppyFileIds()
+          expect(releaseHeldBackFiles).not.toHaveBeenCalled()
+          return Promise.resolve({ files: [conflictFile, freshFile], mergedFolders: [] })
+        })
+
+        await instance.handleUpload([conflictFile, freshFile])
+        expect(mocks.opts.uppyService.holdBackFiles).toHaveBeenCalledWith(['conflict', 'fresh'])
+        expect(idsWhileDialogOpen).toEqual(['other'])
+        expect(releaseHeldBackFiles).toHaveBeenCalledWith(['conflict', 'fresh'])
+        expect(uppyFileIds()).toEqual(['other', 'conflict', 'fresh'])
+        expect(mocks.opts.uppyService.uploadFiles).toHaveBeenCalled()
+      })
+      it('does not re-add skipped files to uppy and uploads the remaining ones', async () => {
         const skippedFile = mock<OcUppyFile>({ id: 'skipped', name: 'one.txt' })
         const freshFile = mock<OcUppyFile>({ id: 'fresh', name: 'fresh.txt' })
         const { instance, mocks } = getWrapper({
           conflicts: [{}],
           conflictHandlerResult: [freshFile]
         })
         vi.spyOn(instance, 'prepareFiles').mockReturnValue([skippedFile, freshFile])
+        const uppyFileIds = useUppyFileState(mocks, [skippedFile, freshFile])
 
         await instance.handleUpload([skippedFile, freshFile])
-        expect(mocks.uppy.removeFile).toHaveBeenCalledWith('skipped')
-        expect(mocks.uppy.removeFile).not.toHaveBeenCalledWith('fresh')
+        expect(uppyFileIds()).toEqual(['fresh'])
         expect(mocks.opts.uppyService.uploadFiles).toHaveBeenCalled()
       })
       it('sets the result of the conflict handler as uppy file state', async () => {
-        const conflictHandlerResult = [mock<OcUppyFile>({ id: '1' })]
-        const { instance, mocks } = getWrapper({ conflicts: [{}], conflictHandlerResult })
-        await instance.handleUpload([mock<OcUppyFile>(), mock<OcUppyFile>()])
-
-        expect(mocks.uppy.setState).toHaveBeenCalledWith({
-          files: { [conflictHandlerResult[0].id]: conflictHandlerResult[0] }
+        const files = [mock<OcUppyFile>({ id: '1' }), mock<OcUppyFile>({ id: '2' })]
+        const renamedFile = mock<OcUppyFile>({ id: '1', name: 'renamed' })
+        const { instance, mocks } = getWrapper({
+          conflicts: [{}],
+          conflictHandlerResult: [renamedFile]
         })
+        vi.spyOn(instance, 'prepareFiles').mockReturnValue(files)
+        useUppyFileState(mocks, files)
+        await instance.handleUpload(files)
+
+        expect(mocks.uppy.getState().files).toEqual({ '1': renamedFile })
       })
     })
     describe('vault name length check', () => {

packages/web-app-files/tests/unit/helpers/resource/actions/upload.spec.ts

@@ -2,16 +2,28 @@ import { dirname } from 'path'
 import { mock, mockDeep } from 'vitest-mock-extended'
 import { Language } from 'vue3-gettext'
 import { UploadResourceConflict } from '../../../../../src/helpers/resource'
-import { ResolveStrategy, OcUppyFile, useModals, useResourcesStore } from '@opencloud-eu/web-pkg'
+import {
+  ResolveConflict,
+  ResolveStrategy,
+  OcUppyFile,
+  useModals,
+  useResourcesStore
+} from '@opencloud-eu/web-pkg'
+import { nextTick } from 'vue'
 import { Resource, urlJoin } from '@opencloud-eu/web-client'
 import { createTestingPinia } from '@opencloud-eu/web-test-helpers'
 
 const getResourceConflictInstance = ({
-  currentFiles = [mockDeep<Resource>()]
+  currentFiles = [mockDeep<Resource>()],
+  stubActions = true
 }: {
   currentFiles?: Resource[]
+  stubActions?: boolean
 } = {}) => {
-  createTestingPinia({ initialState: { resources: { resources: currentFiles } } })
+  createTestingPinia({ stubActions, initialState: { resources: { resources: currentFiles } } })
+  if (stubActions) {
+    vi.mocked(useModals().dispatchModal).mockImplementation((data) => ({ ...data, id: 'modal' }))
+  }
   const resourcesStore = useResourcesStore()
   return new UploadResourceConflict(resourcesStore, mock<Language>())
 }
@@ -100,6 +112,30 @@ describe('upload helper', () => {
       const attrs = vi.mocked(dispatchModal).mock.calls[0][0].customComponentAttrs()
       expect(attrs.hideSecondaryAction).toBe(false)
     })
+    it('resolves with the answer given in the dialog', async () => {
+      const instance = getResourceConflictInstance({ stubActions: false })
+
+      const promise = instance.resolveFileExists({ name: 'dup' } as Resource, 1)
+      answerDialog({ strategy: ResolveStrategy.REPLACE, doForAllConflicts: false })
+      await nextTick()
+
+      await expect(promise).resolves.toEqual({
+        strategy: ResolveStrategy.REPLACE,
+        doForAllConflicts: false
+      })
+    })
+    it('skips all conflicts if the dialog is closed without an answer', async () => {
+      const instance = getResourceConflictInstance({ stubActions: false })
+      const modalStore = useModals()
+
+      const promise = instance.resolveFileExists({ name: 'dup' } as Resource, 1)
+      modalStore.removeAllModals()
+
+      await expect(promise).resolves.toEqual({
+        strategy: ResolveStrategy.SKIP,
+        doForAllConflicts: true
+      })
+    })
   })
   describe('method "displayOverwriteDialog"', () => {
     it.each([false, true])(
@@ -831,6 +867,42 @@ describe('upload helper', () => {
       )
     })
 
+    describe('with real dialogs', () => {
+      const files = [createUploadFile('one.txt'), createUploadFile('a.txt', '/docs')]
+      const conflicts = [
+        { name: 'one.txt', type: 'file', existingIsFolder: false },
+        { name: 'docs', type: 'folder', existingIsFolder: true }
+      ]
+      const currentFiles = [
+        mock<Resource>({ name: 'one.txt', isFolder: false }),
+        mock<Resource>({ name: 'docs', isFolder: true })
+      ]
+
+      it('drops the whole batch if a dialog is closed without an answer', async () => {
+        const instance = getResourceConflictInstance({ currentFiles, stubActions: false })
+        const modalStore = useModals()
+
+        const promise = instance.displayOverwriteDialog(files, conflicts)
+        modalStore.removeAllModals()
+
+        await expect(promise).resolves.toEqual({ files: [], mergedFolders: [], dismissed: true })
+        expect(modalStore.modals).toEqual([])
+      })
+      it('keeps asking for the remaining conflicts after a dialog is answered', async () => {
+        const instance = getResourceConflictInstance({ currentFiles, stubActions: false })
+        const modalStore = useModals()
+
+        const promise = instance.displayOverwriteDialog(files, conflicts)
+        answerDialog({ strategy: ResolveStrategy.SKIP, doForAllConflicts: false })
+        await vi.waitFor(() => expect(modalStore.modals).toHaveLength(1))
+        answerDialog({ strategy: ResolveStrategy.MERGE, doForAllConflicts: false })
+
+        const result = await promise
+        expect(result.files.map(({ id }) => id)).toEqual(['/docs/a.txt'])
+        expect(result.mergedFolders).toEqual(['docs'])
+      })
+    })
+
     it('should show dialog twice if do for all conflicts is ticked and folders and files are uploaded', async () => {
       const OcUppyFileOne = mockDeep<OcUppyFile>({ name: 'test' })
       const OcUppyFileTwo = mockDeep<OcUppyFile>({ name: 'folder' })
@@ -856,6 +928,17 @@ describe('upload helper', () => {
   })
 })
 
+// mimics ResourceConflictModal, which removes itself before calling back
+function answerDialog(conflict: ResolveConflict) {
+  const modalStore = useModals()
+  const modal = modalStore.activeModal
+  const { callbackFn } = modal.customComponentAttrs() as {
+    callbackFn: (conflict: ResolveConflict) => void
+  }
+  modalStore.removeModal(modal.id)
+  callbackFn(conflict)
+}
+
 function createUploadFile(name: string, relativeFolder = '', transport: 'xhr' | 'tus' = 'xhr') {
   const relativePath = `${relativeFolder}/${name}`
   const folderEndpoint = `https://example.com/dav${relativeFolder

packages/web-pkg/src/services/uppy/uppyService.ts

@@ -3,6 +3,7 @@ import Tus from '@uppy/tus'
 import { TusOptions } from '@uppy/tus'
 import XHRUpload, { XHRUploadOptions } from '@uppy/xhr-upload'
 import { Language } from 'vue3-gettext'
+import { omit } from 'lodash-es'
 import { eventBus } from '../eventBus'
 import DropTarget from './DropTarget/plugin'
 import { Resource, urlJoin } from '@opencloud-eu/web-client'
@@ -86,6 +87,7 @@ export class UppyService {
   uppy: Uppy<OcUppyMeta, OcUppyBody>
   uploadInputs: HTMLInputElement[] = []
   uploadFolderMap: Record<string, Resource> = {}
+  private heldBackFileIds = new Set<string>()
 
   constructor({ language }: UppyServiceOptions) {
     const { $gettext } = language
@@ -100,6 +102,10 @@ export class UppyService {
         file.meta.relativePath = this.getRelativeFilePath(file)?.normalize('NFC')
         // id needs to be generated after the relative path has been set.
         file.id = this.generateUploadId(file)
+        if (this.heldBackFileIds.has(file.id)) {
+          // file is currently being prepared, e.g. awaits the conflict dialog
+          return false
+        }
         return file
       }
     })
@@ -332,6 +338,22 @@ export class UppyService {
     this.uppy.log(message, type)
   }
 
+  /**
+   * Takes files out of Uppy's state so that upload() ignores them, e.g. while their batch awaits
+   * user input. Adding them again is rejected until releaseHeldBackFiles is called.
+   */
+  holdBackFiles(fileIds: string[]) {
+    fileIds.forEach((id) => this.heldBackFileIds.add(id))
+    this.uppy.setState({ files: omit(this.uppy.getState().files, fileIds) })
+  }
+
+  /**
+   * Allows adding held back files again. Does not put them back into Uppy's state.
+   */
+  releaseHeldBackFiles(fileIds: string[]) {
+    fileIds.forEach((id) => this.heldBackFileIds.delete(id))
+  }
+
   addFiles(files: OcMinimalUppyFile[] | File[]) {
     // uppy types say they do not accept File[] but they are wrong
     this.uppy.addFiles(files as OcMinimalUppyFile[])