From 3748dac818f1056b6520f8ad637e1a54e9627ceb Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 20 Nov 2024 13:43:15 -0600 Subject: [PATCH] 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. --- plugins/ui/src/js/src/DashboardPlugin.tsx | 10 +- .../ui/src/js/src/widget/WidgetHandler.tsx | 92 +++++++++++-------- plugins/ui/src/js/src/widget/WidgetUtils.tsx | 3 + tests/app.d/tests.app | 3 +- tests/app.d/ui_panel_loading.py | 28 ++++++ tests/ui.spec.ts | 39 ++++---- tests/ui_loading.spec.ts | 55 +++++++++++ tests/utils.ts | 16 +++- 8 files changed, 179 insertions(+), 67 deletions(-) create mode 100644 tests/app.d/ui_panel_loading.py create mode 100644 tests/ui_loading.spec.ts diff --git a/plugins/ui/src/js/src/DashboardPlugin.tsx b/plugins/ui/src/js/src/DashboardPlugin.tsx index 614dfe17f..9f27f04cf 100644 --- 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 index 62e740616..c4b518605 100644 --- 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(); + 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(); + const [document, setDocument] = useState(() => { + 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(() => ); + } + // Default to a single panel so we can immediately show a loading spinner + return ; + } + // 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 ; } - 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 ? ( - - - {renderedDocument} - - - ) : null, - [ - widgetDescriptor, - renderedDocument, - initialData, - onClose, - onDataChange, - widgetStatus, - ] - ); + return { status: 'ready', descriptor: widgetDescriptor }; + }, [error, widgetDescriptor, isLoading]); + + return renderedDocument != null ? ( + + + {renderedDocument} + + + ) : 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 index e29d3c1ea..014a1c6e4 100644 --- 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 components. */ diff --git a/tests/app.d/tests.app b/tests/app.d/tests.app index d9c378984..8d273c183 100644 --- 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 \ No newline at end of file +file_5=ui_table.py +file_6=ui_panel_loading.py \ No newline at end of file diff --git a/tests/app.d/ui_panel_loading.py b/tests/app.d/ui_panel_loading.py new file mode 100644 index 000000000..a09b62cc2 --- /dev/null +++ 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 index 144f7bbb3..7e182bc11 100644 --- a/tests/ui.spec.ts +++ b/tests/ui.spec.ts @@ -1,28 +1,23 @@ 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(); }); @@ -30,9 +25,9 @@ test('boom counter component shows error overlay after clicking the button twice 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 new file mode 100644 index 000000000..ce4c8e999 --- /dev/null +++ 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); +}); diff --git a/tests/utils.ts b/tests/utils.ts index 7c25458c5..f5c4eb058 100644 --- a/tests/utils.ts +++ b/tests/utils.ts @@ -1,5 +1,11 @@ import test, { Page, expect } from '@playwright/test'; +export const SELECTORS = { + REACT_PANEL: '.dh-react-panel', + REACT_PANEL_VISIBLE: '.dh-react-panel:visible', + REACT_PANEL_OVERLAY: '.dh-react-panel-overlay', +}; + /** * Goes to a page and waits for the progress bar to disappear * @param page The page @@ -28,12 +34,14 @@ export async function gotoPage( * @param page The page * @param name The name of the panel * @param panelLocator The locator for the panel, passed to `page.locator` + * @param awaitLoad If we should wait for the loading spinner to disappear */ export async function openPanel( page: Page, name: string, - panelLocator = '.dh-panel' -) { + panelLocator = '.dh-panel', + awaitLoad = true +): Promise { await test.step(`Open panel (${name})`, async () => { const panelCount = await page.locator(panelLocator).count(); @@ -62,6 +70,8 @@ export async function openPanel( // check for panel to be loaded await expect(page.locator(panelLocator)).toHaveCount(panelCount + 1); - await expect(page.locator('.loading-spinner')).toHaveCount(0); + if (awaitLoad) { + await expect(page.locator('.loading-spinner')).toHaveCount(0); + } }); }