Skip to content

Commit 8c3828b

Browse files
ParkreinerParkreiner
authored
fix: stop SSHKeysPage from flaking (#10553)
* refactor: reorganize SSHKeysPage * refactor: update render behavior for GlobalSnackbar * fix: remove redundant error handling * docs: Clean up wording on docs * fix: remove temp error handling tests * fix: remove local error alert * fix: remove error logging hacks
1 parent b83a8ce commit 8c3828b

File tree

7 files changed

+82
-98
lines changed

7 files changed

+82
-98
lines changed

‎site/src/components/GlobalSnackbar/GlobalSnackbar.tsx

+47-57
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
import { type FC, useCallback, useState } from "react";
1+
import { type FC, useState } from "react";
22
import { useCustomEvent } from "hooks/events";
3-
import type { CustomEventListener } from "utils/events";
43
import { EnterpriseSnackbar } from "./EnterpriseSnackbar";
54
import { ErrorIcon } from "../Icons/ErrorIcon";
65
import { Typography } from "../Typography/Typography";
@@ -29,54 +28,10 @@ export const GlobalSnackbar: FC = () => {
2928
const [open, setOpen] = useState<boolean>(false);
3029
const [notification, setNotification] = useState<NotificationMsg>();
3130

32-
const handleNotification = useCallback<CustomEventListener<NotificationMsg>>(
33-
(event) => {
34-
setNotification(event.detail);
35-
setOpen(true);
36-
},
37-
[],
38-
);
39-
40-
useCustomEvent(SnackbarEventType, handleNotification);
41-
42-
const renderAdditionalMessage = (msg: AdditionalMessage, idx: number) => {
43-
if (isNotificationText(msg)) {
44-
return (
45-
<Typography
46-
key={idx}
47-
gutterBottom
48-
variant="body2"
49-
css={styles.messageSubtitle}
50-
>
51-
{msg}
52-
</Typography>
53-
);
54-
} else if (isNotificationTextPrefixed(msg)) {
55-
return (
56-
<Typography
57-
key={idx}
58-
gutterBottom
59-
variant="body2"
60-
css={styles.messageSubtitle}
61-
>
62-
<strong>{msg.prefix}:</strong> {msg.text}
63-
</Typography>
64-
);
65-
} else if (isNotificationList(msg)) {
66-
return (
67-
<ul css={styles.list} key={idx}>
68-
{msg.map((item, idx) => (
69-
<li key={idx}>
70-
<Typography variant="body2" css={styles.messageSubtitle}>
71-
{item}
72-
</Typography>
73-
</li>
74-
))}
75-
</ul>
76-
);
77-
}
78-
return null;
79-
};
31+
useCustomEvent<NotificationMsg>(SnackbarEventType, (event) => {
32+
setNotification(event.detail);
33+
setOpen(true);
34+
});
8035

8136
if (!notification) {
8237
return null;
@@ -87,26 +42,27 @@ export const GlobalSnackbar: FC = () => {
8742
key={notification.msg}
8843
open={open}
8944
variant={variantFromMsgType(notification.msgType)}
45+
onClose={() => setOpen(false)}
46+
autoHideDuration={notification.msgType === MsgType.Error ? 22000 : 6000}
47+
anchorOrigin={{ vertical: "bottom", horizontal: "right" }}
9048
message={
9149
<div css={styles.messageWrapper}>
9250
{notification.msgType === MsgType.Error && (
9351
<ErrorIcon css={styles.errorIcon} />
9452
)}
53+
9554
<div css={styles.message}>
9655
<Typography variant="body1" css={styles.messageTitle}>
9756
{notification.msg}
9857
</Typography>
58+
9959
{notification.additionalMsgs &&
100-
notification.additionalMsgs.map(renderAdditionalMessage)}
60+
notification.additionalMsgs.map((msg, index) => (
61+
<AdditionalMessageDisplay key={index} message={msg} />
62+
))}
10163
</div>
10264
</div>
10365
}
104-
onClose={() => setOpen(false)}
105-
autoHideDuration={notification.msgType === MsgType.Error ? 22000 : 6000}
106-
anchorOrigin={{
107-
vertical: "bottom",
108-
horizontal: "right",
109-
}}
11066
/>
11167
);
11268
};
@@ -133,3 +89,37 @@ const styles = {
13389
marginRight: 16,
13490
}),
13591
} satisfies Record<string, Interpolation<Theme>>;
92+
93+
function AdditionalMessageDisplay({ message }: { message: AdditionalMessage }) {
94+
if (isNotificationText(message)) {
95+
return (
96+
<Typography gutterBottom variant="body2" css={styles.messageSubtitle}>
97+
{message}
98+
</Typography>
99+
);
100+
}
101+
102+
if (isNotificationTextPrefixed(message)) {
103+
return (
104+
<Typography gutterBottom variant="body2" css={styles.messageSubtitle}>
105+
<strong>{message.prefix}:</strong> {message.text}
106+
</Typography>
107+
);
108+
}
109+
110+
if (isNotificationList(message)) {
111+
return (
112+
<ul css={styles.list}>
113+
{message.map((item, idx) => (
114+
<li key={idx}>
115+
<Typography variant="body2" css={styles.messageSubtitle}>
116+
{item}
117+
</Typography>
118+
</li>
119+
))}
120+
</ul>
121+
);
122+
}
123+
124+
return null;
125+
}

‎site/src/hooks/events.ts

+8-7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { useEffect } from "react";
22
import { CustomEventListener } from "utils/events";
3+
import { useEffectEvent } from "./hookPolyfills";
34

45
/**
56
* Handles a custom event with descriptive type information.
@@ -11,14 +12,14 @@ export const useCustomEvent = <T, E extends string = string>(
1112
eventType: E,
1213
listener: CustomEventListener<T>,
1314
): void => {
14-
useEffect(() => {
15-
const handleEvent: CustomEventListener<T> = (event) => {
16-
listener(event);
17-
};
18-
window.addEventListener(eventType, handleEvent as EventListener);
15+
// Ensures that the useEffect call only re-syncs when the eventType changes,
16+
// without needing parent component to memoize via useCallback
17+
const stableListener = useEffectEvent(listener);
1918

19+
useEffect(() => {
20+
window.addEventListener(eventType, stableListener as EventListener);
2021
return () => {
21-
window.removeEventListener(eventType, handleEvent as EventListener);
22+
window.removeEventListener(eventType, stableListener as EventListener);
2223
};
23-
}, [eventType, listener]);
24+
}, [eventType, stableListener]);
2425
};

‎site/src/hooks/hookPolyfills.ts

+14-6
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,22 @@ import { useCallback, useEffect, useRef } from "react";
1212
* A DIY version of useEffectEvent.
1313
*
1414
* Works like useCallback, except that it doesn't take a dependency array, and
15-
* always returns out a stable function on every single render. The returned-out
15+
* always returns out the same function on every single render. The returned-out
1616
* function is always able to "see" the most up-to-date version of the callback
17-
* passed in.
17+
* passed in (including its closure values).
1818
*
19-
* Should only be used as a last resort when useCallback does not work, but you
20-
* still need to avoid dependency array violations. (e.g., You need an on-mount
21-
* effect, but an external library doesn't give their functions stable
22-
* references, so useEffect/useMemo/useCallback run too often).
19+
* This is not a 1:1 replacement for useCallback. 99% of the time,
20+
* useEffectEvent should be called in the same component/custom hook where you
21+
* have a useEffect call. A useEffectEvent function probably shouldn't be a
22+
* prop, unless you're trying to wrangle a weird library.
23+
*
24+
* Example uses of useEffectEvent:
25+
* 1. Stabilizing a function that you don't have direct control over (because it
26+
* comes from a library) without violating useEffect dependency arrays
27+
* 2. Moving the burden of memoization from the parent to the custom hook (e.g.,
28+
* making it so that you don't need your components to always use useCallback
29+
* just to get things wired up properly. Similar example: the queryFn
30+
* property on React Query's useQuery)
2331
*
2432
* @see {@link https://react.dev/reference/react/experimental_useEffectEvent}
2533
*/

‎site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.test.tsx

+3-2
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ describe("SSH keys Page", () => {
5959

6060
jest.spyOn(API, "regenerateUserSSHKey").mockRejectedValueOnce(
6161
mockApiError({
62-
message: "Error regenerating SSH key",
62+
message: SSHKeysPageLanguage.regenerationError,
6363
}),
6464
);
6565

@@ -78,7 +78,8 @@ describe("SSH keys Page", () => {
7878
fireEvent.click(confirmButton);
7979

8080
// Check if the error message is displayed
81-
await screen.findByText("Error regenerating SSH key");
81+
const alert = await screen.findByRole("alert");
82+
expect(alert).toHaveTextContent(SSHKeysPageLanguage.regenerationError);
8283

8384
// Check if the API was called correctly
8485
expect(API.regenerateUserSSHKey).toBeCalledTimes(1);

‎site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPage.tsx

+9-13
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import { getErrorMessage } from "api/errors";
1010
export const Language = {
1111
title: "SSH keys",
1212
regenerateDialogTitle: "Regenerate SSH key?",
13+
regenerationError: "Failed to regenerate SSH key",
14+
regenerationSuccess: "SSH Key regenerated successfully.",
1315
regenerateDialogMessage:
1416
"You will need to replace the public SSH key on services you use it with, and you'll need to rebuild existing workspaces.",
1517
confirmLabel: "Confirm",
@@ -19,8 +21,9 @@ export const Language = {
1921
export const SSHKeysPage: FC<PropsWithChildren<unknown>> = () => {
2022
const [isConfirmingRegeneration, setIsConfirmingRegeneration] =
2123
useState(false);
22-
const queryClient = useQueryClient();
24+
2325
const userSSHKeyQuery = useQuery(userSSHKey("me"));
26+
const queryClient = useQueryClient();
2427
const regenerateSSHKeyMutation = useMutation(
2528
regenerateUserSSHKey("me", queryClient),
2629
);
@@ -31,11 +34,8 @@ export const SSHKeysPage: FC<PropsWithChildren<unknown>> = () => {
3134
<SSHKeysPageView
3235
isLoading={userSSHKeyQuery.isLoading}
3336
getSSHKeyError={userSSHKeyQuery.error}
34-
regenerateSSHKeyError={regenerateSSHKeyMutation.error}
3537
sshKey={userSSHKeyQuery.data}
36-
onRegenerateClick={() => {
37-
setIsConfirmingRegeneration(true);
38-
}}
38+
onRegenerateClick={() => setIsConfirmingRegeneration(true)}
3939
/>
4040
</Section>
4141

@@ -45,23 +45,19 @@ export const SSHKeysPage: FC<PropsWithChildren<unknown>> = () => {
4545
open={isConfirmingRegeneration}
4646
confirmLoading={regenerateSSHKeyMutation.isLoading}
4747
title={Language.regenerateDialogTitle}
48+
description={Language.regenerateDialogMessage}
4849
confirmText={Language.confirmLabel}
50+
onClose={() => setIsConfirmingRegeneration(false)}
4951
onConfirm={async () => {
5052
try {
5153
await regenerateSSHKeyMutation.mutateAsync();
52-
displaySuccess("SSH Key regenerated successfully.");
54+
displaySuccess(Language.regenerationSuccess);
5355
} catch (error) {
54-
displayError(
55-
getErrorMessage(error, "Failed to regenerate SSH key"),
56-
);
56+
displayError(getErrorMessage(error, Language.regenerationError));
5757
} finally {
5858
setIsConfirmingRegeneration(false);
5959
}
6060
}}
61-
onClose={() => {
62-
setIsConfirmingRegeneration(false);
63-
}}
64-
description={<>{Language.regenerateDialogMessage}</>}
6561
/>
6662
</>
6763
);

‎site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.stories.tsx

-8
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,3 @@ export const WithGetSSHKeyError: Story = {
3535
}),
3636
},
3737
};
38-
39-
export const WithRegenerateSSHKeyError: Story = {
40-
args: {
41-
regenerateSSHKeyError: mockApiError({
42-
message: "Failed to regenerate SSH key",
43-
}),
44-
},
45-
};

‎site/src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.tsx

+1-5
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,13 @@ import { ErrorAlert } from "components/Alert/ErrorAlert";
1111
export interface SSHKeysPageViewProps {
1212
isLoading: boolean;
1313
getSSHKeyError?: unknown;
14-
regenerateSSHKeyError?: unknown;
1514
sshKey?: GitSSHKey;
1615
onRegenerateClick: () => void;
1716
}
1817

1918
export const SSHKeysPageView: FC<PropsWithChildren<SSHKeysPageViewProps>> = ({
2019
isLoading,
2120
getSSHKeyError,
22-
regenerateSSHKeyError,
2321
sshKey,
2422
onRegenerateClick,
2523
}) => {
@@ -38,9 +36,7 @@ export const SSHKeysPageView: FC<PropsWithChildren<SSHKeysPageViewProps>> = ({
3836
{/* Regenerating the key is not an option if getSSHKey fails.
3937
Only one of the error messages will exist at a single time */}
4038
{Boolean(getSSHKeyError) && <ErrorAlert error={getSSHKeyError} />}
41-
{Boolean(regenerateSSHKeyError) && (
42-
<ErrorAlert error={regenerateSSHKeyError} dismissible />
43-
)}
39+
4440
{sshKey && (
4541
<>
4642
<p

0 commit comments

Comments
 (0)