From eea7402e4e85ed0db564118f5e98a2dcddfba0b9 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Fri, 3 Oct 2025 16:58:32 -0700 Subject: [PATCH] fix(dataset): add comprehensive mount guards to DatasourceEditor async methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the isMountedRef pattern to all async methods in DatasourceEditor to prevent state updates and toast notifications after component unmount. **Methods Protected**: - onQueryFormat: Guards SQL formatting state updates and toasts - formatSql: Guards SQL formatting via SupersetClient.post - syncMetadata: Guards metadata sync state updates and toasts - fetchUsageData: Guards usage data state updates and toasts (already had partial guards) **Pattern Applied**: - Initialize: this.isComponentMounted = false (constructor) - Set true: componentDidMount - Set false: componentWillUnmount - Guard all post-await state updates and toast calls with if (this.isComponentMounted) **Why This Matters**: The parent DatasourceEditor contains the child DatasetUsageTab. Both components make async calls that can resolve after unmount: - Parent: SupersetClient.get, formatQuery, fetchSyncedColumns - Child: onFetchCharts (calls parent's fetchUsageData) Both need mount guards because they manage independent state. This commit completes the pattern in the parent, matching the fix applied to the child in the previous commit. **Benefits**: - Eliminates potential intermittent test failures in parent component - Prevents memory leaks and stale state updates - Consistent async safety pattern across parent and child - Production safety for all SQL formatting and metadata operations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../DatasourceEditor/DatasourceEditor.jsx | 70 ++++++++++++++----- 1 file changed, 51 insertions(+), 19 deletions(-) diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx index a5ad47aefa9..ad6b01f2cf9 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx @@ -622,6 +622,7 @@ const ResultTable = class DatasourceEditor extends PureComponent { constructor(props) { super(props); + this.isComponentMounted = false; this.state = { datasource: { ...props.datasource, @@ -760,11 +761,17 @@ class DatasourceEditor extends PureComponent { try { const response = await this.props.formatQuery(datasource.sql); + if (!this.isComponentMounted) { + return; + } this.onDatasourcePropChange('sql', response.json.result); this.props.addSuccessToast(t('SQL was formatted')); } catch (error) { const { error: clientError, statusText } = await getClientErrorObject(error); + if (!this.isComponentMounted) { + return; + } this.props.addDangerToast( clientError || statusText || @@ -808,11 +815,17 @@ class DatasourceEditor extends PureComponent { body: JSON.stringify({ sql: datasource.sql }), headers: { 'Content-Type': 'application/json' }, }); + if (!this.isComponentMounted) { + return; + } this.onDatasourcePropChange('sql', response.json.result); this.props.addSuccessToast(t('SQL was formatted')); } catch (error) { const { error: clientError, statusText } = await getClientErrorObject(error); + if (!this.isComponentMounted) { + return; + } this.props.addDangerToast( clientError || statusText || @@ -823,28 +836,41 @@ class DatasourceEditor extends PureComponent { async syncMetadata() { const { datasource } = this.state; + if (!this.isComponentMounted) { + return; + } this.setState({ metadataLoading: true }); try { const newCols = await fetchSyncedColumns(datasource); + if (!this.isComponentMounted) { + return; + } const columnChanges = updateColumns( datasource.columns, newCols, this.props.addSuccessToast, ); + if (!this.isComponentMounted) { + return; + } this.setColumns({ databaseColumns: columnChanges.finalColumns.filter( col => !col.expression, // remove calculated columns ), }); - this.props.addSuccessToast(t('Metadata has been synced')); - this.setState({ metadataLoading: false }); + if (this.isComponentMounted) { + this.props.addSuccessToast(t('Metadata has been synced')); + this.setState({ metadataLoading: false }); + } } catch (error) { const { error: clientError, statusText } = await getClientErrorObject(error); - this.props.addDangerToast( - clientError || statusText || t('An error has occurred'), - ); - this.setState({ metadataLoading: false }); + if (this.isComponentMounted) { + this.props.addDangerToast( + clientError || statusText || t('An error has occurred'), + ); + this.setState({ metadataLoading: false }); + } } } @@ -901,10 +927,12 @@ class DatasourceEditor extends PureComponent { id: ids[index], })); - this.setState({ - usageCharts: chartsWithIds, - usageChartsCount: json?.count || 0, - }); + if (this.isComponentMounted) { + this.setState({ + usageCharts: chartsWithIds, + usageChartsCount: json?.count || 0, + }); + } return { charts: chartsWithIds, @@ -914,15 +942,17 @@ class DatasourceEditor extends PureComponent { } catch (error) { const { error: clientError, statusText } = await getClientErrorObject(error); - this.props.addDangerToast( - clientError || - statusText || - t('An error occurred while fetching usage data'), - ); - this.setState({ - usageCharts: [], - usageChartsCount: 0, - }); + if (this.isComponentMounted) { + this.props.addDangerToast( + clientError || + statusText || + t('An error occurred while fetching usage data'), + ); + this.setState({ + usageCharts: [], + usageChartsCount: 0, + }); + } return { charts: [], count: 0, @@ -1883,6 +1913,7 @@ class DatasourceEditor extends PureComponent { } componentDidMount() { + this.isComponentMounted = true; Mousetrap.bind('ctrl+shift+f', e => { e.preventDefault(); if (this.state.isEditMode) { @@ -1894,6 +1925,7 @@ class DatasourceEditor extends PureComponent { } componentWillUnmount() { + this.isComponentMounted = false; Mousetrap.unbind('ctrl+shift+f'); this.props.resetQuery(); }