codeant-ai-for-open-source[bot] commented on code in PR #36239:
URL: https://github.com/apache/superset/pull/36239#discussion_r2619552229
##########
superset-frontend/packages/superset-ui-core/src/components/Icons/index.tsx:
##########
@@ -42,6 +42,7 @@ const customIcons = [
'Error',
'Full',
'Layers',
+ 'Move',
Review Comment:
**Suggestion:** The newly added `Move` custom icon will cause a runtime
failure when used because `AsyncIcon` will try to dynamically import
`src/assets/images/icons/move.svg`, but there is no corresponding SVG file in
the repository, leading to a rejected dynamic import and missing icon rendering
(and likely an error in the console); either add the missing `move.svg` asset
or remove this icon registration until the asset exists. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion identifies a real runtime risk: AsyncIcon dynamically builds
a file path from the icon name (filename 'move') and will attempt to import the
corresponding asset. I searched the repository for both "icons/move" and
"move.svg" and found no matches, so the asset doesn't exist in the codebase.
This will result in failed dynamic imports and missing icons (and likely
console errors) when this icon is used. Adding the missing SVG asset or
removing the registration are both valid fixes.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Icons/index.tsx
**Line:** 45:45
**Comment:**
*Logic Error: The newly added `Move` custom icon will cause a runtime
failure when used because `AsyncIcon` will try to dynamically import
`src/assets/images/icons/move.svg`, but there is no corresponding SVG file in
the repository, leading to a rejected dynamic import and missing icon rendering
(and likely an error in the console); either add the missing `move.svg` asset
or remove this icon registration until the asset exists.
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/components/Datasource/FoldersEditor/folderOperations.test.ts:
##########
@@ -0,0 +1,751 @@
+/**
+ * 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 { Metric } from '@superset-ui/chart-controls';
+import { ColumnObject } from 'src/features/datasets/types';
+import {
+ DEFAULT_METRICS_FOLDER_UUID,
+ DEFAULT_COLUMNS_FOLDER_UUID,
+ isDefaultFolder,
+} from './constants';
+import {
+ createFolder,
+ deleteFolder,
+ resetToDefault,
+ filterItemsBySearch,
+ renameFolder,
+ nestFolder,
+ reorderFolders,
+ moveItems,
+ cleanupFolders,
+ getAllSelectedItems,
+ areAllVisibleItemsSelected,
+ ensureDefaultFolders,
+} from './folderOperations';
+import {
+ canDropItems,
+ canDropFolder,
+ getFolderDescendants,
+ findFolderById,
+ validateFolders,
+} from './folderValidation';
+import { DatasourceFolder } from
'src/explore/components/DatasourcePanel/types';
+import { FoldersEditorItemType } from '../types';
+
+describe('folderUtils', () => {
+ const mockMetrics: Metric[] = [
+ {
+ id: 1,
+ uuid: 'metric-1',
+ metric_name: 'Test Metric 1',
+ metric_type: 'count',
+ expression: 'COUNT(*)',
+ } as Metric,
+ {
+ id: 2,
+ uuid: 'metric-2',
+ metric_name: 'Test Metric 2',
+ metric_type: 'sum',
+ expression: 'SUM(value)',
+ } as Metric,
Review Comment:
**Suggestion:** The test uses a type assertion (`as Metric`) to coerce plain
objects into `Metric`, which can mask missing runtime properties (like `uuid`)
and allow malformed mock data to slip through compilation. Make the mock type
explicitly include `uuid` and remove `as` casts so the compiler enforces the
object shape and prevents accidental omissions that would break functions
expecting `uuid` at runtime. [type error]
**Severity Level:** Minor ⚠️
```suggestion
const mockMetrics: (Metric & { uuid: string })[] = [
{
id: 1,
uuid: 'metric-1',
metric_name: 'Test Metric 1',
metric_type: 'count',
expression: 'COUNT(*)',
},
{
id: 2,
uuid: 'metric-2',
metric_name: 'Test Metric 2',
metric_type: 'sum',
expression: 'SUM(value)',
},
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The test file currently uses `as Metric` casts for the mock metric objects.
Replacing the casts with an explicit type that ensures `uuid` (e.g. `(Metric &
{ uuid: string })[]`) and removing `as` improves compile-time safety and
prevents malformed mocks from slipping through. This is a real, non-cosmetic
improvement to test robustness and type correctness.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.test.ts
**Line:** 52:66
**Comment:**
*Type Error: The test uses a type assertion (`as Metric`) to coerce
plain objects into `Metric`, which can mask missing runtime properties (like
`uuid`) and allow malformed mock data to slip through compilation. Make the
mock type explicitly include `uuid` and remove `as` casts so the compiler
enforces the object shape and prevents accidental omissions that would break
functions expecting `uuid` at runtime.
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/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts:
##########
@@ -0,0 +1,561 @@
+/**
+ * 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 { useCallback, useMemo, useRef, useState } from 'react';
+import { t } from '@superset-ui/core';
+import {
+ UniqueIdentifier,
+ DragStartEvent,
+ DragMoveEvent,
+ DragEndEvent,
+ DragOverEvent,
+} from '@dnd-kit/core';
+import { FoldersEditorItemType } from '../../types';
+import {
+ isDefaultFolder,
+ DEFAULT_COLUMNS_FOLDER_UUID,
+ DEFAULT_METRICS_FOLDER_UUID,
+ TreeItem as TreeItemType,
+ FlattenedTreeItem,
+ DRAG_INDENTATION_WIDTH,
+} from '../constants';
+import { buildTree, getProjection, serializeForAPI } from '../treeUtils';
+
+interface UseDragHandlersProps {
+ items: TreeItemType[];
+ setItems: React.Dispatch<React.SetStateAction<TreeItemType[]>>;
+ computeFlattenedItems: (
+ activeId: UniqueIdentifier | null,
+ ) => FlattenedTreeItem[];
+ fullFlattenedItems: FlattenedTreeItem[];
+ selectedItemIds: Set<string>;
+ onChange: (folders: ReturnType<typeof serializeForAPI>) => void;
+ addWarningToast: (message: string) => void;
+}
+
+export function useDragHandlers({
+ items,
+ setItems,
+ computeFlattenedItems,
+ fullFlattenedItems,
+ selectedItemIds,
+ onChange,
+ addWarningToast,
+}: UseDragHandlersProps) {
+ const [activeId, setActiveId] = useState<UniqueIdentifier | null>(null);
+ const [overId, setOverId] = useState<UniqueIdentifier | null>(null);
+ const [dragOverlayWidth, setDragOverlayWidth] = useState<number |
null>(null);
+ const offsetLeftRef = useRef(0);
+ const [projectedParentId, setProjectedParentId] = useState<string | null>(
+ null,
+ );
+ const [draggedItemIds, setDraggedItemIds] = useState<Set<string>>(new Set());
+
+ const pendingParentIdRef = useRef<string | null>(null);
+ const rafIdRef = useRef<number | null>(null);
+
+ const scheduleProjectedParentUpdate = useCallback(
+ (newParentId: string | null) => {
+ pendingParentIdRef.current = newParentId;
+
+ // Only schedule if not already scheduled
+ if (rafIdRef.current === null) {
+ rafIdRef.current = requestAnimationFrame(() => {
+ rafIdRef.current = null;
+ setProjectedParentId(prev =>
+ prev === pendingParentIdRef.current
+ ? prev
+ : pendingParentIdRef.current,
+ );
+ });
+ }
+ },
+ [],
+ );
+
+ const flattenedItems = useMemo(
+ () => computeFlattenedItems(activeId),
+ [computeFlattenedItems, activeId],
+ );
+
+ const flattenedItemsIndexMap = useMemo(() => {
+ const map = new Map<string, number>();
+ flattenedItems.forEach((item, index) => {
+ map.set(item.uuid, index);
+ });
+ return map;
+ }, [flattenedItems]);
+
+ const resetDragState = useCallback(() => {
+ setActiveId(null);
+ setOverId(null);
+ offsetLeftRef.current = 0;
+ setProjectedParentId(null);
+ setDraggedItemIds(new Set());
+ setDragOverlayWidth(null);
+ if (rafIdRef.current !== null) {
+ cancelAnimationFrame(rafIdRef.current);
+ rafIdRef.current = null;
+ }
+ pendingParentIdRef.current = null;
+ }, []);
+
+ const handleDragStart = ({ active }: DragStartEvent) => {
+ setActiveId(active.id);
+
+ const element = active.rect.current.initial;
+ if (element) {
+ setDragOverlayWidth(element.width);
+ }
+
+ if (selectedItemIds.has(active.id as string)) {
+ setDraggedItemIds(new Set(selectedItemIds));
+ } else {
+ setDraggedItemIds(new Set([active.id as string]));
+ }
+ };
+
+ const handleDragMove = useCallback(
+ ({ delta }: DragMoveEvent) => {
+ offsetLeftRef.current = delta.x;
+
+ if (activeId && overId) {
+ if (typeof overId === 'string' && overId.endsWith('-empty')) {
+ const folderId = overId.replace('-empty', '');
+ scheduleProjectedParentUpdate(folderId);
+ return;
+ }
+
+ const projection = getProjection(
+ flattenedItems,
+ activeId,
+ overId,
+ delta.x,
+ DRAG_INDENTATION_WIDTH,
+ flattenedItemsIndexMap,
+ );
+ const newParentId = projection?.parentId ?? null;
+ scheduleProjectedParentUpdate(newParentId);
+ }
+ },
+ [
+ activeId,
+ overId,
+ flattenedItems,
+ flattenedItemsIndexMap,
+ scheduleProjectedParentUpdate,
+ ],
+ );
+
+ const handleDragOver = useCallback(
+ ({ over }: DragOverEvent) => {
+ setOverId(over?.id ?? null);
+
+ if (activeId && over) {
+ if (typeof over.id === 'string' && over.id.endsWith('-empty')) {
+ const folderId = over.id.replace('-empty', '');
+ scheduleProjectedParentUpdate(folderId);
+ return;
Review Comment:
**Suggestion:** The drag-over handler uses `if (activeId && over)` which
again treats numeric IDs of 0 as falsy and can skip projection updates; change
the check to explicit null/undefined comparison (`activeId != null && over !=
null`) to ensure valid numeric IDs are processed. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
if (activeId != null && over != null) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Similar to the drag-move handler, this handler uses a truthy check (if
(activeId && over)) which will skip projection when activeId is 0. Changing to
explicit null/undefined checks (activeId != null && over != null) ensures valid
numeric IDs are handled and prevents subtle incorrect behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts
**Line:** 174:174
**Comment:**
*Logic Error: The drag-over handler uses `if (activeId && over)` which
again treats numeric IDs of 0 as falsy and can skip projection updates; change
the check to explicit null/undefined comparison (`activeId != null && over !=
null`) to ensure valid numeric IDs are processed.
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/components/Datasource/FoldersEditor/TreeItem.styles.ts:
##########
@@ -0,0 +1,183 @@
+/**
+ * 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 { styled, css } from '@apache-superset/core/ui';
+
+export const FOLDER_INDENTATION_WIDTH = 24;
+export const ITEM_INDENTATION_WIDTH = 4;
+
+export const TreeItemContainer = styled.div<{
+ depth: number;
+ isDragging: boolean;
+ isOver: boolean;
+ isOverlay?: boolean;
+}>`
+ ${({ theme, depth, isDragging, isOverlay }) => `
+ margin: ${theme.marginXXS}px ${isOverlay ? 0 : theme.marginMD}px;
+ margin-left: ${isOverlay ? 0 : (depth - 1) * FOLDER_INDENTATION_WIDTH +
ITEM_INDENTATION_WIDTH}px;
+ padding-left: ${theme.paddingSM}px;
+ display: flex;
+ align-items: center;
+ cursor: pointer;
+ opacity: ${isDragging ? 0.4 : 1};
+ user-select: none;
+ ${isDragging || isOverlay ? 'will-change: transform;' : ''}
+ `}
+`;
+
+export const ItemSeparator = styled.div<{
+ variant: 'visible' | 'transparent';
+}>`
+ ${({ theme, variant }) => `
+ height: 1px;
+ background-color: ${variant === 'visible' ? theme.colorBorderSecondary :
'transparent'};
+ margin: ${variant === 'visible' ? theme.marginLG : theme.marginSM}px
${theme.marginMD}px;
+ margin-left: ${theme.marginSM}px;
+ `}
+`;
+
+export const TreeFolderContainer = styled(TreeItemContainer)<{
+ isDropTarget?: boolean;
+ isForbiddenDropTarget?: boolean;
+}>`
+ ${({ theme, depth, isDropTarget, isForbiddenDropTarget, isOverlay }) => `
+ margin-top: ${isOverlay ? 0 : theme.marginLG}px;
+ margin-bottom: ${isOverlay ? 0 : theme.margin}px;
+ margin-left: ${isOverlay ? 0 : depth * FOLDER_INDENTATION_WIDTH}px;
+ border-radius: ${theme.borderRadius}px;
+ padding: 0 ${theme.paddingSM}px;
+ margin-right: ${isOverlay ? 0 : theme.marginMD}px;
+ transition: background-color 0.15s ease-in-out, box-shadow 0.15s
ease-in-out;
+ ${
+ isDropTarget && isForbiddenDropTarget
+ ? `
+ background-color: ${theme.colorErrorBg};
+ box-shadow: inset 0 0 0 2px ${theme.colorError};
+ cursor: not-allowed;
+ `
+ : isDropTarget
+ ? `
+ background-color: ${theme.colorPrimaryBg};
+ box-shadow: inset 0 0 0 2px ${theme.colorPrimary};
+ `
+ : ''
+ }
+ `}
+`;
+
+export const DragHandle = styled.span`
+ ${({ theme }) => `
+ color: ${theme.colorTextTertiary};
+ display: inline-flex;
+ align-items: center;
+ cursor: grab;
+
+ &:hover {
+ color: ${theme.colorText};
+ }
+
+ &:active {
+ cursor: grabbing;
+ }
+ `}
+`;
+
+export const CollapseButton = styled.span`
+ ${({ theme }) => `
+ display: inline-flex;
+ align-items: center;
+ justify-content: center;
+ width: 12px;
+ height: 12px;
+ cursor: pointer;
+ color: ${theme.colorTextSecondary};
+ margin-left: auto;
+
+ &:hover {
+ color: ${theme.colorText};
+ }
+ `}
+`;
+
+export const DefaultFolderIconContainer = styled.span`
+ ${({ theme }) => `
+ display: inline-flex;
+ align-items: center;
+ color: ${theme.colorTextSecondary};
+ margin-right: ${theme.marginXS}px;
+ `}
+`;
+
+export const FolderName = styled.span`
+ ${({ theme }) => `
+ margin-right: ${theme.marginMD}px;
+ font-weight: ${theme.fontWeightStrong};
+ `}
+`;
+
+export const DragHandleContainer = styled.div`
+ ${({ theme }) => `
+ height: 100%;
+ display: flex;
+ align-items: center;
+ padding: 0 ${theme.sizeUnit}px;
+ margin-left: auto;
+ cursor: grab;
+ color: ${theme.colorTextTertiary};
+
+ &:hover {
+ color: ${theme.colorText};
+ }
+
+ &:active {
+ cursor: grabbing;
+ }
+ `}
+`;
+
+export const EmptyFolderDropZone = styled.div<{
+ depth: number;
+ isOver: boolean;
+ isForbidden: boolean;
+}>`
+ ${({ theme, depth, isOver, isForbidden }) => css`
+ margin: 0 ${depth * ITEM_INDENTATION_WIDTH + theme.marginMD}px;
Review Comment:
**Suggestion:** The empty-folder drop zone currently uses the much smaller
item indentation width to compute its horizontal margin, so its indentation
grows only a few pixels per depth while folder and item rows shift by the
larger folder indentation width, causing the empty-state drop area to visually
drift out of alignment with the tree hierarchy at deeper levels; using the
folder indentation constant here keeps the empty state aligned with the actual
nesting. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
margin: 0 ${depth * FOLDER_INDENTATION_WIDTH + theme.marginMD}px;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Folder rows use FOLDER_INDENTATION_WIDTH for per-level horizontal shifts
while the drop zone uses ITEM_INDENTATION_WIDTH, which only grows a few pixels
per level and will visibly drift out of alignment at deeper depths. Switching
to depth * FOLDER_INDENTATION_WIDTH aligns the empty drop zone with folder/item
rows and fixes a real visual inconsistency.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/FoldersEditor/TreeItem.styles.ts
**Line:** 160:160
**Comment:**
*Logic Error: The empty-folder drop zone currently uses the much
smaller item indentation width to compute its horizontal margin, so its
indentation grows only a few pixels per depth while folder and item rows shift
by the larger folder indentation width, causing the empty-state drop area to
visually drift out of alignment with the tree hierarchy at deeper levels; using
the folder indentation constant here keeps the empty state aligned with the
actual nesting.
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/components/Datasource/FoldersEditor/folderOperations.test.ts:
##########
@@ -0,0 +1,751 @@
+/**
+ * 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 { Metric } from '@superset-ui/chart-controls';
+import { ColumnObject } from 'src/features/datasets/types';
+import {
+ DEFAULT_METRICS_FOLDER_UUID,
+ DEFAULT_COLUMNS_FOLDER_UUID,
+ isDefaultFolder,
+} from './constants';
+import {
+ createFolder,
+ deleteFolder,
+ resetToDefault,
+ filterItemsBySearch,
+ renameFolder,
+ nestFolder,
+ reorderFolders,
+ moveItems,
+ cleanupFolders,
+ getAllSelectedItems,
+ areAllVisibleItemsSelected,
+ ensureDefaultFolders,
+} from './folderOperations';
+import {
+ canDropItems,
+ canDropFolder,
+ getFolderDescendants,
+ findFolderById,
+ validateFolders,
+} from './folderValidation';
+import { DatasourceFolder } from
'src/explore/components/DatasourcePanel/types';
+import { FoldersEditorItemType } from '../types';
+
+describe('folderUtils', () => {
+ const mockMetrics: Metric[] = [
+ {
+ id: 1,
+ uuid: 'metric-1',
+ metric_name: 'Test Metric 1',
+ metric_type: 'count',
+ expression: 'COUNT(*)',
+ } as Metric,
+ {
+ id: 2,
+ uuid: 'metric-2',
+ metric_name: 'Test Metric 2',
+ metric_type: 'sum',
+ expression: 'SUM(value)',
+ } as Metric,
+ ];
+
+ const mockColumns: (ColumnObject & { uuid: string })[] = [
+ {
+ id: 1,
+ uuid: 'column-1',
+ column_name: 'Test Column 1',
+ type: 'VARCHAR',
+ filterable: true,
+ groupby: true,
+ is_active: true,
+ is_dttm: false,
+ },
+ {
+ id: 2,
+ uuid: 'column-2',
+ column_name: 'Test Column 2',
+ type: 'INTEGER',
+ filterable: true,
+ groupby: true,
+ is_active: true,
+ is_dttm: false,
+ },
+ ];
+
+ describe('createFolder', () => {
+ test('should create a folder with correct properties', () => {
+ const folder = createFolder('Test Folder');
+
+ expect(folder.name).toBe('Test Folder');
+ expect(folder.type).toBe(FoldersEditorItemType.Folder);
+ expect(folder.children).toEqual([]);
+ expect(folder.uuid).toBeDefined();
+ });
+ });
+
+ describe('resetToDefault', () => {
+ test('should create default folders with correct structure', () => {
+ const result = resetToDefault(mockMetrics, mockColumns);
+
+ expect(result).toHaveLength(2);
+
+ const metricsFolder = result.find(
+ f => f.uuid === DEFAULT_METRICS_FOLDER_UUID,
+ );
+ const columnsFolder = result.find(
+ f => f.uuid === DEFAULT_COLUMNS_FOLDER_UUID,
+ );
+
+ expect(metricsFolder).toBeDefined();
+ expect(metricsFolder?.name).toBe('Metrics');
+ expect(metricsFolder?.children).toHaveLength(2);
+
+ expect(columnsFolder).toBeDefined();
+ expect(columnsFolder?.name).toBe('Columns');
+ expect(columnsFolder?.children).toHaveLength(2);
+ });
+ });
+
+ describe('filterItemsBySearch', () => {
+ test('should filter items by search term', () => {
+ const allItems = [...mockMetrics, ...mockColumns];
+ const result = filterItemsBySearch('Test Metric', allItems);
+
+ expect(result.size).toBe(2);
+ expect(result.has('metric-1')).toBe(true);
+ expect(result.has('metric-2')).toBe(true);
+ });
+
+ test('should return all items for empty search', () => {
+ const allItems = [...mockMetrics, ...mockColumns];
+ const result = filterItemsBySearch('', allItems);
+
+ expect(result.size).toBe(4);
+ });
+ });
+
+ describe('renameFolder', () => {
+ test('should rename folder correctly', () => {
+ const folders = resetToDefault(mockMetrics, mockColumns);
+ const result = renameFolder(
+ DEFAULT_METRICS_FOLDER_UUID,
+ 'Custom Metrics',
+ folders,
+ );
+
+ const renamedFolder = result.find(
+ f => f.uuid === DEFAULT_METRICS_FOLDER_UUID,
+ );
+ expect(renamedFolder?.name).toBe('Custom Metrics');
+ });
+ });
+
+ describe('canDropItems', () => {
+ test('should allow dropping metrics in Metrics folder', () => {
+ const folders = resetToDefault(mockMetrics, mockColumns);
+ const result = canDropItems(
+ ['metric-1'],
+ DEFAULT_METRICS_FOLDER_UUID,
+ folders,
+ mockMetrics,
+ mockColumns,
+ );
+
+ expect(result).toBe(true);
+ });
+
+ test('should not allow dropping columns in Metrics folder', () => {
+ const folders = resetToDefault(mockMetrics, mockColumns);
+ const result = canDropItems(
+ ['column-1'],
+ DEFAULT_METRICS_FOLDER_UUID,
+ folders,
+ mockMetrics,
+ mockColumns,
+ );
+
+ expect(result).toBe(false);
+ });
+
+ test('should allow dropping any items in custom folders', () => {
+ const customFolder = createFolder('Custom Folder');
+ const folders = [customFolder];
+
+ const metricResult = canDropItems(
+ ['metric-1'],
+ customFolder.uuid,
+ folders,
+ mockMetrics,
+ mockColumns,
+ );
+ const columnResult = canDropItems(
+ ['column-1'],
+ customFolder.uuid,
+ folders,
+ mockMetrics,
+ mockColumns,
+ );
+
+ expect(metricResult).toBe(true);
+ expect(columnResult).toBe(true);
+ });
+ });
+
+ describe('isDefaultFolder', () => {
+ test('should identify default folders by UUID', () => {
+ expect(isDefaultFolder(DEFAULT_METRICS_FOLDER_UUID)).toBe(true);
+ expect(isDefaultFolder(DEFAULT_COLUMNS_FOLDER_UUID)).toBe(true);
+ expect(isDefaultFolder('custom-folder-uuid')).toBe(false);
+ });
+ });
+
+ describe('validateFolders', () => {
+ test('should validate folders successfully', () => {
+ const folders = resetToDefault(mockMetrics, mockColumns);
+ const result = validateFolders(folders);
+
+ expect(result.isValid).toBe(true);
+ expect(result.errors).toHaveLength(0);
+ });
+
+ test('should allow empty folders without names', () => {
+ // Empty folders without names are valid (they get filtered out anyway)
+ const folders = [createFolder('')];
+ const result = validateFolders(folders);
+
+ expect(result.isValid).toBe(true);
+ expect(result.errors).toHaveLength(0);
+ });
+
+ test('should detect folders with content but no name', () => {
+ const folder = createFolder('');
+ folder.children = [
+ { uuid: 'metric-1', type: FoldersEditorItemType.Metric, name: 'Test' },
+ ];
+ const folders = [folder];
+ const result = validateFolders(folders);
+
+ expect(result.isValid).toBe(false);
+ expect(result.errors).toContain('Folder with content must have a name');
+ });
+
+ test('should detect duplicate folder names', () => {
+ const folder1 = createFolder('My Folder');
+ const folder2 = createFolder('My Folder');
+ const folders = [folder1, folder2];
+ const result = validateFolders(folders);
+
+ expect(result.isValid).toBe(false);
+ expect(result.errors.some(e => e.includes('my folder'))).toBe(true);
+ });
+
+ test('should detect duplicate folder names case-insensitively', () => {
+ const folder1 = createFolder('Test Folder');
+ const folder2 = createFolder('test folder');
+ const folders = [folder1, folder2];
+ const result = validateFolders(folders);
+
+ expect(result.isValid).toBe(false);
+ expect(result.errors.some(e => e.includes('test folder'))).toBe(true);
+ });
+ });
+
+ describe('ensureDefaultFolders', () => {
+ test('should create default folders when none exist', () => {
+ const result = ensureDefaultFolders([], mockMetrics, mockColumns);
+
+ expect(result).toHaveLength(2);
+
+ const metricsFolder = result.find(
+ f => f.uuid === DEFAULT_METRICS_FOLDER_UUID,
+ );
+ const columnsFolder = result.find(
+ f => f.uuid === DEFAULT_COLUMNS_FOLDER_UUID,
+ );
+
+ expect(metricsFolder).toBeDefined();
+ expect(columnsFolder).toBeDefined();
+ });
+
+ test('should preserve existing folders', () => {
+ const existingFolders = [createFolder('Custom Folder')];
+ const result = ensureDefaultFolders(
+ existingFolders,
+ mockMetrics,
+ mockColumns,
+ );
+
+ expect(result.length).toBeGreaterThan(2);
+ expect(result.find(f => f.name === 'Custom Folder')).toBeDefined();
+ });
+ });
+
+ describe('deleteFolder', () => {
+ test('should delete a folder by id', () => {
+ const folders = resetToDefault(mockMetrics, mockColumns);
+ const result = deleteFolder(DEFAULT_METRICS_FOLDER_UUID, folders);
+
+ expect(result).toHaveLength(1);
+ expect(
+ result.find(f => f.uuid === DEFAULT_METRICS_FOLDER_UUID),
+ ).toBeUndefined();
+ expect(
+ result.find(f => f.uuid === DEFAULT_COLUMNS_FOLDER_UUID),
+ ).toBeDefined();
+ });
+
+ test('should delete nested folders', () => {
+ const parentFolder: DatasourceFolder = {
+ uuid: 'parent',
+ type: FoldersEditorItemType.Folder,
+ name: 'Parent',
+ children: [
+ {
+ uuid: 'child',
+ type: FoldersEditorItemType.Folder,
+ name: 'Child',
+ children: [],
+ } as DatasourceFolder,
+ ],
+ };
+ const folders = [parentFolder];
+ const result = deleteFolder('child', folders);
+
+ expect(result).toHaveLength(1);
+ expect(
+ (result[0].children as DatasourceFolder[]).find(
+ c => c.uuid === 'child',
+ ),
+ ).toBeUndefined();
+ });
+
+ test('should return unchanged array if folder not found', () => {
+ const folders = resetToDefault(mockMetrics, mockColumns);
+ const result = deleteFolder('nonexistent', folders);
+
+ expect(result).toHaveLength(2);
+ });
+ });
+
+ describe('nestFolder', () => {
+ test('should nest a folder inside another folder', () => {
+ const folder1: DatasourceFolder = {
+ uuid: 'folder1',
+ type: FoldersEditorItemType.Folder,
+ name: 'Folder 1',
+ children: [],
+ };
+ const folder2: DatasourceFolder = {
+ uuid: 'folder2',
+ type: FoldersEditorItemType.Folder,
+ name: 'Folder 2',
+ children: [],
+ };
+ const folders = [folder1, folder2];
+
+ const result = nestFolder('folder2', 'folder1', folders);
+
+ expect(result).toHaveLength(1);
+ expect(result[0].uuid).toBe('folder1');
+ expect(result[0].children).toHaveLength(1);
+ expect((result[0].children as DatasourceFolder[])[0].uuid).toBe(
+ 'folder2',
+ );
+ });
+
+ test('should return unchanged if folder to move not found', () => {
+ const folder1: DatasourceFolder = {
+ uuid: 'folder1',
+ type: FoldersEditorItemType.Folder,
+ name: 'Folder 1',
+ children: [],
+ };
+ const folders = [folder1];
+
+ const result = nestFolder('nonexistent', 'folder1', folders);
+
+ expect(result).toEqual(folders);
+ });
+ });
+
+ describe('reorderFolders', () => {
+ test('should reorder folders at root level', () => {
+ const folder1: DatasourceFolder = {
+ uuid: 'folder1',
+ type: FoldersEditorItemType.Folder,
+ name: 'Folder 1',
+ children: [],
+ };
+ const folder2: DatasourceFolder = {
+ uuid: 'folder2',
+ type: FoldersEditorItemType.Folder,
+ name: 'Folder 2',
+ children: [],
+ };
+ const folder3: DatasourceFolder = {
+ uuid: 'folder3',
+ type: FoldersEditorItemType.Folder,
+ name: 'Folder 3',
+ children: [],
+ };
+ const folders = [folder1, folder2, folder3];
+
+ const result = reorderFolders('folder3', 0, folders);
+
+ expect(result[0].uuid).toBe('folder3');
Review Comment:
**Suggestion:** The reorder test only asserts ordering but doesn't assert
the resulting list length equals the original, so a buggy implementation that
drops or duplicates folders could still satisfy the current expectations; add
an assertion that the length is preserved to catch such regressions. [possible
bug]
**Severity Level:** Critical 🚨
```suggestion
// Ensure no items were lost or duplicated during reorder
expect(result).toHaveLength(folders.length);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Adding an assertion that result length equals the original length guards
against implementations that accidentally drop or duplicate items during
reorder. This is a small, valuable test hardening change that can catch
regressions in reorder logic.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.test.ts
**Line:** 412:412
**Comment:**
*Possible Bug: The reorder test only asserts ordering but doesn't
assert the resulting list length equals the original, so a buggy implementation
that drops or duplicates folders could still satisfy the current expectations;
add an assertion that the length is preserved to catch such regressions.
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/components/Datasource/FoldersEditor/TreeItem.styles.ts:
##########
@@ -0,0 +1,183 @@
+/**
+ * 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 { styled, css } from '@apache-superset/core/ui';
+
+export const FOLDER_INDENTATION_WIDTH = 24;
+export const ITEM_INDENTATION_WIDTH = 4;
+
+export const TreeItemContainer = styled.div<{
+ depth: number;
+ isDragging: boolean;
+ isOver: boolean;
+ isOverlay?: boolean;
+}>`
+ ${({ theme, depth, isDragging, isOverlay }) => `
+ margin: ${theme.marginXXS}px ${isOverlay ? 0 : theme.marginMD}px;
+ margin-left: ${isOverlay ? 0 : (depth - 1) * FOLDER_INDENTATION_WIDTH +
ITEM_INDENTATION_WIDTH}px;
Review Comment:
**Suggestion:** The current margin-left calculation for non-folder items
will become negative when an item has depth 0, which is allowed by the shared
tree utilities and would cause such items to render shifted off-screen or
clipped instead of aligned at the left edge; clamping the depth used in the
calculation to a minimum of 0 prevents this visual break for any root-level
non-folder items. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
margin-left: ${isOverlay ? 0 : Math.max(depth - 1, 0) *
FOLDER_INDENTATION_WIDTH + ITEM_INDENTATION_WIDTH}px;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing expression produces a negative margin when depth === 0: (0 - 1)
* 24 + 4 = -20px, which will shift root-level non-folder items left off the
intended alignment. Clamping with Math.max(depth - 1, 0) is a minimal, correct
defensive change that prevents a real visual bug. There's no indication in the
PR that negative indentation is intended.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/FoldersEditor/TreeItem.styles.ts
**Line:** 33:33
**Comment:**
*Logic Error: The current margin-left calculation for non-folder items
will become negative when an item has depth 0, which is allowed by the shared
tree utilities and would cause such items to render shifted off-screen or
clipped instead of aligned at the left edge; clamping the depth used in the
calculation to a minimum of 0 prevents this visual break for any root-level
non-folder items.
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/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts:
##########
@@ -0,0 +1,561 @@
+/**
+ * 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 { useCallback, useMemo, useRef, useState } from 'react';
+import { t } from '@superset-ui/core';
+import {
+ UniqueIdentifier,
+ DragStartEvent,
+ DragMoveEvent,
+ DragEndEvent,
+ DragOverEvent,
+} from '@dnd-kit/core';
+import { FoldersEditorItemType } from '../../types';
+import {
+ isDefaultFolder,
+ DEFAULT_COLUMNS_FOLDER_UUID,
+ DEFAULT_METRICS_FOLDER_UUID,
+ TreeItem as TreeItemType,
+ FlattenedTreeItem,
+ DRAG_INDENTATION_WIDTH,
+} from '../constants';
+import { buildTree, getProjection, serializeForAPI } from '../treeUtils';
+
+interface UseDragHandlersProps {
+ items: TreeItemType[];
+ setItems: React.Dispatch<React.SetStateAction<TreeItemType[]>>;
+ computeFlattenedItems: (
+ activeId: UniqueIdentifier | null,
+ ) => FlattenedTreeItem[];
+ fullFlattenedItems: FlattenedTreeItem[];
+ selectedItemIds: Set<string>;
+ onChange: (folders: ReturnType<typeof serializeForAPI>) => void;
+ addWarningToast: (message: string) => void;
+}
+
+export function useDragHandlers({
+ items,
+ setItems,
+ computeFlattenedItems,
+ fullFlattenedItems,
+ selectedItemIds,
+ onChange,
+ addWarningToast,
+}: UseDragHandlersProps) {
+ const [activeId, setActiveId] = useState<UniqueIdentifier | null>(null);
+ const [overId, setOverId] = useState<UniqueIdentifier | null>(null);
+ const [dragOverlayWidth, setDragOverlayWidth] = useState<number |
null>(null);
+ const offsetLeftRef = useRef(0);
+ const [projectedParentId, setProjectedParentId] = useState<string | null>(
+ null,
+ );
+ const [draggedItemIds, setDraggedItemIds] = useState<Set<string>>(new Set());
+
+ const pendingParentIdRef = useRef<string | null>(null);
+ const rafIdRef = useRef<number | null>(null);
+
+ const scheduleProjectedParentUpdate = useCallback(
+ (newParentId: string | null) => {
+ pendingParentIdRef.current = newParentId;
+
+ // Only schedule if not already scheduled
+ if (rafIdRef.current === null) {
+ rafIdRef.current = requestAnimationFrame(() => {
+ rafIdRef.current = null;
+ setProjectedParentId(prev =>
+ prev === pendingParentIdRef.current
+ ? prev
+ : pendingParentIdRef.current,
+ );
+ });
+ }
+ },
+ [],
+ );
+
+ const flattenedItems = useMemo(
+ () => computeFlattenedItems(activeId),
+ [computeFlattenedItems, activeId],
+ );
+
+ const flattenedItemsIndexMap = useMemo(() => {
+ const map = new Map<string, number>();
+ flattenedItems.forEach((item, index) => {
+ map.set(item.uuid, index);
+ });
+ return map;
+ }, [flattenedItems]);
+
+ const resetDragState = useCallback(() => {
+ setActiveId(null);
+ setOverId(null);
+ offsetLeftRef.current = 0;
+ setProjectedParentId(null);
+ setDraggedItemIds(new Set());
+ setDragOverlayWidth(null);
+ if (rafIdRef.current !== null) {
+ cancelAnimationFrame(rafIdRef.current);
+ rafIdRef.current = null;
+ }
+ pendingParentIdRef.current = null;
+ }, []);
+
+ const handleDragStart = ({ active }: DragStartEvent) => {
+ setActiveId(active.id);
+
+ const element = active.rect.current.initial;
+ if (element) {
+ setDragOverlayWidth(element.width);
+ }
+
+ if (selectedItemIds.has(active.id as string)) {
+ setDraggedItemIds(new Set(selectedItemIds));
+ } else {
+ setDraggedItemIds(new Set([active.id as string]));
+ }
+ };
+
+ const handleDragMove = useCallback(
+ ({ delta }: DragMoveEvent) => {
+ offsetLeftRef.current = delta.x;
+
+ if (activeId && overId) {
+ if (typeof overId === 'string' && overId.endsWith('-empty')) {
+ const folderId = overId.replace('-empty', '');
+ scheduleProjectedParentUpdate(folderId);
+ return;
+ }
+
+ const projection = getProjection(
+ flattenedItems,
+ activeId,
+ overId,
+ delta.x,
+ DRAG_INDENTATION_WIDTH,
+ flattenedItemsIndexMap,
+ );
+ const newParentId = projection?.parentId ?? null;
+ scheduleProjectedParentUpdate(newParentId);
+ }
+ },
+ [
+ activeId,
+ overId,
+ flattenedItems,
+ flattenedItemsIndexMap,
+ scheduleProjectedParentUpdate,
+ ],
+ );
+
+ const handleDragOver = useCallback(
+ ({ over }: DragOverEvent) => {
+ setOverId(over?.id ?? null);
+
+ if (activeId && over) {
+ if (typeof over.id === 'string' && over.id.endsWith('-empty')) {
+ const folderId = over.id.replace('-empty', '');
+ scheduleProjectedParentUpdate(folderId);
+ return;
+ }
+
+ const projection = getProjection(
+ flattenedItems,
+ activeId,
+ over.id,
+ offsetLeftRef.current,
+ DRAG_INDENTATION_WIDTH,
+ flattenedItemsIndexMap,
+ );
+ const newParentId = projection?.parentId ?? null;
+ scheduleProjectedParentUpdate(newParentId);
+ } else {
+ scheduleProjectedParentUpdate(null);
+ }
+ },
+ [
+ activeId,
+ flattenedItems,
+ flattenedItemsIndexMap,
+ scheduleProjectedParentUpdate,
+ ],
+ );
+
+ const handleDragEnd = ({ active, over }: DragEndEvent) => {
+ const itemsBeingDragged = Array.from(draggedItemIds);
+ const finalOffsetLeft = offsetLeftRef.current;
+ resetDragState();
+
+ if (!over || itemsBeingDragged.length === 0) {
+ return;
+ }
+
+ let targetOverId = over.id;
+ let isEmptyDrop = false;
+ if (typeof over.id === 'string' && over.id.endsWith('-empty')) {
+ targetOverId = over.id.replace('-empty', '');
+ isEmptyDrop = true;
+
+ if (itemsBeingDragged.includes(targetOverId as string)) {
+ return;
+ }
+ }
+
+ const activeIndex = fullFlattenedItems.findIndex(
+ ({ uuid }) => uuid === active.id,
+ );
+ const overIndex = fullFlattenedItems.findIndex(
+ ({ uuid }) => uuid === targetOverId,
Review Comment:
**Suggestion:** `active.id` and `targetOverId` are UniqueIdentifier values
(may be numbers), while `fullFlattenedItems[].uuid` are strings; comparing them
directly can fail when IDs are numeric (e.g. 0). Coerce identifiers to strings
when comparing to `uuid` to avoid failing to find the indexes and
early-returning. [type error]
**Severity Level:** Minor ⚠️
```suggestion
({ uuid }) => uuid === String(active.id),
);
const overIndex = fullFlattenedItems.findIndex(
({ uuid }) => uuid === String(targetOverId),
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
active.id and targetOverId are UniqueIdentifier values (string | number).
fullFlattenedItems[].uuid is a string. If an id ever comes through as a number
(e.g. 0), strict === will fail to match and cause the function to early-return.
Coercing to String() ensures reliable comparisons and prevents subtle
mismatches at runtime.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts
**Line:** 220:223
**Comment:**
*Type Error: `active.id` and `targetOverId` are UniqueIdentifier values
(may be numbers), while `fullFlattenedItems[].uuid` are strings; comparing them
directly can fail when IDs are numeric (e.g. 0). Coerce identifiers to strings
when comparing to `uuid` to avoid failing to find the indexes and
early-returning.
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/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts:
##########
@@ -0,0 +1,561 @@
+/**
+ * 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 { useCallback, useMemo, useRef, useState } from 'react';
+import { t } from '@superset-ui/core';
+import {
+ UniqueIdentifier,
+ DragStartEvent,
+ DragMoveEvent,
+ DragEndEvent,
+ DragOverEvent,
+} from '@dnd-kit/core';
+import { FoldersEditorItemType } from '../../types';
+import {
+ isDefaultFolder,
+ DEFAULT_COLUMNS_FOLDER_UUID,
+ DEFAULT_METRICS_FOLDER_UUID,
+ TreeItem as TreeItemType,
+ FlattenedTreeItem,
+ DRAG_INDENTATION_WIDTH,
+} from '../constants';
+import { buildTree, getProjection, serializeForAPI } from '../treeUtils';
+
+interface UseDragHandlersProps {
+ items: TreeItemType[];
+ setItems: React.Dispatch<React.SetStateAction<TreeItemType[]>>;
+ computeFlattenedItems: (
+ activeId: UniqueIdentifier | null,
+ ) => FlattenedTreeItem[];
+ fullFlattenedItems: FlattenedTreeItem[];
+ selectedItemIds: Set<string>;
+ onChange: (folders: ReturnType<typeof serializeForAPI>) => void;
+ addWarningToast: (message: string) => void;
+}
+
+export function useDragHandlers({
+ items,
+ setItems,
+ computeFlattenedItems,
+ fullFlattenedItems,
+ selectedItemIds,
+ onChange,
+ addWarningToast,
+}: UseDragHandlersProps) {
+ const [activeId, setActiveId] = useState<UniqueIdentifier | null>(null);
+ const [overId, setOverId] = useState<UniqueIdentifier | null>(null);
+ const [dragOverlayWidth, setDragOverlayWidth] = useState<number |
null>(null);
+ const offsetLeftRef = useRef(0);
+ const [projectedParentId, setProjectedParentId] = useState<string | null>(
+ null,
+ );
+ const [draggedItemIds, setDraggedItemIds] = useState<Set<string>>(new Set());
+
+ const pendingParentIdRef = useRef<string | null>(null);
+ const rafIdRef = useRef<number | null>(null);
+
+ const scheduleProjectedParentUpdate = useCallback(
+ (newParentId: string | null) => {
+ pendingParentIdRef.current = newParentId;
+
+ // Only schedule if not already scheduled
+ if (rafIdRef.current === null) {
+ rafIdRef.current = requestAnimationFrame(() => {
+ rafIdRef.current = null;
+ setProjectedParentId(prev =>
+ prev === pendingParentIdRef.current
+ ? prev
+ : pendingParentIdRef.current,
+ );
+ });
+ }
+ },
+ [],
+ );
+
+ const flattenedItems = useMemo(
+ () => computeFlattenedItems(activeId),
+ [computeFlattenedItems, activeId],
+ );
+
+ const flattenedItemsIndexMap = useMemo(() => {
+ const map = new Map<string, number>();
+ flattenedItems.forEach((item, index) => {
+ map.set(item.uuid, index);
+ });
+ return map;
+ }, [flattenedItems]);
+
+ const resetDragState = useCallback(() => {
+ setActiveId(null);
+ setOverId(null);
+ offsetLeftRef.current = 0;
+ setProjectedParentId(null);
+ setDraggedItemIds(new Set());
+ setDragOverlayWidth(null);
+ if (rafIdRef.current !== null) {
+ cancelAnimationFrame(rafIdRef.current);
+ rafIdRef.current = null;
+ }
+ pendingParentIdRef.current = null;
+ }, []);
+
+ const handleDragStart = ({ active }: DragStartEvent) => {
+ setActiveId(active.id);
+
+ const element = active.rect.current.initial;
+ if (element) {
+ setDragOverlayWidth(element.width);
+ }
+
+ if (selectedItemIds.has(active.id as string)) {
+ setDraggedItemIds(new Set(selectedItemIds));
+ } else {
+ setDraggedItemIds(new Set([active.id as string]));
+ }
+ };
+
+ const handleDragMove = useCallback(
+ ({ delta }: DragMoveEvent) => {
+ offsetLeftRef.current = delta.x;
+
+ if (activeId && overId) {
+ if (typeof overId === 'string' && overId.endsWith('-empty')) {
+ const folderId = overId.replace('-empty', '');
+ scheduleProjectedParentUpdate(folderId);
+ return;
Review Comment:
**Suggestion:** The handler checks `if (activeId && overId)` which treats
numeric IDs of 0 as falsy and will skip projection logic; use explicit
null/undefined checks (`!= null`) so valid numeric IDs (including 0) are
handled. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
if (activeId != null && overId != null) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
activeId and overId are UniqueIdentifier | null. Using a truthy check (if
(activeId && overId)) will treat valid numeric IDs like 0 as falsy and skip the
projection logic. Replacing with explicit null/undefined checks (activeId !=
null && overId != null) is a small correctness fix that avoids a rare but real
bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts
**Line:** 142:142
**Comment:**
*Logic Error: The handler checks `if (activeId && overId)` which treats
numeric IDs of 0 as falsy and will skip projection logic; use explicit
null/undefined checks (`!= null`) so valid numeric IDs (including 0) are
handled.
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/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts:
##########
@@ -0,0 +1,561 @@
+/**
+ * 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 { useCallback, useMemo, useRef, useState } from 'react';
+import { t } from '@superset-ui/core';
+import {
+ UniqueIdentifier,
+ DragStartEvent,
+ DragMoveEvent,
+ DragEndEvent,
+ DragOverEvent,
+} from '@dnd-kit/core';
+import { FoldersEditorItemType } from '../../types';
+import {
+ isDefaultFolder,
+ DEFAULT_COLUMNS_FOLDER_UUID,
+ DEFAULT_METRICS_FOLDER_UUID,
+ TreeItem as TreeItemType,
+ FlattenedTreeItem,
+ DRAG_INDENTATION_WIDTH,
+} from '../constants';
+import { buildTree, getProjection, serializeForAPI } from '../treeUtils';
+
+interface UseDragHandlersProps {
+ items: TreeItemType[];
+ setItems: React.Dispatch<React.SetStateAction<TreeItemType[]>>;
+ computeFlattenedItems: (
+ activeId: UniqueIdentifier | null,
+ ) => FlattenedTreeItem[];
+ fullFlattenedItems: FlattenedTreeItem[];
+ selectedItemIds: Set<string>;
+ onChange: (folders: ReturnType<typeof serializeForAPI>) => void;
+ addWarningToast: (message: string) => void;
+}
+
+export function useDragHandlers({
+ items,
+ setItems,
+ computeFlattenedItems,
+ fullFlattenedItems,
+ selectedItemIds,
+ onChange,
+ addWarningToast,
+}: UseDragHandlersProps) {
+ const [activeId, setActiveId] = useState<UniqueIdentifier | null>(null);
+ const [overId, setOverId] = useState<UniqueIdentifier | null>(null);
+ const [dragOverlayWidth, setDragOverlayWidth] = useState<number |
null>(null);
+ const offsetLeftRef = useRef(0);
+ const [projectedParentId, setProjectedParentId] = useState<string | null>(
+ null,
+ );
+ const [draggedItemIds, setDraggedItemIds] = useState<Set<string>>(new Set());
+
+ const pendingParentIdRef = useRef<string | null>(null);
+ const rafIdRef = useRef<number | null>(null);
+
+ const scheduleProjectedParentUpdate = useCallback(
+ (newParentId: string | null) => {
+ pendingParentIdRef.current = newParentId;
+
+ // Only schedule if not already scheduled
+ if (rafIdRef.current === null) {
+ rafIdRef.current = requestAnimationFrame(() => {
+ rafIdRef.current = null;
+ setProjectedParentId(prev =>
+ prev === pendingParentIdRef.current
+ ? prev
+ : pendingParentIdRef.current,
+ );
+ });
+ }
+ },
+ [],
+ );
+
+ const flattenedItems = useMemo(
+ () => computeFlattenedItems(activeId),
+ [computeFlattenedItems, activeId],
+ );
+
+ const flattenedItemsIndexMap = useMemo(() => {
+ const map = new Map<string, number>();
+ flattenedItems.forEach((item, index) => {
+ map.set(item.uuid, index);
+ });
+ return map;
+ }, [flattenedItems]);
+
+ const resetDragState = useCallback(() => {
+ setActiveId(null);
+ setOverId(null);
+ offsetLeftRef.current = 0;
+ setProjectedParentId(null);
+ setDraggedItemIds(new Set());
+ setDragOverlayWidth(null);
+ if (rafIdRef.current !== null) {
+ cancelAnimationFrame(rafIdRef.current);
+ rafIdRef.current = null;
+ }
+ pendingParentIdRef.current = null;
+ }, []);
+
+ const handleDragStart = ({ active }: DragStartEvent) => {
+ setActiveId(active.id);
+
+ const element = active.rect.current.initial;
+ if (element) {
+ setDragOverlayWidth(element.width);
+ }
+
+ if (selectedItemIds.has(active.id as string)) {
+ setDraggedItemIds(new Set(selectedItemIds));
+ } else {
+ setDraggedItemIds(new Set([active.id as string]));
+ }
+ };
+
+ const handleDragMove = useCallback(
+ ({ delta }: DragMoveEvent) => {
+ offsetLeftRef.current = delta.x;
+
+ if (activeId && overId) {
+ if (typeof overId === 'string' && overId.endsWith('-empty')) {
+ const folderId = overId.replace('-empty', '');
+ scheduleProjectedParentUpdate(folderId);
+ return;
+ }
+
+ const projection = getProjection(
+ flattenedItems,
+ activeId,
+ overId,
+ delta.x,
+ DRAG_INDENTATION_WIDTH,
+ flattenedItemsIndexMap,
+ );
+ const newParentId = projection?.parentId ?? null;
+ scheduleProjectedParentUpdate(newParentId);
+ }
+ },
+ [
+ activeId,
+ overId,
+ flattenedItems,
+ flattenedItemsIndexMap,
+ scheduleProjectedParentUpdate,
+ ],
+ );
+
+ const handleDragOver = useCallback(
+ ({ over }: DragOverEvent) => {
+ setOverId(over?.id ?? null);
+
+ if (activeId && over) {
+ if (typeof over.id === 'string' && over.id.endsWith('-empty')) {
+ const folderId = over.id.replace('-empty', '');
+ scheduleProjectedParentUpdate(folderId);
+ return;
+ }
+
+ const projection = getProjection(
+ flattenedItems,
+ activeId,
+ over.id,
+ offsetLeftRef.current,
+ DRAG_INDENTATION_WIDTH,
+ flattenedItemsIndexMap,
+ );
+ const newParentId = projection?.parentId ?? null;
+ scheduleProjectedParentUpdate(newParentId);
+ } else {
+ scheduleProjectedParentUpdate(null);
+ }
+ },
+ [
+ activeId,
+ flattenedItems,
+ flattenedItemsIndexMap,
+ scheduleProjectedParentUpdate,
+ ],
+ );
+
+ const handleDragEnd = ({ active, over }: DragEndEvent) => {
+ const itemsBeingDragged = Array.from(draggedItemIds);
+ const finalOffsetLeft = offsetLeftRef.current;
+ resetDragState();
+
+ if (!over || itemsBeingDragged.length === 0) {
+ return;
+ }
+
+ let targetOverId = over.id;
+ let isEmptyDrop = false;
+ if (typeof over.id === 'string' && over.id.endsWith('-empty')) {
+ targetOverId = over.id.replace('-empty', '');
+ isEmptyDrop = true;
+
+ if (itemsBeingDragged.includes(targetOverId as string)) {
+ return;
+ }
+ }
+
+ const activeIndex = fullFlattenedItems.findIndex(
+ ({ uuid }) => uuid === active.id,
+ );
+ const overIndex = fullFlattenedItems.findIndex(
+ ({ uuid }) => uuid === targetOverId,
+ );
+
+ if (activeIndex === -1 || overIndex === -1) {
+ return;
+ }
+
+ const draggedItems = fullFlattenedItems.filter((item: FlattenedTreeItem) =>
+ itemsBeingDragged.includes(item.uuid),
+ );
+
+ let projectedPosition = getProjection(
+ flattenedItems,
+ active.id,
+ targetOverId,
+ finalOffsetLeft,
+ DRAG_INDENTATION_WIDTH,
+ );
Review Comment:
**Suggestion:** The call to `getProjection` in the drag-end handler omits
the `flattenedItemsIndexMap` argument that other calls pass; if `getProjection`
relies on that map the incorrect call can return a wrong projection or
undefined, causing incorrect nesting/movement or early returns — add
`flattenedItemsIndexMap` as the final argument to match other usages. [possible
bug]
**Severity Level:** Critical 🚨
```suggestion
flattenedItemsIndexMap,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Other calls to getProjection in this file pass flattenedItemsIndexMap as the
final argument (see handleDragMove/handleDragOver). The drag-end call omits
that map, which is likely part of getProjection's inputs (the function is
consistently called with that map elsewhere). Omitting it can change projection
results or return undefined, causing incorrect insertion decisions or early
returns. Adding the missing argument aligns this call with the rest of the file
and fixes a real bug risk.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts
**Line:** 240:240
**Comment:**
*Possible Bug: The call to `getProjection` in the drag-end handler
omits the `flattenedItemsIndexMap` argument that other calls pass; if
`getProjection` relies on that map the incorrect call can return a wrong
projection or undefined, causing incorrect nesting/movement or early returns —
add `flattenedItemsIndexMap` as the final argument to match other usages.
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]
