fix: ChartBuilderPlugin fixes for charts built from PPQs in Enterprise (#2167)

mofojed · web-flow · commit 99b8d5952ba3 · 2024-09-11T09:59:00.000-04:00
- Don't sanitize the descriptor in `AppDashboards` - the descriptor gets sanitized within the objectFetcher itself - By sanitizing too early, we lose metadata needed to load an object - ChartPanelPlugin uses the `panelFetch` to get the underlying object - In cases of a `ParameterizedQuery` on Enterprise, we only have the result of the `ParameterizedQuery` run in the `ParameterizedQueryPanel` - Aside, if we're looking at improving `ParameterizedQuery` support, we should complete DH-15760 (which would be breaking an internal API) - Use correct API when fetching a Chart object
diff --git a/packages/app-utils/src/components/AppDashboards.tsx b/packages/app-utils/src/components/AppDashboards.tsx
@@ -5,10 +5,7 @@ import {
   DehydratedDashboardPanelProps,
   LazyDashboard,
 } from '@deephaven/dashboard';
-import {
-  sanitizeVariableDescriptor,
-  useObjectFetcher,
-} from '@deephaven/jsapi-bootstrap';
+import { useObjectFetcher } from '@deephaven/jsapi-bootstrap';
 import LayoutManager, {
   ItemConfig,
   Settings as LayoutSettings,
@@ -44,9 +41,8 @@ export function AppDashboards({
       const { metadata } = hydrateProps;
       try {
         if (metadata != null) {
-          const widget = sanitizeVariableDescriptor(metadata);
           return {
-            fetch: async () => fetchObject(widget),
+            fetch: async () => fetchObject(metadata),
             ...hydrateProps,
             localDashboardId: id,
           };
diff --git a/packages/dashboard-core-plugins/src/ChartBuilderPlugin.tsx b/packages/dashboard-core-plugins/src/ChartBuilderPlugin.tsx
@@ -1,17 +1,11 @@
 import { useCallback } from 'react';
-import {
-  ChartModel,
-  ChartModelFactory,
-  ChartModelSettings,
-  ChartUtils,
-} from '@deephaven/chart';
+import { ChartModelSettings, ChartUtils } from '@deephaven/chart';
 import {
   assertIsDashboardPluginProps,
   DashboardPluginComponentProps,
   LayoutUtils,
   useListener,
 } from '@deephaven/dashboard';
-import { useApi } from '@deephaven/jsapi-bootstrap';
 import type { dh } from '@deephaven/jsapi-types';
 import { nanoid } from 'nanoid';
 import { IrisGridEvent } from './events';
@@ -28,7 +22,6 @@ export function ChartBuilderPlugin(
 ): JSX.Element | null {
   assertIsDashboardPluginProps(props);
   const { id, layout } = props;
-  const dh = useApi();
 
   const handleCreateChart = useCallback(
     ({
@@ -45,8 +38,7 @@ export function ChartBuilderPlugin(
       table: dh.Table;
     }) => {
       const { settings } = metadata;
-      const makeModel = (): Promise<ChartModel> =>
-        ChartModelFactory.makeModelFromSettings(dh, settings, table);
+      const fetchTable = async () => table;
       const title = ChartUtils.titleFromSettings(settings);
 
       const config = {
@@ -56,7 +48,7 @@ export function ChartBuilderPlugin(
           localDashboardId: id,
           id: panelId,
           metadata,
-          makeModel,
+          fetch: fetchTable,
         },
         title,
         id: panelId,
@@ -65,7 +57,7 @@ export function ChartBuilderPlugin(
       const { root } = layout;
       LayoutUtils.openComponent({ root, config });
     },
-    [dh, id, layout]
+    [id, layout]
   );
 
   useListener(layout.eventHub, IrisGridEvent.CREATE_CHART, handleCreateChart);
diff --git a/packages/dashboard-core-plugins/src/ChartPanelPlugin.tsx b/packages/dashboard-core-plugins/src/ChartPanelPlugin.tsx
@@ -1,19 +1,15 @@
-import { forwardRef, useMemo } from 'react';
-import {
-  ObjectFetcher,
-  useApi,
-  useObjectFetcher,
-} from '@deephaven/jsapi-bootstrap';
+import { forwardRef, useCallback } from 'react';
+import { useDeferredApi } from '@deephaven/jsapi-bootstrap';
 import { ChartModel, ChartModelFactory } from '@deephaven/chart';
 import type { dh as DhType } from '@deephaven/jsapi-types';
 import { IrisGridUtils } from '@deephaven/iris-grid';
 import { getTimeZone, store } from '@deephaven/redux';
 import { WidgetPanelProps } from '@deephaven/plugin';
+import { assertNotNull } from '@deephaven/utils';
 import {
   ChartPanelMetadata,
   GLChartPanelState,
   isChartPanelDehydratedProps,
-  isChartPanelFigureMetadata,
   isChartPanelTableMetadata,
 } from './panels';
 import ConnectedChartPanel, {
@@ -23,28 +19,18 @@ import ConnectedChartPanel, {
 
 async function createChartModel(
   dh: typeof DhType,
-  fetchObject: ObjectFetcher,
   metadata: ChartPanelMetadata,
-  fetchFigure: () => Promise<DhType.plot.Figure>,
+  panelFetch: () => Promise<DhType.plot.Figure | DhType.Table>,
   panelState?: GLChartPanelState
 ): Promise<ChartModel> {
-  let settings;
-  let tableName;
-  let figureName;
-  let tableSettings;
+  let settings = {};
+  let tableName = '';
+  let tableSettings = {};
 
   if (isChartPanelTableMetadata(metadata)) {
     settings = metadata.settings;
     tableName = metadata.table;
-    figureName = undefined;
     tableSettings = metadata.tableSettings;
-  } else {
-    settings = {};
-    tableName = '';
-    figureName = isChartPanelFigureMetadata(metadata)
-      ? metadata.figure
-      : metadata.name;
-    tableSettings = {};
   }
   if (panelState != null) {
     if (panelState.tableSettings != null) {
@@ -53,9 +39,6 @@ async function createChartModel(
     if (panelState.table != null) {
       tableName = panelState.table;
     }
-    if (panelState.figure != null) {
-      figureName = panelState.figure;
-    }
     if (panelState.settings != null) {
       settings = {
         ...settings,
@@ -64,83 +47,59 @@ async function createChartModel(
     }
   }
 
-  if (figureName == null && tableName == null) {
-    const figure = await fetchFigure();
-
-    return ChartModelFactory.makeModel(dh, settings, figure);
-  }
-
-  if (figureName != null) {
-    let figure: DhType.plot.Figure;
-
-    if (metadata.type === dh.VariableType.FIGURE) {
-      const descriptor = {
-        ...metadata,
-        name: figureName,
-        type: dh.VariableType.FIGURE,
-      };
-      figure = await fetchObject<DhType.plot.Figure>(descriptor);
-    } else {
-      figure = await fetchFigure();
-    }
+  if (tableName != null && tableName !== '') {
+    const table = (await panelFetch()) as DhType.Table;
+    new IrisGridUtils(dh).applyTableSettings(
+      table,
+      tableSettings,
+      getTimeZone(store.getState())
+    );
 
-    return ChartModelFactory.makeModel(dh, settings, figure);
+    return ChartModelFactory.makeModelFromSettings(dh, settings, table);
   }
 
-  const descriptor = {
-    ...metadata,
-    name: tableName,
-    type: dh.VariableType.TABLE,
-  };
-  const table = await fetchObject<DhType.Table>(descriptor);
-  new IrisGridUtils(dh).applyTableSettings(
-    table,
-    tableSettings,
-    getTimeZone(store.getState())
-  );
+  const figure = (await panelFetch()) as DhType.plot.Figure;
 
-  return ChartModelFactory.makeModelFromSettings(
-    dh,
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    settings as any,
-    table
-  );
+  return ChartModelFactory.makeModel(dh, settings, figure);
 }
 
 export const ChartPanelPlugin = forwardRef(
   (props: WidgetPanelProps<DhType.plot.Figure>, ref: React.Ref<ChartPanel>) => {
-    const dh = useApi();
-    const fetchObject = useObjectFetcher();
-
     const panelState = isChartPanelDehydratedProps(props)
       ? (props as unknown as ChartPanelProps).panelState
       : undefined;
 
     const { fetch: panelFetch, metadata, localDashboardId } = props;
-
-    const hydratedProps = useMemo(
-      () => ({
-        metadata: metadata as ChartPanelMetadata,
-        localDashboardId,
-        makeModel: () => {
-          if (metadata == null) {
-            throw new Error('Metadata is required for chart panel');
-          }
-
-          return createChartModel(
-            dh,
-            fetchObject,
-            metadata as ChartPanelMetadata,
-            panelFetch,
-            panelState
-          );
-        },
-      }),
-      [metadata, localDashboardId, dh, fetchObject, panelFetch, panelState]
+    assertNotNull(metadata);
+    const [dh, error] = useDeferredApi(metadata);
+
+    const makeModel = useCallback(async () => {
+      if (error != null) {
+        throw error;
+      }
+      if (dh == null) {
+        return new Promise<ChartModel>(() => {
+          // We don't have the API yet, just return an unresolved promise so it shows as loading
+        });
+      }
+      return createChartModel(
+        dh,
+        metadata as ChartPanelMetadata,
+        panelFetch,
+        panelState
+      );
+    }, [dh, error, metadata, panelFetch, panelState]);
+
+    return (
+      <ConnectedChartPanel
+        ref={ref}
+        // eslint-disable-next-line react/jsx-props-no-spreading
+        {...props}
+        metadata={metadata}
+        localDashboardId={localDashboardId}
+        makeModel={makeModel}
+      />
     );
-
-    // eslint-disable-next-line react/jsx-props-no-spreading
-    return <ConnectedChartPanel ref={ref} {...props} {...hydratedProps} />;
   }
 );
 
diff --git a/packages/dashboard-core-plugins/src/panels/ChartPanel.tsx b/packages/dashboard-core-plugins/src/panels/ChartPanel.tsx
@@ -6,8 +6,8 @@ import debounce from 'lodash.debounce';
 import {
   Chart,
   ChartModel,
+  ChartModelFactory,
   ChartModelSettings,
-  ChartUtils,
   FilterMap,
   isFigureChartModel,
 } from '@deephaven/chart';
@@ -286,14 +286,14 @@ export class ChartPanel extends Component<ChartPanelProps, ChartPanelState> {
     const { columnMap, model, filterMap, filterValueMap, isLinked, settings } =
       this.state;
 
-    if (!model) {
-      return;
-    }
-
     if (makeModel !== prevProps.makeModel) {
       this.initModel();
     }
 
+    if (model == null) {
+      return;
+    }
+
     if (columnMap !== prevState.columnMap) {
       this.pruneFilterMaps();
     }
@@ -352,6 +352,8 @@ export class ChartPanel extends Component<ChartPanelProps, ChartPanelState> {
 
     const { makeModel } = this.props;
 
+    this.pending.cancel();
+
     this.pending
       .add(makeModel(), resolved => {
         resolved.close();
@@ -651,12 +653,8 @@ export class ChartPanel extends Component<ChartPanelProps, ChartPanelState> {
       const { settings } = metadata;
       this.pending
         .add(
-          dh.plot.Figure.create(
-            new ChartUtils(dh).makeFigureSettings(
-              settings,
-              source
-            ) as unknown as dh.plot.FigureDescriptor
-          )
+          ChartModelFactory.makeFigureFromSettings(dh, settings, source),
+          resolved => resolved.close()
         )
         .then(figure => {
           if (isFigureChartModel(model)) {
diff --git a/packages/dashboard-core-plugins/src/panels/WidgetPanel.tsx b/packages/dashboard-core-plugins/src/panels/WidgetPanel.tsx
@@ -1,9 +1,9 @@
 import React, { PureComponent, ReactElement, ReactNode } from 'react';
 import classNames from 'classnames';
 import memoize from 'memoize-one';
+import { ContextActions, createXComponent } from '@deephaven/components';
 import { PanelComponent } from '@deephaven/dashboard';
 import type { Container, EventEmitter } from '@deephaven/golden-layout';
-import { ContextActions, createXComponent } from '@deephaven/components';
 import { copyToClipboard } from '@deephaven/utils';
 import Panel from './Panel';
 import WidgetPanelTooltip from './WidgetPanelTooltip';
diff --git a/packages/jsapi-bootstrap/src/useDeferredApi.test.ts b/packages/jsapi-bootstrap/src/useDeferredApi.test.ts
@@ -29,7 +29,10 @@ describe('useDeferredApi', () => {
     expect(result.current).toEqual([dh1, null]);
 
     const { result: result2 } = renderHook(() =>
-      useDeferredApi({ type: 'foo', foo: 'bar' })
+      useDeferredApi({
+        type: 'foo',
+        foo: 'bar',
+      } as DhType.ide.VariableDescriptor)
     );
     expect(result2.current).toEqual([dh1, null]);
   });
@@ -68,4 +71,10 @@ describe('useDeferredApi', () => {
     const { result } = renderHook(() => useDeferredApi(objectMetadata));
     expect(result.current).toEqual([null, expect.any(Error)]);
   });
+
+  it('returns an error if the metadata is null', async () => {
+    asMock(useContext).mockReturnValue(dh1);
+    const { result } = renderHook(() => useDeferredApi(null));
+    expect(result.current).toEqual([null, expect.any(Error)]);
+  });
 });
diff --git a/packages/jsapi-bootstrap/src/useDeferredApi.ts b/packages/jsapi-bootstrap/src/useDeferredApi.ts
@@ -24,8 +24,8 @@ export const DeferredApiContext = createContext<
  * @returns A tuple with the API instance, and an error if one occurred.
  */
 export function useDeferredApi(
-  widget: DhType.ide.VariableDescriptor
-): [typeof DhType | null, unknown | null] {
+  widget: DhType.ide.VariableDescriptor | null
+): [dh: typeof DhType | null, error: unknown | null] {
   const [api, setApi] = useState<typeof DhType | null>(null);
   const [error, setError] = useState<unknown | null>(null);
   const deferredApi = useContext(DeferredApiContext);
@@ -49,7 +49,12 @@ export function useDeferredApi(
     let isCancelled = false;
 
     async function loadApi() {
-      if (typeof deferredApi === 'function') {
+      if (widget == null) {
+        if (!isCancelled) {
+          setApi(null);
+          setError(new Error('No widget provided to useDeferredApi'));
+        }
+      } else if (typeof deferredApi === 'function') {
         try {
           const newApi = await deferredApi(widget);
           if (!isCancelled) {
