From 34663ec5f9bd8846e66960c8db4f17c18d266400 Mon Sep 17 00:00:00 2001 From: gorkem-bwl Date: Mon, 7 Sep 2026 06:44:16 -0400 Subject: [PATCH] fix(client): dashboard review pass - correctness, redundancy, comments Code review of the dashboard branch. Four correctness bugs, one shared helper, and the comment density brought down. Correctness: - CurrentlyDownCard: downSince() broke its backwards walk only on `status === true`, so a check with an undefined status kept the run going and reported a downtime reaching back to the start of the retained window. Break on `status !== false` instead, matching what the doc comment already claimed. - RankingCards: the lowest-uptime card barred the uptime percentage itself, so the worst monitor drew the shortest bar - a chart that read as "best uptime" directly against its own title and sort order. Bar the shortfall instead. - useCardTransitions: the FLIP rAF and its nested 260ms timeout were never cancelled. Navigating away mid-animation wrote styles to unmounted nodes, and a second move while the first was settling had its inversion wiped by the earlier timeout, so the card jumped rather than travelled. Handles are now tracked and cancelled on unmount and at the start of each move. - BreakdownCard: the bar scale took buckets[0].total, silently assuming the caller sorted descending. Compute the max instead - both callers do sort today, but nothing in the signature said they had to. Redundancy: - "newest check is the last element of recentChecks" was implemented twice with its own copy of the explanation. Now one `latestCheck` helper in checks.ts. RankingCards averages over all checks, which is a different operation, and is left alone. - DEFAULT_CARD_IDS is `readonly` and copied into state rather than assigned by reference. Comments and conventions: - 283 comment lines down to ~220. Design rationale ("two lines rather than one wide row", "the header controls stay invisible until...") is gone; comments recording non-obvious API facts - recentChecks is oldest-first, snake_case here but camelCase there, credential fields must not reach the DOM - are kept and shortened. - DashboardCard: display/flexDirection/height hoisted out of sx to native props (frontend rule 1). A stale comment claiming a "34px control height" is corrected; the codebase settled on 32. Gates: tsc clean, build passes, format-check clean, lint unchanged from develop (12 errors, all pre-existing in untouched files). --- .../Dashboard/Components/CardSlotContext.tsx | 5 -- .../Components/Cards/BreakdownCard.tsx | 4 +- .../Components/Cards/BusiestServersCard.tsx | 10 +-- .../Components/Cards/CurrentlyDownCard.tsx | 36 +++------- .../Components/Cards/RankingCards.tsx | 4 +- .../Dashboard/Components/DashboardCard.tsx | 24 ++----- client/src/Pages/Dashboard/cards.ts | 2 +- client/src/Pages/Dashboard/checks.ts | 8 +++ client/src/Pages/Dashboard/index.tsx | 4 -- .../src/Pages/Dashboard/useCardSelection.ts | 25 +++---- .../src/Pages/Dashboard/useCardTransitions.ts | 72 +++++++++++-------- .../src/Pages/Dashboard/useDashboardData.ts | 11 +-- 12 files changed, 85 insertions(+), 120 deletions(-) create mode 100644 client/src/Pages/Dashboard/checks.ts diff --git a/client/src/Pages/Dashboard/Components/CardSlotContext.tsx b/client/src/Pages/Dashboard/Components/CardSlotContext.tsx index ef164295c..f5f493309 100644 --- a/client/src/Pages/Dashboard/Components/CardSlotContext.tsx +++ b/client/src/Pages/Dashboard/Components/CardSlotContext.tsx @@ -2,11 +2,6 @@ import { createContext, useContext } from "react"; import type { ReactNode } from "react"; -/** - * Lets the grid inject per-card header controls without every card component - * having to accept and forward a prop it does not care about. The grid provides - * the node; DashboardCard renders it in the header. - */ export const CardSlotContext = createContext(null); export const useCardSlot = (): ReactNode => useContext(CardSlotContext); diff --git a/client/src/Pages/Dashboard/Components/Cards/BreakdownCard.tsx b/client/src/Pages/Dashboard/Components/Cards/BreakdownCard.tsx index f09515c31..b2b7cadef 100644 --- a/client/src/Pages/Dashboard/Components/Cards/BreakdownCard.tsx +++ b/client/src/Pages/Dashboard/Components/Cards/BreakdownCard.tsx @@ -42,9 +42,7 @@ export const BreakdownCard = ({ const theme = useTheme(); const { t } = useTranslation(); - // Bars are relative to the largest bucket, not to the total, so a fleet of - // one dominant type still shows readable proportions. - const max = buckets[0]?.total ?? 0; + const max = Math.max(0, ...buckets.map((bucket) => bucket.total)); return ( { return (data?.monitors ?? []) .filter((monitor) => monitor.type === "hardware") .map((monitor) => { - const checks = monitor.recentChecks ?? []; - // recentChecks is oldest-first: the newest snapshot is last. - const latest = checks[checks.length - 1]; - // Note snake_case here, unlike camelCase in HardwareCheckStats. - // These are 0-1 fractions, so scale to percent. + const latest = latestCheck(monitor); + // snake_case here, unlike camelCase in HardwareCheckStats, and + // these are 0-1 fractions, so scale to percent. const toPercent = (value: number | undefined) => typeof value === "number" ? value * 100 : null; const cpu = toPercent(latest?.cpu?.usage_percent); diff --git a/client/src/Pages/Dashboard/Components/Cards/CurrentlyDownCard.tsx b/client/src/Pages/Dashboard/Components/Cards/CurrentlyDownCard.tsx index 81553afd1..af2295a3c 100644 --- a/client/src/Pages/Dashboard/Components/Cards/CurrentlyDownCard.tsx +++ b/client/src/Pages/Dashboard/Components/Cards/CurrentlyDownCard.tsx @@ -15,14 +15,10 @@ import { CardRow } from "../CardPrimitives"; import { Sparkline } from "../Sparkline"; import { useMonitors } from "../../useDashboardData"; +import { latestCheck } from "../../checks"; + import type { Monitor, MonitorStatus } from "@/Types/Monitor"; -// Ordering, not a severity scale — Checkmate has none. Every input already -// exists in the API response. -// -// `initializing` is deliberately excluded: on a fresh install every new monitor -// starts there, and counting it would open the card red claiming a fleet-wide -// outage that does not exist. It is "not checked yet", not "broken". const SPARKLINE_WIDTH = 80; const STATUS_RANK: Partial> = { @@ -33,18 +29,14 @@ const STATUS_RANK: Partial> = { const isProblem = (monitor: Monitor) => STATUS_RANK[monitor.status] !== undefined; /** - * How long the monitor has been in its current state, inferred from the oldest - * consecutive failing check. - * - * recentChecks is oldest-first — the server appends each snapshot with - * `$push … $slice: -N` — so the run of current failures is walked backwards - * from the end. + * How long the monitor has been down, from the oldest consecutive failing + * check. recentChecks is oldest-first, so the run is walked back from the end. */ const downSince = (monitor: Monitor): string | null => { const checks = monitor.recentChecks ?? []; let oldestFailing: string | null = null; for (let index = checks.length - 1; index >= 0; index -= 1) { - if (checks[index].status === true) { + if (checks[index].status !== false) { break; } oldestFailing = checks[index].createdAt; @@ -52,10 +44,6 @@ const downSince = (monitor: Monitor): string | null => { return oldestFailing; }; -/** The newest retained check — recentChecks is oldest-first. */ -const latestCheck = (monitor: Monitor) => - monitor.recentChecks?.[monitor.recentChecks.length - 1]; - export const CurrentlyDownCard = () => { const theme = useTheme(); const { t } = useTranslation(); @@ -107,12 +95,6 @@ export const CurrentlyDownCard = () => { color={getStatusColor(monitor.status, theme)} size="md" /> - {/* - * Two lines rather than one wide row: at half width seven - * columns would crush the monitor name, so the name and how long - * it has been down lead, and the supporting detail sits beneath - * in a quieter line. - */} { justifyContent="space-between" gap={theme.spacing(LAYOUT.SM)} > - {/* - * Plain text rather than StatusCodeLabel: that component - * wraps its body in a Tooltip, and a focusable element - * inside the row's link is unreachable by keyboard. - */} + {/* Not StatusCodeLabel: its Tooltip is focusable, and a + focusable element inside the row's link is unreachable + by keyboard. */} { > } - value={percentage} + // The card ranks worst uptime first, so the bar shows the + // shortfall - the worst monitor gets the longest bar. + value={100 - percentage} color={theme.palette[palette].main} trailing={