codeant-ai-for-open-source[bot] commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3481517228
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -84,714 +200,652 @@ interface SaveModalProps extends RouteComponentProps {
dashboardId: '' | number | null;
isVisible: boolean;
dispatch: Dispatch;
- theme: SupersetTheme;
metadata?: ExplorePageInitialData['metadata'];
}
-type SaveModalState = {
- newSliceName?: string;
- datasetName: string;
- action: SaveActionType;
- isLoading: boolean;
- saveStatus?: string | null;
- dashboard?: { label: string; value: string | number };
- selectedTab?: { label: string; value: string | number };
- tabsData: TabTreeNode[];
-};
-
export const StyledModal = styled(Modal)`
.ant-modal-body {
overflow: visible;
}
`;
-class SaveModal extends Component<SaveModalProps, SaveModalState> {
- constructor(props: SaveModalProps) {
- super(props);
- this.state = {
- newSliceName: props.sliceName,
- datasetName: props.datasource?.name,
- action: this.canOverwriteSlice()
- ? ChartStatusType.overwrite
- : ChartStatusType.saveas,
- isLoading: false,
- dashboard: undefined,
- tabsData: [],
- selectedTab: undefined,
- };
- this.onDashboardChange = this.onDashboardChange.bind(this);
- this.onSliceNameChange = this.onSliceNameChange.bind(this);
- this.changeAction = this.changeAction.bind(this);
- this.saveOrOverwrite = this.saveOrOverwrite.bind(this);
- this.isNewDashboard = this.isNewDashboard.bind(this);
- this.onHide = this.onHide.bind(this);
- }
+const SaveModal = ({
+ addDangerToast,
+ actions,
+ form_data,
+ user,
+ alert: alertProp,
+ sliceName = '',
+ slice,
+ can_overwrite,
+ datasource,
+ dashboardId: dashboardIdProp,
+ isVisible,
+ metadata,
+}: SaveModalProps) => {
+ const dispatch = useDispatch();
+ const history = useHistory();
+ const theme = useTheme();
+
+ const canOverwriteSlice = useCallback(
+ (): boolean =>
+ (can_overwrite ||
+ isUserAdmin(user) ||
+ slice?.owners?.includes(user.userId)) &&
+ !slice?.is_managed_externally,
+ [can_overwrite, slice, user],
+ );
- isNewDashboard(): boolean {
- const { dashboard } = this.state;
- return typeof dashboard?.value === 'string';
- }
+ const [newSliceName, setNewSliceName] = useState<string | undefined>(
+ sliceName,
+ );
+ const [datasetName, setDatasetName] = useState<string>(datasource?.name);
+ const [action, setAction] = useState<SaveActionType>(
+ canOverwriteSlice() ? ChartStatusType.overwrite : ChartStatusType.saveas,
+ );
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [dashboard, setDashboard] = useState<
+ { label: string; value: string | number } | undefined
+ >(undefined);
+ const [tabsData, setTabsData] = useState<TabTreeNode[]>([]);
+ const [selectedTab, setSelectedTab] = useState<
+ { label: string; value: string | number } | undefined
+ >(undefined);
+
+ const isNewDashboard = useCallback(
+ (): boolean => typeof dashboard?.value === 'string',
+ [dashboard?.value],
+ );
- canOverwriteSlice(): boolean {
- return (
- (this.props.can_overwrite ||
- isUserAdmin(this.props.user) ||
- this.props.slice?.owners?.includes(this.props.user.userId)) &&
- !this.props.slice?.is_managed_externally
- );
- }
+ const loadDashboard = useCallback(async (id: number) => {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${id}`,
+ });
+ return response.json.result;
+ }, []);
- async componentDidMount() {
- let { dashboardId } = this.props;
- if (!dashboardId) {
- let lastDashboard = null;
- try {
- lastDashboard = sessionStorage.getItem(SK_DASHBOARD_ID);
- } catch (error) {
- // continue regardless of error
- }
- dashboardId = lastDashboard && parseInt(lastDashboard, 10);
- }
- if (dashboardId) {
+ const loadTabs = useCallback(
+ async (dashboardId: number) => {
try {
- const result = (await this.loadDashboard(dashboardId)) as Dashboard;
- if (canUserEditDashboard(result, this.props.user)) {
- this.setState({
- dashboard: { label: result.dashboard_title, value: result.id },
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}/tabs`,
+ });
+
+ const { result } = response.json;
+ if (!result || !Array.isArray(result.tab_tree)) {
+ logging.warn('Invalid tabs response format');
+ setTabsData([]);
+ return [];
Review Comment:
**Suggestion:** When tab loading fails or returns no tabs, only `tabsData`
is cleared; `selectedTab` is left untouched. That stale tab value can later be
reused for save/redirect behavior against a different dashboard and trigger
incorrect tab targeting errors. Clear `selectedTab` whenever tab data is reset.
[stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Save-and-go-to-dashboard may fail to add to tab.
- ⚠️ User can be redirected with an incorrect tab anchor.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. From the Explore view, trigger the "Save chart" flow so the connected
`SaveModal`
component at
`superset-frontend/src/explore/components/SaveModal.tsx:212-225` is shown.
During initialization, the `useEffect` at `SaveModal.tsx:334-396` calls
`loadTabs(dashboardId)` for a pre-selected dashboard, which populates
`tabsData` and sets
`selectedTab` (typically to `'OUT_OF_TAB'` or the first tab) in the success
paths at
`SaveModal.tsx:302-320`.
2. Without unmounting the component (Explore keeps it connected and toggles
`isVisible`
via `setSaveChartModalVisibility`), open the save modal again for a chart
tied to a
different dashboard. `initializeDashboard` runs again due to changed
`dashboardIdProp`/`metadata`, and calls `loadTabs(newDashboardId)` at
`SaveModal.tsx:348-352`.
3. If the `/api/v1/dashboard/${dashboardId}/tabs` request fails or returns
an invalid
shape (e.g., missing `tab_tree`), the error/invalid-path in `loadTabs`
executes: at
`SaveModal.tsx:275-278` it logs `logging.warn('Invalid tabs response
format')`, calls
`setTabsData([])`, and returns, but it does not clear `selectedTab`, leaving
whatever tab
value was set for the previous dashboard.
4. When the user clicks "Save & go to dashboard" (`renderFooter` at
`SaveModal.tsx:44-81`), `saveOrOverwrite` at `SaveModal.tsx:463-612` uses
`selectedTab?.value` to compute `selectedTabId` and either passes it into
`addChartToDashboardTab` at `SaveModal.tsx:561-568` or appends it as a URL
fragment at
`SaveModal.tsx:590-597`. Because `tabsData` was reset for the new dashboard
but
`selectedTab` was not, this can target a stale tab ID from a previous
dashboard, causing
`addChartToDashboard` at `SaveModal.tsx:97-188` to throw "Tab <id> not found
in
positionJson" and show a toast instead of adding the chart to the intended
tab.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=555e203ca6904113a5c452fd8850b4ac&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=555e203ca6904113a5c452fd8850b4ac&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/SaveModal.tsx
**Line:** 275:278
**Comment:**
*Stale Reference: When tab loading fails or returns no tabs, only
`tabsData` is cleared; `selectedTab` is left untouched. That stale tab value
can later be reused for save/redirect behavior against a different dashboard
and trigger incorrect tab targeting errors. Clear `selectedTab` whenever tab
data is reset.
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=631f68502344e759d6af78e90c44ab671cccf02036700c910f0ec350dfbbd84e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=631f68502344e759d6af78e90c44ab671cccf02036700c910f0ec350dfbbd84e&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -883,602 +869,701 @@ const mapStateToProps = (state: RootState) => ({
const connector = connect(mapStateToProps, mapDispatchToProps);
type PropsFromRedux = ConnectedProps<typeof connector>;
-type DatasourceEditorProps = DatasourceEditorOwnProps &
- PropsFromRedux & {
- theme?: SupersetTheme;
- };
-
-class DatasourceEditor extends PureComponent<
- DatasourceEditorProps,
- DatasourceEditorState
-> {
- private isComponentMounted: boolean;
+type DatasourceEditorProps = DatasourceEditorOwnProps & PropsFromRedux;
+
+function DatasourceEditor({
+ datasource: propsDatasource,
+ onChange = () => {},
+ addSuccessToast,
+ addDangerToast,
+ setIsEditing = () => {},
+ database,
+ runQuery,
+ resetQuery,
+ formatQuery: formatQueryAction,
+}: DatasourceEditorProps) {
+ const theme = useTheme();
+ const isComponentMounted = useRef(false);
+ const isInitialMount = useRef(true);
+ const prevPropsDatasourceRef = useRef(propsDatasource);
+ const isSyncingColumnsFromProps = useRef(false);
+ const abortControllers = useRef<AbortControllers>({
+ formatQuery: null,
+ formatSql: null,
+ syncMetadata: null,
+ fetchUsageData: null,
+ });
+
+ // Initialize datasource state with transformed owners and metrics
+ const [datasource, setDatasource] = useState<DatasourceObject>(() => ({
+ ...propsDatasource,
+ owners: propsDatasource.owners.map(owner => {
+ const ownerName = owner.label || `${owner.first_name}
${owner.last_name}`;
+ return {
+ value: owner.value || owner.id,
+ label: OwnerSelectLabel({
+ name: typeof ownerName === 'string' ? ownerName : '',
+ email: owner.email,
+ }),
+ [OWNER_TEXT_LABEL_PROP]: typeof ownerName === 'string' ? ownerName :
'',
+ [OWNER_EMAIL_PROP]: owner.email ?? '',
+ };
+ }),
+ metrics: propsDatasource.metrics?.map(metric => {
+ const {
+ certified_by: certifiedByMetric,
+ certification_details: certificationDetails,
+ } = metric;
+ const {
+ certification: {
+ details = undefined,
+ certified_by: certifiedBy = undefined,
+ } = {},
+ warning_markdown: warningMarkdown,
+ } = JSON.parse(metric.extra || '{}') || {};
+ return {
+ ...metric,
+ certification_details: certificationDetails || details,
+ warning_markdown: warningMarkdown || '',
+ certified_by: certifiedBy || certifiedByMetric,
+ };
+ }),
+ }));
- private abortControllers: AbortControllers;
+ const [errors, setErrors] = useState<string[]>([]);
+ const [isSqla] = useState(
+ propsDatasource.datasource_type === 'table' ||
+ propsDatasource.type === 'table',
+ );
+ const [isEditMode, setIsEditMode] = useState(false);
+ const [databaseColumns, setDatabaseColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !col.expression),
+ );
+ const [calculatedColumns, setCalculatedColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !!col.expression),
+ );
+ const [folders, setFolders] = useState<DatasourceFolder[]>(
+ propsDatasource.folders || [],
+ );
+ const [folderCount, setFolderCount] = useState(() => {
+ const savedFolders = propsDatasource.folders || [];
+ const savedCount = countAllFolders(savedFolders);
+ const hasDefaultsSaved = savedFolders.some(f => isDefaultFolder(f.uuid));
+ return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
+ });
+ const [metadataLoading, setMetadataLoading] = useState(false);
+ const [activeTabKey, setActiveTabKey] = useState(TABS_KEYS.SOURCE);
+ const [datasourceType, setDatasourceType] = useState(
+ propsDatasource.sql
+ ? DATASOURCE_TYPES.virtual.key
+ : DATASOURCE_TYPES.physical.key,
+ );
+ const [usageCharts, setUsageCharts] = useState<ChartUsageData[]>([]);
+ const [usageChartsCount, setUsageChartsCount] = useState(0);
+ const [metricSearchTerm, setMetricSearchTerm] = useState('');
+ const [columnSearchTerm, setColumnSearchTerm] = useState('');
+ const [calculatedColumnSearchTerm, setCalculatedColumnSearchTerm] =
+ useState('');
+
+ const findDuplicates = useCallback(
+ <T,>(arr: T[], accessor: (obj: T) => string): string[] => {
+ const seen: Record<string, null> = {};
+ const dups: string[] = [];
+ arr.forEach((obj: T) => {
+ const item = accessor(obj);
+ if (item in seen) {
+ dups.push(item);
+ } else {
+ seen[item] = null;
+ }
+ });
+ return dups;
+ },
+ [],
+ );
- static defaultProps = {
- onChange: () => {},
- setIsEditing: () => {},
- };
+ const validate = useCallback(
+ (callback: (validationErrors: string[]) => void) => {
+ let validationErrors: string[] = [];
+ let dups: string[];
- constructor(props: DatasourceEditorProps) {
- super(props);
- this.state = {
- datasource: {
- ...props.datasource,
- owners: props.datasource.owners.map(owner => {
- const ownerName =
- owner.label || `${owner.first_name} ${owner.last_name}`;
- return {
- value: owner.value || owner.id,
- label: OwnerSelectLabel({
- name: typeof ownerName === 'string' ? ownerName : '',
- email: owner.email,
- }),
- [OWNER_TEXT_LABEL_PROP]:
- typeof ownerName === 'string' ? ownerName : '',
- [OWNER_EMAIL_PROP]: owner.email ?? '',
- };
- }),
- metrics: props.datasource.metrics?.map(metric => {
- const {
- certified_by: certifiedByMetric,
- certification_details: certificationDetails,
- } = metric;
- const {
- certification: {
- details = undefined,
- certified_by: certifiedBy = undefined,
- } = {},
- warning_markdown: warningMarkdown,
- } = JSON.parse(metric.extra || '{}') || {};
- return {
- ...metric,
- certification_details: certificationDetails || details,
- warning_markdown: warningMarkdown || '',
- certified_by: certifiedBy || certifiedByMetric,
- };
- }),
- },
- errors: [],
- isSqla:
- props.datasource.datasource_type === 'table' ||
- props.datasource.type === 'table',
- isEditMode: false,
- databaseColumns: props.datasource.columns.filter(col => !col.expression),
- calculatedColumns: props.datasource.columns.filter(
- col => !!col.expression,
- ),
- folders: props.datasource.folders || [],
- folderCount: (() => {
- const savedFolders = props.datasource.folders || [];
- const savedCount = countAllFolders(savedFolders);
- const hasDefaultsSaved = savedFolders.some(f =>
- isDefaultFolder(f.uuid),
- );
- return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
- })(),
- metadataLoading: false,
- activeTabKey: TABS_KEYS.SOURCE,
- datasourceType: props.datasource.sql
- ? DATASOURCE_TYPES.virtual.key
- : DATASOURCE_TYPES.physical.key,
- usageCharts: [],
- usageChartsCount: 0,
- metricSearchTerm: '',
- columnSearchTerm: '',
- calculatedColumnSearchTerm: '',
- };
+ // Looking for duplicate column_name
+ dups = findDuplicates(datasource.columns, obj => obj.column_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Column name [%s] is duplicated', name)),
+ );
- this.isComponentMounted = false;
- this.abortControllers = {
- formatQuery: null,
- formatSql: null,
- syncMetadata: null,
- fetchUsageData: null,
- };
+ // Looking for duplicate metric_name
+ dups = findDuplicates(datasource.metrics ?? [], obj => obj.metric_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Metric name [%s] is duplicated', name)),
+ );
- this.onChange = this.onChange.bind(this);
- this.onChangeEditMode = this.onChangeEditMode.bind(this);
- this.onDatasourcePropChange = this.onDatasourcePropChange.bind(this);
- this.onDatasourceChange = this.onDatasourceChange.bind(this);
- this.tableChangeAndSyncMetadata =
- this.tableChangeAndSyncMetadata.bind(this);
- this.syncMetadata = this.syncMetadata.bind(this);
- this.setColumns = this.setColumns.bind(this);
- this.validateAndChange = this.validateAndChange.bind(this);
- this.handleTabSelect = this.handleTabSelect.bind(this);
- this.formatSql = this.formatSql.bind(this);
- this.fetchUsageData = this.fetchUsageData.bind(this);
- this.handleFoldersChange = this.handleFoldersChange.bind(this);
- }
+ // Making sure calculatedColumns have an expression defined
+ const noFilterCalcCols = calculatedColumns.filter(
+ col => !col.expression && !col.json,
+ );
+ validationErrors = validationErrors.concat(
+ noFilterCalcCols.map(col =>
+ t('Calculated column [%s] requires an expression', col.column_name),
+ ),
+ );
- onChange() {
- // Emptying SQL if "Physical" radio button is selected
- // Currently the logic to know whether the source is
- // physical or virtual is based on whether SQL is empty or not.
- const { datasourceType, datasource } = this.state;
- const sql =
- datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql;
-
- const columns = [
- ...this.state.databaseColumns,
- ...this.state.calculatedColumns,
- ];
-
- // Remove deleted column/metric references from folders
- const validUuids = new Set<string>();
- for (const col of columns) {
- if (col.uuid) validUuids.add(col.uuid);
- }
- for (const metric of datasource.metrics ?? []) {
- if (metric.uuid) validUuids.add(metric.uuid);
- }
- const folders = filterFoldersByValidUuids(this.state.folders, validUuids);
+ // validate currency code (skip 'AUTO' - it's a placeholder for
auto-detection)
+ try {
+ datasource.metrics?.forEach(
+ metric =>
+ metric.currency?.symbol &&
+ metric.currency.symbol !== 'AUTO' &&
+ new Intl.NumberFormat('en-US', {
+ style: 'currency',
+ currency: metric.currency.symbol,
+ }),
+ );
+ } catch {
+ validationErrors = validationErrors.concat([
+ t('Invalid currency code in saved metrics'),
+ ]);
+ }
- const newDatasource = {
- ...this.state.datasource,
- sql,
- columns,
- folders,
- };
+ // Validate folders
+ if (folders?.length > 0) {
+ const folderValidation = validateFolders(folders);
+ validationErrors = validationErrors.concat(folderValidation.errors);
+ }
- this.props.onChange?.(newDatasource, this.state.errors);
- }
+ setErrors(validationErrors);
+ callback(validationErrors);
+ },
+ [datasource, calculatedColumns, folders, findDuplicates],
+ );
- onChangeEditMode() {
- this.props.setIsEditing?.(!this.state.isEditMode);
- this.setState(prevState => ({ isEditMode: !prevState.isEditMode }));
- }
+ const onChangeInternal = useCallback(
+ (validationErrors: string[] = errors) => {
+ // Emptying SQL if "Physical" radio button is selected
+ const sql =
+ datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql;
- onDatasourceChange(
- datasource: DatasourceObject,
- callback: () => void = this.validateAndChange,
- ) {
- this.setState({ datasource }, callback);
- }
+ const columns = [...databaseColumns, ...calculatedColumns];
- onDatasourcePropChange(attr: string, value: unknown) {
- if (value === undefined) return; // if value is undefined do not update
state
- const datasource = { ...this.state.datasource, [attr]: value };
- this.setState(
- prevState => ({
- datasource: { ...prevState.datasource, [attr]: value },
- }),
- () =>
- attr === 'table_name'
- ? this.onDatasourceChange(datasource,
this.tableChangeAndSyncMetadata)
- : this.onDatasourceChange(datasource, this.validateAndChange),
- );
- }
+ // Remove deleted column/metric references from folders
+ const validUuids = new Set<string>();
+ for (const col of columns) {
+ if (col.uuid) validUuids.add(col.uuid);
+ }
+ for (const metric of datasource.metrics ?? []) {
+ if (metric.uuid) validUuids.add(metric.uuid);
+ }
+ const filteredFolders = filterFoldersByValidUuids(folders, validUuids);
- onDatasourceTypeChange(datasourceType: string) {
- // Call onChange after setting datasourceType to ensure
- // SQL is cleared when switching to a physical dataset
- this.setState({ datasourceType }, this.onChange);
- }
+ const newDatasource = {
+ ...datasource,
+ sql,
+ columns,
+ folders: filteredFolders,
+ };
- handleFoldersChange(folders: DatasourceFolder[]) {
- const folderCount = countAllFolders(folders);
- this.setState({ folders, folderCount }, () => {
- this.onDatasourceChange({
- ...this.state.datasource,
- folders,
- });
- });
- }
+ onChange(newDatasource, validationErrors);
+ },
+ [
+ datasource,
+ datasourceType,
+ databaseColumns,
+ calculatedColumns,
+ folders,
+ errors,
+ onChange,
+ ],
+ );
- setColumns(
- obj: { databaseColumns?: Column[] } | { calculatedColumns?: Column[] },
- ) {
- // update calculatedColumns or databaseColumns
- this.setState(
- obj as Pick<
- DatasourceEditorState,
- 'databaseColumns' | 'calculatedColumns'
- >,
- this.validateAndChange,
- );
- }
+ const validateAndChange = useCallback(() => {
+ validate(onChangeInternal);
+ }, [validate, onChangeInternal]);
- validateAndChange() {
- this.validate(this.onChange);
- }
+ const onDatasourceChange = useCallback((newDatasource: DatasourceObject) => {
+ setDatasource(newDatasource);
+ }, []);
- async onQueryRun() {
- const databaseId = this.state.datasource.database?.id;
- const { sql } = this.state.datasource;
- if (!databaseId || !sql) {
- return;
- }
- this.props.runQuery({
- client_id: this.props.database?.clientId,
- database_id: databaseId,
- runAsync: false,
- catalog: this.state.datasource.catalog,
- schema: this.state.datasource.schema,
- sql,
- tmp_table_name: '',
- select_as_cta: false,
- ctas_method: 'TABLE',
- queryLimit: 25,
- expand_data: true,
+ const onDatasourcePropChange = useCallback((attr: string, value: unknown) =>
{
+ if (value === undefined) return;
+ setDatasource(prev => {
+ const newDatasource = { ...prev, [attr]: value };
+ return newDatasource;
});
Review Comment:
**Suggestion:** `onDatasourcePropChange` drops `undefined` updates, but
`DatabaseSelector` intentionally calls `onCatalogChange(undefined)` and
`onSchemaChange(undefined)` when the DB changes. This leaves stale
catalog/schema values in state and can run queries/syncs against the wrong
namespace. Allow `undefined` so clear events propagate. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset preview queries use stale schema/catalog values.
- ❌ SQL Lab link opens for wrong catalog/schema combination.
- ⚠️ Column sync may target incorrect physical table namespace.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dataset in the Dataset Editor, which renders `DatasourceEditor` via
`DatasourceModal` at
`superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:90-99`
where
`<DatasourceEditor datasource={currentDatasource} ... />` is mounted.
2. In the "Source" tab of `DatasourceEditor`, change the database using the
`DatabaseSelector` control configured at `DatasourceEditor.tsx:1421-1456`
(`<DatabaseSelector ... onCatalogChange={...} onSchemaChange={...}
onDbChange={...} />`).
3. When the user selects a new database, `DatabaseSelector.changeDatabase()`
at
`superset-frontend/src/components/DatabaseSelector/index.tsx:127-145` calls
`onDbChange(databaseWithId)` and then explicitly clears dependent namespace
fields by
calling `onCatalogChange(undefined)` and `onSchemaChange(undefined)`.
4. In `DatasourceEditor`, these callbacks are wired to
`onDatasourcePropChange` at
`DatasourceEditor.tsx:1087-1092`, which currently early-returns when `value
===
undefined`. As a result, the clear operations for `catalog` and `schema` are
dropped and
`datasource.catalog` / `datasource.schema` keep their old values. Subsequent
actions that
rely on these fields—such as running a preview query in `onQueryRun` at
`DatasourceEditor.tsx:752-771` or building the SQL Lab URL in `getSQLLabUrl`
at
`DatasourceEditor.tsx:736-745` and syncing metadata in `syncMetadata` at
`DatasourceEditor.tsx:815-863`—will use the stale catalog/schema from the
previous
database, causing queries and metadata syncs to run against the wrong
namespace.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0df873adfbca4bab99f142526610e85c&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=0df873adfbca4bab99f142526610e85c&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/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 1087:1092
**Comment:**
*Api Mismatch: `onDatasourcePropChange` drops `undefined` updates, but
`DatabaseSelector` intentionally calls `onCatalogChange(undefined)` and
`onSchemaChange(undefined)` when the DB changes. This leaves stale
catalog/schema values in state and can run queries/syncs against the wrong
namespace. Allow `undefined` so clear events propagate.
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=8c30fc18ba360e86a6c7b0fd074e60cbeffd3a0c6649b32607cba891731476df&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=8c30fc18ba360e86a6c7b0fd074e60cbeffd3a0c6649b32607cba891731476df&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -883,602 +869,701 @@ const mapStateToProps = (state: RootState) => ({
const connector = connect(mapStateToProps, mapDispatchToProps);
type PropsFromRedux = ConnectedProps<typeof connector>;
-type DatasourceEditorProps = DatasourceEditorOwnProps &
- PropsFromRedux & {
- theme?: SupersetTheme;
- };
-
-class DatasourceEditor extends PureComponent<
- DatasourceEditorProps,
- DatasourceEditorState
-> {
- private isComponentMounted: boolean;
+type DatasourceEditorProps = DatasourceEditorOwnProps & PropsFromRedux;
+
+function DatasourceEditor({
+ datasource: propsDatasource,
+ onChange = () => {},
+ addSuccessToast,
+ addDangerToast,
+ setIsEditing = () => {},
+ database,
+ runQuery,
+ resetQuery,
+ formatQuery: formatQueryAction,
+}: DatasourceEditorProps) {
+ const theme = useTheme();
+ const isComponentMounted = useRef(false);
+ const isInitialMount = useRef(true);
+ const prevPropsDatasourceRef = useRef(propsDatasource);
+ const isSyncingColumnsFromProps = useRef(false);
+ const abortControllers = useRef<AbortControllers>({
+ formatQuery: null,
+ formatSql: null,
+ syncMetadata: null,
+ fetchUsageData: null,
+ });
+
+ // Initialize datasource state with transformed owners and metrics
+ const [datasource, setDatasource] = useState<DatasourceObject>(() => ({
+ ...propsDatasource,
+ owners: propsDatasource.owners.map(owner => {
+ const ownerName = owner.label || `${owner.first_name}
${owner.last_name}`;
+ return {
+ value: owner.value || owner.id,
+ label: OwnerSelectLabel({
+ name: typeof ownerName === 'string' ? ownerName : '',
+ email: owner.email,
+ }),
+ [OWNER_TEXT_LABEL_PROP]: typeof ownerName === 'string' ? ownerName :
'',
+ [OWNER_EMAIL_PROP]: owner.email ?? '',
+ };
+ }),
+ metrics: propsDatasource.metrics?.map(metric => {
+ const {
+ certified_by: certifiedByMetric,
+ certification_details: certificationDetails,
+ } = metric;
+ const {
+ certification: {
+ details = undefined,
+ certified_by: certifiedBy = undefined,
+ } = {},
+ warning_markdown: warningMarkdown,
+ } = JSON.parse(metric.extra || '{}') || {};
+ return {
+ ...metric,
+ certification_details: certificationDetails || details,
+ warning_markdown: warningMarkdown || '',
+ certified_by: certifiedBy || certifiedByMetric,
+ };
+ }),
+ }));
- private abortControllers: AbortControllers;
+ const [errors, setErrors] = useState<string[]>([]);
+ const [isSqla] = useState(
+ propsDatasource.datasource_type === 'table' ||
+ propsDatasource.type === 'table',
+ );
+ const [isEditMode, setIsEditMode] = useState(false);
+ const [databaseColumns, setDatabaseColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !col.expression),
+ );
+ const [calculatedColumns, setCalculatedColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !!col.expression),
+ );
+ const [folders, setFolders] = useState<DatasourceFolder[]>(
+ propsDatasource.folders || [],
+ );
+ const [folderCount, setFolderCount] = useState(() => {
+ const savedFolders = propsDatasource.folders || [];
+ const savedCount = countAllFolders(savedFolders);
+ const hasDefaultsSaved = savedFolders.some(f => isDefaultFolder(f.uuid));
+ return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
+ });
+ const [metadataLoading, setMetadataLoading] = useState(false);
+ const [activeTabKey, setActiveTabKey] = useState(TABS_KEYS.SOURCE);
+ const [datasourceType, setDatasourceType] = useState(
+ propsDatasource.sql
+ ? DATASOURCE_TYPES.virtual.key
+ : DATASOURCE_TYPES.physical.key,
+ );
+ const [usageCharts, setUsageCharts] = useState<ChartUsageData[]>([]);
+ const [usageChartsCount, setUsageChartsCount] = useState(0);
+ const [metricSearchTerm, setMetricSearchTerm] = useState('');
+ const [columnSearchTerm, setColumnSearchTerm] = useState('');
+ const [calculatedColumnSearchTerm, setCalculatedColumnSearchTerm] =
+ useState('');
+
+ const findDuplicates = useCallback(
+ <T,>(arr: T[], accessor: (obj: T) => string): string[] => {
+ const seen: Record<string, null> = {};
+ const dups: string[] = [];
+ arr.forEach((obj: T) => {
+ const item = accessor(obj);
+ if (item in seen) {
+ dups.push(item);
+ } else {
+ seen[item] = null;
+ }
+ });
+ return dups;
+ },
+ [],
+ );
- static defaultProps = {
- onChange: () => {},
- setIsEditing: () => {},
- };
+ const validate = useCallback(
+ (callback: (validationErrors: string[]) => void) => {
+ let validationErrors: string[] = [];
+ let dups: string[];
- constructor(props: DatasourceEditorProps) {
- super(props);
- this.state = {
- datasource: {
- ...props.datasource,
- owners: props.datasource.owners.map(owner => {
- const ownerName =
- owner.label || `${owner.first_name} ${owner.last_name}`;
- return {
- value: owner.value || owner.id,
- label: OwnerSelectLabel({
- name: typeof ownerName === 'string' ? ownerName : '',
- email: owner.email,
- }),
- [OWNER_TEXT_LABEL_PROP]:
- typeof ownerName === 'string' ? ownerName : '',
- [OWNER_EMAIL_PROP]: owner.email ?? '',
- };
- }),
- metrics: props.datasource.metrics?.map(metric => {
- const {
- certified_by: certifiedByMetric,
- certification_details: certificationDetails,
- } = metric;
- const {
- certification: {
- details = undefined,
- certified_by: certifiedBy = undefined,
- } = {},
- warning_markdown: warningMarkdown,
- } = JSON.parse(metric.extra || '{}') || {};
- return {
- ...metric,
- certification_details: certificationDetails || details,
- warning_markdown: warningMarkdown || '',
- certified_by: certifiedBy || certifiedByMetric,
- };
- }),
- },
- errors: [],
- isSqla:
- props.datasource.datasource_type === 'table' ||
- props.datasource.type === 'table',
- isEditMode: false,
- databaseColumns: props.datasource.columns.filter(col => !col.expression),
- calculatedColumns: props.datasource.columns.filter(
- col => !!col.expression,
- ),
- folders: props.datasource.folders || [],
- folderCount: (() => {
- const savedFolders = props.datasource.folders || [];
- const savedCount = countAllFolders(savedFolders);
- const hasDefaultsSaved = savedFolders.some(f =>
- isDefaultFolder(f.uuid),
- );
- return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
- })(),
- metadataLoading: false,
- activeTabKey: TABS_KEYS.SOURCE,
- datasourceType: props.datasource.sql
- ? DATASOURCE_TYPES.virtual.key
- : DATASOURCE_TYPES.physical.key,
- usageCharts: [],
- usageChartsCount: 0,
- metricSearchTerm: '',
- columnSearchTerm: '',
- calculatedColumnSearchTerm: '',
- };
+ // Looking for duplicate column_name
+ dups = findDuplicates(datasource.columns, obj => obj.column_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Column name [%s] is duplicated', name)),
Review Comment:
**Suggestion:** Column duplicate validation reads `datasource.columns`, but
column edits are stored in `databaseColumns`/`calculatedColumns` and not
written back into `datasource.columns` first. This makes duplicate detection
run on stale data and miss real conflicts after user edits. Validate against
the merged live column state instead. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dataset editor can save duplicate column names silently.
- ⚠️ Downstream charts may see ambiguous or conflicting column keys.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dataset in the Dataset Editor so `DatasourceEditor` is rendered
from
`DatasourceModal` at
`superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:90-99`
with
`onChange={onDatasourceChange}` wired back into the modal.
2. Edit physical or calculated columns in the "Columns" or "Calculated
columns" tabs:
`ColumnCollectionTable` in the columns tab at
`DatasourceEditor.tsx:2020-2027` and in the
calculated columns tab at `DatasourceEditor.tsx:2054-2079` calls
`onColumnsChange`, which
in turn calls `setColumns` at `DatasourceEditor.tsx:709-719` to update
`databaseColumns` /
`calculatedColumns` state. These column edits do not update
`datasource.columns` directly.
3. After column edits, the `useEffect` at `DatasourceEditor.tsx:723-734`
triggers
validation via `validateAndChange()`, which calls `validate` at
`DatasourceEditor.tsx:556-610`. The duplicate-column check inside `validate`
uses
`findDuplicates(datasource.columns, obj => obj.column_name)` at
`DatasourceEditor.tsx:991`, but `datasource.columns` still reflects the
original
`propsDatasource.columns` (it is only re-derived from props in the
`propsDatasource`
effect at `DatasourceEditor.tsx:1020-1061`, not from `setColumns`).
4. Because `datasource.columns` is stale, any duplicate column names
introduced or left
unresolved in the live `databaseColumns`/`calculatedColumns` state are not
seen by
`findDuplicates`, so the `validationErrors` array does not include
duplicate-name errors.
`onChangeInternal` at `DatasourceEditor.tsx:612-638` still merges
`[...databaseColumns,
...calculatedColumns]` and passes them to `onChange`, allowing the user to
save a dataset
with duplicate column names without front-end validation catching the
conflict.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f11e37eba41944e8a031424ed164ed3a&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=f11e37eba41944e8a031424ed164ed3a&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/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 991:993
**Comment:**
*Logic Error: Column duplicate validation reads `datasource.columns`,
but column edits are stored in `databaseColumns`/`calculatedColumns` and not
written back into `datasource.columns` first. This makes duplicate detection
run on stale data and miss real conflicts after user edits. Validate against
the merged live column state 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=6352e02b1a712c6c269c9c99f077e985db25c2bc7411ec2859e4fb9472c8f632&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=6352e02b1a712c6c269c9c99f077e985db25c2bc7411ec2859e4fb9472c8f632&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]
