codeant-ai-for-open-source[bot] commented on code in PR #36239:
URL: https://github.com/apache/superset/pull/36239#discussion_r2741259079
##########
superset/commands/dataset/update.py:
##########
@@ -212,27 +212,26 @@ def _validate_semantics(self, exceptions:
list[ValidationError]) -> None:
valid_uuids: set[UUID] = set()
if metrics:
valid_uuids.update(
- UUID(metric["uuid"]) for metric in metrics if "uuid" in
metric
+ metric["uuid"] for metric in metrics if "uuid" in metric
)
else:
valid_uuids.update(metric.uuid for metric in
self._model.metrics)
if columns:
valid_uuids.update(
- UUID(column["uuid"]) for column in columns if "uuid" in
column
+ column["uuid"] for column in columns if "uuid" in column
Review Comment:
**Suggestion:** Similarly to metrics, if the request explicitly provides an
empty columns list (removing all columns), `valid_uuids` is still built from
the existing model columns because the code tests `if columns` instead of
checking for the presence of the `columns` key, so folders can keep referencing
columns that are being removed without triggering a validation error. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset update API accepts removed-column references.
- ⚠️ Folder integrity checks incorrectly pass invalid columns.
```
</details>
```suggestion
if "columns" in self._properties:
columns_prop = self._properties.get("columns") or []
valid_uuids.update(
column["uuid"] for column in columns_prop if "uuid" in
column
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call PUT /dataset/<pk> handled by superset/datasets/api.py at lines
386-444. The
request body is parsed at datasets/api.py:438 (item =
self.edit_model_schema.load(request.json)) and passed to
UpdateDatasetCommand at
datasets/api.py:443.
2. Provide a payload that explicitly sets "columns": [] and includes a
folders structure
referencing a column UUID that is intended to be removed, e.g. {"columns":
[], "folders":
[{"uuid": "<column-uuid>", "children": []}]}.
3. Inside UpdateDatasetCommand.validate()
(superset/commands/dataset/update.py),
_validate_semantics() is invoked (update.py:202). The code checks columns
with "if
columns:" at update.py:220 and will treat an explicit empty list as falsy.
4. Because an explicit empty list is falsy, the else branch at
update.py:224-225 runs and
populates valid_uuids from self._model.columns, allowing folders to
reference column UUIDs
slated for removal without validation failure.
(Explanation: the current code uses truthiness at update.py:220-225; the
proposed change
checks for presence of the "columns" key so empty-but-present lists are
handled
correctly.)
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/update.py
**Line:** 220:222
**Comment:**
*Logic Error: Similarly to metrics, if the request explicitly provides
an empty columns list (removing all columns), `valid_uuids` is still built from
the existing model columns because the code tests `if columns` instead of
checking for the presence of the `columns` key, so folders can keep referencing
columns that are being removed without triggering a validation error.
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,631 @@
+/**
+ * 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);
+ // Use ref for projectedParentId to avoid re-renders during drag
+ const projectedParentIdRef = useRef<string | null>(null);
+ const [draggedItemIds, setDraggedItemIds] = useState<Set<string>>(new Set());
+
+ const rafIdRef = useRef<number | null>(null);
+
+ // 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);
+
+ // Update visual drop target indicators via DOM manipulation to avoid
re-renders
+ const updateDropTargetVisuals = useCallback(
+ (newParentId: string | null) => {
+ const oldParentId = projectedParentIdRef.current;
+ if (oldParentId === newParentId) return;
+
+ // Remove highlight from old target
+ if (oldParentId) {
+ const oldElement = document.querySelector(
+ `[data-folder-id="${oldParentId}"]`,
+ );
+ if (oldElement) {
+ oldElement.removeAttribute('data-drop-target');
+ oldElement.removeAttribute('data-forbidden-drop');
+ }
+ }
+
+ // Add highlight to new target
+ if (newParentId) {
+ const newElement = document.querySelector(
+ `[data-folder-id="${newParentId}"]`,
+ );
+ if (newElement) {
+ newElement.setAttribute('data-drop-target', 'true');
+ // Check if it's a forbidden drop target
+ const draggedTypes = new Set<FoldersEditorItemType>();
+ draggedItemIds.forEach((id: string) => {
+ const item = fullFlattenedItems.find(i => i.uuid === id);
+ if (item) draggedTypes.add(item.type);
+ });
+
+ const isForbidden =
+ (newParentId === DEFAULT_COLUMNS_FOLDER_UUID &&
+ draggedTypes.has(FoldersEditorItemType.Metric)) ||
+ (newParentId === DEFAULT_METRICS_FOLDER_UUID &&
+ draggedTypes.has(FoldersEditorItemType.Column)) ||
+ ((newParentId === DEFAULT_COLUMNS_FOLDER_UUID ||
+ newParentId === DEFAULT_METRICS_FOLDER_UUID) &&
+ draggedTypes.has(FoldersEditorItemType.Folder));
+
+ if (isForbidden) {
+ newElement.setAttribute('data-forbidden-drop', 'true');
+ }
+ }
+ }
+
+ projectedParentIdRef.current = newParentId;
+ },
+ [draggedItemIds, fullFlattenedItems],
+ );
+
+ const scheduleProjectedParentUpdate = useCallback(
+ (newParentId: string | null) => {
+ // Only schedule if not already scheduled
+ if (rafIdRef.current === null) {
+ rafIdRef.current = requestAnimationFrame(() => {
+ rafIdRef.current = null;
+ updateDropTargetVisuals(newParentId);
+ });
+ } else {
+ // Update pending value for next frame
+ projectedParentIdRef.current = newParentId;
+ }
+ },
+ [updateDropTargetVisuals],
+ );
+
+ // 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]);
+
+ const resetDragState = useCallback(() => {
+ setActiveId(null);
+ setOverId(null);
+ offsetLeftRef.current = 0;
+ // Clear visual indicators
+ updateDropTargetVisuals(null);
+ setDraggedItemIds(new Set());
+ setDragOverlayWidth(null);
+ // Clear the stable snapshot so next render uses fresh computed items
+ dragStartFlattenedItemsRef.current = null;
+ if (rafIdRef.current !== null) {
+ cancelAnimationFrame(rafIdRef.current);
+ rafIdRef.current = null;
+ }
+ }, [updateDropTargetVisuals]);
+
+ 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);
Review Comment:
**Suggestion:** The stable `flattenedItems` snapshot used during drag is
captured with `computeFlattenedItems(null)`, so it still includes descendants
of the active folder; this allows a folder to be dropped into one of its own
descendants, creating a parent–child cycle (folder becomes child of its child)
so the affected subtree will effectively disappear from the rebuilt tree
structure. Capture the snapshot with the current active id so descendants are
excluded, matching the intended contract of `computeFlattenedItems` and
preventing self-nesting. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset folders drag-and-drop creates invalid cycles.
- ⚠️ Dataset modal folder structure becomes inconsistent.
- ⚠️ Explore panel shows missing folder subtree entries.
```
</details>
```suggestion
// Use the active id so descendants are excluded and cycles are prevented
dragStartFlattenedItemsRef.current = computeFlattenedItems(active.id);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open dataset folders editor UI that uses this hook.
2. Start dragging a folder item. In file
superset-frontend/src/components/Datasource/FoldersEditor/hooks/useDragHandlers.ts
at
lines 183-200 the handler captures a flattened snapshot with
computeFlattenedItems(null).
3. Because computeFlattenedItems(null) includes descendants of the active
folder, the
flattened snapshot still contains the active folder's subtree (verify via
computeFlattenedItems implementation called from this hook).
4. While dragging, drop the active folder onto one of its descendant folders
(the UI
allows hovering over descendants because they exist in the snapshot).
5. On drop, handleDragEnd (lines 267-535) uses fullFlattenedItems and the
projectedPosition computed from the (incorrect) snapshot; the active folder
can become
child of its own descendant, producing a parent-child cycle which then
causes the subtree
to be lost when buildTree(sortedItems) is called (lines 532-534), observable
as missing
metrics/columns in the dataset panel.
6. Expected: computeFlattenedItems(active.id) would exclude descendants
preventing
self-nesting; the suggested change replaces computeFlattenedItems(null) with
computeFlattenedItems(active.id) at the capture point (line 186).
```
</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:** 185:186
**Comment:**
*Logic Error: The stable `flattenedItems` snapshot used during drag is
captured with `computeFlattenedItems(null)`, so it still includes descendants
of the active folder; this allows a folder to be dropped into one of its own
descendants, creating a parent–child cycle (folder becomes child of its child)
so the affected subtree will effectively disappear from the rebuilt tree
structure. Capture the snapshot with the current active id so descendants are
excluded, matching the intended contract of `computeFlattenedItems` and
preventing self-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/commands/dataset/update.py:
##########
@@ -212,27 +212,26 @@ def _validate_semantics(self, exceptions:
list[ValidationError]) -> None:
valid_uuids: set[UUID] = set()
if metrics:
valid_uuids.update(
- UUID(metric["uuid"]) for metric in metrics if "uuid" in
metric
+ metric["uuid"] for metric in metrics if "uuid" in metric
Review Comment:
**Suggestion:** When the request explicitly provides an empty metrics list
(meaning all metrics are being removed), `valid_uuids` is incorrectly populated
from the existing model metrics because the code uses the truthiness of
`metrics` instead of the presence of the `metrics` key; this allows folders to
reference metrics that are about to be deleted without raising a validation
error. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset update API accepts removed-metric references.
- ⚠️ Folders may reference deleted metrics silently.
```
</details>
```suggestion
if "metrics" in self._properties:
metrics_prop = self._properties.get("metrics") or []
valid_uuids.update(
metric["uuid"] for metric in metrics_prop if "uuid" in
metric
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send a PUT request to update a dataset via API endpoint handled at
superset/datasets/api.py:386-444 — specifically the PUT endpoint loads
payload at lines
438-444 and calls UpdateDatasetCommand(pk, item, override_columns).run() at
line 443.
2. Include in the JSON body an explicit empty metrics list and folders that
reference a
metric UUID being removed, e.g. {"metrics": [], "folders": [{"uuid":
"<metric-uuid>",
"children": []}]}. The edit payload is validated by DatasetPutSchema in
datasets/api.py:438 (item = self.edit_model_schema.load(request.json)).
3. Execution enters UpdateDatasetCommand.validate() in
superset/commands/dataset/update.py
(called from UpdateDatasetCommand.run()). The code reaches
_validate_semantics() at
update.py:202-212 and binds metrics with "if metrics :=
self._properties.get(\"metrics\"):" (update.py:208).
4. Because the code uses truthiness (if metrics) rather than presence, an
explicit empty
list (metrics = []) is falsy and the else branch at update.py:217-218 runs,
adding
existing model metric UUIDs (self._model.metrics) into valid_uuids. As a
result, folders
referencing metric UUIDs that the request intends to remove pass validation
and are not
caught.
(Explanation: the current pattern checks truthiness at update.py:213-218;
the suggestion
changes the check to detect an explicit "metrics" key so empty lists are
honored.)
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/update.py
**Line:** 213:215
**Comment:**
*Logic Error: When the request explicitly provides an empty metrics
list (meaning all metrics are being removed), `valid_uuids` is incorrectly
populated from the existing model metrics because the code uses the truthiness
of `metrics` instead of the presence of the `metrics` key; this allows folders
to reference metrics that are about to be deleted without raising a validation
error.
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,429 @@
+/**
+ * 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, 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,
+ getChildCount,
+ 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 [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(
+ () =>
+ fullFlattenedItems.reduce<UniqueIdentifier[]>(
+ (acc, { uuid, type, children }) => {
+ if (
+ type === FoldersEditorItemType.Folder &&
+ collapsedIds.has(uuid) &&
+ children?.length
+ ) {
+ return [...acc, uuid];
+ }
+ return acc;
+ },
+ [],
+ ),
+ [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,
+ draggedItemIds,
+ dragOverlayWidth,
+ flattenedItems,
+ dragOverlayItems,
+ forbiddenDropFolderIds,
+ handleDragStart,
+ handleDragMove,
+ handleDragOver,
+ handleDragEnd,
+ handleDragCancel,
+ } = useDragHandlers({
+ items,
+ 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);
Review Comment:
**Suggestion:** The debounced search function can continue to call
`setSearchTerm` after the component has unmounted, because the debounced
callback is never canceled; this can trigger React warnings about state updates
on unmounted components and cause subtle memory leaks. A simple and robust fix
is to remove the debounce here and update `searchTerm` directly in the change
handler. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Console shows React setState-on-unmounted warnings.
- ⚠️ No direct user-facing search failure in dataset modal.
- ⚠️ Developer debugging noise when opening/closing modal quickly.
```
</details>
```suggestion
const handleSearch = (e: React.ChangeEvent<HTMLInputElement>) => {
setSearchTerm(e.target.value);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Dataset Folders editor UI (component at
superset-frontend/src/components/Datasource/FoldersEditor/index.tsx). The
debounced search
is defined at lines 167-171 and called from handleSearch at lines 174-176.
2. Focus the search input in the Folders tab and type text, which triggers
handleSearch ->
debouncedSearch (call path: handleSearch at line 174 calls debouncedSearch
at line 175).
3. Immediately navigate away or close the dataset modal (unmounting
FoldersEditor) within
the debounce window (300ms). The component unmounts but the lodash debounced
callback is
still scheduled.
4. When the debounced callback runs after 300ms, it calls setSearchTerm at
line 169 on an
unmounted component. React logs a "setState on an unmounted component"
warning in the
console and may retain references until GC, causing a small memory leak and
noisy logs.
5. Expected outcome: React warning appears in console and developer
experience is
degraded; no user-visible functional break, but repeated occurrences during
quick
open/close can clutter logs.
Explanation: The debounced function returned by debounce(...) is never
canceled on unmount
(no cleanup using the debounced function's .cancel()). Therefore scheduled
callbacks can
run after the component has been removed. The code paths and line references
above were
taken from the PR file
superset-frontend/src/components/Datasource/FoldersEditor/index.tsx
(debouncedSearch: lines 167-171; handleSearch call: lines 174-176).
```
</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:** 167:175
**Comment:**
*Possible Bug: The debounced search function can continue to call
`setSearchTerm` after the component has unmounted, because the debounced
callback is never canceled; this can trigger React warnings about state updates
on unmounted components and cause subtle memory leaks. A simple and robust fix
is to remove the debounce here and update `searchTerm` directly in the change
handler.
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,195 @@
+/**
+ * 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 ${isOverlay ? 0 : theme.marginMD}px;
+ margin-left: ${isOverlay ? 0 : (depth - 1) * FOLDER_INDENTATION_WIDTH +
ITEM_INDENTATION_WIDTH}px;
Review Comment:
**Suggestion:** The left margin for non-folder items is calculated as
`(depth - 1) * FOLDER_INDENTATION_WIDTH + ITEM_INDENTATION_WIDTH`, which
becomes negative when `depth` is 0 (root-level items), causing those items to
render shifted to the left outside their intended container and potentially
misaligning hitboxes and layout; clamping the `(depth - 1)` term to a minimum
of 0 avoids negative indentation while preserving the intended relative
indentation for deeper items. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset folders UI layout misaligned for root items.
- ⚠️ Hitbox misalignment for folder/item interactions.
- ⚠️ Affects TreeItem rendering in Dataset modal.
```
</details>
```suggestion
margin-left: ${
isOverlay
? 0
: Math.max(depth - 1, 0) * FOLDER_INDENTATION_WIDTH +
ITEM_INDENTATION_WIDTH
}px;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the frontend with the DATASET_FOLDERS feature enabled and open the
dataset modal
(per PR testing instructions).
2. The dataset folders UI renders TreeItem components. The file that renders
the styled
container is
superset-frontend/src/components/Datasource/FoldersEditor/TreeItem.tsx
which imports
TreeItemContainer
from TreeItem.styles.ts (import at TreeItem.tsx:45 — verified via Grep
output).
3. At runtime TreeItem.tsx renders <TreeItemContainer {...containerProps}>
(usage at
TreeItem.tsx:341 as shown by Grep).
The container is supplied a numeric depth prop (prop name verified by the
styled
definition at
superset-frontend/src/components/Datasource/FoldersEditor/TreeItem.styles.ts:25-30).
4. When a root-level item is rendered with depth = 0, the current expression
in
TreeItem.styles.ts line 33:
(depth - 1) * FOLDER_INDENTATION_WIDTH + ITEM_INDENTATION_WIDTH evaluates
to -1 * 24 +
4 = -20,
producing margin-left: -20px. This causes the element to shift left out
of its intended
container and
may misalign hitboxes and visible layout in the dataset folders UI.
5. Reproduce locally: open Dataset modal → Folders tab → ensure there are
root-level items
(common for top-level metrics/columns).
Observe using browser devtools that the computed margin-left for
TreeItemContainer (see
element styles)
is negative when depth is 0 (inspect element shows margin-left: -20px).
6. The suggested improved code clamps (depth - 1) to minimum 0 via
Math.max(depth - 1, 0)
so depth=0 yields margin-left: ITEM_INDENTATION_WIDTH (4px),
preventing negative indentation and aligning root items correctly.
```
</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 left margin for non-folder items is calculated as
`(depth - 1) * FOLDER_INDENTATION_WIDTH + ITEM_INDENTATION_WIDTH`, which
becomes negative when `depth` is 0 (root-level items), causing those items to
render shifted to the left outside their intended container and potentially
misaligning hitboxes and layout; clamping the `(depth - 1)` term to a minimum
of 0 avoids negative indentation while preserving the intended relative
indentation for deeper 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,631 @@
+/**
+ * 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);
+ // Use ref for projectedParentId to avoid re-renders during drag
+ const projectedParentIdRef = useRef<string | null>(null);
+ const [draggedItemIds, setDraggedItemIds] = useState<Set<string>>(new Set());
+
+ const rafIdRef = useRef<number | null>(null);
+
+ // 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);
+
+ // Update visual drop target indicators via DOM manipulation to avoid
re-renders
+ const updateDropTargetVisuals = useCallback(
+ (newParentId: string | null) => {
+ const oldParentId = projectedParentIdRef.current;
+ if (oldParentId === newParentId) return;
+
+ // Remove highlight from old target
+ if (oldParentId) {
+ const oldElement = document.querySelector(
+ `[data-folder-id="${oldParentId}"]`,
+ );
+ if (oldElement) {
+ oldElement.removeAttribute('data-drop-target');
+ oldElement.removeAttribute('data-forbidden-drop');
+ }
+ }
+
+ // Add highlight to new target
+ if (newParentId) {
+ const newElement = document.querySelector(
+ `[data-folder-id="${newParentId}"]`,
+ );
+ if (newElement) {
+ newElement.setAttribute('data-drop-target', 'true');
+ // Check if it's a forbidden drop target
+ const draggedTypes = new Set<FoldersEditorItemType>();
+ draggedItemIds.forEach((id: string) => {
+ const item = fullFlattenedItems.find(i => i.uuid === id);
+ if (item) draggedTypes.add(item.type);
+ });
+
+ const isForbidden =
+ (newParentId === DEFAULT_COLUMNS_FOLDER_UUID &&
+ draggedTypes.has(FoldersEditorItemType.Metric)) ||
+ (newParentId === DEFAULT_METRICS_FOLDER_UUID &&
+ draggedTypes.has(FoldersEditorItemType.Column)) ||
+ ((newParentId === DEFAULT_COLUMNS_FOLDER_UUID ||
+ newParentId === DEFAULT_METRICS_FOLDER_UUID) &&
+ draggedTypes.has(FoldersEditorItemType.Folder));
Review Comment:
**Suggestion:** The forbidden-drop calculation in `updateDropTargetVisuals`
only enforces type rules for default metric/column folders, but it ignores the
rule that default folders themselves cannot be nested into non-default folders;
this leads to inconsistent UX where a non-default folder is visually
highlighted as a valid drop target when dragging a default folder even though
`handleDragEnd` will reject the drop with a warning. Align the forbidden-drop
logic here with the one in `forbiddenDropFolderIds` so any folder that will be
rejected on drop is also marked as a forbidden drop target visually. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Visual target highlights contradict drop validation.
- ❌ Users see allowed highlight but drop is rejected.
- ⚠️ UX inconsistency in dataset folders editor.
```
</details>
```suggestion
let hasDraggedDefaultFolder = false;
draggedItemIds.forEach((id: string) => {
const item = fullFlattenedItems.find(i => i.uuid === id);
if (item) {
draggedTypes.add(item.type);
if (
item.type === FoldersEditorItemType.Folder &&
isDefaultFolder(item.uuid)
) {
hasDraggedDefaultFolder = true;
}
}
});
const targetIsDefaultFolder = isDefaultFolder(newParentId);
const isDefaultMetricsFolder =
newParentId === DEFAULT_METRICS_FOLDER_UUID &&
targetIsDefaultFolder;
const isDefaultColumnsFolder =
newParentId === DEFAULT_COLUMNS_FOLDER_UUID &&
targetIsDefaultFolder;
const isForbidden =
// Default folders cannot be nested into non-default folders
(hasDraggedDefaultFolder && !targetIsDefaultFolder) ||
// Default folders cannot contain other folders
((isDefaultMetricsFolder || isDefaultColumnsFolder) &&
draggedTypes.has(FoldersEditorItemType.Folder)) ||
// Metrics/columns type mismatches for default folders
(isDefaultMetricsFolder &&
draggedTypes.has(FoldersEditorItemType.Column)) ||
(isDefaultColumnsFolder &&
draggedTypes.has(FoldersEditorItemType.Metric));
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open dataset folders editor and begin dragging the default Metrics or
Columns folder
(these UUIDs are referenced at top of the hook: DEFAULT_METRICS_FOLDER_UUID
and
DEFAULT_COLUMNS_FOLDER_UUID).
2. While dragging a default folder, hover a non-default folder target. In
updateDropTargetVisuals (file lines ~76-122, new-target handling at lines
92-119) the code
computes draggedTypes but does not check if a dragged item is a default
folder and whether
the hovered target is non-default.
3. The DOM element receives data-drop-target and is not marked
data-forbidden-drop even
though handleDragEnd (lines 372-380 and lines 551-615 logic in
forbiddenDropFolderIds)
will later reject nesting default folders into non-default folders and show
a warning.
4. Observe the visual highlight indicating a permitted drop, but on mouse up
the hook
rejects the action and shows a warning toast (addWarningToast called at
lines 378-379),
causing inconsistent UX.
5. The suggested improved code adds the same default-folder checks used
elsewhere
(forbiddenDropFolderIds and handleDragEnd) and sets data-forbidden-drop
earlier so the
visual state matches the actual drop validation.
```
</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:** 101:113
**Comment:**
*Logic Error: The forbidden-drop calculation in
`updateDropTargetVisuals` only enforces type rules for default metric/column
folders, but it ignores the rule that default folders themselves cannot be
nested into non-default folders; this leads to inconsistent UX where a
non-default folder is visually highlighted as a valid drop target when dragging
a default folder even though `handleDragEnd` will reject the drop with a
warning. Align the forbidden-drop logic here with the one in
`forbiddenDropFolderIds` so any folder that will be rejected on drop is also
marked as a forbidden drop target visually.
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.tsx:
##########
@@ -0,0 +1,380 @@
+/**
+ * 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, useState, memo, useMemo } from 'react';
+import { useSortable } from '@dnd-kit/sortable';
+import { useDroppable } from '@dnd-kit/core';
+import { CSS } from '@dnd-kit/utilities';
+import { css } from '@apache-superset/core/ui';
+import { Metric, t } from '@superset-ui/core';
Review Comment:
**Suggestion:** The `metric` prop here is typed using `Metric` from
`@superset-ui/core`, but the rest of the folders editor (e.g.,
`VirtualizedTreeItem` and `FoldersEditor` types) uses `Metric` from
`@superset-ui/chart-controls`, meaning `TreeItem` is assuming a different type
than the actual metric objects being passed; this mismatch can easily lead to
incorrect field assumptions if the two `Metric` definitions diverge, so the
import should be aligned to the same source type as the callers. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Type-check failures in FoldersEditor compilation/tests.
- ⚠️ Developer friction during local builds and CI checks.
- ❌ Potential incorrect field assumptions in TreeItem rendering.
- ⚠️ Inconsistent types across VirtualizedTreeItem and types.ts.
```
</details>
```suggestion
import { t } from '@superset-ui/core';
import type { Metric } from '@superset-ui/chart-controls';
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the file that defines the TreeItem component at
superset-frontend/src/components/Datasource/FoldersEditor/TreeItem.tsx and
observe the
import on line 25: `import { Metric, t } from '@superset-ui/core';`.
(Verified via Grep
output showing TreeItem.tsx:25.)
2. Inspect the surrounding Folder editor types and consumers:
superset-frontend/src/components/Datasource/FoldersEditor/types.ts imports
Metric from
@superset-ui/chart-controls (see types.ts:20 in Grep results), and
VirtualizedTreeItem.tsx
also imports type { Metric } from '@superset-ui/chart-controls'
(VirtualizedTreeItem.tsx:23 in Grep results). This indicates callers and
shared types are
using the chart-controls Metric type.
3. Reproduce by running TypeScript checks or the FoldersEditor unit tests:
- From repository root run the TypeScript build or tests that type-check
this area, for
example `yarn build` or `yarn test
src/components/Datasource/FoldersEditor/FoldersEditor.test.tsx`.
4. Expected observable failure: TypeScript may report type incompatibilities
or mismatched
property names when TreeItem's `metric` prop is assumed to match
`@superset-ui/core`'s
Metric but callers provide the `@superset-ui/chart-controls` Metric shape
(for example,
different optional/required fields or different property names used
elsewhere). Concrete
evidence of inconsistent imports was found via Grep: other files in this
feature
(folderValidation.ts, VirtualizedTreeList.tsx, types.ts,
folderOperations.ts, tests)
import Metric from '@superset-ui/chart-controls' while TreeItem imports it
from
'@superset-ui/core' (see Grep output lines referencing those files).
5. If the codebase is compiled or linted in CI, this mismatch can cause type
errors or
masking of real runtime mismatches; aligning the import to `import type {
Metric } from
'@superset-ui/chart-controls'` (as proposed) will match the rest of the
folder editor
callers and avoid the type conflict.
Note: This is not a runtime crash reproduction but a concrete
type-check/test failure
scenario: run the project's TypeScript checker or the FoldersEditor tests to
observe
type-related failures caused by the mismatched Metric import.
```
</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.tsx
**Line:** 25:25
**Comment:**
*Type Error: The `metric` prop here is typed using `Metric` from
`@superset-ui/core`, but the rest of the folders editor (e.g.,
`VirtualizedTreeItem` and `FoldersEditor` types) uses `Metric` from
`@superset-ui/chart-controls`, meaning `TreeItem` is assuming a different type
than the actual metric objects being passed; this mismatch can easily lead to
incorrect field assumptions if the two `Metric` definitions diverge, so the
import should be aligned to the same source type as the callers.
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]
