fix(file-upload): modify the error reported by the upload component on the mobile client and add the demo #2724
Conversation
WalkthroughThis pull request introduces comprehensive modifications to the file upload component across multiple files in the mobile application. The changes primarily focus on refining the Changes
Sequence DiagramsequenceDiagram
participant User
participant FileUploadComponent
participant FileUploadService
User->>FileUploadComponent: Select Files
FileUploadComponent->>FileUploadComponent: Validate Files
alt Files Valid
FileUploadComponent->>FileUploadService: Upload Files
FileUploadService-->>FileUploadComponent: Upload Response
FileUploadComponent-->>User: Show Upload Status
else Files Invalid
FileUploadComponent-->>User: Show Error Message
end
Possibly related PRs
Suggested Labels
Suggested Reviewers
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
WalkthroughThis pull request addresses a bug fix in the file upload component for mobile clients. It modifies error reporting and adds a demo to illustrate the changes. The update involves changes in demo configurations and the removal of certain demo files. Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| @@ -315,7 +315,7 @@ export const startUpload = | |||
| }: Pick<IFileUploadRenderlessParams, 'state' | 'constants' | 'vm' | 'api' | 't'> & IFileUploadModalVm) => | |||
| (file: IFileUploadFile, isList: boolean) => { | |||
| if (state.isHwh5) { | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(file.raw) | |||
| vm.$refs[constants.UPLOAD_INNER].upload(file.raw) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the upload call.
| @@ -335,7 +335,7 @@ export const startUpload = | |||
| status: 'warning' | |||
| }) | |||
| } else { | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(file.raw) | |||
| vm.$refs[constants.UPLOAD_INNER].upload(file.raw) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the upload call.
| @@ -629,16 +629,14 @@ export const handleStart = | |||
|
|
|||
| if (!state.isEdm && props.autoUpload) { | |||
| if (props.multiple && props.mergeService) { | |||
| const handler = (file) => | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(file.raw) | |||
| const handler = (file) => vm.$refs[constants.UPLOAD_INNER].upload(file.raw) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the upload call.
|
|
||
| rawFiles.length && api.beforeUpload({ raw: rawFiles }, true, handler) | ||
| } else { | ||
| rawFiles.forEach((rawFile) => { | ||
| const file = api.getFile(rawFile) | ||
| if (!file) return | ||
| const handler = (file) => | ||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(file.raw) | ||
| const handler = (file) => vm.$refs[constants.UPLOAD_INNER].upload(file.raw) |
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the upload call.
| @@ -863,7 +861,7 @@ export const handleReUpload = | |||
| const { READY } = constants.FILE_STATUS | |||
| file.status = READY | |||
| file.percentage = 0 | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(file.raw) | |||
| vm.$refs[constants.UPLOAD_INNER].upload(file.raw) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the upload call.
| @@ -918,7 +916,7 @@ export const abort = | |||
| }) | |||
| } | |||
|
|
|||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].abort(file) | |||
| vm.$refs[constants.UPLOAD_INNER].abort(file) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the abort call.
| @@ -990,12 +988,12 @@ export const submit = | |||
| const rawFiles = files.map((file) => file.raw) | |||
| rawFiles.length && | |||
| api.beforeUpload({ raw: rawFiles }, false, (file) => { | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(file.raw) | |||
| vm.$refs[constants.UPLOAD_INNER].upload(file.raw) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the upload call.
| }) | ||
| } else { | ||
| files.forEach((file) => { | ||
| api.beforeUpload(file, false, (file) => { | ||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(file.raw) | ||
| vm.$refs[constants.UPLOAD_INNER].upload(file.raw) |
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the upload call.
| @@ -1005,7 +1003,7 @@ export const submit = | |||
| export const handleClick = | |||
| ({ constants, vm }: Pick<IFileUploadRenderlessParams, 'constants' | 'vm'>) => | |||
| () => | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].handleClick() | |||
| vm.$refs[constants.UPLOAD_INNER].handleClick() | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the handleClick call.
| @@ -2121,7 +2119,7 @@ export const batchSegmentUpload = | |||
| } else { | |||
| typeof props.edm.upload.closeloading === 'function' && props.edm.upload.closeloading() | |||
|
|
|||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].upload(state.largeFileInfo[docId]) | |||
| vm.$refs[constants.UPLOAD_INNER].upload(state.largeFileInfo[docId]) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the upload call.
| @@ -2623,7 +2621,7 @@ export const getFileSourceType = | |||
| export const updateFile = | |||
| ({ constants, vm }: Pick<IFileUploadRenderlessParams, 'constants' | 'vm'>) => | |||
| (file: IFileUploadFile) => { | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].handleUpdate(file) | |||
| vm.$refs[constants.UPLOAD_INNER].handleUpdate(file) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the handleUpdate call.
| @@ -2634,7 +2632,7 @@ export const handleChange = | |||
| if (!Array.isArray(file)) { | |||
| files = [file] | |||
| } | |||
| vm.$refs[constants.UPLOAD_INNER].$refs[constants.UPLOAD_INNER_TEMPLATE].handleChange({ target: { files } }) | |||
| vm.$refs[constants.UPLOAD_INNER].handleChange({ target: { files } }) | |||
There was a problem hiding this comment.
Refactored to remove unnecessary nested reference to UPLOAD_INNER_TEMPLATE, simplifying the handleChange call.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
examples/sites/demos/mobile/app/file-upload/file-icon-list.vue (1)
2-8: Inline title might need localization supportAdding
file-title="我是标题"helps illustrate usage but it’s hard-coded in Chinese. If your project supports multiple languages, consider extracting this string for localization or i18n, ensuring it’s easily translatable.packages/mobile/components/file-upload/src/mobile.vue (1)
53-53: ReturningpictureArrwithin the.map()callback.
Although you’re returningpictureArr, consider whether this return value is actually needed in the.map()chain or if aforEachmight be cleaner.- uploaArr && - uploaArr.map((item) => { - // ... - return pictureArr - }) + if (uploaArr) { + uploaArr.forEach((item) => { + // ... + }) + }packages/mobile/components/file-upload/src/renderless/index.ts (3)
639-639:beforeUploadinvocation.
Be mindful if any UI blocking or concurrency issues might arise if large sets of files are processed.
919-919: Global abort call.
Callingabort()with no file argument correctly sets all files inFAILstate. Be mindful of partial states if new files are being added concurrently.
2635-2635: Trigger handleChange with a custom file array.
Good approach for programmatic file addition. Consider adding input validation if malicious file objects appear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
examples/sites/demos/apis/file-upload.js(10 hunks)examples/sites/demos/mobile/app/file-upload/accept-file-image.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/custom-prefix.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/custom-upload-request.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/drag-select-file.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/drag-upload.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/file-icon-list.vue(1 hunks)examples/sites/demos/mobile/app/file-upload/file-title.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/image-size.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/manual-upload.vue(1 hunks)examples/sites/demos/mobile/app/file-upload/prevent-delete-file.vue(3 hunks)examples/sites/demos/mobile/app/file-upload/prevent-upload-file.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/upload-limit.vue(0 hunks)examples/sites/demos/mobile/app/file-upload/webdoc/file-upload.js(1 hunks)packages/mobile/components/file-upload/src/file-upload.ts(2 hunks)packages/mobile/components/file-upload/src/mobile.vue(2 hunks)packages/mobile/components/file-upload/src/renderless/index.ts(10 hunks)packages/mobile/components/file-upload/src/renderless/vue.ts(3 hunks)packages/mobile/components/upload-list/src/renderless/vue.ts(1 hunks)packages/mobile/components/upload-list/src/upload-list.ts(2 hunks)packages/mobile/components/upload/src/renderless/index.ts(0 hunks)packages/mobile/components/upload/src/renderless/vue.ts(1 hunks)
💤 Files with no reviewable changes (10)
- examples/sites/demos/mobile/app/file-upload/drag-upload.vue
- examples/sites/demos/mobile/app/file-upload/file-title.vue
- examples/sites/demos/mobile/app/file-upload/custom-upload-request.vue
- examples/sites/demos/mobile/app/file-upload/prevent-upload-file.vue
- examples/sites/demos/mobile/app/file-upload/accept-file-image.vue
- examples/sites/demos/mobile/app/file-upload/upload-limit.vue
- examples/sites/demos/mobile/app/file-upload/drag-select-file.vue
- examples/sites/demos/mobile/app/file-upload/image-size.vue
- packages/mobile/components/upload/src/renderless/index.ts
- examples/sites/demos/mobile/app/file-upload/custom-prefix.vue
🧰 Additional context used
🪛 Biome (1.9.4)
packages/mobile/components/file-upload/src/renderless/index.ts
[error] 2624-2624: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (56)
examples/sites/demos/mobile/app/file-upload/manual-upload.vue (1)
2-2: Removeddisplayattribute and usedheader-showprop – verify potential impactYou’ve removed the
displayattribute from<tiny-file-upload>and retained:header-show="false". Make sure this change doesn’t introduce regressions in any other demos or usage scenarios that relied on the olddisplayprop.examples/sites/demos/mobile/app/file-upload/prevent-delete-file.vue (3)
8-8: Validate newly addedsuccess-statisticspropThe
success-statisticsprop is informative but ensure that corresponding logic exists downstream to process or track these statistics. Without handling or documentation, it might be unclear to users how to leverage this feature.
17-17: ImportingTinyModalfor confirmationsGood move to centralize modal usage rather than calling
window.confirm. This fosters a consistent UI/UX across the app.
37-41: Changes in removal flow – confirm dialog behaviorSwitching from
window.confirmtoTinyModal.confirmis a solid improvement. However, ensure downstream consumers handle the rejected promise appropriately, especially if any logic expects an immediate boolean return.packages/mobile/components/upload/src/renderless/vue.ts (1)
68-70: ExpandeduploadInnerto reference entire uploader instancePointing
uploadInnerdirectly to the entire uploader broadens its scope. Verify any existing code or computed properties relying on a more granular reference still function correctly, especially if they expect a template ref. Also confirm that code referencinguploadInner.stateoruploadInner.edmremains stable.packages/mobile/components/upload-list/src/upload-list.ts (2)
28-29: Exporting theISharedRenderlessParamHookstype is consistent and clear.
No functionality issues appear here. Good job adding back the export for shared usage in other components.
119-120: Adding thereUploadableBoolean prop for toggling re-upload functionality.
Ensure that any dependent logic in the upload list or file upload flow checks this property before showing a re-upload button or trigger.You could add a default value (e.g.,
false) for clarity:- reUploadable: Boolean + reUploadable: { + type: Boolean, + default: false + }✅ Verification successful
Let me gather information about the usage and dependencies of this new
reUploadableprop.
Based on the search results, I can now provide a final response about the
reUploadableprop and its dependencies:The
reUploadableprop is properly integrated with re-upload functionality across the codebaseThe codebase shows that:
- The prop is used to control re-upload UI elements and functionality in both mobile and PC components
- Re-upload buttons/triggers only appear when
reUploadableis true and file status is 'fail'- The prop is checked before removing failed files from the upload list
- Re-upload functionality is properly wired through events and handlers
Key integrations:
packages/mobile/components/file-upload/src/renderless/index.ts: ChecksreUploadablebefore removing failed filespackages/mobile/components/upload-list/src/mobile.vue: Conditionally renders re-upload button based onreUploadableand file statuspackages/vue/src/upload-list/src/pc.vue: Similar conditional rendering for PC version🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for any existing usage of reUploadable prop rg "reUploadable" -l # Look for re-upload related UI elements or logic rg -i "reupload" -A 3 # Check the component implementation cat "packages/mobile/components/upload-list/src/upload-list.ts"Length of output: 63922
packages/mobile/components/upload-list/src/renderless/vue.ts (1)
67-68: Reassigningvmto the injected uploader.
Switching from the parent to the injecteduploaderis valid, but ensure thatvmindeed carries everything you need (constants, etc.). Verify that other references toparentin the code remain consistent.packages/mobile/components/file-upload/src/mobile.vue (3)
14-14: Replacing direct imports with the existingvue-commonreferences.
No issues here—this maintains consistent import practices.
23-23: ImportingfileUploadPropscentralizes prop definitions.
This reduces duplication and keeps prop definitions in one place.
27-27: Using the consolidatedfileUploadProps.
This simplifies prop management and helps ensure consistency across different file upload components.examples/sites/demos/mobile/app/file-upload/webdoc/file-upload.js (23)
3-4: Column and owner changes.
No functional issues. Verify whether “owner” is populated later if needed.
5-16: Addedmultiple-filedemo configuration.
Demonstrates multiple file selection. Looks correct and consistent with the docs.
17-29: Addedmanual-uploaddemo configuration.
Showcases manual uploading with explicitauto-uploadoff. This is aligned with best practices.
30-43: Addedheader-showdemo configuration.
Toggles the header, with an optionalfile-titlecustomization. Straightforward usage.
44-57: Addedfile-icon-listdemo.
Your approach for specifying icon paths is clear. No obvious issues.
58-70: Addedaccept-filedemo.
Highlights restricting file types. This properly showcasesacceptusage.
71-84: Addedmax-file-countdemo.
Implementslimitand optional hiding of the button. Looks good.
85-98: Addedprevent-delete-filedemo.
Demonstrates how to block file deletion. The logic seems sound.
99-112: Addedupload-file-listdemo.
Supports toggling the file list’s visibility and enabling file downloads. Well-structured example.
113-125: Addedupload-http-requestdemo.
Showcasing custom HTTP requests is a valuable addition. No concerns.
126-139: Addedpicture-carddemo.
Demonstrates photo wall mode and preview. Proper usage of thelist-type="picture-card"property.
140-151: Addedsizedemo.
Defines a smaller or larger upload button. Straightforward configuration.
152-164: Addeddatademo.
Showcases additional upload parameters and theupload-iconusage. Clear and correct.
165-176: Addedmini-modedemo.
Enables a minimal interface with straightforward toggles.
177-188: Addeddynamic-disabledemo.
Disables upload according to user input or conditions. Basic but effective demonstration.
189-202: Addedupload-requestdemo for custom request headers.
No issues. Looks consistent with the rest of the upload architecture.
203-214: Addedwith-credentialsdemo.
Demonstrates cookie credential usage. This is a nice complement to the standard scenarios.
215-228: Addedclear-filesdemo.
Exemplifies manual clearing of the file list. Straightforward usage.
229-240: Addedabort-questdemo.
Showcases how to cancel an upload in progress. Good demonstration.
241-253: Addedcustom-triggerdemo.
Slots for customizing triggers are well explained, with no issues found.
254-267: Addedcustom-upload-tipdemo.
LeveragingtipandreUploadableto customize the re-upload flow. Great improvement.
[approve]
268-281: Addedupload-eventsdemo.
Comprehensive overview of events likepreview,remove,erroretc. No concerns.
282-282: No further structural changes beyond the newdemosarray.
Everything is wrapped properly and consistent with the new items above.packages/mobile/components/file-upload/src/renderless/vue.ts (3)
296-296: Use optional chaining carefully for EDMS.
The optional chaining onprops.edm?.uploadprevents runtime errors but ensure you handle null or undefined cases for deeper nested properties insideprops.edm.uploadif needed.
334-334: Good practice to replaceparentwithvm.
This makes the code more consistent with Vue’s recommended usage of the component instance.
357-357: Injected dependency clarity.
Providing'uploader'withvmensures consistent injection logic. No action needed, just acknowledging this design approach.packages/mobile/components/file-upload/src/file-upload.ts (3)
88-89: Importing$propsimproves maintainability.
Integrating$propsfrom a central file can help you avoid duplicating definitions across multiple files.
90-90: Duplicate type export confirmation.
ExportingISharedRenderlessParamHooksagain might be intentional to ensure availability. Confirm it doesn’t create conflicts in the consuming modules.
191-191: Merging$propsis a sensible design choice.
Spreading$propsintofileUploadPropscentralizes prop definitions and simplifies updates.examples/sites/demos/apis/file-upload.js (9)
18-18: Propertyacceptmobile demo updated.
Renaming demo references helps maintain consistency with actual usage scenarios.
74-74: Propertybefore-removemobile demo updated.
Good to see self-descriptive naming (e.g.,'prevent-delete-file').
89-89: Propertybefore-uploaddemo alignment.
Switching to'upload-request'clarifies the custom request demonstration.
162-162: Propertydisplaymobile demo removed.
Ensure you intentionally removed the prior'manual-upload'reference if it’s no longer valid.
267-267: Propertyfile-title→'header-show'.
This aligns better with the newly introduced logic for toggling header visibility.
378-378: Propertylimit→'max-file-count'.
Updated name is more descriptive and user-friendly.
403-403: Propertylist-type→'picture-card'.
Helps unify naming schemes across demos.
651-651: Propertysuccess-statistics→'prevent-delete-file'.
Check if the property’s function is truly relevant to “prevent delete” or if the name needs further refinement.
900-900: Methodabort→'abort-quest'.
Descriptive naming clarifies the user-facing demo, good job.packages/mobile/components/file-upload/src/renderless/index.ts (7)
318-318: Mobile HWH5 block clarity.
Theif (state.isHwh5)condition is well-placed to separate mobile logic.
338-338: Large-file fallback.
After checking file size logic, gracefully falling back to normal upload is correct.
632-632: Conditional uploading with merge services.
Ensure that therawFiles.lengthcondition handles empty arrays to prevent corner-case errors.
864-864: Re-upload scenario check.
Make sure re-upload logic resets all relevant file properties, such asstatus,percentage, andresponse.
1006-1006: Direct click invocation.
vm.$refs[constants.UPLOAD_INNER].handleClick()bypasses any additional checks. Confirm no extra validations are needed.
2122-2122: Delegated large-file final upload.
Resuming the upload with the large-file info is clean. Keep an eye on concurrency or partial states.
2624-2624: False positive from static analysis on assignment.
The code here is a simple function call with no assignment. You can safely ignore the lint insinuation unless there’s truly an assignment hidden in an expression.🧰 Tools
🪛 Biome (1.9.4)
[error] 2624-2624: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
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
Based on the comprehensive summary of changes, here are the release notes:
New Features
Bug Fixes
Refactoring
Removals
Documentation