fix: Support deephaven-plugin-plotly-express 0.12.0 in v0.85 (#2335)

- Cherry-pick of #2174 - Needed for deephaven/deephaven-plugins#657 because when using ChartPanel `containerRef` is undefined on first render due to the loading logic. This means we need to allow a ref callback so we can use a callback to set state in the plugin and add event listeners to the container when it actually exists. --------- Co-authored-by: Matthew Runyon <[email protected]> Co-authored-by: Joe <[email protected]>
deephaven · Jan 8, 2025 · ce9d684 · ce9d684
1 parent 6480e1a
commit ce9d684
Show file tree

Hide file tree

Showing 11 changed files with 177 additions and 60 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/app-utils/src/plugins/remote-component.config.ts b/packages/app-utils/src/plugins/remote-component.config.ts
@@ -11,6 +11,7 @@ import * as AdobeReactSpectrum from '@adobe/react-spectrum';
 import * as DeephavenAuthPlugins from '@deephaven/auth-plugins';
 import * as DeephavenChart from '@deephaven/chart';
 import * as DeephavenComponents from '@deephaven/components';
+import * as DeephavenConsole from '@deephaven/console';
 import * as DeephavenDashboard from '@deephaven/dashboard';
 import * as DeephavenDashboardCorePlugins from '@deephaven/dashboard-core-plugins';
 import * as DeephavenIcons from '@deephaven/icons';
@@ -32,6 +33,7 @@ export const resolve = {
   '@deephaven/auth-plugins': DeephavenAuthPlugins,
   '@deephaven/chart': DeephavenChart,
   '@deephaven/components': DeephavenComponents,
+  '@deephaven/console': DeephavenConsole,
   '@deephaven/dashboard': DeephavenDashboard,
   '@deephaven/dashboard-core-plugins': DeephavenDashboardCorePlugins,
   '@deephaven/icons': DeephavenIcons,

diff --git a/packages/chart/src/Chart.tsx b/packages/chart/src/Chart.tsx
@@ -28,6 +28,7 @@ import {
   ModeBarButtonAny,
 } from 'plotly.js';
 import type { PlotParams } from 'react-plotly.js';
+import { mergeRefs } from '@deephaven/react-hooks';
 import { bindAllMethods } from '@deephaven/utils';
 import createPlotlyComponent from './plotly/createPlotlyComponent';
 import Plotly from './plotly/Plotly';
@@ -57,7 +58,7 @@ interface ChartProps {
 
   isActive: boolean;
   Plotly: typeof Plotly;
-  containerRef?: React.RefObject<HTMLDivElement>;
+  containerRef?: React.Ref<HTMLDivElement>;
   onDisconnect: () => void;
   onReconnect: () => void;
   onUpdate: (obj: { isLoading: boolean }) => void;
@@ -81,11 +82,14 @@ interface ChartState {
   isDownsampleInProgress: boolean;
   isDownsamplingDisabled: boolean;
 
-  /** Any other kind of error */
+  /** Any other kind of error that doesn't completely block the chart from rendering */
   error: unknown;
   shownError: string | null;
   layout: Partial<Layout>;
   revision: number;
+
+  /** A message that blocks the chart from rendering. It can be bypassed by the user to continue rendering.  */
+  shownBlocker: string | null;
 }
 
 class Chart extends Component<ChartProps, ChartState> {
@@ -156,7 +160,8 @@ class Chart extends Component<ChartProps, ChartState> {
 
     this.PlotComponent = createPlotlyComponent(props.Plotly);
     this.plot = React.createRef();
-    this.plotWrapper = props.containerRef ?? React.createRef();
+    this.plotWrapper = React.createRef();
+    this.plotWrapperMerged = mergeRefs(this.plotWrapper, props.containerRef);
     this.columnFormats = [];
     this.dateTimeFormatterOptions = {};
     this.decimalFormatOptions = {};
@@ -178,6 +183,7 @@ class Chart extends Component<ChartProps, ChartState> {
         datarevision: 0,
       },
       revision: 0,
+      shownBlocker: null,
     };
   }
 
@@ -238,6 +244,8 @@ class Chart extends Component<ChartProps, ChartState> {
 
   plotWrapper: RefObject<HTMLDivElement>;
 
+  plotWrapperMerged: React.RefCallback<HTMLDivElement>;
+
   columnFormats?: FormattingRule[];
 
   dateTimeFormatterOptions?: DateTimeColumnFormatterOptions;
@@ -508,6 +516,15 @@ class Chart extends Component<ChartProps, ChartState> {
         onError(new Error(error));
         break;
       }
+      case ChartModel.EVENT_BLOCKER: {
+        const blocker = `${detail}`;
+        this.setState({ shownBlocker: blocker });
+        break;
+      }
+      case ChartModel.EVENT_BLOCKER_CLEAR: {
+        this.setState({ shownBlocker: null });
+        break;
+      }
       default:
         log.debug('Unknown event type', type, event);
     }
@@ -701,6 +718,7 @@ class Chart extends Component<ChartProps, ChartState> {
       shownError,
       layout,
       revision,
+      shownBlocker,
     } = this.state;
     const config = this.getCachedConfig(
       downsamplingError,
@@ -710,10 +728,49 @@ class Chart extends Component<ChartProps, ChartState> {
       data ?? [],
       error
     );
-    const isPlotShown = data != null;
+    const { model } = this.props;
+    const isPlotShown = data != null && shownBlocker == null;
+
+    let errorOverlay: React.ReactNode = null;
+    if (shownBlocker != null) {
+      errorOverlay = (
+        <ChartErrorOverlay
+          errorMessage={`${shownBlocker}`}
+          onConfirm={() => {
+            model.fireBlockerClear();
+          }}
+        />
+      );
+    } else if (shownError != null) {
+      errorOverlay = (
+        <ChartErrorOverlay
+          errorMessage={`${downsamplingError}`}
+          onDiscard={() => {
+            this.handleDownsampleErrorClose();
+          }}
+          onConfirm={() => {
+            this.handleDownsampleErrorClose();
+            this.handleDownsampleClick();
+          }}
+        />
+      );
+    } else if (downsamplingError != null) {
+      errorOverlay = (
+        <ChartErrorOverlay
+          errorMessage={`${downsamplingError}`}
+          onDiscard={() => {
+            this.handleDownsampleErrorClose();
+          }}
+          onConfirm={() => {
+            this.handleDownsampleErrorClose();
+            this.handleDownsampleClick();
+          }}
+        />
+      );
+    }
 
     return (
-      <div className="h-100 w-100 chart-wrapper" ref={this.plotWrapper}>
+      <div className="h-100 w-100 chart-wrapper" ref={this.plotWrapperMerged}>
         {isPlotShown && (
           <PlotComponent
             // eslint-disable-next-line @typescript-eslint/ban-ts-comment
@@ -731,26 +788,7 @@ class Chart extends Component<ChartProps, ChartState> {
             style={{ height: '100%', width: '100%' }}
           />
         )}
-        {downsamplingError != null && shownError == null && (
-          <ChartErrorOverlay
-            errorMessage={`${downsamplingError}`}
-            onDiscard={() => {
-              this.handleDownsampleErrorClose();
-            }}
-            onConfirm={() => {
-              this.handleDownsampleErrorClose();
-              this.handleDownsampleClick();
-            }}
-          />
-        )}
-        {shownError != null && (
-          <ChartErrorOverlay
-            errorMessage={`${shownError}`}
-            onDiscard={() => {
-              this.handleErrorClose();
-            }}
-          />
-        )}
+        {errorOverlay}
       </div>
     );
   }

diff --git a/packages/chart/src/ChartModel.ts b/packages/chart/src/ChartModel.ts
@@ -37,6 +37,10 @@ class ChartModel {
 
   static EVENT_ERROR = 'ChartModel.EVENT_ERROR';
 
+  static EVENT_BLOCKER = 'ChartModel.EVENT_BLOCKER';
+
+  static EVENT_BLOCKER_CLEAR = 'ChartModel.EVENT_BLOCKER_CLEAR';
+
   constructor(dh: typeof DhType) {
     this.dh = dh;
     this.listeners = [];
@@ -184,6 +188,14 @@ class ChartModel {
   fireError(detail: string[]): void {
     this.fireEvent(new CustomEvent(ChartModel.EVENT_ERROR, { detail }));
   }
+
+  fireBlocker(detail: string[]): void {
+    this.fireEvent(new CustomEvent(ChartModel.EVENT_BLOCKER, { detail }));
+  }
+
+  fireBlockerClear(): void {
+    this.fireEvent(new CustomEvent(ChartModel.EVENT_BLOCKER_CLEAR));
+  }
 }
 
 export default ChartModel;
diff --git a/packages/components/src/spectrum/comboBox/ComboBox.tsx b/packages/components/src/spectrum/comboBox/ComboBox.tsx
@@ -5,9 +5,9 @@ import {
 } from '@adobe/react-spectrum';
 import type { DOMRef } from '@react-types/shared';
 import cl from 'classnames';
+import { useMergeRef } from '@deephaven/react-hooks';
 import type { NormalizedItem } from '../utils';
 import { PickerPropsT, usePickerProps } from '../picker';
-import useMultiRef from '../picker/useMultiRef';
 
 export type ComboBoxProps = PickerPropsT<SpectrumComboBoxProps<NormalizedItem>>;
 
@@ -22,7 +22,7 @@ export const ComboBox = React.forwardRef(function ComboBox(
     ref: scrollRef,
     ...comboBoxProps
   } = usePickerProps(props);
-  const pickerRef = useMultiRef(ref, scrollRef);
+  const pickerRef = useMergeRef(ref, scrollRef);
   return (
     <SpectrumComboBox
       // eslint-disable-next-line react/jsx-props-no-spreading

diff --git a/packages/components/src/spectrum/picker/Picker.tsx b/packages/components/src/spectrum/picker/Picker.tsx
@@ -5,9 +5,9 @@ import {
 import type { DOMRef } from '@react-types/shared';
 import cl from 'classnames';
 import React from 'react';
+import { useMergeRef } from '@deephaven/react-hooks';
 import type { NormalizedItem } from '../utils';
 import type { PickerProps } from './PickerProps';
-import useMultiRef from './useMultiRef';
 import { usePickerProps } from './usePickerProps';
 
 /**
@@ -28,7 +28,7 @@ export const Picker = React.forwardRef(function Picker(
     ref: scrollRef,
     ...pickerProps
   } = usePickerProps(props);
-  const pickerRef = useMultiRef(ref, scrollRef);
+  const pickerRef = useMergeRef(ref, scrollRef);
   return (
     <SpectrumPicker
       // eslint-disable-next-line react/jsx-props-no-spreading

diff --git a/packages/components/src/spectrum/picker/useMultiRef.ts b/packages/components/src/spectrum/picker/useMultiRef.ts
diff --git a/packages/dashboard-core-plugins/src/panels/ChartPanel.tsx b/packages/dashboard-core-plugins/src/panels/ChartPanel.tsx
@@ -123,8 +123,11 @@ interface OwnProps extends DashboardPanelProps {
   makeModel: () => Promise<ChartModel>;
   localDashboardId: string;
   Plotly?: typeof PlotlyType;
-  /** The plot container div */
-  containerRef?: RefObject<HTMLDivElement>;
+  /**
+   * The plot container div.
+   * The ref will be undefined on initial render if the chart needs to be loaded.
+   */
+  containerRef?: React.Ref<HTMLDivElement>;
 
   panelState?: GLChartPanelState;
 }

diff --git a/packages/react-hooks/src/index.ts b/packages/react-hooks/src/index.ts
@@ -34,3 +34,4 @@ export * from './useSetAttributesCallback';
 export * from './useSpectrumDisableSpellcheckRef';
 export * from './useWindowedListData';
 export * from './useResizeObserver';
+export * from './useMergeRef';
diff --git a/...s/src/spectrum/picker/useMultiRef.test.ts → packages/react-hooks/src/useMergeRef.test.ts b/...s/src/spectrum/picker/useMultiRef.test.ts → packages/react-hooks/src/useMergeRef.test.ts
@@ -1,12 +1,52 @@
+import React from 'react';
 import { renderHook } from '@testing-library/react-hooks';
-import useMultiRef from './useMultiRef';
+import { useMergeRef, mergeRefs } from './useMergeRef';
 
-describe('useMultiRef', () => {
+describe('mergeRefs', () => {
+  it('merges ref objects', () => {
+    const refA = React.createRef();
+    const refB = React.createRef();
+    const mergedRef = mergeRefs(refA, refB);
+
+    const refValue = {};
+    mergedRef(refValue);
+    expect(refA.current).toBe(refValue);
+    expect(refB.current).toBe(refValue);
+  });
+
+  it('merges ref callbacks', () => {
+    const refA: React.RefCallback<unknown> = jest.fn();
+    const refB: React.RefCallback<unknown> = jest.fn();
+    const mergedRef = mergeRefs(refA, refB);
+
+    const refValue = {};
+    mergedRef(refValue);
+    expect(refA).toHaveBeenCalledWith(refValue);
+    expect(refB).toHaveBeenCalledWith(refValue);
+  });
+
+  it('ignores null/undefined refs', () => {
+    const refA = React.createRef();
+    const refB: React.RefCallback<unknown> = jest.fn();
+    const refC = null;
+    const refD = undefined;
+    const mergedRef = mergeRefs(refA, refB, refC, refD);
+
+    const refValue = {};
+    mergedRef(refValue);
+    expect(refA.current).toBe(refValue);
+    expect(refB).toHaveBeenCalledWith(refValue);
+    expect(refC).toBe(null);
+    expect(refD).toBe(undefined);
+  });
+});
+
+describe('useMergeRef', () => {
   it('should assign the ref to all refs passed in', () => {
     const ref1 = jest.fn();
     const ref2 = jest.fn();
     const ref3 = jest.fn();
-    const { result } = renderHook(() => useMultiRef(ref1, ref2, ref3));
+    const { result } = renderHook(() => useMergeRef(ref1, ref2, ref3));
     const multiRef = result.current;
     const element = document.createElement('div');
     multiRef(element);
@@ -19,7 +59,7 @@ describe('useMultiRef', () => {
     const ref1 = jest.fn();
     const ref2 = jest.fn();
     const ref3 = jest.fn();
-    const { result } = renderHook(() => useMultiRef(ref1, ref2, ref3));
+    const { result } = renderHook(() => useMergeRef(ref1, ref2, ref3));
     const multiRef = result.current;
     multiRef(null);
     expect(ref1).toHaveBeenCalledWith(null);
@@ -32,7 +72,7 @@ describe('useMultiRef', () => {
     const ref2 = { current: null };
     const ref3 = { current: null };
     const { result } = renderHook(() =>
-      useMultiRef<HTMLDivElement | null>(ref1, ref2, ref3)
+      useMergeRef<HTMLDivElement | null>(ref1, ref2, ref3)
     );
     const multiRef = result.current;
     const element = document.createElement('div');
@@ -47,7 +87,7 @@ describe('useMultiRef', () => {
     const ref2 = { current: null };
     const ref3 = jest.fn();
     const { result } = renderHook(() =>
-      useMultiRef<HTMLDivElement | null>(ref1, ref2, ref3)
+      useMergeRef<HTMLDivElement | null>(ref1, ref2, ref3)
     );
     const multiRef = result.current;
     const element = document.createElement('div');