Repository: deephaven/deephaven-plugins (language: python)

Issue:
deephaven.ui component that is persisted is not re-opened correctly
**Description**

When you re-hydrate a deephaven.ui component, it gets in a weird state if you re-open that component.

**Steps to reproduce**
1. Make sure that "Close Panels on Disconnect" option is _not_ selected in the console menu dropdown
2. Create a deephaven.ui component. The component should open automatically:
```
from deephaven import ui

foo = ui.text("Bar")
```
3. Refresh the page
4. Run the same command from Step 2 again
5. Run it yet again

**Expected results**
4. The `foo` panel should replace the existing panel, and still have it's content
5. The `foo` panel should yet again replace the existing panel

**Actual results**

4. The `foo` panel appears empty
5. Another `foo` panel appears in a new spot, leaving the old empty one behind

**Additional details and attachments**

![image](https://github.com/deephaven/deephaven-plugins/assets/4505624/5439e512-2be2-4d85-a7d0-39073d25511d)

**Versions**

Engine Version: 0.34.0-SNAPSHOT
Web UI Version: 0.69.1
Java Version: 11.0.22
Barrage Version: 0.6.0
Browser Name: Chrome 122
OS Name: Linux


Discussion / hints:

Fix patch:
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.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<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 +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) {
         <ReactPanelContext.Provider value={panelId}>
           {children}
         </ReactPanelContext.Provider>,
-        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<WidgetData>(() => 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<string[]>([]);
+
   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.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<string>(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))
+  );
+}


Test patch:
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<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,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(
+    <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 metadata 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);
+});
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 });
+  });
+});