fix: supplement mistakenly deleted files #2736
Conversation
WalkthroughThis pull request introduces a comprehensive renderless component for file upload management in a mobile application. The implementation provides a robust set of functions to handle various aspects of file uploads, including file selection, clipboard paste, form data preparation, upload execution, abort functionality, and component lifecycle management. The code is designed to be flexible, supporting different upload scenarios with configurable options and event handling. Changes
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 (
|
|
Walkthrough: This pull request reintroduces files that were mistakenly deleted, specifically focusing on the upload component's renderless functionality. The changes involve handling file uploads, including paste and click events, and managing form data for uploads. Changes:
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (rawFile.isLargeFile) { | ||
| service.common.getChunkMergeUrl().then((url) => { |
There was a problem hiding this comment.
Ensure that service.common.getChunkMergeUrl() handles errors properly. If the promise is rejected, it could lead to unhandled promise rejections and potentially break the upload functionality.
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
packages/mobile/components/upload/src/renderless/index.ts (10)
24-24: Consider more robust detection for image formats
Currently, the checkstr.includes('image')can mesh with strings containing “image” in other contexts. For enhanced reliability, consider checking for the MIME prefix'image/'or verifying common extensions.
26-34: Add optional chaining for safer file access
When accessing(<HTMLInputElement>event.target).files, you could use optional chaining to avoid runtime errors, e.g.,event.target?.files.
66-105: Improve comprehensibility with helper functions
ThegetFormDatafunction includes nested conditionals and multiple appended fields. Consider extracting repeated logic (e.g., EDM handling vs. folder handling) into small helper methods for clarity and maintainability.
107-156: Use optional chaining for nested object property access
At line 120, you can simplify property checks, for example:if (!fileUploadTem?.state?.listeners?.exceed) { ... }to reduce guard code and improve readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
158-201: Post-upload concurrency considerations
If multiple files are processed in quick succession, ensure that any asynchronousbeforeUploadhooks don’t cause race conditions or incorrectly overwrite shared state variables.🧰 Tools
🪛 Biome (1.9.4)
[error] 169-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
203-240: Avoid using thedeleteoperator for performance
The repeated use ofdelete(lines 213 and 214) can degrade performance. Instead, assignundefinedor null, and rely on garbage collection.- delete reqs[uid] - delete state.cancelToken[uid] + reqs[uid] = undefined + state.cancelToken[uid] = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 217-217: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 218-218: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 220-220: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 234-234: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
328-362: Optional chain for property access
IngetOptionsOfHwh5(lines 335–338), the code referencesedm?.upload?.paramsoruploaderInner?.hwh5?.appId. This approach helps avoid potential null pointer issues.🧰 Tools
🪛 Biome (1.9.4)
[error] 336-336: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
364-404: Check concurrency for large file merges
When multiple large file uploads occur concurrently, be sure that the chunk-merge URL retrieval (service.common.getChunkMergeUrl()) is safe from race conditions or collisions.🧰 Tools
🪛 Biome (1.9.4)
[error] 390-390: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
406-452: Replace manual checks with optional chaining
At lines 445–446, you can useencryptConfig?.enabledorfileUploadVm?.state?.encryptDialogConfig.showto simplify the logic and reduce repetitive guard statements.🧰 Tools
🪛 Biome (1.9.4)
[error] 445-445: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
474-485: Potential memory optimization
Creating and freezing a new file input element on each component mount might increase memory usage if components re-mount frequently. Consider reusing or lazily instantiating it only when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/mobile/components/upload/src/renderless/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/mobile/components/upload/src/renderless/index.ts
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 169-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 217-217: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 218-218: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 220-220: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 234-234: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 288-288: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 297-297: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 303-303: 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)
[error] 336-336: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 390-390: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 445-445: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
packages/mobile/components/upload/src/renderless/index.ts (6)
36-64: Guard event default usage
Theevent.preventDefault()call automatically prevents the browser's default paste behavior. Confirm that this is intentional, as it might block other clipboard data from being pasted into relevant fields.
107-156: Validate partial uploads
If the user stops picking files mid-selection, consider verifying that the selectedfilesarray remains valid (e.g., no null entries).🧰 Tools
🪛 Biome (1.9.4)
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
242-274: Handle undefined properties gracefully
IngetOptionsOfPost, consider verifyingactionandheadersbefore using them to avoid potential undefined behavior.
454-462: Ensure accessibility for keyboard handlers
handleKeydownchecksEnterandSpace. Confirm that the target element is focusable and accessible so that screen readers properly indicate it as an interactive element.
464-472: Additional checks forfileobject
When updating a file (line 466–471), ensure thatfile.docIdaligns with the rest of the logic (like EDM checks). Mismatched docIds could lead to unexpected overwrites.
486-488: Reset state references
Ensure that cleaning upstate.updateInputis sufficient if the same component instance can re-mount. Check whether additional references or event listeners should also be cleared out.
| const modifyOptionsOfPost = ({ | ||
| service, | ||
| props, | ||
| options, | ||
| rawFile, | ||
| state, | ||
| uid, | ||
| uploaderInner, | ||
| api, | ||
| constants | ||
| }: Pick<IUploadRenderlessParams, 'service' | 'props' | 'state' | 'api' | 'constants'> & | ||
| IUploadRenderlessOtherParams & { options: IUploadOptionsOfPost }) => { | ||
| if (service && service.network && props.httpRequest === service.network.request) { | ||
| options.method = 'post' | ||
| options.url = options.action | ||
| options.onUploadProgress = (event) => { | ||
| if (props.onProgress) { | ||
| props.onProgress(event, rawFile) | ||
| } | ||
| } | ||
| delete options.action | ||
| delete options.onProgress | ||
| const formData = new FormData() as IUploadFormData | ||
| const source = service.network.CancelToken.source() | ||
| options.cancelToken = source.token | ||
| state.cancelToken[uid] = source.cancel | ||
| if (uploaderInner.edm.upload) { | ||
| !rawFile.isLargeFile && (options.method = 'put') | ||
| options.data = options.data || {} | ||
| } | ||
| if (options.data) { | ||
| Object.keys(options.data).forEach((key) => { | ||
| formData.append(key, options.data[key]) | ||
| }) | ||
| } | ||
| if (Array.isArray(rawFile)) { | ||
| rawFile.forEach((file) => formData.append(file.name, file.raw || file)) | ||
| } else { | ||
| if (state.isEdm) { | ||
| api.getFormData({ | ||
| formData, | ||
| file: rawFile, | ||
| type: !rawFile.isLargeFile ? constants.EDM.SINGLEUPLOAD : '' | ||
| }) | ||
| } else { | ||
| formData.append(options.filename, rawFile, rawFile.name) | ||
| } | ||
| } | ||
| options.data = formData | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate or rename complex logic
modifyOptionsOfPost performs multiple responsibilities: generating form data, setting up HTTP options, and handling large file logic. Consider splitting it to better adhere to S in SOLID (“single responsibility”).
🧰 Tools
🪛 Biome (1.9.4)
[error] 288-288: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 297-297: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 303-303: 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