Skip to content

fix: improve click UX and styling for Auth Token page #11863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 32 commits into from
Feb 1, 2024
Merged

fix: improve click UX and styling for Auth Token page #11863

Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7e4d964
wip: commit progress for clipboard update
Parkreiner Jan 26, 2024
ece3f6d
wip: push more progress
Parkreiner Jan 26, 2024
bf6e73a
chore: finish initial version of useClipboard revamp
Parkreiner Jan 26, 2024
697bdf0
refactor: update API query to use newer RQ patterns
Parkreiner Jan 26, 2024
9b881c0
fix: update importers of useClipboard
Parkreiner Jan 26, 2024
ccdca08
fix: increase clickable area of CodeExample
Parkreiner Jan 26, 2024
a7c7cc7
fix: update styles for CliAuthPageView
Parkreiner Jan 26, 2024
163db92
fix: resolve issue with ref re-routing
Parkreiner Jan 26, 2024
38b80a8
docs: update comments for clarity
Parkreiner Jan 27, 2024
92f94d7
wip: commit progress on clipboard tests
Parkreiner Jan 27, 2024
e0e5e6f
chore: add extra test case for referential stability
Parkreiner Jan 27, 2024
0ca1558
wip: disable test stub to avoid breaking CI
Parkreiner Jan 27, 2024
88b96df
wip: add test case for tab-switching
Parkreiner Jan 27, 2024
15feb14
feat: finish changes
Parkreiner Jan 27, 2024
e3feffc
fix: improve styling for strong text
Parkreiner Jan 27, 2024
3ec5196
fix: make sure period doesn't break onto separate line
Parkreiner Jan 27, 2024
cf2d179
fix: make center styling more friendly to screen readers
Parkreiner Jan 27, 2024
98bd1af
refactor: clean up mocking implementation
Parkreiner Jan 28, 2024
400e07c
fix: resolve security concern for clipboard text
Parkreiner Jan 28, 2024
3307432
fix: update CodeExample to obscure text when appropriate
Parkreiner Jan 28, 2024
f10134f
fix: apply secret changes to relevant code examples
Parkreiner Jan 28, 2024
c4469f3
refactor: simplify code for obfuscating text
Parkreiner Jan 28, 2024
d8b6727
fix: partially revert clipboard changes
Parkreiner Jan 29, 2024
6157049
fix: clean up page styling further
Parkreiner Jan 29, 2024
89c74da
fix: remove duplicate property identifier
Parkreiner Jan 29, 2024
b95cfd6
refactor: rename variables for clarity
Parkreiner Feb 1, 2024
4ab6326
fix: simplify/revert CopyButton component design
Parkreiner Feb 1, 2024
778560f
fix: update how dummy input is hidden from page
Parkreiner Feb 1, 2024
cf53114
fix: remove unused onClick handler prop
Parkreiner Feb 1, 2024
97fed6f
fix: resolve unused import
Parkreiner Feb 1, 2024
83febe2
Merge branch 'main' into mes/clipboard-fix
Parkreiner Feb 1, 2024
fb0f4b7
fix: opt code examples out of secret behavior
Parkreiner Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
chore: finish initial version of useClipboard revamp
  • Loading branch information
@Parkreiner
Parkreiner committed Jan 26, 2024
commit bf6e73ad3bce80bef5754795bffa5630b508778b
127 changes: 58 additions & 69 deletions site/src/hooks/useClipboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { useCallback, useEffect, useState } from "react";
import { useEffectEvent } from "./hookPolyfills";

type UseClipboardResult = Readonly<{
isCopied: boolean;
Expand All @@ -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,
Expand All @@ -59,13 +57,16 @@ type ResultWithData<T = unknown> = Readonly<

type Result<T = unknown> = void extends T ? VoidResult : ResultWithData<T>;

export async function readFromClipboard(): Promise<Result<string>> {
async function readFromClipboard(): Promise<Result<string>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand this part. Why do we need to read something from the clipboard? I would expect to just write things in it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe listing the flow using bullet points would make things easier to understand for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mainly for setting up a way to watch the clipboard, since there isn't an observer-based API for it, like there is for a lot of other browser APIs

The main idea is that isCopied is always a derived value that comes from checking whether the text passed into the hook matches the text currently in the clipboard. And the only way to check the current clipboard is by reading from it – if we only write to it, then we can't make any assumptions about what's actually in the clipboard, because it can also change from sources outside React

So basically, there's three main "sync cues":

  1. The user copies our text to the clipboard
  2. The user changes their clipboard text while in the same page
  3. The user changes to a different tab, and then comes back

In all of the cases, we don't have a direct, event-based way to check the clipboard contents to verify that the content matches. You would think that writing to the clipboard would expose that information, but the event objects don't actually contain the new clipboard text. So in all three cases, the only choice is reading from it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to check if the content in the clipboard matches the one passed into the hook? Is there a use case in the app where this can happen?

Copy link
Member Author

@Parkreiner Parkreiner Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is splitting semantic hairs, but I guess the question boils down to, "What do we want isCopied to represent?

  • Do we want it to indicate that we've just finished handling a copy operation, and this value represents how long we want to tell the user that the text is copied?
  • Or do we want this property to act as a source of truth, where isCopied answers the question, "Each time the component renders, is the text we provided into the hook copied into the clipboard?"
    • If we don't want to do that, maybe that's a sign we should rename the property, but because the clipboard is one of the most mutable values in web development, the only way to have 100% confidence to that question is by checking the clipboard

That mismatch between the property name and actual functionality is what led me to consider a more synchronization-based solution, but maybe this did end up putting me on a weird, wild goose chase

// 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",
),
};
}
Expand Down Expand Up @@ -109,54 +110,42 @@ export async function readFromClipboard(): Promise<Result<string>> {
}
}

export async function writeToClipboard(
textToCopy: string,
): Promise<Result<void>> {
return {
success: true,
error: null,
};
// Comments for this function mirror the ones for readFromClipboard
async function writeToClipboard(textToCopy: string): Promise<Result<void>> {
if (!document.hasFocus()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... why is this necessary?

Thinking about the most common use case, like the user clicking the button and the value being copied, I don't see how this could happen if the document is not in focus 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a comment in readToClipboard, but it's mainly to suppress errors when trying to do stuff in development mode

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think is there a way to improve readability without adding a comment mentioning another comment? Thinking on a named variable or function like isDevMode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, now that you mention it, I can use an assertion function. I'll get that swapped in

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<{
Expand Down