codeant-ai-for-open-source[bot] commented on code in PR #37880: URL: https://github.com/apache/superset/pull/37880#discussion_r2791865284
########## 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) Review Comment: **Suggestion:** External drops (dragging items from the datasource panel into control dropzones) are never handled because `dropHandlers` is never populated and `handleDragEnd` only consults that map; since droppable zones (e.g., `DndSelectLabel`) provide their `onDrop`/`onDropValue` callbacks via `over.data.current`, the current code silently ignores valid drops and no `onDrop` is ever called. You can fix this by first checking `over.data.current` for droppable metadata and invoking its `onDrop`/`onDropValue` when the dragged item's type is accepted, falling back to `dropHandlers` only if no inline handlers are present. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Explore controls cannot accept new columns via drag-drop. - ❌ Metrics dropped from datasource never update control state. - ⚠️ DatasourcePanel filtering uses DropzoneContext but drops still fail. ``` </details> ```suggestion const droppableData = over.data.current as | { accept?: string[]; onDrop?: (item: { type: string; value?: unknown }) => void; onDropValue?: (value: unknown) => void; } | undefined; if (activeDataCurrent && droppableData) { const { accept, onDrop, onDropValue } = droppableData; // If an accept list is provided, ensure this type is allowed if (!accept || accept.includes(activeDataCurrent.type)) { const item = { type: activeDataCurrent.type, value: activeDataCurrent.value, }; onDrop?.(item); if (activeDataCurrent.value !== undefined) { onDropValue?.(activeDataCurrent.value); } // Drop was handled by the droppable itself; no need to consult registered handlers return; } } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the Explore view, which is wrapped by `ExploreDndContextProvider` in `superset-frontend/src/explore/components/ExploreContainer/index.tsx:22-40`, so all drag-and-drop in Explore goes through `ExploreDndContextProvider` and its `DndContext` `onDragEnd` handler in `ExploreDndContext.tsx:150-195`. 2. Note that control dropzones are implemented using `DndSelectLabel` in `superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndSelectLabel.tsx:40-55`, which calls `useDroppable` with `id: \`dropzone-\${props.name}\`` and `data: { accept, onDrop, onDropValue }` at lines 80-87; this means the droppable-specific drop callbacks live in `over.data.current`, not in any global map. 3. Observe that `DropHandlersContext` from `ExploreDndContext.tsx:100-106` is never used by any consumer: a search for `DropHandlersContext` only finds its definition and provider usage within `ExploreDndContext.tsx` itself (no imports or calls elsewhere), so `register` is never called and the internal `dropHandlers` state remains an empty object for the lifetime of the app. 4. In the same provider, the `handleDragEnd` logic at `ExploreDndContext.tsx:150-195` first handles sortable reordering, and then for non-sortable external drops executes the existing code snippet (lines 186-193) that resolves `const overId = String(over.id); const handler = dropHandlers[overId];` and only calls `handler(active.id, over.id, activeDataCurrent)` if a handler exists, without consulting `over.data.current` at all. 5. Run the app and in Explore drag a column or metric from the datasource panel (rendered by `DatasourcePanel` in `superset-frontend/src/explore/components/DatasourcePanel/index.tsx:128-158`, which uses `DropzoneContext` only for validation) into a control using `DndSelectLabel` (e.g., a metrics/columns/filter control). The drag is recognized visually (because `useDroppable` and `useIsDragging` / `DraggingContext` drive the hover/border styling), and `DndContext` correctly fires `onDragEnd`, but since `dropHandlers` has no entries, the condition `if (handler && activeDataCurrent)` at `ExploreDndContext.tsx:191-193` fails. 6. Observe the functional result: the control's `onDrop`/`onDropValue` callbacks supplied via `DndSelectLabel`'s props are never invoked from `handleDragEnd`, so the control state is not updated and the dragged datasource item does not appear in the control; drag-and-drop from the datasource panel into controls is effectively a no-op despite UI feedback. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx **Line:** 187:187 **Comment:** *Logic Error: External drops (dragging items from the datasource panel into control dropzones) are never handled because `dropHandlers` is never populated and `handleDragEnd` only consults that map; since droppable zones (e.g., `DndSelectLabel`) provide their `onDrop`/`onDropValue` callbacks via `over.data.current`, the current code silently ignores valid drops and no `onDrop` is ever called. You can fix this by first checking `over.data.current` for droppable metadata and invoking its `onDrop`/`onDropValue` when the dragged item's type is accepted, falling back to `dropHandlers` only if no inline handlers are 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37880&comment_hash=7a72f341f09de25e131c859b07787056d92ac9fca4ede6ab5566bbf397cb9fd6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37880&comment_hash=7a72f341f09de25e131c859b07787056d92ac9fca4ede6ab5566bbf397cb9fd6&reaction=dislike'>👎</a> -- 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]
