Skip to content

Commit

Permalink
@uppy/provider-views: fix race condition when adding folders (#4384)
Browse files Browse the repository at this point in the history 
* fix race condtion when adding files

don't call addFolder from listAllFiles
because that would call addFile before all folders were loaded

also remove unused selectedFolders state

* fix also the case of adding multiple folders

* fix todo: remove SharedHandler

* remove loaderWrapper

* fix logic

* fix the last race condition

* fix broken duplicate file check

* fix logic

* prettiyfy loop

* add user feedback

so they know that something is happening

* run corepack yarn run build:locale-pack

* Revert "run corepack yarn run build:locale-pack"

This reverts commit 85548ce.

* Revert "add user feedback"

This reverts commit 4060019.

* use async generator instead of p-map

* re-fix race-condition

* remove providerFileToId

as suggested by @arturi

* use addFiles instead of addFile

* rename function

* use provider-supplied file ID

instead of generating an ID, for providers that we whitelsit
this allows adding the same time many times (with a different path)

* call core directly

* improve dev dashboard

* disable experimental getAsFileSystemHandle

it seems to be causing problems when dropping folders with subfolders in newest chrome
e.g a folder with 50 files and a subfolder which in turn has another 50 files

also refactor and document the code more

* Update packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

* mov eto utils

---------

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
@mifi @aduh95
mifi and aduh95 committed Apr 4, 2023
1 parent 6df62c5 commit 3dd1e5e
Show file tree
Hide file tree
Showing 8 changed files with 273 additions and 282 deletions.
11 changes: 4 additions & 7 deletions packages/@uppy/core/src/Uppy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import throttle from 'lodash.throttle'
import DefaultStore from '@uppy/store-default'
import getFileType from '@uppy/utils/lib/getFileType'
import getFileNameAndExtension from '@uppy/utils/lib/getFileNameAndExtension'
import generateFileID from '@uppy/utils/lib/generateFileID'
import { getSafeFileId } from '@uppy/utils/lib/generateFileID'
import supportsUploadProgress from './supportsUploadProgress.js'
import getFileName from './getFileName.js'
import { justErrorsLogger, debugLogger } from './loggers.js'
Expand Down Expand Up @@ -458,12 +458,9 @@ class Uppy {
const fileName = getFileName(fileType, fileDescriptor)
const fileExtension = getFileNameAndExtension(fileName).extension
const isRemote = Boolean(fileDescriptor.isRemote)
const fileID = generateFileID({
...fileDescriptor,
type: fileType,
})
const id = getSafeFileId(fileDescriptor)

if (this.checkIfFileAlreadyExists(fileID)) {
if (this.checkIfFileAlreadyExists(id)) {
const error = new RestrictionError(this.i18n('noDuplicates', { fileName }))
this.#informAndEmit(error, fileDescriptor)
throw error
Expand All @@ -478,7 +475,7 @@ class Uppy {

let newFile = {
source: fileDescriptor.source || '',
id: fileID,
id,
name: fileName,
extension: fileExtension || '',
meta: {
Expand Down
221 changes: 105 additions & 116 deletions packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { h } from 'preact'

import { getSafeFileId } from '@uppy/utils/lib/generateFileID'

import AuthView from './AuthView.jsx'
import Header from './Header.jsx'
import Browser from '../Browser.jsx'
Expand Down Expand Up @@ -59,7 +61,6 @@ export default class ProviderView extends View {
this.logout = this.logout.bind(this)
this.handleAuth = this.handleAuth.bind(this)
this.handleScroll = this.handleScroll.bind(this)
this.listAllFiles = this.listAllFiles.bind(this)
this.donePicking = this.donePicking.bind(this)

// Visual
Expand Down Expand Up @@ -101,29 +102,31 @@ export default class ProviderView extends View {
* @param {string} id Folder id
* @returns {Promise} Folders/files in folder
*/
getFolder (id, name) {
return this.sharedHandler.loaderWrapper(
this.provider.list(id),
(res) => {
const folders = []
const files = []
let updatedDirectories

const state = this.plugin.getPluginState()
const index = state.directories.findIndex((dir) => id === dir.id)

if (index !== -1) {
updatedDirectories = state.directories.slice(0, index + 1)
} else {
updatedDirectories = state.directories.concat([{ id, title: name }])
}
async getFolder (id, name) {
this.setLoading(true)
try {
const res = await this.provider.list(id)
const folders = []
const files = []
let updatedDirectories

const state = this.plugin.getPluginState()
const index = state.directories.findIndex((dir) => id === dir.id)

if (index !== -1) {
updatedDirectories = state.directories.slice(0, index + 1)
} else {
updatedDirectories = state.directories.concat([{ id, title: name }])
}

this.username = res.username || this.username
this.#updateFilesAndFolders(res, files, folders)
this.plugin.setPluginState({ directories: updatedDirectories, filterInput: '' })
},
this.handleError,
)
this.username = res.username || this.username
this.#updateFilesAndFolders(res, files, folders)
this.plugin.setPluginState({ directories: updatedDirectories, filterInput: '' })
} catch (err) {
this.handleError(err)
} finally {
this.setLoading(false)
}
}

/**
Expand Down Expand Up @@ -171,74 +174,6 @@ export default class ProviderView extends View {
this.plugin.setPluginState({ filterInput: '' })
}

/**
* Adds all files found inside of specified folder.
*
* Uses separated state while folder contents are being fetched and
* mantains list of selected folders, which are separated from files.
*/
addFolder (folder) {
const folderId = this.providerFileToId(folder)
const folders = { ...this.plugin.getPluginState().selectedFolders }

if (folderId in folders && folders[folderId].loading) {
return
}

folders[folderId] = { loading: true, files: [] }

this.plugin.setPluginState({ selectedFolders: { ...folders } })

// eslint-disable-next-line consistent-return
return this.listAllFiles(folder.requestPath).then((files) => {
let count = 0

// 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.
files.forEach(file => {
const id = this.providerFileToId(file)
if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) {
count++
}
})

if (count > 0) {
files.forEach((file) => this.addFile(file))
}

const ids = files.map(this.providerFileToId)

folders[folderId] = {
loading: false,
files: ids,
}
this.plugin.setPluginState({ selectedFolders: folders, filterInput: '' })

let message

if (count === 0) {
message = this.plugin.uppy.i18n('folderAlreadyAdded', {
folder: folder.name,
})
} else if (files.length) {
message = this.plugin.uppy.i18n('folderAdded', {
smart_count: count, folder: folder.name,
})
} else {
message = this.plugin.uppy.i18n('emptyFolderAdded')
}

this.plugin.uppy.info(message)
}).catch((e) => {
const selectedFolders = { ...this.plugin.getPluginState().selectedFolders }
delete selectedFolders[folderId]
this.plugin.setPluginState({ selectedFolders })
this.handleError(e)
})
}

async handleAuth () {
await this.provider.ensurePreAuth()

Expand Down Expand Up @@ -300,35 +235,91 @@ export default class ProviderView extends View {
}
}

async listAllFiles (path, files = null) {
files = files || [] // eslint-disable-line no-param-reassign
const res = await this.provider.list(path)
res.items.forEach((item) => {
if (!item.isFolder) {
files.push(item)
} else {
this.addFolder(item)
async* recursivelyListAllFiles (path) {
let curPath = path

// need to repeat the list call until there are no more pages
while (curPath) {
const res = await this.provider.list(curPath)

for (const item of res.items) {
if (item.isFolder) {
// recursively call self for folder
yield* this.recursivelyListAllFiles(item.requestPath)
} else {
yield item
}
}
})
const moreFiles = res.nextPagePath
if (moreFiles) {
return this.listAllFiles(moreFiles, files)

curPath = res.nextPagePath
}
return files
}

donePicking () {
const { currentSelection } = this.plugin.getPluginState()
const promises = currentSelection.map((file) => {
if (file.isFolder) {
return this.addFolder(file)
async donePicking () {
this.setLoading(true)
try {
const { currentSelection } = this.plugin.getPluginState()

const messages = []
const newFiles = []

// eslint-disable-next-line no-unreachable-loop
for (const file of currentSelection) {
if (file.isFolder) {
const { requestPath, name } = file
let isEmpty = true
let numNewFiles = 0

for await (const fileInFolder of this.recursivelyListAllFiles(requestPath)) {
const tagFile = this.getTagFile(fileInFolder)
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(fileInFolder)
numNewFiles++
}
isEmpty = false
}

let message
if (isEmpty) {
message = this.plugin.uppy.i18n('emptyFolderAdded')
} else if (numNewFiles === 0) {
message = this.plugin.uppy.i18n('folderAlreadyAdded', {
folder: name,
})
} else {
message = this.plugin.uppy.i18n('folderAdded', {
smart_count: numNewFiles, folder: name,
})
}

messages.push(message)
} else {
newFiles.push(file)
}
}
return this.addFile(file)
})

this.sharedHandler.loaderWrapper(Promise.all(promises), () => {
// 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.clearSelection()
}, () => {})
} catch (err) {
this.handleError(err)
} finally {
this.setLoading(false)
}
}

render (state, viewOptions = {}) {
Expand All @@ -341,7 +332,7 @@ export default class ProviderView extends View {

const targetViewOptions = { ...this.opts, ...viewOptions }
const { files, folders, filterInput, loading, currentSelection } = this.plugin.getPluginState()
const { isChecked, toggleCheckbox, recordShiftKeyPress, filterItems } = this.sharedHandler
const { isChecked, toggleCheckbox, recordShiftKeyPress, filterItems } = this
const hasInput = filterInput !== ''
const headerProps = {
showBreadcrumbs: targetViewOptions.showBreadcrumbs,
Expand All @@ -364,7 +355,6 @@ export default class ProviderView extends View {
username: this.username,
getNextFolder: this.getNextFolder,
getFolder: this.getFolder,
filterItems: this.sharedHandler.filterItems,

// For SearchFilterInput component
showSearchFilter: targetViewOptions.showFilter,
Expand All @@ -378,7 +368,6 @@ export default class ProviderView extends View {
noResultsLabel: i18n('noFilesFound'),
logout: this.logout,
handleScroll: this.handleScroll,
listAllFiles: this.listAllFiles,
done: this.donePicking,
cancel: this.cancelPicking,
headerComponent: Header(headerProps),
Expand Down
31 changes: 15 additions & 16 deletions packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ export default class SearchProviderView extends View {
this.search = this.search.bind(this)
this.clearSearch = this.clearSearch.bind(this)
this.resetPluginState = this.resetPluginState.bind(this)
this.addFile = this.addFile.bind(this)
this.handleScroll = this.handleScroll.bind(this)
this.donePicking = this.donePicking.bind(this)

Expand Down Expand Up @@ -77,20 +76,22 @@ export default class SearchProviderView extends View {
})
}

search (query) {
async search (query) {
const { searchTerm } = this.plugin.getPluginState()
if (query && query === searchTerm) {
// no need to search again as this is the same as the previous search
return undefined
return
}

return this.sharedHandler.loaderWrapper(
this.provider.search(query),
(res) => {
this.#updateFilesAndInputMode(res, [])
},
this.handleError,
)
this.setLoading(true)
try {
const res = await this.provider.search(query)
this.#updateFilesAndInputMode(res, [])
} catch (err) {
this.handleError(err)
} finally {
this.setLoading(false)
}
}

clearSearch () {
Expand Down Expand Up @@ -122,11 +123,9 @@ export default class SearchProviderView extends View {

donePicking () {
const { currentSelection } = this.plugin.getPluginState()
const promises = currentSelection.map((file) => this.addFile(file))

this.sharedHandler.loaderWrapper(Promise.all(promises), () => {
this.resetPluginState()
}, () => {})
this.plugin.uppy.log('Adding remote search provider files')
this.plugin.uppy.addFiles(currentSelection.map((file) => this.getTagFile(file)))
this.resetPluginState()
}

render (state, viewOptions = {}) {
Expand All @@ -139,7 +138,7 @@ export default class SearchProviderView extends View {

const targetViewOptions = { ...this.opts, ...viewOptions }
const { files, folders, filterInput, loading, currentSelection } = this.plugin.getPluginState()
const { isChecked, toggleCheckbox, filterItems } = this.sharedHandler
const { isChecked, toggleCheckbox, filterItems } = this
const hasInput = filterInput !== ''

const browserProps = {
Expand Down
Loading

0 comments on commit 3dd1e5e

Please sign in to comment.