From 6bdb9705fe39220758204cf7778101f9bb1a0167 Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 10 Jul 2026 13:33:02 -0400 Subject: [PATCH 1/7] test: add dashboard persistence e2e tests (DH-19717) Adds e2e tests and fixtures covering dashboard/layout persistence across a page refresh: input values, moved panels, active tab in a stack, active tab in ui.tabs, and custom columns on ui.table. --- tests/app.d/tests.app | 1 + tests/app.d/ui_dashboard_persistence.py | 138 ++++++++++ tests/ui_dashboard_persistence.spec.ts | 341 ++++++++++++++++++++++++ 3 files changed, 480 insertions(+) create mode 100644 tests/app.d/ui_dashboard_persistence.py create mode 100644 tests/ui_dashboard_persistence.spec.ts diff --git a/tests/app.d/tests.app b/tests/app.d/tests.app index 1299210a0..27d54ece5 100644 --- a/tests/app.d/tests.app +++ b/tests/app.d/tests.app @@ -24,3 +24,4 @@ file_17=ui_events.py file_18=ui_memo.py file_19=ui_combo_box.py file_20=ui_multi_select.py +file_21=ui_dashboard_persistence.py diff --git a/tests/app.d/ui_dashboard_persistence.py b/tests/app.d/ui_dashboard_persistence.py new file mode 100644 index 000000000..fb73796c5 --- /dev/null +++ b/tests/app.d/ui_dashboard_persistence.py @@ -0,0 +1,138 @@ +""" +Test fixtures for dashboard/layout persistence. + +These fixtures back the tests in ``tests/ui_dashboard_persistence.spec.ts``. +Each exercises a different kind of state that should survive a page refresh +when the layout is persisted (i.e. "Close Panels on Disconnect" disabled): + +- ``ui_persist_inputs``: controlled text fields and pickers keep their values. +- ``ui_persist_move_panel``: a moved panel stays where it was dropped. +- ``ui_persist_stack``: the active tab of a stack of panels is retained. +- ``ui_persist_tabs``: the selected tab of a ``ui.tabs`` component is retained. +- ``ui_persist_table_columns``: custom columns added to a ``ui.table`` persist. +""" + +from deephaven import ui, empty_table + + +@ui.component +def persist_inputs_component(): + """A panel with controlled text fields and pickers. + + The values are held in server-side state so that, when the widget + reconnects after a refresh, the selected values should be restored. + """ + text, set_text = ui.use_state("") + number_text, set_number_text = ui.use_state("") + color, set_color = ui.use_state(None) + fruit, set_fruit = ui.use_state(None) + + return ui.panel( + ui.text_field( + label="Name", + value=text, + on_change=set_text, + ), + ui.text_field( + label="Amount", + value=number_text, + on_change=set_number_text, + ), + ui.picker( + "Red", + "Green", + "Blue", + label="Color", + selected_key=color, + on_selection_change=set_color, + ), + ui.picker( + "Apple", + "Banana", + "Cherry", + label="Fruit", + selected_key=fruit, + on_selection_change=set_fruit, + ), + title="Persist Inputs", + ) + + +@ui.component +def persist_move_panel_component(): + """A nested dashboard whose panels can be rearranged via drag-and-drop. + + The panels start stacked together; a test drags one into its own stack and + verifies the new location is retained after a refresh. + """ + return ui.panel( + ui.dashboard( + ui.stack( + ui.panel(ui.text("Content A"), title="Move Panel A"), + ui.panel(ui.text("Content B"), title="Move Panel B"), + ) + ), + title="Persist Move Panel", + ) + + +@ui.component +def persist_stack_component(): + """A nested dashboard with a single stack of three panels. + + The active tab of the stack should persist across a refresh. + """ + return ui.panel( + ui.dashboard( + ui.stack( + ui.panel(ui.text("Content One"), title="Stack Panel 1"), + ui.panel(ui.text("Content Two"), title="Stack Panel 2"), + ui.panel(ui.text("Content Three"), title="Stack Panel 3"), + ) + ), + title="Persist Stack", + ) + + +@ui.component +def persist_tabs_component(): + """A panel containing a ``ui.tabs`` component. + + The selected tab should persist across a refresh. + """ + return ui.panel( + ui.tabs( + ui.tab(ui.text("This is the content of the first tab."), title="Tab One"), + ui.tab(ui.text("This is the content of the second tab."), title="Tab Two"), + ui.tab(ui.text("This is the content of the third tab."), title="Tab Three"), + ), + title="Persist Tabs", + ) + + +@ui.component +def persist_table_columns_component(): + """A nested dashboard with multiple tables in a stack. + + Custom columns added to a table through the UI should persist across a + refresh. + """ + _t1 = empty_table(20).update(["a = i", "b = i * 2"]) + _t2 = empty_table(20).update(["c = i", "d = i * 3"]) + return ui.panel( + ui.dashboard( + ui.stack( + ui.panel(ui.table(_t1), title="Table One"), + ui.panel(ui.table(_t2), title="Table Two"), + ) + ), + title="Persist Table Columns", + ) + + +# Export the test components +ui_persist_inputs = persist_inputs_component() +ui_persist_move_panel = persist_move_panel_component() +ui_persist_stack = persist_stack_component() +ui_persist_tabs = persist_tabs_component() +ui_persist_table_columns = persist_table_columns_component() diff --git a/tests/ui_dashboard_persistence.spec.ts b/tests/ui_dashboard_persistence.spec.ts new file mode 100644 index 000000000..630c6275b --- /dev/null +++ b/tests/ui_dashboard_persistence.spec.ts @@ -0,0 +1,341 @@ +import { expect, test, type Locator, type Page } from '@playwright/test'; +import { openPanel, gotoPage, waitForLoad, SELECTORS } from './utils'; + +/** + * Disables "Close Panels on Disconnect" so the layout (and the widget session) + * is persisted across a page refresh, waits for the setting/layout to be saved, + * then reloads the page. + * @param page The page + */ +async function persistLayoutAndReload(page: Page): Promise { + await test.step('Persist layout and reload', async () => { + // Reset mouse position to not cause unintended hover effects + await page.mouse.move(0, 0); + + await page + .getByRole('button', { name: 'More Actions...', exact: true }) + .click(); + await page + .getByRole('button', { name: 'Close Panels on Disconnect', exact: true }) + .click(); + + // Wait for the debounced layout/settings to be saved before refreshing + await page.waitForTimeout(2000); + + await page.reload(); + await waitForLoad(page); + }); +} + +/** + * Performs a golden-layout tab drag by manually driving the mouse. A simple + * `dragTo` does not work with golden-layout because it relies on a sequence of + * mousedown/mousemove/mouseup events and a drag threshold before the drag proxy + * is created. + * @param page The page + * @param tab The tab to drag + * @param target The target point to drop the tab at + */ +async function dragTabToTarget( + page: Page, + tab: Locator, + target: { x: number; y: number } +): Promise { + const box = await tab.boundingBox(); + if (box == null) { + throw new Error('Could not get bounding box for tab to drag'); + } + const startX = box.x + box.width / 2; + const startY = box.y + box.height / 2; + + await page.mouse.move(startX, startY); + await page.mouse.down(); + // Exceed the drag threshold to start the golden-layout drag proxy + await page.mouse.move(startX + 15, startY + 15, { steps: 5 }); + // Move towards the drop target + await page.mouse.move(target.x, target.y, { steps: 20 }); + // Settle on the target so golden-layout registers the drop zone + await page.mouse.move(target.x, target.y, { steps: 5 }); + await page.mouse.up(); +} + +/** + * Selects an option from a `ui.picker`. + * @param page The page + * @param scope The locator to scope the picker search to + * @param label The label of the picker + * @param option The option to select + */ +async function selectPickerOption( + page: Page, + scope: Locator, + label: string, + option: string +): Promise { + await scope.getByRole('button', { name: new RegExp(label) }).click(); + const listbox = page.getByRole('listbox'); + await expect(listbox).toBeVisible(); + await listbox.getByRole('option', { name: option, exact: true }).click(); +} + +test.describe('Dashboard persistence', () => { + // Input persistence: text fields and pickers should keep their values after a + // refresh when the layout is persisted. + test('input values persist across a refresh', async ({ page }) => { + await gotoPage(page, ''); + await openPanel( + page, + 'ui_persist_inputs', + SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE, + true + ); + + const panel = page.locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE).first(); + await expect(panel).toBeVisible(); + + // Change all inputs away from their default (empty/unselected) values + await panel.getByLabel('Name').fill('Alice'); + await panel.getByLabel('Amount').fill('42'); + await selectPickerOption(page, panel, 'Color', 'Green'); + await selectPickerOption(page, panel, 'Fruit', 'Banana'); + + // Sanity check the values were applied before the refresh + await expect(panel.getByLabel('Name')).toHaveValue('Alice'); + await expect(panel.getByLabel('Amount')).toHaveValue('42'); + await expect(panel.getByRole('button', { name: /Color/ })).toContainText( + 'Green' + ); + await expect(panel.getByRole('button', { name: /Fruit/ })).toContainText( + 'Banana' + ); + + await persistLayoutAndReload(page); + + // The panel should be restored with all of its input values intact + const restoredPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(restoredPanel).toBeVisible(); + + await expect(restoredPanel.getByLabel('Name')).toHaveValue('Alice'); + await expect(restoredPanel.getByLabel('Amount')).toHaveValue('42'); + await expect( + restoredPanel.getByRole('button', { name: /Color/ }) + ).toContainText('Green'); + await expect( + restoredPanel.getByRole('button', { name: /Fruit/ }) + ).toContainText('Banana'); + }); + + // Panel move persistence: a panel that is dragged into its own stack should + // stay in that location after a refresh. + test('moved panel stays in place across a refresh', async ({ page }) => { + await gotoPage(page, ''); + await openPanel( + page, + 'ui_persist_move_panel', + SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE, + true + ); + + const outerPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(outerPanel).toBeVisible(); + + // The nested dashboard starts with both panels in a single stack + await expect(outerPanel.locator('.lm_stack')).toHaveCount(1); + + // Drag "Move Panel B" to the right edge of the nested dashboard to split it + // into its own stack + const dragTab = outerPanel.locator('.lm_tab', { hasText: 'Move Panel B' }); + const dashboardBox = await outerPanel.boundingBox(); + if (dashboardBox == null) { + throw new Error('Could not get bounding box for the nested dashboard'); + } + await dragTabToTarget(page, dragTab, { + x: dashboardBox.x + dashboardBox.width * 0.85, + y: dashboardBox.y + dashboardBox.height * 0.5, + }); + + // After the drag the panels should be in two separate stacks + await expect(outerPanel.locator('.lm_stack')).toHaveCount(2); + + // Record the location of the dragged panel so we can verify it does not move + // after the refresh + const beforeBox = await outerPanel + .locator('.lm_tab', { hasText: 'Move Panel B' }) + .boundingBox(); + if (beforeBox == null) { + throw new Error('Could not get bounding box for the dragged tab'); + } + + await persistLayoutAndReload(page); + + const restoredPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(restoredPanel).toBeVisible(); + + // The panels should still be in two separate stacks after the refresh... + await expect(restoredPanel.locator('.lm_stack')).toHaveCount(2); + + // ...and the dragged panel should be in the same location it was dropped + const afterBox = await restoredPanel + .locator('.lm_tab', { hasText: 'Move Panel B' }) + .boundingBox(); + if (afterBox == null) { + throw new Error('Could not get bounding box for the restored tab'); + } + expect(Math.abs(afterBox.x - beforeBox.x)).toBeLessThan(10); + expect(Math.abs(afterBox.y - beforeBox.y)).toBeLessThan(10); + }); + + // Active panel in a stack: with multiple panels stacked together, the active + // tab should persist across a refresh. + test('active tab in a stack persists across a refresh', async ({ page }) => { + await gotoPage(page, ''); + await openPanel( + page, + 'ui_persist_stack', + SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE, + true + ); + + const outerPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(outerPanel).toBeVisible(); + + // Activate the middle panel in the stack (distinguishes from a layout that + // defaults to the first or last tab) + await outerPanel.locator('.lm_tab', { hasText: 'Stack Panel 2' }).click(); + await expect(outerPanel.getByText('Content Two')).toBeVisible(); + await expect(outerPanel.locator('.lm_tab.lm_active')).toContainText( + 'Stack Panel 2' + ); + + await persistLayoutAndReload(page); + + const restoredPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(restoredPanel).toBeVisible(); + + // The middle panel should still be the active tab after the refresh + await expect(restoredPanel.locator('.lm_tab.lm_active')).toContainText( + 'Stack Panel 2' + ); + await expect(restoredPanel.getByText('Content Two')).toBeVisible(); + }); + + // Active tab in a ui.tabs component should persist across a refresh. + test('active tab in ui.tabs persists across a refresh', async ({ page }) => { + await gotoPage(page, ''); + await openPanel( + page, + 'ui_persist_tabs', + SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE, + true + ); + + const outerPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(outerPanel).toBeVisible(); + + // Switch to the third tab + await outerPanel.getByRole('tab', { name: 'Tab Three' }).first().click(); + await expect( + outerPanel.getByText('This is the content of the third tab.') + ).toBeVisible(); + await expect( + outerPanel.getByRole('tab', { name: 'Tab Three' }).first() + ).toHaveAttribute('aria-selected', 'true'); + + await persistLayoutAndReload(page); + + const restoredPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(restoredPanel).toBeVisible(); + + // The third tab should still be selected after the refresh + await expect( + restoredPanel.getByRole('tab', { name: 'Tab Three' }).first() + ).toHaveAttribute('aria-selected', 'true'); + await expect( + restoredPanel.getByText('This is the content of the third tab.') + ).toBeVisible(); + }); + + // ui.table persistence: custom columns added to a table through the UI should + // persist across a refresh. + test('custom columns on a ui.table persist across a refresh', async ({ + page, + }) => { + await gotoPage(page, ''); + await openPanel( + page, + 'ui_persist_table_columns', + SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE, + true + ); + + const outerPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(outerPanel).toBeVisible(); + + // Make sure "Table One" (columns a, b) is the active table in the stack + await outerPanel.locator('.lm_tab', { hasText: 'Table One' }).click(); + await waitForLoad(page); + + // Open Table Options -> Custom Columns and add a custom column + await page.getByRole('button', { name: 'Table Options' }).click(); + await page.getByTestId('menu-item-Custom Columns').click(); + + // Enter the formula first, then the name: the builder resets the name field + // shortly after it mounts, so setting the name last ensures it sticks + const formulaEditor = page + .locator('.custom-column-input-container .monaco-editor') + .first(); + await formulaEditor.click(); + await page.keyboard.type('a * 2'); + await expect(formulaEditor.locator('textarea')).toHaveValue('a * 2'); + + const columnNameInput = page.getByRole('textbox', { name: 'Column Name' }); + await columnNameInput.fill('Doubled'); + await expect(columnNameInput).toHaveValue('Doubled'); + + await page + .getByRole('button', { name: 'Save Column', exact: true }) + .click(); + // Give the custom column time to be applied to the table + await page.waitForTimeout(1500); + + // Navigate back to the Table Options menu and confirm the custom column was + // applied by checking the Organize Columns list + await page.getByRole('button', { name: 'Back', exact: true }).click(); + await page.getByTestId('menu-item-Organize Columns').click(); + await expect(page.locator('.visibility-ordering-builder')).toContainText( + 'Doubled' + ); + + await persistLayoutAndReload(page); + + const restoredPanel = page + .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) + .first(); + await expect(restoredPanel).toBeVisible(); + await restoredPanel.locator('.lm_tab', { hasText: 'Table One' }).click(); + await waitForLoad(page); + + // The custom column should still be present after the refresh + await page.getByRole('button', { name: 'Table Options' }).click(); + await page.getByTestId('menu-item-Organize Columns').click(); + await expect(page.locator('.visibility-ordering-builder')).toContainText( + 'Doubled' + ); + }); +}); From 3f131e8c6a30eec3a12f57b676d3899d6b57d08c Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 10 Jul 2026 13:50:09 -0400 Subject: [PATCH 2/7] fix(ui): persist active tab in ui.tabs across refresh (DH-19717) Store the selected tab in the panel's persistent state via usePersistentState so it is restored on rehydration. When the server controls the selection (selectedKey provided) it remains the source of truth. --- plugins/ui/src/js/src/elements/Tabs.tsx | 40 +++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/plugins/ui/src/js/src/elements/Tabs.tsx b/plugins/ui/src/js/src/elements/Tabs.tsx index 056c05be0..d4e9a739d 100644 --- a/plugins/ui/src/js/src/elements/Tabs.tsx +++ b/plugins/ui/src/js/src/elements/Tabs.tsx @@ -1,4 +1,9 @@ -import React, { type Key, type ReactElement, useMemo } from 'react'; +import React, { + type Key, + type ReactElement, + useCallback, + useMemo, +} from 'react'; import { Tabs as DHCTabs, type TabsProps, @@ -9,6 +14,7 @@ import { type TabListProps, } from '@deephaven/components'; import { isElementOfType } from '@deephaven/react-hooks'; +import { usePersistentState } from '@deephaven/plugin'; import { ensureArray } from '@deephaven/utils'; import classNames from 'classnames'; import { TabPanels } from './TabPanels'; @@ -83,11 +89,37 @@ export function Tabs(props: TabComponentProps): JSX.Element { onSelectionChange: onSelectionChangeProp, onChange, UNSAFE_className, + selectedKey, + defaultSelectedKey, ...otherTabProps } = props; const childrenArray = useMemo(() => ensureArray(children), [children]); - const onSelectionChange = onSelectionChangeProp ?? onChange; + // Persist the active tab so it is restored when the widget is rehydrated + // (e.g. after a page refresh). When the component is controlled by the server + // (`selectedKey` provided) the server remains the source of truth. + const [persistedKey, setPersistedKey] = usePersistentState( + defaultSelectedKey, + { type: 'UITabs', version: 1 } + ); + + const userOnSelectionChange = onSelectionChangeProp ?? onChange; + + const onSelectionChange = useCallback( + (key: Key) => { + setPersistedKey(key); + userOnSelectionChange?.(key); + }, + [setPersistedKey, userOnSelectionChange] + ); + + // When the server controls the selection, pass it straight through. + // Otherwise seed the (uncontrolled) default from the persisted value so the + // previously active tab is selected on rehydration. + const selectionProps = + selectedKey !== undefined + ? { selectedKey } + : { defaultSelectedKey: persistedKey ?? defaultSelectedKey }; const tabPanelsOrLists = childrenArray.filter( child => @@ -141,6 +173,8 @@ export function Tabs(props: TabComponentProps): JSX.Element { onSelectionChange={onSelectionChange} UNSAFE_className={classNames('dh-tabs', UNSAFE_className)} // eslint-disable-next-line react/jsx-props-no-spreading + {...selectionProps} + // eslint-disable-next-line react/jsx-props-no-spreading {...otherTabProps} > {children} @@ -153,6 +187,8 @@ export function Tabs(props: TabComponentProps): JSX.Element { onSelectionChange={onSelectionChange} UNSAFE_className={classNames('dh-tabs', UNSAFE_className)} // eslint-disable-next-line react/jsx-props-no-spreading + {...selectionProps} + // eslint-disable-next-line react/jsx-props-no-spreading {...otherTabProps} > {tabListChildren} From 4d5f97ad8f0219c89528bb7d5bc1088eec9e05b3 Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 10 Jul 2026 14:47:50 -0400 Subject: [PATCH 3/7] fix(ui): persist active tab in a nested dashboard stack (DH-19717) On rehydration, panels remount before their portals are registered, so openedMetadataRef starts undefined and the panel-open effect called setActiveContentItem for every panel (last one winning), clobbering the active tab restored from the persisted layout. Only bring a panel to the front when its metadata actually changed from a previously-seen value. --- plugins/ui/src/js/src/layout/ReactPanel.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/plugins/ui/src/js/src/layout/ReactPanel.tsx b/plugins/ui/src/js/src/layout/ReactPanel.tsx index d9442e54f..af98b3ac9 100644 --- a/plugins/ui/src/js/src/layout/ReactPanel.tsx +++ b/plugins/ui/src/js/src/layout/ReactPanel.tsx @@ -211,7 +211,15 @@ function ReactPanel({ } LayoutUtils.openComponent({ root: parent, config }); log.debug('Opened panel', panelId, config); - } else if (openedMetadataRef.current !== metadata) { + } else if ( + openedMetadataRef.current != null && + openedMetadataRef.current !== metadata + ) { + // The panel already exists and its metadata has changed, so bring it to + // the front of its stack. We intentionally skip this when there is no + // previously-seen metadata (`openedMetadataRef.current == null`), which + // is the case when a whole dashboard is being rehydrated - stealing the + // active item then would clobber the persisted active tab. const contentItem = LayoutUtils.getContentItemInStack( existingStack, itemConfig From 2aed8f51fdb56b0c4549ba686d1b9737560fcd91 Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 10 Jul 2026 16:34:54 -0400 Subject: [PATCH 4/7] fix(ui): don't drop debounced input changes on server re-render (DH-19717) useDebouncedCallback cancels its pending call whenever the callback reference changes. Because the server hands each input a new onChange callable on every re-render, editing one field and then another in quick succession caused the first field's re-render to cancel the second field's still-pending debounced update - so that value never reached the server and was lost on refresh. Hold propOnChange in a ref so the debounced callback stays stable across re-renders. --- .../src/elements/hooks/useDebouncedOnChange.ts | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts b/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts index ce8bd5bc9..44f902d8b 100644 --- a/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts +++ b/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts @@ -1,4 +1,4 @@ -import { useState, useCallback } from 'react'; +import { useState, useCallback, useRef, useEffect } from 'react'; import Log from '@deephaven/log'; import { useDebouncedCallback, usePrevious } from '@deephaven/react-hooks'; @@ -15,6 +15,14 @@ function useDebouncedOnChange( const prevIsFocused = usePrevious(isFocused); const log = Log.module('@deephaven/js-plugin-ui/useDebouncedValue'); + // Keep a ref to the latest `propOnChange`. The server hands us a new callable + // reference on every re-render, so reading it through a ref lets the + // debounced callback below stay stable (see below). + const propOnChangeRef = useRef(propOnChange); + useEffect(() => { + propOnChangeRef.current = propOnChange; + }, [propOnChange]); + // Update local value to match a new propValue from the server when no user // changes are queued. Skip while the input is focused so we don't change the // value out from under the user while they are typing. When the input loses @@ -35,14 +43,17 @@ function useDebouncedOnChange( const propDebouncedOnChange = useCallback( async (newValue: T) => { try { - await propOnChange?.(newValue); + await propOnChangeRef.current?.(newValue); } catch (e) { log.warn('Error returned from onChange', e); } setPending(false); }, + // The callback must stay stable so the debounced callback isn't recreated + // (and any pending call cancelled) when the server hands us a new + // `propOnChange` on re-render. We read the latest `propOnChange` from a ref. // eslint-disable-next-line react-hooks/exhaustive-deps - [propOnChange] + [] ); const debouncedOnChange = useDebouncedCallback( From 897f32846e8c8f50cb321a96a2eb9eec45bfbf07 Mon Sep 17 00:00:00 2001 From: mikebender Date: Mon, 13 Jul 2026 13:16:25 -0400 Subject: [PATCH 5/7] fix(ui): return a stable set_state function from use_state (DH-19717) use_state created a new setter closure on every render. Callables are serialized to the client by object identity, so a fresh setter each render was assigned a new callable id - the client received a new onChange (etc.) prop on every render. This churned props and, for debounced inputs, caused a re-render triggered by one field to cancel another field's pending (debounced) change, losing it before it reached the server. Cache the setter per hook index in RenderContext so the same instance is returned across renders and serializes to a stable callable id. This supersedes the useDebouncedOnChange workaround, which is reverted. --- .../deephaven/ui/_internal/RenderContext.py | 39 +++++++++++++++++++ .../ui/src/deephaven/ui/hooks/use_state.py | 7 ++-- .../elements/hooks/useDebouncedOnChange.ts | 17 ++------ .../ui/test/deephaven/ui/hooks/test_state.py | 27 +++++++++++++ 4 files changed, 72 insertions(+), 18 deletions(-) diff --git a/plugins/ui/src/deephaven/ui/_internal/RenderContext.py b/plugins/ui/src/deephaven/ui/_internal/RenderContext.py index f9dd4952a..bb017297d 100644 --- a/plugins/ui/src/deephaven/ui/_internal/RenderContext.py +++ b/plugins/ui/src/deephaven/ui/_internal/RenderContext.py @@ -263,6 +263,14 @@ class RenderContext: A value that can be used to store arbitrary data for this context. """ + _hook_setters: Dict[StateKey, Callable[[Any], None]] + """ + Cache of stable state setter functions keyed by hook index. The same setter + instance is returned on every render so it serializes to a stable callable id + for the client (a fresh setter each render would be assigned a new callable + id, causing the client to receive a new prop - e.g. `onChange` - every render). + """ + def __init__(self, root: RootRenderContextProtocol): """ Create a new render context. @@ -285,6 +293,7 @@ def __init__(self, root: RootRenderContextProtocol): self._is_mounted = True self._is_dirty = True self._cache = None + self._hook_setters = {} def __del__(self): logger.debug("Deleting context") @@ -537,6 +546,35 @@ def update_state(): self._root.on_change(update_state) self.mark_dirty() + def make_state_setter(self, key: StateKey) -> Callable[[Any], None]: + """ + Get a stable setter function for the given state key. + + The same function instance is returned on every render for a given key. + This matters because callables are serialized to the client by object + identity - returning a fresh setter each render would be assigned a new + callable id and the client would see a new prop (e.g. `onChange`) on + every render, needlessly churning props and cancelling in-flight work + such as debounced input changes. + + Args: + key: The state key (hook index) to get the setter for. + + Returns: + A stable function that sets the state for the given key. + """ + setter = self._hook_setters.get(key) + if setter is None: + + def set_value(new_value: Any) -> None: + # Set the value in the context state and trigger a re-render + logger.debug("use_state set_value called with %s", new_value) + self.queue_render(lambda: self.set_state(key, new_value)) + + self._hook_setters[key] = set_value + setter = set_value + return setter + def get_child_context( self, key: ContextKey, fetch_only: bool = False ) -> "RenderContext": @@ -712,6 +750,7 @@ def unmount(self) -> None: self._collected_effects.clear() self._collected_unmount_listeners.clear() self._collected_contexts.clear() + self._hook_setters.clear() @property def cache(self) -> Any: diff --git a/plugins/ui/src/deephaven/ui/hooks/use_state.py b/plugins/ui/src/deephaven/ui/hooks/use_state.py index 821eb422b..7d0dfdaca 100644 --- a/plugins/ui/src/deephaven/ui/hooks/use_state.py +++ b/plugins/ui/src/deephaven/ui/hooks/use_state.py @@ -51,9 +51,8 @@ def use_state( value: T = context.get_state(hook_index) - def set_value(new_value: T | UpdaterFunction[T]): - # Set the value in the context state and trigger a re-render - logger.debug("use_state set_value called with %s", new_value) - context.queue_render(lambda: context.set_state(hook_index, new_value)) + # Use a stable setter instance across renders so it serializes to a stable + # callable id for the client (see RenderContext.make_state_setter). + set_value = context.make_state_setter(hook_index) return value, set_value diff --git a/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts b/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts index 44f902d8b..ce8bd5bc9 100644 --- a/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts +++ b/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts @@ -1,4 +1,4 @@ -import { useState, useCallback, useRef, useEffect } from 'react'; +import { useState, useCallback } from 'react'; import Log from '@deephaven/log'; import { useDebouncedCallback, usePrevious } from '@deephaven/react-hooks'; @@ -15,14 +15,6 @@ function useDebouncedOnChange( const prevIsFocused = usePrevious(isFocused); const log = Log.module('@deephaven/js-plugin-ui/useDebouncedValue'); - // Keep a ref to the latest `propOnChange`. The server hands us a new callable - // reference on every re-render, so reading it through a ref lets the - // debounced callback below stay stable (see below). - const propOnChangeRef = useRef(propOnChange); - useEffect(() => { - propOnChangeRef.current = propOnChange; - }, [propOnChange]); - // Update local value to match a new propValue from the server when no user // changes are queued. Skip while the input is focused so we don't change the // value out from under the user while they are typing. When the input loses @@ -43,17 +35,14 @@ function useDebouncedOnChange( const propDebouncedOnChange = useCallback( async (newValue: T) => { try { - await propOnChangeRef.current?.(newValue); + await propOnChange?.(newValue); } catch (e) { log.warn('Error returned from onChange', e); } setPending(false); }, - // The callback must stay stable so the debounced callback isn't recreated - // (and any pending call cancelled) when the server hands us a new - // `propOnChange` on re-render. We read the latest `propOnChange` from a ref. // eslint-disable-next-line react-hooks/exhaustive-deps - [] + [propOnChange] ); const debouncedOnChange = useDebouncedCallback( diff --git a/plugins/ui/test/deephaven/ui/hooks/test_state.py b/plugins/ui/test/deephaven/ui/hooks/test_state.py index c39121984..e2d76c69b 100644 --- a/plugins/ui/test/deephaven/ui/hooks/test_state.py +++ b/plugins/ui/test/deephaven/ui/hooks/test_state.py @@ -43,3 +43,30 @@ def _test_state(value1: int = 1, value2: int = 2): val1, set_val1, val2, set_val2 = result self.assertEqual(val1, 3) self.assertEqual(val2, 4) + + def test_state_setter_is_stable(self): + from deephaven.ui.hooks import use_state + + def _test_state(): + _, set_value1 = use_state(1) + _, set_value2 = use_state(2) + return set_value1, set_value2 + + render_result = render_hook(_test_state) + result, rerender = itemgetter("result", "rerender")(render_result) + first_set1, first_set2 = result + + # The setter instances must be stable across renders so that they + # serialize to a stable callable id for the client (otherwise the client + # would receive a new prop, e.g. onChange, on every render). + rerender() + second_set1, second_set2 = itemgetter("result")(render_result) + self.assertIs(first_set1, second_set1) + self.assertIs(first_set2, second_set2) + + # Setting state and re-rendering should still return the same setters + first_set1(3) + rerender() + third_set1, third_set2 = itemgetter("result")(render_result) + self.assertIs(first_set1, third_set1) + self.assertIs(first_set2, third_set2) From 1ebbd884543fbd0df034723907382eeb97f8e578 Mon Sep 17 00:00:00 2001 From: mikebender Date: Mon, 13 Jul 2026 14:54:39 -0400 Subject: [PATCH 6/7] fix(ui): persist state for all panels in a nested dashboard (DH-19717) usePanelManager.handleDataChange merged each panel's update into the initial widgetData, which is never updated. Sequential updates from different panels therefore each carried only their own state (plus the stale initial), so the last panel to update overwrote the others - only its state was persisted. Accumulate the latest panel states in a ref so every panel's state is retained. Also update the open/close path to emit the accumulated states. Extends the e2e custom-columns test to add and verify custom columns on both tables in the stack, and adds a usePanelManager unit test covering sequential multi-panel updates. --- .../src/js/src/layout/usePanelManager.test.ts | 26 ++++ .../ui/src/js/src/layout/usePanelManager.ts | 26 +++- tests/ui_dashboard_persistence.spec.ts | 135 ++++++++++++------ 3 files changed, 141 insertions(+), 46 deletions(-) diff --git a/plugins/ui/src/js/src/layout/usePanelManager.test.ts b/plugins/ui/src/js/src/layout/usePanelManager.test.ts index e0797a5a1..505729041 100644 --- a/plugins/ui/src/js/src/layout/usePanelManager.test.ts +++ b/plugins/ui/src/js/src/layout/usePanelManager.test.ts @@ -262,6 +262,32 @@ describe('usePanelManager', () => { }, }); }); + + it('accumulates state across sequential updates from different panels', () => { + const widget = makeWidget(); + const onDataChange = jest.fn(); + const { result } = renderHook(() => + usePanelManager({ widget, onDataChange }) + ); + + const panel1Data = [{ a: 1 }]; + const panel2Data = [{ b: 2 }]; + + act(() => { + result.current.onDataChange('panel-1', panel1Data); + }); + act(() => { + result.current.onDataChange('panel-2', panel2Data); + }); + + // The second panel's update must not drop the first panel's state + expect(onDataChange).toHaveBeenLastCalledWith({ + panelStates: { + 'panel-1': panel1Data, + 'panel-2': panel2Data, + }, + }); + }); }); describe('metadata', () => { diff --git a/plugins/ui/src/js/src/layout/usePanelManager.ts b/plugins/ui/src/js/src/layout/usePanelManager.ts index 0b547bac3..4914bdea2 100644 --- a/plugins/ui/src/js/src/layout/usePanelManager.ts +++ b/plugins/ui/src/js/src/layout/usePanelManager.ts @@ -53,6 +53,15 @@ export function usePanelManager({ // initialization function into `useRef` like you can with `useState` const [widgetData] = useState(() => structuredClone(initialData)); + // Accumulates the latest state for every panel. `widgetData` only holds the + // initial data (used for rehydration lookups) and is never updated, so we + // cannot merge into it - doing so would make each panel's update drop the + // other panels' updates, and only the most recently updated panel would be + // persisted. + const panelStatesRef = useRef>({ + ...widgetData.panelStates, + }); + // 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 = useRef([]); @@ -100,14 +109,15 @@ export function usePanelManager({ const handleDataChange = useCallback( (panelId: string, panelData: unknown[]) => { + panelStatesRef.current = { + ...panelStatesRef.current, + [panelId]: panelData, + }; onDataChange({ - panelStates: { - ...widgetData.panelStates, - [panelId]: panelData, - }, + panelStates: { ...panelStatesRef.current }, }); }, - [onDataChange, widgetData] + [onDataChange] ); /** @@ -129,7 +139,11 @@ export function usePanelManager({ log.debug('Widget', id, 'closed all panels, triggering onClose'); onClose?.(); } else { - onDataChange({ ...widgetData, panelIds: [...panelIds.current] }); + onDataChange({ + ...widgetData, + panelStates: { ...panelStatesRef.current }, + panelIds: [...panelIds.current], + }); } }, [isPanelsDirty, id, onClose, onDataChange, widgetData] diff --git a/tests/ui_dashboard_persistence.spec.ts b/tests/ui_dashboard_persistence.spec.ts index 630c6275b..e5d759c9a 100644 --- a/tests/ui_dashboard_persistence.spec.ts +++ b/tests/ui_dashboard_persistence.spec.ts @@ -78,6 +78,85 @@ async function selectPickerOption( await listbox.getByRole('option', { name: option, exact: true }).click(); } +/** + * Closes the currently open iris-grid Table Options sidebar. Any of the sidebar + * pages' close buttons dismiss the whole menu, and the sidebar is unmounted once + * closed. + * @param page The page + */ +async function closeTableSidebar(page: Page): Promise { + await page + .locator('.table-sidebar') + .getByRole('button', { name: 'Close', exact: true }) + .last() + .click(); + await expect(page.locator('.table-sidebar')).toHaveCount(0); +} + +/** + * Adds a custom column to the currently active ui.table via the Table Options + * sidebar, confirms it was applied, and leaves the sidebar closed. + * @param page The page + * @param formula The column formula to enter + * @param name The name to give the custom column + */ +async function addCustomColumnToActiveTable( + page: Page, + formula: string, + name: string +): Promise { + await page.getByRole('button', { name: 'Table Options' }).click(); + await page.getByTestId('menu-item-Custom Columns').click(); + + // Enter the formula first, then the name: the builder resets the name field + // shortly after it mounts, so setting the name last ensures it sticks + const formulaEditor = page + .locator('.custom-column-input-container .monaco-editor') + .first(); + await formulaEditor.click(); + await page.keyboard.type(formula); + await expect(formulaEditor.locator('textarea')).toHaveValue(formula); + + const columnNameInput = page.getByRole('textbox', { name: 'Column Name' }); + await columnNameInput.fill(name); + await expect(columnNameInput).toHaveValue(name); + + await page.getByRole('button', { name: 'Save Column', exact: true }).click(); + // Give the custom column time to be applied to the table + await page.waitForTimeout(1500); + + // Navigate back to the Table Options menu and confirm the column was applied + await page.getByRole('button', { name: 'Back', exact: true }).click(); + await page.getByTestId('menu-item-Organize Columns').click(); + await expect(page.locator('.visibility-ordering-builder')).toContainText( + name + ); + await closeTableSidebar(page); +} + +/** + * Opens Organize Columns for the currently active ui.table and asserts the + * `present` column is listed and (optionally) the `absent` column is not, then + * closes the sidebar. Assumes the sidebar starts closed. + * @param page The page + * @param present A column name expected to be present + * @param absent A column name expected to be absent + */ +async function expectActiveTableColumns( + page: Page, + present: string, + absent?: string +): Promise { + await page.getByRole('button', { name: 'Table Options' }).click(); + await page.getByTestId('menu-item-Organize Columns').click(); + const builder = page.locator('.visibility-ordering-builder'); + await expect(builder).toContainText(present); + if (absent != null) { + await expect(builder).not.toContainText(absent); + } + await closeTableSidebar(page); +} + test.describe('Dashboard persistence', () => { // Input persistence: text fields and pickers should keep their values after a // refresh when the layout is persisted. @@ -269,9 +348,9 @@ test.describe('Dashboard persistence', () => { ).toBeVisible(); }); - // ui.table persistence: custom columns added to a table through the UI should - // persist across a refresh. - test('custom columns on a ui.table persist across a refresh', async ({ + // ui.table persistence: custom columns added to both tables in the stack + // through the UI should each persist across a refresh. + test('custom columns on both ui.tables in a stack persist across a refresh', async ({ page, }) => { await gotoPage(page, ''); @@ -287,40 +366,15 @@ test.describe('Dashboard persistence', () => { .first(); await expect(outerPanel).toBeVisible(); - // Make sure "Table One" (columns a, b) is the active table in the stack + // Add a custom column to "Table One" (columns a, b) await outerPanel.locator('.lm_tab', { hasText: 'Table One' }).click(); await waitForLoad(page); + await addCustomColumnToActiveTable(page, 'a * 2', 'Doubled'); - // Open Table Options -> Custom Columns and add a custom column - await page.getByRole('button', { name: 'Table Options' }).click(); - await page.getByTestId('menu-item-Custom Columns').click(); - - // Enter the formula first, then the name: the builder resets the name field - // shortly after it mounts, so setting the name last ensures it sticks - const formulaEditor = page - .locator('.custom-column-input-container .monaco-editor') - .first(); - await formulaEditor.click(); - await page.keyboard.type('a * 2'); - await expect(formulaEditor.locator('textarea')).toHaveValue('a * 2'); - - const columnNameInput = page.getByRole('textbox', { name: 'Column Name' }); - await columnNameInput.fill('Doubled'); - await expect(columnNameInput).toHaveValue('Doubled'); - - await page - .getByRole('button', { name: 'Save Column', exact: true }) - .click(); - // Give the custom column time to be applied to the table - await page.waitForTimeout(1500); - - // Navigate back to the Table Options menu and confirm the custom column was - // applied by checking the Organize Columns list - await page.getByRole('button', { name: 'Back', exact: true }).click(); - await page.getByTestId('menu-item-Organize Columns').click(); - await expect(page.locator('.visibility-ordering-builder')).toContainText( - 'Doubled' - ); + // Add a different custom column to "Table Two" (columns c, d) + await outerPanel.locator('.lm_tab', { hasText: 'Table Two' }).click(); + await waitForLoad(page); + await addCustomColumnToActiveTable(page, 'c * 3', 'Tripled'); await persistLayoutAndReload(page); @@ -328,14 +382,15 @@ test.describe('Dashboard persistence', () => { .locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE) .first(); await expect(restoredPanel).toBeVisible(); + + // Each table should keep its own custom column after the refresh (and not + // pick up the other table's column) await restoredPanel.locator('.lm_tab', { hasText: 'Table One' }).click(); await waitForLoad(page); + await expectActiveTableColumns(page, 'Doubled', 'Tripled'); - // The custom column should still be present after the refresh - await page.getByRole('button', { name: 'Table Options' }).click(); - await page.getByTestId('menu-item-Organize Columns').click(); - await expect(page.locator('.visibility-ordering-builder')).toContainText( - 'Doubled' - ); + await restoredPanel.locator('.lm_tab', { hasText: 'Table Two' }).click(); + await waitForLoad(page); + await expectActiveTableColumns(page, 'Tripled', 'Doubled'); }); }); From c30696a2f47898df06a0a18c2e1535a2bce6d57c Mon Sep 17 00:00:00 2001 From: mikebender Date: Tue, 14 Jul 2026 09:21:25 -0400 Subject: [PATCH 7/7] fix(ui): use Spectrum key type for persisted tab selection usePersistentState was typed with React's Key (which includes bigint), but the Spectrum Tabs selectedKey/defaultSelectedKey only accept string | number, breaking the type check. Derive the key type from the component props. --- plugins/ui/src/js/src/elements/Tabs.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/plugins/ui/src/js/src/elements/Tabs.tsx b/plugins/ui/src/js/src/elements/Tabs.tsx index d4e9a739d..bc54c1091 100644 --- a/plugins/ui/src/js/src/elements/Tabs.tsx +++ b/plugins/ui/src/js/src/elements/Tabs.tsx @@ -38,6 +38,11 @@ type TabChild = | ReactElement, typeof TabList> | ReactElement, typeof TabPanels>; +// The selection key type accepted by the underlying Spectrum Tabs +// (`string | number`). Note this is narrower than React's `Key`, which also +// includes `bigint`. +type TabSelectionKey = NonNullable; + function containsDuplicateKeys(childrenArray: JSX.Element[]) { const keys = childrenArray.map(child => child.key); return new Set(keys).size !== keys.length; @@ -98,15 +103,14 @@ export function Tabs(props: TabComponentProps): JSX.Element { // Persist the active tab so it is restored when the widget is rehydrated // (e.g. after a page refresh). When the component is controlled by the server // (`selectedKey` provided) the server remains the source of truth. - const [persistedKey, setPersistedKey] = usePersistentState( - defaultSelectedKey, - { type: 'UITabs', version: 1 } - ); + const [persistedKey, setPersistedKey] = usePersistentState< + TabSelectionKey | undefined + >(defaultSelectedKey, { type: 'UITabs', version: 1 }); const userOnSelectionChange = onSelectionChangeProp ?? onChange; const onSelectionChange = useCallback( - (key: Key) => { + (key: TabSelectionKey) => { setPersistedKey(key); userOnSelectionChange?.(key); },