fix: Re-opening widgets after re-hydrated

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(),

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;
       });

plugins/ui/src/js/src/layout/ReactPanel.test.tsx

@@ -8,14 +8,37 @@ import {
 } from './ReactPanelManager';
 import { ReactPanelProps } from './LayoutUtils';
 import PortalPanelManager from './PortalPanelManager';
+import PortalPanelManagerContext from './PortalPanelManagerContext';
 
 const mockPanelId = 'test-panel-id';
 
 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<ReactPanelProps> & Partial<ReactPanelManager> = {}) {
+  return (
+    <ReactPanelManagerContext.Provider
+      value={{
+        getPanelId,
+        metadata,
+        onClose,
+        onOpen,
+      }}
+    >
+      <ReactPanel title={title}>{children}</ReactPanel>
+    </ReactPanelManagerContext.Provider>
+  );
+}
+
+function makeTestComponent({
   children,
   metadata = { name: 'test-name', type: 'test-type' },
   onClose = jest.fn(),
@@ -25,16 +48,14 @@ function makeReactPanel({
 }: Partial<ReactPanelProps> & Partial<ReactPanelManager> = {}) {
   return (
     <PortalPanelManager>
-      <ReactPanelManagerContext.Provider
-        value={{
-          getPanelId,
-          metadata,
-          onClose,
-          onOpen,
-        }}
-      >
-        <ReactPanel title={title}>{children}</ReactPanel>
-      </ReactPanelManagerContext.Provider>
+      {makeReactPanelManager({
+        children,
+        metadata,
+        onClose,
+        onOpen,
+        getPanelId,
+        title,
+      })}
     </PortalPanelManager>
   );
 }
@@ -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,34 +84,34 @@ 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();
   expect(onOpen).toHaveBeenCalledTimes(1);
   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(
+    <PortalPanelManagerContext.Provider value={portals}>
+      {makeReactPanelManager({
+        children,
+        onOpen,
+        onClose,
+        metadata,
+      })}
+    </PortalPanelManagerContext.Provider>
+  );
+  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(
+    <PortalPanelManagerContext.Provider value={portals}>
+      {makeReactPanelManager({
+        children: 'world',
+        onOpen,
+        onClose,
+        metadata: { type: 'baz' },
+      })}
+    </PortalPanelManagerContext.Provider>
+  );
+
+  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);
+});

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();
@@ -27,15 +31,18 @@ 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<ReactPanelControl['metadata']>(
     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 +57,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();
@@ -64,21 +71,9 @@ function ReactPanel({ children, title }: ReactPanelProps) {
   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,
@@ -90,9 +85,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();
       }
     },
@@ -101,7 +107,7 @@ function ReactPanel({ children, title }: ReactPanelProps) {
 
   return portal
     ? ReactDOM.createPortal(
-        <ReactPanelContext.Provider value={panelId}>
+        <ReactPanelContext.Provider value={panelId} key={contentKey}>
           {children}
         </ReactPanelContext.Provider>,
         portal