Skip to content

Commit

Permalink
fix: ChartBuilderPlugin fixes for charts built from PPQs in Enterprise (
Browse files Browse the repository at this point in the history
#2167)

- 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
  • Loading branch information
mofojed authored Sep 11, 2024
1 parent d78ad6d commit 99b8d59
Show file tree
Hide file tree
Showing 7 changed files with 80 additions and 121 deletions.
8 changes: 2 additions & 6 deletions packages/app-utils/src/components/AppDashboards.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
};
Expand Down
16 changes: 4 additions & 12 deletions packages/dashboard-core-plugins/src/ChartBuilderPlugin.tsx
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -28,7 +22,6 @@ export function ChartBuilderPlugin(
): JSX.Element | null {
assertIsDashboardPluginProps(props);
const { id, layout } = props;
const dh = useApi();

const handleCreateChart = useCallback(
({
Expand All @@ -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 = {
Expand All @@ -56,7 +48,7 @@ export function ChartBuilderPlugin(
localDashboardId: id,
id: panelId,
metadata,
makeModel,
fetch: fetchTable,
},
title,
id: panelId,
Expand All @@ -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);
Expand Down
133 changes: 46 additions & 87 deletions packages/dashboard-core-plugins/src/ChartPanelPlugin.tsx
Original file line number Diff line number Diff line change
@@ -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, {
Expand All @@ -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) {
Expand All @@ -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,
Expand All @@ -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} />;
}
);

Expand Down
20 changes: 9 additions & 11 deletions packages/dashboard-core-plugins/src/panels/ChartPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import debounce from 'lodash.debounce';
import {
Chart,
ChartModel,
ChartModelFactory,
ChartModelSettings,
ChartUtils,
FilterMap,
isFigureChartModel,
} from '@deephaven/chart';
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)) {
Expand Down
2 changes: 1 addition & 1 deletion packages/dashboard-core-plugins/src/panels/WidgetPanel.tsx
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
11 changes: 10 additions & 1 deletion packages/jsapi-bootstrap/src/useDeferredApi.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
});
Expand Down Expand Up @@ -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)]);
});
});
11 changes: 8 additions & 3 deletions packages/jsapi-bootstrap/src/useDeferredApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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) {
Expand Down

0 comments on commit 99b8d59

Please sign in to comment.