From 75783d0ed96e9e28214ca8681a73f23b1dc78085 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Fri, 10 Nov 2023 14:17:17 -0500 Subject: [PATCH] fix: Log figure errors, don't show infinite spinner (#1614) - Requires Core PR https://github.com/deephaven/deephaven-core/pull/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 --- __mocks__/dh-core.js | 2 + packages/chart/src/Chart.scss | 4 ++ packages/chart/src/Chart.tsx | 75 +++++++++++++++++++-- packages/chart/src/ChartModel.ts | 6 ++ packages/chart/src/ChartTestUtils.ts | 9 ++- packages/chart/src/DownsamplingError.ts | 5 ++ packages/chart/src/FigureChartModel.test.ts | 21 +++++- packages/chart/src/FigureChartModel.ts | 12 +++- packages/chart/src/index.ts | 1 + packages/chart/tsconfig.json | 25 ++----- packages/jsapi-types/src/dh.types.ts | 1 + 11 files changed, 132 insertions(+), 29 deletions(-) create mode 100644 packages/chart/src/DownsamplingError.ts diff --git a/__mocks__/dh-core.js b/__mocks__/dh-core.js index d28c4b22e7..35bd50b73b 100644 --- a/__mocks__/dh-core.js +++ b/__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) { diff --git a/packages/chart/src/Chart.scss b/packages/chart/src/Chart.scss index 745b158bef..19d101fb5e 100644 --- a/packages/chart/src/Chart.scss +++ b/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; +} diff --git a/packages/chart/src/Chart.tsx b/packages/chart/src/Chart.tsx index b79e4d0d4a..5a403f9a02 100644 --- a/packages/chart/src/Chart.tsx +++ b/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[] | 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; revision: number; } @@ -129,6 +136,7 @@ export class Chart extends Component { 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 { isDownsampleFinished: false, isDownsampleInProgress: false, isDownsamplingDisabled: false, + error: null, + shownError: null, layout: { datarevision: 0, }, @@ -236,7 +246,8 @@ export class Chart extends Component { isDownsampleFinished: boolean, isDownsampleInProgress: boolean, isDownsamplingDisabled: boolean, - data: Partial[] + data: Partial[], + error: unknown ): Partial => { const customButtons: ModeBarButtonAny[] = []; const hasDownsampleError = Boolean(downsamplingError); @@ -244,7 +255,21 @@ export class Chart extends Component { 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 { // 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 { ); } + 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 { }); 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 { } } + /** + * 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 { isDownsampleFinished, isDownsampleInProgress, isDownsamplingDisabled, + error, + shownError, layout, revision, } = this.state; @@ -609,7 +656,8 @@ export class Chart extends Component { isDownsampleFinished, isDownsampleInProgress, isDownsamplingDisabled, - data ?? [] + data ?? [], + error ); const isPlotShown = data != null; return ( @@ -632,6 +680,23 @@ export class Chart extends Component { style={{ height: '100%', width: '100%' }} /> )} + + {shownError != null && ( + <> +
{shownError}
+ + Copy Error + + + )} + ); } diff --git a/packages/chart/src/ChartModel.ts b/packages/chart/src/ChartModel.ts index d5cda1a005..6d6365c39a 100644 --- a/packages/chart/src/ChartModel.ts +++ b/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; diff --git a/packages/chart/src/ChartTestUtils.ts b/packages/chart/src/ChartTestUtils.ts index 9c097d382d..406f15a199 100644 --- a/packages/chart/src/ChartTestUtils.ts +++ b/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, + }); } } diff --git a/packages/chart/src/DownsamplingError.ts b/packages/chart/src/DownsamplingError.ts new file mode 100644 index 0000000000..7a680e69bb --- /dev/null +++ b/packages/chart/src/DownsamplingError.ts @@ -0,0 +1,5 @@ +export class DownsamplingError extends Error { + isDownsamplingError = true; +} + +export default DownsamplingError; diff --git a/packages/chart/src/FigureChartModel.test.ts b/packages/chart/src/FigureChartModel.test.ts index 18857886c5..2993f981f9 100644 --- a/packages/chart/src/FigureChartModel.test.ts +++ b/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[] { + function testLegend(showLegend: boolean | null): Partial[] { const series1 = chartTestUtils.makeSeries({ name: 'S1' }); const chart = chartTestUtils.makeChart({ series: [series1], showLegend }); const figure = chartTestUtils.makeFigure({ diff --git a/packages/chart/src/FigureChartModel.ts b/packages/chart/src/FigureChartModel.ts index 42dbfcbf1c..8b04260398 100644 --- a/packages/chart/src/FigureChartModel.ts +++ b/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,12 +464,12 @@ 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); @@ -472,6 +477,7 @@ class FigureChartModel extends ChartModel { handleRequestFailed(event: ChartEvent): void { log.error('Request failed', event); + this.fireError([`${event.detail}`]); } /** diff --git a/packages/chart/src/index.ts b/packages/chart/src/index.ts index 0959cc5d2f..3693c39bc9 100644 --- a/packages/chart/src/index.ts +++ b/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'; diff --git a/packages/chart/tsconfig.json b/packages/chart/tsconfig.json index ea693b3b92..f1431b1167 100644 --- a/packages/chart/tsconfig.json +++ b/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" } ] } diff --git a/packages/jsapi-types/src/dh.types.ts b/packages/jsapi-types/src/dh.types.ts index ab2318b9a8..ccb020395a 100644 --- a/packages/jsapi-types/src/dh.types.ts +++ b/packages/jsapi-types/src/dh.types.ts @@ -330,6 +330,7 @@ export interface Figure extends Evented { readonly cols: number; readonly rows: number; readonly charts: Chart[]; + readonly errors: string[]; /** * Subscribes to all series in this figure.