feat(content-uploader): Implement cancel all retry all handlers#4577
feat(content-uploader): Implement cancel all retry all handlers#4577dealwith wants to merge 9 commits into
Conversation
Wire @box/uploads-manager UploadsManager into ContentUploader behind the enableModernizedUploads flag. Maps legacy upload state to the shared feature's item shape and delegates per-item cancel/retry/remove actions to existing handlers.
Add STATUS_CANCELED constant. ContentUploader gains handleCancelAllUploads, handleRetryAllUploads, plus per-item cancel and retry handlers used by the modernized uploads manager. Canceled items keep their entry in the list rather than being removed. All behavior is gated on the enableModernizedUploads flag; the legacy flow is unchanged.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
b7bf9cc to
4acdc6a
Compare
…ntent-uploader' into implement-cancel-all-retry-all-handlers # Conflicts: # src/elements/content-uploader/ContentUploader.tsx # src/elements/content-uploader/__tests__/ContentUploader.test.js # src/elements/content-uploader/utils/__tests__/mapToModernizedUploadItem.test.ts # src/elements/content-uploader/utils/mapToModernizedUploadItem.ts
… handlers Mirror legacy onClick behavior in handleModernizedRetryAll and handleModernizedItemRetry: drop ERROR_CODE_ITEM_NAME_IN_USE items instead of looping on the same conflict, and fire onClickResume or onClickRetry per item so consumer telemetry stays consistent with the per-item flow. Snapshot the retry targets before iterating since removeFileFromUploadQueue mutates itemsRef.current. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…retry flows Cover Cancel All confirmation, Retry All on errored batches, and single-item cancel/retry hover actions behind enableModernizedUploads. useUploadsManager is required so the modernized panel actually expands. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| | typeof STATUS_STAGED | ||
| | typeof STATUS_COMPLETE | ||
| | typeof STATUS_ERROR; | ||
| // TODO: replace with `UploadItemStatus` from @box/uploads-manager once 'inprogress' is aligned to 'uploading'. |
There was a problem hiding this comment.
i think we could add | typeof STATUS_CANCELED to the existing type instead of refactoring the whole union to inline strings. with inline strings the type and constants might drift. unless there is a strong reason to replace these all with inline strings..
i think adding 'canceled' is fine since that's additive. but the TODO mentions replacing 'inprogress' to 'uploading'. that would be a breaking change for consumers using useUploadsManager: true, since onComplete, onUpload, onClickCancel etc. all pass UploadItem with status directly to consumers. So i just want to make sure that's an "add" and not a "replace".
| markItemCanceled = (item: UploadItem) => { | ||
| const { onClickCancel } = this.props; | ||
| const { api } = item; | ||
| if (api && typeof api.cancel === 'function') { |
There was a problem hiding this comment.
i don't think we need the typeof api.cancel === 'function' check here. cancel is defined as a method on both upload API classes:
box-ui-elements/src/api/uploads/MultiputUpload.js
Line 1224 in 90bbd3a
and api seems to always be constructed internally by ContentUploader so there's no path i can see where this would be something other than a function.
| item => item.status === STATUS_PENDING || item.status === STATUS_IN_PROGRESS, | ||
| ); | ||
| cancelable.forEach(item => this.markItemCanceled(item)); | ||
| if (cancelable.length > 0) { |
There was a problem hiding this comment.
is this guard ever reachable? I woudl think UploadsManagerBP only renders the Cancel All button when hasActiveItems is true in the shared feature which means cancelable will always have items by the time this statement runs.
| }; | ||
|
|
||
| handleModernizedItemAction = (id: string) => { | ||
| handleModernizedItemCancel = (id: string) => { |
There was a problem hiding this comment.
same feedback as #4573 on the naming. handleModernizedItemCancel reads like the item cancel is modernized, not that this is a cancel handler for the modernized component.
| name: 'b.pdf', | ||
| extension: 'pdf', | ||
| progress: 0, | ||
| status: 'canceled', |
There was a problem hiding this comment.
this uses the string 'canceled' while the rest of the test file uses imported constants (STATUS_ERROR, STATUS_COMPLETE, etc.). consider using STATUS_CANCELED for consistency.
| ), | ||
| ]; | ||
|
|
||
| export const modernizedSingleUpload = { |
There was a problem hiding this comment.
the visual state of UploadsManagerBP should be covered by the shared feature's own tests. we're generally trying to reduce VRTs, so i'd leave it up to you on whether we'd lose any reliability if we removed or replaced these with unit tests
| } | ||
| }; | ||
|
|
||
| handleModernizedItemRetry = (id: string) => { |
There was a problem hiding this comment.
the retry logic here (name-in-use check, resumable vs restart) is duplicated from handleModernizedRetryAll. a shared helper that both call would make it easier to keep them in sync if the retry behavior changes.
box-ui-elements/src/elements/content-uploader/ContentUploader.tsx
Lines 1205 to 1223 in 90bbd3a
box-ui-elements/src/elements/content-uploader/ContentUploader.tsx
Lines 1318 to 1336 in 90bbd3a
jpan-box
left a comment
There was a problem hiding this comment.
great work and thank you for splitting up this feature into different pr, but there are some conventions that we really want to stick to, and also the naming modernized sticks out a lot to me.
| const item = this.findItemByModernizedId(id); | ||
| if (item) { | ||
| this.onClick(item); | ||
| if (!item) { |
There was a problem hiding this comment.
same concern as #4573. if findItemByModernizedId doesn't find a match, this silently returns. we could catch any unhandled scenarios here with an else. not sure how you want to surface that though.
Summary
handleModernizedCancelAllandhandleModernizedRetryAllwired to the panel'sonCancelAll / onRetryAll.handleModernizedItemActioninto dedicatedhandleModernizedItemCancel / handleModernizedItemRetryso each calls the right legacy callback (onClickCancel,onClickRetry/onClickResume) and respects status preconditions.markItemCanceledhelper: cancels viaapi.cancel(), sets status toSTATUS_CANCELED, firesonClickCancel. Shared by single-item and bulk cancel.STATUS_CANCELEDto constants and the 'canceled' status to theUploadStatus Flowunion; extendmapToModernizedUploadItemSTATUS_MAPwith the canceled entry so canceled rows surface in the modernized panel.UploadStatusfrom internal status constants — declare the union as string literals with a TODO to align withUploadItemStatusfrom@box/uploads-manageronce the 'inprogress' value is migrated to 'uploading'.2/5 PR in the queue: