fix: upload multi files with maxNumberofFiles #247
Conversation
|
WalkthroughThe changes involve modifications to several files related to file uploading functionality. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/src/upload/createUploader.ts (1 hunks)
- packages/editor/examples/upload-image.html (3 hunks)
- packages/upload-image-module/src/module/upload-images.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/core/src/upload/createUploader.ts (2)
Line range hint
1-124: Overall assessment: The change improves upload controlThe modification to set
autoProceed: falsein the Uppy configuration is the only change in this file. This alteration aligns well with the PR objective of managing multiple file uploads and enforcing a maximum limit. It provides better control over the upload process, which is particularly useful when dealing with multiple files.The rest of the file remains unchanged, maintaining its existing functionality for error handling, progress tracking, and success callbacks. This focused change minimizes the risk of introducing new issues while achieving the desired improvement in upload control.
To ensure a smooth integration, it's recommended to review any code that directly uses the
createUploaderfunction or Uppy's upload methods, as they might need adjustments to manually trigger uploads now thatautoProceedis set to false.
58-58: Approve the addition ofautoProceed: falseThe addition of
autoProceed: falseto the Uppy configuration is a good change. This prevents automatic file uploads as soon as they are added, which aligns with the PR objective of controlling the upload of multiple files.This change gives more control to the user interface, allowing for better management of file uploads, especially when dealing with multiple files and enforcing a maximum limit.
To ensure this change doesn't negatively impact the existing codebase, let's verify its usage:
✅ Verification successful
Verified Addition of
autoProceed: falseThe addition of
autoProceed: falseto the Uppy configuration has been thoroughly verified across the codebase. No instances were found where this change conflicts with existing usages or disrupts the upload functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any direct calls to `createUploader` that might be affected by this change # Test: Search for createUploader calls echo "Searching for createUploader calls:" rg --type typescript "createUploader\(" -A 5 # Test: Search for Uppy's upload method calls, which might need to be triggered manually now echo "Searching for Uppy upload method calls:" rg --type typescript "\.upload\(" -A 5Length of output: 327
Script:
#!/bin/bash # Description: Check for any direct calls to `createUploader` that might be affected by this change # Test: Search for createUploader calls in TypeScript files echo "Searching for createUploader calls in .ts and .tsx files:" rg "createUploader\(" -g "*.ts" -g "*.tsx" -A 5 # Test: Search for Uppy's upload method calls in TypeScript files echo "Searching for Uppy upload method calls in .ts and .tsx files:" rg "\.upload\(" -g "*.ts" -g "*.tsx" -A 5Length of output: 10717
packages/editor/examples/upload-image.html (3)
Line range hint
1-124: Summary of changes and alignment with PR objectives.The changes in this file are minimal and align well with the PR objectives:
- The server repository URL has been updated, which may be related to the implementation of the new file upload functionality.
- A commented-out
maxNumberOfFilesoption has been added, directly addressing the objective of limiting the number of files that can be uploaded.These changes seem to be on track with the PR's goal of fixing multi-file uploads with a maximum file limit. However, please ensure that:
- The new server repository is properly set up and documented.
- The
maxNumberOfFilesfeature is fully implemented, tested, and documented before being uncommented.
53-53: Clarify the purpose of the commented-outmaxNumberOfFilesoption.A new configuration option
maxNumberOfFileshas been added but is currently commented out. This aligns with the PR objective of addressing functionality related to uploading multiple files with a maximum limit. Please clarify:
- Is this a new feature that's not yet fully implemented or tested?
- Is it meant to be an optional configuration that users can uncomment as needed?
- If it's a new feature, has documentation been updated to explain its usage and implications?
Consider adding a comment explaining the purpose and usage of this option, especially if it's a new feature. For example:
// Uncomment and set the value to limit the number of files that can be uploaded // maxNumberOfFiles: 2,To check if this option is used elsewhere in the codebase:
#!/bin/bash # Description: Search for usage of maxNumberOfFiles in the codebase # Test: Search for maxNumberOfFiles in all files rg --type-add 'web:*.{html,js,ts}' --type web 'maxNumberOfFiles'
19-19: Verify the repository change and its implications.The hyperlink has been updated from the original wangeditor-team server repository to a new repository under cycleccc. Please confirm:
- Is this change intentional?
- Does the new repository contain the necessary server-side functionality?
- Are there any implications for users who might be relying on the original repository?
To check if the new repository exists and contains the expected content:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/upload-image-module/src/module/upload-images.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/upload-image-module/src/module/upload-images.ts (2)
130-139: EnsureuploadFilehandles empty file arrays to prevent errorsAs previously noted, calling
uppy.addFiles(uploadList)with an emptyuploadListmay cause unexpected behavior. Consider adding a check at the start of theuploadFilefunction to return early iffiles.length === 0.This ensures that the function behaves correctly even when an empty array is passed.
173-175: Good practice: Checking for files before uploadingAdding the condition
if (uploadFiles.length > 0)before callinguploadFileis a smart move. It prevents unnecessary function calls and potential errors when there are no files to upload.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/upload-image-module/__tests__/upload-files.test.ts (2)
61-61: LGTM! Consider enhancing test coverage.The addition of the
addFilesmethod to the mock uploader object is a good change. It aligns with the PR objective of supporting multi-file uploads and makes the mock more accurately represent the expected uploader interface.Consider adding a new test case that specifically verifies the behavior of the
addFilesmethod to ensure it's used correctly in theuploadImagesfunction. This would improve the test coverage for the new multi-file upload functionality.
Line range hint
1-74: Suggest adding a new test case for multi-file uploadsWhile the addition of the
addFilesmethod to the mock uploader is a good start, it would be beneficial to add a new test case that specifically verifies the behavior of theuploadImagesfunction when multiple files are provided. This would ensure that the new multi-file upload functionality is working as expected.Consider adding a test case similar to the following:
test('uploadImages should handle multiple files correctly', async () => { const editor = createEditor() const mockUploader = { addFiles: vi.fn(), upload: vi.fn(), } vi.spyOn(core, 'createUploader').mockReturnValue(mockUploader as any) const files = [mockFile('test1.jpg'), mockFile('test2.jpg')] as unknown as FileList await uploadImages(editor, files) expect(mockUploader.addFiles).toHaveBeenCalledWith(files) expect(mockUploader.upload).toHaveBeenCalled() })This test would verify that the
uploadImagesfunction correctly handles multiple files and uses the newaddFilesmethod of the uploader.packages/upload-image-module/src/module/upload-images.ts (1)
Line range hint
155-173: LGTM: Main function updated to handle multiple file uploads efficiently.The changes correctly implement batch file uploading and address the previous comment about avoiding unnecessary function calls.
For improved clarity, consider using
Array.prototype.filter()to createuploadFileList:-const uploadFileList : File[] = [] +const uploadFileList = fileList.filter(file => { + const size = file.size + if (base64LimitSize && size <= base64LimitSize) { + insertBase64(editor, file) + return false + } + if (customUpload) { + customUpload(file, (src, alt, href) => insertImageNode(editor, src, alt, href)) + return false + } + return true +}) -for await (const file of fileList) { - const size = file.size // size kb - - if (base64LimitSize && size <= base64LimitSize) { - // 允许 base64 ，而且 size 在 base64 限制之内，则插入 base64 格式 - await insertBase64(editor, file) - } else if (customUpload) { - // 自定义上传 - await customUpload(file, (src, alt, href) => insertImageNode(editor, src, alt, href)) - } else { - // 默认上传 - uploadFileList.push(file) - } -} if (uploadFileList.length > 0) { await uploadFile(editor, uploadFileList) }This approach simplifies the logic and makes the code more declarative.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/upload-image-module/tests/upload-files.test.ts (1 hunks)
- packages/upload-image-module/src/module/upload-images.ts (3 hunks)
- packages/video-module/src/module/helper/upload-videos.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/upload-image-module/src/module/upload-images.ts (1)
130-130: LGTM: Function signature updated to support multiple file uploads.The change from
file: Filetofiles: File[]aligns with the PR objective of handling multiple file uploads.As mentioned in a previous review, please ensure all calls to
uploadFilein the codebase have been updated to pass an array of files. Run the following script to verify:✅ Verification successful
All
uploadFilecalls correctly pass arrays of files.
- Verified that
uploadFileListis declared asFile[]in bothupload-images.tsandupload-videos.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all calls to `uploadFile` in TypeScript files rg --type ts -A 2 'uploadFile\('Length of output: 1045
Script:
#!/bin/bash # Find declarations of `uploadFileList` in TypeScript files within the specified modules rg --type ts 'uploadFileList\s*:\s*File\[\]' packages/upload-image-module/src/module/upload-images.ts packages/video-module/src/module/helper/upload-videos.tsLength of output: 348
packages/video-module/src/module/helper/upload-videos.ts (1)
96-96: Verify all usages ofuploadFilehave been updated to accept an array of filesThe
uploadFilefunction signature has changed to accept an array ofFileobjects. Ensure that all calls touploadFilewithin the codebase are updated to match the new signature to prevent any runtime errors.Use the following script to find all usages of
uploadFile:✅ Verification successful
All usages of
uploadFilehave been updated successfully
- Verified that all calls to
uploadFilein the codebase now accept an array of files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `uploadFile` to verify they use the updated signature. rg --type ts 'uploadFile\(' -A 2Length of output: 1045
446fb33 to
80b7b3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/video-module/src/module/helper/upload-videos.ts (1)
127-128: Efficient batch upload implementationThe addition of the check for non-empty
uploadFileListbefore callinguploadFileis a good practice. It ensures that the default upload method is only invoked when there are actually files to upload.Consider using optional chaining for a slightly more concise implementation:
- if (uploadFileList.length > 0) { await uploadFile(editor, uploadFileList) } + uploadFileList.length && await uploadFile(editor, uploadFileList);This change is optional and doesn't affect functionality, but it might align better with modern JavaScript practices.
packages/upload-image-module/src/module/upload-images.ts (1)
132-139: LGTM: Function body updated to handle multiple files.The changes correctly implement the logic for handling multiple file uploads. However, consider adding a unique
idto each file object for better tracking in Uppy:const uploadList = files.map((file, index) => ({ + id: `${file.name}_${Date.now()}_${index}`, name: file.name, type: file.type, size: file.size, data: file, }))This modification ensures that each file has a unique
id, which can be beneficial for Uppy's internal file tracking and potential debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/editor/examples/upload-image.html (3 hunks)
- packages/upload-image-module/tests/upload-files.test.ts (1 hunks)
- packages/upload-image-module/src/module/upload-images.ts (3 hunks)
- packages/video-module/src/module/helper/upload-videos.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/examples/upload-image.html
- packages/upload-image-module/tests/upload-files.test.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/video-module/src/module/helper/upload-videos.ts (1)
Line range hint
113-124: Improved flexibility in upload handlingThe changes in this section enhance the upload process by introducing more flexibility. The new
uploadFileListarray and the conditional logic for custom uploads allow for a more versatile approach to handling file uploads. This implementation supports both custom and default upload methods effectively.packages/upload-image-module/src/module/upload-images.ts (2)
Line range hint
155-173: LGTM: Main function updated to handle multiple file uploads.The changes correctly implement the logic for handling multiple file uploads, addressing different scenarios such as base64 encoding, custom uploads, and default uploads.
I notice that you've implemented the suggestion from a previous review to avoid calling
uploadFilewhen there are no files to upload:if (uploadFileList.length > 0) { await uploadFile(editor, uploadFileList) }This is a good practice to prevent unnecessary function calls.
130-130: LGTM: Function signature updated to handle multiple files.The change from
file: Filetofiles: File[]allows for processing multiple files in a single call, which is a good improvement in functionality.Please ensure that all calls to
uploadFileacross the codebase have been updated to pass an array of files. Run the following script to identify any outdated calls:✅ Verification successful
Verified: All
uploadFilecalls have been updated to accept an array of files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `uploadFile` in TypeScript files rg --type ts 'uploadFile\(' -A 2Length of output: 1045
| async function uploadFile(editor: IDomEditor, files: File[]) { | ||
| const uppy = getUppy(editor) | ||
|
|
||
| const { name, type, size } = file | ||
|
|
||
| uppy.addFile({ | ||
| name, | ||
| type, | ||
| size, | ||
| const uploadList = files.map(file => ({ | ||
| name: file.name, | ||
| type: file.type, | ||
| size: file.size, | ||
| data: file, | ||
| }) | ||
| })) | ||
|
|
||
| uppy.addFiles(uploadList) |
There was a problem hiding this comment.
Improved batch upload functionality, but consider resetting Uppy instance
The changes to uploadFile function improve its functionality by allowing batch uploads of multiple files. This is a good enhancement that increases efficiency.
However, the previous review comment about resetting the Uppy instance before adding new files is still relevant and should be addressed to prevent potential issues with residual uploads.
Consider applying this diff to reset the Uppy instance before adding new files:
async function uploadFile(editor: IDomEditor, files: File[]) {
const uppy = getUppy(editor)
+ uppy.reset()
const uploadList = files.map(file => ({
name: file.name,
type: file.type,
size: file.size,
data: file,
}))
uppy.addFiles(uploadList)
await uppy.upload()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function uploadFile(editor: IDomEditor, files: File[]) { | |
| const uppy = getUppy(editor) | |
| const { name, type, size } = file | |
| uppy.addFile({ | |
| name, | |
| type, | |
| size, | |
| const uploadList = files.map(file => ({ | |
| name: file.name, | |
| type: file.type, | |
| size: file.size, | |
| data: file, | |
| }) | |
| })) | |
| uppy.addFiles(uploadList) | |
| async function uploadFile(editor: IDomEditor, files: File[]) { | |
| const uppy = getUppy(editor) | |
| uppy.reset() | |
| const uploadList = files.map(file => ({ | |
| name: file.name, | |
| type: file.type, | |
| size: file.size, | |
| data: file, | |
| })) | |
| uppy.addFiles(uploadList) |
Changes Overview
Implementation Approach
Testing Done
Verification Steps
Additional Notes
Checklist
Related Issues
Summary by CodeRabbit
New Features
autoProceedoption for file uploads, allowing users to control when uploads start.Bug Fixes