codeant-ai-for-open-source[bot] commented on code in PR #34785:
URL: https://github.com/apache/superset/pull/34785#discussion_r3651598072
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -109,14 +119,65 @@ export default function DrillDetailModal({
findPermission('can_explore', 'Superset', state.user?.roles),
);
- const exploreUrl = useMemo(
- () => `/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${chartId}`,
- [chartId, dashboardPageId],
+ const showEditButton = Boolean(dataset?.drill_through_chart_id);
+ const dashboardContextFormData = useDashboardFormData(
+ dataset?.drill_through_chart_id,
);
- const exploreChart = useCallback(() => {
- history.push(exploreUrl);
- }, [exploreUrl, history]);
+ const drillThroughFormData = useMemo(() => {
+ if (!dataset?.drill_through_chart_id || !dataset?.id) {
+ return null;
+ }
+
+ const drillThroughBaseFormData = {
+ slice_id: dataset.drill_through_chart_id,
+ datasource: `${dataset.id}__table`,
+ viz_type: 'table',
+ };
Review Comment:
**Suggestion:** The configured target chart is always initialized with
`viz_type: 'table'`, and no target chart form data is loaded here.
Consequently, selecting a non-table chart or a table chart with saved
visualization settings causes the drill-through visualization to use the wrong
chart type or lose its configuration. The target chart's saved form data should
be used instead of hardcoding the table type. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Configured non-table drill targets render as tables.
- ⚠️ Table targets lose saved visualization configuration.
- ⚠️ Dataset-level drill customization becomes unreliable.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure `SqlaTable.drill_through_chart_id` through the dataset editor
so it
references an existing chart, as represented by
`superset/connectors/sqla/models.py:1152-1159`.
2. Open any dashboard chart using that dataset and trigger drill-to-detail,
causing
`DrillDetailModal` to compute `drillThroughFormData` at lines 127-149 and
pass it to
`DrillDetailPane` at lines 216-221.
3. The form data created at lines 132-136 uses the selected chart ID but
forces `viz_type`
to `table` and sets the datasource to the dataset table; it does not load
the selected
slice's saved form data.
4. Observe that a configured non-table target is rendered through the table
visualization
path, while saved target-specific controls such as columns, metrics,
ordering, formatting,
and other visualization settings are not applied.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6087cad12b7944b8b5cdfad8b7b5e1d4&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=6087cad12b7944b8b5cdfad8b7b5e1d4&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/components/Chart/DrillDetail/DrillDetailModal.tsx
**Line:** 132:136
**Comment:**
*Logic Error: The configured target chart is always initialized with
`viz_type: 'table'`, and no target chart form data is loaded here.
Consequently, selecting a non-table chart or a table chart with saved
visualization settings causes the drill-through visualization to use the wrong
chart type or lose its configuration. The target chart's saved form data should
be used instead of hardcoding the table type.
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%2F34785&comment_hash=ff4390ecbd30c487b160fb2f815eed37ee09fba6fd9f7edc266439bd2670dfe4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=ff4390ecbd30c487b160fb2f815eed37ee09fba6fd9f7edc266439bd2670dfe4&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -131,7 +192,12 @@ export default function DrillDetailModal({
name={t('Drill to detail: %s', chartName)}
title={t('Drill to detail: %s', chartName)}
footer={
- <ModalFooter exploreChart={exploreChart} canExplore={canExplore} />
+ <ModalFooter
+ canExplore={canExplore}
+ showEditButton={showEditButton}
+ onExploreClick={handleExploreClick}
+ isGeneratingUrl={isGeneratingUrl}
+ />
Review Comment:
**Suggestion:** The modal footer no longer receives `closeModal`, so
`ModalFooter` renders its primary close button with an undefined `onClick`
handler. Users cannot close the drill-to-detail modal through the Close button;
pass `onHideModal` (or the appropriate close callback) as `closeModal`. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Drill-to-detail users cannot close the modal using Close.
- ⚠️ Users must rely on alternate modal dismissal behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard chart and trigger the existing drill-to-detail flow,
which renders
`DrillDetailModal` from
`superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx`.
2. The modal renders `ModalFooter` at lines 195-200, but does not pass the
`closeModal`
prop.
3. `ModalFooter` declares `closeModal` at line 45 and assigns it to the
primary button's
`onClick` handler at lines 77-81.
4. Click the Close button (`data-test="close-drilltodetail-modal"`); because
`closeModal`
is `undefined`, `onHideModal` is never invoked and the drill-to-detail modal
remains open.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a963eb5e31774485bbd14906d7552f9d&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=a963eb5e31774485bbd14906d7552f9d&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/components/Chart/DrillDetail/DrillDetailModal.tsx
**Line:** 195:200
**Comment:**
*Logic Error: The modal footer no longer receives `closeModal`, so
`ModalFooter` renders its primary close button with an undefined `onClick`
handler. Users cannot close the drill-to-detail modal through the Close button;
pass `onHideModal` (or the appropriate close callback) as `closeModal`.
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%2F34785&comment_hash=e3d94cb262cf33de50aff2569eede16d443507eb2fd8cae9b80ed767bca71b99&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=e3d94cb262cf33de50aff2569eede16d443507eb2fd8cae9b80ed767bca71b99&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1148,6 +1148,16 @@ class SqlaTable(
always_filter_main_dttm = Column(Boolean, default=False)
folders = Column(JSON, nullable=True)
+ # Drill-through configuration
+ drill_through_chart_id = Column(
+ Integer, ForeignKey("slices.id", ondelete="SET NULL"), nullable=True
+ )
Review Comment:
**Suggestion:** This adds a database column and foreign key in the model
without an accompanying migration in the PR. On an existing deployment, queries
or updates that serialize or reference `drill_through_chart_id` will fail
because the physical column does not exist. Add and deploy the corresponding
Alembic migration. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Existing deployments cannot reliably load affected datasets.
- ❌ Dataset editor reads and updates can fail.
- ⚠️ Upgrades require manual schema repair without migration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Upgrade an existing Superset deployment to the PR version without
applying a schema
migration; the provided PR contains the model change but no Alembic
migration.
2. Open a dataset through the datasource editor or another dataset API path
that
serializes `SqlaTable.data`, which now reads `self.drill_through_chart_id` at
`superset/connectors/sqla/models.py:1365-1380`.
3. SQLAlchemy generates a query or update referencing
`drill_through_chart_id`, but the
existing `tables` database table has no such physical column.
4. Observe a database error such as an unknown-column error, causing dataset
loading or
saving to fail until the migration is applied.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83d72909a03c4651b46e17e75e6726c6&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=83d72909a03c4651b46e17e75e6726c6&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/connectors/sqla/models.py
**Line:** 1152:1154
**Comment:**
*Possible Bug: This adds a database column and foreign key in the model
without an accompanying migration in the PR. On an existing deployment, queries
or updates that serialize or reference `drill_through_chart_id` will fail
because the physical column does not exist. Add and deploy the corresponding
Alembic migration.
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%2F34785&comment_hash=c6ad7930acadbcdb41172f0eeaaf3d99b415a424883038898d3e5f7b12f7ad4d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=c6ad7930acadbcdb41172f0eeaaf3d99b415a424883038898d3e5f7b12f7ad4d&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1169,6 +1179,7 @@ class SqlaTable(
"normalize_columns",
"always_filter_main_dttm",
"folders",
+ "drill_through_chart_id",
]
Review Comment:
**Suggestion:** Including the raw integer `drill_through_chart_id` in
dataset export/import makes exports environment-dependent. Slice IDs are not
stable across environments, so an imported dataset can point to an unrelated
chart or fail its foreign-key constraint instead of referencing the selected
drill-through chart. Export a stable chart identifier and resolve it during
import, or omit this relationship from portable dataset exports. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Imported datasets may drill to an unrelated chart.
- ⚠️ Cross-environment dataset exports lose chart relationship portability.
- ⚠️ Imports can fail when the referenced slice ID is absent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a dataset with `drill_through_chart_id` set to a chart, using the
relationship
defined at `superset/connectors/sqla/models.py:1152-1159`.
2. Export the dataset through Superset's model export mechanism;
`drill_through_chart_id`
is included in `export_fields` at lines 1163-1184 and is therefore
serialized as a raw
integer.
3. Import that export into another Superset environment where slice IDs
differ, or where
the numeric ID is already assigned to a different chart.
4. The import update path uses `update_from_object_fields` at line 1184, so
the numeric
value can fail its foreign-key constraint or resolve to an unrelated chart;
subsequent
drill-to-detail requests then use the wrong target.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f91df71aa69b477eb4ace8f674f59870&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=f91df71aa69b477eb4ace8f674f59870&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/connectors/sqla/models.py
**Line:** 1181:1183
**Comment:**
*Possible Bug: Including the raw integer `drill_through_chart_id` in
dataset export/import makes exports environment-dependent. Slice IDs are not
stable across environments, so an imported dataset can point to an unrelated
chart or fail its foreign-key constraint instead of referencing the selected
drill-through chart. Export a stable chart identifier and resolve it during
import, or omit this relationship from portable dataset exports.
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%2F34785&comment_hash=db8f5c33c9ed4d96fd42ebf5dd6de22d8ccab165ded295e9b9b634154f14ed41&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=db8f5c33c9ed4d96fd42ebf5dd6de22d8ccab165ded295e9b9b634154f14ed41&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]
