codeant-ai-for-open-source[bot] commented on code in PR #36778:
URL: https://github.com/apache/superset/pull/36778#discussion_r2637556818
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx:
##########
@@ -152,27 +164,52 @@ const FilterTitleContainer = forwardRef<HTMLDivElement,
Props>(
);
};
- const renderFilterGroups = () => {
- const items: ReactNode[] = [];
- filters.forEach((item, index) => {
- items.push(
+ const handleDragEnd = (event: DragEndEvent) => {
+ const { active, over } = event;
+ if (!active || !over) return;
+ const from: number = active?.data?.current?.sortable?.index;
+ const to: number = over?.data?.current?.sortable?.index;
+ if (from === undefined || to === undefined) return;
Review 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 🚨
```suggestion
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;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx:
##########
@@ -54,92 +48,32 @@ const DragIcon = styled(Icons.Drag, {
`;
interface FilterTabTitleProps {
- index: number;
- filterIds: string[];
- onRearrange: (dragItemIndex: number, targetIndex: number) => void;
-}
-
-interface DragItem {
- index: number;
- filterIds: string[];
- type: string;
+ filterId: string;
}
export const DraggableFilter: FC<FilterTabTitleProps> = ({
- index,
- onRearrange,
- filterIds,
+ filterId,
children,
}) => {
- const ref = useRef<HTMLDivElement>(null);
- const [{ isDragging }, drag] = useDrag({
- item: { filterIds, type: FILTER_TYPE, index },
- collect: (monitor: DragSourceMonitor) => ({
- isDragging: monitor.isDragging(),
- }),
- });
- const [, drop] = useDrop({
- accept: FILTER_TYPE,
- hover: (item: DragItem, monitor: DropTargetMonitor) => {
- if (!ref.current) {
- return;
- }
-
- const dragIndex = item.index;
- const hoverIndex = index;
-
- // Don't replace items with themselves
- if (dragIndex === hoverIndex) {
- return;
- }
- // Determine rectangle on screen
- const hoverBoundingRect = ref.current?.getBoundingClientRect();
-
- // Get vertical middle
- const hoverMiddleY =
- (hoverBoundingRect.bottom - hoverBoundingRect.top) / 2;
-
- // Determine mouse position
- const clientOffset = monitor.getClientOffset();
-
- // Get pixels to the top
- const hoverClientY = (clientOffset as XYCoord).y - hoverBoundingRect.top;
-
- // Only perform the move when the mouse has crossed half of the items
height
- // When dragging downwards, only move when the cursor is below 50%
- // When dragging upwards, only move when the cursor is above 50%
- // Dragging downwards
- if (dragIndex < hoverIndex && hoverClientY < hoverMiddleY) {
- return;
- }
+ const { attributes, listeners, setNodeRef, isDragging, transform, transition
} = useSortable({ id: filterId });
- // Dragging upwards
- if (dragIndex > hoverIndex && hoverClientY > hoverMiddleY) {
- return;
- }
+ const style = {
+ transform: CSS.Transform.toString(transform),
+ transition,
+ };
- onRearrange(dragIndex, hoverIndex);
- // Note: we're mutating the monitor item here.
- // Generally it's better to avoid mutations,
- // but it's good here for the sake of performance
- // to avoid expensive index searches.
- // eslint-disable-next-line no-param-reassign
- item.index = hoverIndex;
- },
- });
- drag(drop(ref));
return (
- <Container ref={ref} isDragging={isDragging}>
- <DragIcon
- isDragging={isDragging}
- alt="Move icon"
- className="dragIcon"
- viewBox="4 4 16 16"
- />
- <div css={{ flex: 1 }}>{children}</div>
+ <Container ref={setNodeRef} style={style} isDragging={isDragging}
{...listeners} {...attributes}>
+ <DragIcon
+ isDragging={isDragging}
+ alt="Move icon"
Review 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 🚨
```suggestion
aria-label="Move filter"
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
`alt` is an attribute for <img>, 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.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx:
##########
@@ -54,92 +48,32 @@ const DragIcon = styled(Icons.Drag, {
`;
interface FilterTabTitleProps {
- index: number;
- filterIds: string[];
- onRearrange: (dragItemIndex: number, targetIndex: number) => void;
-}
-
-interface DragItem {
- index: number;
- filterIds: string[];
- type: string;
+ filterId: string;
}
export const DraggableFilter: FC<FilterTabTitleProps> = ({
- index,
- onRearrange,
- filterIds,
+ filterId,
children,
}) => {
- const ref = useRef<HTMLDivElement>(null);
- const [{ isDragging }, drag] = useDrag({
- item: { filterIds, type: FILTER_TYPE, index },
- collect: (monitor: DragSourceMonitor) => ({
- isDragging: monitor.isDragging(),
- }),
- });
- const [, drop] = useDrop({
- accept: FILTER_TYPE,
- hover: (item: DragItem, monitor: DropTargetMonitor) => {
- if (!ref.current) {
- return;
- }
-
- const dragIndex = item.index;
- const hoverIndex = index;
-
- // Don't replace items with themselves
- if (dragIndex === hoverIndex) {
- return;
- }
- // Determine rectangle on screen
- const hoverBoundingRect = ref.current?.getBoundingClientRect();
-
- // Get vertical middle
- const hoverMiddleY =
- (hoverBoundingRect.bottom - hoverBoundingRect.top) / 2;
-
- // Determine mouse position
- const clientOffset = monitor.getClientOffset();
-
- // Get pixels to the top
- const hoverClientY = (clientOffset as XYCoord).y - hoverBoundingRect.top;
-
- // Only perform the move when the mouse has crossed half of the items
height
- // When dragging downwards, only move when the cursor is below 50%
- // When dragging upwards, only move when the cursor is above 50%
- // Dragging downwards
- if (dragIndex < hoverIndex && hoverClientY < hoverMiddleY) {
- return;
- }
+ const { attributes, listeners, setNodeRef, isDragging, transform, transition
} = useSortable({ id: filterId });
- // Dragging upwards
- if (dragIndex > hoverIndex && hoverClientY > hoverMiddleY) {
- return;
- }
+ const style = {
+ transform: CSS.Transform.toString(transform),
Review 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 🚨
```suggestion
transform: transform ? CSS.Transform.toString(transform) : undefined,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx:
##########
@@ -17,25 +17,19 @@
* under the License.
*/
import { styled } from '@apache-superset/core/ui';
-import { useRef, FC } from 'react';
-import {
- DragSourceMonitor,
- DropTargetMonitor,
- useDrag,
- useDrop,
- XYCoord,
-} from 'react-dnd';
+import { FC } from 'react';
+import { CSS } from '@dnd-kit/utilities';
+import { useSortable } from '@dnd-kit/sortable';
+
import { Icons } from '@superset-ui/core/components/Icons';
import type { IconType } from '@superset-ui/core/components/Icons/types';
interface TitleContainerProps {
readonly isDragging: boolean;
}
-const FILTER_TYPE = 'FILTER';
-
const Container = styled.div<TitleContainerProps>`
Review Comment:
**Suggestion:** 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`). [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const Container = styled('div', {
shouldForwardProp: propName => propName !== 'isDragging',
})<TitleContainerProps>`
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The Container currently receives a custom prop (isDragging) which would be
forwarded to the underlying DOM <div> and cause React warnings or unexpected
attributes in the 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.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
