From 9bdda946e2d5fe181094fbdf0ab0ed23c919426b Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 6 Nov 2023 15:31:05 +0000 Subject: [PATCH 1/7] refactor: reorganize SSHKeysPage --- .../UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx index 10512591c28b0..e5f28c25f9d70 100644 --- a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx +++ b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx @@ -19,8 +19,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), ); @@ -33,9 +34,7 @@ export const SSHKeysPage: FC> = () => { getSSHKeyError={userSSHKeyQuery.error} regenerateSSHKeyError={regenerateSSHKeyMutation.error} sshKey={userSSHKeyQuery.data} - onRegenerateClick={() => { - setIsConfirmingRegeneration(true); - }} + onRegenerateClick={() => setIsConfirmingRegeneration(true)} /> @@ -45,7 +44,9 @@ 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(); @@ -58,10 +59,6 @@ export const SSHKeysPage: FC> = () => { setIsConfirmingRegeneration(false); } }} - onClose={() => { - setIsConfirmingRegeneration(false); - }} - description={<>{Language.regenerateDialogMessage}} /> ); From 0e1caf246eab570325b7b0b6d2a8df841019f0aa Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 6 Nov 2023 17:26:12 +0000 Subject: [PATCH 2/7] refactor: update render behavior for GlobalSnackbar --- .../GlobalSnackbar/GlobalSnackbar.tsx | 104 ++++++++---------- site/src/hooks/events.ts | 15 +-- site/src/hooks/hookPolyfills.ts | 16 ++- 3 files changed, 66 insertions(+), 69 deletions(-) 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..4118eb163496d 100644 --- a/site/src/hooks/hookPolyfills.ts +++ b/site/src/hooks/hookPolyfills.ts @@ -14,12 +14,18 @@ import { useCallback, useEffect, useRef } from "react"; * 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 * 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, and should not ever be a prop. + * + * 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. See also: React Query's queryFn.) * * @see {@link https://react.dev/reference/react/experimental_useEffectEvent} */ From bc9b5afd3342624483f4d628630133664025bb0d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 6 Nov 2023 18:19:47 +0000 Subject: [PATCH 3/7] fix: remove redundant error handling --- .../SSHKeysPage/SSHKeysPage.test.tsx | 15 +++++++++++++++ .../SSHKeysPage/SSHKeysPage.tsx | 19 ++++++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx index d5d84ec767876..93f4a3fc16a43 100644 --- 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(); diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx index e5f28c25f9d70..b40206598dad3 100644 --- a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx +++ b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx @@ -1,11 +1,10 @@ import { PropsWithChildren, FC, useState } from "react"; import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"; -import { displayError, displaySuccess } from "components/GlobalSnackbar/utils"; +import { displaySuccess } from "components/GlobalSnackbar/utils"; import { Section } from "components/SettingsLayout/Section"; import { SSHKeysPageView } from "./SSHKeysPageView"; import { regenerateUserSSHKey, userSSHKey } from "api/queries/sshKeys"; import { useMutation, useQuery, useQueryClient } from "react-query"; -import { getErrorMessage } from "api/errors"; export const Language = { title: "SSH keys", @@ -22,9 +21,12 @@ export const SSHKeysPage: FC> = () => { const userSSHKeyQuery = useQuery(userSSHKey("me")); const queryClient = useQueryClient(); - const regenerateSSHKeyMutation = useMutation( - regenerateUserSSHKey("me", queryClient), - ); + const regenerateSSHKeyMutation = useMutation({ + ...regenerateUserSSHKey("me", queryClient), + onError: () => { + // + }, + }); return ( <> @@ -51,10 +53,9 @@ export const SSHKeysPage: FC> = () => { try { await regenerateSSHKeyMutation.mutateAsync(); displaySuccess("SSH Key regenerated successfully."); - } catch (error) { - displayError( - getErrorMessage(error, "Failed to regenerate SSH key"), - ); + } catch (err) { + // No error handling because UI displays the error message after + // React Query automatically puts it into state } finally { setIsConfirmingRegeneration(false); } From 3f491e1dd9603e357b06ae0f149839af252ceabe Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 6 Nov 2023 18:24:35 +0000 Subject: [PATCH 4/7] docs: Clean up wording on docs --- site/src/hooks/hookPolyfills.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/site/src/hooks/hookPolyfills.ts b/site/src/hooks/hookPolyfills.ts index 4118eb163496d..e5ba705296229 100644 --- a/site/src/hooks/hookPolyfills.ts +++ b/site/src/hooks/hookPolyfills.ts @@ -12,20 +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 (including its closure values). * * 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, and should not ever be a prop. + * 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. See also: React Query's queryFn.) + * 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} */ From c619d3420b8b26b18b5df8dd89ff6e8185d305ad Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 6 Nov 2023 18:26:47 +0000 Subject: [PATCH 5/7] fix: remove temp error handling tests --- .../pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx index b40206598dad3..9be60b108aa1b 100644 --- a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx +++ b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx @@ -21,12 +21,9 @@ export const SSHKeysPage: FC> = () => { const userSSHKeyQuery = useQuery(userSSHKey("me")); const queryClient = useQueryClient(); - const regenerateSSHKeyMutation = useMutation({ - ...regenerateUserSSHKey("me", queryClient), - onError: () => { - // - }, - }); + const regenerateSSHKeyMutation = useMutation( + regenerateUserSSHKey("me", queryClient), + ); return ( <> From 8083591bc758b58d955d8aace8b4421321d05f4b Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 6 Nov 2023 19:36:01 +0000 Subject: [PATCH 6/7] fix: remove local error alert --- .../SSHKeysPage/SSHKeysPage.test.tsx | 5 +++-- .../UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx | 13 +++++++------ .../SSHKeysPage/SSHKeysPageView.stories.tsx | 8 -------- .../SSHKeysPage/SSHKeysPageView.tsx | 6 +----- 4 files changed, 11 insertions(+), 21 deletions(-) diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx index 93f4a3fc16a43..61644e9e8d4a5 100644 --- a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx +++ b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx @@ -74,7 +74,7 @@ describe("SSH keys Page", () => { jest.spyOn(API, "regenerateUserSSHKey").mockRejectedValueOnce( mockApiError({ - message: "Error regenerating SSH key", + message: SSHKeysPageLanguage.regenerationError, }), ); @@ -93,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 index 9be60b108aa1b..a2222262433ca 100644 --- a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx +++ b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx @@ -1,14 +1,17 @@ import { PropsWithChildren, FC, useState } from "react"; import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"; -import { displaySuccess } from "components/GlobalSnackbar/utils"; +import { displayError, displaySuccess } from "components/GlobalSnackbar/utils"; import { Section } from "components/SettingsLayout/Section"; import { SSHKeysPageView } from "./SSHKeysPageView"; import { regenerateUserSSHKey, userSSHKey } from "api/queries/sshKeys"; import { useMutation, useQuery, useQueryClient } from "react-query"; +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", @@ -31,7 +34,6 @@ export const SSHKeysPage: FC> = () => { setIsConfirmingRegeneration(true)} /> @@ -49,10 +51,9 @@ export const SSHKeysPage: FC> = () => { onConfirm={async () => { try { await regenerateSSHKeyMutation.mutateAsync(); - displaySuccess("SSH Key regenerated successfully."); - } catch (err) { - // No error handling because UI displays the error message after - // React Query automatically puts it into state + displaySuccess(Language.regenerationSuccess); + } catch (error) { + displayError(getErrorMessage(error, Language.regenerationError)); } finally { setIsConfirmingRegeneration(false); } 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 && ( <>

Date: Mon, 6 Nov 2023 19:47:23 +0000 Subject: [PATCH 7/7] fix: remove error logging hacks --- .../SSHKeysPage/SSHKeysPage.test.tsx | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx index 61644e9e8d4a5..bb0ddb2535bf1 100644 --- a/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx +++ b/site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx @@ -4,21 +4,6 @@ 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();