feat(content-uploader): implement cancel all freeze#4665
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds pause and resume support to ChangesPause/Resume Upload Flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
![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 (2)
src/api/uploads/__tests__/MultiputUpload.test.js (1)
805-813: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
reset()is called for the paused in-flight parts.This test verifies
pause()calls but not the pairedreset()behavior that restores upload-ready counters/state. Add reset expectations so a regression that only pauses without resetting is caught.Suggested assertions
expect(multiputUploadTest.numPartsDigestReady).toBe(2); + expect(multiputUploadTest.parts[0].reset).toBeCalled(); expect(multiputUploadTest.parts[0].pause).toBeCalled(); + expect(multiputUploadTest.parts[1].reset).not.toBeCalled(); expect(multiputUploadTest.parts[1].pause).not.toBeCalled(); + expect(multiputUploadTest.parts[2].reset).toBeCalled(); expect(multiputUploadTest.parts[2].pause).toBeCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/uploads/__tests__/MultiputUpload.test.js` around lines 805 - 813, The pause test for MultiputUpload only checks that in-flight parts are paused, but it does not verify the paired reset behavior that should restore upload-ready state. Update the test around multiputUploadTest.pause() to also assert that reset() is called on the paused in-flight parts (the same parts that receive pause()), using the existing parts array and pause/reset spies so regressions that omit reset() are caught.src/elements/content-uploader/ContentUploader.tsx (1)
1326-1340: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFlip the pause guard before invoking upload APIs.
Line 1340 sets
isCancelAllPausedafterapi.pause()/api.cancel(). If those calls synchronously trigger progress/error callbacks,upload()can still start pending work before the freeze guard is active.Proposed fix
pauseUploadsForCancelAll = () => { + this.isCancelAllPaused = true; this.itemsRef.current.forEach(item => { // Folder containers have no partial state and their child files are // tracked as separate queue items handled by this same loop. if (item.status !== STATUS_IN_PROGRESS || item.isFolder) { return; @@ api.cancel(); } }); - this.isCancelAllPaused = true; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-uploader/ContentUploader.tsx` around lines 1326 - 1340, The cancel-all pause guard in pauseUploadsForCancelAll is being set too late, so synchronous callbacks from api.pause() or api.cancel() can still let upload() proceed. Move the isCancelAllPaused assignment to before iterating items and invoking the upload APIs, and keep the existing STATUS_IN_PROGRESS and isFolder checks intact so the freeze guard is active before any callbacks fire.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/uploads/MultiputUpload.js`:
- Around line 1285-1295: The pause scan in the Multipart upload flow can run
past the end of the parts list if numPartsUploading and the part states get out
of sync, so bound the loop by this.parts.length in the upload pause path. Update
the logic around this.firstUnuploadedPartIndex in MultiputUpload.js so the scan
stops once all parts have been checked, while still resetting and pausing any
PART_STATE_UPLOADING entries and adjusting numPartsUploading/numPartsDigestReady
as needed.
In `@src/elements/content-uploader/ContentUploader.tsx`:
- Around line 1231-1236: The chunked-upload check in
ContentUploader.getIsChunkedUpload is recomputing upload mode from the mutable
chunked prop, which can diverge from the API chosen when the item was queued.
Update the logic around getIsChunkedUpload and any pause/cancel handling to use
the actual upload API capability or a mode flag stored on the UploadItem at
creation time, so active uploads are classified consistently even if props
change.
---
Nitpick comments:
In `@src/api/uploads/__tests__/MultiputUpload.test.js`:
- Around line 805-813: The pause test for MultiputUpload only checks that
in-flight parts are paused, but it does not verify the paired reset behavior
that should restore upload-ready state. Update the test around
multiputUploadTest.pause() to also assert that reset() is called on the paused
in-flight parts (the same parts that receive pause()), using the existing parts
array and pause/reset spies so regressions that omit reset() are caught.
In `@src/elements/content-uploader/ContentUploader.tsx`:
- Around line 1326-1340: The cancel-all pause guard in pauseUploadsForCancelAll
is being set too late, so synchronous callbacks from api.pause() or api.cancel()
can still let upload() proceed. Move the isCancelAllPaused assignment to before
iterating items and invoking the upload APIs, and keep the existing
STATUS_IN_PROGRESS and isFolder checks intact so the freeze guard is active
before any callbacks fire.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f6b439c-924c-4fa6-ac83-5fb278f125fb
📒 Files selected for processing (4)
src/api/uploads/MultiputUpload.jssrc/api/uploads/__tests__/MultiputUpload.test.jssrc/elements/content-uploader/ContentUploader.tsxsrc/elements/content-uploader/__tests__/ContentUploader.test.js
| let nextUploadIndex = this.firstUnuploadedPartIndex; | ||
| while (this.numPartsUploading > 0) { | ||
| const part = this.parts[nextUploadIndex]; | ||
| if (part && part.state === PART_STATE_UPLOADING) { | ||
| part.reset(); | ||
| part.pause(); | ||
| this.numPartsUploading -= 1; | ||
| this.numPartsDigestReady += 1; | ||
| } | ||
| nextUploadIndex += 1; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the pause scan by the parts array.
Line 1286 loops until numPartsUploading reaches zero, but the loop can run past this.parts.length forever if the counter and part states drift. Since Cancel All calls pause() synchronously, that can hang the UI instead of freezing it.
Suggested bounded scan
- let nextUploadIndex = this.firstUnuploadedPartIndex;
- while (this.numPartsUploading > 0) {
- const part = this.parts[nextUploadIndex];
+ let numPartsPaused = 0;
+ for (let nextUploadIndex = this.firstUnuploadedPartIndex; nextUploadIndex < this.parts.length; nextUploadIndex += 1) {
+ const part = this.parts[nextUploadIndex];
if (part && part.state === PART_STATE_UPLOADING) {
part.reset();
part.pause();
- this.numPartsUploading -= 1;
- this.numPartsDigestReady += 1;
+ numPartsPaused += 1;
}
- nextUploadIndex += 1;
}
+
+ this.numPartsUploading -= numPartsPaused;
+ this.numPartsDigestReady += numPartsPaused;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let nextUploadIndex = this.firstUnuploadedPartIndex; | |
| while (this.numPartsUploading > 0) { | |
| const part = this.parts[nextUploadIndex]; | |
| if (part && part.state === PART_STATE_UPLOADING) { | |
| part.reset(); | |
| part.pause(); | |
| this.numPartsUploading -= 1; | |
| this.numPartsDigestReady += 1; | |
| } | |
| nextUploadIndex += 1; | |
| } | |
| let numPartsPaused = 0; | |
| for (let nextUploadIndex = this.firstUnuploadedPartIndex; nextUploadIndex < this.parts.length; nextUploadIndex += 1) { | |
| const part = this.parts[nextUploadIndex]; | |
| if (part && part.state === PART_STATE_UPLOADING) { | |
| part.reset(); | |
| part.pause(); | |
| numPartsPaused += 1; | |
| } | |
| } | |
| this.numPartsUploading -= numPartsPaused; | |
| this.numPartsDigestReady += numPartsPaused; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/uploads/MultiputUpload.js` around lines 1285 - 1295, The pause scan
in the Multipart upload flow can run past the end of the parts list if
numPartsUploading and the part states get out of sync, so bound the loop by
this.parts.length in the upload pause path. Update the logic around
this.firstUnuploadedPartIndex in MultiputUpload.js so the scan stops once all
parts have been checked, while still resetting and pausing any
PART_STATE_UPLOADING entries and adjusting numPartsUploading/numPartsDigestReady
as needed.
| getIsChunkedUpload = (item: UploadItem): boolean => { | ||
| const { chunked } = this.props; | ||
| const { file, isFolder } = item; | ||
| return Boolean( | ||
| chunked && !isFolder && file && file.size > CHUNKED_UPLOAD_MIN_SIZE_BYTES && isMultiputSupported(), | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid recomputing chunkedness from mutable props.
Line 1232 classifies an existing item using the current chunked prop, but the concrete API was selected when the item was queued. If chunked changes while uploads are active, this can call api.pause() on a plain API or cancel a real chunked upload instead of pausing it. Prefer checking the actual API capability or storing the upload mode on the item when it is created.
Proposed fix
getIsChunkedUpload = (item: UploadItem): boolean => {
- const { chunked } = this.props;
- const { file, isFolder } = item;
+ const { api, file, isFolder } = item;
return Boolean(
- chunked && !isFolder && file && file.size > CHUNKED_UPLOAD_MIN_SIZE_BYTES && isMultiputSupported(),
+ !isFolder &&
+ file &&
+ file.size > CHUNKED_UPLOAD_MIN_SIZE_BYTES &&
+ api &&
+ typeof api.pause === 'function' &&
+ typeof api.unpause === 'function',
);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getIsChunkedUpload = (item: UploadItem): boolean => { | |
| const { chunked } = this.props; | |
| const { file, isFolder } = item; | |
| return Boolean( | |
| chunked && !isFolder && file && file.size > CHUNKED_UPLOAD_MIN_SIZE_BYTES && isMultiputSupported(), | |
| ); | |
| getIsChunkedUpload = (item: UploadItem): boolean => { | |
| const { api, file, isFolder } = item; | |
| return Boolean( | |
| !isFolder && | |
| file && | |
| file.size > CHUNKED_UPLOAD_MIN_SIZE_BYTES && | |
| api && | |
| typeof api.pause === 'function' && | |
| typeof api.unpause === 'function', | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/elements/content-uploader/ContentUploader.tsx` around lines 1231 - 1236,
The chunked-upload check in ContentUploader.getIsChunkedUpload is recomputing
upload mode from the mutable chunked prop, which can diverge from the API chosen
when the item was queued. Update the logic around getIsChunkedUpload and any
pause/cancel handling to use the actual upload API capability or a mode flag
stored on the UploadItem at creation time, so active uploads are classified
consistently even if props change.
d714dd5 to
a2b499e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/api/uploads/MultiputUpload.js (1)
1285-1295: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the pause scan by the parts array.
while (this.numPartsUploading > 0)at Line 1286 never decrements the counter oncenextUploadIndexruns pastthis.parts.length(the indexedpartbecomesundefined, so theifbody is skipped). IfnumPartsUploadingand the per-part states ever drift, this loop spins forever, and since Cancel All invokespause()synchronously it hangs the UI instead of freezing it. Bound the iteration bythis.parts.length.Suggested bounded scan
- let nextUploadIndex = this.firstUnuploadedPartIndex; - while (this.numPartsUploading > 0) { - const part = this.parts[nextUploadIndex]; + let numPartsPaused = 0; + for (let nextUploadIndex = this.firstUnuploadedPartIndex; nextUploadIndex < this.parts.length; nextUploadIndex += 1) { + const part = this.parts[nextUploadIndex]; if (part && part.state === PART_STATE_UPLOADING) { part.reset(); part.pause(); - this.numPartsUploading -= 1; - this.numPartsDigestReady += 1; + numPartsPaused += 1; } - nextUploadIndex += 1; } + + this.numPartsUploading -= numPartsPaused; + this.numPartsDigestReady += numPartsPaused;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/uploads/MultiputUpload.js` around lines 1285 - 1295, The pause scan in `MultiputUpload.pause()` can loop forever because it only checks `this.numPartsUploading` and keeps advancing `nextUploadIndex` past `this.parts.length` when no matching `part` is found. Update the loop to also bound iteration by `this.parts.length` (using `nextUploadIndex` and the `this.parts` array length) so the scan stops even if `numPartsUploading` and part states drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/api/uploads/MultiputUpload.js`:
- Around line 1285-1295: The pause scan in `MultiputUpload.pause()` can loop
forever because it only checks `this.numPartsUploading` and keeps advancing
`nextUploadIndex` past `this.parts.length` when no matching `part` is found.
Update the loop to also bound iteration by `this.parts.length` (using
`nextUploadIndex` and the `this.parts` array length) so the scan stops even if
`numPartsUploading` and part states drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13eca6dd-5dbd-419f-9db2-1c0f097c2814
📒 Files selected for processing (4)
src/api/uploads/MultiputUpload.jssrc/api/uploads/__tests__/MultiputUpload.test.jssrc/elements/content-uploader/ContentUploader.tsxsrc/elements/content-uploader/__tests__/ContentUploader.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/content-uploader/tests/ContentUploader.test.js
- src/api/uploads/tests/MultiputUpload.test.js
- src/elements/content-uploader/ContentUploader.tsx
tjuanitas
left a comment
tjuanitas
left a comment
There was a problem hiding this comment.
I think the implementation could be renamed to work for all pause / resume but lgtm
|
|
||
| isAutoExpanded: boolean = false; | ||
|
|
||
| isCancelAllPaused: boolean = false; |
There was a problem hiding this comment.
I think based on the implementation of the methods, you could have a generic isPaused state and pauseUploads / resumeUploads instead of making it specific to "Cancel All"
Merge Queue Status
This pull request spent 11 minutes in the queue, including 10 minutes 43 seconds running CI. Required conditions to merge
|
4 participants
Context:
In the modernized Uploads Manager the new feature "cancel all" is slow in the UI. Users have to click cancel all and then confirm the in the modal. In order to make operation a bit faster we decided to pause upload at the time user clicks cancel all.
Summary by CodeRabbit