fix: Log figure errors, don't show infinite spinner (#1614)

- Requires Core PR deephaven/deephaven-core#4763
  - Also need to port that to Enterprise
- Actually look at the errors reported by the figure, and log them so we
have a clearer idea of what the issue is when plots are reported as not
working
- Also show the error message in the chart panel button bar. Clicking
the button will display the full error message.
- Don't show the spinner infinitely if there are no series to load

---------

Co-authored-by: Matthew Runyon <mattrunyonstuff@gmail.com>

Author: mofojed

__mocks__/dh-core.js

@@ -1589,6 +1589,7 @@ class Figure extends DeephavenObject {
     updateSize = 10,
     rows = 1,
     cols = 1,
+    errors = [],
   } = {}) {
     super();
 
@@ -1603,6 +1604,7 @@ class Figure extends DeephavenObject {
     this.rowIndex = 0;
     this.rows = rows;
     this.cols = cols;
+    this.errors = errors;
   }
 
   addEventListener(...args) {

packages/chart/src/Chart.scss

@@ -108,3 +108,7 @@ $plotly-color-btn-active: rgba(255, 255, 255, 70%);
     }
   }
 }
+
+.chart-error-popper .popper-content {
+  padding: $spacer-1;
+}

packages/chart/src/Chart.tsx

@@ -1,6 +1,7 @@
 import React, { Component, ReactElement, RefObject } from 'react';
 import deepEqual from 'deep-equal';
 import memoize from 'memoize-one';
+import { CopyButton, Popper } from '@deephaven/components';
 import {
   vsLoading,
   dhGraphLineDown,
@@ -33,6 +34,7 @@ import Plotly from './plotly/Plotly';
 import ChartModel from './ChartModel';
 import ChartUtils, { ChartModelSettings } from './ChartUtils';
 import './Chart.scss';
+import DownsamplingError from './DownsamplingError';
 
 const log = Log.module('Chart');
 
@@ -56,10 +58,15 @@ interface ChartProps {
 
 interface ChartState {
   data: Partial<Data>[] | null;
+  /** An error specific to downsampling */
   downsamplingError: unknown;
   isDownsampleFinished: boolean;
   isDownsampleInProgress: boolean;
   isDownsamplingDisabled: boolean;
+
+  /** Any other kind of error */
+  error: unknown;
+  shownError: string | null;
   layout: Partial<Layout>;
   revision: number;
 }
@@ -129,6 +136,7 @@ export class Chart extends Component<ChartProps, ChartState> {
 
     this.handleAfterPlot = this.handleAfterPlot.bind(this);
     this.handleDownsampleClick = this.handleDownsampleClick.bind(this);
+    this.handleErrorClose = this.handleErrorClose.bind(this);
     this.handleModelEvent = this.handleModelEvent.bind(this);
     this.handlePlotUpdate = this.handlePlotUpdate.bind(this);
     this.handleRelayout = this.handleRelayout.bind(this);
@@ -153,6 +161,8 @@ export class Chart extends Component<ChartProps, ChartState> {
       isDownsampleFinished: false,
       isDownsampleInProgress: false,
       isDownsamplingDisabled: false,
+      error: null,
+      shownError: null,
       layout: {
         datarevision: 0,
       },
@@ -236,15 +246,30 @@ export class Chart extends Component<ChartProps, ChartState> {
       isDownsampleFinished: boolean,
       isDownsampleInProgress: boolean,
       isDownsamplingDisabled: boolean,
-      data: Partial<Data>[]
+      data: Partial<Data>[],
+      error: unknown
     ): Partial<PlotlyConfig> => {
       const customButtons: ModeBarButtonAny[] = [];
       const hasDownsampleError = Boolean(downsamplingError);
       if (hasDownsampleError) {
         customButtons.push({
           name: `Downsampling failed: ${downsamplingError}`,
           title: 'Downsampling failed',
-          click: () => undefined,
+          click: () => {
+            this.toggleErrorMessage(`${downsamplingError}`);
+          },
+          icon: Chart.convertIcon(dhWarningFilled),
+          attr: 'fill-warning',
+        });
+      }
+      const hasError = Boolean(error);
+      if (hasError) {
+        customButtons.push({
+          name: `Error: ${error}`,
+          title: `Error`,
+          click: () => {
+            this.toggleErrorMessage(`${error}`);
+          },
           icon: Chart.convertIcon(dhWarningFilled),
           attr: 'fill-warning',
         });
@@ -301,7 +326,7 @@ export class Chart extends Component<ChartProps, ChartState> {
         // Yes, the value is a boolean or the string 'hover': https://github.com/plotly/plotly.js/blob/master/src/plot_api/plot_config.js#L249
         displayModeBar:
           // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
-          isDownsampleInProgress || hasDownsampleError
+          isDownsampleInProgress || hasDownsampleError || hasError
             ? true
             : ('hover' as const),
 
@@ -376,6 +401,10 @@ export class Chart extends Component<ChartProps, ChartState> {
     );
   }
 
+  handleErrorClose(): void {
+    this.setState({ shownError: null });
+  }
+
   handleModelEvent(event: CustomEvent): void {
     const { type, detail } = event;
     log.debug2('Received data update', type, detail);
@@ -442,7 +471,14 @@ export class Chart extends Component<ChartProps, ChartState> {
         });
 
         const { onError } = this.props;
-        onError(new Error(downsamplingError));
+        onError(new DownsamplingError(downsamplingError));
+        break;
+      }
+      case ChartModel.EVENT_ERROR: {
+        const error = `${detail}`;
+        this.setState({ error });
+        const { onError } = this.props;
+        onError(new Error(error));
         break;
       }
       default:
@@ -502,6 +538,15 @@ export class Chart extends Component<ChartProps, ChartState> {
     }
   }
 
+  /**
+   * Toggle the error message. If it is already being displayed, then hide it.
+   */
+  toggleErrorMessage(error: string): void {
+    this.setState(({ shownError }) => ({
+      shownError: shownError === error ? null : error,
+    }));
+  }
+
   /**
    * Update the models dimensions and ranges.
    * Note that this will update it all whether the plot size changes OR the range
@@ -601,6 +646,8 @@ export class Chart extends Component<ChartProps, ChartState> {
       isDownsampleFinished,
       isDownsampleInProgress,
       isDownsamplingDisabled,
+      error,
+      shownError,
       layout,
       revision,
     } = this.state;
@@ -609,7 +656,8 @@ export class Chart extends Component<ChartProps, ChartState> {
       isDownsampleFinished,
       isDownsampleInProgress,
       isDownsamplingDisabled,
-      data ?? []
+      data ?? [],
+      error
     );
     const isPlotShown = data != null;
     return (
@@ -632,6 +680,23 @@ export class Chart extends Component<ChartProps, ChartState> {
             style={{ height: '100%', width: '100%' }}
           />
         )}
+        <Popper
+          className="chart-error-popper"
+          options={{ placement: 'top' }}
+          isShown={shownError != null}
+          onExited={this.handleErrorClose}
+          closeOnBlur
+          interactive
+        >
+          {shownError != null && (
+            <>
+              <div className="chart-error">{shownError}</div>
+              <CopyButton tooltip="Copy Error" copy={shownError}>
+                Copy Error
+              </CopyButton>
+            </>
+          )}
+        </Popper>
       </div>
     );
   }

packages/chart/src/ChartModel.ts

@@ -29,6 +29,8 @@ class ChartModel {
 
   static EVENT_LOADFINISHED = 'ChartModel.EVENT_LOADFINISHED';
 
+  static EVENT_ERROR = 'ChartModel.EVENT_ERROR';
+
   constructor(dh: DhType) {
     this.dh = dh;
     this.listeners = [];
@@ -161,6 +163,10 @@ class ChartModel {
   fireLoadFinished(): void {
     this.fireEvent(new CustomEvent(ChartModel.EVENT_LOADFINISHED));
   }
+
+  fireError(detail: string[]): void {
+    this.fireEvent(new CustomEvent(ChartModel.EVENT_ERROR, { detail }));
+  }
 }
 
 export default ChartModel;

packages/chart/src/ChartTestUtils.ts

@@ -131,9 +131,16 @@ class ChartTestUtils {
     charts = [this.makeChart()],
     rows = 1,
     cols = 1,
+    errors = [],
   } = {}): Figure {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    return new (this.dh as any).plot.Figure({ title, charts, rows, cols });
+    return new (this.dh as any).plot.Figure({
+      title,
+      charts,
+      rows,
+      cols,
+      errors,
+    });
   }
 }
 

packages/chart/src/DownsamplingError.ts

@@ -0,0 +1,5 @@
+export class DownsamplingError extends Error {
+  isDownsamplingError = true;
+}
+
+export default DownsamplingError;

packages/chart/src/FigureChartModel.test.ts

@@ -1,6 +1,6 @@
 import dh from '@deephaven/jsapi-shim';
 import { TestUtils } from '@deephaven/utils';
-import { PlotData } from 'plotly.js';
+import { Data } from 'plotly.js';
 import ChartTestUtils from './ChartTestUtils';
 import type { ChartTheme } from './ChartTheme';
 import FigureChartModel from './FigureChartModel';
@@ -464,8 +464,25 @@ it('adds new series', () => {
   ]);
 });
 
+it('emits finished loading if no series are added', () => {
+  const figure = chartTestUtils.makeFigure({
+    charts: [],
+  });
+  const model = new FigureChartModel(dh, figure, chartTheme);
+  const callback = jest.fn();
+  model.subscribe(callback);
+
+  jest.runOnlyPendingTimers();
+
+  expect(callback).toHaveBeenCalledWith(
+    expect.objectContaining({
+      type: FigureChartModel.EVENT_LOADFINISHED,
+    })
+  );
+});
+
 describe('legend visibility', () => {
-  function testLegend(showLegend: boolean | null): Partial<PlotData>[] {
+  function testLegend(showLegend: boolean | null): Partial<Data>[] {
     const series1 = chartTestUtils.makeSeries({ name: 'S1' });
     const chart = chartTestUtils.makeChart({ series: [series1], showLegend });
     const figure = chartTestUtils.makeFigure({

packages/chart/src/FigureChartModel.ts

@@ -266,6 +266,11 @@ class FigureChartModel extends ChartModel {
         ? this.dh.plot.DownsampleOptions.DISABLE
         : this.dh.plot.DownsampleOptions.DEFAULT
     );
+
+    if (this.figure.errors.length > 0) {
+      log.error('Errors in figure', this.figure.errors);
+      this.fireError(this.figure.errors);
+    }
   }
 
   unsubscribeFigure(): void {
@@ -459,19 +464,20 @@ class FigureChartModel extends ChartModel {
       }
 
       this.seriesToProcess.delete(series.name);
-      if (this.seriesToProcess.size === 0) {
-        this.fireLoadFinished();
-      }
 
       this.cleanSeries(series);
     }
+    if (this.seriesToProcess.size === 0) {
+      this.fireLoadFinished();
+    }
 
     const { data } = this;
     this.fireUpdate(data);
   }
 
   handleRequestFailed(event: ChartEvent): void {
     log.error('Request failed', event);
+    this.fireError([`${event.detail}`]);
   }
 
   /**

packages/chart/src/index.ts

@@ -3,6 +3,7 @@ export { default as ChartModelFactory } from './ChartModelFactory';
 export { default as ChartModel } from './ChartModel';
 export { default as ChartUtils } from './ChartUtils';
 export * from './ChartUtils';
+export * from './DownsamplingError';
 export { default as FigureChartModel } from './FigureChartModel';
 export { default as MockChartModel } from './MockChartModel';
 export { default as Plot } from './plotly/Plot';

packages/chart/tsconfig.json

@@ -7,23 +7,12 @@
   "include": ["src/**/*.ts", "src/**/*.tsx", "src/**/*.js", "src/**/*.jsx"],
   "exclude": ["node_modules", "src/**/*.test.*", "src/**/__mocks__/*"],
   "references": [
-    {
-      "path": "../components"
-    },
-    {
-      "path": "../jsapi-shim"
-    },
-    {
-      "path": "../jsapi-utils"
-    },
-    {
-      "path": "../log"
-    },
-    {
-      "path": "../react-hooks"
-    },
-    {
-      "path": "../utils"
-    }
+    { "path": "../components" },
+    { "path": "../jsapi-shim" },
+    { "path": "../jsapi-types" },
+    { "path": "../jsapi-utils" },
+    { "path": "../log" },
+    { "path": "../react-hooks" },
+    { "path": "../utils" }
   ]
 }