refactor(draggable filters): react-dnd to dnd-kit migration for DraggableFilters #36778
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #279352Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
codeant-ai-for-open-source
bot
added
the
size:M
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
| const FILTER_TYPE = 'FILTER'; | ||
|
|
||
| const Container = styled.div<TitleContainerProps>` |
There was a problem hiding this comment.
Suggestion: The isDragging prop is being forwarded to a native DOM
Container) which will render an unknown attribute on the DOM and cause React warnings or unexpected behavior; stop forwarding custom props to DOM by using shouldForwardProp or a transient prop name (like $isDragging). [possible bug]
Severity Level: Critical 🚨
| const Container = styled.div<TitleContainerProps>` | |
| const Container = styled('div', { | |
| shouldForwardProp: propName => propName !== 'isDragging', | |
| })<TitleContainerProps>` |
Why it matters? ⭐
The Container currently receives a custom prop (isDragging) which would be forwarded to the underlying DOM
The proposed change to opt out forwarding (shouldForwardProp) or use a transient prop like $isDragging is correct and fixes a real issue. The improved code sample demonstrates the right approach by preventing the prop from reaching the DOM.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx
**Line:** 31:31
**Comment:**
*Possible Bug: The `isDragging` prop is being forwarded to a native DOM <div> (the styled `Container`) which will render an unknown attribute on the DOM and cause React warnings or unexpected behavior; stop forwarding custom props to DOM by using `shouldForwardProp` or a transient prop name (like `$isDragging`).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| return; | ||
| } | ||
| const style = { | ||
| transform: CSS.Transform.toString(transform), |
There was a problem hiding this comment.
Suggestion: transform returned by useSortable can be null/undefined; calling CSS.Transform.toString(transform) without guarding may produce an unexpected value — add a null-check so style.transform is undefined when there's no transform. [possible bug]
Severity Level: Critical 🚨
| transform: CSS.Transform.toString(transform), | |
| transform: transform ? CSS.Transform.toString(transform) : undefined, |
Why it matters? ⭐
useSortable can return a null/undefined transform. Guarding the call avoids passing an invalid string (or "null") to the style.transform and ensures the style either contains a valid transform string or is undefined (so React omits it). This is a safe defensive change that prevents odd inline-style values.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx
**Line:** 62:62
**Comment:**
*Possible Bug: `transform` returned by `useSortable` can be null/undefined; calling `CSS.Transform.toString(transform)` without guarding may produce an unexpected value — add a null-check so `style.transform` is `undefined` when there's no transform.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| <Container ref={setNodeRef} style={style} isDragging={isDragging} {...listeners} {...attributes}> | ||
| <DragIcon | ||
| isDragging={isDragging} | ||
| alt="Move icon" |
There was a problem hiding this comment.
Suggestion: alt is not a valid attribute for SVG-based icon components and should be replaced with an accessible property such as aria-label (or a <title> inside the SVG) to provide screen-reader text; using alt may be ignored and hurts accessibility. [possible bug]
Severity Level: Critical 🚨
| alt="Move icon" | |
| aria-label="Move filter" |
Why it matters? ⭐
alt is an attribute for , not for inline SVGs. Replacing it with
aria-label (or including a <title> inside the SVG) is the correct accessibility fix. The suggested change improves screen-reader semantics and is appropriate for this icon component.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx
**Line:** 70:70
**Comment:**
*Possible Bug: `alt` is not a valid attribute for SVG-based icon components and should be replaced with an accessible property such as `aria-label` (or a `<title>` inside the SVG) to provide screen-reader text; using `alt` may be ignored and hurts accessibility.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const from: number = active?.data?.current?.sortable?.index; | ||
| const to: number = over?.data?.current?.sortable?.index; | ||
| if (from === undefined || to === undefined) return; |
There was a problem hiding this comment.
Suggestion: The drag end handler relies on active.data.current.sortable.index and over.data.current.sortable.index, which are only present if sortable children populate that data structure; those fields can be undefined and make rearrange a no-op. Compute source/target indices from active.id and over.id against the current filters array instead, which is more reliable and does not require sortable-specific data to be present. [possible bug]
Severity Level: Critical 🚨
| const from: number = active?.data?.current?.sortable?.index; | |
| const to: number = over?.data?.current?.sortable?.index; | |
| if (from === undefined || to === undefined) return; | |
| const activeId = active.id; | |
| const overId = over.id; | |
| if (activeId == null || overId == null) return; | |
| const from = filters.indexOf(String(activeId)); | |
| const to = filters.indexOf(String(overId)); | |
| if (from === -1 || to === -1) return; |
Why it matters? ⭐
This suggestion addresses a real reliability issue: the data.current.sortable.index payload isn't guaranteed unless the child sortable implementation populates it. Using active.id / over.id and resolving indices from the filters array is more robust and resilient to different sortable implementations. The change is straightforward and fixes a class of silent no-ops when data is missing.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx
**Line:** 170:172
**Comment:**
*Possible Bug: The drag end handler relies on `active.data.current.sortable.index` and `over.data.current.sortable.index`, which are only present if sortable children populate that `data` structure; those fields can be undefined and make rearrange a no-op. Compute source/target indices from `active.id` and `over.id` against the current `filters` array instead, which is more reliable and does not require sortable-specific data to be present.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
Code Review Agent Run #c78b2aActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #fcece6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rubaizmomin I think this is in need of a rebase and cleanup to pass ci |
2 participants
User description
SUMMARY
Since react-dnd is no longer maintained, dnd-kit is being used as a replacement, as it is modern with hooks and is maintained regularly. With dnd-kit, I had two options: either make the dragIcon draggable or make the container draggable. I went with making the container draggable, as this was the previous behaviour.
Making the container draggable came with some challenges, as the container contains a delete button and an undo button (after deletion). These buttons have their listeners, like onClick, which caused an issue as we wrap the whole container with dnd-kit listeners. So, if we click on the delete button, it won't trigger, as all the events are captured by the dnd-kit listener and are never passed down to the delete and undo buttons. So for this, we used sensors to make the draggable action active based on the distance of drag, i.e., move the container by 5 px, to make the draggable action active, which would make the dnd listener catch that event otherwise, the events are passed to delete and undo and work as expected.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CodeAnt-AI Description
Replace react-dnd with dnd-kit for native filter drag-and-drop
What Changed
Impact
✅ Fewer accidental drag interceptions when clicking filter actions✅ Smoother filter reordering animations✅ Clickable delete and undo buttons during non-drag interactions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.