From fc8922e96ed8e7cba2b18343afde16716db1898c Mon Sep 17 00:00:00 2001 From: mikebender Date: Wed, 20 Mar 2024 17:02:43 -0400 Subject: [PATCH 1/8] fix: Re-opening widgets after re-hydrated - There was a race condition when replacing the widget, where the old panel content was getting removed before the new panel was being put up - In that case, we want to wait for the portal opened event before emitting an open event - Also check that we're not getting a duplicate open event in the panel manager - The duplicate panel ID was getting logged with the widget that was saved, which would sometimes cause widgets to randomly re-open - Still screwing up in some cases. Panel manager is getting the open event twice and therefore not keeping track of the IDs correctly --- .../ui/src/js/src/layout/ReactPanel.test.tsx | 68 +++++++++++++++++-- plugins/ui/src/js/src/layout/ReactPanel.tsx | 26 +++++++ .../ui/src/js/src/widget/DocumentHandler.tsx | 16 +++-- .../ui/src/js/src/widget/WidgetHandler.tsx | 3 +- 4 files changed, 104 insertions(+), 9 deletions(-) diff --git a/plugins/ui/src/js/src/layout/ReactPanel.test.tsx b/plugins/ui/src/js/src/layout/ReactPanel.test.tsx index 2e6b15283..e4663a59d 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.test.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.test.tsx @@ -1,6 +1,7 @@ import React from 'react'; -import { render } from '@testing-library/react'; +import { act, render } from '@testing-library/react'; import { LayoutUtils, useListener } from '@deephaven/dashboard'; +import { usePortalOpenedListener } from './PortalPanelEvent'; import ReactPanel from './ReactPanel'; import { ReactPanelManager, @@ -11,6 +12,12 @@ import PortalPanelManager from './PortalPanelManager'; const mockPanelId = 'test-panel-id'; +// Mock usePortalOpenedListener +jest.mock('./PortalPanelEvent', () => ({ + ...jest.requireActual('./PortalPanelEvent'), + usePortalOpenedListener: jest.fn(), +})); + beforeEach(() => { jest.clearAllMocks(); }); @@ -46,7 +53,18 @@ function simulatePanelClosed() { (useListener as jest.Mock).mock.calls[0][2](mockPanelId); } -it('opens panel on mount, and closes panel on unmount', async () => { +function simulatePortalOpened() { + (usePortalOpenedListener as jest.Mock).mock.calls[1][1]({ + container: { + _config: { + id: mockPanelId, + }, + }, + element: document.createElement('div'), + }); +} + +it('opens panel on mount, and closes panel on unmount', () => { const onOpen = jest.fn(); const onClose = jest.fn(); const { unmount } = render(makeReactPanel({ onOpen, onClose })); @@ -55,6 +73,10 @@ it('opens panel on mount, and closes panel on unmount', async () => { expect(onOpen).toHaveBeenCalledTimes(1); expect(onClose).not.toHaveBeenCalled(); + act(() => { + simulatePortalOpened(); + }); + unmount(); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); @@ -63,7 +85,7 @@ 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' }; @@ -76,6 +98,10 @@ it('only calls open once if the panel has not closed and only children change', expect(onOpen).toHaveBeenCalledTimes(1); expect(onClose).not.toHaveBeenCalled(); + act(() => { + simulatePortalOpened(); + }); + rerender(makeReactPanel({ children: 'world', onOpen, onClose, metadata })); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); @@ -84,7 +110,7 @@ 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' }; @@ -97,6 +123,9 @@ it('calls openComponent again after panel is closed only if the metadata changes metadata, }) ); + act(() => { + simulatePortalOpened(); + }); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); expect(onOpen).toHaveBeenCalledTimes(1); @@ -140,3 +169,34 @@ it('calls openComponent again after panel is closed only if the metadata changes expect(onOpen).toHaveBeenCalledTimes(2); expect(onClose).toHaveBeenCalledTimes(1); }); + +it('does not call onClose when still waiting for an open', async () => { + const onOpen = jest.fn(); + const onClose = jest.fn(); + const { unmount } = render(makeReactPanel({ onOpen, onClose })); + expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); + expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); + expect(onOpen).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + + simulatePanelClosed(); + + expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); + expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); + expect(onOpen).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + + act(() => { + simulatePortalOpened(); + }); + + expect(onOpen).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + + unmount(); + + expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); + expect(LayoutUtils.closeComponent).toHaveBeenCalledTimes(1); + expect(onOpen).toHaveBeenCalledTimes(1); + expect(onClose).toHaveBeenCalledTimes(1); +}); diff --git a/plugins/ui/src/js/src/layout/ReactPanel.tsx b/plugins/ui/src/js/src/layout/ReactPanel.tsx index d77037a8b..aa77abc6a 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.tsx @@ -13,6 +13,7 @@ import { ReactPanelProps } from './LayoutUtils'; import { useParentItem } from './ParentItemContext'; import { ReactPanelContext } from './ReactPanelContext'; import { usePortalPanelManager } from './PortalPanelManagerContext'; +import { usePortalOpenedListener } from './PortalPanelEvent'; const log = Log.module('@deephaven/js-plugin-ui/ReactPanel'); @@ -31,6 +32,9 @@ function ReactPanel({ children, title }: ReactPanelProps) { const openedMetadataRef = useRef( portal != null ? metadata : null ); + // When we're re-opening a widget, we may get a close event for the previous panel before the open event + // In that situation, we want to ignore the close until the open event has occurred + const isWaitingForOpen = useRef(true); const parent = useParentItem(); const { eventHub } = layoutManager; @@ -40,6 +44,10 @@ function ReactPanel({ children, title }: ReactPanelProps) { () => () => { if (isPanelOpenRef.current) { log.debug('Closing panel', panelId); + if (isWaitingForOpen.current) { + log.debug('handlePanelClosed Ignoring close event, waiting for open'); + return; + } LayoutUtils.closeComponent(parent, { id: panelId }); isPanelOpenRef.current = false; onClose(); @@ -52,6 +60,10 @@ function ReactPanel({ children, title }: ReactPanelProps) { closedPanelId => { if (closedPanelId === panelId) { log.debug('Panel closed', panelId); + if (isWaitingForOpen.current) { + log.debug('handlePanelClosed Ignoring close event, waiting for open'); + return; + } isPanelOpenRef.current = false; onClose(); } @@ -59,6 +71,19 @@ function ReactPanel({ children, title }: ReactPanelProps) { [onClose, panelId] ); + const handlePortalOpened = useCallback( + payload => { + const { container } = payload; + const containerId = LayoutUtils.getIdFromContainer(container); + if (containerId === panelId) { + log.debug('Panel opened', panelId); + isWaitingForOpen.current = false; + } + }, + [panelId] + ); + + usePortalOpenedListener(eventHub, handlePortalOpened); useListener(eventHub, PanelEvent.CLOSED, handlePanelClosed); useEffect( @@ -92,6 +117,7 @@ function ReactPanel({ children, title }: ReactPanelProps) { log.debug('Opened panel', panelId, config); isPanelOpenRef.current = true; openedMetadataRef.current = metadata; + isWaitingForOpen.current = true; onOpen(); } diff --git a/plugins/ui/src/js/src/widget/DocumentHandler.tsx b/plugins/ui/src/js/src/widget/DocumentHandler.tsx index fddf8d7a2..a6707afc9 100644 --- a/plugins/ui/src/js/src/widget/DocumentHandler.tsx +++ b/plugins/ui/src/js/src/widget/DocumentHandler.tsx @@ -52,13 +52,21 @@ function DocumentHandler({ const handleOpen = useCallback( (panelId: string) => { - panelOpenCountRef.current += 1; - log.debug('Panel opened, open count', panelOpenCountRef.current); - if (widgetData.panelIds == null) { widgetData.panelIds = []; } - widgetData.panelIds?.push(panelId); + if ( + widgetData.panelIds == null || + widgetData.panelIds.includes(panelId) + ) { + log.debug2('Panel already open', panelId); + return; + } + + panelOpenCountRef.current += 1; + log.debug('Panel opened, open count', panelOpenCountRef.current); + + widgetData.panelIds.push(panelId); onDataChange(widgetData); }, [onDataChange, widgetData] 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(); From b9003330b3ece5b6297fd735763d280a5be91c05 Mon Sep 17 00:00:00 2001 From: mikebender Date: Wed, 20 Mar 2024 20:16:02 -0400 Subject: [PATCH 2/8] Behaviour seems to be correct in manual testing - Need to update unit tests - Tested with two widgets, `foo` and `bar` - Tested that they saved with the layout when opened - Tested that they re-activated when re-creating the item in the console (re-opening it) - Tested that they were removed from the plugin data when all panels closed - TODO: Handling if there's errors loading the widget on startup, showing a loading spinner in panels while loading the widget --- plugins/ui/src/js/src/DashboardPlugin.tsx | 5 ++ plugins/ui/src/js/src/layout/ReactPanel.tsx | 78 +++++++------------ .../ui/src/js/src/widget/DocumentHandler.tsx | 30 ++++--- 3 files changed, 47 insertions(+), 66 deletions(-) diff --git a/plugins/ui/src/js/src/DashboardPlugin.tsx b/plugins/ui/src/js/src/DashboardPlugin.tsx index 33cad703e..a35a61a47 100644 --- a/plugins/ui/src/js/src/DashboardPlugin.tsx +++ b/plugins/ui/src/js/src/DashboardPlugin.tsx @@ -96,10 +96,15 @@ 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: { + // We don't want to blow away the panel IDs that may already be open for this widget + panelIds: oldWidget?.data?.panelIds ?? [], + }, }); return newWidgetMap; }); diff --git a/plugins/ui/src/js/src/layout/ReactPanel.tsx b/plugins/ui/src/js/src/layout/ReactPanel.tsx index aa77abc6a..94bd2ce8c 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, @@ -13,12 +14,14 @@ import { ReactPanelProps } from './LayoutUtils'; import { useParentItem } from './ParentItemContext'; import { ReactPanelContext } from './ReactPanelContext'; import { usePortalPanelManager } from './PortalPanelManagerContext'; -import { usePortalOpenedListener } from './PortalPanelEvent'; 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(); @@ -28,26 +31,22 @@ function ReactPanel({ children, title }: ReactPanelProps) { // 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); + const isPanelOpenRef = useRef(false); const openedMetadataRef = useRef( portal != null ? metadata : null ); - // When we're re-opening a widget, we may get a close event for the previous panel before the open event - // In that situation, we want to ignore the close until the open event has occurred - const isWaitingForOpen = useRef(true); + + // 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) { log.debug('Closing panel', panelId); - if (isWaitingForOpen.current) { - log.debug('handlePanelClosed Ignoring close event, waiting for open'); - return; - } LayoutUtils.closeComponent(parent, { id: panelId }); isPanelOpenRef.current = false; onClose(); @@ -58,12 +57,8 @@ function ReactPanel({ children, title }: ReactPanelProps) { const handlePanelClosed = useCallback( closedPanelId => { - if (closedPanelId === panelId) { + if (closedPanelId === panelId && isPanelOpenRef.current) { log.debug('Panel closed', panelId); - if (isWaitingForOpen.current) { - log.debug('handlePanelClosed Ignoring close event, waiting for open'); - return; - } isPanelOpenRef.current = false; onClose(); } @@ -71,39 +66,14 @@ function ReactPanel({ children, title }: ReactPanelProps) { [onClose, panelId] ); - const handlePortalOpened = useCallback( - payload => { - const { container } = payload; - const containerId = LayoutUtils.getIdFromContainer(container); - if (containerId === panelId) { - log.debug('Panel opened', panelId); - isWaitingForOpen.current = false; - } - }, - [panelId] - ); - - usePortalOpenedListener(eventHub, handlePortalOpened); 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. */ 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, @@ -115,10 +85,20 @@ function ReactPanel({ children, title }: ReactPanelProps) { LayoutUtils.openComponent({ root: parent, config }); log.debug('Opened panel', panelId, config); - isPanelOpenRef.current = true; - openedMetadataRef.current = metadata; - isWaitingForOpen.current = true; + } 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(); } }, @@ -127,7 +107,7 @@ function ReactPanel({ children, title }: ReactPanelProps) { return portal ? ReactDOM.createPortal( - + {children} , portal diff --git a/plugins/ui/src/js/src/widget/DocumentHandler.tsx b/plugins/ui/src/js/src/widget/DocumentHandler.tsx index a6707afc9..a587f62b1 100644 --- a/plugins/ui/src/js/src/widget/DocumentHandler.tsx +++ b/plugins/ui/src/js/src/widget/DocumentHandler.tsx @@ -49,31 +49,29 @@ function DocumentHandler({ const panelOpenCountRef = useRef(0); const panelIdIndex = useRef(0); const [widgetData] = useState(() => structuredClone(data)); + const [panelIds] = useState([]); const handleOpen = useCallback( (panelId: string) => { - if (widgetData.panelIds == null) { - widgetData.panelIds = []; - } - if ( - widgetData.panelIds == null || - widgetData.panelIds.includes(panelId) - ) { - log.debug2('Panel already open', panelId); - return; + if (panelIds.includes(panelId)) { + throw new Error('Duplicate panel opens received'); } panelOpenCountRef.current += 1; log.debug('Panel opened, open count', panelOpenCountRef.current); - 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 || panelIndex == null) { + throw new Error('Panel close received for unknown panel'); + } panelOpenCountRef.current -= 1; if (panelOpenCountRef.current < 0) { throw new Error('Panel open count is negative'); @@ -84,12 +82,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(() => { From 1a8976d5b130493d957e4a9f2b6881f4cf386d5c Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 21 Mar 2024 09:54:44 -0400 Subject: [PATCH 3/8] Fix up tests - Basically reverted to what they were before, which is good --- .../ui/src/js/src/layout/ReactPanel.test.tsx | 62 +------------------ 1 file changed, 1 insertion(+), 61 deletions(-) diff --git a/plugins/ui/src/js/src/layout/ReactPanel.test.tsx b/plugins/ui/src/js/src/layout/ReactPanel.test.tsx index e4663a59d..5f4876926 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.test.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.test.tsx @@ -1,7 +1,6 @@ import React from 'react'; -import { act, render } from '@testing-library/react'; +import { render } from '@testing-library/react'; import { LayoutUtils, useListener } from '@deephaven/dashboard'; -import { usePortalOpenedListener } from './PortalPanelEvent'; import ReactPanel from './ReactPanel'; import { ReactPanelManager, @@ -12,12 +11,6 @@ import PortalPanelManager from './PortalPanelManager'; const mockPanelId = 'test-panel-id'; -// Mock usePortalOpenedListener -jest.mock('./PortalPanelEvent', () => ({ - ...jest.requireActual('./PortalPanelEvent'), - usePortalOpenedListener: jest.fn(), -})); - beforeEach(() => { jest.clearAllMocks(); }); @@ -53,17 +46,6 @@ function simulatePanelClosed() { (useListener as jest.Mock).mock.calls[0][2](mockPanelId); } -function simulatePortalOpened() { - (usePortalOpenedListener as jest.Mock).mock.calls[1][1]({ - container: { - _config: { - id: mockPanelId, - }, - }, - element: document.createElement('div'), - }); -} - it('opens panel on mount, and closes panel on unmount', () => { const onOpen = jest.fn(); const onClose = jest.fn(); @@ -73,10 +55,6 @@ it('opens panel on mount, and closes panel on unmount', () => { expect(onOpen).toHaveBeenCalledTimes(1); expect(onClose).not.toHaveBeenCalled(); - act(() => { - simulatePortalOpened(); - }); - unmount(); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); @@ -98,10 +76,6 @@ it('only calls open once if the panel has not closed and only children change', expect(onOpen).toHaveBeenCalledTimes(1); expect(onClose).not.toHaveBeenCalled(); - act(() => { - simulatePortalOpened(); - }); - rerender(makeReactPanel({ children: 'world', onOpen, onClose, metadata })); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); @@ -123,9 +97,6 @@ it('calls openComponent again after panel is closed only if the metadata changes metadata, }) ); - act(() => { - simulatePortalOpened(); - }); expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); expect(onOpen).toHaveBeenCalledTimes(1); @@ -169,34 +140,3 @@ it('calls openComponent again after panel is closed only if the metadata changes expect(onOpen).toHaveBeenCalledTimes(2); expect(onClose).toHaveBeenCalledTimes(1); }); - -it('does not call onClose when still waiting for an open', async () => { - const onOpen = jest.fn(); - const onClose = jest.fn(); - const { unmount } = render(makeReactPanel({ onOpen, onClose })); - expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); - expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); - expect(onOpen).toHaveBeenCalledTimes(1); - expect(onClose).not.toHaveBeenCalled(); - - simulatePanelClosed(); - - expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); - expect(LayoutUtils.closeComponent).not.toHaveBeenCalled(); - expect(onOpen).toHaveBeenCalledTimes(1); - expect(onClose).not.toHaveBeenCalled(); - - act(() => { - simulatePortalOpened(); - }); - - expect(onOpen).toHaveBeenCalledTimes(1); - expect(onClose).not.toHaveBeenCalled(); - - unmount(); - - expect(LayoutUtils.openComponent).toHaveBeenCalledTimes(1); - expect(LayoutUtils.closeComponent).toHaveBeenCalledTimes(1); - expect(onOpen).toHaveBeenCalledTimes(1); - expect(onClose).toHaveBeenCalledTimes(1); -}); From 26dae55c952159aff6886d79472083a41ded034c Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 21 Mar 2024 10:26:25 -0400 Subject: [PATCH 4/8] Fix up some more of the tests, clean them up --- .../src/js/__mocks__/@deephaven/dashboard.js | 1 + .../ui/src/js/src/layout/ReactPanel.test.tsx | 154 ++++++++++++++++-- 2 files changed, 138 insertions(+), 17 deletions(-) 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/layout/ReactPanel.test.tsx b/plugins/ui/src/js/src/layout/ReactPanel.test.tsx index 5f4876926..4c00c9e36 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, + })} ); } @@ -49,7 +70,7 @@ function simulatePanelClosed() { 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); @@ -69,14 +90,14 @@ it('only calls open once if the panel has not closed and only children change', 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(); @@ -90,7 +111,7 @@ it('calls openComponent again after panel is closed only if the metadata changes 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 metadat 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); +}); From 7c3f34f9b6d3b8f449997770ab7b1aef74815068 Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 21 Mar 2024 10:33:51 -0400 Subject: [PATCH 5/8] Clean up based on self review --- plugins/ui/src/js/src/layout/ReactPanel.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/ui/src/js/src/layout/ReactPanel.tsx b/plugins/ui/src/js/src/layout/ReactPanel.tsx index 94bd2ce8c..ce3e44ae1 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.tsx @@ -29,9 +29,10 @@ function ReactPanel({ children, title }: ReactPanelProps) { const portalManager = usePortalPanelManager(); const portal = portalManager.get(panelId); - // If there is already a portal that exists, then we're rehydrating from a dehydrated state - // Initialize the `isPanelOpenRef` and `openedWidgetRef` accordingly on initialization + // 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 `openedWidgetRef` accordingly on initialization const openedMetadataRef = useRef( portal != null ? metadata : null ); From 7483c55214abe51620b715354be5461121272a7d Mon Sep 17 00:00:00 2001 From: mikebender Date: Tue, 26 Mar 2024 13:57:59 -0400 Subject: [PATCH 6/8] Clean up based on review - Extract getPreservedData method - Add tests - Add some more comments --- plugins/ui/src/js/src/DashboardPlugin.tsx | 6 ++---- .../ui/src/js/src/layout/ReactPanel.test.tsx | 2 +- plugins/ui/src/js/src/layout/ReactPanel.tsx | 10 ++++++++- .../ui/src/js/src/widget/DocumentHandler.tsx | 12 +++++++++-- .../ui/src/js/src/widget/WidgetUtils.test.tsx | 21 +++++++++++++++++++ plugins/ui/src/js/src/widget/WidgetUtils.tsx | 21 +++++++++++++++++++ 6 files changed, 64 insertions(+), 8 deletions(-) diff --git a/plugins/ui/src/js/src/DashboardPlugin.tsx b/plugins/ui/src/js/src/DashboardPlugin.tsx index a35a61a47..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'; @@ -101,10 +102,7 @@ export function DashboardPlugin( fetch, id: widgetId, widget, - data: { - // We don't want to blow away the panel IDs that may already be open for this widget - panelIds: oldWidget?.data?.panelIds ?? [], - }, + 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 4c00c9e36..793d29014 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.test.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.test.tsx @@ -197,7 +197,7 @@ it('does not call openComponent or setActiveContentItem if panel already exists expect(onOpen).toHaveBeenCalledTimes(1); expect(onClose).not.toHaveBeenCalled(); - // Now check that it focuses it if it's called after the metadat changes + // Now check that it focuses it if it's called after the metadata changes rerender( {makeReactPanelManager({ diff --git a/plugins/ui/src/js/src/layout/ReactPanel.tsx b/plugins/ui/src/js/src/layout/ReactPanel.tsx index ce3e44ae1..0bd3dbac9 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.tsx @@ -70,7 +70,15 @@ 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 it's 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() { const itemConfig = { id: panelId }; const existingStack = LayoutUtils.getStackForConfig(parent, itemConfig); diff --git a/plugins/ui/src/js/src/widget/DocumentHandler.tsx b/plugins/ui/src/js/src/widget/DocumentHandler.tsx index a587f62b1..449e00da7 100644 --- a/plugins/ui/src/js/src/widget/DocumentHandler.tsx +++ b/plugins/ui/src/js/src/widget/DocumentHandler.tsx @@ -48,7 +48,15 @@ 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( @@ -68,8 +76,8 @@ function DocumentHandler({ const handleClose = useCallback( (panelId: string) => { - const panelIndex = panelIds?.indexOf(panelId); - if (panelIndex === -1 || panelIndex == null) { + const panelIndex = panelIds.indexOf(panelId); + if (panelIndex === -1) { throw new Error('Panel close received for unknown panel'); } panelOpenCountRef.current -= 1; 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..1e13097fe 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 (i.e. opening it again from console) a widget. + * 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)) + ); +} From 42ac99d42965ede4daa2f97a876df52ac833a866 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Wed, 3 Apr 2024 13:42:18 -0400 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Matthew Runyon --- plugins/ui/src/js/src/layout/ReactPanel.tsx | 2 +- plugins/ui/src/js/src/widget/WidgetUtils.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/ui/src/js/src/layout/ReactPanel.tsx b/plugins/ui/src/js/src/layout/ReactPanel.tsx index 0bd3dbac9..09bc8ed64 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.tsx @@ -73,7 +73,7 @@ function ReactPanel({ children, title }: ReactPanelProps) { /** * 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 it's current stack, + * 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 diff --git a/plugins/ui/src/js/src/widget/WidgetUtils.tsx b/plugins/ui/src/js/src/widget/WidgetUtils.tsx index 1e13097fe..3848b365d 100644 --- a/plugins/ui/src/js/src/widget/WidgetUtils.tsx +++ b/plugins/ui/src/js/src/widget/WidgetUtils.tsx @@ -89,7 +89,7 @@ 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 (i.e. opening it again from console) a widget. + * 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. From 27e17445ca67fd2793b8c947c693ae2270f57839 Mon Sep 17 00:00:00 2001 From: mikebender Date: Wed, 3 Apr 2024 17:02:31 -0400 Subject: [PATCH 8/8] Clean up based on review --- plugins/ui/src/js/src/layout/ReactPanel.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/ui/src/js/src/layout/ReactPanel.tsx b/plugins/ui/src/js/src/layout/ReactPanel.tsx index 09bc8ed64..8bc7d255e 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.tsx @@ -116,10 +116,11 @@ function ReactPanel({ children, title }: ReactPanelProps) { return portal ? ReactDOM.createPortal( - + {children} , - portal + portal, + contentKey ) : null; }