code review: recoveryActionHandler should not be optional

Author: BillCarsonFr

src/livekit/useECConnectionState.test.tsx

@@ -6,7 +6,7 @@ Please see LICENSE in the repository root for full details.
 */
 
 import { type FC, useCallback, useState } from "react";
-import { test } from "vitest";
+import { test, vi } from "vitest";
 import {
   ConnectionError,
   ConnectionErrorReason,
@@ -60,7 +60,7 @@ test.each<[string, ConnectionError]>([
     const user = userEvent.setup();
     render(
       <MemoryRouter>
-        <GroupCallErrorBoundary>
+        <GroupCallErrorBoundary recoveryActionHandler={vi.fn()}>
           <TestComponent />
         </GroupCallErrorBoundary>
       </MemoryRouter>,

src/room/GroupCallErrorBoundary.test.tsx

@@ -17,7 +17,10 @@ import {
 import { BrowserRouter } from "react-router-dom";
 import userEvent from "@testing-library/user-event";
 
-import { GroupCallErrorBoundary } from "./GroupCallErrorBoundary.tsx";
+import {
+  type CallErrorRecoveryAction,
+  GroupCallErrorBoundary,
+} from "./GroupCallErrorBoundary.tsx";
 import {
   ConnectionLostError,
   E2EENotSupportedError,
@@ -60,7 +63,10 @@ test.each([
     const onErrorMock = vi.fn();
     const { asFragment } = render(
       <BrowserRouter>
-        <GroupCallErrorBoundary onError={onErrorMock}>
+        <GroupCallErrorBoundary
+          onError={onErrorMock}
+          recoveryActionHandler={vi.fn()}
+        >
           <TestComponent />
         </GroupCallErrorBoundary>
       </BrowserRouter>,
@@ -85,14 +91,17 @@ test("should render the error page with link back to home", async () => {
   const onErrorMock = vi.fn();
   const { asFragment } = render(
     <BrowserRouter>
-      <GroupCallErrorBoundary onError={onErrorMock}>
+      <GroupCallErrorBoundary
+        onError={onErrorMock}
+        recoveryActionHandler={vi.fn()}
+      >
         <TestComponent />
       </GroupCallErrorBoundary>
     </BrowserRouter>,
   );
 
   await screen.findByText("Call is not supported");
-  expect(screen.getByText(/Domain: example.com/i)).toBeInTheDocument();
+  expect(screen.getByText(/Domain: example\.com/i)).toBeInTheDocument();
   expect(
     screen.getByText(/Error Code: MISSING_MATRIX_RTC_FOCUS/i),
   ).toBeInTheDocument();
@@ -105,39 +114,7 @@ test("should render the error page with link back to home", async () => {
   expect(asFragment()).toMatchSnapshot();
 });
 
-test("should have a reconnect button for ConnectionLostError", async () => {
-  const user = userEvent.setup();
-
-  const reconnectCallback = vi.fn();
-
-  const TestComponent = (): ReactNode => {
-    throw new ConnectionLostError();
-  };
-
-  const { asFragment } = render(
-    <BrowserRouter>
-      <GroupCallErrorBoundary
-        onError={vi.fn()}
-        recoveryActionHandler={reconnectCallback}
-      >
-        <TestComponent />
-      </GroupCallErrorBoundary>
-    </BrowserRouter>,
-  );
-
-  await screen.findByText("Connection lost");
-  await screen.findByRole("button", { name: "Reconnect" });
-  await screen.findByRole("button", { name: "Return to home screen" });
-
-  expect(asFragment()).toMatchSnapshot();
-
-  await user.click(screen.getByRole("button", { name: "Reconnect" }));
-
-  expect(reconnectCallback).toHaveBeenCalledOnce();
-  expect(reconnectCallback).toHaveBeenCalledWith("reconnect");
-});
-
-test("Action handling should reset error state", async () => {
+test("ConnectionLostError: Action handling should reset error state", async () => {
   const user = userEvent.setup();
 
   const TestComponent: FC<{ fail: boolean }> = ({ fail }): ReactNode => {
@@ -147,11 +124,17 @@ test("Action handling should reset error state", async () => {
     return <div>HELLO</div>;
   };
 
+  const reconnectCallbackSpy = vi.fn();
+
   const WrapComponent = (): ReactNode => {
     const [failState, setFailState] = useState(true);
-    const reconnectCallback = useCallback(() => {
-      setFailState(false);
-    }, [setFailState]);
+    const reconnectCallback = useCallback(
+      (action: CallErrorRecoveryAction) => {
+        reconnectCallbackSpy(action);
+        setFailState(false);
+      },
+      [setFailState],
+    );
 
     return (
       <BrowserRouter>
@@ -162,15 +145,22 @@ test("Action handling should reset error state", async () => {
     );
   };
 
-  render(<WrapComponent />);
+  const { asFragment } = render(<WrapComponent />);
 
   // Should fail first
   await screen.findByText("Connection lost");
+  await screen.findByRole("button", { name: "Reconnect" });
+  await screen.findByRole("button", { name: "Return to home screen" });
+
+  expect(asFragment()).toMatchSnapshot();
 
   await user.click(screen.getByRole("button", { name: "Reconnect" }));
 
   // reconnect should have reset the error, thus rendering should be ok
   await screen.findByText("HELLO");
+
+  expect(reconnectCallbackSpy).toHaveBeenCalledOnce();
+  expect(reconnectCallbackSpy).toHaveBeenCalledWith("reconnect");
 });
 
 describe("Rageshake button", () => {
@@ -187,7 +177,10 @@ describe("Rageshake button", () => {
 
     render(
       <BrowserRouter>
-        <GroupCallErrorBoundary onError={vi.fn()}>
+        <GroupCallErrorBoundary
+          onError={vi.fn()}
+          recoveryActionHandler={vi.fn()}
+        >
           <TestComponent />
         </GroupCallErrorBoundary>
       </BrowserRouter>,

src/room/GroupCallErrorBoundary.tsx

@@ -38,7 +38,7 @@ export type RecoveryActionHandler = (action: CallErrorRecoveryAction) => void;
 
 interface ErrorPageProps {
   error: ElementCallError;
-  recoveryActionHandler?: RecoveryActionHandler;
+  recoveryActionHandler: RecoveryActionHandler;
   resetError: () => void;
 }
 
@@ -48,7 +48,6 @@ const ErrorPage: FC<ErrorPageProps> = ({
 }: ErrorPageProps): ReactElement => {
   const { t } = useTranslation();
 
-  // let title: string;
   let icon: ComponentType<SVGAttributes<SVGElement>>;
   switch (error.category) {
     case ErrorCategory.CONFIGURATION_ISSUE:
@@ -68,7 +67,7 @@ const ErrorPage: FC<ErrorPageProps> = ({
   if (error instanceof ConnectionLostError) {
     actions.push({
       label: t("call_ended_view.reconnect_button"),
-      onClick: () => recoveryActionHandler?.("reconnect"),
+      onClick: () => recoveryActionHandler("reconnect"),
     });
   }
 
@@ -101,7 +100,7 @@ const ErrorPage: FC<ErrorPageProps> = ({
 
 interface BoundaryProps {
   children: ReactNode | (() => ReactNode);
-  recoveryActionHandler?: RecoveryActionHandler;
+  recoveryActionHandler: RecoveryActionHandler;
   onError?: (error: unknown) => void;
 }
 
@@ -122,7 +121,7 @@ export const GroupCallErrorBoundary = ({
           resetError={resetError}
           recoveryActionHandler={(action: CallErrorRecoveryAction) => {
             resetError();
-            recoveryActionHandler?.(action);
+            recoveryActionHandler(action);
           }}
         />
       );

src/room/__snapshots__/GroupCallErrorBoundary.test.tsx.snap

@@ -1,6 +1,6 @@
 // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
 
-exports[`should have a reconnect button for ConnectionLostError 1`] = `
+exports[`ConnectionLostError: Action handling should reset error state 1`] = `
 <DocumentFragment>
   <div
     class="page"
@@ -150,158 +150,6 @@ exports[`should have a reconnect button for ConnectionLostError 1`] = `
 </DocumentFragment>
 `;
 
-exports[`should render the error page 1`] = `
-<DocumentFragment>
-  <div
-    class="page"
-  >
-    <header
-      class="header"
-    >
-      <div
-        class="nav leftNav"
-      >
-        <a
-          aria-label="Element Call Home"
-          class="headerLogo"
-          data-discover="true"
-          href="/"
-        >
-          <svg
-            fill="none"
-            height="30"
-            viewBox="0 0 260 30"
-            width="260"
-            xmlns="http://www.w3.org/2000/svg"
-          >
-            <desc>
-              Element Call (Beta)
-            </desc>
-            <circle
-              cx="15"
-              cy="15"
-              fill="white"
-              r="13"
-            />
-            <path
-              clip-rule="evenodd"
-              d="M15 30C23.2843 30 30 23.2843 30 15C30 6.71573 23.2843 0 15 0C6.71573 0 0 6.71573 0 15C0 23.2843 6.71573 30 15 30ZM12.2579 6.98923C12.2579 6.38376 12.7497 5.89292 13.3565 5.89292C17.4687 5.89292 20.8024 9.21967 20.8024 13.3234C20.8024 13.9289 20.3106 14.4197 19.7038 14.4197C19.0971 14.4197 18.6052 13.9289 18.6052 13.3234C18.6052 10.4306 16.2553 8.08554 13.3565 8.08554C12.7497 8.08554 12.2579 7.59471 12.2579 6.98923ZM24.1066 13.3235C24.1066 12.7181 23.6148 12.2272 23.008 12.2272C22.4013 12.2272 21.9094 12.7181 21.9094 13.3235C21.9094 16.2163 19.5595 18.5614 16.6607 18.5614C16.0539 18.5614 15.5621 19.0523 15.5621 19.6577C15.5621 20.2632 16.0539 20.754 16.6607 20.754C20.7729 20.754 24.1066 17.4273 24.1066 13.3235ZM17.7601 23.011C17.7601 23.6164 17.2682 24.1073 16.6615 24.1073C12.5492 24.1073 9.21553 20.7805 9.21553 16.6768C9.21553 16.0713 9.70739 15.5805 10.3141 15.5805C10.9209 15.5805 11.4127 16.0713 11.4127 16.6768C11.4127 19.5696 13.7627 21.9146 16.6615 21.9146C17.2682 21.9146 17.7601 22.4055 17.7601 23.011ZM5.89281 16.6769C5.89281 17.2824 6.38466 17.7732 6.9914 17.7732C7.59813 17.7732 8.08999 17.2824 8.08999 16.6769C8.08999 13.7841 10.4399 11.439 13.3388 11.439C13.9455 11.439 14.4373 10.9482 14.4373 10.3427C14.4373 9.73722 13.9455 9.24639 13.3388 9.24639C9.22647 9.24639 5.89281 12.5731 5.89281 16.6769Z"
-              fill="#0DBD8B"
-              fill-rule="evenodd"
-            />
-            <path
-              d="M53.5406 17.258H42.8052C42.932 18.3814 43.3397 19.2782 44.0282 19.9486C44.7167 20.6009 45.6227 20.927 46.746 20.927C47.4889 20.927 48.1593 20.7459 48.7572 20.3835C49.3551 20.0211 49.7809 19.5319 50.0346 18.9159H53.296C52.8611 20.3472 52.0458 21.5068 50.8499 22.3947C49.6722 23.2644 48.2771 23.6992 46.6645 23.6992C44.5627 23.6992 42.8596 23.0016 41.555 21.6065C40.2686 20.2114 39.6254 18.4448 39.6254 16.3068C39.6254 14.2231 40.2776 12.4747 41.5822 11.0614C42.8867 9.64814 44.5718 8.94151 46.6373 8.94151C48.7029 8.94151 50.3698 9.63908 51.6381 11.0342C52.9245 12.4112 53.5677 14.1506 53.5677 16.2524L53.5406 17.258ZM46.6373 11.5778C45.6227 11.5778 44.7801 11.8767 44.1098 12.4747C43.4394 13.0726 43.0226 13.8698 42.8596 14.8663H50.3607C50.2158 13.8698 49.8172 13.0726 49.1649 12.4747C48.5126 11.8767 47.6701 11.5778 46.6373 11.5778Z"
-              fill="currentColor"
-            />
-            <path
-              d="M55.7934 19.1605V2.9895H59.0276V19.2148C59.0276 19.9396 59.4262 20.302 60.2234 20.302L60.7941 20.2748V23.3459C60.4861 23.4003 60.16 23.4274 59.8157 23.4274C58.4206 23.4274 57.3969 23.0741 56.7446 22.3675C56.1104 21.6609 55.7934 20.5919 55.7934 19.1605Z"
-              fill="currentColor"
-            />
-            <path
-              d="M75.8563 17.258H65.121C65.2478 18.3814 65.6555 19.2782 66.344 19.9486C67.0325 20.6009 67.9384 20.927 69.0618 20.927C69.8047 20.927 70.4751 20.7459 71.073 20.3835C71.6709 20.0211 72.0967 19.5319 72.3503 18.9159H75.6117C75.1769 20.3472 74.3615 21.5068 73.1657 22.3947C71.988 23.2644 70.5928 23.6992 68.9803 23.6992C66.8785 23.6992 65.1753 23.0016 63.8708 21.6065C62.5843 20.2114 61.9411 18.4448 61.9411 16.3068C61.9411 14.2231 62.5934 12.4747 63.898 11.0614C65.2025 9.64814 66.8875 8.94151 68.9531 8.94151C71.0186 8.94151 72.6855 9.63908 73.9539 11.0342C75.2403 12.4112 75.8835 14.1506 75.8835 16.2524L75.8563 17.258ZM68.9531 11.5778C67.9384 11.5778 67.0959 11.8767 66.4255 12.4747C65.7551 13.0726 65.3384 13.8698 65.1753 14.8663H72.6765C72.5315 13.8698 72.1329 13.0726 71.4806 12.4747C70.8284 11.8767 69.9859 11.5778 68.9531 11.5778Z"
-              fill="currentColor"
-            />
-            <path
-              d="M90.448 15.274V23.3731H87.2138V14.9207C87.2138 12.7827 86.326 11.7137 84.5503 11.7137C83.59 11.7137 82.82 12.0217 82.2402 12.6377C81.6785 13.2538 81.3977 14.0963 81.3977 15.1653V23.3731H78.1635V9.26764H81.1531V11.1429C81.4973 10.5088 82.0228 9.98333 82.7294 9.5666C83.436 9.14987 84.3148 8.94151 85.3657 8.94151C87.3225 8.94151 88.7358 9.68438 89.6055 11.1701C90.8013 9.68438 92.3958 8.94151 94.3888 8.94151C96.0376 8.94151 97.3059 9.45789 98.1937 10.4907C99.0816 11.5053 99.5255 12.8461 99.5255 14.513V23.3731H96.2913V14.9207C96.2913 12.7827 95.4035 11.7137 93.6278 11.7137C92.6494 11.7137 91.8703 12.0308 91.2905 12.6649C90.7288 13.2809 90.448 14.1506 90.448 15.274Z"
-              fill="currentColor"
-            />
-            <path
-              d="M115.61 17.258H104.874C105.001 18.3814 105.409 19.2782 106.097 19.9486C106.786 20.6009 107.692 20.927 108.815 20.927C109.558 20.927 110.228 20.7459 110.826 20.3835C111.424 20.0211 111.85 19.5319 112.104 18.9159H115.365C114.93 20.3472 114.115 21.5068 112.919 22.3947C111.741 23.2644 110.346 23.6992 108.734 23.6992C106.632 23.6992 104.929 23.0016 103.624 21.6065C102.338 20.2114 101.694 18.4448 101.694 16.3068C101.694 14.2231 102.347 12.4747 103.651 11.0614C104.956 9.64814 106.641 8.94151 108.706 8.94151C110.772 8.94151 112.439 9.63908 113.707 11.0342C114.994 12.4112 115.637 14.1506 115.637 16.2524L115.61 17.258ZM108.706 11.5778C107.692 11.5778 106.849 11.8767 106.179 12.4747C105.508 13.0726 105.092 13.8698 104.929 14.8663H112.43C112.285 13.8698 111.886 13.0726 111.234 12.4747C110.582 11.8767 109.739 11.5778 108.706 11.5778Z"
-              fill="currentColor"
-            />
-            <path
-              d="M120.906 9.26764V11.1429C121.232 10.5269 121.767 10.0105 122.51 9.59378C123.271 9.15893 124.186 8.94151 125.255 8.94151C126.922 8.94151 128.208 9.44883 129.114 10.4635C130.038 11.4781 130.5 12.828 130.5 14.513V23.3731H127.266V14.9207C127.266 13.9242 127.03 13.1451 126.559 12.5834C126.106 12.0036 125.409 11.7137 124.467 11.7137C123.434 11.7137 122.619 12.0217 122.021 12.6377C121.441 13.2538 121.151 14.1053 121.151 15.1925V23.3731H117.917V9.26764H120.906Z"
-              fill="currentColor"
-            />
-            <path
-              d="M139.946 20.4922V23.2915C139.547 23.4003 138.985 23.4546 138.261 23.4546C135.507 23.4546 134.13 22.0685 134.13 19.2964V11.8496H131.982V9.26764H134.13V5.5986H137.364V9.26764H140V11.8496H137.364V18.9702C137.364 20.0755 137.889 20.6281 138.94 20.6281L139.946 20.4922Z"
-              fill="currentColor"
-            />
-            <path
-              d="M148.304 20.864C146.768 19.184 146 17.056 146 14.48C146 11.904 146.768 9.784 148.304 8.12C149.856 6.44 151.896 5.6 154.424 5.6C156.504 5.6 158.264 6.176 159.704 7.328C161.144 8.48 162.064 10.024 162.464 11.96H160.616C160.28 10.52 159.552 9.376 158.432 8.528C157.312 7.68 155.976 7.256 154.424 7.256C152.44 7.256 150.84 7.92 149.624 9.248C148.424 10.576 147.824 12.32 147.824 14.48C147.824 16.64 148.424 18.384 149.624 19.712C150.84 21.04 152.44 21.704 154.424 21.704C155.976 21.704 157.312 21.28 158.432 20.432C159.552 19.584 160.28 18.44 160.616 17H162.464C162.064 18.936 161.144 20.48 159.704 21.632C158.264 22.784 156.504 23.36 154.424 23.36C151.896 23.36 149.856 22.528 148.304 20.864Z"
-              fill="currentColor"
-            />
-            <path
-              d="M173.63 17.192C171.438 17.192 169.942 17.24 169.142 17.336C168.358 17.416 167.782 17.552 167.414 17.744C166.758 18.112 166.43 18.704 166.43 19.52C166.43 21.088 167.358 21.872 169.214 21.872C170.638 21.872 171.726 21.552 172.478 20.912C173.246 20.272 173.63 19.416 173.63 18.344V17.192ZM169.022 23.288C167.63 23.288 166.566 22.952 165.83 22.28C165.11 21.592 164.75 20.688 164.75 19.568C164.75 18.832 164.942 18.176 165.326 17.6C165.726 17.024 166.27 16.6 166.958 16.328C167.534 16.104 168.278 15.96 169.19 15.896C170.102 15.816 171.582 15.776 173.63 15.776V14.984C173.63 12.968 172.478 11.96 170.174 11.96C168.19 11.96 167.038 12.768 166.718 14.384H165.062C165.238 13.2 165.742 12.256 166.574 11.552C167.422 10.848 168.646 10.496 170.246 10.496C171.958 10.496 173.23 10.896 174.062 11.696C174.91 12.496 175.334 13.6 175.334 15.008V23H173.702V21.224C172.854 22.6 171.294 23.288 169.022 23.288Z"
-              fill="currentColor"
-            />
-            <path
-              d="M179.418 20.312V5H181.122V20.12C181.122 20.616 181.202 20.96 181.362 21.152C181.538 21.344 181.85 21.44 182.298 21.44L182.778 21.392V22.952C182.506 23 182.21 23.024 181.89 23.024C180.242 23.024 179.418 22.12 179.418 20.312Z"
-              fill="currentColor"
-            />
-            <path
-              d="M185.582 20.312V5H187.286V20.12C187.286 20.616 187.366 20.96 187.526 21.152C187.702 21.344 188.014 21.44 188.462 21.44L188.942 21.392V22.952C188.67 23 188.374 23.024 188.054 23.024C186.406 23.024 185.582 22.12 185.582 20.312Z"
-              fill="currentColor"
-            />
-            <path
-              d="M201 10C201 5.58172 204.582 2 209 2H252C256.418 2 260 5.58172 260 10V20C260 24.4183 256.418 28 252 28H209C204.582 28 201 24.4183 201 20V10Z"
-              fill="#368BD6"
-            />
-            <path
-              d="M212.076 20H216.492C218.99 20 220.215 18.7269 220.215 17.0277C220.215 15.3764 219.043 14.407 217.882 14.3484V14.2418C218.947 13.9915 219.789 13.2457 219.789 11.9194C219.789 10.2947 218.617 9.09091 216.252 9.09091H212.076V20ZM214.052 18.3487V15.1527H216.231C217.451 15.1527 218.207 15.8984 218.207 16.8732C218.207 17.7415 217.61 18.3487 216.178 18.3487H214.052ZM214.052 13.7305V10.7209H216.05C217.211 10.7209 217.813 11.3335 217.813 12.1751C217.813 13.1339 217.035 13.7305 216.007 13.7305H214.052ZM221.934 20H229.072V18.3434H223.911V15.3658H228.662V13.7092H223.911V10.7475H229.03V9.09091H221.934V20ZM230.566 10.7475H233.938V20H235.898V10.7475H239.27V9.09091H230.566V10.7475ZM241.031 20L241.931 17.31H246.032L246.938 20H249.047L245.201 9.09091H242.762L238.921 20H241.031ZM242.464 15.7227L243.939 11.3281H244.024L245.5 15.7227H242.464Z"
-              fill="white"
-            />
-          </svg>
-        </a>
-      </div>
-      <div
-        class="nav rightNav"
-      />
-    </header>
-    <div
-      class="container"
-    >
-      <div
-        class="content"
-      >
-        <div
-          class="error"
-        >
-          <div
-            class="_content_md016_17 icon"
-            data-size="large"
-          >
-            <svg
-              fill="currentColor"
-              height="1em"
-              viewBox="0 0 24 24"
-              width="1em"
-              xmlns="http://www.w3.org/2000/svg"
-            >
-              <path
-                d="M16.712 6.713A.968.968 0 0 1 16 7a.968.968 0 0 1-.713-.287A.967.967 0 0 1 15 6c0-.283.096-.52.287-.713A.968.968 0 0 1 16 5a.97.97 0 0 1 .712.287c.192.192.288.43.288.713s-.096.52-.288.713Z"
-              />
-              <path
-                clip-rule="evenodd"
-                d="M6 2a2 2 0 0 0-2 2v16a2 2 0 0 0 2 2h12a2 2 0 0 0 2-2V4a2 2 0 0 0-2-2H6Zm12 2H6v4h12V4ZM6 12v-2h12v2H6Zm0 2v2h12v-2H6Zm0 6v-2h12v2H6Z"
-                fill-rule="evenodd"
-              />
-            </svg>
-          </div>
-          <h1
-            class="_typography_yh5dq_162 _font-heading-md-semibold_yh5dq_121"
-          >
-            Call is not supported
-          </h1>
-          <p>
-            The server is not configured to work with Element Call. Please contact your server admin (Domain: example.com, Error Code: MISSING_MATRIX_RTC_FOCUS).
-          </p>
-          <button
-            class="_button_i91xf_17 homeLink"
-            data-kind="tertiary"
-            data-size="lg"
-            role="button"
-            tabindex="0"
-          >
-            Return to home screen
-          </button>
-        </div>
-      </div>
-    </div>
-  </div>
-</DocumentFragment>
-`;
-
 exports[`should render the error page with link back to home 1`] = `
 <DocumentFragment>
   <div