codeant-ai-for-open-source[bot] commented on code in PR #34785:
URL: https://github.com/apache/superset/pull/34785#discussion_r3658076279
##########
superset/datasets/api.py:
##########
@@ -220,6 +220,7 @@ class DatasetRestApi(BaseSupersetModelRestApi):
"changed_on_humanized",
"changed_by.first_name",
"changed_by.last_name",
+ "drill_through_chart_id",
Review Comment:
**Suggestion:** The new API column references `drill_through_chart_id`, but
this PR does not add the corresponding SQLAlchemy attribute to the `SqlaTable`
model. Dataset queries that build their select list from this value will fail
with an attribute-resolution error, and updates cannot persist the field. Add
the model column and relationship before exposing it through the API. [possible
bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset API reads can fail when selecting exposed fields.
- ❌ Dataset editor updates cannot persist drill-through chart configuration.
- ⚠️ Drill-to-details configuration remains unavailable through backend APIs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR applied and use the dataset REST API route
implemented by
`DatasetRestApi` in `superset/datasets/api.py`.
2. Request a dataset using the API's selectable-column path, such as `GET
/api/v1/dataset/<dataset_id>`, which uses `show_select_columns` at
`superset/datasets/api.py:210-224`.
3. The datamodel attempts to resolve `drill_through_chart_id` from the
`SqlaTable` ORM
model while constructing the query, but the supplied PR changes show no
corresponding
`SqlaTable` attribute or relationship.
4. Observe an ORM attribute-resolution/query construction error; dataset
updates using
`DatasetPutSchema.drill_through_chart_id` at
`superset/datasets/schemas.py:156-178`
likewise cannot persist the value unless the model column and migration are
added.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f627ac7ee50849ec93cd6eef4354abed&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=f627ac7ee50849ec93cd6eef4354abed&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/datasets/api.py
**Line:** 223:223
**Comment:**
*Possible Bug: The new API column references `drill_through_chart_id`,
but this PR does not add the corresponding SQLAlchemy attribute to the
`SqlaTable` model. Dataset queries that build their select list from this value
will fail with an attribute-resolution error, and updates cannot persist the
field. Add the model column and relationship before exposing it through the API.
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=f9cf68e9b52cdcc7b1df68a51f38c9a89e228c70e2b3e9c374fed25a6166808f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=f9cf68e9b52cdcc7b1df68a51f38c9a89e228c70e2b3e9c374fed25a6166808f&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Select/ChartSelect.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * 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 { useMemo } from 'react';
+import { t } from '@superset-ui/core';
+import SelectAsyncControl from
'src/explore/components/controls/SelectAsyncControl';
+import type { ComponentProps } from 'react';
+import rison from 'rison';
+
+// Extract the actual props from SelectAsyncControl component
+type SelectAsyncControlProps = ComponentProps<typeof SelectAsyncControl>;
+
+export interface ChartSelectProps
+ extends Omit<
+ SelectAsyncControlProps,
+ 'onChange' | 'dataEndpoint' | 'mutator' | 'addDangerToast'
+ > {
+ // ChartSelect-specific props that override base props
+ value?: number | null;
+ onChange: (value: number | null) => void;
+ datasetId?: number;
+}
+
+/**
+ * A chart selection component built on SelectAsyncControl
+ * @param value - The selected chart ID
+ * @param onChange - Callback when selection changes
+ * @param datasetId - Optional dataset ID to filter charts
+ * @param placeholder - Optional placeholder text
+ * @param ariaLabel - ARIA label for accessibility
+ * @param rest - All other props are passed through to SelectAsyncControl
+ */
+export default function ChartSelectUsingAsync({
+ value,
+ onChange,
+ datasetId,
+ placeholder = t('Select a chart'),
+ ariaLabel = t('Select drill-to-details chart'),
+ ...rest
+}: ChartSelectProps) {
+ // Build query parameters for filtering charts by dataset
+ const queryParams = useMemo(() => {
+ if (!datasetId) return undefined;
+
+ const filters = [
+ {
+ col: 'datasource_id',
+ opr: 'eq',
+ value: datasetId,
+ },
+ {
+ col: 'datasource_type',
+ opr: 'eq',
+ value: 'table',
+ },
+ ];
+
+ return {
+ q: rison.encode({
+ filters,
+ order_column: 'slice_name',
+ order_direction: 'asc',
+ }),
+ };
+ }, [datasetId]);
+
+ // Transform response to format expected by SelectAsyncControl
+ const mutator = useMemo(
+ () => (response: any) =>
+ response.result.map((chart: any) => ({
+ value: chart.id,
+ label: `${chart.slice_name} (${chart.viz_type})`,
+ })),
+ [],
+ );
+
+ return (
+ <SelectAsyncControl
+ ariaLabel={ariaLabel}
+ dataEndpoint="/api/v1/chart/"
+ searchParams={queryParams}
+ mutator={mutator}
+ value={value}
+ onChange={onChange}
+ placeholder={placeholder}
+ multi={false}
+ {...rest}
Review Comment:
**Suggestion:** The spread occurs after `multi={false}`, so callers can pass
`multi` through `rest` and override the component's single-select
configuration. In that case `SelectAsyncControl` will return an array to the
`onChange` callback even though `ChartSelectProps` declares a `number | null`
value, causing incorrect state updates or downstream type/logic failures. Omit
`multi` from the forwarded props or place the enforced value after the spread.
[type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart selection callbacks can receive arrays unexpectedly.
- ⚠️ Dataset chart selection may store incompatible multi-select state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the new `ChartSelectUsingAsync()` component from
`superset-frontend/src/components/Select/ChartSelect.tsx:48` with a `multi`
prop set to
`true`; `multi` remains accepted because it is not omitted from
`ChartSelectProps` at
lines 28-32.
2. The component renders `SelectAsyncControl` at line 93, first setting
`multi={false}` at
line 101.
3. The subsequent `{...rest}` spread at line 102 overwrites that setting with
`multi={true}`.
4. SelectAsyncControl at
`superset-frontend/src/explore/components/controls/SelectAsyncControl/index.tsx:56`
operates in multi-select mode and can invoke the callback with an array,
while
`ChartSelectProps.onChange` at line 35 only accepts `number | null`,
creating an
incompatible state value for any consumer that forwards `multi`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3823493fd9754736964d9cf654e40aef&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=3823493fd9754736964d9cf654e40aef&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/Select/ChartSelect.tsx
**Line:** 101:102
**Comment:**
*Type Error: The spread occurs after `multi={false}`, so callers can
pass `multi` through `rest` and override the component's single-select
configuration. In that case `SelectAsyncControl` will return an array to the
`onChange` callback even though `ChartSelectProps` declares a `number | null`
value, causing incorrect state updates or downstream type/logic failures. Omit
`multi` from the forwarded props or place the enforced value after the spread.
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=2ea08e11465df486d1baf6b74680753f37b4b2c4cd42af544db6a1625f0d0cb0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=2ea08e11465df486d1baf6b74680753f37b4b2c4cd42af544db6a1625f0d0cb0&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]
