8000 fix(site): update `useClipboard` to work better with effect logic (#2… · coder/coder@156f985 · GitHub
[go: up one dir, main page]
Skip to content

Commit 156f985

Browse files
ParkreinerParkreiner
authored
fix(site): update useClipboard to work better with effect logic (#20183)
## Changes made - Updated `useClipboard` API to require passing the text in via the `copyToClipboard` function, rather than requiring that the text gets specified in render logic - Ensured that the `copyToClipboard` function always stays stable across all React lifecycles - Updated all existing uses to use the new function signatures - Updated all tests and added new cases
1 parent 09f69ef commit 156f985

File tree

9 files changed

+173
-87
lines changed

9 files changed

+173
-87
lines changed

‎site/src/components/CopyButton/CopyButton.tsx‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,7 @@ export const CopyButton: FC<CopyButtonProps> = ({
1919
label,
2020
...buttonProps
2121
}) => {
22-
const { showCopiedSuccess, copyToClipboard } = useClipboard({
23-
textToCopy: text,
24-
});
22+
const { showCopiedSuccess, copyToClipboard } = useClipboard();
2523

2624
return (
2725
<TooltipProvider>
@@ -30,7 +28,7 @@ export const CopyButton: FC<CopyButtonProps> = ({
3028
<Button
3129
size="icon"
3230
variant="subtle"
33-
onClick={copyToClipboard}
31+
onClick={() => copyToClipboard(text)}
3432
{...buttonProps}
3533
>
3634
{showCopiedSuccess ? <CheckIcon /> : <CopyIcon />}

‎site/src/components/CopyableValue/CopyableValue.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@ export const CopyableValue: FC<CopyableValueProps> = ({
1616
children,
1717
...attrs
1818
}) => {
19-
const { showCopiedSuccess, copyToClipboard } = useClipboard({
20-
textToCopy: value,
19+
const { showCopiedSuccess, copyToClipboard } = useClipboard();
20+
const clickableProps = useClickable<HTMLSpanElement>(() => {
21+
copyToClipboard(value);
2122
});
22-
const clickableProps = useClickable<HTMLSpanElement>(copyToClipboard);
2323

2424
return (
2525
<Tooltip

‎site/src/hooks/useClickable.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
type RefObject,
55
useRef,
66
} from "react";
7+
import { useEffectEvent } from "./hookPolyfills";
78

89
// Literally any object (ideally an HTMLElement) that has a .click method
910
type ClickableElement = {
@@ -43,10 +44,11 @@ export const useClickable = <
4344
role?: TRole,
4445
): UseClickableResult<TElement, TRole> => {
4546
const ref = useRef<TElement>(null);
47+
const stableOnClick = useEffectEvent(onClick);
4648

4749
return {
4850
ref,
49-
onClick,
51+
onClick: stableOnClick,
5052
tabIndex: 0,
5153
role: (role ?? "button") as TRole,
5254

‎site/src/hooks/useClipboard.test.tsx‎

Lines changed: 96 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,11 @@
99
* immediately pollutes the tests with false negatives. Even if something should
1010
* fail, it won't.
1111
*/
12-
import { act, renderHook, screen } from "@testing-library/react";
12+
13+
import { renderHook, screen } from "@testing-library/react";
1314
import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar";
1415
import { ThemeOverride } from "contexts/ThemeProvider";
16+
import { act } from "react";
1517
import themes, { DEFAULT_THEME } from "theme";
1618
import {
1719
COPY_FAILED_MESSAGE,
@@ -115,8 +117,8 @@ function setupMockClipboard(isSecure: boolean): SetupMockClipboardResult {
115117
};
116118
}
117119

118-
function renderUseClipboard<TInput extends UseClipboardInput>(inputs: TInput) {
119-
return renderHook<UseClipboardResult, TInput>(
120+
function renderUseClipboard(inputs?: UseClipboardInput) {
121+
return renderHook<UseClipboardResult, UseClipboardInput>(
120122
(props) => useClipboard(props),
121123
{
122124
initialProps: inputs,
@@ -188,9 +190,9 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
188190

189191
const assertClipboardUpdateLifecycle = async (
190192
result: RenderResult,
191-
textToCheck: string,
193+
textToCopy: string,
192194
): Promise<void> => {
193-
await act(() => result.current.copyToClipboard());
195+
await act(() => result.current.copyToClipboard(textToCopy));
194196
expect(result.current.showCopiedSuccess).toBe(true);
195197

196198
// Because of timing trickery, any timeouts for flipping the copy status
@@ -203,35 +205,35 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
203205
await act(() => jest.runAllTimersAsync());
204206

205207
const clipboardText = getClipboardText();
206-
expect(clipboardText).toEqual(textToCheck);
208+
expect(clipboardText).toEqual(textToCopy);
207209
};
208210

209211
it("Copies the current text to the user's clipboard", async () => {
210212
const textToCopy = "dogs";
211-
const { result } = renderUseClipboard({ textToCopy });
213+
const { result } = renderUseClipboard();
212214
await assertClipboardUpdateLifecycle(result, textToCopy);
213215
});
214216

215217
it("Should indicate to components not to show successful copy after a set period of time", async () => {
216218
const textToCopy = "cats";
217-
const { result } = renderUseClipboard({ textToCopy });
219+
const { result } = renderUseClipboard();
218220
await assertClipboardUpdateLifecycle(result, textToCopy);
219221
expect(result.current.showCopiedSuccess).toBe(false);
220222
});
221223

222224
it("Should notify the user of an error using the provided callback", async () => {
223225
const textToCopy = "birds";
224226
const onError = jest.fn();
225-
const { result } = renderUseClipboard({ textToCopy, onError });
227+
const { result } = renderUseClipboard({ onError });
226228

227229
setSimulateFailure(true);
228-
await act(() => result.current.copyToClipboard());
230+
await act(() => result.current.copyToClipboard(textToCopy));
229231
expect(onError).toBeCalled();
230232
});
231233

232234
it("Should dispatch a new toast message to the global snackbar when errors happen while no error callback is provided to the hook", async () => {
233235
const textToCopy = "crow";
234-
const { result } = renderUseClipboard({ textToCopy });
236+
const { result } = renderUseClipboard();
235237

236238
/**
237239
* @todo Look into why deferring error-based state updates to the global
@@ -241,7 +243,7 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
241243
* flushed through the GlobalSnackbar component afterwards
242244
*/
243245
setSimulateFailure(true);
244-
await act(() => result.current.copyToClipboard());
246+
await act(() => result.current.copyToClipboard(textToCopy));
245247

246248
const errorMessageNode = screen.queryByText(COPY_FAILED_MESSAGE);
247249
expect(errorMessageNode).not.toBeNull();
@@ -252,11 +254,91 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
252254
// Snackbar state transitions that you might get if the hook uses the
253255
// default
254256
const textToCopy = "hamster";
255-
const { result } = renderUseClipboard({ textToCopy, onError: jest.fn() });
257+
const { result } = renderUseClipboard({ onError: jest.fn() });
258+
259+
setSimulateFailure(true);
260+
await act(() => result.current.copyToClipboard(textToCopy));
261+
262+
expect(result.current.error).toBeInstanceOf(Error);
263+
});
256264

265+
it("Clears out existing errors if a new copy operation succeeds", async () => {
266< 6B28 /code>+
const text = "dummy-text";
267+
const { result } = renderUseClipboard();
257268
setSimulateFailure(true);
258-
await act(() => result.current.copyToClipboard());
259269

270+
await act(() => result.current.copyToClipboard(text));
260271
expect(result.current.error).toBeInstanceOf(Error);
272+
273+
setSimulateFailure(false);
274+
await assertClipboardUpdateLifecycle(result, text);
275+
expect(result.current.error).toBeUndefined();
276+
});
277+
278+
// This test case is really important to ensure that it's easy to plop this
279+
// inside of useEffect calls without having to think about dependencies too
280+
// much
281+
it("Ensures that the copyToClipboard function always maintains a stable reference across all re-renders", async () => {
282+
const initialOnError = jest.fn();
283+
const { result, rerender } = renderUseClipboard({
284+
onError: initialOnError,
285+
clearErrorOnSuccess: true,
286+
});
287+
const initialCopy = result.current.copyToClipboard;
288+
289+
// Re-render arbitrarily with no clipboard state transitions to make
290+
// sure that a parent re-rendering doesn't break anything
291+
rerender({ onError: initialOnError });
292+
expect(result.current.copyToClipboard).toBe(initialCopy);
293+
294+
// Re-render with new onError prop and then swap back to simplify
295+
// testing
296+
rerender({ onError: jest.fn() });
297+
expect(result.current.copyToClipboard).toBe(initialCopy);
298+
rerender({ onError: initialOnError });
299+
300+
// Re-render with a new clear value then swap back to simplify testing
301+
rerender({ onError: initialOnError, clearErrorOnSuccess: false });
302+
expect(result.current.copyToClipboard).toBe(initialCopy);
303+
rerender({ onError: initialOnError, clearErrorOnSuccess: true });
304+
305+
// Trigger a failed clipboard interaction
306+
setSimulateFailure(true);
307+
await act(() => result.current.copyToClipboard("dummy-text-2"));
308+
expect(result.current.copyToClipboard).toBe(initialCopy);
309+
310+
/**
311+
* Trigger a successful clipboard interaction
312+
*
313+
* @todo For some reason, using the assertClipboardUpdateLifecycle
314+
* helper triggers Jest errors with it thinking that values are being
315+
* accessed after teardown, even though the problem doesn't exist for
316+
* any other test case.
317+
*
318+
* It's not a huge deal, because we only need to inspect React after the
319+
* interaction, instead of the full DOM, but for correctness, it would
320+
* be nice if we could get this issue figured out.
321+
*/
322+
setSimulateFailure(false);
323+
await act(() => result.current.copyToClipboard("dummy-text-2"));
324+
expect(result.current.copyToClipboard).toBe(initialCopy);
325+
});
326+
327+
it("Always uses the most up-to-date onError prop", async () => {
328+
const initialOnError = jest.fn();
329+
const { result, rerender } = renderUseClipboard({
330+
onError: initialOnError,
331+
});
332+
setSimulateFailure(true);
333+
334+
const secondOnError = jest.fn();
335+
rerender({ onError: secondOnError });
336+
await act(() => result.current.copyToClipboard("dummy-text"));
337+
338+
expect(initialOnError).not.toHaveBeenCalled();
339+
expect(secondOnError).toHaveBeenCalledTimes(1);
340+
expect(secondOnError).toHaveBeenCalledWith(
341+
"Failed to copy text to clipboard",
342+
);
261343
});
262344
});

‎site/src/hooks/useClipboard.ts‎

Lines changed: 41 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,18 @@
11
import { displayError } from "components/GlobalSnackbar/utils";
2-
import { useEffect, useRef, useState } from "react";
2+
import { useCallback, useEffect, useRef, useState } from "react";
3+
import { useEffectEvent } from "./hookPolyfills";
34

45
const CLIPBOARD_TIMEOUT_MS = 1_000;
56
export const COPY_FAILED_MESSAGE = "Failed to copy text to clipboard";
67
export const HTTP_FALLBACK_DATA_ID = "http-fallback";
78

89
export type UseClipboardInput = Readonly<{
9-
textToCopy: string;
10-
11-
/**
12-
* Optional callback to call when an error happens. If not specified, the hook
13-
* will dispatch an error message to the GlobalSnackbar
14-
*/
1510
onError?: (errorMessage: string) => void;
11+
clearErrorOnSuccess?: boolean;
1612
}>;
1713

1814
export type UseClipboardResult = Readonly<{
19-
copyToClipboard: () => Promise<void>;
15+
copyToClipboard: (textToCopy: string) => Promise<void>;
2016
error: Error | undefined;
2117

2218
/**
@@ -40,47 +36,56 @@ export type UseClipboardResult = Readonly<{
4036
showCopiedSuccess: boolean;
4137
}>;
4238

43-
export const useClipboard = (input: UseClipboardInput): UseClipboardResult => {
44-
const { textToCopy, onError: errorCallback } = input;
39+
export const useClipboard = (input?: UseClipboardInput): UseClipboardResult => {
40+
const { onError = displayError, clearErrorOnSuccess = true } = input ?? {};
41+
4542
const [showCopiedSuccess, setShowCopiedSuccess] = useState(false);
4643
const [error, setError] = useState<Error>();
4744
const timeoutIdRef = useRef<number | undefined>(undefined);
4845

4946
useEffect(() => {
50-
const clearIdOnUnmount = () => window.clearTimeout(timeoutIdRef.current);
51-
return clearIdOnUnmount;
47+
const clearTimeoutOnUnmount = () => {
48+
window.clearTimeout(timeoutIdRef.current);
49+
};
50+
return clearTimeoutOnUnmount;
5251
}, []);
5352

54-
const handleSuccessfulCopy = () => {
53+
const stableOnError = useEffectEvent(() => onError(COPY_FAILED_MESSAGE));
54+
const handleSuccessfulCopy = useEffectEvent(() => {
5555
setShowCopiedSuccess(true);
56+
if (clearErrorOnSuccess) {
57+
setError(undefined);
58+
}
59+
5660
timeoutIdRef.current = window.setTimeout(() => {
5761
setShowCopiedSuccess(false);
5862
}, CLIPBOARD_TIMEOUT_MS);
59-
};
60-
61-
const copyToClipboard = async () => {
62-
try {
63-
await window.navigator.clipboard.writeText(textToCopy);
64-
handleSuccessfulCopy();
65-
} catch (err) {
66-
const fallbackCopySuccessful = simulateClipboardWrite(textToCopy);
67-
if (fallbackCopySuccessful) {
68-
handleSuccessfulCopy();
69-
return;
70-
}
63+
});
7164

72-
const wrappedErr = new Error(COPY_FAILED_MESSAGE);
73-
if E31B (err instanceof Error) {
74-
wrappedErr.stack = err.stack;
65+
const copyToClipboard = useCallback(
66+
async (textToCopy: string) => {
67+
try {
68+
await window.navigator.clipboard.writeText(textToCopy);
69+
handleSuccessfulCopy();
70+
} catch (err) {
71+
const fallbackCopySuccessful = simulateClipboardWrite(textToCopy);
72+
if (fallbackCopySuccessful) {
73+
handleSuccessfulCopy();
74+
return;
75+
}
76+
77+
const wrappedErr = new Error(COPY_FAILED_MESSAGE);
78+
if (err instanceof Error) {
79+
wrappedErr.stack = err.stack;
80+
}
81+
82+
console.error(wrappedErr);
83+
setError(wrappedErr);
84+
stableOnError();
7585
}
76-
77-
console.error(wrappedErr);
78-
setError(wrappedErr);
79-
80-
const notifyUser = errorCallback ?? displayError;
81-
notifyUser(COPY_FAILED_MESSAGE);
82-
}
83-
};
86+
},
87+
[stableOnError, handleSuccessfulCopy],
88+
);
8489

8590
return { showCopiedSuccess, error, copyToClipboard };
8691
};

‎site/src/pages/CliAuthPage/CliAuthPageView.tsx‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,7 @@ interface CliAuthPageViewProps {
1212
}
1313

1414
export const CliAuthPageView: FC<CliAuthPageViewProps> = ({ sessionToken }) => {
15-
const clipboard = useClipboard({
16-
textToCopy: sessionToken ?? "",
17-
});
18-
15+
const clipboardState = useClipboard();
1916
return (
2017
<SignInLayout>
2118
<Welcome>Session token</Welcome>
@@ -30,16 +27,20 @@ export const CliAuthPageView: FC<CliAuthPageViewProps> = ({ sessionToken }) => {
3027
className="w-full"
3128
size="lg"
3229
disabled={!sessionToken}
33-
onClick={clipboard.copyToClipboard}
30+
onClick={() => {
31+
if (sessionToken) {
32+
clipboardState.copyToClipboard(sessionToken);
33+
}
34+
}}
3435
>
35-
{clipboard.showCopiedSuccess ? (
36+
{clipboardState.showCopiedSuccess ? (
3637
<CheckIcon />
3738
) : (
3839
<Spinner loading={!sessionToken}>
3940
<CopyIcon />
4041
</Spinner>
4142
)}
42-
{clipboard.showCopiedSuccess
43+
{clipboardState.showCopiedSuccess
4344
? "Session token copied!"
4445
: "Copy session token"}
4546
</Button>

0 commit comments

Comments
 (0)
0