fix(file-upload): Fix bug in file-upload component where multiple selections are merge-service and uploaded #3424
Conversation
WalkthroughA helper function Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/renderless/src/file-upload/index.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
chenxi-20
changed the title
WalkthroughThis PR addresses a bug in the file upload component where multiple file selections were incorrectly merged and uploaded as a single file. The changes introduce a mechanism to handle multiple file uploads correctly by checking file types and ensuring only valid files are processed. Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| const isAcceptType = (acceptArray, file, constants) => { | ||
| return acceptArray.some((type) => { | ||
| if (type.toLowerCase() === constants.IMAGE_TYPE) { | ||
| return constants.FILE_TYPE.PICTURE.split('/').includes(fileType) |
There was a problem hiding this comment.
fileType is used here without being defined in the scope of isAcceptType. This will lead to a reference error. Ensure fileType is properly defined or passed to the function.
chenxi-20
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/renderless/src/file-upload/index.ts (1)
253-253: Add proper type annotation for the isMergeUpload parameter.The function signature has been updated with a new parameter
isMergeUploadbut it lacks a type annotation, which is inconsistent with the other parameters.Apply this fix:
- (file: IFileUploadFile, autoRemove: boolean, doUpload: Function, isMergeUpload) => { + (file: IFileUploadFile, autoRemove: boolean, doUpload: Function, isMergeUpload: boolean) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/renderless/src/file-upload/index.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (1)
packages/renderless/src/file-upload/index.ts (1)
266-286: The file filtering logic for merged uploads is a solid improvement.The implementation correctly handles both merged and single file upload scenarios:
- For merged uploads, it filters out files that don't match the accept types
- For single file uploads, it maintains the original validation behavior
This should effectively fix the issue where multiple file selections could include invalid files when merged.
| @@ -612,7 +631,7 @@ | |||
| const handler = (file) => | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(file.raw) | |||
|
|
|||
| rawFiles.length && api.beforeUpload({ raw: rawFiles }, true, handler) | |||
| rawFiles.length && api.beforeUpload({ raw: rawFiles }, true, handler, true) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify this implementation properly handles all edge cases.
The fix for the multiple file selection bug looks good, but please verify that this implementation properly handles all edge cases, especially when:
- Some files in the selection are invalid
- All files in the selection are invalid
- Very large batches of files are selected at once
Run the following test to verify the behavior:
🏁 Script executed:
#!/bin/bash
# Search for all usages of beforeUpload and check for any potential edge cases
# Look for places where beforeUpload is called
rg "beforeUpload\(" --type ts -A 3 -B 3Length of output: 5563
🏁 Script executed:
#!/bin/bash
# Extract definition of beforeUpload in file-upload/index.ts
rg -n "export.*beforeUpload" -A50 packages/renderless/src/file-upload/index.ts
rg -n "const beforeUpload" -A50 packages/renderless/src/file-upload/index.ts
rg -n "function beforeUpload" -A50 packages/renderless/src/file-upload/index.tsLength of output: 3909
🏁 Script executed:
#!/bin/bash
# Print lines 290 to 360 of beforeUpload implementation for full context
sed -n '290,360p' packages/renderless/src/file-upload/index.tsLength of output: 1961
Ensure beforeUpload fully covers merge‐upload edge cases
I’ve reviewed the beforeUpload implementation in
packages/renderless/src/file-upload/index.ts around the multiple‐file branch:
// caller for merged uploads
rawFiles.length && api.beforeUpload({ raw: rawFiles }, true, handler, true)The isMergeUpload path currently:
- Silently filters out invalid files via
remove({ …file: f }) - Does not early‐exit if all files are invalid (it will still invoke
props.beforeUpload/doUploadwith an emptyrawarray) - Suppresses user feedback for each invalid file, unlike the single‐file path which shows a warning
Please adjust to handle these scenarios:
• Some invalid files:
– After filtering, if file.raw.length < originalCount, display a warning (e.g. “3 of 5 files were rejected.”)
• All invalid files:
– If file.raw.length === 0, abort and show a consolidated Modal message, do not call beforeUpload/doUpload
• Very large batches:
– Confirm that a large rawFiles array does not degrade performance—consider batching or deferring UI updates if necessary
Suggested diff snippet:
if (isMergeUpload) {
const originalCount = file.raw.length
const fileRow: File[] = []
file.raw.flatMap(f => {
if (accept && !isAcceptType(acceptArray, f, constants)) {
- remove({ api, file: f, autoRemove })
+ remove({ api, file: f, autoRemove })
} else {
fileRow.push(f)
}
return getFileType({ file: f })
})
file.raw = fileRow
+ // edge‐case: no valid files left
+ if (file.raw.length === 0) {
+ return Modal.message({
+ message: t(constants.EDM.NO_VALID_FILES_SELECTED),
+ status: 'warning'
+ })
+ }
+
+ // edge‐case: some files invalid
+ if (file.raw.length < originalCount) {
+ Modal.message({
+ message: t(constants.EDM.SOME_FILES_REJECTED, { count: originalCount - file.raw.length }),
+ status: 'warning'
+ })
+ }
}These changes will ensure that users get clear, consolidated feedback and that we never proceed with an empty upload.
chenxi-20
force-pushed
the
fix/file-upload-20250514
branch
from
b6eb3a3 to
3f7b9a7
Compare
chenxi-20
force-pushed
the
fix/file-upload-20250514
branch
from
3f7b9a7 to
fe7ea25
Compare
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes