aminghadersohi commented on code in PR #37880:
URL: https://github.com/apache/superset/pull/37880#discussion_r3362561699
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx:
##########
@@ -45,41 +49,55 @@ export type DndSelectLabelProps = {
displayGhostButton?: boolean;
onClickGhostButton: () => void;
isLoading?: boolean;
+ // For sortable items - the type string and count to generate sortable IDs
+ sortableType?: string;
+ itemCount?: number;
};
export default function DndSelectLabel({
displayGhostButton = true,
accept,
valuesRenderer,
isLoading,
+ sortableType,
+ itemCount = 0,
...props
}: DndSelectLabelProps) {
const canDropProp = props.canDrop;
const canDropValueProp = props.canDropValue;
+ const acceptTypes = useMemo(
+ () => (Array.isArray(accept) ? accept : [accept]),
+ [accept],
+ );
+
const dropValidator = useCallback(
(item: DatasourcePanelDndItem) =>
canDropProp(item) && (canDropValueProp?.(item.value) ?? true),
[canDropProp, canDropValueProp],
);
- const [{ isOver, canDrop }, datasourcePanelDrop] = useDrop({
- accept: isLoading ? [] : accept,
-
- drop: (item: DatasourcePanelDndItem) => {
- props.onDrop(item);
- props.onDropValue?.(item.value);
+ const { setNodeRef, isOver, active } = useDroppable({
+ id: `dropzone-${props.name}`,
+ disabled: isLoading,
+ data: {
+ accept: acceptTypes,
+ onDrop: props.onDrop,
Review Comment:
**BLOCKER (counterpart)** — `onDrop` and `onDropValue` are stored in the
droppable's data here, which is the right instinct. However, `handleDragEnd` in
`ExploreDndContext.tsx` uses `dropHandlers[overId]` (from
`DropHandlersContext`) rather than `over.data.current.onDrop`, so these
handlers are never called. See the comment on `ExploreDndContext.tsx:188` for
the full trace and fix options.
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx:
##########
@@ -105,8 +132,31 @@ export default function DndSelectLabel({
);
}
+ // Handle drop events from dnd-kit
+ useEffect(() => {
Review Comment:
**MEDIUM — empty effect body, should be removed**
```ts
useEffect(() => {
if (isOver && active?.data.current && canDrop) {
// The actual drop is handled in ExploreDndContext's onDragEnd
// This effect is for any side effects needed during hover
}
}, [isOver, active, canDrop]);
```
This runs on every `isOver`/`active`/`canDrop` change and does nothing — the
`if` body is empty comments. It looks like a placeholder left over from an
incomplete implementation (consistent with the B1 gap). Please remove it; it
also incidentally avoids the exhaustive-deps lint warning on `isOver`.
##########
superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:
##########
@@ -0,0 +1,255 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ createContext,
+ useContext,
+ useState,
+ useCallback,
+ useMemo,
+ FC,
+ Dispatch,
+ useReducer,
+} from 'react';
+import {
+ DndContext,
+ useSensor,
+ useSensors,
+ PointerSensor,
+ DragStartEvent,
+ DragEndEvent,
+ UniqueIdentifier,
+} from '@dnd-kit/core';
+import { DatasourcePanelDndItem } from '../DatasourcePanel/types';
+
+/**
+ * Type for the active drag item data
+ */
+export interface ActiveDragData {
+ type: string;
+ value?: unknown;
+ dragIndex?: number;
+ // For sortable items - callback to handle reorder
+ onShiftOptions?: (dragIndex: number, hoverIndex: number) => void;
+ onMoveLabel?: (dragIndex: number, hoverIndex: number) => void;
+ onDropLabel?: () => void;
+}
+
+/**
+ * Context to track if something is being dragged (for visual feedback)
+ */
+export const DraggingContext = createContext(false);
+
+/**
+ * Context to access the currently active drag item
+ */
+export const ActiveDragContext = createContext<ActiveDragData | null>(null);
+
+/**
+ * Dropzone validation - used by controls to register what they can accept
+ */
+type CanDropValidator = (item: DatasourcePanelDndItem) => boolean;
+type DropzoneSet = Record<string, CanDropValidator>;
+type Action = { key: string; canDrop?: CanDropValidator };
+
+export const DropzoneContext = createContext<[DropzoneSet, Dispatch<Action>]>([
+ {},
+ () => {},
+]);
+
+const dropzoneReducer = (state: DropzoneSet = {}, action: Action) => {
+ if (action.canDrop) {
+ return {
+ ...state,
+ [action.key]: action.canDrop,
+ };
+ }
+ if (action.key) {
+ const newState = { ...state };
+ delete newState[action.key];
+ return newState;
+ }
+ return state;
+};
+
+/**
+ * Context for handling drag end events - controls register their onDrop
handlers
+ */
+type DropHandler = (
+ activeId: UniqueIdentifier,
+ overId: UniqueIdentifier,
+ activeData: ActiveDragData,
+) => void;
+type DropHandlerSet = Record<string, DropHandler>;
+
+export const DropHandlersContext = createContext<{
+ register: (id: string, handler: DropHandler) => void;
+ unregister: (id: string) => void;
+}>({
+ register: () => {},
+ unregister: () => {},
+});
+
+interface ExploreDndContextProps {
+ children: React.ReactNode;
+}
+
+/**
+ * DnD context provider for the Explore view.
+ * Wraps @dnd-kit/core's DndContext and provides:
+ * - Dragging state tracking (for visual feedback)
+ * - Dropzone registration (for validation)
+ * - Drop handler registration (for handling drops)
+ */
+export const ExploreDndContextProvider: FC<ExploreDndContextProps> = ({
+ children,
+}) => {
+ const [isDragging, setIsDragging] = useState(false);
+ const [activeData, setActiveData] = useState<ActiveDragData | null>(null);
+ const [dropHandlers, setDropHandlers] = useState<DropHandlerSet>({});
+
+ const dropzoneValue = useReducer(dropzoneReducer, {});
+
+ // Configure sensors for drag detection
+ const sensors = useSensors(
+ useSensor(PointerSensor, {
+ activationConstraint: {
+ distance: 5, // 5px movement required before drag starts
+ },
+ }),
+ );
+
+ const handleDragStart = useCallback((event: DragStartEvent) => {
+ const { active } = event;
+ const data = active.data.current as ActiveDragData | undefined;
+
+ // Don't set dragging state for reordering within a list
+ if (data && 'dragIndex' in data) {
+ return;
+ }
+
+ setIsDragging(true);
+ setActiveData(data || null);
+ }, []);
+
+ const handleDragEnd = useCallback(
+ (event: DragEndEvent) => {
+ const { active, over } = event;
+
+ setIsDragging(false);
+ setActiveData(null);
+
+ if (over && active.id !== over.id) {
+ const activeDataCurrent = active.data.current as
+ | ActiveDragData
+ | undefined;
+ const overDataCurrent = over.data.current as ActiveDragData |
undefined;
+
+ // Check if this is a sortable reorder operation
+ // Both items need dragIndex and the same type
+ if (
+ activeDataCurrent &&
+ overDataCurrent &&
+ typeof activeDataCurrent.dragIndex === 'number' &&
+ typeof overDataCurrent.dragIndex === 'number' &&
+ activeDataCurrent.type === overDataCurrent.type
+ ) {
+ const { dragIndex } = activeDataCurrent;
+ const hoverIndex = overDataCurrent.dragIndex;
+
+ // Call the appropriate reorder callback
+ const reorderCallback =
+ activeDataCurrent.onShiftOptions || activeDataCurrent.onMoveLabel;
+ if (reorderCallback) {
+ reorderCallback(dragIndex, hoverIndex);
+ }
+
+ // Call onDropLabel if provided (for finalization after reorder)
+ activeDataCurrent.onDropLabel?.();
+ return;
+ }
+
+ // Handle external drop (from DatasourcePanel to dropzone)
+ const overId = String(over.id);
Review Comment:
**BLOCKER — DatasourcePanel → control drops silently no-op**
The code path for an external drop is:
```ts
const overId = String(over.id); // "dropzone-<name>"
const handler = dropHandlers[overId]; // always undefined — nothing ever
calls register()
if (handler && activeDataCurrent) { ... }
```
`dropHandlers` is only populated via `DropHandlersContext.register(id,
handler)`, but no component in this PR calls `register`. `DndSelectLabel`
stores `onDrop`/`onDropValue` in its `useDroppable` data object at lines 85–86,
but `handleDragEnd` never reads `over.data.current.onDrop`.
Result: dragging a column or metric from the Datasource Panel and dropping
it on any chart control (Metrics, Filters, Columns…) silently does nothing — no
state change, no popover.
**Why CI doesn't catch it:** the deleted tests used
`fireEvent.dragStart/dragOver/drop` (HTML5 drag events). `PointerSensor`
listens to `pointerdown/pointermove/pointerup`, so those events never triggered
the new code. The tests were correct to delete (wrong event model), but no
pointer-event replacements were added.
**Concrete fix — two equivalent options:**
Option A — read `over.data.current` directly in `handleDragEnd` (minimal
change):
```ts
// Handle external drop (from DatasourcePanel to dropzone)
const overId = String(over.id);
const registeredHandler = dropHandlers[overId];
if (registeredHandler && activeDataCurrent) {
registeredHandler(active.id, over.id, activeDataCurrent);
return;
}
// Fall back to onDrop stored in droppable data
const overDropData = over.data.current as {
accept?: string[];
onDrop?: (item: { type: string; value: unknown }) => void;
onDropValue?: (value: unknown) => void;
} | undefined;
if (overDropData?.onDrop && activeDataCurrent) {
const accepted = !overDropData.accept?.length ||
overDropData.accept.includes(activeDataCurrent.type);
if (accepted) {
overDropData.onDrop({ type: activeDataCurrent.type, value:
activeDataCurrent.value });
overDropData.onDropValue?.(activeDataCurrent.value);
}
}
```
Option B — have `DndSelectLabel` register via `DropHandlersContext` in a
`useEffect` (cleaner for the intended architecture):
```ts
const { register, unregister } = useContext(DropHandlersContext);
useEffect(() => {
register(`dropzone-${props.name}`, (_activeId, _overId, activeData) => {
const accepted = acceptTypes.includes(activeData.type as DndItemType);
if (accepted && dropValidator({ type: activeData.type as DndItemType,
value: activeData.value as DndItemValue })) {
props.onDrop({ type: activeData.type as DndItemType, value:
activeData.value as DndItemValue });
props.onDropValue?.(activeData.value);
}
});
return () => unregister(`dropzone-${props.name}`);
}, [register, unregister, props.name, acceptTypes, dropValidator,
props.onDrop, props.onDropValue]);
```
Can you confirm with a manual drag repro (drag a column from the datasource
panel onto e.g. the X Axis control)? Happy to be wrong if there's a path I
missed.
##########
superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:
##########
@@ -0,0 +1,255 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ createContext,
+ useContext,
+ useState,
+ useCallback,
+ useMemo,
+ FC,
+ Dispatch,
+ useReducer,
+} from 'react';
+import {
+ DndContext,
+ useSensor,
+ useSensors,
+ PointerSensor,
+ DragStartEvent,
+ DragEndEvent,
+ UniqueIdentifier,
+} from '@dnd-kit/core';
+import { DatasourcePanelDndItem } from '../DatasourcePanel/types';
+
+/**
+ * Type for the active drag item data
+ */
+export interface ActiveDragData {
+ type: string;
+ value?: unknown;
+ dragIndex?: number;
+ // For sortable items - callback to handle reorder
+ onShiftOptions?: (dragIndex: number, hoverIndex: number) => void;
+ onMoveLabel?: (dragIndex: number, hoverIndex: number) => void;
+ onDropLabel?: () => void;
+}
+
+/**
+ * Context to track if something is being dragged (for visual feedback)
+ */
+export const DraggingContext = createContext(false);
+
+/**
+ * Context to access the currently active drag item
+ */
+export const ActiveDragContext = createContext<ActiveDragData | null>(null);
+
+/**
+ * Dropzone validation - used by controls to register what they can accept
+ */
+type CanDropValidator = (item: DatasourcePanelDndItem) => boolean;
+type DropzoneSet = Record<string, CanDropValidator>;
+type Action = { key: string; canDrop?: CanDropValidator };
+
+export const DropzoneContext = createContext<[DropzoneSet, Dispatch<Action>]>([
+ {},
+ () => {},
+]);
+
+const dropzoneReducer = (state: DropzoneSet = {}, action: Action) => {
+ if (action.canDrop) {
+ return {
+ ...state,
+ [action.key]: action.canDrop,
+ };
+ }
+ if (action.key) {
+ const newState = { ...state };
+ delete newState[action.key];
+ return newState;
+ }
+ return state;
+};
+
+/**
+ * Context for handling drag end events - controls register their onDrop
handlers
+ */
+type DropHandler = (
+ activeId: UniqueIdentifier,
+ overId: UniqueIdentifier,
+ activeData: ActiveDragData,
+) => void;
+type DropHandlerSet = Record<string, DropHandler>;
+
+export const DropHandlersContext = createContext<{
+ register: (id: string, handler: DropHandler) => void;
+ unregister: (id: string) => void;
+}>({
+ register: () => {},
+ unregister: () => {},
+});
+
+interface ExploreDndContextProps {
+ children: React.ReactNode;
+}
+
+/**
+ * DnD context provider for the Explore view.
+ * Wraps @dnd-kit/core's DndContext and provides:
+ * - Dragging state tracking (for visual feedback)
+ * - Dropzone registration (for validation)
+ * - Drop handler registration (for handling drops)
+ */
+export const ExploreDndContextProvider: FC<ExploreDndContextProps> = ({
+ children,
+}) => {
+ const [isDragging, setIsDragging] = useState(false);
+ const [activeData, setActiveData] = useState<ActiveDragData | null>(null);
+ const [dropHandlers, setDropHandlers] = useState<DropHandlerSet>({});
+
+ const dropzoneValue = useReducer(dropzoneReducer, {});
+
+ // Configure sensors for drag detection
+ const sensors = useSensors(
Review Comment:
**NIT — `KeyboardSensor` not wired up**
Not a regression — `react-dnd-html5-backend` had no keyboard DnD support
either. But @dnd-kit ships `KeyboardSensor` as a first-class primitive (the
main a11y pitch of the library). This migration is the natural place to add it:
```ts
import { PointerSensor, KeyboardSensor, useSensor, useSensors } from
'@dnd-kit/core';
import { sortableKeyboardCoordinates } from '@dnd-kit/sortable';
const sensors = useSensors(
useSensor(PointerSensor, { activationConstraint: { distance: 5 } }),
useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates
}),
);
```
Worth a follow-up issue if not in scope for this PR.
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx:
##########
@@ -311,157 +303,8 @@ test('update adhoc metric name when column label in
dataset changes', () => {
expect(screen.getByText('SUM(new col B name)')).toBeVisible();
});
-test('can drag metrics', async () => {
- const metricValues = ['metric_a', 'metric_b', adhocMetricB];
- render(<DndMetricSelect {...defaultProps} value={metricValues} multi />, {
- useDnd: true,
- useRedux: true,
- });
-
- expect(screen.getByText('metric_a')).toBeVisible();
- expect(screen.getByText('Metric B')).toBeVisible();
-
- const container = screen.getByTestId('dnd-labels-container');
- expect(container.childElementCount).toBe(4);
-
- const firstMetric = container.children[0] as HTMLElement;
- const lastMetric = container.children[2] as HTMLElement;
- expect(within(firstMetric).getByText('metric_a')).toBeVisible();
- expect(within(lastMetric).getByText('SUM(Column B)')).toBeVisible();
-
- fireEvent.mouseOver(within(firstMetric).getByText('metric_a'));
- expect(await screen.findByText('Metric name')).toBeInTheDocument();
-
- fireEvent.dragStart(firstMetric);
- fireEvent.dragEnter(lastMetric);
- fireEvent.dragOver(lastMetric);
- fireEvent.drop(lastMetric);
-
- expect(within(firstMetric).getByText('SUM(Column B)')).toBeVisible();
- expect(within(lastMetric).getByText('metric_a')).toBeVisible();
-});
-
-test('cannot drop a duplicated item', () => {
- const metricValues = ['metric_a'];
- const { getByTestId } = render(
- <>
- <DatasourcePanelDragOption
- value={{ metric_name: 'metric_a', uuid: '1' }}
- type={DndItemType.Metric}
- />
- <DndMetricSelect {...defaultProps} value={metricValues} multi />
- </>,
- {
- useDnd: true,
- useRedux: true,
- },
- );
-
- const acceptableMetric = getByTestId('DatasourcePanelDragOption');
- const currentMetric = getByTestId('dnd-labels-container');
-
- const currentMetricSelection = currentMetric.children.length;
-
- fireEvent.dragStart(acceptableMetric);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection);
- expect(currentMetric).toHaveTextContent('metric_a');
-});
-
-test('can drop a saved metric when disallow_adhoc_metrics', () => {
- const metricValues = ['metric_b'];
- const { getByTestId } = render(
- <>
- <DatasourcePanelDragOption
- value={{ metric_name: 'metric_a', uuid: '1' }}
- type={DndItemType.Metric}
- />
- <DndMetricSelect
- {...defaultProps}
- value={metricValues}
- multi
- datasource={{ extra: '{ "disallow_adhoc_metrics": true }' }}
- />
- </>,
- {
- useDnd: true,
- useRedux: true,
- },
- );
-
- const acceptableMetric = getByTestId('DatasourcePanelDragOption');
- const currentMetric = getByTestId('dnd-labels-container');
-
- const currentMetricSelection = currentMetric.children.length;
-
- fireEvent.dragStart(acceptableMetric);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection + 1);
- expect(currentMetric.children[1]).toHaveTextContent('metric_a');
-});
-
-test('cannot drop non-saved metrics when disallow_adhoc_metrics', () => {
- const metricValues = ['metric_b'];
- const { getByTestId, getAllByTestId } = render(
- <>
- <DatasourcePanelDragOption
- value={{ metric_name: 'metric_a', uuid: '1' }}
- type={DndItemType.Metric}
- />
- <DatasourcePanelDragOption
- value={{ metric_name: 'metric_c', uuid: '2' }}
- type={DndItemType.Metric}
- />
- <DatasourcePanelDragOption
- value={{ column_name: 'column_1', uuid: '3' }}
- type={DndItemType.Column}
- />
- <DndMetricSelect
- {...defaultProps}
- value={metricValues}
- multi
- datasource={{ extra: '{ "disallow_adhoc_metrics": true }' }}
- />
- </>,
- {
- useDnd: true,
- useRedux: true,
- },
- );
-
- const selections = getAllByTestId('DatasourcePanelDragOption');
- const acceptableMetric = selections[0];
- const unacceptableMetric = selections[1];
- const unacceptableType = selections[2];
- const currentMetric = getByTestId('dnd-labels-container');
-
- const currentMetricSelection = currentMetric.children.length;
-
- fireEvent.dragStart(unacceptableMetric);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection);
- expect(currentMetric).not.toHaveTextContent('metric_c');
-
- fireEvent.dragStart(unacceptableType);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection);
- expect(currentMetric).not.toHaveTextContent('column_1');
-
- fireEvent.dragStart(acceptableMetric);
- fireEvent.dragOver(currentMetric);
- fireEvent.drop(currentMetric);
-
- expect(currentMetric.children).toHaveLength(currentMetricSelection + 1);
- expect(currentMetric).toHaveTextContent('metric_a');
-});
+// Note: Drag-and-drop tests removed - @dnd-kit uses pointer events instead of
Review Comment:
**HIGH (Rule 26) — 15 behavioral drop/reorder tests deleted without
pointer-event replacements**
The deletion rationale is correct: `fireEvent.dragStart/dragOver/drop` fire
HTML5 events that `PointerSensor` never receives, so those tests were already
not testing real behavior. But the right fix is pointer-event replacements, not
just deletion.
Tests removed without replacement across affected files:
| File | Tests removed |
|---|---|
| `DndColumnMetricSelect.test.tsx:91` | "can drop columns and metrics",
"cannot drop duplicate items", "can drop only selected metrics", "can drag and
reorder items" |
| `DndFilterSelect.test.tsx:177` | "cannot drop a column not in simple
selection", "can drop column type (disallow_adhoc)", "cannot drop other types
(disallow_adhoc)" |
| `DndMetricSelect.test.tsx:306` | "can drag metrics", "cannot drop
duplicate item", "can drop saved metric (disallow_adhoc)", "cannot drop
non-saved metrics (disallow_adhoc)" |
| `OptionControls.test.tsx` | "triggers onMoveLabel on drop" |
| `OptionWrapper.test.tsx` | "triggers onShiftOptions on drop" |
The survivors (renders, label visibility) would pass even if all DnD code
were removed. `disallow_adhoc_metrics` type-gating and duplicate-rejection
logic are now completely untested at the unit level.
`@dnd-kit/testing` provides `MockedMonitor`/pointer-event helpers, and
Playwright's `page.mouse` API is another option for E2E coverage. Either would
be a natural follow-up, but ideally at least the drop-acceptance smoke tests
come back before merge.
--
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]
