codeant-ai-for-open-source[bot] commented on code in PR #36239:
URL: https://github.com/apache/superset/pull/36239#discussion_r2754223528
##########
superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnTypeLabel/ColumnTypeLabel.tsx:
##########
@@ -59,7 +60,9 @@ export function ColumnTypeLabel({ type }:
ColumnTypeLabelProps) {
<QuestionOutlined aria-label={t('unknown type icon')} />
);
- if (type === '' || type === 'expression') {
+ if (type === 'metric') {
+ typeIcon = <Icons.Sigma aria-label={t('metric type icon')} />;
+ } else if (type === '' || type === 'expression') {
Review Comment:
**Suggestion:** The `ColumnTypeLabel` component is being called with the
string value `aggregate` (e.g., from `AggregateOption`), but this case is not
handled in the type switch, so aggregate functions fall back to the generic
"unknown type" icon instead of a more appropriate function-style icon; treating
`aggregate` like an expression fixes this logic mismatch and aligns the UI with
caller intent. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Aggregate icon shows as unknown in Metric aggregate UI.
- ⚠️ Metric pickers and aggregate option displays inconsistent.
- ⚠️ Tests referencing AggregateOption may assert wrong icon.
```
</details>
```suggestion
} else if (type === '' || type === 'expression' || type === 'aggregate') {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the file
superset-frontend/src/explore/components/controls/MetricControl/AggregateOption.tsx
and
inspect line 33 where the component renders the type icon: `{showType &&
<ColumnTypeLabel
type={'aggregate' as any} />}` (file and line discovered via Grep:
"src/explore/components/controls/MetricControl/AggregateOption.tsx:33:
{showType &&
<ColumnTypeLabel type={'aggregate' as any} />}" ).
2. With the current PR code, when AggregateOption renders, it passes the
literal string
'aggregate' into ColumnTypeLabel (as above). ColumnTypeLabel implementation
(file:
superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnTypeLabel/ColumnTypeLabel.tsx)
checks for '' and 'expression' but does not check for 'aggregate' (existing
code at lines
shown in PR: "65 } else if (type === '' || type === 'expression') {" and "67
typeIcon =
<FunctionOutlined aria-label={t('function type icon')} />;").
3. Render the UI that shows AggregateOption (e.g., open the metric selection
UI or run the
unit test that mounts AggregateOption — tests referencing AggregateOption
exist at
"src/explore/components/controls/MetricControl/AggregateOption.test.tsx:25").
Observe that
instead of the function-style icon, the ColumnTypeLabel falls back to the
default
QuestionOutlined unknown icon (because 'aggregate' does not match any
handled branch).
4. Apply the proposed change (treat 'aggregate' like 'expression') and
re-run the same
render (UI or test). Confirm the FunctionOutlined (function-style) icon now
appears for
AggregateOption ('aggregate'), matching caller intent seen in
AggregateOption.tsx.
```
</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-chart-controls/src/components/ColumnTypeLabel/ColumnTypeLabel.tsx
**Line:** 65:65
**Comment:**
*Logic Error: The `ColumnTypeLabel` component is being called with the
string value `aggregate` (e.g., from `AggregateOption`), but this case is not
handled in the type switch, so aggregate functions fall back to the generic
"unknown type" icon instead of a more appropriate function-style icon; treating
`aggregate` like an expression fixes this logic mismatch and aligns the UI with
caller intent.
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/VirtualizedTreeItem.tsx:
##########
@@ -0,0 +1,220 @@
+/**
+ * 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 { CSSProperties, memo } from 'react';
+import type { ListChildComponentProps } from 'react-window';
+import { useDroppable } from '@dnd-kit/core';
+import type { UniqueIdentifier } from '@dnd-kit/core';
+import type { Metric, ColumnMeta } from '@superset-ui/chart-controls';
+import { FoldersEditorItemType } from '../types';
+import type { FlattenedTreeItem } from './constants';
+import { isDefaultFolder } from './constants';
+import { TreeItem } from './TreeItem';
+
+// Invisible placeholder that keeps the droppable area for horizontal drag
depth changes
+interface DragPlaceholderProps {
+ id: string;
+ style: CSSProperties;
+ type: FoldersEditorItemType;
+ isFolder: boolean;
+}
+
+const DragPlaceholder = memo(function DragPlaceholder({
+ id,
+ style,
+ type,
+ isFolder,
+}: DragPlaceholderProps) {
+ const { setNodeRef } = useDroppable({
+ id,
+ data: { type, isFolder },
+ });
+
+ return <div ref={setNodeRef} style={{ ...style, visibility: 'hidden' }} />;
+});
+
+export interface VirtualizedTreeItemData {
+ flattenedItems: FlattenedTreeItem[];
+ collapsedIds: Set<string>;
+ selectedItemIds: Set<string>;
+ editingFolderId: string | null;
+ folderChildCounts: Map<string, number>;
+ itemSeparatorInfo: Map<string, 'visible' | 'transparent'>;
+ visibleItemIds: Set<string>;
+ searchTerm: string;
+ metricsMap: Map<string, Metric>;
+ columnsMap: Map<string, ColumnMeta>;
+ activeId: UniqueIdentifier | null;
+ forbiddenDropFolderIds: Set<string>;
+ currentDropTargetId: string | null;
+ onToggleCollapse: (id: string) => void;
+ onSelect: (id: string, selected: boolean, shiftKey?: boolean) => void;
+ onStartEdit: (id: string) => void;
+ onFinishEdit: (id: string, newName: string) => void;
+}
+
+// Inner component that receives state as props for proper memoization
+interface TreeItemWrapperProps {
+ item: FlattenedTreeItem;
+ style: CSSProperties;
+ isFolder: boolean;
+ isCollapsed: boolean;
+ isSelected: boolean;
+ isEditing: boolean;
+ showEmptyState: boolean;
+ separatorType?: 'visible' | 'transparent';
+ isForbiddenDrop: boolean;
+ isDropTarget: boolean;
+ metric?: Metric;
+ column?: ColumnMeta;
+ onToggleCollapse?: (id: string) => void;
+ onSelect?: (id: string, selected: boolean, shiftKey?: boolean) => void;
+ onStartEdit?: (id: string) => void;
+ onFinishEdit?: (id: string, newName: string) => void;
+}
+
+const TreeItemWrapper = memo(function TreeItemWrapper({
+ item,
+ style,
+ isFolder,
+ isCollapsed,
+ isSelected,
+ isEditing,
+ showEmptyState,
+ separatorType,
+ isForbiddenDrop,
+ isDropTarget,
+ metric,
+ column,
+ onToggleCollapse,
+ onSelect,
+ onStartEdit,
+ onFinishEdit,
+}: TreeItemWrapperProps) {
+ return (
+ <div style={style}>
+ <TreeItem
+ id={item.uuid}
+ type={item.type}
+ name={item.name}
+ depth={item.depth}
+ isFolder={isFolder}
+ isCollapsed={isCollapsed}
+ isSelected={isSelected}
+ isEditing={isEditing}
+ isDefaultFolder={isDefaultFolder(item.uuid)}
+ showEmptyState={showEmptyState}
+ separatorType={separatorType}
+ isForbiddenDrop={isForbiddenDrop}
+ isDropTarget={isDropTarget}
+ onToggleCollapse={onToggleCollapse}
+ onSelect={onSelect}
+ onStartEdit={onStartEdit}
+ onFinishEdit={onFinishEdit}
+ metric={metric}
+ column={column}
+ />
+ </div>
+ );
+});
+
+function VirtualizedTreeItemComponent({
+ index,
+ style,
+ data,
+}: ListChildComponentProps<VirtualizedTreeItemData>) {
+ const {
+ flattenedItems,
+ collapsedIds,
+ selectedItemIds,
+ editingFolderId,
+ folderChildCounts,
+ itemSeparatorInfo,
+ visibleItemIds,
+ searchTerm,
+ metricsMap,
+ columnsMap,
+ activeId,
+ forbiddenDropFolderIds,
+ currentDropTargetId,
+ onToggleCollapse,
+ onSelect,
+ onStartEdit,
+ onFinishEdit,
+ } = data;
+
+ const item = flattenedItems[index];
+
+ if (!item) {
+ return null;
+ }
+
+ const isFolder = item.type === FoldersEditorItemType.Folder;
+
+ // Hide items that don't match search (unless they're folders)
+ if (!isFolder && searchTerm && !visibleItemIds.has(item.uuid)) {
+ return null;
+ }
+
+ // Render invisible placeholder for active dragged item - keeps droppable
area
+ // for horizontal drag depth changes while visual is in DragOverlay
+ if (activeId === item.uuid) {
+ return (
+ <DragPlaceholder
+ id={item.uuid}
+ style={style}
+ type={item.type}
+ isFolder={isFolder}
+ />
+ );
Review Comment:
**Suggestion:** The search filter check runs before the `activeId`
placeholder check, so if the user starts dragging a metric/column and then
changes the search so that the dragged item no longer matches, the active item
row is short-circuited by the search filter and returns `null`. This removes
the droppable placeholder for the active item during an ongoing drag, which can
break horizontal drag-depth adjustments and drop behavior for that item. Move
the `activeId` check before the search filter so the placeholder is always
rendered for the active item, even if it no longer matches the current search.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset modal Folders tab drag-and-drop breaks mid-drag.
- ⚠️ Horizontal drag-depth adjustments behave inconsistently.
- ⚠️ Moving metrics/columns between folders unreliable.
- ⚠️ UX regression during search while dragging items.
```
</details>
```suggestion
// Render invisible placeholder for active dragged item - keeps droppable
area
// for horizontal drag depth changes while visual is in DragOverlay
if (activeId === item.uuid) {
return (
<DragPlaceholder
id={item.uuid}
style={style}
type={item.type}
isFolder={isFolder}
/>
);
}
// Hide items that don't match search (unless they're folders)
if (!isFolder && searchTerm && !visibleItemIds.has(item.uuid)) {
return null;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the DATASET_FOLDERS feature flag and open the dataset modal
Folders tab (UI
entry described in PR). The UI uses VirtualizedTreeItemComponent at
superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeItem.tsx.
2. Start dragging a metric or column item so that
VirtualizedTreeItemComponent renders the
dragged item; the activeId prop is set to the dragged item's uuid (see code
at line
168-186 in the same file).
3. While the drag is ongoing, type a search that filters out the dragged
item (so
visibleItemIds no longer contains item.uuid). With the existing code (lines
168-186), the
component runs the search-filter check first and returns null, removing the
row and its
droppable placeholder.
4. Observe that the DragPlaceholder (the invisible droppable node created
when activeId
=== item.uuid) is not rendered and horizontal drag-depth adjustments or drop
behavior may
fail or behave inconsistently.
5. The suggested change moves the activeId check before the search filter
(improved code),
ensuring the DragPlaceholder remains present during an active drag even if
the search
would otherwise hide the item.
```
</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/VirtualizedTreeItem.tsx
**Line:** 170:185
**Comment:**
*Logic Error: The search filter check runs before the `activeId`
placeholder check, so if the user starts dragging a metric/column and then
changes the search so that the dragged item no longer matches, the active item
row is short-circuited by the search filter and returns `null`. This removes
the droppable placeholder for the active item during an ongoing drag, which can
break horizontal drag-depth adjustments and drop behavior for that item. Move
the `activeId` check before the search filter so the placeholder is always
rendered for the active item, even if it no longer matches the current search.
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,663 @@
+/**
+ * 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 '@apache-superset/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,
+ MAX_DEPTH,
+} from '../constants';
+import { buildTree, getProjection, serializeForAPI } from '../treeUtils';
+
+interface UseDragHandlersProps {
+ 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({
+ 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);
+ // Track current drop target - use state so virtualized items can render
correctly
+ const [currentDropTargetId, setCurrentDropTargetId] = useState<string |
null>(
+ null,
+ );
+ const [draggedItemIds, setDraggedItemIds] = useState<Set<string>>(new Set());
+
+ // Store the flattened items at drag start to keep them stable during drag
+ // This prevents react-window from re-rendering due to flattenedItems
reference changes
+ const dragStartFlattenedItemsRef = useRef<FlattenedTreeItem[] | null>(null);
+
+ // Compute flattened items, but during drag use the stable snapshot from
drag start
+ // This prevents react-window from re-rendering/re-measuring when
flattenedItems changes
+ const computedFlattenedItems = useMemo(
+ () => computeFlattenedItems(activeId),
+ [computeFlattenedItems, activeId],
+ );
+
+ // Use stable items during drag to prevent scroll jumping
+ // Memoize to avoid creating new array references on every render
+ const flattenedItems = useMemo(
+ () =>
+ activeId && dragStartFlattenedItemsRef.current
+ ? dragStartFlattenedItemsRef.current
+ : computedFlattenedItems,
+ [activeId, computedFlattenedItems],
+ );
+
+ const flattenedItemsIndexMap = useMemo(() => {
+ const map = new Map<string, number>();
+ flattenedItems.forEach((item, index) => {
+ map.set(item.uuid, index);
+ });
+ return map;
+ }, [flattenedItems]);
+
+ // Shared lookup maps for O(1) access - used by handleDragEnd and
forbiddenDropFolderIds
+ const fullItemsByUuid = useMemo(() => {
+ const map = new Map<string, FlattenedTreeItem>();
+ fullFlattenedItems.forEach(item => {
+ map.set(item.uuid, item);
+ });
+ return map;
+ }, [fullFlattenedItems]);
+
+ const fullItemsIndexMap = useMemo(() => {
+ const map = new Map<string, number>();
+ fullFlattenedItems.forEach((item, index) => {
+ map.set(item.uuid, index);
+ });
+ return map;
+ }, [fullFlattenedItems]);
+
+ const childrenByParentId = useMemo(() => {
+ const map = new Map<string, FlattenedTreeItem[]>();
+ fullFlattenedItems.forEach(item => {
+ if (item.parentId) {
+ const children = map.get(item.parentId) ?? [];
+ children.push(item);
+ map.set(item.parentId, children);
+ }
+ });
+ return map;
+ }, [fullFlattenedItems]);
+
+ // Shared helper to calculate max folder descendant depth
+ // Only counts folder depths, not items (columns/metrics)
+ const getMaxFolderDescendantDepth = useCallback(
+ (parentId: string, baseDepth: number): number => {
+ const children = childrenByParentId.get(parentId);
+ if (!children || children.length === 0) {
+ return baseDepth;
+ }
+ let maxDepth = baseDepth;
+ for (const child of children) {
+ if (child.type === FoldersEditorItemType.Folder) {
+ maxDepth = Math.max(maxDepth, child.depth);
+ maxDepth = Math.max(
+ maxDepth,
+ getMaxFolderDescendantDepth(child.uuid, child.depth),
+ );
+ }
+ }
+ return maxDepth;
+ },
+ [childrenByParentId],
+ );
+
+ const resetDragState = useCallback(() => {
+ setActiveId(null);
+ setOverId(null);
+ offsetLeftRef.current = 0;
+ setCurrentDropTargetId(null);
+ setDraggedItemIds(new Set());
+ setDragOverlayWidth(null);
+ // Clear the stable snapshot so next render uses fresh computed items
+ dragStartFlattenedItemsRef.current = null;
+ }, []);
+
+ const handleDragStart = ({ active }: DragStartEvent) => {
+ // Capture the current flattened items BEFORE setting activeId
+ // This ensures the list stays stable during the entire drag operation
+ dragStartFlattenedItemsRef.current = computeFlattenedItems(null);
+
+ 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', '');
+ setCurrentDropTargetId(folderId);
+ return;
+ }
+
+ const projection = getProjection(
+ flattenedItems,
+ activeId,
+ overId,
+ delta.x,
+ DRAG_INDENTATION_WIDTH,
+ flattenedItemsIndexMap,
+ );
+ const newParentId = projection?.parentId ?? null;
+ setCurrentDropTargetId(newParentId);
+ }
+ },
+ [activeId, overId, flattenedItems, flattenedItemsIndexMap],
+ );
+
+ 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', '');
+ setCurrentDropTargetId(folderId);
+ return;
+ }
+
+ const projection = getProjection(
+ flattenedItems,
+ activeId,
+ over.id,
+ offsetLeftRef.current,
+ DRAG_INDENTATION_WIDTH,
+ flattenedItemsIndexMap,
+ );
+ const newParentId = projection?.parentId ?? null;
+ setCurrentDropTargetId(newParentId);
+ } else {
+ setCurrentDropTargetId(null);
+ }
+ },
+ [activeId, flattenedItems, flattenedItemsIndexMap],
+ );
+
+ 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 = fullItemsIndexMap.get(active.id as string) ?? -1;
+ const overIndex = fullItemsIndexMap.get(targetOverId as string) ?? -1;
+
+ if (activeIndex === -1 || overIndex === -1) {
+ return;
+ }
+
+ // Use Set for O(1) lookup instead of Array.includes
+ const itemsBeingDraggedSet = new Set(itemsBeingDragged);
+ const draggedItems = fullFlattenedItems.filter((item: FlattenedTreeItem) =>
+ itemsBeingDraggedSet.has(item.uuid),
+ );
+
+ let projectedPosition = getProjection(
+ flattenedItems,
+ active.id,
+ targetOverId,
+ finalOffsetLeft,
+ DRAG_INDENTATION_WIDTH,
+ flattenedItemsIndexMap,
+ );
+
+ if (isEmptyDrop) {
+ const targetFolder = fullFlattenedItems[overIndex];
+ projectedPosition = {
+ depth: targetFolder.depth + 1,
+ maxDepth: targetFolder.depth + 1,
+ minDepth: targetFolder.depth + 1,
+ parentId: targetOverId as string,
+ };
+ }
+
+ const activeItem = fullFlattenedItems[activeIndex];
+ if (active.id === targetOverId) {
+ const newParentId = projectedPosition?.parentId ?? null;
+ const currentParentId = activeItem.parentId;
+ if (newParentId === currentParentId) {
+ return;
+ }
+ }
+
+ // Single pass to gather info about dragged items
+ let hasNonFolderItems = false;
+ let hasDraggedFolder = false;
+ let hasDraggedDefaultFolder = false;
+ for (const item of draggedItems) {
+ if (item.type === FoldersEditorItemType.Folder) {
+ hasDraggedFolder = true;
+ if (isDefaultFolder(item.uuid)) {
+ hasDraggedDefaultFolder = true;
+ }
+ } else {
+ hasNonFolderItems = true;
+ }
+ }
+
+ if (hasNonFolderItems) {
+ if (!projectedPosition || !projectedPosition.parentId) {
+ return;
+ }
+ }
+
+ if (projectedPosition && projectedPosition.parentId) {
+ const targetFolder = fullItemsByUuid.get(projectedPosition.parentId);
+
+ if (targetFolder && isDefaultFolder(targetFolder.uuid)) {
+ const isDefaultMetricsFolder =
+ targetFolder.uuid === DEFAULT_METRICS_FOLDER_UUID;
+ const isDefaultColumnsFolder =
+ targetFolder.uuid === DEFAULT_COLUMNS_FOLDER_UUID;
+
+ for (const draggedItem of draggedItems) {
+ if (draggedItem.type === FoldersEditorItemType.Folder) {
+ addWarningToast(t('Cannot nest folders in default folders'));
+ return;
+ }
+ if (
+ isDefaultMetricsFolder &&
+ draggedItem.type === FoldersEditorItemType.Column
+ ) {
+ addWarningToast(t('This folder only accepts metrics'));
+ return;
+ }
+ if (
+ isDefaultColumnsFolder &&
+ draggedItem.type === FoldersEditorItemType.Metric
+ ) {
+ addWarningToast(t('This folder only accepts columns'));
+ return;
+ }
+ }
+ }
+ }
+
+ if (hasDraggedDefaultFolder && projectedPosition?.parentId) {
+ addWarningToast(t('Default folders cannot be nested'));
+ return;
+ }
+
+ // Check max depth for folders
+ if (hasDraggedFolder && projectedPosition) {
+ for (const draggedItem of draggedItems) {
+ if (draggedItem.type === FoldersEditorItemType.Folder) {
+ const currentDepth = draggedItem.depth;
+ const maxFolderDescendantDepth = getMaxFolderDescendantDepth(
+ draggedItem.uuid,
+ currentDepth,
+ );
+ const descendantDepthOffset = maxFolderDescendantDepth -
currentDepth;
+ const newDepth = projectedPosition.depth;
+ const newMaxDescendantDepth = newDepth + descendantDepthOffset;
+
+ // MAX_DEPTH is 3, meaning we allow depths 0, 1, 2 (3 levels total)
+ if (newMaxDescendantDepth >= MAX_DEPTH) {
+ addWarningToast(t('Maximum folder nesting depth reached'));
+ return;
+ }
+ }
+ }
+ }
+
+ let newItems = fullFlattenedItems;
+
+ if (projectedPosition) {
+ const depthChange = projectedPosition.depth - activeItem.depth;
+
+ const itemsToUpdate = new Map<
+ string,
+ { depth: number; parentId: string | null | undefined }
+ >();
+
+ draggedItems.forEach((item: FlattenedTreeItem) => {
+ if (item.uuid === active.id) {
+ itemsToUpdate.set(item.uuid, {
+ depth: projectedPosition.depth,
+ parentId: projectedPosition.parentId,
+ });
+ } else {
Review Comment:
**Suggestion:** When dragging multiple items, including a folder and items
(or subfolders) inside that folder, the current logic sets the same parentId
for every dragged item, which flattens the hierarchy by turning children into
siblings of the folder instead of keeping them nested; this will cause folder
contents to "fall out" of their folder after a multi-select drag, contradicting
the expected behavior of moving whole subtrees together. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset Folders editor breaks subtree moves.
- ⚠️ Folder nesting lost after multi-select drag.
- ⚠️ Explore dataset panel shows incorrect folder structure.
```
</details>
```suggestion
// Precompute set of dragged IDs to detect items whose parent is
also being dragged.
const draggedItemIdSet = new Set(draggedItems.map(item =>
item.uuid));
draggedItems.forEach((item: FlattenedTreeItem) => {
// Active item always moves to the projected parent.
if (item.uuid === active.id) {
itemsToUpdate.set(item.uuid, {
depth: projectedPosition.depth,
parentId: projectedPosition.parentId,
});
return;
}
const isChildOfDraggedItem =
item.parentId != null && draggedItemIdSet.has(item.parentId);
// If this item is inside another dragged folder, keep its
parentId so the
// folder hierarchy is preserved and only adjust depth.
if (isChildOfDraggedItem) {
itemsToUpdate.set(item.uuid, {
depth: item.depth + depthChange,
parentId: undefined,
});
} else {
// Standalone dragged items (e.g. metrics or top-level folders)
// should move under the same parent as the active item.
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dataset Folders editor UI and select multiple items that include
a folder and
one or more items contained inside that folder (e.g. select Folder A and
Column X where
Column X is a child of Folder A). This selection triggers the drag flow
implemented in
handleDragStart -> handleDragEnd in useDragHandlers.ts (entry: handleDragEnd
at
superset-frontend/src/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts:235).
2. Begin a multi-select drag and drop operation so handleDragEnd executes.
The function
computes projectedPosition and depthChange, then reaches the itemsToUpdate
population code
at lines
superset-frontend/src/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts:381-397
(the existing_code block).
3. The current logic (existing_code) assigns projectedPosition.parentId to
every dragged
item that is not the active item, regardless of whether that item is
actually a descendant
of another dragged folder. This converts nested children into siblings under
the new
parent (flattening the subtree). Observe resulting newTree created at
buildTree(...) and
applied via setItems/onChange (lines ~531-533).
4. In the UI after drop, the previously nested items will appear as siblings
of the
dragged folder (their nesting is lost). Reproduce by dragging a folder with
nested
subfolders + columns — verify children are no longer nested beneath their
original folder
in the dataset panel in Explore view.
Explanation: The change proposed in improved_code preserves parent-child
relationships
among dragged items by detecting when a dragged item's parent is also
dragged and avoiding
overwriting its parentId, therefore keeping subtrees intact. The current
implementation
(existing_code) flattens nested items because it unconditionally sets
parentId to
projectedPosition.parentId for all dragged items except the active item.
```
</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:** 386:392
**Comment:**
*Logic Error: When dragging multiple items, including a folder and
items (or subfolders) inside that folder, the current logic sets the same
parentId for every dragged item, which flattens the hierarchy by turning
children into siblings of the folder instead of keeping them nested; this will
cause folder contents to "fall out" of their folder after a multi-select drag,
contradicting the expected behavior of moving whole subtrees together.
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/useAutoScroll.ts:
##########
@@ -0,0 +1,191 @@
+/**
+ * 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 { useEffect, useRef } from 'react';
+import type { VariableSizeList as List } from 'react-window';
+
+// Distance from edge where auto-scroll activates (in pixels)
+const SCROLL_THRESHOLD = 80;
+// Scroll speed (pixels per 16ms frame at ~60fps)
+const BASE_SCROLL_SPEED = 8;
+// Maximum scroll speed multiplier when at the very edge
+const MAX_SPEED_MULTIPLIER = 3;
+
+interface UseAutoScrollOptions {
+ listRef: React.RefObject<List>;
+ containerRef: React.RefObject<HTMLDivElement>;
+ isDragging: boolean;
+ listHeight: number;
+}
+
+/**
+ * Custom auto-scroll hook for virtualized lists during drag operations.
+ * This replaces dnd-kit's built-in auto-scroll which conflicts with
virtualization.
+ *
+ * When the user drags near the top or bottom edge of the list container,
+ * this hook smoothly scrolls the react-window list.
+ */
+export function useAutoScroll({
+ listRef,
+ containerRef,
+ isDragging,
+ listHeight,
+}: UseAutoScrollOptions) {
+ // Use refs for all mutable state to avoid re-creating callbacks
+ const scrollStateRef = useRef({
+ direction: null as 'up' | 'down' | null,
+ speed: 0,
+ mouseY: 0,
+ rafId: null as number | null,
+ lastTime: 0,
+ isScrolling: false,
+ });
+
+ useEffect(() => {
+ if (!isDragging) {
+ // Clean up when not dragging
+ const state = scrollStateRef.current;
+ if (state.rafId !== null) {
+ cancelAnimationFrame(state.rafId);
+ state.rafId = null;
+ }
+ state.direction = null;
+ state.speed = 0;
+ return;
+ }
+
+ const state = scrollStateRef.current;
+
+ // Calculate scroll parameters based on mouse position
+ const updateScrollParams = () => {
+ const container = containerRef.current;
+ if (!container) return;
+
+ const containerRect = container.getBoundingClientRect();
+ const relativeY = state.mouseY - containerRect.top;
+
+ // Near top edge - scroll up
+ if (relativeY < SCROLL_THRESHOLD && relativeY >= 0) {
+ const proximity = 1 - relativeY / SCROLL_THRESHOLD;
+ state.direction = 'up';
+ state.speed =
+ BASE_SCROLL_SPEED * (1 + proximity * (MAX_SPEED_MULTIPLIER - 1));
+ return;
+ }
+
+ // Near bottom edge - scroll down
+ if (
+ relativeY > listHeight - SCROLL_THRESHOLD &&
+ relativeY <= listHeight
+ ) {
+ const distanceFromBottom = listHeight - relativeY;
+ const proximity = 1 - distanceFromBottom / SCROLL_THRESHOLD;
+ state.direction = 'down';
+ state.speed =
+ BASE_SCROLL_SPEED * (1 + proximity * (MAX_SPEED_MULTIPLIER - 1));
+ return;
+ }
+
+ // Not in scroll zone
+ state.direction = null;
+ state.speed = 0;
+ };
+
+ // Animation frame callback - uses time-based scrolling for consistent
speed
+ const scrollFrame = (currentTime: number) => {
+ const list = listRef.current;
+ const outerElement = (list as any)?._outerRef;
+
+ if (!list || !outerElement || !state.direction) {
+ // Restore pointer events when scrolling stops
+ const container = containerRef.current;
+ if (container && state.isScrolling) {
+ container.style.pointerEvents = '';
+ state.isScrolling = false;
+ }
+ state.rafId = null;
+ return;
+ }
+
+ // Disable pointer events during scroll to prevent hover recalculations
+ const container = containerRef.current;
+ if (container && !state.isScrolling) {
+ container.style.pointerEvents = 'none';
+ state.isScrolling = true;
+ }
+
+ // Calculate time delta for frame-rate independent scrolling
+ const deltaTime = state.lastTime
+ ? (currentTime - state.lastTime) / 16
+ : 1;
+ state.lastTime = currentTime;
+
+ const currentScroll = outerElement.scrollTop;
+ const maxScroll = outerElement.scrollHeight - outerElement.clientHeight;
+ const scrollAmount = state.speed * deltaTime;
+
+ let newScroll = currentScroll;
+ if (state.direction === 'up') {
+ newScroll = Math.max(0, currentScroll - scrollAmount);
+ } else if (state.direction === 'down') {
+ newScroll = Math.min(maxScroll, currentScroll + scrollAmount);
+ }
+
+ if (Math.abs(newScroll - currentScroll) > 0.5) {
+ // Use direct DOM manipulation for smoother scrolling
+ // react-window's scrollTo triggers re-renders which can cause stutter
+ outerElement.scrollTop = newScroll;
+ }
+
+ // Continue animation loop
+ state.rafId = requestAnimationFrame(scrollFrame);
+ };
+
+ // Handle mouse move - just update position, let animation loop handle
scrolling
+ const handleMouseMove = (event: MouseEvent) => {
+ state.mouseY = event.clientY;
+ updateScrollParams();
Review Comment:
**Suggestion:** The auto-scroll logic only considers the vertical mouse
position, so any drag whose Y coordinate overlaps the list's vertical range
will trigger scrolling even when the pointer is horizontally far away from the
list (e.g., over a different panel), causing unrelated lists to scroll while
dragging; adding a horizontal bounds check around the container prevents this
unintended behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unintended list scrolling during drag across panels.
- ❌ Folders editor drag UX becomes confusing.
- ⚠️ Adjacent panels may appear to move unexpectedly.
```
</details>
```suggestion
const updateScrollParams = (mouseX: number, mouseY: number) => {
const container = containerRef.current;
if (!container) return;
const containerRect = container.getBoundingClientRect();
const { top, bottom, left, right } = containerRect;
// If the cursor is outside the container horizontally or vertically,
// do not auto-scroll this list.
if (
mouseX < left ||
mouseX > right ||
mouseY < top ||
mouseY > bottom
) {
state.direction = null;
state.speed = 0;
return;
}
const relativeY = mouseY - top;
// Near top edge - scroll up
if (relativeY < SCROLL_THRESHOLD && relativeY >= 0) {
const proximity = 1 - relativeY / SCROLL_THRESHOLD;
state.direction = 'up';
state.speed =
BASE_SCROLL_SPEED * (1 + proximity * (MAX_SPEED_MULTIPLIER - 1));
return;
}
// Near bottom edge - scroll down
if (
relativeY > listHeight - SCROLL_THRESHOLD &&
relativeY <= listHeight
) {
const distanceFromBottom = listHeight - relativeY;
const proximity = 1 - distanceFromBottom / SCROLL_THRESHOLD;
state.direction = 'down';
state.speed =
BASE_SCROLL_SPEED * (1 + proximity * (MAX_SPEED_MULTIPLIER - 1));
return;
}
// Not in scroll zone
state.direction = null;
state.speed = 0;
};
// Animation frame callback - uses time-based scrolling for consistent
speed
const scrollFrame = (currentTime: number) => {
const list = listRef.current;
const outerElement = (list as any)?._outerRef;
if (!list || !outerElement || !state.direction) {
// Restore pointer events when scrolling stops
const container = containerRef.current;
if (container && state.isScrolling) {
container.style.pointerEvents = '';
state.isScrolling = false;
}
state.rafId = null;
return;
}
// Disable pointer events during scroll to prevent hover recalculations
const container = containerRef.current;
if (container && !state.isScrolling) {
container.style.pointerEvents = 'none';
state.isScrolling = true;
}
// Calculate time delta for frame-rate independent scrolling
const deltaTime = state.lastTime
? (currentTime - state.lastTime) / 16
: 1;
state.lastTime = currentTime;
const currentScroll = outerElement.scrollTop;
const maxScroll = outerElement.scrollHeight -
outerElement.clientHeight;
const scrollAmount = state.speed * deltaTime;
let newScroll = currentScroll;
if (state.direction === 'up') {
newScroll = Math.max(0, currentScroll - scrollAmount);
} else if (state.direction === 'down') {
newScroll = Math.min(maxScroll, currentScroll + scrollAmount);
}
if (Math.abs(newScroll - currentScroll) > 0.5) {
// Use direct DOM manipulation for smoother scrolling
// react-window's scrollTo triggers re-renders which can cause
stutter
outerElement.scrollTop = newScroll;
}
// Continue animation loop
state.rafId = requestAnimationFrame(scrollFrame);
};
// Handle mouse move - just update position, let animation loop handle
scrolling
const handleMouseMove = (event: MouseEvent) => {
updateScrollParams(event.clientX, event.clientY);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dataset Folders editor which renders VirtualizedTreeList (file:
superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeList.tsx).
The
containerRef is created at VirtualizedTreeList.tsx:83-85 and applied to the
wrapper div at
VirtualizedTreeList.tsx:215.
2. Start dragging an item in the Folders editor so isDragging becomes true
(drag
interactions handled by the Folders editor; VirtualizedTreeList passes
isDragging into
useAutoScroll at VirtualizedTreeList.tsx:86-92).
3. While dragging, move the mouse vertically so its Y coordinate overlaps
the list's
vertical bounds, but keep the pointer horizontally outside the list (for
example, over an
adjacent panel). The current implementation only uses mouseY (see
useAutoScroll.ts lines
75-82 and handleMouseMove at 160-169) so updateScrollParams will treat the
pointer as
"near edge".
4. The hook starts the animation loop (requestAnimationFrame at
useAutoScroll.ts lines
~157-169) and manipulates the list's outerElement.scrollTop
(useAutoScroll.ts lines
139-154), causing the VirtualizedTreeList (the element referenced at
VirtualizedTreeList.tsx:217) to scroll even though the pointer is not above
it
horizontally.
5. Observe unintended scrolling of the list while the pointer is over a
different panel.
This reproduces the behavior described by the suggestion.
Note: The reproduction is based on direct references in the code:
useAutoScroll is
imported and used in VirtualizedTreeList (VirtualizedTreeList.tsx:28 and
86-92),
containerRef is set on the wrapper div (VirtualizedTreeList.tsx:215), and
useAutoScroll
only reads event.clientY into state.mouseY (useAutoScroll.ts lines 161-163).
```
</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/useAutoScroll.ts
**Line:** 76:163
**Comment:**
*Logic Error: The auto-scroll logic only considers the vertical mouse
position, so any drag whose Y coordinate overlaps the list's vertical range
will trigger scrolling even when the pointer is horizontally far away from the
list (e.g., over a different panel), causing unrelated lists to scroll while
dragging; adding a horizontal bounds check around the container prevents this
unintended behavior.
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/index.tsx:
##########
@@ -0,0 +1,467 @@
+/**
+ * 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 { debounce } from 'lodash';
+import AutoSizer from 'react-virtualized-auto-sizer';
+import {
+ DndContext,
+ DragOverlay,
+ PointerSensor,
+ useSensor,
+ useSensors,
+ UniqueIdentifier,
+} from '@dnd-kit/core';
+import {
+ SortableContext,
+ verticalListSortingStrategy,
+} from '@dnd-kit/sortable';
+import { FoldersEditorItemType } from '../types';
+import { TreeItem as TreeItemType } from './constants';
+import {
+ flattenTree,
+ buildTree,
+ removeChildrenOf,
+ serializeForAPI,
+} from './treeUtils';
+import {
+ createFolder,
+ resetToDefault,
+ ensureDefaultFolders,
+ filterItemsBySearch,
+} from './folderOperations';
+import {
+ pointerSensorOptions,
+ measuringConfig,
+ autoScrollConfig,
+} from './sensors';
+import { FoldersContainer, FoldersContent } from './styles';
+import { FoldersEditorProps } from './types';
+import { useToasts } from 'src/components/MessageToasts/withToasts';
+import { useDragHandlers } from './hooks/useDragHandlers';
+import { useItemHeights } from './hooks/useItemHeights';
+import { useHeightCache } from './hooks/useHeightCache';
+import {
+ FoldersToolbarComponent,
+ ResetConfirmModal,
+ DragOverlayContent,
+} from './components';
+import { VirtualizedTreeList } from './VirtualizedTreeList';
+
+export default function FoldersEditor({
+ folders: initialFolders,
+ metrics,
+ columns,
+ onChange,
+}: FoldersEditorProps) {
+ const { addWarningToast } = useToasts();
+ const itemHeights = useItemHeights();
+ const heightCache = useHeightCache();
+
+ const [items, setItems] = useState<TreeItemType[]>(() => {
+ const ensured = ensureDefaultFolders(initialFolders, metrics, columns);
+ return ensured;
+ });
+
+ const [selectedItemIds, setSelectedItemIds] = useState<Set<string>>(
+ new Set(),
+ );
+ const lastSelectedItemIdRef = useRef<string | null>(null);
+ const [searchTerm, setSearchTerm] = useState('');
+ const [collapsedIds, setCollapsedIds] = useState<Set<string>>(new Set());
+ const [editingFolderId, setEditingFolderId] = useState<string | null>(null);
+ const [showResetConfirm, setShowResetConfirm] = useState(false);
+
+ const sensors = useSensors(useSensor(PointerSensor, pointerSensorOptions));
+
+ const fullFlattenedItems = useMemo(() => flattenTree(items), [items]);
+
+ const collapsedFolderIds = useMemo(() => {
+ const result: UniqueIdentifier[] = [];
+ for (const { uuid, type, children } of fullFlattenedItems) {
+ if (
+ type === FoldersEditorItemType.Folder &&
+ collapsedIds.has(uuid) &&
+ children?.length
+ ) {
+ result.push(uuid);
+ }
+ }
+ return result;
+ }, [fullFlattenedItems, collapsedIds]);
+
+ const computeFlattenedItems = useCallback(
+ (activeId: UniqueIdentifier | null) =>
+ removeChildrenOf(
+ fullFlattenedItems,
+ activeId != null
+ ? [activeId, ...collapsedFolderIds]
+ : collapsedFolderIds,
+ ),
+ [fullFlattenedItems, collapsedFolderIds],
+ );
+
+ const visibleItemIds = useMemo(() => {
+ if (!searchTerm) {
+ const allIds = new Set<string>();
+ metrics.forEach(m => allIds.add(m.uuid));
+ columns.forEach(c => allIds.add(c.uuid));
+ return allIds;
+ }
+ const allItems = [...metrics, ...columns];
+ return filterItemsBySearch(searchTerm, allItems);
+ }, [searchTerm, metrics, columns]);
+
+ const metricsMap = useMemo(
+ () => new Map(metrics.map(m => [m.uuid, m])),
+ [metrics],
+ );
+ const columnsMap = useMemo(
+ () => new Map(columns.map(c => [c.uuid, c])),
+ [columns],
+ );
+
+ const {
+ isDragging,
+ activeId,
+ dragOverlayWidth,
+ flattenedItems,
+ dragOverlayItems,
+ forbiddenDropFolderIds,
+ currentDropTargetId,
+ fullItemsByUuid,
+ handleDragStart,
+ handleDragMove,
+ handleDragOver,
+ handleDragEnd,
+ handleDragCancel,
+ } = useDragHandlers({
+ setItems,
+ computeFlattenedItems,
+ fullFlattenedItems,
+ selectedItemIds,
+ onChange,
+ addWarningToast,
+ });
+
+ const debouncedSearch = useCallback(
+ debounce((term: string) => {
+ setSearchTerm(term);
+ }, 300),
+ [],
+ );
+
+ const handleSearch = (e: React.ChangeEvent<HTMLInputElement>) => {
+ debouncedSearch(e.target.value);
+ };
+
+ const handleAddFolder = () => {
+ const newFolder = createFolder('');
+ const updatedItems = [newFolder, ...items];
+ setItems(updatedItems);
+ setEditingFolderId(newFolder.uuid);
+ onChange(serializeForAPI(updatedItems));
+ };
+
+ const allVisibleSelected = useMemo(() => {
+ const selectableItems = Array.from(visibleItemIds).filter(id => {
+ const item = fullItemsByUuid.get(id);
+ return item && item.type !== FoldersEditorItemType.Folder;
+ });
+ return (
+ selectableItems.length > 0 &&
+ selectableItems.every(id => selectedItemIds.has(id))
+ );
+ }, [fullItemsByUuid, visibleItemIds, selectedItemIds]);
+
+ const handleSelectAll = useCallback(() => {
+ const itemsToSelect = new Set(
+ Array.from(visibleItemIds).filter(id => {
+ const item = fullItemsByUuid.get(id);
+ return item && item.type !== FoldersEditorItemType.Folder;
+ }),
+ );
+
+ if (allVisibleSelected) {
+ setSelectedItemIds(new Set());
+ } else {
+ setSelectedItemIds(itemsToSelect);
+ }
+ }, [visibleItemIds, fullItemsByUuid, allVisibleSelected]);
+
+ const handleResetToDefault = () => {
+ setShowResetConfirm(true);
+ };
+
+ const handleConfirmReset = () => {
+ const resetFolders = resetToDefault(metrics, columns);
+ setItems(resetFolders);
+ setSelectedItemIds(new Set());
+ setEditingFolderId(null);
+ setShowResetConfirm(false);
+ onChange(serializeForAPI(resetFolders));
+ };
+
+ const handleCancelReset = () => {
+ setShowResetConfirm(false);
+ };
+
+ const handleToggleCollapse = useCallback((folderId: string) => {
+ setCollapsedIds(prev => {
+ const newSet = new Set(prev);
+ if (newSet.has(folderId)) {
+ newSet.delete(folderId);
+ } else {
+ newSet.add(folderId);
+ }
+ return newSet;
+ });
+ }, []);
+
+ const handleSelect = useCallback(
+ (itemId: string, selected: boolean, shiftKey?: boolean) => {
+ // Capture ref value before setState to avoid timing issues with React
18 batching
+ const lastSelectedId = lastSelectedItemIdRef.current;
+
+ // Update ref immediately for next interaction
+ if (selected) {
+ lastSelectedItemIdRef.current = itemId;
+ }
+
+ setSelectedItemIds(prev => {
+ const newSet = new Set(prev);
+
+ // Range selection when shift is held and we have a previous selection
+ if (shiftKey && selected && lastSelectedId) {
+ const selectableItems = flattenedItems.filter(
+ item => item.type !== FoldersEditorItemType.Folder,
+ );
+
+ const currentIndex = selectableItems.findIndex(
+ item => item.uuid === itemId,
+ );
+ const lastIndex = selectableItems.findIndex(
+ item => item.uuid === lastSelectedId,
+ );
+
+ if (currentIndex !== -1 && lastIndex !== -1) {
+ const startIndex = Math.min(currentIndex, lastIndex);
+ const endIndex = Math.max(currentIndex, lastIndex);
+
+ for (let i = startIndex; i <= endIndex; i += 1) {
+ newSet.add(selectableItems[i].uuid);
+ }
+ }
+ } else if (selected) {
+ newSet.add(itemId);
+ } else {
+ newSet.delete(itemId);
+ }
+
+ return newSet;
+ });
+ },
+ [flattenedItems],
Review Comment:
**Suggestion:** Shift-click range selection currently uses all flattened
items regardless of the active search filter, so range selection can include
items that are not visible in the UI, causing hidden metrics/columns to be
selected and moved unintentionally. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Folder editor selects hidden items during search.
- ❌ Dragging moves invisible metrics/columns unexpectedly.
- ⚠️ Dataset modal Folders tab shows inconsistent selection.
- ⚠️ Subsequent Explore panel folder layout may change.
```
</details>
```suggestion
item =>
item.type !== FoldersEditorItemType.Folder &&
visibleItemIds.has(item.uuid),
);
const currentIndex = selectableItems.findIndex(
item => item.uuid === itemId,
);
const lastIndex = selectableItems.findIndex(
item => item.uuid === lastSelectedId,
);
if (currentIndex !== -1 && lastIndex !== -1) {
const startIndex = Math.min(currentIndex, lastIndex);
const endIndex = Math.max(currentIndex, lastIndex);
for (let i = startIndex; i <= endIndex; i += 1) {
newSet.add(selectableItems[i].uuid);
}
}
} else if (selected) {
newSet.add(itemId);
} else {
newSet.delete(itemId);
}
return newSet;
});
},
[flattenedItems, visibleItemIds],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable DATASET_FOLDERS and open Dataset modal → Folders tab (UI entry
point).
2. In
superset-frontend/src/components/Datasource/FoldersEditor/index.tsx:120-129,
note
visibleItemIds is computed from searchTerm and metrics/columns
(visibleItemIds referenced
at lines ~120-129).
3. Type into the search box (handled by handleSearch at lines 170-172) so
the UI filters
items — many items become hidden in the rendered list.
4. Click one visible non-folder item, then Shift+click another visible
non-folder item to
trigger range selection. The Shift-range logic uses flattenedItems (from
useDragHandlers)
at lines 93 and 140-161 and filters only by type (existing code, lines
247-269 in the
hunk).
5. Because the existing implementation builds the selectableItems array from
flattenedItems without checking visibleItemIds (existing code lines
249-258), the
selection range includes items that match the indices in flattenedItems but
are currently
filtered out by search.
6. Observe more items become selected than those visible in the UI
(selection state
includes hidden UUIDs). Subsequent move/drag operations (DndContext handlers
at lines
410-419 and useDragHandlers) will move hidden items unexpectedly.
7. The improved code (uses visibleItemIds.has(...) and updates dependency to
visibleItemIds at lines ... ) restricts the range to visible items only,
preventing hidden
selection.
```
</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/index.tsx
**Line:** 253:280
**Comment:**
*Logic Error: Shift-click range selection currently uses all flattened
items regardless of the active search filter, so range selection can include
items that are not visible in the UI, causing hidden metrics/columns to be
selected and moved unintentionally.
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/VirtualizedTreeList.tsx:
##########
@@ -0,0 +1,229 @@
+/**
+ * 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, useEffect, useMemo, useRef } from 'react';
+import { VariableSizeList as List } from 'react-window';
+import type { UniqueIdentifier } from '@dnd-kit/core';
+import type { Metric, ColumnMeta } from '@superset-ui/chart-controls';
+import { FoldersEditorItemType } from '../types';
+import type { FlattenedTreeItem } from './constants';
+import type { ItemHeights } from './hooks/useItemHeights';
+import type { HeightCache } from './hooks/useHeightCache';
+import { useAutoScroll } from './hooks/useAutoScroll';
+import {
+ VirtualizedTreeItem,
+ VirtualizedTreeItemData,
+} from './VirtualizedTreeItem';
+
+interface VirtualizedTreeListProps {
+ width: number;
+ height: number;
+ flattenedItems: FlattenedTreeItem[];
+ itemHeights: ItemHeights;
+ heightCache: HeightCache;
+ collapsedIds: Set<string>;
+ selectedItemIds: Set<string>;
+ editingFolderId: string | null;
+ folderChildCounts: Map<string, number>;
+ itemSeparatorInfo: Map<string, 'visible' | 'transparent'>;
+ visibleItemIds: Set<string>;
+ searchTerm: string;
+ metricsMap: Map<string, Metric>;
+ columnsMap: Map<string, ColumnMeta>;
+ isDragging: boolean;
+ activeId: UniqueIdentifier | null;
+ forbiddenDropFolderIds: Set<string>;
+ currentDropTargetId: string | null;
+ onToggleCollapse: (id: string) => void;
+ onSelect: (id: string, selected: boolean, shiftKey?: boolean) => void;
+ onStartEdit: (id: string) => void;
+ onFinishEdit: (id: string, newName: string) => void;
+}
+
+export function VirtualizedTreeList({
+ width,
+ height,
+ flattenedItems,
+ itemHeights,
+ heightCache,
+ collapsedIds,
+ selectedItemIds,
+ editingFolderId,
+ folderChildCounts,
+ itemSeparatorInfo,
+ visibleItemIds,
+ searchTerm,
+ metricsMap,
+ columnsMap,
+ isDragging,
+ activeId,
+ forbiddenDropFolderIds,
+ currentDropTargetId,
+ onToggleCollapse,
+ onSelect,
+ onStartEdit,
+ onFinishEdit,
+}: VirtualizedTreeListProps) {
+ const listRef = useRef<List>(null);
+ const containerRef = useRef<HTMLDivElement>(null);
+
+ // Custom auto-scroll during drag (replaces dnd-kit's auto-scroll which
conflicts with virtualization)
+ useAutoScroll({
+ listRef,
+ containerRef,
+ isDragging,
+ listHeight: height,
+ });
+
+ // Reset list cache when items structure changes, but not during drag
+ // Resetting during drag causes jumping/flickering
+ useEffect(() => {
+ if (!isDragging) {
+ listRef.current?.resetAfterIndex(0);
+ }
+ }, [
+ flattenedItems,
+ collapsedIds,
+ folderChildCounts,
+ itemSeparatorInfo,
+ visibleItemIds,
+ isDragging,
Review Comment:
**Suggestion:** The list height for empty expanded folders uses a cached
measurement from `heightCache`, but the react-window `VariableSizeList` cache
is only reset when `flattenedItems`, `collapsedIds`, `folderChildCounts`,
`itemSeparatorInfo`, or `visibleItemIds` change; if `heightCache` is updated
(for example when the actual empty-folder content is measured), the list will
keep using the old fallback height, causing inconsistent item positioning and
visual glitches because the virtualized row sizes no longer match their
rendered heights. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Visual misalignment in Dataset Folders editor list.
- ⚠️ Scrolling and drag interactions show layout glitches.
- ⚠️ Affects empty-folder rendering measured via heightCache.
- ⚠️ Occurs in folder editor used inside dataset modal.
```
</details>
```suggestion
heightCache,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dataset Folders editor (component at
superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeList.tsx).
The
list reset effect is defined at lines 94-107 and does not depend on
heightCache.
2. Create or open a folder that is empty and expanded. The empty-folder
height is read
from heightCache via heightCache.getHeight(item.uuid) at lines 139-141 of
the same file
(see totalHeight calculation).
3. When the empty folder's actual rendered empty-state is measured elsewhere
(the
measurement logic updates heightCache with the real height), heightCache
changes but the
useEffect that calls listRef.current?.resetAfterIndex(0) will not run
because heightCache
is not listed in the dependency array (lines 96-106).
4. As a result, react-window's internal size cache (VariableSizeList) keeps
the old size
for that row while the DOM now has a different rendered height (measured
height), causing
visual misalignment and possible jump/glitch when scrolling or interacting
with the list.
5. Reproducing locally: (a) enable DATASET_FOLDERS flag; (b) open dataset
modal → Folders
tab; (c) create an empty folder, expand it so measurement code stores a
height into
heightCache (measurement happens in the empty-folder render path); (d)
observe that the
visual row height differs from the virtualized slot until an action that
resets the
react-window cache occurs (e.g., toggle collapse or restructure items). This
demonstrates
heightCache updates are not triggering a List reset.
6. Explanation why change is needed: the code reads from heightCache at
lines ~139-141 but
the reset effect (lines 94-107) only listens to flattenedItems, collapsedIds,
folderChildCounts, itemSeparatorInfo, visibleItemIds, and isDragging — so it
won't react
when cached measurements change. Adding heightCache to dependencies ensures
resetAfterIndex runs when measurements update.
```
</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/VirtualizedTreeList.tsx
**Line:** 106:106
**Comment:**
*Possible Bug: The list height for empty expanded folders uses a cached
measurement from `heightCache`, but the react-window `VariableSizeList` cache
is only reset when `flattenedItems`, `collapsedIds`, `folderChildCounts`,
`itemSeparatorInfo`, or `visibleItemIds` change; if `heightCache` is updated
(for example when the actual empty-folder content is measured), the list will
keep using the old fallback height, causing inconsistent item positioning and
visual glitches because the virtualized row sizes no longer match their
rendered heights.
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/explore/components/controls/MetricControl/FilterDefinitionOption.tsx:
##########
@@ -62,7 +62,7 @@ export default function FilterDefinitionOption({
if (option.label) {
return (
<StyledColumnOption
- column={{ column_name: option.label, type: 'expression' }}
+ column={{ column_name: option.label, type: 'metric' }}
Review Comment:
**Suggestion:** Similarly, for adhoc metrics rendered via the `label`
branch, setting `type: 'metric'` on the column-like object conflates
metric/column classification with the column data type field, which downstream
components (like `ColumnOption`) typically use for physical type information
and UI badges. This can result in inconsistent or incorrect UI rendering for
adhoc metrics, so the synthetic object should omit the `type` override. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Adhoc metric UI shows missing type badge.
- ⚠️ Tooltip about column type becomes inaccurate.
- ⚠️ Metric editing popovers may display inconsistent labels.
```
</details>
```suggestion
column={{ column_name: option.label }}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render FilterDefinitionOption with an adhoc metric option: open
`superset-frontend/src/explore/components/controls/MetricControl/FilterDefinitionOption.tsx`
and trigger the `if (option.label)` branch (file lines 62-68). The component
returns JSX
at lines 64-66 creating a synthetic column object with `type: 'metric'`.
2. That JSX passes the synthetic object to `StyledColumnOption`, which is
rendered by
ColumnOption
(`packages/superset-ui-chart-controls/src/components/ColumnOption.tsx`).
ColumnOption uses `type_generic` to derive the displayed type and uses
`column.type` for
tooltips (see ColumnOption lines 55-63 and 70-83).
3. Because the synthetic object supplies `type: 'metric'` but not
`type_generic` or other
expected column metadata, ColumnOption's logic will not correctly display a
physical type
label and may display unexpected tooltip content or no badge for adhoc
metrics. This
behavior can be observed by opening the metric picker/popover that renders
adhoc metrics
and inspecting the type badge next to labels.
4. Reproduce in UI: open metric creation/edit UI or any control that lists
adhoc metrics
(the MetricControl/AdhocMetric edit popover paths reference
StyledColumnOption usage—see
grep hits). Add or view an adhoc metric with a `label` value so the `label`
branch runs;
observe the ColumnTypeLabel or tooltip behavior next to the adhoc metric
label. The
incorrect or missing type label is caused by misusing `type` for
classification rather
than providing the column metadata ColumnOption expects.
Note: Steps reference concrete files and lines discovered with Grep/Read:
FilterDefinitionOption branches (file/lines) and ColumnOption implementation
(file/lines).
```
</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/controls/MetricControl/FilterDefinitionOption.tsx
**Line:** 65:65
**Comment:**
*Logic Error: Similarly, for adhoc metrics rendered via the `label`
branch, setting `type: 'metric'` on the column-like object conflates
metric/column classification with the column data type field, which downstream
components (like `ColumnOption`) typically use for physical type information
and UI badges. This can result in inconsistent or incorrect UI rendering for
adhoc metrics, so the synthetic object should omit the `type` override.
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/explore/components/controls/MetricControl/FilterDefinitionOption.tsx:
##########
@@ -46,7 +46,7 @@ export default function FilterDefinitionOption({
if (option.saved_metric_name) {
return (
<StyledColumnOption
- column={{ column_name: option.saved_metric_name, type: 'expression' }}
+ column={{ column_name: option.saved_metric_name, type: 'metric' }}
Review Comment:
**Suggestion:** The `type` property on the synthetic column object for saved
metrics is being set to the string `'metric'`, but `type` in column metadata is
generally used for the underlying data type (e.g., VARCHAR, numeric).
Overloading it with `'metric'` can confuse consumers like `ColumnOption` that
rely on this field for type badges or behavior, leading to inconsistent or
incorrect rendering for saved metrics. Instead, keep the minimal shape needed
for display and omit the `type` override. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Metric selection UI shows incorrect type badge.
- ⚠️ Column type tooltip content may be missing.
- ⚠️ FilterDefinitionOption affects MetricControl metric list.
```
</details>
```suggestion
column={{ column_name: option.saved_metric_name }}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render FilterDefinitionOption with a saved metric option: open the code at
`superset-frontend/src/explore/components/controls/MetricControl/FilterDefinitionOption.tsx`
and trigger the branch guarded by `if (option.saved_metric_name)` (see file
lines 46-52).
The component returns the exact JSX at lines 48-51 which creates a synthetic
column object
with `type: 'metric'`.
2. That JSX passes the synthetic object to `StyledColumnOption` (same JSX).
`StyledColumnOption` delegates to the ColumnOption component implemented at
`superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnOption.tsx`
(see read result). ColumnOption expects column metadata with fields like
`type_generic`
and `type` (see ColumnOption lines 55-63 and 72-83).
3. ColumnOption computes the displayed type label via `const type =
hasExpression ?
'expression' : type_generic;` and uses `showType` to render a
ColumnTypeLabel using `type`
(ColumnOption lines 55 and 72-81). The synthetic object only supplies `type:
'metric'` and
omits `type_generic`, so ColumnOption will treat `type_generic` as undefined
and derive
`type` incorrectly (no physical type), potentially causing the type label or
tooltip to be
missing or inconsistent for saved metrics.
4. Reproduce in UI: open any UI that uses FilterDefinitionOption (metric
selection/filter
UI). Select or list saved metrics so the saved-metric branch is rendered;
inspect the type
badge shown next to the metric name. Expect missing/incorrect type badge or
tooltip text
because ColumnOption relies on `type_generic`/`type` semantics found in the
code at
`packages/superset-ui-chart-controls/src/components/ColumnOption.tsx`.
Note: This reproduction is based on concrete code paths: the branch in
FilterDefinitionOption (file/lines cited) and ColumnOption implementation
(file/lines
cited). The current code creates a synthetic object with `type: 'metric'`
which does not
match ColumnOption's expected `type_generic` usage.
```
</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/controls/MetricControl/FilterDefinitionOption.tsx
**Line:** 49:49
**Comment:**
*Logic Error: The `type` property on the synthetic column object for
saved metrics is being set to the string `'metric'`, but `type` in column
metadata is generally used for the underlying data type (e.g., VARCHAR,
numeric). Overloading it with `'metric'` can confuse consumers like
`ColumnOption` that rely on this field for type badges or behavior, leading to
inconsistent or incorrect rendering for saved metrics. Instead, keep the
minimal shape needed for display and omit the `type` override.
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,214 @@
+/**
+ * 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: 0 ${theme.marginMD}px;
+ margin-left: ${isOverlay ? ITEM_INDENTATION_WIDTH : (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;' : ''}
+ `}
Review Comment:
**Suggestion:** The `margin-left` calculation in the base tree item
container assumes `depth` is at least 1, so when a non-folder item has `depth`
0 (which is allowed by the tree utilities for root-level items) it produces a
negative margin and shifts the item off-screen, making it effectively unusable;
clamping the effective depth to a minimum of 1 before computing `margin-left`
prevents this. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Root-level item layout shifted off-screen.
- ⚠️ Dataset Folders editor item selection impaired.
- ⚠️ Drag preview coordinates may appear incorrect.
```
</details>
```suggestion
${({ theme, depth, isDragging, isOverlay }) => {
const effectiveDepth = Math.max(depth, 1);
const marginLeft = isOverlay
? ITEM_INDENTATION_WIDTH
: (effectiveDepth - 1) * FOLDER_INDENTATION_WIDTH +
ITEM_INDENTATION_WIDTH;
return `
margin: 0 ${theme.marginMD}px;
margin-left: ${marginLeft}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;' : ''}
`;
}}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dataset Folders editor UI (feature flag DATASET_FOLDERS enabled)
so the
component tree rendering includes TreeItem (file:
superset-frontend/src/components/Datasource/FoldersEditor/TreeItem.tsx). The
TreeItem
component receives a numeric depth prop (see TreeItemProps depth at
TreeItem.tsx:68-69).
2. Render a root-level non-folder item that is passed depth = 0 (TreeItem is
used for
non-folder items in the non-folder branch at TreeItem.tsx:357-361 where
<TreeItemContainer
{...restContainerProps}> is returned). The container props include depth (see
containerProps assignment at TreeItem.tsx:252-259).
3. With depth = 0 the current style function in TreeItem.styles.ts builds
margin-left
using (depth - 1) * FOLDER_INDENTATION_WIDTH + ITEM_INDENTATION_WIDTH
(TreeItem.styles.ts:31-34). Substituting depth = 0 yields a negative
margin-left value ((0
- 1) * 24 + 4 = -20), shifting the item left off-screen.
4. Observe the rendered non-folder item visually misaligned (partially/fully
off-screen)
in the dataset Folders editor; verify by inspecting the element's computed
style in
browser devtools—computed margin-left will be negative (file:
superset-frontend/src/components/Datasource/FoldersEditor/TreeItem.styles.ts
lines 31-34).
Explanation: TreeItem.props.depth is a plain number and can be 0 for
root-level items
(TreeItem.tsx declares depth: number at line 68). The existing code assumes
depth >= 1 and
therefore produces negative margins for depth 0.
```
</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:** 31:41
**Comment:**
*Logic Error: The `margin-left` calculation in the base tree item
container assumes `depth` is at least 1, so when a non-folder item has `depth`
0 (which is allowed by the tree utilities for root-level items) it produces a
negative margin and shifts the item off-screen, making it effectively unusable;
clamping the effective depth to a minimum of 1 before computing `margin-left`
prevents this.
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]
