From 76228a9a89f4e351a91921dc90fd5a9793d4729e Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 20 Jun 2024 10:58:56 -0400 Subject: [PATCH 1/3] fix: DH-17292 Handle disconnect from GridWidgetPlugin - When the model is disconnected, we should just display an error. There's no option to reconnect, as the widget schema could change entirely - By unloading the IrisGrid component, it's no longer throwing an error by trying to access table methods after a table.close() - Fixes DH-17292 from Enterprise - Tested using the steps in the description --- .../src/GridWidgetPlugin.tsx | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx b/packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx index 6f24bf663e..4c2da9d222 100644 --- a/packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx +++ b/packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx @@ -4,11 +4,13 @@ import { type dh } from '@deephaven/jsapi-types'; import { useApi } from '@deephaven/jsapi-bootstrap'; import { IrisGrid, + IrisGridModel, IrisGridModelFactory, - type IrisGridModel, } from '@deephaven/iris-grid'; import { useSelector } from 'react-redux'; import { getSettings, RootState } from '@deephaven/redux'; +import { LoadingOverlay } from '@deephaven/components'; +import { getErrorMessage } from '@deephaven/utils'; export function GridWidgetPlugin( props: WidgetComponentProps @@ -16,12 +18,14 @@ export function GridWidgetPlugin( const dh = useApi(); const settings = useSelector(getSettings); const [model, setModel] = useState(); + const [error, setError] = useState(); const { fetch } = props; useEffect(() => { let cancelled = false; async function init() { + setError(undefined); const table = await fetch(); const newModel = await IrisGridModelFactory.makeModel(dh, table); if (!cancelled) { @@ -36,6 +40,36 @@ export function GridWidgetPlugin( }; }, [dh, fetch]); + useEffect( + function startListeningModel() { + if (!model) { + return; + } + + // If the table inside a widget is disconnected, then don't bother trying to listen to reconnect, just close it and show a message + // Widget closes the table already when it is disconnected, so no need to close it again + function handleDisconnect() { + setError(new Error('Model disconnected')); + setModel(undefined); + } + + model.addEventListener(IrisGridModel.EVENT.DISCONNECT, handleDisconnect); + + return () => { + model.removeEventListener( + IrisGridModel.EVENT.DISCONNECT, + handleDisconnect + ); + }; + }, + [model] + ); + + const errorMessage = getErrorMessage(error); + if (errorMessage != null) { + return ; + } + return model ? : null; } From 344a56e7a8f5eeeddc4ad38c86e2a188a5393bca Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 21 Jun 2024 10:14:40 -0400 Subject: [PATCH 2/3] Add a useIrisGridModel hook for GridWidgetPlugin and PandasWidgetPlugin - Minimize code duplication - Added unit tests --- .../src/GridWidgetPlugin.tsx | 78 +++--------- .../src/PandasWidgetPlugin.tsx | 82 ++++-------- .../src/useIrisGridModel.test.ts | 76 +++++++++++ .../src/useIrisGridModel.ts | 119 ++++++++++++++++++ 4 files changed, 239 insertions(+), 116 deletions(-) create mode 100644 packages/dashboard-core-plugins/src/useIrisGridModel.test.ts create mode 100644 packages/dashboard-core-plugins/src/useIrisGridModel.ts diff --git a/packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx b/packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx index 4c2da9d222..dd12853d94 100644 --- a/packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx +++ b/packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx @@ -1,76 +1,34 @@ -import { useEffect, useState } from 'react'; import { type WidgetComponentProps } from '@deephaven/plugin'; import { type dh } from '@deephaven/jsapi-types'; -import { useApi } from '@deephaven/jsapi-bootstrap'; -import { - IrisGrid, - IrisGridModel, - IrisGridModelFactory, -} from '@deephaven/iris-grid'; +import { IrisGrid } from '@deephaven/iris-grid'; import { useSelector } from 'react-redux'; import { getSettings, RootState } from '@deephaven/redux'; import { LoadingOverlay } from '@deephaven/components'; import { getErrorMessage } from '@deephaven/utils'; +import { useIrisGridModel } from './useIrisGridModel'; -export function GridWidgetPlugin( - props: WidgetComponentProps -): JSX.Element | null { - const dh = useApi(); +export function GridWidgetPlugin({ + fetch, +}: WidgetComponentProps): JSX.Element | null { const settings = useSelector(getSettings); - const [model, setModel] = useState(); - const [error, setError] = useState(); - const { fetch } = props; + const fetchResult = useIrisGridModel(fetch); - useEffect(() => { - let cancelled = false; - async function init() { - setError(undefined); - const table = await fetch(); - const newModel = await IrisGridModelFactory.makeModel(dh, table); - if (!cancelled) { - setModel(newModel); - } - } - - init(); - - return () => { - cancelled = true; - }; - }, [dh, fetch]); - - useEffect( - function startListeningModel() { - if (!model) { - return; - } - - // If the table inside a widget is disconnected, then don't bother trying to listen to reconnect, just close it and show a message - // Widget closes the table already when it is disconnected, so no need to close it again - function handleDisconnect() { - setError(new Error('Model disconnected')); - setModel(undefined); - } - - model.addEventListener(IrisGridModel.EVENT.DISCONNECT, handleDisconnect); - - return () => { - model.removeEventListener( - IrisGridModel.EVENT.DISCONNECT, - handleDisconnect - ); - }; - }, - [model] - ); + if (fetchResult.status === 'loading') { + return ; + } - const errorMessage = getErrorMessage(error); - if (errorMessage != null) { - return ; + if (fetchResult.status === 'error') { + return ( + + ); } - return model ? : null; + const { model } = fetchResult; + return ; } export default GridWidgetPlugin; diff --git a/packages/dashboard-core-plugins/src/PandasWidgetPlugin.tsx b/packages/dashboard-core-plugins/src/PandasWidgetPlugin.tsx index 69c228a5c0..8b43a0ec43 100644 --- a/packages/dashboard-core-plugins/src/PandasWidgetPlugin.tsx +++ b/packages/dashboard-core-plugins/src/PandasWidgetPlugin.tsx @@ -1,64 +1,34 @@ -import { useCallback, useEffect, useState } from 'react'; import { WidgetComponentProps } from '@deephaven/plugin'; import { type dh } from '@deephaven/jsapi-types'; -import IrisGrid, { - IrisGridModelFactory, - type IrisGridModel, -} from '@deephaven/iris-grid'; -import { useApi } from '@deephaven/jsapi-bootstrap'; +import IrisGrid from '@deephaven/iris-grid'; import { LoadingOverlay } from '@deephaven/components'; +import { getErrorMessage } from '@deephaven/utils'; import { PandasReloadButton } from './panels/PandasReloadButton'; - -export function PandasWidgetPlugin( - props: WidgetComponentProps -): JSX.Element | null { - const dh = useApi(); - const [model, setModel] = useState(); - const [isLoading, setIsLoading] = useState(true); - const [isLoaded, setIsLoaded] = useState(false); - - const { fetch } = props; - - const makeModel = useCallback(async () => { - const table = await fetch(); - return IrisGridModelFactory.makeModel(dh, table); - }, [dh, fetch]); - - const handleReload = useCallback(async () => { - setIsLoading(true); - const newModel = await makeModel(); - setModel(newModel); - setIsLoading(false); - }, [makeModel]); - - useEffect(() => { - let cancelled = false; - async function init() { - const newModel = await makeModel(); - if (!cancelled) { - setModel(newModel); - setIsLoaded(true); - setIsLoading(false); - } - } - - init(); - setIsLoading(true); - - return () => { - cancelled = true; - }; - }, [makeModel]); - +import { useIrisGridModel } from './useIrisGridModel'; + +export function PandasWidgetPlugin({ + fetch, +}: WidgetComponentProps): JSX.Element | null { + const fetchResult = useIrisGridModel(fetch); + + if (fetchResult.status === 'loading') { + return ; + } + + if (fetchResult.status === 'error') { + return ( + + ); + } + + const { model, reload } = fetchResult; return ( - <> - - {model && ( - - - - )} - + + + ); } diff --git a/packages/dashboard-core-plugins/src/useIrisGridModel.test.ts b/packages/dashboard-core-plugins/src/useIrisGridModel.test.ts new file mode 100644 index 0000000000..7ed20c136b --- /dev/null +++ b/packages/dashboard-core-plugins/src/useIrisGridModel.test.ts @@ -0,0 +1,76 @@ +import { IrisGridModel } from '@deephaven/iris-grid'; +import { type dh } from '@deephaven/jsapi-types'; +import { TestUtils } from '@deephaven/utils'; +import { renderHook } from '@testing-library/react-hooks'; +import { act } from 'react-test-renderer'; +import { + IrisGridModelFetchErrorResult, + IrisGridModelFetchSuccessResult, + useIrisGridModel, +} from './useIrisGridModel'; + +const mockApi = TestUtils.createMockProxy(); +// Mock out the useApi hook to just return the API +jest.mock('@deephaven/jsapi-bootstrap', () => ({ + useApi: () => mockApi, +})); + +const mockModel = TestUtils.createMockProxy(); +// Mock out the IrisGridModelFactory as well +jest.mock('@deephaven/iris-grid', () => ({ + ...jest.requireActual('@deephaven/iris-grid'), + IrisGridModelFactory: { + makeModel: jest.fn(() => mockModel), + }, +})); + +it('should return loading status while fetching', () => { + const fetch = jest.fn( + () => + new Promise(() => { + // Do nothing + }) + ); + const { result } = renderHook(() => useIrisGridModel(fetch)); + expect(result.current.status).toBe('loading'); +}); + +it('should return error status on fetch error', async () => { + const error = new Error('Test error'); + const fetch = jest.fn(() => Promise.reject(error)); + const { result, waitForNextUpdate } = renderHook(() => + useIrisGridModel(fetch) + ); + await waitForNextUpdate(); + const fetchResult = result.current; + expect(fetchResult.status).toBe('error'); + expect((fetchResult as IrisGridModelFetchErrorResult).error).toBe(error); +}); + +it('should return success status on fetch success', async () => { + const table = TestUtils.createMockProxy(); + const fetch = jest.fn(() => Promise.resolve(table)); + const { result, waitForNextUpdate } = renderHook(() => + useIrisGridModel(fetch) + ); + await waitForNextUpdate(); + const fetchResult = result.current; + expect(fetchResult.status).toBe('success'); + expect((fetchResult as IrisGridModelFetchSuccessResult).model).toBeDefined(); +}); + +it('should reload the model on reload', async () => { + const table = TestUtils.createMockProxy(); + const fetch = jest.fn(() => Promise.resolve(table)); + const { result, waitForNextUpdate } = renderHook(() => + useIrisGridModel(fetch) + ); + await waitForNextUpdate(); + const fetchResult = result.current; + expect(fetchResult.status).toBe('success'); + fetch.mockClear(); + await act(async () => { + fetchResult.reload(); + }); + expect(fetch).toHaveBeenCalledTimes(1); +}); diff --git a/packages/dashboard-core-plugins/src/useIrisGridModel.ts b/packages/dashboard-core-plugins/src/useIrisGridModel.ts new file mode 100644 index 0000000000..6193ddb768 --- /dev/null +++ b/packages/dashboard-core-plugins/src/useIrisGridModel.ts @@ -0,0 +1,119 @@ +import { type dh } from '@deephaven/jsapi-types'; +import { useApi } from '@deephaven/jsapi-bootstrap'; +import { IrisGridModel, IrisGridModelFactory } from '@deephaven/iris-grid'; +import { useCallback, useEffect, useState } from 'react'; + +export type IrisGridModelFetch = () => Promise; + +export type IrisGridModelFetchErrorResult = { + error: NonNullable; + status: 'error'; +}; + +export type IrisGridModelFetchLoadingResult = { + status: 'loading'; +}; + +export type IrisGridModelFetchSuccessResult = { + status: 'success'; + model: IrisGridModel; +}; + +export type IrisGridModelFetchResult = ( + | IrisGridModelFetchErrorResult + | IrisGridModelFetchLoadingResult + | IrisGridModelFetchSuccessResult +) & { + reload: () => void; +}; + +/** Pass in a table `fetch` function, will load the model and handle any errors */ +export function useIrisGridModel( + fetch: IrisGridModelFetch +): IrisGridModelFetchResult { + const dh = useApi(); + const [model, setModel] = useState(); + const [error, setError] = useState(); + const [isLoading, setIsLoading] = useState(true); + + const makeModel = useCallback(async () => { + const table = await fetch(); + return IrisGridModelFactory.makeModel(dh, table); + }, [dh, fetch]); + + const reload = useCallback(async () => { + setIsLoading(true); + setError(undefined); + try { + const newModel = await makeModel(); + setModel(newModel); + setIsLoading(false); + } catch (e) { + setError(e); + setIsLoading(false); + } + }, [makeModel]); + + useEffect(() => { + let cancelled = false; + async function init() { + setIsLoading(true); + setError(undefined); + try { + const newModel = await makeModel(); + if (!cancelled) { + setModel(newModel); + setIsLoading(false); + } + } catch (e) { + if (!cancelled) { + setError(e); + setIsLoading(false); + } + } + } + + init(); + + return () => { + cancelled = true; + }; + }, [makeModel]); + + useEffect( + function startListeningModel() { + if (!model) { + return; + } + + // If the table inside a widget is disconnected, then don't bother trying to listen to reconnect, just close it and show a message + // Widget closes the table already when it is disconnected, so no need to close it again + function handleDisconnect() { + setError(new Error('Table disconnected')); + setModel(undefined); + setIsLoading(false); + } + + model.addEventListener(IrisGridModel.EVENT.DISCONNECT, handleDisconnect); + + return () => { + model.removeEventListener( + IrisGridModel.EVENT.DISCONNECT, + handleDisconnect + ); + }; + }, + [model] + ); + + if (isLoading) { + return { reload, status: 'loading' }; + } + if (error != null) { + return { error, reload, status: 'error' }; + } + if (model != null) { + return { model, reload, status: 'success' }; + } + throw new Error('Invalid state'); +} From 818c4b37b17b66831fdc01d95d6f45c453920069 Mon Sep 17 00:00:00 2001 From: mikebender Date: Tue, 20 Aug 2024 11:43:01 -0400 Subject: [PATCH 3/3] Clean up based on review - Add some more unit tests for transitions of states --- .../src/useIrisGridModel.test.ts | 68 +++++++++++++++++-- 1 file changed, 64 insertions(+), 4 deletions(-) diff --git a/packages/dashboard-core-plugins/src/useIrisGridModel.test.ts b/packages/dashboard-core-plugins/src/useIrisGridModel.test.ts index 7ed20c136b..2dc2002642 100644 --- a/packages/dashboard-core-plugins/src/useIrisGridModel.test.ts +++ b/packages/dashboard-core-plugins/src/useIrisGridModel.test.ts @@ -61,16 +61,76 @@ it('should return success status on fetch success', async () => { it('should reload the model on reload', async () => { const table = TestUtils.createMockProxy(); - const fetch = jest.fn(() => Promise.resolve(table)); + let fetchResolve; + const fetch = jest.fn( + () => + new Promise(resolve => { + fetchResolve = resolve; + }) + ); const { result, waitForNextUpdate } = renderHook(() => useIrisGridModel(fetch) ); + expect(result.current.status).toBe('loading'); + fetchResolve(table); await waitForNextUpdate(); - const fetchResult = result.current; - expect(fetchResult.status).toBe('success'); + expect(result.current.status).toBe('success'); + // Check that it will reload, transitioning to loading then to success again + + fetch.mockClear(); + fetch.mockReturnValue( + new Promise(resolve => { + fetchResolve = resolve; + }) + ); + await act(async () => { + result.current.reload(); + }); + expect(fetch).toHaveBeenCalledTimes(1); + expect(result.current.status).toBe('loading'); + fetchResolve(table); + await waitForNextUpdate(); + expect(result.current.status).toBe('success'); + expect( + (result.current as IrisGridModelFetchSuccessResult).model + ).toBeDefined(); + + // Now check that it will handle a failure on reload, transitioning from loading to failure fetch.mockClear(); + + let fetchReject; + fetch.mockReturnValue( + new Promise((resolve, reject) => { + fetchReject = reject; + }) + ); await act(async () => { - fetchResult.reload(); + result.current.reload(); }); expect(fetch).toHaveBeenCalledTimes(1); + expect(result.current.status).toBe('loading'); + const error = new Error('Test error'); + fetchReject(error); + await waitForNextUpdate(); + expect(result.current.status).toBe('error'); + expect((result.current as IrisGridModelFetchErrorResult).error).toBe(error); + + // Check that it will reload again after an error + fetch.mockClear(); + fetch.mockReturnValue( + new Promise(resolve => { + fetchResolve = resolve; + }) + ); + await act(async () => { + result.current.reload(); + }); + expect(fetch).toHaveBeenCalledTimes(1); + expect(result.current.status).toBe('loading'); + fetchResolve(table); + await waitForNextUpdate(); + expect(result.current.status).toBe('success'); + expect( + (result.current as IrisGridModelFetchSuccessResult).model + ).toBeDefined(); });