coder · Parkreiner · Nov 7, 2023 · Nov 6, 2023 · Nov 6, 2023 · Nov 6, 2023
diff --git a/site/src/components/GlobalSnackbar/GlobalSnackbar.tsx b/site/src/components/GlobalSnackbar/GlobalSnackbar.tsx
@@ -1,6 +1,5 @@
-import { type FC, useCallback, useState } from "react";
+import { type FC, useState } from "react";
 import { useCustomEvent } from "hooks/events";
-import type { CustomEventListener } from "utils/events";
 import { EnterpriseSnackbar } from "./EnterpriseSnackbar";
 import { ErrorIcon } from "../Icons/ErrorIcon";
 import { Typography } from "../Typography/Typography";
@@ -29,54 +28,10 @@ export const GlobalSnackbar: FC = () => {
   const [open, setOpen] = useState<boolean>(false);
   const [notification, setNotification] = useState<NotificationMsg>();
 
-  const handleNotification = useCallback<CustomEventListener<NotificationMsg>>(
-    (event) => {
-      setNotification(event.detail);
-      setOpen(true);
-    },
-    [],
-  );
-
-  useCustomEvent(SnackbarEventType, handleNotification);
-
-  const renderAdditionalMessage = (msg: AdditionalMessage, idx: number) => {
-    if (isNotificationText(msg)) {
-      return (
-        <Typography
-          key={idx}
-          gutterBottom
-          variant="body2"
-          css={styles.messageSubtitle}
-        >
-          {msg}
-        </Typography>
-      );
-    } else if (isNotificationTextPrefixed(msg)) {
-      return (
-        <Typography
-          key={idx}
-          gutterBottom
-          variant="body2"
-          css={styles.messageSubtitle}
-        >
-          <strong>{msg.prefix}:</strong> {msg.text}
-        </Typography>
-      );
-    } else if (isNotificationList(msg)) {
-      return (
-        <ul css={styles.list} key={idx}>
-          {msg.map((item, idx) => (
-            <li key={idx}>
-              <Typography variant="body2" css={styles.messageSubtitle}>
-                {item}
-              </Typography>
-            </li>
-          ))}
-        </ul>
-      );
-    }
-    return null;
-  };
+  useCustomEvent<NotificationMsg>(SnackbarEventType, (event) => {
+    setNotification(event.detail);
+    setOpen(true);
+  });
 
   if (!notification) {
     return null;
@@ -87,26 +42,27 @@ export const GlobalSnackbar: FC = () => {
       key={notification.msg}
       open={open}
       variant={variantFromMsgType(notification.msgType)}
+      onClose={() => setOpen(false)}
+      autoHideDuration={notification.msgType === MsgType.Error ? 22000 : 6000}
+      anchorOrigin={{ vertical: "bottom", horizontal: "right" }}
       message={
         <div css={styles.messageWrapper}>
           {notification.msgType === MsgType.Error && (
             <ErrorIcon css={styles.errorIcon} />
           )}
+
           <div css={styles.message}>
             <Typography variant="body1" css={styles.messageTitle}>
               {notification.msg}
             </Typography>
+
             {notification.additionalMsgs &&
-              notification.additionalMsgs.map(renderAdditionalMessage)}
+              notification.additionalMsgs.map((msg, index) => (
+                <AdditionalMessageDisplay key={index} message={msg} />
+              ))}
           </div>
         </div>
       }
-      onClose={() => setOpen(false)}
-      autoHideDuration={notification.msgType === MsgType.Error ? 22000 : 6000}
-      anchorOrigin={{
-        vertical: "bottom",
-        horizontal: "right",
-      }}
     />
   );
 };
@@ -133,3 +89,37 @@ const styles = {
     marginRight: theme.spacing(2),
   }),
 } satisfies Record<string, Interpolation<Theme>>;
+
+function AdditionalMessageDisplay({ message }: { message: AdditionalMessage }) {
+  if (isNotificationText(message)) {
+    return (
+      <Typography gutterBottom variant="body2" css={styles.messageSubtitle}>
+        {message}
+      </Typography>
+    );
+  }
+
+  if (isNotificationTextPrefixed(message)) {
+    return (
+      <Typography gutterBottom variant="body2" css={styles.messageSubtitle}>
+        <strong>{message.prefix}:</strong> {message.text}
+      </Typography>
+    );
+  }
+
+  if (isNotificationList(message)) {
+    return (
+      <ul css={styles.list}>
+        {message.map((item, idx) => (
+          <li key={idx}>
+            <Typography variant="body2" css={styles.messageSubtitle}>
+              {item}
+            </Typography>
+          </li>
+        ))}
+      </ul>
+    );
+  }
+
+  return null;
+}
diff --git a/site/src/hooks/events.ts b/site/src/hooks/events.ts
@@ -1,5 +1,6 @@
 import { useEffect } from "react";
 import { CustomEventListener } from "utils/events";
+import { useEffectEvent } from "./hookPolyfills";
 
 /**
  * Handles a custom event with descriptive type information.
@@ -11,14 +12,14 @@ export const useCustomEvent = <T, E extends string = string>(
   eventType: E,
   listener: CustomEventListener<T>,
 ): void => {
-  useEffect(() => {
-    const handleEvent: CustomEventListener<T> = (event) => {
-      listener(event);
-    };
-    window.addEventListener(eventType, handleEvent as EventListener);
+  // Ensures that the useEffect call only re-syncs when the eventType changes,
+  // without needing parent component to memoize via useCallback
+  const stableListener = useEffectEvent(listener);
 
+  useEffect(() => {
+    window.addEventListener(eventType, stableListener as EventListener);
     return () => {
-      window.removeEventListener(eventType, handleEvent as EventListener);
+      window.removeEventListener(eventType, stableListener as EventListener);
     };
-  }, [eventType, listener]);
+  }, [eventType, stableListener]);
 };
diff --git a/site/src/hooks/hookPolyfills.ts b/site/src/hooks/hookPolyfills.ts
@@ -12,14 +12,22 @@ import { useCallback, useEffect, useRef } from "react";
  * A DIY version of useEffectEvent.
  *
  * Works like useCallback, except that it doesn't take a dependency array, and
- * always returns out a stable function on every single render. The returned-out
+ * always returns out the same function on every single render. The returned-out
  * function is always able to "see" the most up-to-date version of the callback
- * passed in.
+ * passed in (including its closure values).
  *
- * Should only be used as a last resort when useCallback does not work, but you
- * still need to avoid dependency array violations. (e.g., You need an on-mount
- * effect, but an external library doesn't give their functions stable
- * references, so useEffect/useMemo/useCallback run too often).
+ * This is not a 1:1 replacement for useCallback. 99% of the time,
+ * useEffectEvent should be called in the same component/custom hook where you
+ * have a useEffect call. A useEffectEvent function probably shouldn't be a
+ * prop, unless you're trying to wrangle a weird library.
+ *
+ * Example uses of useEffectEvent:
+ * 1. Stabilizing a function that you don't have direct control over (because it
+ *    comes from a library) without violating useEffect dependency arrays
+ * 2. Moving the burden of memoization from the parent to the custom hook (e.g.,
+ *    making it so that you don't need your components to always use useCallback
+ *    just to get things wired up properly. Similar example: the queryFn
+ *    property on React Query's useQuery)
  *
  * @see {@link https://react.dev/reference/react/experimental_useEffectEvent}
  */

diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx
@@ -4,6 +4,21 @@ import { renderWithAuth } from "testHelpers/renderHelpers";
 import { Language as SSHKeysPageLanguage, SSHKeysPage } from "./SSHKeysPage";
 import { MockGitSSHKey, mockApiError } from "testHelpers/entities";
 
+/**
+ * React Query will automatically log all errors outside of production mode,
+ * even if you're running a test where they're expected
+ *
+ * v4 had a way to set custom loggers, but they're removed in v5. Only other
+ * option is changing the env directly, which feels super dicey
+ */
+beforeAll(() => {
+  jest.spyOn(console, "error").mockImplementation(() => {});
+});
+
+afterAll(() => {
+  jest.clearAllMocks();
+});
+
 describe("SSH keys Page", () => {
   it("shows the SSH key", async () => {
     renderWithAuth(<SSHKeysPage />);
@@ -59,7 +74,7 @@ describe("SSH keys Page", () => {
 
         jest.spyOn(API, "regenerateUserSSHKey").mockRejectedValueOnce(
           mockApiError({
-            message: "Error regenerating SSH key",
+            message: SSHKeysPageLanguage.regenerationError,
           }),
         );
 
@@ -78,7 +93,8 @@ describe("SSH keys Page", () => {
         fireEvent.click(confirmButton);
 
         // Check if the error message is displayed
-        await screen.findByText("Error regenerating SSH key");
+        const alert = await screen.findByRole("alert");
+        expect(alert).toHaveTextContent(SSHKeysPageLanguage.regenerationError);
 
         // Check if the API was called correctly
         expect(API.regenerateUserSSHKey).toBeCalledTimes(1);

diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx
@@ -10,6 +10,8 @@ import { getErrorMessage } from "api/errors";
 export const Language = {
   title: "SSH keys",
   regenerateDialogTitle: "Regenerate SSH key?",
+  regenerationError: "Failed to regenerate SSH key",
+  regenerationSuccess: "SSH Key regenerated successfully.",
   regenerateDialogMessage:
     "You will need to replace the public SSH key on services you use it with, and you'll need to rebuild existing workspaces.",
   confirmLabel: "Confirm",
@@ -19,8 +21,9 @@ export const Language = {
 export const SSHKeysPage: FC<PropsWithChildren<unknown>> = () => {
   const [isConfirmingRegeneration, setIsConfirmingRegeneration] =
     useState(false);
-  const queryClient = useQueryClient();
+
   const userSSHKeyQuery = useQuery(userSSHKey("me"));
+  const queryClient = useQueryClient();
   const regenerateSSHKeyMutation = useMutation(
     regenerateUserSSHKey("me", queryClient),
   );
@@ -31,11 +34,8 @@ export const SSHKeysPage: FC<PropsWithChildren<unknown>> = () => {
         <SSHKeysPageView
           isLoading={userSSHKeyQuery.isLoading}
           getSSHKeyError={userSSHKeyQuery.error}
-          regenerateSSHKeyError={regenerateSSHKeyMutation.error}
           sshKey={userSSHKeyQuery.data}
-          onRegenerateClick={() => {
-            setIsConfirmingRegeneration(true);
-          }}
+          onRegenerateClick={() => setIsConfirmingRegeneration(true)}
         />
       </Section>
 
@@ -45,23 +45,19 @@ export const SSHKeysPage: FC<PropsWithChildren<unknown>> = () => {
         open={isConfirmingRegeneration}
         confirmLoading={regenerateSSHKeyMutation.isLoading}
         title={Language.regenerateDialogTitle}
+        description={Language.regenerateDialogMessage}
         confirmText={Language.confirmLabel}
+        onClose={() => setIsConfirmingRegeneration(false)}
         onConfirm={async () => {
           try {
             await regenerateSSHKeyMutation.mutateAsync();
-            displaySuccess("SSH Key regenerated successfully.");
+            displaySuccess(Language.regenerationSuccess);
           } catch (error) {
-            displayError(
-              getErrorMessage(error, "Failed to regenerate SSH key"),
-            );
+            displayError(getErrorMessage(error, Language.regenerationError));
           } finally {
             setIsConfirmingRegeneration(false);
           }
         }}
-        onClose={() => {
-          setIsConfirmingRegeneration(false);
-        }}
-        description={<>{Language.regenerateDialogMessage}</>}
       />
     </>
   );

diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.stories.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.stories.tsx
@@ -35,11 +35,3 @@ export const WithGetSSHKeyError: Story = {
     }),
   },
 };
-
-export const WithRegenerateSSHKeyError: Story = {
-  args: {
-    regenerateSSHKeyError: mockApiError({
-      message: "Failed to regenerate SSH key",
-    }),
-  },
-};
diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.tsx
@@ -11,15 +11,13 @@ import { ErrorAlert } from "components/Alert/ErrorAlert";
 export interface SSHKeysPageViewProps {
   isLoading: boolean;
   getSSHKeyError?: unknown;
-  regenerateSSHKeyError?: unknown;
   sshKey?: GitSSHKey;
   onRegenerateClick: () => void;
 }
 
 export const SSHKeysPageView: FC<PropsWithChildren<SSHKeysPageViewProps>> = ({
   isLoading,
   getSSHKeyError,
-  regenerateSSHKeyError,
   sshKey,
   onRegenerateClick,
 }) => {
@@ -38,9 +36,7 @@ export const SSHKeysPageView: FC<PropsWithChildren<SSHKeysPageViewProps>> = ({
       {/* Regenerating the key is not an option if getSSHKey fails.
         Only one of the error messages will exist at a single time */}
       {Boolean(getSSHKeyError) && <ErrorAlert error={getSSHKeyError} />}
-      {Boolean(regenerateSSHKeyError) && (
-        <ErrorAlert error={regenerateSSHKeyError} dismissible />
-      )}
+
       {sshKey && (
         <>
           <p