From 9358e41fd3d7c587a45788819eec0962a8361202 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Wed, 3 May 2023 19:28:41 -0400 Subject: [PATCH] fix: DH-14657 Better disconnect handling (#1261) - In some cases, the API disconnects and the reconnects almost immediately afterwards, causing the "Reconnecting..." modal to appear and be distracting - Rather than have the API lie about whether it's connected or not, UI debounces when it displays the modal, just blocking interaction with a transparent modal for the duration of the disconnection if it reconnects immediately - Also added logging so we can see when connected/disconnected, in case this is further observed - Tested using the steps in #1149 , and also manually setting the `debounceMs` to `5000` to be able to react to it --------- Co-authored-by: Matthew Runyon --- .../code-studio/src/main/AppMainContainer.tsx | 26 +++-- .../src/modal/DebouncedModal.test.tsx | 94 +++++++++++++++++++ .../components/src/modal/DebouncedModal.tsx | 47 ++++++++++ packages/components/src/modal/InfoModal.tsx | 17 +++- packages/components/src/modal/index.ts | 1 + packages/react-hooks/src/index.ts | 1 + .../react-hooks/src/useDebouncedValue.test.ts | 72 ++++++++++++++ packages/react-hooks/src/useDebouncedValue.ts | 25 +++++ 8 files changed, 272 insertions(+), 11 deletions(-) create mode 100644 packages/components/src/modal/DebouncedModal.test.tsx create mode 100644 packages/components/src/modal/DebouncedModal.tsx create mode 100644 packages/react-hooks/src/useDebouncedValue.test.ts create mode 100644 packages/react-hooks/src/useDebouncedValue.ts diff --git a/packages/code-studio/src/main/AppMainContainer.tsx b/packages/code-studio/src/main/AppMainContainer.tsx index 3489173c11..e1578915b3 100644 --- a/packages/code-studio/src/main/AppMainContainer.tsx +++ b/packages/code-studio/src/main/AppMainContainer.tsx @@ -22,6 +22,7 @@ import { InfoModal, LoadingSpinner, BasicModal, + DebouncedModal, } from '@deephaven/components'; import { IrisGridModel, @@ -549,14 +550,17 @@ export class AppMainContainer extends Component< } handleDisconnect() { + log.info('Disconnected from server'); this.setState({ isDisconnected: true }); } handleReconnect() { + log.info('Reconnected to server'); this.setState({ isDisconnected: false }); } handleReconnectAuthFailed() { + log.warn('Reconnect authentication failed'); this.setState({ isAuthFailed: true }); } @@ -948,16 +952,20 @@ export class AppMainContainer extends Component< style={{ display: 'none' }} onChange={this.handleImportLayoutFiles} /> - - Attempting to reconnect... - - } - subtitle="Please check your network connection." - /> + debounceMs={250} + > + + Attempting to reconnect... + + } + subtitle="Please check your network connection." + /> + +
{mockChildText}
+ +); +const DEFAULT_DEBOUNCE_MS = 250; + +beforeAll(() => { + jest.useFakeTimers(); +}); + +afterAll(() => { + jest.useRealTimers(); +}); + +describe('display modal after debounce', () => { + it('should render the modal after the debounce time has passed', () => { + const { rerender } = render( + + {children} + + ); + expect( + screen.queryByTestId('debounced-modal-backdrop') + ).not.toBeInTheDocument(); + expect(screen.queryByText(mockChildText)).not.toBeInTheDocument(); + + act(() => { + rerender( + + {children} + + ); + }); + expect( + screen.queryByTestId('debounced-modal-backdrop') + ).toBeInTheDocument(); + expect(screen.queryByText(mockChildText)).not.toBeInTheDocument(); + + act(() => { + jest.advanceTimersByTime(DEFAULT_DEBOUNCE_MS); + }); + expect( + screen.queryByTestId('debounced-modal-backdrop') + ).toBeInTheDocument(); + expect(screen.queryByText(mockChildText)).toBeInTheDocument(); + }); + + it('should not block interaction if set to false', () => { + const { rerender } = render( + + {children} + + ); + expect( + screen.queryByTestId('debounced-modal-backdrop') + ).not.toBeInTheDocument(); + expect(screen.queryByText(mockChildText)).not.toBeInTheDocument(); + + act(() => { + rerender( + + {children} + + ); + }); + expect( + screen.queryByTestId('debounced-modal-backdrop') + ).not.toBeInTheDocument(); + expect(screen.queryByText(mockChildText)).not.toBeInTheDocument(); + + act(() => { + jest.advanceTimersByTime(DEFAULT_DEBOUNCE_MS + 5); + }); + expect( + screen.queryByTestId('debounced-modal-backdrop') + ).not.toBeInTheDocument(); + expect(screen.queryByText(mockChildText)).toBeInTheDocument(); + }); +}); diff --git a/packages/components/src/modal/DebouncedModal.tsx b/packages/components/src/modal/DebouncedModal.tsx new file mode 100644 index 0000000000..22a1600657 --- /dev/null +++ b/packages/components/src/modal/DebouncedModal.tsx @@ -0,0 +1,47 @@ +import React from 'react'; +import { useDebouncedValue } from '@deephaven/react-hooks'; + +export type DebouncedModalProps = { + /** Whether to block interaction immediately */ + blockInteraction?: boolean; + + /** Children to render after the alloted debounce time */ + children: React.ReactElement; + + /** Time to debounce */ + debounceMs: number; + + /** + * Will render the `children` `debounceMs` after `isOpen` is set to `true. + * Will stop rendering immediately after `isOpen` is set to `false`. + */ + isOpen?: boolean; +}; + +/** + * Display a modal after a debounce time. Blocks the screen from interaction immediately, + * but then waits the set debounce time before rendering the modal. + */ +function DebouncedModal({ + blockInteraction = true, + children, + debounceMs, + isOpen = false, +}: DebouncedModalProps) { + const debouncedIsOpen = useDebouncedValue(isOpen, debounceMs); + + return ( + <> + {blockInteraction && isOpen && ( +
+ )} + {React.cloneElement(children, { isOpen: isOpen && debouncedIsOpen })} + + ); +} + +export default DebouncedModal; diff --git a/packages/components/src/modal/InfoModal.tsx b/packages/components/src/modal/InfoModal.tsx index ccdefcc4b2..9259fae40a 100644 --- a/packages/components/src/modal/InfoModal.tsx +++ b/packages/components/src/modal/InfoModal.tsx @@ -6,17 +6,30 @@ import ModalBody from './ModalBody'; import './InfoModal.scss'; type InfoModalProps = { + /** Class name to give the info modal */ className?: string; + + /** Icon to display in the modal */ icon?: IconProp; + + /** Title to display in the modal */ title: React.ReactNode; + + /** Subtitle/detail to display in the modal */ subtitle?: React.ReactNode; - isOpen: boolean; + + /** Whether the modal is open/visible or not. */ + isOpen?: boolean; }; +/** + * A modal that displays a message with an icon. Can be used for informational messages, warnings, or errors. + * Does not have any buttons and cannot be dismissed. + */ function InfoModal({ className, icon, - isOpen, + isOpen = false, subtitle, title, }: InfoModalProps): JSX.Element { diff --git a/packages/components/src/modal/index.ts b/packages/components/src/modal/index.ts index 1cab923586..2946705cfb 100644 --- a/packages/components/src/modal/index.ts +++ b/packages/components/src/modal/index.ts @@ -1,3 +1,4 @@ +export { default as DebouncedModal } from './DebouncedModal'; export { default as InfoModal } from './InfoModal'; export { default as Modal } from './Modal'; export { default as ModalBody } from './ModalBody'; diff --git a/packages/react-hooks/src/index.ts b/packages/react-hooks/src/index.ts index 9b0397044a..8308ba4d44 100644 --- a/packages/react-hooks/src/index.ts +++ b/packages/react-hooks/src/index.ts @@ -7,3 +7,4 @@ export type { UsePromiseFactoryOptions, UsePromiseFactoryResult, } from './usePromiseFactory'; +export * from './useDebouncedValue'; diff --git a/packages/react-hooks/src/useDebouncedValue.test.ts b/packages/react-hooks/src/useDebouncedValue.test.ts new file mode 100644 index 0000000000..502f19bc35 --- /dev/null +++ b/packages/react-hooks/src/useDebouncedValue.test.ts @@ -0,0 +1,72 @@ +import { act, renderHook } from '@testing-library/react-hooks'; +import useDebouncedValue from './useDebouncedValue'; + +const DEFAULT_DEBOUNCE_MS = 100; +beforeEach(() => { + jest.useFakeTimers(); +}); + +afterAll(() => { + jest.useRealTimers(); +}); + +it('should return the initial value', () => { + const value = 'mock value'; + const { result } = renderHook(() => + useDebouncedValue(value, DEFAULT_DEBOUNCE_MS) + ); + expect(result.current).toBe(value); +}); + +it('should return the initial value after the debounce time has elapsed', () => { + const value = 'mock value'; + const { result, rerender } = renderHook(() => + useDebouncedValue(value, DEFAULT_DEBOUNCE_MS) + ); + expect(result.current).toBe(value); + expect(result.all.length).toBe(1); + rerender(); + act(() => { + jest.advanceTimersByTime(DEFAULT_DEBOUNCE_MS); + }); + expect(result.current).toBe(value); + expect(result.all.length).toBe(2); +}); + +it('should return the updated value after the debounce time has elapsed', () => { + const value = 'mock value'; + const newValue = 'mock new value'; + const { result, rerender } = renderHook((val = value) => + useDebouncedValue(val, DEFAULT_DEBOUNCE_MS) + ); + expect(result.current).toBe(value); + rerender(newValue); + act(() => { + jest.advanceTimersByTime(DEFAULT_DEBOUNCE_MS); + }); + expect(result.current).toBe(newValue); +}); + +it('should not return an intermediate value if the debounce time has not elapsed', () => { + const value = 'mock value'; + const intermediateValue = 'mock intermediate value'; + const newValue = 'mock new value'; + const { result, rerender } = renderHook((val = value) => + useDebouncedValue(val, DEFAULT_DEBOUNCE_MS) + ); + expect(result.current).toBe(value); + rerender(intermediateValue); + act(() => { + jest.advanceTimersByTime(DEFAULT_DEBOUNCE_MS - 5); + }); + expect(result.current).toBe(value); + rerender(newValue); + act(() => { + jest.advanceTimersByTime(DEFAULT_DEBOUNCE_MS - 5); + }); + expect(result.current).toBe(value); + act(() => { + jest.advanceTimersByTime(DEFAULT_DEBOUNCE_MS); + }); + expect(result.current).toBe(newValue); +}); diff --git a/packages/react-hooks/src/useDebouncedValue.ts b/packages/react-hooks/src/useDebouncedValue.ts new file mode 100644 index 0000000000..fecf4323e1 --- /dev/null +++ b/packages/react-hooks/src/useDebouncedValue.ts @@ -0,0 +1,25 @@ +import { useEffect, useState } from 'react'; + +/** + * Debounces a value. + * Returns the initial value immediately. + * Returns the latest value after no changes have occurred for the debounce duration. + * @param value Value to debounce + * @param debounceMs Amount of time to debounce + * @returns The debounced value + */ +export function useDebouncedValue(value: T, debounceMs: number) { + const [debouncedValue, setDebouncedValue] = useState(value); + useEffect(() => { + const timeoutId = setTimeout(() => { + setDebouncedValue(value); + }, debounceMs); + return () => { + clearTimeout(timeoutId); + }; + }, [value, debounceMs]); + + return debouncedValue; +} + +export default useDebouncedValue;