From 087f973415ecb5b9d5b1b78c3e4f53768ca930a0 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 27 Feb 2024 21:05:37 -0500 Subject: [PATCH] refactor(site): clean up clipboard functionality and define tests (#12296) * refactor: clean up and update API for useClipboard * wip: commit current progress on useClipboard test * docs: clean up wording on showCopySuccess * chore: make sure tests can differentiate between HTTP/HTTPS * chore: add test ID to dummy input * wip: commit progress on useClipboard test * wip: commit more test progress * refactor: rewrite code for clarity * chore: finish clipboard tests * fix: prevent double-firing for button click aliases * refactor: clean up test setup * fix: rename incorrect test file * refactor: update code to display user errors * refactor: redesign useClipboard to be easier to test * refactor: clean up GlobalSnackbar * feat: add functionality for notifying user of errors (with tests) * refactor: clean up test code * refactor: centralize cleanup steps --- .../components/CodeExample/CodeExample.tsx | 6 +- site/src/components/CopyButton/CopyButton.tsx | 6 +- .../CopyableValue/CopyableValue.tsx | 6 +- .../GlobalSnackbar/GlobalSnackbar.tsx | 30 +-- ...ableRow.test.tsx => useClickable.test.tsx} | 0 site/src/hooks/useClipboard-http.test.ts | 15 ++ site/src/hooks/useClipboard-https.test.ts | 15 ++ site/src/hooks/useClipboard.test-setup.tsx | 218 ++++++++++++++++++ site/src/hooks/useClipboard.ts | 161 ++++++++++--- .../TemplateEmbedPage/TemplateEmbedPage.tsx | 33 ++- 10 files changed, 426 insertions(+), 64 deletions(-) rename site/src/hooks/{useClickableTableRow.test.tsx => useClickable.test.tsx} (100%) create mode 100644 site/src/hooks/useClipboard-http.test.ts create mode 100644 site/src/hooks/useClipboard-https.test.ts create mode 100644 site/src/hooks/useClipboard.test-setup.tsx diff --git a/site/src/components/CodeExample/CodeExample.tsx b/site/src/components/CodeExample/CodeExample.tsx index 14afaf11c3..7d6f2c661e 100644 --- a/site/src/components/CodeExample/CodeExample.tsx +++ b/site/src/components/CodeExample/CodeExample.tsx @@ -23,7 +23,11 @@ export const CodeExample: FC = ({ }) => { const buttonRef = useRef(null); const triggerButton = (event: KeyboardEvent | MouseEvent) => { - if (event.target !== buttonRef.current) { + const clickTriggeredOutsideButton = + event.target instanceof HTMLElement && + !buttonRef.current?.contains(event.target); + + if (clickTriggeredOutsideButton) { buttonRef.current?.click(); } }; diff --git a/site/src/components/CopyButton/CopyButton.tsx b/site/src/components/CopyButton/CopyButton.tsx index 64b0197447..8065fa1e64 100644 --- a/site/src/components/CopyButton/CopyButton.tsx +++ b/site/src/components/CopyButton/CopyButton.tsx @@ -32,7 +32,9 @@ export const CopyButton = forwardRef( buttonStyles, tooltipTitle = Language.tooltipTitle, } = props; - const { isCopied, copyToClipboard } = useClipboard(text); + const { showCopiedSuccess, copyToClipboard } = useClipboard({ + textToCopy: text, + }); return ( @@ -45,7 +47,7 @@ export const CopyButton = forwardRef( variant="text" onClick={copyToClipboard} > - {isCopied ? ( + {showCopiedSuccess ? ( ) : ( diff --git a/site/src/components/CopyableValue/CopyableValue.tsx b/site/src/components/CopyableValue/CopyableValue.tsx index d829682730..8c13732699 100644 --- a/site/src/components/CopyableValue/CopyableValue.tsx +++ b/site/src/components/CopyableValue/CopyableValue.tsx @@ -16,12 +16,14 @@ export const CopyableValue: FC = ({ children, ...attrs }) => { - const { isCopied, copyToClipboard } = useClipboard(value); + const { showCopiedSuccess, copyToClipboard } = useClipboard({ + textToCopy: value, + }); const clickableProps = useClickable(copyToClipboard); return ( diff --git a/site/src/components/GlobalSnackbar/GlobalSnackbar.tsx b/site/src/components/GlobalSnackbar/GlobalSnackbar.tsx index a39e9ad880..0d8be21206 100644 --- a/site/src/components/GlobalSnackbar/GlobalSnackbar.tsx +++ b/site/src/components/GlobalSnackbar/GlobalSnackbar.tsx @@ -24,37 +24,37 @@ const variantFromMsgType = (type: MsgType) => { }; export const GlobalSnackbar: FC = () => { - const [open, setOpen] = useState(false); - const [notification, setNotification] = useState(); - + const [notificationMsg, setNotificationMsg] = useState(); useCustomEvent(SnackbarEventType, (event) => { - setNotification(event.detail); - setOpen(true); + setNotificationMsg(event.detail); }); - if (!notification) { + const hasNotification = notificationMsg !== undefined; + if (!hasNotification) { return null; } return ( setOpen(false)} - autoHideDuration={notification.msgType === MsgType.Error ? 22000 : 6000} + key={notificationMsg.msg} + open={hasNotification} + variant={variantFromMsgType(notificationMsg.msgType)} + onClose={() => setNotificationMsg(undefined)} + autoHideDuration={ + notificationMsg.msgType === MsgType.Error ? 22000 : 6000 + } anchorOrigin={{ vertical: "bottom", horizontal: "right" }} message={
- {notification.msgType === MsgType.Error && ( + {notificationMsg.msgType === MsgType.Error && ( )}
- {notification.msg} + {notificationMsg.msg} - {notification.additionalMsgs && - notification.additionalMsgs.map((msg, index) => ( + {notificationMsg.additionalMsgs && + notificationMsg.additionalMsgs.map((msg, index) => ( ))}
diff --git a/site/src/hooks/useClickableTableRow.test.tsx b/site/src/hooks/useClickable.test.tsx similarity index 100% rename from site/src/hooks/useClickableTableRow.test.tsx rename to site/src/hooks/useClickable.test.tsx diff --git a/site/src/hooks/useClipboard-http.test.ts b/site/src/hooks/useClipboard-http.test.ts new file mode 100644 index 0000000000..ca22fc6daa --- /dev/null +++ b/site/src/hooks/useClipboard-http.test.ts @@ -0,0 +1,15 @@ +/** + * This test is for all useClipboard functionality, with the browser context + * set to insecure (HTTP connections). + * + * See useClipboard.test-setup.ts for more info on why this file is set up the + * way that it is. + */ +import { useClipboard } from "./useClipboard"; +import { scheduleClipboardTests } from "./useClipboard.test-setup"; + +describe(useClipboard.name, () => { + describe("HTTP (non-secure) connections", () => { + scheduleClipboardTests({ isHttps: false }); + }); +}); diff --git a/site/src/hooks/useClipboard-https.test.ts b/site/src/hooks/useClipboard-https.test.ts new file mode 100644 index 0000000000..e2e3ac264c --- /dev/null +++ b/site/src/hooks/useClipboard-https.test.ts @@ -0,0 +1,15 @@ +/** + * This test is for all useClipboard functionality, with the browser context + * set to secure (HTTPS connections). + * + * See useClipboard.test-setup.ts for more info on why this file is set up the + * way that it is. + */ +import { useClipboard } from "./useClipboard"; +import { scheduleClipboardTests } from "./useClipboard.test-setup"; + +describe(useClipboard.name, () => { + describe("HTTPS (secure/default) connections", () => { + scheduleClipboardTests({ isHttps: true }); + }); +}); diff --git a/site/src/hooks/useClipboard.test-setup.tsx b/site/src/hooks/useClipboard.test-setup.tsx new file mode 100644 index 0000000000..32837598b1 --- /dev/null +++ b/site/src/hooks/useClipboard.test-setup.tsx @@ -0,0 +1,218 @@ +/** + * @file This is a very weird test setup. + * + * There are two main things that it's fighting against to insure that the + * clipboard functionality is working as expected: + * 1. userEvent.setup's default global behavior + * 2. The fact that we need to reuse the same set of test cases for two separate + * contexts (secure and insecure), each with their own version of global + * state. + * + * The goal of this file is to provide a shared set of test behavior that can + * be imported into two separate test files (one for HTTP, one for HTTPS), + * without any risk of global state conflicts. + * + * --- + * For (1), normally you could call userEvent.setup to enable clipboard mocking, + * but userEvent doesn't expose a teardown function. It also modifies the global + * scope for the whole test file, so enabling just one userEvent session will + * make a mock clipboard exist for all other tests, even though you didn't tell + * them to set up a session. The mock also assumes that the clipboard API will + * always be available, which is not true on HTTP-only connections + * + * Since these tests need to split hairs and differentiate between HTTP and + * HTTPS connections, setting up a single userEvent is disastrous. It will make + * all the tests pass, even if they shouldn't. Have to avoid that by creating a + * custom clipboard mock. + * + * --- + * For (2), we're fighting against Jest's default behavior, which is to treat + * the test file as the main boundary for test environments, with each test case + * able to run in parallel. That works if you have one single global state, but + * we need two separate versions of the global state, while repeating the exact + * same test cases for each one. + * + * If both tests were to be placed in the same file, Jest would not isolate them + * and would let their setup steps interfere with each other. This leads to one + * of two things: + * 1. One of the global mocks overrides the other, making it so that one + * connection type always fails + * 2. The two just happen not to conflict each other, through some convoluted + * order of operations involving closure, but you have no idea why the code + * is working, and it's impossible to debug. + */ +import { + type UseClipboardInput, + type UseClipboardResult, + useClipboard, +} from "./useClipboard"; +import { act, renderHook } from "@testing-library/react"; +import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar"; + +const initialExecCommand = global.document.execCommand; +beforeAll(() => { + jest.useFakeTimers(); +}); + +afterAll(() => { + jest.restoreAllMocks(); + jest.useRealTimers(); + global.document.execCommand = initialExecCommand; +}); + +type MockClipboardEscapeHatches = Readonly<{ + getMockText: () => string; + setMockText: (newText: string) => void; + simulateFailure: boolean; + setSimulateFailure: (failureMode: boolean) => void; +}>; + +type MockClipboard = Readonly; +function makeMockClipboard(isSecureContext: boolean): MockClipboard { + let mockClipboardValue = ""; + let shouldFail = false; + + return { + get simulateFailure() { + return shouldFail; + }, + setSimulateFailure: (value) => { + shouldFail = value; + }, + + readText: async () => { + if (shouldFail) { + throw new Error("Clipboard deliberately failed"); + } + + if (!isSecureContext) { + throw new Error( + "Trying to read from clipboard outside secure context!", + ); + } + + return mockClipboardValue; + }, + writeText: async (newText) => { + if (shouldFail) { + throw new Error("Clipboard deliberately failed"); + } + + if (!isSecureContext) { + throw new Error("Trying to write to clipboard outside secure context!"); + } + + mockClipboardValue = newText; + }, + + getMockText: () => mockClipboardValue, + setMockText: (newText) => { + mockClipboardValue = newText; + }, + + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + dispatchEvent: jest.fn(), + read: jest.fn(), + write: jest.fn(), + }; +} + +function renderUseClipboard(inputs: UseClipboardInput) { + return renderHook( + (props) => useClipboard(props), + { + initialProps: inputs, + wrapper: ({ children }) => ( + <> + <>{children} + + + ), + }, + ); +} + +type ScheduleConfig = Readonly<{ isHttps: boolean }>; + +export function scheduleClipboardTests({ isHttps }: ScheduleConfig) { + const mockClipboardInstance = makeMockClipboard(isHttps); + + const originalNavigator = window.navigator; + beforeAll(() => { + jest.spyOn(window, "navigator", "get").mockImplementation(() => ({ + ...originalNavigator, + clipboard: mockClipboardInstance, + })); + + if (!isHttps) { + // Not the biggest fan of exposing implementation details like this, but + // making any kind of mock for execCommand is really gnarly in general + global.document.execCommand = jest.fn(() => { + if (mockClipboardInstance.simulateFailure) { + return false; + } + + const dummyInput = document.querySelector("input[data-testid=dummy]"); + const inputIsFocused = + dummyInput instanceof HTMLInputElement && + document.activeElement === dummyInput; + + let copySuccessful = false; + if (inputIsFocused) { + mockClipboardInstance.setMockText(dummyInput.value); + copySuccessful = true; + } + + return copySuccessful; + }); + } + }); + + afterEach(() => { + mockClipboardInstance.setMockText(""); + mockClipboardInstance.setSimulateFailure(false); + }); + + const assertClipboardTextUpdate = async ( + result: ReturnType["result"], + textToCheck: string, + ): Promise => { + await act(() => result.current.copyToClipboard()); + expect(result.current.showCopiedSuccess).toBe(true); + + const clipboardText = mockClipboardInstance.getMockText(); + expect(clipboardText).toEqual(textToCheck); + }; + + /** + * Start of test cases + */ + it("Copies the current text to the user's clipboard", async () => { + const textToCopy = "dogs"; + const { result } = renderUseClipboard({ textToCopy }); + await assertClipboardTextUpdate(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 }); + await assertClipboardTextUpdate(result, textToCopy); + + setTimeout(() => { + expect(result.current.showCopiedSuccess).toBe(false); + }, 10_000); + + await jest.runAllTimersAsync(); + }); + + 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 }); + + mockClipboardInstance.setSimulateFailure(true); + await act(() => result.current.copyToClipboard()); + expect(onError).toBeCalled(); + }); +} diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index 6d85963da3..83ec8283ed 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -1,51 +1,146 @@ import { useEffect, useRef, useState } from "react"; +import { displayError } from "components/GlobalSnackbar/utils"; -type UseClipboardResult = Readonly<{ - isCopied: boolean; - copyToClipboard: () => Promise; +const CLIPBOARD_TIMEOUT_MS = 1_000; +const COPY_FAILED_MESSAGE = "Failed to copy text to clipboard"; + +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; }>; -export const useClipboard = (textToCopy: string): UseClipboardResult => { - const [isCopied, setIsCopied] = useState(false); +export type UseClipboardResult = Readonly<{ + copyToClipboard: () => Promise; + error: Error | undefined; + + /** + * Indicates whether the UI should show a successfully-copied status to the + * user. When flipped to true, this will eventually flip to false, with no + * action from the user. + * + * --- + * + * This is _not_ the same as an `isCopied` property, because the hook never + * actually checks the clipboard to determine any state, so it is possible for + * there to be misleading state combos like: + * - User accidentally copies new text before showCopiedSuccess naturally + * flips to false + * + * Trying to make this property accurate enough that it could safely be called + * `isCopied` led to browser compatibility issues in Safari. + * + * @see {@link https://github.com/coder/coder/pull/11863} + */ + showCopiedSuccess: boolean; +}>; + +export const useClipboard = (input: UseClipboardInput): UseClipboardResult => { + const { textToCopy, onError: errorCallback } = input; + const [showCopiedSuccess, setShowCopiedSuccess] = useState(false); + const [error, setError] = useState(); const timeoutIdRef = useRef(); useEffect(() => { - const clearIdsOnUnmount = () => window.clearTimeout(timeoutIdRef.current); - return clearIdsOnUnmount; + const clearIdOnUnmount = () => window.clearTimeout(timeoutIdRef.current); + return clearIdOnUnmount; }, []); + const handleSuccessfulCopy = () => { + setShowCopiedSuccess(true); + timeoutIdRef.current = window.setTimeout(() => { + setShowCopiedSuccess(false); + }, CLIPBOARD_TIMEOUT_MS); + }; + const copyToClipboard = async () => { try { await window.navigator.clipboard.writeText(textToCopy); - setIsCopied(true); - timeoutIdRef.current = window.setTimeout(() => { - setIsCopied(false); - }, 1000); + handleSuccessfulCopy(); } catch (err) { - const input = document.createElement("input"); - input.value = textToCopy; - document.body.appendChild(input); - input.focus(); - input.select(); - const isCopied = document.execCommand("copy"); - document.body.removeChild(input); - - if (isCopied) { - setIsCopied(true); - timeoutIdRef.current = window.setTimeout(() => { - setIsCopied(false); - }, 1000); - } else { - const wrappedErr = new Error( - "copyToClipboard: failed to copy text to clipboard", - ); - if (err instanceof Error) { - wrappedErr.stack = err.stack; - } - console.error(wrappedErr); + 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); + + const notifyUser = errorCallback ?? displayError; + notifyUser(COPY_FAILED_MESSAGE); } }; - return { isCopied, copyToClipboard }; + return { showCopiedSuccess, error, copyToClipboard }; }; + +/** + * Provides a fallback clipboard method for when browsers do not have access + * to the clipboard API (the browser is older, or the deployment is only running + * on HTTP, when the clipboard API is only available in secure contexts). + * + * It feels silly that you have to make a whole dummy input just to simulate a + * clipboard, but that's really the recommended approach for older browsers. + * + * @see {@link https://web.dev/patterns/clipboard/copy-text?hl=en} + */ +function simulateClipboardWrite(textToCopy: string): boolean { + const previousFocusTarget = document.activeElement; + const dummyInput = document.createElement("input"); + + // Have to add test ID to dummy element for mocking purposes in tests + dummyInput.setAttribute("data-testid", "dummy"); + + // Using visually-hidden styling to ensure that inserting the element doesn't + // cause any content reflows on the page (removes any risk of UI flickers). + // Can't use visibility:hidden or display:none, because then the elements + // can't receive focus, which is needed for the execCommand method to work + const style = dummyInput.style; + style.display = "inline-block"; + style.position = "absolute"; + style.overflow = "hidden"; + style.clip = "rect(0 0 0 0)"; + style.clipPath = "rect(0 0 0 0)"; + style.height = "1px"; + style.width = "1px"; + style.margin = "-1px"; + style.padding = "0"; + style.border = "0"; + + document.body.appendChild(dummyInput); + dummyInput.value = textToCopy; + dummyInput.focus(); + dummyInput.select(); + + /** + * The document.execCommand method is officially deprecated. Browsers are free + * to remove the method entirely or choose to turn it into a no-op function + * that always returns false. You cannot make any assumptions about how its + * core functionality will be removed. + * + * @see {@link https://developer.mozilla.org/en-US/docs/Web/API/Clipboard} + */ + let copySuccessful: boolean; + try { + copySuccessful = document?.execCommand("copy") ?? false; + } catch { + copySuccessful = false; + } + + dummyInput.remove(); + if (previousFocusTarget instanceof HTMLElement) { + previousFocusTarget.focus(); + } + + return copySuccessful; +} diff --git a/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx b/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx index 2bbbaa0e3e..6cf91723ad 100644 --- a/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx +++ b/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx @@ -47,19 +47,26 @@ interface TemplateEmbedPageViewProps { templateParameters?: TemplateVersionParameter[]; } +function getClipboardCopyContent( + templateName: string, + buttonValues: ButtonValues | undefined, +): string { + const deploymentUrl = `${window.location.protocol}//${window.location.host}`; + const createWorkspaceUrl = `${deploymentUrl}/templates/${templateName}/workspace`; + const createWorkspaceParams = new URLSearchParams(buttonValues); + const buttonUrl = `${createWorkspaceUrl}?${createWorkspaceParams.toString()}`; + + return `[![Open in Coder](${deploymentUrl}/open-in-coder.svg)](${buttonUrl})`; +} + export const TemplateEmbedPageView: FC = ({ template, templateParameters, }) => { - const [buttonValues, setButtonValues] = useState( - undefined, - ); - const deploymentUrl = `${window.location.protocol}//${window.location.host}`; - const createWorkspaceUrl = `${deploymentUrl}/templates/${template.name}/workspace`; - const createWorkspaceParams = new URLSearchParams(buttonValues); - const buttonUrl = `${createWorkspaceUrl}?${createWorkspaceParams.toString()}`; - const buttonMkdCode = `[![Open in Coder](${deploymentUrl}/open-in-coder.svg)](${buttonUrl})`; - const clipboard = useClipboard(buttonMkdCode); + const [buttonValues, setButtonValues] = useState(); + const clipboard = useClipboard({ + textToCopy: getClipboardCopyContent(template.name, buttonValues), + }); // template parameters is async so we need to initialize the values after it // is loaded @@ -173,11 +180,15 @@ export const TemplateEmbedPageView: FC = ({