fix(client): address review feedback on the copy-message change
Four issues from code review: - The selection guard read the document-wide selection, so a leftover selection anywhere on the page could swallow the first click on any row. It now checks that the selection is inside the row that was clicked, with the event threaded through to both call sites. - CopyButton's reset timer was never cleared, so it fired against an unmounted component when a row went away on pagination, and a rapid second click could not cancel the pending reset. Held in a ref and cleared on unmount and on re-copy. - navigator.clipboard only exists on secure origins, and the README documents reaching Checkmate over plain HTTP on a LAN address. There the API is undefined, so every click showed "could not copy" with no way forward. Falls back to a hidden textarea. - The two message cells were byte-identical, so they are now one shared CopyableText component. Claude-Session: https://claude.ai/code/session_01AbYLvwpEaZFXktHVcqrFRd
This commit is contained in:
1 parent
db4689207a
commit
84a1b994a1
6 files changed
+97
-50
No files matched your search
@@ -1,4 +1,4 @@
|
||||
import { useState } from "react";
|
||||
import { useEffect, useRef, useState } from "react";
|
||||
import IconButton from "@mui/material/IconButton";
|
||||
import Tooltip from "@mui/material/Tooltip";
|
||||
import { useTheme } from "@mui/material/styles";
|
||||
@@ -8,6 +8,31 @@ import { useToast } from "@/Hooks/UseToast";
|
||||
|
||||
const RESET_DELAY_MS = 1500;
|
||||
|
||||
/**
|
||||
* `navigator.clipboard` only exists on secure origins, and Checkmate is often
|
||||
* self-hosted over plain HTTP on a LAN address. Fall back to a hidden textarea
|
||||
* there rather than leaving the button permanently broken.
|
||||
*/
|
||||
const copyText = async (value: string) => {
|
||||
if (navigator.clipboard?.writeText) {
|
||||
return navigator.clipboard.writeText(value);
|
||||
}
|
||||
|
||||
const textarea = document.createElement("textarea");
|
||||
textarea.value = value;
|
||||
textarea.setAttribute("readonly", "");
|
||||
textarea.style.position = "fixed";
|
||||
textarea.style.opacity = "0";
|
||||
document.body.appendChild(textarea);
|
||||
textarea.select();
|
||||
|
||||
try {
|
||||
if (!document.execCommand("copy")) throw new Error("Copy command was rejected");
|
||||
} finally {
|
||||
document.body.removeChild(textarea);
|
||||
}
|
||||
};
|
||||
|
||||
/**
|
||||
* Copies `value` to the clipboard. Shows a tick for a moment so the click is
|
||||
* acknowledged without moving the user anywhere.
|
||||
@@ -17,15 +42,20 @@ const CopyButton = ({ value, label }: { value: string; label?: string }) => {
|
||||
const { t } = useTranslation();
|
||||
const { toastError } = useToast();
|
||||
const [copied, setCopied] = useState(false);
|
||||
const resetTimer = useRef<ReturnType<typeof setTimeout>>(undefined);
|
||||
|
||||
// These rows unmount on pagination, so the tick timer has to be cancelled.
|
||||
useEffect(() => () => clearTimeout(resetTimer.current), []);
|
||||
|
||||
const handleCopy = async (e: React.MouseEvent) => {
|
||||
// The button usually sits inside a clickable row.
|
||||
e.stopPropagation();
|
||||
|
||||
try {
|
||||
await navigator.clipboard.writeText(value);
|
||||
await copyText(value);
|
||||
setCopied(true);
|
||||
setTimeout(() => setCopied(false), RESET_DELAY_MS);
|
||||
clearTimeout(resetTimer.current);
|
||||
resetTimer.current = setTimeout(() => setCopied(false), RESET_DELAY_MS);
|
||||
} catch {
|
||||
toastError(t("common.copyFailed"));
|
||||
}
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
import Stack from "@mui/material/Stack";
|
||||
import { useTheme } from "@mui/material/styles";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { LAYOUT } from "@/Utils/Theme/constants";
|
||||
import CopyButton from "./CopyButton";
|
||||
|
||||
/**
|
||||
* Text alongside a copy button, for table cells holding values worth lifting
|
||||
* out - error messages in particular.
|
||||
*/
|
||||
const CopyableText = ({
|
||||
value,
|
||||
fallback = "N/A",
|
||||
copyLabel,
|
||||
}: {
|
||||
value?: string | null;
|
||||
fallback?: string;
|
||||
copyLabel?: string;
|
||||
}) => {
|
||||
const theme = useTheme();
|
||||
const { t } = useTranslation();
|
||||
|
||||
if (!value) return <>{fallback}</>;
|
||||
|
||||
return (
|
||||
<Stack
|
||||
direction="row"
|
||||
alignItems="center"
|
||||
justifyContent="center"
|
||||
gap={theme.spacing(LAYOUT.XS)}
|
||||
>
|
||||
<span>{value}</span>
|
||||
<CopyButton
|
||||
value={value}
|
||||
label={copyLabel ?? t("common.copyMessage")}
|
||||
/>
|
||||
</Stack>
|
||||
);
|
||||
};
|
||||
|
||||
export default CopyableText;
|
||||
@@ -82,10 +82,21 @@ export function DataTable<
|
||||
|
||||
// Selecting text inside a row ends in a mouseup on that row, which would
|
||||
// otherwise navigate away and take the text with it. Ignore the click when
|
||||
// the user has actually selected something.
|
||||
const hasTextSelection = () => {
|
||||
// the selection is inside the row that was clicked - a leftover selection
|
||||
// elsewhere on the page must not swallow a legitimate click.
|
||||
const hasTextSelectionWithin = (element: Element) => {
|
||||
const selection = window.getSelection();
|
||||
return Boolean(selection && !selection.isCollapsed && selection.toString().trim());
|
||||
if (!selection || selection.isCollapsed || !selection.toString().trim()) {
|
||||
return false;
|
||||
}
|
||||
return (
|
||||
selection.anchorNode !== null &&
|
||||
element.contains(
|
||||
selection.anchorNode.nodeType === Node.TEXT_NODE
|
||||
? selection.anchorNode.parentNode
|
||||
: selection.anchorNode
|
||||
)
|
||||
);
|
||||
};
|
||||
const isInteractive = Boolean(onRowClick) || expandableRows;
|
||||
|
||||
@@ -108,8 +119,8 @@ export function DataTable<
|
||||
keys.push(key);
|
||||
return (
|
||||
<Stack
|
||||
onClick={() => {
|
||||
if (hasTextSelection()) return;
|
||||
onClick={(e) => {
|
||||
if (hasTextSelectionWithin(e.currentTarget)) return;
|
||||
if (onRowClick) onRowClick(row);
|
||||
}}
|
||||
spacing={theme.spacing(LAYOUT.XS)}
|
||||
@@ -257,8 +268,8 @@ export function DataTable<
|
||||
cursor: isInteractive ? "pointer" : "default",
|
||||
...(getRowSx?.(row) as object),
|
||||
}}
|
||||
onClick={() => {
|
||||
if (hasTextSelection()) return;
|
||||
onClick={(e) => {
|
||||
if (hasTextSelectionWithin(e.currentTarget)) return;
|
||||
if (expandableRows) handleExpand(row);
|
||||
else if (onRowClick) onRowClick(row);
|
||||
}}
|
||||
|
||||
@@ -29,3 +29,4 @@ export * from "./StatusCodeLabel";
|
||||
export * from "./StrategyBadge";
|
||||
export * from "./StepProgress";
|
||||
export { default as CopyButton } from "./CopyButton";
|
||||
export { default as CopyableText } from "./CopyableText";
|
||||
@@ -3,12 +3,9 @@ import {
|
||||
Pagination,
|
||||
StatusLabel,
|
||||
StatusCodeLabel,
|
||||
CopyButton,
|
||||
CopyableText,
|
||||
} from "@/Components/design-elements";
|
||||
import Box from "@mui/material/Box";
|
||||
import Stack from "@mui/material/Stack";
|
||||
import { useTheme } from "@mui/material/styles";
|
||||
import { LAYOUT } from "@/Utils/Theme/constants";
|
||||
import type { Header } from "@/Components/design-elements/Table";
|
||||
import type { Monitor } from "@/Types/Monitor";
|
||||
import { useTranslation } from "react-i18next";
|
||||
@@ -35,7 +32,6 @@ export const ChecksTable = ({
|
||||
setRowsPerPage: (rowsPerPage: number) => void;
|
||||
}) => {
|
||||
const { t } = useTranslation();
|
||||
const theme = useTheme();
|
||||
const uiTimezone = useSelector((state: RootState) => state.ui.timezone);
|
||||
|
||||
const getHeaders = () => {
|
||||
@@ -84,21 +80,7 @@ export const ChecksTable = ({
|
||||
id: "message",
|
||||
content: t("common.table.headers.message"),
|
||||
render: (row) => {
|
||||
if (!row.message) return "N/A";
|
||||
return (
|
||||
<Stack
|
||||
direction="row"
|
||||
alignItems="center"
|
||||
justifyContent="center"
|
||||
gap={theme.spacing(LAYOUT.XS)}
|
||||
>
|
||||
<span>{row.message}</span>
|
||||
<CopyButton
|
||||
value={row.message}
|
||||
label={t("common.copyMessage")}
|
||||
/>
|
||||
</Stack>
|
||||
);
|
||||
return <CopyableText value={row.message} />;
|
||||
},
|
||||
},
|
||||
];
|
||||
|
||||
@@ -3,12 +3,9 @@ import {
|
||||
Pagination,
|
||||
StatusLabel,
|
||||
StatusCodeLabel,
|
||||
CopyButton,
|
||||
CopyableText,
|
||||
} from "@/Components/design-elements";
|
||||
import Box from "@mui/material/Box";
|
||||
import Stack from "@mui/material/Stack";
|
||||
import { useTheme } from "@mui/material/styles";
|
||||
import { LAYOUT } from "@/Utils/Theme/constants";
|
||||
import type { Header } from "@/Components/design-elements";
|
||||
import type { Check } from "@/Types/Check";
|
||||
|
||||
@@ -35,7 +32,6 @@ export const ChecksTable = ({
|
||||
}) => {
|
||||
const navigate = useNavigate();
|
||||
const { t } = useTranslation();
|
||||
const theme = useTheme();
|
||||
const uiTimezone = useSelector((state: RootState) => state.ui.timezone);
|
||||
|
||||
const headers: Header<Check>[] = [
|
||||
@@ -57,21 +53,7 @@ export const ChecksTable = ({
|
||||
id: "message",
|
||||
content: t("common.table.headers.message"),
|
||||
render: (row) => {
|
||||
if (!row.message) return "N/A";
|
||||
return (
|
||||
<Stack
|
||||
direction="row"
|
||||
alignItems="center"
|
||||
justifyContent="center"
|
||||
gap={theme.spacing(LAYOUT.XS)}
|
||||
>
|
||||
<span>{row.message}</span>
|
||||
<CopyButton
|
||||
value={row.message}
|
||||
label={t("common.copyMessage")}
|
||||
/>
|
||||
</Stack>
|
||||
);
|
||||
return <CopyableText value={row.message} />;
|
||||
},
|
||||
},
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user