From 156f985fb0908f9e6d99ae5cc5f0c1d1360603e4 Mon Sep 17 00:00:00 2001
From: Michael Smith <throwawayclover@gmail.com>
Date: Mon, 6 Oct 2025 15:41:57 -0400
Subject: [PATCH] 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
---
 site/src/components/CopyButton/CopyButton.tsx |   6 +-
 .../CopyableValue/CopyableValue.tsx           |   6 +-
 site/src/hooks/useClickable.ts                |   4 +-
 site/src/hooks/useClipboard.test.tsx          | 110 +++++++++++++++---
 site/src/hooks/useClipboard.ts                |  75 ++++++------
 .../src/pages/CliAuthPage/CliAuthPageView.tsx |  15 +--
 site/src/pages/TaskPage/TaskTopbar.tsx        |   7 +-
 .../TemplateEmbedPage/TemplateEmbedPage.tsx   |  17 +--
 .../TemplateEmbedPageExperimental.tsx         |  18 +--
 9 files changed, 172 insertions(+), 86 deletions(-)

diff --git a/site/src/components/CopyButton/CopyButton.tsx b/site/src/components/CopyButton/CopyButton.tsx
index 9110bb4cd6..c00b9360e4 100644
--- a/site/src/components/CopyButton/CopyButton.tsx
+++ b/site/src/components/CopyButton/CopyButton.tsx
@@ -19,9 +19,7 @@ export const CopyButton: FC<CopyButtonProps> = ({
 	label,
 	...buttonProps
 }) => {
-	const { showCopiedSuccess, copyToClipboard } = useClipboard({
-		textToCopy: text,
-	});
+	const { showCopiedSuccess, copyToClipboard } = useClipboard();
 
 	return (
 		<TooltipProvider>
@@ -30,7 +28,7 @@ export const CopyButton: FC<CopyButtonProps> = ({
 					<Button
 						size="icon"
 						variant="subtle"
-						onClick={copyToClipboard}
+						onClick={() => copyToClipboard(text)}
 						{...buttonProps}
 					>
 						{showCopiedSuccess ? <CheckIcon /> : <CopyIcon />}
diff --git a/site/src/components/CopyableValue/CopyableValue.tsx b/site/src/components/CopyableValue/CopyableValue.tsx
index 1cd3ea4a15..92cb6ec3c4 100644
--- a/site/src/components/CopyableValue/CopyableValue.tsx
+++ b/site/src/components/CopyableValue/CopyableValue.tsx
@@ -16,10 +16,10 @@ export const CopyableValue: FC<CopyableValueProps> = ({
 	children,
 	...attrs
 }) => {
-	const { showCopiedSuccess, copyToClipboard } = useClipboard({
-		textToCopy: value,
+	const { showCopiedSuccess, copyToClipboard } = useClipboard();
+	const clickableProps = useClickable<HTMLSpanElement>(() => {
+		copyToClipboard(value);
 	});
-	const clickableProps = useClickable<HTMLSpanElement>(copyToClipboard);
 
 	return (
 		<Tooltip
diff --git a/site/src/hooks/useClickable.ts b/site/src/hooks/useClickable.ts
index 3f59851b8f..6f5e0afff7 100644
--- a/site/src/hooks/useClickable.ts
+++ b/site/src/hooks/useClickable.ts
@@ -4,6 +4,7 @@ import {
 	type RefObject,
 	useRef,
 } from "react";
+import { useEffectEvent } from "./hookPolyfills";
 
 // Literally any object (ideally an HTMLElement) that has a .click method
 type ClickableElement = {
@@ -43,10 +44,11 @@ export const useClickable = <
 	role?: TRole,
 ): UseClickableResult<TElement, TRole> => {
 	const ref = useRef<TElement>(null);
+	const stableOnClick = useEffectEvent(onClick);
 
 	return {
 		ref,
-		onClick,
+		onClick: stableOnClick,
 		tabIndex: 0,
 		role: (role ?? "button") as TRole,
 
diff --git a/site/src/hooks/useClipboard.test.tsx b/site/src/hooks/useClipboard.test.tsx
index 1d4d2eb702..156d70ab31 100644
--- a/site/src/hooks/useClipboard.test.tsx
+++ b/site/src/hooks/useClipboard.test.tsx
@@ -9,9 +9,11 @@
  * immediately pollutes the tests with false negatives. Even if something should
  * fail, it won't.
  */
-import { act, renderHook, screen } from "@testing-library/react";
+
+import { renderHook, screen } from "@testing-library/react";
 import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar";
 import { ThemeOverride } from "contexts/ThemeProvider";
+import { act } from "react";
 import themes, { DEFAULT_THEME } from "theme";
 import {
 	COPY_FAILED_MESSAGE,
@@ -115,8 +117,8 @@ function setupMockClipboard(isSecure: boolean): SetupMockClipboardResult {
 	};
 }
 
-function renderUseClipboard<TInput extends UseClipboardInput>(inputs: TInput) {
-	return renderHook<UseClipboardResult, TInput>(
+function renderUseClipboard(inputs?: UseClipboardInput) {
+	return renderHook<UseClipboardResult, UseClipboardInput>(
 		(props) => useClipboard(props),
 		{
 			initialProps: inputs,
@@ -188,9 +190,9 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
 
 	const assertClipboardUpdateLifecycle = async (
 		result: RenderResult,
-		textToCheck: string,
+		textToCopy: string,
 	): Promise<void> => {
-		await act(() => result.current.copyToClipboard());
+		await act(() => result.current.copyToClipboard(textToCopy));
 		expect(result.current.showCopiedSuccess).toBe(true);
 
 		// Because of timing trickery, any timeouts for flipping the copy status
@@ -203,18 +205,18 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
 		await act(() => jest.runAllTimersAsync());
 
 		const clipboardText = getClipboardText();
-		expect(clipboardText).toEqual(textToCheck);
+		expect(clipboardText).toEqual(textToCopy);
 	};
 
 	it("Copies the current text to the user's clipboard", async () => {
 		const textToCopy = "dogs";
-		const { result } = renderUseClipboard({ textToCopy });
+		const { result } = renderUseClipboard();
 		await assertClipboardUpdateLifecycle(result, textToCopy);
 	});
 
 	it("Should indicate to components not to show successful copy after a set period of time", async () => {
 		const textToCopy = "cats";
-		const { result } = renderUseClipboard({ textToCopy });
+		const { result } = renderUseClipboard();
 		await assertClipboardUpdateLifecycle(result, textToCopy);
 		expect(result.current.showCopiedSuccess).toBe(false);
 	});
@@ -222,16 +224,16 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
 	it("Should notify the user of an error using the provided callback", async () => {
 		const textToCopy = "birds";
 		const onError = jest.fn();
-		const { result } = renderUseClipboard({ textToCopy, onError });
+		const { result } = renderUseClipboard({ onError });
 
 		setSimulateFailure(true);
-		await act(() => result.current.copyToClipboard());
+		await act(() => result.current.copyToClipboard(textToCopy));
 		expect(onError).toBeCalled();
 	});
 
 	it("Should dispatch a new toast message to the global snackbar when errors happen while no error callback is provided to the hook", async () => {
 		const textToCopy = "crow";
-		const { result } = renderUseClipboard({ textToCopy });
+		const { result } = renderUseClipboard();
 
 		/**
 		 * @todo Look into why deferring error-based state updates to the global
@@ -241,7 +243,7 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
 		 * flushed through the GlobalSnackbar component afterwards
 		 */
 		setSimulateFailure(true);
-		await act(() => result.current.copyToClipboard());
+		await act(() => result.current.copyToClipboard(textToCopy));
 
 		const errorMessageNode = screen.queryByText(COPY_FAILED_MESSAGE);
 		expect(errorMessageNode).not.toBeNull();
@@ -252,11 +254,91 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
 		// Snackbar state transitions that you might get if the hook uses the
 		// default
 		const textToCopy = "hamster";
-		const { result } = renderUseClipboard({ textToCopy, onError: jest.fn() });
+		const { result } = renderUseClipboard({ onError: jest.fn() });
 
 		setSimulateFailure(true);
-		await act(() => result.current.copyToClipboard());
+		await act(() => result.current.copyToClipboard(textToCopy));
 
 		expect(result.current.error).toBeInstanceOf(Error);
 	});
+
+	it("Clears out existing errors if a new copy operation succeeds", async () => {
+		const text = "dummy-text";
+		const { result } = renderUseClipboard();
+		setSimulateFailure(true);
+
+		await act(() => result.current.copyToClipboard(text));
+		expect(result.current.error).toBeInstanceOf(Error);
+
+		setSimulateFailure(false);
+		await assertClipboardUpdateLifecycle(result, text);
+		expect(result.current.error).toBeUndefined();
+	});
+
+	// This test case is really important to ensure that it's easy to plop this
+	// inside of useEffect calls without having to think about dependencies too
+	// much
+	it("Ensures that the copyToClipboard function always maintains a stable reference across all re-renders", async () => {
+		const initialOnError = jest.fn();
+		const { result, rerender } = renderUseClipboard({
+			onError: initialOnError,
+			clearErrorOnSuccess: true,
+		});
+		const initialCopy = result.current.copyToClipboard;
+
+		// Re-render arbitrarily with no clipboard state transitions to make
+		// sure that a parent re-rendering doesn't break anything
+		rerender({ onError: initialOnError });
+		expect(result.current.copyToClipboard).toBe(initialCopy);
+
+		// Re-render with new onError prop and then swap back to simplify
+		// testing
+		rerender({ onError: jest.fn() });
+		expect(result.current.copyToClipboard).toBe(initialCopy);
+		rerender({ onError: initialOnError });
+
+		// Re-render with a new clear value then swap back to simplify testing
+		rerender({ onError: initialOnError, clearErrorOnSuccess: false });
+		expect(result.current.copyToClipboard).toBe(initialCopy);
+		rerender({ onError: initialOnError, clearErrorOnSuccess: true });
+
+		// Trigger a failed clipboard interaction
+		setSimulateFailure(true);
+		await act(() => result.current.copyToClipboard("dummy-text-2"));
+		expect(result.current.copyToClipboard).toBe(initialCopy);
+
+		/**
+		 * Trigger a successful clipboard interaction
+		 *
+		 * @todo For some reason, using the assertClipboardUpdateLifecycle
+		 * helper triggers Jest errors with it thinking that values are being
+		 * accessed after teardown, even though the problem doesn't exist for
+		 * any other test case.
+		 *
+		 * It's not a huge deal, because we only need to inspect React after the
+		 * interaction, instead of the full DOM, but for correctness, it would
+		 * be nice if we could get this issue figured out.
+		 */
+		setSimulateFailure(false);
+		await act(() => result.current.copyToClipboard("dummy-text-2"));
+		expect(result.current.copyToClipboard).toBe(initialCopy);
+	});
+
+	it("Always uses the most up-to-date onError prop", async () => {
+		const initialOnError = jest.fn();
+		const { result, rerender } = renderUseClipboard({
+			onError: initialOnError,
+		});
+		setSimulateFailure(true);
+
+		const secondOnError = jest.fn();
+		rerender({ onError: secondOnError });
+		await act(() => result.current.copyToClipboard("dummy-text"));
+
+		expect(initialOnError).not.toHaveBeenCalled();
+		expect(secondOnError).toHaveBeenCalledTimes(1);
+		expect(secondOnError).toHaveBeenCalledWith(
+			"Failed to copy text to clipboard",
+		);
+	});
 });
diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts
index 1eb91cc356..88a57a61fc 100644
--- a/site/src/hooks/useClipboard.ts
+++ b/site/src/hooks/useClipboard.ts
@@ -1,22 +1,18 @@
 import { displayError } from "components/GlobalSnackbar/utils";
-import { useEffect, useRef, useState } from "react";
+import { useCallback, useEffect, useRef, useState } from "react";
+import { useEffectEvent } from "./hookPolyfills";
 
 const CLIPBOARD_TIMEOUT_MS = 1_000;
 export const COPY_FAILED_MESSAGE = "Failed to copy text to clipboard";
 export const HTTP_FALLBACK_DATA_ID = "http-fallback";
 
 export type UseClipboardInput = Readonly<{
-	textToCopy: string;
-
-	/**
-	 * Optional callback to call when an error happens. If not specified, the hook
-	 * will dispatch an error message to the GlobalSnackbar
-	 */
 	onError?: (errorMessage: string) => void;
+	clearErrorOnSuccess?: boolean;
 }>;
 
 export type UseClipboardResult = Readonly<{
-	copyToClipboard: () => Promise<void>;
+	copyToClipboard: (textToCopy: string) => Promise<void>;
 	error: Error | undefined;
 
 	/**
@@ -40,47 +36,56 @@ export type UseClipboardResult = Readonly<{
 	showCopiedSuccess: boolean;
 }>;
 
-export const useClipboard = (input: UseClipboardInput): UseClipboardResult => {
-	const { textToCopy, onError: errorCallback } = input;
+export const useClipboard = (input?: UseClipboardInput): UseClipboardResult => {
+	const { onError = displayError, clearErrorOnSuccess = true } = input ?? {};
+
 	const [showCopiedSuccess, setShowCopiedSuccess] = useState(false);
 	const [error, setError] = useState<Error>();
 	const timeoutIdRef = useRef<number | undefined>(undefined);
 
 	useEffect(() => {
-		const clearIdOnUnmount = () => window.clearTimeout(timeoutIdRef.current);
-		return clearIdOnUnmount;
+		const clearTimeoutOnUnmount = () => {
+			window.clearTimeout(timeoutIdRef.current);
+		};
+		return clearTimeoutOnUnmount;
 	}, []);
 
-	const handleSuccessfulCopy = () => {
+	const stableOnError = useEffectEvent(() => onError(COPY_FAILED_MESSAGE));
+	const handleSuccessfulCopy = useEffectEvent(() => {
 		setShowCopiedSuccess(true);
+		if (clearErrorOnSuccess) {
+			setError(undefined);
+		}
+
 		timeoutIdRef.current = window.setTimeout(() => {
 			setShowCopiedSuccess(false);
 		}, CLIPBOARD_TIMEOUT_MS);
-	};
+	});
 
-	const copyToClipboard = async () => {
-		try {
-			await window.navigator.clipboard.writeText(textToCopy);
-			handleSuccessfulCopy();
-		} catch (err) {
-			const fallbackCopySuccessful = simulateClipboardWrite(textToCopy);
-			if (fallbackCopySuccessful) {
+	const copyToClipboard = useCallback(
+		async (textToCopy: string) => {
+			try {
+				await window.navigator.clipboard.writeText(textToCopy);
 				handleSuccessfulCopy();
-				return;
+			} catch (err) {
+				const fallbackCopySuccessful = simulateClipboardWrite(textToCopy);
+				if (fallbackCopySuccessful) {
+					handleSuccessfulCopy();
+					return;
+				}
+
+				const wrappedErr = new Error(COPY_FAILED_MESSAGE);
+				if (err instanceof Error) {
+					wrappedErr.stack = err.stack;
+				}
+
+				console.error(wrappedErr);
+				setError(wrappedErr);
+				stableOnError();
 			}
-
-			const wrappedErr = new Error(COPY_FAILED_MESSAGE);
-			if (err instanceof Error) {
-				wrappedErr.stack = err.stack;
-			}
-
-			console.error(wrappedErr);
-			setError(wrappedErr);
-
-			const notifyUser = errorCallback ?? displayError;
-			notifyUser(COPY_FAILED_MESSAGE);
-		}
-	};
+		},
+		[stableOnError, handleSuccessfulCopy],
+	);
 
 	return { showCopiedSuccess, error, copyToClipboard };
 };
diff --git a/site/src/pages/CliAuthPage/CliAuthPageView.tsx b/site/src/pages/CliAuthPage/CliAuthPageView.tsx
index e836127f61..10df8b225d 100644
--- a/site/src/pages/CliAuthPage/CliAuthPageView.tsx
+++ b/site/src/pages/CliAuthPage/CliAuthPageView.tsx
@@ -12,10 +12,7 @@ interface CliAuthPageViewProps {
 }
 
 export const CliAuthPageView: FC<CliAuthPageViewProps> = ({ sessionToken }) => {
-	const clipboard = useClipboard({
-		textToCopy: sessionToken ?? "",
-	});
-
+	const clipboardState = useClipboard();
 	return (
 		<SignInLayout>
 			<Welcome>Session token</Welcome>
@@ -30,16 +27,20 @@ export const CliAuthPageView: FC<CliAuthPageViewProps> = ({ sessionToken }) => {
 					className="w-full"
 					size="lg"
 					disabled={!sessionToken}
-					onClick={clipboard.copyToClipboard}
+					onClick={() => {
+						if (sessionToken) {
+							clipboardState.copyToClipboard(sessionToken);
+						}
+					}}
 				>
-					{clipboard.showCopiedSuccess ? (
+					{clipboardState.showCopiedSuccess ? (
 						<CheckIcon />
 					) : (
 						<Spinner loading={!sessionToken}>
 							<CopyIcon />
 						</Spinner>
 					)}
-					{clipboard.showCopiedSuccess
+					{clipboardState.showCopiedSuccess
 						? "Session token copied!"
 						: "Copy session token"}
 				</Button>
diff --git a/site/src/pages/TaskPage/TaskTopbar.tsx b/site/src/pages/TaskPage/TaskTopbar.tsx
index 1ad9ca5f0f..989e1b6473 100644
--- a/site/src/pages/TaskPage/TaskTopbar.tsx
+++ b/site/src/pages/TaskPage/TaskTopbar.tsx
@@ -81,14 +81,11 @@ export const TaskTopbar: FC<TaskTopbarProps> = ({ task }) => {
 type CopyPromptButtonProps = { prompt: string };
 
 const CopyPromptButton: FC<CopyPromptButtonProps> = ({ prompt }) => {
-	const { copyToClipboard, showCopiedSuccess } = useClipboard({
-		textToCopy: prompt,
-	});
-
+	const { copyToClipboard, showCopiedSuccess } = useClipboard();
 	return (
 		<Button
 			disabled={showCopiedSuccess}
-			onClick={copyToClipboard}
+			onClick={() => copyToClipboard(prompt)}
 			size="sm"
 			variant="subtle"
 			className="p-0 min-w-0"
diff --git a/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx b/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx
index ee92303b5d..8b50ef2447 100644
--- a/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx
+++ b/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx
@@ -74,13 +74,7 @@ export const TemplateEmbedPageView: FC<TemplateEmbedPageViewProps> = ({
 	templateParameters,
 }) => {
 	const [buttonValues, setButtonValues] = useState<ButtonValues | undefined>();
-	const clipboard = useClipboard({
-		textToCopy: getClipboardCopyContent(
-			template.name,
-			template.organization_name,
-			buttonValues,
-		),
-	});
+	const clipboard = useClipboard();
 
 	// template parameters is async so we need to initialize the values after it
 	// is loaded
@@ -237,8 +231,15 @@ export const TemplateEmbedPageView: FC<TemplateEmbedPageViewProps> = ({
 						>
 							<Button
 								className="rounded-full"
-								onClick={clipboard.copyToClipboard}
 								disabled={clipboard.showCopiedSuccess}
+								onClick={() => {
+									const textToCopy = getClipboardCopyContent(
+										template.name,
+										template.organization_name,
+										buttonValues,
+									);
+									clipboard.copyToClipboard(textToCopy);
+								}}
 							>
 								{clipboard.showCopiedSuccess ? <CheckIcon /> : <CopyIcon />}
 								Copy button code
diff --git a/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPageExperimental.tsx b/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPageExperimental.tsx
index a34d719f4c..c9b63f1422 100644
--- a/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPageExperimental.tsx
+++ b/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPageExperimental.tsx
@@ -289,14 +289,7 @@ interface ButtonPreviewProps {
 }
 
 const ButtonPreview: FC<ButtonPreviewProps> = ({ template, buttonValues }) => {
-	const clipboard = useClipboard({
-		textToCopy: getClipboardCopyContent(
-			template.name,
-			template.organization_name,
-			buttonValues,
-		),
-	});
-
+	const clipboard = useClipboard();
 	return (
 		<div
 			className="sticky top-10 flex gap-16 h-96 flex-1 flex-col items-center justify-center
@@ -305,8 +298,15 @@ const ButtonPreview: FC<ButtonPreviewProps> = ({ template, buttonValues }) => {
 			<img src="/open-in-coder.svg" alt="Open in Coder button" />
 			<Button
 				variant="default"
-				onClick={clipboard.copyToClipboard}
 				disabled={clipboard.showCopiedSuccess}
+				onClick={() => {
+					const textToCopy = getClipboardCopyContent(
+						template.name,
+						template.organization_name,
+						buttonValues,
+					);
+					clipboard.copyToClipboard(textToCopy);
+				}}
 			>
 				{clipboard.showCopiedSuccess ? <CheckOutlined /> : <FileCopyOutlined />}{" "}
 				Copy button code