From 7e4d964e4572d00dd17cd0e5e65903961ec7ffee Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 26 Jan 2024 19:15:19 +0000 Subject: [PATCH 01/31] wip: commit progress for clipboard update --- site/src/hooks/useClipboard.ts | 206 ++++++++++++++++++++++++++------- 1 file changed, 167 insertions(+), 39 deletions(-) diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index 0acad67a26580..c9e27d0254807 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -1,44 +1,172 @@ -import { useState } from "react"; - -export const useClipboard = ( - text: string, -): { isCopied: boolean; copy: () => Promise } => { - const [isCopied, setIsCopied] = useState(false); - - const copy = async (): Promise => { - try { - await window.navigator.clipboard.writeText(text); - setIsCopied(true); - window.setTimeout(() => { - setIsCopied(false); - }, 1000); - } catch (err) { - const input = document.createElement("input"); - input.value = text; - document.body.appendChild(input); - input.focus(); - input.select(); - const result = document.execCommand("copy"); - document.body.removeChild(input); - if (result) { - setIsCopied(true); - 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); - } +import { useCallback, useEffect, useState } from "react"; + +type UseClipboardResult = Readonly<{ + isCopied: boolean; + copyToClipboard: () => Promise; +}>; + +export const useClipboard = (textToCopy: string): UseClipboardResult => { + // Can't initialize clipboardText with a more specific value because reading + // is an async operation + const [clipboardText, setClipboardText] = useState(""); + + useEffect(() => { + console.log(clipboardText); + }, [clipboardText]); + + useEffect(() => { + // Copy events have a ClipboardEvent associated with them, but sadly, the + // event only gives you information about what caused the event, not the new + // data that's just been copied. Have to use same handler for all operations + const copyClipboardToState = async () => { + const result = await readFromClipboard(); + setClipboardText((current) => (result.success ? result.value : current)); + }; + + // Focus event handles case where user navigates to a different tab, copies + // new text, and then comes back to Coder + window.addEventListener("focus", copyClipboardToState); + window.addEventListener("copy", copyClipboardToState); + void copyClipboardToState(); + + return () => { + window.removeEventListener("focus", copyClipboardToState); + window.removeEventListener("copy", copyClipboardToState); + }; + }, []); + + const copyToClipboard = useCallback(async () => { + const result = await writeToClipboard(textToCopy); + if (!result.success) { + console.error(result.error); } - }; + }, [textToCopy]); return { - isCopied, - copy, + copyToClipboard, + isCopied: textToCopy === clipboardText, }; }; + +type Result = Readonly< + | { + success: false; + value: null; + error: Error; + } + | (void extends T + ? { success: true; error: null } + : { success: true; value: T; error: null }) +>; + +export async function readFromClipboard(): Promise> { + if (!document.hasFocus()) { + return { + success: false, + value: null, + error: new Error( + "Security issue - clipboard read queued while tab was not active", + ), + }; + } + + try { + // navigator.clipboard is a newer API. It should be defined in most browsers + // nowadays, but there's a fallback if not + if (typeof window?.navigator?.clipboard?.readText === "function") { + return { + success: true, + value: await window.navigator.clipboard.readText(), + error: null, + }; + } + + // Yes, this is the "proper" way to do things with the old API - very hokey + const previousFocusTarget = document.activeElement; + const dummyInput = document.createElement("input"); + dummyInput.setAttribute("hidden", "true"); + document.body.appendChild(dummyInput); + dummyInput.focus(); + + const success = document.execCommand("paste"); + if (!success) { + throw new Error("Failed to simulate clipboard with "); + } + + const newText = dummyInput.value; + document.body.removeChild(dummyInput); + + if (previousFocusTarget instanceof HTMLElement) { + previousFocusTarget.focus(); + } + + return { + success: true, + value: newText, + error: null, + }; + } catch (err) { + // Only expected error at this point is the user not granting the webpage + // permission to access the clipboard + const flattenedError = + err instanceof Error + ? err + : new Error("Unknown error thrown while reading"); + + return { + success: false, + value: null, + error: flattenedError, + }; + } +} + +export async function writeToClipboard( + textToCopy: string, +): Promise> { + return { + success: true, + error: null, + }; + + // // Expected throw case: user's browser is old enough that it doesn't have + // // the navigator API + // let wrappedError: Error | null = null; + // try { + // await window.navigator.clipboard.writeText(textToCopy); + // return { success: true, error: null }; + // } catch (err) { + // wrappedError = err as Error; + // } + + // let copySuccessful = false; + // if (!copySuccessful) { + // const wrappedErr = new Error( + // "copyToClipboard: failed to copy text to clipboard", + // ); + + // if (err instanceof Error) { + // wrappedErr.stack = err.stack; + // } + + // console.error(wrappedErr); + // } + + // const previousFocusTarget = document.activeElement; + // const dummyInput = document.createElement("input"); + // dummyInput.value = textToCopy; + + // document.body.appendChild(dummyInput); + // dummyInput.focus(); + // dummyInput.select(); + + // if (typeof document.execCommand !== "undefined") { + // copySuccessful = document.execCommand("copy"); + // } + + // if (previousFocusTarget instanceof HTMLElement) { + // previousFocusTarget.focus(); + // } + + // return copySuccessful; +} From ece3f6d4d7b7676de14ba1b2eec1806c97691a2e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 26 Jan 2024 19:56:56 +0000 Subject: [PATCH 02/31] wip: push more progress --- site/src/hooks/useClipboard.ts | 75 +++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index c9e27d0254807..5b2f0e26ea489 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -48,17 +48,17 @@ export const useClipboard = (textToCopy: string): UseClipboardResult => { }; }; -type Result = Readonly< - | { - success: false; - value: null; - error: Error; - } - | (void extends T - ? { success: true; error: null } - : { success: true; value: T; error: null }) +type VoidResult = Readonly< + { success: true; error: null } | { success: false; error: Error } +>; + +type ResultWithData = Readonly< + | { success: true; value: T; error: null } + | { success: false; value: null; error: Error } >; +type Result = void extends T ? VoidResult : ResultWithData; + export async function readFromClipboard(): Promise> { if (!document.hasFocus()) { return { @@ -81,33 +81,21 @@ export async function readFromClipboard(): Promise> { }; } - // Yes, this is the "proper" way to do things with the old API - very hokey - const previousFocusTarget = document.activeElement; - const dummyInput = document.createElement("input"); - dummyInput.setAttribute("hidden", "true"); - document.body.appendChild(dummyInput); - dummyInput.focus(); - - const success = document.execCommand("paste"); - if (!success) { - throw new Error("Failed to simulate clipboard with "); - } - - const newText = dummyInput.value; - document.body.removeChild(dummyInput); - - if (previousFocusTarget instanceof HTMLElement) { - previousFocusTarget.focus(); + const { isExecSupported, value } = simulateClipboard("read"); + if (!isExecSupported) { + throw new Error( + "document.execCommand has been removed for the user's browser, but they do not have access to newer API", + ); } return { success: true, - value: newText, + value: value, error: null, }; } catch (err) { - // Only expected error at this point is the user not granting the webpage - // permission to access the clipboard + // Only expected error not covered by function logic is the user not + // granting the webpage permission to access the clipboard const flattenedError = err instanceof Error ? err @@ -170,3 +158,32 @@ export async function writeToClipboard( // return copySuccessful; } + +type SimulateClipboardResult = Readonly<{ + isExecSupported: boolean; + value: string; +}>; + +function simulateClipboard( + operation: "read" | "write", +): SimulateClipboardResult { + // Absolutely cartoonish logic, but it's how you do things with the exec API + const previousFocusTarget = document.activeElement; + const dummyInput = document.createElement("input"); + dummyInput.style.visibility = "hidden"; + document.body.appendChild(dummyInput); + dummyInput.focus(); + + // Confusingly, you want to call the method opposite of what you want to do to + // interact with the exec method + const command = operation === "read" ? "paste" : "copy"; + const isExecSupported = document.execCommand(command); + const value = dummyInput.value; + dummyInput.remove(); + + if (previousFocusTarget instanceof HTMLElement) { + previousFocusTarget.focus(); + } + + return { isExecSupported, value }; +} From bf6e73ad3bce80bef5754795bffa5630b508778b Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 26 Jan 2024 20:22:39 +0000 Subject: [PATCH 03/31] chore: finish initial version of useClipboard revamp --- site/src/hooks/useClipboard.ts | 127 +++++++++++++++------------------ 1 file changed, 58 insertions(+), 69 deletions(-) diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index 5b2f0e26ea489..80733aa356167 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -1,4 +1,5 @@ import { useCallback, useEffect, useState } from "react"; +import { useEffectEvent } from "./hookPolyfills"; type UseClipboardResult = Readonly<{ isCopied: boolean; @@ -10,37 +11,34 @@ export const useClipboard = (textToCopy: string): UseClipboardResult => { // is an async operation const [clipboardText, setClipboardText] = useState(""); - useEffect(() => { - console.log(clipboardText); - }, [clipboardText]); + // Copy events have a ClipboardEvent associated with them, but sadly, the + // event only gives you information about what caused the event, not the new + // data that's just been copied. Have to use same handler for all operations + const syncClipboardToState = useEffectEvent(async () => { + const result = await readFromClipboard(); + setClipboardText((current) => (result.success ? result.value : current)); + }); useEffect(() => { - // Copy events have a ClipboardEvent associated with them, but sadly, the - // event only gives you information about what caused the event, not the new - // data that's just been copied. Have to use same handler for all operations - const copyClipboardToState = async () => { - const result = await readFromClipboard(); - setClipboardText((current) => (result.success ? result.value : current)); - }; - // Focus event handles case where user navigates to a different tab, copies // new text, and then comes back to Coder - window.addEventListener("focus", copyClipboardToState); - window.addEventListener("copy", copyClipboardToState); - void copyClipboardToState(); + window.addEventListener("focus", syncClipboardToState); + window.addEventListener("copy", syncClipboardToState); + void syncClipboardToState(); return () => { - window.removeEventListener("focus", copyClipboardToState); - window.removeEventListener("copy", copyClipboardToState); + window.removeEventListener("focus", syncClipboardToState); + window.removeEventListener("copy", syncClipboardToState); }; - }, []); + }, [syncClipboardToState]); const copyToClipboard = useCallback(async () => { const result = await writeToClipboard(textToCopy); - if (!result.success) { - console.error(result.error); + if (result.success) { + void syncClipboardToState(); + return; } - }, [textToCopy]); + }, [syncClipboardToState, textToCopy]); return { copyToClipboard, @@ -59,13 +57,16 @@ type ResultWithData = Readonly< type Result = void extends T ? VoidResult : ResultWithData; -export async function readFromClipboard(): Promise> { +async function readFromClipboard(): Promise> { + // This is mainly here for the sake of being exhaustive, but the main thing it + // helps with is suppressing error messages when Vite does HMR refreshes in + // dev mode if (!document.hasFocus()) { return { success: false, value: null, error: new Error( - "Security issue - clipboard read queued while tab was not active", + "Security error - clipboard read queued while tab was not active", ), }; } @@ -109,54 +110,42 @@ export async function readFromClipboard(): Promise> { } } -export async function writeToClipboard( - textToCopy: string, -): Promise> { - return { - success: true, - error: null, - }; +// Comments for this function mirror the ones for readFromClipboard +async function writeToClipboard(textToCopy: string): Promise> { + if (!document.hasFocus()) { + return { + success: false, + error: new Error( + "Security error - clipboard read queued while tab was not active", + ), + }; + } + + try { + if (typeof window?.navigator?.clipboard?.writeText === "function") { + await window.navigator.clipboard.writeText(textToCopy); + return { success: true, error: null }; + } + + const { isExecSupported } = simulateClipboard("write"); + if (!isExecSupported) { + throw new Error( + "document.execCommand has been removed for the user's browser, but they do not have access to newer API", + ); + } + + return { success: true, error: null }; + } catch (err) { + const flattenedError = + err instanceof Error + ? err + : new Error("Unknown error thrown while reading"); - // // Expected throw case: user's browser is old enough that it doesn't have - // // the navigator API - // let wrappedError: Error | null = null; - // try { - // await window.navigator.clipboard.writeText(textToCopy); - // return { success: true, error: null }; - // } catch (err) { - // wrappedError = err as Error; - // } - - // let copySuccessful = false; - // if (!copySuccessful) { - // const wrappedErr = new Error( - // "copyToClipboard: failed to copy text to clipboard", - // ); - - // if (err instanceof Error) { - // wrappedErr.stack = err.stack; - // } - - // console.error(wrappedErr); - // } - - // const previousFocusTarget = document.activeElement; - // const dummyInput = document.createElement("input"); - // dummyInput.value = textToCopy; - - // document.body.appendChild(dummyInput); - // dummyInput.focus(); - // dummyInput.select(); - - // if (typeof document.execCommand !== "undefined") { - // copySuccessful = document.execCommand("copy"); - // } - - // if (previousFocusTarget instanceof HTMLElement) { - // previousFocusTarget.focus(); - // } - - // return copySuccessful; + return { + success: false, + error: flattenedError, + }; + } } type SimulateClipboardResult = Readonly<{ From 697bdf0ccf40fb5a19d19544fcff9c87a22396fb Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 26 Jan 2024 20:23:15 +0000 Subject: [PATCH 04/31] refactor: update API query to use newer RQ patterns --- site/src/api/queries/users.ts | 8 ++++++++ site/src/pages/CliAuthPage/CliAuthPage.tsx | 6 ++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 0249315071b13..9299831985b40 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -13,6 +13,7 @@ import type { UpdateUserAppearanceSettingsRequest, UsersRequest, User, + GenerateAPIKeyResponse, } from "api/typesGenerated"; import { getAuthorizationKey } from "./authCheck"; import { getMetadataAsJSON } from "utils/metadata"; @@ -134,6 +135,13 @@ export const me = (): UseQueryOptions & { }; }; +export function apiKey(): UseQueryOptions { + return { + queryKey: [...meKey, "apiKey"], + queryFn: () => API.getApiKey(), + }; +} + export const hasFirstUser = () => { return { queryKey: ["hasFirstUser"], diff --git a/site/src/pages/CliAuthPage/CliAuthPage.tsx b/site/src/pages/CliAuthPage/CliAuthPage.tsx index 902651d5cdc0c..381ff88507cd8 100644 --- a/site/src/pages/CliAuthPage/CliAuthPage.tsx +++ b/site/src/pages/CliAuthPage/CliAuthPage.tsx @@ -1,14 +1,12 @@ import { type FC } from "react"; import { Helmet } from "react-helmet-async"; import { useQuery } from "react-query"; -import { getApiKey } from "api/api"; import { pageTitle } from "utils/page"; import { CliAuthPageView } from "./CliAuthPageView"; +import { apiKey } from "api/queries/users"; export const CliAuthenticationPage: FC = () => { - const { data } = useQuery({ - queryFn: () => getApiKey(), - }); + const { data } = useQuery(apiKey()); return ( <> From 9b881c019e1b8904a1c8de49553223215218ce51 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 26 Jan 2024 20:23:50 +0000 Subject: [PATCH 05/31] fix: update importers of useClipboard --- site/src/components/CopyButton/CopyButton.tsx | 2 +- site/src/components/CopyableValue/CopyableValue.tsx | 4 ++-- .../TemplatePage/TemplateEmbedPage/TemplateEmbedPage.tsx | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/site/src/components/CopyButton/CopyButton.tsx b/site/src/components/CopyButton/CopyButton.tsx index b28823948facb..0f1ccace88cc9 100644 --- a/site/src/components/CopyButton/CopyButton.tsx +++ b/site/src/components/CopyButton/CopyButton.tsx @@ -30,7 +30,7 @@ export const CopyButton: FC = ({ buttonStyles, tooltipTitle = Language.tooltipTitle, }) => { - const { isCopied, copy: copyToClipboard } = useClipboard(text); + const { isCopied, copyToClipboard } = useClipboard(text); return ( diff --git a/site/src/components/CopyableValue/CopyableValue.tsx b/site/src/components/CopyableValue/CopyableValue.tsx index c2d14e322256d..d8296827305b1 100644 --- a/site/src/components/CopyableValue/CopyableValue.tsx +++ b/site/src/components/CopyableValue/CopyableValue.tsx @@ -16,8 +16,8 @@ export const CopyableValue: FC = ({ children, ...attrs }) => { - const { isCopied, copy } = useClipboard(value); - const clickableProps = useClickable(copy); + const { isCopied, copyToClipboard } = useClipboard(value); + const clickableProps = useClickable(copyToClipboard); return ( = ({ clipboard.isCopied ? : } variant="contained" - onClick={clipboard.copy} + onClick={clipboard.copyToClipboard} disabled={clipboard.isCopied} > Copy button code From ccdca08811847a33d1fbb3510a0438fdf21e1b8f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 26 Jan 2024 21:26:38 +0000 Subject: [PATCH 06/31] fix: increase clickable area of CodeExample --- .../components/CodeExample/CodeExample.tsx | 36 +++++++++- site/src/components/CopyButton/CopyButton.tsx | 69 +++++++++++-------- 2 files changed, 72 insertions(+), 33 deletions(-) diff --git a/site/src/components/CodeExample/CodeExample.tsx b/site/src/components/CodeExample/CodeExample.tsx index 9014f04a58ce5..85240aa82db4a 100644 --- a/site/src/components/CodeExample/CodeExample.tsx +++ b/site/src/components/CodeExample/CodeExample.tsx @@ -1,4 +1,4 @@ -import { type FC } from "react"; +import { useRef, type FC } from "react"; import { type Interpolation, type Theme } from "@emotion/react"; import { MONOSPACE_FONT_FAMILY } from "theme/constants"; import { CopyButton } from "../CopyButton/CopyButton"; @@ -17,16 +17,42 @@ export const CodeExample: FC = ({ secret, className, }) => { + const buttonRef = useRef(null); + const triggerButton = () => buttonRef.current?.click(); + return ( -
+ /* eslint-disable-next-line jsx-a11y/no-static-element-interactions -- + Expanding clickable area of CodeExample for better ergonomics, but don't + want to change the semantics of the HTML elements being rendered + */ +
{ + if (event.target !== buttonRef.current) { + triggerButton(); + } + }} + onKeyDown={(event) => { + if (event.key === "Enter") { + triggerButton(); + } + }} + onKeyUp={(event) => { + if (event.key === " ") { + triggerButton(); + } + }} + > {code} - +
); }; const styles = { container: (theme) => ({ + cursor: "pointer", display: "flex", flexDirection: "row", alignItems: "center", @@ -37,6 +63,10 @@ const styles = { padding: 8, lineHeight: "150%", border: `1px solid ${theme.experimental.l1.outline}`, + + "&:hover": { + backgroundColor: theme.experimental.l2.hover.background, + }, }), code: { diff --git a/site/src/components/CopyButton/CopyButton.tsx b/site/src/components/CopyButton/CopyButton.tsx index 0f1ccace88cc9..886fa8a085059 100644 --- a/site/src/components/CopyButton/CopyButton.tsx +++ b/site/src/components/CopyButton/CopyButton.tsx @@ -2,7 +2,7 @@ import IconButton from "@mui/material/Button"; import Tooltip from "@mui/material/Tooltip"; import Check from "@mui/icons-material/Check"; import { css, type Interpolation, type Theme } from "@emotion/react"; -import { type FC, type ReactNode } from "react"; +import { forwardRef, MouseEventHandler, type ReactNode } from "react"; import { useClipboard } from "hooks/useClipboard"; import { FileCopyIcon } from "../Icons/FileCopyIcon"; @@ -13,6 +13,7 @@ interface CopyButtonProps { wrapperStyles?: Interpolation; buttonStyles?: Interpolation; tooltipTitle?: string; + onClick?: MouseEventHandler; } export const Language = { @@ -23,36 +24,44 @@ export const Language = { /** * Copy button used inside the CodeBlock component internally */ -export const CopyButton: FC = ({ - text, - ctaCopy, - wrapperStyles, - buttonStyles, - tooltipTitle = Language.tooltipTitle, -}) => { - const { isCopied, copyToClipboard } = useClipboard(text); +export const CopyButton = forwardRef( + (props, ref) => { + const { + text, + ctaCopy, + wrapperStyles, + buttonStyles, + onClick: outsideOnClick, + tooltipTitle = Language.tooltipTitle, + } = props; + const { isCopied, copyToClipboard } = useClipboard(text); - return ( - -
- - {isCopied ? ( - - ) : ( - - )} - {ctaCopy &&
{ctaCopy}
} - -
- - ); -}; + return ( + +
+ { + void copyToClipboard(); + outsideOnClick?.(event); + }} + > + {isCopied ? ( + + ) : ( + + )} + {ctaCopy &&
{ctaCopy}
} + +
+ + ); + }, +); const styles = { button: (theme) => css` From a7c7cc7e158e9754246f25fcf0487b683d3ee501 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 26 Jan 2024 21:30:24 +0000 Subject: [PATCH 07/31] fix: update styles for CliAuthPageView --- site/src/pages/CliAuthPage/CliAuthPageView.tsx | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/site/src/pages/CliAuthPage/CliAuthPageView.tsx b/site/src/pages/CliAuthPage/CliAuthPageView.tsx index 135a79580e65e..01109432b9b69 100644 --- a/site/src/pages/CliAuthPage/CliAuthPageView.tsx +++ b/site/src/pages/CliAuthPage/CliAuthPageView.tsx @@ -21,11 +21,9 @@ export const CliAuthPageView: FC = ({ sessionToken }) => { Session token

- Copy the session token below and{" "} - - paste it in your terminal - - . + Copy the session token below and +
+ paste it in your terminal.

@@ -43,7 +41,7 @@ const styles = { instructions: (theme) => ({ fontSize: 16, color: theme.palette.text.secondary, - marginBottom: 32, + paddingBottom: 32, textAlign: "center", lineHeight: "160%", }), @@ -51,6 +49,6 @@ const styles = { backButton: { display: "flex", justifyContent: "flex-end", - paddingTop: 8, + paddingTop: 16, }, } satisfies Record>; From 163db9296b68df08a7e08fec87cf150cfa0bd16e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 26 Jan 2024 21:43:13 +0000 Subject: [PATCH 08/31] fix: resolve issue with ref re-routing --- .../src/components/CodeExample/CodeExample.tsx | 18 +++++++++--------- site/src/components/CopyButton/CopyButton.tsx | 2 +- site/src/pages/CliAuthPage/CliAuthPageView.tsx | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/site/src/components/CodeExample/CodeExample.tsx b/site/src/components/CodeExample/CodeExample.tsx index 85240aa82db4a..11abfdd9b82fc 100644 --- a/site/src/components/CodeExample/CodeExample.tsx +++ b/site/src/components/CodeExample/CodeExample.tsx @@ -1,4 +1,4 @@ -import { useRef, type FC } from "react"; +import { type FC, type KeyboardEvent, type MouseEvent, useRef } from "react"; import { type Interpolation, type Theme } from "@emotion/react"; import { MONOSPACE_FONT_FAMILY } from "theme/constants"; import { CopyButton } from "../CopyButton/CopyButton"; @@ -18,7 +18,11 @@ export const CodeExample: FC = ({ className, }) => { const buttonRef = useRef(null); - const triggerButton = () => buttonRef.current?.click(); + const triggerButton = (event: KeyboardEvent | MouseEvent) => { + if (event.target !== buttonRef.current) { + buttonRef.current?.click(); + } + }; return ( /* eslint-disable-next-line jsx-a11y/no-static-element-interactions -- @@ -28,19 +32,15 @@ export const CodeExample: FC = ({
{ - if (event.target !== buttonRef.current) { - triggerButton(); - } - }} + onClick={triggerButton} onKeyDown={(event) => { if (event.key === "Enter") { - triggerButton(); + triggerButton(event); } }} onKeyUp={(event) => { if (event.key === " ") { - triggerButton(); + triggerButton(event); } }} > diff --git a/site/src/components/CopyButton/CopyButton.tsx b/site/src/components/CopyButton/CopyButton.tsx index 886fa8a085059..38634476dcc07 100644 --- a/site/src/components/CopyButton/CopyButton.tsx +++ b/site/src/components/CopyButton/CopyButton.tsx @@ -2,7 +2,7 @@ import IconButton from "@mui/material/Button"; import Tooltip from "@mui/material/Tooltip"; import Check from "@mui/icons-material/Check"; import { css, type Interpolation, type Theme } from "@emotion/react"; -import { forwardRef, MouseEventHandler, type ReactNode } from "react"; +import { forwardRef, type MouseEventHandler, type ReactNode } from "react"; import { useClipboard } from "hooks/useClipboard"; import { FileCopyIcon } from "../Icons/FileCopyIcon"; diff --git a/site/src/pages/CliAuthPage/CliAuthPageView.tsx b/site/src/pages/CliAuthPage/CliAuthPageView.tsx index 01109432b9b69..a33dfe405e226 100644 --- a/site/src/pages/CliAuthPage/CliAuthPageView.tsx +++ b/site/src/pages/CliAuthPage/CliAuthPageView.tsx @@ -43,7 +43,7 @@ const styles = { color: theme.palette.text.secondary, paddingBottom: 32, textAlign: "center", - lineHeight: "160%", + lineHeight: 1.6, }), backButton: { From 38b80a84d761b637ecd95b49655c2dee70ebd04e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 00:48:17 +0000 Subject: [PATCH 09/31] docs: update comments for clarity --- site/src/hooks/useClipboard.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index 80733aa356167..c9b837749cda0 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -163,8 +163,8 @@ function simulateClipboard( document.body.appendChild(dummyInput); dummyInput.focus(); - // Confusingly, you want to call the method opposite of what you want to do to - // interact with the exec method + // Confusingly, you want to use the command opposite of what you actually want + // to do to interact with the execCommand method const command = operation === "read" ? "paste" : "copy"; const isExecSupported = document.execCommand(command); const value = dummyInput.value; From 92f94d761e16c8fcbbebad4e4664cfa9f5de3b59 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 00:48:28 +0000 Subject: [PATCH 10/31] wip: commit progress on clipboard tests --- site/src/hooks/useClipboard.test.tsx | 92 ++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 site/src/hooks/useClipboard.test.tsx diff --git a/site/src/hooks/useClipboard.test.tsx b/site/src/hooks/useClipboard.test.tsx new file mode 100644 index 0000000000000..7f658a5b50a82 --- /dev/null +++ b/site/src/hooks/useClipboard.test.tsx @@ -0,0 +1,92 @@ +import { renderHook, waitFor } from "@testing-library/react"; +import { useClipboard } from "./useClipboard"; + +/** + * userEvent.setup is supposed to stub out a clipboard for you, since it doesn't + * normally exist in JSDOM. Spent ages trying to figure out how to make it work, + * but couldn't figure it out. So the code is using a home-grown mock. + * + * The bad news is that not using userEvent.setup means that you have to test + * all events through fireEvent instead of userEvent + * + * @todo Figure out how to swap userEvent in. + */ +let mockClipboardValue = ""; + +beforeAll(() => { + const originalNavigator = window.navigator; + jest.spyOn(window, "navigator", "get").mockImplementation(() => { + return { + ...originalNavigator, + clipboard: { + readText: async () => mockClipboardValue, + writeText: async (newText) => { + mockClipboardValue = newText; + }, + } as typeof window.navigator.clipboard, + }; + }); + + jest.spyOn(document, "hasFocus").mockImplementation(() => true); + jest.useFakeTimers(); +}); + +afterAll(() => { + jest.resetAllMocks(); + jest.useRealTimers(); +}); + +async function prepareInitialClipboardValue(clipboardText: string) { + /* eslint-disable-next-line testing-library/render-result-naming-convention -- + Need to pass the whole render result back */ + const rendered = renderHook(({ text }) => useClipboard(text), { + initialProps: { text: clipboardText }, + }); + + await rendered.result.current.copyToClipboard(); + await waitFor(() => expect(rendered.result.current.isCopied).toBe(true)); + + return rendered; +} + +describe(useClipboard.name, () => { + describe("copyToClipboard", () => { + it("Injects a new value into the clipboard when called", async () => { + await prepareInitialClipboardValue("blah"); + }); + + it("Injects the most recent value the hook rendered with", async () => { + const text1 = "blah"; + const text2 = "nah"; + const { result, rerender } = await prepareInitialClipboardValue(text1); + + rerender({ text: text2 }); + await waitFor(() => expect(result.current.isCopied).toBe(false)); + + await result.current.copyToClipboard(); + await waitFor(() => expect(result.current.isCopied).toBe(true)); + expect(result.current.isCopied).toBe(true); + }); + }); + + describe("isCopied property", () => { + it("Does not change its value if the clipboard never changes", async () => { + const { result } = await prepareInitialClipboardValue("blah"); + for (let i = 1; i <= 10; i++) { + setTimeout(() => { + expect(result.current.isCopied).toBe(true); + }, i * 10_000); + } + + await jest.advanceTimersByTimeAsync(100_000); + }); + + it.skip("Listens to the user copying different text while in the same tab", async () => { + expect.hasAssertions(); + }); + + it.skip("Re-syncs state when user navigates to a different tab and comes back", async () => { + expect.hasAssertions(); + }); + }); +}); From e0e5e6f4ab0d7159edce20f2f89f493e31b32eb8 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 00:58:27 +0000 Subject: [PATCH 11/31] chore: add extra test case for referential stability --- ...lipboard.test.tsx => useClipboard.test.ts} | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) rename site/src/hooks/{useClipboard.test.tsx => useClipboard.test.ts} (80%) diff --git a/site/src/hooks/useClipboard.test.tsx b/site/src/hooks/useClipboard.test.ts similarity index 80% rename from site/src/hooks/useClipboard.test.tsx rename to site/src/hooks/useClipboard.test.ts index 7f658a5b50a82..a31ec3bf89e67 100644 --- a/site/src/hooks/useClipboard.test.tsx +++ b/site/src/hooks/useClipboard.test.ts @@ -50,7 +50,7 @@ async function prepareInitialClipboardValue(clipboardText: string) { } describe(useClipboard.name, () => { - describe("copyToClipboard", () => { + describe(".copyToClipboard", () => { it("Injects a new value into the clipboard when called", async () => { await prepareInitialClipboardValue("blah"); }); @@ -65,11 +65,23 @@ describe(useClipboard.name, () => { await result.current.copyToClipboard(); await waitFor(() => expect(result.current.isCopied).toBe(true)); - expect(result.current.isCopied).toBe(true); + }); + + it("Maintains a stable reference as long as text doesn't change", async () => { + const text1 = "blah"; + const text2 = "nah"; + const { result, rerender } = await prepareInitialClipboardValue(text1); + const originalFunction = result.current.copyToClipboard; + + rerender({ text: text1 }); + expect(result.current.copyToClipboard).toBe(originalFunction); + + rerender({ text: text2 }); + expect(result.current.copyToClipboard).not.toBe(originalFunction); }); }); - describe("isCopied property", () => { + describe(".isCopied", () => { it("Does not change its value if the clipboard never changes", async () => { const { result } = await prepareInitialClipboardValue("blah"); for (let i = 1; i <= 10; i++) { @@ -81,7 +93,7 @@ describe(useClipboard.name, () => { await jest.advanceTimersByTimeAsync(100_000); }); - it.skip("Listens to the user copying different text while in the same tab", async () => { + it("Listens to the user copying different text while in the same tab", async () => { expect.hasAssertions(); }); From 0ca1558e2df363f5595e81a2a7acca0c5630d972 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 00:59:13 +0000 Subject: [PATCH 12/31] wip: disable test stub to avoid breaking CI --- site/src/hooks/useClipboard.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/hooks/useClipboard.test.ts b/site/src/hooks/useClipboard.test.ts index a31ec3bf89e67..427a8442f15f8 100644 --- a/site/src/hooks/useClipboard.test.ts +++ b/site/src/hooks/useClipboard.test.ts @@ -93,7 +93,7 @@ describe(useClipboard.name, () => { await jest.advanceTimersByTimeAsync(100_000); }); - it("Listens to the user copying different text while in the same tab", async () => { + it.skip("Listens to the user copying different text while in the same tab", async () => { expect.hasAssertions(); }); From 88b96df4f6ce068291b45d85f7c97ccb60317e0f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 02:36:29 +0000 Subject: [PATCH 13/31] wip: add test case for tab-switching --- site/src/hooks/useClipboard.test.ts | 64 +++++++++++++++++++---------- 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/site/src/hooks/useClipboard.test.ts b/site/src/hooks/useClipboard.test.ts index 427a8442f15f8..d2efa53f373a1 100644 --- a/site/src/hooks/useClipboard.test.ts +++ b/site/src/hooks/useClipboard.test.ts @@ -1,4 +1,9 @@ -import { renderHook, waitFor } from "@testing-library/react"; +import { + createEvent, + fireEvent, + renderHook, + waitFor, +} from "@testing-library/react"; import { useClipboard } from "./useClipboard"; /** @@ -9,28 +14,31 @@ import { useClipboard } from "./useClipboard"; * The bad news is that not using userEvent.setup means that you have to test * all events through fireEvent instead of userEvent * - * @todo Figure out how to swap userEvent in. + * @todo Figure out how to swap userEvent in, just to help make sure that the + * tests mirror actual user flows more closely. */ let mockClipboardValue = ""; +const mockClipboard: Partial = { + readText: async () => mockClipboardValue, + writeText: async (newText) => { + mockClipboardValue = newText; + }, +}; beforeAll(() => { const originalNavigator = window.navigator; jest.spyOn(window, "navigator", "get").mockImplementation(() => { - return { - ...originalNavigator, - clipboard: { - readText: async () => mockClipboardValue, - writeText: async (newText) => { - mockClipboardValue = newText; - }, - } as typeof window.navigator.clipboard, - }; + return { ...originalNavigator, clipboard: mockClipboard as Clipboard }; }); jest.spyOn(document, "hasFocus").mockImplementation(() => true); jest.useFakeTimers(); }); +afterEach(() => { + mockClipboardValue = ""; +}); + afterAll(() => { jest.resetAllMocks(); jest.useRealTimers(); @@ -49,17 +57,17 @@ async function prepareInitialClipboardValue(clipboardText: string) { return rendered; } +const text1 = "blah"; +const text2 = "nah"; + describe(useClipboard.name, () => { describe(".copyToClipboard", () => { it("Injects a new value into the clipboard when called", async () => { - await prepareInitialClipboardValue("blah"); + await prepareInitialClipboardValue(text1); }); it("Injects the most recent value the hook rendered with", async () => { - const text1 = "blah"; - const text2 = "nah"; const { result, rerender } = await prepareInitialClipboardValue(text1); - rerender({ text: text2 }); await waitFor(() => expect(result.current.isCopied).toBe(false)); @@ -68,8 +76,6 @@ describe(useClipboard.name, () => { }); it("Maintains a stable reference as long as text doesn't change", async () => { - const text1 = "blah"; - const text2 = "nah"; const { result, rerender } = await prepareInitialClipboardValue(text1); const originalFunction = result.current.copyToClipboard; @@ -83,7 +89,7 @@ describe(useClipboard.name, () => { describe(".isCopied", () => { it("Does not change its value if the clipboard never changes", async () => { - const { result } = await prepareInitialClipboardValue("blah"); + const { result } = await prepareInitialClipboardValue(text1); for (let i = 1; i <= 10; i++) { setTimeout(() => { expect(result.current.isCopied).toBe(true); @@ -94,11 +100,27 @@ describe(useClipboard.name, () => { }); it.skip("Listens to the user copying different text while in the same tab", async () => { - expect.hasAssertions(); + const { result } = await prepareInitialClipboardValue(text1); + const copyEvent = createEvent.copy(window, { + clipboardData: { + getData: () => text2, + }, + }); + + fireEvent(window, copyEvent); + await waitFor(() => expect(result.current.isCopied).toBe(false)); }); - it.skip("Re-syncs state when user navigates to a different tab and comes back", async () => { - expect.hasAssertions(); + it("Re-syncs state when user navigates to a different tab and comes back", async () => { + const { result, rerender } = await prepareInitialClipboardValue(text1); + rerender({ text: text2 }); + await waitFor(() => expect(result.current.isCopied).toBe(false)); + + fireEvent(window, new FocusEvent("blur")); + mockClipboardValue = text2; + + fireEvent(window, new FocusEvent("focus")); + await waitFor(() => expect(result.current.isCopied).toBe(true)); }); }); }); From 15feb142a3ece26fc577b8408bf193d53c6f7935 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 02:50:20 +0000 Subject: [PATCH 14/31] feat: finish changes --- site/src/hooks/useClipboard.test.ts | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/site/src/hooks/useClipboard.test.ts b/site/src/hooks/useClipboard.test.ts index d2efa53f373a1..eb31ee6683e88 100644 --- a/site/src/hooks/useClipboard.test.ts +++ b/site/src/hooks/useClipboard.test.ts @@ -1,9 +1,4 @@ -import { - createEvent, - fireEvent, - renderHook, - waitFor, -} from "@testing-library/react"; +import { fireEvent, renderHook, waitFor } from "@testing-library/react"; import { useClipboard } from "./useClipboard"; /** @@ -99,15 +94,13 @@ describe(useClipboard.name, () => { await jest.advanceTimersByTimeAsync(100_000); }); - it.skip("Listens to the user copying different text while in the same tab", async () => { + it("Listens to the user copying different text while in the same tab", async () => { const { result } = await prepareInitialClipboardValue(text1); - const copyEvent = createEvent.copy(window, { - clipboardData: { - getData: () => text2, - }, - }); - fireEvent(window, copyEvent); + // Very hokey, but ClipboardEvents don't exist in testing environments + mockClipboardValue = text2; + fireEvent(window, new Event("copy")); + await waitFor(() => expect(result.current.isCopied).toBe(false)); }); From e3feffc2b87f225e12770517c18d4185906c8247 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 02:57:59 +0000 Subject: [PATCH 15/31] fix: improve styling for strong text --- site/src/pages/CliAuthPage/CliAuthPageView.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/site/src/pages/CliAuthPage/CliAuthPageView.tsx b/site/src/pages/CliAuthPage/CliAuthPageView.tsx index a33dfe405e226..f5fc7e534aac4 100644 --- a/site/src/pages/CliAuthPage/CliAuthPageView.tsx +++ b/site/src/pages/CliAuthPage/CliAuthPageView.tsx @@ -22,8 +22,7 @@ export const CliAuthPageView: FC = ({ sessionToken }) => {

Copy the session token below and -
- paste it in your terminal. + paste it in your terminal.

From 3ec51966fd797d6b8f4f4564068b983c9de1e5a3 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 02:58:33 +0000 Subject: [PATCH 16/31] fix: make sure period doesn't break onto separate line --- site/src/pages/CliAuthPage/CliAuthPageView.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/CliAuthPage/CliAuthPageView.tsx b/site/src/pages/CliAuthPage/CliAuthPageView.tsx index f5fc7e534aac4..c71fe6c7a2944 100644 --- a/site/src/pages/CliAuthPage/CliAuthPageView.tsx +++ b/site/src/pages/CliAuthPage/CliAuthPageView.tsx @@ -22,7 +22,7 @@ export const CliAuthPageView: FC = ({ sessionToken }) => {

Copy the session token below and - paste it in your terminal. + paste it in your terminal.

From cf2d179cfed517b6de9451dcb622c23d07693c1a Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sat, 27 Jan 2024 03:44:12 +0000 Subject: [PATCH 17/31] fix: make center styling more friendly to screen readers --- site/src/pages/CliAuthPage/CliAuthPageView.tsx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/site/src/pages/CliAuthPage/CliAuthPageView.tsx b/site/src/pages/CliAuthPage/CliAuthPageView.tsx index c71fe6c7a2944..86dc5d3c6081c 100644 --- a/site/src/pages/CliAuthPage/CliAuthPageView.tsx +++ b/site/src/pages/CliAuthPage/CliAuthPageView.tsx @@ -6,11 +6,14 @@ import { CodeExample } from "components/CodeExample/CodeExample"; import { SignInLayout } from "components/SignInLayout/SignInLayout"; import { Welcome } from "components/Welcome/Welcome"; import { FullScreenLoader } from "components/Loader/FullScreenLoader"; +import { visuallyHidden } from "@mui/utils"; export interface CliAuthPageViewProps { sessionToken?: string; } +const VISUALLY_HIDDEN_SPACE = " "; + export const CliAuthPageView: FC = ({ sessionToken }) => { if (!sessionToken) { return ; @@ -22,6 +25,12 @@ export const CliAuthPageView: FC = ({ sessionToken }) => {

Copy the session token below and + {/* + * This looks silly, but it's a case where you want to hide the space + * visually because it messes up the centering, but you want the space + * to still be available to screen readers + */} + {VISUALLY_HIDDEN_SPACE} paste it in your terminal.

From 98bd1af808126ac804140d05bd599d128bf5448f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sun, 28 Jan 2024 14:48:43 +0000 Subject: [PATCH 18/31] refactor: clean up mocking implementation --- site/src/hooks/useClipboard.test.ts | 51 +++++++++++++++++++---------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/site/src/hooks/useClipboard.test.ts b/site/src/hooks/useClipboard.test.ts index eb31ee6683e88..dbf5064b945bf 100644 --- a/site/src/hooks/useClipboard.test.ts +++ b/site/src/hooks/useClipboard.test.ts @@ -1,6 +1,3 @@ -import { fireEvent, renderHook, waitFor } from "@testing-library/react"; -import { useClipboard } from "./useClipboard"; - /** * userEvent.setup is supposed to stub out a clipboard for you, since it doesn't * normally exist in JSDOM. Spent ages trying to figure out how to make it work, @@ -12,18 +9,41 @@ import { useClipboard } from "./useClipboard"; * @todo Figure out how to swap userEvent in, just to help make sure that the * tests mirror actual user flows more closely. */ -let mockClipboardValue = ""; -const mockClipboard: Partial = { - readText: async () => mockClipboardValue, - writeText: async (newText) => { - mockClipboardValue = newText; - }, -}; +import { fireEvent, renderHook, waitFor } from "@testing-library/react"; +import { useClipboard } from "./useClipboard"; + +type MockClipboard = Readonly< + Clipboard & { + resetText: () => void; + } +>; + +function makeMockClipboard(): MockClipboard { + let mockClipboardValue = ""; + + return { + readText: async () => mockClipboardValue, + writeText: async (newText) => { + mockClipboardValue = newText; + }, + resetText: () => { + mockClipboardValue = ""; + }, + + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + dispatchEvent: jest.fn(), + read: jest.fn(), + write: jest.fn(), + }; +} + +const mockClipboard = makeMockClipboard(); beforeAll(() => { const originalNavigator = window.navigator; jest.spyOn(window, "navigator", "get").mockImplementation(() => { - return { ...originalNavigator, clipboard: mockClipboard as Clipboard }; + return { ...originalNavigator, clipboard: mockClipboard }; }); jest.spyOn(document, "hasFocus").mockImplementation(() => true); @@ -31,7 +51,7 @@ beforeAll(() => { }); afterEach(() => { - mockClipboardValue = ""; + mockClipboard.resetText(); }); afterAll(() => { @@ -97,10 +117,8 @@ describe(useClipboard.name, () => { it("Listens to the user copying different text while in the same tab", async () => { const { result } = await prepareInitialClipboardValue(text1); - // Very hokey, but ClipboardEvents don't exist in testing environments - mockClipboardValue = text2; + await mockClipboard.writeText(text2); fireEvent(window, new Event("copy")); - await waitFor(() => expect(result.current.isCopied).toBe(false)); }); @@ -110,8 +128,7 @@ describe(useClipboard.name, () => { await waitFor(() => expect(result.current.isCopied).toBe(false)); fireEvent(window, new FocusEvent("blur")); - mockClipboardValue = text2; - + await mockClipboard.writeText(text2); fireEvent(window, new FocusEvent("focus")); await waitFor(() => expect(result.current.isCopied).toBe(true)); }); From 400e07cc71f0d96163bf0f49e3bf2208e1463db5 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sun, 28 Jan 2024 15:29:00 +0000 Subject: [PATCH 19/31] fix: resolve security concern for clipboard text --- .../components/CodeExample/CodeExample.tsx | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/site/src/components/CodeExample/CodeExample.tsx b/site/src/components/CodeExample/CodeExample.tsx index 11abfdd9b82fc..613fbe00e5181 100644 --- a/site/src/components/CodeExample/CodeExample.tsx +++ b/site/src/components/CodeExample/CodeExample.tsx @@ -2,6 +2,7 @@ import { type FC, type KeyboardEvent, type MouseEvent, useRef } from "react"; import { type Interpolation, type Theme } from "@emotion/react"; import { MONOSPACE_FONT_FAMILY } from "theme/constants"; import { CopyButton } from "../CopyButton/CopyButton"; +import { visuallyHidden } from "@mui/utils"; export interface CodeExampleProps { code: string; @@ -44,12 +45,37 @@ export const CodeExample: FC = ({ } }} > - {code} + + {/* + * Obfuscating text even though we have the characters replaced with + * discs in the CSS for two reasons: + * 1. The CSS property is non-standard and won't work everywhere; MDN + * warns you not to rely on it alone in production + * 2. Even with it turned on and supported, the plaintext is still + * readily available in the HTML itself + */} + {obfuscateText(code)} + + Encrypted text. Please access via the copy button. + + +
); }; +const wildcardRe = /./g; +function obfuscateText(text: string): string { + // Every global regex is stateful; have to reset the state after each exec to + // make sure that the operations remain pure + const initialLastIndex = wildcardRe.lastIndex; + const result = text.replaceAll(wildcardRe, "*"); + wildcardRe.lastIndex = initialLastIndex; + + return result; +} + const styles = { container: (theme) => ({ cursor: "pointer", From 33074320b9979f9e8f3fc45fec74d21e8262f219 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sun, 28 Jan 2024 21:04:20 +0000 Subject: [PATCH 20/31] fix: update CodeExample to obscure text when appropriate --- .../components/CodeExample/CodeExample.tsx | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/site/src/components/CodeExample/CodeExample.tsx b/site/src/components/CodeExample/CodeExample.tsx index 613fbe00e5181..bf77c31872ab5 100644 --- a/site/src/components/CodeExample/CodeExample.tsx +++ b/site/src/components/CodeExample/CodeExample.tsx @@ -15,8 +15,8 @@ export interface CodeExampleProps { */ export const CodeExample: FC = ({ code, - secret, className, + secret = true, }) => { const buttonRef = useRef(null); const triggerButton = (event: KeyboardEvent | MouseEvent) => { @@ -46,18 +46,24 @@ export const CodeExample: FC = ({ }} > - {/* - * Obfuscating text even though we have the characters replaced with - * discs in the CSS for two reasons: - * 1. The CSS property is non-standard and won't work everywhere; MDN - * warns you not to rely on it alone in production - * 2. Even with it turned on and supported, the plaintext is still - * readily available in the HTML itself - */} - {obfuscateText(code)} - - Encrypted text. Please access via the copy button. - + {secret ? ( + <> + {/* + * Obfuscating text even though we have the characters replaced with + * discs in the CSS for two reasons: + * 1. The CSS property is non-standard and won't work everywhere; + * MDN warns you not to rely on it alone in production + * 2. Even with it turned on and supported, the plaintext is still + * readily available in the HTML itself + */} + {obfuscateText(code)} + + Encrypted text. Please access via the copy button. + + + ) : ( + <>{code} + )} From f10134fcf250485f0e57c6383953352355e257b8 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Sun, 28 Jan 2024 21:12:31 +0000 Subject: [PATCH 21/31] fix: apply secret changes to relevant code examples --- site/src/components/CodeExample/CodeExample.tsx | 3 +++ site/src/pages/CreateTokenPage/CreateTokenPage.tsx | 1 + .../src/pages/UserSettingsPage/SSHKeysPage/SSHKeysPageView.tsx | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/site/src/components/CodeExample/CodeExample.tsx b/site/src/components/CodeExample/CodeExample.tsx index bf77c31872ab5..13dd678280f15 100644 --- a/site/src/components/CodeExample/CodeExample.tsx +++ b/site/src/components/CodeExample/CodeExample.tsx @@ -16,6 +16,9 @@ export interface CodeExampleProps { export const CodeExample: FC = ({ code, className, + + // Defaulting to true to be on the safe side; you should have to opt out of + // the secure option, not remember to opt in secret = true, }) => { const buttonRef = useRef(null); diff --git a/site/src/pages/CreateTokenPage/CreateTokenPage.tsx b/site/src/pages/CreateTokenPage/CreateTokenPage.tsx index 0c6bad22e9cd6..e48cba07c55fc 100644 --- a/site/src/pages/CreateTokenPage/CreateTokenPage.tsx +++ b/site/src/pages/CreateTokenPage/CreateTokenPage.tsx @@ -71,6 +71,7 @@ export const CreateTokenPage: FC = () => { <>

Make sure you copy the below token before proceeding:

> = ({ .

- +
+
); @@ -49,14 +48,26 @@ const styles = { instructions: (theme) => ({ fontSize: 16, color: theme.palette.text.secondary, - paddingBottom: 32, + paddingBottom: 8, textAlign: "center", - lineHeight: 1.6, + lineHeight: 1.4, + + // Have to undo styling side effects from component + marginTop: -24, }), - backButton: { - display: "flex", - justifyContent: "flex-end", + backLink: (theme) => ({ + display: "block", + textAlign: "center", + color: theme.palette.text.primary, + textDecoration: "underline", + textUnderlineOffset: 3, + textDecorationColor: "hsla(0deg, 0%, 100%, 0.7)", paddingTop: 16, - }, + paddingBottom: 16, + + "&:hover": { + textDecoration: "none", + }, + }), } satisfies Record>; From 89c74dafc7ec4e7b12876fd2da1ca8b6fe56bc13 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 29 Jan 2024 14:38:47 +0000 Subject: [PATCH 25/31] fix: remove duplicate property identifier --- site/src/hooks/useClipboard.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index 1bbde2665769c..efa548a47320a 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -40,7 +40,7 @@ export const useClipboard = (textToCopy: string): UseClipboardResult => { } }; - return { isCopied: isCopied, copyToClipboard }; + return { isCopied, copyToClipboard }; }; function simulateClipboardWrite(): boolean { From b95cfd63abe69e49e73e9c0c18002e4b1f5bd12f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 1 Feb 2024 01:26:41 +0000 Subject: [PATCH 26/31] refactor: rename variables for clarity --- site/src/hooks/useClipboard.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index efa548a47320a..af41c4cad3dc6 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -22,8 +22,8 @@ export const useClipboard = (textToCopy: string): UseClipboardResult => { setIsCopied(false); }, 1000); } catch (err) { - const isExecSupported = simulateClipboardWrite(); - if (isExecSupported) { + const isExecCommandSupported = simulateClipboardWrite(); + if (isExecCommandSupported) { setIsCopied(true); timeoutIdRef.current = window.setTimeout(() => { setIsCopied(false); @@ -51,12 +51,12 @@ function simulateClipboardWrite(): boolean { dummyInput.focus(); dummyInput.select(); - const isExecSupported = document.execCommand("copy"); + const isExecCommandSupported = document.execCommand("copy"); dummyInput.remove(); if (previousFocusTarget instanceof HTMLElement) { previousFocusTarget.focus(); } - return isExecSupported; + return isExecCommandSupported; } From 4ab6326ab9f9e8dc1f2a623afbd121323fd0e0e0 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 1 Feb 2024 01:28:33 +0000 Subject: [PATCH 27/31] fix: simplify/revert CopyButton component design --- site/src/components/CopyButton/CopyButton.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/site/src/components/CopyButton/CopyButton.tsx b/site/src/components/CopyButton/CopyButton.tsx index 38634476dcc07..2cce987b88381 100644 --- a/site/src/components/CopyButton/CopyButton.tsx +++ b/site/src/components/CopyButton/CopyButton.tsx @@ -45,10 +45,7 @@ export const CopyButton = forwardRef( size="small" aria-label={Language.ariaLabel} variant="text" - onClick={(event) => { - void copyToClipboard(); - outsideOnClick?.(event); - }} + onClick={copyToClipboard} > {isCopied ? ( From 778560f22fdeb23bc82d1bf8c2d5fb9e78572c35 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 1 Feb 2024 01:51:57 +0000 Subject: [PATCH 28/31] fix: update how dummy input is hidden from page --- site/src/hooks/useClipboard.ts | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index af41c4cad3dc6..ed50a67f44e92 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -22,8 +22,8 @@ export const useClipboard = (textToCopy: string): UseClipboardResult => { setIsCopied(false); }, 1000); } catch (err) { - const isExecCommandSupported = simulateClipboardWrite(); - if (isExecCommandSupported) { + const isCopied = simulateClipboardWrite(); + if (isCopied) { setIsCopied(true); timeoutIdRef.current = window.setTimeout(() => { setIsCopied(false); @@ -43,20 +43,42 @@ export const useClipboard = (textToCopy: string): UseClipboardResult => { return { isCopied, copyToClipboard }; }; +/** + * 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(): boolean { const previousFocusTarget = document.activeElement; const dummyInput = document.createElement("input"); - dummyInput.style.visibility = "hidden"; + + // 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.focus(); dummyInput.select(); - const isExecCommandSupported = document.execCommand("copy"); + const isCopied = document.execCommand("copy"); dummyInput.remove(); if (previousFocusTarget instanceof HTMLElement) { previousFocusTarget.focus(); } - return isExecCommandSupported; + return isCopied; } From cf53114cc7f1e30b25469d27024de018c49a489c Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 1 Feb 2024 02:01:04 +0000 Subject: [PATCH 29/31] fix: remove unused onClick handler prop --- site/src/components/CopyButton/CopyButton.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/site/src/components/CopyButton/CopyButton.tsx b/site/src/components/CopyButton/CopyButton.tsx index 2cce987b88381..da198ffc2d1f3 100644 --- a/site/src/components/CopyButton/CopyButton.tsx +++ b/site/src/components/CopyButton/CopyButton.tsx @@ -13,7 +13,6 @@ interface CopyButtonProps { wrapperStyles?: Interpolation; buttonStyles?: Interpolation; tooltipTitle?: string; - onClick?: MouseEventHandler; } export const Language = { @@ -31,7 +30,6 @@ export const CopyButton = forwardRef( ctaCopy, wrapperStyles, buttonStyles, - onClick: outsideOnClick, tooltipTitle = Language.tooltipTitle, } = props; const { isCopied, copyToClipboard } = useClipboard(text); From 97fed6f95a1678eb5e4bee043d429476f8f875ed Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 1 Feb 2024 02:05:21 +0000 Subject: [PATCH 30/31] fix: resolve unused import --- site/src/components/CopyButton/CopyButton.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/components/CopyButton/CopyButton.tsx b/site/src/components/CopyButton/CopyButton.tsx index da198ffc2d1f3..64b01974478b0 100644 --- a/site/src/components/CopyButton/CopyButton.tsx +++ b/site/src/components/CopyButton/CopyButton.tsx @@ -2,7 +2,7 @@ import IconButton from "@mui/material/Button"; import Tooltip from "@mui/material/Tooltip"; import Check from "@mui/icons-material/Check"; import { css, type Interpolation, type Theme } from "@emotion/react"; -import { forwardRef, type MouseEventHandler, type ReactNode } from "react"; +import { forwardRef, type ReactNode } from "react"; import { useClipboard } from "hooks/useClipboard"; import { FileCopyIcon } from "../Icons/FileCopyIcon"; From fb0f4b7d6f7a672cd9b2f286c9675b0473ddf516 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 1 Feb 2024 02:17:49 +0000 Subject: [PATCH 31/31] fix: opt code examples out of secret behavior --- site/src/components/CodeExample/CodeExample.stories.tsx | 8 +++++++- site/src/modules/resources/SSHButton/SSHButton.tsx | 3 ++- site/src/pages/TemplatesPage/EmptyTemplates.tsx | 2 +- site/src/pages/UsersPage/ResetPasswordDialog.tsx | 1 + 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/site/src/components/CodeExample/CodeExample.stories.tsx b/site/src/components/CodeExample/CodeExample.stories.tsx index 1af88cd98dafd..e7fe5203d4177 100644 --- a/site/src/components/CodeExample/CodeExample.stories.tsx +++ b/site/src/components/CodeExample/CodeExample.stories.tsx @@ -5,6 +5,7 @@ const meta: Meta = { title: "components/CodeExample", component: CodeExample, args: { + secret: false, code: `echo "hello, friend!"`, }, }; @@ -12,7 +13,11 @@ const meta: Meta = { export default meta; type Story = StoryObj; -export const Example: Story = {}; +export const Example: Story = { + args: { + secret: false, + }, +}; export const Secret: Story = { args: { @@ -22,6 +27,7 @@ export const Secret: Story = { export const LongCode: Story = { args: { + secret: false, code: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAICnKzATuWwmmt5+CKTPuRGN0R1PBemA+6/SStpLiyX+L", }, }; diff --git a/site/src/modules/resources/SSHButton/SSHButton.tsx b/site/src/modules/resources/SSHButton/SSHButton.tsx index 62d1beb648dbe..d4f8f374f8aba 100644 --- a/site/src/modules/resources/SSHButton/SSHButton.tsx +++ b/site/src/modules/resources/SSHButton/SSHButton.tsx @@ -57,7 +57,7 @@ export const SSHButton: FC = ({ Configure SSH hosts on machine: - +
@@ -67,6 +67,7 @@ export const SSHButton: FC = ({
diff --git a/site/src/pages/TemplatesPage/EmptyTemplates.tsx b/site/src/pages/TemplatesPage/EmptyTemplates.tsx index b628c30e86fb7..372c3dd326a30 100644 --- a/site/src/pages/TemplatesPage/EmptyTemplates.tsx +++ b/site/src/pages/TemplatesPage/EmptyTemplates.tsx @@ -91,7 +91,7 @@ export const EmptyTemplates: FC = ({ css={styles.withImage} message="Create a Template" description="Contact your Coder administrator to create a template. You can share the code below." - cta={} + cta={} image={
diff --git a/site/src/pages/UsersPage/ResetPasswordDialog.tsx b/site/src/pages/UsersPage/ResetPasswordDialog.tsx index a96e7d34cbdb8..381543ee5a6d7 100644 --- a/site/src/pages/UsersPage/ResetPasswordDialog.tsx +++ b/site/src/pages/UsersPage/ResetPasswordDialog.tsx @@ -34,6 +34,7 @@ export const ResetPasswordDialog: FC = ({ <>

{Language.message(user?.username)}