From 5e572d63511a51fe9a22f9b95025fd88f25de7e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 22 Mar 2026 17:17:54 +0000 Subject: [PATCH] Code review fixes: module-level comparators, correct type on SortableHead, handle negative point change, remove redundant spread - Move comparators object to module level (closes over nothing, no useMemo needed) - Use SortConfig on SortableHead instead of inline duplicate type - SevenDayChange: handle negative pointChange with correct sign and color - Remove redundant sevenDayPointChange explicit assignment (already in ...standing spread) https://claude.ai/code/session_01CuCKFVYbpsKSQoFDcTYfY7 --- app/components/standings/StandingsTable.tsx | 76 ++++++++----------- .../leagues/$leagueId.standings.$seasonId.tsx | 1 - 2 files changed, 33 insertions(+), 44 deletions(-) diff --git a/app/components/standings/StandingsTable.tsx b/app/components/standings/StandingsTable.tsx index 951fc06..e90287a 100644 --- a/app/components/standings/StandingsTable.tsx +++ b/app/components/standings/StandingsTable.tsx @@ -3,7 +3,7 @@ import { Table, TableHeader, TableRow, TableHead, TableBody, TableCell } from "~ import { Badge } from "~/components/ui/badge"; import { TeamNameDisplay } from "~/components/ui/team-name-display"; import { PointsDisplay } from "~/components/standings/PointsDisplay"; -import { useSortableData, type SortDirection } from "~/hooks/useSortableData"; +import { useSortableData, type SortConfig, type SortDirection } from "~/hooks/useSortableData"; import { type TeamStanding } from "~/types/standings"; interface StandingsTableProps { @@ -17,6 +17,34 @@ interface StandingRow extends TeamStanding { _sortPoints: number; } +// Comparators are pure functions with no component state — define at module level +const comparators = { + // Rank: ties broken alphabetically by team name regardless of sort direction + currentRank: (a: StandingRow, b: StandingRow, dir: SortDirection) => { + const cmp = a.currentRank - b.currentRank; + if (cmp !== 0) return dir === "asc" ? cmp : -cmp; + return a.teamName.localeCompare(b.teamName); + }, + _sortPoints: (a: StandingRow, b: StandingRow, dir: SortDirection) => { + const cmp = b._sortPoints - a._sortPoints; + return dir === "asc" ? -cmp : cmp; + }, + sevenDayPointChange: (a: StandingRow, b: StandingRow, dir: SortDirection) => { + const aVal = a.sevenDayPointChange ?? 0; + const bVal = b.sevenDayPointChange ?? 0; + const cmp = bVal - aVal; + return dir === "asc" ? -cmp : cmp; + }, + teamName: (a: StandingRow, b: StandingRow, dir: SortDirection) => { + const cmp = a.teamName.localeCompare(b.teamName); + return dir === "asc" ? cmp : -cmp; + }, + participantsRemaining: (a: StandingRow, b: StandingRow, dir: SortDirection) => { + const cmp = a.participantsRemaining - b.participantsRemaining; + return dir === "asc" ? cmp : -cmp; + }, +}; + /** * Display team standings with ranking, points, 7-day change, and sortable columns. */ @@ -30,42 +58,6 @@ export function StandingsTable({ standings, leagueId, seasonId }: StandingsTable [standings] ); - // Custom comparators - const comparators = useMemo( - () => ({ - // Rank: ascending by rank, ties broken alphabetically by team name - currentRank: (a: StandingRow, b: StandingRow, dir: SortDirection) => { - const cmp = a.currentRank - b.currentRank; - if (cmp !== 0) return dir === "asc" ? cmp : -cmp; - // Within ties, always sort by team name A→Z regardless of direction - return a.teamName.localeCompare(b.teamName); - }, - // Points: descending by default means best score first - _sortPoints: (a: StandingRow, b: StandingRow, dir: SortDirection) => { - const cmp = b._sortPoints - a._sortPoints; - return dir === "asc" ? -cmp : cmp; - }, - // 7-day point change: higher change = better, descending first - sevenDayPointChange: (a: StandingRow, b: StandingRow, dir: SortDirection) => { - const aVal = a.sevenDayPointChange ?? 0; - const bVal = b.sevenDayPointChange ?? 0; - const cmp = bVal - aVal; - return dir === "asc" ? -cmp : cmp; - }, - // Team name: A→Z ascending - teamName: (a: StandingRow, b: StandingRow, dir: SortDirection) => { - const cmp = a.teamName.localeCompare(b.teamName); - return dir === "asc" ? cmp : -cmp; - }, - // Remaining: fewer remaining = closer to done, ascending = most remaining first - participantsRemaining: (a: StandingRow, b: StandingRow, dir: SortDirection) => { - const cmp = a.participantsRemaining - b.participantsRemaining; - return dir === "asc" ? cmp : -cmp; - }, - }), - [] - ); - const { sortedData, sortConfig, requestSort } = useSortableData( rows, "currentRank", @@ -187,8 +179,8 @@ export function StandingsTable({ standings, leagueId, seasonId }: StandingsTable interface SortableHeadProps { label: React.ReactNode; - sortKey: string; - sortConfig: { key: string | null; direction: "asc" | "desc" }; + sortKey: keyof StandingRow; + sortConfig: SortConfig; onSort: () => void; className?: string; } @@ -278,11 +270,9 @@ function SevenDayChange({ return (
- {/* Point change — always non-negative */} - - +{pointChange.toFixed(2)} pts + = 0 ? "text-emerald-400" : "text-coral-accent"}`}> + {pointChange >= 0 ? "+" : ""}{pointChange.toFixed(2)} pts - {/* Rank change */} {rankChange > 0 ? ( ↑{rankChange} rank ) : rankChange < 0 ? ( diff --git a/app/routes/leagues/$leagueId.standings.$seasonId.tsx b/app/routes/leagues/$leagueId.standings.$seasonId.tsx index 3b78887..09abf2c 100644 --- a/app/routes/leagues/$leagueId.standings.$seasonId.tsx +++ b/app/routes/leagues/$leagueId.standings.$seasonId.tsx @@ -102,7 +102,6 @@ export async function loader(args: Route.LoaderArgs) { const formattedStandings = standingsWithComparison.map((standing) => ({ ...standing, rankChange: standing.sevenDayRankChange, - sevenDayPointChange: standing.sevenDayPointChange, ownerName: ownerNameByTeamId.get(standing.teamId) ?? null, }));