codeant-ai-for-open-source[bot] commented on code in PR #42483:
URL: https://github.com/apache/superset/pull/42483#discussion_r3658389358
##########
superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:
##########
@@ -362,7 +411,22 @@ export const ExploreDndContextProvider:
FC<ExploreDndContextProps> = ({
DatasourcePanel drags (which carry a value) get a preview.
*/}
<DragOverlay dropAnimation={null}>
- {activeData?.value ? (
+ {activeData?.type === DndItemType.Folder ? (
+ <DragOverlayContainer align="center" justify="space-between">
+ <Flex align="center" gap={4}>
+ <Icons.FolderOutlined iconSize="l" />
+ <span>{activeData.name}</span>
+ </Flex>
+ <FolderDragBadge>
+ {tn(
+ '%s column',
+ '%s columns',
+ activeData.items?.length ?? 0,
+ activeData.items?.length ?? 0,
+ )}
Review Comment:
**Suggestion:** The badge is labeled as a column count but uses
`activeData.items.length`, and folder contents can include metrics as
demonstrated by the folder drag tests. A folder containing metrics therefore
displays an incorrect count and label; either count only columns or label the
total as fields/items. [possible bug]
<details>
<summary><b>Severity Level:</b> Minor π§Ή</summary>
```mdx
- β οΈ Folder drag overlay mislabels metric counts.
- β οΈ Users receive inaccurate drag feedback.
- β οΈ Drop behavior remains functionally unaffected.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Create or use a datasource folder whose drag payload contains metric
items; the new
`ActiveDragData.items` field is explicitly documented as holding
βcolumns/metricsβ at
`superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:66-69`.
2. Start dragging that folder so the provider renders the folder-specific
overlay selected
by the condition at
`superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:414`.
3. The overlay passes `activeData.items.length` twice to `tn()` at lines
421-426 and uses
the singular/plural messages `%s column` and `%s columns` at lines 422-423.
4. Observe that a folder containing metrics, or a mixture of metrics and
columns, is
displayed as a number of columns rather than fields or items. This affects
the drag
preview text only; the underlying drop payload is not changed.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=755bb8cbbce94764a4632228ea7ba765&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=755bb8cbbce94764a4632228ea7ba765&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx
**Line:** 421:426
**Comment:**
*Possible Bug: The badge is labeled as a column count but uses
`activeData.items.length`, and folder contents can include metrics as
demonstrated by the folder drag tests. A folder containing metrics therefore
displays an incorrect count and label; either count only columns or label the
total as fields/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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42483&comment_hash=6c4f3a7a22d8e8b1b948e79ed4df820619c650ca69c41bc29f3b8ddf15546400&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42483&comment_hash=6c4f3a7a22d8e8b1b948e79ed4df820619c650ca69c41bc29f3b8ddf15546400&reaction=dislike'>π</a>
##########
superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:
##########
@@ -177,6 +203,29 @@ export function resolveDragEnd(
return;
}
+ // Folder drop: expand the folder into its individual columns/metrics and
+ // hand the accepted subset to the droppable's bulk handler. Only controls
+ // that opt in via `onDropFolder` react (filters, for instance, don't). Each
+ // item is gated by the droppable's own `accept`/`canDrop`, so duplicates and
+ // unsupported types are dropped β satisfying "only add columns not already
+ // present".
+ if (activeData?.type === DndItemType.Folder) {
+ const onDropFolder = overData?.onDropFolder;
+ const items = Array.isArray(activeData.items) ? activeData.items : [];
+ if (!onDropFolder || items.length === 0) {
+ return;
Review Comment:
**Suggestion:** Folder drops are discarded unless the target provides
`onDropFolder`, but this PR does not add that handler to any of the existing
explore drop zones. Consequently, dragging a folder onto dimensions, groupbys,
or metrics reaches this branch and returns without adding anything. Either wire
`onDropFolder` into every intended control or expand the folder items through
the existing per-item drop handlers. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Folder drops fail on unregistered explore drop zones.
- β Dimensions, groupbys, and metrics remain unchanged.
- β οΈ New bulk-drop functionality requires consumer-specific wiring.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Start an Explore chart and begin dragging a folder from the datasource
panel; the
folder drag data is represented by `ActiveDragData` with `type`, `items`,
and `name` at
`superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:62-71`.
2. Drop the folder on a dimensions, groupbys, or metrics drop zone, which
causes the
drag-end dispatcher `resolveDragEnd()` at
`superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:193`
to
receive `activeData.type === DndItemType.Folder`.
3. The folder branch reads `overData.onDropFolder` at
`superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx:213`
and
obtains the folder items at line 214.
4. If the target drop zone only exposes the existing per-item handlers and
does not
register `onDropFolder`, the guard at lines 215-216 returns before invoking
`onDrop`, so
no folder columns or metrics are added. The supplied PR changes introduce the
`onDropFolder` contract at lines 163-166 but do not show registrations for
the intended
explore drop zones, so the feature is a no-op unless those registrations
exist elsewhere.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e0b8b739bc464fbeb8bd365792a38343&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e0b8b739bc464fbeb8bd365792a38343&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreContainer/ExploreDndContext.tsx
**Line:** 213:216
**Comment:**
*Logic Error: Folder drops are discarded unless the target provides
`onDropFolder`, but this PR does not add that handler to any of the existing
explore drop zones. Consequently, dragging a folder onto dimensions, groupbys,
or metrics reaches this branch and returns without adding anything. Either wire
`onDropFolder` into every intended control or expand the folder items through
the existing per-item drop handlers.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42483&comment_hash=cc00a4f6ae455a34b53c7121aab2f1745aba4de5c1efc6c0edbc141320a1d2f5&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42483&comment_hash=cc00a4f6ae455a34b53c7121aab2f1745aba4de5c1efc6c0edbc141320a1d2f5&reaction=dislike'>π</a>
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnMetricSelect.tsx:
##########
@@ -257,6 +261,25 @@ function DndColumnMetricSelect(props:
DndColumnMetricSelectProps) {
[combinedOptionsMap, coercedValue, isMetricSelected],
);
+ const onDropFolder = useCallback(
+ (items: DatasourcePanelDndItem[]) => {
+ // Items already passed `canDrop` (valid, not already selected).
+ const newValues = [...coercedValue];
+ items.forEach(item => {
+ if (item.type === DndItemType.Column) {
+ newValues.push((item.value as ColumnMeta).column_name);
+ } else if (item.type === DndItemType.Metric) {
+ newValues.push((item.value as Metric).metric_name);
+ }
+ });
+ if (newValues.length === coercedValue.length) {
+ return;
+ }
+ onChange(multi ? newValues : newValues[0]);
+ },
+ [onChange, coercedValue, multi],
Review Comment:
**Suggestion:** When this control is single-valued and already contains an
item, the new folder items are appended to `newValues` but `newValues[0]` is
sent to `onChange`, so the existing value is retained and the dropped item is
silently ignored. Replace the existing value or select the newly dropped item
for non-multi controls. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Single-value Explore fields silently ignore folder replacements.
- β οΈ Folder drag-and-drop appears successful but preserves stale selections.
- β οΈ Users must remove existing values before dropping folders.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open Explore and configure a field rendered by `DndColumnMetricSelect` in
`superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnMetricSelect.tsx`
with `multi` set to `false` and an existing column or metric selected.
2. Drag a folder from the datasource panel; `folderDrag.ts:46` collects its
descendant
columns and metrics, and `DndSelectLabel` invokes the `onDropFolder`
callback registered
at `DndColumnMetricSelect.tsx:463`.
3. `onDropFolder` at `DndColumnMetricSelect.tsx:264-273` copies the existing
`coercedValue` and appends the dropped items, so the first element remains
the previously
selected value.
4. Because `multi` is false, `DndColumnMetricSelect.tsx:278` calls
`onChange(newValues[0])`; the control retains the old selection and silently
ignores the
newly dropped folder contents.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ea01c2ad7ed348cd8dca1a22bcf83c87&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ea01c2ad7ed348cd8dca1a22bcf83c87&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/DndColumnSelectControl/DndColumnMetricSelect.tsx
**Line:** 264:280
**Comment:**
*Logic Error: When this control is single-valued and already contains
an item, the new folder items are appended to `newValues` but `newValues[0]` is
sent to `onChange`, so the existing value is retained and the dropped item is
silently ignored. Replace the existing value or select the newly dropped item
for non-multi controls.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42483&comment_hash=13f3c312d1f2283d524b707f225508059787aefe882540ba887c4deda97da82f&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42483&comment_hash=13f3c312d1f2283d524b707f225508059787aefe882540ba887c4deda97da82f&reaction=dislike'>π</a>
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx:
##########
@@ -46,7 +46,35 @@ import { AGGREGATES } from 'src/explore/constants';
import { datasetLabelLower } from 'src/features/semanticLayers/label';
const EMPTY_OBJECT = {};
-const DND_ACCEPTED_TYPES = [DndItemType.Column, DndItemType.Metric];
+const DND_ACCEPTED_TYPES = [
+ DndItemType.Column,
+ DndItemType.Metric,
+ DndItemType.Folder,
+];
+
+/**
+ * Build an adhoc metric from a dropped column, picking a sensible default
+ * aggregation from the column's data type: SUM for numeric columns,
+ * COUNT_DISTINCT for string/boolean/temporal ones.
+ */
+export const createAdhocMetricFromColumn = (
+ column: ColumnMeta,
+): AdhocMetric => {
+ // Cast config to handle ColumnMeta/ColumnType mismatch
+ const config = {
+ column,
+ } as Partial<AdhocMetric>;
+ if (column.type_generic === GenericDataType.Numeric) {
+ config.aggregate = AGGREGATES.SUM;
+ } else if (
+ column.type_generic === GenericDataType.String ||
+ column.type_generic === GenericDataType.Boolean ||
+ column.type_generic === GenericDataType.Temporal
+ ) {
+ config.aggregate = AGGREGATES.COUNT_DISTINCT;
+ }
+ return new AdhocMetric(config);
Review Comment:
**Suggestion:** `createAdhocMetricFromColumn` leaves `config.aggregate`
unset for any column whose `type_generic` is not Numeric, String, Boolean, or
Temporal. Folder drops bypass the aggregation popover, so such columns produce
adhoc metrics without an aggregation, which can result in an invalid metric or
a failed query. Provide a valid fallback or reject unsupported column types
before creating the metric. [possible bug]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Folder drops can create incomplete metrics.
- β Explore queries may fail for unsupported column types.
- β οΈ Metric creation bypasses aggregation selection.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. In Explore, create a folder containing a datasource column whose
`ColumnMeta.type_generic` is not `Numeric`, `String`, `Boolean`, or
`Temporal`.
2. Drag that folder onto the metrics control; `DndSelectLabel` calls
`DndMetricSelect.tsx:416-433` through its `onDropFolder` prop.
3. `onDropFolder` maps the column to `createAdhocMetricFromColumn()` at
`DndMetricSelect.tsx:421-425`, bypassing the individual-column popover
handled by
`handleDrop()` at `DndMetricSelect.tsx:403-411`.
4. `createAdhocMetricFromColumn()` at `DndMetricSelect.tsx:67-76` leaves
`config.aggregate` unset for the unsupported type and immediately stores the
resulting
`AdhocMetric` through `setValue()` and `handleChange()`, producing an
incomplete metric
that may be rejected during query construction or execution.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3ea2874c669b437f9ed993122fa2609d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3ea2874c669b437f9ed993122fa2609d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/DndColumnSelectControl/DndMetricSelect.tsx
**Line:** 67:76
**Comment:**
*Possible Bug: `createAdhocMetricFromColumn` leaves `config.aggregate`
unset for any column whose `type_generic` is not Numeric, String, Boolean, or
Temporal. Folder drops bypass the aggregation popover, so such columns produce
adhoc metrics without an aggregation, which can result in an invalid metric or
a failed query. Provide a valid fallback or reject unsupported column types
before creating the metric.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42483&comment_hash=edd669c940e11c798f1ef25c159b8e721bed33bd36c01e672156d6e7e65d6a77&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42483&comment_hash=edd669c940e11c798f1ef25c159b8e721bed33bd36c01e672156d6e7e65d6a77&reaction=dislike'>π</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
