diff --git a/plugins/ui/src/js/__mocks__/@deephaven/dashboard.js b/plugins/ui/src/js/__mocks__/@deephaven/dashboard.js index bed03cf0c..6e03f2e07 100644 --- a/plugins/ui/src/js/__mocks__/@deephaven/dashboard.js +++ b/plugins/ui/src/js/__mocks__/@deephaven/dashboard.js @@ -13,6 +13,7 @@ module.exports = { ...DashboardActual, LayoutUtils: { getComponentName: jest.fn(), + getContentItemInStack: jest.fn(), getStackForConfig: jest.fn(), getIdFromContainer: DashboardActual.LayoutUtils.getIdFromContainer, openComponent: jest.fn(), diff --git a/plugins/ui/src/js/src/DashboardPlugin.tsx b/plugins/ui/src/js/src/DashboardPlugin.tsx index 33cad703e..629a116d3 100644 --- a/plugins/ui/src/js/src/DashboardPlugin.tsx +++ b/plugins/ui/src/js/src/DashboardPlugin.tsx @@ -31,6 +31,7 @@ import { import PortalPanel from './layout/PortalPanel'; import PortalPanelManager from './layout/PortalPanelManager'; import DashboardWidgetHandler from './widget/DashboardWidgetHandler'; +import { getPreservedData } from './widget/WidgetUtils'; const NAME_ELEMENT = 'deephaven.ui.Element'; const DASHBOARD_ELEMENT = 'deephaven.ui.Dashboard'; @@ -96,10 +97,12 @@ export function DashboardPlugin( log.info('Opening widget with ID', widgetId, widget); setWidgetMap(prevWidgetMap => { const newWidgetMap = new Map(prevWidgetMap); + const oldWidget = newWidgetMap.get(widgetId); newWidgetMap.set(widgetId, { fetch, id: widgetId, widget, + data: getPreservedData(oldWidget?.data), }); return newWidgetMap; }); diff --git a/plugins/ui/src/js/src/layout/ReactPanel.test.tsx b/plugins/ui/src/js/src/layout/ReactPanel.test.tsx index 2e6b15283..793d29014 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.test.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.test.tsx @@ -8,6 +8,7 @@ import { } from './ReactPanelManager'; import { ReactPanelProps } from './LayoutUtils'; import PortalPanelManager from './PortalPanelManager'; +import PortalPanelManagerContext from './PortalPanelManagerContext'; const mockPanelId = 'test-panel-id'; @@ -15,7 +16,29 @@ beforeEach(() => { jest.clearAllMocks(); }); -function makeReactPanel({ +function makeReactPanelManager({ + children, + metadata = { name: 'test-name', type: 'test-type' }, + onClose = jest.fn(), + onOpen = jest.fn(), + getPanelId = jest.fn(() => mockPanelId), + title = 'test title', +}: Partial & Partial = {}) { + return ( + + {children} + + ); +} + +function makeTestComponent({ children, metadata = { name: 'test-name', type: 'test-type' }, onClose = jest.fn(), @@ -25,16 +48,14 @@ function makeReactPanel({ }: Partial & Partial = {}) { return ( - - {children} - + {makeReactPanelManager({ + children, + metadata, + onClose, + onOpen, + getPanelId, + title, + })} ); } @@ -46,10 +67,10 @@ function simulatePanelClosed() { (useListener as jest.Mock).mock.calls[0][2](mockPanelId); } -it('opens panel on mount, and closes panel on unmount', async () => { +it('opens panel on mount, and closes panel on unmount', () => { const onOpen = jest.fn(); const onClose = jest.fn(); - const { unmount } = render(makeReactPanel({ onOpen, onClose })); + const { unmount } = render(makeTestComponent({ onOpen, onClose })); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); expect(onOpen).toHaveBeenCalledTimes(1); @@ -63,20 +84,20 @@ it('opens panel on mount, and closes panel on unmount', async () => { expect(onClose).toHaveBeenCalledTimes(1); }); -it('only calls open once if the panel has not closed and only children change', async () => { +it('only calls open once if the panel has not closed and only children change', () => { const onOpen = jest.fn(); const onClose = jest.fn(); const metadata = { type: 'bar' }; const children = 'hello'; const { rerender } = render( - makeReactPanel({ children, onOpen, onClose, metadata }) + makeTestComponent({ children, onOpen, onClose, metadata }) ); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); expect(onOpen).toHaveBeenCalledTimes(1); expect(onClose).not.toHaveBeenCalled(); - rerender(makeReactPanel({ children: 'world', onOpen, onClose, metadata })); + rerender(makeTestComponent({ children: 'world', onOpen, onClose, metadata })); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); @@ -84,13 +105,13 @@ it('only calls open once if the panel has not closed and only children change', expect(onClose).not.toHaveBeenCalled(); }); -it('calls openComponent again after panel is closed only if the metadata changes', async () => { +it('calls openComponent again after panel is closed only if the metadata changes', () => { const onOpen = jest.fn(); const onClose = jest.fn(); const metadata = { type: 'bar' }; const children = 'hello'; const { rerender } = render( - makeReactPanel({ + makeTestComponent({ children, onOpen, onClose, @@ -112,7 +133,7 @@ it('calls openComponent again after panel is closed only if the metadata changes // Should not re-open if just the children change but the metadata stays the same rerender( - makeReactPanel({ + makeTestComponent({ children: 'world', onOpen, onClose, @@ -127,7 +148,7 @@ it('calls openComponent again after panel is closed only if the metadata changes // Should re-open after the metadata change rerender( - makeReactPanel({ + makeTestComponent({ children, onOpen, onClose, @@ -140,3 +161,102 @@ it('calls openComponent again after panel is closed only if the metadata changes expect(onOpen).toHaveBeenCalledTimes(2); expect(onClose).toHaveBeenCalledTimes(1); }); + +// Case when rehydrating a widget +it('does not call openComponent or setActiveContentItem if panel already exists when created', () => { + const onOpen = jest.fn(); + const onClose = jest.fn(); + const mockStack = { + setActiveContentItem: jest.fn(), + }; + const mockContentItem = {}; + (LayoutUtils.getStackForConfig as jest.Mock).mockReturnValue(mockStack); + (LayoutUtils.getContentItemInStack as jest.Mock).mockReturnValue( + mockContentItem + ); + const portal = document.createElement('div'); + const portals = new Map([[mockPanelId, portal]]); + + const metadata = { type: 'bar' }; + const children = 'hello'; + const { rerender } = render( + + {makeReactPanelManager({ + children, + onOpen, + onClose, + metadata, + })} + + ); + expect(LayoutUtils.openComponent).not.toHaveBeenCalled(); + expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); + expect(LayoutUtils.getStackForConfig).toHaveBeenCalled(); + expect(mockStack.setActiveContentItem).not.toHaveBeenCalled(); + + expect(onOpen).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + + // Now check that it focuses it if it's called after the metadata changes + rerender( + + {makeReactPanelManager({ + children: 'world', + onOpen, + onClose, + metadata: { type: 'baz' }, + })} + + ); + + expect(LayoutUtils.openComponent).not.toHaveBeenCalled(); + expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); + expect(onOpen).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + + expect(mockStack.setActiveContentItem).toHaveBeenCalledTimes(1); + expect(mockStack.setActiveContentItem).toHaveBeenCalledWith(mockContentItem); +}); + +it('calls setActiveContentItem if metadata changed while the panel already exists', () => { + const onOpen = jest.fn(); + const onClose = jest.fn(); + const metadata = { type: 'bar' }; + const children = 'hello'; + const { rerender } = render( + makeTestComponent({ + children, + onOpen, + onClose, + metadata, + }) + ); + expect(LayoutUtils.openComponent).not.toHaveBeenCalled(); + expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); + expect(onOpen).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + expect(useListener).toHaveBeenCalledTimes(1); + + const mockStack = { + setActiveContentItem: jest.fn(), + }; + const mockContentItem = {}; + (LayoutUtils.getStackForConfig as jest.Mock).mockReturnValue(mockStack); + (LayoutUtils.getContentItemInStack as jest.Mock).mockReturnValue( + mockContentItem + ); + rerender( + makeTestComponent({ + children: 'world', + onOpen, + onClose, + metadata: { type: 'baz' }, + }) + ); + + expect(LayoutUtils.openComponent).not.toHaveBeenCalled(); + expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); + expect(onOpen).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + expect(mockStack.setActiveContentItem).toHaveBeenCalledTimes(1); +}); diff --git a/plugins/ui/src/js/src/layout/ReactPanel.tsx b/plugins/ui/src/js/src/layout/ReactPanel.tsx index d77037a8b..8bc7d255e 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.tsx @@ -1,5 +1,6 @@ -import React, { useCallback, useEffect, useRef } from 'react'; +import React, { useCallback, useEffect, useMemo, useRef } from 'react'; import ReactDOM from 'react-dom'; +import shortid from 'shortid'; import { LayoutUtils, PanelEvent, @@ -18,6 +19,9 @@ const log = Log.module('@deephaven/js-plugin-ui/ReactPanel'); /** * 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. + * Will trigger an `onOpen` when the portal is opened, and `onClose` when closed. + * Note that because the `PortalPanel` will be saved with the GoldenLayout config, it's possible there is already a panel that exists with the same ID. + * In that case, the existing panel will be re-used. */ function ReactPanel({ children, title }: ReactPanelProps) { const layoutManager = useLayoutManager(); @@ -25,17 +29,21 @@ function ReactPanel({ children, title }: ReactPanelProps) { const portalManager = usePortalPanelManager(); const portal = portalManager.get(panelId); + // Tracks whether the panel is open and that we have emitted the onOpen event + const isPanelOpenRef = useRef(false); // If there is already a portal that exists, then we're rehydrating from a dehydrated state - // Initialize the `isPanelOpenRef` and `openedWidgetRef` accordingly on initialization - const isPanelOpenRef = useRef(portal != null); + // Initialize the `openedWidgetRef` accordingly on initialization const openedMetadataRef = useRef( portal != null ? metadata : null ); + + // We want to regenerate the key every time the metadata changes, so that the portal is re-rendered + // eslint-disable-next-line react-hooks/exhaustive-deps + const contentKey = useMemo(() => shortid.generate(), [metadata]); + const parent = useParentItem(); const { eventHub } = layoutManager; - log.debug2('Rendering panel', panelId); - useEffect( () => () => { if (isPanelOpenRef.current) { @@ -50,7 +58,7 @@ function ReactPanel({ children, title }: ReactPanelProps) { const handlePanelClosed = useCallback( closedPanelId => { - if (closedPanelId === panelId) { + if (closedPanelId === panelId && isPanelOpenRef.current) { log.debug('Panel closed', panelId); isPanelOpenRef.current = false; onClose(); @@ -62,23 +70,19 @@ function ReactPanel({ children, title }: ReactPanelProps) { useListener(eventHub, PanelEvent.CLOSED, handlePanelClosed); useEffect( - /** Opens a panel in the layout if necessary. Triggered when the panel metadata changes or the panel has not been opened yet. */ + /** + * Opens a panel in the layout if necessary. There are a few cases this is triggered: + * 1. Panel has not been opened yet: we need to open the panel in this case. + * 2. Panel metadata changes: we need to update the panel with the new metadata, show the panel in its current stack, + * and refresh the content with new state. + * 3. Widget is being re-hydrated: we need to check if the panel ID is already open, and then use that existing portal. + * We don't need to focus in this case, as this is when a whole dashboard is being re-hydrated - not when the user is + * opening this widget in particular. + */ function openIfNecessary() { - if (isPanelOpenRef.current === false) { - const existingStack = LayoutUtils.getStackForConfig(parent, { - id: panelId, - }); - if (existingStack != null) { - log.debug2('Panel already exists, just re-using'); - isPanelOpenRef.current = true; - return; - } - } - - if ( - isPanelOpenRef.current === false || - openedMetadataRef.current !== metadata - ) { + const itemConfig = { id: panelId }; + const existingStack = LayoutUtils.getStackForConfig(parent, itemConfig); + if (existingStack == null) { const panelTitle = title ?? metadata?.name ?? ''; const config = { type: 'react-component' as const, @@ -90,9 +94,20 @@ function ReactPanel({ children, title }: ReactPanelProps) { LayoutUtils.openComponent({ root: parent, config }); log.debug('Opened panel', panelId, config); - isPanelOpenRef.current = true; - openedMetadataRef.current = metadata; + } else if (openedMetadataRef.current !== metadata) { + const contentItem = LayoutUtils.getContentItemInStack( + existingStack, + itemConfig + ); + if (contentItem != null) { + existingStack.setActiveContentItem(contentItem); + } + } + openedMetadataRef.current = metadata; + if (!isPanelOpenRef.current) { + // We don't need to send an opened signal again + isPanelOpenRef.current = true; onOpen(); } }, @@ -104,7 +119,8 @@ function ReactPanel({ children, title }: ReactPanelProps) { {children} , - portal + portal, + contentKey ) : null; } diff --git a/plugins/ui/src/js/src/widget/DocumentHandler.tsx b/plugins/ui/src/js/src/widget/DocumentHandler.tsx index fddf8d7a2..449e00da7 100644 --- a/plugins/ui/src/js/src/widget/DocumentHandler.tsx +++ b/plugins/ui/src/js/src/widget/DocumentHandler.tsx @@ -48,24 +48,38 @@ function DocumentHandler({ log.debug('Rendering document', widget); const panelOpenCountRef = useRef(0); const panelIdIndex = useRef(0); + + // Using `useState` here to initialize the data only once. + // 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(() => structuredClone(data)); + // 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([]); + const handleOpen = useCallback( (panelId: string) => { + if (panelIds.includes(panelId)) { + throw new Error('Duplicate panel opens received'); + } + panelOpenCountRef.current += 1; log.debug('Panel opened, open count', panelOpenCountRef.current); - if (widgetData.panelIds == null) { - widgetData.panelIds = []; - } - widgetData.panelIds?.push(panelId); - onDataChange(widgetData); + panelIds.push(panelId); + onDataChange({ ...widgetData, panelIds }); }, - [onDataChange, widgetData] + [onDataChange, panelIds, widgetData] ); const handleClose = useCallback( (panelId: string) => { + const panelIndex = panelIds.indexOf(panelId); + if (panelIndex === -1) { + throw new Error('Panel close received for unknown panel'); + } panelOpenCountRef.current -= 1; if (panelOpenCountRef.current < 0) { throw new Error('Panel open count is negative'); @@ -76,12 +90,10 @@ function DocumentHandler({ return; } - widgetData.panelIds = (widgetData.panelIds ?? [])?.filter( - id => id !== panelId - ); - onDataChange(widgetData); + panelIds.splice(panelIndex, 1); + onDataChange({ ...widgetData, panelIds }); }, - [onClose, onDataChange, widgetData] + [onClose, onDataChange, panelIds, widgetData] ); const getPanelId = useCallback(() => { diff --git a/plugins/ui/src/js/src/widget/WidgetHandler.tsx b/plugins/ui/src/js/src/widget/WidgetHandler.tsx index 455498a48..0a034a8ef 100644 --- a/plugins/ui/src/js/src/widget/WidgetHandler.tsx +++ b/plugins/ui/src/js/src/widget/WidgetHandler.tsx @@ -276,13 +276,14 @@ function WidgetHandler({ async function loadWidgetInternal() { const newWidget = await fetch(); if (isCancelled) { + log.debug2('loadWidgetInternal cancelled', descriptor, newWidget); newWidget.close(); newWidget.exportedObjects.forEach(exportedObject => { exportedObject.close(); }); return; } - log.debug('newWidget', descriptor, newWidget); + log.debug('loadWidgetInternal done', descriptor, newWidget); setWidget(newWidget); } loadWidgetInternal(); diff --git a/plugins/ui/src/js/src/widget/WidgetUtils.test.tsx b/plugins/ui/src/js/src/widget/WidgetUtils.test.tsx index 7266a1cfc..40a5621ef 100644 --- a/plugins/ui/src/js/src/widget/WidgetUtils.test.tsx +++ b/plugins/ui/src/js/src/widget/WidgetUtils.test.tsx @@ -15,6 +15,7 @@ import { elementComponentMap, getComponentForElement, getComponentTypeForElement, + getPreservedData, } from './WidgetUtils'; describe('getComponentTypeForElement', () => { @@ -75,3 +76,23 @@ describe('getComponentForElement', () => { ); }); }); + +describe('getPreservedData', () => { + it('should handle undefined widget data', () => { + const actual = getPreservedData(undefined); + expect(actual).toEqual({}); + }); + it('should handle empty widget data', () => { + const actual = getPreservedData({}); + expect(actual).toEqual({}); + }); + it('should return only the panelIds from the widget data', () => { + const widgetData = { + panelIds: ['1', '2', '3'], + state: { foo: 'bar' }, + }; + + const actual = getPreservedData(widgetData); + expect(actual).toEqual({ panelIds: widgetData.panelIds }); + }); +}); diff --git a/plugins/ui/src/js/src/widget/WidgetUtils.tsx b/plugins/ui/src/js/src/widget/WidgetUtils.tsx index c04153978..3848b365d 100644 --- a/plugins/ui/src/js/src/widget/WidgetUtils.tsx +++ b/plugins/ui/src/js/src/widget/WidgetUtils.tsx @@ -4,6 +4,7 @@ import React, { ComponentType } from 'react'; // Importing `Item` and `Section` compnents directly since they should not be // wrapped due to how Spectrum collection components consume them. import { Item, Section } from '@deephaven/components'; +import { ReadonlyWidgetData } from './WidgetTypes'; import { ElementNode, ELEMENT_KEY, @@ -82,3 +83,23 @@ export function getComponentForElement(element: ElementNode): React.ReactNode { return newElement.props?.children; } + +/** Data keys of a widget to preserve across re-opening. */ +const PRESERVED_DATA_KEYS: (keyof ReadonlyWidgetData)[] = ['panelIds']; +const PRESERVED_DATA_KEYS_SET = new Set(PRESERVED_DATA_KEYS); + +/** + * Returns an object with only the data preserved that should be preserved when re-opening a widget (e.g. opening it again from console). + * For example, if you re-open a widget, you want to keep the `panelIds` data because that will re-open the widget to where it was before. + * However, we do _not_ want to preserve the `state` in this case - we want to widget to start from a fresh state. + * Similar to how when you re-open a table, it'll open in the same spot, but all UI applied filters/operations will be reset. + * @param oldData The old data to get the preserved data from + * @returns The data to preserve + */ +export function getPreservedData( + oldData: ReadonlyWidgetData = {} +): ReadonlyWidgetData { + return Object.fromEntries( + Object.entries(oldData).filter(([key]) => PRESERVED_DATA_KEYS_SET.has(key)) + ); +}