From 84a1b994a147197b7e0f5e4e806c142a0c9d3f91 Mon Sep 17 00:00:00 2001 From: gorkem-bwl Date: Tue, 1 Sep 2026 22:40:29 -0400 Subject: [PATCH] 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 --- .../Components/design-elements/CopyButton.tsx | 36 ++++++++++++++-- .../design-elements/CopyableText.tsx | 41 +++++++++++++++++++ .../src/Components/design-elements/Table.tsx | 25 +++++++---- .../src/Components/design-elements/index.tsx | 1 + .../Pages/Checks/Components/ChecksTable.tsx | 22 +--------- .../Uptime/Details/Components/ChecksTable.tsx | 22 +--------- 6 files changed, 97 insertions(+), 50 deletions(-) create mode 100644 client/src/Components/design-elements/CopyableText.tsx diff --git a/client/src/Components/design-elements/CopyButton.tsx b/client/src/Components/design-elements/CopyButton.tsx index caa317598..5d45b9739 100644 --- a/client/src/Components/design-elements/CopyButton.tsx +++ b/client/src/Components/design-elements/CopyButton.tsx @@ -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>(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")); } diff --git a/client/src/Components/design-elements/CopyableText.tsx b/client/src/Components/design-elements/CopyableText.tsx new file mode 100644 index 000000000..5d09e307b --- /dev/null +++ b/client/src/Components/design-elements/CopyableText.tsx @@ -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 ( + + {value} + + + ); +}; + +export default CopyableText; diff --git a/client/src/Components/design-elements/Table.tsx b/client/src/Components/design-elements/Table.tsx index c7bdad9f8..8650f2bc6 100644 --- a/client/src/Components/design-elements/Table.tsx +++ b/client/src/Components/design-elements/Table.tsx @@ -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 ( { - 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); }} diff --git a/client/src/Components/design-elements/index.tsx b/client/src/Components/design-elements/index.tsx index e35d95823..e1c1c3965 100644 --- a/client/src/Components/design-elements/index.tsx +++ b/client/src/Components/design-elements/index.tsx @@ -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"; diff --git a/client/src/Pages/Checks/Components/ChecksTable.tsx b/client/src/Pages/Checks/Components/ChecksTable.tsx index 0e75116dc..4d5eebbdf 100644 --- a/client/src/Pages/Checks/Components/ChecksTable.tsx +++ b/client/src/Pages/Checks/Components/ChecksTable.tsx @@ -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 ( - - {row.message} - - - ); + return ; }, }, ]; diff --git a/client/src/Pages/Uptime/Details/Components/ChecksTable.tsx b/client/src/Pages/Uptime/Details/Components/ChecksTable.tsx index 91b4ce588..ae1d87d45 100644 --- a/client/src/Pages/Uptime/Details/Components/ChecksTable.tsx +++ b/client/src/Pages/Uptime/Details/Components/ChecksTable.tsx @@ -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[] = [ @@ -57,21 +53,7 @@ export const ChecksTable = ({ id: "message", content: t("common.table.headers.message"), render: (row) => { - if (!row.message) return "N/A"; - return ( - - {row.message} - - - ); + return ; }, }, {