feat: Show loading spinners immediately in ui (#1023)

I can't find any tickets on this even though I thought we had some. Fixes a few issues 1. Immediately open a panel and show a loading spinner before server rendering is complete The panel will be re-used and have its title updated if necessary when rendering completes. Works with multiple panels (opens 1 panel for loading and then re-uses for the 1st of multiple panels) 2. Shows a loader for all panels on page reload until rendering is complete. Previously, we rendered a blank panel until re-hydration was complete 3. If a widget throws a rendering error, the state is reset to loading when the "Reload" button is pressed. A loader is immediately shown until the reload is over instead of just showing the error message.
deephaven · Nov 20, 2024 · 3748dac · 3748dac
1 parent 701b004
commit 3748dac
Show file tree

Hide file tree

Showing 8 changed files with 179 additions and 67 deletions.
diff --git a/plugins/ui/src/js/src/DashboardPlugin.tsx b/plugins/ui/src/js/src/DashboardPlugin.tsx
@@ -27,10 +27,12 @@ import {
 import PortalPanel from './layout/PortalPanel';
 import PortalPanelManager from './layout/PortalPanelManager';
 import DashboardWidgetHandler from './widget/DashboardWidgetHandler';
-import { getPreservedData } from './widget/WidgetUtils';
+import {
+  getPreservedData,
+  DASHBOARD_ELEMENT,
+  WIDGET_ELEMENT,
+} from './widget/WidgetUtils';
 
-const NAME_ELEMENT = 'deephaven.ui.Element';
-const DASHBOARD_ELEMENT = 'deephaven.ui.Dashboard';
 const PLUGIN_NAME = '@deephaven/js-plugin-ui.DashboardPlugin';
 
 const log = Log.module('@deephaven/js-plugin-ui.DashboardPlugin');
@@ -119,7 +121,7 @@ export function DashboardPlugin(
       const { type } = widget;
 
       switch (type) {
-        case NAME_ELEMENT: {
+        case WIDGET_ELEMENT: {
           handleWidgetOpen({ widgetId, widget });
           break;
         }

diff --git a/plugins/ui/src/js/src/widget/WidgetHandler.tsx b/plugins/ui/src/js/src/widget/WidgetHandler.tsx
@@ -37,11 +37,16 @@ import {
   METHOD_DOCUMENT_UPDATED,
 } from './WidgetTypes';
 import DocumentHandler from './DocumentHandler';
-import { getComponentForElement, wrapCallable } from './WidgetUtils';
+import {
+  getComponentForElement,
+  WIDGET_ELEMENT,
+  wrapCallable,
+} from './WidgetUtils';
 import WidgetStatusContext, {
   WidgetStatus,
 } from '../layout/WidgetStatusContext';
 import WidgetErrorView from './WidgetErrorView';
+import ReactPanel from '../layout/ReactPanel';
 
 const log = Log.module('@deephaven/js-plugin-ui/WidgetHandler');
 
@@ -77,13 +82,33 @@ function WidgetHandler({
     setIsLoading(true);
   }
 
-  const [document, setDocument] = useState<ReactNode>();
+  if (widgetError != null && isLoading) {
+    setIsLoading(false);
+  }
 
   // We want to update the initial data if the widget changes, as we'll need to re-fetch the widget and want to start with a fresh state.
   // eslint-disable-next-line react-hooks/exhaustive-deps
   const initialData = useMemo(() => initialDataProp, [widget]);
   const [internalError, setInternalError] = useState<WidgetError>();
 
+  const [document, setDocument] = useState<ReactNode>(() => {
+    if (widgetDescriptor.type === WIDGET_ELEMENT) {
+      // Rehydration. Mount ReactPanels for each panelId in the initial data
+      // so loading spinners or widget errors are shown
+      if (initialData?.panelIds != null && initialData.panelIds.length > 0) {
+        // Do not add a key here
+        // When the real document mounts, it doesn't use keys and will cause a remount
+        // which triggers the DocumentHandler to think the panels were closed and messes up the layout
+        // eslint-disable-next-line react/jsx-key
+        return initialData.panelIds.map(() => <ReactPanel />);
+      }
+      // Default to a single panel so we can immediately show a loading spinner
+      return <ReactPanel />;
+    }
+    // Dashboards should not have a default document. It breaks its render flow
+    return null;
+  });
+
   const error = useMemo(
     () => internalError ?? widgetError ?? undefined,
     [internalError, widgetError]
@@ -261,9 +286,16 @@ function WidgetHandler({
         const newError: WidgetError = JSON.parse(params[0]);
         newError.action = {
           title: 'Reload',
-          action: () => sendSetState(),
+          action: () => {
+            setInternalError(undefined);
+            setIsLoading(true);
+            sendSetState();
+          },
         };
-        setInternalError(newError);
+        unstable_batchedUpdates(() => {
+          setIsLoading(false);
+          setInternalError(newError);
+        });
       });
 
       return () => {
@@ -345,48 +377,34 @@ function WidgetHandler({
       return document;
     }
     if (error != null) {
-      // If there's an error and the document hasn't rendered yet, explicitly show an error view
+      // If there's an error and the document hasn't rendered yet (mostly applies to dashboards), explicitly show an error view
       return <WidgetErrorView error={error} />;
     }
-    return null;
+    return document;
   }, [document, error]);
 
   const widgetStatus: WidgetStatus = useMemo(() => {
-    if (error != null) {
-      return { status: 'error', descriptor: widgetDescriptor, error };
-    }
     if (isLoading) {
       return { status: 'loading', descriptor: widgetDescriptor };
     }
-    if (renderedDocument != null) {
-      return { status: 'ready', descriptor: widgetDescriptor };
+    if (error != null) {
+      return { status: 'error', descriptor: widgetDescriptor, error };
     }
-    return { status: 'loading', descriptor: widgetDescriptor };
-  }, [error, renderedDocument, widgetDescriptor, isLoading]);
-
-  return useMemo(
-    () =>
-      renderedDocument ? (
-        <WidgetStatusContext.Provider value={widgetStatus}>
-          <DocumentHandler
-            widget={widgetDescriptor}
-            initialData={initialData}
-            onDataChange={onDataChange}
-            onClose={onClose}
-          >
-            {renderedDocument}
-          </DocumentHandler>
-        </WidgetStatusContext.Provider>
-      ) : null,
-    [
-      widgetDescriptor,
-      renderedDocument,
-      initialData,
-      onClose,
-      onDataChange,
-      widgetStatus,
-    ]
-  );
+    return { status: 'ready', descriptor: widgetDescriptor };
+  }, [error, widgetDescriptor, isLoading]);
+
+  return renderedDocument != null ? (
+    <WidgetStatusContext.Provider value={widgetStatus}>
+      <DocumentHandler
+        widget={widgetDescriptor}
+        initialData={initialData}
+        onDataChange={onDataChange}
+        onClose={onClose}
+      >
+        {renderedDocument}
+      </DocumentHandler>
+    </WidgetStatusContext.Provider>
+  ) : null;
 }
 
 WidgetHandler.displayName = '@deephaven/js-plugin-ui/WidgetHandler';

diff --git a/plugins/ui/src/js/src/widget/WidgetUtils.tsx b/plugins/ui/src/js/src/widget/WidgetUtils.tsx
@@ -82,6 +82,9 @@ import {
   Tabs,
 } from '../elements';
 
+export const WIDGET_ELEMENT = 'deephaven.ui.Element';
+export const DASHBOARD_ELEMENT = 'deephaven.ui.Dashboard';
+
 /**
  * Elements to implicitly wrap primitive children in <Text> components.
  */

diff --git a/tests/app.d/tests.app b/tests/app.d/tests.app
@@ -8,4 +8,5 @@ file_1=matplotlib.py
 file_2=ui.py
 file_3=ui_render_all.py
 file_4=ui_flex.py
-file_5=ui_table.py
+file_5=ui_table.py
+file_6=ui_panel_loading.py
diff --git a/tests/app.d/ui_panel_loading.py b/tests/app.d/ui_panel_loading.py
@@ -0,0 +1,28 @@
+from deephaven import ui
+import time
+
+
+@ui.component
+def ui_boom_button():
+    value, set_value = ui.use_state(0)
+
+    if value > 0:
+        raise ValueError("BOOM! Value too big.")
+
+    return ui.button("Go BOOM!", on_press=lambda _: set_value(value + 1))
+
+
+@ui.component
+def ui_slow_multi_panel_component():
+    is_mounted, set_is_mounted = ui.use_state(None)
+    if not is_mounted:
+        time.sleep(1)
+        set_is_mounted(ui_boom_button)  # type: ignore Use a complex value that won't save between page loads
+    return [
+        ui.panel(ui.button("Hello")),
+        ui.panel(ui.text("World")),
+        ui.panel(ui_boom_button()),
+    ]
+
+
+ui_slow_multi_panel = ui_slow_multi_panel_component()
diff --git a/tests/ui.spec.ts b/tests/ui.spec.ts
@@ -1,38 +1,33 @@
 import { expect, test } from '@playwright/test';
-import { openPanel, gotoPage } from './utils';
-
-const selector = {
-  REACT_PANEL_VISIBLE: '.dh-react-panel:visible',
-  REACT_PANEL_OVERLAY: '.dh-react-panel-overlay',
-};
+import { openPanel, gotoPage, SELECTORS } from './utils';
 
 test('UI loads', async ({ page }) => {
   await gotoPage(page, '');
-  await openPanel(page, 'ui_component', selector.REACT_PANEL_VISIBLE);
-  await expect(page.locator(selector.REACT_PANEL_VISIBLE)).toHaveScreenshot();
+  await openPanel(page, 'ui_component', SELECTORS.REACT_PANEL_VISIBLE);
+  await expect(page.locator(SELECTORS.REACT_PANEL_VISIBLE)).toHaveScreenshot();
 });
 
 test('boom component shows an error in a panel', async ({ page }) => {
   await gotoPage(page, '');
-  await openPanel(page, 'ui_boom', selector.REACT_PANEL_VISIBLE);
-  await expect(page.locator(selector.REACT_PANEL_VISIBLE)).toBeVisible();
+  await openPanel(page, 'ui_boom', SELECTORS.REACT_PANEL_VISIBLE);
+  await expect(page.locator(SELECTORS.REACT_PANEL_VISIBLE)).toBeVisible();
   await expect(
     page
-      .locator(selector.REACT_PANEL_VISIBLE)
+      .locator(SELECTORS.REACT_PANEL_VISIBLE)
       .getByText('Exception', { exact: true })
   ).toBeVisible();
   await expect(
-    page.locator(selector.REACT_PANEL_VISIBLE).getByText('BOOM!')
+    page.locator(SELECTORS.REACT_PANEL_VISIBLE).getByText('BOOM!')
   ).toBeVisible();
 });
 
 test('boom counter component shows error overlay after clicking the button twice', async ({
   page,
 }) => {
   await gotoPage(page, '');
-  await openPanel(page, 'ui_boom_counter', selector.REACT_PANEL_VISIBLE);
+  await openPanel(page, 'ui_boom_counter', SELECTORS.REACT_PANEL_VISIBLE);
 
-  const panelLocator = page.locator(selector.REACT_PANEL_VISIBLE);
+  const panelLocator = page.locator(SELECTORS.REACT_PANEL_VISIBLE);
 
   let btn = await panelLocator.getByRole('button', { name: 'Count is 0' });
   await expect(btn).toBeVisible();
@@ -50,7 +45,7 @@ test('boom counter component shows error overlay after clicking the button twice
 
 test('Using keys for lists works', async ({ page }) => {
   await gotoPage(page, '');
-  await openPanel(page, 'ui_cells', selector.REACT_PANEL_VISIBLE);
+  await openPanel(page, 'ui_cells', SELECTORS.REACT_PANEL_VISIBLE);
 
   // setup cells
   await page.getByRole('button', { name: 'Add cell' }).click();
@@ -69,14 +64,14 @@ test('Using keys for lists works', async ({ page }) => {
 
 test('UI all components render 1', async ({ page }) => {
   await gotoPage(page, '');
-  await openPanel(page, 'ui_render_all1', selector.REACT_PANEL_VISIBLE);
-  await expect(page.locator(selector.REACT_PANEL_VISIBLE)).toHaveScreenshot();
+  await openPanel(page, 'ui_render_all1', SELECTORS.REACT_PANEL_VISIBLE);
+  await expect(page.locator(SELECTORS.REACT_PANEL_VISIBLE)).toHaveScreenshot();
 });
 
 test('UI all components render 2', async ({ page }) => {
   await gotoPage(page, '');
-  await openPanel(page, 'ui_render_all2', selector.REACT_PANEL_VISIBLE);
-  await expect(page.locator(selector.REACT_PANEL_VISIBLE)).toHaveScreenshot();
+  await openPanel(page, 'ui_render_all2', SELECTORS.REACT_PANEL_VISIBLE);
+  await expect(page.locator(SELECTORS.REACT_PANEL_VISIBLE)).toHaveScreenshot();
 });
 
 // Tests flex components render as expected
@@ -110,18 +105,18 @@ test.describe('UI flex components', () => {
   ].forEach(i => {
     test(i.name, async ({ page }) => {
       await gotoPage(page, '');
-      await openPanel(page, i.name, selector.REACT_PANEL_VISIBLE);
+      await openPanel(page, i.name, SELECTORS.REACT_PANEL_VISIBLE);
 
       // need to wait for plots to be loaded before taking screenshot
       // easiest way to check that is if the traces are present
       if (i.traces > 0) {
         await expect(
-          await page.locator(selector.REACT_PANEL_VISIBLE).locator('.trace')
+          await page.locator(SELECTORS.REACT_PANEL_VISIBLE).locator('.trace')
         ).toHaveCount(i.traces);
       }
 
       await expect(
-        page.locator(selector.REACT_PANEL_VISIBLE)
+        page.locator(SELECTORS.REACT_PANEL_VISIBLE)
       ).toHaveScreenshot();
     });
   });

diff --git a/tests/ui_loading.spec.ts b/tests/ui_loading.spec.ts
@@ -0,0 +1,55 @@
+import { expect, test } from '@playwright/test';
+import { openPanel, gotoPage, SELECTORS } from './utils';
+
+test('slow multi-panel shows 1 loader immediately and multiple after loading', async ({
+  page,
+}) => {
+  await gotoPage(page, '');
+  await openPanel(
+    page,
+    'ui_slow_multi_panel',
+    SELECTORS.REACT_PANEL_VISIBLE,
+    false
+  );
+  const locator = page.locator(SELECTORS.REACT_PANEL);
+  // 1 loader should show up
+  await expect(locator.locator('.loading-spinner')).toHaveCount(1);
+  // Then disappear and show 3 panels
+  await expect(locator.locator('.loading-spinner')).toHaveCount(0);
+  await expect(locator).toHaveCount(3);
+  await expect(locator.getByText('Hello')).toHaveCount(1);
+  await expect(locator.getByText('World')).toHaveCount(1);
+  await expect(locator.getByText('Go BOOM!')).toHaveCount(1);
+});
+
+test('slow multi-panel shows loaders on element Reload', async ({ page }) => {
+  await gotoPage(page, '');
+  await openPanel(page, 'ui_slow_multi_panel', SELECTORS.REACT_PANEL_VISIBLE);
+  const locator = page.locator(SELECTORS.REACT_PANEL);
+  await expect(locator).toHaveCount(3);
+  await locator.getByText('Go BOOM!').click();
+  await expect(locator.getByText('ValueError', { exact: true })).toHaveCount(3);
+  await expect(locator.getByText('BOOM!')).toHaveCount(3);
+  await locator.locator(':visible').getByText('Reload').first().click();
+  // Loaders should show up
+  await expect(locator.locator('.loading-spinner')).toHaveCount(3);
+  // Then disappear and show components again
+  await expect(locator.locator('.loading-spinner')).toHaveCount(0);
+  await expect(locator.getByText('Hello')).toHaveCount(1);
+  await expect(locator.getByText('World')).toHaveCount(1);
+  await expect(locator.getByText('Go BOOM!')).toHaveCount(1);
+});
+
+test('slow multi-panel shows loaders on page reload', async ({ page }) => {
+  await gotoPage(page, '');
+  await openPanel(page, 'ui_slow_multi_panel', SELECTORS.REACT_PANEL_VISIBLE);
+  await page.reload();
+  const locator = page.locator(SELECTORS.REACT_PANEL);
+  // Loader should show up
+  await expect(locator.locator('.loading-spinner')).toHaveCount(3);
+  // Then disappear and show error again
+  await expect(locator.locator('.loading-spinner')).toHaveCount(0);
+  await expect(locator.getByText('Hello')).toHaveCount(1);
+  await expect(locator.getByText('World')).toHaveCount(1);
+  await expect(locator.getByText('Go BOOM!')).toHaveCount(1);
+});