[Bug Fix]Prevent re-rendering when non-instant controls change (#6483)

fix: Prevent re-rendering when non-instant controls change

superset/assets/src/chart/Chart.jsx

@@ -1,15 +1,11 @@
-import dompurify from 'dompurify';
-import { snakeCase } from 'lodash';
 import PropTypes from 'prop-types';
 import React from 'react';
-import { Tooltip } from 'react-bootstrap';
+import { Logger, LOG_ACTIONS_RENDER_CHART_CONTAINER } from '../logger';
-import { ChartProps } from '@superset-ui/chart';
-import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger';
 import Loading from '../components/Loading';
 import RefreshChartOverlay from '../components/RefreshChartOverlay';
 import StackTraceMessage from '../components/StackTraceMessage';
-import SuperChart from '../visualizations/core/components/SuperChart';
 import ErrorBoundary from '../components/ErrorBoundary';
+import ChartRenderer from './ChartRenderer';
 import './chart.css';
 
 const propTypes = {
@@ -24,6 +20,7 @@ const propTypes = {
   setControlValue: PropTypes.func,
   timeout: PropTypes.number,
   vizType: PropTypes.string.isRequired,
+  triggerRender: PropTypes.bool,
   // state
   chartAlert: PropTypes.string,
   chartStatus: PropTypes.string,
@@ -35,7 +32,6 @@ const propTypes = {
   // dashboard callbacks
   addFilter: PropTypes.func,
   onQuery: PropTypes.func,
-  onDismissRefreshOverlay: PropTypes.func,
 };
 
 const BLANK = {};
@@ -44,20 +40,10 @@ const defaultProps = {
   addFilter: () => BLANK,
   filters: BLANK,
   setControlValue() {},
+  triggerRender: false,
 };
 
 class Chart extends React.PureComponent {
-  constructor(props) {
-    super(props);
-    this.state = {};
-
-    this.createChartProps = ChartProps.createSelector();
-    this.handleAddFilter = this.handleAddFilter.bind(this);
-    this.handleRenderSuccess = this.handleRenderSuccess.bind(this);
-    this.handleRenderFailure = this.handleRenderFailure.bind(this);
-    this.setTooltip = this.setTooltip.bind(this);
-  }
-
   componentDidMount() {
     if (this.props.triggerQuery) {
       this.props.actions.runQuery(
@@ -69,34 +55,12 @@ class Chart extends React.PureComponent {
     }
   }
 
-  setTooltip(tooltip) {
-    this.setState({ tooltip });
-  }
-
-  handleAddFilter(col, vals, merge = true, refresh = true) {
-    this.props.addFilter(col, vals, merge, refresh);
-  }
-
-  handleRenderSuccess() {
-    const { actions, chartStatus, chartId, vizType } = this.props;
-    if (['loading', 'rendered'].indexOf(chartStatus) < 0) {
-      actions.chartRenderingSucceeded(chartId);
-    }
-
-    Logger.append(LOG_ACTIONS_RENDER_CHART, {
-      slice_id: chartId,
-      viz_type: vizType,
-      start_offset: this.renderStartTime,
-      duration: Logger.getTimestamp() - this.renderStartTime,
-    });
-  }
-
   handleRenderFailure(error, info) {
     const { actions, chartId } = this.props;
     console.warn(error); // eslint-disable-line
     actions.chartRenderingFailed(error.toString(), chartId, info ? info.componentStack : null);
 
-    Logger.append(LOG_ACTIONS_RENDER_CHART, {
+    Logger.append(LOG_ACTIONS_RENDER_CHART_CONTAINER, {
       slice_id: chartId,
       has_err: true,
       error_details: error.toString(),
@@ -105,57 +69,6 @@ class Chart extends React.PureComponent {
     });
   }
 
-  prepareChartProps() {
-    const {
-      width,
-      height,
-      annotationData,
-      datasource,
-      filters,
-      formData,
-      queryResponse,
-      setControlValue,
-    } = this.props;
-
-    return this.createChartProps({
-      width,
-      height,
-      annotationData,
-      datasource,
-      filters,
-      formData,
-      onAddFilter: this.handleAddFilter,
-      onError: this.handleRenderFailure,
-      payload: queryResponse,
-      setControlValue,
-      setTooltip: this.setTooltip,
-    });
-  }
-
-  renderTooltip() {
-    const { tooltip } = this.state;
-    if (tooltip && tooltip.content) {
-      return (
-        <Tooltip
-          className="chart-tooltip"
-          id="chart-tooltip"
-          placement="right"
-          positionTop={tooltip.y + 30}
-          positionLeft={tooltip.x + 30}
-          arrowOffsetTop={10}
-        >
-          {typeof (tooltip.content) === 'string' ?
-            <div // eslint-disable-next-line react/no-danger
-              dangerouslySetInnerHTML={{ __html: dompurify.sanitize(tooltip.content) }}
-            />
-            : tooltip.content
-          }
-        </Tooltip>
-      );
-    }
-    return null;
-  }
-
   render() {
     const {
       width,
@@ -164,11 +77,9 @@ class Chart extends React.PureComponent {
       chartStackTrace,
       chartStatus,
       errorMessage,
-      onDismissRefreshOverlay,
       onQuery,
       queryResponse,
       refreshOverlayVisible,
-      vizType,
     } = this.props;
 
     const isLoading = chartStatus === 'loading';
@@ -176,18 +87,16 @@ class Chart extends React.PureComponent {
     // this allows <Loading /> to be positioned in the middle of the chart
     const containerStyles = isLoading ? { height, width } : null;
     const isFaded = refreshOverlayVisible && !errorMessage;
-    const skipChartRendering = isLoading || !!chartAlert;
+    this.renderContainerStartTime = Logger.getTimestamp();
-    this.renderStartTime = Logger.getTimestamp();
 
     return (
-      <ErrorBoundary onError={this.handleRenderFailure} showMessage={false}>
+      <ErrorBoundary onError={this.handleRenderContainerFailure} showMessage={false}>
         <div
           className={`chart-container ${isLoading ? 'is-loading' : ''}`}
           style={containerStyles}
         >
-          {this.renderTooltip()}
 
-          {['loading', 'success'].indexOf(chartStatus) >= 0 && <Loading size={50} />}
+          {isLoading && <Loading size={50} />}
 
           {chartAlert && (
             <StackTraceMessage
@@ -202,16 +111,13 @@ class Chart extends React.PureComponent {
               width={width}
               height={height}
               onQuery={onQuery}
-              onDismiss={onDismissRefreshOverlay}
             />
           )}
-          <SuperChart
+          <div className={`slice_container ${isFaded ? ' faded' : ''}`}>
-            className={`slice_container ${snakeCase(vizType)} ${isFaded ? ' faded' : ''}`}
+            <ChartRenderer
-            chartType={vizType}
+              {...this.props}
-            chartProps={skipChartRendering ? null : this.prepareChartProps()}
+            />
-            onRenderSuccess={this.handleRenderSuccess}
+          </div>
-            onRenderFailure={this.handleRenderFailure}
-          />
         </div>
       </ErrorBoundary>
     );

superset/assets/src/chart/ChartRenderer.jsx

@@ -0,0 +1,188 @@
+import dompurify from 'dompurify';
+import { snakeCase } from 'lodash';
+import PropTypes from 'prop-types';
+import React from 'react';
+import { ChartProps } from '@superset-ui/chart';
+import { Tooltip } from 'react-bootstrap';
+import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger';
+import SuperChart from '../visualizations/core/components/SuperChart';
+
+const propTypes = {
+  annotationData: PropTypes.object,
+  actions: PropTypes.object,
+  chartId: PropTypes.number.isRequired,
+  datasource: PropTypes.object.isRequired,
+  filters: PropTypes.object,
+  formData: PropTypes.object.isRequired,
+  height: PropTypes.number,
+  width: PropTypes.number,
+  setControlValue: PropTypes.func,
+  vizType: PropTypes.string.isRequired,
+  triggerRender: PropTypes.bool,
+  // state
+  chartAlert: PropTypes.string,
+  chartStatus: PropTypes.string,
+  queryResponse: PropTypes.object,
+  triggerQuery: PropTypes.bool,
+  refreshOverlayVisible: PropTypes.bool,
+  // dashboard callbacks
+  addFilter: PropTypes.func,
+};
+
+const BLANK = {};
+
+const defaultProps = {
+  addFilter: () => BLANK,
+  filters: BLANK,
+  setControlValue() {},
+  triggerRender: false,
+};
+
+class ChartRenderer extends React.PureComponent {
+  constructor(props) {
+    super(props);
+    this.state = {};
+
+    this.createChartProps = ChartProps.createSelector();
+
+    this.setTooltip = this.setTooltip.bind(this);
+    this.handleAddFilter = this.handleAddFilter.bind(this);
+    this.handleRenderSuccess = this.handleRenderSuccess.bind(this);
+    this.handleRenderFailure = this.handleRenderFailure.bind(this);
+  }
+
+  shouldComponentUpdate(nextProps) {
+    if (
+      nextProps.queryResponse &&
+      ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
+      !nextProps.queryResponse.error &&
+      !nextProps.refreshOverlayVisible &&
+      (nextProps.annotationData !== this.props.annotationData ||
+        nextProps.queryResponse !== this.props.queryResponse ||
+        nextProps.height !== this.props.height ||
+        nextProps.width !== this.props.width ||
+        nextProps.triggerRender)
+    ) {
+      return true;
+    }
+    return false;
+  }
+
+  setTooltip(tooltip) {
+    this.setState({ tooltip });
+  }
+
+  prepareChartProps() {
+    const {
+      width,
+      height,
+      annotationData,
+      datasource,
+      filters,
+      formData,
+      queryResponse,
+      setControlValue,
+    } = this.props;
+
+    return this.createChartProps({
+      width,
+      height,
+      annotationData,
+      datasource,
+      filters,
+      formData,
+      onAddFilter: this.handleAddFilter,
+      onError: this.handleRenderFailure,
+      payload: queryResponse,
+      setControlValue,
+      setTooltip: this.setTooltip,
+    });
+  }
+
+  handleAddFilter(col, vals, merge = true, refresh = true) {
+    this.props.addFilter(col, vals, merge, refresh);
+  }
+
+  handleRenderSuccess() {
+    const { actions, chartStatus, chartId, vizType } = this.props;
+    if (['loading', 'rendered'].indexOf(chartStatus) < 0) {
+      actions.chartRenderingSucceeded(chartId);
+    }
+
+    Logger.append(LOG_ACTIONS_RENDER_CHART, {
+      slice_id: chartId,
+      viz_type: vizType,
+      start_offset: this.renderStartTime,
+      duration: Logger.getTimestamp() - this.renderStartTime,
+    });
+  }
+
+  handleRenderFailure(error, info) {
+    const { actions, chartId } = this.props;
+    console.warn(error); // eslint-disable-line
+    actions.chartRenderingFailed(error.toString(), chartId, info ? info.componentStack : null);
+
+    Logger.append(LOG_ACTIONS_RENDER_CHART, {
+      slice_id: chartId,
+      has_err: true,
+      error_details: error.toString(),
+      start_offset: this.renderStartTime,
+      duration: Logger.getTimestamp() - this.renderStartTime,
+    });
+  }
+
+  renderTooltip() {
+    const { tooltip } = this.state;
+    if (tooltip && tooltip.content) {
+      return (
+        <Tooltip
+          className="chart-tooltip"
+          id="chart-tooltip"
+          placement="right"
+          positionTop={tooltip.y + 30}
+          positionLeft={tooltip.x + 30}
+          arrowOffsetTop={10}
+        >
+          {typeof (tooltip.content) === 'string' ?
+            <div // eslint-disable-next-line react/no-danger
+              dangerouslySetInnerHTML={{ __html: dompurify.sanitize(tooltip.content) }}
+            />
+            : tooltip.content
+          }
+        </Tooltip>
+      );
+    }
+    return null;
+  }
+
+  render() {
+    const {
+      chartAlert,
+      chartStatus,
+      vizType,
+    } = this.props;
+
+    const isLoading = chartStatus === 'loading';
+
+    const skipChartRendering = isLoading || !!chartAlert;
+    this.renderStartTime = Logger.getTimestamp();
+
+    return (
+      <React.Fragment>
+        {this.renderTooltip()}
+        <SuperChart
+          className={`${snakeCase(vizType)}`}
+          chartType={vizType}
+          chartProps={skipChartRendering ? null : this.prepareChartProps()}
+          onRenderSuccess={this.handleRenderSuccess}
+          onRenderFailure={this.handleRenderFailure}
+        />
+      </React.Fragment>
+    );
+  }
+}
+
+ChartRenderer.propTypes = propTypes;
+ChartRenderer.defaultProps = defaultProps;
+
+export default ChartRenderer;

superset/assets/src/chart/chartReducer.js

@@ -85,7 +85,11 @@ export default function chartReducer(charts = {}, action) {
       };
     },
     [actions.TRIGGER_QUERY](state) {
-      return { ...state, triggerQuery: action.value };
+      return {
+        ...state,
+        triggerQuery: action.value,
+        chartStatus: 'loading',
+      };
     },
     [actions.RENDER_TRIGGERED](state) {
       return { ...state, lastRendered: action.value };

superset/assets/src/components/RefreshChartOverlay.jsx

@@ -7,7 +7,6 @@ const propTypes = {
   height: PropTypes.number.isRequired,
   width: PropTypes.number.isRequired,
   onQuery: PropTypes.func,
-  onDismiss: PropTypes.func,
 };
 
 class RefreshChartOverlay extends React.PureComponent {
@@ -25,12 +24,6 @@ class RefreshChartOverlay extends React.PureComponent {
           >
             {t('Run Query')}
           </Button>
-          <Button
-            className="dismiss-overlay-btn"
-            onClick={this.props.onDismiss}
-          >
-            {t('Dismiss')}
-          </Button>
         </div>
       </div>
     );

superset/assets/src/explore/components/ExploreChartPanel.jsx

@@ -10,7 +10,6 @@ const propTypes = {
   actions: PropTypes.object.isRequired,
   addHistory: PropTypes.func,
   onQuery: PropTypes.func,
-  onDismissRefreshOverlay: PropTypes.func,
   can_overwrite: PropTypes.bool.isRequired,
   can_download: PropTypes.bool.isRequired,
   datasource: PropTypes.object,
@@ -28,6 +27,7 @@ const propTypes = {
   refreshOverlayVisible: PropTypes.bool,
   chart: chartPropShape,
   errorMessage: PropTypes.node,
+  triggerRender: PropTypes.bool,
 };
 
 class ExploreChartPanel extends React.PureComponent {
@@ -46,10 +46,10 @@ class ExploreChartPanel extends React.PureComponent {
             chartStackTrace={chart.chartStackTrace}
             chartId={chart.id}
             chartStatus={chart.chartStatus}
+            triggerRender={this.props.triggerRender}
             datasource={this.props.datasource}
             errorMessage={this.props.errorMessage}
             formData={this.props.form_data}
-            onDismissRefreshOverlay={this.props.onDismissRefreshOverlay}
             onQuery={this.props.onQuery}
             queryResponse={chart.queryResponse}
             refreshOverlayVisible={this.props.refreshOverlayVisible}

superset/assets/src/explore/components/ExploreViewContainer.jsx

@@ -122,10 +122,6 @@ class ExploreViewContainer extends React.Component {
     this.addHistory({});
   }
 
-  onDismissRefreshOverlay() {
-    this.setState({ refreshOverlayVisible: false });
-  }
-
   onStop() {
     if (this.props.chart && this.props.chart.queryController) {
       this.props.chart.queryController.abort();
@@ -247,7 +243,6 @@ class ExploreViewContainer extends React.Component {
         refreshOverlayVisible={this.state.refreshOverlayVisible}
         addHistory={this.addHistory}
         onQuery={this.onQuery.bind(this)}
-        onDismissRefreshOverlay={this.onDismissRefreshOverlay.bind(this)}
       />
     );
   }
@@ -319,6 +314,7 @@ function mapStateToProps(state) {
     containerId: explore.slice ? `slice-container-${explore.slice.slice_id}` : 'slice-container',
     isStarred: explore.isStarred,
     slice: explore.slice,
+    triggerRender: explore.triggerRender,
     form_data,
     table_name: form_data.datasource_name,
     vizType: form_data.viz_type,

superset/assets/src/explore/reducers/exploreReducer.js

@@ -80,11 +80,14 @@ export default function exploreReducer(state = {}, action) {
       };
       if (control.renderTrigger) {
         changes.triggerRender = true;
+      } else {
+        changes.triggerRender = false;
       }
-      return {
+      const newState = {
         ...state,
         ...changes,
       };
+      return newState;
     },
     [actions.SET_EXPLORE_CONTROLS]() {
       return {

superset/assets/src/logger.js

@@ -132,6 +132,7 @@ export const LOG_ACTIONS_MOUNT_EXPLORER = 'mount_explorer';
 export const LOG_ACTIONS_FIRST_DASHBOARD_LOAD = 'first_dashboard_load';
 export const LOG_ACTIONS_LOAD_DASHBOARD_PANE = 'load_dashboard_pane';
 export const LOG_ACTIONS_LOAD_CHART = 'load_chart_data';
+export const LOG_ACTIONS_RENDER_CHART_CONTAINER = 'render_chart_container';
 export const LOG_ACTIONS_RENDER_CHART = 'render_chart';
 export const LOG_ACTIONS_REFRESH_CHART = 'force_refresh_chart';
 
@@ -158,4 +159,5 @@ export const EXPLORE_EVENT_NAMES = [
   LOG_ACTIONS_LOAD_CHART,
   LOG_ACTIONS_RENDER_CHART,
   LOG_ACTIONS_REFRESH_CHART,
+  LOG_ACTIONS_RENDER_CHART_CONTAINER,
 ];