fix: uid conflict when multiple upload components paste at same time #686
Conversation
|
@arksia is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Walkthrough在粘贴/拖放处理路径中，AjaxUploader 在接收的 Changes
Sequence Diagram(s)（已跳过：变更为修复并发 uid 覆盖的问题，属于漏洞修复，流程简单且不需序列图。） Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 分钟 Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
Summary of ChangesHello @arksia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求修复了一个关键错误，该错误影响了当多个组件同时处理粘贴文件时的文件上传功能。核心问题源于共享的文件引用导致 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
src/AjaxUploader.tsx
Outdated
| let files: File[] = [...(dataTransfer.files || [])].map((file: File) => { | ||
| return new File([file], file.name, { | ||
| type: file.type, | ||
| lastModified: file.lastModified, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
为了代码更简洁，这里的 map 函数可以简化为单行表达式，使用隐式返回。
| let files: File[] = [...(dataTransfer.files || [])].map((file: File) => { | |
| return new File([file], file.name, { | |
| type: file.type, | |
| lastModified: file.lastModified, | |
| }); | |
| }); | |
| let files: File[] = [...(dataTransfer.files || [])].map(file => ( | |
| new File([file], file.name, { | |
| type: file.type, | |
| lastModified: file.lastModified, | |
| }) | |
| )); |
| await sleep(100); | ||
| requests[0].respond(200, {}, `["","${files[0].name}"]`); |
There was a problem hiding this comment.
这个测试用例很好地覆盖了并发粘贴的场景。不过，它可以更健壮一些。目前它只完成了一个上传请求，并且没有验证两个组件生成的 uid 是唯一的。
我建议更新测试以：
- 断言创建了两个独立的请求。
- 断言
uid是不同的。 - 完成两个上传请求，以确保两个组件的生命周期回调都得到测试。
await sleep(100);
expect(requests).toHaveLength(2);
expect(uid1).toBeDefined();
expect(uid2).toBeDefined();
expect(uid1).not.toEqual(uid2);
// 完成两个上传
requests[0].respond(200, {}, `["","${files[0].name}"]`);
requests[1].respond(200, {}, `["","${files[0].name}"]`);
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #686 +/- ##
==========================================
+ Coverage 89.58% 89.62% +0.03%
==========================================
Files 6 6
Lines 317 318 +1
Branches 90 94 +4
==========================================
+ Hits 284 285 +1
Misses 33 33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
看下 gemini 的 review 建议 |
已根据 review 调整补充 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a concurrency bug where multiple upload components with pastable: true would interfere with each other when receiving the same paste/drop event simultaneously. The issue occurred because all components were mutating the same shared File objects from the DataTransfer API, causing UID conflicts and incorrect upload state tracking.
Key Changes:
- Creates new File instances from DataTransfer files to prevent shared object mutation
- Preserves essential file metadata (name, type, lastModified)
- Adds comprehensive test coverage for the concurrent paste scenario
- Updates existing tests to use consistent mocking patterns
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/AjaxUploader.tsx |
Implements fix by creating new File instances from dataTransfer.files using File constructor, preventing shared object mutations across multiple upload components |
tests/uploader.spec.tsx |
Adds test validating UID uniqueness when multiple components receive the same paste event; updates existing tests to use response mocking consistently |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(uidsInOnSuccess[0]).toBeDefined(); | ||
| expect(uidsInOnSuccess[1]).toBeDefined(); | ||
| expect(uidsInOnSuccess[0]).not.toEqual(uidsInOnSuccess[1]); | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected. Consider removing it for consistency with the codebase style.
|
|
||
| delete this.reqs[uid]; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This whitespace-only change appears to be unintentional. Consider keeping the blank line after the object definition for better readability.
| }; | |
| }; |
| value: i => files[i], | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
This whitespace-only change appears to be unintentional. Consider keeping the original blank line for consistency with the rest of the codebase.
src/AjaxUploader.tsx
Outdated
| let files: File[] = [...(dataTransfer.files || [])].map(file => ( | ||
| new File([file], file.name, { | ||
| type: file.type, | ||
| lastModified: file.lastModified, | ||
| }) | ||
| )); |
There was a problem hiding this comment.
// Clone File objects to avoid shared uid mutation between multiple Upload components
const files: File[] = Array.from(dataTransfer.files ?? [], file =>
new File([file], file.name, {
type: file.type,
lastModified: file.lastModified,
})
);There was a problem hiding this comment.
改为 Array.form 克隆文件数组，但还是得用 let，因为后面有对 files 赋值
if (directory) {
files = await traverseFileTree(Array.prototype.slice.call(items), this.filterFile);
this.uploadFiles(files);
} else {
let acceptFiles = [...files].filter(file => this.filterFile(file, true));
if (multiple === false) {
acceptFiles = files.slice(0, 1);
}
this.uploadFiles(acceptFiles);
}
2 participants
修复多个上传组件同时粘贴上传会覆写 dataTransfer 中文件的 uid，导致文件状态更新异常的问题。
图中两个文件上传都失败了，但是第一个文件始终处于 uploading 状态
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.