Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions plugins/ui/src/js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
"classnames": "^2.5.1",
"fast-json-patch": "^3.1.1",
"json-rpc-2.0": "^1.6.0",
"memoizee": "^0.4.17",
"nanoid": "^5.0.7",
"react-markdown": "^8.0.7",
"react-redux": "^7.x",
Expand Down
175 changes: 124 additions & 51 deletions plugins/ui/src/js/src/elements/UITable/UITable.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import React, {
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from 'react';
import { useSelector } from 'react-redux';
import classNames from 'classnames';
import {
Expand All @@ -8,6 +14,10 @@ import {
type IrisGridContextMenuData,
IrisGridProps,
IrisGridUtils,
IrisGridCacheUtils,
IrisGridState,
type DehydratedIrisGridState,
type DehydratedGridState,
} from '@deephaven/iris-grid';
import {
ColorValues,
Expand All @@ -18,12 +28,12 @@ import {
viewStyleProps,
} from '@deephaven/components';
import { useApi } from '@deephaven/jsapi-bootstrap';
import { TableUtils } from '@deephaven/jsapi-utils';
import type { dh as DhType } from '@deephaven/jsapi-types';
import Log from '@deephaven/log';
import { getSettings, RootState } from '@deephaven/redux';
import { GridMouseHandler } from '@deephaven/grid';
import { GridMouseHandler, GridState } from '@deephaven/grid';
import { EMPTY_ARRAY, ensureArray } from '@deephaven/utils';
import { usePersistentState } from '@deephaven/plugin';
import {
DatabarConfig,
FormattingRule,
Expand Down Expand Up @@ -275,6 +285,35 @@ export function UITable({
model.setColorMap(colorMap);
}

const [dehydratedState, setDehydratedState] = usePersistentState<
(DehydratedIrisGridState & DehydratedGridState) | undefined
>(undefined, { type: 'UITable', version: 1 });
const initialState = useRef(dehydratedState);

const memoizedStateFn = useMemo(
() => IrisGridCacheUtils.makeMemoizedCombinedGridStateDehydrator(),
[]
);

const onStateChange = useCallback(
(irisGridState: IrisGridState, gridState: GridState) => {
if (model == null) {
return;
}
setDehydratedState(memoizedStateFn(model, irisGridState, gridState));
},
[memoizedStateFn, model, setDehydratedState]
);

const initialHydratedState = useMemo(() => {
if (model && initialState.current != null) {
return {
...utils.hydrateIrisGridState(model, initialState.current),
...IrisGridUtils.hydrateGridState(model, initialState.current),
};
}
}, [model, utils]);

const hydratedSorts = useMemo(() => {
if (sorts !== undefined && columns !== undefined) {
log.debug('Hydrating sorts', sorts);
Expand Down Expand Up @@ -401,59 +440,92 @@ export function UITable({
[contextMenu, alwaysFetchColumns]
);

const irisGridProps = useMemo(
() =>
({
mouseHandlers,
alwaysFetchColumns,
showSearchBar,
sorts: hydratedSorts,
quickFilters: hydratedQuickFilters,
isFilterBarShown: showQuickFilters,
reverseType: reverse
? TableUtils.REVERSE_TYPE.POST_SORT
: TableUtils.REVERSE_TYPE.NONE,
density,
settings: { ...settings, showExtraGroupColumn: showGroupingColumn },
onContextMenu,
aggregationSettings: {
aggregations:
aggregations != null
? ensureArray(aggregations).map(agg => {
if (agg.cols != null && agg.ignore_cols != null) {
throw new Error(
'Cannot specify both cols and ignore_cols in a UI table aggregation'
);
}
return {
operation: getAggregationOperation(agg.agg),
selected: ensureArray(agg.cols ?? agg.ignore_cols ?? []),
// If agg.cols is set, we don't want to invert
// If it is not set, then the only other options are ignore_cols or neither
// In both cases, we want to invert since we are either ignoring, or selecting all as [] inverted
invert: agg.cols == null,
};
})
: [],
showOnTop: aggregationsPosition === 'top',
},
}) satisfies Partial<IrisGridProps>,
[
const irisGridServerProps = useMemo(() => {
const props = {
mouseHandlers,
alwaysFetchColumns,
showSearchBar,
showQuickFilters,
hydratedSorts,
hydratedQuickFilters,
sorts: hydratedSorts,
quickFilters: hydratedQuickFilters,
isFilterBarShown: showQuickFilters,
reverse,
density,
settings,
showGroupingColumn,
settings: { ...settings, showExtraGroupColumn: showGroupingColumn },
onContextMenu,
aggregations,
aggregationsPosition,
]
);
aggregationSettings: {
aggregations:
aggregations != null
? ensureArray(aggregations).map(agg => {
if (agg.cols != null && agg.ignore_cols != null) {
throw new Error(
'Cannot specify both cols and ignore_cols in a UI table aggregation'
);
}
return {
operation: getAggregationOperation(agg.agg),
selected: ensureArray(agg.cols ?? agg.ignore_cols ?? []),
// If agg.cols is set, we don't want to invert
// If it is not set, then the only other options are ignore_cols or neither
// In both cases, we want to invert since we are either ignoring, or selecting all as [] inverted
invert: agg.cols == null,
};
})
: [],
showOnTop: aggregationsPosition === 'top',
},
} satisfies Partial<IrisGridProps>;

// Remove any explicit undefined values so we can use client state if available
(
Object.entries(props) as [
keyof typeof props,
(typeof props)[keyof typeof props],
][]
).forEach(([key, value]) => {
if (value === undefined) {
delete props[key];
}
});

return props;
}, [
mouseHandlers,
alwaysFetchColumns,
showSearchBar,
showQuickFilters,
hydratedSorts,
hydratedQuickFilters,
reverse,
density,
settings,
showGroupingColumn,
onContextMenu,
aggregations,
aggregationsPosition,
]);

const initialIrisGridServerProps = useRef(irisGridServerProps);

/**
* We want to set the props based on a combination of server state and client state.
* If the server state is the same as its initial state, then we are rehydrating and
* the client state should take precedence.
* Otherwise, we have received changes from the server and we should use those over client state.
* In the future we may want to do a smarter merge of these.
*/
const mergedIrisGridProps = useMemo(() => {

Copilot AI May 14, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The conditional merge using reference equality to compare irisGridServerProps may be fragile. Consider using a more robust state comparison (or an explicit rehydration flag) to ensure the correct precedence of client vs server state.

Copilot uses AI. Check for mistakes.
if (initialIrisGridServerProps.current === irisGridServerProps) {

Copilot AI May 1, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing initialIrisGridServerProps.current with irisGridServerProps using reference equality may be unreliable if the server state object is recreated on each render. Consider using a deep comparison utility or ensuring that the server props maintain stable references so that rehydration logic behaves as intended.

Copilot uses AI. Check for mistakes.
return {
...irisGridServerProps,
...initialHydratedState,
};
}

return {
...initialHydratedState,
...irisGridServerProps,
};
}, [irisGridServerProps, initialHydratedState]);

return model ? (
<div
Expand All @@ -464,8 +536,9 @@ export function UITable({
<IrisGrid
ref={ref => setIrisGrid(ref)}
model={model}
onStateChange={onStateChange}
// eslint-disable-next-line react/jsx-props-no-spreading
{...irisGridProps}
{...mergedIrisGridProps}
/>
</div>
) : null;
Expand Down
4 changes: 4 additions & 0 deletions plugins/ui/src/js/src/layout/ReactPanel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ function makeReactPanelManager({
onClose = jest.fn(),
onOpen = jest.fn(),
getPanelId = jest.fn(() => mockPanelId),
onDataChange = jest.fn(),
getInitialData = jest.fn(() => []),
title = 'test title',
}: Partial<ReactPanelProps> & Partial<ReactPanelManager> = {}) {
return (
Expand All @@ -43,6 +45,8 @@ function makeReactPanelManager({
metadata,
onClose,
onOpen,
onDataChange,
getInitialData,
}}
>
<ReactPanel title={title}>{children}</ReactPanel>
Expand Down
29 changes: 25 additions & 4 deletions plugins/ui/src/js/src/layout/ReactPanel.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import React, { useCallback, useEffect, useMemo, useRef } from 'react';
import React, {
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from 'react';
import ReactDOM from 'react-dom';
import { nanoid } from 'nanoid';
import {
Expand All @@ -15,6 +21,7 @@ import {
LoadingOverlay,
} from '@deephaven/components';
import Log from '@deephaven/log';
import { PersistentStateProvider } from '@deephaven/plugin';
import PortalPanel from './PortalPanel';
import { ReactPanelControl, useReactPanel } from './ReactPanelManager';
import { ReactPanelProps } from './LayoutUtils';
Expand Down Expand Up @@ -89,10 +96,17 @@ function ReactPanel({
UNSAFE_className,
}: Props): JSX.Element | null {
const layoutManager = useLayoutManager();
const { metadata, onClose, onOpen, panelId } = useReactPanel();
const { metadata, onClose, onOpen, panelId, onDataChange, getInitialData } =
useReactPanel();
const portalManager = usePortalPanelManager();
const portal = portalManager.get(panelId);
const panelTitle = title ?? metadata?.name ?? '';
const [initialData, setInitialData] = useState(getInitialData());
const onErrorReset = useCallback(() => {
// Not EMPTY_ARRAY, because we always want to trigger a re-render
// in case a panel is reloaded and errors again
setInitialData([]);
Comment on lines +105 to +108

Copilot AI May 14, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Using a literal empty array to reset state may be less clear in intent. Consider extracting this value to a named constant or adding an inline comment to clarify that an empty array forces a re-render for error recovery.

Suggested change
const onErrorReset = useCallback(() => {
// Not EMPTY_ARRAY, because we always want to trigger a re-render
// in case a panel is reloaded and errors again
setInitialData([]);
const EMPTY_ARRAY: never[] = []; // Used to reset state and trigger a re-render
const onErrorReset = useCallback(() => {
// Always use EMPTY_ARRAY to ensure a re-render for error recovery
setInitialData(EMPTY_ARRAY);

Copilot uses AI. Check for mistakes.
}, []);

// Tracks whether the panel is open and that we have emitted the onOpen event
const isPanelOpenRef = useRef(false);
Expand Down Expand Up @@ -234,12 +248,19 @@ function ReactPanel({
rowGap={rowGap}
columnGap={columnGap}
>
<ReactPanelErrorBoundary>
<ReactPanelErrorBoundary onReset={onErrorReset}>
{/**
* Don't render the children if there's an error with the widget. If there's an error with the widget, we can assume the children won't render properly,
* but we still want the panels to appear so things don't disappear/jump around.
*/}
{renderedChildren ?? null}
<PersistentStateProvider
initialState={initialData}
onChange={onDataChange}
>
{React.Children.map(renderedChildren, child =>
React.cloneElement(child as React.ReactElement)
)}
</PersistentStateProvider>
</ReactPanelErrorBoundary>
</Flex>
</View>
Expand Down
Loading
Loading