feat(content-uploader): Implement cancelled state uploads manager#4578
Merged
Conversation
Contributor
|
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)
WalkthroughModernized upload completion now treats canceled items as terminal: in-progress checks ignore canceled items, finished checks include canceled, and ChangesCompletion Detection with Terminal Canceled Status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
dealwith
changed the base branch from
master
to
implement-cancel-all-retry-all-handlers
This was referenced May 27, 2026
dealwith
changed the title
This was referenced May 27, 2026
jpan-box
reviewed
May 27, 2026
jpan-box
reviewed
May 27, 2026
jpan-box
reviewed
May 27, 2026
dealwith
force-pushed
the
implement-cancel-all-retry-all-handlers
branch
from
90bbd3a to
4b36707
Compare
dealwith
force-pushed
the
implement-cancel-all-retry-all-handlers
branch
2 times, most recently
from
843f559 to
7947462
Compare
Base automatically changed from
implement-cancel-all-retry-all-handlers
to
master
May 28, 2026 17:24
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-uploader/__tests__/ContentUploader.test.js (1)
1199-1463: ⚡ Quick winRemove the duplicated
render()/enableModernizedUploadssuite block.Lines 1199-1463 re-declare the same suite already present earlier (Lines 809-1197). Keeping both doubles runtime and risks assertion drift between copies.
🤖 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/__tests__/ContentUploader.test.js` around lines 1199 - 1463, The test file contains a duplicated test suite: a second describe('render()', () => { describe('enableModernizedUploads', ...) block duplicates the earlier suite and should be removed; locate the duplicate block that contains multiple tests referencing UploadsManager, UploadsManagerBP, DroppableContent and methods like updateViewAndCollection, handleCancelAllUploads, and handleRetryAllUploads, and delete that entire duplicated describe block so only the original suite remains.
🤖 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/elements/content-uploader/__tests__/ContentUploader.test.js`:
- Line 1093: The test creates a persistent spy with jest.spyOn(UploaderUtils,
'isMultiputSupported').mockReturnValue(true) in the
"handleUploadsManagerRetryAll should call onClickResume for resumable chunked
items" test but never restores it, causing leak into other tests; update that
test to call UploaderUtils.isMultiputSupported.mockRestore() in its own
finally/afterEach or restore the spy at the end of the test, or alternatively
use jest.spyOn(...).mockImplementationOnce(...) so the mock is scoped to that
single invocation; ensure you reference the spy created on
UploaderUtils.isMultiputSupported and the existing cleanup pattern used in the
getUploadAPI() describe (UploaderUtils.isMultiputSupported.mockRestore()) to
keep behavior isolated and prevent cross-test leakage.
---
Nitpick comments:
In `@src/elements/content-uploader/__tests__/ContentUploader.test.js`:
- Around line 1199-1463: The test file contains a duplicated test suite: a
second describe('render()', () => { describe('enableModernizedUploads', ...)
block duplicates the earlier suite and should be removed; locate the duplicate
block that contains multiple tests referencing UploadsManager, UploadsManagerBP,
DroppableContent and methods like updateViewAndCollection,
handleCancelAllUploads, and handleRetryAllUploads, and delete that entire
duplicated describe block so only the original suite remains.
🪄 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: 523d7142-a2f9-4799-8f71-6b1b387a1214
📒 Files selected for processing (6)
src/common/types/upload.jssrc/constants.jssrc/elements/content-uploader/ContentUploader.tsxsrc/elements/content-uploader/__tests__/ContentUploader.test.jssrc/elements/content-uploader/utils/__tests__/mapToModernizedUploadItem.test.tssrc/elements/content-uploader/utils/mapToModernizedUploadItem.ts
dealwith
force-pushed
the
implement-cancelled-state-uploads-manager
branch
from
c9536b2 to
c794314
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/elements/content-uploader/__tests__/ContentUploader.test.js (4)
1045-1058:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse STATUS_CANCELED constant instead of raw string.
Line 1055 uses
status: 'canceled'instead ofSTATUS_CANCELED. Update to use the constant for consistency with the rest of the test file.🔧 Proposed fix
instance.updateViewAndCollection([ { status: STATUS_COMPLETE, file: { name: 'a' } }, - { status: 'canceled', file: { name: 'b' } }, + { status: STATUS_CANCELED, file: { name: 'b' } }, ]);🤖 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/__tests__/ContentUploader.test.js` around lines 1045 - 1058, The test "updateViewAndCollection should fire onComplete when at least one item completes (modernized)" uses a raw string 'canceled' for the status; replace that with the STATUS_CANCELED constant used elsewhere in the test file so the test uses the canonical symbol; update the object in the call to instance.updateViewAndCollection (the second item's status) to STATUS_CANCELED to match other tests and avoid fragile string usage.
1129-1157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore the isMultiputSupported spy to prevent test leakage.
Line 1149 creates a spy with
jest.spyOn(UploaderUtils, 'isMultiputSupported').mockReturnValue(true)but never restores it. The onlymockRestore()for this spy is in thedescribe('getUploadAPI()')afterEach at line 553, which doesn't apply to this test.Add cleanup to prevent the mock from leaking into other tests.
🔧 Proposed fix
Add restore after the test assertions:
expect(onClickResume).toHaveBeenCalledWith(resumable); expect(resumable.bytesUploadedOnLastResume).toBe(1024); + UploaderUtils.isMultiputSupported.mockRestore(); });Or use
mockReturnValueOnceto limit scope:- jest.spyOn(UploaderUtils, 'isMultiputSupported').mockReturnValue(true); + jest.spyOn(UploaderUtils, 'isMultiputSupported').mockReturnValueOnce(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/__tests__/ContentUploader.test.js` around lines 1129 - 1157, The test "handleUploadsManagerRetryAll should call onClickResume for resumable chunked items" creates a persistent spy on UploaderUtils.isMultiputSupported which leaks into other tests; fix by either calling mockRestore() on the spy after the assertions or replace mockReturnValue(true) with mockReturnValueOnce(true) so the mock only affects this test; locate the spy created via jest.spyOn(UploaderUtils, 'isMultiputSupported') in this test and add the appropriate cleanup (mockRestore) or change to mockReturnValueOnce to limit scope.
1060-1071:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse STATUS_CANCELED constant instead of raw string.
Line 1068 uses
status: 'canceled'instead ofSTATUS_CANCELED. Update to use the constant.🔧 Proposed fix
instance.updateViewAndCollection([ { status: STATUS_COMPLETE, file: { name: 'a' } }, - { status: 'canceled', file: { name: 'b' } }, + { status: STATUS_CANCELED, file: { name: 'b' } }, ]);🤖 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/__tests__/ContentUploader.test.js` around lines 1060 - 1071, The test uses the raw string 'canceled' for an item's status; change it to use the STATUS_CANCELED constant so the test stays in sync with the codebase. In the test "updateViewAndCollection should treat canceled items as terminal (modernized)" update the second item's status to STATUS_CANCELED and ensure STATUS_CANCELED is available in the test scope (import it or reference it from the same module where STATUS_COMPLETE is coming from) before running the assertion against updateViewAndCollection.
1029-1043:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse STATUS_CANCELED constant instead of raw string.
Lines 1037 and 1039 use the raw string
'canceled'instead of theSTATUS_CANCELEDconstant. The actual code usesSTATUS_CANCELED, so tests should match for consistency and correctness. Changestatus: 'canceled'tostatus: STATUS_CANCELED.🔧 Proposed fix
const instance = wrapper.instance(); - const canceled = { status: 'canceled' }; + const canceled = { status: STATUS_CANCELED }; instance.updateViewAndCollection([🤖 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/__tests__/ContentUploader.test.js` around lines 1029 - 1043, Replace raw 'canceled' status strings in the test with the STATUS_CANCELED constant: in the test "updateViewAndCollection should not fire onComplete when all items are canceled (modernized)" update the two objects that set status: 'canceled' to status: STATUS_CANCELED so the test uses the same constant as the implementation (locate the test around updateViewAndCollection and the onComplete mock).
🤖 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/elements/content-uploader/__tests__/ContentUploader.test.js`:
- Around line 1045-1058: The test "updateViewAndCollection should fire
onComplete when at least one item completes (modernized)" uses a raw string
'canceled' for the status; replace that with the STATUS_CANCELED constant used
elsewhere in the test file so the test uses the canonical symbol; update the
object in the call to instance.updateViewAndCollection (the second item's
status) to STATUS_CANCELED to match other tests and avoid fragile string usage.
- Around line 1129-1157: The test "handleUploadsManagerRetryAll should call
onClickResume for resumable chunked items" creates a persistent spy on
UploaderUtils.isMultiputSupported which leaks into other tests; fix by either
calling mockRestore() on the spy after the assertions or replace
mockReturnValue(true) with mockReturnValueOnce(true) so the mock only affects
this test; locate the spy created via jest.spyOn(UploaderUtils,
'isMultiputSupported') in this test and add the appropriate cleanup
(mockRestore) or change to mockReturnValueOnce to limit scope.
- Around line 1060-1071: The test uses the raw string 'canceled' for an item's
status; change it to use the STATUS_CANCELED constant so the test stays in sync
with the codebase. In the test "updateViewAndCollection should treat canceled
items as terminal (modernized)" update the second item's status to
STATUS_CANCELED and ensure STATUS_CANCELED is available in the test scope
(import it or reference it from the same module where STATUS_COMPLETE is coming
from) before running the assertion against updateViewAndCollection.
- Around line 1029-1043: Replace raw 'canceled' status strings in the test with
the STATUS_CANCELED constant: in the test "updateViewAndCollection should not
fire onComplete when all items are canceled (modernized)" update the two objects
that set status: 'canceled' to status: STATUS_CANCELED so the test uses the same
constant as the implementation (locate the test around updateViewAndCollection
and the onComplete mock).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35b7f20d-9879-4c7b-b6b3-cb233f5b8a6b
📒 Files selected for processing (2)
src/elements/content-uploader/ContentUploader.tsxsrc/elements/content-uploader/__tests__/ContentUploader.test.js
Treat STATUS_CANCELED as terminal in updateViewAndCollection so the view machine no longer reports canceled batches as in-progress. Suppress the upload-success notification when every item in the batch was canceled. Both behaviors gate on enableModernizedUploads; the legacy flow keeps its previous logic.
dealwith
force-pushed
the
implement-cancelled-state-uploads-manager
branch
from
c794314 to
011434c
Compare
Contributor
|
Actionable comments posted: 0 |
…ndling - Suppress onComplete on the partial-upload and non-manager paths when every item was canceled, matching the existing manager-path guard. - Replace raw 'canceled' string literals with STATUS_CANCELED in tests. - Group canceled-batch tests under a shared describe + beforeEach. - Tighten view assertion to expect VIEW_UPLOAD_SUCCESS instead of just not VIEW_UPLOAD_IN_PROGRESS.
Contributor
|
Actionable comments posted: 0 |
These keys belong to the Cancel All confirmation modal feature on a later branch in the stack. Nothing in this PR consumes them, so they should not land here.
jpan-box
approved these changes
May 28, 2026
Collaborator
jpan-box
left a comment
jpan-box
left a comment
There was a problem hiding this comment.
All addressed — STATUS_CANCELED constant, positive view assertion, and shared test setup look good. Nice catch on the partial-upload guard from Rene too.
olehrybak
approved these changes
May 28, 2026
Collaborator
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
This pull request spent 1 minute 12 seconds in the queue, including 8 seconds running CI. Required conditions to merge
|
Contributor
Merge Queue Status
Waiting for
All conditions
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Make
STATUS_CANCELEDa terminal state insideContentUploader.updateViewAndCollectionwhen the modernized flow is on. Without this, canceled items keep the view stuck inVIEW_UPLOAD_IN_PROGRESSandonCompletenever fires for batches where every item was canceled3/5 PR in the queue:
Summary by CodeRabbit