TRX/Checkmate
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(client): dashboard review pass - correctness, redundancy, comments

Browse Source 
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).
This commit is contained in:
gorkem-bwl committed 2026-09-07 06:44:16 -04:00
1 parent 6a54fcd82b
commit 34663ec5f9
12 files changed
+85 -120
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

client/src/Pages/Dashboard/Components/CardSlotContext.tsx
-5
View File
@@ -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<ReactNode>(null);
export const useCardSlot = (): ReactNode => useContext(CardSlotContext);
client/src/Pages/Dashboard/Components/Cards/BreakdownCard.tsx
+1 -3
View File
@@ -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 (
<DashboardCard
client/src/Pages/Dashboard/Components/Cards/BusiestServersCard.tsx
+5 -5
View File
@@ -9,6 +9,8 @@ import { DashboardCard, CardMessage } from "../DashboardCard";
import { CardBar, CardRow, CardRowLabel } from "../CardPrimitives";
import { useMonitors } from "../../useDashboardData";
import { latestCheck } from "../../checks";
import type { Monitor } from "@/Types/Monitor";
// <60% fine, 60-85% warm, above that hot.
@@ -69,11 +71,9 @@ export const BusiestServersCard = () => {
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);
client/src/Pages/Dashboard/Components/Cards/CurrentlyDownCard.tsx
+8 -28
View File
@@ -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<Record<MonitorStatus, number>> = {
@@ -33,18 +29,14 @@ const STATUS_RANK: Partial<Record<MonitorStatus, number>> = {
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.
*/}
<Stack
flex={1}
minWidth={0}
@@ -145,11 +127,9 @@ export const CurrentlyDownCard = () => {
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. */}
<Typography
color={theme.palette.text.secondary}
noWrap
client/src/Pages/Dashboard/Components/Cards/RankingCards.tsx
+3 -1
View File
@@ -152,7 +152,9 @@ export const LowestUptimeCard = () => {
>
<CardMeterRow
label={<CardRowLabel primary={monitor.name} />}
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={
<Typography
client/src/Pages/Dashboard/Components/DashboardCard.tsx
+5 -19
View File
@@ -13,8 +13,8 @@ import { useCardSlot } from "./CardSlotContext";
import type { ReactNode } from "react";
// Matches the 34px control height plus the header's vertical padding, so a card
// with no action still lines up with one that has a button in its header.
// Control height plus the header's vertical padding, so a card with no action
// still lines up with one that has a button in its header.
const HEADER_MIN_HEIGHT = 44;
const SKELETON_ROW_HEIGHT = 20;
@@ -46,25 +46,15 @@ export const DashboardCard = ({
return (
<BaseBox
display="flex"
flexDirection="column"
height="100%"
sx={{
display: "flex",
flexDirection: "column",
height: "100%",
overflow: "hidden",
// The header controls stay invisible until the card is hovered, so a
// settled dashboard is quiet.
//
// `:has(:focus-visible)` rather than `:focus-within`: a mouse click
// leaves the button focused, and :focus-within would then pin that
// card's controls open long after the pointer had left. focus-visible
// only matches keyboard focus, which is the case that actually needs
// the controls revealed.
"&:hover .dashboard-card-slot, &:has(:focus-visible) .dashboard-card-slot": {
opacity: 1,
pointerEvents: "auto",
},
// The controls are overlaid on the action, so the action fades out as
// they fade in rather than the two rendering on top of each other.
"&:hover .dashboard-card-action, &:has(:focus-visible) .dashboard-card-action": {
opacity: 0,
},
@@ -117,10 +107,6 @@ export const DashboardCard = ({
{slot && (
<Box
className="dashboard-card-slot"
// Taken out of flow and overlaid on the right: while hidden it
// would otherwise reserve its own width and push `action` away
// from the card's right edge, leaving it visibly un-aligned
// with the values in the rows beneath.
position="absolute"
right={0}
sx={{
client/src/Pages/Dashboard/cards.ts
+1 -1
View File
@@ -65,7 +65,7 @@ export interface CardDefinition {
// shows something real: Checkmate server works with zero monitors.
// Ordered so the declared widths pair into full rows: a card left alone in a
// row is stretched to fill it, so 8- and 4-wide cards are kept adjacent.
export const DEFAULT_CARD_IDS: CardId[] = [
export const DEFAULT_CARD_IDS: readonly CardId[] = [
"monitorStatus",
"currentlyDown",
"failedChecks",
client/src/Pages/Dashboard/checks.ts
+8
View File
@@ -0,0 +1,8 @@
import type { Monitor } from "@/Types/Monitor";
/**
* `recentChecks` is oldest-first — the server appends each snapshot with
* `$push … $slice: -N` — so the newest retained check is the last element.
*/
export const latestCheck = (monitor: Monitor) =>
monitor.recentChecks?.[monitor.recentChecks.length - 1];
client/src/Pages/Dashboard/index.tsx
-4
View File
@@ -12,10 +12,6 @@ import { CardPicker } from "./Components/CardPicker";
import { MonitorsContext, useMonitorsRequest } from "./useDashboardData";
import { useCardSelection } from "./useCardSelection";
/**
* Answers "is anything wrong, and where" across every monitor type at once —
* the question no single-type page can answer. Problems first and biggest.
*/
const Dashboard = () => {
const theme = useTheme();
const { t } = useTranslation();
client/src/Pages/Dashboard/useCardSelection.ts
+10 -15
View File
@@ -26,10 +26,9 @@ const readStored = (): CardId[] | null => {
if (!Array.isArray(parsed)) {
return null;
}
// An empty array is a real choice — a dashboard the user emptied — and
// must not fall back to the defaults. Only an absent or malformed key
// returns null. Unknown ids are dropped so a card removed in a later
// release cannot break the page.
// An empty array is a real choice and must not fall back to the defaults;
// only an absent or malformed key returns null. Unknown ids are dropped
// so a card removed in a later release cannot break the page.
return parsed.filter(isCardId);
} catch (error) {
logger.error(
@@ -47,13 +46,11 @@ const readStored = (): CardId[] | null => {
export const useCardSelection = () => {
const isAdmin = useIsAdmin();
const [selected, setSelected] = useState<CardId[]>(
() => readStored() ?? DEFAULT_CARD_IDS
() => readStored() ?? [...DEFAULT_CARD_IDS]
);
// Skip the write on mount. The value has just been read from storage, so
// rewriting it achieves nothing — but if the read failed for any reason and
// fell back to the defaults, that write would overwrite the user's real
// selection and make the loss permanent.
// Skip the write on mount: if the read failed and fell back to the defaults,
// writing would overwrite the user's real selection permanently.
const hasMounted = useRef(false);
useEffect(() => {
@@ -96,14 +93,12 @@ export const useCardSelection = () => {
setSelected((current) => current.filter((cardId) => cardId !== id));
}, []);
const resetCards = useCallback(() => setSelected(DEFAULT_CARD_IDS), []);
const resetCards = useCallback(() => setSelected([...DEFAULT_CARD_IDS]), []);
/**
* Swaps a card with its neighbour. `delta` is -1 for up, +1 for down.
*
* Positions are resolved against the *visible* order, not the stored array:
* a non-admin never sees the admin cards, so moving a card past one of them
* would otherwise appear to do nothing.
* Swaps a card with its neighbour; `delta` is -1 for up, +1 for down.
* Resolved against the visible order, so moving past a card hidden from a
* non-admin does not appear to do nothing.
*/
const moveCard = useCallback(
(id: CardId, delta: -1 | 1) => {
client/src/Pages/Dashboard/useCardTransitions.ts
+41 -31
View File
@@ -1,4 +1,4 @@
import { useCallback, useLayoutEffect, useRef } from "react";
import { useCallback, useEffect, useLayoutEffect, useRef } from "react";
import type { CardId } from "./cards";
@@ -7,18 +7,6 @@ const DURATION_MS = 260;
/** Decelerating: quick to leave, gentle to settle. */
const EASING = "cubic-bezier(0.2, 0.8, 0.2, 1)";
/**
* Animates cards between layout positions using FLIP — First, Last, Invert,
* Play. The grid re-renders in its new order immediately; each card is then
* offset back to where it used to be and released, so it appears to travel
* from the old position to the new one.
*
* This animates the real layout rather than faking movement, so cards that
* shift because *another* card moved animate too, and the effect stays correct
* however the grid reflows.
*
* Honours prefers-reduced-motion by skipping the animation entirely.
*/
export const useCardTransitions = () => {
const nodes = useRef(new Map<CardId, HTMLElement>());
const positions = useRef(new Map<CardId, DOMRect>());
@@ -26,6 +14,18 @@ export const useCardTransitions = () => {
const isAnimating = useRef(false);
/** The card the user moved, lifted above the rest while it travels. */
const movedCard = useRef<CardId | null>(null);
/** Pending frame and settle handles, cancelled on unmount and on a new move. */
const frames = useRef<number[]>([]);
const timers = useRef<number[]>([]);
const cancelPending = useCallback(() => {
frames.current.forEach(cancelAnimationFrame);
timers.current.forEach(clearTimeout);
frames.current = [];
timers.current = [];
}, []);
useEffect(() => cancelPending, [cancelPending]);
const registerCard = useCallback((id: CardId, node: HTMLElement | null) => {
if (node) {
@@ -40,14 +40,20 @@ export const useCardTransitions = () => {
* `movedId` is the card the user acted on; it rides above the others so the
* swap reads as one card moving rather than two sliding through each other.
*/
const capturePositions = useCallback((movedId: CardId) => {
positions.current.clear();
for (const [id, node] of nodes.current) {
positions.current.set(id, node.getBoundingClientRect());
}
movedCard.current = movedId;
isAnimating.current = true;
}, []);
const capturePositions = useCallback(
(movedId: CardId) => {
// A move while a previous one is still settling would otherwise have
// its inversion wiped by the earlier timeout, and the card would jump.
cancelPending();
positions.current.clear();
for (const [id, node] of nodes.current) {
positions.current.set(id, node.getBoundingClientRect());
}
movedCard.current = movedId;
isAnimating.current = true;
},
[cancelPending]
);
useLayoutEffect(() => {
if (!isAnimating.current) {
@@ -84,16 +90,20 @@ export const useCardTransitions = () => {
}
// ...then play: on the next frame, release to the new one.
requestAnimationFrame(() => {
node.style.transition = `transform ${DURATION_MS}ms ${EASING}`;
node.style.transform = "";
// Clear the inline styles once settled. Left in place they would
// animate the card again on any later reflow it did not ask for.
window.setTimeout(() => {
node.style.transition = "";
node.style.zIndex = "";
}, DURATION_MS);
});
frames.current.push(
requestAnimationFrame(() => {
node.style.transition = `transform ${DURATION_MS}ms ${EASING}`;
node.style.transform = "";
// Clear the inline styles once settled. Left in place they would
// animate the card again on any later reflow it did not ask for.
timers.current.push(
window.setTimeout(() => {
node.style.transition = "";
node.style.zIndex = "";
}, DURATION_MS)
);
})
);
}
positions.current.clear();
movedCard.current = null;
client/src/Pages/Dashboard/useDashboardData.ts
+3 -8
View File
@@ -3,14 +3,9 @@ import { useGet } from "@/Hooks/UseApi";
import { MonitorTypes, type MonitorsWithChecksResponse } from "@/Types/Monitor";
import { REFRESH_INTERVAL_MS } from "./cards";
// One request feeds every monitor-derived card. Cards read it from context so
// adding a monitor card costs no extra call.
//
// No rowsPerPage: the repository treats 0 as "no limit", so every monitor comes
// back. The dashboard summarises the whole fleet, it never pages. `limit` is
// deliberately not sent — findByTeamIdWithStats derives its page size from
// rowsPerPage alone and never reads it, so passing it would only imply a
// recent-check truncation that is not actually in effect.
// One request feeds every monitor-derived card, read from context so adding a
// card costs no extra call. No rowsPerPage: the dashboard summarises the whole
// fleet, and the repository treats its absence as "no limit".
const buildMonitorsUrl = () => {
const params = new URLSearchParams();
MonitorTypes.forEach((type) => params.append("type", type));