fix: Reuse dashboard tabs when reassigning the variable (#2243)

Fixes #1971 Tested with this code run 1 line at a time ```python import deephaven.ui as ui a = ui.dashboard(ui.text("Hello")) a = ui.dashboard(ui.text("World")) a = ui.text("Hello") a = ui.dashboard(ui.text("Hello")) del a ``` Also tested dashboards still load in embed-widget. There was a race condition since the `makeUseListenerFunction` hook was using `useEffect`. This caused the event to emit between the event hub being set and the listener being added.
deephaven · Oct 2, 2024 · d2c6eab · d2c6eab
1 parent 91bd8fe
commit d2c6eab
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 76 deletions.
diff --git a/packages/code-studio/src/main/AppMainContainer.tsx b/packages/code-studio/src/main/AppMainContainer.tsx
@@ -34,10 +34,10 @@ import {
   getAllDashboardsData,
   getDashboardData,
   listenForCreateDashboard,
+  listenForCloseDashboard,
   PanelEvent,
   setDashboardData as setDashboardDataAction,
   setDashboardPluginData as setDashboardPluginDataAction,
-  stopListenForCreateDashboard,
   updateDashboardData as updateDashboardDataAction,
 } from '@deephaven/dashboard';
 import {
@@ -189,6 +189,8 @@ export class AppMainContainer extends Component<
     const { allDashboardData } = this.props;
 
     this.dashboardLayouts = new Map();
+    this.createDashboardListenerRemovers = new Map();
+    this.closeDashboardListenerRemovers = new Map();
 
     this.state = {
       contextActions: [
@@ -275,9 +277,8 @@ export class AppMainContainer extends Component<
   componentWillUnmount(): void {
     this.deinitWidgets();
 
-    this.dashboardLayouts.forEach(layout => {
-      stopListenForCreateDashboard(layout.eventHub, this.handleCreateDashboard);
-    });
+    this.createDashboardListenerRemovers.forEach(rm => rm());
+    this.closeDashboardListenerRemovers.forEach(rm => rm());
 
     window.removeEventListener(
       'beforeunload',
@@ -288,6 +289,12 @@ export class AppMainContainer extends Component<
   /** Map from the dashboard ID to the GoldenLayout instance for that dashboard */
   dashboardLayouts: Map<string, GoldenLayout>;
 
+  /** Map from dashboard ID to remover functions for create dashboard listener */
+  createDashboardListenerRemovers: Map<string, () => void>;
+
+  /** Map from dashboard ID to remover functions for close dashboard listener */
+  closeDashboardListenerRemovers: Map<string, () => void>;
+
   importElement: RefObject<HTMLInputElement>;
 
   widgetListenerRemover?: () => void;
@@ -504,16 +511,21 @@ export class AppMainContainer extends Component<
     const oldLayout = this.dashboardLayouts.get(activeTabKey);
     if (oldLayout === newLayout) return;
 
+    this.dashboardLayouts.set(activeTabKey, newLayout);
+
     if (oldLayout != null) {
-      stopListenForCreateDashboard(
-        oldLayout.eventHub,
-        this.handleCreateDashboard
-      );
+      this.createDashboardListenerRemovers.get(activeTabKey)?.();
+      this.closeDashboardListenerRemovers.get(activeTabKey)?.();
     }
 
-    this.dashboardLayouts.set(activeTabKey, newLayout);
-
-    listenForCreateDashboard(newLayout.eventHub, this.handleCreateDashboard);
+    this.createDashboardListenerRemovers.set(
+      activeTabKey,
+      listenForCreateDashboard(newLayout.eventHub, this.handleCreateDashboard)
+    );
+    this.closeDashboardListenerRemovers.set(
+      activeTabKey,
+      listenForCloseDashboard(newLayout.eventHub, this.handleCloseDashboard)
+    );
   }
 
   handleCreateDashboard({
@@ -524,16 +536,38 @@ export class AppMainContainer extends Component<
     const newId = nanoid();
     const { setDashboardPluginData } = this.props;
     setDashboardPluginData(newId, pluginId, data);
-    this.setState(({ tabs }) => ({
-      tabs: [
-        ...tabs,
-        {
-          key: newId,
-          title,
-        },
-      ],
-      activeTabKey: newId,
-    }));
+    this.setState(({ tabs }) => {
+      const existingTab = tabs.findIndex(
+        ({ title: tabTitle }) => tabTitle === title
+      );
+      if (existingTab !== -1) {
+        // Replace the existing tab
+        return {
+          tabs: tabs.map((tab, index) =>
+            index === existingTab ? { key: newId, title } : tab
+          ),
+          activeTabKey: newId,
+        };
+      }
+      return {
+        tabs: [
+          ...tabs,
+          {
+            key: newId,
+            title,
+          },
+        ],
+        activeTabKey: newId,
+      };
+    });
+  }
+
+  handleCloseDashboard(title: string): void {
+    const { tabs } = this.state;
+    const tab = tabs.find(t => t.title === title);
+    if (tab != null) {
+      this.handleTabClose(tab.key);
+    }
   }
 
   handleWidgetMenuClick(): void {

diff --git a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx
@@ -14,6 +14,7 @@ import {
 } from '@deephaven/console';
 import {
   type DashboardPanelProps,
+  emitCloseDashboard,
   emitPanelOpen,
   LayoutManagerContext,
   LayoutUtils,
@@ -278,6 +279,8 @@ export class ConsolePanel extends PureComponent<
       if (id != null) {
         const { glEventHub } = this.props;
         glEventHub.emit(PanelEvent.CLOSE, id);
+        // Just emit for all panels since there shouldn't be dashboard and panel name conflicts
+        emitCloseDashboard(glEventHub, title);
       }
     }
   }

diff --git a/packages/dashboard/src/DashboardEvents.ts b/packages/dashboard/src/DashboardEvents.ts
@@ -1,35 +1,23 @@
-import type { EventHub } from '@deephaven/golden-layout';
+import { makeEventFunctions } from '@deephaven/golden-layout';
 
 export const CREATE_DASHBOARD = 'CREATE_DASHBOARD';
 
+export const CLOSE_DASHBOARD = 'CLOSE_DASHBOARD';
+
 export interface CreateDashboardPayload<T = unknown> {
   pluginId: string;
   title: string;
   data: T;
 }
 
-export function stopListenForCreateDashboard<T = unknown>(
-  eventHub: EventHub,
-  handler: (p: CreateDashboardPayload<T>) => void
-): void {
-  try {
-    eventHub.off(CREATE_DASHBOARD, handler);
-  } catch {
-    // golden-layout throws if the handler is not found. Instead catch it and no-op
-  }
-}
+export const {
+  listen: listenForCreateDashboard,
+  emit: emitCreateDashboard,
+  useListener: useCreateDashboardListener,
+} = makeEventFunctions<[detail: CreateDashboardPayload]>(CREATE_DASHBOARD);
 
-export function listenForCreateDashboard<T = unknown>(
-  eventHub: EventHub,
-  handler: (p: CreateDashboardPayload<T>) => void
-): () => void {
-  eventHub.on(CREATE_DASHBOARD, handler);
-  return () => stopListenForCreateDashboard(eventHub, handler);
-}
-
-export function emitCreateDashboard<T = unknown>(
-  eventHub: EventHub,
-  payload: CreateDashboardPayload<T>
-): void {
-  eventHub.emit(CREATE_DASHBOARD, payload);
-}
+export const {
+  listen: listenForCloseDashboard,
+  emit: emitCloseDashboard,
+  useListener: useCloseDashboardListener,
+} = makeEventFunctions<[title: string]>(CLOSE_DASHBOARD);
diff --git a/packages/embed-widget/src/App.tsx b/packages/embed-widget/src/App.tsx
@@ -24,11 +24,10 @@ import Log from '@deephaven/log';
 import { useDashboardPlugins } from '@deephaven/plugin';
 import {
   getAllDashboardsData,
-  listenForCreateDashboard,
   type CreateDashboardPayload,
   setDashboardPluginData,
-  stopListenForCreateDashboard,
   emitPanelOpen,
+  useCreateDashboardListener,
 } from '@deephaven/dashboard';
 import {
   getVariableDescriptor,
@@ -147,31 +146,17 @@ function App(): JSX.Element {
   const [goldenLayout, setGoldenLayout] = useState<GoldenLayout | null>(null);
   const [dashboardId, setDashboardId] = useState('default-embed-widget'); // Can't be DEFAULT_DASHBOARD_ID because its dashboard layout is not stored in dashboardData
 
-  const handleGoldenLayoutChange = useCallback(
-    (newLayout: GoldenLayout) => {
-      function handleCreateDashboard({
-        pluginId,
-        data,
-      }: CreateDashboardPayload) {
-        const id = nanoid();
-        dispatch(setDashboardPluginData(id, pluginId, data));
-        setDashboardId(id);
-      }
-
-      setGoldenLayout(oldLayout => {
-        if (oldLayout != null) {
-          stopListenForCreateDashboard(
-            oldLayout.eventHub,
-            handleCreateDashboard
-          );
-        }
-        listenForCreateDashboard(newLayout.eventHub, handleCreateDashboard);
-        return newLayout;
-      });
+  const handleCreateDashboard = useCallback(
+    ({ pluginId, data }: CreateDashboardPayload) => {
+      const id = nanoid();
+      dispatch(setDashboardPluginData(id, pluginId, data));
+      setDashboardId(id);
     },
     [dispatch]
   );
 
+  useCreateDashboardListener(goldenLayout?.eventHub, handleCreateDashboard);
+
   const [hasEmittedWidget, setHasEmittedWidget] = useState(false);
 
   const handleDashboardInitialized = useCallback(() => {
@@ -243,7 +228,7 @@ function App(): JSX.Element {
             ]}
             activeDashboard={dashboardId}
             onLayoutInitialized={handleDashboardInitialized}
-            onGoldenLayoutChange={handleGoldenLayoutChange}
+            onGoldenLayoutChange={setGoldenLayout}
             plugins={dashboardPlugins}
           />
         </ErrorBoundary>

diff --git a/packages/golden-layout/src/utils/EventUtils.ts b/packages/golden-layout/src/utils/EventUtils.ts
@@ -1,4 +1,4 @@
-import { useEffect } from 'react';
+import { useEffect, useLayoutEffect, useRef } from 'react';
 import EventEmitter from './EventEmitter';
 
 type AsArray<P> = P extends unknown[] ? P : [P];
@@ -16,7 +16,7 @@ export type EventEmitFunction<TParameters = []> = (
 ) => void;
 
 export type EventListenerHook<TParameters = []> = (
-  eventEmitter: EventEmitter,
+  eventEmitter: EventEmitter | null | undefined,
   handler: EventHandlerFunction<TParameters>
 ) => void;
 
@@ -57,10 +57,19 @@ export function makeUseListenerFunction<TParameters = []>(
   event: string
 ): EventListenerHook<TParameters> {
   return (eventEmitter, handler) => {
-    useEffect(
-      () => listenForEvent(eventEmitter, event, handler),
-      [eventEmitter, handler]
-    );
+    const eventEmitterRef = useRef<typeof eventEmitter>(null);
+    const handlerRef = useRef<typeof handler>(() => false);
+
+    if (
+      eventEmitterRef.current != eventEmitter ||
+      handlerRef.current != handler
+    ) {
+      eventEmitterRef.current?.off(event, handlerRef.current);
+      eventEmitter?.on(event, handler);
+    }
+
+    eventEmitterRef.current = eventEmitter;
+    handlerRef.current = handler;
   };
 }