feat(content-uploader): allow host app to reset upload items state by olehrybak · Pull Request #4617 · box/box-ui-elements

olehrybak · 2026-06-09T15:07:44Z

Summary

Currently the state of the upload items is reset internally in the ContentUploader by calling resetUploadsManagerItemsWhenUploadsComplete method with a timer. This PR allows to register resetUploadsManagerItemsWhenUploadsComplete as a callback that can be used in the host application using setClearItemsCallback and disables internal state reset for modernized uploader. This change ensures that the modernized uploads experience is more flexible and can be adjusted to the requirements in the host applications.

Summary by CodeRabbit

Bug Fixes
- Improved upload-item clearing so items are retained while the upload view/manager is active and are reliably cleared when uploads complete, including with the modernized uploads flow; added support for wiring an external “clear items after uploads complete” callback.
Tests
- Added coverage for lifecycle and clearing behaviors to prevent regressions in upload cleanup logic.

coderabbitai · 2026-06-09T15:07:59Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8c249aa9-4b4a-41e1-a17f-35dc3c3851dc

📥 Commits

Reviewing files that changed from the base of the PR and between 5cdef38 and c0650f0.

📒 Files selected for processing (1)

src/elements/content-uploader/ContentUploader.tsx

🚧 Files skipped from review as they are similar to previous changes (1)

src/elements/content-uploader/ContentUploader.tsx

Walkthrough

ContentUploader adds an optional setClearItemsCallback prop that parents can use; componentDidMount wires resetUploadsManagerItemsWhenUploadsComplete when provided. Clearing behavior now skips delayed clear when enableModernizedUploads is true and adjusts early-return logic to account for that flag and manager/view state.

Changes

Upload Item Clearing with Modernized Uploads

Layer / File(s)	Summary
Callback contract and mount wiring `src/elements/content-uploader/ContentUploader.tsx`	`ContentUploaderProps` adds an optional `setClearItemsCallback` prop. On mount, the component calls it with `resetUploadsManagerItemsWhenUploadsComplete` when provided; on unmount it registers a noop.
Clearing behavior for modernized uploads `src/elements/content-uploader/ContentUploader.tsx`	`checkClearUploadItems` returns immediately when `enableModernizedUploads` is true (disabling delayed clear). `resetUploadsManagerItemsWhenUploadsComplete` updates its early-return logic to exclude the expanded-manager/items short-circuit when modernized uploads are enabled while keeping the `VIEW_UPLOAD_IN_PROGRESS` short-circuit.
Lifecycle and clearing behavior tests `src/elements/content-uploader/__tests__/ContentUploader.test.js`	Adds tests: mount wiring for `setClearItemsCallback`, `checkClearUploadItems` scheduling behavior with fake timers for modernized vs legacy flows, and multiple `resetUploadsManagerItemsWhenUploadsComplete` scenarios covering manager expansion, `useUploadsManager`, `view`, and the feature flag.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

box/box-ui-elements#4571: Both PRs modify src/elements/content-uploader/ContentUploader.tsx around the same enableModernizedUploads feature flag—feat(uploads-manager): add enableModernizedUploads feature flag to ContentUploader #4571 introduces the flag and modernized render path, while this PR adjusts upload-manager item-clearing behavior when that flag is enabled.

Suggested labels

ready-to-merge

Suggested reviewers

jpan-box
tjiang-box
reneshen0328

Poem

🐰 A tiny rabbit hops and gleams,
I wire a callback into streams,
When uploads finish, I softly sweep,
Clearing items while others sleep,
A tidy trail where data gleams.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: allowing host applications to reset upload items state through a callback mechanism.
Description check	✅ Passed	The description clearly explains the purpose (registering a callback for host app control), the problem it solves (disabling internal timer-based reset for modernized uploads), and the benefits (flexibility for host applications).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Omfalos

Lgtm

reneshen0328 · 2026-06-10T15:28:19Z

        const { isUploadsManagerExpanded, view } = this.state;

-        // Do not reset items when upload manger is expanded or there're uploads in progress
+        // Do not reset items when upload manger is expanded (for non-modernized uploader) or there're uploads in progress


Typo? manager instead of manger

reneshen0328 · 2026-06-10T15:36:31Z

    uploadHost: string;
    useUploadsManager?: boolean;
    enableModernizedUploads?: boolean;
+    setClearItemsCallback?: (callback: () => void) => void;


[Question] Does the host only calls this while the uploader is visibly mounted? If not perhaps it's safer to add this as a no-op into componentWillUnmount()?
Please correct me if I'm wrong but here's the concern:
The callback registered via setClearItemsCallback gives the host app a direct reference to this.resetUploadsManagerItemsWhenUploadsComplete. If ContentUploader unmounts while the host still holds that reference:

Host calls the function → this.setState(...) fires on an unmounted component (React will silently swallow it, but onCancel still fires against stale itemsRef data)

The host has no way to know the call was a no-op

Yeah, makes sense, added setting of the clear callback to noop on unmount

reneshen0328

Code logic makes sense to me, approved!

tjuanitas · 2026-06-10T18:34:11Z

    uploadHost: string;
    useUploadsManager?: boolean;
    enableModernizedUploads?: boolean;
+    setClearItemsCallback?: (callback: () => void) => void;


~~can we follow the naming pattern of other props and call this something like onClear / onReset?~~

nvm I see what's happening here. why do you need to call the reset callback outside of ContentUploader?

Ideally, all the functionality should live within the UI Element (ContentUploader) and not within the host applications. The UI Elements (like ContentUploader, ContentSidebar, etc) are supposed to be self-contained, essentially "uncontrolled" components. We publish these elements for external customers to use in their own applications.

feat(content-uploader): allow host app to reset upload items state

286221f

olehrybak marked this pull request as ready for review June 10, 2026 09:27

olehrybak requested review from a team as code owners June 10, 2026 09:27

Omfalos approved these changes Jun 10, 2026

View reviewed changes

test: add test coverage

5cdef38

Omfalos approved these changes Jun 10, 2026

View reviewed changes

reneshen0328 reviewed Jun 10, 2026

View reviewed changes

reneshen0328 previously approved these changes Jun 10, 2026

View reviewed changes

tjuanitas reviewed Jun 10, 2026

View reviewed changes

fix: set clear callback to noop on uploader unmount

c0650f0

olehrybak dismissed reneshen0328’s stale review via c0650f0 June 10, 2026 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(content-uploader): allow host app to reset upload items state#4617

feat(content-uploader): allow host app to reset upload items state#4617
olehrybak wants to merge 3 commits into
box:masterfrom
olehrybak:modernized-uploads-clear-state

olehrybak commented Jun 9, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading

Uh oh!

Omfalos left a comment

Uh oh!

reneshen0328 Jun 10, 2026

Uh oh!

reneshen0328 Jun 10, 2026

Uh oh!

olehrybak Jun 10, 2026

Uh oh!

reneshen0328 left a comment

Uh oh!

tjuanitas Jun 10, 2026 •

edited

Loading

Uh oh!

tjuanitas Jun 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

olehrybak commented Jun 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

Uh oh!

Omfalos left a comment

Choose a reason for hiding this comment

Uh oh!

reneshen0328 Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

reneshen0328 Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

olehrybak Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

reneshen0328 left a comment

Choose a reason for hiding this comment

Uh oh!

tjuanitas Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjuanitas Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olehrybak commented Jun 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading

tjuanitas Jun 10, 2026 •

edited

Loading