diff --git a/site/src/components/GlobalSnackbar/GlobalSnackbar.tsx b/site/src/components/GlobalSnackbar/GlobalSnackbar.tsx index d521812316468..a2f2b9b11f4ac 100644 --- 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(false); const [notification, setNotification] = useState(); - const handleNotification = useCallback>( - (event) => { - setNotification(event.detail); - setOpen(true); - }, - [], - ); - - useCustomEvent(SnackbarEventType, handleNotification); - - const renderAdditionalMessage = (msg: AdditionalMessage, idx: number) => { - if (isNotificationText(msg)) { - return ( - - {msg} - - ); - } else if (isNotificationTextPrefixed(msg)) { - return ( - - {msg.prefix}: {msg.text} - - ); - } else if (isNotificationList(msg)) { - return ( -
    - {msg.map((item, idx) => ( -
  • - - {item} - -
  • - ))} -
- ); - } - return null; - }; + useCustomEvent(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={
{notification.msgType === MsgType.Error && ( )} +
{notification.msg} + {notification.additionalMsgs && - notification.additionalMsgs.map(renderAdditionalMessage)} + notification.additionalMsgs.map((msg, index) => ( + + ))}
} - 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>; + +function AdditionalMessageDisplay({ message }: { message: AdditionalMessage }) { + if (isNotificationText(message)) { + return ( + + {message} + + ); + } + + if (isNotificationTextPrefixed(message)) { + return ( + + {message.prefix}: {message.text} + + ); + } + + if (isNotificationList(message)) { + return ( +
    + {message.map((item, idx) => ( +
  • + + {item} + +
  • + ))} +
+ ); + } + + return null; +} diff --git a/site/src/hooks/events.ts b/site/src/hooks/events.ts index 4a67319a7666a..82004ea1d7ed5 100644 --- 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 = ( eventType: E, listener: CustomEventListener, ): void => { - useEffect(() => { - const handleEvent: CustomEventListener = (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 index 9adfa6b91059f..e5ba705296229 100644 --- 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 index d5d84ec767876..bb0ddb2535bf1 100644 --- a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx +++ b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx @@ -59,7 +59,7 @@ describe("SSH keys Page", () => { jest.spyOn(API, "regenerateUserSSHKey").mockRejectedValueOnce( mockApiError({ - message: "Error regenerating SSH key", + message: SSHKeysPageLanguage.regenerationError, }), ); @@ -78,7 +78,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 index 10512591c28b0..a2222262433ca 100644 --- 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> = () => { 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> = () => { { - setIsConfirmingRegeneration(true); - }} + onRegenerateClick={() => setIsConfirmingRegeneration(true)} /> @@ -45,23 +45,19 @@ export const SSHKeysPage: FC> = () => { 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 index 4d8d46b5d4087..1986c7ef0cbdd 100644 --- 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 index 7ded569057a85..a371d487a453b 100644 --- a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.tsx +++ b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.tsx @@ -11,7 +11,6 @@ import { ErrorAlert } from "components/Alert/ErrorAlert"; export interface SSHKeysPageViewProps { isLoading: boolean; getSSHKeyError?: unknown; - regenerateSSHKeyError?: unknown; sshKey?: GitSSHKey; onRegenerateClick: () => void; } @@ -19,7 +18,6 @@ export interface SSHKeysPageViewProps { export const SSHKeysPageView: FC> = ({ isLoading, getSSHKeyError, - regenerateSSHKeyError, sshKey, onRegenerateClick, }) => { @@ -38,9 +36,7 @@ export const SSHKeysPageView: FC> = ({ {/* Regenerating the key is not an option if getSSHKey fails. Only one of the error messages will exist at a single time */} {Boolean(getSSHKeyError) && } - {Boolean(regenerateSSHKeyError) && ( - - )} + {sshKey && ( <>