feat(Upload): use local preview instead of base64 format #6157
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideThis PR replaces the existing Base64-based image preview workflow with a faster, local object-URL approach by adding a file reader utility in JavaScript, wiring up a client-side preview URL API, and adjusting C# upload components and samples to use that API instead of Base64. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6157 +/- ##
=======================================
Coverage 99.99% 99.99%
=======================================
Files 704 704
Lines 31093 31099 +6
Branches 4394 4395 +1
=======================================
+ Hits 31092 31098 +6
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- Consider revoking the created object URLs (via URL.revokeObjectURL) when they’re no longer needed to avoid accumulating memory leaks.
- You can simplify the preview logic by calling URL.createObjectURL(file) directly instead of reading into an ArrayBuffer and re-wrapping into a Blob.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const preventHandler = e => e.preventDefault(); | ||
| const body = el.querySelector('.upload-drop-body'); | ||
| const upload = { el, body, preventHandler } | ||
| const inputFile = el.querySelector('[type="file"]'); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): inputFile can be null; add a null-check before usage
Add a guard or early return to handle cases where inputFile is null to avoid runtime errors.
|
|
||
| /** | ||
| * @param {File} file | ||
| * @returns {Blob} |
Contributor
| /// </summary> | ||
| /// <param name="file"></param> | ||
| /// <returns></returns> | ||
| protected override async Task TriggerOnChanged(UploadFile file) |
Contributor
There was a problem hiding this comment.
suggestion: Image preview logic duplicated here as well
Consider refactoring this logic into a shared base class or utility method to avoid duplication.
Comment on lines
+142
to
+147
| // 从客户端获得预览地址不使用 base64 编码 | ||
| if (file.IsImage(AllowExtensions, CanPreviewCallback)) | ||
| { | ||
| await OnChange(file); | ||
| file.PrevUrl = await InvokeAsync<string?>("getPreviewUrl", Id, file.OriginFileName); | ||
| } | ||
| await base.TriggerOnChanged(file); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Non-image files retain previous PrevUrl
Reset PrevUrl for non-image files to prevent showing outdated previews.
Suggested change
| // 从客户端获得预览地址不使用 base64 编码 | |
| if (file.IsImage(AllowExtensions, CanPreviewCallback)) | |
| { | |
| await OnChange(file); | |
| file.PrevUrl = await InvokeAsync<string?>("getPreviewUrl", Id, file.OriginFileName); | |
| } | |
| await base.TriggerOnChanged(file); | |
| // 从客户端获得预览地址不使用 base64 编码 | |
| if (file.IsImage(AllowExtensions, CanPreviewCallback)) | |
| { | |
| file.PrevUrl = await InvokeAsync<string?>("getPreviewUrl", Id, file.OriginFileName); | |
| } | |
| else | |
| { | |
| // 非图片文件重置预览地址，防止显示过期预览 | |
| file.PrevUrl = null; | |
| } | |
| await base.TriggerOnChanged(file); |
| }) | ||
| } | ||
|
|
||
| export async function getPreviewUrl(id, fileName) { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring getPreviewUrl to use early returns and optional chaining to reduce nesting and intermediate variables.
Here’s a way to flatten out the nesting in getPreviewUrl (and even drop the manual let url = '' accumulator) by using early-returns and optional chaining:
export async function getPreviewUrl(id, fileName) {
const files = Data.get(id)?.files;
if (!files) return '';
// find the File object
const file = Array.from(files).find(f => f.name === fileName);
if (!file) return '';
// readFileAsync → blob URL
const blob = await readFileAsync(file);
return blob ? URL.createObjectURL(blob) : '';
}If you’re not actually transforming the file (just wrapping it in a blob URL), you can simplify further and drop readFileAsync entirely:
export function getPreviewUrl(id, fileName) {
const files = Data.get(id)?.files || [];
const file = files.find(f => f.name === fileName);
return file ? URL.createObjectURL(file) : '';
}Both versions preserve the current behavior while reducing indentation, intermediate variables, and nested if blocks.
| * @param {File} file | ||
| * @returns {Blob} | ||
| */ | ||
| export function readFileAsync(file) { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider removing or simplifying the readFileAsync helper to avoid unnecessary FileReader logic.
Suggested change
| export function readFileAsync(file) { | |
| The new `readFileAsync` helper isn’t needed since `File` already implements the `Blob` interface. You can either: | |
| 1. Drop the helper entirely and pass `file` wherever a `Blob` is expected, or | |
| 2. If you really need an immutable copy, use `File.prototype.slice` instead of a `FileReader` round‐trip. | |
| Example (identity / no‐op helper): | |
| ```js | |
| /** @param {File} file | |
| * @returns {Promise<Blob>} | |
| */ | |
| export function readFileAsync(file) { | |
| return Promise.resolve(file); | |
| } |
Or (clone via slice):
/** @param {File} file
* @returns {Promise<Blob>}
*/
export function readFileAsync(file) {
const blobCopy = file.slice(0, file.size, file.type);
return Promise.resolve(blobCopy);
}Both eliminate the 20+ lines of FileReader logic while preserving all functionality.```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #6156
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Implement local preview for image uploads by replacing base64 image requests with Blob URL previews via a new JavaScript API, update upload components and samples to use the new mechanism, and adjust tests accordingly.
New Features:
Enhancements:
Tests: