feat(content-uploader): add drop support for modernized uploads manager#4674
feat(content-uploader): add drop support for modernized uploads manager#4674bkepka-box wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a ChangesModernized Uploads Manager Drop Zone Tests
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)src/elements/content-uploader/__tests__/ContentUploader.test.jsast-grep timed out on this file 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-uploader/ContentUploader.tsx (1)
658-687: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSnapshot
rootFolderIdfor the modernized flowsrc/elements/content-uploader/ContentUploader.tsx:658-687The modernized drop zone is active wheneverenableModernizedUploadsis true, even ifuseUploadsManageris false, so this guard should match that path. Otherwise dropped items skip the folder snapshot and can upload into a laterrootFolderId.🤖 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 658 - 687, The modernized upload path in ContentUploader.tsx does not snapshot rootFolderId for all modernized drops because shouldAddRootFolderIdToUploadOptions is gated by useUploadsManager too narrowly. Update the upload-item creation flow in the newItems map so the folderId snapshot is applied whenever enableModernizedUploads is enabled for the modernized drop zone, using rootFolderId and getUploadAPI/getFileAPIOptions logic in ContentUploader to ensure dropped files keep the original folder context.
🧹 Nitpick comments (1)
src/elements/content-uploader/ModernizedUploadsManagerDropZone.tsx (1)
14-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared drop-type check
ModernizedUploadsManagerDropZoneandDroppableContentnow keep the same Array/DOMStringList intersection logic in two places; pull that into a shared helper and keep only thecanDropgate here.🤖 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/ModernizedUploadsManagerDropZone.tsx` around lines 14 - 40, The Array/DOMStringList intersection logic in ModernizedUploadsManagerDropZone is duplicated with DroppableContent, so extract that shared drop-type check into a common helper and reuse it here. Keep dropDefinition.dropValidator focused on the canDrop gate plus a call to the shared helper, and preserve the existing allowedTypes/types behavior in ModernizedUploadsManagerDropZone and the shared utility used by DroppableContent.
🤖 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.
Outside diff comments:
In `@src/elements/content-uploader/ContentUploader.tsx`:
- Around line 658-687: The modernized upload path in ContentUploader.tsx does
not snapshot rootFolderId for all modernized drops because
shouldAddRootFolderIdToUploadOptions is gated by useUploadsManager too narrowly.
Update the upload-item creation flow in the newItems map so the folderId
snapshot is applied whenever enableModernizedUploads is enabled for the
modernized drop zone, using rootFolderId and getUploadAPI/getFileAPIOptions
logic in ContentUploader to ensure dropped files keep the original folder
context.
---
Nitpick comments:
In `@src/elements/content-uploader/ModernizedUploadsManagerDropZone.tsx`:
- Around line 14-40: The Array/DOMStringList intersection logic in
ModernizedUploadsManagerDropZone is duplicated with DroppableContent, so extract
that shared drop-type check into a common helper and reuse it here. Keep
dropDefinition.dropValidator focused on the canDrop gate plus a call to the
shared helper, and preserve the existing allowedTypes/types behavior in
ModernizedUploadsManagerDropZone and the shared utility used by
DroppableContent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3f5c79c-f9d9-4e63-a3ac-918b663d4236
📒 Files selected for processing (3)
src/elements/content-uploader/ContentUploader.tsxsrc/elements/content-uploader/ModernizedUploadsManagerDropZone.tsxsrc/elements/content-uploader/__tests__/ContentUploader.test.js
|
|
||
| const dropDefinition = { | ||
| dropValidator: ( | ||
| { allowedTypes, isDropEnabled = true }: ModernizedUploadsManagerDropZoneProps, |
There was a problem hiding this comment.
sorry should dropenabled be true or false? i saw this
f8fb79c#diff-71da4c8322f44025d67f136a0fd91e42a42d269dfc74654fa335bc7d24cc5c88R175-R177
There was a problem hiding this comment.
It should be false at the ContentUploader level by default because use should not be able to drop files when canDropOnUploadsManager is ommited. The DropZone component defaults to true only when used directly.
| @@ -678,7 +682,9 @@ class ContentUploader extends Component<ContentUploaderProps, State> { | |||
| }; | |||
|
|
|||
| if (uploadAPIOptions) { | |||
There was a problem hiding this comment.
i know this isn't a part of your pr but one thing i've been looking for in recent PRs are unnecessary guards. is uploadAPIOptions ever falsy for a file? i see it's defined a few lines above
and i see that function always returns something ie it should be truthy
box-ui-elements/src/utils/uploads.js
Lines 76 to 79 in f8fb79c
in other repos i know we have quality gates for cognitive load (ie extra or unused if conditions), but not so much in this one, however it might still be good to clean this up while you're in here. up to you though on how much creep you're ok with in this pr
There was a problem hiding this comment.
I would like to avoid changing it as part of this PR because I do not have enough context why this guard is there. If something breaks it could force us to revert the whole drop support as well, so I’d rather keep this PR scoped to the related changes.
| try { | ||
| renderModernizedUploadsManagerDropZone(); | ||
|
|
||
| fireEvent.dragEnter(screen.getByTestId('bcu-modernized-panel'), { dataTransfer }); |
There was a problem hiding this comment.
i know this test id was already in here but i have to ask, is it totally necessary to use it here? we always prefer to use the role based lookups
There was a problem hiding this comment.
EUA’s dropzone component also use data-testid for testing these wrapper-only drop targets. Since this element doesn’t currently expose a meaningful role/name I would like to keep the data-testid approach.
3 participants
Adds drag-and-drop support to the modernized Content Uploader uploads manager panel.
This wraps the modernized uploads manager in a droppable panel so files dropped onto it are queued instead of triggering browser navigation. It also ensures raw file uploads through the modernized uploads manager include the current
rootFolderIdin upload options, while preserving existing behavior outside that flow.Summary by CodeRabbit
New Features
Bug Fixes