EPMRPP-104644 add outside drop for sortable component#248
Conversation
…outside-drop-for-Sortable-component
evshipilo
requested review from
allaprischepa,
maria-hambardzumian and
siarheirazuvalau
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdds external drag-and-drop support to the tree sortable system: new external drop constant, types, hook options and helper, component props and external handlers, plus README documentation; behavior routes external drops to dedicated handlers and conditions hover/drop logic on acceptExternalDrop. Changes
Sequence DiagramsequenceDiagram
participant External as External Drag Source
participant Item as TreeSortableItem
participant Hook as useTreeSortable
participant Container as TreeSortableContainer
participant Handler as onMoveExternal/onDuplicateExternal
External->>Item: Drag external item over
Item->>Hook: Provide dragged item (isExternal=true)
Hook->>Hook: Check acceptExternalDrop & externalDropType
alt acceptExternalDrop == true
Hook->>Hook: Calculate allowed drop positions (inside-only for external)
Hook->>Item: Update hover state / dropPosition
External->>Item: Drop
Item->>Hook: Trigger drop with external item
Hook->>Container: Emit drop event with item.isExternal
Container->>Container: Detect external flag
Container->>Handler: Route to onMoveExternal/onDuplicateExternal
Handler->>Container: Handle external move/duplicate
else acceptExternalDrop == false
Hook->>Item: Reject drop, clear hover
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai full review |
|
✅ Actions performedFull review triggered. |
![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/components/sortable/README.md (1)
129-153:⚠️ Potential issue | 🟡 Minor
useTreeSortablehook docs are missing the newacceptExternalDropandexternalDropTypeoptions.The "Options" section for
useTreeSortable(lines 133–143) doesn't list these two new parameters, even though the hook accepts them and they're documented onTreeSortableItem. Consumers using the hook directly would miss these.📝 Suggested addition after line 143
- **hideDefaultPreview**: _boolean_, optional - Hide default preview +- **acceptExternalDrop**: _boolean_, optional, default = false - Whether this item accepts external drops +- **externalDropType**: _string_, optional - DnD type for external items (default: `EXTERNAL_TREE_DROP_TYPE`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sortable/README.md` around lines 129 - 153, The README's useTreeSortable options are missing the new acceptExternalDrop and externalDropType parameters; update the "Options" list in the useTreeSortable docs to include entries for acceptExternalDrop: _boolean_, optional (whether the hook accepts drops from external sources) and externalDropType: _string | string[]_, optional (the DnD type(s) accepted from external drags), mirroring how these are described on TreeSortableItem so consumers using the hook directly see the new options.
🧹 Nitpick comments (2)
src/components/sortable/treeSortableContainer/treeSortableContainer.tsx (1)
105-146: Confirmation popover shows "Duplicate" for external items even ifonDuplicateExternalis not provided.When
showDropConfirmationis true and an external item is dropped, both "Move" and "Duplicate" buttons are rendered. If onlyonMoveExternalis supplied, clicking "Duplicate" silently no-ops via optional chaining. This is the same pattern as the internal callbacks, but for external items the "Duplicate" action may be less common and could confuse users.Consider conditionally hiding the "Duplicate" button when the corresponding handler is absent — but this applies equally to the internal case and is arguably outside the scope of this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sortable/treeSortableContainer/treeSortableContainer.tsx` around lines 105 - 146, The confirmation popover currently renders a "Duplicate" button even when the corresponding handler is not provided, causing a silent no-op; update the popover rendering logic so the "Duplicate" button is only shown when a duplicate handler exists for the current drop type (i.e., for external drops show the button only if onDuplicateExternal is defined, and for internal drops show it only if onDuplicate is defined), and ensure this matches the branch logic used in handleAction (DROP_ACTIONS.DUPLICATE) so clicking the button always invokes a valid handler.src/common/hooks/useTreeSortable.ts (1)
61-62: Consider whether the OR condition inisExternalDragItemcould lead to false positives.The helper treats an item as external if either
isExternal === trueoritem.type === externalDropType. If a consumer happens to set the internal tree'stypeprop to the same string asexternalDropType, internal items would be misclassified as external and routed toonMoveExternal/onDuplicateExternal.Since both signals are under developer control and the README clearly instructs setting both
type: EXTERNAL_TREE_DROP_TYPEandisExternal: true, this is a low-risk edge case, but the dual-check makes the contract slightly ambiguous. Consider documenting (or asserting at dev time) that the tree's owntypemust differ fromexternalDropType.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/hooks/useTreeSortable.ts` around lines 61 - 62, The current isExternalDragItem helper can misclassify internal items because it returns true when either item.isExternal is true OR item.type === externalDropType; change isExternalDragItem to require both signals (i.e., treat an item as external only if item.isExternal === true AND item.type === externalDropType) to avoid false positives, and add a dev-time warning/assert inside isExternalDragItem when item.type === externalDropType but item.isExternal !== true to help developers detect a conflicting tree type configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/sortable/README.md`:
- Around line 129-153: The README's useTreeSortable options are missing the new
acceptExternalDrop and externalDropType parameters; update the "Options" list in
the useTreeSortable docs to include entries for acceptExternalDrop: _boolean_,
optional (whether the hook accepts drops from external sources) and
externalDropType: _string | string[]_, optional (the DnD type(s) accepted from
external drags), mirroring how these are described on TreeSortableItem so
consumers using the hook directly see the new options.
---
Nitpick comments:
In `@src/common/hooks/useTreeSortable.ts`:
- Around line 61-62: The current isExternalDragItem helper can misclassify
internal items because it returns true when either item.isExternal is true OR
item.type === externalDropType; change isExternalDragItem to require both
signals (i.e., treat an item as external only if item.isExternal === true AND
item.type === externalDropType) to avoid false positives, and add a dev-time
warning/assert inside isExternalDragItem when item.type === externalDropType but
item.isExternal !== true to help developers detect a conflicting tree type
configuration.
In `@src/components/sortable/treeSortableContainer/treeSortableContainer.tsx`:
- Around line 105-146: The confirmation popover currently renders a "Duplicate"
button even when the corresponding handler is not provided, causing a silent
no-op; update the popover rendering logic so the "Duplicate" button is only
shown when a duplicate handler exists for the current drop type (i.e., for
external drops show the button only if onDuplicateExternal is defined, and for
internal drops show it only if onDuplicate is defined), and ensure this matches
the branch logic used in handleAction (DROP_ACTIONS.DUPLICATE) so clicking the
button always invokes a valid handler.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/common/constants/sortable.tssrc/common/hooks/useTreeSortable.tssrc/common/types/sortableTypes.tssrc/components/sortable/README.mdsrc/components/sortable/treeSortableContainer/treeSortableContainer.tsxsrc/components/sortable/treeSortableItem/treeSortableItem.tsx
evshipilo
changed the title
src/components/sortable/treeSortableContainer/treeSortableContainer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/hooks/useTreeSortable.ts`:
- Around line 61-63: isExternalDragItem currently infers external drags by
checking item.type but the raw item passed into onDrop doesn't get its
isExternal flag set, causing downstream routing that checks
draggedItem.isExternal to misclassify drops; update the code paths that call
onDrop to normalize the item first by setting item.isExternal = true when
isExternalDragItem(item, externalDropType) is true (use the same logic in the
other occurrence around the second block that performs drops), and then call
onDrop with this normalized TreeDragItem so downstream handlers see a consistent
isExternal property.
In `@src/components/sortable/README.md`:
- Around line 60-61: Update the README prop docs to reference the actual
exported constant name used by consumers: change the mention of
EXTERNAL_TREE_DROP to EXTERNAL_TREE_DROP_TYPE in the description for
externalDropType (and keep acceptExternalDrop description unchanged), and note
that this constant is exported from `@common/constants/sortable` so consumers can
import EXTERNAL_TREE_DROP_TYPE when using externalDropType.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/common/constants/sortable.tssrc/common/hooks/useTreeSortable.tssrc/common/types/sortableTypes.tssrc/components/sortable/README.mdsrc/components/sortable/treeSortableContainer/treeSortableContainer.tsxsrc/components/sortable/treeSortableItem/treeSortableItem.tsx
evshipilo
deleted the
EPMRPP-106692-add-outside-drop-for-Sortable-component
branch
…rop-for-Sortable-component EPMRPP-104644 add outside drop for sortable component fb83f63
2 participants
Screen.Recording.2026-02-25.at.14.18.20.mov
Summary by CodeRabbit
New Features
Documentation