Skip to content

Commit

Permalink
fix: Don't render objects/children of panels if there's a widget error (
Browse files Browse the repository at this point in the history
#577)

- In that case the children are unlikely to render correctly anyway
- Have a `WidgetStatus` context that passes the widget status down
- `ReactPanel` checks the context and only displays the content if
there's no error with the widget
- Tested with DHE branch `bender_DH-16737-vplus` against dev-vplus
  • Loading branch information
mofojed committed Jun 26, 2024
1 parent c95e291 commit 3a74dcc
Show file tree
Hide file tree
Showing 15 changed files with 483 additions and 322 deletions.
434 changes: 254 additions & 180 deletions package-lock.json

Large diffs are not rendered by default.

14 changes: 7 additions & 7 deletions plugins/ui/src/js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,18 @@
"@adobe/react-spectrum": "^3.34.1",
"@deephaven/chart": "^0.78.0",
"@deephaven/components": "^0.78.0",
"@deephaven/dashboard": "^0.78.0",
"@deephaven/dashboard-core-plugins": "^0.78.0",
"@deephaven/dashboard": "^0.78.2",
"@deephaven/dashboard-core-plugins": "^0.78.2",
"@deephaven/grid": "^0.78.0",
"@deephaven/icons": "^0.78.0",
"@deephaven/iris-grid": "^0.78.0",
"@deephaven/jsapi-bootstrap": "^0.78.0",
"@deephaven/jsapi-components": "^0.78.0",
"@deephaven/iris-grid": "^0.78.2",
"@deephaven/jsapi-bootstrap": "^0.78.2",
"@deephaven/jsapi-components": "^0.78.2",
"@deephaven/jsapi-types": "^1.0.0-dev0.34.2",
"@deephaven/log": "^0.78.0",
"@deephaven/plugin": "^0.78.0",
"@deephaven/plugin": "^0.78.2",
"@deephaven/react-hooks": "^0.78.0",
"@deephaven/redux": "^0.78.0",
"@deephaven/redux": "^0.78.2",
"@deephaven/utils": "^0.78.0",
"@fortawesome/react-fontawesome": "^0.2.0",
"@react-types/shared": "^3.22.0",
Expand Down
118 changes: 75 additions & 43 deletions plugins/ui/src/js/src/layout/ReactPanel.test.tsx
Original file line number Diff line number Diff line change
@@ -1,25 +1,36 @@
import React from 'react';
import { render, within } from '@testing-library/react';
import { LayoutUtils, useListener } from '@deephaven/dashboard';
import {
LayoutUtils,
WidgetDescriptor,
useListener,
} from '@deephaven/dashboard';
import { TestUtils } from '@deephaven/utils';
import ReactPanel from './ReactPanel';
import {
ReactPanelManager,
ReactPanelManagerContext,
} from './ReactPanelManager';
import { ReactPanelProps } from './LayoutUtils';
import PortalPanelManager from './PortalPanelManager';
import PortalPanelManagerContext from './PortalPanelManagerContext';
import PortalPanelManagerContext, {
PortalPanelMap,
} from './PortalPanelManagerContext';
import WidgetStatusContext, { WidgetStatus } from './WidgetStatusContext';

const mockPanelId = 'test-panel-id';
const defaultDescriptor = { name: 'test-name', type: 'test-type' };
const defaultStatus: WidgetStatus = {
status: 'ready',
descriptor: defaultDescriptor,
};

beforeEach(() => {
jest.clearAllMocks();
});

function makeReactPanelManager({
children,
metadata = { name: 'test-name', type: 'test-type' },
metadata = defaultDescriptor,
onClose = jest.fn(),
onOpen = jest.fn(),
getPanelId = jest.fn(() => mockPanelId),
Expand All @@ -41,23 +52,32 @@ function makeReactPanelManager({

function makeTestComponent({
children,
metadata = { name: 'test-name', type: 'test-type' },
metadata = defaultDescriptor,
onClose = jest.fn(),
onOpen = jest.fn(),
getPanelId = jest.fn(() => mockPanelId),
portals = new Map(),
status = defaultStatus,
title = 'test title',
}: Partial<ReactPanelProps> & Partial<ReactPanelManager> = {}) {
}: Partial<ReactPanelProps> &
Partial<ReactPanelManager> & {
metadata?: WidgetDescriptor;
portals?: PortalPanelMap;
status?: WidgetStatus;
} = {}) {
return (
<PortalPanelManager>
{makeReactPanelManager({
children,
metadata,
onClose,
onOpen,
getPanelId,
title,
})}
</PortalPanelManager>
<WidgetStatusContext.Provider value={status}>
<PortalPanelManagerContext.Provider value={portals}>
{makeReactPanelManager({
children,
metadata,
onClose,
onOpen,
getPanelId,
title,
})}
</PortalPanelManagerContext.Provider>
</WidgetStatusContext.Provider>
);
}

Expand Down Expand Up @@ -181,14 +201,13 @@ it('does not call openComponent or setActiveContentItem if panel already exists
const metadata = { type: 'bar' };
const children = 'hello';
const { rerender } = render(
<PortalPanelManagerContext.Provider value={portals}>
{makeReactPanelManager({
children,
onOpen,
onClose,
metadata,
})}
</PortalPanelManagerContext.Provider>
makeTestComponent({
children,
onOpen,
onClose,
metadata,
portals,
})
);
expect(LayoutUtils.openComponent).not.toHaveBeenCalled();
expect(LayoutUtils.closeComponent).not.toHaveBeenCalled();
Expand All @@ -200,14 +219,13 @@ it('does not call openComponent or setActiveContentItem if panel already exists

// Now check that it focuses it if it's called after the metadata changes
rerender(
<PortalPanelManagerContext.Provider value={portals}>
{makeReactPanelManager({
children: 'world',
onOpen,
onClose,
metadata: { type: 'baz' },
})}
</PortalPanelManagerContext.Provider>
makeTestComponent({
children: 'world',
onOpen,
onClose,
metadata: { type: 'baz' },
portals,
})
);

expect(LayoutUtils.openComponent).not.toHaveBeenCalled();
Expand Down Expand Up @@ -274,22 +292,36 @@ it('catches an error thrown by children, renders error view', () => {
const portals = new Map([[mockPanelId, portal]]);

const { rerender } = render(
<PortalPanelManagerContext.Provider value={portals}>
{makeReactPanelManager({
children: <ErrorComponent />,
})}
</PortalPanelManagerContext.Provider>
makeTestComponent({
children: <ErrorComponent />,
portals,
})
);
const { getByText } = within(portal);
expect(getByText('Error: test error')).toBeDefined();
expect(getByText('test error')).toBeDefined();

rerender(
<PortalPanelManagerContext.Provider value={portals}>
{makeReactPanelManager({
children: <div>Hello</div>,
})}
</PortalPanelManagerContext.Provider>
makeTestComponent({
children: <div>Hello</div>,
portals,
})
);

expect(getByText('Hello')).toBeDefined();
});

it('displays an error if the widget is in an error state', () => {
const error = new Error('test error');
const portal = document.createElement('div');
const portals = new Map([[mockPanelId, portal]]);
const status: WidgetStatus = {
status: 'error',
descriptor: defaultDescriptor,
error,
};

render(makeTestComponent({ portals, status }));

const { getByText } = within(portal);
expect(getByText('test error')).toBeDefined();
});
27 changes: 16 additions & 11 deletions plugins/ui/src/js/src/layout/ReactPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,17 @@ import {
useLayoutManager,
useListener,
} from '@deephaven/dashboard';
import {
View,
ViewProps,
Flex,
FlexProps,
ErrorBoundary,
} from '@deephaven/components';
import { View, ViewProps, Flex, FlexProps } from '@deephaven/components';
import Log from '@deephaven/log';
import PortalPanel from './PortalPanel';
import { ReactPanelControl, useReactPanel } from './ReactPanelManager';
import { ReactPanelProps } from './LayoutUtils';
import { useParentItem } from './ParentItemContext';
import { ReactPanelContext } from './ReactPanelContext';
import { usePortalPanelManager } from './PortalPanelManagerContext';
import ReactPanelContentOverlay from './ReactPanelContentOverlay';
import ReactPanelErrorBoundary from './ReactPanelErrorBoundary';
import useWidgetStatus from './useWidgetStatus';
import WidgetErrorView from '../widget/WidgetErrorView';

const log = Log.module('@deephaven/js-plugin-ui/ReactPanel');

Expand Down Expand Up @@ -172,6 +168,7 @@ function ReactPanel({
},
[parent, metadata, onOpen, panelId, title]
);
const widgetStatus = useWidgetStatus();

return portal
? ReactDOM.createPortal(
Expand Down Expand Up @@ -205,11 +202,19 @@ function ReactPanel({
rowGap={rowGap}
columnGap={columnGap}
>
{/* Have an ErrorBoundary around the children to display an error in the panel if there's any errors thrown when rendering the children */}
<ErrorBoundary>{children}</ErrorBoundary>
<ReactPanelErrorBoundary>
{/**
* Don't render the children if there's an error with the widget. If there's an error with the widget, we can assume the children won't render properly,
* but we still want the panels to appear so things don't disappear/jump around.
*/}
{widgetStatus.status === 'error' ? (
<WidgetErrorView error={widgetStatus.error} />
) : (
children
)}
</ReactPanelErrorBoundary>
</Flex>
</View>
<ReactPanelContentOverlay />
</ReactPanelContext.Provider>,
portal,
contentKey
Expand Down
12 changes: 0 additions & 12 deletions plugins/ui/src/js/src/layout/ReactPanelContentOverlay.tsx

This file was deleted.

This file was deleted.

59 changes: 59 additions & 0 deletions plugins/ui/src/js/src/layout/ReactPanelErrorBoundary.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import Log from '@deephaven/log';
import React, { Component, ReactNode } from 'react';
import WidgetErrorView from '../widget/WidgetErrorView';

const log = Log.module('ReactPanelErrorBoundary');

export interface ReactPanelErrorBoundaryProps {
/** Children to catch errors from. Error will reset when the children have been updated. */
children: ReactNode;
}

export interface ReactPanelErrorBoundaryState {
/** Currently displayed error. Reset when children are updated. */
error?: Error;
}

/**
* Error boundary for catching render errors in React. Displays an error message until the children have updated.
*/
export class ReactPanelErrorBoundary extends Component<
ReactPanelErrorBoundaryProps,
ReactPanelErrorBoundaryState
> {
static getDerivedStateFromError(error: Error): ReactPanelErrorBoundaryState {
return { error };
}

constructor(props: ReactPanelErrorBoundaryProps) {
super(props);
this.state = { error: undefined };
}

componentDidUpdate(
prevProps: Readonly<ReactPanelErrorBoundaryProps>,
prevState: Readonly<ReactPanelErrorBoundaryState>
): void {
const { children } = this.props;
if (prevProps.children !== children && prevState.error != null) {
log.debug(
'ReactPanelErrorBoundary clearing previous error',
prevState.error,
children
);
this.setState({ error: undefined });
}
}

componentDidCatch(error: Error, errorInfo: React.ErrorInfo): void {
log.error('Error caught by ErrorBoundary', error, errorInfo);
}

render(): ReactNode {
const { children } = this.props;
const { error } = this.state;
return error != null ? <WidgetErrorView error={error} /> : children;
}
}

export default ReactPanelErrorBoundary;
28 changes: 28 additions & 0 deletions plugins/ui/src/js/src/layout/WidgetStatusContext.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { WidgetDescriptor } from '@deephaven/dashboard';
import { createContext } from 'react';

export type WidgetStatusLoading = {
status: 'loading';
descriptor: WidgetDescriptor;
};

export type WidgetStatusError = {
status: 'error';
descriptor: WidgetDescriptor;
error: NonNullable<unknown>;
};

export type WidgetStatusReady = {
status: 'ready';
descriptor: WidgetDescriptor;
};

export type WidgetStatus =
| WidgetStatusLoading
| WidgetStatusError
| WidgetStatusReady;

/** Status of the widget within this context */
export const WidgetStatusContext = createContext<WidgetStatus | null>(null);

export default WidgetStatusContext;
12 changes: 0 additions & 12 deletions plugins/ui/src/js/src/layout/usePanelContentOverlay.ts

This file was deleted.

12 changes: 12 additions & 0 deletions plugins/ui/src/js/src/layout/useWidgetStatus.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { useContextOrThrow } from '@deephaven/react-hooks';
import { WidgetStatus, WidgetStatusContext } from './WidgetStatusContext';

/**
* Gets the widget status from the closest WidgetStatusContext.
* @returns Widget status or throws an error if WidgetStatusContext is not set
*/
export function useWidgetStatus(): WidgetStatus {
return useContextOrThrow(WidgetStatusContext);
}

export default useWidgetStatus;
Loading

0 comments on commit 3a74dcc

Please sign in to comment.