From 4ce475f2872fc4fd3a459cd1ec5d32bfad05dac6 Mon Sep 17 00:00:00 2001
From: Maxime Beauchemin <maximebeauchemin@gmail.com>
Date: Wed, 7 Nov 2018 17:11:52 -0800
Subject: [PATCH] Wrap <LoadableRenderer /> with <ErrorBoundary /> (#6294)

* Wrap <LoadableRenderer /> with <ErrorBoundary />

It appears that since the introduction of <SuperChart />, errors in the
visualization javascript (which are somewhat common and expected,
especially as we'll support plugins) were not handled and the whole
page would throw and go missing.

Here I'm introducing a new <ErrorBoundary /> component that elegantly
wraps other
components and handles errors. It's inspired by:
https://reactjs.org/docs/error-boundaries.html

The default behavior of the component is to simply surface the error
as an <Alert bsStyle="danger" /> and exposes the React stacktrace
when clicking on the error.

It's also possible to use component and pass an onError handler and not
show the default message.

This also fixes some minor bugs in TimeTable.

* Addressing comments
---
 superset/assets/src/chart/Chart.jsx           | 24 +++++-----
 .../assets/src/components/CachedLabel.jsx     |  2 +-
 .../assets/src/components/ErrorBoundary.jsx   | 45 +++++++++++++++++++
 .../src/components/StackTraceMessage.jsx      | 25 +++++------
 .../visualizations/TimeTable/TimeTable.jsx    |  2 +-
 .../TimeTable/transformProps.js               |  2 +-
 6 files changed, 72 insertions(+), 28 deletions(-)
 create mode 100644 superset/assets/src/components/ErrorBoundary.jsx

diff --git a/superset/assets/src/chart/Chart.jsx b/superset/assets/src/chart/Chart.jsx
index a4be8dcc8..d0d346184 100644
--- a/superset/assets/src/chart/Chart.jsx
+++ b/superset/assets/src/chart/Chart.jsx
@@ -9,6 +9,7 @@ import RefreshChartOverlay from '../components/RefreshChartOverlay';
 import StackTraceMessage from '../components/StackTraceMessage';
 import ChartProps from '../visualizations/core/models/ChartProps';
 import SuperChart from '../visualizations/core/components/SuperChart';
+import ErrorBoundary from '../components/ErrorBoundary';
 import './chart.css';
 
 const propTypes = {
@@ -92,7 +93,7 @@ class Chart extends React.PureComponent {
   handleRenderFailure(e) {
     const { actions, chartId } = this.props;
     console.warn(e); // eslint-disable-line
-    actions.chartRenderingFailed(e, chartId);
+    actions.chartRenderingFailed(e.toString(), chartId);
   }
 
   prepareChartProps() {
@@ -181,7 +182,8 @@ class Chart extends React.PureComponent {
         {chartAlert && (
           <StackTraceMessage
             message={chartAlert}
-            queryResponse={queryResponse}
+            link={queryResponse ? queryResponse.link : null}
+            stackTrace={queryResponse ? queryResponse.stacktrace : null}
           />
         )}
 
@@ -194,14 +196,16 @@ class Chart extends React.PureComponent {
           />
         )}
 
-        <SuperChart
-          className={`slice_container ${snakeCase(vizType)} ${isFaded ? ' faded' : ''}`}
-          chartType={vizType}
-          chartProps={skipChartRendering ? null : this.prepareChartProps()}
-          onRenderSuccess={this.handleRenderSuccess}
-          onRenderFailure={this.handleRenderFailure}
-          skipRendering={skipChartRendering}
-        />
+        <ErrorBoundary onError={this.handleRenderFailure} showMessage={false}>
+          <SuperChart
+            className={`slice_container ${snakeCase(vizType)} ${isFaded ? ' faded' : ''}`}
+            chartType={vizType}
+            chartProps={skipChartRendering ? null : this.prepareChartProps()}
+            onRenderSuccess={this.handleRenderSuccess}
+            onRenderFailure={this.handleRenderFailure}
+            skipRendering={skipChartRendering}
+          />
+        </ErrorBoundary>
       </div>
     );
   }
diff --git a/superset/assets/src/components/CachedLabel.jsx b/superset/assets/src/components/CachedLabel.jsx
index 845eac570..f529cde3a 100644
--- a/superset/assets/src/components/CachedLabel.jsx
+++ b/superset/assets/src/components/CachedLabel.jsx
@@ -60,7 +60,7 @@ class CacheLabel extends React.PureComponent {
           onMouseOver={this.mouseOver.bind(this)}
           onMouseOut={this.mouseOut.bind(this)}
         >
-          cached <i className="fa fa-refresh" />
+          {t('cached')} <i className="fa fa-refresh" />
         </Label>
       </TooltipWrapper>);
   }
diff --git a/superset/assets/src/components/ErrorBoundary.jsx b/superset/assets/src/components/ErrorBoundary.jsx
new file mode 100644
index 000000000..1be2d2e7a
--- /dev/null
+++ b/superset/assets/src/components/ErrorBoundary.jsx
@@ -0,0 +1,45 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import { t } from '@superset-ui/translation';
+import StackTraceMessage from './StackTraceMessage';
+
+const propTypes = {
+  children: PropTypes.node.isRequired,
+  onError: PropTypes.func,
+  showMessage: PropTypes.bool,
+};
+const defaultProps = {
+  onError: () => {},
+  showMessage: true,
+};
+
+export default class ErrorBoundary extends React.Component {
+  constructor(props) {
+    super(props);
+    this.state = { error: null, info: null };
+  }
+
+  componentDidCatch(error, info) {
+    this.props.onError(error, info);
+    this.setState({ error, info });
+  }
+
+  render() {
+    const { error, info } = this.state;
+    if (error) {
+      const firstLine = error ? error.toString() : null;
+      const message = (
+        <span>
+          <strong>{t('Unexpected error')}</strong>{firstLine ? `: ${firstLine}` : ''}
+        </span>);
+      if (this.props.showMessage) {
+        return (
+          <StackTraceMessage message={message} stackTrace={info ? info.componentStack : null} />);
+      }
+      return null;
+    }
+    return this.props.children;
+  }
+}
+ErrorBoundary.propTypes = propTypes;
+ErrorBoundary.defaultProps = defaultProps;
diff --git a/superset/assets/src/components/StackTraceMessage.jsx b/superset/assets/src/components/StackTraceMessage.jsx
index 9253108d2..d3a0f68be 100644
--- a/superset/assets/src/components/StackTraceMessage.jsx
+++ b/superset/assets/src/components/StackTraceMessage.jsx
@@ -4,12 +4,15 @@ import PropTypes from 'prop-types';
 import { Alert, Collapse } from 'react-bootstrap';
 
 const propTypes = {
-  message: PropTypes.string,
-  queryResponse: PropTypes.object,
+  message: PropTypes.node.isRequired,
+  link: PropTypes.string,
+  stackTrace: PropTypes.string,
   showStackTrace: PropTypes.bool,
 };
 const defaultProps = {
   showStackTrace: false,
+  link: null,
+  stackTrace: null,
 };
 
 class StackTraceMessage extends React.PureComponent {
@@ -21,25 +24,17 @@ class StackTraceMessage extends React.PureComponent {
     };
   }
 
-  hasTrace() {
-    return this.props.queryResponse && this.props.queryResponse.stacktrace;
-  }
-
-  hasLink() {
-    return this.props.queryResponse && this.props.queryResponse.link;
-  }
-
   render() {
     return (
-      <div className={`stack-trace-container${this.hasTrace() ? ' has-trace' : ''}`}>
+      <div className={`stack-trace-container${this.props.stackTrace ? ' has-trace' : ''}`}>
         <Alert
           bsStyle="warning"
           onClick={() => this.setState({ showStackTrace: !this.state.showStackTrace })}
         >
           {this.props.message}
-          {this.hasLink() &&
+          {this.props.link &&
           <a
-            href={this.props.queryResponse.link}
+            href={this.props.link}
             target="_blank"
             rel="noopener noreferrer"
           >
@@ -47,10 +42,10 @@ class StackTraceMessage extends React.PureComponent {
           </a>
        }
         </Alert>
-        {this.hasTrace() &&
+        {this.props.stackTrace &&
           <Collapse in={this.state.showStackTrace}>
             <pre>
-              {this.props.queryResponse.stacktrace}
+              {this.props.stackTrace}
             </pre>
           </Collapse>
         }
diff --git a/superset/assets/src/visualizations/TimeTable/TimeTable.jsx b/superset/assets/src/visualizations/TimeTable/TimeTable.jsx
index c414a7cbb..0eaafc7f6 100644
--- a/superset/assets/src/visualizations/TimeTable/TimeTable.jsx
+++ b/superset/assets/src/visualizations/TimeTable/TimeTable.jsx
@@ -226,7 +226,7 @@ class TimeTable extends React.PureComponent {
       .map(time => ({ ...data[time], time }));
     const reversedEntries = entries.concat().reverse();
 
-    const defaultSort = rowType === 'column' ? {
+    const defaultSort = rowType === 'column' && columnConfigs.length ? {
       column: columnConfigs[0].key,
       direction: 'desc',
     } : false;
diff --git a/superset/assets/src/visualizations/TimeTable/transformProps.js b/superset/assets/src/visualizations/TimeTable/transformProps.js
index d880faeb7..d52088b99 100644
--- a/superset/assets/src/visualizations/TimeTable/transformProps.js
+++ b/superset/assets/src/visualizations/TimeTable/transformProps.js
@@ -1,7 +1,7 @@
 export default function transformProps(chartProps) {
   const { height, datasource, formData, payload } = chartProps;
   const {
-    columnCollection,
+    columnCollection = 0,
     groupby,
     metrics,
     url,