Skip to content

Commit

Permalink
fix: ReactPanel state management and disappearing panels
Browse files Browse the repository at this point in the history
- Show a loading spinner while the widget is still loading
- Show an error message on all panels if there's an error loading a re-hydrated widget (e.g widget no longer exists)
- Re-use an error panel if it's already placed
  • Loading branch information
mofojed committed Jul 26, 2024
1 parent 0f58049 commit b896270
Show file tree
Hide file tree
Showing 8 changed files with 203 additions and 95 deletions.
17 changes: 7 additions & 10 deletions plugins/ui/src/js/src/DashboardPlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
useDashboardPanel,
} from '@deephaven/dashboard';
import Log from '@deephaven/log';
import { DeferredApiBootstrap } from '@deephaven/jsapi-bootstrap';
import { dh } from '@deephaven/jsapi-types';
import { ErrorBoundary } from '@deephaven/components';
import { useDebouncedCallback } from '@deephaven/react-hooks';
Expand Down Expand Up @@ -278,15 +277,13 @@ export function DashboardPlugin(
key={widgetId}
fallback={id === DEFAULT_DASHBOARD_ID ? [] : null}
>
<DeferredApiBootstrap widget={wrapper.widget}>
<DashboardWidgetHandler
widgetDescriptor={wrapper.widget}
id={wrapper.id}
initialData={wrapper.data}
onDataChange={handleWidgetDataChange}
onClose={handleWidgetClose}
/>
</DeferredApiBootstrap>
<DashboardWidgetHandler
widgetDescriptor={wrapper.widget}
id={wrapper.id}
data={wrapper.data}
onDataChange={handleWidgetDataChange}
onClose={handleWidgetClose}
/>
</ErrorBoundary>
)),
[handleWidgetClose, handleWidgetDataChange, widgetMap, id]
Expand Down
4 changes: 4 additions & 0 deletions plugins/ui/src/js/src/layout/ReactPanel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ const defaultStatus: WidgetStatus = {
descriptor: defaultDescriptor,
};

jest.mock('@deephaven/jsapi-bootstrap', () => ({
DeferredApiBootstrap: jest.fn(({ children }) => children),
}));

beforeEach(() => {
jest.clearAllMocks();
});
Expand Down
32 changes: 24 additions & 8 deletions plugins/ui/src/js/src/layout/ReactPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,14 @@ import {
useLayoutManager,
useListener,
} from '@deephaven/dashboard';
import { View, ViewProps, Flex, FlexProps } from '@deephaven/components';
import {
View,
ViewProps,
Flex,
FlexProps,
LoadingOverlay,
} from '@deephaven/components';
import { DeferredApiBootstrap } from '@deephaven/jsapi-bootstrap';
import Log from '@deephaven/log';
import PortalPanel from './PortalPanel';
import { ReactPanelControl, useReactPanel } from './ReactPanelManager';
Expand Down Expand Up @@ -46,7 +53,10 @@ interface Props
| 'gap'
| 'rowGap'
| 'columnGap'
> {}
> {
// Whether to close the panel in the UI on unmount
closeOnUnmount?: boolean;
}

/**
* Adds and tracks a panel to the GoldenLayout. When the child element is updated, the contents of the panel will also be updated. When unmounted, the panel will be removed.
Expand All @@ -59,6 +69,7 @@ function ReactPanel({
// but also defined here, incase the panel
// is being implicitly created
children,
closeOnUnmount = true,
title,
backgroundColor,
direction = 'column',
Expand Down Expand Up @@ -101,14 +112,14 @@ function ReactPanel({

useEffect(
() => () => {
if (isPanelOpenRef.current) {
if (isPanelOpenRef.current && closeOnUnmount) {
log.debug('Closing panel', panelId);
LayoutUtils.closeComponent(parent, { id: panelId });
isPanelOpenRef.current = false;
onClose();
}
},
[parent, onClose, panelId]
[closeOnUnmount, parent, onClose, panelId]
);

const handlePanelClosed = useCallback(
Expand Down Expand Up @@ -204,13 +215,18 @@ function ReactPanel({
>
<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,
* Don't render the children if there's an error with the widget or it's still loading.
* 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' ? (
{widgetStatus.status === 'error' && (
<WidgetErrorView error={widgetStatus.error} />
) : (
children
)}
{widgetStatus.status === 'loading' && <LoadingOverlay />}
{widgetStatus.status === 'ready' && (
<DeferredApiBootstrap widget={widgetStatus.descriptor}>
{children}
</DeferredApiBootstrap>
)}
</ReactPanelErrorBoundary>
</Flex>
Expand Down
20 changes: 15 additions & 5 deletions plugins/ui/src/js/src/layout/ReactPanelManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { PanelProps } from '@deephaven/dashboard';
import { useContextOrThrow } from '@deephaven/react-hooks';
import { createContext, useCallback, useMemo } from 'react';

export type ReactPanelId = string;

/**
* Manager for panels within a widget. This is used to manage the lifecycle of panels within a widget.
*/
Expand All @@ -13,16 +15,24 @@ export interface ReactPanelManager {
*/
metadata: PanelProps['metadata'];

/** Triggered when a panel is opened */
onOpen: (panelId: string) => void;
/**
* Triggered when a panel is opened
* @param panelId The panelId of the panel that was opened
*/
onOpen: (panelId: ReactPanelId) => void;

/** Triggered when a panel is closed */
onClose: (panelId: string) => void;
/**
* Triggered when a panel is closed
* @param panelId The panelId of the panel that was closed
*/
onClose: (panelId: ReactPanelId) => void;

/**
* Get a unique panelId from the panel manager. This should be used to identify the panel in the layout.
* This should be called once per panel when it is mounted
* @returns A unique panelId
*/
getPanelId: () => string;
getPanelId: () => ReactPanelId;
}

/** Interface for using a react panel */
Expand Down
80 changes: 55 additions & 25 deletions plugins/ui/src/js/src/widget/DocumentHandler.tsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
import React, { useCallback, useMemo, useRef, useState } from 'react';
import React, {
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from 'react';
import shortid from 'shortid';
import { WidgetDescriptor } from '@deephaven/dashboard';
import Log from '@deephaven/log';
import { EMPTY_FUNCTION } from '@deephaven/utils';
import { ReactPanelManagerContext } from '../layout/ReactPanelManager';
import { getRootChildren } from './DocumentUtils';
import { EMPTY_ARRAY, EMPTY_FUNCTION } from '@deephaven/utils';
import {
ReadonlyWidgetData,
WidgetData,
WidgetDataUpdate,
} from './WidgetTypes';
ReactPanelId,
ReactPanelManagerContext,
} from '../layout/ReactPanelManager';
import { getRootChildren } from './DocumentUtils';

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

const EMPTY_OBJECT = Object.freeze({});

export type DocumentHandlerProps = React.PropsWithChildren<{
/** Definition of the widget used to create this document. Used for titling panels if necessary. */
widget: WidgetDescriptor;
Expand All @@ -23,10 +25,10 @@ export type DocumentHandlerProps = React.PropsWithChildren<{
* Data state to use when loading the widget.
* When the data state is updated, the new state is emitted via the `onDataChange` callback.
*/
initialData?: ReadonlyWidgetData;
initialPanelIds?: readonly ReactPanelId[];

/** Triggered when the data in the document changes */
onDataChange?: (data: WidgetDataUpdate) => void;
/** Triggered when the panels that are open change */
onPanelsChange?: (panelIds: readonly ReactPanelId[]) => void;

/** Triggered when all panels opened from this document have closed */
onClose?: () => void;
Expand All @@ -41,8 +43,8 @@ export type DocumentHandlerProps = React.PropsWithChildren<{
function DocumentHandler({
children,
widget,
initialData: data = EMPTY_OBJECT,
onDataChange = EMPTY_FUNCTION,
initialPanelIds: initialPanelIdsProp = EMPTY_ARRAY,
onPanelsChange = EMPTY_FUNCTION,
onClose,
}: DocumentHandlerProps): JSX.Element {
log.debug('Rendering document', widget);
Expand All @@ -53,12 +55,16 @@ function DocumentHandler({
// We don't want to use `useMemo`, because we only want it to be initialized once with the `initialData` (uncontrolled)
// We don't want to use `useRef`, because we only want to run `structuredClone` once, and you can't pass an
// initialization function into `useRef` like you can with `useState`
const [widgetData] = useState<WidgetData>(() => structuredClone(data));
const [initialPanelIds] = useState(() => initialPanelIdsProp);

// panelIds that are currently opened within this document. This list is tracked by the `onOpen`/`onClose` call on the `ReactPanelManager` from a child component.
// Note that the initial widget data provided will be the `panelIds` for this document to use; this array is what is actually opened currently.
const [panelIds] = useState<string[]>([]);

// Flag to signal the panel counts have changed in the last render
// We may need to check if we need to close this widget if all panels are closed
const [isPanelsDirty, setPanelsDirty] = useState(false);

const handleOpen = useCallback(
(panelId: string) => {
if (panelIds.includes(panelId)) {
Expand All @@ -69,9 +75,10 @@ function DocumentHandler({
log.debug('Panel opened, open count', panelOpenCountRef.current);

panelIds.push(panelId);
onDataChange({ ...widgetData, panelIds });

setPanelsDirty(true);
},
[onDataChange, panelIds, widgetData]
[panelIds]
);

const handleClose = useCallback(
Expand All @@ -85,26 +92,49 @@ function DocumentHandler({
throw new Error('Panel open count is negative');
}
log.debug('Panel closed, open count', panelOpenCountRef.current);
if (panelOpenCountRef.current === 0) {
onClose?.();

panelIds.splice(panelIndex, 1);

setPanelsDirty(true);
},
[panelIds]
);

useEffect(
function checkOpenPanelCount() {
if (!isPanelsDirty) {
return;
}

panelIds.splice(panelIndex, 1);
onDataChange({ ...widgetData, panelIds });
setPanelsDirty(false);

// Check if all the panels in this widget are closed
// We do it outside of the `handleClose` function in case a new panel opens up in the same render cycle
log.debug2(
'Widget',
widget.id,
'open panel count',
panelOpenCountRef.current
);
if (panelOpenCountRef.current === 0) {
log.debug('Widget', widget.id, 'closed all panels, triggering onClose');
onClose?.();
} else {
onPanelsChange(panelIds);
}
},
[onClose, onDataChange, panelIds, widgetData]
[isPanelsDirty, onClose, onPanelsChange, panelIds, widget.id]
);

const getPanelId = useCallback(() => {
// On rehydration, yield known IDs first
// If there are no more known IDs, generate a new one.
// This can happen if the document hasn't been opened before, or if it's rehydrated and a new panel is added.
// Note that if the order of panels changes, the worst case scenario is that panels appear in the wrong location in the layout.
const panelId = widgetData.panelIds?.[panelIdIndex.current] ?? shortid();
const panelId = initialPanelIds[panelIdIndex.current] ?? shortid();
panelIdIndex.current += 1;
return panelId;
}, [widgetData]);
}, [initialPanelIds]);

const panelManager = useMemo(
() => ({
Expand Down
Loading

0 comments on commit b896270

Please sign in to comment.