fix: DH-14657 Better disconnect handling (#1261)

mofojed · mattrunyon · web-flow · commit 9358e41fd3d7 · 2023-05-03T19:28:41.000-04:00
- 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 <mattrunyonstuff@gmail.com>
diff --git 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}
         />
-        <InfoModal
+        <DebouncedModal
           isOpen={isDisconnected && !isAuthFailed}
-          icon={vsDebugDisconnect}
-          title={
-            <>
-              <LoadingSpinner /> Attempting to reconnect...
-            </>
-          }
-          subtitle="Please check your network connection."
-        />
+          debounceMs={250}
+        >
+          <InfoModal
+            icon={vsDebugDisconnect}
+            title={
+              <>
+                <LoadingSpinner /> Attempting to reconnect...
+              </>
+            }
+            subtitle="Please check your network connection."
+          />
+        </DebouncedModal>
         <BasicModal
           confirmButtonText="Refresh"
           onConfirm={AppMainContainer.handleRefresh}
diff --git a/packages/components/src/modal/DebouncedModal.test.tsx b/packages/components/src/modal/DebouncedModal.test.tsx
@@ -0,0 +1,94 @@
+import React from 'react';
+import { act, render, screen } from '@testing-library/react';
+import DebouncedModal from './DebouncedModal';
+import Modal from './Modal';
+
+const mockChildText = 'Mock Child';
+const children = (
+  <Modal>
+    <div>{mockChildText}</div>
+  </Modal>
+);
+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(
+      <DebouncedModal isOpen={false} debounceMs={DEFAULT_DEBOUNCE_MS}>
+        {children}
+      </DebouncedModal>
+    );
+    expect(
+      screen.queryByTestId('debounced-modal-backdrop')
+    ).not.toBeInTheDocument();
+    expect(screen.queryByText(mockChildText)).not.toBeInTheDocument();
+
+    act(() => {
+      rerender(
+        <DebouncedModal isOpen debounceMs={DEFAULT_DEBOUNCE_MS}>
+          {children}
+        </DebouncedModal>
+      );
+    });
+    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(
+      <DebouncedModal
+        isOpen={false}
+        blockInteraction={false}
+        debounceMs={DEFAULT_DEBOUNCE_MS}
+      >
+        {children}
+      </DebouncedModal>
+    );
+    expect(
+      screen.queryByTestId('debounced-modal-backdrop')
+    ).not.toBeInTheDocument();
+    expect(screen.queryByText(mockChildText)).not.toBeInTheDocument();
+
+    act(() => {
+      rerender(
+        <DebouncedModal
+          isOpen
+          blockInteraction={false}
+          debounceMs={DEFAULT_DEBOUNCE_MS}
+        >
+          {children}
+        </DebouncedModal>
+      );
+    });
+    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
@@ -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 && (
+        <div
+          className="modal-backdrop"
+          style={{ backgroundColor: 'transparent' }}
+          data-testid="debounced-modal-backdrop"
+        />
+      )}
+      {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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<T>(value: T, debounceMs: number) {
+  const [debouncedValue, setDebouncedValue] = useState<T>(value);
+  useEffect(() => {
+    const timeoutId = setTimeout(() => {
+      setDebouncedValue(value);
+    }, debounceMs);
+    return () => {
+      clearTimeout(timeoutId);
+    };
+  }, [value, debounceMs]);
+
+  return debouncedValue;
+}
+
+export default useDebouncedValue;

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+export { default as DebouncedModal } from './DebouncedModal';`
`1`	`2`	`export { default as InfoModal } from './InfoModal';`
`2`	`3`	`export { default as Modal } from './Modal';`
`3`	`4`	`export { default as ModalBody } from './ModalBody';`