codeant-ai-for-open-source[bot] commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3481529585
##########
superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx:
##########
@@ -237,413 +216,388 @@ const preventRouterLinkWhileMetaClicked = (evt:
React.MouseEvent) => {
}
};
-class DatasourceControl extends PureComponent<
- DatasourceControlProps,
- DatasourceControlState
-> {
- static defaultProps = defaultProps;
-
- constructor(props: DatasourceControlProps) {
- super(props);
- this.state = {
- showEditDatasourceModal: false,
- showChangeDatasourceModal: false,
- showSaveDatasetModal: false,
- };
- }
-
- onDatasourceSave = (datasource: Datasource) => {
- // Cast to ExtendedDatasource for the component's internal use
- this.props.actions.changeDatasource(datasource as ExtendedDatasource);
- // Cast datasource for getTemporalColumns which expects Dataset |
QueryResponse
- const { temporalColumns, defaultTemporalColumn } = getTemporalColumns(
- datasource as Parameters<typeof getTemporalColumns>[0],
- );
- const { columns } = datasource;
- // the current granularity_sqla might not be a temporal column anymore
- const timeCol = this.props.form_data?.granularity_sqla;
- const isGranularitySqlaTemporal = columns.find(
- ({ column_name }) => column_name === timeCol,
- )?.is_dttm;
- // the current main_dttm_col might not be a temporal column anymore
- const isDefaultTemporal = columns.find(
- ({ column_name }) => column_name === defaultTemporalColumn,
- )?.is_dttm;
-
- // if the current granularity_sqla is empty or it is not a temporal column
anymore
- // let's update the control value
- if (datasource.type === 'table' && !isGranularitySqlaTemporal) {
- const temporalColumn = isDefaultTemporal
- ? defaultTemporalColumn
- : temporalColumns?.[0];
- this.props.actions.setControlValue(
- 'granularity_sqla',
- temporalColumn || null,
+export default function DatasourceControl({
+ actions,
+ onChange = () => {},
+ value = null,
+ datasource,
+ form_data,
+ isEditable = true,
+ onDatasourceSave = null,
+ user,
+}: DatasourceControlProps) {
+ const theme = useTheme();
+
+ const [showEditDatasourceModal, setShowEditDatasourceModal] =
useState(false);
+ const [showChangeDatasourceModal, setShowChangeDatasourceModal] =
+ useState(false);
+ const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
+
+ const handleDatasourceSave = useCallback(
+ (savedDatasource: Datasource) => {
+ // Cast to ExtendedDatasource for the component's internal use
+ actions.changeDatasource(savedDatasource as ExtendedDatasource);
+ // Cast datasource for getTemporalColumns which expects Dataset |
QueryResponse
+ const { temporalColumns, defaultTemporalColumn } = getTemporalColumns(
+ savedDatasource as Parameters<typeof getTemporalColumns>[0],
);
- }
+ const { columns } = savedDatasource;
+ // the granularity_sqla might not be a temporal column anymore
+ const timeCol = form_data?.granularity_sqla;
+ const isGranularitySqlaTemporal = columns.find(
+ ({ column_name }) => column_name === timeCol,
+ )?.is_dttm;
Review Comment:
**Suggestion:** This path assumes `savedDatasource.columns` is always
present and immediately calls `.find(...)`. For datasource payloads without a
`columns` array (for example partial saves), this throws at runtime and breaks
save handling. Guard `columns` before accessing `.find` (or default to an empty
array). [null pointer]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Datasource edit modal can crash on missing columns.
- ⚠️ Explore granularity_sqla not updated after dataset changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Explore `DatasourceControl` component at
`superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx:219-228`
is
used by the Explore datasource panel (see `DatasourcePanel` wiring at
`src/explore/components/DatasourcePanel/index.tsx:40-62`) to display and
manage the active
dataset in the chart builder.
2. From the Explore UI, the user opens the dataset edit flow via the
datasource menu; this
renders `DatasourceModal` or `SemanticViewEditModal` from
`DatasourceControl` lines
`104-124` and passes `handleDatasourceSave` as `onDatasourceSave` (see
`index.tsx:104-124`
and `236-33`).
3. When the child modal calls `onDatasourceSave(savedDatasource)` with a
payload that
omits `columns` (possible because `ExtendedDatasource` extends `Datasource`
from
`@superset-ui/core`, and `ExtendedDatasource` at `index.tsx:56-73` does not
declare
`columns` as required), `handleDatasourceSave` executes at
`index.tsx:236-33`,
destructures `const { columns } = savedDatasource;`, and immediately calls
`columns.find(...)` at `index.tsx:247-252`, causing `TypeError: columns.find
is not a
function` if `columns` is `undefined`.
4. This runtime error occurs on the save path for the datasource: it
interrupts the logic
that checks and resets `granularity_sqla` (`index.tsx:245-257`) and prevents
any
subsequent logic after the thrown exception from running, resulting in a
broken save
experience when incomplete datasource metadata is returned.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c945ade8a0f64794b459a75ff8a6b053&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=c945ade8a0f64794b459a75ff8a6b053&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/DatasourceControl/index.tsx
**Line:** 244:249
**Comment:**
*Null Pointer: This path assumes `savedDatasource.columns` is always
present and immediately calls `.find(...)`. For datasource payloads without a
`columns` array (for example partial saves), this throws at runtime and breaks
save handling. Guard `columns` before accessing `.find` (or default to an empty
array).
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%2F39461&comment_hash=dcc8759d7de76fe77ec01328dab4448bf63d6b2252dd66ceba7b1dc11b0ccafb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=dcc8759d7de76fe77ec01328dab4448bf63d6b2252dd66ceba7b1dc11b0ccafb&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.tsx:
##########
@@ -235,174 +214,205 @@ class AdhocFilterControl extends Component<
});
}
}
- }
+ }, [datasource]);
- componentDidUpdate(prevProps: AdhocFilterControlProps): void {
- if (this.props.columns !== prevProps.columns) {
- this.setState({ options: optionsForSelect(this.props) });
- }
- if (this.props.value !== prevProps.value) {
- this.setState({
- values: (this.props.value || []).map(filter =>
+ useEffect(() => {
+ if (value !== undefined) {
+ setValues(
+ (value || []).map(filter =>
isDictionaryForAdhocFilter(filter) ? new AdhocFilter(filter) :
filter,
),
- });
+ );
}
- }
+ }, [value]);
- removeFilter(index: number): void {
- const valuesCopy = [...this.state.values];
- valuesCopy.splice(index, 1);
- this.setState(prevState => ({
- ...prevState,
- values: valuesCopy,
- }));
- this.props.onChange?.(valuesCopy);
- }
+ const getMetricExpression = useCallback(
+ (savedMetricName: string): string => {
+ const metric = savedMetrics?.find(
+ savedMetric => savedMetric.metric_name === savedMetricName,
+ );
+ return metric?.expression ?? '';
+ },
+ [savedMetrics],
+ );
- onRemoveFilter(index: number): void {
- const { canDelete } = this.props;
- const { values } = this.state;
- const result = canDelete?.(values[index], values);
- if (typeof result === 'string') {
- warning({ title: t('Warning'), content: result });
- return;
- }
- this.removeFilter(index);
- }
+ const mapOption = useCallback(
+ (option: FilterOption | AdhocFilter): AdhocFilter | null => {
+ // already a AdhocFilter, skip
+ if (option instanceof AdhocFilter) {
+ return option;
+ }
+ // via datasource saved metric
+ if (option.saved_metric_name) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Sql,
+ subject: getMetricExpression(option.saved_metric_name),
+ operator:
+ OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
+ comparator: 0,
+ clause: Clauses.Having,
+ });
+ }
+ // has a custom label, meaning it's custom column
+ if (option.label) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Sql,
+ subject: new AdhocMetric(option).translateToSql(),
+ operator:
+ OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
+ comparator: 0,
+ clause: Clauses.Having,
+ });
+ }
+ // add a new filter item
+ if (option.column_name) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Simple,
+ subject: option.column_name,
+ operator: OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.Equals].operation,
+ comparator: '',
+ clause: Clauses.Where,
+ isNew: true,
+ });
+ }
+ return null;
+ },
+ [getMetricExpression],
+ );
- onNewFilter(newFilter: FilterOption | AdhocFilter): void {
- const mappedOption = this.mapOption(newFilter);
- if (mappedOption) {
- this.setState(
- prevState => ({
- ...prevState,
- values: [...prevState.values, mappedOption],
- }),
- () => {
- this.props.onChange?.(this.state.values);
- },
- );
- }
- }
+ const removeFilter = useCallback(
+ (index: number) => {
+ const valuesCopy = [...values];
+ valuesCopy.splice(index, 1);
+ setValues(valuesCopy);
+ onChange?.(valuesCopy);
+ },
+ [values, onChange],
+ );
- onFilterEdit(changedFilter: AdhocFilter): void {
- this.props.onChange?.(
- this.state.values.map(value => {
- if (value.filterOptionName === changedFilter.filterOptionName) {
- return changedFilter;
- }
- return value;
- }),
- );
- }
+ const onRemoveFilter = useCallback(
+ (index: number) => {
+ const result = canDelete?.(values[index], values);
+ if (typeof result === 'string') {
+ warning({ title: t('Warning'), content: result });
+ return;
+ }
+ removeFilter(index);
+ },
+ [canDelete, values, removeFilter],
+ );
- onChange(opts: FilterOption[] | null): void {
- const options = (opts || [])
- .map(option => this.mapOption(option))
- .filter((option): option is AdhocFilter => option !== null);
- this.props.onChange?.(options);
- }
+ const onFilterEdit = useCallback(
+ (changedFilter: AdhocFilter) => {
+ onChange?.(
+ values.map(val => {
+ if (val.filterOptionName === changedFilter.filterOptionName) {
+ return changedFilter;
+ }
+ return val;
+ }),
+ );
+ },
+ [values, onChange],
+ );
- getMetricExpression(savedMetricName: string): string {
- const metric = this.props.savedMetrics?.find(
- savedMetric => savedMetric.metric_name === savedMetricName,
- );
- return metric?.expression ?? '';
- }
+ const moveLabel = useCallback((dragIndex: number, hoverIndex: number) => {
+ setValues(prevValues => {
+ const newValues = [...prevValues];
+ [newValues[hoverIndex], newValues[dragIndex]] = [
+ newValues[dragIndex],
+ newValues[hoverIndex],
+ ];
+ return newValues;
+ });
+ }, []);
- moveLabel(dragIndex: number, hoverIndex: number): void {
- const { values } = this.state;
+ const onDropLabel = useCallback(() => {
+ onChange?.(values);
+ }, [onChange, values]);
- const newValues = [...values];
- [newValues[hoverIndex], newValues[dragIndex]] = [
- newValues[dragIndex],
- newValues[hoverIndex],
- ];
- this.setState({ values: newValues });
- }
+ const onNewFilter = useCallback(
+ (newFilter: FilterOption | AdhocFilter) => {
+ const mappedOption = mapOption(newFilter);
+ if (mappedOption) {
+ const newValues = [...values, mappedOption];
+ setValues(newValues);
+ onChange?.(newValues);
+ }
+ },
+ [mapOption, values, onChange],
+ );
- mapOption(option: FilterOption | AdhocFilter): AdhocFilter | null {
- // already a AdhocFilter, skip
- if (option instanceof AdhocFilter) {
- return option;
- }
- // via datasource saved metric
- if (option.saved_metric_name) {
- return new AdhocFilter({
- expressionType: ExpressionTypes.Sql,
- subject: this.getMetricExpression(option.saved_metric_name),
- operator:
- OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
- comparator: 0,
- clause: Clauses.Having,
- });
- }
- // has a custom label, meaning it's custom column
- if (option.label) {
- return new AdhocFilter({
- expressionType: ExpressionTypes.Sql,
- subject: new AdhocMetric(option).translateToSql(),
- operator:
- OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
- comparator: 0,
- clause: Clauses.Having,
- });
- }
- // add a new filter item
- if (option.column_name) {
- return new AdhocFilter({
- expressionType: ExpressionTypes.Simple,
- subject: option.column_name,
- operator: OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.Equals].operation,
- comparator: '',
- clause: Clauses.Where,
- isNew: true,
- });
- }
- return null;
- }
+ const valueRenderer = useCallback(
+ (adhocFilter: AdhocFilter, index: number) => (
+ <AdhocFilterOption
+ key={index}
Review Comment:
**Suggestion:** Using the array index as the React key in a
draggable/reorderable list causes item identity to drift after reordering, so
edits/removals can apply to the wrong rendered row state. Use a stable
per-filter key (for example the filter option identifier) instead. [missing
react key]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Native filter editor may delete or edit wrong filter.
- ⚠️ Dragged adhoc filters can retain incorrect DnD identity.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Dashboard native filter configuration uses `AdhocFilterControl` from
`superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.tsx`
via `FiltersConfigForm` at
`src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:85-89`
and `1217-1221`, where it passes dataset columns and filters for editing.
2. Inside `AdhocFilterControl`, the list of filters is rendered by
`valueRenderer` at
`AdhocFilterControl/index.tsx:87-105`, which creates `<AdhocFilterOption
key={index}
index={index} ... />` for each filter, explicitly using the array index both
as the React
`key` and as the DnD index.
3. `AdhocFilterOption` at `FilterControl/AdhocFilterOption/index.tsx:42-81`
in turn
renders `OptionControlLabel`, and `OptionControlLabel`
(`OptionControls/index.tsx:236-42`)
uses `useSortable` from `@dnd-kit/sortable` with a sortable id derived from
the index
(`sortable-${type}-${index}`) and tracks `dragIndex` in its sortable data.
4. When a user configures multiple adhoc filters and then reorders them via
drag-and-drop
in the FiltersConfig modal, `AdhocFilterControl` swaps entries in the
`values` array (see
`moveLabel` and `onDropLabel` at `AdhocFilterControl/index.tsx:60-73`), but
because React
keys are the indices, React reuses component instances for different filters
and
`useSortable` ids/drag indices no longer correspond to the same filter
objects; this can
cause subsequent edits or removals (wired via `onFilterEdit` and
`onRemoveFilter` at
`AdhocFilterControl/index.tsx:46-44` and `87-105`) to apply to the wrong
filter row after
reordering.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=09bfb3c4280d478fbad6a87d9e5124c5&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=09bfb3c4280d478fbad6a87d9e5124c5&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/FilterControl/AdhocFilterControl/index.tsx
**Line:** 347:349
**Comment:**
*Missing React Key: Using the array index as the React key in a
draggable/reorderable list causes item identity to drift after reordering, so
edits/removals can apply to the wrong rendered row state. Use a stable
per-filter key (for example the filter option identifier) instead.
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%2F39461&comment_hash=6dc274f7090cff3b8cc44baa4d266fb126d95f638e7c5b46bad186529af0a0a5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=6dc274f7090cff3b8cc44baa4d266fb126d95f638e7c5b46bad186529af0a0a5&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]
